fix: restore CRD-defaulted fields inside lists on the client-side three-way merge

README.md

@@ -277,7 +277,7 @@ Notes:
 `--three-way-merge-mode` controls how the patch is applied:
 
 - `server` sends the patch to the API server as a dry-run (`PATCH ...?dryRun=All`). The API server fills in defaults and runs mutating webhooks, so the result is the most faithful preview of the upgrade — but the credentials need the `patch` permission on every diffed resource.
-- `client` applies the patch locally, using the same strategic-merge (or JSON merge patch, for custom resources) logic the API server would use. Only `get` is required. The merged object is then round-tripped through its Go type, the way the API server does before it answers, and a field is copied back from the live object whenever the old and the new release manifest agree about it — without that, the defaults the API server re-applies after patching would show up as spurious removals. An empty list, an empty map and a `null` are also treated as the same value, because Kubernetes stores objects as protobuf and cannot tell them apart: a chart that writes `rules: []` gets `rules: null` back from the cluster. Validation and mutating webhooks are still not applied.
+- `client` applies the patch locally, using the same strategic-merge (or JSON merge patch, for custom resources) logic the API server would use. Only `get` is required. The merged object is then round-tripped through its Go type, the way the API server does before it answers, and a field is copied back from the live object whenever the old and the new release manifest agree about it — without that, the defaults the API server re-applies after patching would show up as spurious removals. That holds for custom resources too: a JSON merge patch replaces a list as a whole, so the defaults a CRD schema put into its elements are copied back as long as the chart left the list alone. For built-in kinds an empty list, an empty map and a `null` are also treated as the same value, because Kubernetes stores them as protobuf and cannot tell them apart: a chart that writes `rules: []` gets `rules: null` back from the cluster. Custom resources are stored as JSON and keep the three apart, so there a chart turning one into another is reported. Validation and mutating webhooks are still not applied.
 - `auto` (the default) tries `server` first and falls back to `client` per run when the API server rejects the dry-run with `Forbidden` or `MethodNotAllowed`, printing a note on stderr. Any other error still aborts the diff.
 
 Because the defaulting functions are not part of client-go, `client` mode cannot reproduce them exactly. What it can do is leave the live value alone: a field is only reported as gone when the two release manifests disagree about it. That matters more than it sounds, because a chart that leaves a value unset usually renders the field as an explicit `null` — a bare `replicas:` — and a `null` in a manifest reaches the patch as a change rather than a deletion, wiping a value the API server had defaulted in even when the chart did not change at all.
@@ -286,9 +286,9 @@ Four known deviations from `server` mode remain.
 
 **A field the chart stops pinning is reported as removed** rather than as changing to its default. `server` mode shows `replicas: 3` becoming the defaulted `1`; `client` mode reports the field going away, because it cannot name the value that replaces it. The change is reported either way.
 
-**Drift can be hidden inside a `retainKeys` struct or an atomic list.** The restoration described above copies back every live-only field that the patch replaced wholesale, and locally there is no way to tell a value the API server defaulted from one somebody set by hand — both are simply fields neither manifest mentions. If a NetworkPolicy port carries a hand-added `endPort`, the upgrade removes it and `server` mode says so, while `client` mode restores it along with the defaulted `protocol: TCP` and reports nothing. This is the one case where `client` mode can be quieter than the truth; everywhere else it errs towards reporting a change that does not happen. Use `--three-way-merge-mode=server` where that matters.
+**Drift can be hidden inside a `retainKeys` struct, an atomic list or a list of a custom resource.** The restoration described above copies back every live-only field that the patch replaced wholesale, and locally there is no way to tell a value the API server defaulted from one somebody set by hand — both are simply fields neither manifest mentions. If a NetworkPolicy port carries a hand-added `endPort`, the upgrade removes it and `server` mode says so, while `client` mode restores it along with the defaulted `protocol: TCP` and reports nothing. This is the one case where `client` mode can be quieter than the truth; everywhere else it errs towards reporting a change that does not happen. Use `--three-way-merge-mode=server` where that matters.
 
-**Server defaults are dropped from an atomic-list entry the chart changes.** Changing a NetworkPolicy port from `27017` to `27018` also drops the defaulted `protocol: TCP` from the diff, because positions in a list the chart rewrote can no longer be matched up safely.
+**Server defaults are dropped from an atomic-list entry the chart changes.** Changing a NetworkPolicy port from `27017` to `27018` also drops the defaulted `protocol: TCP` from the diff, because positions in a list the chart rewrote can no longer be matched up safely. A list of a custom resource behaves the same: its JSON merge patch rewrites the list as a whole, so an entry the chart touched loses the defaults the CRD schema had filled in.
 
 **A field unknown to the compiled-in Kubernetes libraries is dropped.** To strip explicit zero values, `client` mode round-trips the merged object through the Go types helm-diff was built with. A field on a built-in kind that those libraries do not know — a freshly introduced field, on a cluster newer than the libraries this build was compiled against — does not survive that round-trip: a chart that adds it gets no diff line, and a chart that changes it sees it reported as removed instead. Custom resources are unaffected, since they are merged as JSON without a typed round-trip. `server` mode reports such fields, because the dry-run response is kept exactly as the API server sent it.
 

manifest/generate.go

@@ -328,16 +328,6 @@ func (p *resourcePatch) apply(liveData []byte) ([]byte, error) {
 		return nil, err
 	}
 
-	// A JSON merge patch carries only what the chart changed between the two
-	// release manifests, so it never prunes a field the cluster populated and
-	// there is nothing to put back. Custom resources are also stored as JSON
-	// rather than protobuf, which keeps `null`, `[]` and `{}` apart, so the
-	// restoration pass would misread a chart changing one into another as noise
-	// and undo it.
-	if p.patchType == types.MergePatchType {
-		return merged, nil
-	}
-
 	return p.restoreServerPopulatedFields(merged, liveData)
 }
 
@@ -378,12 +368,15 @@ func (p *resourcePatch) normalize(merged []byte) ([]byte, error) {
 // replaces `retainKeys` structs and atomic lists as a whole, which drops what
 // the API server had defaulted into them - the rolling update strategy of a
 // Deployment, the `protocol: TCP` of a NetworkPolicy port, the `volumeMode` of a
-// volume claim template. And a manifest that renders a field as an explicit
-// `null`, the way a chart writes `replicas:` for a value it leaves unset, turns
-// into a `null` in the patch: the key is in both manifests, so it counts as a
-// change rather than a deletion and deletes the live value. Either way the API
-// server defaults the field straight back, but client-go does not ship the
-// defaulting functions, so the local merge has to recover the value differently.
+// volume claim template. A JSON merge patch, which is what custom resources get,
+// replaces every list as a whole, so the defaults a CRD schema put into the
+// list elements go the same way. And a manifest that renders a field as an
+// explicit `null`, the way a chart writes `replicas:` for a value it leaves
+// unset, turns into a `null` in the patch: the key is in both manifests, so it
+// counts as a change rather than a deletion and deletes the live value. Either
+// way the API server defaults the field straight back, but client-go does not
+// ship the defaulting functions, so the local merge has to recover the value
+// differently.
 //
 // A field is copied back from the live object when the old and the new release
 // manifest agree about it - neither mentions it, or both give it the same value,
@@ -395,6 +388,12 @@ func (p *resourcePatch) normalize(merged []byte) ([]byte, error) {
 // Only fields missing from the merged object are restored, never values that it
 // already carries, so drift between the cluster and the chart is still
 // reported.
+//
+// Built-in kinds are stored as protobuf, which cannot tell `null`, `[]` and `{}`
+// apart, so for a strategic merge patch two empty values are read as the same
+// value. A custom resource is stored as JSON and keeps the three apart: a chart
+// changing one into another is a change the upgrade makes, so for a JSON merge
+// patch the equivalence is off.
 func (p *resourcePatch) restoreServerPopulatedFields(merged, liveData []byte) ([]byte, error) {
 	var mergedObj, liveObj, originalObj, modifiedObj interface{}
 
@@ -415,7 +414,8 @@ func (p *resourcePatch) restoreServerPopulatedFields(merged, liveData []byte) ([
 		}
 	}
 
-	restored := restoreMissing(mergedObj, liveObj, originalObj, modifiedObj)
+	coalesceEmpty := p.patchType == types.StrategicMergePatchType
+	restored := restoreMissing(mergedObj, liveObj, originalObj, modifiedObj, coalesceEmpty)
 
 	out, err := json.Marshal(restored)
 	if err != nil {
@@ -443,7 +443,9 @@ func isEmpty(v interface{}) bool {
 // live that merged lost without either release manifest asking for it. It
 // returns the updated merged value and never overwrites a value merged already
 // has, so a field the chart does specify keeps whatever the patch made of it.
-func restoreMissing(merged, live, original, modified interface{}) interface{} {
+// coalesceEmpty reads `null`, `[]` and `{}` as the same value, which only holds
+// for objects the API server stores as protobuf.
+func restoreMissing(merged, live, original, modified interface{}, coalesceEmpty bool) interface{} {
 	switch live := live.(type) {
 	case map[string]interface{}:
 		mergedMap, ok := merged.(map[string]interface{})
@@ -455,7 +457,7 @@ func restoreMissing(merged, live, original, modified interface{}) interface{} {
 
 		for key, liveValue := range live {
 			mergedValue, inMerged := mergedMap[key]
-			if inMerged && isEmpty(mergedValue) && isEmpty(liveValue) {
+			if coalesceEmpty && inMerged && isEmpty(mergedValue) && isEmpty(liveValue) {
 				// Two empty values that differ only in how they are written are
 				// not a change: the API server stores objects as protobuf,
 				// which cannot tell an empty list from an absent one, so a
@@ -477,7 +479,7 @@ func restoreMissing(merged, live, original, modified interface{}) interface{} {
 				}
 				continue
 			}
-			mergedMap[key] = restoreMissing(mergedValue, liveValue, originalMap[key], modifiedMap[key])
+			mergedMap[key] = restoreMissing(mergedValue, liveValue, originalMap[key], modifiedMap[key], coalesceEmpty)
 		}
 		return mergedMap
 
@@ -497,7 +499,7 @@ func restoreMissing(merged, live, original, modified interface{}) interface{} {
 		}
 
 		for i := range mergedList {
-			mergedList[i] = restoreMissing(mergedList[i], live[i], originalList[i], modifiedList[i])
+			mergedList[i] = restoreMissing(mergedList[i], live[i], originalList[i], modifiedList[i], coalesceEmpty)
 		}
 		return mergedList
 

manifest/generate_test.go

@@ -691,10 +691,12 @@ spec:
 		"a chart-owned value must not be restored from the cluster, got hostNetwork=%v", value)
 }
 
-// A custom resource is stored as JSON, which keeps null, [] and {} apart, and a
-// JSON merge patch already leaves untouched live fields alone. Neither the
-// empty-value equivalence nor the restoration pass may run over one.
-func TestLocalMerge_LeavesCustomResourcesToTheMergePatch(t *testing.T) {
+// A custom resource is stored as JSON, which keeps null, [] and {} apart, so
+// the empty-value equivalence must not run over one: a chart turning one into
+// another is a change the upgrade makes. The restoration pass still runs, and
+// must leave the fields the merge patch never touched exactly as the cluster
+// has them.
+func TestLocalMerge_KeepsEmptyValuesApartOnCustomResources(t *testing.T) {
 	widget := func(items string) string {
 		return fmt.Sprintf(`
 apiVersion: example.com/v1
@@ -733,6 +735,147 @@ spec:
 		require.True(t, found, "a JSON merge patch must not prune a field it never mentions")
 		assert.EqualValues(t, 3, observed)
 	})
+
+	// A `null` the two manifests share is not a change: the upgrade sends no
+	// patch for the field, so what the cluster carries - here a whole nested
+	// object - is what the upgrade result keeps. The restoration pass is what
+	// recovers it: applying the chart's null to the live object drops the
+	// field, and the manifests agree it should say nothing.
+	t.Run("a null both manifests share leaves the live value alone", func(t *testing.T) {
+		unset := `
+apiVersion: example.com/v1
+kind: Widget
+metadata: {name: w, namespace: default}
+spec:
+  items: null
+  template: null
+  size: large
+`
+		liveTemplate := `
+apiVersion: example.com/v1
+kind: Widget
+metadata: {name: w, namespace: default}
+spec:
+  items: null
+  template: {greeting: hi}
+  size: large
+`
+		got := applyLocally(t, unset, liveTemplate, unset)
+		template, found := nested(t, got, "spec", "template")
+		require.True(t, found, "a field both manifests render as null keeps what the cluster carries")
+		assert.Equal(t, map[string]interface{}{"greeting": "hi"}, template)
+	})
+}
+
+// A JSON merge patch replaces a list as a whole, so the fields the CRD defaulted
+// into its elements - `name: ""` on every watched resource below - are lost
+// whenever the list reaches the patch, which it does on every run that renders
+// the chart unchanged. The API server would default them straight back, so
+// they must be restored the same way they are for an atomic list of a built-in
+// kind.
+func TestLocalMerge_KeepsDefaultedFieldsInCustomResourceLists(t *testing.T) {
+	policy := func(resources string) string {
+		return fmt.Sprintf(`
+apiVersion: example.com/v1
+kind: Policy
+metadata: {name: p}
+spec:
+  watchedResources:
+%s
+`, resources)
+	}
+	chart := policy(`    - {group: apps, version: v1, resource: deployments}
+    - {group: apps, version: v1, resource: statefulsets}`)
+	live := policy(`    - {group: apps, version: v1, resource: deployments, name: "", namespace: ""}
+    - {group: apps, version: v1, resource: statefulsets, name: "", namespace: ""}`)
+
+	t.Run("defaults inside a list the chart leaves alone are restored", func(t *testing.T) {
+		got := applyLocally(t, chart, live, chart)
+		resources, _ := nested(t, got, "spec", "watchedResources")
+		require.Len(t, resources, 2)
+		for i, resource := range resources.([]interface{}) {
+			element := resource.(map[string]interface{})
+			name, found := element["name"]
+			require.True(t, found, "element %d lost the defaulted name", i)
+			assert.Empty(t, name)
+			namespace, found := element["namespace"]
+			require.True(t, found, "element %d lost the defaulted namespace", i)
+			assert.Empty(t, namespace)
+		}
+	})
+
+	t.Run("drift inside the list is still reported", func(t *testing.T) {
+		drifted := policy(`    - {group: apps, version: v1, resource: deployments, name: "", namespace: ""}
+    - {group: apps, version: v1, resource: daemonsets, name: "", namespace: ""}`)
+		got := applyLocally(t, chart, drifted, chart)
+		resources, _ := nested(t, got, "spec", "watchedResources")
+		require.Len(t, resources, 2)
+		second := resources.([]interface{})[1].(map[string]interface{})
+		assert.Equal(t, "statefulsets", second["resource"], "the chart-owned value must win over the drifted one")
+		name, found := second["name"]
+		require.True(t, found, "a default next to a drifted value is still a default")
+		assert.Empty(t, name)
+	})
+
+	t.Run("a list the chart changes is left to the patch", func(t *testing.T) {
+		extended := policy(`    - {group: apps, version: v1, resource: deployments}
+    - {group: apps, version: v1, resource: statefulsets}
+    - {group: apps, version: v1, resource: daemonsets}`)
+		got := applyLocally(t, chart, live, extended)
+		resources, _ := nested(t, got, "spec", "watchedResources")
+		require.Len(t, resources, 3, "the element the chart adds must be reported")
+		for i, resource := range resources.([]interface{}) {
+			_, found := resource.(map[string]interface{})["name"]
+			assert.False(t, found, "element %d: positions cannot be paired once the chart changes the list", i)
+		}
+	})
+
+	// Helm patches an unstructured object with a two-way merge patch from the
+	// old to the new manifest, which is empty when they agree, so a value the
+	// chart renders as `null` on both sides is left in the cluster untouched.
+	t.Run("a null in both manifests leaves the live value alone", func(t *testing.T) {
+		unset := policy(`    - {group: apps, version: v1, resource: deployments, namespace: null}
+    - {group: apps, version: v1, resource: statefulsets}`)
+		got := applyLocally(t, unset, live, unset)
+		resources, _ := nested(t, got, "spec", "watchedResources")
+		require.Len(t, resources, 2)
+		namespace, found := resources.([]interface{})[0].(map[string]interface{})["namespace"]
+		require.True(t, found, "the upgrade sends no patch for a field both manifests leave unset")
+		ns, isString := namespace.(string)
+		require.True(t, isString, "the value the cluster carries must survive, not the null the chart renders")
+		assert.Empty(t, ns)
+	})
+
+	// The guard holds on the live side as well: an element somebody added to
+	// the cluster breaks the pairing of positions even though the chart left
+	// the list alone, so the list is left as the patch produced it and the
+	// defaults go un-restored - the same trade-off an atomic list of a built-in
+	// kind makes.
+	t.Run("a list the cluster has drifted in length is left to the patch", func(t *testing.T) {
+		drifted := policy(`    - {group: apps, version: v1, resource: deployments, name: ""}
+    - {group: apps, version: v1, resource: statefulsets, name: ""}
+    - {group: apps, version: v1, resource: daemonsets, name: ""}`)
+		got := applyLocally(t, chart, drifted, chart)
+		resources, _ := nested(t, got, "spec", "watchedResources")
+		require.Len(t, resources, 2, "the element somebody added by hand is reported as removed")
+		for i, resource := range resources.([]interface{}) {
+			_, found := resource.(map[string]interface{})["name"]
+			assert.False(t, found, "element %d: positions cannot be paired once the lengths drift", i)
+		}
+	})
+
+	t.Run("a field the chart drops from an element is reported", func(t *testing.T) {
+		pinned := policy(`    - {group: apps, version: v1, resource: deployments, namespace: kube-system}
+    - {group: apps, version: v1, resource: statefulsets}`)
+		livePinned := policy(`    - {group: apps, version: v1, resource: deployments, name: "", namespace: kube-system}
+    - {group: apps, version: v1, resource: statefulsets, name: "", namespace: ""}`)
+		got := applyLocally(t, pinned, livePinned, chart)
+		resources, _ := nested(t, got, "spec", "watchedResources")
+		require.Len(t, resources, 2)
+		first := resources.([]interface{})[0].(map[string]interface{})
+		_, found := first["namespace"]
+		assert.False(t, found, "the chart stopped pinning the namespace, which is a change to report")
+	})
 }
 
 // A Forbidden that RBAC did not produce - an admission webhook or a quota