diff --git a/README.md b/README.md index 492ff14e..c7369d6e 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. @@ -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.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..6d55930d 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 @@ -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