Skip to content

Commit

Permalink
Revert "Merge pull request kubernetes#12402 from stevekuznetsov/skuzn…
Browse files Browse the repository at this point in the history 
…ets/revert-owners-plugin-feature"

This reverts commit 7972f39, reversing
changes made to 034554b.
  • Loading branch information
@nikhita
nikhita committed Jun 25, 2019
1 parent 0b17ca8 commit 8d591a2
Show file tree
Hide file tree
Showing 4 changed files with 435 additions and 19 deletions.
1 change: 1 addition & 0 deletions prow/plugins/verify-owners/BUILD.bazel
View file
Expand Up @@ -12,6 +12,7 @@ go_library(
"//prow/pluginhelp:go_default_library",
"//prow/plugins:go_default_library",
"//prow/plugins/golint:go_default_library",
"//prow/plugins/trigger:go_default_library",
"//prow/repoowners:go_default_library",
"//vendor/github.com/sirupsen/logrus:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",
Expand Down
202 changes: 185 additions & 17 deletions prow/plugins/verify-owners/verify-owners.go
View file
Expand Up @@ -19,6 +19,7 @@ package verifyowners
import (
"fmt"
"io/ioutil"
"os"
"path/filepath"
"regexp"
"strconv"
Expand All @@ -33,13 +34,16 @@ import (
"k8s.io/test-infra/prow/pluginhelp"
"k8s.io/test-infra/prow/plugins"
"k8s.io/test-infra/prow/plugins/golint"
"k8s.io/test-infra/prow/plugins/trigger"
"k8s.io/test-infra/prow/repoowners"
)

const (
// PluginName defines this plugin's registered name.
PluginName = "verify-owners"
ownersFileName = "OWNERS"
PluginName = "verify-owners"
ownersFileName = "OWNERS"
ownersAliasesFileName = "OWNERS_ALIASES"
nonCollaboratorResponseFormat = "The following users are mentioned in %s file(s) but are not members of the %s org."
)

func init() {
Expand All @@ -48,7 +52,7 @@ func init() {

func helpProvider(config *plugins.Configuration, enabledRepos []string) (*pluginhelp.PluginHelp, error) {
pluginHelp := &pluginhelp.PluginHelp{
Description: fmt.Sprintf("The verify-owners plugin validates %s files if they are modified in a PR. On validation failure it automatically adds the '%s' label to the PR, and a review comment on the incriminating file(s).", ownersFileName, labels.InvalidOwners),
Description: fmt.Sprintf("The verify-owners plugin validates %s and %s files and ensures that they always contain collaborators of the org, if they are modified in a PR. On validation failure it automatically adds the '%s' label to the PR, and a review comment on the incriminating file(s).", ownersFileName, ownersAliasesFileName, labels.InvalidOwners),
}
if config.Owners.LabelsBlackList != nil {
pluginHelp.Config = map[string]string{
Expand All @@ -65,28 +69,41 @@ type ownersClient interface {
}

type githubClient interface {
IsCollaborator(owner, repo, login string) (bool, error)
IsMember(org, user string) (bool, error)
AddLabel(org, repo string, number int, label string) error
CreateComment(owner, repo string, number int, comment string) error
CreateReview(org, repo string, number int, r github.DraftReview) error
GetPullRequestChanges(org, repo string, number int) ([]github.PullRequestChange, error)
RemoveLabel(owner, repo string, number int, label string) error
GetIssueLabels(org, repo string, number int) ([]github.Label, error)
}

type commentPruner interface {
PruneComments(shouldPrune func(github.IssueComment) bool)
}

func handlePullRequest(pc plugins.Agent, pre github.PullRequestEvent) error {
if pre.Action != github.PullRequestActionOpened && pre.Action != github.PullRequestActionReopened && pre.Action != github.PullRequestActionSynchronize {
return nil
}
return handle(pc.GitHubClient, pc.GitClient, pc.Logger, &pre, pc.PluginConfig.Owners.LabelsBlackList)

cp, err := pc.CommentPruner()
if err != nil {
return err
}
return handle(pc.GitHubClient, pc.GitClient, pc.Logger, &pre, pc.PluginConfig.Owners.LabelsBlackList, pc.PluginConfig.TriggerFor(pre.Repo.Owner.Login, pre.Repo.Name), cp)
}

type messageWithLine struct {
line int
message string
}

func handle(ghc githubClient, gc *git.Client, log *logrus.Entry, pre *github.PullRequestEvent, labelsBlackList []string) error {
func handle(ghc githubClient, gc *git.Client, log *logrus.Entry, pre *github.PullRequestEvent, labelsBlackList []string, triggerConfig plugins.Trigger, cp commentPruner) error {
org := pre.Repo.Owner.Login
repo := pre.Repo.Name
number := pre.Number
wrongOwnersFiles := map[string]messageWithLine{}

// Get changes.
Expand All @@ -102,7 +119,18 @@ func handle(ghc githubClient, gc *git.Client, log *logrus.Entry, pre *github.Pul
modifiedOwnersFiles = append(modifiedOwnersFiles, change)
}
}
if len(modifiedOwnersFiles) == 0 {

// Check if the OWNERS_ALIASES file was modified.
var modifiedOwnerAliasesFile github.PullRequestChange
var ownerAliasesModified bool
for _, change := range changes {
if change.Filename == ownersAliasesFileName {
modifiedOwnerAliasesFile = change
ownerAliasesModified = true
}
}

if len(modifiedOwnersFiles) == 0 && !ownerAliasesModified {
return nil
}

Expand All @@ -126,27 +154,50 @@ func handle(ghc githubClient, gc *git.Client, log *logrus.Entry, pre *github.Pul
}
}

// Check each OWNERS file.
// If OWNERS_ALIASES file exists, get all aliases.
// If the file was modified, check for non trusted users in the newly added owners.
nonTrustedUsers, repoAliases, err := nonTrustedUsersInOwnersAliases(ghc, log, triggerConfig, org, repo, r.Dir, modifiedOwnerAliasesFile.Patch, ownerAliasesModified)
if err != nil {
return err
}

// Check if OWNERS files have the correct config and if they do,
// check if all newly added owners are trusted users.
for _, c := range modifiedOwnersFiles {
// Try to load OWNERS file.
path := filepath.Join(r.Dir, c.Filename)
b, err := ioutil.ReadFile(path)
if err != nil {
log.WithError(err).Warningf("Failed to read %s.", path)
return nil
}
if msg := parseOwnersFile(b, c, log, labelsBlackList); msg != nil {
msg, owners := parseOwnersFile(b, c, log, labelsBlackList)
if msg != nil {
wrongOwnersFiles[c.Filename] = *msg
continue
}

nonTrustedUsers, err = nonTrustedUsersInOwners(ghc, log, triggerConfig, org, repo, c.Patch, c.Filename, owners, nonTrustedUsers, repoAliases)
if err != nil {
return err
}
}
// React if we saw something.

// React if there are files with incorrect configs or non-trusted users.
issueLabels, err := ghc.GetIssueLabels(org, repo, number)
if err != nil {
return err
}
hasInvalidOwnersLabel := github.HasLabel(labels.InvalidOwners, issueLabels)

if len(wrongOwnersFiles) > 0 {
s := "s"
if len(wrongOwnersFiles) == 1 {
s = ""
}
if err := ghc.AddLabel(org, repo, pre.Number, labels.InvalidOwners); err != nil {
return err
if !hasInvalidOwnersLabel {
if err := ghc.AddLabel(org, repo, number, labels.InvalidOwners); err != nil {
return err
}
}
log.Debugf("Creating a review for %d %s file%s.", len(wrongOwnersFiles), ownersFileName, s)
var comments []github.DraftReviewComment
Expand All @@ -171,17 +222,40 @@ func handle(ghc githubClient, gc *git.Client, log *logrus.Entry, pre *github.Pul
if err != nil {
return fmt.Errorf("error creating a review for invalid %s file%s: %v", ownersFileName, s, err)
}
} else {
}

if len(nonTrustedUsers) > 0 {
if !hasInvalidOwnersLabel {
if err := ghc.AddLabel(org, repo, number, labels.InvalidOwners); err != nil {
return err
}
}

// prune old comments before adding a new one
cp.PruneComments(func(comment github.IssueComment) bool {
return strings.Contains(comment.Body, fmt.Sprintf(nonCollaboratorResponseFormat, ownersFileName, org))
})
if err := ghc.CreateComment(org, repo, number, markdownFriendlyComment(org, nonTrustedUsers)); err != nil {
log.WithError(err).Errorf("Could not create comment for listing non-collaborators in %s files", ownersFileName)
}
}

if len(wrongOwnersFiles) == 0 && len(nonTrustedUsers) == 0 {
// Don't bother checking if it has the label...it's a race, and we'll have
// to handle failure due to not being labeled anyway.
if err := ghc.RemoveLabel(org, repo, pre.Number, labels.InvalidOwners); err != nil {
return fmt.Errorf("failed removing %s label: %v", labels.InvalidOwners, err)
}
cp.PruneComments(func(comment github.IssueComment) bool {
return strings.Contains(comment.Body, fmt.Sprintf(nonCollaboratorResponseFormat, ownersFileName, org))
})
}

return nil
}

func parseOwnersFile(b []byte, c github.PullRequestChange, log *logrus.Entry, labelsBlackList []string) *messageWithLine {
func parseOwnersFile(b []byte, c github.PullRequestChange, log *logrus.Entry, labelsBlackList []string) (*messageWithLine, []string) {
var reviewers []string
var approvers []string
var labels []string
// by default we bind errors to line 1
Expand All @@ -207,15 +281,17 @@ func parseOwnersFile(b []byte, c github.PullRequestChange, log *logrus.Entry, la
return &messageWithLine{
lineNumber,
fmt.Sprintf("Cannot parse file: %v.", err),
}
}, nil
}
// it's a FullConfig
for _, config := range full.Filters {
reviewers = append(reviewers, config.Reviewers...)
approvers = append(approvers, config.Approvers...)
labels = append(labels, config.Labels...)
}
} else {
// it's a SimpleConfig
reviewers = simple.Config.Reviewers
approvers = simple.Config.Approvers
labels = simple.Config.Labels
}
Expand All @@ -224,14 +300,106 @@ func parseOwnersFile(b []byte, c github.PullRequestChange, log *logrus.Entry, la
return &messageWithLine{
lineNumber,
fmt.Sprintf("File contains blacklisted labels: %s.", sets.NewString(labels...).Intersection(sets.NewString(labelsBlackList...)).List()),
}
}, nil
}
// Check approvers isn't empty
if filepath.Dir(c.Filename) == "." && len(approvers) == 0 {
return &messageWithLine{
lineNumber,
fmt.Sprintf("No approvers defined in this root directory %s file.", ownersFileName),
}, nil
}
owners := append(reviewers, approvers...)
return nil, owners
}

func markdownFriendlyComment(org string, nonTrustedUsers map[string][]string) string {
var commentLines []string
commentLines = append(commentLines, fmt.Sprintf(nonCollaboratorResponseFormat, ownersFileName, org))

for user, ownersFiles := range nonTrustedUsers {
commentLines = append(commentLines, fmt.Sprintf("- @%s", user))
for _, filename := range ownersFiles {
commentLines = append(commentLines, fmt.Sprintf(" - %s", filename))
}
}
return nil
return strings.Join(commentLines, "\n")
}

func nonTrustedUsersInOwnersAliases(ghc githubClient, log *logrus.Entry, triggerConfig plugins.Trigger, org, repo, dir, patch string, ownerAliasesModified bool) (map[string][]string, repoowners.RepoAliases, error) {
repoAliases := make(repoowners.RepoAliases)
// nonTrustedUsers is a map of non-trusted users to the list of files they are being added in
nonTrustedUsers := map[string][]string{}
var err error

// If OWNERS_ALIASES exists, get all aliases.
path := filepath.Join(dir, ownersAliasesFileName)
if _, err := os.Stat(path); err == nil {
b, err := ioutil.ReadFile(path)
if err != nil {
return nonTrustedUsers, repoAliases, fmt.Errorf("Failed to read %s: %v", path, err)
}
repoAliases, err = repoowners.ParseAliasesConfig(b)
if err != nil {
return nonTrustedUsers, repoAliases, fmt.Errorf("error parsing aliases config for %s file: %v", ownersAliasesFileName, err)
}
}

// If OWNERS_ALIASES file was modified, check if newly added owners are trusted.
if ownerAliasesModified {
allOwners := repoAliases.ExpandAllAliases().List()
for _, owner := range allOwners {
// cap the number of checks to avoid exhausting tokens in case of large OWNERS refactors.
if len(nonTrustedUsers) > 20 {
break
}
nonTrustedUsers, err = checkIfTrustedUser(ghc, log, triggerConfig, owner, patch, ownersAliasesFileName, org, repo, nonTrustedUsers, repoAliases)
if err != nil {
return nonTrustedUsers, repoAliases, err
}
}
}

return nonTrustedUsers, repoAliases, nil
}

func nonTrustedUsersInOwners(ghc githubClient, log *logrus.Entry, triggerConfig plugins.Trigger, org, repo, patch, fileName string, owners []string, nonTrustedUsers map[string][]string, repoAliases repoowners.RepoAliases) (map[string][]string, error) {
var err error
for _, owner := range owners {
// cap the number of checks to avoid exhausting tokens in case of large OWNERS refactors.
if len(nonTrustedUsers) > 20 {
break
}

// ignore if owner is an alias
if _, ok := repoAliases[owner]; ok {
continue
}

nonTrustedUsers, err = checkIfTrustedUser(ghc, log, triggerConfig, owner, patch, fileName, org, repo, nonTrustedUsers, repoAliases)
if err != nil {
return nonTrustedUsers, err
}
}
return nonTrustedUsers, nil
}

// checkIfTrustedUser looks for newly addded owners by checking if they are in the patch
// and then checks if the owner is a trusted user.
func checkIfTrustedUser(ghc githubClient, log *logrus.Entry, triggerConfig plugins.Trigger, owner, patch, fileName, org, repo string, nonTrustedUsers map[string][]string, repoAliases repoowners.RepoAliases) (map[string][]string, error) {
if strings.Contains(patch, owner) {
isTrustedUser, err := trigger.TrustedUser(ghc, triggerConfig, owner, org, repo)
if err != nil {
return nonTrustedUsers, err
}

if !isTrustedUser {
if ownersFiles, ok := nonTrustedUsers[owner]; ok {
nonTrustedUsers[owner] = append(ownersFiles, fileName)
} else {
nonTrustedUsers[owner] = []string{fileName}
}
}
}
return nonTrustedUsers, nil
}

0 comments on commit 8d591a2

Please sign in to comment.