search

zapier / kubechecks

Check your Kubernetes changes before they hit the cluster
https://kubechecks.readthedocs.io/en/latest/
Mozilla Public License 2.0
155 stars 14 forks source link

add support for github app auth #249

Open DrFaust92 opened 1 month ago

DrFaust92 commented 1 month ago

Closes https://github.com/zapier/kubechecks/issues/247

zapier-sre-bot commented 1 month ago

Mergecat's Review

Click to read mergecats review! ## 😼 Mergecat review of pkg/config/config.go ```diff @@ -37,6 +37,11 @@ type ServerConfig struct { VcsToken string `mapstructure:"vcs-token"` VcsType string `mapstructure:"vcs-type"` + //github + GithubPrivateKey string `mapstructure:"github-private-key"` + GithubAppID int64 `mapstructure:"github-app-id"` + GithubInstallationID int64 `mapstructure:"github-installation-id"` + // webhooks EnsureWebhooks bool `mapstructure:"ensure-webhooks"` WebhookSecret string `mapstructure:"webhook-secret"` ``` Feedback & Suggestions: 1. **Security Concern**: Storing sensitive information like `GithubPrivateKey` directly in the configuration can be risky. Consider using environment variables or a secrets management service to handle sensitive data securely. ```go GithubPrivateKey string `mapstructure:"github-private-key" env:"GITHUB_PRIVATE_KEY"` ``` This way, you can use a library like `github.com/kelseyhightower/envconfig` to load the environment variables. 2. **Documentation**: Add comments to explain the purpose of the new GitHub-related fields. This will help future developers understand why these fields were added and how they should be used. 3. **Validation**: Ensure that the new fields are validated properly. For example, check that `GithubAppID` and `GithubInstallationID` are positive integers and that `GithubPrivateKey` is not empty. ```go if cfg.GithubAppID <= 0 { return cfg, errors.New("invalid GitHub App ID") } if cfg.GithubInstallationID <= 0 { return cfg, errors.New("invalid GitHub Installation ID") } if cfg.GithubPrivateKey == "" { return cfg, errors.New("GitHub Private Key cannot be empty") } ``` 4. **Consistency**: Ensure that the naming conventions are consistent. For example, consider using `GitHub` instead of `Github` to match the official branding. --- ## 😼 Mergecat review of docs/usage.md ```diff @@ -46,6 +46,9 @@ The full list of supported environment variables is described below: |`KUBECHECKS_ENABLE_PREUPGRADE`|Enable preupgrade checks.|`true`| |`KUBECHECKS_ENSURE_WEBHOOKS`|Ensure that webhooks are created in repositories referenced by argo.|`false`| |`KUBECHECKS_FALLBACK_K8S_VERSION`|Fallback target Kubernetes version for schema / upgrade checks.|`1.23.0`| +|`KUBECHECKS_GITHUB_APP_ID`|Github App ID.|`0`| +|`KUBECHECKS_GITHUB_INSTALLATION_ID`|Github Installation ID.|`0`| +|`KUBECHECKS_GITHUB_PRIVATE_KEY`|Github App Private Key.|| |`KUBECHECKS_KUBERNETES_CLUSTERID`|Kubernetes Cluster ID, must be specified if kubernetes-type is eks.|| |`KUBECHECKS_KUBERNETES_CONFIG`|Path to your kubernetes config file, used to monitor applications.|| |`KUBECHECKS_KUBERNETES_TYPE`|Kubernetes Type One of eks, or local.|`local`| ``` Feedback & Suggestions: 1. **Security Concern**: The `KUBECHECKS_GITHUB_PRIVATE_KEY` should be handled with care. Consider adding a note about securely managing this key, such as using Kubernetes secrets or a secret management tool. 2. **Default Values**: The default values for `KUBECHECKS_GITHUB_APP_ID` and `KUBECHECKS_GITHUB_INSTALLATION_ID` are set to `0`. If `0` is not a valid value, it might be better to leave these fields empty to avoid confusion. 3. **Consistency**: Ensure that the descriptions for the new environment variables are as detailed as the existing ones. For example, specifying the format or any constraints for `KUBECHECKS_GITHUB_PRIVATE_KEY`. --- ## 😼 Mergecat review of go.mod ```diff @@ -12,6 +12,7 @@ require ( github.com/aws/aws-sdk-go-v2/service/eks v1.46.0 github.com/aws/aws-sdk-go-v2/service/sts v1.30.1 github.com/aws/smithy-go v1.20.3 + github.com/bradleyfalzon/ghinstallation/v2 v2.6.0 github.com/cenkalti/backoff/v4 v4.3.0 github.com/chainguard-dev/git-urls v1.0.2 github.com/creasty/defaults v1.7.0 @@ -106,7 +107,6 @@ require ( github.com/blang/semver/v4 v4.0.0 // indirect github.com/bmatcuk/doublestar/v4 v4.6.0 // indirect github.com/bombsimon/logrusr/v2 v2.0.1 // indirect - github.com/bradleyfalzon/ghinstallation/v2 v2.6.0 // indirect github.com/bufbuild/protocompile v0.6.0 // indirect github.com/cespare/xxhash/v2 v2.3.0 // indirect github.com/chai2010/gettext-go v1.0.2 // indirect ``` Feedback & Suggestions: 1. **Duplicate Dependency**: The diff adds `github.com/bradleyfalzon/ghinstallation/v2 v2.6.0` as a direct dependency while removing it as an indirect dependency. This change is fine if you intend to use this package directly in your code. However, ensure that this change is intentional and that the package is indeed required directly. 2. **Dependency Management**: If `github.com/bradleyfalzon/ghinstallation/v2 v2.6.0` is not needed directly, it would be better to keep it as an indirect dependency to avoid unnecessary clutter in the `require` block. --- ## 😼 Mergecat review of cmd/root.go ```diff @@ -45,7 +45,7 @@ func init() { zerolog.LevelDebugValue, zerolog.LevelTraceValue, ). - withDefault("info"). + withDefault(zerolog.LevelInfoValue). withShortHand("l"), ) boolFlag(flags, "persist-log-level", "Persists the set log level down to other module loggers.") @@ -55,6 +55,9 @@ func init() { withChoices("github", "gitlab"). withDefault("gitlab")) stringFlag(flags, "vcs-token", "VCS API token.") + stringFlag(flags, "github-private-key", "Github App Private Key.") + int64Flag(flags, "github-app-id", "Github App ID.") + int64Flag(flags, "github-installation-id", "Github Installation ID.") stringFlag(flags, "argocd-api-token", "ArgoCD API token.") stringFlag(flags, "argocd-api-server-addr", "ArgoCD API Server Address.", newStringOpts(). @@ -119,7 +122,10 @@ func setupLogOutput() { // Default level is info, unless debug flag is present levelFlag := viper.GetString("log-level") - level, _ := zerolog.ParseLevel(levelFlag) + level, err := zerolog.ParseLevel(levelFlag) + if err != nil { + log.Error().Err(err).Msg("Invalid log level") + } zerolog.SetGlobalLevel(level) log.Debug().Msg("Debug level logging enabled.") ``` Feedback & Suggestions: 1. **Log Level Default Value**: - The change from `"info"` to `zerolog.LevelInfoValue` is a good improvement for consistency. 👍 2. **New GitHub Flags**: - Adding flags for `github-private-key`, `github-app-id`, and `github-installation-id` is a useful addition. Ensure these values are securely managed and not exposed in logs or error messages. 🔒 3. **Error Handling for Log Level Parsing**: - Adding error handling for `zerolog.ParseLevel` is a great improvement. However, consider exiting the application if the log level is invalid, as continuing with an undefined log level might lead to unexpected behavior. - Suggested change: ```go if err != nil { log.Error().Err(err).Msg("Invalid log level") os.Exit(1) // Exit the application if the log level is invalid } ``` 4. **Security Considerations**: - Ensure that sensitive information such as `github-private-key` is not logged or exposed in any way. Consider adding validation to check for the presence of these flags and handle errors appropriately. 5. **Performance Considerations**: - No specific performance issues identified in the diff. The changes are primarily related to configuration and logging. --- ## 😼 Mergecat review of pkg/vcs/github_client/client.go ```diff @@ -9,7 +9,8 @@ import ( "strconv" "strings" - "github.com/chainguard-dev/git-urls" + "github.com/bradleyfalzon/ghinstallation/v2" + giturls "github.com/chainguard-dev/git-urls" "github.com/google/go-github/v62/github" "github.com/pkg/errors" "github.com/rs/zerolog/log" @@ -46,34 +47,51 @@ func CreateGithubClient(cfg config.ServerConfig) (*Client, error) { err error googleClient *github.Client shurcoolClient *githubv4.Client + githubClient *http.Client ) - // Initialize the GitLab client with access token - t := cfg.VcsToken - if t == "" { - log.Fatal().Msg("github token needs to be set") + if (cfg.GithubAppID == 0 || cfg.GithubInstallationID == 0 || cfg.GithubPrivateKey == "") && cfg.VcsToken == "" { + log.Fatal().Msg("Either GitHub token or GitHub App credentials (App ID, Installation ID, Private Key) must be set") } - log.Debug().Msgf("Token Length - %d", len(t)) + + // Initialize the GitHub client with app key if provided + if cfg.GithubAppID != 0 && cfg.GithubInstallationID != 0 && cfg.GithubPrivateKey != "" { + appTransport, err := ghinstallation.New(http.DefaultTransport, cfg.GithubAppID, cfg.GithubInstallationID, []byte(cfg.GithubPrivateKey)) + if err != nil { + log.Fatal().Err(err).Msg("failed to create github app transport") + } + githubClient = &http.Client{Transport: appTransport} + } + ctx := context.Background() - ts := oauth2.StaticTokenSource( - &oauth2.Token{AccessToken: t}, - ) - tc := oauth2.NewClient(ctx, ts) + + // Initialize the GitHub client with access token if app key is not provided + if githubClient == nil { + vscToken := cfg.VcsToken + if vscToken != "" { + log.Debug().Msgf("Token Length - %d", len(vscToken)) + ts := oauth2.StaticTokenSource( + &oauth2.Token{AccessToken: vscToken}, + ) + githubClient = oauth2.NewClient(ctx, ts) + } + } else { + log.Fatal().Msg("github client is not initialized") + } githubUrl := cfg.VcsBaseUrl githubUploadUrl := cfg.VcsUploadUrl // we need both urls to be set for github enterprise if githubUrl == "" || githubUploadUrl == "" { - googleClient = github.NewClient(tc) // If this has failed, we'll catch it on first call + googleClient = github.NewClient(githubClient) // If this has failed, we'll catch it on first call - // Use the client from shurcooL's githubv4 library for queries. - shurcoolClient = githubv4.NewClient(tc) + shurcoolClient = githubv4.NewClient(githubClient) } else { - googleClient, err = github.NewClient(tc).WithEnterpriseURLs(githubUrl, githubUploadUrl) + googleClient, err = github.NewClient(githubClient).WithEnterpriseURLs(githubUrl, githubUploadUrl) if err != nil { log.Fatal().Err(err).Msg("failed to create github enterprise client") } - shurcoolClient = githubv4.NewEnterpriseClient(githubUrl, tc) + shurcoolClient = githubv4.NewEnterpriseClient(githubUrl, githubClient) } user, _, err := googleClient.Users.Get(ctx, "") ``` Feedback & Suggestions: 1. **Error Handling for GitHub Client Initialization**: The current logic will always trigger a fatal log if neither the GitHub App credentials nor the VcsToken are provided. However, if the `githubClient` is not initialized, it should also trigger a fatal log. The current `else` block for `githubClient == nil` should be an `else if` to ensure proper error handling. ```diff + } else if githubClient == nil { + log.Fatal().Msg("github client is not initialized") ``` 2. **Redundant Error Check**: The check for `githubClient == nil` after attempting to initialize it with the app key or token is redundant. This should be handled more cleanly by ensuring the initialization logic is correct. ```diff + if githubClient == nil { + log.Fatal().Msg("github client is not initialized") ``` 3. **Logging Improvements**: The log message "github client is not initialized" should be more descriptive to indicate the specific reason why the initialization failed. ```diff + log.Fatal().Msg("Failed to initialize GitHub client with either App credentials or VcsToken") ``` 4. **Security Consideration**: Ensure that sensitive information such as `GithubPrivateKey` is handled securely and not logged. The current implementation does not log this, which is good, but it's worth mentioning to maintain this practice. 5. **Code Duplication**: The logic for initializing the GitHub client with either the app key or token is duplicated. This can be refactored into a helper function to reduce redundancy and improve readability. --- By addressing these points, the code will be more robust, secure, and maintainable. 🛠️

Dependency Review

Click to read mergecats review! No suggestions found
djeebus commented 2 weeks ago

Looks good to me! I'll run some tests locally, but (assuming all looks good) we'll get this merged. Thanks!

DrFaust92 commented 1 week ago

djeebus any news?