Ensure finalizer is present before reconciling (#218)

* Ensure finalizer is present before reconciling

* Fix gomod

go.mod

@@ -11,6 +11,7 @@ require (
 	github.com/onsi/ginkgo/v2 v2.11.0
 	github.com/onsi/gomega v1.27.8
 	github.com/operator-framework/operator-lib v0.11.1-0.20230607132417-ecb9be488378
+	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.15.1
 	github.com/sergi/go-diff v1.2.0
 	github.com/sirupsen/logrus v1.9.3
@@ -129,7 +130,6 @@ require (
 	github.com/opencontainers/image-spec v1.1.0-rc2.0.20221005185240-3a7f492d3f1b // indirect
 	github.com/peterbourgon/diskv v2.0.1+incompatible // indirect
 	github.com/phayes/freeport v0.0.0-20220201140144-74d24b5ae9f5 // indirect
-	github.com/pkg/errors v0.9.1 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/prometheus/client_model v0.4.0 // indirect
 	github.com/prometheus/common v0.44.0 // indirect

pkg/reconciler/internal/updater/updater.go

@@ -122,16 +122,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

@@ -59,7 +59,10 @@ 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(func(u *unstructured.Unstructured) bool {
+				u.SetAnnotations(map[string]string{"foo": "bar"})
+				return true
+			})
 			err := u.Apply(context.TODO(), obj)
 			Expect(err).NotTo(BeNil())
 			Expect(apierrors.IsNotFound(err)).To(BeTrue())
@@ -68,12 +71,15 @@ var _ = Describe("Updater", func() {
 
 	When("an update is a change", func() {
 		It("should apply an update function", func() {
-			u.Update(EnsureFinalizer(testFinalizer))
+			u.Update(func(u *unstructured.Unstructured) bool {
+				u.SetAnnotations(map[string]string{"foo": "bar"})
+				return true
+			})
 			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 +95,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"
@@ -553,6 +554,18 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.
 		return ctrl.Result{}, err
 	}
 
+	// The finalizer must be present on the CR before we can do anything. Otherwise, if the reconciliation fails,
+	// there might be resources created by the chart that will not be garbage-collected
+	// (cluster-scoped resources or resources in other namespaces, which are not bound by an owner reference).
+	// This is a safety measure to ensure that the chart is fully uninstalled before the CR is deleted.
+	if obj.GetDeletionTimestamp() == nil && !controllerutil.ContainsFinalizer(obj, uninstallFinalizer) {
+		log.V(1).Info("Adding uninstall finalizer.")
+		obj.SetFinalizers(append(obj.GetFinalizers(), uninstallFinalizer))
+		if err := r.client.Update(ctx, obj); err != nil {
+			return ctrl.Result{}, errs.Wrapf(err, "failed to add uninstall finalizer to %s/%s", req.NamespacedName.Namespace, req.NamespacedName.Name)
+		}
+	}
+
 	u := updater.New(r.client)
 	defer func() {
 		applyErr := u.Apply(ctx, obj)
@@ -570,14 +583,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 means a release WAS deployed at some point
-		//   in the past, but we don't know if it still is because we don't have an actionClient to check.
-		//   So the question is, what do we do with the finalizer? We could:
-		//      - 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.
@@ -984,7 +991,6 @@ func ensureDeployedRelease(u *updater.Updater, rel *release.Release) {
 	if rel.Info != nil && len(rel.Info.Notes) > 0 {
 		message = rel.Info.Notes
 	}
-	u.Update(updater.EnsureFinalizer(uninstallFinalizer))
 	u.UpdateStatus(
 		updater.EnsureCondition(conditions.Deployed(corev1.ConditionTrue, reason, message)),
 		updater.EnsureDeployedRelease(rel),

pkg/reconciler/reconciler_test.go

@@ -607,8 +607,8 @@ var _ = Describe("Reconciler", func() {
 								Expect(c.Message).To(Equal(acgErr.Error()))
 							})
 
-							By("verifying the uninstall finalizer is not present on the CR", func() {
-								Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeFalse())
+							By("verifying the uninstall finalizer is present on the CR", func() {
+								Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeTrue())
 							})
 						})
 						It("returns an error getting the release", func() {
@@ -644,8 +644,8 @@ var _ = Describe("Reconciler", func() {
 								Expect(c.Message).To(Equal("get not implemented"))
 							})
 
-							By("verifying the uninstall finalizer is not present on the CR", func() {
-								Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeFalse())
+							By("verifying the uninstall finalizer is present on the CR", func() {
+								Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeTrue())
 							})
 						})
 					})
@@ -680,8 +680,8 @@ var _ = Describe("Reconciler", func() {
 								Expect(c.Message).To(ContainSubstring("error parsing index"))
 							})
 
-							By("verifying the uninstall finalizer is not present on the CR", func() {
-								Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeFalse())
+							By("verifying the uninstall finalizer is present on the CR", func() {
+								Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeTrue())
 							})
 						})
 					})
@@ -747,8 +747,8 @@ var _ = Describe("Reconciler", func() {
 									Expect(c.Message).To(ContainSubstring("install failed: foobar"))
 								})
 
-								By("ensuring the uninstall finalizer is not present on the CR", func() {
-									Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeFalse())
+								By("verifying the uninstall finalizer is present on the CR", func() {
+									Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeTrue())
 								})
 							})
 						})
@@ -928,7 +928,7 @@ var _ = Describe("Reconciler", func() {
 								Expect(objStat.Status.DeployedRelease.Manifest).To(Equal(currentRelease.Manifest))
 							})
 
-							By("verifying the uninstall finalizer is not present on the CR", func() {
+							By("verifying the uninstall finalizer is present on the CR", func() {
 								Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeTrue())
 							})
 						})
@@ -967,7 +967,7 @@ var _ = Describe("Reconciler", func() {
 								Expect(objStat.Status.DeployedRelease.Manifest).To(Equal(currentRelease.Manifest))
 							})
 
-							By("verifying the uninstall finalizer is not present on the CR", func() {
+							By("verifying the uninstall finalizer is present on the CR", func() {
 								Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeTrue())
 							})
 						})