Skip to content

fix: skip target normalization merge patch for resources without matching ignoreDifferences (#26588)#26994

Open
BEvgeniyS wants to merge 14 commits intoargoproj:masterfrom
BEvgeniyS:fix/normalize-target-replace-strategy
Open

fix: skip target normalization merge patch for resources without matching ignoreDifferences (#26588)#26994
BEvgeniyS wants to merge 14 commits intoargoproj:masterfrom
BEvgeniyS:fix/normalize-target-replace-strategy

Conversation

@BEvgeniyS
Copy link
Copy Markdown

@BEvgeniyS BEvgeniyS commented Mar 25, 2026

Fixes #26588 (partially)

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Title of the PR
  • I've included "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates? No.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • I have added a brief description of why this PR is necessary and/or what this PR solves.

normalizeTargetResources() computes a merge patch between normalized-live and raw-live, then applies it to the sync target. The idea is to copy ignored field values from the live object into the target so they don't get changed during sync.

Problem is, this runs for every resource regardless of whether any ignoreDifferences rules actually match it. For most fields that's fine — the patch is effectively a no-op. But it breaks for:

  1. Fields with patchStrategy:"replace"CreateThreeWayMergePatch always includes these in the patch even when the values are identical across all inputs. This is by-design in the k8s strategic merge patch library (replace-strategy fields are atomic). Example: PodDisruptionBudget.spec.selector.

  2. CRDs not registered in the k8s scheme — falls back to JSON merge patch which replaces arrays wholesale instead of merging by key. This is the Rollout env var case from Enabling RespectIgnoreDifferences causes diff to not be applied and app to become stuck in an OutOfSync state #26588.

In both cases the live state gets written into the target, ArgoCD applies it (no actual change), the diff persists, and the app is stuck OutOfSync forever while reporting sync "Succeeded".

I hit this on EKS clusters where the coredns PDB had a different selector in live (set by EKS) vs desired (set by our Helm chart). Removing RespectIgnoreDifferences=true immediately fixed sync.

The fix has two parts:

  1. Before computing the merge patch for a resource, check if it has any matching ignoreDifferences rules (app-level or system-level overrides). If not, skip the merge-patch-and-apply step entirely and return the original target. There's nothing to copy from live if no fields are being ignored.

  2. Filter out /status from resource overrides before the per-resource check. By default, GetResourceOverrides() injects a */* override with /status ignored — this is for diffing, not target normalization (status isn't in targets and gets stripped by the API server). Without filtering, HasIgnoreDifference returns true for every resource via the wildcard match, defeating the skip logic entirely.

This doesn't help the case where ignore rules DO match the resource — patchStrategy:"replace" fields will still get overwritten there. That's a harder problem for a follow-up.

Note: there's an existing PR #26924 that skips normalization when ignores are empty. This PR goes further — it skips per-resource when the rules don't match, which also handles the case from the original issue reporter's comment where unrelated ignore rules are present.

Cherry-pick candidates: 2.14, 2.13 (probably any release with normalizeTargetResources).

@bunnyshell
Copy link
Copy Markdown

bunnyshell Bot commented Mar 25, 2026

❌ Preview Environment undeployed from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

…ut matching ignoreDifferences rules

Fixes argoproj#26588

Signed-off-by: Eugene Babichev <eugene.babichev@clickhouse.com>
@BEvgeniyS BEvgeniyS force-pushed the fix/normalize-target-replace-strategy branch from 5fd7fd7 to 1b64ad3 Compare March 25, 2026 05:05
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.98%. Comparing base (191b4e3) to head (12449f2).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #26994      +/-   ##
==========================================
+ Coverage   63.96%   63.98%   +0.02%     
==========================================
  Files         421      421              
  Lines       57774    57791      +17     
==========================================
+ Hits        36954    36978      +24     
+ Misses      17338    17333       -5     
+ Partials     3482     3480       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BEvgeniyS BEvgeniyS marked this pull request as ready for review March 25, 2026 06:24
@BEvgeniyS BEvgeniyS requested a review from a team as a code owner March 25, 2026 06:24
@Qodo-Free-For-OSS
Copy link
Copy Markdown

Hi, normalizeTargetResources skips merge patch only when HasIgnoreDifference returns false, but the controller’s DiffConfig includes a default */* ResourceOverride with /status ignored; this makes HasIgnoreDifference true for (nearly) every resource so the merge patch still runs and can still corrupt replace-strategy fields (e.g., PDB selector) and CRD arrays.

Severity: action required | Category: correctness

How to fix: Ignore non-sync overrides in guard

Agent prompt to fix - you can give this to your LLM of choice:

Issue description

normalizeTargetResources now decides whether to compute/apply the merge patch by calling HasIgnoreDifference(...). In real controller runs, the DiffConfig includes system overrides, and by default GetResourceOverrides() injects a */* ignoreDifferences override for /status. That makes HasIgnoreDifference return true for almost every resource, so the merge patch still runs and the original corruption scenario can still occur.

Issue Context

The merge patch is intended to copy values for ignored sync-relevant fields from live into target. A global /status ignore rule is relevant to diffing, but it should not cause target normalization merge patches.

Fix Focus Areas

  • controller/sync.go[427-453]
  • util/argo/diff/ignore.go[43-71]
  • util/settings/settings.go[973-1016]
  • controller/sync_test.go[489-689]

What to change

  1. Use the IgnoreDifference returned by HasIgnoreDifference and gate patching on meaningful ignore rules for sync (e.g., ManagedFieldsManagers, JSONPointers/JQPathExpressions that are not exclusively status-related).
  2. At minimum, ensure that an ignore config consisting only of /status (from overrides) does not enable merge patching.
  3. Extend tests to include a DiffConfig built with overrides containing the default */* /status ignoreDifferences and assert the PDB/CRD cases are not corrupted.

We noticed a couple of other issues in this PR as well — happy to share if helpful.


Found by Qodo code review

@BEvgeniyS
Copy link
Copy Markdown
Author

Thanks for the review!

We noticed a couple of other issues in this PR as well — happy to share if helpful.

Yes, please, do let me know.

Signed-off-by: Eugene Babichev <eugene.babichev@clickhouse.com>
@BEvgeniyS BEvgeniyS force-pushed the fix/normalize-target-replace-strategy branch from 0f703a7 to 2442a09 Compare April 21, 2026 00:57
@BEvgeniyS
Copy link
Copy Markdown
Author

@qodo-ai-reviewer The issue should be fixed now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enabling RespectIgnoreDifferences causes diff to not be applied and app to become stuck in an OutOfSync state

2 participants