argo-cd: sync is not working with ignoreDifferences and RespectIgnoreDifferences=true

There may be more than one problem, but I think most of them can be traced back to this function: https://github.com/argoproj/argo-cd/blob/ef8dae885d988bad9033276cd27ccc28945f3433/controller/sync.go#L446

Imagine I want to modify a CronJob to add a new container.

To construct the patch, Argo CD takes the git state (templateMap) and the live state (valueMap) and merges them, using this intersectMap function.

When intersectMap encounters an array, it loops over the array from git (say, an array with two containers) and for each item merges the same array from the live object (say, an array with only one container). If the array in the live object is shorter than the array in the git object, intersectMap just drops any remaining items from the git state. So the patch contains only the first container.

That’s the answer for the specific case of “adding a new container.” The general answer is: this patch function is custom and is not Kubernetes-aware. A Kubernetes-aware patch function would have merged the array on a merge key (like container name) if present.

Do not plan on using ignoreDifferences until it’s maybe fixed by the valiant effort here #14602

I just pushed a change to #14602 where array entries are merged by a merge key (currently hardcoded to name). There are likely many dragons here, both in terms of edge cases and performance. Due to a new child, my ability to carry on the work will be quite limited these next couple of weeks. Hopefully someone can move this PR forward by either bringing on some more test cases, reviewing the PR itself or by fixing it where necessary. If someone wants to move this forward, I’ll try to help when I can.

I added a draft PR (see #14602), there are likely hidden dragons (I can’t believe that the fix is that simple 😛). Feel free to add test cases which I can add to the unit tests.

I think I’ve hit the same bug. Is there any progress or estimation on this?

@si-c613 I’m super excited about the PR. My two hesitations are:

1. this changes what Argo CD applies to the cluster - that’s a little spooky, and we need reeeally good test coverage
2. the hard-coded “name” field kinda stinks, and I’m curious if @leoluz has any idea if we could fetch the merge key info from somewhere rather than making a maybe-wrong assumption

But overall, this is really cool. Even if the PR has limitations, we can document them, and still be in a much better place than we are today.

what are the chances that this issue is related? i just saw that argo using gojq and there was bug in compiler related to arrays that was fixed in 0.12.11 and argo 2.5 uses 0.12.9 at the latest

I like how I’m following this issue, and every week or so I basically get an email notification with a *joke XD

Thanks for making me laugh once again @cirano-eusebi

** I’ll be ruthlessly truthful, The joke is this ‘tool’ and that more and more people are being affected by this issue… (I’m on your team) I tend to try and not to spam a meaningful conversations, but 4 months have past since my last outburst, and also, my Argo instance malfunctioned today [got stuck refreshing], as expected at this point…

https://github.com/argoproj/argo-cd/issues/9678#issuecomment-1669716126 Sorry 🙏

same issue on latest version

@QuingKhaos, I’ll elaborate We have deployment applied with argo, on the app configured ignoreDifferences

      ignoreDifferences:
      - group: apps
        kind: Deployment
        name: some-deployment
        jqPathExpressions:
         - '.spec.template.spec.containers[] | select(.name == "some-container").env[] | select(.name == "_JAVA_OPTIONS")'

at this stage behavior is following:
on every change in the app, argocd does “full” sync, overriding env var to what it see in the git, as it happens, the operator sees changes and applies its patch to env var. Argocd according to ignoreDifferences not doing anything with this change. Until a change in any other place of the app.

After adding RespectIgnoreDifferences=true behaviors is the following:
argocd not triggering sync at all when change is related to anything inside '.spec.template.spec.containers[]. the workaround of syncing from UI with removing V from RespectIgnoreDifferences is working, but it is “not the GitOps that we want” 😃

Confirmed this is also not working with 2.5.2. We are have an ignore like this

ignoreDifferences:
- group: certmanager.io
  kind: Certificate
  jsonPointers:
  - /spec/dnsNames/2

We see the ignore works correctly, but the 3rd dns name is removed when syncing with RespectIgnoreDifferences. We also tried /spec/dnsNames in case there is an oddity with arrays and had the same behavior. Our source file has this for the item being removed for live state:

spec:
  dnsNames:
    - in git
    - also in git
    - manually added and needs to be ignored

And desired state

spec:
  dnsNames:
    - in git
    - also in git

Sounds like a good case to unit test and add to #14602 😃

hi folks, update on my comment above: https://github.com/argoproj/argo-cd/issues/9678#issuecomment-1631504590

for those seeing this issue with Secrets, in our case this was caused by the target Secret resource not having attribute data set. we set the data field to a foo value (foo: 'bar') and it began respecting the ignoreDifferences. so it’s unable to match on the fields when a null value is coming from the source manifest AND there are out-of-band modifications to the live object. this hack only works for key-value fields like data though.

@FredrikAugust indeed it was easy to reproduce, and it also seems like it’s currently failing locally. I’ll look into that and fix it, and push the update to #14602.

@blakepettersson Sure!

I’ve posted the live state to a gist here: https://gist.github.com/FredrikAugust/8ba73d9665b60b168ab96d3f5ec69949

The desired state, if I understand the UI correctly should be that, but with the initialDelaySeconds: 15 instead of : 20. I don’t have the desired state at ready right now as I’ve manually applied the changes, but it should be easy to reproduce.

Added a test case in #14602; for this to pass we’ll likely need to merge the target object(s) with a merge key. At the moment I’m working on doing this by having name as a hard-coded merge key, but could likely use some refinement. Once I get this working we can see if we can/should have some way of specifying merge keys.

I managed to get that test to pass anyways 😄 The gist of it is that at times we also need to compare with values from the full live object.

Brilliant! I’m sure you’re already deep into this now but I’ve had a look at the test case and your PR updates. I wonder if the bug that has now been uncovered is in the diff.Normalize function called here.

I wonder if the return values for live and target shouldn’t include the ignored fields, this is true for live but not true for target? The test that’s failing shows a value changing outside the cluster and not being present in the cluster.

Hope this makes some sort of sense. Apologies that I’m keeping a little distance from this, if it’s more of a hinderance than a help feel free to say 😃

@si-c613 from your test cases I managed to get them to pass, but that caused another test (which I need a bit more info on the reasoning behind why it is the way it is) to fail; the will remove fields from target if not present in live test case.

Beyond that, we’ll need to consider how entries should be merged by key. Keep bringing on the test cases 😄

Had a brief after thought about the previous test being focused on adding values, slightly refactored the naming and added a test for the reverse as well in this commit. Currently this test is passing, but given it’s looking for the lack of something may need a bit more scrutiny.

@si-c613 this is great, thank you!

Same here with 2.7.6:

ignoreDifferences:
    - group: batch
      kind: CronJob
      jqPathExpressions:
      - .spec.jobTemplate.spec.template.spec.containers[] | select(.image|test(":0.19.*")).image
  syncPolicy:
    automated:
    syncOptions:
      - RespectIgnoreDifferences=true
      - ApplyOutOfSyncOnly=true

Upon changing container command arguments, ArgoCD sync succeeds with unchanged resource, we are using argoproj-labs/argocd-image-updater, that particular jq is about ignoring our double tagged images in our registry ( we do both minor ‘latest’ and patch update upon deployment, similar to how you would have python:3.11 equivalent to python:3.11.4 as of writing this )

the workaround of syncing from UI with removing V from RespectIgnoreDifferences is working, but it is “not the GitOps that we want” 😃

indeed works, and the resource is updated… this is not GitOps… 🫤

I had a similar issue where I couldn’t sync because of a difference in an immutable field which I wanted to ignore. However, setting ServerSideApply=true fixed the issue for me:

  syncPolicy:
    syncOptions:
    - ServerSideApply=true
    - RespectIgnoreDifferences=true
  ignoreDifferences:
  - group: '*'
    kind: PersistentVolumeClaim
    jqPathExpressions:
    - .spec.volumeName

same issue on latest version

We are using some operator that is changing the env vars of the pod in the deployment. So now, because of this issue, argo rolling back operator changes to its defaults on every sync (and then operator changes it again). So we have a restart of the deployment on every change of the application, even when it is not related to mentioned deployments at all.