fix: fix calculating patch for respect ignore diff feature #17693

alexmt · 2024-04-01T22:07:39Z

When ignoreDifferences and RespectIgnoreDifferences=true are used the Argo CD needs to "copy" values of ignored paths from the k8s resource of the managed cluster to the expected resource. This way "ignored" fields won't be modified. The "copying" was implemented incorrectly for arrays. PR fixes the implementation.

It is still impossible to ignoring differences in selected array elements. Created #17694 to track fix.

crenshaw-dev · 2024-04-02T01:10:01Z

@alexmt I'm not sure how much you're trying to tackle in this PR, but this issue has a lot of currently-broken use cases documented, and @blakepettersson converted many (all?) of them to unit tests which maybe could be borrowed in this PR.

alexmt · 2024-04-02T02:51:44Z

@crenshaw-dev thank you for letting me know @crenshaw-dev . The PR solves the #9678 partially: it is possible to ignore array completely but still impossible to ignore one element of the array. I propose to merge PR to unblock users who want to ignore array fields and continue working on improvement in #17694

gdsoumya

LGTM

Signed-off-by: Jesse Suen <jesse@akuity.io>

controller/sync_test.go

jessesuen · 2024-04-02T19:55:19Z

controller/sync_test.go

+				JQPathExpressions: []string{".spec.routes[]"},
+				//JSONPointers: []string{"/spec/routes"},


Is there any value in trying both jqpathexpression and jsonpointer? e.g. Should the test iterate the two options?

controller/sync_test.go

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

alexmt · 2024-04-03T22:00:04Z

The e2e test was failing because my change introduced another bug. I had to implement strategic merge patch support and partially resolve #17694 . So now user can ignore array elements with merge keys in well known k8s types. CRDs are not supported (same as in diffing).

I've added most of tests from #14602 except Rollouts test (since it is CRD) and mutating webhook test (it is trying to merge arrays without merge key).

PTAL

blakepettersson · 2024-04-03T22:09:12Z

I've added most of tests from #14602 except mutating webhook test (it is trying to merge arrays without merge key).

I might be missing something here, but doesn't the mutating webhook test have merge keys in its arrays?

webhooks:
  - admissionReviewVersions:
      - v1beta1
    clientConfig:
      caBundle: >-
        something-new
      service:
        name: aws-load-balancer-webhook-service
        namespace: kube-system
        path: /mutate-v1-pod
    failurePolicy: Fail
    name: mpod.elbv2.k8s.aws # <- merge key?
# Omitted other elements

Or do you mean that because each webhook has a rules array that does not have merge keys, strategic merge patching would not work?

blakepettersson · 2024-04-03T22:19:48Z

controller/sync_test.go

+		second := env[1]
+		third := env[2]
+
+		// Currently the defined order at this time is the insertion order of the target manifest.


Is this still true when using SMP?

alexmt · 2024-04-04T16:06:22Z

@blakepettersson , thank you for review. I've clicked auto-merge and Github automatically merged PR once CI is green. Checking your comment about mutating webhook test

alexmt · 2024-04-04T17:51:56Z

/cherry-pick release-2.7

gcp-cherry-pick-bot · 2024-04-04T17:52:01Z

Cherry-pick failed with Merge error 44da2063c7e915d0fc8342cdd834e6869b2910bc into temp-cherry-pick-c40e0a-release-2.7

alexmt · 2024-04-04T17:52:52Z

/cherry-pick release-2.8

gcp-cherry-pick-bot · 2024-04-04T17:52:56Z

Cherry-pick failed with Merge error 44da2063c7e915d0fc8342cdd834e6869b2910bc into temp-cherry-pick-c40e0a-release-2.8

alexmt · 2024-04-04T17:54:09Z

/cherry-pick release-2.9

gcp-cherry-pick-bot · 2024-04-04T17:54:14Z

Cherry-pick failed with Merge error 44da2063c7e915d0fc8342cdd834e6869b2910bc into temp-cherry-pick-c40e0a-release-2.9

alexmt · 2024-04-04T17:54:19Z

/cherry-pick release-2.10

gcp-cherry-pick-bot · 2024-04-04T17:54:23Z

Cherry-pick failed with Merge error 44da2063c7e915d0fc8342cdd834e6869b2910bc into temp-cherry-pick-c40e0a-release-2.10

…17693) * test: unit test for respectIgnoreDifferences bug Signed-off-by: Jesse Suen <jesse@akuity.io> * test: simplify unit test Signed-off-by: Jesse Suen <jesse@akuity.io> * fix: fix calculating patch for respect ignore diff feature Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> --------- Signed-off-by: Jesse Suen <jesse@akuity.io> Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Co-authored-by: Jesse Suen <jesse@akuity.io>

* test: unit test for respectIgnoreDifferences bug Signed-off-by: Jesse Suen <jesse@akuity.io> * test: simplify unit test Signed-off-by: Jesse Suen <jesse@akuity.io> * fix: fix calculating patch for respect ignore diff feature Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> --------- Signed-off-by: Jesse Suen <jesse@akuity.io> Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Co-authored-by: Jesse Suen <jesse@akuity.io>

…17733) * test: unit test for respectIgnoreDifferences bug * test: simplify unit test * fix: fix calculating patch for respect ignore diff feature --------- Signed-off-by: Jesse Suen <jesse@akuity.io> Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Co-authored-by: Jesse Suen <jesse@akuity.io>

* fix: elements should be optional (argoproj#17424) (argoproj#17510) A bug was reported, where an applicationset with an empty elements array, when created with `argocd appset create <filename>.yaml` gets a `...list.elements: Required value` error. My hypothesis is that when calling the K8s API, golang JSON marshalling mangles the empty `elements` array to `nil`, rather than creating an empty array when submitting the `POST`. Still need to figure out why the same setup seemingly works fine when the same appset is in an app-of-apps. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com> * Merge pull request from GHSA-jhwx-mhww-rgc3 * sec: limit helm index max size Signed-off-by: pashakostohrys <pavel@codefresh.io> * sec: limit helm index max size Signed-off-by: pashakostohrys <pavel@codefresh.io> * feat: fix tests and linter Signed-off-by: pashakostohrys <pavel@codefresh.io> --------- Signed-off-by: pashakostohrys <pavel@codefresh.io> * Bump version to 2.10.5 (argoproj#17654) Signed-off-by: GitHub <noreply@github.com> Co-authored-by: pasha-codefresh <pasha-codefresh@users.noreply.github.com> * fix cosign (argoproj#17656) Signed-off-by: Justin Marquis <justin@akuity.io> * chore(deps): bump webpack-dev-middleware from 5.3.1 to 5.3.4 in /ui (argoproj#17598) (argoproj#17686) Bumps [webpack-dev-middleware](https://github.com/webpack/webpack-dev-middleware) from 5.3.1 to 5.3.4. - [Release notes](https://github.com/webpack/webpack-dev-middleware/releases) - [Changelog](https://github.com/webpack/webpack-dev-middleware/blob/v5.3.4/CHANGELOG.md) - [Commits](webpack/webpack-dev-middleware@v5.3.1...v5.3.4) --- updated-dependencies: - dependency-name: webpack-dev-middleware dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * fix(ui): Fix color generation for pod name in logs viewer. Fixes argoproj#17704 (argoproj#17706) (argoproj#17710) * Fix color generation for pod name in logs viewer * Add rebuy to users.md --------- Signed-off-by: Philipp Trulson <der-eismann@users.noreply.github.com> Co-authored-by: Philipp Trulson <der-eismann@users.noreply.github.com> * fix: fix calculating patch for respect ignore diff feature (argoproj#17693) * test: unit test for respectIgnoreDifferences bug Signed-off-by: Jesse Suen <jesse@akuity.io> * test: simplify unit test Signed-off-by: Jesse Suen <jesse@akuity.io> * fix: fix calculating patch for respect ignore diff feature Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> --------- Signed-off-by: Jesse Suen <jesse@akuity.io> Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Co-authored-by: Jesse Suen <jesse@akuity.io> * fix(security): use Chainguard fork of git-urls (argoproj#17732) (argoproj#17735) Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * Bump version to 2.10.6 (argoproj#17744) Signed-off-by: GitHub <noreply@github.com> Co-authored-by: alexmt <alexmt@users.noreply.github.com> * Merge pull request from GHSA-2gvw-w6fj-7m3c Signed-off-by: pashakostohrys <pavel@codefresh.io> * Bump version to 2.10.7 (argoproj#17831) Signed-off-by: GitHub <noreply@github.com> Co-authored-by: pasha-codefresh <pasha-codefresh@users.noreply.github.com> * fix: docker build fails due to "The repository 'http://deb.debian.org/debian buster-backports Release' does not have a Release file." Signed-off-by: pashakostohrys <pavel@codefresh.io> * fix: codegen and e2e tests in release-2.10 (argoproj#17844) * fix: codegen and e2e tests Signed-off-by: pashakostohrys <pavel@codefresh.io> * fix: codegen and e2e tests Signed-off-by: pashakostohrys <pavel@codefresh.io> --------- Signed-off-by: pashakostohrys <pavel@codefresh.io> * chore: upgrade redis to 7.0.15 (argoproj#17666) Upgrade to latest stable 7.0.x version to fix CVEs: CVE-2023-41056 Signed-off-by: Tais P. Hansen <taishansen@gmail.com> * Merge pull request from GHSA-9m6p-x4h2-6frq * feat: limit jq.Run with timeout Signed-off-by: pashakostohrys <pavel@codefresh.io> * feat: ignore normalizer jq execution timeout as env variable Signed-off-by: pashakostohrys <pavel@codefresh.io> * feat: customize error message and add doc section Signed-off-by: pashakostohrys <pavel@codefresh.io> * feat: improve log and change a way how to get variable Signed-off-by: pashakostohrys <pavel@codefresh.io> * chore: fix import`s order Signed-off-by: pashakostohrys <pavel@codefresh.io> * chore: rename variable inside sts Signed-off-by: pashakostohrys <pavel@codefresh.io> * chore: fix import order Signed-off-by: pashakostohrys <pavel@codefresh.io> * chore: fix import`s order Signed-off-by: pashakostohrys <pavel@codefresh.io> --------- Signed-off-by: pashakostohrys <pavel@codefresh.io> * Merge pull request from GHSA-9m6p-x4h2-6frq * feat: limit jq.Run with timeout Signed-off-by: pashakostohrys <pavel@codefresh.io> * feat: ignore normalizer jq execution timeout as env variable Signed-off-by: pashakostohrys <pavel@codefresh.io> * feat: customize error message and add doc section Signed-off-by: pashakostohrys <pavel@codefresh.io> * feat: improve log and change a way how to get variable Signed-off-by: pashakostohrys <pavel@codefresh.io> * chore: fix import`s order Signed-off-by: pashakostohrys <pavel@codefresh.io> * chore: rename variable inside sts Signed-off-by: pashakostohrys <pavel@codefresh.io> * chore: fix import order Signed-off-by: pashakostohrys <pavel@codefresh.io> * chore: fix import`s order Signed-off-by: pashakostohrys <pavel@codefresh.io> --------- Signed-off-by: pashakostohrys <pavel@codefresh.io> * fix: codegen after security fix - 2.10 (argoproj#17985) * fix: codegen after security fix Signed-off-by: pashakostohrys <pavel@codefresh.io> * fix: codegen after security fix Signed-off-by: pashakostohrys <pavel@codefresh.io> --------- Signed-off-by: pashakostohrys <pavel@codefresh.io> * Bump version to 2.10.8 (argoproj#17990) Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: pasha-codefresh <39732895+pasha-codefresh@users.noreply.github.com> * fix: enable sha256 and sha512 for git ssh (argoproj#18028) (argoproj#18029) * fix: bumping the knownhosts to v1.2.2 since this contains a fix that allows for sha256 and sha512 algorithms when using git ssh * chore: remove older version of module from go sum --------- Signed-off-by: Marc Arndt <marc@marcarndt.com> Signed-off-by: Marc Arndt <m.arndt@evana.de> Co-authored-by: Marc Arndt <marc@marcarndt.com> Co-authored-by: Marc Arndt <m.arndt@evana.de> * Bump version to 2.10.9 (argoproj#18033) Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: jannfis <3942683+jannfis@users.noreply.github.com> * fix: status.sync.comparedTo should use replace patch strategy (argoproj#18061) (argoproj#18075) * fix: status.sync.comparedTo should use replace patch strategy * add e2e tests --------- Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> * chore: bump go-jose from 3.0.1 to 3.0.3 (argoproj#18102) Signed-off-by: Jayendra Parsai <jparsai@jparsai-thinkpadp1gen4i.remote.csb> Co-authored-by: Jayendra Parsai <jparsai@jparsai-thinkpadp1gen4i.remote.csb> * docs: fix 404 styling (argoproj#18094) (argoproj#18105) * docs: fix 404 styling * hack around custom tag destruction --------- Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * chore: update gitops engine for force sync option (argoproj#5882) - 2.10 (argoproj#18123) Signed-off-by: pashakostohrys <pavel@codefresh.io> * fix: Enable Redis authentication in the default installation * fix: linter issue * fix: linter issue --------- Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> Signed-off-by: pashakostohrys <pavel@codefresh.io> Signed-off-by: GitHub <noreply@github.com> Signed-off-by: Justin Marquis <justin@akuity.io> Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Philipp Trulson <der-eismann@users.noreply.github.com> Signed-off-by: Jesse Suen <jesse@akuity.io> Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: Tais P. Hansen <taishansen@gmail.com> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: Marc Arndt <marc@marcarndt.com> Signed-off-by: Marc Arndt <m.arndt@evana.de> Signed-off-by: Jayendra Parsai <jparsai@jparsai-thinkpadp1gen4i.remote.csb> Co-authored-by: gcp-cherry-pick-bot[bot] <98988430+gcp-cherry-pick-bot[bot]@users.noreply.github.com> Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: pasha-codefresh <pasha-codefresh@users.noreply.github.com> Co-authored-by: Justin Marquis <76892343+34fathombelow@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Philipp Trulson <der-eismann@users.noreply.github.com> Co-authored-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Co-authored-by: Jesse Suen <jesse@akuity.io> Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Co-authored-by: alexmt <alexmt@users.noreply.github.com> Co-authored-by: Tais P. Hansen <taisph@users.noreply.github.com> Co-authored-by: Marc Arndt <marc@marcarndt.com> Co-authored-by: Marc Arndt <m.arndt@evana.de> Co-authored-by: jannfis <3942683+jannfis@users.noreply.github.com> Co-authored-by: Jayendra Parsai <jparsai@redhat.com> Co-authored-by: Jayendra Parsai <jparsai@jparsai-thinkpadp1gen4i.remote.csb> Co-authored-by: May Zhang <may_zhang@intuit.com>

…17693) * test: unit test for respectIgnoreDifferences bug Signed-off-by: Jesse Suen <jesse@akuity.io> * test: simplify unit test Signed-off-by: Jesse Suen <jesse@akuity.io> * fix: fix calculating patch for respect ignore diff feature Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> --------- Signed-off-by: Jesse Suen <jesse@akuity.io> Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Co-authored-by: Jesse Suen <jesse@akuity.io>

alexmt force-pushed the alex/bug/respect-ignore-differences branch from a77046c to a96ba8a Compare April 2, 2024 02:24

alexmt mentioned this pull request Apr 2, 2024

ignoreDifferences and RespectIgnoreDifferences=true does allow ignoring differences in selected array elements #17694

Open

3 tasks

alexmt marked this pull request as ready for review April 2, 2024 02:48

alexmt requested a review from a team as a code owner April 2, 2024 02:48

alexmt force-pushed the alex/bug/respect-ignore-differences branch from a96ba8a to dcac669 Compare April 2, 2024 10:47

gdsoumya approved these changes Apr 2, 2024

View reviewed changes

jessesuen added 2 commits April 2, 2024 12:53

test: unit test for respectIgnoreDifferences bug

136aad7

Signed-off-by: Jesse Suen <jesse@akuity.io>

test: simplify unit test

e62c2fe

Signed-off-by: Jesse Suen <jesse@akuity.io>

alexmt force-pushed the alex/bug/respect-ignore-differences branch from dcac669 to ec031a3 Compare April 2, 2024 19:53

alexmt enabled auto-merge (squash) April 2, 2024 19:54

jessesuen reviewed Apr 2, 2024

View reviewed changes

jsvk reviewed Apr 3, 2024

View reviewed changes

controller/sync_test.go Outdated Show resolved Hide resolved

alexmt force-pushed the alex/bug/respect-ignore-differences branch from ec031a3 to 5fd0115 Compare April 3, 2024 21:53

fix: fix calculating patch for respect ignore diff feature

17e164b

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

alexmt force-pushed the alex/bug/respect-ignore-differences branch from 5fd0115 to 17e164b Compare April 3, 2024 21:55

blakepettersson reviewed Apr 3, 2024

View reviewed changes

alexmt merged commit 44da206 into argoproj:master Apr 3, 2024
27 checks passed

alexmt mentioned this pull request Apr 4, 2024

fix: fix calculating patch for respect ignore diff feature (#17693) #17733

Merged

crenshaw-dev mentioned this pull request Apr 11, 2024

Breaking Change in ApplicationSets git generator in v2.10.6 after upgrading from v2.10.5 #17801

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix calculating patch for respect ignore diff feature #17693

fix: fix calculating patch for respect ignore diff feature #17693

alexmt commented Apr 1, 2024 •

edited

Loading

crenshaw-dev commented Apr 2, 2024

alexmt commented Apr 2, 2024

gdsoumya left a comment

jessesuen Apr 2, 2024

alexmt commented Apr 3, 2024

blakepettersson commented Apr 3, 2024

blakepettersson Apr 3, 2024

alexmt commented Apr 4, 2024

alexmt commented Apr 4, 2024

gcp-cherry-pick-bot bot commented Apr 4, 2024

alexmt commented Apr 4, 2024

gcp-cherry-pick-bot bot commented Apr 4, 2024

alexmt commented Apr 4, 2024

gcp-cherry-pick-bot bot commented Apr 4, 2024

alexmt commented Apr 4, 2024

gcp-cherry-pick-bot bot commented Apr 4, 2024

		JQPathExpressions: []string{".spec.routes[]"},
		//JSONPointers: []string{"/spec/routes"},

fix: fix calculating patch for respect ignore diff feature #17693

fix: fix calculating patch for respect ignore diff feature #17693

Conversation

alexmt commented Apr 1, 2024 • edited Loading

crenshaw-dev commented Apr 2, 2024

alexmt commented Apr 2, 2024

gdsoumya left a comment

Choose a reason for hiding this comment

jessesuen Apr 2, 2024

Choose a reason for hiding this comment

alexmt commented Apr 3, 2024

blakepettersson commented Apr 3, 2024

blakepettersson Apr 3, 2024

Choose a reason for hiding this comment

alexmt commented Apr 4, 2024

alexmt commented Apr 4, 2024

gcp-cherry-pick-bot bot commented Apr 4, 2024

alexmt commented Apr 4, 2024

gcp-cherry-pick-bot bot commented Apr 4, 2024

alexmt commented Apr 4, 2024

gcp-cherry-pick-bot bot commented Apr 4, 2024

alexmt commented Apr 4, 2024

gcp-cherry-pick-bot bot commented Apr 4, 2024

alexmt commented Apr 1, 2024 •

edited

Loading