From 982f621def8da2a4e3267cff1082c6e850309518 Mon Sep 17 00:00:00 2001 From: Jared Hicks Date: Thu, 5 Nov 2026 16:20:26 -0500 Subject: [PATCH 1/2] fix: keep metadata added outside the chart when it renders a null map helm install, upgrade and rollback merge the release's ownership metadata (meta.helm.sh/release-name, meta.helm.sh/release-namespace and the app.kubernetes.io/managed-by label) into every object before computing the three-way patch. helm-diff computed the patch from the rendered manifest alone, so when a chart renders `annotations:` or `labels:` with nothing under it, the patch nulled the whole map. The server-side dry-run then dropped every annotation or label added outside the chart and reported their removal, while the real upgrade keeps them. Add the same ownership metadata to the new manifest before createPatch, via a WithRelease option that the upgrade command sets. The custom resource path is unaffected: it tidies these keys away before comparing the manifests. Behavioral change. Co-Authored-By: Claude Opus 5.5 --- cmd/upgrade.go | 7 +- manifest/generate.go | 58 ++++++++++- manifest/generate_test.go | 55 ++++++++++ scripts/issues/three-way-null-metadata.sh | 117 ++++++++++++++++++++++ 4 files changed, 234 insertions(+), 3 deletions(-) create mode 100755 scripts/issues/three-way-null-metadata.sh diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 3c3f7a33..31c4fdb0 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -356,9 +356,13 @@ func (d *diffCmd) runHelm3() error { } var actionConfig *action.Configuration + releaseNs := d.namespace if d.threeWayMerge || d.takeOwnership { actionConfig = new(action.Configuration) localEnv := prepareEnvSettings(d.kubeContext) + if releaseNs == "" { + releaseNs = localEnv.Namespace() + } storageNs := d.storage() if storageNs == "" { storageNs = localEnv.Namespace() @@ -373,7 +377,8 @@ func (d *diffCmd) runHelm3() error { if d.threeWayMerge { releaseManifest, installManifest, err = manifest.Generate(actionConfig, releaseManifest, installManifest, - manifest.WithThreeWayMergeMode(manifest.ThreeWayMergeMode(d.threeWayMergeMode))) + manifest.WithThreeWayMergeMode(manifest.ThreeWayMergeMode(d.threeWayMergeMode)), + manifest.WithRelease(d.release, releaseNs)) if err != nil { return fmt.Errorf("unable to generate manifests: %w", err) } diff --git a/manifest/generate.go b/manifest/generate.go index cf3731a7..402e6238 100644 --- a/manifest/generate.go +++ b/manifest/generate.go @@ -16,6 +16,7 @@ import ( authorizationv1 "k8s.io/api/authorization/v1" apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -54,7 +55,9 @@ var ValidThreeWayMergeModes = []string{ } type generateOptions struct { - mergeMode ThreeWayMergeMode + mergeMode ThreeWayMergeMode + release string + releaseNamespace string } // GenerateOption customizes the behavior of Generate. @@ -68,6 +71,16 @@ func WithThreeWayMergeMode(mode ThreeWayMergeMode) GenerateOption { } } +// WithRelease names the release the manifests belong to. The three-way merge +// then adds the release's ownership metadata to the new manifest, the way helm +// upgrade does, so that the computed patch matches the one the upgrade sends. +func WithRelease(name, namespace string) GenerateOption { + return func(o *generateOptions) { + o.release = name + o.releaseNamespace = namespace + } +} + func Generate(actionConfig *action.Configuration, originalManifest, targetManifest []byte, opts ...GenerateOption) ([]byte, []byte, error) { options := generateOptions{mergeMode: ThreeWayMergeAuto} for _, opt := range opts { @@ -165,7 +178,16 @@ func Generate(actionConfig *action.Configuration, originalManifest, targetManife return fmt.Errorf("could not find %q", info.Name) } - patch, err := createPatch(originalInfo.Object, currentObj, info) + target := info + if options.release != "" { + withMetadata := *info + if withMetadata.Object, err = withReleaseMetadata(info.Object, options.release, options.releaseNamespace); err != nil { + return fmt.Errorf("adding release metadata to %q with kind %s: %w", info.Name, kind, err) + } + target = &withMetadata + } + + patch, err := createPatch(originalInfo.Object, currentObj, target) if err != nil { return err } @@ -673,6 +695,38 @@ func createPatch(originalObj, currentObj runtime.Object, target *resource.Info) return patch, nil } +// withReleaseMetadata returns a copy of obj carrying the ownership metadata that +// helm install, upgrade and rollback merge into every object before computing +// the patch. A three-way patch built without it nulls a map the chart renders +// empty, and with it everything that was added to the map outside the chart. +func withReleaseMetadata(obj runtime.Object, release, namespace string) (runtime.Object, error) { + obj = obj.DeepCopyObject() + accessor, err := meta.Accessor(obj) + if err != nil { + return nil, err + } + + accessor.SetLabels(withEntries(accessor.GetLabels(), map[string]string{ + "app.kubernetes.io/managed-by": "Helm", + })) + accessor.SetAnnotations(withEntries(accessor.GetAnnotations(), map[string]string{ + "meta.helm.sh/release-name": release, + "meta.helm.sh/release-namespace": namespace, + })) + return obj, nil +} + +// withEntries adds entries to m, allocating m if it is nil. +func withEntries(m, entries map[string]string) map[string]string { + if m == nil { + m = make(map[string]string, len(entries)) + } + for k, v := range entries { + m[k] = v + } + return m +} + // dropNulls removes every null-valued key from the JSON object in data, at any // depth of nested objects. A list is left as it is, since a merge patch // replaces a list whole. diff --git a/manifest/generate_test.go b/manifest/generate_test.go index 99367087..1d28e1b3 100644 --- a/manifest/generate_test.go +++ b/manifest/generate_test.go @@ -18,6 +18,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/strategicpatch" "k8s.io/cli-runtime/pkg/resource" "sigs.k8s.io/yaml" ) @@ -113,6 +114,60 @@ func TestCreatePatchApplyLocally_StrategicRemoval(t *testing.T) { assert.Equal(t, map[string]string{"keep": "me"}, got.ObjectMeta.Labels) } +// A chart that renders `annotations:` or `labels:` with nothing under it leaves +// the map null in both manifests. helm upgrade adds its ownership metadata to +// the object before it computes the patch, so the patch it sends does not null +// the map, and what was added to it outside the chart survives. The patch sent +// for the server-side dry-run must do the same. +func TestCreatePatch_KeepsMetadataAddedOutsideTheChartUnderANullMap(t *testing.T) { + chart := ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: res + namespace: default + annotations: + labels: +data: {foo: bar} +` + live := fromYAML(t, ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: res + namespace: default + annotations: + meta.helm.sh/release-name: rel + meta.helm.sh/release-namespace: default + example.com/added-outside: "yes" + labels: + app.kubernetes.io/managed-by: Helm + example.com/added-outside: "yes" +data: {foo: bar} +`) + + target, err := withReleaseMetadata(fromYAML(t, chart), "rel", "default") + require.NoError(t, err) + + patch, err := createPatch(fromYAML(t, chart), live, infoFor(t, target, corev1.SchemeGroupVersion.WithKind("ConfigMap"))) + require.NoError(t, err) + + liveJSON, err := live.MarshalJSON() + require.NoError(t, err) + // Apply the patch the way the API server does, without the post-processing + // of the local merge. + merged, err := strategicpatch.StrategicMergePatchUsingLookupPatchMeta(liveJSON, patch.data, patch.patchMeta) + require.NoError(t, err) + + var got map[string]interface{} + require.NoError(t, json.Unmarshal(merged, &got)) + for _, field := range []string{"annotations", "labels"} { + value, found := nested(t, got, "metadata", field, "example.com/added-outside") + assert.True(t, found, "the %s added outside the chart must survive", field) + assert.Equal(t, "yes", value) + } +} + // TestCreatePatchApplyLocally_Unstructured covers custom resources, which use a // plain JSON merge patch instead of a strategic merge patch. func TestCreatePatchApplyLocally_Unstructured(t *testing.T) { diff --git a/scripts/issues/three-way-null-metadata.sh b/scripts/issues/three-way-null-metadata.sh new file mode 100755 index 00000000..fe41d3a1 --- /dev/null +++ b/scripts/issues/three-way-null-metadata.sh @@ -0,0 +1,117 @@ +#!/usr/bin/env bash +# Reproduction test: --three-way-merge reports the removal of every live +# annotation (or label) when the chart renders the map as null. +# +# A chart that renders `annotations:` with nothing under it produces a null map. +# helm upgrade merges its ownership metadata (meta.helm.sh/release-name and +# meta.helm.sh/release-namespace, app.kubernetes.io/managed-by) into every +# object before it computes the three-way patch, so the map it sends is never +# null and annotations added outside the chart survive the upgrade. helm-diff +# must compute the same patch and report no change. +# +# Scenarios (each prints its full diff output to the CI log): +# A. null annotations, live object annotated outside the chart +# B. null labels, live object labelled outside the chart +# C. A with --three-way-merge-mode client +# D. an annotation the chart sets, edited outside the chart, is still drift + +set -euo pipefail + +ISSUE=three-way-null-metadata +# shellcheck source=lib.sh +source "$(dirname "$0")/lib.sh" + +kubectl create namespace "$NS" 2>/dev/null || true + +# expect_exit fails the +# test unless helm diff --detailed-exitcode exits with the expected code. +expect_exit() { + local scenario="$1" want="$2"; shift 2 + local out="$WORK/${scenario// /_}.out" + echo "" + echo "===== helm diff $* [$scenario] =====" + set +e + helm diff upgrade --detailed-exitcode "$@" > "$out" 2>&1 + local rc="$?" + set -e + strip_ansi < "$out" + echo "----- exit code: $rc -----" + if [ "$rc" -ne "$want" ]; then + echo "FAIL: scenario [$scenario] exited $rc, want $want" + FAIL=1 + else + echo "OK: scenario [$scenario] exited $want" + fi +} + +############################################################################### +# Variant A: null annotations, annotation added outside the chart +############################################################################### +chart "$WORK/a" 'apiVersion: v1 +kind: ConfigMap +metadata: + name: res-a + annotations: +data: + foo: bar +' +helm upgrade -i rel-a "$WORK/a" -n "$NS" >/dev/null +kubectl annotate configmap res-a -n "$NS" example.com/added-outside=yes --overwrite >/dev/null + +expect_exit "A three-way" 0 rel-a "$WORK/a" -n "$NS" --three-way-merge + +############################################################################### +# Variant B: null labels, label added outside the chart +############################################################################### +chart "$WORK/b" 'apiVersion: v1 +kind: ConfigMap +metadata: + name: res-b + labels: +data: + foo: bar +' +helm upgrade -i rel-b "$WORK/b" -n "$NS" >/dev/null +kubectl label configmap res-b -n "$NS" example.com/added-outside=yes --overwrite >/dev/null + +expect_exit "B three-way" 0 rel-b "$WORK/b" -n "$NS" --three-way-merge + +############################################################################### +# Variant C: variant A, merged locally +############################################################################### +expect_exit "C three-way client" 0 rel-a "$WORK/a" -n "$NS" --three-way-merge --three-way-merge-mode client + +############################################################################### +# Variant D: the chart sets an annotation that is edited outside the chart +############################################################################### +chart "$WORK/d" 'apiVersion: v1 +kind: ConfigMap +metadata: + name: res-d + annotations: + example.com/owned-by-chart: chart +data: + foo: bar +' +helm upgrade -i rel-d "$WORK/d" -n "$NS" >/dev/null +kubectl annotate configmap res-d -n "$NS" example.com/owned-by-chart=edited --overwrite >/dev/null + +expect_exit "D three-way drift" 2 rel-d "$WORK/d" -n "$NS" --three-way-merge + +############################################################################### +# The upgrade itself keeps what was added outside the chart; this is the +# behavior the diffs above must agree with. +############################################################################### +helm upgrade rel-a "$WORK/a" -n "$NS" >/dev/null +helm upgrade rel-b "$WORK/b" -n "$NS" >/dev/null +for check in "res-a {.metadata.annotations.example\.com/added-outside}" \ + "res-b {.metadata.labels.example\.com/added-outside}"; do + read -r name path <<< "$check" + if [ "$(kubectl get configmap "$name" -n "$NS" -o jsonpath="$path")" != "yes" ]; then + echo "FAIL: helm upgrade removed the metadata added outside the chart from $name; the premise of this test is wrong" + FAIL=1 + fi +done + +############################################################################### +finish From be715016b0e0908ecff24b5f71e0a02e5f192a23 Mon Sep 17 00:00:00 2001 From: Jared Hicks Date: Thu, 5 Nov 2026 21:16:34 -0500 Subject: [PATCH 2/2] test: name the null metadata reproduction after issue #1101 Rename the script so scripts/issues/run.sh picks it up, and link the issue. Structural change. Co-Authored-By: Claude Opus 5.5 --- scripts/issues/{three-way-null-metadata.sh => 1101.sh} | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) rename scripts/issues/{three-way-null-metadata.sh => 1101.sh} (96%) diff --git a/scripts/issues/three-way-null-metadata.sh b/scripts/issues/1101.sh similarity index 96% rename from scripts/issues/three-way-null-metadata.sh rename to scripts/issues/1101.sh index fe41d3a1..26773e2b 100755 --- a/scripts/issues/three-way-null-metadata.sh +++ b/scripts/issues/1101.sh @@ -1,5 +1,7 @@ #!/usr/bin/env bash -# Reproduction test: --three-way-merge reports the removal of every live +# Reproduction test for https://github.com/databus23/helm-diff/issues/1101 +# +# Bug: --three-way-merge reports the removal of every live # annotation (or label) when the chart renders the map as null. # # A chart that renders `annotations:` with nothing under it produces a null map. @@ -17,7 +19,7 @@ set -euo pipefail -ISSUE=three-way-null-metadata +ISSUE=1101 # shellcheck source=lib.sh source "$(dirname "$0")/lib.sh"