⚠️ 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.

Author: alvaroaleman

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(obj, false, opts...)
+}
+
+func (c *fakeClient) update(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(obj, patch, false, opts...)
+}
+
+func (c *fakeClient) patch(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(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(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 {

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() {