fix: don't clobber non-ignored fields on "replace"#27136
fix: don't clobber non-ignored fields on "replace"#27136blakepettersson wants to merge 4 commits into
Conversation
When normalizeTargetResources computes the live-to-target patch for RespectIgnoreDifferences, it uses strategic merge patch. For resources whose fields carry `patchStrategy:"replace"` (e.g. `policy/v1` PDB `spec.selector`), strategic merge treats the entire field as atomic — so patching in one ignored sub-field (say, a single `matchLabels` key) pulls the entire parent from the live object, overwriting non-ignored sibling fields that the user intentionally changed. Add a post-correction step that walks the patched target against the original target and normalized target. If a field was not touched by the normalizer (`normalized == original`) but was overwritten by the patch (`patched != original`), restore the original target value. This surgically undoes collateral damage from replace semantics without affecting array merge behavior. Currently only `policy/v1` PDB `spec.selector` uses `patchStrategy:"replace"` among built-in types, but the fix is generic and will also cover CRDs with `x-kubernetes-patch-strategy: replace` when OpenAPI-based patching is re-introduced. Fixes argoproj#18232. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
❌ Preview Environment undeployed from BunnyshellAvailable commands (reply to this comment):
|
There was a problem hiding this comment.
Pull request overview
Fixes a normalization/patching edge case where strategic merge patch patchStrategy:"replace" fields (notably policy/v1 PodDisruptionBudget spec.selector) cause RespectIgnoreDifferences patching to overwrite non-ignored sibling fields in the target.
Changes:
- Adds a post-patch correction step in
normalizeTargetResourcesto restore non-ignored fields that were unintentionally overwritten due topatchStrategy:"replace". - Introduces a regression test reproducing #18232 using new live/target PDB fixtures.
- Embeds new PDB YAML testdata into the controller testdata package.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| controller/sync.go | Adds restoreNonIgnoredFields and applies it after merge-patching to prevent clobbering non-ignored fields under replace semantics. |
| controller/sync_test.go | Adds a regression test for PDB selector behavior with RespectIgnoreDifferences=true. |
| controller/testdata/data.go | Embeds the new PDB YAML fixtures for tests. |
| controller/testdata/live-pdb.yaml | Live PDB fixture representing the cluster state used in the regression test. |
| controller/testdata/target-pdb.yaml | Target PDB fixture representing the desired state used in the regression test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #27136 +/- ##
==========================================
+ Coverage 63.23% 63.36% +0.12%
==========================================
Files 415 415
Lines 56545 56569 +24
==========================================
+ Hits 35759 35845 +86
+ Misses 17408 17345 -63
- Partials 3378 3379 +1 ☔ View full report in Codecov by Sentry. |
| // When a PDB (policy/v1) has an ignoreDifferences rule for a matchLabels sub-field and | ||
| // RespectIgnoreDifferences=true is set, normalizeTargetResources should only patch the | ||
| // ignored field — not clobber the entire selector due to patchStrategy:"replace". | ||
| func TestNormalizeTargetResourcesPDBSelector(t *testing.T) { |
There was a problem hiding this comment.
Should we add a test case when server side apply is enabled + respect ignore diff? I would assume the server handles all the diffing/merging and we wouldn't need to handle any of the replace logic?
When normalizeTargetResources computes the live-to-target patch for RespectIgnoreDifferences, it uses strategic merge patch. For resources whose fields carry
patchStrategy:"replace"(e.g.policy/v1PDBspec.selector), strategic merge treats the entire field as atomic — so patching in one ignored sub-field (say, a singlematchLabelskey) pulls the entire parent from the live object, overwriting non-ignored sibling fields that the user intentionally changed.Add a post-correction step that walks the patched target against the original target and normalized target. If a field was not touched by the normalizer (
normalized == original) but was overwritten by the patch (patched != original), restore the original target value. This surgically undoes collateral damage from replace semantics without affecting array merge behavior.Currently only
policy/v1PDBspec.selectorusespatchStrategy:"replace"among built-in types, but the fix is generic and will also cover CRDs withx-kubernetes-patch-strategy: replacewhen OpenAPI-based patching is re-introduced. Fixes #18232.Checklist: