pkg/helm/controller/reconcile.go: finalizer fix (#3046)

This PR fixes a bug in the helm operator that caused the
CR to be unable to be deleted without manually intervening
to delete a prematurely added finalizer.

Helm operator now applies its uninstall finalizer only when
a release is deployed.

Co-authored-by: Joe Lanford <[email protected]>

Author: openshift-cherrypick-robot

changelog/fragments/helm-fix-finalizer.yaml

@@ -0,0 +1,7 @@
+entries:
+  - description: >
+      Helm operator now applies its uninstall finalizer only when
+      a release is deployed. This fixes a bug that caused the 
+      CR to be unable to be deleted without manually intervening
+      to delete a prematurely added finalizer.
+    kind: bugfix

pkg/helm/controller/controller.go

@@ -36,7 +36,6 @@ import (
 
 	"github.com/operator-framework/operator-sdk/pkg/helm/release"
 	"github.com/operator-framework/operator-sdk/pkg/internal/predicates"
-	"github.com/operator-framework/operator-sdk/pkg/predicate"
 )
 
 var log = logf.Log.WithName("helm.controller")
@@ -80,8 +79,7 @@ func Add(mgr manager.Manager, options WatchOptions) error {
 
 	o := &unstructured.Unstructured{}
 	o.SetGroupVersionKind(options.GVK)
-	if err := c.Watch(&source.Kind{Type: o}, &crthandler.EnqueueRequestForObject{},
-		predicate.GenerationChangedPredicate{}); err != nil {
+	if err := c.Watch(&source.Kind{Type: o}, &crthandler.EnqueueRequestForObject{}); err != nil {
 		return err
 	}
 

pkg/helm/controller/reconcile.go

@@ -29,7 +29,9 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/tools/record"
+	"k8s.io/client-go/util/retry"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	"github.com/operator-framework/operator-sdk/internal/util/diffutil"
@@ -94,38 +96,8 @@ func (r HelmOperatorReconciler) Reconcile(request reconcile.Request) (reconcile.
 	status := types.StatusFor(o)
 	log = log.WithValues("release", manager.ReleaseName())
 
-	deleted := o.GetDeletionTimestamp() != nil
-	pendingFinalizers := o.GetFinalizers()
-	if !deleted && !contains(pendingFinalizers, finalizer) {
-		log.V(1).Info("Adding finalizer", "finalizer", finalizer)
-		finalizers := append(pendingFinalizers, finalizer)
-		o.SetFinalizers(finalizers)
-		err = r.updateResource(o)
-
-		// Need to requeue because finalizer update does not change metadata.generation
-		return reconcile.Result{Requeue: true}, err
-	}
-
-	status.SetCondition(types.HelmAppCondition{
-		Type:   types.ConditionInitialized,
-		Status: types.StatusTrue,
-	})
-
-	if err := manager.Sync(context.TODO()); err != nil {
-		log.Error(err, "Failed to sync release")
-		status.SetCondition(types.HelmAppCondition{
-			Type:    types.ConditionIrreconcilable,
-			Status:  types.StatusTrue,
-			Reason:  types.ReasonReconcileError,
-			Message: err.Error(),
-		})
-		_ = r.updateResourceStatus(o, status)
-		return reconcile.Result{}, err
-	}
-	status.RemoveCondition(types.ConditionIrreconcilable)
-
-	if deleted {
-		if !contains(pendingFinalizers, finalizer) {
+	if o.GetDeletionTimestamp() != nil {
+		if !contains(o.GetFinalizers(), finalizer) {
 			log.Info("Resource is terminated, skipping reconciliation")
 			return reconcile.Result{}, nil
 		}
@@ -163,13 +135,7 @@ func (r HelmOperatorReconciler) Reconcile(request reconcile.Request) (reconcile.
 			return reconcile.Result{}, err
 		}
 
-		finalizers := []string{}
-		for _, pendingFinalizer := range pendingFinalizers {
-			if pendingFinalizer != finalizer {
-				finalizers = append(finalizers, pendingFinalizer)
-			}
-		}
-		o.SetFinalizers(finalizers)
+		controllerutil.RemoveFinalizer(o, finalizer)
 		if err := r.updateResource(o); err != nil {
 			log.Info("Failed to remove CR uninstall finalizer")
 			return reconcile.Result{}, err
@@ -187,6 +153,24 @@ func (r HelmOperatorReconciler) Reconcile(request reconcile.Request) (reconcile.
 		return reconcile.Result{}, nil
 	}
 
+	status.SetCondition(types.HelmAppCondition{
+		Type:   types.ConditionInitialized,
+		Status: types.StatusTrue,
+	})
+
+	if err := manager.Sync(context.TODO()); err != nil {
+		log.Error(err, "Failed to sync release")
+		status.SetCondition(types.HelmAppCondition{
+			Type:    types.ConditionIrreconcilable,
+			Status:  types.StatusTrue,
+			Reason:  types.ReasonReconcileError,
+			Message: err.Error(),
+		})
+		_ = r.updateResourceStatus(o, status)
+		return reconcile.Result{}, err
+	}
+	status.RemoveCondition(types.ConditionIrreconcilable)
+
 	if !manager.IsInstalled() {
 		for k, v := range r.OverrideValues {
 			r.EventRecorder.Eventf(o, "Warning", "OverrideValuesInUse",
@@ -206,6 +190,13 @@ func (r HelmOperatorReconciler) Reconcile(request reconcile.Request) (reconcile.
 		}
 		status.RemoveCondition(types.ConditionReleaseFailed)
 
+		log.V(1).Info("Adding finalizer", "finalizer", finalizer)
+		controllerutil.AddFinalizer(o, finalizer)
+		if err := r.updateResource(o); err != nil {
+			log.Info("Failed to add CR uninstall finalizer")
+			return reconcile.Result{}, err
+		}
+
 		if r.releaseHook != nil {
 			if err := r.releaseHook(installedRelease); err != nil {
 				log.Error(err, "Failed to run release hook")
@@ -236,6 +227,15 @@ func (r HelmOperatorReconciler) Reconcile(request reconcile.Request) (reconcile.
 		return reconcile.Result{RequeueAfter: r.ReconcilePeriod}, err
 	}
 
+	if !contains(o.GetFinalizers(), finalizer) {
+		log.V(1).Info("Adding finalizer", "finalizer", finalizer)
+		controllerutil.AddFinalizer(o, finalizer)
+		if err := r.updateResource(o); err != nil {
+			log.Info("Failed to add CR uninstall finalizer")
+			return reconcile.Result{}, err
+		}
+	}
+
 	if manager.IsUpdateRequired() {
 		for k, v := range r.OverrideValues {
 			r.EventRecorder.Eventf(o, "Warning", "OverrideValuesInUse",
@@ -343,12 +343,16 @@ func hasHelmUpgradeForceAnnotation(o *unstructured.Unstructured) bool {
 }
 
 func (r HelmOperatorReconciler) updateResource(o runtime.Object) error {
-	return r.Client.Update(context.TODO(), o)
+	return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
+		return r.Client.Update(context.TODO(), o)
+	})
 }
 
 func (r HelmOperatorReconciler) updateResourceStatus(o *unstructured.Unstructured, status *types.HelmAppStatus) error {
-	o.Object["status"] = status
-	return r.Client.Status().Update(context.TODO(), o)
+	return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
+		o.Object["status"] = status
+		return r.Client.Status().Update(context.TODO(), o)
+	})
 }
 
 func (r HelmOperatorReconciler) waitForDeletion(o runtime.Object) error {