From b3f4310da54674e43602f0322d5abf4b5c80f27a Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 13 Oct 2024 23:01:25 -0400 Subject: [PATCH 1/5] :warn: Fakeclient: Add apply support 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 --- go.mod | 3 +- pkg/client/fake/client.go | 75 ++++++++++++++++++++++++++++---- pkg/client/fake/client_test.go | 46 ++++++++++++++++++++ pkg/client/fake/typeconverter.go | 60 +++++++++++++++++++++++++ 4 files changed, 175 insertions(+), 9 deletions(-) create mode 100644 pkg/client/fake/typeconverter.go diff --git a/go.mod b/go.mod index c72c2dd7d0..0afd10ee4e 100644 --- a/go.mod +++ b/go.mod @@ -32,6 +32,8 @@ require ( sigs.k8s.io/yaml v1.4.0 ) +require sigs.k8s.io/structured-merge-diff/v4 v4.6.0 + require ( cel.dev/expr v0.19.1 // indirect github.com/antlr4-go/antlr/v4 v4.13.0 // indirect @@ -96,5 +98,4 @@ require ( sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.31.2 // indirect sigs.k8s.io/json v0.0.0-20241010143419-9aa6b5e7a4b3 // indirect sigs.k8s.io/randfill v1.0.0 // indirect - sigs.k8s.io/structured-merge-diff/v4 v4.6.0 // indirect ) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 99124e2f03..5c62e26264 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -57,13 +57,17 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/managedfields" utilrand "k8s.io/apimachinery/pkg/util/rand" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/strategicpatch" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apimachinery/pkg/watch" + clientgoapplyconfigurations "k8s.io/client-go/applyconfigurations" "k8s.io/client-go/kubernetes/scheme" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/testing" "k8s.io/utils/ptr" @@ -131,6 +135,7 @@ type ClientBuilder struct { withStatusSubresource []client.Object objectTracker testing.ObjectTracker interceptorFuncs *interceptor.Funcs + typeConverters []managedfields.TypeConverter // indexes maps each GroupVersionKind (GVK) to the indexes registered for that GVK. // The inner map maps from index name to IndexerFunc. @@ -172,6 +177,8 @@ func (f *ClientBuilder) WithRuntimeObjects(initRuntimeObjs ...runtime.Object) *C } // WithObjectTracker can be optionally used to initialize this fake client with testing.ObjectTracker. +// Setting this is incompatible with setting WithTypeConverters, as they are a setting on the +// tracker. func (f *ClientBuilder) WithObjectTracker(ot testing.ObjectTracker) *ClientBuilder { f.objectTracker = ot return f @@ -228,6 +235,18 @@ 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. +// This setting is incompatible with WithObjectTracker, as the type converters are a setting on the tracker. +// +// If unset, this defaults to: +// * clientgoapplyconfigurations.NewTypeConverter(clientgoscheme.Scheme), +// * managedfields.NewDeducedTypeConverter(), +func (f *ClientBuilder) WithTypeConverters(typeConverters ...managedfields.TypeConverter) *ClientBuilder { + f.typeConverters = append(f.typeConverters, typeConverters...) + return f +} + // Build builds and returns a new fake client. func (f *ClientBuilder) Build() client.WithWatch { if f.scheme == nil { @@ -248,11 +267,29 @@ func (f *ClientBuilder) Build() client.WithWatch { withStatusSubResource.Insert(gvk) } + if f.objectTracker != nil && len(f.typeConverters) > 0 { + panic(errors.New("WithObjectTracker and WithTypeConverters are incompatible")) + } + if f.objectTracker == nil { - tracker = versionedTracker{ObjectTracker: testing.NewObjectTracker(f.scheme, scheme.Codecs.UniversalDecoder()), scheme: f.scheme, withStatusSubresource: withStatusSubResource} - } else { - tracker = versionedTracker{ObjectTracker: f.objectTracker, scheme: f.scheme, withStatusSubresource: withStatusSubResource} + if len(f.typeConverters) == 0 { + f.typeConverters = []managedfields.TypeConverter{ + // Use corresponding scheme to ensure the converter error + // for types it can't handle. + clientgoapplyconfigurations.NewTypeConverter(clientgoscheme.Scheme), + managedfields.NewDeducedTypeConverter(), + } + } + f.objectTracker = testing.NewFieldManagedObjectTracker( + f.scheme, + serializer.NewCodecFactory(f.scheme).UniversalDecoder(), + multiTypeConverter{upstream: f.typeConverters}, + ) } + tracker = versionedTracker{ + ObjectTracker: f.objectTracker, + scheme: f.scheme, + withStatusSubresource: withStatusSubResource} for _, obj := range f.initObject { if err := tracker.Add(obj); err != nil { @@ -929,6 +966,12 @@ func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client if err != nil { return err } + + // otherwise the merge logic in the tracker complains + if patch.Type() == types.ApplyPatchType { + obj.SetManagedFields(nil) + } + data, err := patch.Data(obj) if err != nil { return err @@ -943,7 +986,10 @@ func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client defer c.trackerWriteLock.Unlock() oldObj, err := c.tracker.Get(gvr, accessor.GetNamespace(), accessor.GetName()) if err != nil { - return err + if patch.Type() != types.ApplyPatchType { + return err + } + oldObj = &unstructured.Unstructured{} } oldAccessor, err := meta.Accessor(oldObj) if err != nil { @@ -958,7 +1004,7 @@ func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client // This ensures that the patch may be rejected if a deletionTimestamp is modified, prior // to updating the object. action := testing.NewPatchAction(gvr, accessor.GetNamespace(), accessor.GetName(), patch.Type(), data) - o, err := dryPatch(action, c.tracker) + o, err := dryPatch(action, c.tracker, obj) if err != nil { return err } @@ -1017,12 +1063,15 @@ func deletionTimestampEqual(newObj metav1.Object, obj metav1.Object) bool { // This results in some code duplication, but was found to be a cleaner alternative than unmarshalling and introspecting the patch data // and easier than refactoring the k8s client-go method upstream. // Duplicate of upstream: https://github.com/kubernetes/client-go/blob/783d0d33626e59d55d52bfd7696b775851f92107/testing/fixture.go#L146-L194 -func dryPatch(action testing.PatchActionImpl, tracker testing.ObjectTracker) (runtime.Object, error) { +func dryPatch(action testing.PatchActionImpl, tracker testing.ObjectTracker, newObj runtime.Object) (runtime.Object, error) { ns := action.GetNamespace() gvr := action.GetResource() obj, err := tracker.Get(gvr, ns, action.GetName()) if err != nil { + if action.GetPatchType() == types.ApplyPatchType { + return &unstructured.Unstructured{}, nil + } return nil, err } @@ -1067,10 +1116,20 @@ func dryPatch(action testing.PatchActionImpl, tracker testing.ObjectTracker) (ru if err = json.Unmarshal(mergedByte, obj); err != nil { return nil, err } - case types.ApplyPatchType: - return nil, errors.New("apply patches are not supported in the fake client. Follow https://github.com/kubernetes/kubernetes/issues/115598 for the current status") case types.ApplyCBORPatchType: return nil, errors.New("apply CBOR patches are not supported in the fake client") + case types.ApplyPatchType: + // There doesn't seem to be a way to test this without actually applying it as apply is implemented in the tracker. + // We have to make sure no reader sees this and we can not handle errors resetting the obj to the original state. + defer func() { + if err := tracker.Add(obj); err != nil { + panic(err) + } + }() + if err := tracker.Apply(gvr, newObj, ns, action.PatchOptions); err != nil { + return nil, err + } + return tracker.Get(gvr, ns, action.GetName()) default: return nil, fmt.Errorf("%s PatchType is not supported", action.GetPatchType()) } diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index f6a493f54d..46efc2d887 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -2516,6 +2516,51 @@ var _ = Describe("Fake client", func() { Expect(cl.SubResource(subResourceScale).Update(context.Background(), obj, client.WithSubResourceBody(scale)).Error()).To(Equal(expectedErr)) }) + It("supports server-side apply of a client-go resource", func() { + cl := NewClientBuilder().Build() + obj := &unstructured.Unstructured{} + obj.SetAPIVersion("v1") + obj.SetKind("ConfigMap") + obj.SetName("foo") + unstructured.SetNestedField(obj.Object, map[string]any{"some": "data"}, "data") + + Expect(cl.Patch(context.Background(), obj, client.Apply)).To(Succeed()) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "foo"}} + + Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(cm), cm)).To(Succeed()) + Expect(cm.Data).To(Equal(map[string]string{"some": "data"})) + + unstructured.SetNestedField(obj.Object, map[string]any{"other": "data"}, "data") + Expect(cl.Patch(context.Background(), obj, client.Apply)).To(Succeed()) + + Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(cm), cm)).To(Succeed()) + Expect(cm.Data).To(Equal(map[string]string{"other": "data"})) + }) + + // It("supports server-side apply of a custom resource", func() { + // cl := NewClientBuilder().Build() + // obj := &unstructured.Unstructured{} + // obj.SetAPIVersion("custom/v1") + // obj.SetKind("FakeResource") + // obj.SetName("foo") + // unstructured.SetNestedField(obj.Object, map[string]any{"some": "data"}, "spec") + // + // Expect(cl.Patch(context.Background(), obj, client.Apply)).To(Succeed()) + // + // result := obj.DeepCopy() + // unstructured.SetNestedField(result.Object, nil, "spec") + // + // Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(result), result)).To(Succeed()) + // Expect(result.Object["spec"]).To(Equal(map[string]any{"some": "data"})) + // + // unstructured.SetNestedField(obj.Object, map[string]any{"other": "data"}, "spec") + // Expect(cl.Patch(context.Background(), obj, client.Apply)).To(Succeed()) + // + // Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(result), result)).To(Succeed()) + // Expect(result.Object["spec"]).To(Equal(map[string]any{"other": "data"})) + // }) + It("is threadsafe", func() { cl := NewClientBuilder().Build() @@ -2681,6 +2726,7 @@ var _ = Describe("Fake client", func() { expected.ResourceVersion = objActual.GetResourceVersion() expected.Spec.Replicas = ptr.To(int32(3)) } + objExpected.SetManagedFields(objActual.GetManagedFields()) Expect(cmp.Diff(objExpected, objActual)).To(BeEmpty()) scaleActual := &autoscalingv1.Scale{} diff --git a/pkg/client/fake/typeconverter.go b/pkg/client/fake/typeconverter.go new file mode 100644 index 0000000000..9e64ae5e4f --- /dev/null +++ b/pkg/client/fake/typeconverter.go @@ -0,0 +1,60 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package fake + +import ( + "fmt" + + "k8s.io/apimachinery/pkg/runtime" + utilerror "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/managedfields" + "sigs.k8s.io/structured-merge-diff/v4/typed" +) + +type multiTypeConverter struct { + upstream []managedfields.TypeConverter +} + +func (m multiTypeConverter) ObjectToTyped(r runtime.Object, o ...typed.ValidationOptions) (*typed.TypedValue, error) { + var errs []error + for _, u := range m.upstream { + res, err := u.ObjectToTyped(r, o...) + if err != nil { + errs = append(errs, err) + continue + } + + return res, nil + } + + return nil, fmt.Errorf("failed to convert Object to Typed: %w", utilerror.NewAggregate(errs)) +} + +func (m multiTypeConverter) TypedToObject(v *typed.TypedValue) (runtime.Object, error) { + var errs []error + for _, u := range m.upstream { + res, err := u.TypedToObject(v) + if err != nil { + errs = append(errs, err) + continue + } + + return res, nil + } + + return nil, fmt.Errorf("failed to convert TypedValue to Object: %w", utilerror.NewAggregate(errs)) +} From 2524cd15ed0ff947e6e2008caaffcb23315eed82 Mon Sep 17 00:00:00 2001 From: Tomas Aschan Date: Fri, 11 Apr 2025 11:06:23 +0200 Subject: [PATCH 2/5] Address review feedback from #2981 --- pkg/client/fake/client.go | 29 +++++++++++++++++++---------- pkg/client/fake/typeconverter.go | 3 +++ 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 5c62e26264..1492cf1e24 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -135,7 +135,7 @@ type ClientBuilder struct { withStatusSubresource []client.Object objectTracker testing.ObjectTracker interceptorFuncs *interceptor.Funcs - typeConverters []managedfields.TypeConverter + typeConverter managedfields.TypeConverter // indexes maps each GroupVersionKind (GVK) to the indexes registered for that GVK. // The inner map maps from index name to IndexerFunc. @@ -243,7 +243,14 @@ func (f *ClientBuilder) WithInterceptorFuncs(interceptorFuncs interceptor.Funcs) // * clientgoapplyconfigurations.NewTypeConverter(clientgoscheme.Scheme), // * managedfields.NewDeducedTypeConverter(), func (f *ClientBuilder) WithTypeConverters(typeConverters ...managedfields.TypeConverter) *ClientBuilder { - f.typeConverters = append(f.typeConverters, typeConverters...) + if f.typeConverter == nil { + f.typeConverter = &multiTypeConverter{upstream: typeConverters} + } else if multiTypeConverter, ok := f.typeConverter.(*multiTypeConverter); ok { + multiTypeConverter.upstream = append(multiTypeConverter.upstream, typeConverters...) + } else { + panic(fmt.Sprintf("unexpected type converter already specified: %T; this is incompatible with WithTypeConverters", f.typeConverter)) + } + return f } @@ -267,23 +274,25 @@ func (f *ClientBuilder) Build() client.WithWatch { withStatusSubResource.Insert(gvk) } - if f.objectTracker != nil && len(f.typeConverters) > 0 { + if f.objectTracker != nil && f.typeConverter != nil { panic(errors.New("WithObjectTracker and WithTypeConverters are incompatible")) } if f.objectTracker == nil { - if len(f.typeConverters) == 0 { - f.typeConverters = []managedfields.TypeConverter{ - // Use corresponding scheme to ensure the converter error - // for types it can't handle. - clientgoapplyconfigurations.NewTypeConverter(clientgoscheme.Scheme), - managedfields.NewDeducedTypeConverter(), + if f.typeConverter == nil { + f.typeConverter = &multiTypeConverter{ + upstream: []managedfields.TypeConverter{ + // Use corresponding scheme to ensure the converter error + // for types it can't handle. + clientgoapplyconfigurations.NewTypeConverter(clientgoscheme.Scheme), + managedfields.NewDeducedTypeConverter(), + }, } } f.objectTracker = testing.NewFieldManagedObjectTracker( f.scheme, serializer.NewCodecFactory(f.scheme).UniversalDecoder(), - multiTypeConverter{upstream: f.typeConverters}, + f.typeConverter, ) } tracker = versionedTracker{ diff --git a/pkg/client/fake/typeconverter.go b/pkg/client/fake/typeconverter.go index 9e64ae5e4f..4d8d6ffd4d 100644 --- a/pkg/client/fake/typeconverter.go +++ b/pkg/client/fake/typeconverter.go @@ -25,6 +25,9 @@ import ( "sigs.k8s.io/structured-merge-diff/v4/typed" ) +// multiTypeConverter is an implementation detail for the fake client used to +// support server-side apply. +// NOTE: this type should not be exported! type multiTypeConverter struct { upstream []managedfields.TypeConverter } From 1b57de08be85b5eda427b295c043af26d6671eea Mon Sep 17 00:00:00 2001 From: Tomas Aschan Date: Fri, 11 Apr 2025 11:06:48 +0200 Subject: [PATCH 3/5] Rename import to satisfy linter --- pkg/client/fake/typeconverter.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/client/fake/typeconverter.go b/pkg/client/fake/typeconverter.go index 4d8d6ffd4d..ebb55541ae 100644 --- a/pkg/client/fake/typeconverter.go +++ b/pkg/client/fake/typeconverter.go @@ -20,7 +20,7 @@ import ( "fmt" "k8s.io/apimachinery/pkg/runtime" - utilerror "k8s.io/apimachinery/pkg/util/errors" + kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/managedfields" "sigs.k8s.io/structured-merge-diff/v4/typed" ) @@ -44,7 +44,7 @@ func (m multiTypeConverter) ObjectToTyped(r runtime.Object, o ...typed.Validatio return res, nil } - return nil, fmt.Errorf("failed to convert Object to Typed: %w", utilerror.NewAggregate(errs)) + return nil, fmt.Errorf("failed to convert Object to Typed: %w", kerrors.NewAggregate(errs)) } func (m multiTypeConverter) TypedToObject(v *typed.TypedValue) (runtime.Object, error) { @@ -59,5 +59,5 @@ func (m multiTypeConverter) TypedToObject(v *typed.TypedValue) (runtime.Object, return res, nil } - return nil, fmt.Errorf("failed to convert TypedValue to Object: %w", utilerror.NewAggregate(errs)) + return nil, fmt.Errorf("failed to convert TypedValue to Object: %w", kerrors.NewAggregate(errs)) } From 159e1f3c147c515a655e666ffa8968f758dfb9f4 Mon Sep 17 00:00:00 2001 From: Tomas Aschan Date: Fri, 11 Apr 2025 11:13:41 +0200 Subject: [PATCH 4/5] fix: Remove redundant import --- pkg/client/fake/client.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 1492cf1e24..43825315b0 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -67,7 +67,6 @@ import ( "k8s.io/apimachinery/pkg/watch" clientgoapplyconfigurations "k8s.io/client-go/applyconfigurations" "k8s.io/client-go/kubernetes/scheme" - clientgoscheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/testing" "k8s.io/utils/ptr" @@ -284,7 +283,7 @@ func (f *ClientBuilder) Build() client.WithWatch { upstream: []managedfields.TypeConverter{ // Use corresponding scheme to ensure the converter error // for types it can't handle. - clientgoapplyconfigurations.NewTypeConverter(clientgoscheme.Scheme), + clientgoapplyconfigurations.NewTypeConverter(scheme.Scheme), managedfields.NewDeducedTypeConverter(), }, } From 22440ba7850fa6a7d5737628a69466080f6af9f5 Mon Sep 17 00:00:00 2001 From: Tomas Aschan Date: Fri, 11 Apr 2025 11:15:05 +0200 Subject: [PATCH 5/5] fix: Add missing error checks --- pkg/client/fake/client_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index 46efc2d887..0c531e81a4 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -2522,7 +2522,7 @@ var _ = Describe("Fake client", func() { obj.SetAPIVersion("v1") obj.SetKind("ConfigMap") obj.SetName("foo") - unstructured.SetNestedField(obj.Object, map[string]any{"some": "data"}, "data") + Expect(unstructured.SetNestedField(obj.Object, map[string]any{"some": "data"}, "data")).To(Succeed()) Expect(cl.Patch(context.Background(), obj, client.Apply)).To(Succeed()) @@ -2531,7 +2531,7 @@ var _ = Describe("Fake client", func() { Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(cm), cm)).To(Succeed()) Expect(cm.Data).To(Equal(map[string]string{"some": "data"})) - unstructured.SetNestedField(obj.Object, map[string]any{"other": "data"}, "data") + Expect(unstructured.SetNestedField(obj.Object, map[string]any{"other": "data"}, "data")).To(Succeed()) Expect(cl.Patch(context.Background(), obj, client.Apply)).To(Succeed()) Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(cm), cm)).To(Succeed())