Remove EnsureFinalizer

pkg/reconciler/internal/updater/updater.go

@@ -94,16 +94,6 @@ func (u *Updater) Apply(ctx context.Context, obj *unstructured.Unstructured) err
 	return nil
 }
 
-func EnsureFinalizer(finalizer string) UpdateFunc {
-	return func(obj *unstructured.Unstructured) bool {
-		if controllerutil.ContainsFinalizer(obj, finalizer) {
-			return false
-		}
-		controllerutil.AddFinalizer(obj, finalizer)
-		return true
-	}
-}
-
 func RemoveFinalizer(finalizer string) UpdateFunc {
 	return func(obj *unstructured.Unstructured) bool {
 		if !controllerutil.ContainsFinalizer(obj, finalizer) {

pkg/reconciler/internal/updater/updater_test.go

@@ -34,6 +34,23 @@ import (
 
 const testFinalizer = "testFinalizer"
 
+func EnsureAnnotation(key, value string) UpdateFunc {
+	return func(obj *unstructured.Unstructured) bool {
+		var hasUpdate bool
+		if obj.GetAnnotations() == nil {
+			obj.SetAnnotations(map[string]string{})
+			hasUpdate = true
+		}
+		var annotations = obj.GetAnnotations()
+		if annotations[key] != value {
+			annotations[key] = value
+			obj.SetAnnotations(annotations)
+			hasUpdate = true
+		}
+		return hasUpdate
+	}
+}
+
 var _ = Describe("Updater", func() {
 	var (
 		client client.Client
@@ -59,7 +76,7 @@ var _ = Describe("Updater", func() {
 	When("the object does not exist", func() {
 		It("should fail", func() {
 			Expect(client.Delete(context.TODO(), obj)).To(Succeed())
-			u.Update(EnsureFinalizer(testFinalizer))
+			u.Update(EnsureAnnotation("foo", "bar"))
 			err := u.Apply(context.TODO(), obj)
 			Expect(err).NotTo(BeNil())
 			Expect(apierrors.IsNotFound(err)).To(BeTrue())
@@ -68,12 +85,12 @@ var _ = Describe("Updater", func() {
 
 	When("an update is a change", func() {
 		It("should apply an update function", func() {
-			u.Update(EnsureFinalizer(testFinalizer))
+			u.Update(EnsureAnnotation("foo", "bar"))
 			resourceVersion := obj.GetResourceVersion()
 
 			Expect(u.Apply(context.TODO(), obj)).To(Succeed())
 			Expect(client.Get(context.TODO(), types.NamespacedName{Namespace: "testNamespace", Name: "testDeployment"}, obj)).To(Succeed())
-			Expect(obj.GetFinalizers()).To(Equal([]string{testFinalizer}))
+			Expect(obj.GetAnnotations()["foo"]).To(Equal("bar"))
 			Expect(obj.GetResourceVersion()).NotTo(Equal(resourceVersion))
 		})
 
@@ -89,25 +106,6 @@ var _ = Describe("Updater", func() {
 	})
 })
 
-var _ = Describe("EnsureFinalizer", func() {
-	var obj *unstructured.Unstructured
-
-	BeforeEach(func() {
-		obj = &unstructured.Unstructured{}
-	})
-
-	It("should add finalizer if not present", func() {
-		Expect(EnsureFinalizer(testFinalizer)(obj)).To(BeTrue())
-		Expect(obj.GetFinalizers()).To(Equal([]string{testFinalizer}))
-	})
-
-	It("should not add duplicate finalizer", func() {
-		obj.SetFinalizers([]string{testFinalizer})
-		Expect(EnsureFinalizer(testFinalizer)(obj)).To(BeFalse())
-		Expect(obj.GetFinalizers()).To(Equal([]string{testFinalizer}))
-	})
-})
-
 var _ = Describe("RemoveFinalizer", func() {
 	var obj *unstructured.Unstructured
 

pkg/reconciler/reconciler.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	errs "github.com/pkg/errors"
 	"strings"
 	"sync"
 	"time"
@@ -529,8 +530,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.
 		log.V(1).Info("Adding uninstall finalizer")
 		obj.SetFinalizers(append(obj.GetFinalizers(), uninstallFinalizer))
 		if err := r.client.Update(ctx, obj); err != nil {
-			log.Error(err, "Failed to add uninstall finalizer")
-			return ctrl.Result{}, err
+			return ctrl.Result{}, errs.Wrapf(err, "failed to add uninstall finalizer to %s/%s", req.NamespacedName.Namespace, req.NamespacedName.Name)
 		}
 	}
 
@@ -551,14 +551,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.
 			updater.EnsureConditionUnknown(conditions.TypeReleaseFailed),
 			updater.EnsureDeployedRelease(nil),
 		)
-		// NOTE: If obj has the uninstall finalizer, that doesn't mean a release was deployed at some point in the past.
-		//   This just means the CR was accepted by the reconcile method. The presence of the finalizer is required
-		//   to deploy any resources.
-		//      - Leave it in place. This would make the CR impossible to delete without either resolving this error, or
-		//        manually uninstalling the release, deleting the finalizer, and deleting the CR.
-		//      - Remove the finalizer. This would make it possible to delete the CR, but it would leave around any
-		//        release resources that are not owned by the CR (those in the cluster scope or in other namespaces).
-		//
+		// When it is impossible to obtain an actionClient, we cannot proceed with the reconciliation. Question is
+		// what to do with the finalizer?
 		// The decision made for now is to leave the finalizer in place, so that the user can intervene and try to
 		// resolve the issue, instead of the operator silently leaving some dangling resources hanging around after the
 		// CR is deleted.