⚠️ Add native SSA support #3253

alvaroaleman · 2025-07-05T18:24:16Z

This change adds native server-side apply support to the client by extending it with an Apply method that takes a runtime.ApplyConfiguration.

Ref #3183

k8s-ci-robot · 2025-07-05T18:24:19Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

k8s-ci-robot · 2025-07-05T18:24:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alvaroaleman · 2025-07-05T18:32:05Z

pkg/client/fieldvalidation.go

@@ -53,6 +54,10 @@ func (c *clientWithFieldValidation) Patch(ctx context.Context, obj Object, patch
 	return c.client.Patch(ctx, obj, patch, append([]PatchOption{c.validation}, opts...)...)
 }

+func (c *clientWithFieldValidation) Apply(ctx context.Context, obj runtime.ApplyConfiguration, opts ...ApplyOption) error {
+	return errors.New("Apply is not supported with field validation")


TODO: It actually is, just not part of the upstream apply options

Never mind, this doesn't work - SSA will error out if there are unknown fields even when FieldValidation=None is set. What is the best way for us to deal with that in this wrapper, just erroring doesn't seem great...?

There's no way to adjust the interfaces to have something separate for Apply without making it awkward to use I guess?

Could you elaborate on what you mean by adjust the interfaces to have something separate for Apply?

Part of this PR adds Appply to the Client interface right? What if there was an ApplyClient extension to the interface that embeds the Client interface? Would that allow us to implement the Apply within only the different client mutators that we are able to support?

The result of that would be that you have two clients, one for apply, one for everything else - You can also achieve that with what we have here aleady by storing the client before wrapping and using that for apply or not?

The best I can think of in these wrappers is a config option to not error when apply is used 🤔

alvaroaleman · 2025-07-05T18:33:09Z

pkg/client/namespaced_client.go

@@ -147,6 +148,10 @@ func (n *namespacedClient) Patch(ctx context.Context, obj Object, patch Patch, o
 	return n.client.Patch(ctx, obj, patch, opts...)
 }

+func (n *namespacedClient) Apply(ctx context.Context, obj runtime.ApplyConfiguration, opts ...ApplyOption) error {
+	return errors.New("Apply is not supported on namespaced client")


TODO: Can we make this work? All the ACs should have a SetNamespace, this would be nice to have

This is actualy pretty difficult, I'd prefer it if we punt on this for now, it can still be done in a follow-up if there is interest.

JoelSpeed · 2025-07-07T10:39:26Z

pkg/client/applyconfigurations.go

+func (u *unstructuredApplyConfiguration) IsApplyConfiguration() {}
+
+// ApplyConfigurationFromUnstructured creates a runtime.ApplyConfiguration from an *unstructured.Unstructured object.
+func ApplyConfigurationFromUnstructured(u *unstructured.Unstructured) runtime.ApplyConfiguration {


I'm surprised this doesn't exist anywhere in the API machinery already today 🤔

Upstream didn't want that: kubernetes/kubernetes#132194 (comment)

There are some pretty strong arguments against creating this in the thread there, what would the likely impact to CR users be if we decided that ApplyConfiguration through CR was only supported for concrete types and not unstructured?

I guess my main concern would be

It doesn't, as the typed object has zero values which will then be present in the unstructured.

How do we elevate this message to help prevent folks from shooting themselves in the foot?

In general, marshalling zero values is a bad idea, but there's a lot of legacy required fields out there without omitempty assigned

There are some pretty strong arguments against creating this in the thread there, what would the likely impact to CR users be if we decided that ApplyConfiguration through CR was only supported for concrete types and not unstructured?

We break all existing usages of Apply in controller-runtime and make it impossible to apply objects for which you do not have the AC, for example because you read them from disk.

How do we elevate this message to help prevent folks from shooting themselves in the foot?

Its a nonsensical argument. That issue only exists if someone converts from concrete type to unstructured, there is no reason to do that. Using unstructured.Unstrucutred itself is fine, as it has no zero values. People could also use a serializer to convert from conrete type to applyconfig and the exact same issue would arise.

I think adding godoc to the functions here to explain that this should not be used when converting structured types to unstructured would cover us in terms of messaging

But what is the reason why anyone would ever do that? And what makes us assume that is any more likely than converting from API type to AC type?

I don't know, but since it's possible, someone will probably be doing it. Since we know it causes issues, IMO it makes sense to document that this is not something we recommend.

I think I'd put this down to Hyrum's Law

JoelSpeed · 2025-07-07T10:42:51Z

pkg/client/fake/client_test.go

+
+		err := cl.Apply(context.Background(), obj, client.FieldOwner("\x00"))
+		Expect(err).To(HaveOccurred())
+		Expect(apierrors.IsInvalid(err)).To(BeTrue())


Nit, with BeTrue(), when this fails it's hard to understand why, you just get expected False to be True, if you add a comment after BeTrue() with the context it's much easier to debug failures

Suggested change

Expect(apierrors.IsInvalid(err)).To(BeTrue())

Expect(apierrors.IsInvalid(err)).To(BeTrue(), "Expected error to be an invalid error")

Makes sense, updated it

JoelSpeed · 2025-07-07T10:46:59Z

pkg/client/fieldvalidation.go

@@ -53,6 +54,10 @@ func (c *clientWithFieldValidation) Patch(ctx context.Context, obj Object, patch
 	return c.client.Patch(ctx, obj, patch, append([]PatchOption{c.validation}, opts...)...)
 }

+func (c *clientWithFieldValidation) Apply(ctx context.Context, obj runtime.ApplyConfiguration, opts ...ApplyOption) error {
+	return errors.New("Apply is not supported with field validation")


There's no way to adjust the interfaces to have something separate for Apply without making it awkward to use I guess?

JoelSpeed · 2025-07-07T10:49:04Z

pkg/client/options.go

+
+	// Force is going to "force" Apply requests. It means user will
+	// re-acquire conflicting fields owned by other people.
+	Force *bool `json:"force" protobuf:"varint,2,opt,name=force"`


Do we expose a force applyoption already?

Good question, there is already ForceOwnership, I updated it to also support Apply and not only Patch

This change adds native server-side apply support to the client by extending it with an `Apply` method that takes an `runtime.ApplyConfiguration`.

This reverts commit ad1966a. This doesn't work, the server will always error on additional fields when using SSA, even when fieldValiation=None is configured.

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 5, 2025

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 5, 2025

k8s-ci-robot requested review from sbueringer and varshaprasad96 July 5, 2025 18:24

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 5, 2025

alvaroaleman force-pushed the ssa branch from 536abec to a148900 Compare July 5, 2025 18:39

alvaroaleman commented Jul 5, 2025

View reviewed changes

alvaroaleman added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 6, 2025

alvaroaleman marked this pull request as ready for review July 6, 2025 13:23

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 6, 2025

k8s-ci-robot requested review from inteon and JoelSpeed July 6, 2025 13:23

alvaroaleman changed the title ~~✨ Add native SSA support~~ ⚠️ Add native SSA support Jul 6, 2025

JoelSpeed reviewed Jul 7, 2025

View reviewed changes

alvaroaleman added 4 commits July 7, 2025 16:43

✨ Add native SSA support

da779f9

This change adds native server-side apply support to the client by extending it with an `Apply` method that takes an `runtime.ApplyConfiguration`.

Field validation

3165151

Revert "Field validation"

3c81fa7

This reverts commit ad1966a. This doesn't work, the server will always error on additional fields when using SSA, even when fieldValiation=None is configured.

Explain why we don't update the namespaced client

d20f30a

alvaroaleman force-pushed the ssa branch from f92f587 to d20f30a Compare July 7, 2025 14:43

Client: Force ownership supports Apply

3f2934c

zachaller mentioned this pull request Jul 8, 2025

Switch to SSA argoproj-labs/gitops-promoter#398

Open

	Expect(apierrors.IsInvalid(err)).To(BeTrue())
	Expect(apierrors.IsInvalid(err)).To(BeTrue(), "Expected error to be an invalid error")

⚠️ Add native SSA support #3253

Are you sure you want to change the base?

⚠️ Add native SSA support #3253

Conversation

alvaroaleman commented Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jul 5, 2025

Uh oh!

k8s-ci-robot commented Jul 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvaroaleman Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alvaroaleman commented Jul 5, 2025 •

edited

Loading

alvaroaleman Jul 6, 2025 •

edited

Loading