From 6860cfb3d210ac028dc5d744e7824a99c8841f07 Mon Sep 17 00:00:00 2001 From: Scott Andrews Date: Thu, 14 Sep 2023 15:56:07 -0400 Subject: [PATCH 1/2] Handle unstructured status update with fake client In order to prevent unintentional mutations outside of the status during a status update, the non-status fields are copied back onto the passed object. This operation now gracefully handles both unstructured and typed objects. Previously, it would panic if an unstructured object was passed for a GVK known to the scheme, as internally the object within the tracker is converted to the typed equivalent. The two types cannot be directly assigned to each other and instead must be copied. Signed-off-by: Scott Andrews --- pkg/client/fake/client.go | 17 ++++++++++++- pkg/client/fake/client_test.go | 46 ++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index d70237e950..95a39893d3 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -404,7 +404,9 @@ func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Ob return fmt.Errorf("failed to copy non-status field for object with status subresouce: %w", err) } passedRV := accessor.GetResourceVersion() - reflect.ValueOf(obj).Elem().Set(reflect.ValueOf(oldObject.DeepCopyObject()).Elem()) + if err := copyFrom(oldObject, obj); err != nil { + return fmt.Errorf("failed to restore non-status fields: %w", err) + } accessor.SetResourceVersion(passedRV) } else { // copy status from original object if err := copyStatusFrom(oldObject, obj); err != nil { @@ -972,6 +974,19 @@ func copyStatusFrom(old, new runtime.Object) error { return nil } +// copyFrom copies from old into new +func copyFrom(old, new runtime.Object) error { + oldMapStringAny, err := toMapStringAny(old) + if err != nil { + return fmt.Errorf("failed to convert old to *unstructured.Unstructured: %w", err) + } + if err := fromMapStringAny(oldMapStringAny, new); err != nil { + return fmt.Errorf("failed to convert back from map[string]any: %w", err) + } + + return nil +} + func toMapStringAny(obj runtime.Object) (map[string]any, error) { if unstructured, isUnstructured := obj.(*unstructured.Unstructured); isUnstructured { return unstructured.Object, nil diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index bc857d7be8..9c1e99d768 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -1677,6 +1677,52 @@ var _ = Describe("Fake client", func() { Expect(obj.Object["spec"]).To(BeEquivalentTo("original")) }) + It("should not change the status of known unstructured objects that have a status subresource on update", func() { + obj := &unstructured.Unstructured{} + obj.SetAPIVersion("v1") + obj.SetKind("Pod") + obj.SetName("pod") + err := unstructured.SetNestedField(obj.Object, "Always", "spec", "restartPolicy") + Expect(err).NotTo(HaveOccurred()) + err = unstructured.SetNestedField(obj.Object, "Pending", "status", "phase") + Expect(err).NotTo(HaveOccurred()) + cl := NewClientBuilder().WithStatusSubresource(obj).WithObjects(obj).Build() + + err = unstructured.SetNestedField(obj.Object, "Never", "spec", "restartPolicy") + Expect(err).NotTo(HaveOccurred()) + err = unstructured.SetNestedField(obj.Object, "Running", "status", "phase") + Expect(err).NotTo(HaveOccurred()) + + Expect(cl.Update(context.Background(), obj)).To(Succeed()) + Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)).To(Succeed()) + + Expect(obj.Object["spec"].(map[string]any)["restartPolicy"]).To(Equal("Never")) + Expect(obj.Object["status"].(map[string]any)["phase"]).To(Equal("Pending")) + }) + + It("should not change non-status field of known unstructured objects that have a status subresource on status update", func() { + obj := &unstructured.Unstructured{} + obj.SetAPIVersion("v1") + obj.SetKind("Pod") + obj.SetName("pod") + err := unstructured.SetNestedField(obj.Object, "Always", "spec", "restartPolicy") + Expect(err).NotTo(HaveOccurred()) + err = unstructured.SetNestedField(obj.Object, "Pending", "status", "phase") + Expect(err).NotTo(HaveOccurred()) + cl := NewClientBuilder().WithStatusSubresource(obj).WithObjects(obj).Build() + + err = unstructured.SetNestedField(obj.Object, "Never", "spec", "restartPolicy") + Expect(err).NotTo(HaveOccurred()) + err = unstructured.SetNestedField(obj.Object, "Running", "status", "phase") + Expect(err).NotTo(HaveOccurred()) + + Expect(cl.Status().Update(context.Background(), obj)).To(Succeed()) + Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)).To(Succeed()) + + Expect(obj.Object["spec"].(map[string]any)["restartPolicy"]).To(Equal("Always")) + Expect(obj.Object["status"].(map[string]any)["phase"]).To(Equal("Running")) + }) + It("should not change the status of unstructured objects that are configured to have a status subresource on patch", func() { obj := &unstructured.Unstructured{} obj.SetAPIVersion("foo/v1") From 42608d32ec5a5fabe6545fef56877c74cc35448c Mon Sep 17 00:00:00 2001 From: Scott Andrews Date: Mon, 2 Oct 2023 16:41:22 -0400 Subject: [PATCH 2/2] Review feedback Using typed objects for the initial and actual object content assertion. Unstructured objects are only used for the update. Signed-off-by: Scott Andrews --- pkg/client/fake/client_test.go | 88 +++++++++++++++++++++++----------- 1 file changed, 60 insertions(+), 28 deletions(-) diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index 9c1e99d768..d583ff0ffb 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -1678,49 +1678,81 @@ var _ = Describe("Fake client", func() { }) It("should not change the status of known unstructured objects that have a status subresource on update", func() { - obj := &unstructured.Unstructured{} - obj.SetAPIVersion("v1") - obj.SetKind("Pod") - obj.SetName("pod") - err := unstructured.SetNestedField(obj.Object, "Always", "spec", "restartPolicy") - Expect(err).NotTo(HaveOccurred()) - err = unstructured.SetNestedField(obj.Object, "Pending", "status", "phase") - Expect(err).NotTo(HaveOccurred()) + obj := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + }, + Spec: corev1.PodSpec{ + RestartPolicy: corev1.RestartPolicyAlways, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + }, + } cl := NewClientBuilder().WithStatusSubresource(obj).WithObjects(obj).Build() - err = unstructured.SetNestedField(obj.Object, "Never", "spec", "restartPolicy") + // update using unstructured + u := &unstructured.Unstructured{} + u.SetAPIVersion("v1") + u.SetKind("Pod") + u.SetName(obj.Name) + err := cl.Get(context.Background(), client.ObjectKeyFromObject(u), u) Expect(err).NotTo(HaveOccurred()) - err = unstructured.SetNestedField(obj.Object, "Running", "status", "phase") + + err = unstructured.SetNestedField(u.Object, string(corev1.RestartPolicyNever), "spec", "restartPolicy") + Expect(err).NotTo(HaveOccurred()) + err = unstructured.SetNestedField(u.Object, string(corev1.PodRunning), "status", "phase") Expect(err).NotTo(HaveOccurred()) - Expect(cl.Update(context.Background(), obj)).To(Succeed()) - Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)).To(Succeed()) + Expect(cl.Update(context.Background(), u)).To(Succeed()) - Expect(obj.Object["spec"].(map[string]any)["restartPolicy"]).To(Equal("Never")) - Expect(obj.Object["status"].(map[string]any)["phase"]).To(Equal("Pending")) + actual := &corev1.Pod{} + Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(obj), actual)).To(Succeed()) + obj.APIVersion = u.GetAPIVersion() + obj.Kind = u.GetKind() + obj.ResourceVersion = actual.ResourceVersion + // only the spec mutation should persist + obj.Spec.RestartPolicy = corev1.RestartPolicyNever + Expect(cmp.Diff(obj, actual)).To(BeEmpty()) }) It("should not change non-status field of known unstructured objects that have a status subresource on status update", func() { - obj := &unstructured.Unstructured{} - obj.SetAPIVersion("v1") - obj.SetKind("Pod") - obj.SetName("pod") - err := unstructured.SetNestedField(obj.Object, "Always", "spec", "restartPolicy") - Expect(err).NotTo(HaveOccurred()) - err = unstructured.SetNestedField(obj.Object, "Pending", "status", "phase") - Expect(err).NotTo(HaveOccurred()) + obj := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + }, + Spec: corev1.PodSpec{ + RestartPolicy: corev1.RestartPolicyAlways, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + }, + } cl := NewClientBuilder().WithStatusSubresource(obj).WithObjects(obj).Build() - err = unstructured.SetNestedField(obj.Object, "Never", "spec", "restartPolicy") + // status update using unstructured + u := &unstructured.Unstructured{} + u.SetAPIVersion("v1") + u.SetKind("Pod") + u.SetName(obj.Name) + err := cl.Get(context.Background(), client.ObjectKeyFromObject(u), u) Expect(err).NotTo(HaveOccurred()) - err = unstructured.SetNestedField(obj.Object, "Running", "status", "phase") + + err = unstructured.SetNestedField(u.Object, string(corev1.RestartPolicyNever), "spec", "restartPolicy") + Expect(err).NotTo(HaveOccurred()) + err = unstructured.SetNestedField(u.Object, string(corev1.PodRunning), "status", "phase") Expect(err).NotTo(HaveOccurred()) - Expect(cl.Status().Update(context.Background(), obj)).To(Succeed()) - Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)).To(Succeed()) + Expect(cl.Status().Update(context.Background(), u)).To(Succeed()) - Expect(obj.Object["spec"].(map[string]any)["restartPolicy"]).To(Equal("Always")) - Expect(obj.Object["status"].(map[string]any)["phase"]).To(Equal("Running")) + actual := &corev1.Pod{} + Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(obj), actual)).To(Succeed()) + obj.APIVersion = "v1" + obj.Kind = "Pod" + obj.ResourceVersion = actual.ResourceVersion + // only the status mutation should persist + obj.Status.Phase = corev1.PodRunning + Expect(cmp.Diff(obj, actual)).To(BeEmpty()) }) It("should not change the status of unstructured objects that are configured to have a status subresource on patch", func() {