diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 380c728b0d..0ce5dc9f1e 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -41,6 +41,7 @@ import ( https://github.com/kubernetes/kubernetes/pull/120326 (v5.6.0+incompatible missing a critical fix) */ + jsonpatch "gopkg.in/evanphx/json-patch.v4" appsv1 "k8s.io/api/apps/v1" authenticationv1 "k8s.io/api/authentication/v1" @@ -717,6 +718,10 @@ func (c *fakeClient) List(ctx context.Context, obj client.ObjectList, opts ...cl if err := ensureTypeMeta(o, gvk); err != nil { return err } + + if !c.returnManagedFields { + o.(metav1.Object).SetManagedFields(nil) + } } if listOpts.LabelSelector == nil && listOpts.FieldSelector == nil { @@ -870,7 +875,7 @@ func (c *fakeClient) Create(ctx context.Context, obj client.Object, opts ...clie c.trackerWriteLock.Lock() defer c.trackerWriteLock.Unlock() - if err := c.tracker.Create(gvr, obj, accessor.GetNamespace()); err != nil { + if err := c.tracker.Create(gvr, obj, accessor.GetNamespace(), *createOptions.AsCreateOptions()); err != nil { // The managed fields tracker sets gvk even on errors _ = ensureTypeMeta(obj, gvk) return err @@ -1113,11 +1118,7 @@ func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client return err } - data, err := patch.Data(obj) - if err != nil { - return err - } - + var isApplyCreate bool c.trackerWriteLock.Lock() defer c.trackerWriteLock.Unlock() oldObj, err := c.tracker.Get(gvr, accessor.GetNamespace(), accessor.GetName()) @@ -1126,32 +1127,59 @@ func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client return err } oldObj = &unstructured.Unstructured{} + isApplyCreate = true } oldAccessor, err := meta.Accessor(oldObj) if err != nil { return err } - // Apply patch without updating object. - // To remain in accordance with the behavior of k8s api behavior, - // a patch must not allow for changes to the deletionTimestamp of an object. - // The reaction() function applies the patch to the object and calls Update(), - // whereas dryPatch() replicates this behavior but skips the call to Update(). - // This ensures that the patch may be rejected if a deletionTimestamp is modified, prior - // to updating the object. - action := testing.NewPatchAction(gvr, accessor.GetNamespace(), accessor.GetName(), patch.Type(), data) - o, err := dryPatch(action, c.tracker, obj) - if err != nil { - return err + // SSA deletionTimestamp updates are silently ignored + if patch.Type() == types.ApplyPatchType && !isApplyCreate { + obj.SetDeletionTimestamp(oldAccessor.GetDeletionTimestamp()) } - newObj, err := meta.Accessor(o) + + data, err := patch.Data(obj) if err != nil { return err } - // Validate that deletionTimestamp has not been changed - if !deletionTimestampEqual(newObj, oldAccessor) { - return fmt.Errorf("rejected patch, metadata.deletionTimestamp immutable") + action := testing.NewPatchActionWithOptions( + gvr, + accessor.GetNamespace(), + accessor.GetName(), + patch.Type(), + data, + *patchOptions.AsPatchOptions(), + ) + + // Apply is implemented in the tracker and calling it has side-effects + // such as bumping RV and updating managedFields timestamps, hence we + // can not dry-run it. Luckily, the only validation we use it for + // doesn't apply to SSA - Creating objects with non-nil deletionTimestamp + // through SSA is possible and updating the deletionTimestamp is valid, + // but has no effect. + if patch.Type() != types.ApplyPatchType { + // Apply patch without updating object. + // To remain in accordance with the behavior of k8s api behavior, + // a patch must not allow for changes to the deletionTimestamp of an object. + // The reaction() function applies the patch to the object and calls Update(), + // whereas dryPatch() replicates this behavior but skips the call to Update(). + // This ensures that the patch may be rejected if a deletionTimestamp is modified, prior + // to updating the object. + o, err := dryPatch(action, c.tracker) + if err != nil { + return err + } + newObj, err := meta.Accessor(o) + if err != nil { + return err + } + + // Validate that deletionTimestamp has not been changed + if !deletionTimestampEqual(newObj, oldAccessor) { + return fmt.Errorf("rejected patch, metadata.deletionTimestamp immutable") + } } reaction := testing.ObjectReaction(c.tracker) @@ -1206,7 +1234,7 @@ func deletionTimestampEqual(newObj metav1.Object, obj metav1.Object) bool { // This results in some code duplication, but was found to be a cleaner alternative than unmarshalling and introspecting the patch data // and easier than refactoring the k8s client-go method upstream. // Duplicate of upstream: https://github.com/kubernetes/client-go/blob/783d0d33626e59d55d52bfd7696b775851f92107/testing/fixture.go#L146-L194 -func dryPatch(action testing.PatchActionImpl, tracker testing.ObjectTracker, newObj runtime.Object) (runtime.Object, error) { +func dryPatch(action testing.PatchActionImpl, tracker testing.ObjectTracker) (runtime.Object, error) { ns := action.GetNamespace() gvr := action.GetResource() @@ -1262,20 +1290,7 @@ func dryPatch(action testing.PatchActionImpl, tracker testing.ObjectTracker, new case types.ApplyCBORPatchType: return nil, errors.New("apply CBOR patches are not supported in the fake client") case types.ApplyPatchType: - // There doesn't seem to be a way to test this without actually applying it as apply is implemented in the tracker. - // We have to make sure no reader sees this and we can not handle errors resetting the obj to the original state. - defer func() { - if unstructured, isUnstructured := obj.(*unstructured.Unstructured); isUnstructured && unstructured.GetKind() == "" { - unstructured.SetGroupVersionKind(newObj.GetObjectKind().GroupVersionKind()) - } - if err := tracker.Add(obj); err != nil { - panic(err) - } - }() - if err := tracker.Apply(gvr, newObj, ns, action.PatchOptions); err != nil { - return nil, err - } - return tracker.Get(gvr, ns, action.GetName()) + return nil, errors.New("bug in controller-runtime: should not end up in dryPatch for SSA") default: return nil, fmt.Errorf("%s PatchType is not supported", action.GetPatchType()) } diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index 0d1589c2ec..bad2d78b8a 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -2668,6 +2668,27 @@ var _ = Describe("Fake client", func() { _, exists, err := unstructured.NestedFieldNoCopy(u.Object, "metadata", "managedFields") Expect(err).NotTo(HaveOccurred()) Expect(exists).To(BeTrue()) + + var list corev1.ConfigMapList + Expect(cl.List(ctx, &list)).NotTo(HaveOccurred()) + for _, item := range list.Items { + Expect(item.ManagedFields).NotTo(BeNil()) + } + }) + + It("clears managedFields from objects in a list", func(ctx SpecContext) { + cl := NewClientBuilder().Build() + obj := corev1applyconfigurations. + ConfigMap("foo", "default"). + WithData(map[string]string{"some": "data"}) + + Expect(cl.Apply(ctx, obj, &client.ApplyOptions{FieldManager: "test-manager"})).To(Succeed()) + + var list corev1.ConfigMapList + Expect(cl.List(ctx, &list)).NotTo(HaveOccurred()) + for _, item := range list.Items { + Expect(item.ManagedFields).To(BeNil()) + } }) It("supports server-side apply of a client-go resource via Apply method", func(ctx SpecContext) { @@ -2736,6 +2757,88 @@ var _ = Describe("Fake client", func() { Expect(result.Object["spec"]).To(Equal(map[string]any{"other": "data"})) }) + It("sets managed fields through all methods", func(ctx SpecContext) { + owner := "test-owner" + cl := client.WithFieldOwner( + NewClientBuilder().WithReturnManagedFields().Build(), + owner, + ) + + obj := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "foo"}, + Data: map[string]string{"method": "create"}, + } + Expect(cl.Create(ctx, obj)).NotTo(HaveOccurred()) + + Expect(obj.ManagedFields).NotTo(BeEmpty()) + for _, f := range obj.ManagedFields { + Expect(f.Manager).To(BeEquivalentTo(owner)) + } + + originalObj := obj.DeepCopy() + obj.Data["method"] = "patch" + Expect(cl.Patch(ctx, obj, client.MergeFrom(originalObj))).NotTo(HaveOccurred()) + Expect(obj.ManagedFields).NotTo(BeEmpty()) + for _, f := range obj.ManagedFields { + Expect(f.Manager).To(BeEquivalentTo(owner)) + } + + obj.Data["method"] = "update" + Expect(cl.Update(ctx, obj)).NotTo(HaveOccurred()) + Expect(obj.ManagedFields).NotTo(BeEmpty()) + for _, f := range obj.ManagedFields { + Expect(f.Manager).To(BeEquivalentTo(owner)) + } + }) + + // GH-3267 + It("Doesn't leave stale data when updating an object through SSA", func(ctx SpecContext) { + obj := corev1applyconfigurations. + ConfigMap("foo", "default"). + WithData(map[string]string{"some": "data"}) + + cl := NewClientBuilder().Build() + Expect(cl.Apply(ctx, obj, client.FieldOwner("foo"))).NotTo(HaveOccurred()) + + obj.WithData(map[string]string{"bar": "baz"}) + Expect(cl.Apply(ctx, obj, client.FieldOwner("foo"))).NotTo(HaveOccurred()) + var cms corev1.ConfigMapList + Expect(cl.List(ctx, &cms)).NotTo(HaveOccurred()) + Expect(len(cms.Items)).To(BeEquivalentTo(1)) + }) + + It("allows to set deletionTimestamp on an object during SSA create", func(ctx SpecContext) { + now := metav1.Now() + obj := corev1applyconfigurations. + ConfigMap("foo", "default"). + WithDeletionTimestamp(now). + WithData(map[string]string{"some": "data"}) + + cl := NewClientBuilder().Build() + Expect(cl.Apply(ctx, obj, client.FieldOwner("foo"))).NotTo(HaveOccurred()) + + Expect(obj.DeletionTimestamp).To(BeEquivalentTo(&now)) + }) + + It("will silently ignore a deletionTimestamp update through SSA", func(ctx SpecContext) { + Skip("the apply logic in the managedFieldObjectTracker seems to override this") + now := metav1.Now() + obj := corev1applyconfigurations. + ConfigMap("foo", "default"). + WithDeletionTimestamp(now). + WithFinalizers("foo.bar"). + WithData(map[string]string{"some": "data"}) + + cl := NewClientBuilder().Build() + Expect(cl.Apply(ctx, obj, client.FieldOwner("foo"))).NotTo(HaveOccurred()) + Expect(obj.DeletionTimestamp).To(BeEquivalentTo(&now)) + + later := metav1.Time{Time: now.Add(time.Second)} + obj.DeletionTimestamp = &later + Expect(cl.Apply(ctx, obj, client.FieldOwner("foo"))).NotTo(HaveOccurred()) + Expect(*obj.DeletionTimestamp).To(BeEquivalentTo(now)) + }) + scalableObjs := []client.Object{ &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{