Merge pull request #346 from zzzeek/OSPRH-14916-pr5

openshift-merge-bot[bot] · web-flow · commit 84780d6c3078 · 2025-11-21T10:28:37.000Z
support in-place secret changes (PR 5 of 6)
diff --git a/api/bases/mariadb.openstack.org_mariadbaccounts.yaml b/api/bases/mariadb.openstack.org_mariadbaccounts.yaml
@@ -114,6 +114,14 @@ spec:
                   - type
                   type: object
                 type: array
+              currentSecret:
+                description: |-
+                  the Secret that's currently in use for the account.
+                  keeping a handle to this secret allows us to remove its finalizer
+                  when it's replaced with a new one. It also is useful for storing
+                  the current "root" secret separate from a newly proposed one which is
+                  needed when changing the database root password.
+                type: string
               hash:
                 additionalProperties:
                   type: string
diff --git a/api/v1beta1/mariadbaccount_types.go b/api/v1beta1/mariadbaccount_types.go
@@ -22,8 +22,8 @@ import (
 )
 
 const (
-	// AccountCreateHash hash
-	AccountCreateHash = "accountcreate"
+	// AccountCreateOrUpdateHash hash
+	AccountCreateOrUpdateHash = "accountcreateorupdate"
 
 	// AccountDeleteHash hash
 	AccountDeleteHash = "accountdelete"
@@ -63,6 +63,13 @@ const (
 
 // MariaDBAccountStatus defines the observed state of MariaDBAccount
 type MariaDBAccountStatus struct {
+	// the Secret that's currently in use for the account.
+	// keeping a handle to this secret allows us to remove its finalizer
+	// when it's replaced with a new one. It also is useful for storing
+	// the current "root" secret separate from a newly proposed one which is
+	// needed when changing the database root password.
+	CurrentSecret string `json:"currentSecret,omitempty"`
+
 	// Deployment Conditions
 	Conditions condition.Conditions `json:"conditions,omitempty" optional:"true"`
 
diff --git a/config/crd/bases/mariadb.openstack.org_mariadbaccounts.yaml b/config/crd/bases/mariadb.openstack.org_mariadbaccounts.yaml
@@ -114,6 +114,14 @@ spec:
                   - type
                   type: object
                 type: array
+              currentSecret:
+                description: |-
+                  the Secret that's currently in use for the account.
+                  keeping a handle to this secret allows us to remove its finalizer
+                  when it's replaced with a new one. It also is useful for storing
+                  the current "root" secret separate from a newly proposed one which is
+                  needed when changing the database root password.
+                type: string
               hash:
                 additionalProperties:
                   type: string
diff --git a/controllers/mariadbaccount_controller.go b/controllers/mariadbaccount_controller.go
@@ -129,17 +129,17 @@ func (r *MariaDBAccountReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	instance.Status.Conditions.Init(&cl)
 
 	if instance.DeletionTimestamp.IsZero() || isNewInstance { //revive:disable:indent-error-flow
-		return r.reconcileCreate(ctx, log, helper, instance)
+		return r.reconcileCreateOrUpdate(ctx, log, helper, instance, isNewInstance)
 	} else {
 		return r.reconcileDelete(ctx, log, helper, instance)
 	}
 
 }
 
-// reconcileDelete - run reconcile for case where delete timestamp is zero
-func (r *MariaDBAccountReconciler) reconcileCreate(
+// reconcileCreateOrUpdate - run reconcile for case where delete timestamp is zero
+func (r *MariaDBAccountReconciler) reconcileCreateOrUpdate(
 	ctx context.Context, log logr.Logger,
-	helper *helper.Helper, instance *databasev1beta1.MariaDBAccount) (result ctrl.Result, _err error) {
+	helper *helper.Helper, instance *databasev1beta1.MariaDBAccount, isNewInstance bool) (result ctrl.Result, _err error) {
 
 	var mariadbDatabase *databasev1beta1.MariaDBDatabase
 	var err error
@@ -190,47 +190,69 @@ func (r *MariaDBAccountReconciler) reconcileCreate(
 
 	var jobDef *batchv1.Job
 
+	var createOrUpdate string
+	if isNewInstance {
+		createOrUpdate = "create"
+	} else {
+		createOrUpdate = "update"
+	}
+
 	if instance.IsUserAccount() {
-		log.Info(fmt.Sprintf("Running account create '%s' MariaDBDatabase '%s'",
+		log.Info(fmt.Sprintf("Checking %s account job '%s' MariaDBDatabase '%s'",
+			createOrUpdate,
 			instance.Name, mariadbDatabase.Spec.Name))
-		jobDef, err = mariadb.CreateDbAccountJob(
+		jobDef, err = mariadb.CreateOrUpdateDbAccountJob(
 			dbGalera, instance, mariadbDatabase.Spec.Name, dbHostname,
 			dbContainerImage, serviceAccountName, dbGalera.Spec.NodeSelector)
 		if err != nil {
 			return ctrl.Result{}, err
 		}
 	} else {
-		log.Info(fmt.Sprintf("Running system account create '%s'", instance.Name))
-		jobDef, err = mariadb.CreateDbAccountJob(
+		log.Info(fmt.Sprintf("Checking %s system account job '%s'", createOrUpdate,
+			instance.Name))
+		jobDef, err = mariadb.CreateOrUpdateDbAccountJob(
 			dbGalera, instance, "", dbHostname,
 			dbContainerImage, serviceAccountName, dbGalera.Spec.NodeSelector)
 		if err != nil {
 			return ctrl.Result{}, err
 		}
 	}
 
-	accountCreateHash := instance.Status.Hash[databasev1beta1.AccountCreateHash]
-	accountCreateJob := job.NewJob(
+	accountCreateOrUpdateHash := instance.Status.Hash[databasev1beta1.AccountCreateOrUpdateHash]
+	accountCreateOrUpdateJob := job.NewJob(
 		jobDef,
-		databasev1beta1.AccountCreateHash,
+		databasev1beta1.AccountCreateOrUpdateHash,
 		false,
 		time.Duration(5)*time.Second,
-		accountCreateHash,
+		accountCreateOrUpdateHash,
 	)
-	ctrlResult, err := accountCreateJob.DoJob(
+	ctrlResult, err := accountCreateOrUpdateJob.DoJob(
 		ctx,
 		helper,
 	)
 	if (ctrlResult != ctrl.Result{} || err != nil) {
 		return ctrlResult, err
 	}
 
-	if accountCreateJob.HasChanged() {
+	if accountCreateOrUpdateJob.HasChanged() {
+
 		if instance.Status.Hash == nil {
 			instance.Status.Hash = make(map[string]string)
 		}
-		instance.Status.Hash[databasev1beta1.AccountCreateHash] = accountCreateJob.GetHash()
-		log.Info(fmt.Sprintf("Job %s hash added - %s", jobDef.Name, instance.Status.Hash[databasev1beta1.AccountCreateHash]))
+		instance.Status.Hash[databasev1beta1.AccountCreateOrUpdateHash] = accountCreateOrUpdateJob.GetHash()
+		log.Info(fmt.Sprintf("Job %s hash added - %s", jobDef.Name, instance.Status.Hash[databasev1beta1.AccountCreateOrUpdateHash]))
+
+		// set up new Secret and remove finalizer from old secret
+		if instance.Status.CurrentSecret != instance.Spec.Secret {
+			currentSecret := instance.Status.CurrentSecret
+			err = r.removeSecretFinalizer(ctx, helper, currentSecret, instance.Namespace)
+			if err == nil {
+				instance.Status.CurrentSecret = instance.Spec.Secret
+			} else {
+				return ctrl.Result{}, err
+			}
+		}
+
 	}
 
 	// database creation finished
@@ -711,7 +733,22 @@ func (r *MariaDBAccountReconciler) ensureAccountSecret(
 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)
+	err := r.removeSecretFinalizer(
+		ctx, helper, instance.Spec.Secret, instance.Namespace,
+	)
+	if err != nil && !k8s_errors.IsNotFound(err) {
+		return err
+	}
+
+	// remove mariadbaccount finalizer which will update at end of reconcile
+	controllerutil.RemoveFinalizer(instance, helper.GetFinalizer())
+
+	return nil
+}
+
+func (r *MariaDBAccountReconciler) removeSecretFinalizer(ctx context.Context,
+	helper *helper.Helper, secretName string, namespace string) error {
+	accountSecret, _, err := secret.GetSecret(ctx, helper, secretName, namespace)
 
 	if err == nil {
 		if controllerutil.RemoveFinalizer(accountSecret, helper.GetFinalizer()) {
@@ -724,9 +761,6 @@ func (r *MariaDBAccountReconciler) removeAccountAndSecretFinalizer(ctx context.C
 		return err
 	}
 
-	// will take effect when reconcile ends
-	controllerutil.RemoveFinalizer(instance, helper.GetFinalizer())
-
 	return nil
 
 }
diff --git a/pkg/mariadb/account.go b/pkg/mariadb/account.go
@@ -11,24 +11,24 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-type accountCreateOrDeleteOptions struct {
+type accountCRUDOptions struct {
 	UserName              string
 	DatabaseName          string
 	DatabaseHostname      string
 	DatabaseAdminUsername string
 	RequireTLS            string
 }
 
-// CreateDbAccountJob creates a Kubernetes job for creating a MariaDB database account
-func CreateDbAccountJob(galera *mariadbv1.Galera, account *mariadbv1.MariaDBAccount, databaseName string, databaseHostName string, containerImage string, serviceAccountName string, nodeSelector *map[string]string) (*batchv1.Job, error) {
+// CreateOrUpdateDbAccountJob creates or updates a Kubernetes job for creating a MariaDB database account
+func CreateOrUpdateDbAccountJob(galera *mariadbv1.Galera, account *mariadbv1.MariaDBAccount, databaseName string, databaseHostName string, containerImage string, serviceAccountName string, nodeSelector *map[string]string) (*batchv1.Job, error) {
 	var tlsStatement string
 	if account.Spec.RequireTLS {
 		tlsStatement = " REQUIRE SSL"
 	} else {
 		tlsStatement = ""
 	}
 
-	opts := accountCreateOrDeleteOptions{
+	opts := accountCRUDOptions{
 		account.Spec.UserName,
 		databaseName,
 		databaseHostName,
@@ -47,7 +47,7 @@ func CreateDbAccountJob(galera *mariadbv1.Galera, account *mariadbv1.MariaDBAcco
 			// provided db name is used as metadata name where underscore is a not allowed
 			// character. Lets replace all underscores with hypen. Underscores in the db name are
 			// possible.
-			Name:      strings.ReplaceAll(account.Spec.UserName, "_", "-") + "-account-create",
+			Name:      strings.ReplaceAll(account.Spec.UserName, "_", "-") + "-account-create-update",
 			Namespace: account.Namespace,
 			Labels:    labels,
 		},
@@ -58,7 +58,7 @@ func CreateDbAccountJob(galera *mariadbv1.Galera, account *mariadbv1.MariaDBAcco
 					ServiceAccountName: serviceAccountName,
 					Containers: []corev1.Container{
 						{
-							Name:    "mariadb-account-create",
+							Name:    "mariadb-account-create-update",
 							Image:   containerImage,
 							Command: []string{"/bin/sh", "-c", dbCmd},
 							Env: []corev1.EnvVar{
@@ -93,7 +93,7 @@ func CreateDbAccountJob(galera *mariadbv1.Galera, account *mariadbv1.MariaDBAcco
 // DeleteDbAccountJob creates a Kubernetes job for deleting a MariaDB database account
 func DeleteDbAccountJob(galera *mariadbv1.Galera, account *mariadbv1.MariaDBAccount, databaseName string, databaseHostName string, containerImage string, serviceAccountName string, nodeSelector *map[string]string) (*batchv1.Job, error) {
 
-	opts := accountCreateOrDeleteOptions{account.Spec.UserName, databaseName, databaseHostName, "root", ""}
+	opts := accountCRUDOptions{account.Spec.UserName, databaseName, databaseHostName, "root", ""}
 
 	delCmd, err := util.ExecuteTemplateFile("delete_account.sh", &opts)
 	if err != nil {
diff --git a/templates/account.sh b/templates/account.sh
@@ -4,15 +4,35 @@ MYSQL_REMOTE_HOST={{.DatabaseHostname}} source /var/lib/operator-scripts/mysql_r
 
 export DatabasePassword=${DatabasePassword:?"Please specify a DatabasePassword variable."}
 
+MYSQL_CMD="mysql -h {{.DatabaseHostname}} -u {{.DatabaseAdminUsername}} -P 3306"
+
 if [ -n "{{.DatabaseName}}" ]; then
-    mysql -h {{.DatabaseHostname}} -u {{.DatabaseAdminUsername}} -P 3306 -e "GRANT ALL PRIVILEGES ON {{.DatabaseName}}.* TO '{{.UserName}}'@'localhost' IDENTIFIED BY '$DatabasePassword'{{.RequireTLS}};GRANT ALL PRIVILEGES ON {{.DatabaseName}}.* TO '{{.UserName}}'@'%' IDENTIFIED BY '$DatabasePassword'{{.RequireTLS}};"
+    GRANT_DATABASE="{{.DatabaseName}}"
 else
-    mysql -h {{.DatabaseHostname}} -u {{.DatabaseAdminUsername}} -P 3306 -e "GRANT ALL PRIVILEGES ON *.* TO '{{.UserName}}'@'localhost' IDENTIFIED BY '$DatabasePassword'{{.RequireTLS}};GRANT ALL PRIVILEGES ON *.* TO '{{.UserName}}'@'%' IDENTIFIED BY '$DatabasePassword'{{.RequireTLS}};"
+    GRANT_DATABASE="*"
 fi
 
+# going for maximum compatibility here:
+# 1. MySQL 8 no longer allows implicit create user when GRANT is used
+# 2. MariaDB has "CREATE OR REPLACE", but MySQL does not
+# 3. create user with CREATE but then do all password and TLS with ALTER to
+#    support updates
+
+$MYSQL_CMD <<EOF
+CREATE USER IF NOT EXISTS '{{.UserName}}'@'localhost';
+CREATE USER IF NOT EXISTS '{{.UserName}}'@'%';
+
+ALTER USER '{{.UserName}}'@'localhost' IDENTIFIED BY '$DatabasePassword'{{.RequireTLS}};
+ALTER USER '{{.UserName}}'@'%'  IDENTIFIED BY '$DatabasePassword'{{.RequireTLS}};
+
+GRANT ALL PRIVILEGES ON ${GRANT_DATABASE}.* TO '{{.UserName}}'@'localhost';
+GRANT ALL PRIVILEGES ON ${GRANT_DATABASE}.* TO '{{.UserName}}'@'%';
+EOF
+
+
 # search for the account.  not using SHOW CREATE USER to avoid displaying
 # password hash
-username=$(mysql -h {{.DatabaseHostname}} -u {{.DatabaseAdminUsername}} -P 3306 -NB -e "select user from mysql.user where user='{{.UserName}}' and host='localhost';" )
+username=$($MYSQL_CMD -NB -e "select user from mysql.user where user='{{.UserName}}' and host='localhost';" )
 
 if [[ ${username} != "{{.UserName}}" ]]; then
     exit 1
diff --git a/tests/chainsaw/common/system-account-assert.yaml b/tests/chainsaw/common/system-account-assert.yaml
@@ -6,6 +6,7 @@ metadata:
     dbName: openstack
   name: chainsawdb-some-system-db-account
 status:
+  currentSecret: some-system-db-secret
   conditions:
   - message: Setup complete
     reason: Ready
@@ -34,7 +35,7 @@ type: Opaque
 apiVersion: batch/v1
 kind: Job
 metadata:
-  name: systemuser-account-create
+  name: systemuser-account-create-update
 spec:
   template:
     spec: {}
diff --git a/tests/chainsaw/common/system-account-secret.yaml b/tests/chainsaw/common/system-account-secret.yaml
@@ -1,5 +1,6 @@
 apiVersion: v1
 data:
+  # dbsecret1
   DatabasePassword: ZGJzZWNyZXQx
 kind: Secret
 metadata:
diff --git a/tests/chainsaw/tests/create-system-account/chainsaw-test.yaml b/tests/chainsaw/tests/create-system-account/chainsaw-test.yaml
@@ -49,6 +49,20 @@ spec:
         check:
           ($error): ~
 
+  - name: update secret in place
+    description: change the secret to a new one and verify password is updated in database
+    skipDelete: true
+    try:
+    - apply:
+        file: system-account-update-secret.yaml
+    - assert:
+        file: system-account-update-assert.yaml
+    - script:
+        content: |
+          ../../scripts/check_db_account.sh openstack-galera-0 '' systemuser dbsecret2
+        check:
+          ($error): ~
+
   - name: drop system account
     description: delete the MariaDBAccount CR and verify account is removed from database
     try:
@@ -59,9 +73,11 @@ spec:
           name: chainsawdb-some-system-db-account
     - script:
         content: |
-          ../../scripts/check_db_account.sh openstack-galera-0 "" systemuser dbsecret1 --reverse
+          ../../scripts/check_db_account.sh openstack-galera-0 "" systemuser dbsecret2 --reverse
         check:
           ($error): ~
     finally:
     - delete:
         file: ../../common/system-account-secret.yaml
+    - delete:
+        file: system-account-update-secret.yaml
diff --git a/tests/chainsaw/tests/create-system-account/system-account-update-assert.yaml b/tests/chainsaw/tests/create-system-account/system-account-update-assert.yaml
@@ -0,0 +1,39 @@
+---
+apiVersion: mariadb.openstack.org/v1beta1
+kind: MariaDBAccount
+metadata:
+  labels:
+    dbName: openstack
+  name: chainsawdb-some-system-db-account
+status:
+  currentSecret: some-new-system-db-secret
+  conditions:
+  - message: Setup complete
+    reason: Ready
+    status: "True"
+    type: Ready
+  - message: MariaDBAccount creation complete
+    reason: Ready
+    status: "True"
+    type: MariaDBAccountReady
+  - message: MariaDB / Galera server ready
+    reason: Ready
+    status: "True"
+    type: MariaDBServerReady
+---
+apiVersion: v1
+kind: Secret
+metadata:
+  name: some-new-system-db-secret
+  # ensure finalizer was added to new secret
+  finalizers:
+  - openstack.org/mariadbaccount
+type: Opaque
+---
+apiVersion: v1
+kind: Secret
+metadata:
+  name: some-system-db-secret
+  # ensure finalizer was removed from old secret (field should not exist)
+  (finalizers): null
+type: Opaque
diff --git a/tests/chainsaw/tests/create-system-account/system-account-update-secret.yaml b/tests/chainsaw/tests/create-system-account/system-account-update-secret.yaml
diff --git a/tests/kuttl/tests/account_create/05-assert.yaml b/tests/kuttl/tests/account_create/05-assert.yaml
