diff --git a/cmd/release.go b/cmd/release.go index ebb00a0d..82fba635 100644 --- a/cmd/release.go +++ b/cmd/release.go @@ -109,9 +109,14 @@ func (d *release) differentiateHelm3() error { } if releaseChart1 == releaseChart2 { + oldSpecs := manifest.Parse(releaseResponse1, namespace1, d.normalizeManifests, excludes...) + newSpecs := manifest.Parse(releaseResponse2, namespace2, d.normalizeManifests, excludes...) + releaseResponse1 = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation + releaseResponse2 = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation + seenAnyChanges := diff.Releases( - manifest.Parse(string(releaseResponse1), namespace1, d.normalizeManifests, excludes...), - manifest.Parse(string(releaseResponse2), namespace2, d.normalizeManifests, excludes...), + oldSpecs, + newSpecs, &d.Options, os.Stdout) diff --git a/cmd/revision.go b/cmd/revision.go index 929a66d7..32d53904 100644 --- a/cmd/revision.go +++ b/cmd/revision.go @@ -99,9 +99,14 @@ func (d *revision) differentiateHelm3() error { return err } + oldSpecs := manifest.Parse(revisionResponse, namespace, d.normalizeManifests, excludes...) + newSpecs := manifest.Parse(releaseResponse, namespace, d.normalizeManifests, excludes...) + revisionResponse = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation + releaseResponse = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation + diff.Manifests( - manifest.Parse(string(revisionResponse), namespace, d.normalizeManifests, excludes...), - manifest.Parse(string(releaseResponse), namespace, d.normalizeManifests, excludes...), + oldSpecs, + newSpecs, &d.Options, os.Stdout) @@ -122,9 +127,14 @@ func (d *revision) differentiateHelm3() error { return err } + oldSpecs := manifest.Parse(revisionResponse1, namespace, d.normalizeManifests, excludes...) + newSpecs := manifest.Parse(revisionResponse2, namespace, d.normalizeManifests, excludes...) + revisionResponse1 = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation + revisionResponse2 = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation + seenAnyChanges := diff.Manifests( - manifest.Parse(string(revisionResponse1), namespace, d.normalizeManifests, excludes...), - manifest.Parse(string(revisionResponse2), namespace, d.normalizeManifests, excludes...), + oldSpecs, + newSpecs, &d.Options, os.Stdout) diff --git a/cmd/rollback.go b/cmd/rollback.go index 4e70ba1e..5d65f8ad 100644 --- a/cmd/rollback.go +++ b/cmd/rollback.go @@ -90,9 +90,14 @@ 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 + oldSpecs := manifest.Parse(releaseResponse, namespace, d.normalizeManifests, excludes...) + newSpecs := manifest.Parse(revisionResponse, namespace, d.normalizeManifests, excludes...) + releaseResponse = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation + revisionResponse = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation + seenAnyChanges := diff.Manifests( - manifest.Parse(string(releaseResponse), namespace, d.normalizeManifests, excludes...), - manifest.Parse(string(revisionResponse), namespace, d.normalizeManifests, excludes...), + oldSpecs, + newSpecs, &d.Options, os.Stdout) diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 00a4e4bf..f5e7dada 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -327,11 +327,12 @@ func (d *diffCmd) runHelm3() error { releaseManifest = append(releaseManifest, hooks...) } if d.includeTests { - currentSpecs = manifest.Parse(string(releaseManifest), d.namespace, d.normalizeManifests) + currentSpecs = manifest.Parse(releaseManifest, d.namespace, d.normalizeManifests) } else { - currentSpecs = manifest.Parse(string(releaseManifest), d.namespace, d.normalizeManifests, manifest.Helm3TestHook, manifest.Helm2TestSuccessHook) + currentSpecs = manifest.Parse(releaseManifest, d.namespace, d.normalizeManifests, manifest.Helm3TestHook, manifest.Helm2TestSuccessHook) } } + releaseManifest = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation var newOwnedReleases map[string]diff.OwnershipDiff if d.takeOwnership { @@ -347,10 +348,11 @@ func (d *diffCmd) runHelm3() error { var newSpecs map[string]*manifest.MappingResult if d.includeTests { - newSpecs = manifest.Parse(string(installManifest), d.namespace, d.normalizeManifests) + newSpecs = manifest.Parse(installManifest, d.namespace, d.normalizeManifests) } else { - newSpecs = manifest.Parse(string(installManifest), d.namespace, d.normalizeManifests, manifest.Helm3TestHook, manifest.Helm2TestSuccessHook) + newSpecs = manifest.Parse(installManifest, d.namespace, d.normalizeManifests, manifest.Helm3TestHook, manifest.Helm2TestSuccessHook) } + installManifest = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation seenAnyChanges := diff.ManifestsOwnership(currentSpecs, newSpecs, newOwnedReleases, &d.Options, os.Stdout) diff --git a/diff/diff.go b/diff/diff.go index d0ef4854..3cfedfa8 100644 --- a/diff/diff.go +++ b/diff/diff.go @@ -181,6 +181,11 @@ func actualChanges(diff []difflib.DiffRecord) int { return changes } +const ( + renameDetectionMinLengthRatio float32 = 0.1 + renameDetectionMaxLengthRatio float32 = 10.0 +) + func contentSearch(report *Report, possiblyRemoved []string, oldIndex map[string]*manifest.MappingResult, possiblyAdded []string, newIndex map[string]*manifest.MappingResult, options *Options) ([]string, []string) { if options.FindRenames <= 0 { return possiblyRemoved, possiblyAdded @@ -198,6 +203,21 @@ func contentSearch(report *Report, possiblyRemoved []string, oldIndex map[string continue } + oldLen := len(oldContent.Content) + newLen := len(newContent.Content) + if oldLen == 0 || newLen == 0 { + continue + } + // Skip the length-ratio filter for Secrets: their raw content length can + // differ greatly from the post-processed (redacted/decoded) length, so the + // ratio would be an unreliable predictor of content similarity. + if oldContent.Kind != "Secret" { + ratio := float32(oldLen) / float32(newLen) + if ratio < renameDetectionMinLengthRatio || ratio > renameDetectionMaxLengthRatio { + continue + } + } + switch { case options.ShowSecretsDecoded: decodeSecrets(oldContent, newContent) @@ -208,7 +228,7 @@ func contentSearch(report *Report, possiblyRemoved []string, oldIndex map[string diff := diffMappingResults(oldContent, newContent, options.StripTrailingCR) delta := actualChanges(diff) if delta == 0 || len(diff) == 0 { - continue // Should never happen, but better safe than sorry + continue } fraction := float32(delta) / float32(len(diff)) if fraction > 0 && fraction < smallestFraction { diff --git a/diff/diff_test.go b/diff/diff_test.go index 2e1efafc..b3965e80 100644 --- a/diff/diff_test.go +++ b/diff/diff_test.go @@ -617,8 +617,8 @@ spec: - name: app image: demo:v2 ` - oldIndex := manifest.Parse(oldManifest, "prod", true) - newIndex := manifest.Parse(newManifest, "prod", true) + oldIndex := manifest.Parse([]byte(oldManifest), "prod", true) + newIndex := manifest.Parse([]byte(newManifest), "prod", true) var buf bytes.Buffer changed := Manifests(oldIndex, newIndex, opts, &buf) @@ -656,7 +656,7 @@ metadata: namespace: ops spec: {} ` - newIndex := manifest.Parse(newManifest, "ops", true) + newIndex := manifest.Parse([]byte(newManifest), "ops", true) var buf bytes.Buffer changed := Manifests(map[string]*manifest.MappingResult{}, newIndex, opts, &buf) @@ -702,8 +702,8 @@ metadata: data: password: Zm9v ` - oldIndex := manifest.Parse(oldManifest, "default", true) - newIndex := manifest.Parse(newManifest, "default", true) + oldIndex := manifest.Parse([]byte(oldManifest), "default", true) + newIndex := manifest.Parse([]byte(newManifest), "default", true) var buf bytes.Buffer changed := Manifests(oldIndex, newIndex, opts, &buf) @@ -822,8 +822,8 @@ metadata: name: test namespace: default ` - oldIndex := manifest.Parse(emptyManifest, "default", true) - newIndex := manifest.Parse(validManifest, "default", true) + oldIndex := manifest.Parse([]byte(emptyManifest), "default", true) + newIndex := manifest.Parse([]byte(validManifest), "default", true) var buf bytes.Buffer changed := Manifests(oldIndex, newIndex, opts, &buf) @@ -846,8 +846,8 @@ metadata: name: test namespace: default ` - oldIndex := manifest.Parse(nullManifest, "default", true) - newIndex := manifest.Parse(validManifest, "default", true) + oldIndex := manifest.Parse([]byte(nullManifest), "default", true) + newIndex := manifest.Parse([]byte(validManifest), "default", true) var buf bytes.Buffer changed := Manifests(oldIndex, newIndex, opts, &buf) @@ -890,8 +890,8 @@ data: deeply: value: new ` - oldIndex := manifest.Parse(oldManifest, "default", true) - newIndex := manifest.Parse(newManifest, "default", true) + oldIndex := manifest.Parse([]byte(oldManifest), "default", true) + newIndex := manifest.Parse([]byte(newManifest), "default", true) var buf bytes.Buffer changed := Manifests(oldIndex, newIndex, opts, &buf) @@ -944,8 +944,8 @@ spec: - name: KEY2 value: val2 ` - oldIndex := manifest.Parse(oldManifest, "prod", true) - newIndex := manifest.Parse(newManifest, "prod", true) + oldIndex := manifest.Parse([]byte(oldManifest), "prod", true) + newIndex := manifest.Parse([]byte(newManifest), "prod", true) var buf bytes.Buffer changed := Manifests(oldIndex, newIndex, opts, &buf) @@ -967,8 +967,8 @@ spec: emptyManifest1 := `` emptyManifest2 := `` - oldIndex := manifest.Parse(emptyManifest1, "default", true) - newIndex := manifest.Parse(emptyManifest2, "default", true) + oldIndex := manifest.Parse([]byte(emptyManifest1), "default", true) + newIndex := manifest.Parse([]byte(emptyManifest2), "default", true) var buf bytes.Buffer changed := Manifests(oldIndex, newIndex, opts, &buf) @@ -1593,3 +1593,128 @@ data: require.Contains(t, new.Content, "key1: '++++++++ # (6 bytes)'") }) } + +func TestRenameDetectionLengthRatio(t *testing.T) { + ansi.DisableColors(true) + + makeSpec := func(name string, content string) map[string]*manifest.MappingResult { + return map[string]*manifest.MappingResult{ + name: { + Name: name, + Kind: "Deployment", + Content: content, + }, + } + } + + shortContent := ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: short +spec: + replicas: 1 +` + + shortContentRenamed := ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: short-renamed +spec: + replicas: 1 +` + + longContent := ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: very-long +spec: + replicas: 1 + template: + spec: + containers: + - name: app + image: myapp:v1 + ports: + - containerPort: 8080 + env: + - name: VAR1 + value: "hello" + - name: VAR2 + value: "world" + - name: VAR3 + value: "foo" + - name: VAR4 + value: "bar" + - name: VAR5 + value: "baz" + resources: + limits: + cpu: "1" + memory: "1Gi" +` + + t.Run("similar length detects rename", func(t *testing.T) { + var buf bytes.Buffer + opts := &Options{OutputFormat: "diff", OutputContext: 10, ShowSecrets: true, FindRenames: 0.5} + + oldSpec := makeSpec("default, short, Deployment (apps)", shortContent) + newSpec := makeSpec("default, short-renamed, Deployment (apps)", shortContentRenamed) + + changed := Manifests(oldSpec, newSpec, opts, &buf) + require.True(t, changed) + require.Contains(t, buf.String(), "default, short, Deployment (apps) has changed") + }) + + t.Run("very different length skips rename", func(t *testing.T) { + var buf bytes.Buffer + opts := &Options{OutputFormat: "diff", OutputContext: 10, ShowSecrets: true, FindRenames: 0.5} + + oldSpec := makeSpec("default, short, Deployment (apps)", shortContent) + newSpec := makeSpec("default, very-long, Deployment (apps)", longContent) + + changed := Manifests(oldSpec, newSpec, opts, &buf) + require.True(t, changed) + require.Contains(t, buf.String(), "default, short, Deployment (apps) has been removed") + require.Contains(t, buf.String(), "default, very-long, Deployment (apps) has been added") + }) + + t.Run("empty content skipped", func(t *testing.T) { + var buf bytes.Buffer + opts := &Options{OutputFormat: "diff", OutputContext: 10, ShowSecrets: true, FindRenames: 0.5} + + oldSpec := map[string]*manifest.MappingResult{ + "default, empty, Deployment (apps)": { + Name: "default, empty, Deployment (apps)", + Kind: "Deployment", + Content: "", + }, + } + newSpec := makeSpec("default, short-renamed, Deployment (apps)", shortContentRenamed) + + changed := Manifests(oldSpec, newSpec, opts, &buf) + require.True(t, changed) + require.Contains(t, buf.String(), "has been added") + }) + + t.Run("different kind skipped", func(t *testing.T) { + var buf bytes.Buffer + opts := &Options{OutputFormat: "diff", OutputContext: 10, ShowSecrets: true, FindRenames: 0.5} + + oldSpec := map[string]*manifest.MappingResult{ + "default, svc, Service (v1)": { + Name: "default, svc, Service (v1)", + Kind: "Service", + Content: shortContent, + }, + } + newSpec := makeSpec("default, svc-renamed, Deployment (apps)", shortContentRenamed) + + changed := Manifests(oldSpec, newSpec, opts, &buf) + require.True(t, changed) + require.Contains(t, buf.String(), "has been removed") + require.Contains(t, buf.String(), "has been added") + }) +} diff --git a/manifest/parse.go b/manifest/parse.go index aae6c71d..c0228d3e 100644 --- a/manifest/parse.go +++ b/manifest/parse.go @@ -4,6 +4,7 @@ import ( "bufio" "bytes" "fmt" + "io" "log" "strings" @@ -68,10 +69,9 @@ func scanYamlSpecs(data []byte, atEOF bool) (advance int, token []byte, err erro return 0, nil, nil } -// Parse parses manifest strings into MappingResult -func Parse(manifest string, defaultNamespace string, normalizeManifests bool, excludedHooks ...string) map[string]*MappingResult { - // Ensure we have a newline in front of the yaml separator - scanner := bufio.NewScanner(strings.NewReader("\n" + manifest)) +// Parse parses manifest bytes into MappingResult +func Parse(manifest []byte, defaultNamespace string, normalizeManifests bool, excludedHooks ...string) map[string]*MappingResult { + scanner := bufio.NewScanner(io.MultiReader(strings.NewReader("\n"), bytes.NewReader(manifest))) scanner.Split(scanYamlSpecs) // Allow for tokens (specs) up to 10MiB in size scanner.Buffer(make([]byte, bufio.MaxScanTokenSize), 10485760) @@ -79,8 +79,8 @@ func Parse(manifest string, defaultNamespace string, normalizeManifests bool, ex result := make(map[string]*MappingResult) for scanner.Scan() { - content := strings.TrimSpace(scanner.Text()) - if content == "" { + content := bytes.TrimSpace(scanner.Bytes()) + if len(content) == 0 { continue } @@ -133,7 +133,7 @@ func ParseObject(object runtime.Object, defaultNamespace string, excludedHooks . return nil, "", err } - result, err := parseContent(string(content), defaultNamespace, true, excludedHooks...) + result, err := parseContent(content, defaultNamespace, true, excludedHooks...) if err != nil { return nil, "", err } @@ -147,9 +147,9 @@ func ParseObject(object runtime.Object, defaultNamespace string, excludedHooks . return result[0], oldRelease, nil } -func parseContent(content string, defaultNamespace string, normalizeManifests bool, excludedHooks ...string) ([]*MappingResult, error) { +func parseContent(content []byte, defaultNamespace string, normalizeManifests bool, excludedHooks ...string) ([]*MappingResult, error) { var parsedMetadata metadata - if err := yaml.Unmarshal([]byte(content), &parsedMetadata); err != nil { + if err := yaml.Unmarshal(content, &parsedMetadata); err != nil { log.Fatalf("YAML unmarshal error: %s\nCan't unmarshal %s", err, content) } @@ -166,7 +166,7 @@ func parseContent(content string, defaultNamespace string, normalizeManifests bo var list ListV1 - if err := yaml.Unmarshal([]byte(content), &list); err != nil { + if err := yaml.Unmarshal(content, &list); err != nil { log.Fatalf("YAML unmarshal error: %s\nCan't unmarshal %s", err, content) } @@ -178,7 +178,7 @@ func parseContent(content string, defaultNamespace string, normalizeManifests bo log.Printf("YAML marshal error: %s\nCan't marshal %v", err, item) } - subs, err := parseContent(string(subcontent), defaultNamespace, normalizeManifests, excludedHooks...) + subs, err := parseContent(subcontent, defaultNamespace, normalizeManifests, excludedHooks...) if err != nil { return nil, fmt.Errorf("Parsing YAML list item: %w", err) } @@ -190,18 +190,15 @@ func parseContent(content string, defaultNamespace string, normalizeManifests bo } 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 { + if err := yaml.Unmarshal(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 = normalizedContent } if isHook(parsedMetadata, excludedHooks...) { @@ -217,7 +214,7 @@ func parseContent(content string, defaultNamespace string, normalizeManifests bo { Name: name, Kind: parsedMetadata.Kind, - Content: content, + Content: string(content), ResourcePolicy: parsedMetadata.Metadata.Annotations[resourcePolicyAnnotation], }, }, nil diff --git a/manifest/parse_test.go b/manifest/parse_test.go index eef3ea5f..efe50fd5 100644 --- a/manifest/parse_test.go +++ b/manifest/parse_test.go @@ -27,7 +27,7 @@ func TestPod(t *testing.T) { require.Equal(t, []string{"default, nginx, Pod (v1)"}, - foundObjects(Parse(string(spec), "default", false)), + foundObjects(Parse(spec, "default", false)), ) } @@ -37,7 +37,7 @@ func TestPodNamespace(t *testing.T) { require.Equal(t, []string{"batcave, nginx, Pod (v1)"}, - foundObjects(Parse(string(spec), "default", false)), + foundObjects(Parse(spec, "default", false)), ) } @@ -47,17 +47,17 @@ func TestPodHook(t *testing.T) { require.Equal(t, []string{"default, nginx, Pod (v1)"}, - foundObjects(Parse(string(spec), "default", false)), + foundObjects(Parse(spec, "default", false)), ) require.Equal(t, []string{"default, nginx, Pod (v1)"}, - foundObjects(Parse(string(spec), "default", false, "test-success")), + foundObjects(Parse(spec, "default", false, "test-success")), ) require.Equal(t, []string{}, - foundObjects(Parse(string(spec), "default", false, "test")), + foundObjects(Parse(spec, "default", false, "test")), ) } @@ -67,7 +67,7 @@ func TestDeployV1(t *testing.T) { require.Equal(t, []string{"default, nginx, Deployment (apps)"}, - foundObjects(Parse(string(spec), "default", false)), + foundObjects(Parse(spec, "default", false)), ) } @@ -77,7 +77,7 @@ func TestDeployV1Beta1(t *testing.T) { require.Equal(t, []string{"default, nginx, Deployment (apps)"}, - foundObjects(Parse(string(spec), "default", false)), + foundObjects(Parse(spec, "default", false)), ) } @@ -90,7 +90,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", false)), + foundObjects(Parse(spec, "default", false)), ) } @@ -103,7 +103,7 @@ func TestConfigMapList(t *testing.T) { "default, configmap-2-1, ConfigMap (v1)", "default, configmap-2-2, ConfigMap (v1)", }, - foundObjects(Parse(string(spec), "default", false)), + foundObjects(Parse(spec, "default", false)), ) } @@ -116,7 +116,7 @@ func TestSecretList(t *testing.T) { "default, my-secret-1, Secret (v1)", "default, my-secret-2, Secret (v1)", }, - foundObjects(Parse(string(spec), "default", false)), + foundObjects(Parse(spec, "default", false)), ) } @@ -126,7 +126,7 @@ func TestEmpty(t *testing.T) { require.Equal(t, []string{}, - foundObjects(Parse(string(spec), "default", false)), + foundObjects(Parse(spec, "default", false)), ) } @@ -136,7 +136,7 @@ func TestBaseNameAnnotation(t *testing.T) { require.Equal(t, []string{"default, bat-secret, Secret (v1)"}, - foundObjects(Parse(string(spec), "default", false)), + foundObjects(Parse(spec, "default", false)), ) }