From b908a6aa4c38dca66086546d9d4fcabdca96c88d Mon Sep 17 00:00:00 2001 From: "joe.lombrozo" Date: Wed, 27 Mar 2024 13:03:09 -0400 Subject: [PATCH 1/6] skip unchanged apps --- pkg/checks/diff/check.go | 19 -------------- pkg/checks/diff/diff.go | 56 +++++++++++++++++++++++++--------------- pkg/msg/message.go | 22 +++++++++++----- 3 files changed, 50 insertions(+), 47 deletions(-) delete mode 100644 pkg/checks/diff/check.go diff --git a/pkg/checks/diff/check.go b/pkg/checks/diff/check.go deleted file mode 100644 index 0b7a1d50..00000000 --- a/pkg/checks/diff/check.go +++ /dev/null @@ -1,19 +0,0 @@ -package diff - -import ( - "context" - - "github.com/zapier/kubechecks/pkg/checks" - "github.com/zapier/kubechecks/pkg/msg" -) - -func Check(ctx context.Context, request checks.Request) (msg.Result, error) { - cr, rawDiff, err := getDiff(ctx, request.JsonManifests, request.App, request.Container, request.QueueApp, request.RemoveApp) - if err != nil { - return cr, err - } - - aiDiffSummary(ctx, request.Note, request.Container.Config, request.AppName, request.JsonManifests, rawDiff) - - return cr, nil -} diff --git a/pkg/checks/diff/diff.go b/pkg/checks/diff/diff.go index c81a1891..2226b2e5 100644 --- a/pkg/checks/diff/diff.go +++ b/pkg/checks/diff/diff.go @@ -24,7 +24,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" - "github.com/zapier/kubechecks/pkg/container" + "github.com/zapier/kubechecks/pkg/checks" "github.com/zapier/kubechecks/pkg/msg" "github.com/zapier/kubechecks/telemetry" ) @@ -40,21 +40,32 @@ func isAppMissingErr(err error) bool { return strings.Contains(err.Error(), "PermissionDenied") } +//func Check(ctx context.Context, request checks.Request) (msg.Result, error) { +// cr, rawDiff, err := getDiff(ctx, request.JsonManifests, request.App, request.Container, request.QueueApp, request.RemoveApp) +// if err != nil { +// return cr, err +// } +// +// return cr, nil +//} + /* -getDiff takes cli output and return as a string or an array of strings instead of printing +Check takes cli output and return as a string or an array of strings instead of printing 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, manifests []string, app argoappv1.Application, - ctr container.Container, - addApp, removeApp func(application2 argoappv1.Application), -) (msg.Result, string, error) { +func Check(ctx context.Context, request checks.Request) (msg.Result, error) { ctx, span := tracer.Start(ctx, "getDiff") defer span.End() + ctr := request.Container + app := request.App + manifests := request.JsonManifests + removeApp := request.RemoveApp + addApp := request.QueueApp + argoClient := ctr.ArgoClient log.Debug().Str("name", app.Name).Msg("generating diff for application...") @@ -71,7 +82,7 @@ func getDiff( if err != nil { if !isAppMissingErr(err) { telemetry.SetError(span, err, "Get Argo Managed Resources") - return msg.Result{}, "", err + return msg.Result{}, err } resources = new(application.ManagedResourcesResponse) @@ -91,22 +102,22 @@ func getDiff( argoSettings, err := settingsClient.Get(ctx, &settings.SettingsQuery{}) if err != nil { telemetry.SetError(span, err, "Get Argo Cluster Settings") - return msg.Result{}, "", err + return msg.Result{}, err } liveObjs, err := cmdutil.LiveObjects(resources.Items) if err != nil { telemetry.SetError(span, err, "Get Argo Live Objects") - return msg.Result{}, "", err + return msg.Result{}, err } groupedObjs, err := groupObjsByKey(unstructureds, liveObjs, app.Spec.Destination.Namespace) if err != nil { - return msg.Result{}, "", err + return msg.Result{}, err } if items, err = groupObjsForDiff(resources, groupedObjs, items, argoSettings, app.Name); err != nil { - return msg.Result{}, "", err + return msg.Result{}, err } diffBuffer := &strings.Builder{} @@ -134,13 +145,13 @@ func getDiff( Build() if err != nil { telemetry.SetError(span, err, "Build Diff") - return msg.Result{}, "failed to build diff", err + return msg.Result{}, err } diffRes, err := argodiff.StateDiff(item.live, item.target, diffConfig) if err != nil { telemetry.SetError(span, err, "State Diff") - return msg.Result{}, "failed to state diff", err + return msg.Result{}, err } if diffRes.Modified || item.target == nil || item.live == nil { @@ -162,7 +173,7 @@ func getDiff( err := PrintDiff(diffBuffer, live, target) if err != nil { telemetry.SetError(span, err, "Print Diff") - return msg.Result{}, "", err + return msg.Result{}, err } switch { case item.target == nil: @@ -183,20 +194,23 @@ func getDiff( } } } - var summary string + + var cr msg.Result + if added != 0 || modified != 0 || removed != 0 { - summary = fmt.Sprintf("%d added, %d modified, %d removed", added, modified, removed) + cr.Summary = fmt.Sprintf("%d added, %d modified, %d removed", added, modified, removed) } else { - summary = "No changes" + cr.Summary = "No changes" + cr.NoChangesDetected = true } diff := diffBuffer.String() - var cr msg.Result - cr.Summary = summary cr.Details = fmt.Sprintf("```diff\n%s\n```", diff) - return cr, diff, nil + aiDiffSummary(ctx, request.Note, request.Container.Config, request.AppName, request.JsonManifests, diff) + + return cr, nil } var nilApp = argoappv1.Application{} diff --git a/pkg/msg/message.go b/pkg/msg/message.go index c528ed0b..245044c1 100644 --- a/pkg/msg/message.go +++ b/pkg/msg/message.go @@ -18,8 +18,9 @@ import ( var tracer = otel.Tracer("pkg/msg") type Result struct { - State pkg.CommitState - Summary, Details string + State pkg.CommitState + Summary, Details string + NoChangesDetected bool } type AppResults struct { @@ -142,12 +143,8 @@ func (m *Message) SetFooter(start time.Time, commitSHA, labelFilter string, show m.footer = fmt.Sprintf("_Done: Pod: %s, Dur: %v, SHA: %s%s_\n", hostname, duration, pkg.GitCommit, envStr) } +// BuildComment iterates the map of all apps in this message, building a final comment from their current state func (m *Message) BuildComment(ctx context.Context) string { - return m.buildComment(ctx) -} - -// Iterate the map of all apps in this message, building a final comment from their current state -func (m *Message) buildComment(ctx context.Context) string { _, span := tracer.Start(ctx, "buildComment") defer span.End() @@ -165,7 +162,14 @@ func (m *Message) buildComment(ctx context.Context) string { results := m.apps[appName] appState := pkg.StateSuccess + noChangesDetected := false + for _, check := range results.results { + if check.NoChangesDetected { + noChangesDetected = true + continue + } + var summary string if check.State == pkg.StateNone { summary = check.Summary @@ -178,6 +182,10 @@ func (m *Message) buildComment(ctx context.Context) string { appState = pkg.WorstState(appState, check.State) } + if noChangesDetected { + continue + } + sb.WriteString("
\n") sb.WriteString("\n\n") sb.WriteString(fmt.Sprintf("## ArgoCD Application Checks: `%s` %s\n", appName, m.vcs.ToEmoji(appState))) From edfbe6bd215a2be5aa677f10d2bb81eacda3aef3 Mon Sep 17 00:00:00 2001 From: "joe.lombrozo" Date: Wed, 27 Mar 2024 13:21:44 -0400 Subject: [PATCH 2/6] fix test, add a new one --- pkg/msg/message_test.go | 47 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/pkg/msg/message_test.go b/pkg/msg/message_test.go index 2f61c230..e72d889c 100644 --- a/pkg/msg/message_test.go +++ b/pkg/msg/message_test.go @@ -30,7 +30,52 @@ func TestBuildComment(t *testing.T) { } m := NewMessage("message", 1, 2, fakeEmojiable{":test:"}) m.apps = appResults - comment := m.buildComment(context.TODO()) + comment := m.BuildComment(context.TODO()) + assert.Equal(t, `# Kubechecks Report +
+ + +## ArgoCD Application Checks: `+"`myapp`"+` :test: + + +
+this failed bigly Error :test: + +should add some important details here +
`, comment) +} + +func TestBuildComment_SkipUnchanged(t *testing.T) { + appResults := map[string]*AppResults{ + "myapp": { + results: []Result{ + { + State: pkg.StateError, + Summary: "this failed bigly", + Details: "should add some important details here", + }, + }, + }, + "myapp2": { + results: []Result{ + { + State: pkg.StateError, + Summary: "this thing failed", + Details: "should add some important details here", + }, + { + State: pkg.StateError, + Summary: "this also failed", + Details: "there are no important details", + NoChangesDetected: true, // this should remove the app entirely + }, + }, + }, + } + + m := NewMessage("message", 1, 2, fakeEmojiable{":test:"}) + m.apps = appResults + comment := m.BuildComment(context.TODO()) assert.Equal(t, `# Kubechecks Report
From 756a78cb84d8a69ade57d1e49f42c52432a8acd7 Mon Sep 17 00:00:00 2001 From: Joseph Lombrozo Date: Wed, 27 Mar 2024 22:33:28 -0400 Subject: [PATCH 3/6] refactor diff check to make it more grokkable Signed-off-by: Joseph Lombrozo --- pkg/checks/diff/diff.go | 224 ++++++++++++++++++++++++---------------- 1 file changed, 135 insertions(+), 89 deletions(-) diff --git a/pkg/checks/diff/diff.go b/pkg/checks/diff/diff.go index 2226b2e5..938b6293 100644 --- a/pkg/checks/diff/diff.go +++ b/pkg/checks/diff/diff.go @@ -14,6 +14,7 @@ import ( argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/argoproj/argo-cd/v2/util/argo" argodiff "github.com/argoproj/argo-cd/v2/util/argo/diff" + "github.com/argoproj/gitops-engine/pkg/diff" "github.com/argoproj/gitops-engine/pkg/sync/hook" "github.com/argoproj/gitops-engine/pkg/sync/ignore" "github.com/argoproj/gitops-engine/pkg/utils/kube" @@ -60,37 +61,13 @@ func Check(ctx context.Context, request checks.Request) (msg.Result, error) { ctx, span := tracer.Start(ctx, "getDiff") defer span.End() - ctr := request.Container app := request.App - manifests := request.JsonManifests - removeApp := request.RemoveApp - addApp := request.QueueApp - - argoClient := ctr.ArgoClient log.Debug().Str("name", app.Name).Msg("generating diff for application...") - settingsCloser, settingsClient := argoClient.GetSettingsClient() - defer settingsCloser.Close() - - closer, appClient := argoClient.GetApplicationClient() - defer closer.Close() - - 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 msg.Result{}, err - } - - resources = new(application.ManagedResourcesResponse) - } - items := make([]objKeyLiveTarget, 0) var unstructureds []*unstructured.Unstructured - for _, mfst := range manifests { + for _, mfst := range request.JsonManifests { obj, err := argoappv1.UnmarshalToUnstructured(mfst) if err != nil { log.Warn().Err(err).Msg("failed to unmarshal to unstructured") @@ -99,13 +76,18 @@ func Check(ctx context.Context, request checks.Request) (msg.Result, error) { unstructureds = append(unstructureds, obj) } - argoSettings, err := settingsClient.Get(ctx, &settings.SettingsQuery{}) + + argoSettings, err := getArgoSettings(ctx, request) + if err != nil { + return msg.Result{}, err + } + + resources, err := getResources(ctx, request) if err != nil { - telemetry.SetError(span, err, "Get Argo Cluster Settings") return msg.Result{}, err } - liveObjs, err := cmdutil.LiveObjects(resources.Items) + liveObjs, err := cmdutil.LiveObjects(resources) if err != nil { telemetry.SetError(span, err, "Get Argo Live Objects") return msg.Result{}, err @@ -120,7 +102,7 @@ func Check(ctx context.Context, request checks.Request) (msg.Result, error) { return msg.Result{}, err } - diffBuffer := &strings.Builder{} + var diffBuffer strings.Builder var added, modified, removed int for _, item := range items { resourceId := fmt.Sprintf("%s/%s %s/%s", item.key.Group, item.key.Kind, item.key.Namespace, item.key.Name) @@ -128,70 +110,19 @@ func Check(ctx context.Context, request checks.Request) (msg.Result, error) { if item.target != nil && hook.IsHook(item.target) || item.live != nil && hook.IsHook(item.live) { continue } - overrides := make(map[string]argoappv1.ResourceOverride) - for k := range argoSettings.ResourceOverrides { - val := argoSettings.ResourceOverrides[k] - overrides[k] = *val - } - - // TODO remove hardcoded IgnoreAggregatedRoles and retrieve the - // compareOptions in the protobuf - ignoreAggregatedRoles := false - diffConfig, err := argodiff.NewDiffConfigBuilder(). - WithLogger(zerologr.New(&log.Logger)). - WithDiffSettings(app.Spec.IgnoreDifferences, overrides, ignoreAggregatedRoles). - WithTracking(argoSettings.AppLabelKey, argoSettings.TrackingMethod). - WithNoCache(). - Build() - if err != nil { - telemetry.SetError(span, err, "Build Diff") - return msg.Result{}, err - } - diffRes, err := argodiff.StateDiff(item.live, item.target, diffConfig) + diffRes, err := generateDiff(ctx, request, argoSettings, item) if err != nil { - telemetry.SetError(span, err, "State Diff") return msg.Result{}, err } if diffRes.Modified || item.target == nil || item.live == nil { - diffBuffer.WriteString(fmt.Sprintf("===== %s ======\n", resourceId)) - var live *unstructured.Unstructured - var target *unstructured.Unstructured - if item.target != nil && item.live != nil { - target = &unstructured.Unstructured{} - live = item.live - 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 - } - - err := PrintDiff(diffBuffer, live, target) + err := addResourceDiffToMessage(ctx, &diffBuffer, resourceId, item, diffRes) if err != nil { - telemetry.SetError(span, err, "Print Diff") return msg.Result{}, err } - switch { - case item.target == nil: - removed++ - if app, ok := isApp(item, diffRes.NormalizedLive); ok { - removeApp(app) - } - case item.live == nil: - added++ - if app, ok := isApp(item, diffRes.PredictedLive); ok { - addApp(app) - } - case diffRes.Modified: - modified++ - if app, ok := isApp(item, diffRes.PredictedLive); ok { - addApp(app) - } - } + + processResources(item, diffRes, request, &added, &modified, &removed) } } @@ -204,15 +135,130 @@ func Check(ctx context.Context, request checks.Request) (msg.Result, error) { cr.NoChangesDetected = true } - diff := diffBuffer.String() + renderedDiff := diffBuffer.String() - cr.Details = fmt.Sprintf("```diff\n%s\n```", diff) + cr.Details = fmt.Sprintf("```diff\n%s\n```", renderedDiff) - aiDiffSummary(ctx, request.Note, request.Container.Config, request.AppName, request.JsonManifests, diff) + aiDiffSummary(ctx, request.Note, request.Container.Config, request.AppName, request.JsonManifests, renderedDiff) return cr, nil } +func processResources(item objKeyLiveTarget, diffRes diff.DiffResult, request checks.Request, added, modified, removed *int) { + switch { + case item.target == nil: + *removed++ + if app, ok := isApp(item, diffRes.NormalizedLive); ok { + request.RemoveApp(app) + } + case item.live == nil: + *added++ + if app, ok := isApp(item, diffRes.PredictedLive); ok { + request.QueueApp(app) + } + case diffRes.Modified: + *modified++ + if app, ok := isApp(item, diffRes.PredictedLive); ok { + request.QueueApp(app) + } + } +} + +func addResourceDiffToMessage(ctx context.Context, diffBuffer *strings.Builder, resourceId string, item objKeyLiveTarget, diffRes diff.DiffResult) error { + ctx, span := tracer.Start(ctx, "addResourceDiffToMessage") + defer span.End() + + diffBuffer.WriteString(fmt.Sprintf("===== %s ======\n", resourceId)) + + var live *unstructured.Unstructured + var target *unstructured.Unstructured + if item.target != nil && item.live != nil { + target = &unstructured.Unstructured{} + live = item.live + 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 + } + + err := PrintDiff(diffBuffer, live, target) + if err != nil { + telemetry.SetError(span, err, "Print Diff") + return err + } + + return nil +} + +func generateDiff(ctx context.Context, request checks.Request, argoSettings *settings.Settings, item objKeyLiveTarget) (diff.DiffResult, error) { + ctx, span := tracer.Start(ctx, "getResources") + defer span.End() + + overrides := make(map[string]argoappv1.ResourceOverride) + for k := range argoSettings.ResourceOverrides { + val := argoSettings.ResourceOverrides[k] + overrides[k] = *val + } + + ignoreAggregatedRoles := false + diffConfig, err := argodiff.NewDiffConfigBuilder(). + WithLogger(zerologr.New(&log.Logger)). + WithDiffSettings(request.App.Spec.IgnoreDifferences, overrides, ignoreAggregatedRoles). + WithTracking(argoSettings.AppLabelKey, argoSettings.TrackingMethod). + WithNoCache(). + Build() + if err != nil { + telemetry.SetError(span, err, "Build Diff") + return diff.DiffResult{}, err + } + + diffRes, err := argodiff.StateDiff(item.live, item.target, diffConfig) + if err != nil { + telemetry.SetError(span, err, "State Diff") + return diff.DiffResult{}, err + } + return diffRes, nil +} + +func getResources(ctx context.Context, request checks.Request) ([]*argoappv1.ResourceDiff, error) { + ctx, span := tracer.Start(ctx, "getResources") + defer span.End() + + closer, appClient := request.Container.ArgoClient.GetApplicationClient() + defer closer.Close() + + resources, err := appClient.ManagedResources(ctx, &application.ResourcesQuery{ + ApplicationName: &request.App.Name, + }) + if err != nil { + if !isAppMissingErr(err) { + telemetry.SetError(span, err, "Get Argo Managed Resources") + return nil, nil + } + + resources = new(application.ManagedResourcesResponse) + } + return resources.Items, err +} + +func getArgoSettings(ctx context.Context, request checks.Request) (*settings.Settings, error) { + ctx, span := tracer.Start(ctx, "getArgoSettings") + defer span.End() + + settingsCloser, settingsClient := request.Container.ArgoClient.GetSettingsClient() + defer settingsCloser.Close() + + argoSettings, err := settingsClient.Get(ctx, &settings.SettingsQuery{}) + if err != nil { + telemetry.SetError(span, err, "Get Argo Cluster Settings") + return nil, err + } + return argoSettings, nil +} + var nilApp = argoappv1.Application{} func isApp(item objKeyLiveTarget, manifests []byte) (argoappv1.Application, bool) { @@ -259,9 +305,9 @@ func groupObjsByKey(localObs []*unstructured.Unstructured, liveObjs []*unstructu } // 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, error) { +func groupObjsForDiff(resources []*argoappv1.ResourceDiff, objs map[kube.ResourceKey]*unstructured.Unstructured, items []objKeyLiveTarget, argoSettings *settings.Settings, appName string) ([]objKeyLiveTarget, error) { resourceTracking := argo.NewResourceTracking() - for _, res := range resources.Items { + for _, res := range resources { var live = &unstructured.Unstructured{} if err := json.Unmarshal([]byte(res.NormalizedLiveState), &live); err != nil { return nil, err From 081078e18d383006b8689bd61067bd1f77b0028c Mon Sep 17 00:00:00 2001 From: "joe.lombrozo" Date: Wed, 27 Mar 2024 22:37:09 -0400 Subject: [PATCH 4/6] remove commented out code --- pkg/checks/diff/diff.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/pkg/checks/diff/diff.go b/pkg/checks/diff/diff.go index 938b6293..52f381df 100644 --- a/pkg/checks/diff/diff.go +++ b/pkg/checks/diff/diff.go @@ -41,15 +41,6 @@ func isAppMissingErr(err error) bool { return strings.Contains(err.Error(), "PermissionDenied") } -//func Check(ctx context.Context, request checks.Request) (msg.Result, error) { -// cr, rawDiff, err := getDiff(ctx, request.JsonManifests, request.App, request.Container, request.QueueApp, request.RemoveApp) -// if err != nil { -// return cr, err -// } -// -// return cr, nil -//} - /* Check takes cli output and return as a string or an array of strings instead of printing From 4359741ae5d7a37f8dcd023e36645797cf8b4745 Mon Sep 17 00:00:00 2001 From: "joe.lombrozo" Date: Thu, 28 Mar 2024 10:07:53 -0400 Subject: [PATCH 5/6] remove unused golang just target --- justfile | 3 --- 1 file changed, 3 deletions(-) diff --git a/justfile b/justfile index e6b05fb3..d3fed4b5 100644 --- a/justfile +++ b/justfile @@ -67,8 +67,5 @@ unit_test_race: rebuild_docs: ./earthly +rebuild-docs -lint-golang: - ./earthly +lint-golang - ci-golang: ./earthly +ci-golang From 2ff750f0f101fee3dfae13a7869968a2600e7742 Mon Sep 17 00:00:00 2001 From: "joe.lombrozo" Date: Thu, 28 Mar 2024 10:10:59 -0400 Subject: [PATCH 6/6] remove ineffectual assignments --- pkg/checks/diff/diff.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/checks/diff/diff.go b/pkg/checks/diff/diff.go index 52f381df..dccfcf0f 100644 --- a/pkg/checks/diff/diff.go +++ b/pkg/checks/diff/diff.go @@ -156,7 +156,7 @@ func processResources(item objKeyLiveTarget, diffRes diff.DiffResult, request ch } func addResourceDiffToMessage(ctx context.Context, diffBuffer *strings.Builder, resourceId string, item objKeyLiveTarget, diffRes diff.DiffResult) error { - ctx, span := tracer.Start(ctx, "addResourceDiffToMessage") + _, span := tracer.Start(ctx, "addResourceDiffToMessage") defer span.End() diffBuffer.WriteString(fmt.Sprintf("===== %s ======\n", resourceId)) @@ -185,7 +185,7 @@ func addResourceDiffToMessage(ctx context.Context, diffBuffer *strings.Builder, } func generateDiff(ctx context.Context, request checks.Request, argoSettings *settings.Settings, item objKeyLiveTarget) (diff.DiffResult, error) { - ctx, span := tracer.Start(ctx, "getResources") + _, span := tracer.Start(ctx, "getResources") defer span.End() overrides := make(map[string]argoappv1.ResourceOverride)