Post-Delete Hook support mutate Application object

[llavaud]

Describe the bug

The recent Post-Delete hook support PR, mutate the Application object when refreshing by adding two finalizers if a resource have a "helm.sh/hook": post-delete annotation.
In an apps of apps context it causes unnecesssary reconciliations due to object updated:

{"api-version":"argoproj.io/v1alpha1","application":"myRootApp","cluster-name":"myCluster","fields.level":0,"kind":"Application","level":"debug","msg":"Requesting app refresh caused by object update","name":"myApp","namespace":"argo-cd","server":"myServer","time":"2024-03-07T08:14:40Z"}

To Reproduce

Create an Application that deploy another Application, in the nested Application deploy a resource that have a "helm.sh/hook": post-delete annotation. Wait for the 3m default refresh interval and search for the Requesting app refresh caused by object update debug log line.

Expected behavior

Do not mutate the Application object or at least maybe automaticaly ignore thoses finalizers like it is currently done for the metadata fields generation and resourceVersion

Version

argocd: v2.10.2+fcf5d8c
  BuildDate: 2024-03-01T21:24:51Z
  GitCommit: fcf5d8c2381b68ab1621b90be63913b12cca2eb7
  GitTreeState: clean
  GoVersion: go1.21.3
  Compiler: gc
  Platform: linux/amd64

[llavaud]

A possible workaround:

resource.customizations.ignoreDifferences.argoproj.io_Application: |
  jqPathExpressions:
    - .metadata.finalizers[]? | select(. == "post-delete-finalizer.argocd.argoproj.io" or . == "post-delete-finalizer.argocd.argoproj.io/cleanup")
    - if (.metadata.finalizers | length) == 0 then .metadata.finalizers else empty end

[llavaud]

@alexmt Hello, can I have your opinion on this please ? do you consider it as a bug or normal behavior ?

[msvechla]

we are experiencing the same issue. The workaround above works, but it looks like this should be handled on argocd side

[didlawowo]

A possible workaround:

resource.customizations.ignoreDifferences.argoproj.io_Application: |
  jqPathExpressions:
    - .metadata.finalizers[]? | select(. == "post-delete-finalizer.argocd.argoproj.io" or . == "post-delete-finalizer.argocd.argoproj.io/cleanup")
    - if (.metadata.finalizers | length) == 0 then .metadata.finalizers else empty end

how can i make that in config map ?

[llavaud]

A possible workaround:

resource.customizations.ignoreDifferences.argoproj.io_Application: |
  jqPathExpressions:
    - .metadata.finalizers[]? | select(. == "post-delete-finalizer.argocd.argoproj.io" or . == "post-delete-finalizer.argocd.argoproj.io/cleanup")
    - if (.metadata.finalizers | length) == 0 then .metadata.finalizers else empty end

how can i make that in config map ?

not sure to understand your question, the example is what you must add to the argocd-cm ConfigMap

[nbarrientos]

In case this was useful somehow, here it is a good example of a "real world" chart triggering this:

https://github.com/kubernetes-sigs/node-feature-discovery/blob/6030c974d38ed9a1e4592f0f133d64e27d3c50a7/deployment/helm/node-feature-discovery/templates/post-delete-job.yaml#L8-L10

which is used in NVIDIA's gpu operator:

https://github.com/NVIDIA/gpu-operator/blob/main/deployments/gpu-operator/Chart.yaml#L21

[andrii-korotkov-verkada]

@nbarrientos, which ArgoCD version are you using when reproducing?

[nbarrientos]

@andrii-korotkov-verkada Hi! v2.12.6+4dab5bd

[cmontemuino]

We're also experiencing the same as @nbarrientos, NVIDIA GPU Operator including NFD.

Argo CD v2.12.3+6b9cd82

[Farfaday]

Experiencing the same with the current latest kyverno helm chart :)
Using argo-cd v2.12.3+6b9cd82

[diranged]

Experiencing the same with the current latest kyverno helm chart :) Using argo-cd v2.12.3+6b9cd82

Agreed - we just ran into this with the latest Kyverno chart as well due to kyverno/kyverno#11504.

[andrii-korotkov-verkada]

Why are extra refreshes an issue? They are normal to happen a lot, e.g. when some pods are updated and watches identify there's an update. If they are slow, try 2.13.1, there's been some major perf improvements for when it comes to reconciliation.

[diranged]

@andrii-korotkov-verkada No the issue we have is that our app-of-apps applications show up with a diff:

[andrii-korotkov-verkada]

Which diffing strategy are you using? Is it default or server side diff?

[andrii-korotkov-verkada]

Server side diff can be enabled on the controller level as well as application level and has some logic to remove webhook mutations when diffing. If you use server side diff and don't explicitly add webhook mutations and see this, let me know, as it could be a bug in that logic.

[diranged]

Which diffing strategy are you using? Is it default or server side diff?

Hmm. We have not made any changes - so whatever the default is in the latest ArgoCD.

[andrii-korotkov-verkada]

Then you are using legacy, at least I don't think we've switched to server side diff by default.
Try that out https://argo-cd.readthedocs.io/en/stable/user-guide/diff-strategies/#enabling-it.

[diranged]

For now - the ignoredifferences fix works for us.. Going into the holidays, I'm hesitant to change the apply method (we have thousands of applications across dozens of argocd installations... it would be risky!). Either way, just reporting here that this is definitely a bug that should be fixed (regardless of the apply-policy).

[cmontemuino]

btw, I think this comment is relevant, especially for Kyverno users: kyverno/kyverno#9451 (comment)