add MariaDBAccount finalizer to Secret (and remove on delete)

in mariadbdatabase_funcs, the EnsureMariaDBAccount function
called by external controllers adds a finalizer for that calling
controller to the Secret referenced by the MariaDBAccount.  This
seems a little off, since the Secret is most immediately needed
by the MariaDBAccount CR itself, and the controller refers to that
MariaDBAccount CR also.   It seems more appropriate that
MariaDBAccount itself should maintain its own finalizer on that
Secret, so this logic is added there.

The change here causes the API function EnsureMariaDBAccount
to add a finalizer to the secret that is local to the mariadbaccount,
rather than the helper passed for the calling controller.
Existing "remove finalizer" calls which look for the calling
controller's finalizer tag in the secret are maintained however
to assist with backwards compatibility.

This comes up now because we are seeking to add a new class of
system-level MariaDBAccount that
is used only by the Galera controller itself, but also that these
accounts (really all accounts, but mainly the system ones) will
support in-place password changes by updating the name of the Secret
to be used, implying the old one is no longer needed once the change
takes place; it therefore is most appropriate that MariaDBAccount
maintain its own finalizers on these secrets.

Author: zzzeek

api/v1beta1/mariadbdatabase_funcs.go

@@ -449,6 +449,9 @@ func (d *Database) DeleteFinalizer(
 	h *helper.Helper,
 ) error {
 
+	// LEGACY: remove finalizer from the secret in terms of the caller.
+	// we now don't add the caller's finalizer to the secret, we only add
+	// the mariadbaccount finalizer.
 	if d.secretObj != nil && controllerutil.RemoveFinalizer(d.secretObj, h.GetFinalizer()) {
 		err := h.GetClient().Update(ctx, d.secretObj)
 		if err != nil && !k8s_errors.IsNotFound(err) {
@@ -548,6 +551,9 @@ func DeleteDatabaseAndAccountFinalizers(
 				return err
 			}
 
+			// LEGACY: remove finalizer from the secret in terms of the caller.
+			// we now don't add the caller's finalizer to the secret, we only add
+			// the mariadbaccount finalizer.
 			if err == nil && controllerutil.RemoveFinalizer(dbSecret, h.GetFinalizer()) {
 				err := h.GetClient().Update(ctx, dbSecret)
 				if err != nil && !k8s_errors.IsNotFound(err) {
@@ -624,6 +630,9 @@ func DeleteUnusedMariaDBAccountFinalizers(
 				return err
 			}
 
+			// LEGACY: remove finalizer from the secret in terms of the caller.
+			// we now don't add the caller's finalizer to the secret, we only add
+			// the mariadbaccount finalizer.
 			if dbSecret != nil && controllerutil.RemoveFinalizer(dbSecret, h.GetFinalizer()) {
 				err := h.GetClient().Update(ctx, dbSecret)
 				if err != nil && !k8s_errors.IsNotFound(err) {
@@ -708,7 +717,6 @@ func createOrPatchAccountAndSecret(
 			// GetDatabaseByNameAndAccount to locate the Database which is how
 			// they remove finalizers.  this will return not found if secret
 			// is not present, so finalizer will keep it around
-			controllerutil.AddFinalizer(accountSecret, h.GetFinalizer())
 
 			return nil
 		})

controllers/mariadbaccount_controller.go

@@ -27,12 +27,12 @@ import (
 	helper "github.com/openstack-k8s-operators/lib-common/modules/common/helper"
 	job "github.com/openstack-k8s-operators/lib-common/modules/common/job"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/secret"
+	util "github.com/openstack-k8s-operators/lib-common/modules/common/util"
 	databasev1beta1 "github.com/openstack-k8s-operators/mariadb-operator/api/v1beta1"
 	mariadb "github.com/openstack-k8s-operators/mariadb-operator/pkg/mariadb"
 	k8s_errors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -168,28 +168,10 @@ func (r *MariaDBAccountReconciler) reconcileCreate(
 
 	// account create
 
-	// ensure secret is present before running a job
-	_, secretResult, err := secret.VerifySecret(
-		ctx,
-		types.NamespacedName{Name: instance.Spec.Secret, Namespace: instance.Namespace},
-		[]string{databasev1beta1.DatabasePasswordSelector},
-		r.Client,
-		time.Duration(30)*time.Second,
-	)
-	if (err != nil || secretResult != ctrl.Result{}) {
-		// Since the account secret should have been manually created by the user and referenced in the spec,
-		// we treat this as a warning because it means that the service will not be able to start.
-		instance.Status.Conditions.Set(condition.FalseCondition(
-			databasev1beta1.MariaDBAccountReadyCondition,
-			secret.ReasonSecretMissing,
-			condition.SeverityWarning,
-			databasev1beta1.MariaDBAccountSecretNotReadyMessage, err))
-
-		log.Info(fmt.Sprintf(
-			"MariaDBAccount '%s' didn't find Secret '%s'; requeueing",
-			instance.Name, instance.Spec.Secret))
-
-		return secretResult, client.IgnoreNotFound(err)
+	// ensure secret is present, add a finalizer for mariadbaccount
+	result, err = r.ensureAccountSecret(ctx, log, helper, instance)
+	if (result != ctrl.Result{} || err != nil) {
+		return result, err
 	}
 
 	log.Info(fmt.Sprintf("Running account create '%s' MariaDBDatabase '%s'",
@@ -302,9 +284,8 @@ func (r *MariaDBAccountReconciler) reconcileDelete(
 		}
 
 		// remove local finalizer
-		controllerutil.RemoveFinalizer(instance, helper.GetFinalizer())
-
-		return ctrl.Result{}, nil
+		err = r.removeAccountAndSecretFinalizer(ctx, helper, instance)
+		return ctrl.Result{}, err
 	} else if dbGalera == nil {
 		return result, err
 	}
@@ -351,9 +332,9 @@ func (r *MariaDBAccountReconciler) reconcileDelete(
 	}
 
 	// then remove finalizer from our own instance
-	controllerutil.RemoveFinalizer(instance, helper.GetFinalizer())
+	err = r.removeAccountAndSecretFinalizer(ctx, helper, instance)
 
-	return ctrl.Result{}, nil
+	return ctrl.Result{}, err
 }
 
 // getMariaDBDatabaseForCreate - waits for a MariaDBDatabase to be available in preparation
@@ -446,8 +427,9 @@ func (r *MariaDBAccountReconciler) getMariaDBDatabaseForDelete(ctx context.Conte
 			instance.Name,
 		))
 
-		controllerutil.RemoveFinalizer(instance, helper.GetFinalizer())
-		return nil, ctrl.Result{}, nil
+		// remove local finalizer
+		err := r.removeAccountAndSecretFinalizer(ctx, helper, instance)
+		return nil, ctrl.Result{}, err
 	}
 
 	// locate the MariaDBDatabase object itself
@@ -461,8 +443,9 @@ func (r *MariaDBAccountReconciler) getMariaDBDatabaseForDelete(ctx context.Conte
 				"MariaDBAccount '%s' Didn't find MariaDBDatabase '%s'; no account delete needed",
 				instance.Name, mariadbDatabaseName))
 
-			controllerutil.RemoveFinalizer(instance, helper.GetFinalizer())
-			return nil, ctrl.Result{}, nil
+			// remove local finalizer
+			err = r.removeAccountAndSecretFinalizer(ctx, helper, instance)
+			return nil, ctrl.Result{}, err
 		} else {
 			// unhandled error; exit without change
 			log.Error(err, "unhandled error retrieving MariaDBDatabase instance")
@@ -492,8 +475,9 @@ func (r *MariaDBAccountReconciler) getMariaDBDatabaseForDelete(ctx context.Conte
 			}
 		}
 
-		controllerutil.RemoveFinalizer(instance, helper.GetFinalizer())
-		return nil, ctrl.Result{}, nil
+		// remove local finalizer
+		err = r.removeAccountAndSecretFinalizer(ctx, helper, instance)
+		return nil, ctrl.Result{}, err
 	}
 
 	// return MariaDBDatabase where account delete flow will then continue
@@ -572,3 +556,81 @@ func (r *MariaDBAccountReconciler) getMariaDBDatabaseObject(ctx context.Context,
 	return mariaDBDatabase, nil
 
 }
+
+// ensureAccountSecret - ensures the Secret exists, is valid, adds a finalizer.
+// includes requeue for secret does not exist
+func (r *MariaDBAccountReconciler) ensureAccountSecret(
+	ctx context.Context,
+	log logr.Logger,
+	h *helper.Helper,
+	instance *databasev1beta1.MariaDBAccount,
+) (ctrl.Result, error) {
+
+	secretName := instance.Spec.Secret
+	secretNamespace := instance.Namespace
+	secretObj, _, err := secret.GetSecret(ctx, h, secretName, secretNamespace)
+	if err != nil {
+		// Since the account secret should have been manually created by the user and referenced in the spec,
+		// we treat this as a warning because it means that the service will not be able to start.
+		if k8s_errors.IsNotFound(err) {
+			instance.Status.Conditions.Set(condition.FalseCondition(
+				databasev1beta1.MariaDBAccountReadyCondition,
+				secret.ReasonSecretMissing,
+				condition.SeverityWarning,
+				databasev1beta1.MariaDBAccountSecretNotReadyMessage, err))
+
+			log.Info(fmt.Sprintf(
+				"MariaDBAccount '%s' didn't find Secret '%s'; requeueing",
+				instance.Name, instance.Spec.Secret))
+
+			return ctrl.Result{RequeueAfter: time.Duration(30) * time.Second}, nil
+
+		} else {
+			return ctrl.Result{}, err
+		}
+	}
+
+	var expectedFields = []string{databasev1beta1.DatabasePasswordSelector}
+
+	// collect the secret values the caller expects to exist
+	for _, field := range expectedFields {
+		_, ok := secretObj.Data[field]
+		if !ok {
+			err := fmt.Errorf("%w: field %s not found in Secret %s", util.ErrFieldNotFound, field, secretName)
+			return ctrl.Result{}, err
+		}
+	}
+	if controllerutil.AddFinalizer(secretObj, h.GetFinalizer()) {
+		err = r.Update(ctx, secretObj)
+		if err != nil {
+			return ctrl.Result{}, err
+		}
+	}
+
+	return ctrl.Result{}, err
+}
+
+// removeAccountAndSecretFinalizer - removes finalizer from mariadbaccount as well
+// as current primary secret
+func (r *MariaDBAccountReconciler) removeAccountAndSecretFinalizer(ctx context.Context,
+	helper *helper.Helper, instance *databasev1beta1.MariaDBAccount) error {
+
+	accountSecret, _, err := secret.GetSecret(ctx, helper, instance.Spec.Secret, instance.Namespace)
+
+	if err == nil {
+		if controllerutil.RemoveFinalizer(accountSecret, helper.GetFinalizer()) {
+			err = r.Update(ctx, accountSecret)
+			if err != nil {
+				return err
+			}
+		}
+	} else if !k8s_errors.IsNotFound(err) {
+		return err
+	}
+
+	// will take effect when reconcile ends
+	controllerutil.RemoveFinalizer(instance, helper.GetFinalizer())
+
+	return nil
+
+}

tests/chainsaw/tests/account/chainsaw-test.yaml

@@ -25,6 +25,8 @@ spec:
 
   - name: create account without secret
     description: account CR has to wait for a secret to create account in the database
+    # we will delete the account manually
+    skipDelete: true
     try:
     - apply:
         file: account.yaml
@@ -33,8 +35,20 @@ spec:
 
   - name: add secret and finish account creation
     description: make sure the account is created in the database
+    # we will delete the secret manually
+    skipDelete: true
     try:
     - apply:
         file: account-secret.yaml
     - assert:
         file: account-assert.yaml
+    finally:
+    - delete:
+        # delete account first, otherwise the secret can't be deleted b.c.
+        # it has a finalizer from the account
+        # perhaps I shouldn't have allowed MariaDBAccount to be created without
+        # a secret, but here we are...
+        file: account.yaml
+    - delete:
+        # then delete the secret
+        file: account-secret.yaml

tests/kuttl/tests/account_create/04-assert.yaml

@@ -23,3 +23,14 @@ status:
     reason: Ready
     status: "True"
     type: MariaDBServerReady
+---
+apiVersion: v1
+data:
+  DatabasePassword: ZGJzZWNyZXQx
+kind: Secret
+metadata:
+  name: some-db-secret
+  # ensure finalizer was added
+  finalizers:
+  - openstack.org/mariadbaccount
+type: Opaque