diff --git a/cmd/server.go b/cmd/server.go index 6d8a12e011..08fe04a3ae 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -82,6 +82,7 @@ const ( DisableGlobalApplyLockFlag = "disable-global-apply-lock" DisableUnlockLabelFlag = "disable-unlock-label" DiscardApprovalOnPlanFlag = "discard-approval-on-plan" + DiscardApprovalAfterPlanFlag = "discard-approval-after-plan" EmojiReaction = "emoji-reaction" EnableDiffMarkdownFormat = "enable-diff-markdown-format" EnablePolicyChecksFlag = "enable-policy-checks" @@ -519,7 +520,11 @@ var boolFlags = map[string]boolFlag{ description: "Disable atlantis global apply lock in UI", }, DiscardApprovalOnPlanFlag: { - description: "Enables the discarding of approval if a new plan has been executed. Currently only Github is supported", + description: "Enables the discarding of approval if a new plan has been executed. Currently supported on GitHub, GitLab, and Gitea", + defaultValue: false, + }, + DiscardApprovalAfterPlanFlag: { + description: "Discard approval after plan has been executed (regardless of trigger). Currently supported on GitHub, GitLab, and Gitea", defaultValue: false, }, EnablePolicyChecksFlag: { diff --git a/cmd/server_test.go b/cmd/server_test.go index daef8709b2..12815a3458 100644 --- a/cmd/server_test.go +++ b/cmd/server_test.go @@ -83,6 +83,7 @@ var testFlags = map[string]interface{}{ DisableRepoLockingFlag: true, DisableGlobalApplyLockFlag: false, DiscardApprovalOnPlanFlag: true, + DiscardApprovalAfterPlanFlag: true, EmojiReaction: "eyes", ExecutableName: "atlantis", FailOnPreWorkflowHookError: false, diff --git a/runatlantis.io/docs/server-configuration.md b/runatlantis.io/docs/server-configuration.md index 400b2f22c8..5e56f38964 100644 --- a/runatlantis.io/docs/server-configuration.md +++ b/runatlantis.io/docs/server-configuration.md @@ -493,6 +493,17 @@ ATLANTIS_DISABLE_UNLOCK_LABEL="do-not-unlock" Stops atlantis from unlocking a pull request with this label. Defaults to "" (feature disabled). +### `--discard-approval-after-plan` + +```bash +atlantis server --discard-approval-after-plan +# or +ATLANTIS_DISCARD_APPROVAL_AFTER_PLAN=true +``` + +If set, discard approval **after** a plan has been executed (regardless of whether it's triggered via a comment or from the PR being newly created). This addresses race conditions where someone could approve a plan when a PR is immediately opened and the plan is not ready yet. Currently supported on GitHub, GitLab, and Gitea. For GitLab a bot, group or project token is required for this feature. + Reference: [reset-approvals-of-a-merge-request](https://docs.gitlab.com/api/merge_request_approvals/#reset-approvals-of-a-merge-request) + ### `--discard-approval-on-plan` ```bash @@ -501,7 +512,7 @@ atlantis server --discard-approval-on-plan ATLANTIS_DISCARD_APPROVAL_ON_PLAN=true ``` -If set, discard approval if a new plan has been executed. Currently only supported on GitHub and GitLab. For GitLab a bot, group or project token is required for this feature. +If set, discard approval if a new plan has been executed. Currently supported on GitHub, GitLab, and Gitea. For GitLab a bot, group or project token is required for this feature. Reference: [reset-approvals-of-a-merge-request](https://docs.gitlab.com/api/merge_request_approvals/#reset-approvals-of-a-merge-request) ### `--emoji-reaction` diff --git a/server/controllers/events/events_controller_e2e_test.go b/server/controllers/events/events_controller_e2e_test.go index ad3aacaa35..ce1a94ff9d 100644 --- a/server/controllers/events/events_controller_e2e_test.go +++ b/server/controllers/events/events_controller_e2e_test.go @@ -1299,6 +1299,7 @@ type setupOption struct { func setupE2E(t *testing.T, repoDir string, opt setupOption) (events_controllers.VCSEventsController, *vcsmocks.MockClient, *mocks.MockGithubPullGetter, *events.FileWorkspace) { allowForkPRs := false discardApprovalOnPlan := true + discardApprovalAfterPlan := false dataDir, binDir, cacheDir := mkSubDirs(t) // Mocks. e2eVCSClient := vcsmocks.NewMockClient() @@ -1561,6 +1562,7 @@ func setupE2E(t *testing.T, repoDir string, opt setupOption) (events_controllers boltdb, lockingClient, discardApprovalOnPlan, + discardApprovalAfterPlan, e2ePullReqStatusFetcher, ) diff --git a/server/events/command_runner_test.go b/server/events/command_runner_test.go index 322a0a813a..1c148c82b8 100644 --- a/server/events/command_runner_test.go +++ b/server/events/command_runner_test.go @@ -76,6 +76,7 @@ type TestConfig struct { silenceVCSStatusNoProjects bool StatusName string discardApprovalOnPlan bool + discardApprovalAfterPlan bool backend locking.Backend DisableUnlockLabel string } @@ -92,12 +93,13 @@ func setup(t *testing.T, options ...func(testConfig *TestConfig)) *vcsmocks.Mock Ok(t, err) testConfig := &TestConfig{ - parallelPoolSize: 1, - SilenceNoProjects: false, - StatusName: "atlantis-test", - discardApprovalOnPlan: false, - backend: defaultBoltDB, - DisableUnlockLabel: "do-not-unlock", + parallelPoolSize: 1, + SilenceNoProjects: false, + StatusName: "atlantis-test", + discardApprovalOnPlan: false, + discardApprovalAfterPlan: false, + backend: defaultBoltDB, + DisableUnlockLabel: "do-not-unlock", } for _, op := range options { @@ -165,6 +167,7 @@ func setup(t *testing.T, options ...func(testConfig *TestConfig)) *vcsmocks.Mock testConfig.backend, lockingLocker, testConfig.discardApprovalOnPlan, + testConfig.discardApprovalAfterPlan, pullReqStatusFetcher, ) @@ -1068,6 +1071,137 @@ func TestRunGenericPlanCommand_DiscardApprovals(t *testing.T) { vcsClient.VerifyWasCalledOnce().DiscardReviews(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]()) } +func TestRunGenericPlanCommand_DiscardApprovalsAfterPlan(t *testing.T) { + vcsClient := setup(t, func(testConfig *TestConfig) { + testConfig.discardApprovalAfterPlan = true + }) + + tmp := t.TempDir() + boltDB, err := db.New(tmp) + t.Cleanup(func() { + boltDB.Close() + }) + Ok(t, err) + dbUpdater.Backend = boltDB + applyCommandRunner.Backend = boltDB + autoMerger.GlobalAutomerge = true + defer func() { autoMerger.GlobalAutomerge = false }() + + When(projectCommandBuilder.BuildPlanCommands(Any[*command.Context](), Any[*events.CommentCommand]())).ThenReturn([]command.ProjectContext{ + { + CommandName: command.Plan, + }, + }, nil) + When(projectCommandRunner.Plan(Any[command.ProjectContext]())).ThenReturn(command.ProjectResult{PlanSuccess: &models.PlanSuccess{}}) + When(workingDir.GetPullDir(Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(tmp, nil) + pull := &github.PullRequest{State: github.Ptr("open")} + modelPull := models.PullRequest{BaseRepo: testdata.GithubRepo, State: models.OpenPullState, Num: testdata.Pull.Num} + When(githubGetter.GetPullRequest(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(testdata.Pull.Num))).ThenReturn(pull, nil) + When(eventParsing.ParseGithubPull(Any[logging.SimpleLogging](), Eq(pull))).ThenReturn(modelPull, modelPull.BaseRepo, testdata.GithubRepo, nil) + testdata.Pull.BaseRepo = testdata.GithubRepo + ch.RunCommentCommand(testdata.GithubRepo, nil, nil, testdata.User, testdata.Pull.Num, &events.CommentCommand{Name: command.Plan}) + pendingPlanFinder.VerifyWasCalledOnce().DeletePlans(tmp) + + // Verify DiscardReviews is called after plan completion (should be called once) + vcsClient.VerifyWasCalledOnce().DiscardReviews(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]()) +} + +func TestRunGenericPlanCommand_DiscardApprovalsAfterPlan_EvenWhenPlanFails(t *testing.T) { + vcsClient := setup(t, func(testConfig *TestConfig) { + testConfig.discardApprovalAfterPlan = true + }) + + tmp := t.TempDir() + boltDB, err := db.New(tmp) + t.Cleanup(func() { + boltDB.Close() + }) + Ok(t, err) + dbUpdater.Backend = boltDB + applyCommandRunner.Backend = boltDB + autoMerger.GlobalAutomerge = true + defer func() { autoMerger.GlobalAutomerge = false }() + + // Set up project builder to return a project, then plan fails with an error + When(projectCommandBuilder.BuildPlanCommands(Any[*command.Context](), Any[*events.CommentCommand]())).ThenReturn([]command.ProjectContext{ + { + CommandName: command.Plan, + }, + }, nil) + When(projectCommandRunner.Plan(Any[command.ProjectContext]())).ThenReturn(command.ProjectResult{Error: errors.New("plan failed")}) + When(workingDir.GetPullDir(Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(tmp, nil) + pull := &github.PullRequest{State: github.Ptr("open")} + modelPull := models.PullRequest{BaseRepo: testdata.GithubRepo, State: models.OpenPullState, Num: testdata.Pull.Num} + When(githubGetter.GetPullRequest(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(testdata.Pull.Num))).ThenReturn(pull, nil) + When(eventParsing.ParseGithubPull(Any[logging.SimpleLogging](), Eq(pull))).ThenReturn(modelPull, modelPull.BaseRepo, testdata.GithubRepo, nil) + testdata.Pull.BaseRepo = testdata.GithubRepo + ch.RunCommentCommand(testdata.GithubRepo, nil, nil, testdata.User, testdata.Pull.Num, &events.CommentCommand{Name: command.Plan}) + pendingPlanFinder.VerifyWasCalledOnce().DeletePlans(tmp) + + // Verify DiscardReviews IS called even when plan fails (because the timing issue still applies) + vcsClient.VerifyWasCalledOnce().DiscardReviews(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]()) +} + +func TestRunAutoplanCommand_DiscardApprovalsAfterPlan(t *testing.T) { + vcsClient := setup(t, func(testConfig *TestConfig) { + testConfig.discardApprovalAfterPlan = true + }) + + tmp := t.TempDir() + boltDB, err := db.New(tmp) + t.Cleanup(func() { + boltDB.Close() + }) + Ok(t, err) + dbUpdater.Backend = boltDB + + When(projectCommandRunner.Plan(Any[command.ProjectContext]())).ThenReturn(command.ProjectResult{PlanSuccess: &models.PlanSuccess{}}) + When(projectCommandBuilder.BuildAutoplanCommands(Any[*command.Context]())).ThenReturn([]command.ProjectContext{ + { + CommandName: command.Plan, + }, + }, nil) + + When(workingDir.GetPullDir(Any[models.Repo](), Any[models.PullRequest]())). + ThenReturn(tmp, nil) + testdata.Pull.BaseRepo = testdata.GithubRepo + ch.RunAutoplanCommand(testdata.GithubRepo, testdata.GithubRepo, testdata.Pull, testdata.User) + + // Verify DiscardReviews is called after autoplan completion + vcsClient.VerifyWasCalledOnce().DiscardReviews(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]()) +} + +func TestRunGenericPlanCommand_DiscardApprovalsAfterPlan_EvenWithNoProjects(t *testing.T) { + vcsClient := setup(t, func(testConfig *TestConfig) { + testConfig.discardApprovalAfterPlan = true + }) + + tmp := t.TempDir() + boltDB, err := db.New(tmp) + t.Cleanup(func() { + boltDB.Close() + }) + Ok(t, err) + dbUpdater.Backend = boltDB + applyCommandRunner.Backend = boltDB + autoMerger.GlobalAutomerge = true + defer func() { autoMerger.GlobalAutomerge = false }() + + // No projects found + When(projectCommandBuilder.BuildPlanCommands(Any[*command.Context](), Any[*events.CommentCommand]())).ThenReturn([]command.ProjectContext{}, nil) + When(workingDir.GetPullDir(Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(tmp, nil) + pull := &github.PullRequest{State: github.Ptr("open")} + modelPull := models.PullRequest{BaseRepo: testdata.GithubRepo, State: models.OpenPullState, Num: testdata.Pull.Num} + When(githubGetter.GetPullRequest(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(testdata.Pull.Num))).ThenReturn(pull, nil) + When(eventParsing.ParseGithubPull(Any[logging.SimpleLogging](), Eq(pull))).ThenReturn(modelPull, modelPull.BaseRepo, testdata.GithubRepo, nil) + testdata.Pull.BaseRepo = testdata.GithubRepo + ch.RunCommentCommand(testdata.GithubRepo, nil, nil, testdata.User, testdata.Pull.Num, &events.CommentCommand{Name: command.Plan}) + pendingPlanFinder.VerifyWasCalledOnce().DeletePlans(tmp) + + // Verify DiscardReviews is called even when no projects are found (addresses race condition) + vcsClient.VerifyWasCalledOnce().DiscardReviews(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]()) +} + func TestFailedApprovalCreatesFailedStatusUpdate(t *testing.T) { t.Log("if \"atlantis approve_policies\" is run by non policy owner policy check status fails.") setup(t) diff --git a/server/events/plan_command_runner.go b/server/events/plan_command_runner.go index 926126e7d8..4d2abca5d2 100644 --- a/server/events/plan_command_runner.go +++ b/server/events/plan_command_runner.go @@ -33,6 +33,7 @@ func NewPlanCommandRunner( pullStatusFetcher PullStatusFetcher, lockingLocker locking.Locker, discardApprovalOnPlan bool, + discardApprovalAfterPlan bool, pullReqStatusFetcher vcs.PullReqStatusFetcher, ) *PlanCommandRunner { return &PlanCommandRunner{ @@ -53,6 +54,7 @@ func NewPlanCommandRunner( pullStatusFetcher: pullStatusFetcher, lockingLocker: lockingLocker, DiscardApprovalOnPlan: discardApprovalOnPlan, + DiscardApprovalAfterPlan: discardApprovalAfterPlan, pullReqStatusFetcher: pullReqStatusFetcher, } } @@ -83,8 +85,11 @@ type PlanCommandRunner struct { // DiscardApprovalOnPlan controls if all already existing approvals should be removed/dismissed before executing // a plan. DiscardApprovalOnPlan bool - pullReqStatusFetcher vcs.PullReqStatusFetcher - SilencePRComments []string + // DiscardApprovalAfterPlan controls if all already existing approvals should be removed/dismissed after executing + // a plan. + DiscardApprovalAfterPlan bool + pullReqStatusFetcher vcs.PullReqStatusFetcher + SilencePRComments []string } func (p *PlanCommandRunner) runAutoplan(ctx *command.Context) { @@ -154,6 +159,13 @@ func (p *PlanCommandRunner) runAutoplan(ctx *command.Context) { p.pullUpdater.updatePull(ctx, AutoplanCommand{}, result) + // Discard approvals after autoplan completion if flag is set + if p.DiscardApprovalAfterPlan { + if err := p.pullUpdater.VCSClient.DiscardReviews(ctx.Log, baseRepo, pull); err != nil { + ctx.Log.Err("removing approvals after autoplan - %s", err) + } + } + pullStatus, err := p.dbUpdater.updateDB(ctx, ctx.Pull, result.ProjectResults) if err != nil { ctx.Log.Err("writing results: %s", err) @@ -288,6 +300,13 @@ func (p *PlanCommandRunner) run(ctx *command.Context, cmd *CommentCommand) { cmd, result) + // Discard approvals after plan completion if flag is set + if p.DiscardApprovalAfterPlan { + if err := p.pullUpdater.VCSClient.DiscardReviews(ctx.Log, baseRepo, pull); err != nil { + ctx.Log.Err("removing approvals after plan - %s", err) + } + } + pullStatus, err := p.dbUpdater.updateDB(ctx, pull, result.ProjectResults) if err != nil { ctx.Log.Err("writing results: %s", err) diff --git a/server/server.go b/server/server.go index 37b77b5247..6af57ae750 100644 --- a/server/server.go +++ b/server/server.go @@ -788,6 +788,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { backend, lockingClient, userConfig.DiscardApprovalOnPlanFlag, + userConfig.DiscardApprovalAfterPlanFlag, pullReqStatusFetcher, ) diff --git a/server/user_config.go b/server/user_config.go index e981dbeb10..aca9411f21 100644 --- a/server/user_config.go +++ b/server/user_config.go @@ -14,40 +14,41 @@ import ( // The mapstructure tags correspond to flags in cmd/server.go and are used when // the config is parsed from a YAML file. type UserConfig struct { - AllowForkPRs bool `mapstructure:"allow-fork-prs"` - AllowCommands string `mapstructure:"allow-commands"` - AtlantisURL string `mapstructure:"atlantis-url"` - AutoDiscoverModeFlag string `mapstructure:"autodiscover-mode"` - Automerge bool `mapstructure:"automerge"` - AutoplanFileList string `mapstructure:"autoplan-file-list"` - AutoplanModules bool `mapstructure:"autoplan-modules"` - AutoplanModulesFromProjects string `mapstructure:"autoplan-modules-from-projects"` - AzureDevopsToken string `mapstructure:"azuredevops-token"` - AzureDevopsUser string `mapstructure:"azuredevops-user"` - AzureDevopsWebhookPassword string `mapstructure:"azuredevops-webhook-password"` - AzureDevopsWebhookUser string `mapstructure:"azuredevops-webhook-user"` - AzureDevOpsHostname string `mapstructure:"azuredevops-hostname"` - BitbucketBaseURL string `mapstructure:"bitbucket-base-url"` - BitbucketToken string `mapstructure:"bitbucket-token"` - BitbucketUser string `mapstructure:"bitbucket-user"` - BitbucketWebhookSecret string `mapstructure:"bitbucket-webhook-secret"` - CheckoutDepth int `mapstructure:"checkout-depth"` - CheckoutStrategy string `mapstructure:"checkout-strategy"` - DataDir string `mapstructure:"data-dir"` - DisableApplyAll bool `mapstructure:"disable-apply-all"` - DisableAutoplan bool `mapstructure:"disable-autoplan"` - DisableAutoplanLabel string `mapstructure:"disable-autoplan-label"` - DisableMarkdownFolding bool `mapstructure:"disable-markdown-folding"` - DisableRepoLocking bool `mapstructure:"disable-repo-locking"` - DisableGlobalApplyLock bool `mapstructure:"disable-global-apply-lock"` - DisableUnlockLabel string `mapstructure:"disable-unlock-label"` - DiscardApprovalOnPlanFlag bool `mapstructure:"discard-approval-on-plan"` - EmojiReaction string `mapstructure:"emoji-reaction"` - EnablePolicyChecksFlag bool `mapstructure:"enable-policy-checks"` - EnableRegExpCmd bool `mapstructure:"enable-regexp-cmd"` - EnableProfilingAPI bool `mapstructure:"enable-profiling-api"` - EnableDiffMarkdownFormat bool `mapstructure:"enable-diff-markdown-format"` - ExecutableName string `mapstructure:"executable-name"` + AllowForkPRs bool `mapstructure:"allow-fork-prs"` + AllowCommands string `mapstructure:"allow-commands"` + AtlantisURL string `mapstructure:"atlantis-url"` + AutoDiscoverModeFlag string `mapstructure:"autodiscover-mode"` + Automerge bool `mapstructure:"automerge"` + AutoplanFileList string `mapstructure:"autoplan-file-list"` + AutoplanModules bool `mapstructure:"autoplan-modules"` + AutoplanModulesFromProjects string `mapstructure:"autoplan-modules-from-projects"` + AzureDevopsToken string `mapstructure:"azuredevops-token"` + AzureDevopsUser string `mapstructure:"azuredevops-user"` + AzureDevopsWebhookPassword string `mapstructure:"azuredevops-webhook-password"` + AzureDevopsWebhookUser string `mapstructure:"azuredevops-webhook-user"` + AzureDevOpsHostname string `mapstructure:"azuredevops-hostname"` + BitbucketBaseURL string `mapstructure:"bitbucket-base-url"` + BitbucketToken string `mapstructure:"bitbucket-token"` + BitbucketUser string `mapstructure:"bitbucket-user"` + BitbucketWebhookSecret string `mapstructure:"bitbucket-webhook-secret"` + CheckoutDepth int `mapstructure:"checkout-depth"` + CheckoutStrategy string `mapstructure:"checkout-strategy"` + DataDir string `mapstructure:"data-dir"` + DisableApplyAll bool `mapstructure:"disable-apply-all"` + DisableAutoplan bool `mapstructure:"disable-autoplan"` + DisableAutoplanLabel string `mapstructure:"disable-autoplan-label"` + DisableMarkdownFolding bool `mapstructure:"disable-markdown-folding"` + DisableRepoLocking bool `mapstructure:"disable-repo-locking"` + DisableGlobalApplyLock bool `mapstructure:"disable-global-apply-lock"` + DisableUnlockLabel string `mapstructure:"disable-unlock-label"` + DiscardApprovalOnPlanFlag bool `mapstructure:"discard-approval-on-plan"` + DiscardApprovalAfterPlanFlag bool `mapstructure:"discard-approval-after-plan"` + EmojiReaction string `mapstructure:"emoji-reaction"` + EnablePolicyChecksFlag bool `mapstructure:"enable-policy-checks"` + EnableRegExpCmd bool `mapstructure:"enable-regexp-cmd"` + EnableProfilingAPI bool `mapstructure:"enable-profiling-api"` + EnableDiffMarkdownFormat bool `mapstructure:"enable-diff-markdown-format"` + ExecutableName string `mapstructure:"executable-name"` // Fail and do not run the Atlantis command request if any of the pre workflow hooks error. FailOnPreWorkflowHookError bool `mapstructure:"fail-on-pre-workflow-hook-error"` HideUnchangedPlanComments bool `mapstructure:"hide-unchanged-plan-comments"`