✨ Fakeclient: Add apply support #2981

alvaroaleman · 2024-10-14T03:14:23Z

This change adds apply support into the fake client.

This relies on the upstream support for this which is implemented in a new FieldManagedObjectTracker. In order to support many types, a custom multiTypeConverter is added.

The FieldManagedObjectTracker results in ManagedFields being set after any operations. As that would be a very breaking change, the fake client will by default unset them and allows to optionally leave them.

Based on all existing tests still passing, I believe this change is fully backwards-compatible (But depends on breaking changes such as a very new client-go and apimachinery and #3228).

Fixes #2341

k8s-ci-robot · 2024-10-14T03:14:25Z

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 · 2024-10-14T03:14:28Z

[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

k8s-triage-robot · 2025-02-08T22:37:05Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

alvaroaleman · 2025-02-08T22:51:59Z

/lifecycle frozen
This is a requested feature so I'd like to keep this open to demonstrate the challenges. I personally will not be working further on it though.

k8s-ci-robot · 2025-02-08T22:52:01Z

@alvaroaleman: The lifecycle/frozen label cannot be applied to Pull Requests.

In response to this:

/lifecycle frozen
This is a requested feature so I'd like to keep this open to demonstrate the challenges. I personally will not be working further on it though.

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.

k8s-triage-robot · 2025-02-09T01:43:05Z

The lifecycle/frozen label can not be applied to PRs.

This bot removes lifecycle/frozen from PRs because:

Commenting /lifecycle frozen on a PR has not worked since March 2021
PRs that remain open for >150 days are unlikely to be easily rebased

You can:

Rebase this PR and attempt to get it merged
Close this PR with /close

Please send feedback to sig-contributor-experience at kubernetes/community.

/remove-lifecycle frozen

jpbetz · 2025-02-25T16:26:22Z

pkg/client/fake/typeconverter.go

+type multiTypeConverter struct {
+	upstream []managedfields.TypeConverter
+}


Providing this composite type converter seems fine.

We have a different use case for a composite type converter in the apiserver for admission: https://github.com/kubernetes/kubernetes/blob/25e11cd1c143ef136418c33bfbbbd4f24e32e529/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/typeconverter.go#L37. But the use case is different, it takes a single static type converter and an openapi client, instead of multiple underlying type converters, so it is not useful here.

I'd like to ensure we keep this implementation unexported. Maybe add some godoc to that effect?

Yep makes sense, will do that

jpbetz · 2025-02-25T16:30:15Z

pkg/client/fake/client.go

@@ -119,6 +123,7 @@ type ClientBuilder struct {
 	withStatusSubresource []client.Object
 	objectTracker         testing.ObjectTracker
 	interceptorFuncs      *interceptor.Funcs
+	typeConverters        []managedfields.TypeConverter


nit: Can this simply be typeConverter managedfields.TypeConverter? When multiTypeConverter is needed, it should implement the managedfields.TypeConverter interface and so can be initialized and used here?

This is on the builder and often times it will be needed to have more than one, as both in-tree and CRs are used

pkg/client/fake/client.go

jpbetz

This looks like a good approach. Left some minor feedback then LGTM.

sbueringer · 2025-06-19T05:49:07Z

pkg/client/fake/client.go

+// * managedfields.NewDeducedTypeConverter(),
+//
+// Be aware that the behavior of the `NewDeducedTypeConverter` might not match the behavior of the
+// Kubernetes APIServer, it is recommended to provide a type converter for your types.


it is recommended to provide a type converter for your types

Do we have an example somewhere on how to write one? (e.g. via unit tests, I don't see this func called there)

I think a unit test for this would be good and it also works as an example

They get generated alongside the applyconfigs, I have added a comment to that effect. As we don't have custom applyconfigs in this repo, I've added a test that uses the client-go type converter.

sbueringer · 2025-06-19T05:50:01Z

pkg/client/fake/client.go

@@ -228,6 +237,29 @@ func (f *ClientBuilder) WithInterceptorFuncs(interceptorFuncs interceptor.Funcs)
 	return f
 }

+// WithTypeConverters sets the type converters for the fake client. The list is ordered and the first
+// non-erroring converter is used. A type converter must be provided for all types the client is used


Under which exact circumstances must these be provided?

Only if SSA with custom types is used with the fake client? (based on the unit test it looks like Unstructured also works without setting TypeConverters)

It is always requiered if the FieldManagedObjectTracker is used, it seems to error out on all of its methods if the typeConverter doesn't handle a given type

I think I'm missing something.

So if I'm using the fake client today with my own CRDs, this will now break if I don't start setting type converters for them? This would break a lot of people

(FieldManagedObjectTracker is always used if objectTracker is not explicitly set, right?)

So if I'm using the fake client today with my own CRDs, this will now break if I don't start setting type converters for them? This would break a lot of people

No it will not break. SSA might not work correctly due to the inferencing type converter, but that is all.

sbueringer · 2025-06-19T05:52:39Z

pkg/client/fake/client.go

+			f.typeConverters = []managedfields.TypeConverter{
+				// Use corresponding scheme to ensure the converter error
+				// for types it can't handle.
+				clientgoapplyconfigurations.NewTypeConverter(scheme.Scheme),


The goal is to use a scheme that only contains clientgo types, right?

I wonder if we should use another scheme. This is a package global variable and people might have added all kinds of types to it (I've seen that in the past.. :))

But then we have to deal with keeping that scheme in sync with client-go, right? I.E. update it every time a new type is added there?

Could it be that we can do ~ the following https://github.com/kubernetes/client-go/blob/master/kubernetes/scheme/register.go#L159-L162 (with a local scheme that we create)

Oh nice, yeah that works, updated

pkg/client/fake/client.go

sbueringer · 2025-06-19T06:04:43Z

pkg/client/fake/client.go

@@ -523,6 +583,10 @@ func (t versionedTracker) updateObject(gvr schema.GroupVersionResource, obj runt
 }

 func (c *fakeClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
+	if err := c.addToSchemeIfUnknownAndUnstructured(obj); err != nil {


Looks like also if Partial, we should rename the func somehow

Maybe addUnstructuredAndPartialToSchemeIfUnknown

or

addToSchemeIfUnknownAndUnstructuredOrPartial

sbueringer · 2025-06-19T06:13:04Z

pkg/client/fake/client.go

+		if patch.Type() != types.ApplyPatchType {
+			return err
+		}
+		oldObj = &unstructured.Unstructured{}


Fine to ignore all errors in this case or could/should we only ignore certain errors? (like "not found")

Updated to specifically only ignore NotFound

sbueringer · 2025-06-19T06:14:06Z

pkg/client/fake/client.go

 	ns := action.GetNamespace()
 	gvr := action.GetResource()

 	obj, err := tracker.Get(gvr, ns, action.GetName())
 	if err != nil {
+		if action.GetPatchType() == types.ApplyPatchType {


Fine to ignore all errors in this case or could/should we only ignore certain errors?

Updated to specifically only ignore NotFound

sbueringer · 2025-06-19T06:18:31Z

pkg/client/fake/typeconverter.go

+		return res, nil
+	}
+
+	return nil, fmt.Errorf("failed to convert Object to Typed: %w", kerrors.NewAggregate(errs))


Suggested change

return nil, fmt.Errorf("failed to convert Object to Typed: %w", kerrors.NewAggregate(errs))

return nil, fmt.Errorf("failed to convert Object to TypedValue: %w", kerrors.NewAggregate(errs))

nit: Or drop the Value suffix in the error in l.59

sbueringer · 2025-06-19T06:26:19Z

pkg/client/fake/client_test.go

@@ -2554,6 +2538,51 @@ var _ = Describe("Fake client", func() {
 		wg.Wait()
 	})

+	It("supports server-side apply of a client-go resource", func() {


Should we have test coverage that tests that managedFields are returned? (and only if the flag is set)

sbueringer · 2025-06-19T06:27:31Z

pkg/client/fake/client_test.go

+
+		Expect(cl.Patch(context.Background(), obj, client.Apply)).To(Succeed())
+
+		result := obj.DeepCopy()


nit: Maybe just deepcopy in l.2569 so you don't need l.2574

sbueringer · 2025-06-19T06:32:37Z

Nice! Thx for working on this.

My comments are mostly around improving test coverage and godoc a bit

alvaroaleman · 2025-06-19T16:21:31Z

@tomasaschan @fsommar could you please find a different place for your comments that are not about feedback to the code changes in here to avoid polluting this? Thanks!

This change adds apply support into the fake client. This relies on the upstream support for this which is implemented in a new [FieldManagedObjectTracker][0]. In order to support many types, a custom `multiTypeConverter` is added. [0]: https://github.com/kubernetes/kubernetes/blob/4dc7a48ac6fb631a84e1974772bf7b8fd0bb9c59/staging/src/k8s.io/client-go/testing/fixture.go#L643

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 Oct 14, 2024

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 14, 2024

k8s-ci-robot requested review from inteon and vincepri October 14, 2024 03:14

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 14, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2024

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 8, 2025

alvaroaleman added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Feb 8, 2025

k8s-ci-robot removed the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Feb 9, 2025

alvaroaleman force-pushed the apply-fake branch from ac5705d to b8202b9 Compare February 23, 2025 17:56

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2025

alvaroaleman changed the title ~~✨ Fakeclient: Add apply support~~ ⚠️ : Fakeclient: Add apply support Feb 23, 2025

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 23, 2025

alvaroaleman force-pushed the apply-fake branch from b8202b9 to 71e305a Compare February 23, 2025 17:57

alvaroaleman changed the title ~~⚠️ : Fakeclient: Add apply support~~ ⚠️ Fakeclient: Add apply support Feb 23, 2025

jpbetz reviewed Feb 25, 2025

View reviewed changes

pkg/client/fake/client.go Show resolved Hide resolved

jpbetz reviewed Feb 25, 2025

View reviewed changes

pkg/client/fake/client.go Show resolved Hide resolved

jpbetz reviewed Feb 25, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2025

sbueringer mentioned this pull request Mar 30, 2025

Feature: Native SSA Support #3183

Open

9 tasks

alvaroaleman mentioned this pull request Jun 3, 2025

🌱 Bump k8s.io/* deps for fakeclient SSA fixes #3231

Merged

alvaroaleman force-pushed the apply-fake branch 2 times, most recently from 7d429f9 to e140397 Compare June 4, 2025 18:49

alvaroaleman marked this pull request as ready for review June 5, 2025 04:49

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 Jun 5, 2025

k8s-ci-robot requested review from FillZpp and troy0820 June 5, 2025 04:49

alvaroaleman force-pushed the apply-fake branch 2 times, most recently from e2e2905 to 46e8e54 Compare June 6, 2025 23:21

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 6, 2025

alvaroaleman force-pushed the apply-fake branch from 46e8e54 to 3e3e290 Compare June 6, 2025 23:33

alvaroaleman changed the title ~~⚠️ Fakeclient: Add apply support~~ ✨ Fakeclient: Add apply support Jun 6, 2025

alvaroaleman removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jun 6, 2025

alvaroaleman force-pushed the apply-fake branch 3 times, most recently from d6585af to e087e01 Compare June 7, 2025 20:25

This comment was marked as off-topic.

Sign in to view

sbueringer reviewed Jun 19, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2025

alvaroaleman force-pushed the apply-fake branch from e087e01 to 5f2b677 Compare June 19, 2025 16:03

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2025

alvaroaleman force-pushed the apply-fake branch 2 times, most recently from ddb6d57 to c33ac89 Compare June 20, 2025 00:32

alvaroaleman force-pushed the apply-fake branch from c33ac89 to 04c0089 Compare June 20, 2025 17:36

	return nil, fmt.Errorf("failed to convert Object to Typed: %w", kerrors.NewAggregate(errs))
	return nil, fmt.Errorf("failed to convert Object to TypedValue: %w", kerrors.NewAggregate(errs))


		Expect(cl.Patch(context.Background(), obj, client.Apply)).To(Succeed())

		result := obj.DeepCopy()

✨ Fakeclient: Add apply support #2981

Are you sure you want to change the base?

✨ Fakeclient: Add apply support #2981

Conversation

alvaroaleman commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 14, 2024

Uh oh!

k8s-ci-robot commented Oct 14, 2024

Uh oh!

k8s-triage-robot commented Feb 8, 2025

Uh oh!

alvaroaleman commented Feb 8, 2025

Uh oh!

k8s-ci-robot commented Feb 8, 2025

Uh oh!

k8s-triage-robot commented Feb 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpbetz Feb 25, 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!

Uh oh!

Uh oh!

jpbetz left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbueringer Jun 19, 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!

sbueringer Jun 19, 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!

sbueringer Jun 20, 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!

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!

alvaroaleman commented Oct 14, 2024 •

edited

Loading

jpbetz Feb 25, 2025 •

edited

Loading

sbueringer Jun 19, 2025 •

edited

Loading

sbueringer Jun 19, 2025 •

edited

Loading

sbueringer Jun 20, 2025 •

edited

Loading

sbueringer Jun 19, 2025 •

edited

Loading

sbueringer commented Jun 19, 2025 •

edited

Loading

alvaroaleman commented Jun 19, 2025 •

edited

Loading