add support for github app auth #287
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Joe Lombrozo <joe.lombrozo@zapier.com>
Mergecat's ReviewClick to read mergecats review!😼 Mergecat review of pkg/vcs/types.go@@ -17,7 +17,7 @@ type WebHookConfig struct {
// Client represents a VCS client
type Client interface {
// PostMessage takes in project name in form "owner/repo" (ie zapier/kubechecks), the PR/MR id, and the actual message
- PostMessage(context.Context, PullRequest, string) *msg.Message
+ PostMessage(context.Context, PullRequest, string) (*msg.Message, error)
// UpdateMessage update a message with new content
UpdateMessage(context.Context, *msg.Message, string) error
// VerifyHook validates a webhook secret and return the body; must be called even if no secretFeedback & Suggestions:
Suggested change to the comment: // PostMessage takes in project name in form "owner/repo" (ie zapier/kubechecks), the PR/MR id, and the actual message
- PostMessage(context.Context, PullRequest, string) *msg.Message
+ // and returns the created message and an error if any occurred
+ PostMessage(context.Context, PullRequest, string) (*msg.Message, error)😼 Mergecat review of go.mod@@ -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
@@ -105,7 +106,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 // indirectFeedback & Suggestions:
😼 Mergecat review of pkg/vcs/github_client/message.go@@ -6,6 +6,7 @@ import (
"strings"
"github.com/google/go-github/v62/github"
+ "github.com/pkg/errors"
"github.com/rs/zerolog/log"
"github.com/shurcooL/githubv4"
@@ -17,7 +18,7 @@ import (
const MaxCommentLength = 64 * 1024
-func (c *Client) PostMessage(ctx context.Context, pr vcs.PullRequest, message string) *msg.Message {
+func (c *Client) PostMessage(ctx context.Context, pr vcs.PullRequest, message string) (*msg.Message, error) {
_, span := tracer.Start(ctx, "PostMessageToMergeRequest")
defer span.End()
@@ -37,10 +38,10 @@ func (c *Client) PostMessage(ctx context.Context, pr vcs.PullRequest, message st
if err != nil {
telemetry.SetError(span, err, "Create Pull Request comment")
- log.Error().Err(err).Msg("could not post message to PR")
+ return nil, errors.Wrap(err, "could not post message to PR")
}
- return msg.NewMessage(pr.FullName, pr.CheckID, int(*comment.ID), c)
+ return msg.NewMessage(pr.FullName, pr.CheckID, int(*comment.ID), c), nil
}
func (c *Client) UpdateMessage(ctx context.Context, m *msg.Message, msg string) error {Feedback & Suggestions:
Example: if err != nil {
telemetry.SetError(span, err, fmt.Sprintf("Create Pull Request comment for PR %d in repo %s", pr.CheckID, pr.FullName))
return nil, errors.Wrapf(err, "could not post message to PR %d in repo %s", pr.CheckID, pr.FullName)
}
😼 Mergecat review of cmd/root.go@@ -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,11 @@ func init() {
withChoices("github", "gitlab").
withDefault("gitlab"))
stringFlag(flags, "vcs-token", "VCS API token.")
+ stringFlag(flags, "vcs-username", "VCS Username.")
+ stringFlag(flags, "vcs-email", "VCS Email.")
+ 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().
@@ -120,7 +125,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:
😼 Mergecat review of pkg/config/config.go@@ -33,11 +33,18 @@ type ServerConfig struct {
OtelCollectorPort string `mapstructure:"otel-collector-port"`
// vcs
+ VcsUsername string `mapstructure:"vcs-username"`
+ VcsEmail string `mapstructure:"vcs-email"`
VcsBaseUrl string `mapstructure:"vcs-base-url"`
VcsUploadUrl string `mapstructure:"vcs-upload-url"` // github enterprise upload URL
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:
By addressing these points, you can improve the security, maintainability, and clarity of your configuration code. 🛡️📈 😼 Mergecat review of pkg/vcs/gitlab_client/message.go@@ -5,6 +5,7 @@ import (
"fmt"
"strings"
+ "github.com/pkg/errors"
"github.com/rs/zerolog/log"
"github.com/xanzy/go-gitlab"
@@ -16,8 +17,8 @@ import (
const MaxCommentLength = 1_000_000
-func (c *Client) PostMessage(ctx context.Context, pr vcs.PullRequest, message string) *msg.Message {
- _, span := tracer.Start(ctx, "PostMessageToMergeRequest")
+func (c *Client) PostMessage(ctx context.Context, pr vcs.PullRequest, message string) (*msg.Message, error) {
+ _, span := tracer.Start(ctx, "PostMessage")
defer span.End()
if len(message) > MaxCommentLength {
@@ -32,10 +33,10 @@ func (c *Client) PostMessage(ctx context.Context, pr vcs.PullRequest, message st
})
if err != nil {
telemetry.SetError(span, err, "Create Merge Request Note")
- log.Error().Err(err).Msg("could not post message to MR")
+ return nil, errors.Wrap(err, "could not post message to MR")
}
- return msg.NewMessage(pr.FullName, pr.CheckID, n.ID, c)
+ return msg.NewMessage(pr.FullName, pr.CheckID, n.ID, c), nil
}
func (c *Client) hideOutdatedMessages(ctx context.Context, projectName string, mergeRequestID int, notes []*gitlab.Note) error {Feedback & Suggestions:
Suggested improvement: @@ -32,10 +33,12 @@ func (c *Client) PostMessage(ctx context.Context, pr vcs.PullRequest, message st
telemetry.SetError(span, err, "Create Merge Request Note")
- log.Error().Err(err).Msg("could not post message to MR")
+ log.Error().Err(err).Msg("could not post message to MR")
+ return nil, errors.Wrap(err, "could not post message to MR")
}
- return msg.NewMessage(pr.FullName, pr.CheckID, n.ID, c)
+ return msg.NewMessage(pr.FullName, pr.CheckID, n.ID, c), nil
}This way, you maintain the logging while also returning the wrapped error. 😼 Mergecat review of docs/usage.md@@ -47,6 +47,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`|
@@ -66,8 +69,10 @@ The full list of supported environment variables is described below:
|`KUBECHECKS_SHOW_DEBUG_INFO`|Set to true to print debug info to the footer of MR comments.|`false`|
|`KUBECHECKS_TIDY_OUTDATED_COMMENTS_MODE`|Sets the mode to use when tidying outdated comments. One of hide, delete.|`hide`|
|`KUBECHECKS_VCS_BASE_URL`|VCS base url, useful if self hosting gitlab, enterprise github, etc.||
+|`KUBECHECKS_VCS_EMAIL`|VCS Email.||
|`KUBECHECKS_VCS_TOKEN`|VCS API token.||
|`KUBECHECKS_VCS_TYPE`|VCS type. One of gitlab or github.|`gitlab`|
+|`KUBECHECKS_VCS_USERNAME`|VCS Username.||
|`KUBECHECKS_WEBHOOK_SECRET`|Optional secret key for validating the source of incoming webhooks.||
|`KUBECHECKS_WEBHOOK_URL_BASE`|The endpoint to listen on for incoming PR/MR event webhooks. For example, 'https://checker.mycompany.com'.||
|`KUBECHECKS_WEBHOOK_URL_PREFIX`|If your application is running behind a proxy that uses path based routing, set this value to match the path prefix. For example, '/hello/world'.||Feedback & Suggestions:
😼 Mergecat review of pkg/events/check.go@@ -201,7 +201,7 @@ func (ce *CheckEvent) getRepo(ctx context.Context, vcsClient hasUsername, cloneU
ce.clonedRepos[reposKey] = repo
// if we cloned 'HEAD', figure out its original branch and store a copy of the repo there
- if branchName == "" || branchName == "HEAD" {
+ if branchName == "HEAD" {
remoteHeadBranchName, err := repo.GetRemoteHead()
if err != nil {
return repo, errors.Wrap(err, "failed to determine remote head")
@@ -260,12 +260,17 @@ func (ce *CheckEvent) Process(ctx context.Context) error {
if len(ce.affectedItems.Applications) <= 0 && len(ce.affectedItems.ApplicationSets) <= 0 {
ce.logger.Info().Msg("No affected apps or appsets, skipping")
- ce.ctr.VcsClient.PostMessage(ctx, ce.pullRequest, "No changes")
+ if _, err := ce.ctr.VcsClient.PostMessage(ctx, ce.pullRequest, "No changes"); err != nil {
+ return errors.Wrap(err, "failed to post changes")
+ }
return nil
}
// We make one comment per run, containing output for all the apps
- ce.vcsNote = ce.createNote(ctx)
+ ce.vcsNote, err = ce.createNote(ctx)
+ if err != nil {
+ return errors.Wrap(err, "failed to create note")
+ }
for num := 0; num <= ce.ctr.Config.MaxConcurrenctChecks; num++ {
@@ -375,8 +380,8 @@ Check kubechecks application logs for more information.
`
)
-// Creates a generic Note struct that we can write into across all worker threads
-func (ce *CheckEvent) createNote(ctx context.Context) *msg.Message {
+// createNote creates a generic Note struct that we can write into across all worker threads
+func (ce *CheckEvent) createNote(ctx context.Context) (*msg.Message, error) {
ctx, span := otel.Tracer("check").Start(ctx, "createNote")
defer span.End()
Feedback & Suggestions:
Suggested Diff: @@ -201,7 +201,10 @@ func (ce *CheckEvent) getRepo(ctx context.Context, vcsClient hasUsername, cloneU
ce.clonedRepos[reposKey] = repo
// if we cloned 'HEAD', figure out its original branch and store a copy of the repo there
- if branchName == "HEAD" {
+ if branchName == "" {
+ branchName = "HEAD"
+ }
+ if branchName == "HEAD" {
remoteHeadBranchName, err := repo.GetRemoteHead()
if err != nil {
return repo, errors.Wrap(err, "failed to determine remote head")
@@ -260,12 +263,17 @@ func (ce *CheckEvent) Process(ctx context.Context) error {
if len(ce.affectedItems.Applications) <= 0 && len(ce.affectedItems.ApplicationSets) <= 0 {
ce.logger.Info().Msg("No affected apps or appsets, skipping")
- ce.ctr.VcsClient.PostMessage(ctx, ce.pullRequest, "No changes")
+ if _, err := ce.ctr.VcsClient.PostMessage(ctx, ce.pullRequest, "No changes"); err != nil {
+ ce.logger.Error().Err(err).Msg("Failed to post 'No changes' message")
+ return errors.Wrap(err, "failed to post changes")
+ }
return nil
}
// We make one comment per run, containing output for all the apps
- ce.vcsNote = ce.createNote(ctx)
+ ce.vcsNote, err = ce.createNote(ctx)
+ if err != nil {
+ ce.logger.Error().Err(err).Msg("Failed to create note")
+ return errors.Wrap(err, "failed to create note")
+ }
for num := 0; num <= ce.ctr.Config.MaxConcurrenctChecks; num++ {
@@ -375,8 +383,8 @@ Check kubechecks application logs for more information.
`
)
-// Creates a generic Note struct that we can write into across all worker threads
-func (ce *CheckEvent) createNote(ctx context.Context) *msg.Message {
+// createNote creates a generic Note struct that we can write into across all worker threads
+func (ce *CheckEvent) createNote(ctx context.Context) (*msg.Message, error) {
ctx, span := otel.Tracer("check").Start(ctx, "createNote")
defer span.End()
ce.logger.Info().Msgf("Creating note")
- return ce.ctr.VcsClient.PostMessage(ctx, ce.pullRequest, ":hourglass: kubechecks running ... ")
+ return ce.ctr.VcsClient.PostMessage(ctx, ce.pullRequest, ":hourglass: kubechecks running ... "), nil
}😼 Mergecat review of pkg/vcs/github_client/client.go@@ -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"
@@ -48,37 +49,26 @@ func CreateGithubClient(cfg config.ServerConfig) (*Client, error) {
shurcoolClient *githubv4.Client
)
- // Initialize the GitLab client with access token
- t := cfg.VcsToken
- if t == "" {
- log.Fatal().Msg("github token needs to be set")
- }
- log.Debug().Msgf("Token Length - %d", len(t))
ctx := context.Background()
- ts := oauth2.StaticTokenSource(
- &oauth2.Token{AccessToken: t},
- )
- tc := oauth2.NewClient(ctx, ts)
+
+ githubClient, err := createHttpClient(ctx, cfg)
+ if err != nil {
+ return nil, errors.Wrap(err, "failed to create github http client")
+ }
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)
- }
-
- user, _, err := googleClient.Users.Get(ctx, "")
- if err != nil {
- return nil, errors.Wrap(err, "failed to get user")
+ shurcoolClient = githubv4.NewEnterpriseClient(githubUrl, githubClient)
}
client := &Client{
@@ -89,13 +79,20 @@ func CreateGithubClient(cfg config.ServerConfig) (*Client, error) {
Issues: IssuesService{googleClient.Issues},
},
shurcoolClient: shurcoolClient,
+ username: cfg.VcsUsername,
+ email: cfg.VcsEmail,
}
- if user != nil {
- if user.Login != nil {
- client.username = *user.Login
- }
- if user.Email != nil {
- client.email = *user.Email
+
+ if client.username == "" || client.email == "" {
+ user, _, err := googleClient.Users.Get(ctx, "")
+ if err == nil {
+ if user.Login != nil {
+ client.username = *user.Login
+ }
+
+ if user.Email != nil {
+ client.email = *user.Email
+ }
}
}
@@ -108,6 +105,32 @@ func CreateGithubClient(cfg config.ServerConfig) (*Client, error) {
return client, nil
}
+func createHttpClient(ctx context.Context, cfg config.ServerConfig) (*http.Client, error) {
+ // 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 {
+ return nil, errors.Wrap(err, "failed to create github app transport")
+ }
+
+ return &http.Client{Transport: appTransport}, nil
+ }
+
+ // Initialize the GitHub client with access token if app key is not provided
+ vcsToken := cfg.VcsToken
+ if vcsToken != "" {
+ log.Debug().Msgf("Token Length - %d", len(vcsToken))
+ ts := oauth2.StaticTokenSource(
+ &oauth2.Token{AccessToken: vcsToken},
+ )
+ return oauth2.NewClient(ctx, ts), nil
+ }
+
+ return nil, errors.New("Either GitHub token or GitHub App credentials (App ID, Installation ID, Private Key) must be set")
+}
+
func (c *Client) Username() string { return c.username }
func (c *Client) Email() string { return c.email }
func (c *Client) GetName() string {Feedback & Suggestions:
Dependency ReviewClick to read mergecats review!No suggestions found |
|
Can't push images from forks =/ opened #288 locally instead |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #247. Builds on top of #249. Also gracefully handles missing comment permissions