fix(app): use jsonpatch to check for changes in status field (#15126)

[vladfr]

This PR changes the way patches are computed for annotations and status. Because Helm source can have unstructured fields in helm.valuesObject, the strategic patching gives unpredictable behaviour. It was never meant to be used without a struct (see Helm Issue). So we use jsonpatch instead. This should yield the same results, and the library is already present in Argo as a dependency.

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 Toolchain Guide
• I've included "Closes [ISSUE #]" or "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've updated documentation as required by this PR. N/A
• Optional. My organization is added to USERS.md.
• 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).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.

Fixes #15126

[vladfr]

I couldn't really find a good spot for the utility function. It should be moved in gitops-engine at some point.

[jannfis]

LGTM, thanks for fixing this!

I agree that it should move into gitops-engine later on. We probably need to revive lifecycle management between gitops-engine and Argo CD before, or migrate gitops-engine back into Argo CD though.

For now, I'm fine with this code living in the application controller.

Just for clarification and my understanding, this only affects valuesObject introduced in 2.8, right?

[crenshaw-dev]

@jannfis this fixes a problem with the new field, but it changes how we calculate the app status for every field, so it's pretty darn central.

[jannfis]

@crenshaw-dev Yes, I was rather referring to the bug it fixes. So the bug is only in 2.8, and we need to cherry-pick only into that?

[crenshaw-dev]

@jannfis yep!

[jannfis]

And I would assume that since every test, including end-to-end, pass without changes that the change itself is fine. The only concern might be a performance impact of CreateTwoWayMergePatch (which is part of the Kubernetes distribution) and the JSON merge patch we pull in from github.com/evanphx/json-patch (and we seem to pull a pretty outdated version).

[vladfr]

@jannfis the helm.valuesObject field was added in 2.8.0, so I can confirm that the bugfix is meant only for 2.8.x

You have valid points about adding changes to a critical point. I figured it's safe since jsonpatch is so widely used, and it's already a dep, and tests pass :) We could definitely do a series of benches to see how performance looks.

I can dig a bit to see if there's another way to handle this fix. It seems the problem lies in how the object is being stored in status, so we can look at the way it's stored; or perhaps transform valuesObject before the patching is done (load it up and marshal again).

PS: I'm out next week, but I can take another approach after I'm back, or if someone else has input until then, feel free.

[jannfis]

@vladfr I think a good first step would be to use a recent version of jsonpatch, e.g. github.com/evanphx/json-patch/v5

[Zeratoxx]

Hi, is this PR still watched? :)

[vladfr]

Hi, sorry for my big delay here.

I've looked for a different way to address this. Unfortunately, it's not really clear how I can change the logic to store the valuesObject field separately; besides which, I'm not really sure what's causing the issue with the quotes.

So I've modified the patching code to:

• ignore valuesObject when computing the strategic merge patch
• compute diff on valuesObject using jsonpatch
• apply two patches if needed

PS: I'd like to add an e2e test for this, but I'm still struggling a bit with running these locally.

[vladfr]

@crenshaw-dev The new code now uses jsonpatch only for valuesObject. I've managed to arrive at a workable solution, not very happy on how it looks. Pointers appreciated!

PS: Sorry about this, I've mistakenly ran a rebase. We can cut another PR once this is closer to being merged. (i.e. add tests)

[PaulSonOfLars]

Thanks for the hard work @vladfr, I think that should work!

I added some tests on the my branch on #16602. Wasn't sure how you wanted to go forward regarding merging - feel free to cherry pick my changes here if you'd prefer the merge attribution in your name for finding the "fix" itself.

[vladfr]

Closing in favor of #16602