From f4582ea9b6a3dfa034be50d9bba47a13c1643c20 Mon Sep 17 00:00:00 2001 From: Troy Connor Date: Fri, 8 Sep 2023 16:58:07 -0400 Subject: [PATCH 1/4] update subresource client to not incude metadata Signed-off-by: Troy Connor --- pkg/client/fake/client.go | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 48d80bd4f9..18dd639d1c 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -1108,12 +1108,30 @@ func (sw *fakeSubResourceClient) Create(ctx context.Context, obj client.Object, func (sw *fakeSubResourceClient) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { updateOptions := client.SubResourceUpdateOptions{} updateOptions.ApplyOptions(opts) - - body := obj - if updateOptions.SubResourceBody != nil { - body = updateOptions.SubResourceBody + gvr, err := getGVRFromObject(obj, sw.client.scheme) + if err != nil { + return err + } + o, err := sw.client.tracker.Get(gvr, obj.GetNamespace(), obj.GetName()) + if err != nil { + return err + } + gvk, err := apiutil.GVKForObject(obj, sw.client.scheme) + if err != nil { + return err + } + body := o + if sw.client.tracker.withStatusSubresource.Has(gvk) { + err := copyStatusFrom(obj, body) + if err != nil { + return err + } + } + bodyObj := body.(client.Object) + if bodyObj.GetResourceVersion() != obj.GetResourceVersion() { + return apierrors.NewConflict(gvr.GroupResource(), obj.GetName(), fmt.Errorf("resource version conflict")) } - return sw.client.update(body, true, &updateOptions.UpdateOptions) + return sw.client.update(bodyObj, true, &updateOptions.UpdateOptions) } func (sw *fakeSubResourceClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { From 74eb8ea130684ede337587bbd93fd0f0489f3d35 Mon Sep 17 00:00:00 2001 From: Adam Berlin Date: Thu, 7 Sep 2023 22:56:00 -0400 Subject: [PATCH 2/4] Test showing labels and annotations stored during fake client.Status().Update() --- pkg/client/fake/client_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index 1350fa2bdb..16eb04efb7 100644 --- a/pkg/client/fake/client_test.go +++ b/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()) From b3c83a221de9601d5285845c3685c2f1c538f1dd Mon Sep 17 00:00:00 2001 From: Troy Connor Date: Fri, 8 Sep 2023 20:29:45 -0400 Subject: [PATCH 3/4] add test case to show it will not override other status fields when updating Signed-off-by: Troy Connor --- pkg/client/fake/client.go | 32 ++++++++------------------------ pkg/client/fake/client_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 24 deletions(-) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 18dd639d1c..0a529175ed 100644 --- a/pkg/client/fake/client.go +++ b/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,6 +950,7 @@ func dryPatch(action testing.PatchActionImpl, tracker testing.ObjectTracker) (ru return obj, nil } +//lint:ignore U1000 function is causing errors with status updates func copyNonStatusFrom(old, new runtime.Object) error { newClientObject, ok := new.(client.Object) if !ok { @@ -1108,30 +1110,12 @@ func (sw *fakeSubResourceClient) Create(ctx context.Context, obj client.Object, func (sw *fakeSubResourceClient) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { updateOptions := client.SubResourceUpdateOptions{} updateOptions.ApplyOptions(opts) - gvr, err := getGVRFromObject(obj, sw.client.scheme) - if err != nil { - return err - } - o, err := sw.client.tracker.Get(gvr, obj.GetNamespace(), obj.GetName()) - if err != nil { - return err - } - gvk, err := apiutil.GVKForObject(obj, sw.client.scheme) - if err != nil { - return err - } - body := o - if sw.client.tracker.withStatusSubresource.Has(gvk) { - err := copyStatusFrom(obj, body) - if err != nil { - return err - } - } - bodyObj := body.(client.Object) - if bodyObj.GetResourceVersion() != obj.GetResourceVersion() { - return apierrors.NewConflict(gvr.GroupResource(), obj.GetName(), fmt.Errorf("resource version conflict")) + + body := obj + if updateOptions.SubResourceBody != nil { + body = updateOptions.SubResourceBody } - return sw.client.update(bodyObj, true, &updateOptions.UpdateOptions) + return sw.client.update(body, true, &updateOptions.UpdateOptions) } func (sw *fakeSubResourceClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index 16eb04efb7..fed59c07dc 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -1479,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 not overwrite 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{ From 339df9ae10072c46dad99ed5010384b68bbad583 Mon Sep 17 00:00:00 2001 From: Troy Connor Date: Sat, 9 Sep 2023 15:26:33 -0400 Subject: [PATCH 4/4] delete copyNonStatusFrom function, change test case description Signed-off-by: Troy Connor --- pkg/client/fake/client.go | 40 ---------------------------------- pkg/client/fake/client_test.go | 2 +- 2 files changed, 1 insertion(+), 41 deletions(-) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 0a529175ed..eb219cff53 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -950,46 +950,6 @@ func dryPatch(action testing.PatchActionImpl, tracker testing.ObjectTracker) (ru return obj, nil } -//lint:ignore U1000 function is causing errors with status updates -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) diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index fed59c07dc..533037743d 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -1479,7 +1479,7 @@ var _ = Describe("Fake client", func() { objOriginal.Status.NodeInfo.MachineID = "machine-id-from-status-update" Expect(cmp.Diff(objOriginal, actual)).To(BeEmpty()) }) - It("should not overwrite status fields of typed objects that have a status subresource on status update", func() { + 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",