From 4f23760919821da36cf306a1fe8c34a9914ebf8d Mon Sep 17 00:00:00 2001 From: Thibault Jamet Date: Fri, 26 Feb 2021 08:50:34 +0100 Subject: [PATCH 1/2] Handle finalizers in the fake client The fake client differs significantly from the real implementation. With the real client implementation, upon the deletion of a resource, when finalizers are set the DeletionTimestamp is filled with a non-null value. This triggers a number of reconciling loops for all controllers that then can take the opportunity to inspect the resource being deleted, perform the required cleaning actions and then update the resource by removing their finalizers as the cleaning job have been done. In the current implementation, the Delete function implements a straight delete and hence triggers a false positive or negative test when trying to run tests at the client level, without interacting with the objects. For example: ``` client.CreateObject() controller.Reconcile() Expect(Something).To(BeDone()) client.DeleteObject() controller.Reconcile() Expect(SomethingElse).To(BeDone()) ``` In the real life, SomethingElse would actually be done as the controller would still have access to the resource. In the current fake implementation, this is not the case as the controller does not have access to the resource any longer. --- pkg/client/fake/client.go | 15 +++++++ pkg/client/fake/client_test.go | 80 ++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 1747731945..4be4036dad 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -248,6 +248,9 @@ func (t versionedTracker) Update(gvr schema.GroupVersionResource, obj runtime.Ob } intResourceVersion++ accessor.SetResourceVersion(strconv.FormatUint(intResourceVersion, 10)) + if !accessor.GetDeletionTimestamp().IsZero() && len(accessor.GetFinalizers()) == 0 { + return t.ObjectTracker.Delete(gvr, accessor.GetNamespace(), accessor.GetName()) + } return t.ObjectTracker.Update(gvr, obj, ns) } @@ -389,6 +392,18 @@ func (c *fakeClient) Delete(ctx context.Context, obj client.Object, opts ...clie delOptions := client.DeleteOptions{} delOptions.ApplyOptions(opts) + old, err := c.tracker.Get(gvr, accessor.GetNamespace(), accessor.GetName()) + if err == nil { + oldAccessor, err := meta.Accessor(old) + if err == nil { + if len(oldAccessor.GetFinalizers()) > 0 { + now := metav1.Now() + oldAccessor.SetDeletionTimestamp(&now) + return c.tracker.Update(gvr, old, accessor.GetNamespace()) + } + } + } + //TODO: implement propagation return c.tracker.Delete(gvr, accessor.GetNamespace(), accessor.GetName()) } diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index 685a30ecd8..5cc1382386 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -443,6 +443,46 @@ var _ = Describe("Fake client", func() { Expect(list.Items).To(ConsistOf(*dep2)) }) + It("should handle finalizers on Update", func() { + namespacedName := types.NamespacedName{ + Name: "test-cm", + Namespace: "delete-with-finalizers", + } + By("Updating 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("Getting the object") + obj := &corev1.ConfigMap{} + err = cl.Get(context.Background(), namespacedName, obj) + Expect(err).To(BeNil()) + Expect(obj.DeletionTimestamp).NotTo(BeNil()) + + By("Removing the finalizer") + obj.Finalizers = []string{} + err = cl.Update(context.Background(), obj) + Expect(err).To(BeNil()) + + By("Getting the object") + obj = &corev1.ConfigMap{} + err = cl.Get(context.Background(), namespacedName, obj) + Expect(apierrors.IsNotFound(err)).To(BeTrue()) + }) + It("should be able to Delete a Collection", func() { By("Deleting a deploymentList") err := cl.DeleteAllOf(context.Background(), &appsv1.Deployment{}, client.InNamespace("ns1")) @@ -531,6 +571,46 @@ var _ = Describe("Fake client", func() { Expect(obj.Annotations["foo"]).To(Equal("bar")) Expect(obj.ObjectMeta.ResourceVersion).To(Equal("1000")) }) + + It("should handle finalizers on Patch", func() { + namespacedName := types.NamespacedName{ + Name: "test-cm", + Namespace: "delete-with-finalizers", + } + By("Updating 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"}, + DeletionTimestamp: &now, + }, + Data: map[string]string{ + "test-key": "new-value", + }, + } + err := cl.Create(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{}, + DeletionTimestamp: &now, + }, + } + obj.Finalizers = []string{} + err = cl.Patch(context.Background(), obj, client.MergeFrom(newObj)) + Expect(err).To(BeNil()) + + By("Getting the object") + obj = &corev1.ConfigMap{} + err = cl.Get(context.Background(), namespacedName, obj) + Expect(apierrors.IsNotFound(err)).To(BeTrue()) + }) } Context("with default scheme.Scheme", func() { From 286a287c83fb83b1a9545dc119d3c79b49a25d38 Mon Sep 17 00:00:00 2001 From: Thibault Jamet Date: Tue, 2 Mar 2021 16:09:14 +0100 Subject: [PATCH 2/2] Handle finalizers when deleting collections --- pkg/client/fake/client.go | 34 ++++++++++++++++++-------------- pkg/client/fake/client_test.go | 36 ++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 15 deletions(-) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 4be4036dad..39eb9c597e 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -392,20 +392,7 @@ func (c *fakeClient) Delete(ctx context.Context, obj client.Object, opts ...clie delOptions := client.DeleteOptions{} delOptions.ApplyOptions(opts) - old, err := c.tracker.Get(gvr, accessor.GetNamespace(), accessor.GetName()) - if err == nil { - oldAccessor, err := meta.Accessor(old) - if err == nil { - if len(oldAccessor.GetFinalizers()) > 0 { - now := metav1.Now() - oldAccessor.SetDeletionTimestamp(&now) - return c.tracker.Update(gvr, old, accessor.GetNamespace()) - } - } - } - - //TODO: implement propagation - return c.tracker.Delete(gvr, accessor.GetNamespace(), accessor.GetName()) + return c.deleteObject(gvr, accessor) } func (c *fakeClient) DeleteAllOf(ctx context.Context, obj client.Object, opts ...client.DeleteAllOfOption) error { @@ -436,7 +423,7 @@ func (c *fakeClient) DeleteAllOf(ctx context.Context, obj client.Object, opts .. if err != nil { return err } - err = c.tracker.Delete(gvr, accessor.GetNamespace(), accessor.GetName()) + err = c.deleteObject(gvr, accessor) if err != nil { return err } @@ -521,6 +508,23 @@ func (c *fakeClient) Status() client.StatusWriter { return &fakeStatusWriter{client: c} } +func (c *fakeClient) deleteObject(gvr schema.GroupVersionResource, accessor metav1.Object) error { + old, err := c.tracker.Get(gvr, accessor.GetNamespace(), accessor.GetName()) + if err == nil { + oldAccessor, err := meta.Accessor(old) + if err == nil { + if len(oldAccessor.GetFinalizers()) > 0 { + now := metav1.Now() + oldAccessor.SetDeletionTimestamp(&now) + return c.tracker.Update(gvr, old, accessor.GetNamespace()) + } + } + } + + //TODO: implement propagation + return c.tracker.Delete(gvr, accessor.GetNamespace(), accessor.GetName()) +} + func getGVRFromObject(obj runtime.Object, scheme *runtime.Scheme) (schema.GroupVersionResource, error) { gvk, err := apiutil.GVKForObject(obj, scheme) if err != nil { diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index 5cc1382386..1e89bb97bf 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -19,6 +19,7 @@ package fake import ( "context" "encoding/json" + "fmt" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -495,6 +496,41 @@ var _ = Describe("Fake client", func() { Expect(list.Items).To(BeEmpty()) }) + It("should handle finalizers deleting a collection", func() { + for i := 0; i < 5; i++ { + namespacedName := types.NamespacedName{ + Name: fmt.Sprintf("test-cm-%d", i), + Namespace: "delete-collection-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.DeleteAllOf(context.Background(), &corev1.ConfigMap{}, client.InNamespace("delete-collection-with-finalizers")) + Expect(err).To(BeNil()) + + configmaps := corev1.ConfigMapList{} + err = cl.List(context.Background(), &configmaps, client.InNamespace("delete-collection-with-finalizers")) + Expect(err).To(BeNil()) + + Expect(len(configmaps.Items)).To(Equal(5)) + for _, cm := range configmaps.Items { + Expect(cm.DeletionTimestamp).NotTo(BeNil()) + } + }) + Context("with the DryRun option", func() { It("should not create a new object", func() { By("Creating a new configmap with DryRun")