From 733a8ad5003489875f6a21e0da19294fbbe0de3e Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Wed, 25 Mar 2026 10:02:08 +0100 Subject: [PATCH] :bug: The managed fields could not be explicitly modified in Update or merge Patch in the fake client. This is explicitly allowed and sometimes even necessary (e.g. upgrade from CSA to SSA) and so the fake client should support this. --- pkg/client/fake/client.go | 21 ++++-- pkg/client/fake/client_test.go | 130 +++++++++++++++++++++++++++++++-- 2 files changed, 138 insertions(+), 13 deletions(-) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 2a07bd40b2..5793564728 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -802,12 +802,21 @@ func (c *fakeClient) update(obj client.Object, isStatus bool, opts ...client.Upd c.trackerWriteLock.Lock() defer c.trackerWriteLock.Unlock() - // Retain managed fields - // We can ignore all errors here since update will fail if we encounter an error. - obj.SetManagedFields(nil) - current, _ := c.tracker.Get(gvr, accessor.GetNamespace(), accessor.GetName()) - if currentMetaObj, ok := current.(metav1.Object); ok { - obj.SetManagedFields(currentMetaObj.GetManagedFields()) + if isStatus { + // Disallow updates of managed fields directly on the (status) subresource. + // + // Note that managed fields tracking on the subresources other than status is + // broken anyway and this place is one of those that will need to change when + // proper support for subresource managed fields tracking is introduced. + // + // By re-reading the managed fields here, we at least retain the main resource's + // managed fields in case the subresource uses the main resource's body for + // the update (see fakeSubResourceClient.Update). + obj.SetManagedFields(nil) + current, _ := c.tracker.Get(gvr, accessor.GetNamespace(), accessor.GetName()) + if currentMetaObj, ok := current.(metav1.Object); ok { + obj.SetManagedFields(currentMetaObj.GetManagedFields()) + } } if err := c.tracker.update(gvr, obj, accessor.GetNamespace(), isStatus, false, *updateOptions.AsUpdateOptions()); err != nil { diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index db7d1c6279..bf441a288f 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -59,6 +59,7 @@ import ( const ( machineIDFromStatusUpdate = "machine-id-from-status-update" cidrFromStatusUpdate = "cidr-from-status-update" + testOwner = "test-owner" ) var _ = Describe("Fake client", func() { @@ -1823,7 +1824,7 @@ var _ = Describe("Fake client", func() { WithSpec(corev1applyconfigurations.NodeSpec().WithPodCIDR(initial.Spec.PodCIDR + "-updated")). WithStatus(corev1applyconfigurations.NodeStatus().WithPhase(corev1.NodeRunning)) - Expect(cl.Status().Apply(ctx, ac, client.FieldOwner("test-owner"))).To(Succeed()) + Expect(cl.Status().Apply(ctx, ac, client.FieldOwner(testOwner))).To(Succeed()) actual := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: initial.Name}} Expect(cl.Get(ctx, client.ObjectKeyFromObject(actual), actual)).To(Succeed()) @@ -1838,12 +1839,12 @@ var _ = Describe("Fake client", func() { cl := NewClientBuilder().WithStatusSubresource(&corev1.Node{}).Build() node := corev1applyconfigurations.Node("a-node"). WithSpec(corev1applyconfigurations.NodeSpec().WithPodCIDR("some-value")) - Expect(cl.Apply(ctx, node, client.FieldOwner("test-owner"))).To(Succeed()) + Expect(cl.Apply(ctx, node, client.FieldOwner(testOwner))).To(Succeed()) node = node. WithStatus(corev1applyconfigurations.NodeStatus().WithPhase(corev1.NodeRunning)) - Expect(cl.Status().Apply(ctx, node, client.FieldOwner("test-owner"))).To(Succeed()) + Expect(cl.Status().Apply(ctx, node, client.FieldOwner(testOwner))).To(Succeed()) }) It("should allow SSA apply on status without object has changed issues", func(ctx SpecContext) { @@ -1876,7 +1877,7 @@ var _ = Describe("Fake client", func() { resourceAC := client.ApplyConfigurationFromUnstructured(resourceForApply) - err := cl.Status().Apply(ctx, resourceAC, client.FieldOwner("test-owner"), client.ForceOwnership) + err := cl.Status().Apply(ctx, resourceAC, client.FieldOwner(testOwner), client.ForceOwnership) Expect(err).NotTo(HaveOccurred(), "SSA apply on status should succeed when resourceVersion is not set") // Verify the status was applied @@ -1925,7 +1926,7 @@ var _ = Describe("Fake client", func() { resourceAC := client.ApplyConfigurationFromUnstructured(resourceForApply) // This is expected to fail with the wrong rv value passed in in the applied config - err := cl.Status().Apply(ctx, resourceAC, client.FieldOwner("test-owner"), client.ForceOwnership) + err := cl.Status().Apply(ctx, resourceAC, client.FieldOwner(testOwner), client.ForceOwnership) Expect(err).To(HaveOccurred(), "SSA apply on status should not succeed when resourceVersion is wrongly set") Expect(apierrors.IsConflict(err)).To(BeTrue()) }) @@ -1952,6 +1953,62 @@ var _ = Describe("Fake client", func() { Expect(node.ManagedFields[1].Manager).To(Equal("status-owner")) }) + // this is not working properly and can't without a larger change to the codebase + PIt("should not be able to manually update the managed fields through a subresource create,update", func(ctx SpecContext) { + // test with update + dep := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dep", + Namespace: "default", + }, + } + cl := NewClientBuilder().WithObjects(dep).WithReturnManagedFields().Build() + + scale := &autoscalingv1.Scale{ + ObjectMeta: metav1.ObjectMeta{ + Name: "funnyScale", + Namespace: "default", + ManagedFields: []metav1.ManagedFieldsEntry{}, + }, + Spec: autoscalingv1.ScaleSpec{Replicas: 15}, + } + + dep.Spec.Replicas = ptr.To(int32(2)) + Expect(cl.Update(ctx, dep, client.FieldOwner("depManager"))).To(Succeed()) + + Expect(cl.SubResource("scale"). + Update(ctx, dep, client.WithSubResourceBody(scale), client.FieldOwner("scaleManager"))). + To(Succeed()) + Expect(dep.ManagedFields).To(HaveLen(2)) + Expect(dep.ManagedFields[0].Manager).To(Equal("depManager")) + Expect(dep.ManagedFields[1].Manager).To(Equal("scaleManager")) + Expect(dep.ManagedFields[1].Subresource).To(Equal("scale")) + + // test with create + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + Namespace: "default", + }, + } + Expect(cl.Create(ctx, pod, client.FieldOwner("podManager"))).To(Succeed()) + + binding := &corev1.Binding{ + ObjectMeta: metav1.ObjectMeta{ + ManagedFields: []metav1.ManagedFieldsEntry{}, + }, + Target: corev1.ObjectReference{Name: "the-node"}, + } + + Expect(cl.SubResource("binding"). + Create(ctx, pod, binding, client.FieldOwner("bindingManager"))). + To(Succeed()) + Expect(dep.ManagedFields).To(HaveLen(2)) + Expect(dep.ManagedFields[0].Manager).To(Equal("podManager")) + Expect(dep.ManagedFields[1].Manager).To(Equal("bindingManager")) + Expect(dep.ManagedFields[1].Subresource).To(Equal("binding")) + }) + It("should Unmarshal the schemaless object with int64 to preserve ints", func(ctx SpecContext) { gv := schema.GroupVersion{Group: "test", Version: "v1"} scheme := runtime.NewScheme() @@ -3121,7 +3178,7 @@ var _ = Describe("Fake client", func() { }) It("sets the fieldManager in create, patch and update", func(ctx SpecContext) { - owner := "test-owner" + owner := testOwner cl := client.WithFieldOwner( NewClientBuilder().WithReturnManagedFields().Build(), owner, @@ -3155,7 +3212,7 @@ var _ = Describe("Fake client", func() { }) It("sets the fieldManager when creating through update", func(ctx SpecContext) { - owner := "test-owner" + owner := testOwner cl := client.WithFieldOwner( NewClientBuilder().WithReturnManagedFields().Build(), owner, @@ -3168,6 +3225,65 @@ var _ = Describe("Fake client", func() { } }) + // GH-3484 + It("respects the ManagedFields during create, update, merge patch", func(ctx SpecContext) { + cl := NewClientBuilder().WithReturnManagedFields().Build() + fieldV1Map := map[string]any{ + "f:metadata": map[string]any{ + "f:name": map[string]any{}, + "f:labels": map[string]any{}, + "f:annotations": map[string]any{}, + "f:finalizers": map[string]any{}, + }, + } + fieldV1, err := json.Marshal(fieldV1Map) + Expect(err).NotTo(HaveOccurred()) + + obj := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{ + Name: "cm-1", + Namespace: "default", + ManagedFields: []metav1.ManagedFieldsEntry{{ + Manager: testOwner, + Operation: metav1.ManagedFieldsOperationUpdate, + FieldsType: "FieldsV1", + FieldsV1: &metav1.FieldsV1{Raw: fieldV1}, + APIVersion: "v1", + }}, + }} + Expect(cl.Create(ctx, obj)).To(Succeed()) + + persisted := &corev1.ConfigMap{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(obj), persisted)).To(Succeed()) + + Expect(persisted.ManagedFields).To(HaveLen(1)) + Expect(persisted.ManagedFields[0].Manager).To(Equal(testOwner)) + Expect(persisted.ManagedFields[0].Operation).To(Equal(metav1.ManagedFieldsOperationUpdate)) + + // This is similar to what e.g. csaupgrade package from client-go does. + obj = persisted + obj.ManagedFields[0].Manager = "updated-manager" + obj.ManagedFields[0].Operation = metav1.ManagedFieldsOperationApply + Expect(cl.Update(ctx, obj)).To(Succeed()) + + persisted = &corev1.ConfigMap{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(obj), persisted)).To(Succeed()) + Expect(persisted.ManagedFields).To(HaveLen(1)) + Expect(persisted.ManagedFields[0].Manager).To(Equal("updated-manager")) + Expect(persisted.ManagedFields[0].Operation).To(Equal(metav1.ManagedFieldsOperationApply)) + + // and the same thing using a merge patch + obj = persisted.DeepCopy() + obj.ManagedFields[0].Manager = "updated-manager-using-patch" + obj.ManagedFields[0].Operation = metav1.ManagedFieldsOperationUpdate + Expect(cl.Patch(ctx, obj, client.MergeFrom(persisted))).To(Succeed()) + + persisted = &corev1.ConfigMap{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(obj), persisted)).To(Succeed()) + Expect(persisted.ManagedFields).To(HaveLen(1)) + Expect(persisted.ManagedFields[0].Manager).To(Equal("updated-manager-using-patch")) + Expect(persisted.ManagedFields[0].Operation).To(Equal(metav1.ManagedFieldsOperationUpdate)) + }) + // GH-3267 It("Doesn't leave stale data when updating an object through SSA", func(ctx SpecContext) { obj := corev1applyconfigurations.