From 7db31a13992eb13fa5bae82b93a75249656f3802 Mon Sep 17 00:00:00 2001 From: Martin Guibert Date: Thu, 21 Jan 2021 18:05:29 +0100 Subject: [PATCH 1/2] only sent one alert for computed fieds --- pkg/analyser/analyzer.go | 42 +++++++++++++++++------------------ pkg/analyser/analyzer_test.go | 24 ++++++-------------- 2 files changed, 27 insertions(+), 39 deletions(-) diff --git a/pkg/analyser/analyzer.go b/pkg/analyser/analyzer.go index c4ee1821..11373b0e 100644 --- a/pkg/analyser/analyzer.go +++ b/pkg/analyser/analyzer.go @@ -4,7 +4,6 @@ import ( "fmt" "reflect" "sort" - "strings" "github.com/cloudskiff/driftctl/pkg/alerter" "github.com/cloudskiff/driftctl/pkg/resource" @@ -36,6 +35,7 @@ func (a Analyzer) Analyze(remoteResources, resourcesFromState []resource.Resourc filteredRemoteResource = append(filteredRemoteResource, remoteRes) } + nbComputed := 0 for _, stateRes := range resourcesFromState { i, remoteRes, found := findCorrespondingRes(filteredRemoteResource, stateRes) @@ -64,6 +64,9 @@ func (a Analyzer) Analyze(remoteResources, resourcesFromState []resource.Resourc } c := Change{Change: change} c.Computed = a.isComputedField(stateRes, c) + if c.Computed { + nbComputed++ + } changelog = append(changelog, c) } if len(changelog) > 0 { @@ -71,8 +74,9 @@ func (a Analyzer) Analyze(remoteResources, resourcesFromState []resource.Resourc Res: stateRes, Changelog: changelog, }) - a.sendAlertOnComputedField(stateRes, changelog) } + + a.sendAlertOnComputedField(nbComputed) } } @@ -109,27 +113,21 @@ func (a Analyzer) isComputedField(stateRes resource.Resource, change Change) boo return false } -// sendAlertOnComputedField will send an alert to a channel for diffs on computed field -func (a Analyzer) sendAlertOnComputedField(stateRes resource.Resource, delta Changelog) { - for _, d := range delta { - if d.Computed { - // We need to copy the path (for console output compatibility) and remove the - // last index if it's a slice. - // We want a console output of format: struct.0.array.0: "foo" => "bar" (computed) - // We want a json output of format: "message": "struct.0.array is a computed field" - tmp := make([]string, len(d.Path)) - copy(tmp, d.Path) - field, _ := a.getField(reflect.TypeOf(stateRes), tmp) - if field.Type.Kind() == reflect.Slice { - tmp = tmp[:len(tmp)-1] - } - path := strings.Join(tmp, ".") - a.alerter.SendAlert(fmt.Sprintf("%s.%s", stateRes.TerraformType(), stateRes.TerraformId()), - alerter.Alert{ - Message: fmt.Sprintf("%s is a computed field", path), - }) - } +// sendAlertOnComputedField will send an alert if we found computed fields +func (a Analyzer) sendAlertOnComputedField(nbComputed int) { + if nbComputed <= 0 { + return // no computed field } + + computedFields := "1 computed field" + if nbComputed > 1 { + computedFields = fmt.Sprintf("%d computed fields", nbComputed) + } + + a.alerter.SendAlert("*", + alerter.Alert{ + Message: fmt.Sprintf("You have diffs on %s, check the documentation for potential false positive drifts", computedFields), + }) } // getField recursively finds the deepest field inside a resource depending on diff --git a/pkg/analyser/analyzer_test.go b/pkg/analyser/analyzer_test.go index 39198084..240c4949 100644 --- a/pkg/analyser/analyzer_test.go +++ b/pkg/analyser/analyzer_test.go @@ -270,12 +270,9 @@ func TestAnalyze(t *testing.T) { }, }, alerts: alerter.Alerts{ - "FakeResource.foobar": { + "*": { { - Message: "BarFoo is a computed field", - }, - { - Message: "Struct.Baz is a computed field", + Message: "You have diffs on 2 computed fields, check the documentation for potential false positive drifts", }, }, }, @@ -352,9 +349,9 @@ func TestAnalyze(t *testing.T) { }, }, alerts: alerter.Alerts{ - "fakeres.foobar": { + "*": { { - Message: "BarFoo is a computed field", + Message: "You have diffs on 1 computed field, check the documentation for potential false positive drifts", }, }, }, @@ -673,17 +670,10 @@ func TestAnalyze(t *testing.T) { { Message: "Should not be ignored", }, + }, + "*": { { - Message: "BarFoo is a computed field", - }, - { - Message: "Struct.Baz is a computed field", - }, - { - Message: "StructSlice.0.String is a computed field", - }, - { - Message: "StructSlice.0.Array is a computed field", + Message: "You have diffs on 4 computed fields, check the documentation for potential false positive drifts", }, }, }, From e47075729c257d16eb3311805add06b476c8021e Mon Sep 17 00:00:00 2001 From: Martin Guibert Date: Thu, 21 Jan 2021 18:05:29 +0100 Subject: [PATCH 2/2] only send one alert for computed fields --- pkg/analyser/analyzer.go | 30 +++++-------------- pkg/analyser/analyzer_test.go | 12 ++++---- pkg/cmd/scan/output/console.go | 9 +++--- pkg/cmd/scan/output/output_test.go | 13 ++------ .../testdata/output_computed_fields.json | 13 ++------ .../testdata/output_computed_fields.txt | 2 +- 6 files changed, 23 insertions(+), 56 deletions(-) diff --git a/pkg/analyser/analyzer.go b/pkg/analyser/analyzer.go index 11373b0e..e8326148 100644 --- a/pkg/analyser/analyzer.go +++ b/pkg/analyser/analyzer.go @@ -1,7 +1,6 @@ package analyser import ( - "fmt" "reflect" "sort" @@ -35,7 +34,7 @@ func (a Analyzer) Analyze(remoteResources, resourcesFromState []resource.Resourc filteredRemoteResource = append(filteredRemoteResource, remoteRes) } - nbComputed := 0 + haveComputedDiff := false for _, stateRes := range resourcesFromState { i, remoteRes, found := findCorrespondingRes(filteredRemoteResource, stateRes) @@ -65,7 +64,7 @@ func (a Analyzer) Analyze(remoteResources, resourcesFromState []resource.Resourc c := Change{Change: change} c.Computed = a.isComputedField(stateRes, c) if c.Computed { - nbComputed++ + haveComputedDiff = true } changelog = append(changelog, c) } @@ -75,8 +74,12 @@ func (a Analyzer) Analyze(remoteResources, resourcesFromState []resource.Resourc Changelog: changelog, }) } - - a.sendAlertOnComputedField(nbComputed) + } + if haveComputedDiff { + a.alerter.SendAlert("", + alerter.Alert{ + Message: "You have diffs on computed fields, check the documentation for potential false positive drifts", + }) } } @@ -113,23 +116,6 @@ func (a Analyzer) isComputedField(stateRes resource.Resource, change Change) boo return false } -// sendAlertOnComputedField will send an alert if we found computed fields -func (a Analyzer) sendAlertOnComputedField(nbComputed int) { - if nbComputed <= 0 { - return // no computed field - } - - computedFields := "1 computed field" - if nbComputed > 1 { - computedFields = fmt.Sprintf("%d computed fields", nbComputed) - } - - a.alerter.SendAlert("*", - alerter.Alert{ - Message: fmt.Sprintf("You have diffs on %s, check the documentation for potential false positive drifts", computedFields), - }) -} - // getField recursively finds the deepest field inside a resource depending on // its path and its type func (a Analyzer) getField(t reflect.Type, path []string) (reflect.StructField, bool) { diff --git a/pkg/analyser/analyzer_test.go b/pkg/analyser/analyzer_test.go index 240c4949..55b06dcc 100644 --- a/pkg/analyser/analyzer_test.go +++ b/pkg/analyser/analyzer_test.go @@ -270,9 +270,9 @@ func TestAnalyze(t *testing.T) { }, }, alerts: alerter.Alerts{ - "*": { + "": { { - Message: "You have diffs on 2 computed fields, check the documentation for potential false positive drifts", + Message: "You have diffs on computed fields, check the documentation for potential false positive drifts", }, }, }, @@ -349,9 +349,9 @@ func TestAnalyze(t *testing.T) { }, }, alerts: alerter.Alerts{ - "*": { + "": { { - Message: "You have diffs on 1 computed field, check the documentation for potential false positive drifts", + Message: "You have diffs on computed fields, check the documentation for potential false positive drifts", }, }, }, @@ -671,9 +671,9 @@ func TestAnalyze(t *testing.T) { Message: "Should not be ignored", }, }, - "*": { + "": { { - Message: "You have diffs on 4 computed fields, check the documentation for potential false positive drifts", + Message: "You have diffs on computed fields, check the documentation for potential false positive drifts", }, }, }, diff --git a/pkg/cmd/scan/output/console.go b/pkg/cmd/scan/output/console.go index 46f78682..03b089c5 100644 --- a/pkg/cmd/scan/output/console.go +++ b/pkg/cmd/scan/output/console.go @@ -28,8 +28,6 @@ func NewConsole() *Console { } func (c *Console) Write(analysis *analyser.Analysis) error { - var shouldWarnOnComputedFields bool - if analysis.Summary().TotalDeleted > 0 { fmt.Printf("Found deleted resources:\n") deletedByType := groupByType(analysis.Deleted()) @@ -89,7 +87,6 @@ func (c *Console) Write(analysis *analyser.Analysis) error { } fmt.Printf(" %s %s => %s", pref, prettify(change.From), prettify(change.To)) if change.Computed { - shouldWarnOnComputedFields = true fmt.Printf(" %s", color.YellowString("(computed)")) } fmt.Printf("\n") @@ -99,8 +96,10 @@ func (c *Console) Write(analysis *analyser.Analysis) error { c.writeSummary(analysis) - if shouldWarnOnComputedFields { - fmt.Printf("%s\n", color.YellowString("You have diffs on computed field, check the documentation for potential false positive drifts")) + for _, alerts := range analysis.Alerts() { + for _, alert := range alerts { + fmt.Printf("%s\n", color.YellowString(alert.Message)) + } } return nil diff --git a/pkg/cmd/scan/output/output_test.go b/pkg/cmd/scan/output/output_test.go index ffc3f8aa..b387586d 100644 --- a/pkg/cmd/scan/output/output_test.go +++ b/pkg/cmd/scan/output/output_test.go @@ -229,18 +229,9 @@ func fakeAnalysisWithComputedFields() *analyser.Analysis { }, }}) a.SetAlerts(alerter.Alerts{ - "aws_diff_resource.diff-id-1": []alerter.Alert{ + "": []alerter.Alert{ { - Message: "updated.field is a computed field", - }, - { - Message: "a is a computed field", - }, - { - Message: "struct.0.array is a computed field", - }, - { - Message: "struct.0.string is a computed field", + Message: "You have diffs on computed fields, check the documentation for potential false positive drifts", }, }, }) diff --git a/pkg/cmd/scan/output/testdata/output_computed_fields.json b/pkg/cmd/scan/output/testdata/output_computed_fields.json index 7a57e91b..642224ca 100644 --- a/pkg/cmd/scan/output/testdata/output_computed_fields.json +++ b/pkg/cmd/scan/output/testdata/output_computed_fields.json @@ -78,18 +78,9 @@ ], "coverage": 100, "alerts": { - "aws_diff_resource.diff-id-1": [ + "": [ { - "message": "updated.field is a computed field" - }, - { - "message": "a is a computed field" - }, - { - "message": "struct.0.array is a computed field" - }, - { - "message": "struct.0.string is a computed field" + "message": "You have diffs on computed fields, check the documentation for potential false positive drifts" } ] } diff --git a/pkg/cmd/scan/output/testdata/output_computed_fields.txt b/pkg/cmd/scan/output/testdata/output_computed_fields.txt index 7eb5aea7..fbf1daf0 100644 --- a/pkg/cmd/scan/output/testdata/output_computed_fields.txt +++ b/pkg/cmd/scan/output/testdata/output_computed_fields.txt @@ -11,4 +11,4 @@ Found 1 resource(s) - 0 not covered by IaC - 0 deleted on cloud provider - 1/1 drifted from IaC -You have diffs on computed field, check the documentation for potential false positive drifts +You have diffs on computed fields, check the documentation for potential false positive drifts