From a54fa06305ebea101a45cb75ca88647932fa1dca Mon Sep 17 00:00:00 2001 From: Benjamin Yolken Date: Tue, 9 Feb 2021 11:46:03 -0800 Subject: [PATCH 1/2] Make change checks more fine-grained --- cmd/kubeapply-lambda/main.go | 27 +++- cmd/kubeapply-server/main.go | 8 +- cmd/kubeapply/subcmd/pull_request.go | 24 +++- pkg/config/config.go | 6 + pkg/events/handler.go | 31 ++++- pkg/events/handler_test.go | 193 ++++++++++++++++++++++++++- 6 files changed, 274 insertions(+), 15 deletions(-) diff --git a/cmd/kubeapply-lambda/main.go b/cmd/kubeapply-lambda/main.go index 5f111d7..d992808 100644 --- a/cmd/kubeapply-lambda/main.go +++ b/cmd/kubeapply-lambda/main.go @@ -22,9 +22,11 @@ var ( sess *session.Session statsClient stats.StatsClient - automerge bool - debug bool - strictCheck bool + automerge bool + debug bool + strictCheck bool + greenCIRequired bool + reviewRequired bool logsURL = getLogsURL() ) @@ -75,6 +77,15 @@ var ( // Optional, defaults to false. strictCheckStr = os.Getenv("KUBEAPPLY_STRICT_CHECK") + // Whether a green CI is required to apply. Ideally, should be set to "true", + // but based on how long the CI takes, might be easier to have it be "false" in + // non-production environments. + greenCIRequiredStr = os.Getenv("KUBEAPPLY_GREEN_CI_REQUIRED") + + // Whether a review is required to apply. Generally "true" in production and + // otherwise "false". + reviewRequiredStr = os.Getenv("KUBEAPPLY_REVIEW_REQUIRED") + // SSM parameter used for fetching webhook secret. webhookSecretSSMParam = os.Getenv("KUBEAPPLY_WEBHOOK_SECRET_SSM_PARAM") ) @@ -154,6 +165,14 @@ func init() { strictCheck = true } + if strings.ToLower(greenCIRequiredStr) == "true" { + greenCIRequired = true + } + + if strings.ToLower(reviewRequiredStr) == "true" { + reviewRequired = true + } + if strings.ToLower(automergeStr) == "true" { automerge = true } @@ -223,6 +242,8 @@ func handleRequest( Env: env, Version: version.Version, StrictCheck: strictCheck, + GreenCIRequired: greenCIRequired, + ReviewRequired: reviewRequired, Automerge: automerge, UseLocks: true, ApplyConsistencyCheck: false, diff --git a/cmd/kubeapply-server/main.go b/cmd/kubeapply-server/main.go index 44201e5..6c7ccf6 100644 --- a/cmd/kubeapply-server/main.go +++ b/cmd/kubeapply-server/main.go @@ -41,8 +41,12 @@ type Config struct { Env string `conf:"env" help:"only consider changes for this environment"` GithubToken string `conf:"github-token" help:"token for Github API access"` LogsURL string `conf:"logs-url" help:"url for logs; used as link for status checks"` - StrictCheck bool `conf:"strict-check" help:"ensure green status and approval before apply"` WebhookSecret string `conf:"webhook-secret" help:"shared secret set in Github webhooks"` + + // TODO: Deprecate StrictCheck since it's covered by the parameters below that. + StrictCheck bool `conf:"strict-check" help:"ensure green status and approval before apply"` + GreenCIRequired bool `conf:"green-ci-required" help:"require green CI before applying"` + ReviewRequired bool `conf:"review-required" help:"require review before applying:"` } var config = Config{ @@ -121,6 +125,8 @@ func webhookHTTPHandler( ApplyConsistencyCheck: false, Automerge: config.Automerge, StrictCheck: config.StrictCheck, + GreenCIRequired: config.GreenCIRequired, + ReviewRequired: config.ReviewRequired, Debug: config.Debug, }, ) diff --git a/cmd/kubeapply/subcmd/pull_request.go b/cmd/kubeapply/subcmd/pull_request.go index 053e5de..5f77d79 100644 --- a/cmd/kubeapply/subcmd/pull_request.go +++ b/cmd/kubeapply/subcmd/pull_request.go @@ -56,8 +56,16 @@ type pullRequestFlags struct { // Full name of the repo, in [owner]/[name] format repo string - // Whether to be strict about checking for approvals and green github status + // Whether to be strict about checking for approvals and green github status. + // + // Deprecated, to be replaced by the values below. strictCheck bool + + // Whether green CI is required to apply + greenCIRequired bool + + // Whether a review is required to apply + reviewRequired bool } var pullRequestFlagValues pullRequestFlags @@ -111,6 +119,12 @@ func init() { "", "Installation ID for github app", ) + pullRequestCmd.Flags().BoolVar( + &pullRequestFlagValues.greenCIRequired, + "green-ci-required", + false, + "Whether a green CI is required to apply", + ) pullRequestCmd.Flags().IntVar( &pullRequestFlagValues.pullRequestNum, "pull-request", @@ -123,6 +137,12 @@ func init() { "", "Repo to post comment in, in format [owner]/[name]", ) + pullRequestCmd.Flags().BoolVar( + &pullRequestFlagValues.reviewRequired, + "review-required", + false, + "Whether a review is required to apply", + ) pullRequestCmd.Flags().BoolVar( &pullRequestFlagValues.strictCheck, "strict-check", @@ -265,6 +285,8 @@ func pullRequestRun(cmd *cobra.Command, args []string) error { ApplyConsistencyCheck: false, Automerge: pullRequestFlagValues.automerge, StrictCheck: pullRequestFlagValues.strictCheck, + GreenCIRequired: pullRequestFlagValues.greenCIRequired, + ReviewRequired: pullRequestFlagValues.reviewRequired, Debug: debug, }, ) diff --git a/pkg/config/config.go b/pkg/config/config.go index 2b28828..2d83f60 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -76,6 +76,12 @@ type ClusterConfig struct { // Optional, defaults to false. GithubIgnore bool `json:"ignore"` + // ReviewOptional indicates that reviews should not be required for changes in this + // cluster even if strict mode is on. + // + // Optional, and only applicable to webhooks mode. + GithubReviewOptional bool `json:"reviewOptional"` + // VersionConstraint is a string version constraint against with the kubeapply binary // will be checked. See https://github.com/Masterminds/semver for details on the expected // format. diff --git a/pkg/events/handler.go b/pkg/events/handler.go index 4763b9e..27acb61 100644 --- a/pkg/events/handler.go +++ b/pkg/events/handler.go @@ -51,8 +51,16 @@ type WebhookHandlerSettings struct { // StrictCheck indicates whether we should block applies on having an approval and all // green statuses. + // + // Note: To be deprecated and replaced with CICheck and ReviewRequired below. StrictCheck bool + // GreenCIRequired indicates whether CI must be green before allowing applies. + GreenCIRequired bool + + // ReviewRequired indicates whether a review is required before allowing applies. + ReviewRequired bool + // UseLocks indicates whether we should use locking to prevent overlapping handler calls // for a cluster. UseLocks bool @@ -381,14 +389,29 @@ func (whh *WebhookHandler) runApply( Env: whh.settings.Env, } - if whh.settings.StrictCheck && !statusOK { + overrideReviewRequired := true + + for _, clusterClient := range clusterClients { + if !clusterClient.Config().GithubReviewOptional { + overrideReviewRequired = false + break + } + } + if overrideReviewRequired { + log.Info( + "Overriding review required because all clusters in this change have review optional", + ) + } + + if (whh.settings.StrictCheck || whh.settings.GreenCIRequired) && !statusOK { applyErr = multilineError( - "Cannot run apply because strict-check is set to true and commit status is not green.", + "Cannot run apply because green-ci-required is set to true and commit status is not green.", "Please fix status and try again.", ) - } else if whh.settings.StrictCheck && !approved { + } else if (whh.settings.StrictCheck || whh.settings.ReviewRequired) && + !approved && !overrideReviewRequired { applyErr = multilineError( - "Cannot run apply because strict-check is set to true and request is not approved.", + "Cannot run apply because review-required is set to true and request is not approved.", "Please get at least one approval and try again.", ) } else if behindBy > 0 { diff --git a/pkg/events/handler_test.go b/pkg/events/handler_test.go index 0d1f503..125b7ee 100644 --- a/pkg/events/handler_test.go +++ b/pkg/events/handler_test.go @@ -32,6 +32,8 @@ func TestHandleWebhook(t *testing.T) { type testCase struct { description string strictCheck bool + greenCIRequired bool + reviewRequired bool automerge bool kubectlErr bool input *WebhookContext @@ -69,6 +71,24 @@ func TestHandleWebhook(t *testing.T) { }, } + testClusterConfigsReviewOptional := []*config.ClusterConfig{ + { + Cluster: "test-cluster4", + Region: "test-region", + Env: "test-env", + ExpandedPath: "expanded", + ProfilePath: profileDir, + GithubReviewOptional: true, + }, + { + Cluster: "test-cluster5", + Region: "test-region", + Env: "test-env", + ExpandedPath: "expanded", + ProfilePath: profileDir, + }, + } + for _, clusterConfig := range testClusterConfigs { require.Nil( t, @@ -81,6 +101,18 @@ func TestHandleWebhook(t *testing.T) { ), ) } + for _, clusterConfig := range testClusterConfigsReviewOptional { + require.Nil( + t, + clusterConfig.SetDefaults( + fmt.Sprintf( + "/git/repo/clusters/%s.yaml", + clusterConfig.Cluster, + ), + "/git/repo", + ), + ) + } testCases := []testCase{ { @@ -697,6 +729,110 @@ func TestHandleWebhook(t *testing.T) { }, }, }, + { + description: "kubeapply apply not approved (review required)", + reviewRequired: true, + input: &WebhookContext{ + pullRequestClient: &pullreq.FakePullRequestClient{ + ClusterConfigs: testClusterConfigs, + RequestStatuses: []pullreq.PullRequestStatus{}, + ApprovedVal: false, + Mergeable: true, + }, + commentType: commentTypeCommand, + issueCommentEvent: &github.IssueCommentEvent{ + Action: aws.String("created"), + Comment: &github.IssueComment{ + Body: aws.String("kubeapply apply test-env:test-region:test-cluster2"), + }, + }, + }, + expRespStatus: 500, + expComments: []commentMatch{ + { + contains: []string{ + "Error comment: Cannot run apply", + "is not approved", + }, + }, + }, + expRepoStatuses: []statusMatch{ + { + context: "kubeapply/apply (test-env)", + state: "failure", + }, + }, + }, + { + description: "kubeapply apply not approved (review required, partial override)", + reviewRequired: true, + input: &WebhookContext{ + pullRequestClient: &pullreq.FakePullRequestClient{ + ClusterConfigs: testClusterConfigsReviewOptional, + RequestStatuses: []pullreq.PullRequestStatus{}, + ApprovedVal: false, + Mergeable: true, + }, + commentType: commentTypeCommand, + issueCommentEvent: &github.IssueCommentEvent{ + Action: aws.String("created"), + Comment: &github.IssueComment{ + Body: aws.String("kubeapply apply"), + }, + }, + }, + expRespStatus: 500, + expComments: []commentMatch{ + { + contains: []string{ + "Error comment: Cannot run apply", + "is not approved", + }, + }, + }, + expRepoStatuses: []statusMatch{ + { + context: "kubeapply/apply (test-env)", + state: "failure", + }, + }, + }, + { + description: "kubeapply apply not approved (review required, full override)", + input: &WebhookContext{ + pullRequestClient: &pullreq.FakePullRequestClient{ + ClusterConfigs: testClusterConfigsReviewOptional[0:1], + RequestStatuses: []pullreq.PullRequestStatus{}, + ApprovedVal: false, + Mergeable: true, + }, + commentType: commentTypeCommand, + issueCommentEvent: &github.IssueCommentEvent{ + Action: aws.String("created"), + Comment: &github.IssueComment{ + Body: aws.String("kubeapply apply test-env:test-region:test-cluster4"), + }, + }, + }, + expRespStatus: 200, + expComments: []commentMatch{ + { + contains: []string{ + "Kubeapply apply result (test-env)", + "apply result for test-cluster4", + }, + doesNotContain: []string{ + "test-cluster5", + }, + }, + }, + expRepoStatuses: []statusMatch{ + { + context: "kubeapply/apply (test-env)", + state: "success", + }, + }, + }, { description: "kubeapply bad status (not strict)", input: &WebhookContext{ @@ -786,6 +922,49 @@ func TestHandleWebhook(t *testing.T) { }, }, }, + { + description: "kubeapply bad status (green ci required)", + greenCIRequired: true, + input: &WebhookContext{ + pullRequestClient: &pullreq.FakePullRequestClient{ + ClusterConfigs: testClusterConfigs, + RequestStatuses: []pullreq.PullRequestStatus{ + { + Context: "check", + State: "failure", + }, + }, + ApprovedVal: true, + Mergeable: true, + }, + commentType: commentTypeCommand, + issueCommentEvent: &github.IssueCommentEvent{ + Action: aws.String("created"), + Comment: &github.IssueComment{ + Body: aws.String("kubeapply apply test-env:test-region:test-cluster2"), + }, + }, + }, + expRespStatus: 500, + expComments: []commentMatch{ + { + contains: []string{ + "Error comment: Cannot run apply", + "status is not green", + }, + }, + }, + expRepoStatuses: []statusMatch{ + { + context: "check", + state: "failure", + }, + { + context: "kubeapply/apply (test-env)", + state: "failure", + }, + }, + }, { description: "kubeapply apply bad kubeapply status (strict)", strictCheck: true, @@ -1056,12 +1235,14 @@ func TestHandleWebhook(t *testing.T) { stats.NewFakeStatsClient(), generator, WebhookHandlerSettings{ - LogsURL: "test-url", - Env: "test-env", - Version: "1.2.3", - StrictCheck: testCase.strictCheck, - Automerge: testCase.automerge, - Debug: false, + LogsURL: "test-url", + Env: "test-env", + Version: "1.2.3", + StrictCheck: testCase.strictCheck, + GreenCIRequired: testCase.greenCIRequired, + ReviewRequired: testCase.reviewRequired, + Automerge: testCase.automerge, + Debug: false, }, ) From afc3c6af15896e05d5d9e262d861b02c0c27dd74 Mon Sep 17 00:00:00 2001 From: Benjamin Yolken Date: Tue, 9 Feb 2021 12:04:32 -0800 Subject: [PATCH 2/2] Bump kubeapply version --- pkg/version/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/version/version.go b/pkg/version/version.go index 1cc2508..273ef28 100644 --- a/pkg/version/version.go +++ b/pkg/version/version.go @@ -1,4 +1,4 @@ package version // Version stores the current kubeapply version. -const Version = "0.0.25" +const Version = "0.0.26"