*: format finalizers correctly

Eric Stroczynski · web-flow · commit ee7e682a2025 · 2021-02-03T17:10:06.000-08:00
This commit modifies the suggested format for finalizers
from &lt;finalizer-name&gt;.&lt;qualified-group&gt; to &lt;qualified-group&gt;/&lt;finalizer-name&gt;,
which is the recommended format in k8s docs. This change is
not breaking because technically any name format is allowed

Signed-off-by: Eric Stroczynski &lt;ericstroczynski@gmail.com&gt;
diff --git a/changelog/fragments/change-finalizer-format.yaml b/changelog/fragments/change-finalizer-format.yaml
@@ -0,0 +1,12 @@
+entries:
+  - description: >
+      Changed the suggested finalizer format to `<qualified-group>/<finalizer-name>`
+    kind: change
+    migration:
+      header: Change your operator's finalizer names
+      body: >
+        The finalizer name format suggested by [Kubernetes docs](https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#finalizers)
+        is `<qualified-group>/<finalizer-name>`, while the format previously documented by Operator SDK docs was
+        `<finalizer-name>.<qualified-group>`. If your operator uses any finalizers with names matching the incorrect format,
+        change them to match the official format.
+        For example, `finalizer.cache.example.com` should be changed to `cache.example.com/finalizer`.
diff --git a/hack/generate/samples/internal/ansible/constants.go b/hack/generate/samples/internal/ansible/constants.go
@@ -355,7 +355,7 @@ const taskToDeleteConfigMap = `- name: delete configmap for test
 
 const memcachedWatchCustomizations = `playbook: playbooks/memcached.yml
   finalizer:
-    name: finalizer.cache.example.com
+    name: cache.example.com/finalizer
     role: memfin
   blacklist:
     - group: ""
diff --git a/internal/ansible/controller/reconcile.go b/internal/ansible/controller/reconcile.go
@@ -32,6 +32,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
@@ -100,22 +101,21 @@ func (r *AnsibleOperatorReconciler) Reconcile(ctx context.Context, request recon
 
 	deleted := u.GetDeletionTimestamp() != nil
 	finalizer, finalizerExists := r.Runner.GetFinalizer()
-	pendingFinalizers := u.GetFinalizers()
-	// If the resource is being deleted we don't want to add the finalizer again
-	if finalizerExists && !deleted && !contains(pendingFinalizers, finalizer) {
-		logger.V(1).Info("Adding finalizer to resource", "Finalizer", finalizer)
-		finalizers := append(pendingFinalizers, finalizer)
-		u.SetFinalizers(finalizers)
-		err := r.Client.Update(ctx, u)
-		if err != nil {
-			logger.Error(err, "Unable to update cr with finalizer")
-			return reconcileResult, err
+	if !controllerutil.ContainsFinalizer(u, finalizer) {
+		if deleted {
+			// If the resource is being deleted we don't want to add the finalizer again
+			logger.Info("Resource is terminated, skipping reconciliation")
+			return reconcile.Result{}, nil
+		} else if finalizerExists {
+			logger.V(1).Info("Adding finalizer to resource", "Finalizer", finalizer)
+			controllerutil.AddFinalizer(u, finalizer)
+			err := r.Client.Update(ctx, u)
+			if err != nil {
+				logger.Error(err, "Unable to update cr with finalizer")
+				return reconcileResult, err
+			}
 		}
 	}
-	if !contains(pendingFinalizers, finalizer) && deleted {
-		logger.Info("Resource is terminated, skipping reconciliation")
-		return reconcile.Result{}, nil
-	}
 
 	spec := u.Object["spec"]
 	_, ok := spec.(map[string]interface{})
@@ -240,22 +240,14 @@ func (r *AnsibleOperatorReconciler) Reconcile(ctx context.Context, request recon
 		return reconcile.Result{}, err
 	}
 
-	// try to get the updated finalizers
-	pendingFinalizers = u.GetFinalizers()
-
 	// We only want to update the CustomResource once, so we'll track changes
 	// and do it at the end
 	runSuccessful := len(failureMessages) == 0
 
 	// The finalizer has run successfully, time to remove it
+	deleted = u.GetDeletionTimestamp() != nil
 	if deleted && finalizerExists && runSuccessful {
-		finalizers := []string{}
-		for _, pendingFinalizer := range pendingFinalizers {
-			if pendingFinalizer != finalizer {
-				finalizers = append(finalizers, pendingFinalizer)
-			}
-		}
-		u.SetFinalizers(finalizers)
+		controllerutil.RemoveFinalizer(u, finalizer)
 		err := r.Client.Update(ctx, u)
 		if err != nil {
 			logger.Error(err, "Failed to remove finalizer")
@@ -419,15 +411,6 @@ func (r *AnsibleOperatorReconciler) markDone(ctx context.Context, nn types.Names
 	return r.Client.Status().Update(ctx, u)
 }
 
-func contains(l []string, s string) bool {
-	for _, elem := range l {
-		if elem == s {
-			return true
-		}
-	}
-	return false
-}
-
 // getStatus returns u's "status" block as a status.Status.
 func getStatus(u *unstructured.Unstructured) ansiblestatus.Status {
 	statusInterface := u.Object["status"]
diff --git a/internal/ansible/controller/reconcile_test.go b/internal/ansible/controller/reconcile_test.go
@@ -258,7 +258,7 @@ func TestReconcile(t *testing.T) {
 						Created: eventapi.EventTime{Time: eventTime},
 					},
 				},
-				Finalizer: "testing.io",
+				Finalizer: "testing.io/finalizer",
 			},
 			Client: fakeclient.NewClientBuilder().WithObjects(&unstructured.Unstructured{
 				Object: map[string]interface{}{
@@ -292,7 +292,7 @@ func TestReconcile(t *testing.T) {
 							controller.ReconcilePeriodAnnotation: "3s",
 						},
 						"finalizers": []interface{}{
-							"testing.io",
+							"testing.io/finalizer",
 						},
 					},
 					"apiVersion": "operator-sdk/v1beta1",
@@ -329,7 +329,7 @@ func TestReconcile(t *testing.T) {
 						Created: eventapi.EventTime{Time: eventTime},
 					},
 				},
-				Finalizer: "testing.io",
+				Finalizer: "testing.io/finalizer",
 			},
 			Client: fakeclient.NewClientBuilder().WithObjects(&unstructured.Unstructured{
 				Object: map[string]interface{}{
@@ -366,15 +366,15 @@ func TestReconcile(t *testing.T) {
 						Created: eventapi.EventTime{Time: eventTime},
 					},
 				},
-				Finalizer: "testing.io",
+				Finalizer: "testing.io/finalizer",
 			},
 			Client: fakeclient.NewClientBuilder().WithObjects(&unstructured.Unstructured{
 				Object: map[string]interface{}{
 					"metadata": map[string]interface{}{
 						"name":      "reconcile",
 						"namespace": "default",
 						"finalizers": []interface{}{
-							"testing.io",
+							"testing.io/finalizer",
 						},
 						"deletionTimestamp": eventTime.Format(time.RFC3339),
 					},
diff --git a/internal/ansible/runner/runner_test.go b/internal/ansible/runner/runner_test.go
@@ -93,7 +93,7 @@ func TestNew(t *testing.T) {
 			},
 			playbook: validPlaybook,
 			finalizer: &watches.Finalizer{
-				Name:     "example.finalizer.com",
+				Name:     "operator.example.com/finalizer",
 				Playbook: validPlaybook,
 			},
 		},
@@ -106,7 +106,7 @@ func TestNew(t *testing.T) {
 			},
 			role: validRole,
 			finalizer: &watches.Finalizer{
-				Name: "example.finalizer.com",
+				Name: "operator.example.com/finalizer",
 				Role: validRole,
 			},
 		},
@@ -119,7 +119,7 @@ func TestNew(t *testing.T) {
 			},
 			playbook: validPlaybook,
 			finalizer: &watches.Finalizer{
-				Name: "example.finalizer.com",
+				Name: "operator.example.com/finalizer",
 				Vars: map[string]interface{}{
 					"state": "absent",
 				},
@@ -137,7 +137,7 @@ func TestNew(t *testing.T) {
 				"type": "this",
 			},
 			finalizer: &watches.Finalizer{
-				Name: "example.finalizer.com",
+				Name: "operator.example.com/finalizer",
 				Vars: map[string]interface{}{
 					"state": "absent",
 				},
diff --git a/internal/ansible/watches/testdata/duplicate_gvk.yaml b/internal/ansible/watches/testdata/duplicate_gvk.yaml
@@ -4,14 +4,14 @@
   kind: Database
   playbook: playbook.yaml
   finalizer:
-    name: finalizer.app.example.com
+    name: app.example.com/finalizer
     vars:
       sentinel: finalizer_running
 - version: v1alpha1
   group: app.example.com
   kind: Database
   playbook: playbook.yaml
   finalizer:
-    name: finalizer.app.example.com
+    name: app.example.com/finalizer
     vars:
       sentinel: finalizer_running
diff --git a/internal/ansible/watches/testdata/invalid.yaml b/internal/ansible/watches/testdata/invalid.yaml
@@ -4,6 +4,6 @@ version: v1alpha1
   kind: Database
   playbook: playbook.yaml
   finalizer:
-    name: finalizer.app.example.com
+    name: app.example.com/finalizer
     vars:
       sentinel: finalizer_running
diff --git a/internal/ansible/watches/testdata/invalid_finalizer_no_vars.yaml b/internal/ansible/watches/testdata/invalid_finalizer_no_vars.yaml
@@ -4,4 +4,4 @@
   kind: Database
   playbook: playbook.yaml
   finalizer:
-    name: foo.app.example.com
+    name: foo.app.example.com/finalizer
diff --git a/internal/ansible/watches/testdata/invalid_finalizer_playbook_path.yaml b/internal/ansible/watches/testdata/invalid_finalizer_playbook_path.yaml
@@ -4,7 +4,7 @@
   kind: Database
   playbook: playbook.yaml
   finalizer:
-    name: finalizer.app.example.com
+    name: app.example.com/finalizer
     playbook: playbook.yaml
     vars:
       sentinel: finalizer_running
diff --git a/internal/ansible/watches/testdata/invalid_finalizer_role_path.yaml b/internal/ansible/watches/testdata/invalid_finalizer_role_path.yaml
@@ -4,7 +4,7 @@
   kind: Database
   playbook: playbook.yaml
   finalizer:
-    name: finalizer.app.example.com
+    name: app.example.com/finalizer
     role: ansible/role
     vars:
       sentinel: finalizer_running
diff --git a/internal/ansible/watches/testdata/invalid_playbook_path.yaml b/internal/ansible/watches/testdata/invalid_playbook_path.yaml
@@ -4,6 +4,6 @@
   kind: Database
   playbook: invalid/playbook.yaml
   finalizer:
-    name: finalizer.app.example.com
+    name: app.example.com/finalizer
     vars:
       sentinel: finalizer_running
diff --git a/internal/ansible/watches/testdata/invalid_role_path.yaml b/internal/ansible/watches/testdata/invalid_role_path.yaml
@@ -4,6 +4,6 @@
   kind: Database
   role: opt/ansible/playbook.yaml
   finalizer:
-    name: finalizer.app.example.com
+    name: app.example.com/finalizer
     vars:
       sentinel: finalizer_running
diff --git a/internal/ansible/watches/testdata/valid.yaml.tmpl b/internal/ansible/watches/testdata/valid.yaml.tmpl
@@ -9,7 +9,7 @@
   kind: Playbook
   playbook: {{ .ValidPlaybook }}
   finalizer:
-    name: finalizer.app.example.com
+    name: app.example.com/finalizer
     role: {{ .ValidRole }}
     vars:
       sentinel: finalizer_running
@@ -43,7 +43,7 @@
   kind: Role
   role: {{ .ValidRole }}
   finalizer:
-    name: finalizer.app.example.com
+    name: app.example.com/finalizer
     playbook: {{ .ValidPlaybook }}
     vars:
       sentinel: finalizer_running
@@ -52,7 +52,7 @@
   kind: FinalizerRole
   role: {{ .ValidRole }}
   finalizer:
-    name: finalizer.app.example.com
+    name: app.example.com/finalizer
     vars:
       sentinel: finalizer_running
 - version: v1alpha1
diff --git a/internal/ansible/watches/watches_test.go b/internal/ansible/watches/watches_test.go
@@ -160,7 +160,7 @@ func TestLoad(t *testing.T) { //nolint:gocyclo
 			SnakeCaseParameters:         false,
 			WatchClusterScopedResources: false,
 			Finalizer: &Finalizer{
-				Name: "finalizer.app.example.com",
+				Name: "app.example.com/finalizer",
 				Role: validTemplate.ValidRole,
 				Vars: map[string]interface{}{"sentinel": "finalizer_running"},
 			},
@@ -223,7 +223,7 @@ func TestLoad(t *testing.T) { //nolint:gocyclo
 			Role:         validTemplate.ValidRole,
 			ManageStatus: true,
 			Finalizer: &Finalizer{
-				Name:     "finalizer.app.example.com",
+				Name:     "app.example.com/finalizer",
 				Playbook: validTemplate.ValidPlaybook,
 				Vars:     map[string]interface{}{"sentinel": "finalizer_running"},
 			},
@@ -237,7 +237,7 @@ func TestLoad(t *testing.T) { //nolint:gocyclo
 			Role:         validTemplate.ValidRole,
 			ManageStatus: true,
 			Finalizer: &Finalizer{
-				Name: "finalizer.app.example.com",
+				Name: "app.example.com/finalizer",
 				Vars: map[string]interface{}{"sentinel": "finalizer_running"},
 			},
 		},
diff --git a/internal/helm/controller/reconcile.go b/internal/helm/controller/reconcile.go
@@ -56,7 +56,10 @@ type HelmOperatorReconciler struct {
 }
 
 const (
-	finalizer = "uninstall-helm-release"
+	// uninstallFinalizer is added to CRs so they are cleaned up after uninstalling a release.
+	uninstallFinalizer = "helm.sdk.operatorframework.io/uninstall-release"
+	// Deprecated: use uninstallFinalizer. This will be removed in operator-sdk v2.0.0.
+	uninstallFinalizerLegacy = "uninstall-helm-release"
 )
 
 // Reconcile reconciles the requested resource by installing, updating, or
@@ -96,7 +99,9 @@ func (r HelmOperatorReconciler) Reconcile(ctx context.Context, request reconcile
 	log = log.WithValues("release", manager.ReleaseName())
 
 	if o.GetDeletionTimestamp() != nil {
-		if !contains(o.GetFinalizers(), finalizer) {
+		if !(controllerutil.ContainsFinalizer(o, uninstallFinalizer) ||
+			controllerutil.ContainsFinalizer(o, uninstallFinalizerLegacy)) {
+
 			log.Info("Resource is terminated, skipping reconciliation")
 			return reconcile.Result{}, nil
 		}
@@ -136,7 +141,8 @@ func (r HelmOperatorReconciler) Reconcile(ctx context.Context, request reconcile
 			return reconcile.Result{}, err
 		}
 
-		controllerutil.RemoveFinalizer(o, finalizer)
+		controllerutil.RemoveFinalizer(o, uninstallFinalizer)
+		controllerutil.RemoveFinalizer(o, uninstallFinalizerLegacy)
 		if err := r.updateResource(ctx, o); err != nil {
 			log.Info("Failed to remove CR uninstall finalizer")
 			return reconcile.Result{}, err
@@ -195,8 +201,8 @@ func (r HelmOperatorReconciler) Reconcile(ctx context.Context, request reconcile
 		}
 		status.RemoveCondition(types.ConditionReleaseFailed)
 
-		log.V(1).Info("Adding finalizer", "finalizer", finalizer)
-		controllerutil.AddFinalizer(o, finalizer)
+		log.V(1).Info("Adding finalizer", "finalizer", uninstallFinalizer)
+		controllerutil.AddFinalizer(o, uninstallFinalizer)
 		if err := r.updateResource(ctx, o); err != nil {
 			log.Info("Failed to add CR uninstall finalizer")
 			return reconcile.Result{}, err
@@ -232,9 +238,11 @@ func (r HelmOperatorReconciler) Reconcile(ctx context.Context, 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 !(controllerutil.ContainsFinalizer(o, uninstallFinalizer) ||
+		controllerutil.ContainsFinalizer(o, uninstallFinalizerLegacy)) {
+
+		log.V(1).Info("Adding finalizer", "finalizer", uninstallFinalizer)
+		controllerutil.AddFinalizer(o, uninstallFinalizer)
 		if err := r.updateResource(ctx, o); err != nil {
 			log.Info("Failed to add CR uninstall finalizer")
 			return reconcile.Result{}, err
@@ -394,12 +402,3 @@ func (r HelmOperatorReconciler) waitForDeletion(ctx context.Context, o client.Ob
 		return false, nil
 	}, tctx.Done())
 }
-
-func contains(l []string, s string) bool {
-	for _, elem := range l {
-		if elem == s {
-			return true
-		}
-	}
-	return false
-}
diff --git a/test/e2e-ansible/e2e_ansible_suite_test.go b/test/e2e-ansible/e2e_ansible_suite_test.go
@@ -190,7 +190,7 @@ const taskToDeleteConfigMap = `- name: delete configmap for test
 
 const memcachedWatchCustomizations = `playbook: playbooks/memcached.yml
   finalizer:
-    name: finalizer.cache.example.com
+    name: cache.example.com/finalizer
     role: memfin
   blacklist:
     - group: ""
diff --git a/website/content/en/docs/building-operators/ansible/reference/finalizers.md b/website/content/en/docs/building-operators/ansible/reference/finalizers.md
diff --git a/website/content/en/docs/building-operators/ansible/reference/watches.md b/website/content/en/docs/building-operators/ansible/reference/watches.md
diff --git a/website/content/en/docs/building-operators/golang/advanced-topics.md b/website/content/en/docs/building-operators/golang/advanced-topics.md
