From 067b29fb3725ce31d9040b831129b66df9727399 Mon Sep 17 00:00:00 2001 From: Scott Andrews Date: Wed, 4 Oct 2023 20:48:15 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Handle=20unstructured=20status?= =?UTF-8?q?=20update=20with=20fake=20client=20(#2495)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * 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 --------- Signed-off-by: Scott Andrews --- pkg/client/fake/client.go | 17 +++++++- pkg/client/fake/client_test.go | 78 ++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 1 deletion(-) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index b3819926c0..9deb6756c3 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 810cf5499c..0f0415f4f8 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -1700,6 +1700,84 @@ 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 := &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() + + // 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(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(), u)).To(Succeed()) + + 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 := &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() + + // 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(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(), u)).To(Succeed()) + + 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() { obj := &unstructured.Unstructured{} obj.SetAPIVersion("foo/v1")