diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 454127c8..31c977af 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -196,3 +196,6 @@ jobs: set -o pipefail helm diff upgrade storage-diff ./helm-diff -n prod-apps --storage-namespace flux-system --three-way-merge --set replicaCount=2 | tee /tmp/three-way.out grep -q 'replicas: 2' /tmp/three-way.out + + - name: Reproduce issue 1064 (labels-only diff noise) + run: scripts/repro-issue-1064.sh diff --git a/manifest/parse.go b/manifest/parse.go index c6277f01..d2ce8836 100644 --- a/manifest/parse.go +++ b/manifest/parse.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "log" + "regexp" "strings" jsoniter "github.com/json-iterator/go" @@ -20,6 +21,105 @@ const ( var yamlSeparator = []byte("\n---\n") +// metadataLineRegex matches the top-level `metadata:` key of a manifest. +var metadataLineRegex = regexp.MustCompile(`^metadata:[ \t]*(#.*)?$`) + +// stripEmptyMetadataKeys removes `labels:`/`annotations:` lines from content +// when their value is null or an empty mapping. +// +// Kubernetes treats a null or empty labels/annotations map exactly like an +// absent one, but charts frequently render the (empty) key anyway, e.g. via a +// conditional block: +// +// metadata: +// name: example +// labels: +// +// A raw textual diff between such a manifest and one that omits the key +// reports a meaningless `- labels:` change. The removal is done line-based so +// the surrounding text keeps its original formatting; the parsed document is +// only consulted to confirm that the key really is null/empty (a `labels:` +// line followed by indented entries is left alone). +// +// See https://github.com/databus23/helm-diff/issues/1064 +func stripEmptyMetadataKeys(content []byte) []byte { + var doc map[interface{}]interface{} + if err := yaml.Unmarshal(content, &doc); err != nil { + return content + } + + metadata, ok := doc["metadata"].(map[interface{}]interface{}) + if !ok { + return content + } + + strip := map[string]bool{} + for _, key := range []string{"labels", "annotations"} { + value, exists := metadata[key] + if !exists { + continue + } + if value == nil { + strip[key] = true + continue + } + if m, ok := value.(map[interface{}]interface{}); ok && len(m) == 0 { + strip[key] = true + } + } + if len(strip) == 0 { + return content + } + + lines := strings.Split(string(content), "\n") + + // Determine the indentation of metadata's direct children by looking at + // the first non-blank line below the top-level `metadata:` key. + childIndent := -1 + for i, line := range lines { + if !metadataLineRegex.MatchString(line) { + continue + } + for j := i + 1; j < len(lines); j++ { + if strings.TrimSpace(lines[j]) == "" { + continue + } + if indent := len(lines[j]) - len(strings.TrimLeft(lines[j], " \t")); indent > 0 { + childIndent = indent + } + break + } + break + } + if childIndent <= 0 { + return content + } + + keyLine := func(key string) *regexp.Regexp { + return regexp.MustCompile(fmt.Sprintf(`^ {%d}%s:[ \t]*(\{\}[ \t]*)?(#.*)?$`, childIndent, key)) + } + regexes := make([]*regexp.Regexp, 0, len(strip)) + for key := range strip { + regexes = append(regexes, keyLine(key)) + } + + kept := make([]string, 0, len(lines)) + for _, line := range lines { + drop := false + for _, re := range regexes { + if re.MatchString(line) { + drop = true + break + } + } + if !drop { + kept = append(kept, line) + } + } + + return []byte(strings.Join(kept, "\n")) +} + // MappingResult to store result of diff type MappingResult struct { Name string @@ -197,6 +297,12 @@ func parseContent(content []byte, defaultNamespace string, normalizeManifests bo } } + // Remove `labels:`/`annotations:` keys that are null or empty: they are + // semantically identical to an absent key, yet a textual diff between a + // manifest rendering the empty key and one omitting it would otherwise + // report a meaningless change (#1064). + content = stripEmptyMetadataKeys(content) + if isHook(parsedMetadata, excludedHooks...) { return nil, nil } diff --git a/manifest/parse_test.go b/manifest/parse_test.go index efe50fd5..45e8a513 100644 --- a/manifest/parse_test.go +++ b/manifest/parse_test.go @@ -3,6 +3,7 @@ package manifest_test import ( "os" "sort" + "strings" "testing" "github.com/stretchr/testify/require" @@ -31,6 +32,96 @@ func TestPod(t *testing.T) { ) } +func TestParseStripsEmptyMetadataKeys(t *testing.T) { + tests := []struct { + name string + manifest string + wantName string + want []string + }{ + { + name: "null labels line is removed", + manifest: `# Source: chart/templates/cm.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: example + labels: +data: + foo: bar +`, + wantName: "default, example, ConfigMap (v1)", + want: []string{"# Source: chart/templates/cm.yaml", "apiVersion: v1", "kind: ConfigMap", "metadata:", " name: example", "data:", " foo: bar"}, + }, + { + name: "empty flow labels map is removed", + manifest: `apiVersion: v1 +kind: ConfigMap +metadata: + name: example + labels: {} +data: + foo: bar +`, + wantName: "default, example, ConfigMap (v1)", + want: []string{"apiVersion: v1", "kind: ConfigMap", "metadata:", " name: example", "data:", " foo: bar"}, + }, + { + name: "null annotations line is removed", + manifest: `apiVersion: v1 +kind: ConfigMap +metadata: + name: example + annotations: +data: + foo: bar +`, + wantName: "default, example, ConfigMap (v1)", + want: []string{"apiVersion: v1", "kind: ConfigMap", "metadata:", " name: example", "data:", " foo: bar"}, + }, + { + name: "labels with content are kept", + manifest: `apiVersion: v1 +kind: ConfigMap +metadata: + name: example + labels: + app: kept +data: + foo: bar +`, + wantName: "default, example, ConfigMap (v1)", + want: []string{"apiVersion: v1", "kind: ConfigMap", "metadata:", " name: example", " labels:", " app: kept", "data:", " foo: bar"}, + }, + { + name: "nested spec template labels are kept", + manifest: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: example + labels: +spec: + template: + metadata: + labels: + app: kept +`, + wantName: "default, example, Deployment (apps)", + want: []string{"apiVersion: apps/v1", "kind: Deployment", "metadata:", " name: example", "spec:", " template:", " metadata:", " labels:", " app: kept"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := Parse([]byte(tt.manifest), "default", false) + require.Len(t, result, 1) + mapping, ok := result[tt.wantName] + require.True(t, ok, "expected resource %q in %v", tt.wantName, foundObjects(result)) + require.Equal(t, tt.want, strings.Split(mapping.Content, "\n")) + }) + } +} + func TestPodNamespace(t *testing.T) { spec, err := os.ReadFile("testdata/pod_namespace.yaml") require.NoError(t, err) diff --git a/manifest/util.go b/manifest/util.go index a599fbca..151220b2 100644 --- a/manifest/util.go +++ b/manifest/util.go @@ -42,10 +42,19 @@ func deleteStatusAndTidyMetadata(obj []byte) (map[string]interface{}, error) { // pruneNestedMap removes the given fields from the nested map found at key in // target. If the nested map ends up empty afterwards, key itself is removed -// from target. +// from target. A null-valued key (e.g. a chart template rendering "labels:" +// with nothing under it) is removed as well, so it does not show up as a +// confusing "- labels:" diff entry. See +// https://github.com/databus23/helm-diff/issues/1064 func pruneNestedMap(target map[string]interface{}, key string, fields ...string) { sub, ok := target[key].(map[string]interface{}) if !ok { + // The key is either absent or explicitly null. A null value (JSON + // "labels": null) would otherwise survive as an empty "labels:" entry + // in the rendered YAML and produce a meaningless diff. + if target[key] == nil { + delete(target, key) + } return } diff --git a/manifest/util_test.go b/manifest/util_test.go index 6e344366..bfe552f3 100644 --- a/manifest/util_test.go +++ b/manifest/util_test.go @@ -111,6 +111,75 @@ func Test_deleteStatusAndTidyMetadata(t *testing.T) { }, wantErr: false, }, + { + name: "null labels are removed (chart renders bare labels: key)", + obj: []byte(` +{ + "kind": "ConfigMap", + "metadata": { + "labels": null, + "name": "example" + } +} +`), + want: map[string]interface{}{ + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "example", + }, + }, + wantErr: false, + }, + { + name: "null annotations are removed", + obj: []byte(` +{ + "kind": "ConfigMap", + "metadata": { + "annotations": null, + "labels": { + "app": "kept" + }, + "name": "example" + } +} +`), + want: map[string]interface{}{ + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "app": "kept", + }, + "name": "example", + }, + }, + wantErr: false, + }, + { + name: "labels with other keys are kept", + obj: []byte(` +{ + "kind": "ConfigMap", + "metadata": { + "labels": { + "app.kubernetes.io/managed-by": "Helm", + "app.kubernetes.io/name": "myapp" + }, + "name": "example" + } +} +`), + want: map[string]interface{}{ + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "app.kubernetes.io/name": "myapp", + }, + "name": "example", + }, + }, + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/scripts/repro-issue-1064.sh b/scripts/repro-issue-1064.sh new file mode 100755 index 00000000..267c1128 --- /dev/null +++ b/scripts/repro-issue-1064.sh @@ -0,0 +1,292 @@ +#!/usr/bin/env bash +# Reproduction script for https://github.com/databus23/helm-diff/issues/1064 +# +# Bug: "helm-diff shows `- labels`, if resource contains only +# `app.kubernetes.io/managed-by` label". +# +# After the managed-by label is pruned from the diff, the leftover empty (or +# null) `labels` key must not show up as a confusing `- labels:` diff entry. +# +# This script exercises several chart shapes against a real cluster: +# A. a chart whose resource has no labels at all +# B. a release installed with an explicit managed-by label, then diffed +# against a chart version that dropped the label +# C. a chart rendering a bare `labels:` (null) key, then a chart version +# that removed the labels block entirely +# D. a custom resource (unstructured/CRD path, like the ExternalSecret from +# the issue) without labels +# E. flux-style extra labels on the live object +# +# Every helm diff invocation prints its full output so the CI log shows the +# actual behavior. Plain (text) diffs are informational: a chart that really +# dropped a labels block legitimately shows a textual change. The assertion +# targets three-way-merge diffs, which fetch live objects and must not report +# any labels-only change (a labels key with no content left after pruning). + +set -euo pipefail + +NS="d1064" +WORK="$(mktemp -d)" +FAIL=0 + +strip_ansi() { + sed 's/\x1b\[[0-9;]*m//g' +} + +# Detect labels-only diff entries: a +/- line whose payload is `labels:`, +# `labels: null` or `labels: {}` and whose following line is not an indented +# child entry (which would make it a legitimate multi-line label change). +find_symptoms() { + awk ' + { lines[NR] = $0 } + END { + for (i = 1; i <= NR; i++) { + line = lines[i] + if (line !~ /^[+-][[:space:]]*labels:([[:space:]]+(null|\{\}))?[[:space:]]*$/) + continue + payload = line + sub(/^[+-]/, "", payload) + val = payload + sub(/^[[:space:]]*labels:/, "", val) + gsub(/^[[:space:]]+|[[:space:]]+$/, "", val) + if (val == "null" || val == "{}") { + printf " line %d: %s\n", i, line + continue + } + # bare `labels:` key: only a symptom when no child entry follows + match(payload, /^[[:space:]]*/); curind = RLENGTH + nxt = lines[i + 1] + if (nxt != "") { + nprefix = substr(nxt, 1, 1) + nrest = substr(nxt, 2) + if (nprefix == "+" || nprefix == "-" || nprefix == " ") { + match(nrest, /^[[:space:]]*/) + if (RLENGTH > curind) + continue + } + } + printf " line %d: %s\n", i, line + } + }' "$1" +} + +check_no_symptom() { + local output_file="$1" scenario="$2" + strip_ansi < "$output_file" > "$WORK/stripped.out" + local symptoms + symptoms="$(find_symptoms "$WORK/stripped.out")" + if [ -n "$symptoms" ]; then + echo "FAIL: scenario [$scenario] shows a labels-only diff (#1064):" + echo "$symptoms" + FAIL=1 + else + echo "OK: scenario [$scenario] has no labels-only diff" + fi +} + +run_diff() { + local scenario="$1" assert="$2"; shift 2 + local out="$WORK/${scenario// /_}.out" + echo "" + echo "===== helm diff $* [$scenario] =====" + set +e + helm diff upgrade "$@" > "$out" 2>&1 + local rc="$?" + set -e + strip_ansi < "$out" + echo "----- exit code: $rc -----" + if [ "$assert" = "assert" ]; then + check_no_symptom "$out" "$scenario" + fi +} + +chart() { # chart