Address review comments

Signed-off-by: Rashmi Gottipati <[email protected]>

Author: rashmigottipati

pkg/finalizer/finalizer.go

@@ -17,13 +17,16 @@ import (
 	"context"
 	"fmt"
 
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
+type finalizers map[string]Finalizer
+
 // NewFinalizers returns the Finalizers interface
 func NewFinalizers() Finalizers {
-	return finalizers(map[string]Finalizer{})
+	return finalizers{}
 }
 
 func (f finalizers) Register(key string, finalizer Finalizer) error {
@@ -35,19 +38,26 @@ func (f finalizers) Register(key string, finalizer Finalizer) error {
 }
 
 func (f finalizers) Finalize(ctx context.Context, obj client.Object) (bool, error) {
+	var errList []error
 	needsUpdate := false
 	for key, finalizer := range f {
-		if obj.GetDeletionTimestamp().IsZero() && !controllerutil.ContainsFinalizer(obj, key) {
+		if dt := obj.GetDeletionTimestamp(); dt.IsZero() && !controllerutil.ContainsFinalizer(obj, key) {
 			controllerutil.AddFinalizer(obj, key)
 			needsUpdate = true
-		} else if obj.GetDeletionTimestamp() != nil && controllerutil.ContainsFinalizer(obj, key) {
-			ret, err := finalizer.Finalize(ctx, obj)
+		} else if !dt.IsZero() && controllerutil.ContainsFinalizer(obj, key) {
+			finalizerNeedsUpdate, err := finalizer.Finalize(ctx, obj)
 			if err != nil {
-				return ret, fmt.Errorf("finalize failed for key %q: %v", key, err)
+				// Even when the finalizer fails, it may need to signal to update the primary
+				// object (e.g. it may set a condition and need a status update).
+				needsUpdate = needsUpdate || finalizerNeedsUpdate
+				errList = append(errList, fmt.Errorf("finalizer %q failed: %v", key, err))
+			} else {
+				// If the finalizer succeeds, we remove the finalizer from the primary
+				// object's metadata, so we know it will need an update.
+				needsUpdate = true
+				controllerutil.RemoveFinalizer(obj, key)
 			}
-			controllerutil.RemoveFinalizer(obj, key)
-			needsUpdate = true
 		}
 	}
-	return needsUpdate, nil
+	return needsUpdate, utilerrors.NewAggregate(errList)
 }

pkg/finalizer/finalizer_test.go

@@ -43,13 +43,13 @@ var _ = Describe("TestFinalizer", func() {
 		Expect(f).NotTo(BeNil())
 
 	})
-	Describe("Finalizer helper library", func() {
-		It("Register should successfully register a finalizer", func() {
+	Describe("Register", func() {
+		It("successfully registers a finalizer", func() {
 			err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f)
 			Expect(err).To(BeNil())
 		})
 
-		It("Register should return an error when it is called twice with the same key", func() {
+		It("should fail when trying to register a finalizer that was already registered", func() {
 			err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f)
 			Expect(err).To(BeNil())
 
@@ -59,31 +59,72 @@ var _ = Describe("TestFinalizer", func() {
 			Expect(err.Error()).To(ContainSubstring("already registered"))
 
 		})
-
-		It("Finalize should return no error and indicate false for needsUpdate if a finalizer is not registered", func() {
+	})
+	Describe("Finalize", func() {
+		It("should return no error and return false for needsUpdate if a finalizer is not registered", func() {
 			ret, err := finalizers.Finalize(context.TODO(), pod)
 			Expect(err).To(BeNil())
 			Expect(ret).To(BeFalse())
 		})
 
-		It("Finalize should return no error when deletion timestamp is not nil and the finalizer exists", func() {
+		It("successfully finalizes and returns true for needsUpdate when deletion timestamp is nil and finalizer does not exist", func() {
+			err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f)
+			Expect(err).To(BeNil())
+
+			pod.DeletionTimestamp = nil
+			pod.Finalizers = []string{}
+
+			needsUpdate, err := finalizers.Finalize(context.TODO(), pod)
+			Expect(err).To(BeNil())
+			Expect(needsUpdate).To(BeTrue())
+		})
+
+		It("successfully finalizes and returns true for needsUpdate when deletion timestamp is not nil and the finalizer exists", func() {
 			now := metav1.Now()
 			pod.DeletionTimestamp = &now
 
 			err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f)
 			Expect(err).To(BeNil())
 
-			_, err := finalizers.Finalize(context.TODO(), pod)
+			pod.Finalizers = []string{"finalizers.sigs.k8s.io/testfinalizer"}
+
+			needsUpdate, err := finalizers.Finalize(context.TODO(), pod)
 			Expect(err).To(BeNil())
+			Expect(needsUpdate).To(BeTrue())
 		})
 
-		It("Finalize should return no error when deletion timestamp is nil and finalizer does not exist", func() {
-			err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f)
+		It("should return no error and return false for needsUpdate when deletion timestamp is nil and finalizer doesn't exist", func() {
+			pod.DeletionTimestamp = nil
+			pod.Finalizers = []string{}
+
+			needsUpdate, err := finalizers.Finalize(context.TODO(), pod)
 			Expect(err).To(BeNil())
+			Expect(needsUpdate).To(BeFalse())
+		})
 
-			pod.DeletionTimestamp = nil
+		It("should return no error and return false for needsUpdate when deletion timestamp is not nil and the finalizer doesn't exist", func() {
+			now := metav1.Now()
+			pod.DeletionTimestamp = &now
 			pod.Finalizers = []string{}
 
+			needsUpdate, err := finalizers.Finalize(context.TODO(), pod)
+			Expect(err).To(BeNil())
+			Expect(needsUpdate).To(BeFalse())
+
+		})
+
+		It("successfully finalizes multiple finalizers and returns true for needsUpdate when deletion timestamp is not nil and the finalizer exists", func() {
+			now := metav1.Now()
+			pod.DeletionTimestamp = &now
+
+			err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f)
+			Expect(err).To(BeNil())
+
+			err = finalizers.Register("finalizers.sigs.k8s.io/newtestfinalizer", f)
+			Expect(err).To(BeNil())
+
+			pod.Finalizers = []string{"finalizers.sigs.k8s.io/testfinalizer", "finalizers.sigs.k8s.io/newtestfinalizer"}
+
 			needsUpdate, err := finalizers.Finalize(context.TODO(), pod)
 			Expect(err).To(BeNil())
 			Expect(needsUpdate).To(BeTrue())

pkg/finalizer/types.go

@@ -33,8 +33,6 @@ type Finalizer interface {
 	Finalize(context.Context, client.Object) (needsUpdate bool, err error)
 }
 
-type finalizers map[string]Finalizer
-
 // Finalizers implements Registerer and Finalizer to finalize all registered
 // finalizers if the provided object has a deletion timestamp or set all
 // registered finalizers if it does not