From 6792fce32d1489df5a35260962f948279d6ec77f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Rodr=C3=ADguez=20Fern=C3=A1ndez?= Date: Fri, 16 Oct 2026 09:50:34 +0200 Subject: [PATCH 1/3] fix: restore CRD-defaulted fields inside lists on the client-side three-way merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A JSON merge patch replaces a list as a whole, so with --three-way-merge-mode=client the fields a CRD schema defaults into list elements were lost on every run that rendered the chart unchanged, and the diff reported them as removals the upgrade never makes. Run the restoration pass over JSON merge patches as well, keeping null, [] and {} apart for custom resources since they are stored as JSON. Signed-off-by: Javier Rodríguez Fernández --- README.md | 2 +- manifest/generate.go | 44 +++++++++-------- manifest/generate_test.go | 101 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 121 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 492ff14e..4a80de95 100644 --- a/README.md +++ b/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. diff --git a/manifest/generate.go b/manifest/generate.go index f975570b..e6e8ab8a 100644 --- a/manifest/generate.go +++ b/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 diff --git a/manifest/generate_test.go b/manifest/generate_test.go index 4f04ca34..2bb7dbc8 100644 --- a/manifest/generate_test.go +++ b/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 @@ -735,6 +737,97 @@ spec: }) } +// 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") + assert.Empty(t, namespace) + }) + + 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 // controller refusing an authorized request - is the upgrade's own outcome and // must not be worked around locally. From bbe897da9ea0a80027b174778c50bd0c44433395 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sun, 18 Oct 2026 08:29:58 +0800 Subject: [PATCH 2/3] test: pin the custom-resource restoration edges the new tests leave open The restoration pass the PR enables for JSON merge patches behaves correctly on edges its table tests did not actually pin down: - "a null in both manifests leaves the live value alone" only asserted that the key was present and empty, which an explicit null left in the merged object would satisfy just as well as the restored live value; assert the live value itself. The restoration does hold - the merge patch drops the null inside a replaced list, so the field counts as missing and is copied back - but nothing guarded that mechanism. - A list whose length drifted on the cluster side (an element somebody added) cannot be paired by position even when the chart left it alone, so the patch output stands and the defaults go un-restored, the same trade-off an atomic list of a built-in kind makes; no test covered the guard. - A null the two manifests share on a plain map field, outside any list, now keeps the live value too. That is a behaviour the pass newly enables for custom resources - before it, the merged object lost the field and the diff reported a removal the upgrade never makes - so it deserves coverage of its own. The README deviations section said drift can hide inside a retainKeys struct or an atomic list, and that server defaults are dropped from an atomic-list entry the chart changes. Both now hold for custom-resource lists as well, because their JSON merge patch replaces the list as a whole and the restoration pass covers them; say so. Signed-off-by: yxxhero --- README.md | 4 ++-- manifest/generate_test.go | 50 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 4a80de95..c7369d6e 100644 --- a/README.md +++ b/README.md @@ -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. diff --git a/manifest/generate_test.go b/manifest/generate_test.go index 2bb7dbc8..06b82899 100644 --- a/manifest/generate_test.go +++ b/manifest/generate_test.go @@ -735,6 +735,36 @@ 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 @@ -811,7 +841,25 @@ spec: 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") - assert.Empty(t, namespace) + assert.Equal(t, "", namespace, "the value the cluster carries must survive, not the null the chart renders") + }) + + // 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) { From f346068ca90fceff863731789d12b137a3ee2370 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sun, 18 Oct 2026 08:41:49 +0800 Subject: [PATCH 3/3] fix: satisfy testifylint in the null-in-both restoration test assert.Equal against the empty string trips the testifylint empty checker in CI. Pin the same claim through the type instead: the namespace the merge keeps must be the JSON string the cluster carries (not the null the chart renders), which the string type assertion together with assert.Empty states exactly. Signed-off-by: yxxhero --- manifest/generate_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/manifest/generate_test.go b/manifest/generate_test.go index 06b82899..6d55930d 100644 --- a/manifest/generate_test.go +++ b/manifest/generate_test.go @@ -841,7 +841,9 @@ spec: 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") - assert.Equal(t, "", namespace, "the value the cluster carries must survive, not the null the chart renders") + 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