Skip to content

Commit

Permalink
Validate base branches in DefaultCommandRunner (#1768)
Browse files Browse the repository at this point in the history
Fixes #1539

The branch matcher feature has been introduced in #1383, but the current
implementation was broken and doesn't work at all (#1539).

If my understanding is correct, there are two problems:

(1) The `GlobalCfg` has a default `Repo` instance which always matches
any repositries and branches. Therefore the branch matcher never be
functional.
(2) Validating base branches in
`DefaultPreWorkflowHooksCommandRunner.RunPreHooks()` implicitly assumed
that users customize `pre_workflow_hooks`, but the assumption isn't
always true because it defaults to empty.

For (1), I added a new method `MatchingRepo()` to `GlobalCfg` to check
`BranchMatches()` for a single `Repo` instance.

For (2), I moved validating branch to
`DefaultCommandRunner.validateCtxAndComment()`. Since the method has
already validated meta data of pull request, I think it's suitable place
to check base branches, but please let me know if there is anywhere more
suitable.
  • Loading branch information
minamijoyo authored and msarvar committed Sep 27, 2021
1 parent e4162fc commit 787f6a9
Show file tree
Hide file tree
Showing 7 changed files with 124 additions and 1 deletion.
1 change: 1 addition & 0 deletions server/controllers/events/events_controller_e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1059,6 +1059,7 @@ func setupE2E(t *testing.T, repoDir string) (events_controllers.VCSEventsControl
GitlabMergeRequestGetter: e2eGitlabGetter,
Logger: logger,
StatsScope: statsScope,
GlobalCfg: globalCfg,
AllowForkPRs: allowForkPRs,
AllowForkPRsFlag: "allow-fork-prs",
CommentCommandRunnerByCmd: commentCommandRunnerByCmd,
Expand Down
9 changes: 9 additions & 0 deletions server/events/command_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/runatlantis/atlantis/server/events/metrics"
"github.com/runatlantis/atlantis/server/events/models"
"github.com/runatlantis/atlantis/server/events/vcs"
"github.com/runatlantis/atlantis/server/events/yaml/valid"
"github.com/runatlantis/atlantis/server/feature"
"github.com/runatlantis/atlantis/server/logging"
"github.com/runatlantis/atlantis/server/recovery"
Expand Down Expand Up @@ -99,6 +100,7 @@ type DefaultCommandRunner struct {
EventParser EventParsing
Logger logging.SimpleLogging
StatsScope stats.Scope
GlobalCfg valid.GlobalCfg
// AllowForkPRs controls whether we operate on pull requests from forks.
AllowForkPRs bool
// ParallelPoolSize controls the size of the wait group used to run
Expand Down Expand Up @@ -339,6 +341,13 @@ func (c *DefaultCommandRunner) validateCtxAndComment(ctx *CommandContext) bool {
}
return false
}

repo := c.GlobalCfg.MatchingRepo(ctx.Pull.BaseRepo.ID())
if !repo.BranchMatches(ctx.Pull.BaseBranch) {
ctx.Log.Info("command was run on a pull request which doesn't match base branches")
// just ignore it to allow us to use any git workflows without malicious intentions.
return false
}
return true
}

Expand Down
37 changes: 37 additions & 0 deletions server/events/command_runner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package events_test
import (
"errors"
"fmt"
"regexp"
"strings"
"testing"

Expand Down Expand Up @@ -193,6 +194,7 @@ func setup(t *testing.T) *vcsmocks.MockClient {
When(preWorkflowHooksCommandRunner.RunPreHooks(matchers.AnyPtrToEventsCommandContext())).ThenReturn(nil)

scope := stats.NewDefaultStore()
globalCfg := valid.NewGlobalCfgFromArgs(valid.GlobalCfgArgs{})

ch = events.DefaultCommandRunner{
VCSClient: vcsClient,
Expand All @@ -203,6 +205,7 @@ func setup(t *testing.T) *vcsmocks.MockClient {
AzureDevopsPullGetter: azuredevopsGetter,
Logger: logger,
StatsScope: scope,
GlobalCfg: globalCfg,
AllowForkPRs: false,
AllowForkPRsFlag: "allow-fork-prs-flag",
Drainer: drainer,
Expand Down Expand Up @@ -457,6 +460,40 @@ func TestRunCommentCommand_ClosedPull(t *testing.T) {
vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.GithubRepo, modelPull.Num, "Atlantis commands can't be run on closed pull requests", "")
}

func TestRunCommentCommand_MatchedBranch(t *testing.T) {
t.Log("if a command is run on a pull request which matches base branches run plan successfully")
vcsClient := setup(t)

ch.GlobalCfg.Repos = append(ch.GlobalCfg.Repos, valid.Repo{
IDRegex: regexp.MustCompile(".*"),
BranchRegex: regexp.MustCompile("^main$"),
})
var pull github.PullRequest
modelPull := models.PullRequest{BaseRepo: fixtures.GithubRepo, BaseBranch: "main"}
When(githubGetter.GetPullRequest(fixtures.GithubRepo, fixtures.Pull.Num)).ThenReturn(&pull, nil)
When(eventParsing.ParseGithubPull(&pull)).ThenReturn(modelPull, modelPull.BaseRepo, fixtures.GithubRepo, nil)

ch.RunCommentCommand(fixtures.GithubRepo, nil, nil, fixtures.User, fixtures.Pull.Num, &events.CommentCommand{Name: models.PlanCommand})
vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.GithubRepo, modelPull.Num, "Ran Plan for 0 projects:\n\n\n\n", "plan")
}

func TestRunCommentCommand_UnmatchedBranch(t *testing.T) {
t.Log("if a command is run on a pull request which doesn't match base branches do not comment with error")
vcsClient := setup(t)

ch.GlobalCfg.Repos = append(ch.GlobalCfg.Repos, valid.Repo{
IDRegex: regexp.MustCompile(".*"),
BranchRegex: regexp.MustCompile("^main$"),
})
var pull github.PullRequest
modelPull := models.PullRequest{BaseRepo: fixtures.GithubRepo, BaseBranch: "foo"}
When(githubGetter.GetPullRequest(fixtures.GithubRepo, fixtures.Pull.Num)).ThenReturn(&pull, nil)
When(eventParsing.ParseGithubPull(&pull)).ThenReturn(modelPull, modelPull.BaseRepo, fixtures.GithubRepo, nil)

ch.RunCommentCommand(fixtures.GithubRepo, nil, nil, fixtures.User, fixtures.Pull.Num, &events.CommentCommand{Name: models.PlanCommand})
vcsClient.VerifyWasCalled(Never()).CreateComment(matchers.AnyModelsRepo(), AnyInt(), AnyString(), AnyString())
}

func TestRunUnlockCommand_VCSComment(t *testing.T) {
t.Log("if unlock PR command is run, atlantis should" +
" invoke the delete command and comment on PR accordingly")
Expand Down
2 changes: 1 addition & 1 deletion server/events/pre_workflow_hooks_command_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func (w *DefaultPreWorkflowHooksCommandRunner) RunPreHooks(

preWorkflowHooks := make([]*valid.PreWorkflowHook, 0)
for _, repo := range w.GlobalCfg.Repos {
if repo.IDMatches(baseRepo.ID()) && repo.BranchMatches(pull.BaseBranch) && len(repo.PreWorkflowHooks) > 0 {
if repo.IDMatches(baseRepo.ID()) && len(repo.PreWorkflowHooks) > 0 {
preWorkflowHooks = append(preWorkflowHooks, repo.PreWorkflowHooks...)
}
}
Expand Down
12 changes: 12 additions & 0 deletions server/events/yaml/valid/global_cfg.go
Original file line number Diff line number Diff line change
Expand Up @@ -472,3 +472,15 @@ func (g GlobalCfg) getMatchingCfg(log logging.SimpleLogging, repoID string) (app
}
return
}

// MatchingRepo returns an instance of Repo which matches a given repoID.
// If multiple repos match, return the last one for consistency with getMatchingCfg.
func (g GlobalCfg) MatchingRepo(repoID string) *Repo {
for i := len(g.Repos) - 1; i >= 0; i-- {
repo := g.Repos[i]
if repo.IDMatches(repoID) {
return &repo
}
}
return nil
}
63 changes: 63 additions & 0 deletions server/events/yaml/valid/global_cfg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -892,6 +892,69 @@ func TestRepo_BranchMatches(t *testing.T) {
Equals(t, false, (valid.Repo{BranchRegex: regexp.MustCompile("release")}).BranchMatches("main"))
}

func TestGlobalCfg_MatchingRepo(t *testing.T) {
defaultRepo := valid.Repo{
IDRegex: regexp.MustCompile(".*"),
BranchRegex: regexp.MustCompile(".*"),
ApplyRequirements: []string{},
}
repo1 := valid.Repo{
IDRegex: regexp.MustCompile(".*"),
BranchRegex: regexp.MustCompile("^main$"),
ApplyRequirements: []string{"approved"},
}
repo2 := valid.Repo{
ID: "github.com/owner/repo",
BranchRegex: regexp.MustCompile("^master$"),
ApplyRequirements: []string{"approved", "mergeable"},
}

cases := map[string]struct {
gCfg valid.GlobalCfg
repoID string
exp *valid.Repo
}{
"matches to default": {
gCfg: valid.GlobalCfg{
Repos: []valid.Repo{
defaultRepo,
repo2,
},
},
repoID: "foo",
exp: &defaultRepo,
},
"matches to IDRegex": {
gCfg: valid.GlobalCfg{
Repos: []valid.Repo{
defaultRepo,
repo1,
repo2,
},
},
repoID: "foo",
exp: &repo1,
},
"matches to ID": {
gCfg: valid.GlobalCfg{
Repos: []valid.Repo{
defaultRepo,
repo1,
repo2,
},
},
repoID: "github.com/owner/repo",
exp: &repo2,
},
}

for name, c := range cases {
t.Run(name, func(t *testing.T) {
Equals(t, c.exp, c.gCfg.MatchingRepo(c.repoID))
})
}
}

// String is a helper routine that allocates a new string value
// to store v and returns a pointer to it.
func String(v string) *string { return &v }
Expand Down
1 change: 1 addition & 0 deletions server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -702,6 +702,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) {
EventParser: eventParser,
Logger: logger,
StatsScope: statsScope.Scope("cmd"),
GlobalCfg: globalCfg,
AllowForkPRs: userConfig.AllowForkPRs,
AllowForkPRsFlag: config.AllowForkPRsFlag,
SilenceForkPRErrors: userConfig.SilenceForkPRErrors,
Expand Down

0 comments on commit 787f6a9

Please sign in to comment.