🐛 Do not update anything but status when using subresource client (#2479)

* update subresource client to not incude metadata

Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>

* Test showing labels and annotations stored during fake client.Status().Update()

* add test case to show it will not override other status fields when updating

Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>

* delete copyNonStatusFrom function, change test case description

Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>

---------

Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>
Co-authored-by: Adam Berlin <aberlin@vmware.com>

Author: troy0820

pkg/client/fake/client.go

@@ -400,9 +400,10 @@ func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Ob
 
 	if t.withStatusSubresource.Has(gvk) {
 		if isStatus { // copy everything but status and metadata.ResourceVersion from original object
-			if err := copyNonStatusFrom(oldObject, obj); err != nil {
+			if err := copyStatusFrom(obj, oldObject); err != nil {
 				return fmt.Errorf("failed to copy non-status field for object with status subresouce: %w", err)
 			}
+			obj = oldObject.DeepCopyObject().(client.Object)
 		} else { // copy status from original object
 			if err := copyStatusFrom(oldObject, obj); err != nil {
 				return fmt.Errorf("failed to copy the status for object with status subresource: %w", err)
@@ -949,45 +950,6 @@ func dryPatch(action testing.PatchActionImpl, tracker testing.ObjectTracker) (ru
 	return obj, nil
 }
 
-func copyNonStatusFrom(old, new runtime.Object) error {
-	newClientObject, ok := new.(client.Object)
-	if !ok {
-		return fmt.Errorf("%T is not a client.Object", new)
-	}
-	// The only thing other than status we have to retain
-	rv := newClientObject.GetResourceVersion()
-
-	oldMapStringAny, err := toMapStringAny(old)
-	if err != nil {
-		return fmt.Errorf("failed to convert old to *unstructured.Unstructured: %w", err)
-	}
-	newMapStringAny, err := toMapStringAny(new)
-	if err != nil {
-		return fmt.Errorf("failed to convert new to *unststructured.Unstructured: %w", err)
-	}
-
-	// delete everything other than status in case it has fields that were not present in
-	// the old object
-	for k := range newMapStringAny {
-		if k != "status" {
-			delete(newMapStringAny, k)
-		}
-	}
-	// copy everything other than status from the old object
-	for k := range oldMapStringAny {
-		if k != "status" {
-			newMapStringAny[k] = oldMapStringAny[k]
-		}
-	}
-
-	if err := fromMapStringAny(newMapStringAny, new); err != nil {
-		return fmt.Errorf("failed to convert back from map[string]any: %w", err)
-	}
-	newClientObject.SetResourceVersion(rv)
-
-	return nil
-}
-
 // copyStatusFrom copies the status from old into new
 func copyStatusFrom(old, new runtime.Object) error {
 	oldMapStringAny, err := toMapStringAny(old)

pkg/client/fake/client_test.go

@@ -1460,6 +1460,13 @@ var _ = Describe("Fake client", func() {
 		objOriginal := obj.DeepCopy()
 
 		obj.Spec.PodCIDR = "cidr-from-status-update"
+		obj.Annotations = map[string]string{
+			"some-annotation-key": "some-annotation-value",
+		}
+		obj.Labels = map[string]string{
+			"some-label-key": "some-label-value",
+		}
+
 		obj.Status.NodeInfo.MachineID = "machine-id-from-status-update"
 		Expect(cl.Status().Update(context.Background(), obj)).NotTo(HaveOccurred())
 
@@ -1472,6 +1479,36 @@ var _ = Describe("Fake client", func() {
 		objOriginal.Status.NodeInfo.MachineID = "machine-id-from-status-update"
 		Expect(cmp.Diff(objOriginal, actual)).To(BeEmpty())
 	})
+	It("Should only override status fields of typed objects that have a status subresource on status update", func() {
+		obj := &corev1.Node{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "node",
+			},
+			Spec: corev1.NodeSpec{
+				PodCIDR: "old-cidr",
+			},
+			Status: corev1.NodeStatus{
+				NodeInfo: corev1.NodeSystemInfo{
+					MachineID: "machine-id",
+				},
+			},
+		}
+		cl := NewClientBuilder().WithStatusSubresource(obj).WithObjects(obj).Build()
+		objOriginal := obj.DeepCopy()
+
+		obj.Status.Phase = corev1.NodeRunning
+		Expect(cl.Status().Update(context.Background(), obj)).NotTo(HaveOccurred())
+
+		actual := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: obj.Name}}
+		Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(actual), actual)).NotTo(HaveOccurred())
+
+		objOriginal.APIVersion = actual.APIVersion
+		objOriginal.Kind = actual.Kind
+		objOriginal.ResourceVersion = actual.ResourceVersion
+		Expect(cmp.Diff(objOriginal, actual)).ToNot(BeEmpty())
+		Expect(objOriginal.Status.NodeInfo.MachineID).To(Equal(actual.Status.NodeInfo.MachineID))
+		Expect(objOriginal.Status.Phase).ToNot(Equal(actual.Status.Phase))
+	})
 
 	It("should not change the status of typed objects that have a status subresource on patch", func() {
 		obj := &corev1.Node{