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 existing API logic that adds the calling controllers finalizer
is not changed here but maybe at some point we could simplfy this.

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.  While the plan is
that Galera controller will probably also use EnsureMariaDBAccount
and thus add a Galera object finalizer, we may seek to modify this
for system accounts so that the extra non-mariadbaccount finalizers
aren't added in this case.

Author: zzzeek

api/v1beta1/mariadbdatabase_funcs.go

@@ -449,6 +449,8 @@ func (d *Database) DeleteFinalizer(
 	h *helper.Helper,
 ) error {
 
+	// TODO: if we rely only on MariaDBAccount local secret finalizer, this would
+	// not be needed
 	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 +550,8 @@ func DeleteDatabaseAndAccountFinalizers(
 				return err
 			}
 
+			// TODO: if we rely only on MariaDBAccount local secret finalizer, this would
+			// not be needed
 			if err == nil && controllerutil.RemoveFinalizer(dbSecret, h.GetFinalizer()) {
 				err := h.GetClient().Update(ctx, dbSecret)
 				if err != nil && !k8s_errors.IsNotFound(err) {
@@ -624,6 +628,8 @@ func DeleteUnusedMariaDBAccountFinalizers(
 				return err
 			}
 
+			// TODO: if we rely only on MariaDBAccount local secret finalizer, this would
+			// not be needed
 			if dbSecret != nil && controllerutil.RemoveFinalizer(dbSecret, h.GetFinalizer()) {
 				err := h.GetClient().Update(ctx, dbSecret)
 				if err != nil && !k8s_errors.IsNotFound(err) {
@@ -708,6 +714,9 @@ 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
+
+			// TODO: since MariaDBAccount adds a finalizer to Secret, we may
+			// want to look at removing this
 			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"
@@ -163,27 +163,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{}) {
-
-		instance.Status.Conditions.Set(condition.FalseCondition(
-			databasev1beta1.MariaDBAccountReadyCondition,
-			secret.ReasonSecretMissing,
-			condition.SeverityInfo,
-			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
+	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'",
@@ -296,9 +279,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
 	}
@@ -345,9 +327,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
@@ -440,8 +422,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
@@ -455,8 +438,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")
@@ -486,8 +470,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
@@ -566,3 +551,77 @@ 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 {
+		if k8s_errors.IsNotFound(err) {
+			instance.Status.Conditions.Set(condition.FalseCondition(
+				databasev1beta1.MariaDBAccountReadyCondition,
+				secret.ReasonSecretMissing,
+				condition.SeverityInfo,
+				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 {
+	controllerutil.RemoveFinalizer(instance, helper.GetFinalizer())
+
+	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
+	}
+
+	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