Skip to content

Commit

Permalink
🐛 Handle unstructured status update with fake client (#2495)
Browse files Browse the repository at this point in the history
* 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 <andrewssc@vmware.com>

* 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 <andrewssc@vmware.com>

---------

Signed-off-by: Scott Andrews <andrewssc@vmware.com>
  • Loading branch information
scothis authored Oct 5, 2023
1 parent c93e2ab commit 067b29f
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 1 deletion.
17 changes: 16 additions & 1 deletion pkg/client/fake/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
78 changes: 78 additions & 0 deletions pkg/client/fake/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down

0 comments on commit 067b29f

Please sign in to comment.