From c045be87572e2c5306ec5aad7bd40485ae8855c0 Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Mon, 18 Dec 2023 14:18:24 -0500 Subject: [PATCH 01/24] queue new apps when the manifests show up --- pkg/argo_client/manifests.go | 5 ++-- pkg/diff/diff.go | 22 ++++++++++++++- pkg/events/check.go | 54 +++++++++++++++++++++++------------- pkg/server/hook_handler.go | 1 + 4 files changed, 60 insertions(+), 22 deletions(-) diff --git a/pkg/argo_client/manifests.go b/pkg/argo_client/manifests.go index 3e99b4dc..d8e4f179 100644 --- a/pkg/argo_client/manifests.go +++ b/pkg/argo_client/manifests.go @@ -14,9 +14,10 @@ import ( "github.com/argoproj/argo-cd/v2/util/git" "github.com/ghodss/yaml" "github.com/rs/zerolog/log" - "github.com/zapier/kubechecks/telemetry" "go.opentelemetry.io/otel" "k8s.io/apimachinery/pkg/api/resource" + + "github.com/zapier/kubechecks/telemetry" ) func GetManifestsLocal(ctx context.Context, name string, tempRepoDir string, changedAppFilePath string) ([]string, error) { @@ -108,7 +109,7 @@ func GetManifestsLocal(ctx context.Context, name string, tempRepoDir string, cha } func FormatManifestsYAML(manifestBytes []string) []string { - manifests := []string{} + var manifests []string for _, manifest := range manifestBytes { ret, err := yaml.JSONToYAML([]byte(manifest)) if err != nil { diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index fd55672d..cee64aad 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -45,7 +45,7 @@ changedFilePath should be the root of the changed folder from https://github.com/argoproj/argo-cd/blob/d3ff9757c460ae1a6a11e1231251b5d27aadcdd1/cmd/argocd/commands/app.go#L879 */ -func GetDiff(ctx context.Context, name string, manifests []string) (pkg.CheckResult, string, error) { +func GetDiff(ctx context.Context, name string, manifests []string, addApp func(name, path string)) (pkg.CheckResult, string, error) { ctx, span := otel.Tracer("Kubechecks").Start(ctx, "Diff") defer span.End() @@ -151,6 +151,9 @@ func GetDiff(ctx context.Context, name string, manifests []string) (pkg.CheckRes removed++ case item.live == nil: added++ + if app, ok := isApp(item, diffRes.PredictedLive); ok { + addApp(app.Name, app.Spec.GetSource().Path) + } case diffRes.Modified: modified++ } @@ -172,6 +175,23 @@ func GetDiff(ctx context.Context, name string, manifests []string) (pkg.CheckRes return cr, diff, nil } +func isApp(item objKeyLiveTarget, manifests []byte) (*argoappv1.Application, bool) { + if item.key.Group != "argoproj.io/v1alpha1" { + return nil, false + } + if item.key.Kind != "Application" { + return nil, false + } + + var app argoappv1.Application + if err := json.Unmarshal(manifests, &app); err != nil { + log.Warn().Err(err).Msg("failed to deserialize application") + return nil, false + } + + return &app, true +} + // from https://github.com/argoproj/argo-cd/blob/d3ff9757c460ae1a6a11e1231251b5d27aadcdd1/cmd/argocd/commands/app.go#L879 func groupObjsByKey(localObs []*unstructured.Unstructured, liveObjs []*unstructured.Unstructured, appNamespace string) map[kube.ResourceKey]*unstructured.Unstructured { namespacedByGk := make(map[schema.GroupKind]bool) diff --git a/pkg/events/check.go b/pkg/events/check.go index fc72d7ba..4effb208 100644 --- a/pkg/events/check.go +++ b/pkg/events/check.go @@ -43,6 +43,10 @@ type CheckEvent struct { affectedItems affected_apps.AffectedItems cfg *config.ServerConfig + + addedAppsSet map[string]struct{} + appChannel chan appStruct + doneChannel chan bool } var inFlight int32 @@ -213,8 +217,8 @@ func (ce *CheckEvent) ProcessApps(ctx context.Context) { } // Concurrently process all apps, with a corresponding error channel for reporting back failures - appChannel := make(chan appStruct, len(ce.affectedItems.Applications)) - doneChannel := make(chan bool, len(ce.affectedItems.Applications)) + ce.appChannel = make(chan appStruct, len(ce.affectedItems.Applications)) + ce.doneChannel = make(chan bool, len(ce.affectedItems.Applications)) // If the number of affected apps that we have is less than our worker limit, lower the worker limit if ce.workerLimits > len(ce.affectedItems.Applications) { @@ -225,31 +229,26 @@ func (ce *CheckEvent) ProcessApps(ctx context.Context) { ce.vcsNote = ce.createNote(ctx) for w := 0; w <= ce.workerLimits; w++ { - go ce.appWorkers(ctx, w, appChannel, doneChannel) + go ce.appWorkers(ctx, w) } // Produce apps onto channel for _, app := range ce.affectedItems.Applications { - a := appStruct{ - name: app.Name, - dir: app.Path, - } - ce.logger.Trace().Str("app", a.name).Str("dir", a.dir).Msg("producing app on channel") - appChannel <- a + ce.queueApp(app.Name, app.Path) } returnCount := 0 commitStatus := true - for appStatus := range doneChannel { + for appStatus := range ce.doneChannel { if !appStatus { commitStatus = false } returnCount++ - if returnCount == len(ce.affectedItems.Applications) { + if returnCount == len(ce.addedAppsSet) { ce.logger.Debug().Msg("Closing channels") - close(appChannel) - close(doneChannel) + close(ce.appChannel) + close(ce.doneChannel) } } ce.logger.Info().Msg("Finished") @@ -267,6 +266,23 @@ func (ce *CheckEvent) ProcessApps(ctx context.Context) { ce.CommitStatus(ctx, pkg.StateSuccess) } +func (ce *CheckEvent) queueApp(name, path string) { + a := appStruct{ + name: name, + dir: path, + } + + ce.logger.Trace().Str("app", a.name).Str("dir", a.dir).Msg("producing app on channel") + + key := fmt.Sprintf("%s::%s", a.name, a.dir) + if _, ok := ce.addedAppsSet[key]; ok { + return + } + + ce.addedAppsSet[key] = struct{}{} + ce.appChannel <- a +} + // CommitStatus sets the commit status on the MR // To set the PR/MR status func (ce *CheckEvent) CommitStatus(ctx context.Context, status pkg.CommitState) { @@ -279,11 +295,11 @@ func (ce *CheckEvent) CommitStatus(ctx context.Context, status pkg.CommitState) } // Process all apps on the provided channel -func (ce *CheckEvent) appWorkers(ctx context.Context, workerID int, appChannel chan appStruct, resultChannel chan bool) { - for app := range appChannel { +func (ce *CheckEvent) appWorkers(ctx context.Context, workerID int) { + for app := range ce.appChannel { ce.logger.Info().Int("workerID", workerID).Str("app", app.name).Msg("Processing App") isSuccess := ce.processApp(ctx, app.name, app.dir) - resultChannel <- isSuccess + ce.doneChannel <- isSuccess } } @@ -334,7 +350,7 @@ func (ce *CheckEvent) processApp(ctx context.Context, app, dir string) bool { run := ce.createRunner(span, ctx, app, &wg) run("validating app against schema", ce.validateSchemas(ctx, app, k8sVersion, ce.TempWorkingDir, formattedManifests)) - run("generating diff for app", ce.generateDiff(ctx, app, manifests)) + run("generating diff for app", ce.generateDiff(ctx, app, manifests, ce.queueApp)) if viper.GetBool("enable-conftest") { run("validation policy", ce.validatePolicy(ctx, app)) @@ -424,9 +440,9 @@ func (ce *CheckEvent) validatePolicy(ctx context.Context, app string) func() (pk } } -func (ce *CheckEvent) generateDiff(ctx context.Context, app string, manifests []string) func() (pkg.CheckResult, error) { +func (ce *CheckEvent) generateDiff(ctx context.Context, app string, manifests []string, addApp func(name, path string)) func() (pkg.CheckResult, error) { return func() (pkg.CheckResult, error) { - cr, rawDiff, err := diff.GetDiff(ctx, app, manifests) + cr, rawDiff, err := diff.GetDiff(ctx, app, manifests, addApp) if err != nil { return pkg.CheckResult{}, err } diff --git a/pkg/server/hook_handler.go b/pkg/server/hook_handler.go index 06c3bb14..4efc0c32 100644 --- a/pkg/server/hook_handler.go +++ b/pkg/server/hook_handler.go @@ -152,6 +152,7 @@ func (h *VCSHookHandler) processCheckEvent(ctx context.Context, repo *repo.Repo) log.Error().Err(err).Msg("unable to initialize git") return } + // Clone the repo's BaseRef (main etc) locally into the temp dir we just made err = cEvent.CloneRepoLocal(ctx) if err != nil { From 816423b59125a19d0127bb09b92e7ef05e934bee Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Mon, 18 Dec 2023 14:53:00 -0500 Subject: [PATCH 02/24] fix panic --- pkg/events/check.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/events/check.go b/pkg/events/check.go index 4effb208..be94e988 100644 --- a/pkg/events/check.go +++ b/pkg/events/check.go @@ -217,6 +217,7 @@ func (ce *CheckEvent) ProcessApps(ctx context.Context) { } // Concurrently process all apps, with a corresponding error channel for reporting back failures + ce.addedAppsSet = make(map[string]struct{}) ce.appChannel = make(chan appStruct, len(ce.affectedItems.Applications)) ce.doneChannel = make(chan bool, len(ce.affectedItems.Applications)) From 37f805c830ca983af2145ca9c2ed1f0bbdec214e Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Mon, 18 Dec 2023 15:08:14 -0500 Subject: [PATCH 03/24] add some debugging --- pkg/diff/diff.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index cee64aad..6f9716ac 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -176,10 +176,12 @@ func GetDiff(ctx context.Context, name string, manifests []string, addApp func(n } func isApp(item objKeyLiveTarget, manifests []byte) (*argoappv1.Application, bool) { - if item.key.Group != "argoproj.io/v1alpha1" { + if strings.ToLower(item.key.Group) != "argoproj.io/v1alpha1" { + log.Debug().Str("group", item.key.Group).Msg("group is not correct") return nil, false } - if item.key.Kind != "Application" { + if strings.ToLower(item.key.Kind) != "application" { + log.Debug().Str("kind", item.key.Kind).Msg("kind is not correct") return nil, false } From 50b535dc28f73568e3d9f39332e6572c1a3d6e96 Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Mon, 18 Dec 2023 15:15:47 -0500 Subject: [PATCH 04/24] fix group --- pkg/diff/diff.go | 2 +- pkg/events/check.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 6f9716ac..f2265ac4 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -176,7 +176,7 @@ func GetDiff(ctx context.Context, name string, manifests []string, addApp func(n } func isApp(item objKeyLiveTarget, manifests []byte) (*argoappv1.Application, bool) { - if strings.ToLower(item.key.Group) != "argoproj.io/v1alpha1" { + if strings.ToLower(item.key.Group) != "argoproj.io" { log.Debug().Str("group", item.key.Group).Msg("group is not correct") return nil, false } diff --git a/pkg/events/check.go b/pkg/events/check.go index be94e988..cbf0ec08 100644 --- a/pkg/events/check.go +++ b/pkg/events/check.go @@ -273,7 +273,7 @@ func (ce *CheckEvent) queueApp(name, path string) { dir: path, } - ce.logger.Trace().Str("app", a.name).Str("dir", a.dir).Msg("producing app on channel") + ce.logger.Debug().Str("app", a.name).Str("dir", a.dir).Msg("producing app on channel") key := fmt.Sprintf("%s::%s", a.name, a.dir) if _, ok := ce.addedAppsSet[key]; ok { From b82fbf11fd8eb2e38f4d5ab1fb4c1414a8ec97e2 Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Mon, 18 Dec 2023 15:40:37 -0500 Subject: [PATCH 05/24] wrap to identify where errors are --- pkg/argo_client/manifests.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/argo_client/manifests.go b/pkg/argo_client/manifests.go index d8e4f179..d0413668 100644 --- a/pkg/argo_client/manifests.go +++ b/pkg/argo_client/manifests.go @@ -13,6 +13,7 @@ import ( "github.com/argoproj/argo-cd/v2/reposerver/repository" "github.com/argoproj/argo-cd/v2/util/git" "github.com/ghodss/yaml" + "github.com/pkg/errors" "github.com/rs/zerolog/log" "go.opentelemetry.io/otel" "k8s.io/apimachinery/pkg/api/resource" @@ -51,7 +52,7 @@ func GetManifestsLocal(ctx context.Context, name string, tempRepoDir string, cha if err != nil { telemetry.SetError(span, err, "Argo Get App") getManifestsFailed.WithLabelValues(name).Inc() - return nil, err + return nil, errors.Wrap(err, "failed to get application") } log.Trace().Msgf("Argo App: %+v", app) @@ -59,14 +60,14 @@ func GetManifestsLocal(ctx context.Context, name string, tempRepoDir string, cha if err != nil { telemetry.SetError(span, err, "Argo Get Cluster") getManifestsFailed.WithLabelValues(name).Inc() - return nil, err + return nil, errors.Wrap(err, "failed to get cluster") } argoSettings, err := settingsClient.Get(ctx, &settings.SettingsQuery{}) if err != nil { telemetry.SetError(span, err, "Argo Get Settings") getManifestsFailed.WithLabelValues(name).Inc() - return nil, err + return nil, errors.Wrap(err, "failed to get settings") } // Code is commented out until Argo fixes the server side manifest generation @@ -98,7 +99,7 @@ func GetManifestsLocal(ctx context.Context, name string, tempRepoDir string, cha }, true, &git.NoopCredsStore{}, resource.MustParse("0"), nil) if err != nil { telemetry.SetError(span, err, "Generate Manifests") - return nil, err + return nil, errors.Wrap(err, "failed to generate manifests") } if res.Manifests == nil { From eeb7551b20fc0902fc28bb57f721ce1b54b54c8d Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Mon, 18 Dec 2023 15:53:05 -0500 Subject: [PATCH 06/24] support virtual apps --- pkg/argo_client/manifests.go | 22 ++++++++-------- pkg/diff/diff.go | 6 ++--- pkg/events/check.go | 50 ++++++++++++++++++++---------------- 3 files changed, 43 insertions(+), 35 deletions(-) diff --git a/pkg/argo_client/manifests.go b/pkg/argo_client/manifests.go index d0413668..93d7b9c7 100644 --- a/pkg/argo_client/manifests.go +++ b/pkg/argo_client/manifests.go @@ -21,7 +21,7 @@ import ( "github.com/zapier/kubechecks/telemetry" ) -func GetManifestsLocal(ctx context.Context, name string, tempRepoDir string, changedAppFilePath string) ([]string, error) { +func GetManifestsLocal(ctx context.Context, name string, tempRepoDir string, changedAppFilePath string, app *argoappv1.Application) ([]string, error) { ctx, span := otel.Tracer("Kubechecks").Start(ctx, "GetManifestsLocal") defer span.End() @@ -45,16 +45,18 @@ func GetManifestsLocal(ctx context.Context, name string, tempRepoDir string, cha log.Debug().Str("name", name).Msg("generating diff for application...") - appName := name - app, err := appClient.Get(ctx, &application.ApplicationQuery{ - Name: &appName, - }) - if err != nil { - telemetry.SetError(span, err, "Argo Get App") - getManifestsFailed.WithLabelValues(name).Inc() - return nil, errors.Wrap(err, "failed to get application") + if app == nil { + appName := name + app, err := appClient.Get(ctx, &application.ApplicationQuery{ + Name: &appName, + }) + if err != nil { + telemetry.SetError(span, err, "Argo Get App") + getManifestsFailed.WithLabelValues(name).Inc() + return nil, errors.Wrap(err, "failed to get application") + } + log.Trace().Msgf("Argo App: %+v", app) } - log.Trace().Msgf("Argo App: %+v", app) cluster, err := clusterIf.Get(ctx, &cluster.ClusterQuery{Name: app.Spec.Destination.Name, Server: app.Spec.Destination.Server}) if err != nil { diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index f2265ac4..58e42216 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -45,7 +45,7 @@ changedFilePath should be the root of the changed folder from https://github.com/argoproj/argo-cd/blob/d3ff9757c460ae1a6a11e1231251b5d27aadcdd1/cmd/argocd/commands/app.go#L879 */ -func GetDiff(ctx context.Context, name string, manifests []string, addApp func(name, path string)) (pkg.CheckResult, string, error) { +func GetDiff(ctx context.Context, name string, manifests []string, app *argoappv1.Application, addApp func(*argoappv1.Application)) (pkg.CheckResult, string, error) { ctx, span := otel.Tracer("Kubechecks").Start(ctx, "Diff") defer span.End() @@ -62,7 +62,7 @@ func GetDiff(ctx context.Context, name string, manifests []string, addApp func(n var err error appName := name - app, err := appClient.Get(ctx, &application.ApplicationQuery{ + app, err = appClient.Get(ctx, &application.ApplicationQuery{ Name: &appName, }) if err != nil { @@ -152,7 +152,7 @@ func GetDiff(ctx context.Context, name string, manifests []string, addApp func(n case item.live == nil: added++ if app, ok := isApp(item, diffRes.PredictedLive); ok { - addApp(app.Name, app.Spec.GetSource().Path) + addApp(app) } case diffRes.Modified: modified++ diff --git a/pkg/events/check.go b/pkg/events/check.go index cbf0ec08..a37ee6cd 100644 --- a/pkg/events/check.go +++ b/pkg/events/check.go @@ -9,6 +9,7 @@ import ( "sync/atomic" "time" + argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/pkg/errors" "github.com/rs/zerolog" "github.com/rs/zerolog/log" @@ -44,6 +45,7 @@ type CheckEvent struct { cfg *config.ServerConfig + createdApps map[string]*argoappv1.Application addedAppsSet map[string]struct{} appChannel chan appStruct doneChannel chan bool @@ -53,9 +55,10 @@ var inFlight int32 func NewCheckEvent(repo *repo.Repo, client pkg.Client, cfg *config.ServerConfig) *CheckEvent { ce := &CheckEvent{ - cfg: cfg, - client: client, - repo: repo, + cfg: cfg, + client: client, + createdApps: make(map[string]*argoappv1.Application), + repo: repo, } ce.logger = log.Logger.With().Str("repo", repo.Name).Int("event_id", repo.CheckID).Logger() @@ -267,9 +270,9 @@ func (ce *CheckEvent) ProcessApps(ctx context.Context) { ce.CommitStatus(ctx, pkg.StateSuccess) } -func (ce *CheckEvent) queueApp(name, path string) { +func (ce *CheckEvent) queueApp(appName, path string) { a := appStruct{ - name: name, + name: appName, dir: path, } @@ -309,9 +312,9 @@ func (ce *CheckEvent) appWorkers(ctx context.Context, workerID int) { // It takes a context (ctx), application name (app), directory (dir) as input and returns an error if any check fails. // The processing is performed concurrently using Go routines and error groups. Any check results are sent through // the returnChan. The function also manages the inFlight atomic counter to track active processing routines. -func (ce *CheckEvent) processApp(ctx context.Context, app, dir string) bool { +func (ce *CheckEvent) processApp(ctx context.Context, appName, dir string) bool { ctx, span := otel.Tracer("Kubechecks").Start(ctx, "processApp", trace.WithAttributes( - attribute.String("app", app), + attribute.String("app", appName), attribute.String("dir", dir), )) defer span.End() @@ -320,16 +323,16 @@ func (ce *CheckEvent) processApp(ctx context.Context, app, dir string) bool { defer atomic.AddInt32(&inFlight, -1) start := time.Now() - ce.logger.Info().Str("app", app).Msg("Adding new app") + ce.logger.Info().Str("app", appName).Msg("Adding new app") // Build a new section for this app in the parent comment - ce.vcsNote.AddNewApp(ctx, app) + ce.vcsNote.AddNewApp(ctx, appName) - ce.logger.Debug().Msgf("Getting manifests for app: %s with code at %s/%s", app, ce.TempWorkingDir, dir) - manifests, err := argo_client.GetManifestsLocal(ctx, app, ce.TempWorkingDir, dir) + ce.logger.Debug().Msgf("Getting manifests for app: %s with code at %s/%s", appName, ce.TempWorkingDir, dir) + manifests, err := argo_client.GetManifestsLocal(ctx, appName, ce.TempWorkingDir, dir, ce.createdApps[appName]) if err != nil { - ce.logger.Error().Err(err).Msgf("Unable to get manifests for %s in %s", app, dir) + ce.logger.Error().Err(err).Msgf("Unable to get manifests for %s in %s", appName, dir) cr := pkg.CheckResult{State: pkg.StateError, Summary: "Unable to get manifests", Details: fmt.Sprintf("```\n%s\n```", ce.cleanupGetManifestsError(err))} - ce.vcsNote.AddToAppMessage(ctx, app, cr) + ce.vcsNote.AddToAppMessage(ctx, appName, cr) return false } @@ -337,7 +340,7 @@ func (ce *CheckEvent) processApp(ctx context.Context, app, dir string) bool { formattedManifests := argo_client.FormatManifestsYAML(manifests) ce.logger.Trace().Msgf("Manifests:\n%+v\n", formattedManifests) - k8sVersion, err := argo_client.GetArgoClient().GetKubernetesVersionByApplicationName(ctx, app) + k8sVersion, err := argo_client.GetArgoClient().GetKubernetesVersionByApplicationName(ctx, appName) if err != nil { ce.logger.Error().Err(err).Msg("Error retrieving the Kubernetes version") k8sVersion = viper.GetString("fallback-k8s-version") @@ -348,16 +351,19 @@ func (ce *CheckEvent) processApp(ctx context.Context, app, dir string) bool { var wg sync.WaitGroup - run := ce.createRunner(span, ctx, app, &wg) + run := ce.createRunner(span, ctx, appName, &wg) - run("validating app against schema", ce.validateSchemas(ctx, app, k8sVersion, ce.TempWorkingDir, formattedManifests)) - run("generating diff for app", ce.generateDiff(ctx, app, manifests, ce.queueApp)) + run("validating app against schema", ce.validateSchemas(ctx, appName, k8sVersion, ce.TempWorkingDir, formattedManifests)) + run("generating diff for app", ce.generateDiff(ctx, appName, manifests, func(app *argoappv1.Application) { + ce.createdApps[app.Name] = app + ce.queueApp(app.Name, app.Spec.GetSource().Path) + })) if viper.GetBool("enable-conftest") { - run("validation policy", ce.validatePolicy(ctx, app)) + run("validation policy", ce.validatePolicy(ctx, appName)) } - run("running pre-upgrade check", ce.runPreupgradeCheck(ctx, app, k8sVersion, formattedManifests)) + run("running pre-upgrade check", ce.runPreupgradeCheck(ctx, appName, k8sVersion, formattedManifests)) wg.Wait() @@ -441,14 +447,14 @@ func (ce *CheckEvent) validatePolicy(ctx context.Context, app string) func() (pk } } -func (ce *CheckEvent) generateDiff(ctx context.Context, app string, manifests []string, addApp func(name, path string)) func() (pkg.CheckResult, error) { +func (ce *CheckEvent) generateDiff(ctx context.Context, appName string, manifests []string, addApp func(app *argoappv1.Application)) func() (pkg.CheckResult, error) { return func() (pkg.CheckResult, error) { - cr, rawDiff, err := diff.GetDiff(ctx, app, manifests, addApp) + cr, rawDiff, err := diff.GetDiff(ctx, appName, manifests, ce.createdApps[appName], addApp) if err != nil { return pkg.CheckResult{}, err } - diff.AIDiffSummary(ctx, ce.vcsNote, app, manifests, rawDiff) + diff.AIDiffSummary(ctx, ce.vcsNote, appName, manifests, rawDiff) return cr, nil } From a7f719932a97977a5cd8764886f9dd3cc1e0c8fe Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Mon, 18 Dec 2023 15:58:18 -0500 Subject: [PATCH 07/24] whopos, shadowed. fun. --- pkg/argo_client/manifests.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/argo_client/manifests.go b/pkg/argo_client/manifests.go index 93d7b9c7..ea7d4522 100644 --- a/pkg/argo_client/manifests.go +++ b/pkg/argo_client/manifests.go @@ -22,6 +22,8 @@ import ( ) func GetManifestsLocal(ctx context.Context, name string, tempRepoDir string, changedAppFilePath string, app *argoappv1.Application) ([]string, error) { + var err error + ctx, span := otel.Tracer("Kubechecks").Start(ctx, "GetManifestsLocal") defer span.End() @@ -47,7 +49,7 @@ func GetManifestsLocal(ctx context.Context, name string, tempRepoDir string, cha if app == nil { appName := name - app, err := appClient.Get(ctx, &application.ApplicationQuery{ + app, err = appClient.Get(ctx, &application.ApplicationQuery{ Name: &appName, }) if err != nil { From d85c0f6562d1efaf18a95ed8e808b46aecd16a58 Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Mon, 18 Dec 2023 16:18:01 -0500 Subject: [PATCH 08/24] cleaning up name --- pkg/diff/diff.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 58e42216..1d1d21c0 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -46,7 +46,7 @@ changedFilePath should be the root of the changed folder from https://github.com/argoproj/argo-cd/blob/d3ff9757c460ae1a6a11e1231251b5d27aadcdd1/cmd/argocd/commands/app.go#L879 */ func GetDiff(ctx context.Context, name string, manifests []string, app *argoappv1.Application, addApp func(*argoappv1.Application)) (pkg.CheckResult, string, error) { - ctx, span := otel.Tracer("Kubechecks").Start(ctx, "Diff") + ctx, span := otel.Tracer("Kubechecks").Start(ctx, "GetDiff") defer span.End() argoClient := argo_client.GetArgoClient() From 6e5a8f91d05eda39e4d1295198c0f37ddd60f2b5 Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Mon, 18 Dec 2023 16:23:54 -0500 Subject: [PATCH 09/24] one more --- pkg/diff/diff.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 1d1d21c0..676c41d7 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -62,12 +62,14 @@ func GetDiff(ctx context.Context, name string, manifests []string, app *argoappv var err error appName := name - app, err = appClient.Get(ctx, &application.ApplicationQuery{ - Name: &appName, - }) - if err != nil { - telemetry.SetError(span, err, "Get Argo App") - return pkg.CheckResult{}, "", err + if app == nil { + app, err = appClient.Get(ctx, &application.ApplicationQuery{ + Name: &appName, + }) + if err != nil { + telemetry.SetError(span, err, "Get Argo App") + return pkg.CheckResult{}, "", err + } } resources, err := appClient.ManagedResources(ctx, &application.ResourcesQuery{ From 3012a39df760ef6acfe1c0f59c6b559b0caf1aa2 Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Mon, 18 Dec 2023 16:36:42 -0500 Subject: [PATCH 10/24] if we have a created app, assume no resources --- pkg/diff/diff.go | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 676c41d7..4d4221a1 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -59,7 +59,10 @@ func GetDiff(ctx context.Context, name string, manifests []string, app *argoappv settingsCloser, settingsClient := argoClient.GetSettingsClient() defer settingsCloser.Close() - var err error + var ( + err error + resources *application.ManagedResourcesResponse + ) appName := name if app == nil { @@ -70,14 +73,16 @@ func GetDiff(ctx context.Context, name string, manifests []string, app *argoappv telemetry.SetError(span, err, "Get Argo App") return pkg.CheckResult{}, "", err } - } - resources, err := appClient.ManagedResources(ctx, &application.ResourcesQuery{ - ApplicationName: &appName, - }) - if err != nil { - telemetry.SetError(span, err, "Get Argo Managed Resources") - return pkg.CheckResult{}, "", err + resources, err = appClient.ManagedResources(ctx, &application.ResourcesQuery{ + ApplicationName: &appName, + }) + if err != nil { + telemetry.SetError(span, err, "Get Argo Managed Resources") + return pkg.CheckResult{}, "", err + } + } else { + resources = new(application.ManagedResourcesResponse) } errors.CheckError(err) From 4cf1a09ebced4411845e43b9dd623d0a1c69131f Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Tue, 19 Dec 2023 16:25:31 -0500 Subject: [PATCH 11/24] reuse apps, don't pull the same ones regularly --- pkg/affected_apps/best_effort.go | 124 --------- pkg/affected_apps/best_effort_test.go | 320 ------------------------ pkg/affected_apps/config_matcher.go | 18 +- pkg/affected_apps/matcher.go | 4 +- pkg/argo_client/applications.go | 16 +- pkg/argo_client/manifests.go | 19 +- pkg/config/app_directory.go | 58 ++--- pkg/config/app_directory_test.go | 15 +- pkg/config/vcstoargomap.go | 5 +- pkg/config/walk_kustomize_files_test.go | 30 ++- pkg/diff/diff.go | 38 +-- pkg/events/check.go | 67 +++-- 12 files changed, 124 insertions(+), 590 deletions(-) delete mode 100644 pkg/affected_apps/best_effort.go delete mode 100644 pkg/affected_apps/best_effort_test.go diff --git a/pkg/affected_apps/best_effort.go b/pkg/affected_apps/best_effort.go deleted file mode 100644 index 9a709b6a..00000000 --- a/pkg/affected_apps/best_effort.go +++ /dev/null @@ -1,124 +0,0 @@ -package affected_apps - -import ( - "context" - "fmt" - "path/filepath" - "strings" - - "github.com/rs/zerolog/log" - - "github.com/zapier/kubechecks/pkg/config" -) - -var KustomizeSubPaths = []string{"base/", "bases/", "components/", "overlays/", "resources/"} - -type BestEffort struct { - repoName string - repoFileList []string -} - -func NewBestEffortMatcher(repoName string, repoFileList []string) *BestEffort { - return &BestEffort{ - repoName: repoName, - repoFileList: repoFileList, - } -} - -func (b *BestEffort) AffectedApps(_ context.Context, changeList []string, targetBranch string) (AffectedItems, error) { - appsMap := make(map[string]string) - - for _, file := range changeList { - fileParts := strings.Split(file, "/") - // Expected structure is /apps// or /manifests/ - // and thus anything shorter than 3 elements isn't an Argo manifest - if len(fileParts) < 3 { - continue - } - // If using the /apps/ pattern, the application name is cluster-app from the fileparts - if fileParts[0] == "apps" { - if isKustomizeApp(file) { - if isKustomizeBaseComponentsChange(file) { - // return all apps in overlays dir adjacent to the change dir - oversDir := overlaysDir(file) - for _, repoFile := range b.repoFileList { - if strings.Contains(repoFile, oversDir) { - repoFileParts := strings.Split(repoFile, "/") - appName := fmt.Sprintf("%s-%s", repoFileParts[3], fileParts[1]) - appPath := fmt.Sprintf("%s%s/", oversDir, repoFileParts[3]) - log.Debug().Str("app", appName).Str("path", appPath).Msg("adding application to map") - appsMap[appName] = appPath - } - } - } else { - appsMap[fmt.Sprintf("%s-%s", fileParts[3], fileParts[1])] = fmt.Sprintf("%s/%s/%s/%s/", fileParts[0], fileParts[1], fileParts[2], fileParts[3]) - } - } else { - // helm - if isHelmClusterAppFile(file) { - appsMap[fmt.Sprintf("%s-%s", fileParts[2], fileParts[1])] = fmt.Sprintf("%s/%s/%s/", fileParts[0], fileParts[1], fileParts[2]) - } else { - // touching a file that is at the helm root, return list of all cluster apps below this dir - appDir := filepath.Dir(file) - for _, repoFile := range b.repoFileList { - dir := filepath.Dir(repoFile) - if dir != appDir && strings.Contains(dir, appDir) { - repoFileParts := strings.Split(dir, "/") - if len(repoFileParts) > 2 && len(fileParts) > 1 { - appName := fmt.Sprintf("%s-%s", repoFileParts[2], fileParts[1]) - appPath := fmt.Sprintf("%s/%s/", appDir, repoFileParts[2]) - log.Debug().Str("app", appName).Str("path", appPath).Msg("adding application to map") - appsMap[appName] = appPath - } else { - log.Warn().Str("dir", dir).Msg("ignoring dir") - } - } - } - } - } - } - // If using the /manifests/ pattern, we need the repo name to use as the app - if fileParts[0] == "manifests" || fileParts[0] == "charts" { - appsMap[fmt.Sprintf("%s-%s", fileParts[1], b.repoName)] = fmt.Sprintf("%s/%s/", fileParts[0], fileParts[1]) - } - } - - var appsSlice []config.ApplicationStub - for name, path := range appsMap { - appsSlice = append(appsSlice, config.ApplicationStub{Name: name, Path: path}) - } - - return AffectedItems{Applications: appsSlice}, nil -} - -func isHelmClusterAppFile(file string) bool { - dir := filepath.Dir(file) - return len(strings.Split(dir, "/")) > 2 -} - -func isKustomizeApp(file string) bool { - if file == "kustomization.yaml" { - return true - } else { - for _, sub := range KustomizeSubPaths { - if strings.Contains(file, sub) { - return true - } - } - } - return false -} - -func isKustomizeBaseComponentsChange(file string) bool { - return strings.Contains(file, "base/") || - strings.Contains(file, "bases/") || - strings.Contains(file, "components/") || - strings.Contains(file, "resources/") -} - -func overlaysDir(file string) string { - appBaseDir := filepath.Dir(filepath.Dir(file)) - overlays := filepath.Join(appBaseDir, "overlays/") - - return overlays + "/" -} diff --git a/pkg/affected_apps/best_effort_test.go b/pkg/affected_apps/best_effort_test.go deleted file mode 100644 index 9f0702cd..00000000 --- a/pkg/affected_apps/best_effort_test.go +++ /dev/null @@ -1,320 +0,0 @@ -package affected_apps - -import ( - "context" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/zapier/kubechecks/pkg/config" -) - -func TestBestEffortMatcher(t *testing.T) { - type args struct { - fileList []string - repoName string - } - tests := []struct { - name string - args args - want AffectedItems - }{ - { - name: "helm:cluster-change", - args: args{ - fileList: []string{ - "apps/echo-server/foo-eks-01/values.yaml", - }, - repoName: "", - }, - want: AffectedItems{ - Applications: []config.ApplicationStub{ - {Name: "foo-eks-01-echo-server", Path: "apps/echo-server/foo-eks-01/"}, - }, - }, - }, - { - name: "helm:all-cluster-change", - args: args{ - fileList: []string{ - "apps/echo-server/values.yaml", - }, - repoName: "", - }, - want: AffectedItems{ - Applications: []config.ApplicationStub{ - {Name: "foo-eks-01-echo-server", Path: "apps/echo-server/foo-eks-01/"}, - {Name: "foo-eks-02-echo-server", Path: "apps/echo-server/foo-eks-02/"}, - }, - }, - }, - { - name: "helm:all-cluster-change:and:cluster-app-change", - args: args{ - fileList: []string{ - "apps/echo-server/values.yaml", - "apps/echo-server/foo-eks-01/values.yaml", - }, - repoName: "", - }, - want: AffectedItems{ - Applications: []config.ApplicationStub{ - {Name: "foo-eks-01-echo-server", Path: "apps/echo-server/foo-eks-01/"}, - {Name: "foo-eks-02-echo-server", Path: "apps/echo-server/foo-eks-02/"}, - }, - }, - }, - { - name: "helm:all-cluster-change:and:double-cluster-app-change", - args: args{ - fileList: []string{ - "apps/echo-server/values.yaml", - "apps/echo-server/foo-eks-01/values.yaml", - "apps/echo-server/foo-eks-02/values.yaml", - }, - repoName: "", - }, - want: AffectedItems{ - Applications: []config.ApplicationStub{ - {Name: "foo-eks-01-echo-server", Path: "apps/echo-server/foo-eks-01/"}, - {Name: "foo-eks-02-echo-server", Path: "apps/echo-server/foo-eks-02/"}, - }, - }, - }, - { - name: "kustomize:overlays-change", - args: args{ - fileList: []string{ - "apps/httpbin/overlays/foo-eks-01/kustomization.yaml", - }, - repoName: "", - }, - want: AffectedItems{ - Applications: []config.ApplicationStub{ - {Name: "foo-eks-01-httpbin", Path: "apps/httpbin/overlays/foo-eks-01/"}, - }, - }, - }, - { - name: "kustomize:overlays-subdir-change", - args: args{ - fileList: []string{ - "apps/httpbin/overlays/foo-eks-01/server/deploy.yaml", - }, - repoName: "", - }, - want: AffectedItems{ - Applications: []config.ApplicationStub{ - {Name: "foo-eks-01-httpbin", Path: "apps/httpbin/overlays/foo-eks-01/"}, - }, - }, - }, - { - name: "kustomize:base-change", - args: args{ - fileList: []string{ - "apps/httpbin/base/kustomization.yaml", - }, - repoName: "", - }, - want: AffectedItems{ - Applications: []config.ApplicationStub{ - {Name: "foo-eks-01-httpbin", Path: "apps/httpbin/overlays/foo-eks-01/"}, - }, - }, - }, - { - name: "kustomize:bases-change", - args: args{ - fileList: []string{ - "apps/httpbin/bases/foo.yaml", - }, - repoName: "", - }, - want: AffectedItems{ - Applications: []config.ApplicationStub{ - {Name: "foo-eks-01-httpbin", Path: "apps/httpbin/overlays/foo-eks-01/"}, - }, - }, - }, - { - name: "kustomize:resources-change", - args: args{ - fileList: []string{ - "apps/httpbin/resources/foo.yaml", - }, - repoName: "", - }, - want: AffectedItems{ - Applications: []config.ApplicationStub{ - {Name: "foo-eks-01-httpbin", Path: "apps/httpbin/overlays/foo-eks-01/"}, - }, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - var got AffectedItems - var err error - - matcher := NewBestEffortMatcher(tt.args.repoName, testRepoFiles) - got, err = matcher.AffectedApps(context.TODO(), tt.args.fileList, "master") - require.NoError(t, err) - - assert.Equal(t, len(tt.want.Applications), len(got.Applications)) - assert.Equal(t, len(tt.want.ApplicationSets), len(got.ApplicationSets)) - - // ordering doesn't matter, we just want to make sure the items all exist - wantAppsMap := listToMap(tt.want.Applications, appStubKey) - gotAppsMap := listToMap(got.Applications, appStubKey) - assert.Equal(t, wantAppsMap, gotAppsMap, "Applications not equal") - - wantAppSetsMap := listToMap(tt.want.ApplicationSets, appSetKey) - gotAppSetsMap := listToMap(got.ApplicationSets, appSetKey) - assert.Equal(t, wantAppSetsMap, gotAppSetsMap, "ApplicationSets not equal") - }) - } -} - -func appSetKey(item ApplicationSet) string { - return item.Name -} - -func appStubKey(stub config.ApplicationStub) string { - return stub.Name -} - -func listToMap[T any](items []T, makeKey func(T) string) map[string]T { - result := make(map[string]T) - for _, item := range items { - key := makeKey(item) - result[key] = item - } - return result -} - -var testRepoFiles = []string{ - "apps/echo-server/foo-eks-01/Chart.yaml", - "apps/echo-server/foo-eks-01/values.yaml", - "apps/echo-server/foo-eks-01/templates/something.yaml", - "apps/echo-server/foo-eks-02/Chart.yaml", - "apps/echo-server/foo-eks-02/values.yaml", - "apps/echo-server/foo-eks-02/templates/something.yaml", - "apps/echo-server/values.yaml", - "apps/echo-server/opslevel.yml", - "apps/httpbin/base/kustomization.yaml", - "apps/httpbin/bases/deploy.yaml", - "apps/httpbin/resources/configmap.yaml", - "apps/httpbin/overlays/foo-eks-01/kustomization.yaml", - "apps/httpbin/overlays/foo-eks-01/server/deploy.yaml", - "apps/httpbin/components/kustomization.yaml", -} - -func Test_isKustomizeApp(t *testing.T) { - type args struct { - file string - } - tests := []struct { - name string - args args - want bool - }{ - { - "overlayskustomzation.yaml", - args{ - "apps/foo/overlays/kustomization.yaml", - }, - true, - }, - { - "basekustomzation.yaml", - args{ - "apps/foo/overlays/kustomization.yaml", - }, - true, - }, - { - "overlaysfile", - args{ - "apps/foo/overlays/foo.yaml", - }, - true, - }, - { - "basefile", - args{ - "apps/foo/base/foo.yaml", - }, - true, - }, - { - "helmvalues", - args{ - "apps/foo/values.yaml", - }, - false, - }, - { - "helmclustervalues", - args{ - "apps/foo/cluster/values.yaml", - }, - false, - }, - { - "helmvalues", - args{ - "apps/foo/values.yaml", - }, - false, - }, - { - "basesfile", - args{ - "apps/foo/bases/foo.yaml", - }, - true, - }, - { - "resourcesfile", - args{ - "apps/foo/resources/foo.yaml", - }, - true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := isKustomizeApp(tt.args.file); got != tt.want { - t.Errorf("isKustomizeApp() = %v, want %v", got, tt.want) - } - }) - } -} - -func Test_overlaysDir(t *testing.T) { - type args struct { - file string - } - tests := []struct { - name string - args args - want string - }{ - { - "basic", - args{ - file: "apps/foo/base/kustomization.yaml", - }, - "apps/foo/overlays/", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := overlaysDir(tt.args.file); got != tt.want { - t.Errorf("overlaysDir() = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/pkg/affected_apps/config_matcher.go b/pkg/affected_apps/config_matcher.go index f7ef4aba..73f9aafc 100644 --- a/pkg/affected_apps/config_matcher.go +++ b/pkg/affected_apps/config_matcher.go @@ -6,10 +6,11 @@ import ( "path" "strings" + "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" + "github.com/pkg/errors" "github.com/rs/zerolog/log" "github.com/zapier/kubechecks/pkg/argo_client" - "github.com/zapier/kubechecks/pkg/config" "github.com/zapier/kubechecks/pkg/repo_config" ) @@ -40,9 +41,18 @@ func (b *ConfigMatcher) AffectedApps(ctx context.Context, changeList []string, t appSetList = append(appSetList, ApplicationSet{appset.Name}) } - var appsSlice []config.ApplicationStub - for name, appPath := range appsMap { - appsSlice = append(appsSlice, config.ApplicationStub{Name: name, Path: appPath}) + argoApps, err := b.argoClient.GetApplications(ctx) + if err != nil { + return AffectedItems{}, errors.Wrap(err, "failed to list applications") + } + + var appsSlice []v1alpha1.Application + for _, app := range argoApps.Items { + if _, ok := appsMap[app.Name]; !ok { + continue + } + + appsSlice = append(appsSlice, app) } return AffectedItems{Applications: appsSlice, ApplicationSets: appSetList}, nil diff --git a/pkg/affected_apps/matcher.go b/pkg/affected_apps/matcher.go index 9928ad88..42427759 100644 --- a/pkg/affected_apps/matcher.go +++ b/pkg/affected_apps/matcher.go @@ -4,11 +4,11 @@ import ( "context" "path" - "github.com/zapier/kubechecks/pkg/config" + "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" ) type AffectedItems struct { - Applications []config.ApplicationStub + Applications []v1alpha1.Application ApplicationSets []ApplicationSet } diff --git a/pkg/argo_client/applications.go b/pkg/argo_client/applications.go index 598b866e..4203b383 100644 --- a/pkg/argo_client/applications.go +++ b/pkg/argo_client/applications.go @@ -12,8 +12,9 @@ import ( "github.com/argoproj/argo-cd/v2/pkg/apiclient/cluster" "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/pkg/errors" - "github.com/zapier/kubechecks/telemetry" "go.opentelemetry.io/otel" + + "github.com/zapier/kubechecks/telemetry" ) // GetApplicationByName takes a context and a name, then queries the Argo Application client to retrieve the Application with the specified name. @@ -35,23 +36,16 @@ func (argo *ArgoClient) GetApplicationByName(ctx context.Context, name string) ( return resp, nil } -// GetKubernetesVersionByApplicationName is a method on the ArgoClient struct that takes a context and an application name as parameters, +// GetKubernetesVersionByApplication is a method on the ArgoClient struct that takes a context and an application name as parameters, // and returns the Kubernetes version of the destination cluster where the specified application is running. // It returns an error if the application or cluster information cannot be retrieved. -func (argo *ArgoClient) GetKubernetesVersionByApplicationName(ctx context.Context, appName string) (string, error) { +func (argo *ArgoClient) GetKubernetesVersionByApplication(ctx context.Context, app v1alpha1.Application) (string, error) { ctx, span := otel.Tracer("Kubechecks").Start(ctx, "GetKubernetesVersionByApplicationName") defer span.End() - // Get application - app, err := argo.GetApplicationByName(ctx, appName) - if err != nil { - telemetry.SetError(span, err, "Argo Get Application By Name error") - return "", err - } - // Get destination cluster // Some app specs have a Name defined, some have a Server defined, some have both, take a valid one and use it - log.Debug().Msgf("for appname %s, server dest says: %s and name dest says: %s", appName, app.Spec.Destination.Server, app.Spec.Destination.Name) + log.Debug().Msgf("for appname %s, server dest says: %s and name dest says: %s", app.Name, app.Spec.Destination.Server, app.Spec.Destination.Name) var clusterRequest *cluster.ClusterQuery if app.Spec.Destination.Server != "" { clusterRequest = &cluster.ClusterQuery{Server: app.Spec.Destination.Server} diff --git a/pkg/argo_client/manifests.go b/pkg/argo_client/manifests.go index ea7d4522..ad5d1a84 100644 --- a/pkg/argo_client/manifests.go +++ b/pkg/argo_client/manifests.go @@ -5,7 +5,6 @@ import ( "fmt" "time" - "github.com/argoproj/argo-cd/v2/pkg/apiclient/application" "github.com/argoproj/argo-cd/v2/pkg/apiclient/cluster" "github.com/argoproj/argo-cd/v2/pkg/apiclient/settings" argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" @@ -21,7 +20,7 @@ import ( "github.com/zapier/kubechecks/telemetry" ) -func GetManifestsLocal(ctx context.Context, name string, tempRepoDir string, changedAppFilePath string, app *argoappv1.Application) ([]string, error) { +func GetManifestsLocal(ctx context.Context, name string, tempRepoDir string, changedAppFilePath string, app argoappv1.Application) ([]string, error) { var err error ctx, span := otel.Tracer("Kubechecks").Start(ctx, "GetManifestsLocal") @@ -36,9 +35,6 @@ func GetManifestsLocal(ctx context.Context, name string, tempRepoDir string, cha }() argoClient := GetArgoClient() - appCloser, appClient := argoClient.GetApplicationClient() - defer appCloser.Close() - clusterCloser, clusterIf := argoClient.GetClusterClient() defer clusterCloser.Close() @@ -47,19 +43,6 @@ func GetManifestsLocal(ctx context.Context, name string, tempRepoDir string, cha log.Debug().Str("name", name).Msg("generating diff for application...") - if app == nil { - appName := name - app, err = appClient.Get(ctx, &application.ApplicationQuery{ - Name: &appName, - }) - if err != nil { - telemetry.SetError(span, err, "Argo Get App") - getManifestsFailed.WithLabelValues(name).Inc() - return nil, errors.Wrap(err, "failed to get application") - } - log.Trace().Msgf("Argo App: %+v", app) - } - cluster, err := clusterIf.Get(ctx, &cluster.ClusterQuery{Name: app.Spec.Destination.Name, Server: app.Spec.Destination.Server}) if err != nil { telemetry.SetError(span, err, "Argo Get Cluster") diff --git a/pkg/config/app_directory.go b/pkg/config/app_directory.go index 98053fcf..9424678a 100644 --- a/pkg/config/app_directory.go +++ b/pkg/config/app_directory.go @@ -8,24 +8,18 @@ import ( "github.com/rs/zerolog/log" ) -type ApplicationStub struct { - Name, Path, TargetRevision string - - IsHelm, IsKustomize bool -} - type AppDirectory struct { appDirs map[string][]string // directory -> array of app names appFiles map[string][]string // file path -> array of app names - appsMap map[string]ApplicationStub // app name -> app stub + appsMap map[string]v1alpha1.Application // app name -> app stub } func NewAppDirectory() *AppDirectory { return &AppDirectory{ appDirs: make(map[string][]string), appFiles: make(map[string][]string), - appsMap: make(map[string]ApplicationStub), + appsMap: make(map[string]v1alpha1.Application), } } @@ -35,7 +29,7 @@ func (d *AppDirectory) Count() int { func (d *AppDirectory) Union(other *AppDirectory) *AppDirectory { var join AppDirectory - join.appsMap = mergeMaps(d.appsMap, other.appsMap, takeFirst[ApplicationStub]) + join.appsMap = mergeMaps(d.appsMap, other.appsMap, takeFirst[v1alpha1.Application]) join.appDirs = mergeMaps(d.appDirs, other.appDirs, mergeLists[string]) join.appFiles = mergeMaps(d.appFiles, other.appFiles, mergeLists[string]) return &join @@ -44,14 +38,11 @@ func (d *AppDirectory) Union(other *AppDirectory) *AppDirectory { func (d *AppDirectory) ProcessApp(app v1alpha1.Application) { appName := app.Name - src := app.Spec.Source - if src == nil { - return - } + src := app.Spec.GetSource() // common data srcPath := src.Path - d.AddAppStub(appName, srcPath, app.Spec.Source.TargetRevision, src.IsHelm(), !src.Kustomize.IsZero()) + d.AddApp(app) // handle extra helm paths if helm := src.Helm; helm != nil { @@ -67,7 +58,7 @@ func (d *AppDirectory) ProcessApp(app v1alpha1.Application) { } } -func (d *AppDirectory) FindAppsBasedOnChangeList(changeList []string, targetBranch string) []ApplicationStub { +func (d *AppDirectory) FindAppsBasedOnChangeList(changeList []string, targetBranch string) []v1alpha1.Application { log.Debug().Msgf("checking %d changes", len(changeList)) appsSet := make(map[string]struct{}) @@ -93,7 +84,7 @@ func (d *AppDirectory) FindAppsBasedOnChangeList(changeList []string, targetBran } } - var appsSlice []ApplicationStub + var appsSlice []v1alpha1.Application for appName := range appsSet { app, ok := d.appsMap[appName] if !ok { @@ -102,7 +93,7 @@ func (d *AppDirectory) FindAppsBasedOnChangeList(changeList []string, targetBran } if !shouldInclude(app, targetBranch) { - log.Debug().Msgf("target revision of %s is %s and does not match '%s'", appName, app.TargetRevision, targetBranch) + log.Debug().Msgf("target revision of %s is %s and does not match '%s'", appName, getTargetRevision(app), targetBranch) continue } @@ -113,16 +104,25 @@ func (d *AppDirectory) FindAppsBasedOnChangeList(changeList []string, targetBran return appsSlice } -func shouldInclude(app ApplicationStub, targetBranch string) bool { - if app.TargetRevision == "" { +func getTargetRevision(app v1alpha1.Application) string { + return app.Spec.GetSource().TargetRevision +} + +func getSourcePath(app v1alpha1.Application) string { + return app.Spec.GetSource().Path +} + +func shouldInclude(app v1alpha1.Application, targetBranch string) bool { + targetRevision := getTargetRevision(app) + if targetRevision == "" { return true } - if app.TargetRevision == targetBranch { + if targetRevision == targetBranch { return true } - if app.TargetRevision == "HEAD" { + if targetRevision == "HEAD" { if targetBranch == "main" { return true } @@ -135,8 +135,8 @@ func shouldInclude(app ApplicationStub, targetBranch string) bool { return false } -func (d *AppDirectory) GetApps(filter func(stub ApplicationStub) bool) []ApplicationStub { - var result []ApplicationStub +func (d *AppDirectory) GetApps(filter func(stub v1alpha1.Application) bool) []v1alpha1.Application { + var result []v1alpha1.Application for _, value := range d.appsMap { if filter != nil && !filter(value) { continue @@ -146,15 +146,9 @@ func (d *AppDirectory) GetApps(filter func(stub ApplicationStub) bool) []Applica return result } -func (d *AppDirectory) AddAppStub(appName, srcPath, targetRevision string, isHelm, isKustomize bool) { - d.appsMap[appName] = ApplicationStub{ - Name: appName, - TargetRevision: targetRevision, - Path: srcPath, - IsHelm: isHelm, - IsKustomize: isKustomize, - } - d.AddDir(appName, srcPath) +func (d *AppDirectory) AddApp(app v1alpha1.Application) { + d.appsMap[app.Name] = app + d.AddDir(app.Name, getSourcePath(app)) } func (d *AppDirectory) AddDir(appName, path string) { diff --git a/pkg/config/app_directory_test.go b/pkg/config/app_directory_test.go index afe2a094..8453d677 100644 --- a/pkg/config/app_directory_test.go +++ b/pkg/config/app_directory_test.go @@ -32,11 +32,8 @@ func TestPathsAreJoinedProperly(t *testing.T) { rad.ProcessApp(app1) - assert.Equal(t, map[string]ApplicationStub{ - "test-app": { - Name: "test-app", - Path: "/test1/test2", - }, + assert.Equal(t, map[string]v1alpha1.Application{ + "test-app": app1, }, rad.appsMap) assert.Equal(t, map[string][]string{ "/test1/test2": {"test-app"}, @@ -91,7 +88,13 @@ func TestShouldInclude(t *testing.T) { for _, tc := range testcases { t.Run(fmt.Sprintf("%v", tc), func(t *testing.T) { - actual := shouldInclude(ApplicationStub{TargetRevision: tc.argocdAppBranch}, tc.vcsMergeTarget) + actual := shouldInclude(v1alpha1.Application{ + Spec: v1alpha1.ApplicationSpec{ + Source: &v1alpha1.ApplicationSource{ + TargetRevision: tc.argocdAppBranch, + }, + }, + }, tc.vcsMergeTarget) assert.Equal(t, tc.expected, actual) }) } diff --git a/pkg/config/vcstoargomap.go b/pkg/config/vcstoargomap.go index 5a1e7d19..67a18969 100644 --- a/pkg/config/vcstoargomap.go +++ b/pkg/config/vcstoargomap.go @@ -43,8 +43,9 @@ func (v2a *VcsToArgoMap) WalkKustomizeApps(repo *repo.Repo, fs fs.FS) *AppDirect ) for _, app := range apps { - if err = walkKustomizeFiles(result, fs, app.Name, app.Path); err != nil { - log.Error().Err(err).Msgf("failed to parse kustomize.yaml in %s", app.Path) + appPath := app.Spec.GetSource().Path + if err = walkKustomizeFiles(result, fs, app.Name, appPath); err != nil { + log.Error().Err(err).Msgf("failed to parse kustomize.yaml in %s", appPath) } } diff --git a/pkg/config/walk_kustomize_files_test.go b/pkg/config/walk_kustomize_files_test.go index 0a2bef6a..3834fc50 100644 --- a/pkg/config/walk_kustomize_files_test.go +++ b/pkg/config/walk_kustomize_files_test.go @@ -4,8 +4,10 @@ import ( "testing" "testing/fstest" + "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func TestKustomizeWalking(t *testing.T) { @@ -73,10 +75,32 @@ resources: } ) + newApp := func(name, path, revision string, isHelm, isKustomize bool) v1alpha1.Application { + app := v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Spec: v1alpha1.ApplicationSpec{ + Source: &v1alpha1.ApplicationSource{ + Path: path, + TargetRevision: revision, + }, + }, + } + + if isHelm { + app.Spec.Source.Helm = &v1alpha1.ApplicationSourceHelm{} + } + if isKustomize { + app.Spec.Source.Kustomize = &v1alpha1.ApplicationSourceKustomize{} + } + return app + } + appdir := NewAppDirectory() - appdir.AddAppStub(kustomizeApp1Name, kustomizeApp1Path, "HEAD", false, true) - appdir.AddAppStub(kustomizeApp2Name, kustomizeApp2Path, "HEAD", false, true) - appdir.AddAppStub(kustomizeBaseName, kustomizeBasePath, "HEAD", false, true) + appdir.AddApp(newApp(kustomizeApp1Name, kustomizeApp1Path, "HEAD", false, true)) + appdir.AddApp(newApp(kustomizeApp2Name, kustomizeApp2Path, "HEAD", false, true)) + appdir.AddApp(newApp(kustomizeBaseName, kustomizeBasePath, "HEAD", false, true)) err = walkKustomizeFiles(appdir, fs, kustomizeApp1Name, kustomizeApp1Path) require.NoError(t, err) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 4d4221a1..49cce71f 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -45,17 +45,14 @@ changedFilePath should be the root of the changed folder from https://github.com/argoproj/argo-cd/blob/d3ff9757c460ae1a6a11e1231251b5d27aadcdd1/cmd/argocd/commands/app.go#L879 */ -func GetDiff(ctx context.Context, name string, manifests []string, app *argoappv1.Application, addApp func(*argoappv1.Application)) (pkg.CheckResult, string, error) { +func GetDiff(ctx context.Context, name string, manifests []string, app argoappv1.Application, addApp func(argoappv1.Application)) (pkg.CheckResult, string, error) { ctx, span := otel.Tracer("Kubechecks").Start(ctx, "GetDiff") defer span.End() argoClient := argo_client.GetArgoClient() - closer, appClient := argoClient.GetApplicationClient() log.Debug().Str("name", name).Msg("generating diff for application...") - defer closer.Close() - settingsCloser, settingsClient := argoClient.GetSettingsClient() defer settingsCloser.Close() @@ -64,26 +61,7 @@ func GetDiff(ctx context.Context, name string, manifests []string, app *argoappv resources *application.ManagedResourcesResponse ) - appName := name - if app == nil { - app, err = appClient.Get(ctx, &application.ApplicationQuery{ - Name: &appName, - }) - if err != nil { - telemetry.SetError(span, err, "Get Argo App") - return pkg.CheckResult{}, "", err - } - - resources, err = appClient.ManagedResources(ctx, &application.ResourcesQuery{ - ApplicationName: &appName, - }) - if err != nil { - telemetry.SetError(span, err, "Get Argo Managed Resources") - return pkg.CheckResult{}, "", err - } - } else { - resources = new(application.ManagedResourcesResponse) - } + resources = new(application.ManagedResourcesResponse) errors.CheckError(err) items := make([]objKeyLiveTarget, 0) @@ -182,23 +160,25 @@ func GetDiff(ctx context.Context, name string, manifests []string, app *argoappv return cr, diff, nil } -func isApp(item objKeyLiveTarget, manifests []byte) (*argoappv1.Application, bool) { +var nilApp = argoappv1.Application{} + +func isApp(item objKeyLiveTarget, manifests []byte) (argoappv1.Application, bool) { if strings.ToLower(item.key.Group) != "argoproj.io" { log.Debug().Str("group", item.key.Group).Msg("group is not correct") - return nil, false + return nilApp, false } if strings.ToLower(item.key.Kind) != "application" { log.Debug().Str("kind", item.key.Kind).Msg("kind is not correct") - return nil, false + return nilApp, false } var app argoappv1.Application if err := json.Unmarshal(manifests, &app); err != nil { log.Warn().Err(err).Msg("failed to deserialize application") - return nil, false + return nilApp, false } - return &app, true + return app, true } // from https://github.com/argoproj/argo-cd/blob/d3ff9757c460ae1a6a11e1231251b5d27aadcdd1/cmd/argocd/commands/app.go#L879 diff --git a/pkg/events/check.go b/pkg/events/check.go index a37ee6cd..928c8901 100644 --- a/pkg/events/check.go +++ b/pkg/events/check.go @@ -9,7 +9,7 @@ import ( "sync/atomic" "time" - argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" + "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/pkg/errors" "github.com/rs/zerolog" "github.com/rs/zerolog/log" @@ -34,7 +34,6 @@ import ( type CheckEvent struct { client pkg.Client // Client exposing methods to communicate with platform of user choice fileList []string // What files have changed in this PR/MR - repoFiles []string // All files in this repository TempWorkingDir string // Location of the local repo repo *repo.Repo logger zerolog.Logger @@ -45,9 +44,9 @@ type CheckEvent struct { cfg *config.ServerConfig - createdApps map[string]*argoappv1.Application + createdApps map[string]v1alpha1.Application addedAppsSet map[string]struct{} - appChannel chan appStruct + appChannel chan *v1alpha1.Application doneChannel chan bool } @@ -57,7 +56,7 @@ func NewCheckEvent(repo *repo.Repo, client pkg.Client, cfg *config.ServerConfig) ce := &CheckEvent{ cfg: cfg, client: client, - createdApps: make(map[string]*argoappv1.Application), + createdApps: make(map[string]v1alpha1.Application), repo: repo, } @@ -159,23 +158,12 @@ func (ce *CheckEvent) GenerateListOfAffectedApps(ctx context.Context, targetBran if cfg != nil { log.Debug().Msg("using the config matcher") matcher = affected_apps.NewConfigMatcher(cfg) - } else if viper.GetBool("monitor-all-applications") { + } else { log.Debug().Msg("using an argocd matcher") matcher, err = affected_apps.NewArgocdMatcher(ce.cfg.VcsToArgoMap, ce.repo, ce.TempWorkingDir) if err != nil { return errors.Wrap(err, "failed to create argocd matcher") } - } else { - log.Debug().Msg("using best effort matcher") - ce.repoFiles, err = ce.repo.GetListOfRepoFiles() - if err != nil { - telemetry.SetError(span, err, "Get List of Repo Files") - - ce.logger.Error().Err(err).Msg("could not get list of repo files") - // continue with an empty list - ce.repoFiles = []string{} - } - matcher = affected_apps.NewBestEffortMatcher(ce.repo.Name, ce.repoFiles) } ce.affectedItems, err = matcher.AffectedApps(ctx, ce.fileList, targetBranch) if err != nil { @@ -194,11 +182,6 @@ func (ce *CheckEvent) GenerateListOfAffectedApps(ctx context.Context, targetBran return err } -type appStruct struct { - name string - dir string -} - func (ce *CheckEvent) ProcessApps(ctx context.Context) { ctx, span := otel.Tracer("Kubechecks").Start(ctx, "ProcessApps", trace.WithAttributes( @@ -221,7 +204,7 @@ func (ce *CheckEvent) ProcessApps(ctx context.Context) { // Concurrently process all apps, with a corresponding error channel for reporting back failures ce.addedAppsSet = make(map[string]struct{}) - ce.appChannel = make(chan appStruct, len(ce.affectedItems.Applications)) + ce.appChannel = make(chan *v1alpha1.Application) ce.doneChannel = make(chan bool, len(ce.affectedItems.Applications)) // If the number of affected apps that we have is less than our worker limit, lower the worker limit @@ -238,7 +221,7 @@ func (ce *CheckEvent) ProcessApps(ctx context.Context) { // Produce apps onto channel for _, app := range ce.affectedItems.Applications { - ce.queueApp(app.Name, app.Path) + ce.queueApp(app) } returnCount := 0 @@ -270,21 +253,19 @@ func (ce *CheckEvent) ProcessApps(ctx context.Context) { ce.CommitStatus(ctx, pkg.StateSuccess) } -func (ce *CheckEvent) queueApp(appName, path string) { - a := appStruct{ - name: appName, - dir: path, - } +func (ce *CheckEvent) queueApp(app v1alpha1.Application) { + name := app.Name + dir := app.Spec.GetSource().Path - ce.logger.Debug().Str("app", a.name).Str("dir", a.dir).Msg("producing app on channel") + ce.logger.Debug().Str("app", name).Str("dir", dir).Msg("producing app on channel") - key := fmt.Sprintf("%s::%s", a.name, a.dir) + key := fmt.Sprintf("%s::%s", name, dir) if _, ok := ce.addedAppsSet[key]; ok { return } ce.addedAppsSet[key] = struct{}{} - ce.appChannel <- a + ce.appChannel <- &app } // CommitStatus sets the commit status on the MR @@ -301,8 +282,13 @@ func (ce *CheckEvent) CommitStatus(ctx context.Context, status pkg.CommitState) // Process all apps on the provided channel func (ce *CheckEvent) appWorkers(ctx context.Context, workerID int) { for app := range ce.appChannel { - ce.logger.Info().Int("workerID", workerID).Str("app", app.name).Msg("Processing App") - isSuccess := ce.processApp(ctx, app.name, app.dir) + if app == nil { + log.Warn().Msg("appWorkers received a nil app") + continue + } + + ce.logger.Info().Int("workerID", workerID).Str("app", app.Name).Msg("Processing App") + isSuccess := ce.processApp(ctx, *app) ce.doneChannel <- isSuccess } } @@ -312,7 +298,10 @@ func (ce *CheckEvent) appWorkers(ctx context.Context, workerID int) { // It takes a context (ctx), application name (app), directory (dir) as input and returns an error if any check fails. // The processing is performed concurrently using Go routines and error groups. Any check results are sent through // the returnChan. The function also manages the inFlight atomic counter to track active processing routines. -func (ce *CheckEvent) processApp(ctx context.Context, appName, dir string) bool { +func (ce *CheckEvent) processApp(ctx context.Context, app v1alpha1.Application) bool { + appName := app.Name + dir := app.Spec.GetSource().Path + ctx, span := otel.Tracer("Kubechecks").Start(ctx, "processApp", trace.WithAttributes( attribute.String("app", appName), attribute.String("dir", dir), @@ -340,7 +329,7 @@ func (ce *CheckEvent) processApp(ctx context.Context, appName, dir string) bool formattedManifests := argo_client.FormatManifestsYAML(manifests) ce.logger.Trace().Msgf("Manifests:\n%+v\n", formattedManifests) - k8sVersion, err := argo_client.GetArgoClient().GetKubernetesVersionByApplicationName(ctx, appName) + k8sVersion, err := argo_client.GetArgoClient().GetKubernetesVersionByApplication(ctx, app) if err != nil { ce.logger.Error().Err(err).Msg("Error retrieving the Kubernetes version") k8sVersion = viper.GetString("fallback-k8s-version") @@ -354,9 +343,9 @@ func (ce *CheckEvent) processApp(ctx context.Context, appName, dir string) bool run := ce.createRunner(span, ctx, appName, &wg) run("validating app against schema", ce.validateSchemas(ctx, appName, k8sVersion, ce.TempWorkingDir, formattedManifests)) - run("generating diff for app", ce.generateDiff(ctx, appName, manifests, func(app *argoappv1.Application) { + run("generating diff for app", ce.generateDiff(ctx, appName, manifests, func(app v1alpha1.Application) { ce.createdApps[app.Name] = app - ce.queueApp(app.Name, app.Spec.GetSource().Path) + ce.queueApp(app) })) if viper.GetBool("enable-conftest") { @@ -447,7 +436,7 @@ func (ce *CheckEvent) validatePolicy(ctx context.Context, app string) func() (pk } } -func (ce *CheckEvent) generateDiff(ctx context.Context, appName string, manifests []string, addApp func(app *argoappv1.Application)) func() (pkg.CheckResult, error) { +func (ce *CheckEvent) generateDiff(ctx context.Context, appName string, manifests []string, addApp func(app v1alpha1.Application)) func() (pkg.CheckResult, error) { return func() (pkg.CheckResult, error) { cr, rawDiff, err := diff.GetDiff(ctx, appName, manifests, ce.createdApps[appName], addApp) if err != nil { From 5bf3b3f69432fc53cec0f129b403bdf25db99e4e Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Tue, 19 Dec 2023 16:47:57 -0500 Subject: [PATCH 12/24] better debugging --- pkg/argo_client/manifests.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/pkg/argo_client/manifests.go b/pkg/argo_client/manifests.go index ad5d1a84..d61a54a9 100644 --- a/pkg/argo_client/manifests.go +++ b/pkg/argo_client/manifests.go @@ -35,15 +35,17 @@ func GetManifestsLocal(ctx context.Context, name string, tempRepoDir string, cha }() argoClient := GetArgoClient() - clusterCloser, clusterIf := argoClient.GetClusterClient() + clusterCloser, clusterClient := argoClient.GetClusterClient() defer clusterCloser.Close() settingsCloser, settingsClient := argoClient.GetSettingsClient() defer settingsCloser.Close() - log.Debug().Str("name", name).Msg("generating diff for application...") - - cluster, err := clusterIf.Get(ctx, &cluster.ClusterQuery{Name: app.Spec.Destination.Name, Server: app.Spec.Destination.Server}) + log.Debug(). + Str("clusterName", app.Spec.Destination.Name). + Str("clusterServer", app.Spec.Destination.Server). + Msg("getting cluster") + cluster, err := clusterClient.Get(ctx, &cluster.ClusterQuery{Name: app.Spec.Destination.Name, Server: app.Spec.Destination.Server}) if err != nil { telemetry.SetError(span, err, "Argo Get Cluster") getManifestsFailed.WithLabelValues(name).Inc() @@ -70,8 +72,8 @@ func GetManifestsLocal(ctx context.Context, name string, tempRepoDir string, cha */ source := app.Spec.GetSource() - log.Debug().Msgf("App source: %+v", source) + log.Debug().Str("name", name).Msg("generating diff for application...") res, err := repository.GenerateManifests(ctx, fmt.Sprintf("%s/%s", tempRepoDir, changedAppFilePath), tempRepoDir, source.TargetRevision, &repoapiclient.ManifestRequest{ Repo: &argoappv1.Repository{Repo: source.RepoURL}, AppLabelKey: argoSettings.AppLabelKey, From d1291b3e0876f5a683fabd4f34c2048b537bf306 Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Tue, 19 Dec 2023 17:26:16 -0500 Subject: [PATCH 13/24] add a bunch more debugging --- pkg/config/app_directory.go | 6 +++++- pkg/diff/diff.go | 4 ++-- pkg/events/check.go | 33 ++++++++++++++++----------------- 3 files changed, 23 insertions(+), 20 deletions(-) diff --git a/pkg/config/app_directory.go b/pkg/config/app_directory.go index 9424678a..603e6fe5 100644 --- a/pkg/config/app_directory.go +++ b/pkg/config/app_directory.go @@ -66,7 +66,6 @@ func (d *AppDirectory) FindAppsBasedOnChangeList(changeList []string, targetBran log.Debug().Msgf("change: %s", changePath) for dir, appNames := range d.appDirs { - log.Debug().Msgf("- app path: %s", dir) if strings.HasPrefix(changePath, dir) { log.Debug().Msg("dir match!") for _, appName := range appNames { @@ -147,6 +146,11 @@ func (d *AppDirectory) GetApps(filter func(stub v1alpha1.Application) bool) []v1 } func (d *AppDirectory) AddApp(app v1alpha1.Application) { + log.Debug(). + Str("appName", app.Name). + Str("cluster-name", app.Spec.Destination.Name). + Str("cluster-server", app.Spec.Destination.Server). + Msg("found app") d.appsMap[app.Name] = app d.AddDir(app.Name, getSourcePath(app)) } diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 49cce71f..eb66a755 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -45,13 +45,13 @@ changedFilePath should be the root of the changed folder from https://github.com/argoproj/argo-cd/blob/d3ff9757c460ae1a6a11e1231251b5d27aadcdd1/cmd/argocd/commands/app.go#L879 */ -func GetDiff(ctx context.Context, name string, manifests []string, app argoappv1.Application, addApp func(argoappv1.Application)) (pkg.CheckResult, string, error) { +func GetDiff(ctx context.Context, manifests []string, app argoappv1.Application, addApp func(argoappv1.Application)) (pkg.CheckResult, string, error) { ctx, span := otel.Tracer("Kubechecks").Start(ctx, "GetDiff") defer span.End() argoClient := argo_client.GetArgoClient() - log.Debug().Str("name", name).Msg("generating diff for application...") + log.Debug().Str("name", app.Name).Msg("generating diff for application...") settingsCloser, settingsClient := argoClient.GetSettingsClient() defer settingsCloser.Close() diff --git a/pkg/events/check.go b/pkg/events/check.go index 928c8901..fbcb7bbc 100644 --- a/pkg/events/check.go +++ b/pkg/events/check.go @@ -44,7 +44,6 @@ type CheckEvent struct { cfg *config.ServerConfig - createdApps map[string]v1alpha1.Application addedAppsSet map[string]struct{} appChannel chan *v1alpha1.Application doneChannel chan bool @@ -54,10 +53,9 @@ var inFlight int32 func NewCheckEvent(repo *repo.Repo, client pkg.Client, cfg *config.ServerConfig) *CheckEvent { ce := &CheckEvent{ - cfg: cfg, - client: client, - createdApps: make(map[string]v1alpha1.Application), - repo: repo, + cfg: cfg, + client: client, + repo: repo, } ce.logger = log.Logger.With().Str("repo", repo.Name).Int("event_id", repo.CheckID).Logger() @@ -257,14 +255,18 @@ func (ce *CheckEvent) queueApp(app v1alpha1.Application) { name := app.Name dir := app.Spec.GetSource().Path - ce.logger.Debug().Str("app", name).Str("dir", dir).Msg("producing app on channel") + ce.logger.Debug(). + Str("app", name). + Str("dir", dir). + Str("cluster-name", app.Spec.Destination.Name). + Str("cluster-server", app.Spec.Destination.Server). + Msg("producing app on channel") - key := fmt.Sprintf("%s::%s", name, dir) - if _, ok := ce.addedAppsSet[key]; ok { + if _, ok := ce.addedAppsSet[name]; ok { return } - ce.addedAppsSet[key] = struct{}{} + ce.addedAppsSet[name] = struct{}{} ce.appChannel <- &app } @@ -317,7 +319,7 @@ func (ce *CheckEvent) processApp(ctx context.Context, app v1alpha1.Application) ce.vcsNote.AddNewApp(ctx, appName) ce.logger.Debug().Msgf("Getting manifests for app: %s with code at %s/%s", appName, ce.TempWorkingDir, dir) - manifests, err := argo_client.GetManifestsLocal(ctx, appName, ce.TempWorkingDir, dir, ce.createdApps[appName]) + manifests, err := argo_client.GetManifestsLocal(ctx, appName, ce.TempWorkingDir, dir, app) if err != nil { ce.logger.Error().Err(err).Msgf("Unable to get manifests for %s in %s", appName, dir) cr := pkg.CheckResult{State: pkg.StateError, Summary: "Unable to get manifests", Details: fmt.Sprintf("```\n%s\n```", ce.cleanupGetManifestsError(err))} @@ -343,10 +345,7 @@ func (ce *CheckEvent) processApp(ctx context.Context, app v1alpha1.Application) run := ce.createRunner(span, ctx, appName, &wg) run("validating app against schema", ce.validateSchemas(ctx, appName, k8sVersion, ce.TempWorkingDir, formattedManifests)) - run("generating diff for app", ce.generateDiff(ctx, appName, manifests, func(app v1alpha1.Application) { - ce.createdApps[app.Name] = app - ce.queueApp(app) - })) + run("generating diff for app", ce.generateDiff(ctx, app, manifests, ce.queueApp)) if viper.GetBool("enable-conftest") { run("validation policy", ce.validatePolicy(ctx, appName)) @@ -436,14 +435,14 @@ func (ce *CheckEvent) validatePolicy(ctx context.Context, app string) func() (pk } } -func (ce *CheckEvent) generateDiff(ctx context.Context, appName string, manifests []string, addApp func(app v1alpha1.Application)) func() (pkg.CheckResult, error) { +func (ce *CheckEvent) generateDiff(ctx context.Context, app v1alpha1.Application, manifests []string, addApp func(app v1alpha1.Application)) func() (pkg.CheckResult, error) { return func() (pkg.CheckResult, error) { - cr, rawDiff, err := diff.GetDiff(ctx, appName, manifests, ce.createdApps[appName], addApp) + cr, rawDiff, err := diff.GetDiff(ctx, manifests, app, addApp) if err != nil { return pkg.CheckResult{}, err } - diff.AIDiffSummary(ctx, ce.vcsNote, appName, manifests, rawDiff) + diff.AIDiffSummary(ctx, ce.vcsNote, app.Name, manifests, rawDiff) return cr, nil } From f5c102bd8e62c3edb7bcd935f03bef8c09647276 Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Tue, 19 Dec 2023 17:41:10 -0500 Subject: [PATCH 14/24] add the length back again --- pkg/events/check.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/events/check.go b/pkg/events/check.go index fbcb7bbc..eb6625b8 100644 --- a/pkg/events/check.go +++ b/pkg/events/check.go @@ -202,7 +202,7 @@ func (ce *CheckEvent) ProcessApps(ctx context.Context) { // Concurrently process all apps, with a corresponding error channel for reporting back failures ce.addedAppsSet = make(map[string]struct{}) - ce.appChannel = make(chan *v1alpha1.Application) + ce.appChannel = make(chan *v1alpha1.Application, len(ce.affectedItems.Applications)) ce.doneChannel = make(chan bool, len(ce.affectedItems.Applications)) // If the number of affected apps that we have is less than our worker limit, lower the worker limit From 1d7e51856bc3453364b14012d764ed5460d0600c Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Tue, 19 Dec 2023 18:01:54 -0500 Subject: [PATCH 15/24] more testing, debugging --- pkg/events/check.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/pkg/events/check.go b/pkg/events/check.go index eb6625b8..175a2367 100644 --- a/pkg/events/check.go +++ b/pkg/events/check.go @@ -225,11 +225,15 @@ func (ce *CheckEvent) ProcessApps(ctx context.Context) { returnCount := 0 commitStatus := true for appStatus := range ce.doneChannel { + ce.logger.Debug().Msg("finished an app") if !appStatus { + ce.logger.Debug().Msg("app failed, commit status = false") commitStatus = false } returnCount++ + ce.logger.Debug().Int("done apps", returnCount).Int("all apps", len(ce.addedAppsSet)).Msg("completed apps") + if returnCount == len(ce.addedAppsSet) { ce.logger.Debug().Msg("Closing channels") close(ce.appChannel) @@ -284,13 +288,15 @@ func (ce *CheckEvent) CommitStatus(ctx context.Context, status pkg.CommitState) // Process all apps on the provided channel func (ce *CheckEvent) appWorkers(ctx context.Context, workerID int) { for app := range ce.appChannel { - if app == nil { + var isSuccess bool + + if app != nil { + ce.logger.Info().Int("workerID", workerID).Str("app", app.Name).Msg("Processing App") + isSuccess = ce.processApp(ctx, *app) + } else { log.Warn().Msg("appWorkers received a nil app") - continue } - ce.logger.Info().Int("workerID", workerID).Str("app", app.Name).Msg("Processing App") - isSuccess := ce.processApp(ctx, *app) ce.doneChannel <- isSuccess } } From f27e9da3f7964d24cd6e98fb28960057c95f98b9 Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Tue, 19 Dec 2023 18:10:35 -0500 Subject: [PATCH 16/24] try to give some extra cushion --- pkg/events/check.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/events/check.go b/pkg/events/check.go index 175a2367..9796c444 100644 --- a/pkg/events/check.go +++ b/pkg/events/check.go @@ -202,8 +202,8 @@ func (ce *CheckEvent) ProcessApps(ctx context.Context) { // Concurrently process all apps, with a corresponding error channel for reporting back failures ce.addedAppsSet = make(map[string]struct{}) - ce.appChannel = make(chan *v1alpha1.Application, len(ce.affectedItems.Applications)) - ce.doneChannel = make(chan bool, len(ce.affectedItems.Applications)) + ce.appChannel = make(chan *v1alpha1.Application, len(ce.affectedItems.Applications)*2) + ce.doneChannel = make(chan bool, len(ce.affectedItems.Applications)*2) // If the number of affected apps that we have is less than our worker limit, lower the worker limit if ce.workerLimits > len(ce.affectedItems.Applications) { From 8001cfb1eb010ffa7a742a3187f1095ccc25fd8e Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Wed, 20 Dec 2023 15:29:37 -0500 Subject: [PATCH 17/24] support local execution, stop panicing on errors --- cmd/controller_cmd.go | 5 +- cmd/process.go | 46 ++++++++++++++++ pkg/client.go | 6 +-- pkg/config/vcstoargomap.go | 18 +++++++ pkg/diff/diff.go | 86 ++++++++++++++++++++++-------- pkg/events/check.go | 18 ++++--- pkg/github_client/client.go | 101 +++++++++++++++++++++++++++++++++--- pkg/gitlab_client/client.go | 16 +++--- pkg/repo/repo.go | 41 +++++++-------- pkg/server/hook_handler.go | 41 +++++++-------- pkg/server/server.go | 29 +++-------- 11 files changed, 294 insertions(+), 113 deletions(-) create mode 100644 cmd/process.go diff --git a/cmd/controller_cmd.go b/cmd/controller_cmd.go index fc3c77c3..4d6c78d7 100644 --- a/cmd/controller_cmd.go +++ b/cmd/controller_cmd.go @@ -1,6 +1,7 @@ package cmd import ( + "context" "fmt" "os" "os/signal" @@ -30,7 +31,9 @@ var ControllerCmd = &cobra.Command{ UrlPrefix: viper.GetString("webhook-url-prefix"), WebhookSecret: viper.GetString("webhook-secret"), }) - go server.Start() + + ctx := context.Background() + go server.Start(ctx) // graceful termination handler. // when we receive a SIGTERM from kubernetes, check for in-flight requests before exiting. diff --git a/cmd/process.go b/cmd/process.go new file mode 100644 index 00000000..e3efa4be --- /dev/null +++ b/cmd/process.go @@ -0,0 +1,46 @@ +package cmd + +import ( + "context" + + "github.com/rs/zerolog/log" + "github.com/spf13/cobra" + + "github.com/zapier/kubechecks/pkg/config" + "github.com/zapier/kubechecks/pkg/server" +) + +var processCmd = &cobra.Command{ + Use: "process", + Short: "Process a pull request", + Long: "", + Run: func(cmd *cobra.Command, args []string) { + ctx := context.TODO() + + log.Info().Msg("building apps map from argocd") + result, err := config.BuildAppsMap(ctx) + if err != nil { + log.Fatal().Err(err).Msg("failed to build apps map") + } + + cfg := config.ServerConfig{ + UrlPrefix: "--unused--", + WebhookSecret: "--unused--", + VcsToArgoMap: result, + } + + client, _ := server.GetVCSClient() + + repo, err := client.LoadHook(ctx, args[0]) + if err != nil { + log.Fatal().Err(err).Msg("failed to load hook") + return + } + + server.ProcessCheckEvent(ctx, repo, client, &cfg) + }, +} + +func init() { + RootCmd.AddCommand(processCmd) +} diff --git a/pkg/client.go b/pkg/client.go index 76311e8f..d315e1b6 100644 --- a/pkg/client.go +++ b/pkg/client.go @@ -29,9 +29,7 @@ type Client interface { // VerifyHook validates a webhook secret and return the body; must be called even if no secret VerifyHook(*http.Request, string) ([]byte, error) // ParseHook parses webook payload for valid events - ParseHook(*http.Request, []byte) (interface{}, error) - // CreateRepo handles valid events - CreateRepo(context.Context, interface{}) (*repo.Repo, error) + ParseHook(*http.Request, []byte) (*repo.Repo, error) // CommitStatus sets a status for a specific commit on the remote VCS CommitStatus(context.Context, *repo.Repo, CommitState) error // GetHookByUrl gets a webhook by url @@ -42,4 +40,6 @@ type Client interface { GetName() string // Tidy outdated comments either by hiding or deleting them TidyOutdatedComments(context.Context, *repo.Repo) error + // LoadHook creates an EventRequest from the ID of an actual request + LoadHook(ctx context.Context, repoAndId string) (*repo.Repo, error) } diff --git a/pkg/config/vcstoargomap.go b/pkg/config/vcstoargomap.go index 67a18969..47e82f22 100644 --- a/pkg/config/vcstoargomap.go +++ b/pkg/config/vcstoargomap.go @@ -1,10 +1,13 @@ package config import ( + "context" "io/fs" + "github.com/pkg/errors" "github.com/rs/zerolog/log" + "github.com/zapier/kubechecks/pkg/argo_client" "github.com/zapier/kubechecks/pkg/repo" ) @@ -18,6 +21,21 @@ func NewVcsToArgoMap() VcsToArgoMap { } } +func BuildAppsMap(ctx context.Context) (VcsToArgoMap, error) { + result := NewVcsToArgoMap() + argoClient := argo_client.GetArgoClient() + + apps, err := argoClient.GetApplications(ctx) + if err != nil { + return result, errors.Wrap(err, "failed to list applications") + } + for _, app := range apps.Items { + result.AddApp(app) + } + + return result, nil +} + func (v2a *VcsToArgoMap) GetAppsInRepo(repoCloneUrl string) *AppDirectory { repoUrl, err := normalizeRepoUrl(repoCloneUrl) if err != nil { diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index eb66a755..c2bfabce 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -17,7 +17,6 @@ import ( "github.com/argoproj/gitops-engine/pkg/sync/hook" "github.com/argoproj/gitops-engine/pkg/sync/ignore" "github.com/argoproj/gitops-engine/pkg/utils/kube" - "github.com/argoproj/pkg/errors" "github.com/ghodss/yaml" "github.com/go-logr/zerologr" "github.com/pmezard/go-difflib/difflib" @@ -38,6 +37,14 @@ type objKeyLiveTarget struct { target *unstructured.Unstructured } +func isAppMissingErr(err error) bool { + if strings.Contains(err.Error(), "PermissionDenied") { + return true + } + + return false +} + /* Take cli output and return as a string or an array of strings instead of printing @@ -56,19 +63,30 @@ func GetDiff(ctx context.Context, manifests []string, app argoappv1.Application, settingsCloser, settingsClient := argoClient.GetSettingsClient() defer settingsCloser.Close() - var ( - err error - resources *application.ManagedResourcesResponse - ) + closer, appClient := argoClient.GetApplicationClient() + defer closer.Close() - resources = new(application.ManagedResourcesResponse) + resources, err := appClient.ManagedResources(ctx, &application.ResourcesQuery{ + ApplicationName: &app.Name, + }) + if err != nil { + if !isAppMissingErr(err) { + telemetry.SetError(span, err, "Get Argo Managed Resources") + return pkg.CheckResult{}, "", err + } + + resources = new(application.ManagedResourcesResponse) + } - errors.CheckError(err) items := make([]objKeyLiveTarget, 0) var unstructureds []*unstructured.Unstructured for _, mfst := range manifests { obj, err := argoappv1.UnmarshalToUnstructured(mfst) - errors.CheckError(err) + if err != nil { + log.Warn().Err(err).Msg("failed to unmarshal to unstructured") + continue + } + unstructureds = append(unstructureds, obj) } argoSettings, err := settingsClient.Get(ctx, &settings.SettingsQuery{}) @@ -83,8 +101,15 @@ func GetDiff(ctx context.Context, manifests []string, app argoappv1.Application, return pkg.CheckResult{}, "", err } - groupedObjs := groupObjsByKey(unstructureds, liveObjs, app.Spec.Destination.Namespace) - items = groupObjsForDiff(resources, groupedObjs, items, argoSettings, app.Name) + groupedObjs, err := groupObjsByKey(unstructureds, liveObjs, app.Spec.Destination.Namespace) + if err != nil { + return pkg.CheckResult{}, "", err + } + + if items, err = groupObjsForDiff(resources, groupedObjs, items, argoSettings, app.Name); err != nil { + return pkg.CheckResult{}, "", err + } + diffBuffer := &strings.Builder{} var added, modified, removed int for _, item := range items { @@ -108,9 +133,16 @@ func GetDiff(ctx context.Context, manifests []string, app argoappv1.Application, WithTracking(argoSettings.AppLabelKey, argoSettings.TrackingMethod). WithNoCache(). Build() - errors.CheckError(err) + if err != nil { + telemetry.SetError(span, err, "Build Diff") + return pkg.CheckResult{}, "failed to build diff", err + } + diffRes, err := argodiff.StateDiff(item.live, item.target, diffConfig) - errors.CheckError(err) + if err != nil { + telemetry.SetError(span, err, "State Diff") + return pkg.CheckResult{}, "failed to state diff", err + } if diffRes.Modified || item.target == nil || item.live == nil { diffBuffer.WriteString(fmt.Sprintf("===== %s ======\n", resourceId)) @@ -119,8 +151,10 @@ func GetDiff(ctx context.Context, manifests []string, app argoappv1.Application, if item.target != nil && item.live != nil { target = &unstructured.Unstructured{} live = item.live - err = json.Unmarshal(diffRes.PredictedLive, target) - errors.CheckError(err) + if err = json.Unmarshal(diffRes.PredictedLive, target); err != nil { + telemetry.SetError(span, err, "JSON Unmarshall") + log.Warn().Err(err).Msg("failed to unmarshall json") + } } else { live = item.live target = item.target @@ -182,7 +216,7 @@ func isApp(item objKeyLiveTarget, manifests []byte) (argoappv1.Application, bool } // from https://github.com/argoproj/argo-cd/blob/d3ff9757c460ae1a6a11e1231251b5d27aadcdd1/cmd/argocd/commands/app.go#L879 -func groupObjsByKey(localObs []*unstructured.Unstructured, liveObjs []*unstructured.Unstructured, appNamespace string) map[kube.ResourceKey]*unstructured.Unstructured { +func groupObjsByKey(localObs []*unstructured.Unstructured, liveObjs []*unstructured.Unstructured, appNamespace string) (map[kube.ResourceKey]*unstructured.Unstructured, error) { namespacedByGk := make(map[schema.GroupKind]bool) for i := range liveObjs { if liveObjs[i] != nil { @@ -191,7 +225,10 @@ func groupObjsByKey(localObs []*unstructured.Unstructured, liveObjs []*unstructu } } localObs, _, err := controller.DeduplicateTargetObjects(appNamespace, localObs, &resourceInfoProvider{namespacedByGk: namespacedByGk}) - errors.CheckError(err) + if err != nil { + return nil, err + } + objByKey := make(map[kube.ResourceKey]*unstructured.Unstructured) for i := range localObs { obj := localObs[i] @@ -199,16 +236,17 @@ func groupObjsByKey(localObs []*unstructured.Unstructured, liveObjs []*unstructu objByKey[kube.GetResourceKey(obj)] = obj } } - return objByKey + return objByKey, nil } // from https://github.com/argoproj/argo-cd/blob/d3ff9757c460ae1a6a11e1231251b5d27aadcdd1/cmd/argocd/commands/app.go#L879 -func groupObjsForDiff(resources *application.ManagedResourcesResponse, objs map[kube.ResourceKey]*unstructured.Unstructured, items []objKeyLiveTarget, argoSettings *settings.Settings, appName string) []objKeyLiveTarget { +func groupObjsForDiff(resources *application.ManagedResourcesResponse, objs map[kube.ResourceKey]*unstructured.Unstructured, items []objKeyLiveTarget, argoSettings *settings.Settings, appName string) ([]objKeyLiveTarget, error) { resourceTracking := argo.NewResourceTracking() for _, res := range resources.Items { var live = &unstructured.Unstructured{} - err := json.Unmarshal([]byte(res.NormalizedLiveState), &live) - errors.CheckError(err) + if err := json.Unmarshal([]byte(res.NormalizedLiveState), &live); err != nil { + return nil, err + } key := kube.ResourceKey{Name: res.Name, Namespace: res.Namespace, Group: res.Group, Kind: res.Kind} if key.Kind == kube.SecretKind && key.Group == "" { @@ -218,8 +256,9 @@ func groupObjsForDiff(resources *application.ManagedResourcesResponse, objs map[ } if local, ok := objs[key]; ok || live != nil { if local != nil && !kube.IsCRD(local) { - err = resourceTracking.SetAppInstance(local, argoSettings.AppLabelKey, appName, "", argoappv1.TrackingMethod(argoSettings.GetTrackingMethod())) - errors.CheckError(err) + if err := resourceTracking.SetAppInstance(local, argoSettings.AppLabelKey, appName, "", argoappv1.TrackingMethod(argoSettings.GetTrackingMethod())); err != nil { + return nil, err + } } items = append(items, objKeyLiveTarget{key, live, local}) @@ -234,7 +273,8 @@ func groupObjsForDiff(resources *application.ManagedResourcesResponse, objs map[ } items = append(items, objKeyLiveTarget{key, nil, local}) } - return items + + return items, nil } // from https://github.com/argoproj/argo-cd/blob/d3ff9757c460ae1a6a11e1231251b5d27aadcdd1/cmd/argocd/commands/app.go#L879 diff --git a/pkg/events/check.go b/pkg/events/check.go index 9796c444..de6ee72e 100644 --- a/pkg/events/check.go +++ b/pkg/events/check.go @@ -102,7 +102,7 @@ func (ce *CheckEvent) InitializeGit(ctx context.Context) error { _, span := otel.Tracer("Kubechecks").Start(ctx, "InitializeGit") defer span.End() - return repo.InitializeGitSettings(ce.repo.Username, ce.repo.Email) + return ce.repo.InitializeGitSettings() } // CloneRepoLocal takes the repo inside the Check Event and try to clone it locally @@ -259,19 +259,21 @@ func (ce *CheckEvent) queueApp(app v1alpha1.Application) { name := app.Name dir := app.Spec.GetSource().Path - ce.logger.Debug(). - Str("app", name). - Str("dir", dir). - Str("cluster-name", app.Spec.Destination.Name). - Str("cluster-server", app.Spec.Destination.Server). - Msg("producing app on channel") - if _, ok := ce.addedAppsSet[name]; ok { return } ce.addedAppsSet[name] = struct{}{} + + logger := ce.logger.Debug(). + Str("app", name). + Str("dir", dir). + Str("cluster-name", app.Spec.Destination.Name). + Str("cluster-server", app.Spec.Destination.Server) + + logger.Msg("producing app on channel") ce.appChannel <- &app + logger.Msg("finished producing app") } // CommitStatus sets the commit status on the MR diff --git a/pkg/github_client/client.go b/pkg/github_client/client.go index c0c130ef..d74fe790 100644 --- a/pkg/github_client/client.go +++ b/pkg/github_client/client.go @@ -4,6 +4,8 @@ import ( "context" "io" "net/http" + "regexp" + "strconv" "strings" "sync" @@ -101,13 +103,12 @@ func (c *Client) VerifyHook(r *http.Request, secret string) ([]byte, error) { } } -func (c *Client) ParseHook(r *http.Request, payload []byte) (interface{}, error) { - return github.ParseWebHook(github.WebHookType(r), payload) -} +func (c *Client) ParseHook(r *http.Request, request []byte) (*repo.Repo, error) { + payload, err := github.ParseWebHook(github.WebHookType(r), request) + if err != nil { + return nil, err + } -// CreateRepo creates a new generic repo from the webhook payload. Assumes the secret validation/type validation -// Has already occured previously, so we expect a valid event type for the GitHub client in the payload arg -func (c *Client) CreateRepo(_ context.Context, payload interface{}) (*repo.Repo, error) { switch p := payload.(type) { case *github.PullRequestEvent: switch p.GetAction() { @@ -257,3 +258,91 @@ func (c *Client) CreateHook(ctx context.Context, ownerAndRepoName, webhookUrl, w return nil } + +var rePullRequest = regexp.MustCompile(`(.*)/(.*)#(\d+)`) + +func (c *Client) LoadHook(ctx context.Context, id string) (*repo.Repo, error) { + m := rePullRequest.FindStringSubmatch(id) + if len(m) != 4 { + return nil, errors.New("must be in format OWNER/REPO#PR") + } + + ownerName := m[1] + repoName := m[2] + prNumber, err := strconv.ParseInt(m[3], 10, 32) + if err != nil { + return nil, errors.Wrap(err, "failed to parse int") + } + + repoInfo, _, err := c.Repositories.Get(ctx, ownerName, repoName) + if err != nil { + return nil, errors.Wrap(err, "failed to get repo") + } + + pullRequest, _, err := c.PullRequests.Get(ctx, ownerName, repoName, int(prNumber)) + if err != nil { + return nil, errors.Wrap(err, "failed to get pull request") + } + + var labels []string + for _, label := range pullRequest.Labels { + labels = append(labels, label.GetName()) + } + + var ( + baseRef string + headRef, headSha string + login, userName, userEmail string + ) + + if pullRequest.Base != nil { + baseRef = unPtr(pullRequest.Base.Ref) + headRef = unPtr(pullRequest.Head.Ref) + } + + if repoInfo.Owner != nil { + login = unPtr(repoInfo.Owner.Login) + } else { + login = "kubechecks" + } + + if pullRequest.Head != nil { + headSha = unPtr(pullRequest.Head.SHA) + } + + if pullRequest.User != nil { + userName = unPtr(pullRequest.User.Name) + userEmail = unPtr(pullRequest.User.Email) + } + + // these are required for `git merge` later on + if userName == "" { + userName = "kubechecks" + } + if userEmail == "" { + userEmail = "kubechecks@github.com" + } + + return &repo.Repo{ + BaseRef: baseRef, + HeadRef: headRef, + DefaultBranch: unPtr(repoInfo.DefaultBranch), + CloneURL: unPtr(repoInfo.CloneURL), + FullName: repoInfo.GetFullName(), + Owner: login, + Name: repoInfo.GetName(), + CheckID: int(prNumber), + SHA: headSha, + Username: userName, + Email: userEmail, + Labels: labels, + }, nil +} + +func unPtr[T interface{ string | int }](ps *T) T { + if ps == nil { + var t T + return t + } + return *ps +} diff --git a/pkg/gitlab_client/client.go b/pkg/gitlab_client/client.go index 4ebca3cb..f6c0719f 100644 --- a/pkg/gitlab_client/client.go +++ b/pkg/gitlab_client/client.go @@ -89,13 +89,12 @@ func (c *Client) VerifyHook(r *http.Request, secret string) ([]byte, error) { } // ParseHook parses and validates a webhook event; return an err if this isn't valid -func (c *Client) ParseHook(r *http.Request, payload []byte) (interface{}, error) { - return gitlab.ParseHook(gitlab.HookEventType(r), payload) -} +func (c *Client) ParseHook(r *http.Request, request []byte) (*repo.Repo, error) { + eventRequest, err := gitlab.ParseHook(gitlab.HookEventType(r), request) + if err != nil { + return nil, err + } -// CreateRepo takes a valid gitlab webhook event request, and determines if we should process it -// Returns a generic Repo with all info kubechecks needs on success, err if not -func (c *Client) CreateRepo(ctx context.Context, eventRequest interface{}) (*repo.Repo, error) { switch event := eventRequest.(type) { case *gitlab.MergeEvent: switch event.ObjectAttributes.Action { @@ -175,6 +174,11 @@ func (c *Client) CreateHook(ctx context.Context, repoName, webhookUrl, webhookSe return nil } +func (c *Client) LoadHook(ctx context.Context, id string) (*repo.Repo, error) { + //TODO implement me + panic("implement me") +} + func buildRepoFromEvent(event *gitlab.MergeEvent) *repo.Repo { // Convert all labels from this MR to a string array of label names var labels []string diff --git a/pkg/repo/repo.go b/pkg/repo/repo.go index 1483d65d..bd9f362a 100644 --- a/pkg/repo/repo.go +++ b/pkg/repo/repo.go @@ -48,15 +48,14 @@ func (r *Repo) CloneRepoLocal(ctx context.Context, repoDir string) error { // TODO: Look if this is still needed r.RepoDir = repoDir - cmd := execCommand("git", "clone", r.CloneURL, repoDir) + cmd := r.execCommand("git", "clone", r.CloneURL, repoDir) out, err := cmd.CombinedOutput() if err != nil { log.Error().Err(err).Msgf("unable to clone repository, %s", out) return err } - cmd = execCommand("git", "remote") - cmd.Dir = repoDir + cmd = r.execCommand("git", "remote") pipe, _ := cmd.StdoutPipe() var wg sync.WaitGroup scanner := bufio.NewScanner(pipe) @@ -110,8 +109,7 @@ func (r *Repo) MergeIntoTarget(ctx context.Context) error { defer span.End() log.Debug().Msgf("Merging MR commit %s into a tmp branch off of %s for manifest generation...", r.SHA, r.BaseRef) - cmd := execCommand("git", "fetch", r.Remote, r.BaseRef) - cmd.Dir = r.RepoDir + cmd := r.execCommand("git", "fetch", r.Remote, r.BaseRef) err := cmd.Run() if err != nil { telemetry.SetError(span, err, "git fetch remote into target branch") @@ -119,8 +117,7 @@ func (r *Repo) MergeIntoTarget(ctx context.Context) error { return err } - cmd = execCommand("git", "checkout", "-b", "tmp", fmt.Sprintf("%s/%s", r.Remote, r.BaseRef)) - cmd.Dir = r.RepoDir + cmd = r.execCommand("git", "checkout", "-b", "tmp", fmt.Sprintf("%s/%s", r.Remote, r.BaseRef)) _, err = cmd.Output() if err != nil { telemetry.SetError(span, err, "git checkout tmp branch") @@ -128,8 +125,7 @@ func (r *Repo) MergeIntoTarget(ctx context.Context) error { return err } - cmd = execCommand("git", "merge", r.SHA) - cmd.Dir = r.RepoDir + cmd = r.execCommand("git", "merge", r.SHA) out, err := cmd.CombinedOutput() if err != nil { telemetry.SetError(span, err, "merge last commit id into tmp branch") @@ -172,8 +168,7 @@ func (r *Repo) GetListOfChangedFiles(ctx context.Context) ([]string, error) { var fileList = []string{} - cmd := execCommand("git", "diff", "--name-only", fmt.Sprintf("%s/%s", r.Remote, r.BaseRef)) - cmd.Dir = r.RepoDir + cmd := r.execCommand("git", "diff", "--name-only", fmt.Sprintf("%s/%s", r.Remote, r.BaseRef)) pipe, _ := cmd.StdoutPipe() var wg sync.WaitGroup scanner := bufio.NewScanner(pipe) @@ -210,21 +205,28 @@ func walk(s string, d fs.DirEntry, err error) error { return nil } +func (r *Repo) execCommand(name string, args ...string) *exec.Cmd { + log.Debug().Strs("args", args).Msg("building command") + cmd := exec.Command(name, args...) + cmd.Dir = r.RepoDir + return cmd +} + // InitializeGitSettings ensures Git auth is set up for cloning -func InitializeGitSettings(user string, email string) error { - cmd := execCommand("git", "config", "--global", "user.email", email) +func (r *Repo) InitializeGitSettings() error { + cmd := r.execCommand("git", "config", "user.email", r.Email) err := cmd.Run() if err != nil { return errors.Wrap(err, "failed to set git email address") } - cmd = execCommand("git", "config", "--global", "user.name", user) + cmd = r.execCommand("git", "config", "user.name", r.Username) err = cmd.Run() if err != nil { return errors.Wrap(err, "failed to set git user name") } - cloneUrl, err := getCloneUrl(user, viper.GetViper()) + cloneUrl, err := getCloneUrl(r.Username, viper.GetViper()) if err != nil { return errors.Wrap(err, "failed to get clone url") } @@ -241,14 +243,14 @@ func InitializeGitSettings(user string, email string) error { } defer outfile.Close() - cmd = execCommand("echo", cloneUrl) + cmd = r.execCommand("echo", cloneUrl) cmd.Stdout = outfile err = cmd.Run() if err != nil { return errors.Wrap(err, "unable to set git credentials") } - cmd = execCommand("git", "config", "--global", "credential.helper", "store") + cmd = r.execCommand("git", "config", "credential.helper", "store") err = cmd.Run() if err != nil { return errors.Wrap(err, "unable to set git credential usage") @@ -279,8 +281,3 @@ func getCloneUrl(user string, cfg *viper.Viper) (string, error) { } return fmt.Sprintf("%s://%s:%s@%s", scheme, user, vcsToken, hostname), nil } - -func execCommand(name string, args ...string) *exec.Cmd { - log.Debug().Strs("args", args).Msg("building command") - return exec.Command(name, args...) -} diff --git a/pkg/server/hook_handler.go b/pkg/server/hook_handler.go index 4efc0c32..f7373459 100644 --- a/pkg/server/hook_handler.go +++ b/pkg/server/hook_handler.go @@ -88,16 +88,7 @@ func (h *VCSHookHandler) groupHandler(c echo.Context) error { return c.String(http.StatusUnauthorized, "Unauthorized") } - eventRequest, err := h.client.ParseHook(c.Request(), payload) - if err != nil { - // TODO: do something w/ error - log.Error().Err(err).Msg("Failed to parse hook payload. Are you using the right client?") - return echo.ErrBadRequest - } - - // At this point, our client has validated the secret, and parsed a valid event. - // We try to build a generic Repo from this data, to construct our CheckEvent - repo, err := h.client.CreateRepo(ctx, eventRequest) + repo, err := h.client.ParseHook(c.Request(), payload) if err != nil { switch err { case pkg.ErrInvalidType: @@ -118,6 +109,15 @@ func (h *VCSHookHandler) groupHandler(c echo.Context) error { // Takes a constructed Repo, and attempts to run the Kubechecks processing suite against it. // If the Repo is not yet populated, this will fail. func (h *VCSHookHandler) processCheckEvent(ctx context.Context, repo *repo.Repo) { + if !h.passesLabelFilter(repo) { + log.Warn().Str("label-filter", h.labelFilter).Msg("ignoring event, did not have matching label") + return + } + + ProcessCheckEvent(ctx, repo, h.client, h.cfg) +} + +func ProcessCheckEvent(ctx context.Context, repo *repo.Repo, client pkg.Client, cfg *config.ServerConfig) { var span trace.Span ctx, span = otel.Tracer("Kubechecks").Start(ctx, "processCheckEvent", trace.WithAttributes( @@ -131,13 +131,8 @@ func (h *VCSHookHandler) processCheckEvent(ctx context.Context, repo *repo.Repo) ) defer span.End() - if !h.passesLabelFilter(repo) { - log.Warn().Str("label-filter", h.labelFilter).Msg("ignoring event, did not have matching label") - return - } - // If we've gotten here, we can now begin running checks (or trying to) - cEvent := events.NewCheckEvent(repo, h.client, h.cfg) + cEvent := events.NewCheckEvent(repo, client, cfg) err := cEvent.CreateTempDir() if err != nil { @@ -146,13 +141,6 @@ func (h *VCSHookHandler) processCheckEvent(ctx context.Context, repo *repo.Repo) } defer cEvent.Cleanup(ctx) - err = cEvent.InitializeGit(ctx) - if err != nil { - telemetry.SetError(span, err, "Initialize Git") - log.Error().Err(err).Msg("unable to initialize git") - return - } - // Clone the repo's BaseRef (main etc) locally into the temp dir we just made err = cEvent.CloneRepoLocal(ctx) if err != nil { @@ -162,6 +150,13 @@ func (h *VCSHookHandler) processCheckEvent(ctx context.Context, repo *repo.Repo) return } + err = cEvent.InitializeGit(ctx) + if err != nil { + telemetry.SetError(span, err, "Initialize Git") + log.Error().Err(err).Msg("unable to initialize git") + return + } + // Merge the most recent changes into the branch we just cloned err = cEvent.MergeIntoTarget(ctx) if err != nil { diff --git a/pkg/server/server.go b/pkg/server/server.go index f75553d4..31223f5e 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -15,7 +15,6 @@ import ( "github.com/ziflex/lecho/v3" "github.com/zapier/kubechecks/pkg" - "github.com/zapier/kubechecks/pkg/argo_client" "github.com/zapier/kubechecks/pkg/config" ) @@ -36,9 +35,11 @@ func GetServer() *Server { return singleton } -func (s *Server) Start() { - if err := s.buildVcsToArgoMap(); err != nil { +func (s *Server) Start(ctx context.Context) { + if argoMap, err := s.buildVcsToArgoMap(ctx); err != nil { log.Warn().Err(err).Msg("failed to build vcs app map from argo") + } else { + s.cfg.VcsToArgoMap = argoMap } if err := s.ensureWebhooks(); err != nil { @@ -121,26 +122,12 @@ func (s *Server) ensureWebhooks() error { return nil } -func (s *Server) buildVcsToArgoMap() error { - log.Debug().Msg("building VCS to Application Map") +func (s *Server) buildVcsToArgoMap(ctx context.Context) (config.VcsToArgoMap, error) { if !viper.GetBool("monitor-all-applications") { - return nil + return config.NewVcsToArgoMap(), nil } - ctx := context.TODO() - - result := config.NewVcsToArgoMap() - - argoClient := argo_client.GetArgoClient() - - apps, err := argoClient.GetApplications(ctx) - if err != nil { - return errors.Wrap(err, "failed to list applications") - } - for _, app := range apps.Items { - result.AddApp(app) - } + log.Debug().Msg("building VCS to Application Map") - s.cfg.VcsToArgoMap = result - return nil + return config.BuildAppsMap(ctx) } From 028bd28ef57d9dfd00ea52b23ab97e42a77e78ea Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Wed, 20 Dec 2023 16:26:50 -0500 Subject: [PATCH 18/24] lots of refactoring to clean up username/email --- cmd/controller_cmd.go | 20 ++++- cmd/process.go | 12 ++- cmd/vcs.go | 20 +++++ pkg/config/config.go | 3 + pkg/events/check.go | 18 ++--- pkg/github_client/client_test.go | 15 ---- pkg/message.go | 4 +- pkg/repo/repo.go | 19 +++-- pkg/server/hook_handler.go | 85 +++++++--------------- pkg/server/server.go | 6 +- pkg/{ => vcs}/client.go | 14 ++-- pkg/{ => vcs}/github_client/client.go | 74 ++++++++----------- pkg/{ => vcs}/github_client/message.go | 4 +- pkg/{ => vcs}/gitlab_client/backoff.go | 0 pkg/{ => vcs}/gitlab_client/client.go | 53 ++++++-------- pkg/{ => vcs}/gitlab_client/client_test.go | 8 -- pkg/{ => vcs}/gitlab_client/merge.go | 0 pkg/{ => vcs}/gitlab_client/message.go | 4 +- pkg/{ => vcs}/gitlab_client/pipeline.go | 0 pkg/{ => vcs}/gitlab_client/project.go | 0 pkg/{ => vcs}/gitlab_client/status.go | 0 21 files changed, 163 insertions(+), 196 deletions(-) create mode 100644 cmd/vcs.go delete mode 100644 pkg/github_client/client_test.go rename pkg/{ => vcs}/client.go (81%) rename pkg/{ => vcs}/github_client/client.go (86%) rename pkg/{ => vcs}/github_client/message.go (97%) rename pkg/{ => vcs}/gitlab_client/backoff.go (100%) rename pkg/{ => vcs}/gitlab_client/client.go (84%) rename pkg/{ => vcs}/gitlab_client/client_test.go (72%) rename pkg/{ => vcs}/gitlab_client/merge.go (100%) rename pkg/{ => vcs}/gitlab_client/message.go (96%) rename pkg/{ => vcs}/gitlab_client/pipeline.go (100%) rename pkg/{ => vcs}/gitlab_client/project.go (100%) rename pkg/{ => vcs}/gitlab_client/status.go (100%) diff --git a/cmd/controller_cmd.go b/cmd/controller_cmd.go index 4d6c78d7..886c9831 100644 --- a/cmd/controller_cmd.go +++ b/cmd/controller_cmd.go @@ -16,6 +16,7 @@ import ( "github.com/zapier/kubechecks/pkg" "github.com/zapier/kubechecks/pkg/config" "github.com/zapier/kubechecks/pkg/events" + "github.com/zapier/kubechecks/pkg/repo" "github.com/zapier/kubechecks/pkg/server" ) @@ -25,12 +26,25 @@ var ControllerCmd = &cobra.Command{ Short: "Start the VCS Webhook handler.", Long: ``, Run: func(cmd *cobra.Command, args []string) { - fmt.Println("Starting KubeChecks:", pkg.GitTag, pkg.GitCommit) + clientType := viper.GetString("vcs-type") + client, err := createVCSClient(clientType) + if err != nil { + log.Fatal().Err(err).Msg("failed to create vcs client") + } - server := server.NewServer(&config.ServerConfig{ + cfg := config.ServerConfig{ UrlPrefix: viper.GetString("webhook-url-prefix"), WebhookSecret: viper.GetString("webhook-secret"), - }) + VcsClient: client, + } + + log.Info().Msg("Initializing git settings") + if err := repo.InitializeGitSettings(cfg.VcsClient.Username(), cfg.VcsClient.Email()); err != nil { + log.Fatal().Err(err).Msg("failed to initialize git settings") + } + + fmt.Println("Starting KubeChecks:", pkg.GitTag, pkg.GitCommit) + server := server.NewServer(&cfg) ctx := context.Background() go server.Start(ctx) diff --git a/cmd/process.go b/cmd/process.go index e3efa4be..bb9336c6 100644 --- a/cmd/process.go +++ b/cmd/process.go @@ -5,6 +5,7 @@ import ( "github.com/rs/zerolog/log" "github.com/spf13/cobra" + "github.com/spf13/viper" "github.com/zapier/kubechecks/pkg/config" "github.com/zapier/kubechecks/pkg/server" @@ -23,21 +24,26 @@ var processCmd = &cobra.Command{ log.Fatal().Err(err).Msg("failed to build apps map") } + clientType := viper.GetString("vcs-type") + client, err := createVCSClient(clientType) + if err != nil { + log.Fatal().Err(err).Msg("failed to create vcs client") + } + cfg := config.ServerConfig{ UrlPrefix: "--unused--", WebhookSecret: "--unused--", VcsToArgoMap: result, + VcsClient: client, } - client, _ := server.GetVCSClient() - repo, err := client.LoadHook(ctx, args[0]) if err != nil { log.Fatal().Err(err).Msg("failed to load hook") return } - server.ProcessCheckEvent(ctx, repo, client, &cfg) + server.ProcessCheckEvent(ctx, repo, &cfg) }, } diff --git a/cmd/vcs.go b/cmd/vcs.go new file mode 100644 index 00000000..5cad3db7 --- /dev/null +++ b/cmd/vcs.go @@ -0,0 +1,20 @@ +package cmd + +import ( + "fmt" + + "github.com/zapier/kubechecks/pkg/vcs" + "github.com/zapier/kubechecks/pkg/vcs/github_client" + "github.com/zapier/kubechecks/pkg/vcs/gitlab_client" +) + +func createVCSClient(clientType string) (vcs.Client, error) { + switch clientType { + case "gitlab": + return gitlab_client.CreateGitlabClient() + case "github": + return github_client.CreateGithubClient() + default: + return nil, fmt.Errorf("unknown vcs type: %s", clientType) + } +} diff --git a/pkg/config/config.go b/pkg/config/config.go index c6edec28..ace5b34b 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -8,6 +8,8 @@ import ( "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/rs/zerolog/log" giturls "github.com/whilp/git-urls" + + "github.com/zapier/kubechecks/pkg/vcs" ) type repoURL struct { @@ -55,6 +57,7 @@ type ServerConfig struct { UrlPrefix string WebhookSecret string VcsToArgoMap VcsToArgoMap + VcsClient vcs.Client } func (cfg *ServerConfig) GetVcsRepos() []string { diff --git a/pkg/events/check.go b/pkg/events/check.go index de6ee72e..ce4495d8 100644 --- a/pkg/events/check.go +++ b/pkg/events/check.go @@ -28,11 +28,12 @@ import ( "github.com/zapier/kubechecks/pkg/repo" "github.com/zapier/kubechecks/pkg/repo_config" "github.com/zapier/kubechecks/pkg/validate" + "github.com/zapier/kubechecks/pkg/vcs" "github.com/zapier/kubechecks/telemetry" ) type CheckEvent struct { - client pkg.Client // Client exposing methods to communicate with platform of user choice + client vcs.Client // Client exposing methods to communicate with platform of user choice fileList []string // What files have changed in this PR/MR TempWorkingDir string // Location of the local repo repo *repo.Repo @@ -51,10 +52,10 @@ type CheckEvent struct { var inFlight int32 -func NewCheckEvent(repo *repo.Repo, client pkg.Client, cfg *config.ServerConfig) *CheckEvent { +func NewCheckEvent(repo *repo.Repo, cfg *config.ServerConfig) *CheckEvent { ce := &CheckEvent{ cfg: cfg, - client: client, + client: cfg.VcsClient, repo: repo, } @@ -97,14 +98,6 @@ func (ce *CheckEvent) Cleanup(ctx context.Context) { } } -// InitializeGit sets the username and email for a git repo -func (ce *CheckEvent) InitializeGit(ctx context.Context) error { - _, span := otel.Tracer("Kubechecks").Start(ctx, "InitializeGit") - defer span.End() - - return ce.repo.InitializeGitSettings() -} - // CloneRepoLocal takes the repo inside the Check Event and try to clone it locally func (ce *CheckEvent) CloneRepoLocal(ctx context.Context) error { _, span := otel.Tracer("Kubechecks").Start(ctx, "CloneRepoLocal") @@ -242,7 +235,8 @@ func (ce *CheckEvent) ProcessApps(ctx context.Context) { } ce.logger.Info().Msg("Finished") - if err = ce.vcsNote.PushComment(ctx, ce.client); err != nil { + comment := ce.vcsNote.BuildComment(ctx) + if err = ce.client.UpdateMessage(ctx, ce.vcsNote, comment); err != nil { ce.logger.Error().Err(err).Msg("failed to push comment") } diff --git a/pkg/github_client/client_test.go b/pkg/github_client/client_test.go deleted file mode 100644 index 7513ddca..00000000 --- a/pkg/github_client/client_test.go +++ /dev/null @@ -1,15 +0,0 @@ -package github_client - -import ( - "fmt" - "testing" - - "github.com/spf13/viper" - "github.com/stretchr/testify/assert" -) - -func TestCreateClient(t *testing.T) { - viper.Set("vcs-token", "pass") - githubClient := createGithubClient() - assert.Equal(t, "https://api.github.com/", githubClient.BaseURL.String(), fmt.Sprintf("api URL in githubClient (%s) does not match github public API", githubClient.BaseURL.String())) -} diff --git a/pkg/message.go b/pkg/message.go index e047c8f2..08b28f94 100644 --- a/pkg/message.go +++ b/pkg/message.go @@ -94,8 +94,8 @@ func (m *Message) SetFooter(start time.Time, commitSha string) { m.footer = buildFooter(start, commitSha) } -func (m *Message) PushComment(ctx context.Context, client Client) error { - return client.UpdateMessage(ctx, m, buildComment(ctx, m.apps)) +func (m *Message) BuildComment(ctx context.Context) string { + return buildComment(ctx, m.apps) } func buildFooter(start time.Time, commitSHA string) string { diff --git a/pkg/repo/repo.go b/pkg/repo/repo.go index bd9f362a..3e46e060 100644 --- a/pkg/repo/repo.go +++ b/pkg/repo/repo.go @@ -206,27 +206,32 @@ func walk(s string, d fs.DirEntry, err error) error { } func (r *Repo) execCommand(name string, args ...string) *exec.Cmd { + cmd := execCommand(name, args...) + cmd.Dir = r.RepoDir + return cmd +} + +func execCommand(name string, args ...string) *exec.Cmd { log.Debug().Strs("args", args).Msg("building command") cmd := exec.Command(name, args...) - cmd.Dir = r.RepoDir return cmd } // InitializeGitSettings ensures Git auth is set up for cloning -func (r *Repo) InitializeGitSettings() error { - cmd := r.execCommand("git", "config", "user.email", r.Email) +func InitializeGitSettings(username, email string) error { + cmd := execCommand("git", "config", "--global", "user.email", email) err := cmd.Run() if err != nil { return errors.Wrap(err, "failed to set git email address") } - cmd = r.execCommand("git", "config", "user.name", r.Username) + cmd = execCommand("git", "config", "--global", "user.name", username) err = cmd.Run() if err != nil { return errors.Wrap(err, "failed to set git user name") } - cloneUrl, err := getCloneUrl(r.Username, viper.GetViper()) + cloneUrl, err := getCloneUrl(username, viper.GetViper()) if err != nil { return errors.Wrap(err, "failed to get clone url") } @@ -243,14 +248,14 @@ func (r *Repo) InitializeGitSettings() error { } defer outfile.Close() - cmd = r.execCommand("echo", cloneUrl) + cmd = execCommand("echo", cloneUrl) cmd.Stdout = outfile err = cmd.Run() if err != nil { return errors.Wrap(err, "unable to set git credentials") } - cmd = r.execCommand("git", "config", "credential.helper", "store") + cmd = execCommand("git", "config", "credential.helper", "store") err = cmd.Run() if err != nil { return errors.Wrap(err, "unable to set git credential usage") diff --git a/pkg/server/hook_handler.go b/pkg/server/hook_handler.go index f7373459..70a90c8f 100644 --- a/pkg/server/hook_handler.go +++ b/pkg/server/hook_handler.go @@ -2,10 +2,8 @@ package server import ( "context" - "fmt" "net/http" "strings" - "sync" "github.com/labstack/echo/v4" "github.com/rs/zerolog/log" @@ -14,60 +12,27 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" - "github.com/zapier/kubechecks/pkg" "github.com/zapier/kubechecks/pkg/config" "github.com/zapier/kubechecks/pkg/events" - "github.com/zapier/kubechecks/pkg/github_client" - "github.com/zapier/kubechecks/pkg/gitlab_client" "github.com/zapier/kubechecks/pkg/repo" + "github.com/zapier/kubechecks/pkg/vcs" "github.com/zapier/kubechecks/telemetry" ) type VCSHookHandler struct { - client pkg.Client - tokenUser string - cfg *config.ServerConfig + client vcs.Client + cfg *config.ServerConfig // labelFilter is a string specifying the required label name to filter merge events by; if empty, all merge events will pass the filter. labelFilter string } -var once sync.Once -var vcsClient pkg.Client // Currently, only allow one client at a time -var tokenUser string -var ProjectHookPath = "/gitlab/project" - -// High level type representing the fields we care about from an arbitrary Git repository -func GetVCSClient() (pkg.Client, string) { - once.Do(func() { - vcsClient, tokenUser = createVCSClient() - }) - return vcsClient, tokenUser -} - -func createVCSClient() (pkg.Client, string) { - // Determine what client to use based on set config (default Gitlab) - clientType := viper.GetString("vcs-type") - // All hooks set up follow the convention /VCS_PROVIDER/project - ProjectHookPath = fmt.Sprintf("/%s/project", clientType) - switch clientType { - case "gitlab": - return gitlab_client.GetGitlabClient() - case "github": - return github_client.GetGithubClient() - default: - log.Fatal().Msgf("Unknown VCS type: %s", clientType) - return nil, "" - } - -} +var ProjectHookPath string func NewVCSHookHandler(cfg *config.ServerConfig) *VCSHookHandler { - client, tokenUser := GetVCSClient() labelFilter := viper.GetString("label-filter") return &VCSHookHandler{ - client: client, - tokenUser: tokenUser, + client: cfg.VcsClient, cfg: cfg, labelFilter: labelFilter, } @@ -88,10 +53,10 @@ func (h *VCSHookHandler) groupHandler(c echo.Context) error { return c.String(http.StatusUnauthorized, "Unauthorized") } - repo, err := h.client.ParseHook(c.Request(), payload) + r, err := h.client.ParseHook(c.Request(), payload) if err != nil { switch err { - case pkg.ErrInvalidType: + case vcs.ErrInvalidType: log.Debug().Msg("Ignoring event, not a merge request") return c.String(http.StatusOK, "Skipped") default: @@ -102,7 +67,7 @@ func (h *VCSHookHandler) groupHandler(c echo.Context) error { } // We now have a generic repo with all the info we need to start processing an event. Hand off to the event processor - go h.processCheckEvent(ctx, repo) + go h.processCheckEvent(ctx, r) return c.String(http.StatusAccepted, "Accepted") } @@ -114,25 +79,25 @@ func (h *VCSHookHandler) processCheckEvent(ctx context.Context, repo *repo.Repo) return } - ProcessCheckEvent(ctx, repo, h.client, h.cfg) + ProcessCheckEvent(ctx, repo, h.cfg) } -func ProcessCheckEvent(ctx context.Context, repo *repo.Repo, client pkg.Client, cfg *config.ServerConfig) { +func ProcessCheckEvent(ctx context.Context, r *repo.Repo, cfg *config.ServerConfig) { var span trace.Span ctx, span = otel.Tracer("Kubechecks").Start(ctx, "processCheckEvent", trace.WithAttributes( - attribute.Int("mr_id", repo.CheckID), - attribute.String("project", repo.Name), - attribute.String("sha", repo.SHA), - attribute.String("source", repo.HeadRef), - attribute.String("target", repo.BaseRef), - attribute.String("default_branch", repo.DefaultBranch), + attribute.Int("mr_id", r.CheckID), + attribute.String("project", r.Name), + attribute.String("sha", r.SHA), + attribute.String("source", r.HeadRef), + attribute.String("target", r.BaseRef), + attribute.String("default_branch", r.DefaultBranch), ), ) defer span.End() // If we've gotten here, we can now begin running checks (or trying to) - cEvent := events.NewCheckEvent(repo, client, cfg) + cEvent := events.NewCheckEvent(r, cfg) err := cEvent.CreateTempDir() if err != nil { @@ -141,6 +106,13 @@ func ProcessCheckEvent(ctx context.Context, repo *repo.Repo, client pkg.Client, } defer cEvent.Cleanup(ctx) + err = repo.InitializeGitSettings(cfg.VcsClient.Username(), cfg.VcsClient.Email()) + if err != nil { + telemetry.SetError(span, err, "Initialize Git") + log.Error().Err(err).Msg("unable to initialize git") + return + } + // Clone the repo's BaseRef (main etc) locally into the temp dir we just made err = cEvent.CloneRepoLocal(ctx) if err != nil { @@ -150,13 +122,6 @@ func ProcessCheckEvent(ctx context.Context, repo *repo.Repo, client pkg.Client, return } - err = cEvent.InitializeGit(ctx) - if err != nil { - telemetry.SetError(span, err, "Initialize Git") - log.Error().Err(err).Msg("unable to initialize git") - return - } - // Merge the most recent changes into the branch we just cloned err = cEvent.MergeIntoTarget(ctx) if err != nil { @@ -174,7 +139,7 @@ func ProcessCheckEvent(ctx context.Context, repo *repo.Repo, client pkg.Client, } // Generate a list of affected apps, storing them within the CheckEvent (also returns but discarded here) - err = cEvent.GenerateListOfAffectedApps(ctx, repo.BaseRef) + err = cEvent.GenerateListOfAffectedApps(ctx, r.BaseRef) if err != nil { // TODO: Cancel if gitlab etc //mEvent.CancelEvent(ctx, err, "Generate List of Affected Apps") diff --git a/pkg/server/server.go b/pkg/server/server.go index 31223f5e..9312d736 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -14,8 +14,8 @@ import ( "github.com/spf13/viper" "github.com/ziflex/lecho/v3" - "github.com/zapier/kubechecks/pkg" "github.com/zapier/kubechecks/pkg/config" + "github.com/zapier/kubechecks/pkg/vcs" ) const KubeChecksHooksPathPrefix = "/hooks" @@ -96,7 +96,7 @@ func (s *Server) ensureWebhooks() error { log.Info().Msg("ensuring all webhooks are created correctly") ctx := context.TODO() - vcsClient, _ := GetVCSClient() + vcsClient := s.cfg.VcsClient fullUrl, err := url.JoinPath(urlBase, s.hooksPrefix(), vcsClient.GetName(), "project") if err != nil { @@ -107,7 +107,7 @@ func (s *Server) ensureWebhooks() error { for _, repo := range s.cfg.GetVcsRepos() { wh, err := vcsClient.GetHookByUrl(ctx, repo, fullUrl) - if err != nil && !errors.Is(err, pkg.ErrHookNotFound) { + if err != nil && !errors.Is(err, vcs.ErrHookNotFound) { log.Error().Err(err).Msgf("failed to get hook for %s:", repo) continue } diff --git a/pkg/client.go b/pkg/vcs/client.go similarity index 81% rename from pkg/client.go rename to pkg/vcs/client.go index d315e1b6..b5881fa1 100644 --- a/pkg/client.go +++ b/pkg/vcs/client.go @@ -1,10 +1,11 @@ -package pkg +package vcs import ( "context" "errors" "net/http" + "github.com/zapier/kubechecks/pkg" "github.com/zapier/kubechecks/pkg/repo" ) @@ -23,23 +24,26 @@ 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, *repo.Repo, int, string) *Message + PostMessage(context.Context, *repo.Repo, int, string) *pkg.Message // UpdateMessage update a message with new content - UpdateMessage(context.Context, *Message, string) error + UpdateMessage(context.Context, *pkg.Message, string) error // VerifyHook validates a webhook secret and return the body; must be called even if no secret VerifyHook(*http.Request, string) ([]byte, error) // ParseHook parses webook payload for valid events ParseHook(*http.Request, []byte) (*repo.Repo, error) // CommitStatus sets a status for a specific commit on the remote VCS - CommitStatus(context.Context, *repo.Repo, CommitState) error + CommitStatus(context.Context, *repo.Repo, pkg.CommitState) error // GetHookByUrl gets a webhook by url GetHookByUrl(ctx context.Context, repoName, webhookUrl string) (*WebHookConfig, error) // CreateHook creates a webhook that points at kubechecks CreateHook(ctx context.Context, repoName, webhookUrl, webhookSecret string) error // GetName returns the VCS client name (e.g. "github" or "gitlab") GetName() string - // Tidy outdated comments either by hiding or deleting them + // TidyOutdatedComments either by hiding or deleting them TidyOutdatedComments(context.Context, *repo.Repo) error // LoadHook creates an EventRequest from the ID of an actual request LoadHook(ctx context.Context, repoAndId string) (*repo.Repo, error) + + Username() string + Email() string } diff --git a/pkg/github_client/client.go b/pkg/vcs/github_client/client.go similarity index 86% rename from pkg/github_client/client.go rename to pkg/vcs/github_client/client.go index d74fe790..fda0fb8b 100644 --- a/pkg/github_client/client.go +++ b/pkg/vcs/github_client/client.go @@ -7,7 +7,6 @@ import ( "regexp" "strconv" "strings" - "sync" "github.com/google/go-github/v53/github" "github.com/pkg/errors" @@ -18,41 +17,21 @@ import ( "github.com/zapier/kubechecks/pkg" "github.com/zapier/kubechecks/pkg/repo" + "github.com/zapier/kubechecks/pkg/vcs" ) -var githubClient *Client -var githubTokenUser string -var once sync.Once // used to ensure we don't reauth this - type Client struct { - v4Client *githubv4.Client - *github.Client -} + v4Client *githubv4.Client + username, email string -var _ pkg.Client = new(Client) - -func GetGithubClient() (*Client, string) { - once.Do(func() { - githubClient = createGithubClient() - githubTokenUser = getTokenUser() - }) - return githubClient, githubTokenUser + *github.Client } -// We require a username to use with git locally, so get the current auth'd user -func getTokenUser() string { - user, _, err := githubClient.Users.Get(context.Background(), "") - if err != nil { - if err != nil { - log.Fatal().Err(err).Msg("could not get Github user") - } - } - return *user.Login -} +var _ vcs.Client = new(Client) -// Create a new GitHub client using the auth token provided. We +// CreateGithubClient creates a new GitHub client using the auth token provided. We // can't validate the token at this point, so if it exists we assume it works -func createGithubClient() *Client { +func CreateGithubClient() (*Client, error) { var ( err error googleClient *github.Client @@ -85,9 +64,21 @@ func createGithubClient() *Client { shurcoolClient = githubv4.NewEnterpriseClient(githubUrl, tc) } - return &Client{Client: googleClient, v4Client: shurcoolClient} + user, _, err := googleClient.Users.Get(ctx, "") + if err != nil { + return nil, errors.Wrap(err, "failed to get user") + } + + return &Client{ + Client: googleClient, + v4Client: shurcoolClient, + username: *user.Login, + email: *user.Email, + }, nil } +func (c *Client) Username() string { return c.username } +func (c *Client) Email() string { return c.email } func (c *Client) GetName() string { return "github" } @@ -114,14 +105,14 @@ func (c *Client) ParseHook(r *http.Request, request []byte) (*repo.Repo, error) switch p.GetAction() { case "opened", "synchronize", "reopened", "edited": log.Info().Str("action", p.GetAction()).Msg("handling Github event from PR") - return buildRepoFromEvent(p), nil + return c.buildRepoFromEvent(p), nil default: log.Info().Str("action", p.GetAction()).Msg("ignoring Github pull request event due to non commit based action") - return nil, pkg.ErrInvalidType + return nil, vcs.ErrInvalidType } default: log.Error().Msg("invalid event provided to Github client") - return nil, pkg.ErrInvalidType + return nil, vcs.ErrInvalidType } } @@ -143,14 +134,7 @@ func (c *Client) getUserDetails() (string, string, error) { } -func buildRepoFromEvent(event *github.PullRequestEvent) *repo.Repo { - username, email, err := githubClient.getUserDetails() - if err != nil { - log.Fatal().Err(err).Msg("could not load Github user details") - username = "" - email = "" - } - +func (c *Client) buildRepoFromEvent(event *github.PullRequestEvent) *repo.Repo { var labels []string for _, label := range event.PullRequest.Labels { labels = append(labels, label.GetName()) @@ -166,8 +150,8 @@ func buildRepoFromEvent(event *github.PullRequestEvent) *repo.Repo { Name: event.Repo.GetName(), CheckID: *event.PullRequest.Number, SHA: *event.PullRequest.Head.SHA, - Username: username, - Email: email, + Username: c.username, + Email: c.email, Labels: labels, } } @@ -218,7 +202,7 @@ func parseRepo(cloneUrl string) (string, string) { panic(cloneUrl) } -func (c *Client) GetHookByUrl(ctx context.Context, ownerAndRepoName, webhookUrl string) (*pkg.WebHookConfig, error) { +func (c *Client) GetHookByUrl(ctx context.Context, ownerAndRepoName, webhookUrl string) (*vcs.WebHookConfig, error) { owner, repoName := parseRepo(ownerAndRepoName) items, _, err := c.Repositories.ListHooks(ctx, owner, repoName, nil) if err != nil { @@ -227,14 +211,14 @@ func (c *Client) GetHookByUrl(ctx context.Context, ownerAndRepoName, webhookUrl for _, item := range items { if item.URL != nil && *item.URL == webhookUrl { - return &pkg.WebHookConfig{ + return &vcs.WebHookConfig{ Url: item.GetURL(), Events: item.Events, // TODO: translate GH specific event names to VCS agnostic }, nil } } - return nil, pkg.ErrHookNotFound + return nil, vcs.ErrHookNotFound } func (c *Client) CreateHook(ctx context.Context, ownerAndRepoName, webhookUrl, webhookSecret string) error { diff --git a/pkg/github_client/message.go b/pkg/vcs/github_client/message.go similarity index 97% rename from pkg/github_client/message.go rename to pkg/vcs/github_client/message.go index 5f25346c..af2366d4 100644 --- a/pkg/github_client/message.go +++ b/pkg/vcs/github_client/message.go @@ -86,7 +86,7 @@ func (c *Client) pruneOldComments(ctx context.Context, repo *repo.Repo, comments log.Debug().Msgf("Pruning messages from PR %d in repo %s", repo.CheckID, repo.FullName) for _, comment := range comments { - if strings.EqualFold(comment.GetUser().GetLogin(), githubTokenUser) { + if strings.EqualFold(comment.GetUser().GetLogin(), c.username) { _, err := c.Issues.DeleteComment(ctx, repo.Owner, repo.Name, *comment.ID) if err != nil { return fmt.Errorf("failed to delete comment: %w", err) @@ -104,7 +104,7 @@ func (c *Client) hideOutdatedMessages(ctx context.Context, repo *repo.Repo, comm log.Debug().Msgf("Hiding kubecheck messages in PR %d in repo %s", repo.CheckID, repo.FullName) for _, comment := range comments { - if strings.EqualFold(comment.GetUser().GetLogin(), githubTokenUser) { + if strings.EqualFold(comment.GetUser().GetLogin(), c.username) { // Github API does not expose minimizeComment API. IT's only available from the GraphQL API // https://docs.github.com/en/graphql/reference/mutations#minimizecomment var m struct { diff --git a/pkg/gitlab_client/backoff.go b/pkg/vcs/gitlab_client/backoff.go similarity index 100% rename from pkg/gitlab_client/backoff.go rename to pkg/vcs/gitlab_client/backoff.go diff --git a/pkg/gitlab_client/client.go b/pkg/vcs/gitlab_client/client.go similarity index 84% rename from pkg/gitlab_client/client.go rename to pkg/vcs/gitlab_client/client.go index f6c0719f..8bb07b57 100644 --- a/pkg/gitlab_client/client.go +++ b/pkg/vcs/gitlab_client/client.go @@ -6,7 +6,6 @@ import ( "io" "net/http" "strings" - "sync" "github.com/pkg/errors" "github.com/rs/zerolog/log" @@ -16,31 +15,20 @@ import ( "github.com/zapier/kubechecks/pkg" "github.com/zapier/kubechecks/pkg/repo" + "github.com/zapier/kubechecks/pkg/vcs" ) -var gitlabClient *Client -var gitlabTokenUser string -var gitlabTokenEmail string -var once sync.Once - const GitlabTokenHeader = "X-Gitlab-Token" type Client struct { *gitlab.Client -} - -var _ pkg.Client = new(Client) - -func GetGitlabClient() (*Client, string) { - once.Do(func() { - gitlabClient = createGitlabClient() - gitlabTokenUser, gitlabTokenEmail = gitlabClient.getTokenUser() - }) - return gitlabClient, gitlabTokenUser + username, email string } -func createGitlabClient() *Client { +var _ vcs.Client = new(Client) + +func CreateGitlabClient() (*Client, error) { // Initialize the GitLab client with access token gitlabToken := viper.GetString("vcs-token") if gitlabToken == "" { @@ -60,7 +48,12 @@ func createGitlabClient() *Client { log.Fatal().Err(err).Msg("could not create Gitlab client") } - return &Client{c} + user, _, err := c.Users.CurrentUser() + if err != nil { + return nil, errors.Wrap(err, "failed to get current user") + } + + return &Client{Client: c, username: user.Username, email: user.Email}, nil } func (c *Client) getTokenUser() (string, string) { @@ -72,6 +65,8 @@ func (c *Client) getTokenUser() (string, string) { return user.Username, user.Email } +func (c *Client) Email() string { return c.email } +func (c *Client) Username() string { return c.username } func (c *Client) GetName() string { return "gitlab" } @@ -100,20 +95,20 @@ func (c *Client) ParseHook(r *http.Request, request []byte) (*repo.Repo, error) switch event.ObjectAttributes.Action { case "update": if event.ObjectAttributes.OldRev != "" && event.ObjectAttributes.OldRev != event.ObjectAttributes.LastCommit.ID { - return buildRepoFromEvent(event), nil + return c.buildRepoFromEvent(event), nil } log.Trace().Msgf("Skipping update event sha didn't change") case "open", "reopen": - return buildRepoFromEvent(event), nil + return c.buildRepoFromEvent(event), nil default: log.Trace().Msgf("Unhandled Action %s", event.ObjectAttributes.Action) - return nil, pkg.ErrInvalidType + return nil, vcs.ErrInvalidType } default: log.Trace().Msgf("Unhandled Event: %T", event) - return nil, pkg.ErrInvalidType + return nil, vcs.ErrInvalidType } - return nil, pkg.ErrInvalidType + return nil, vcs.ErrInvalidType } func parseRepoName(url string) (string, error) { @@ -128,7 +123,7 @@ func parseRepoName(url string) (string, error) { return path, nil } -func (c *Client) GetHookByUrl(ctx context.Context, repoName, webhookUrl string) (*pkg.WebHookConfig, error) { +func (c *Client) GetHookByUrl(ctx context.Context, repoName, webhookUrl string) (*vcs.WebHookConfig, error) { pid, err := parseRepoName(repoName) if err != nil { return nil, errors.Wrap(err, "failed to parse repo url") @@ -145,14 +140,14 @@ func (c *Client) GetHookByUrl(ctx context.Context, repoName, webhookUrl string) if hook.MergeRequestsEvents { events = append(events, string(gitlab.MergeRequestEventTargetType)) } - return &pkg.WebHookConfig{ + return &vcs.WebHookConfig{ Url: hook.URL, Events: events, }, nil } } - return nil, pkg.ErrHookNotFound + return nil, vcs.ErrHookNotFound } func (c *Client) CreateHook(ctx context.Context, repoName, webhookUrl, webhookSecret string) error { @@ -179,7 +174,7 @@ func (c *Client) LoadHook(ctx context.Context, id string) (*repo.Repo, error) { panic("implement me") } -func buildRepoFromEvent(event *gitlab.MergeEvent) *repo.Repo { +func (c *Client) buildRepoFromEvent(event *gitlab.MergeEvent) *repo.Repo { // Convert all labels from this MR to a string array of label names var labels []string for _, label := range event.Labels { @@ -195,8 +190,8 @@ func buildRepoFromEvent(event *gitlab.MergeEvent) *repo.Repo { Name: event.Project.Name, CheckID: event.ObjectAttributes.IID, SHA: event.ObjectAttributes.LastCommit.ID, - Username: gitlabTokenUser, - Email: gitlabTokenEmail, + Username: c.username, + Email: c.email, Labels: labels, } } diff --git a/pkg/gitlab_client/client_test.go b/pkg/vcs/gitlab_client/client_test.go similarity index 72% rename from pkg/gitlab_client/client_test.go rename to pkg/vcs/gitlab_client/client_test.go index f7013c48..c86c60ce 100644 --- a/pkg/gitlab_client/client_test.go +++ b/pkg/vcs/gitlab_client/client_test.go @@ -1,20 +1,12 @@ package gitlab_client import ( - "fmt" "testing" - "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestCreateClient(t *testing.T) { - viper.Set("vcs-token", "pass") - gitlabClient := createGitlabClient() - assert.Equal(t, "https://gitlab.com/api/v4/", gitlabClient.BaseURL().String(), fmt.Sprintf("api URL in githubClient (%s) does not match github public API", gitlabClient.BaseURL().String())) -} - func TestCustomGitURLParsing(t *testing.T) { testcases := []struct { giturl, expected string diff --git a/pkg/gitlab_client/merge.go b/pkg/vcs/gitlab_client/merge.go similarity index 100% rename from pkg/gitlab_client/merge.go rename to pkg/vcs/gitlab_client/merge.go diff --git a/pkg/gitlab_client/message.go b/pkg/vcs/gitlab_client/message.go similarity index 96% rename from pkg/gitlab_client/message.go rename to pkg/vcs/gitlab_client/message.go index 3519d1e4..3e5a2e14 100644 --- a/pkg/gitlab_client/message.go +++ b/pkg/vcs/gitlab_client/message.go @@ -52,7 +52,7 @@ func (c *Client) hideOutdatedMessages(ctx context.Context, projectName string, m // note user is not the gitlabTokenUser // note is an internal system note such as notes on commit messages // note is already hidden - if note.Author.Username != gitlabTokenUser || note.System || strings.Contains(note.Body, "OUTDATED: Kubechecks Report") { + if note.Author.Username != c.username || note.System || strings.Contains(note.Body, "OUTDATED: Kubechecks Report") { continue } @@ -114,7 +114,7 @@ func (c *Client) pruneOldComments(ctx context.Context, projectName string, mrID log.Debug().Msg("deleting outdated comments") for _, note := range notes { - if note.Author.Username == gitlabTokenUser { + if note.Author.Username == c.username { log.Debug().Int("mr", mrID).Int("note", note.ID).Msg("deleting old comment") _, err := c.Notes.DeleteMergeRequestNote(projectName, mrID, note.ID) if err != nil { diff --git a/pkg/gitlab_client/pipeline.go b/pkg/vcs/gitlab_client/pipeline.go similarity index 100% rename from pkg/gitlab_client/pipeline.go rename to pkg/vcs/gitlab_client/pipeline.go diff --git a/pkg/gitlab_client/project.go b/pkg/vcs/gitlab_client/project.go similarity index 100% rename from pkg/gitlab_client/project.go rename to pkg/vcs/gitlab_client/project.go diff --git a/pkg/gitlab_client/status.go b/pkg/vcs/gitlab_client/status.go similarity index 100% rename from pkg/gitlab_client/status.go rename to pkg/vcs/gitlab_client/status.go From a1cd9f69bdd01d8e99d6fcd75df18f9e650d7b45 Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Wed, 20 Dec 2023 17:01:28 -0500 Subject: [PATCH 19/24] this needs to be global --- pkg/repo/repo.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/repo/repo.go b/pkg/repo/repo.go index 3e46e060..e431eb85 100644 --- a/pkg/repo/repo.go +++ b/pkg/repo/repo.go @@ -255,7 +255,7 @@ func InitializeGitSettings(username, email string) error { return errors.Wrap(err, "unable to set git credentials") } - cmd = execCommand("git", "config", "credential.helper", "store") + cmd = execCommand("git", "config", "--global", "credential.helper", "store") err = cmd.Run() if err != nil { return errors.Wrap(err, "unable to set git credential usage") From 87e4a3ecbd0dd46930bb82435a77294f9538285f Mon Sep 17 00:00:00 2001 From: djeebus Date: Wed, 20 Dec 2023 19:47:22 -0500 Subject: [PATCH 20/24] render out methods for debugging purposes --- pkg/server/hook_handler.go | 9 ++++----- pkg/server/server.go | 15 +++++++-------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/pkg/server/hook_handler.go b/pkg/server/hook_handler.go index 70a90c8f..adeed6bf 100644 --- a/pkg/server/hook_handler.go +++ b/pkg/server/hook_handler.go @@ -2,6 +2,7 @@ package server import ( "context" + "fmt" "net/http" "strings" @@ -24,10 +25,9 @@ type VCSHookHandler struct { cfg *config.ServerConfig // labelFilter is a string specifying the required label name to filter merge events by; if empty, all merge events will pass the filter. labelFilter string + hooksPrefix string } -var ProjectHookPath string - func NewVCSHookHandler(cfg *config.ServerConfig) *VCSHookHandler { labelFilter := viper.GetString("label-filter") @@ -38,9 +38,8 @@ func NewVCSHookHandler(cfg *config.ServerConfig) *VCSHookHandler { } } func (h *VCSHookHandler) AttachHandlers(grp *echo.Group) { - log.Info().Str("path", GetServer().hooksPrefix()).Msg("setting up hook handler") - grp.POST(ProjectHookPath, h.groupHandler) - log.Info().Str("path", GetServer().hooksPrefix()).Str("projectPath", ProjectHookPath).Msg("hook handler setup complete") + projectHookPath := fmt.Sprintf("/%s/project", h.cfg.VcsClient.GetName()) + grp.POST(projectHookPath, h.groupHandler) } func (h *VCSHookHandler) groupHandler(c echo.Context) error { diff --git a/pkg/server/server.go b/pkg/server/server.go index 9312d736..fd2abe11 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -2,6 +2,7 @@ package server import ( "context" + "fmt" "net/url" "strings" @@ -20,19 +21,12 @@ import ( const KubeChecksHooksPathPrefix = "/hooks" -var singleton *Server - type Server struct { cfg *config.ServerConfig } func NewServer(cfg *config.ServerConfig) *Server { - singleton = &Server{cfg: cfg} - return singleton -} - -func GetServer() *Server { - return singleton + return &Server{cfg: cfg} } func (s *Server) Start(ctx context.Context) { @@ -64,6 +58,11 @@ func (s *Server) Start(ctx context.Context) { ghHooks := NewVCSHookHandler(s.cfg) ghHooks.AttachHandlers(hooksGroup) + fmt.Println("Method\tPath") + for _, r := range e.Routes() { + fmt.Printf("%s\t%s\n", r.Method, r.Path) + } + if err := e.Start(":8080"); err != nil { log.Fatal().Err(err).Msg("could not start hooks server") } From 7fa4f01a9d10a0cd1c26567fb79acb16c9edfcc7 Mon Sep 17 00:00:00 2001 From: djeebus Date: Wed, 20 Dec 2023 19:49:55 -0500 Subject: [PATCH 21/24] remove dead and confusing code --- pkg/diff/diff.go | 6 +----- pkg/vcs/github_client/client.go | 18 ------------------ pkg/vcs/gitlab_client/client.go | 9 --------- 3 files changed, 1 insertion(+), 32 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index c2bfabce..efc198de 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -38,11 +38,7 @@ type objKeyLiveTarget struct { } func isAppMissingErr(err error) bool { - if strings.Contains(err.Error(), "PermissionDenied") { - return true - } - - return false + return strings.Contains(err.Error(), "PermissionDenied") } /* diff --git a/pkg/vcs/github_client/client.go b/pkg/vcs/github_client/client.go index fda0fb8b..09b14f99 100644 --- a/pkg/vcs/github_client/client.go +++ b/pkg/vcs/github_client/client.go @@ -116,24 +116,6 @@ func (c *Client) ParseHook(r *http.Request, request []byte) (*repo.Repo, error) } } -// We need an email and username for authenticating our local git repository -// Grab the current authenticated client login and email -func (c *Client) getUserDetails() (string, string, error) { - user, _, err := c.Users.Get(context.Background(), "") - if err != nil { - return "", "", err - } - - // Some users on GitHub don't have an email listed; if so, catch that and return empty string - if user.Email == nil { - log.Error().Msg("could not load Github user email") - return *user.Login, "", nil - } - - return *user.Login, *user.Email, nil - -} - func (c *Client) buildRepoFromEvent(event *github.PullRequestEvent) *repo.Repo { var labels []string for _, label := range event.PullRequest.Labels { diff --git a/pkg/vcs/gitlab_client/client.go b/pkg/vcs/gitlab_client/client.go index 8bb07b57..27b3bbd5 100644 --- a/pkg/vcs/gitlab_client/client.go +++ b/pkg/vcs/gitlab_client/client.go @@ -56,15 +56,6 @@ func CreateGitlabClient() (*Client, error) { return &Client{Client: c, username: user.Username, email: user.Email}, nil } -func (c *Client) getTokenUser() (string, string) { - user, _, err := c.Users.CurrentUser() - if err != nil { - log.Fatal().Err(err).Msg("could not create Gitlab token user") - } - - return user.Username, user.Email -} - func (c *Client) Email() string { return c.email } func (c *Client) Username() string { return c.username } func (c *Client) GetName() string { From b919e1edd7395037e5dc625a8aba522e10e02364 Mon Sep 17 00:00:00 2001 From: djeebus Date: Wed, 20 Dec 2023 19:59:01 -0500 Subject: [PATCH 22/24] remove dead field --- pkg/server/hook_handler.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/server/hook_handler.go b/pkg/server/hook_handler.go index adeed6bf..67fa6614 100644 --- a/pkg/server/hook_handler.go +++ b/pkg/server/hook_handler.go @@ -25,7 +25,6 @@ type VCSHookHandler struct { cfg *config.ServerConfig // labelFilter is a string specifying the required label name to filter merge events by; if empty, all merge events will pass the filter. labelFilter string - hooksPrefix string } func NewVCSHookHandler(cfg *config.ServerConfig) *VCSHookHandler { From 2610b8470e9003b2524b4eef6bf6ba3231d36093 Mon Sep 17 00:00:00 2001 From: "joe.lombrozo" Date: Thu, 21 Dec 2023 11:07:59 -0500 Subject: [PATCH 23/24] rename some variables, implement gitlab mr processing --- pkg/affected_apps/config_matcher.go | 16 +++++------ pkg/vcs/gitlab_client/client.go | 43 +++++++++++++++++++++++++++-- 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/pkg/affected_apps/config_matcher.go b/pkg/affected_apps/config_matcher.go index 73f9aafc..b0f0091e 100644 --- a/pkg/affected_apps/config_matcher.go +++ b/pkg/affected_apps/config_matcher.go @@ -25,7 +25,7 @@ func NewConfigMatcher(cfg *repo_config.Config) *ConfigMatcher { } func (b *ConfigMatcher) AffectedApps(ctx context.Context, changeList []string, targetBranch string) (AffectedItems, error) { - appsMap := make(map[string]string) + triggeredAppsMap := make(map[string]string) var appSetList []ApplicationSet triggeredApps, triggeredAppsets, err := b.triggeredApps(ctx, changeList) @@ -34,28 +34,28 @@ func (b *ConfigMatcher) AffectedApps(ctx context.Context, changeList []string, t } for _, app := range triggeredApps { - appsMap[app.Name] = app.Path + triggeredAppsMap[app.Name] = app.Path } for _, appset := range triggeredAppsets { appSetList = append(appSetList, ApplicationSet{appset.Name}) } - argoApps, err := b.argoClient.GetApplications(ctx) + allArgoApps, err := b.argoClient.GetApplications(ctx) if err != nil { return AffectedItems{}, errors.Wrap(err, "failed to list applications") } - var appsSlice []v1alpha1.Application - for _, app := range argoApps.Items { - if _, ok := appsMap[app.Name]; !ok { + var triggeredAppsSlice []v1alpha1.Application + for _, app := range allArgoApps.Items { + if _, ok := triggeredAppsMap[app.Name]; !ok { continue } - appsSlice = append(appsSlice, app) + triggeredAppsSlice = append(triggeredAppsSlice, app) } - return AffectedItems{Applications: appsSlice, ApplicationSets: appSetList}, nil + return AffectedItems{Applications: triggeredAppsSlice, ApplicationSets: appSetList}, nil } func (b *ConfigMatcher) triggeredApps(ctx context.Context, modifiedFiles []string) ([]*repo_config.ArgoCdApplicationConfig, []*repo_config.ArgocdApplicationSetConfig, error) { diff --git a/pkg/vcs/gitlab_client/client.go b/pkg/vcs/gitlab_client/client.go index 27b3bbd5..6f877d1c 100644 --- a/pkg/vcs/gitlab_client/client.go +++ b/pkg/vcs/gitlab_client/client.go @@ -5,6 +5,8 @@ import ( "fmt" "io" "net/http" + "regexp" + "strconv" "strings" "github.com/pkg/errors" @@ -160,9 +162,46 @@ func (c *Client) CreateHook(ctx context.Context, repoName, webhookUrl, webhookSe return nil } +var reMergeRequest = regexp.MustCompile(`(.*)!(\d+)`) + func (c *Client) LoadHook(ctx context.Context, id string) (*repo.Repo, error) { - //TODO implement me - panic("implement me") + m := reMergeRequest.FindStringSubmatch(id) + if len(m) != 3 { + return nil, errors.New("must be in format REPOPATH!MR") + } + + repoPath := m[1] + mrNumber, err := strconv.ParseInt(m[2], 10, 32) + if err != nil { + return nil, errors.Wrap(err, "failed to parse merge request number") + } + + project, _, err := c.Projects.GetProject(repoPath, nil) + if err != nil { + return nil, errors.Wrapf(err, "failed to get project '%s'", repoPath) + } + + mergeRequest, _, err := c.MergeRequests.GetMergeRequest(repoPath, int(mrNumber), nil) + if err != nil { + return nil, errors.Wrapf(err, "failed to get merge request '%s' in project '%s'", mrNumber, repoPath) + } + + return &repo.Repo{ + BaseRef: mergeRequest.TargetBranch, + HeadRef: mergeRequest.SourceBranch, + DefaultBranch: project.DefaultBranch, + RepoDir: "", + Remote: "", + CloneURL: project.HTTPURLToRepo, + Name: project.Name, + Owner: "", + CheckID: mergeRequest.IID, + SHA: mergeRequest.SHA, + FullName: project.PathWithNamespace, + Username: c.username, + Email: c.email, + Labels: mergeRequest.Labels, + }, nil } func (c *Client) buildRepoFromEvent(event *gitlab.MergeEvent) *repo.Repo { From 65cc28a78ef2326087e0543826b11bc42bddf37c Mon Sep 17 00:00:00 2001 From: "joe.lombrozo" Date: Fri, 22 Dec 2023 09:54:04 -0500 Subject: [PATCH 24/24] fix format string --- pkg/vcs/gitlab_client/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/vcs/gitlab_client/client.go b/pkg/vcs/gitlab_client/client.go index 6f877d1c..877562ab 100644 --- a/pkg/vcs/gitlab_client/client.go +++ b/pkg/vcs/gitlab_client/client.go @@ -183,7 +183,7 @@ func (c *Client) LoadHook(ctx context.Context, id string) (*repo.Repo, error) { mergeRequest, _, err := c.MergeRequests.GetMergeRequest(repoPath, int(mrNumber), nil) if err != nil { - return nil, errors.Wrapf(err, "failed to get merge request '%s' in project '%s'", mrNumber, repoPath) + return nil, errors.Wrapf(err, "failed to get merge request '%d' in project '%s'", mrNumber, repoPath) } return &repo.Repo{