From 8f0fe71288f9499bc9cff4c0f649de9fb068c7b4 Mon Sep 17 00:00:00 2001 From: Adam Berlin Date: Sat, 9 Sep 2023 20:52:33 -0400 Subject: [PATCH 1/3] Draft: Test that an object is updatable after updating its status. --- pkg/client/fake/client_test.go | 78 ++++++++++++++++++++++++++++++++++ pkg/client/interfaces.go | 1 + 2 files changed, 79 insertions(+) diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index 81a2d4ac43..f2ebf6ce60 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -1476,6 +1476,84 @@ var _ = Describe("Fake client", func() { objOriginal.Status.NodeInfo.MachineID = "machine-id-from-status-update" Expect(cmp.Diff(objOriginal, actual)).To(BeEmpty()) }) + + It("should be able to update an object after updating an object's status", 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() + expectedObj := obj.DeepCopy() + + obj.Status.NodeInfo.MachineID = "machine-id-from-status-update" + Expect(cl.Status().Update(context.Background(), obj)).NotTo(HaveOccurred()) + + obj.Annotations = map[string]string{ + "some-annotation-key": "some", + } + expectedObj.Annotations = map[string]string{ + "some-annotation-key": "some", + } + Expect(cl.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()) + + expectedObj.APIVersion = actual.APIVersion + expectedObj.Kind = actual.Kind + expectedObj.ResourceVersion = actual.ResourceVersion + expectedObj.Status.NodeInfo.MachineID = "machine-id-from-status-update" + Expect(cmp.Diff(expectedObj, actual)).To(BeEmpty()) + }) + + It("should be able to update an object's status after updating an object", 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() + expectedObj := obj.DeepCopy() + + obj.Annotations = map[string]string{ + "some-annotation-key": "some", + } + expectedObj.Annotations = map[string]string{ + "some-annotation-key": "some", + } + Expect(cl.Update(context.Background(), obj)).NotTo(HaveOccurred()) + + obj.Spec.PodCIDR = "cidr-from-status-update" + obj.Status.NodeInfo.MachineID = "machine-id-from-status-update" + 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()) + + expectedObj.APIVersion = actual.APIVersion + expectedObj.Kind = actual.Kind + expectedObj.ResourceVersion = actual.ResourceVersion + expectedObj.Status.NodeInfo.MachineID = "machine-id-from-status-update" + Expect(cmp.Diff(expectedObj, 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{ diff --git a/pkg/client/interfaces.go b/pkg/client/interfaces.go index 0ddda3163d..3cd745e4c0 100644 --- a/pkg/client/interfaces.go +++ b/pkg/client/interfaces.go @@ -142,6 +142,7 @@ type SubResourceWriter interface { // Create saves the subResource object in the Kubernetes cluster. obj must be a // struct pointer so that obj can be updated with the content returned by the Server. Create(ctx context.Context, obj Object, subResource Object, opts ...SubResourceCreateOption) error + // Update updates the fields corresponding to the status subresource for the // given obj. obj must be a struct pointer so that obj can be updated // with the content returned by the Server. From c5272395e4403c7b452a6b4dcf70e1eb8f9de373 Mon Sep 17 00:00:00 2001 From: Adam Berlin Date: Sat, 9 Sep 2023 21:49:31 -0400 Subject: [PATCH 2/3] Draft: ensure we udpate the new obj's accessor's ResourceVersion after doing the deep copy --- pkg/client/fake/client.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 91886d278f..d4a47c0974 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -403,6 +403,7 @@ func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Ob 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 { @@ -436,6 +437,14 @@ func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Ob intResourceVersion++ accessor.SetResourceVersion(strconv.FormatUint(intResourceVersion, 10)) + // obtain the current obj accessor's pointer, + // so we can make an update on the current obj + currentAccessor, err := meta.Accessor(obj) + if err != nil { + return fmt.Errorf("failed to get accessor for object: %w", err) + } + currentAccessor.SetResourceVersion(strconv.FormatUint(intResourceVersion, 10)) + if !deleting && !deletionTimestampEqual(accessor, oldAccessor) { return fmt.Errorf("error: Unable to edit %s: metadata.deletionTimestamp field is immutable", accessor.GetName()) } From aa2bc6afddba4939c837a7a83e9825cb050c1c2f Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 10 Sep 2023 15:52:15 -0400 Subject: [PATCH 3/3] Rework code to pass object back on status update --- pkg/client/fake/client.go | 13 +++---------- pkg/client/fake/client_test.go | 23 ++++++++++++++--------- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index d4a47c0974..d70237e950 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -403,8 +403,9 @@ func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Ob 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) + passedRV := accessor.GetResourceVersion() + reflect.ValueOf(obj).Elem().Set(reflect.ValueOf(oldObject.DeepCopyObject()).Elem()) + accessor.SetResourceVersion(passedRV) } 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) @@ -437,14 +438,6 @@ func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Ob intResourceVersion++ accessor.SetResourceVersion(strconv.FormatUint(intResourceVersion, 10)) - // obtain the current obj accessor's pointer, - // so we can make an update on the current obj - currentAccessor, err := meta.Accessor(obj) - if err != nil { - return fmt.Errorf("failed to get accessor for object: %w", err) - } - currentAccessor.SetResourceVersion(strconv.FormatUint(intResourceVersion, 10)) - if !deleting && !deletionTimestampEqual(accessor, oldAccessor) { return fmt.Errorf("error: Unable to edit %s: metadata.deletionTimestamp field is immutable", accessor.GetName()) } diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index f2ebf6ce60..bc857d7be8 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -45,6 +45,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/interceptor" ) +const ( + machineIDFromStatusUpdate = "machine-id-from-status-update" + cidrFromStatusUpdate = "cidr-from-status-update" +) + var _ = Describe("Fake client", func() { var dep *appsv1.Deployment var dep2 *appsv1.Deployment @@ -1456,7 +1461,7 @@ var _ = Describe("Fake client", func() { cl := NewClientBuilder().WithStatusSubresource(obj).WithObjects(obj).Build() objOriginal := obj.DeepCopy() - obj.Spec.PodCIDR = "cidr-from-status-update" + obj.Spec.PodCIDR = cidrFromStatusUpdate obj.Annotations = map[string]string{ "some-annotation-key": "some-annotation-value", } @@ -1464,7 +1469,7 @@ var _ = Describe("Fake client", func() { "some-label-key": "some-label-value", } - obj.Status.NodeInfo.MachineID = "machine-id-from-status-update" + obj.Status.NodeInfo.MachineID = machineIDFromStatusUpdate Expect(cl.Status().Update(context.Background(), obj)).NotTo(HaveOccurred()) actual := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: obj.Name}} @@ -1473,7 +1478,7 @@ var _ = Describe("Fake client", func() { objOriginal.APIVersion = actual.APIVersion objOriginal.Kind = actual.Kind objOriginal.ResourceVersion = actual.ResourceVersion - objOriginal.Status.NodeInfo.MachineID = "machine-id-from-status-update" + objOriginal.Status.NodeInfo.MachineID = machineIDFromStatusUpdate Expect(cmp.Diff(objOriginal, actual)).To(BeEmpty()) }) @@ -1494,7 +1499,7 @@ var _ = Describe("Fake client", func() { cl := NewClientBuilder().WithStatusSubresource(obj).WithObjects(obj).Build() expectedObj := obj.DeepCopy() - obj.Status.NodeInfo.MachineID = "machine-id-from-status-update" + obj.Status.NodeInfo.MachineID = machineIDFromStatusUpdate Expect(cl.Status().Update(context.Background(), obj)).NotTo(HaveOccurred()) obj.Annotations = map[string]string{ @@ -1511,7 +1516,7 @@ var _ = Describe("Fake client", func() { expectedObj.APIVersion = actual.APIVersion expectedObj.Kind = actual.Kind expectedObj.ResourceVersion = actual.ResourceVersion - expectedObj.Status.NodeInfo.MachineID = "machine-id-from-status-update" + expectedObj.Status.NodeInfo.MachineID = machineIDFromStatusUpdate Expect(cmp.Diff(expectedObj, actual)).To(BeEmpty()) }) @@ -1540,8 +1545,8 @@ var _ = Describe("Fake client", func() { } Expect(cl.Update(context.Background(), obj)).NotTo(HaveOccurred()) - obj.Spec.PodCIDR = "cidr-from-status-update" - obj.Status.NodeInfo.MachineID = "machine-id-from-status-update" + obj.Spec.PodCIDR = cidrFromStatusUpdate + obj.Status.NodeInfo.MachineID = machineIDFromStatusUpdate Expect(cl.Status().Update(context.Background(), obj)).NotTo(HaveOccurred()) actual := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: obj.Name}} @@ -1550,7 +1555,7 @@ var _ = Describe("Fake client", func() { expectedObj.APIVersion = actual.APIVersion expectedObj.Kind = actual.Kind expectedObj.ResourceVersion = actual.ResourceVersion - expectedObj.Status.NodeInfo.MachineID = "machine-id-from-status-update" + expectedObj.Status.NodeInfo.MachineID = machineIDFromStatusUpdate Expect(cmp.Diff(expectedObj, actual)).To(BeEmpty()) }) @@ -1614,7 +1619,7 @@ var _ = Describe("Fake client", func() { cl := NewClientBuilder().WithStatusSubresource(obj).WithObjects(obj).Build() objOriginal := obj.DeepCopy() - obj.Spec.PodCIDR = "cidr-from-status-update" + obj.Spec.PodCIDR = cidrFromStatusUpdate obj.Status.NodeInfo.MachineID = "machine-id" Expect(cl.Status().Patch(context.Background(), obj, client.MergeFrom(objOriginal))).NotTo(HaveOccurred())