From c08b1c4235360301d1847a3ba7a3621c6983114a Mon Sep 17 00:00:00 2001 From: Tommaso Pozzetti Date: Wed, 14 Apr 2021 15:41:04 -0500 Subject: [PATCH 1/2] Normalize YAML manifests before running diff The current behavior of helm-diff is to show diffs even when the YAML files are semantically identical but they contain style differences, such as indentation or key ordering. As an example, the following two YAMLs would be showing diffs: ``` apiVersion: v1 kind: Service metadata: name: app-name labels: app.kubernetes.io/name: app-name app.kubernetes.io/managed-by: Helm spec: ports: - name: http port: 80 targetPort: http selector: app.kubernetes.io/name: app-name type: ClusterIP ``` ``` apiVersion: v1 kind: Service metadata: labels: app.kubernetes.io/managed-by: Helm app.kubernetes.io/name: app-name name: app-name spec: ports: - name: http port: 80 targetPort: http selector: app.kubernetes.io/name: app-name type: ClusterIP ``` even though they are semantically the same. This commit exploits the reordering and normalization that is performed by the yaml package when marshaling by unmarshaling and re-marshaling all manifests before running the diff, thus eliminating the issue. --- manifest/parse.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/manifest/parse.go b/manifest/parse.go index 49a0143a..712f5aba 100644 --- a/manifest/parse.go +++ b/manifest/parse.go @@ -135,7 +135,7 @@ func parseContent(content string, defaultNamespace string, excludedHooks ...stri if parsedMetadata.Kind == "List" { type ListV1 struct { - Items []yaml.MapSlice `yaml:"items"` + Items []map[interface{}]interface{} `yaml:"items"` } var list ListV1 @@ -163,6 +163,16 @@ func parseContent(content string, defaultNamespace string, excludedHooks ...stri return result, nil } + var object map[interface{}]interface{} + if err := yaml.Unmarshal([]byte(content), &object); err != nil { + log.Fatalf("YAML unmarshal error: %s\nCan't unmarshal %s", err, content) + } + normalizedContent, err := yaml.Marshal(object) + if err != nil { + log.Fatalf("YAML marshal error: %s\nCan't marshal %v", err, object) + } + content = string(normalizedContent) + if isHook(parsedMetadata, excludedHooks...) { return nil, nil } From 3ce490ed30de63a4d79b2a2b79d3c00204772f21 Mon Sep 17 00:00:00 2001 From: Tommaso Pozzetti Date: Fri, 28 May 2021 11:29:55 -0500 Subject: [PATCH 2/2] Add --normalize-manifests flag to control manifest normalization behavior --- cmd/release.go | 28 +++++++++++++++------------- cmd/revision.go | 38 ++++++++++++++++++++------------------ cmd/rollback.go | 30 ++++++++++++++++-------------- cmd/upgrade.go | 20 +++++++++++--------- manifest/parse.go | 35 ++++++++++++++++++++--------------- manifest/parse_test.go | 18 +++++++++--------- 6 files changed, 91 insertions(+), 78 deletions(-) diff --git a/cmd/release.go b/cmd/release.go index 0706d0ac..1a4a6492 100644 --- a/cmd/release.go +++ b/cmd/release.go @@ -13,15 +13,16 @@ import ( ) type release struct { - client helm.Interface - detailedExitCode bool - suppressedKinds []string - releases []string - outputContext int - includeTests bool - showSecrets bool - output string - stripTrailingCR bool + client helm.Interface + detailedExitCode bool + suppressedKinds []string + releases []string + outputContext int + includeTests bool + showSecrets bool + output string + stripTrailingCR bool + normalizeManifests bool } const releaseCmdLongUsage = ` @@ -81,6 +82,7 @@ func releaseCmd() *cobra.Command { releaseCmd.Flags().BoolVar(&diff.includeTests, "include-tests", false, "enable the diffing of the helm test hooks") releaseCmd.Flags().StringVar(&diff.output, "output", "diff", "Possible values: diff, simple, template. When set to \"template\", use the env var HELM_DIFF_TPL to specify the template.") releaseCmd.Flags().BoolVar(&diff.stripTrailingCR, "strip-trailing-cr", false, "strip trailing carriage return on input") + releaseCmd.Flags().BoolVar(&diff.normalizeManifests, "normalize-manifests", false, "normalize manifests before running diff to exclude style differences from the output") releaseCmd.SuggestionsMinimumDistance = 1 @@ -117,8 +119,8 @@ func (d *release) differentiateHelm3() error { if releaseChart1 == releaseChart2 { seenAnyChanges := diff.Releases( - manifest.Parse(string(releaseResponse1), namespace, excludes...), - manifest.Parse(string(releaseResponse2), namespace, excludes...), + manifest.Parse(string(releaseResponse1), namespace, d.normalizeManifests, excludes...), + manifest.Parse(string(releaseResponse2), namespace, d.normalizeManifests, excludes...), d.suppressedKinds, d.showSecrets, d.outputContext, @@ -152,8 +154,8 @@ func (d *release) differentiate() error { if releaseResponse1.Release.Chart.Metadata.Name == releaseResponse2.Release.Chart.Metadata.Name { seenAnyChanges := diff.Releases( - manifest.ParseRelease(releaseResponse1.Release, d.includeTests), - manifest.ParseRelease(releaseResponse2.Release, d.includeTests), + manifest.ParseRelease(releaseResponse1.Release, d.includeTests, d.normalizeManifests), + manifest.ParseRelease(releaseResponse2.Release, d.includeTests, d.normalizeManifests), d.suppressedKinds, d.showSecrets, d.outputContext, diff --git a/cmd/revision.go b/cmd/revision.go index c187a311..59b29b7d 100644 --- a/cmd/revision.go +++ b/cmd/revision.go @@ -14,16 +14,17 @@ import ( ) type revision struct { - release string - client helm.Interface - detailedExitCode bool - suppressedKinds []string - revisions []string - outputContext int - includeTests bool - showSecrets bool - output string - stripTrailingCR bool + release string + client helm.Interface + detailedExitCode bool + suppressedKinds []string + revisions []string + outputContext int + includeTests bool + showSecrets bool + output string + stripTrailingCR bool + normalizeManifests bool } const revisionCmdLongUsage = ` @@ -91,6 +92,7 @@ func revisionCmd() *cobra.Command { revisionCmd.Flags().BoolVar(&diff.includeTests, "include-tests", false, "enable the diffing of the helm test hooks") revisionCmd.Flags().StringVar(&diff.output, "output", "diff", "Possible values: diff, simple, template. When set to \"template\", use the env var HELM_DIFF_TPL to specify the template.") revisionCmd.Flags().BoolVar(&diff.stripTrailingCR, "strip-trailing-cr", false, "strip trailing carriage return on input") + revisionCmd.Flags().BoolVar(&diff.normalizeManifests, "normalize-manifests", false, "normalize manifests before running diff to exclude style differences from the output") revisionCmd.SuggestionsMinimumDistance = 1 @@ -122,8 +124,8 @@ func (d *revision) differentiateHelm3() error { } diff.Manifests( - manifest.Parse(string(revisionResponse), namespace, excludes...), - manifest.Parse(string(releaseResponse), namespace, excludes...), + manifest.Parse(string(revisionResponse), namespace, d.normalizeManifests, excludes...), + manifest.Parse(string(releaseResponse), namespace, d.normalizeManifests, excludes...), d.suppressedKinds, d.showSecrets, d.outputContext, @@ -149,8 +151,8 @@ func (d *revision) differentiateHelm3() error { } seenAnyChanges := diff.Manifests( - manifest.Parse(string(revisionResponse1), namespace, excludes...), - manifest.Parse(string(revisionResponse2), namespace, excludes...), + manifest.Parse(string(revisionResponse1), namespace, d.normalizeManifests, excludes...), + manifest.Parse(string(revisionResponse2), namespace, d.normalizeManifests, excludes...), d.suppressedKinds, d.showSecrets, d.outputContext, @@ -189,8 +191,8 @@ func (d *revision) differentiate() error { } diff.Manifests( - manifest.ParseRelease(revisionResponse.Release, d.includeTests), - manifest.ParseRelease(releaseResponse.Release, d.includeTests), + manifest.ParseRelease(revisionResponse.Release, d.includeTests, d.normalizeManifests), + manifest.ParseRelease(releaseResponse.Release, d.includeTests, d.normalizeManifests), d.suppressedKinds, d.showSecrets, d.outputContext, @@ -216,8 +218,8 @@ func (d *revision) differentiate() error { } seenAnyChanges := diff.Manifests( - manifest.ParseRelease(revisionResponse1.Release, d.includeTests), - manifest.ParseRelease(revisionResponse2.Release, d.includeTests), + manifest.ParseRelease(revisionResponse1.Release, d.includeTests, d.normalizeManifests), + manifest.ParseRelease(revisionResponse2.Release, d.includeTests, d.normalizeManifests), d.suppressedKinds, d.showSecrets, d.outputContext, diff --git a/cmd/rollback.go b/cmd/rollback.go index 3cec5f67..68ba99c0 100644 --- a/cmd/rollback.go +++ b/cmd/rollback.go @@ -14,16 +14,17 @@ import ( ) type rollback struct { - release string - client helm.Interface - detailedExitCode bool - suppressedKinds []string - revisions []string - outputContext int - includeTests bool - showSecrets bool - output string - stripTrailingCR bool + release string + client helm.Interface + detailedExitCode bool + suppressedKinds []string + revisions []string + outputContext int + includeTests bool + showSecrets bool + output string + stripTrailingCR bool + normalizeManifests bool } const rollbackCmdLongUsage = ` @@ -83,6 +84,7 @@ func rollbackCmd() *cobra.Command { rollbackCmd.Flags().BoolVar(&diff.includeTests, "include-tests", false, "enable the diffing of the helm test hooks") rollbackCmd.Flags().StringVar(&diff.output, "output", "diff", "Possible values: diff, simple, template. When set to \"template\", use the env var HELM_DIFF_TPL to specify the template.") rollbackCmd.Flags().BoolVar(&diff.stripTrailingCR, "strip-trailing-cr", false, "strip trailing carriage return on input") + rollbackCmd.Flags().BoolVar(&diff.normalizeManifests, "normalize-manifests", false, "normalize manifests before running diff to exclude style differences from the output") rollbackCmd.SuggestionsMinimumDistance = 1 @@ -115,8 +117,8 @@ func (d *rollback) backcastHelm3() error { // create a diff between the current manifest and the version of the manifest that a user is intended to rollback seenAnyChanges := diff.Manifests( - manifest.Parse(string(releaseResponse), namespace, excludes...), - manifest.Parse(string(revisionResponse), namespace, excludes...), + manifest.Parse(string(releaseResponse), namespace, d.normalizeManifests, excludes...), + manifest.Parse(string(revisionResponse), namespace, d.normalizeManifests, excludes...), d.suppressedKinds, d.showSecrets, d.outputContext, @@ -152,8 +154,8 @@ func (d *rollback) backcast() error { // create a diff between the current manifest and the version of the manifest that a user is intended to rollback seenAnyChanges := diff.Manifests( - manifest.ParseRelease(releaseResponse.Release, d.includeTests), - manifest.ParseRelease(revisionResponse.Release, d.includeTests), + manifest.ParseRelease(releaseResponse.Release, d.includeTests, d.normalizeManifests), + manifest.ParseRelease(revisionResponse.Release, d.includeTests, d.normalizeManifests), d.suppressedKinds, d.showSecrets, d.outputContext, diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 8c0de4f6..08e5ac31 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -41,6 +41,7 @@ type diffCmd struct { output string install bool stripTrailingCR bool + normalizeManifests bool } func (d *diffCmd) isAllowUnreleased() bool { @@ -121,6 +122,7 @@ func newChartCommand() *cobra.Command { f.StringVar(&diff.postRenderer, "post-renderer", "", "the path to an executable to be used for post rendering. If it exists in $PATH, the binary will be used, otherwise it will try to look for the executable at the given path") f.StringVar(&diff.output, "output", "diff", "Possible values: diff, simple, json, template. When set to \"template\", use the env var HELM_DIFF_TPL to specify the template.") f.BoolVar(&diff.stripTrailingCR, "strip-trailing-cr", false, "strip trailing carriage return on input") + f.BoolVar(&diff.normalizeManifests, "normalize-manifests", false, "normalize manifests before running diff to exclude style differences from the output") if !isHelm3() { f.StringVar(&diff.namespace, "namespace", "default", "namespace to assume the release to be installed into") } @@ -177,16 +179,16 @@ func (d *diffCmd) runHelm3() error { releaseManifest = append(releaseManifest, hooks...) } if d.includeTests { - currentSpecs = manifest.Parse(string(releaseManifest), d.namespace) + currentSpecs = manifest.Parse(string(releaseManifest), d.namespace, d.normalizeManifests) } else { - currentSpecs = manifest.Parse(string(releaseManifest), d.namespace, helm3TestHook, helm2TestSuccessHook) + currentSpecs = manifest.Parse(string(releaseManifest), d.namespace, d.normalizeManifests, helm3TestHook, helm2TestSuccessHook) } } var newSpecs map[string]*manifest.MappingResult if d.includeTests { - newSpecs = manifest.Parse(string(installManifest), d.namespace) + newSpecs = manifest.Parse(string(installManifest), d.namespace, d.normalizeManifests) } else { - newSpecs = manifest.Parse(string(installManifest), d.namespace, helm3TestHook, helm2TestSuccessHook) + newSpecs = manifest.Parse(string(installManifest), d.namespace, d.normalizeManifests, helm3TestHook, helm2TestSuccessHook) } seenAnyChanges := diff.Manifests(currentSpecs, newSpecs, d.suppressedKinds, d.showSecrets, d.outputContext, d.output, d.stripTrailingCR, os.Stdout) @@ -251,7 +253,7 @@ func (d *diffCmd) run() error { } currentSpecs = make(map[string]*manifest.MappingResult) - newSpecs = manifest.Parse(installResponse.Release.Manifest, installResponse.Release.Namespace) + newSpecs = manifest.Parse(installResponse.Release.Manifest, installResponse.Release.Namespace, d.normalizeManifests) } else { upgradeResponse, err := d.client.UpdateRelease( d.release, @@ -266,11 +268,11 @@ func (d *diffCmd) run() error { } if d.noHooks { - currentSpecs = manifest.Parse(releaseResponse.Release.Manifest, releaseResponse.Release.Namespace) - newSpecs = manifest.Parse(upgradeResponse.Release.Manifest, upgradeResponse.Release.Namespace) + currentSpecs = manifest.Parse(releaseResponse.Release.Manifest, releaseResponse.Release.Namespace, d.normalizeManifests) + newSpecs = manifest.Parse(upgradeResponse.Release.Manifest, upgradeResponse.Release.Namespace, d.normalizeManifests) } else { - currentSpecs = manifest.ParseRelease(releaseResponse.Release, d.includeTests) - newSpecs = manifest.ParseRelease(upgradeResponse.Release, d.includeTests) + currentSpecs = manifest.ParseRelease(releaseResponse.Release, d.includeTests, d.normalizeManifests) + newSpecs = manifest.ParseRelease(upgradeResponse.Release, d.includeTests, d.normalizeManifests) } } diff --git a/manifest/parse.go b/manifest/parse.go index 712f5aba..ebfd9fec 100644 --- a/manifest/parse.go +++ b/manifest/parse.go @@ -68,7 +68,7 @@ func splitSpec(token string) (string, string) { } // ParseRelease parses release objects into MappingResult -func ParseRelease(release *release.Release, includeTests bool) map[string]*MappingResult { +func ParseRelease(release *release.Release, includeTests bool, normalizeManifests bool) map[string]*MappingResult { manifest := release.Manifest for _, hook := range release.Hooks { if !includeTests && isTestHook(hook.Events) { @@ -79,11 +79,11 @@ func ParseRelease(release *release.Release, includeTests bool) map[string]*Mappi manifest += fmt.Sprintf("# Source: %s\n", hook.Path) manifest += hook.Manifest } - return Parse(manifest, release.Namespace) + return Parse(manifest, release.Namespace, normalizeManifests) } // Parse parses manifest strings into MappingResult -func Parse(manifest string, defaultNamespace string, excludedHooks ...string) map[string]*MappingResult { +func Parse(manifest string, defaultNamespace string, normalizeManifests bool, excludedHooks ...string) map[string]*MappingResult { // Ensure we have a newline in front of the yaml seperator scanner := bufio.NewScanner(strings.NewReader("\n" + manifest)) scanner.Split(scanYamlSpecs) @@ -100,7 +100,7 @@ func Parse(manifest string, defaultNamespace string, excludedHooks ...string) ma continue } - parsed, err := parseContent(content, defaultNamespace, excludedHooks...) + parsed, err := parseContent(content, defaultNamespace, normalizeManifests, excludedHooks...) if err != nil { log.Fatalf("%v", err) } @@ -121,7 +121,7 @@ func Parse(manifest string, defaultNamespace string, excludedHooks ...string) ma return result } -func parseContent(content string, defaultNamespace string, excludedHooks ...string) ([]*MappingResult, error) { +func parseContent(content string, defaultNamespace string, normalizeManifests bool, excludedHooks ...string) ([]*MappingResult, error) { var parsedMetadata metadata if err := yaml.Unmarshal([]byte(content), &parsedMetadata); err != nil { log.Fatalf("YAML unmarshal error: %s\nCan't unmarshal %s", err, content) @@ -135,7 +135,7 @@ func parseContent(content string, defaultNamespace string, excludedHooks ...stri if parsedMetadata.Kind == "List" { type ListV1 struct { - Items []map[interface{}]interface{} `yaml:"items"` + Items []yaml.MapSlice `yaml:"items"` } var list ListV1 @@ -152,7 +152,7 @@ func parseContent(content string, defaultNamespace string, excludedHooks ...stri log.Printf("YAML marshal error: %s\nCan't marshal %v", err, item) } - subs, err := parseContent(string(subcontent), defaultNamespace, excludedHooks...) + subs, err := parseContent(string(subcontent), defaultNamespace, normalizeManifests, excludedHooks...) if err != nil { return nil, fmt.Errorf("Parsing YAML list item: %v", err) } @@ -163,15 +163,20 @@ func parseContent(content string, defaultNamespace string, excludedHooks ...stri return result, nil } - var object map[interface{}]interface{} - if err := yaml.Unmarshal([]byte(content), &object); err != nil { - log.Fatalf("YAML unmarshal error: %s\nCan't unmarshal %s", err, content) - } - normalizedContent, err := yaml.Marshal(object) - if err != nil { - log.Fatalf("YAML marshal error: %s\nCan't marshal %v", err, object) + if normalizeManifests { + // Unmarshal and marshal again content to normalize yaml structure + // This avoids style differences to show up as diffs but it can + // make the output different from the original template (since it is in normalized form) + var object map[interface{}]interface{} + if err := yaml.Unmarshal([]byte(content), &object); err != nil { + log.Fatalf("YAML unmarshal error: %s\nCan't unmarshal %s", err, content) + } + normalizedContent, err := yaml.Marshal(object) + if err != nil { + log.Fatalf("YAML marshal error: %s\nCan't marshal %v", err, object) + } + content = string(normalizedContent) } - content = string(normalizedContent) if isHook(parsedMetadata, excludedHooks...) { return nil, nil diff --git a/manifest/parse_test.go b/manifest/parse_test.go index c23e4bfd..14c6c2cc 100644 --- a/manifest/parse_test.go +++ b/manifest/parse_test.go @@ -25,7 +25,7 @@ func TestPod(t *testing.T) { require.Equal(t, []string{"default, nginx, Pod (v1)"}, - foundObjects(Parse(string(spec), "default")), + foundObjects(Parse(string(spec), "default", false)), ) } @@ -35,7 +35,7 @@ func TestPodNamespace(t *testing.T) { require.Equal(t, []string{"batcave, nginx, Pod (v1)"}, - foundObjects(Parse(string(spec), "default")), + foundObjects(Parse(string(spec), "default", false)), ) } @@ -45,17 +45,17 @@ func TestPodHook(t *testing.T) { require.Equal(t, []string{"default, nginx, Pod (v1)"}, - foundObjects(Parse(string(spec), "default")), + foundObjects(Parse(string(spec), "default", false)), ) require.Equal(t, []string{"default, nginx, Pod (v1)"}, - foundObjects(Parse(string(spec), "default", "test-success")), + foundObjects(Parse(string(spec), "default", false, "test-success")), ) require.Equal(t, []string{}, - foundObjects(Parse(string(spec), "default", "test")), + foundObjects(Parse(string(spec), "default", false, "test")), ) } @@ -65,7 +65,7 @@ func TestDeployV1(t *testing.T) { require.Equal(t, []string{"default, nginx, Deployment (apps)"}, - foundObjects(Parse(string(spec), "default")), + foundObjects(Parse(string(spec), "default", false)), ) } @@ -75,7 +75,7 @@ func TestDeployV1Beta1(t *testing.T) { require.Equal(t, []string{"default, nginx, Deployment (apps)"}, - foundObjects(Parse(string(spec), "default")), + foundObjects(Parse(string(spec), "default", false)), ) } @@ -88,7 +88,7 @@ func TestList(t *testing.T) { "default, prometheus-operator-example, PrometheusRule (monitoring.coreos.com)", "default, prometheus-operator-example2, PrometheusRule (monitoring.coreos.com)", }, - foundObjects(Parse(string(spec), "default")), + foundObjects(Parse(string(spec), "default", false)), ) } @@ -98,6 +98,6 @@ func TestEmpty(t *testing.T) { require.Equal(t, []string{}, - foundObjects(Parse(string(spec), "default")), + foundObjects(Parse(string(spec), "default", false)), ) }