add support for github app auth #288
Conversation
Signed-off-by: Joe Lombrozo <joe.lombrozo@zapier.com>
|
Temporary image deleted. |
|
Hi, any news on merging this? |
# Conflicts: # .github/actions/build-image/action.yaml # docs/usage.md
Mergecat's ReviewClick to read mergecats review!😼 Mergecat review of .github/actions/build-image/action.yaml@@ -44,7 +44,7 @@ runs:
- name: Build and push the Docker image
shell: bash
- run: >-
+ run: >-
./earthly.sh
${{ inputs.push == 'true' && '--push' || '' }}
+docker-multiarchFeedback & Suggestions: The change in the diff is a minor formatting adjustment, removing trailing spaces. While this change does not affect functionality, it is a good practice to remove unnecessary whitespace for cleaner code. However, ensure that this change does not affect any formatting-sensitive tools or scripts that might rely on the exact formatting of the file. 🧹✨ 😼 Mergecat review of pkg/vcs/types.go // 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:
😼 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:
😼 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/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:
Overall, these changes improve error handling and provide better context for debugging, which is a positive step for code maintainability and reliability. 🛠️ 😼 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.")
@@ -56,6 +56,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().
@@ -121,7 +126,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/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:
😼 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:
😼 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,9 +69,11 @@ 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_UPLOAD_URL`|VCS upload url, required for enterprise github.||
+|`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/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 |
|
Hi, can we please cut a new version with this feature? 🙏 |
Closes #247. Builds on top of #249. Also gracefully handles missing comment permissions
Adds the following env vars:
KUBECHECKS_GITHUB_APP_ID0KUBECHECKS_GITHUB_INSTALLATION_ID0KUBECHECKS_GITHUB_PRIVATE_KEYKUBECHECKS_VCS_EMAILKUBECHECKS_VCS_USERNAMEIn order to use app auth, you'll need to set APP_ID, INSTALLATION_ID, and PRIVATE_KEY. Github apps don't have usernames or emails, so we'll default those to "kubechecks" and "kubechecks@zapier.com". If you don't want those values, you can override them via VCS_EMAIL and VCS_USERNAME. These will also work if you use a gitlab or github token, but they're not required.