Repository navigation
fix: restore CRD-defaulted fields inside lists on the client-side three-way merge - #1090
Merged
yxxhero merged 3 commits intoOct 18, 2026
Merged
Conversation
…ee-way merge
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 <jrofe96@gmail.com>
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 <aiopsclub@163.com>
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 <aiopsclub@163.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
With
--three-way-merge-mode=client(#1063), custom resources are merged with a JSON merge patch and the result skipped therestoreServerPopulatedFieldspass. A JSON merge patch replaces a list as a whole, so every field a CRD schema defaults into a list element was lost on each run that rendered the chart unchanged, and the diff reported it as a removal the upgrade never makes.Run the restoration pass over JSON merge patches as well. The
null/[]/{}equivalence stays off for them: custom resources are stored as JSON and keep the three apart, so a chart turning one into another is still reported (the existing test for that, renamed, keeps passing).Why
A CRD whose list elements carry two string fields with
default: ""produces this on every diff of a release that renders ~15 such objects, even when the chart did not change at all:watchedResources: - group: apps - name: "" - namespace: "" resource: deployments version: v1That is the only case where
clientmode was noisier thanservermode for custom resources; the merged object already had the right shape, it just lost what the API server had filled in.Behaviour
Measured against a live cluster with the same release, before and after:
- name: ""/- namespace: ""The 4 objects left are ones whose chart sets fields the live CRD prunes, which is a real difference and still shows.
What the restoration does and does not do for a custom resource, all covered by
TestLocalMerge_KeepsDefaultedFieldsInCustomResourceLists:nullleaves the live value alone, which matches whathelm upgradedoes for an unstructured object: its patch is the two-way merge patch old→new, empty when they agree (pkg/kube/client.go,createPatch).One limit, shared with the existing handling of atomic lists: elements are paired by position, so if the cluster reorders a set-like list while the chart leaves it unchanged, a default could be copied back onto the wrong element. Only the textual diff is affected.
Checklist
make lint,make test,make verify-readmeandgolangci-lint v2.14.0pass locally.