chore(deps): upgrade k8s version and client-go

[fengshunli]

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

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?
• I've updated documentation as required by this PR.
• 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.

Please see Contribution FAQs if you have questions about your pull-request.

[codecov]

Codecov Report

Attention: 109 lines in your changes are missing coverage. Please review.

Comparison is base (045f5b1) 50.00% compared to head (e0cd79b) 50.03%.
Report is 16 commits behind head on master.

❗ Current head e0cd79b differs from pull request most recent head d4eec56. Consider uploading reports for the commit d4eec56 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #13965      +/-   ##
==========================================
+ Coverage   50.00%   50.03%   +0.03%     
==========================================
  Files         266      266              
  Lines       45713    45960     +247     
==========================================
+ Hits        22858    22997     +139     
- Misses      20621    20711      +90     
- Partials     2234     2252      +18     

Files	Coverage Δ
cmd/util/app.go	56.22% <100.00%> (ø)
util/argo/normalizers/corev1_known_types.go	100.00% <100.00%> (ø)
util/db/db.go	86.66% <100.00%> (+2.88%)	⬆️
applicationset/utils/clusterUtils.go	68.33% <0.00%> (ø)
cmd/argocd/commands/admin/cluster.go	0.00% <0.00%> (ø)
cmd/util/project.go	12.14% <0.00%> (ø)
server/application/terminal.go	12.38% <0.00%> (ø)
util/db/cluster.go	59.87% <0.00%> (ø)
util/db/helmrepository.go	0.00% <0.00%> (ø)
cmd/argocd/commands/app.go	21.40% <33.33%> (-0.02%)	⬇️
... and 8 more

... and 1 file with indirect coverage changes

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

[crenshaw-dev]

I think this is a subtle change in behavior. To keep with previous behavior, we should just log.debug and then continue.

[fengshunli]

done

[crenshaw-dev]

Hey, @blakepettersson, I wonder if you could shed light on some e2e test failures.

    app_management_ns_test.go:2092: 
        	Error Trace:	/Users/mcrenshaw/go/src/github.com/argoproj/argo-cd/test/e2e/app_management_ns_test.go:2092
        	            				/Users/mcrenshaw/go/src/github.com/argoproj/argo-cd/test/e2e/fixture/app/expectation.go:119
        	            				/Users/mcrenshaw/go/src/github.com/argoproj/argo-cd/test/e2e/fixture/app/consequences.go:30
        	            				/Users/mcrenshaw/go/src/github.com/argoproj/argo-cd/test/e2e/app_management_ns_test.go:2084
        	Error:      	Not equal: 
        	            	expected: map[string]string{"foo":"bar", "test":"true"}
        	            	actual  : map[string]string{"foo":"bar"}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,4 +1,3 @@
        	            	-(map[string]string) (len=2) {
        	            	- (string) (len=3) "foo": (string) (len=3) "bar",
        	            	- (string) (len=4) "test": (string) (len=4) "true"
        	            	+(map[string]string) (len=1) {
        	            	+ (string) (len=3) "foo": (string) (len=3) "bar"
        	            	 }
        	Test:       	TestNamespacedNamespaceAutoCreationWithPreexistingNs
    app_management_ns_test.go:2093: 
        	Error Trace:	/Users/mcrenshaw/go/src/github.com/argoproj/argo-cd/test/e2e/app_management_ns_test.go:2093
        	            				/Users/mcrenshaw/go/src/github.com/argoproj/argo-cd/test/e2e/fixture/app/expectation.go:119
        	            				/Users/mcrenshaw/go/src/github.com/argoproj/argo-cd/test/e2e/fixture/app/consequences.go:30
        	            				/Users/mcrenshaw/go/src/github.com/argoproj/argo-cd/test/e2e/app_management_ns_test.go:2084
        	Error:      	Not equal: 
        	            	expected: map[string]string{"argocd.argoproj.io/sync-options":"ServerSideApply=true", "bar":"bat", "something":"whatevs"}
        	            	actual  : map[string]string{"argocd.argoproj.io/sync-options":"ServerSideApply=true", "bar":"bat"}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,5 +1,4 @@
        	            	-(map[string]string) (len=3) {
        	            	+(map[string]string) (len=2) {
        	            	  (string) (len=31) "argocd.argoproj.io/sync-options": (string) (len=20) "ServerSideApply=true",
        	            	- (string) (len=3) "bar": (string) (len=3) "bat",
        	            	- (string) (len=9) "something": (string) (len=7) "whatevs"
        	            	+ (string) (len=3) "bar": (string) (len=3) "bat"
        	            	 }

Basically, as I understand it, managedNamespaceMetadata is meant to be additive with respect to labels and annotations. But instead, after the k8s library upgrade, it's deleting existing labels and annotations in favor of only the stuff configured in managedNamespaceMetadata.

Do you have any hunches why this might be the case?

[crenshaw-dev]

This is so weird. If I manually add a label to the ns, add a label to managedNamespaceMetadata, and then sync, both labels remain. The server side apply seems to have server side applied correctly.

But in the e2e test, the original labels/annotations get blown away.

[crenshaw-dev]

A small clue: if I delete the last-applied-configuration before argocd app set --dest-namespace && argocd app sync, then the existing labels/annotations aren't blown away. So my partial theory is that the way the new k8s libs handle that annotation has changed.

[crenshaw-dev]

Ah! This is what it is: kubernetes/kubectl#1337

Users should follow the documented upgrade path before using server-side-apply features on a resource previously managed through client-side-apply so they can think about who should own the old fields in the new paradigm: https://kubernetes.io/docs/reference/using-api/server-side-apply/#upgrading-from-client-side-apply-to-server-side-apply

Our test uses client-side apply to create the ns and then server-side apply to manage it. In other words, we can't guarantee we won't blow away users' metadata when an app takes over management of a namespace.

Here's my suggestion:

1. Update the tests to create the ns with server-side instead of client-side apply.
2. Add a note to the managedNamespaceMetadata documentation warning the user that, if their namespace was previously applied using client-side apply, old annotations and labels may be lost. They should follow the documentation linked above to upgrade their namespace to server-side apply before turning on metadata management in Argo CD.

[crenshaw-dev]

@fengshunli I opened a PR against your branch.

I also opened a draft PR so we can see how the tests behave: #13984

[crenshaw-dev]

Actually, instead of creating the ns using server-side apply, let's just update the test to expect the client-side fields to be deleted. Updated my PRs.

[fengshunli]

@fengshunli I opened a PR against your branch.

I also opened a draft PR so we can see how the tests behave: #13984

Your branch e2e test still fails @crenshaw-dev

[blakepettersson]

Here's my suggestion:

1. Update the tests to create the ns with server-side instead of client-side apply.
2. Add a note to the managedNamespaceMetadata documentation warning the user that, if their namespace was previously applied using client-side apply, old annotations and labels may be lost. They should follow the documentation linked above to upgrade their namespace to server-side apply before turning on metadata management in Argo CD.

@crenshaw-dev if I understand correctly in order for fields to be managed by SSA, we'd need to first kubectl apply --server-side the relevant resource. Could we not do something like

1. If the liveNs does not have argocd.argoproj.io/sync-options: ServerSideApply=true present, but managedNamespaceMetadata != nil, first perform an empty SSA on the namespace
2. And then once the namespace has been successfully SSA'd we'd then add the relevant annotations/labels on the namespace

I'm guessing this would need some work in gitops-engine for this to be possible, but would be more preferable than risking to lose existing annotations and labels.

And adding a disclaimer stating to the effect "once you go SSA you can't go back (easily)" would hardly hurt either. WDYT?

[crenshaw-dev]

@fengshunli fixed it. But now reading Blake's comments, I'm not sure my fix is best. :-)

@blakepettersson, makes sense to me! So, applying a bare-bones NS resource (apiVersion, kind, name, namespace) will "convert" the resource to SSA, and from then on SSA applies should be additive?

I'll attempt the gitops-engine changes, unless @fengshunli beats me to it. :-)

[blakepettersson]

@blakepettersson, makes sense to me! So, applying a bare-bones NS resource (apiVersion, kind, name, namespace) will "convert" the resource to SSA, and from then on SSA applies should be additive?

I believe that to be the case, perhaps @leoluz can confirm?

[crenshaw-dev]

@blakepettersson just did a quick test with kubectl 1.26.2. Applying an "empty" ns object with --server-side doesn't preserve labels and annotations.

I think we might have to re-construct the "live" object, complete with its existing labels and annotations, and apply those with SSA to "covert" the fields to SSA management.

[crenshaw-dev]

Actually, that doesn't work either. The Server-side apply (without the pre-existing labels/annotations) does not preserve the previously-existing labels and annotations just because they were server-side applied once.

I think the breaking change might be the only option.

[blakepettersson]

@crenshaw-dev hmm.

I think we might have to re-construct the "live" object, complete with its existing labels and annotations, and apply those with SSA to "convert" the fields to SSA management.

Wouldn't that work? I got this response from ChatGPT, but I can verify on my end if that's the case when I get some time if you don't beat me to it 😄

Yes, there is a way to preserve labels in Kubernetes that have been previously applied with client-side when a resource gets converted to using server-side apply. Here's a rough sequence of steps to do it:

1. Fetch the existing version of the object.

kubectl get <resource-type> <resource-name> -o yaml > resource.yaml

This command fetches the current state of the object and writes it to resource.yaml.

2. Edit resource.yaml to add or change fields as necessary, preserving the existing metadata.labels field.

3. Apply the object using server-side apply.

kubectl apply --server-side -f resource.yaml

This command will apply the changes in resource.yaml to the object in the cluster. Because the metadata.labels field in resource.yaml was preserved, it should remain unchanged in the object in the cluster.

[crenshaw-dev]

@blakepettersson I think the ChatGPT answer is only true if your source of truth continues to contain the old labels/annotations. In our case, it will not. We'd have to persist the old labels/annotations somewhere besides the namespace resource itself.

[crenshaw-dev]

I just tested locally, and kubectl apply --server-side does remove old labels/annotations even if they've been server-side-applied once.

[leoluz]

I just tested locally, and kubectl apply --server-side does remove old labels/annotations even if they've been server-side-applied once.

@crenshaw-dev It will if you apply the state under the same manager. For the fields to be preserved, it needs to be owned by a different manager.

[crenshaw-dev]

@blakepettersson yep I like that change. Cherry-picked and pushed!

[blakepettersson]

@crenshaw-dev I have a PR to fix the merge conflicts on fengshunli#6 (Ideally I'd like to just edit the conflicts here but cannot...)

[crenshaw-dev]

@blakepettersson wanna open a new PR directly from your branch? @fengshunli will still get credit in the squashed commit.

[blakepettersson]

Closing since this has been merged with #15852