🐛 Allow SSA after normal resource creation #3346

filipcirtog · 2025-10-10T11:40:14Z

Addressing issue #3344

Issue:

When using Server-Side Apply (SSA) users could encounter the following error: object %v does not implement the protobuf marshalling interface and cannot be encoded to a *client.applyconfigurationRuntimeObject protobuf message.

Root Cause:

By default, the content-type is set to "protobuf" when creating in-tree resources. If resources were previously created without using Server-Side Apply (SSA), attempting an SSA operation later caused the GroupVersionKind (GVK) to be identified, halting execution before a new client with the forceDisableProtoBuf option could be created. This resulted in unintended generation of Protobuf requests, which SSA does not support, causing the previous error.

Changes Made:

The implementation now includes a cache key that combines both the actual GVK (GroupVersionKind) and the forceDisableProtoBuf flag. This ensures that we now support up to two distinct clients for each GVK: one with forceDisableProtoBuf enabled and another without it.

linux-foundation-easycla · 2025-10-10T11:40:20Z

The committers listed above are authorized under a signed CLA.

✅ login: filipcirtog / name: Filip Cirtog (45ba50e, 4e755a1, a285a4f)

k8s-ci-robot · 2025-10-10T11:40:23Z

Welcome @filipcirtog!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-10-10T11:40:23Z

Hi @filipcirtog. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

alvaroaleman · 2025-10-10T11:43:02Z

/ok-to-test

alvaroaleman · 2025-10-10T11:43:35Z

pkg/client/client_rest_resources.go

 	c.mu.RUnlock()

-	if known {
+	if known && !forceDisableProtoBuf {


This means that we will create a new client for every SSA requests. Can we avoid that?

I've noticed the same thing and am currently looking into it.

@alvaroaleman we could also add this to the Apply req. SetHeader("Accept", "application/json").WDYT?

That will override any user-configured preference and is something that needs maintenace. Upstream is for example working on cbor which is more efficient than json. Can we instead just key the cached clients by both known and forceDisableProtobuf?

Yes, I agree that this is the best solution. While forceDisableProtoBuf is always true for unstructuredResource, I also made the changes for consistency. Please let me know if you have any additional feedback or suggestions to improve this further.

alvaroaleman · 2025-10-10T20:03:47Z

pkg/client/client_rest_resources.go

 	// unstructuredResourceByType stores unstructured type metadata
-	unstructuredResourceByType map[schema.GroupVersionKind]*resourceMeta
-	mu                         sync.RWMutex
+	unstructuredResourceByType map[cacheKey]*resourceMeta


From what I can tell the only reason for having these two was that unstructured had the same problem of never being able to use proto - Would you mind to de-duplicate them into a single resourceByType?

That's right. I have addressed this. Thank you!

alvaroaleman · 2025-10-10T20:04:31Z

pkg/client/client_test.go

+					TypeMeta: metav1.TypeMeta{
+						Kind:       "Secret",
+						APIVersion: "v1",
+					},


This isn't needed and can be removed

Suggested change

TypeMeta: metav1.TypeMeta{

Kind: "Secret",

APIVersion: "v1",

},

alvaroaleman · 2025-10-10T20:06:14Z

pkg/client/client_test.go

+				err = cl.Apply(ctx, secretApplyConfiguration, &client.ApplyOptions{FieldManager: "test-manager"})
+				Expect(err).NotTo(HaveOccurred())
+
+				cm, err = clientset.CoreV1().Secrets(ptr.Deref(secretApplyConfiguration.GetNamespace(), "")).Get(ctx, ptr.Deref(secretApplyConfiguration.GetName(), ""), metav1.GetOptions{})


Nit

Suggested change

cm, err = clientset.CoreV1().Secrets(ptr.Deref(secretApplyConfiguration.GetNamespace(), "")).Get(ctx, ptr.Deref(secretApplyConfiguration.GetName(), ""), metav1.GetOptions{})

secret, err = clientset.CoreV1().Secrets(ptr.Deref(secretApplyConfiguration.GetNamespace(), "")).Get(ctx, ptr.Deref(secretApplyConfiguration.GetName(), ""), metav1.GetOptions{})

alvaroaleman · 2025-10-10T20:16:43Z

/cherrypick release-0.22

k8s-infra-cherrypick-robot · 2025-10-10T20:16:46Z

@alvaroaleman: once the present PR merges, I will cherry-pick it on top of release-0.22 in a new PR and assign it to you.

In response to this:

/cherrypick release-0.22

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

alvaroaleman

Thanks!

k8s-ci-robot · 2025-10-10T20:35:24Z

LGTM label has been added.

Git tree hash: 7701c3b0875d44c496bc71f0b70dbd1d4837e36d

k8s-ci-robot · 2025-10-10T20:35:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, filipcirtog

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

k8s-infra-cherrypick-robot · 2025-10-10T20:37:39Z

@alvaroaleman: new pull request created: #3348

In response to this:

/cherrypick release-0.22

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

sbueringer · 2025-10-13T12:33:23Z

/lgtm

This change introduces the possibility of creating multiple clients, which might allow simultaneous requests from different entities. This could lead to concurrency considerations.

How much of a problem do we think this is? (I don't really see the problem)

filipcirtog · 2025-10-13T12:41:09Z

@sbueringer It was this comment that triggered me to have that conclusion.
// It's better to do creation work twice than to not let multiple people make requests at once

However, thanks to Alvaro's feedback, I've updated the implementation to include a cache key that combines both the actual GVK (GroupVersionKind) and the forceDisableProtoBuf flag. This ensures that we now support up to two distinct clients for each GVK: one with forceDisableProtoBuf enabled and another without it. This approach is an improvement over my initial PR, which created a new client for forceDisableProtoBuf each time.

sbueringer · 2025-10-13T12:49:56Z

Got it, thx for the quick response. Let's please update the PR description

filipcirtog · 2025-10-13T12:59:14Z

Done. Thank you. Please let me know if there is anything more to change.

sbueringer · 2025-10-13T13:53:06Z

All good, thank you!

fix: enable apply after normal create

45ba50e

k8s-ci-robot requested review from alvaroaleman and sbueringer October 10, 2025 11:40

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 10, 2025

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 10, 2025

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

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 10, 2025

alvaroaleman reviewed Oct 10, 2025

View reviewed changes

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 10, 2025

improvements: cache now considers disable protobuf flag for lookup

a285a4f

alvaroaleman reviewed Oct 10, 2025

View reviewed changes

feedback improvements

4e755a1

alvaroaleman approved these changes Oct 10, 2025

View reviewed changes

k8s-ci-robot assigned alvaroaleman Oct 10, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2025

k8s-ci-robot merged commit b9bccfd into kubernetes-sigs:main Oct 10, 2025
10 checks passed

k8s-infra-cherrypick-robot mentioned this pull request Oct 10, 2025

[release-0.22] 🐛 Allow SSA after normal resource creation #3348

Merged

alvaroaleman mentioned this pull request Oct 10, 2025

Deserialization Error During Server-Side Apply (SSA) with Protobuf Content Type #3344

Closed

	cm, err = clientset.CoreV1().Secrets(ptr.Deref(secretApplyConfiguration.GetNamespace(), "")).Get(ctx, ptr.Deref(secretApplyConfiguration.GetName(), ""), metav1.GetOptions{})
	secret, err = clientset.CoreV1().Secrets(ptr.Deref(secretApplyConfiguration.GetNamespace(), "")).Get(ctx, ptr.Deref(secretApplyConfiguration.GetName(), ""), metav1.GetOptions{})

🐛 Allow SSA after normal resource creation #3346

🐛 Allow SSA after normal resource creation #3346

Conversation

filipcirtog commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 10, 2025

Uh oh!

k8s-ci-robot commented Oct 10, 2025

Uh oh!

alvaroaleman commented Oct 10, 2025

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!

alvaroaleman Oct 10, 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!

alvaroaleman commented Oct 10, 2025

Uh oh!

k8s-infra-cherrypick-robot commented Oct 10, 2025

Uh oh!

alvaroaleman left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Oct 10, 2025

Uh oh!

k8s-ci-robot commented Oct 10, 2025

Uh oh!

Uh oh!

k8s-infra-cherrypick-robot commented Oct 10, 2025

Uh oh!

sbueringer commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filipcirtog commented Oct 13, 2025

Uh oh!

sbueringer commented Oct 13, 2025

Uh oh!

filipcirtog commented Oct 13, 2025

Uh oh!

sbueringer commented Oct 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

filipcirtog commented Oct 10, 2025 •

edited

Loading

linux-foundation-easycla bot commented Oct 10, 2025 •

edited

Loading

alvaroaleman Oct 10, 2025 •

edited

Loading

sbueringer commented Oct 13, 2025 •

edited

Loading