From be60c9f58489bef5adbf1c0aa5922bd6a6722e81 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Wed, 9 Sep 2026 22:04:47 +0800 Subject: [PATCH 1/2] fix: drop null-valued labels/annotations keys in metadata tidy Charts commonly render a bare `labels:` (null) key when a conditional labels block is empty. deleteStatusAndTidyMetadata only pruned map-typed values, so a null labels/annotations key survived pruning and showed up as a confusing "- labels:" diff entry against the other side that has no such key. pruneNestedMap now removes the key when its value is null, in addition to removing it when the pruned map ends up empty. Also adds a CI reproduction script exercising no-labels charts, dropped label chart versions, bare/null labels blocks, custom resources, flux-style labels, --take-ownership, --dry-run=server and empty labels maps against a real kind cluster. Fixes #1064 Signed-off-by: yxxhero --- .github/workflows/ci.yaml | 3 + manifest/util.go | 11 +- manifest/util_test.go | 69 +++++++++ scripts/repro-issue-1064.sh | 292 ++++++++++++++++++++++++++++++++++++ 4 files changed, 374 insertions(+), 1 deletion(-) create mode 100755 scripts/repro-issue-1064.sh 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/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..561d6f93 --- /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 + mkdir -p "$1/templates" + cat > "$1/Chart.yaml" <<'YAML' +apiVersion: v2 +name: issue1064 +version: 0.1.0 +YAML + cat > "$1/templates/res.yaml" <<< "$2" +} + +kubectl create namespace "$NS" 2>/dev/null || true + +############################################################################### +# Variant A: chart resource without any labels +############################################################################### +chart "$WORK/a" 'apiVersion: v1 +kind: ConfigMap +metadata: + name: res-a +data: + foo: bar +' +helm upgrade -i rel-a "$WORK/a" -n "$NS" >/dev/null +echo "===== live object labels (A) =====" +kubectl get configmap res-a -n "$NS" -o jsonpath='{.metadata.labels}'; echo + +run_diff "A plain" noassert rel-a "$WORK/a" -n "$NS" +run_diff "A three-way" assert rel-a "$WORK/a" -n "$NS" --three-way-merge + +############################################################################### +# Variant B: release installed with explicit managed-by label, new chart +# version drops the label. The three-way diff must not show any labels noise +# (a real helm upgrade re-adds managed-by, and helm-diff prunes it). +############################################################################### +chart "$WORK/b1" 'apiVersion: v1 +kind: ConfigMap +metadata: + name: res-b + labels: + app.kubernetes.io/managed-by: Helm +data: + foo: bar +' +chart "$WORK/b2" 'apiVersion: v1 +kind: ConfigMap +metadata: + name: res-b +data: + foo: bar +' +helm upgrade -i rel-b "$WORK/b1" -n "$NS" >/dev/null +echo "===== live object labels (B) =====" +kubectl get configmap res-b -n "$NS" -o jsonpath='{.metadata.labels}'; echo + +run_diff "B plain" noassert rel-b "$WORK/b2" -n "$NS" +run_diff "B three-way" assert rel-b "$WORK/b2" -n "$NS" --three-way-merge + +############################################################################### +# Variant C: chart renders a bare `labels:` (null) key, new chart version +# removes the labels block entirely. +############################################################################### +chart "$WORK/c1" 'apiVersion: v1 +kind: ConfigMap +metadata: + name: res-c + labels: +data: + foo: bar +' +chart "$WORK/c2" 'apiVersion: v1 +kind: ConfigMap +metadata: + name: res-c +data: + foo: bar +' +helm upgrade -i rel-c "$WORK/c1" -n "$NS" >/dev/null +echo "===== live object labels (C) =====" +kubectl get configmap res-c -n "$NS" -o jsonpath='{.metadata.labels}'; echo + +run_diff "C plain" noassert rel-c "$WORK/c2" -n "$NS" +run_diff "C three-way" assert rel-c "$WORK/c2" -n "$NS" --three-way-merge + +############################################################################### +# Variant D: custom resource (unstructured / merge-patch path) without labels, +# mirroring the ExternalSecret from the issue. +############################################################################### +cat <<'YAML' | kubectl apply -f - >/dev/null +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: widgets.issue1064.com +spec: + group: issue1064.com + names: + kind: Widget + plural: widgets + singular: widget + listKind: WidgetList + scope: Namespaced + versions: + - name: v1 + served: true + storage: true + schema: + openAPIV3Schema: + type: object + properties: + spec: + type: object + properties: + foo: + type: string +YAML +kubectl wait --for=condition=Established crd/widgets.issue1064.com --timeout=120s >/dev/null + +chart "$WORK/d" 'apiVersion: issue1064.com/v1 +kind: Widget +metadata: + name: res-d + finalizers: + - issue1064.com/cleanup +spec: + foo: bar +' +helm upgrade -i rel-d "$WORK/d" -n "$NS" >/dev/null +echo "===== live object labels (D) =====" +kubectl get widget res-d -n "$NS" -o jsonpath='{.metadata.labels}'; echo + +run_diff "D plain" noassert rel-d "$WORK/d" -n "$NS" +run_diff "D three-way" assert rel-d "$WORK/d" -n "$NS" --three-way-merge + +############################################################################### +# Variant E: flux-style labels on the live object (stripped since the fluxcd +# fix), on top of the managed-by label. +############################################################################### +kubectl label configmap res-a -n "$NS" \ + helm.toolkit.fluxcd.io/name=rel-a \ + helm.toolkit.fluxcd.io/namespace="$NS" \ + --overwrite >/dev/null +echo "===== live object labels (E) =====" +kubectl get configmap res-a -n "$NS" -o jsonpath='{.metadata.labels}'; echo + +run_diff "E plain" noassert rel-a "$WORK/a" -n "$NS" +run_diff "E three-way" assert rel-a "$WORK/a" -n "$NS" --three-way-merge + +############################################################################### +# Variant F: --take-ownership path (ParseObject prunes live objects too) +############################################################################### +echo "===== live object labels (F: same as A) =====" +run_diff "F take-ownership" assert rel-a "$WORK/a" -n "$NS" --take-ownership + +############################################################################### +# Variant G: --dry-run=server template mode against variant B charts +############################################################################### +run_diff "G dry-run-server" assert rel-b "$WORK/b2" -n "$NS" --dry-run=server --three-way-merge +run_diff "G2 dry-run-server plain" noassert rel-b "$WORK/b2" -n "$NS" --dry-run=server + +############################################################################### +# Variant H: chart renders an explicit empty labels map `labels: {}` +############################################################################### +chart "$WORK/h1" 'apiVersion: v1 +kind: ConfigMap +metadata: + name: res-h + labels: {} +data: + foo: bar +' +chart "$WORK/h2" 'apiVersion: v1 +kind: ConfigMap +metadata: + name: res-h +data: + foo: bar +' +helm upgrade -i rel-h "$WORK/h1" -n "$NS" >/dev/null +echo "===== live object labels (H) =====" +kubectl get configmap res-h -n "$NS" -o jsonpath='{.metadata.labels}'; echo + +run_diff "H plain" noassert rel-h "$WORK/h2" -n "$NS" +run_diff "H three-way" assert rel-h "$WORK/h2" -n "$NS" --three-way-merge + +############################################################################### +echo "" +if [ "$FAIL" -ne 0 ]; then + echo "issue 1064 reproduced: labels-only diff entries found" + exit 1 +fi +echo "no labels-only diff entries found" From 65ed118f7464bfb702eb271f5faea7d4b181f420 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Wed, 9 Sep 2026 22:22:27 +0800 Subject: [PATCH 2/2] fix: strip null/empty metadata.labels/annotations keys in parsed manifests Kubernetes treats a null or empty labels/annotations map exactly like an absent key, but charts frequently render the empty key anyway (e.g. via a conditional labels block that renders a bare "labels:" line). A raw textual diff between such a manifest and one that omits the key reports a meaningless change: metadata: finalizers: - externalsecrets.external-secrets.io/externalsecret-cleanup - labels: name: webhook-token This is the confusion reported in #1064: after the managed-by label is pruned, the leftover empty key still shows as "- labels:". stripEmptyMetadataKeys now drops "labels:"/"annotations:" lines from each parsed manifest whenever the parsed value is null or an empty mapping. The removal is line-based so surrounding text keeps its original formatting (# Source comments, key order, indentation); the parsed document is consulted only to confirm the key really carries no content, so keys with entries (including nested spec.template.metadata labels) are untouched. The CI reproduction script now asserts that the plain diffs for the bare-null-labels and empty-flow-map-labels chart variants stay clean. Fixes #1064 Signed-off-by: yxxhero --- manifest/parse.go | 106 ++++++++++++++++++++++++++++++++++++ manifest/parse_test.go | 91 +++++++++++++++++++++++++++++++ scripts/repro-issue-1064.sh | 4 +- 3 files changed, 199 insertions(+), 2 deletions(-) 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/scripts/repro-issue-1064.sh b/scripts/repro-issue-1064.sh index 561d6f93..267c1128 100755 --- a/scripts/repro-issue-1064.sh +++ b/scripts/repro-issue-1064.sh @@ -180,7 +180,7 @@ helm upgrade -i rel-c "$WORK/c1" -n "$NS" >/dev/null echo "===== live object labels (C) =====" kubectl get configmap res-c -n "$NS" -o jsonpath='{.metadata.labels}'; echo -run_diff "C plain" noassert rel-c "$WORK/c2" -n "$NS" +run_diff "C plain" assert rel-c "$WORK/c2" -n "$NS" run_diff "C three-way" assert rel-c "$WORK/c2" -n "$NS" --three-way-merge ############################################################################### @@ -280,7 +280,7 @@ helm upgrade -i rel-h "$WORK/h1" -n "$NS" >/dev/null echo "===== live object labels (H) =====" kubectl get configmap res-h -n "$NS" -o jsonpath='{.metadata.labels}'; echo -run_diff "H plain" noassert rel-h "$WORK/h2" -n "$NS" +run_diff "H plain" assert rel-h "$WORK/h2" -n "$NS" run_diff "H three-way" assert rel-h "$WORK/h2" -n "$NS" --three-way-merge ###############################################################################