diff --git a/go.mod b/go.mod index 265fc443f6..e97dd4d03a 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module sigs.k8s.io/controller-runtime go 1.20 require ( + github.com/evanphx/json-patch v4.12.0+incompatible github.com/evanphx/json-patch/v5 v5.6.0 github.com/fsnotify/fsnotify v1.6.0 github.com/go-logr/logr v1.2.4 @@ -31,7 +32,6 @@ require ( github.com/cespare/xxhash/v2 v2.2.0 // indirect github.com/davecgh/go-spew v1.1.1 // indirect github.com/emicklei/go-restful/v3 v3.9.0 // indirect - github.com/evanphx/json-patch v4.12.0+incompatible // indirect github.com/go-openapi/jsonpointer v0.19.6 // indirect github.com/go-openapi/jsonreference v0.20.1 // indirect github.com/go-openapi/swag v0.22.3 // indirect diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 49b81140d1..7167c5505e 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -27,7 +27,10 @@ import ( "strconv" "strings" "sync" + "time" + // Using v4 to match upstream + jsonpatch "github.com/evanphx/json-patch" "sigs.k8s.io/controller-runtime/pkg/client/interceptor" corev1 "k8s.io/api/core/v1" @@ -41,8 +44,10 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" utilrand "k8s.io/apimachinery/pkg/util/rand" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/strategicpatch" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/kubernetes/scheme" @@ -285,6 +290,9 @@ func (t versionedTracker) Add(obj runtime.Object) error { if err != nil { return fmt.Errorf("failed to get accessor for object: %w", err) } + if accessor.GetDeletionTimestamp() != nil && len(accessor.GetFinalizers()) == 0 { + return fmt.Errorf("refusing to create obj %s with metadata.deletionTimestamp but no finalizers", accessor.GetName()) + } if accessor.GetResourceVersion() == "" { // We use a "magic" value of 999 here because this field // is parsed as uint and and 0 is already used in Update. @@ -368,10 +376,10 @@ func (t versionedTracker) Update(gvr schema.GroupVersionResource, obj runtime.Ob if bytes.Contains(debug.Stack(), []byte("sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeSubResourceClient).Patch")) { isStatus = true } - return t.update(gvr, obj, ns, isStatus) + return t.update(gvr, obj, ns, isStatus, false) } -func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Object, ns string, isStatus bool) error { +func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Object, ns string, isStatus bool, deleting bool) error { accessor, err := meta.Accessor(obj) if err != nil { return fmt.Errorf("failed to get accessor for object: %w", err) @@ -438,6 +446,11 @@ func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Ob } intResourceVersion++ accessor.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()) + } + if !accessor.GetDeletionTimestamp().IsZero() && len(accessor.GetFinalizers()) == 0 { return t.ObjectTracker.Delete(gvr, accessor.GetNamespace(), accessor.GetName()) } @@ -667,6 +680,10 @@ func (c *fakeClient) Create(ctx context.Context, obj client.Object, opts ...clie } accessor.SetName(fmt.Sprintf("%s%s", base, utilrand.String(randomLength))) } + // Ignore attempts to set deletion timestamp + if !accessor.GetDeletionTimestamp().IsZero() { + accessor.SetDeletionTimestamp(nil) + } return c.tracker.Create(gvr, obj, accessor.GetNamespace()) } @@ -778,7 +795,7 @@ func (c *fakeClient) update(obj client.Object, isStatus bool, opts ...client.Upd if err != nil { return err } - return c.tracker.update(gvr, obj, accessor.GetNamespace(), isStatus) + return c.tracker.update(gvr, obj, accessor.GetNamespace(), isStatus, false) } func (c *fakeClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { @@ -813,8 +830,39 @@ func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client return err } + oldObj, err := c.tracker.Get(gvr, accessor.GetNamespace(), accessor.GetName()) + if err != nil { + return err + } + 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) + 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) - handled, o, err := reaction(testing.NewPatchAction(gvr, accessor.GetNamespace(), accessor.GetName(), patch.Type(), data)) + handled, o, err := reaction(action) if err != nil { return err } @@ -838,6 +886,81 @@ func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client return err } +// Applying a patch results in a deletionTimestamp that is truncated to the nearest second. +// Check that the diff between a new and old deletion timestamp is within a reasonable threshold +// to be considered unchanged. +func deletionTimestampEqual(newObj metav1.Object, obj metav1.Object) bool { + newTime := newObj.GetDeletionTimestamp() + oldTime := obj.GetDeletionTimestamp() + + if newTime == nil || oldTime == nil { + return newTime == oldTime + } + return newTime.Time.Sub(oldTime.Time).Abs() < time.Second +} + +// The behavior of applying the patch is pulled out into dryPatch(), +// which applies the patch and returns an object, but does not Update() the object. +// This function returns a patched runtime object that may then be validated before a call to Update() is executed. +// 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) (runtime.Object, error) { + ns := action.GetNamespace() + gvr := action.GetResource() + + obj, err := tracker.Get(gvr, ns, action.GetName()) + if err != nil { + return nil, err + } + + old, err := json.Marshal(obj) + if err != nil { + return nil, err + } + + // reset the object in preparation to unmarshal, since unmarshal does not guarantee that fields + // in obj that are removed by patch are cleared + value := reflect.ValueOf(obj) + value.Elem().Set(reflect.New(value.Type().Elem()).Elem()) + + switch action.GetPatchType() { + case types.JSONPatchType: + patch, err := jsonpatch.DecodePatch(action.GetPatch()) + if err != nil { + return nil, err + } + modified, err := patch.Apply(old) + if err != nil { + return nil, err + } + + if err = json.Unmarshal(modified, obj); err != nil { + return nil, err + } + case types.MergePatchType: + modified, err := jsonpatch.MergePatch(old, action.GetPatch()) + if err != nil { + return nil, err + } + + if err := json.Unmarshal(modified, obj); err != nil { + return nil, err + } + case types.StrategicMergePatchType, types.ApplyPatchType: + mergedByte, err := strategicpatch.StrategicMergePatch(old, action.GetPatch(), obj) + if err != nil { + return nil, err + } + if err = json.Unmarshal(mergedByte, obj); err != nil { + return nil, err + } + default: + return nil, fmt.Errorf("PatchType is not supported") + } + return obj, nil +} + func copyNonStatusFrom(old, new runtime.Object) error { newClientObject, ok := new.(client.Object) if !ok { @@ -945,7 +1068,9 @@ func (c *fakeClient) deleteObject(gvr schema.GroupVersionResource, accessor meta if len(oldAccessor.GetFinalizers()) > 0 { now := metav1.Now() oldAccessor.SetDeletionTimestamp(&now) - return c.tracker.Update(gvr, old, accessor.GetNamespace()) + // Call update directly with mutability parameter set to true to allow + // changes to deletionTimestamp + return c.tracker.update(gvr, old, accessor.GetNamespace(), false, true) } } } diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index a5e8abeb5a..a4e807a526 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -726,6 +726,54 @@ var _ = Describe("Fake client", func() { Expect(apierrors.IsNotFound(err)).To(BeTrue()) }) + It("should reject changes to deletionTimestamp on Update", func() { + namespacedName := types.NamespacedName{ + Name: "test-cm", + Namespace: "reject-with-deletiontimestamp", + } + By("Updating a new object") + newObj := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: namespacedName.Name, + Namespace: namespacedName.Namespace, + }, + Data: map[string]string{ + "test-key": "new-value", + }, + } + err := cl.Create(context.Background(), newObj) + Expect(err).To(BeNil()) + + By("Getting the object") + obj := &corev1.ConfigMap{} + err = cl.Get(context.Background(), namespacedName, obj) + Expect(err).To(BeNil()) + Expect(obj.DeletionTimestamp).To(BeNil()) + + By("Adding deletionTimestamp") + now := metav1.Now() + obj.DeletionTimestamp = &now + err = cl.Update(context.Background(), obj) + Expect(err).NotTo(BeNil()) + + By("Deleting the object") + err = cl.Delete(context.Background(), newObj) + Expect(err).To(BeNil()) + + By("Changing the deletionTimestamp to new value") + obj = &corev1.ConfigMap{} + t := metav1.NewTime(time.Now().Add(time.Second)) + obj.DeletionTimestamp = &t + err = cl.Update(context.Background(), obj) + Expect(err).NotTo(BeNil()) + + By("Removing deletionTimestamp") + obj.DeletionTimestamp = nil + err = cl.Update(context.Background(), obj) + Expect(err).NotTo(BeNil()) + + }) + It("should be able to Delete a Collection", func() { By("Deleting a deploymentList") err := cl.DeleteAllOf(context.Background(), &appsv1.Deployment{}, client.InNamespace("ns1")) @@ -897,12 +945,12 @@ var _ = Describe("Fake client", func() { Expect(obj.ObjectMeta.ResourceVersion).To(Equal("1000")) }) - It("should handle finalizers on Patch", func() { + It("should ignore deletionTimestamp without finalizer on Create", func() { namespacedName := types.NamespacedName{ Name: "test-cm", - Namespace: "delete-with-finalizers", + Namespace: "ignore-deletiontimestamp", } - By("Updating a new object") + By("Creating a new object") now := metav1.Now() newObj := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ @@ -915,10 +963,81 @@ var _ = Describe("Fake client", func() { "test-key": "new-value", }, } + err := cl.Create(context.Background(), newObj) Expect(err).To(BeNil()) - By("Removing the finalizer") + By("Getting the object") + obj := &corev1.ConfigMap{} + err = cl.Get(context.Background(), namespacedName, obj) + Expect(err).To(BeNil()) + Expect(obj.DeletionTimestamp).To(BeNil()) + + }) + + It("should reject deletionTimestamp without finalizers on Build", func() { + namespacedName := types.NamespacedName{ + Name: "test-cm", + Namespace: "reject-deletiontimestamp-no-finalizers", + } + By("Build with a new object without finalizer") + now := metav1.Now() + obj := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: namespacedName.Name, + Namespace: namespacedName.Namespace, + DeletionTimestamp: &now, + }, + Data: map[string]string{ + "test-key": "new-value", + }, + } + + Expect(func() { NewClientBuilder().WithObjects(obj).Build() }).To(Panic()) + + By("Build with a new object with finalizer") + newObj := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: namespacedName.Name, + Namespace: namespacedName.Namespace, + Finalizers: []string{"finalizers.sigs.k8s.io/test"}, + DeletionTimestamp: &now, + }, + Data: map[string]string{ + "test-key": "new-value", + }, + } + + cl := NewClientBuilder().WithObjects(newObj).Build() + + By("Getting the object") + obj = &corev1.ConfigMap{} + err := cl.Get(context.Background(), namespacedName, obj) + Expect(err).To(BeNil()) + + }) + + It("should reject changes to deletionTimestamp on Patch", func() { + namespacedName := types.NamespacedName{ + Name: "test-cm", + Namespace: "reject-deletiontimestamp", + } + By("Creating a new object") + now := metav1.Now() + newObj := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: namespacedName.Name, + Namespace: namespacedName.Namespace, + Finalizers: []string{"finalizers.sigs.k8s.io/test"}, + }, + Data: map[string]string{ + "test-key": "new-value", + }, + } + err := cl.Create(context.Background(), newObj) + Expect(err).To(BeNil()) + + By("Add a deletionTimestamp") obj := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: namespacedName.Name, @@ -927,7 +1046,70 @@ var _ = Describe("Fake client", func() { DeletionTimestamp: &now, }, } - obj.Finalizers = []string{} + err = cl.Patch(context.Background(), obj, client.MergeFrom(newObj)) + Expect(err).NotTo(BeNil()) + + By("Deleting the object") + err = cl.Delete(context.Background(), newObj) + Expect(err).To(BeNil()) + + By("Getting the object") + obj = &corev1.ConfigMap{} + err = cl.Get(context.Background(), namespacedName, obj) + Expect(err).To(BeNil()) + Expect(obj.DeletionTimestamp).NotTo(BeNil()) + + By("Changing the deletionTimestamp to new value") + newObj = &corev1.ConfigMap{} + t := metav1.NewTime(time.Now().Add(time.Second)) + newObj.DeletionTimestamp = &t + err = cl.Patch(context.Background(), newObj, client.MergeFrom(obj)) + Expect(err).NotTo(BeNil()) + + By("Removing deletionTimestamp") + newObj = &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: namespacedName.Name, + Namespace: namespacedName.Namespace, + DeletionTimestamp: nil, + }, + } + err = cl.Patch(context.Background(), newObj, client.MergeFrom(obj)) + Expect(err).NotTo(BeNil()) + + }) + + It("should handle finalizers on Patch", func() { + namespacedName := types.NamespacedName{ + Name: "test-cm", + Namespace: "delete-with-finalizers", + } + By("Creating a new object") + newObj := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: namespacedName.Name, + Namespace: namespacedName.Namespace, + Finalizers: []string{"finalizers.sigs.k8s.io/test"}, + }, + Data: map[string]string{ + "test-key": "new-value", + }, + } + err := cl.Create(context.Background(), newObj) + Expect(err).To(BeNil()) + + By("Deleting the object") + err = cl.Delete(context.Background(), newObj) + Expect(err).To(BeNil()) + + By("Removing the finalizer") + obj := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: namespacedName.Name, + Namespace: namespacedName.Namespace, + Finalizers: []string{}, + }, + } err = cl.Patch(context.Background(), obj, client.MergeFrom(newObj)) Expect(err).To(BeNil())