Ensure finalizer is present before reconciling

pkg/reconciler/reconciler.go

@@ -521,6 +521,19 @@ 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 {
+			log.Error(err, "Failed to add uninstall finalizer")
+			return ctrl.Result{}, err
+		}
+	}
+
 	u := updater.New(r.client)
 	defer func() {
 		applyErr := u.Apply(ctx, obj)
@@ -538,9 +551,9 @@ 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:
+		// 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
@@ -953,7 +966,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

@@ -604,8 +604,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() {
@@ -641,8 +641,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())
 							})
 						})
 					})
@@ -677,8 +677,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())
 							})
 						})
 					})
@@ -744,8 +744,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())
 								})
 							})
 						})
@@ -925,7 +925,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())
 							})
 						})
@@ -964,7 +964,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())
 							})
 						})