⚠ Handle finalizers in the fake client (#1399)

* 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.

* Handle finalizers when deleting collections

Author: tjamet

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,8 +392,7 @@ func (c *fakeClient) Delete(ctx context.Context, obj client.Object, opts ...clie
 	delOptions := client.DeleteOptions{}
 	delOptions.ApplyOptions(opts)
 
-	//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 {
@@ -421,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
 		}
@@ -506,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 {

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"
@@ -443,6 +444,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"))
@@ -455,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")
@@ -531,6 +607,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() {