fix: keep metadata added outside the chart when it renders a null map

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)
 		}

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.

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) {

scripts/issues/1101.sh

@@ -0,0 +1,119 @@
+#!/usr/bin/env bash
+# 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.
+# 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=1101
+# shellcheck source=lib.sh
+source "$(dirname "$0")/lib.sh"
+
+kubectl create namespace "$NS" 2>/dev/null || true
+
+# expect_exit <scenario> <expected exit code> <helm diff args...> 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