From ea6cd8cf7107ce7122ea5294f71341a2e3d87e1b Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Fri, 31 Mar 2023 23:10:13 -0400 Subject: [PATCH] :warning: Fakeclient: Drop status changes in Update/Patch if configured This changes makes the Update and Patch method drop the Status potion of the object if it is an in-tree object that has a status subresouce or was configured as having one. This replicas the behavior observed when using an actual Kubernetes API. This change might break tests due to the fact that it configures the in-tree resources that have a status subresoruce as such. If that happens however that likely means the test was passing incorrectly. --- pkg/client/fake/client.go | 155 ++++++++++++++++++++++++++++----- pkg/client/fake/client_test.go | 69 +++++++++++++++ 2 files changed, 201 insertions(+), 23 deletions(-) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 2199927428..6bdce5f03e 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -35,6 +35,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" utilrand "k8s.io/apimachinery/pkg/util/rand" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/kubernetes/scheme" @@ -48,13 +49,15 @@ import ( type versionedTracker struct { testing.ObjectTracker - scheme *runtime.Scheme + scheme *runtime.Scheme + withStatusSubresource sets.Set[schema.GroupVersionKind] } type fakeClient struct { - tracker versionedTracker - scheme *runtime.Scheme - restMapper meta.RESTMapper + tracker versionedTracker + scheme *runtime.Scheme + restMapper meta.RESTMapper + withStatusSubresource sets.Set[schema.GroupVersionKind] // indexes maps each GroupVersionKind (GVK) to the indexes registered for that GVK. // The inner map maps from index name to IndexerFunc. @@ -95,12 +98,13 @@ func NewClientBuilder() *ClientBuilder { // ClientBuilder builds a fake client. type ClientBuilder struct { - scheme *runtime.Scheme - restMapper meta.RESTMapper - initObject []client.Object - initLists []client.ObjectList - initRuntimeObjects []runtime.Object - objectTracker testing.ObjectTracker + scheme *runtime.Scheme + restMapper meta.RESTMapper + initObject []client.Object + initLists []client.ObjectList + initRuntimeObjects []runtime.Object + withStatusSubresource []client.Object + objectTracker testing.ObjectTracker // indexes maps each GroupVersionKind (GVK) to the indexes registered for that GVK. // The inner map maps from index name to IndexerFunc. @@ -185,6 +189,13 @@ func (f *ClientBuilder) WithIndex(obj runtime.Object, field string, extractValue return f } +// WithStatusSubresource configures the passed object with a status subresource, which means +// calls to Update and Patch will not alters its status. +func (f *ClientBuilder) WithStatusSubresource(o ...client.Object) *ClientBuilder { + f.withStatusSubresource = append(f.withStatusSubresource, o...) + return f +} + // Build builds and returns a new fake client. func (f *ClientBuilder) Build() client.WithWatch { if f.scheme == nil { @@ -196,10 +207,19 @@ func (f *ClientBuilder) Build() client.WithWatch { var tracker versionedTracker + withStatusSubResource := sets.New(inTreeResourcesWithStatus()...) + for _, o := range f.withStatusSubresource { + gvk, err := apiutil.GVKForObject(o, f.scheme) + if err != nil { + panic(fmt.Errorf("failed to get gvk for object %T: %w", withStatusSubResource, err)) + } + withStatusSubResource.Insert(gvk) + } + if f.objectTracker == nil { - tracker = versionedTracker{ObjectTracker: testing.NewObjectTracker(f.scheme, scheme.Codecs.UniversalDecoder()), scheme: f.scheme} + tracker = versionedTracker{ObjectTracker: testing.NewObjectTracker(f.scheme, scheme.Codecs.UniversalDecoder()), scheme: f.scheme, withStatusSubresource: withStatusSubResource} } else { - tracker = versionedTracker{ObjectTracker: f.objectTracker, scheme: f.scheme} + tracker = versionedTracker{ObjectTracker: f.objectTracker, scheme: f.scheme, withStatusSubresource: withStatusSubResource} } for _, obj := range f.initObject { @@ -217,11 +237,13 @@ func (f *ClientBuilder) Build() client.WithWatch { panic(fmt.Errorf("failed to add runtime object %v to fake client: %w", obj, err)) } } + return &fakeClient{ - tracker: tracker, - scheme: f.scheme, - restMapper: f.restMapper, - indexes: f.indexes, + tracker: tracker, + scheme: f.scheme, + restMapper: f.restMapper, + indexes: f.indexes, + withStatusSubresource: withStatusSubResource, } } @@ -318,6 +340,10 @@ func convertFromUnstructuredIfNecessary(s *runtime.Scheme, o runtime.Object) (ru } func (t versionedTracker) Update(gvr schema.GroupVersionResource, obj runtime.Object, ns string) error { + return t.update(gvr, obj, ns, false) +} + +func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Object, ns string, isStatus bool) error { accessor, err := meta.Accessor(obj) if err != nil { return fmt.Errorf("failed to get accessor for object: %w", err) @@ -337,6 +363,9 @@ func (t versionedTracker) Update(gvr schema.GroupVersionResource, obj runtime.Ob return err } } + if !isStatus && t.withStatusSubresource.Has(gvk) { + clearObjectStatus(obj) + } oldObject, err := t.ObjectTracker.Get(gvr, ns, accessor.GetName()) if err != nil { @@ -689,6 +718,10 @@ func (c *fakeClient) DeleteAllOf(ctx context.Context, obj client.Object, opts .. } func (c *fakeClient) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + return c.update(ctx, obj, false, opts...) +} + +func (c *fakeClient) update(ctx context.Context, obj client.Object, isStatus bool, opts ...client.UpdateOption) error { updateOptions := &client.UpdateOptions{} updateOptions.ApplyOptions(opts) @@ -706,10 +739,14 @@ func (c *fakeClient) Update(ctx context.Context, obj client.Object, opts ...clie if err != nil { return err } - return c.tracker.Update(gvr, obj, accessor.GetNamespace()) + return c.tracker.update(gvr, obj, accessor.GetNamespace(), isStatus) } func (c *fakeClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + return c.patch(ctx, obj, patch, false, opts...) +} + +func (c *fakeClient) patch(ctx context.Context, obj client.Object, patch client.Patch, isStatus bool, opts ...client.PatchOption) error { patchOptions := &client.PatchOptions{} patchOptions.ApplyOptions(opts) @@ -732,6 +769,18 @@ func (c *fakeClient) Patch(ctx context.Context, obj client.Object, patch client. return err } + gvk, err := apiutil.GVKForObject(obj, c.scheme) + if err != nil { + return err + } + + if !isStatus && c.withStatusSubresource.Has(gvk) { + data, err = clearStatus(data) + if err != nil { + return fmt.Errorf("failed to clear status: %w", err) + } + } + reaction := testing.ObjectReaction(c.tracker) handled, o, err := reaction(testing.NewPatchAction(gvr, accessor.GetNamespace(), accessor.GetName(), patch.Type(), data)) if err != nil { @@ -741,10 +790,6 @@ func (c *fakeClient) Patch(ctx context.Context, obj client.Object, patch client. panic("tracker could not handle patch method") } - gvk, err := apiutil.GVKForObject(obj, c.scheme) - if err != nil { - return err - } ta, err := meta.TypeAccessor(o) if err != nil { return err @@ -762,6 +807,34 @@ func (c *fakeClient) Patch(ctx context.Context, obj client.Object, patch client. return err } +func clearObjectStatus(o runtime.Object) { + if unstructured, isUnstructured := o.(*unstructured.Unstructured); isUnstructured { + delete(unstructured.Object, "status") + return + } + tp := reflect.TypeOf(o) + value := reflect.ValueOf(o) + if tp.Kind() == reflect.Pointer { + tp = tp.Elem() + value = value.Elem() + } + statusType, found := tp.FieldByName("Status") + if !found { + return + } + value.FieldByName("Status").Set(reflect.New(statusType.Type).Elem()) +} + +func clearStatus(in []byte) ([]byte, error) { + m := map[string]any{} + if err := json.Unmarshal(in, &m); err != nil { + return nil, err + } + delete(m, "status") + + return json.Marshal(m) +} + func (c *fakeClient) Status() client.SubResourceWriter { return c.SubResource("status") } @@ -818,7 +891,7 @@ func (sw *fakeSubResourceClient) Update(ctx context.Context, obj client.Object, if updateOptions.SubResourceBody != nil { body = updateOptions.SubResourceBody } - return sw.client.Update(ctx, body, &updateOptions.UpdateOptions) + return sw.client.update(ctx, body, true, &updateOptions.UpdateOptions) } func (sw *fakeSubResourceClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { @@ -833,7 +906,7 @@ func (sw *fakeSubResourceClient) Patch(ctx context.Context, obj client.Object, p body = patchOptions.SubResourceBody } - return sw.client.Patch(ctx, body, patch, &patchOptions.PatchOptions) + return sw.client.patch(ctx, body, patch, true, &patchOptions.PatchOptions) } func allowsUnconditionalUpdate(gvk schema.GroupVersionKind) bool { @@ -933,6 +1006,42 @@ func allowsCreateOnUpdate(gvk schema.GroupVersionKind) bool { return false } +func inTreeResourcesWithStatus() []schema.GroupVersionKind { + return []schema.GroupVersionKind{ + {Version: "v1", Kind: "Namespace"}, + {Version: "v1", Kind: "Node"}, + {Version: "v1", Kind: "PersistentVolumeClaim"}, + {Version: "v1", Kind: "PersistentVolume"}, + {Version: "v1", Kind: "Pod"}, + {Version: "v1", Kind: "ReplicationController"}, + {Version: "v1", Kind: "Service"}, + + {Group: "apps", Version: "v1", Kind: "Deployment"}, + {Group: "apps", Version: "v1", Kind: "DaemonSet"}, + {Group: "apps", Version: "v1", Kind: "ReplicaSet"}, + {Group: "apps", Version: "v1", Kind: "StatefulSet"}, + + {Group: "autoscaling", Version: "v1", Kind: "HorizontalPodAutoscaler"}, + + {Group: "batch", Version: "v1", Kind: "CronJob"}, + {Group: "batch", Version: "v1", Kind: "Job"}, + + {Group: "certificates.k8s.io", Version: "v1", Kind: "CertificateSigningRequest"}, + + {Group: "networking.k8s.io", Version: "v1", Kind: "Ingress"}, + {Group: "networking.k8s.io", Version: "v1", Kind: "NetworkPolicy"}, + + {Group: "policy", Version: "v1", Kind: "PodDisruptionBudget"}, + + {Group: "storage.k8s.io", Version: "v1", Kind: "VolumeAttachment"}, + + {Group: "apiextensions.k8s.io", Version: "v1", Kind: "CustomResourceDefinition"}, + + {Group: "flowcontrol.apiserver.k8s.io", Version: "v1beta2", Kind: "FlowSchema"}, + {Group: "flowcontrol.apiserver.k8s.io", Version: "v1beta2", Kind: "PriorityLevelConfiguration"}, + } +} + // zero zeros the value of a pointer. func zero(x interface{}) { if x == nil { diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index 570cd744ad..dea96e2e0d 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -972,6 +972,38 @@ var _ = Describe("Fake client", func() { Expect(err).To(BeNil()) Expect(len(newObj.Finalizers)).To(Equal(0)) }) + + It("should not change the status of objects that have a status subresource on update", func() { + obj := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node", + }, + } + Expect(cl.Create(context.Background(), obj)).To(BeNil()) + + obj.Status.Phase = corev1.NodeRunning + Expect(cl.Update(context.Background(), obj)).To(BeNil()) + + Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)).To(BeNil()) + + Expect(obj.Status).To(BeEquivalentTo(corev1.NodeStatus{})) + }) + It("should not change the status of objects that have a status subresource on patch", func() { + obj := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node", + }, + } + Expect(cl.Create(context.Background(), obj)).To(BeNil()) + original := obj.DeepCopy() + + obj.Status.Phase = corev1.NodeRunning + Expect(cl.Patch(context.Background(), obj, client.MergeFrom(original))).To(BeNil()) + + Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)).To(BeNil()) + + Expect(obj.Status).To(BeEquivalentTo(corev1.NodeStatus{})) + }) } Context("with default scheme.Scheme", func() { @@ -1220,6 +1252,43 @@ var _ = Describe("Fake client", func() { Expect(err).To(BeNil()) Expect(obj).To(Equal(dep3)) }) + + It("should not change the status of objects that are configured to have a status subresource on update", func() { + obj := &unstructured.Unstructured{} + obj.SetAPIVersion("foo/v1") + obj.SetKind("Foo") + obj.SetName("a-foo") + cl := NewClientBuilder().WithStatusSubresource(obj).Build() + + Expect(cl.Create(context.Background(), obj)).To(BeNil()) + err := unstructured.SetNestedField(obj.Object, map[string]interface{}{"count": int64(2)}, "status") + Expect(err).To(BeNil()) + + Expect(cl.Update(context.Background(), obj)).To(BeNil()) + + Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)).To(BeNil()) + + Expect(obj.Object["status"]).To(BeNil()) + }) + + It("should not change the status of objects that are configured to have a status subresource on patch", func() { + obj := &unstructured.Unstructured{} + obj.SetAPIVersion("foo/v1") + obj.SetKind("Foo") + obj.SetName("a-foo") + cl := NewClientBuilder().WithStatusSubresource(obj).Build() + + Expect(cl.Create(context.Background(), obj)).To(BeNil()) + original := obj.DeepCopy() + + err := unstructured.SetNestedField(obj.Object, map[string]interface{}{"count": int64(2)}, "status") + Expect(err).To(BeNil()) + Expect(cl.Patch(context.Background(), obj, client.MergeFrom(original))).To(BeNil()) + + Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)).To(BeNil()) + + Expect(obj.Object["status"]).To(BeNil()) + }) }) var _ = Describe("Fake client builder", func() {