From e72f0dc413180b9dd609fde32bf28ae526074fd0 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Mon, 24 Mar 2025 18:39:52 -0400 Subject: [PATCH 1/2] mariadbaccount system accounts introduce a new class of MariaDBAccount called a "system" MariaDBAccount, indicated by a new enumerated field AccountType on the CR. Such accounts link directly to a Galera instance and have no dependency on a MariaDBDatabase CR. The expected targets for "system" accounts will include the Galera/mysql root username and password, as well as a system account used by mariadbbackup for SST. Refactor mariadbaccount_controller to isolate logic used for acquiring MariaDBDatabase and Galera CRs into separate functions, and ensure all MariaDBDatabase logic takes place only for "user" accounts (which would be all current MariaDBAccount CRs). Also correct an oversight where MariaDBAccount would not unconditionally apply a finalizer to its Secret object. This logic now takes place in addition to an unconditional removal of the finalizer when the MariaDBAccount object is deleted. A subsequent change will allow system-level passwords to be changed in place by applying the secret name to two separate fields MariaDBAccount/Spec/Secret and MariaDBAccount/Status/Secret. When these two names differ it will indicate an in-place password change should take place. --- ...mariadb.openstack.org_mariadbaccounts.yaml | 6 + api/v1beta1/mariadbaccount_types.go | 19 +++ ...mariadb.openstack.org_mariadbaccounts.yaml | 6 + controllers/galera_controller.go | 3 +- controllers/mariadbaccount_controller.go | 150 ++++++++++++------ templates/account.sh | 7 +- .../common/system-account-assert.yaml | 42 +++++ .../common/system-account-secret.yaml | 7 + tests/chainsaw/common/system-account.yaml | 10 ++ tests/chainsaw/scripts/check_db_account.sh | 36 +++++ .../create-system-account/chainsaw-test.yaml | 67 ++++++++ .../system-account-missing-secret-assert.yaml | 21 +++ .../chainsaw-test.yaml | 63 ++++++++ 13 files changed, 385 insertions(+), 52 deletions(-) create mode 100644 tests/chainsaw/common/system-account-assert.yaml create mode 100644 tests/chainsaw/common/system-account-secret.yaml create mode 100644 tests/chainsaw/common/system-account.yaml create mode 100755 tests/chainsaw/scripts/check_db_account.sh create mode 100644 tests/chainsaw/tests/create-system-account/chainsaw-test.yaml create mode 100644 tests/chainsaw/tests/create-system-account/system-account-missing-secret-assert.yaml create mode 100644 tests/chainsaw/tests/system-account-preexisting/chainsaw-test.yaml diff --git a/api/bases/mariadb.openstack.org_mariadbaccounts.yaml b/api/bases/mariadb.openstack.org_mariadbaccounts.yaml index 9a92f28f..36599ddb 100644 --- a/api/bases/mariadb.openstack.org_mariadbaccounts.yaml +++ b/api/bases/mariadb.openstack.org_mariadbaccounts.yaml @@ -48,6 +48,12 @@ spec: spec: description: MariaDBAccountSpec defines the desired state of MariaDBAccount properties: + accountType: + default: User + enum: + - User + - System + type: string requireTLS: default: false description: Account must use TLS to connect to the database diff --git a/api/v1beta1/mariadbaccount_types.go b/api/v1beta1/mariadbaccount_types.go index 32819281..ee3a07f4 100644 --- a/api/v1beta1/mariadbaccount_types.go +++ b/api/v1beta1/mariadbaccount_types.go @@ -48,8 +48,19 @@ type MariaDBAccountSpec struct { // Account must use TLS to connect to the database // +kubebuilder:default=false RequireTLS bool `json:"requireTLS"` + + // +kubebuilder:validation:Enum=User;System + // +kubebuilder:default=User + AccountType AccountType `json:"accountType,omitempty"` } +type AccountType string + +const ( + User AccountType = "User" + System AccountType = "System" +) + // MariaDBAccountStatus defines the observed state of MariaDBAccount type MariaDBAccountStatus struct { // Deployment Conditions @@ -85,3 +96,11 @@ type MariaDBAccountList struct { func init() { SchemeBuilder.Register(&MariaDBAccount{}, &MariaDBAccountList{}) } + +func (mariadbAccount MariaDBAccount) IsSystemAccount() bool { + return mariadbAccount.Spec.AccountType == System +} + +func (mariadbAccount MariaDBAccount) IsUserAccount() bool { + return mariadbAccount.Spec.AccountType == "" || mariadbAccount.Spec.AccountType == User +} diff --git a/config/crd/bases/mariadb.openstack.org_mariadbaccounts.yaml b/config/crd/bases/mariadb.openstack.org_mariadbaccounts.yaml index 9a92f28f..36599ddb 100644 --- a/config/crd/bases/mariadb.openstack.org_mariadbaccounts.yaml +++ b/config/crd/bases/mariadb.openstack.org_mariadbaccounts.yaml @@ -48,6 +48,12 @@ spec: spec: description: MariaDBAccountSpec defines the desired state of MariaDBAccount properties: + accountType: + default: User + enum: + - User + - System + type: string requireTLS: default: false description: Account must use TLS to connect to the database diff --git a/controllers/galera_controller.go b/controllers/galera_controller.go index 7b81606c..f1b478cf 100644 --- a/controllers/galera_controller.go +++ b/controllers/galera_controller.go @@ -1058,9 +1058,8 @@ func (r *GaleraReconciler) SetupWithManager(mgr ctrl.Manager) error { Complete(r) } -// GetDatabaseObject - returns either a Galera or MariaDB object (and an associated client.Object interface). +// GetDatabaseObject - returns a Galera object. // used by both MariaDBDatabaseReconciler and MariaDBAccountReconciler -// this will later return only Galera objects, so as a lookup it's part of the galera controller func GetDatabaseObject(ctx context.Context, clientObj client.Client, name string, namespace string) (*mariadbv1.Galera, error) { dbGalera := &mariadbv1.Galera{ ObjectMeta: metav1.ObjectMeta{ diff --git a/controllers/mariadbaccount_controller.go b/controllers/mariadbaccount_controller.go index 498cffe3..c5d70fd7 100644 --- a/controllers/mariadbaccount_controller.go +++ b/controllers/mariadbaccount_controller.go @@ -30,6 +30,7 @@ import ( 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" + batchv1 "k8s.io/api/batch/v1" k8s_errors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -113,12 +114,18 @@ func (r *MariaDBAccountReconciler) Reconcile(ctx context.Context, req ctrl.Reque }() // initialize conditions used later as Status=Unknown - cl := condition.CreateList( - condition.UnknownCondition(condition.ReadyCondition, condition.InitReason, condition.ReadyInitMessage), - condition.UnknownCondition(databasev1beta1.MariaDBServerReadyCondition, condition.InitReason, databasev1beta1.MariaDBServerReadyInitMessage), - condition.UnknownCondition(databasev1beta1.MariaDBDatabaseReadyCondition, condition.InitReason, databasev1beta1.MariaDBDatabaseReadyInitMessage), - condition.UnknownCondition(databasev1beta1.MariaDBAccountReadyCondition, condition.InitReason, databasev1beta1.MariaDBAccountReadyInitMessage), - ) + cl := condition.Conditions{ + *condition.UnknownCondition(condition.ReadyCondition, condition.InitReason, condition.ReadyInitMessage), + *condition.UnknownCondition(databasev1beta1.MariaDBServerReadyCondition, condition.InitReason, databasev1beta1.MariaDBServerReadyInitMessage), + } + if instance.IsUserAccount() { + // user accounts also need the database ready condition + // the element is being inserted into the list in a specific location + // to preseve expectations of tests suites that are hardcoded to + // expect a specific ordering of Conditions in YAML displays + cl = append(cl, *condition.UnknownCondition(databasev1beta1.MariaDBDatabaseReadyCondition, condition.InitReason, databasev1beta1.MariaDBDatabaseReadyInitMessage)) + } + cl = append(cl, *condition.UnknownCondition(databasev1beta1.MariaDBAccountReadyCondition, condition.InitReason, databasev1beta1.MariaDBAccountReadyInitMessage)) instance.Status.Conditions.Init(&cl) if instance.DeletionTimestamp.IsZero() || isNewInstance { //revive:disable:indent-error-flow @@ -134,11 +141,16 @@ func (r *MariaDBAccountReconciler) reconcileCreate( ctx context.Context, log logr.Logger, helper *helper.Helper, instance *databasev1beta1.MariaDBAccount) (result ctrl.Result, _err error) { - // get a handle to the current, active MariaDBDatabase. - // if not ready yet, requeue. - mariadbDatabase, result, err := r.getMariaDBDatabaseForCreate(ctx, log, instance) - if mariadbDatabase == nil { - return result, err + var mariadbDatabase *databasev1beta1.MariaDBDatabase + var err error + + if instance.IsUserAccount() { + // for User account, get a handle to the current, active MariaDBDatabase. + // if not ready yet, requeue. + mariadbDatabase, result, err = r.getMariaDBDatabaseForCreate(ctx, log, instance) + if mariadbDatabase == nil { + return result, err + } } if controllerutil.AddFinalizer(instance, helper.GetFinalizer()) { @@ -146,11 +158,13 @@ func (r *MariaDBAccountReconciler) reconcileCreate( return ctrl.Result{}, nil } - // MariaDBdatabase exists and we are a create case. ensure finalizers set up - if controllerutil.AddFinalizer(mariadbDatabase, fmt.Sprintf("%s-%s", helper.GetFinalizer(), instance.Name)) { - err = r.Update(ctx, mariadbDatabase) - if err != nil { - return ctrl.Result{}, err + if instance.IsUserAccount() { + // MariaDBdatabase exists and we are a create case. ensure finalizers set up + if controllerutil.AddFinalizer(mariadbDatabase, fmt.Sprintf("%s-%s", helper.GetFinalizer(), instance.Name)) { + err = r.Update(ctx, mariadbDatabase) + if err != nil { + return ctrl.Result{}, err + } } } @@ -174,13 +188,25 @@ func (r *MariaDBAccountReconciler) reconcileCreate( return result, err } - log.Info(fmt.Sprintf("Running account create '%s' MariaDBDatabase '%s'", - instance.Name, mariadbDatabase.Spec.Name)) - jobDef, err := mariadb.CreateDbAccountJob( - dbGalera, instance, mariadbDatabase.Spec.Name, dbHostname, - dbContainerImage, serviceAccountName, dbGalera.Spec.NodeSelector) - if err != nil { - return ctrl.Result{}, err + var jobDef *batchv1.Job + + if instance.IsUserAccount() { + log.Info(fmt.Sprintf("Running account create '%s' MariaDBDatabase '%s'", + instance.Name, mariadbDatabase.Spec.Name)) + jobDef, err = mariadb.CreateDbAccountJob( + 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( + dbGalera, instance, "", dbHostname, + dbContainerImage, serviceAccountName, dbGalera.Spec.NodeSelector) + if err != nil { + return ctrl.Result{}, err + } } accountCreateHash := instance.Status.Hash[databasev1beta1.AccountCreateHash] @@ -228,9 +254,14 @@ func (r *MariaDBAccountReconciler) reconcileDelete( ctx context.Context, log logr.Logger, helper *helper.Helper, instance *databasev1beta1.MariaDBAccount) (result ctrl.Result, _err error) { - mariadbDatabase, result, err := r.getMariaDBDatabaseForDelete(ctx, log, helper, instance) - if mariadbDatabase == nil { - return result, err + var mariadbDatabase *databasev1beta1.MariaDBDatabase + var err error + + if instance.IsUserAccount() { + mariadbDatabase, result, err = r.getMariaDBDatabaseForDelete(ctx, log, helper, instance) + if mariadbDatabase == nil { + return result, err + } } // dont do actual DROP USER until finalizers from downstream controllers @@ -259,10 +290,12 @@ func (r *MariaDBAccountReconciler) reconcileDelete( databasev1beta1.MariaDBAccountReadyForDeleteMessage, ) - instance.Status.Conditions.MarkTrue( - databasev1beta1.MariaDBDatabaseReadyCondition, - databasev1beta1.MariaDBDatabaseReadyMessage, - ) + if instance.IsUserAccount() { + instance.Status.Conditions.MarkTrue( + databasev1beta1.MariaDBDatabaseReadyCondition, + databasev1beta1.MariaDBDatabaseReadyMessage, + ) + } // now proceed to do actual work. acquire the Galera instance // which will lead us to the hostname and container image to target @@ -278,20 +311,21 @@ func (r *MariaDBAccountReconciler) reconcileDelete( // scheme allows multiple MariaDBAccounts to claim the same MariaDBDatabase // as a dependency) and allows a delete of the MariaDBDatabase to proceed // assuming no other finalizers - if controllerutil.RemoveFinalizer(mariadbDatabase, fmt.Sprintf("%s-%s", helper.GetFinalizer(), instance.Name)) { - err = r.Update(ctx, mariadbDatabase) + if instance.IsUserAccount() { + if controllerutil.RemoveFinalizer(mariadbDatabase, fmt.Sprintf("%s-%s", helper.GetFinalizer(), instance.Name)) { + err = r.Update(ctx, mariadbDatabase) - if err != nil && !k8s_errors.IsNotFound(err) { - return ctrl.Result{}, err + if err != nil && !k8s_errors.IsNotFound(err) { + return ctrl.Result{}, err + } } - } // remove finalizer "openstack.org/mariadbaccount" from both the // MariaDBAccount as well as the Secret which is referenced from the // MariaDBAccount, allowing both to be deleted assuming no other // finalizers - err = r.removeAccountAndSecretFinalizer(ctx, helper, instance) + err := r.removeAccountAndSecretFinalizer(ctx, helper, instance) return ctrl.Result{}, err } else if dbGalera == nil { return result, err @@ -300,13 +334,24 @@ func (r *MariaDBAccountReconciler) reconcileDelete( dbContainerImage := dbGalera.Spec.ContainerImage serviceAccountName := dbGalera.RbacResourceName() - log.Info(fmt.Sprintf("Running account delete '%s' MariaDBDatabase '%s'", instance.Name, mariadbDatabase.Spec.Name)) + var jobDef *batchv1.Job - jobDef, err := mariadb.DeleteDbAccountJob(dbGalera, instance, mariadbDatabase.Spec.Name, dbHostname, dbContainerImage, serviceAccountName, dbGalera.Spec.NodeSelector) - if err != nil { - return ctrl.Result{}, err - } + if instance.IsUserAccount() { + + log.Info(fmt.Sprintf("Running account delete '%s' MariaDBDatabase '%s'", instance.Name, mariadbDatabase.Spec.Name)) + + jobDef, err = mariadb.DeleteDbAccountJob(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 delete '%s'", instance.Name)) + jobDef, err = mariadb.DeleteDbAccountJob(dbGalera, instance, "", dbHostname, dbContainerImage, serviceAccountName, dbGalera.Spec.NodeSelector) + if err != nil { + return ctrl.Result{}, err + } + } accountDeleteHash := instance.Status.Hash[databasev1beta1.AccountDeleteHash] accountDeleteJob := job.NewJob( jobDef, @@ -335,10 +380,12 @@ func (r *MariaDBAccountReconciler) reconcileDelete( // scheme allows multiple MariaDBAccounts to claim the same MariaDBDatabase // as a dependency) and allows a delete of the MariaDBDatabase to proceed // assuming no other finalizers - if controllerutil.RemoveFinalizer(mariadbDatabase, fmt.Sprintf("%s-%s", helper.GetFinalizer(), instance.Name)) { - err = r.Update(ctx, mariadbDatabase) - if err != nil { - return ctrl.Result{}, err + if instance.IsUserAccount() { + if controllerutil.RemoveFinalizer(mariadbDatabase, fmt.Sprintf("%s-%s", helper.GetFinalizer(), instance.Name)) { + err = r.Update(ctx, mariadbDatabase) + if err != nil { + return ctrl.Result{}, err + } } } @@ -346,7 +393,6 @@ func (r *MariaDBAccountReconciler) reconcileDelete( // both the MariaDBAccount as well as the Secret which is referenced // from the MariaDBAccount, allowing both to be deleted err = r.removeAccountAndSecretFinalizer(ctx, helper, instance) - return ctrl.Result{}, err } @@ -526,9 +572,17 @@ func (r *MariaDBAccountReconciler) getGaleraForCreateOrDelete( helper *helper.Helper, instance *databasev1beta1.MariaDBAccount, mariadbDatabase *databasev1beta1.MariaDBDatabase) (*databasev1beta1.Galera, string, ctrl.Result, error) { - dbName := mariadbDatabase.Labels["dbName"] + var dbGalera *databasev1beta1.Galera + var err error + var dbName string - dbGalera, err := GetDatabaseObject(ctx, r.Client, dbName, instance.Namespace) + if instance.IsUserAccount() { + dbName = mariadbDatabase.Labels["dbName"] + } else { + // note mariadbDatabase is passed as nil in this case + dbName = instance.Labels["dbName"] + } + dbGalera, err = GetDatabaseObject(ctx, r.Client, dbName, instance.Namespace) if err != nil { log.Error(err, "Error retrieving Galera instance") diff --git a/templates/account.sh b/templates/account.sh index ba076cb3..dccd5aca 100755 --- a/templates/account.sh +++ b/templates/account.sh @@ -4,8 +4,11 @@ MYSQL_REMOTE_HOST={{.DatabaseHostname}} source /var/lib/operator-scripts/mysql_r export DatabasePassword=${DatabasePassword:?"Please specify a DatabasePassword variable."} -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}};" - +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}};" +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}};" +fi # search for the account. not using SHOW CREATE USER to avoid displaying # password hash diff --git a/tests/chainsaw/common/system-account-assert.yaml b/tests/chainsaw/common/system-account-assert.yaml new file mode 100644 index 00000000..114f6113 --- /dev/null +++ b/tests/chainsaw/common/system-account-assert.yaml @@ -0,0 +1,42 @@ +--- +apiVersion: mariadb.openstack.org/v1beta1 +kind: MariaDBAccount +metadata: + labels: + dbName: openstack + name: chainsawdb-some-system-db-account +status: + 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 +data: + DatabasePassword: ZGJzZWNyZXQx +kind: Secret +metadata: + name: some-system-db-secret + # ensure finalizer was added + finalizers: + - openstack.org/mariadbaccount +type: Opaque +--- +apiVersion: batch/v1 +kind: Job +metadata: + name: systemuser-account-create +spec: + template: + spec: {} +status: + succeeded: 1 diff --git a/tests/chainsaw/common/system-account-secret.yaml b/tests/chainsaw/common/system-account-secret.yaml new file mode 100644 index 00000000..4c46bba6 --- /dev/null +++ b/tests/chainsaw/common/system-account-secret.yaml @@ -0,0 +1,7 @@ +apiVersion: v1 +data: + DatabasePassword: ZGJzZWNyZXQx +kind: Secret +metadata: + name: some-system-db-secret +type: Opaque diff --git a/tests/chainsaw/common/system-account.yaml b/tests/chainsaw/common/system-account.yaml new file mode 100644 index 00000000..2ad267cc --- /dev/null +++ b/tests/chainsaw/common/system-account.yaml @@ -0,0 +1,10 @@ +apiVersion: mariadb.openstack.org/v1beta1 +kind: MariaDBAccount +metadata: + labels: + dbName: openstack + name: chainsawdb-some-system-db-account +spec: + userName: systemuser + secret: some-system-db-secret + accountType: System diff --git a/tests/chainsaw/scripts/check_db_account.sh b/tests/chainsaw/scripts/check_db_account.sh new file mode 100755 index 00000000..d0186971 --- /dev/null +++ b/tests/chainsaw/scripts/check_db_account.sh @@ -0,0 +1,36 @@ +#!/bin/sh + +set -x + +galera="$1" +dbname="$2" +username="$3" +password="$4" + + +found=0 +not_found=1 + +if [ "$5" = "--reverse" ];then + # sometimes we want to check that a user does not exist + found=1 + not_found=0 +fi + +found_username=$(oc exec -n ${NAMESPACE} -c galera ${galera} -- /bin/sh -c 'source /var/lib/operator-scripts/mysql_root_auth.sh; mysql -uroot -p${DB_ROOT_PASSWORD} -Nse "select user from mysql.user"' | grep -o -w ${username}) + +# username was not found, exit +if [ -z "$found_username" ]; then + exit $not_found +fi + +# username was found. if we wanted it to be found, then check the login also. +if [ "$found" = "0" ]; then + if [ -n "$dbname" ]; then + oc exec -n ${NAMESPACE} -c galera ${galera} -- /bin/sh -c "mysql -u${username} -p${password} -Nse 'select database();' ${dbname}" || exit -1 + else + oc exec -n ${NAMESPACE} -c galera ${galera} -- /bin/sh -c "mysql -u${username} -p${password} -Nse 'select 1'" || exit -1 + fi +fi + +exit $found diff --git a/tests/chainsaw/tests/create-system-account/chainsaw-test.yaml b/tests/chainsaw/tests/create-system-account/chainsaw-test.yaml new file mode 100644 index 00000000..d16c352e --- /dev/null +++ b/tests/chainsaw/tests/create-system-account/chainsaw-test.yaml @@ -0,0 +1,67 @@ +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + name: create-system-account +spec: + steps: + - name: Deploy 1-node cluster + description: Deploy a 1-node cluster for tests + bindings: + - name: replicas + value: 1 + try: + - apply: + file: ../../common/galera.yaml + - assert: + file: ../../common/galera-assert.yaml + + - name: create system account without secret + description: system 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: ../../common/system-account.yaml + - assert: + file: system-account-missing-secret-assert.yaml + + - name: add secret and finish system account creation + description: make sure the system account is created in the database + # we will delete the secret manually + skipDelete: true + try: + - apply: + file: ../../common/system-account-secret.yaml + - assert: + file: ../../common/system-account-assert.yaml + + - name: verify system account in database + description: check that system account exists in database with correct permissions + try: + - script: + content: | + ../../scripts/check_db_account.sh openstack-galera-0 '' systemuser dbsecret1 + check: + ($error): ~ + - script: + content: | + oc exec -n ${NAMESPACE} -c galera openstack-galera-0 -- /bin/sh -c 'source /var/lib/operator-scripts/mysql_root_auth.sh; mysql -uroot -p${DB_ROOT_PASSWORD} -e "show grants for \`systemuser\`@\`%\`;"' | grep 'GRANT ALL' | grep -v 'REQUIRE SSL' + check: + ($error): ~ + + - name: drop system account + description: delete the MariaDBAccount CR and verify account is removed from database + try: + - delete: + ref: + apiVersion: mariadb.openstack.org/v1beta1 + kind: MariaDBAccount + name: chainsawdb-some-system-db-account + - script: + content: | + ../../scripts/check_db_account.sh openstack-galera-0 "" systemuser dbsecret1 --reverse + check: + ($error): ~ + finally: + - delete: + file: ../../common/system-account-secret.yaml diff --git a/tests/chainsaw/tests/create-system-account/system-account-missing-secret-assert.yaml b/tests/chainsaw/tests/create-system-account/system-account-missing-secret-assert.yaml new file mode 100644 index 00000000..9f8506d4 --- /dev/null +++ b/tests/chainsaw/tests/create-system-account/system-account-missing-secret-assert.yaml @@ -0,0 +1,21 @@ +--- +apiVersion: mariadb.openstack.org/v1beta1 +kind: MariaDBAccount +metadata: + labels: + dbName: openstack + name: chainsawdb-some-system-db-account +status: + conditions: + - reason: SecretMissing + severity: Warning + status: "False" + type: Ready + - reason: SecretMissing + severity: Warning + status: "False" + type: MariaDBAccountReady + - message: MariaDB / Galera server ready + reason: Ready + status: "True" + type: MariaDBServerReady diff --git a/tests/chainsaw/tests/system-account-preexisting/chainsaw-test.yaml b/tests/chainsaw/tests/system-account-preexisting/chainsaw-test.yaml new file mode 100644 index 00000000..e07474b3 --- /dev/null +++ b/tests/chainsaw/tests/system-account-preexisting/chainsaw-test.yaml @@ -0,0 +1,63 @@ +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + name: system-account-preexisting +spec: + steps: + - name: Deploy 1-node cluster + description: Deploy a 1-node cluster for tests + bindings: + - name: replicas + value: 1 + try: + - apply: + file: ../../common/galera.yaml + - assert: + file: ../../common/galera-assert.yaml + + - name: create user manually with old password + description: manually create the systemuser account with a different password than the MariaDBAccount will use + try: + - script: + content: | + oc exec -n ${NAMESPACE} -c galera openstack-galera-0 -- /bin/sh -c 'source /var/lib/operator-scripts/mysql_root_auth.sh; mysql -uroot -p${DB_ROOT_PASSWORD} -e "GRANT ALL PRIVILEGES ON *.* TO \`systemuser\`@\`localhost\` IDENTIFIED BY \"oldpassword123\"; GRANT ALL PRIVILEGES ON *.* TO \`systemuser\`@\`%\` IDENTIFIED BY \"oldpassword123\";"' + check: + ($error): ~ + + - name: verify old password works + description: verify that the manually created account works with the old password + try: + - script: + content: | + ../../scripts/check_db_account.sh openstack-galera-0 '' systemuser oldpassword123 + check: + ($error): ~ + + - name: apply secret with new password + description: apply the secret with the new password that will be used by MariaDBAccount + try: + - apply: + file: ../../common/system-account-secret.yaml + + - name: apply MariaDBAccount + description: apply the MariaDBAccount CR to update the existing account password + try: + - apply: + file: ../../common/system-account.yaml + - assert: + file: ../../common/system-account-assert.yaml + + - name: verify new password works and old password fails + description: verify that the account password was updated to the new password from the secret + try: + - script: + content: | + ../../scripts/check_db_account.sh openstack-galera-0 '' systemuser dbsecret1 + check: + ($error): ~ + - script: + content: | + # verify old password no longer works + ! oc exec -n ${NAMESPACE} -c galera openstack-galera-0 -- /bin/sh -c "mysql -usystemuser -poldpassword123 -Nse 'select 1'" + check: + ($error): ~ From 2581eb575111b6b0223a68960cfaa8db0622d910 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Tue, 29 Apr 2025 14:55:37 -0400 Subject: [PATCH 2/2] support in-place secret changes The existing mariadb operator in fact already "supports" in-place change of secret, since if you change Spec.Secret to a new secret name, that would imply a new job hash, and the account.sh script running using only GRANT statements would update the password per mariadb. This already works for flipping the TLS flag on and off too. So in this patch, we clean this up and add a test to include: * a new field Status.CurrentSecret, which is used to indicate the previous secret from which the finalizer should be removed. This will also be used when we migrate the root password to use MariaDBAccount by providing the "current" root password when changing to a new root password * improved messaging in log messages, name of job. This changes the job hash for mariadbaccount which will incur a run on existing environments, however the job hashes are already changing on existing environments due to the change in how the mysql root password is sent, i.e. via volume mounted script rather than env var secret * update account.sh to use modern idiomatic patterns for user create /alter, while mariadb is fine with the legacy style of using only GRANT statements, MySQL 8 no longer allows this statement to proceed without a CREATE USER, so formalize the commands used here to use distinct CREATE USER, ALTER USER, GRANT statements and clarify the script is good for all create/update user operations. --- ...mariadb.openstack.org_mariadbaccounts.yaml | 8 +++ api/v1beta1/mariadbaccount_types.go | 7 +++ ...mariadb.openstack.org_mariadbaccounts.yaml | 8 +++ controllers/mariadbaccount_controller.go | 58 +++++++++++++++---- pkg/mariadb/account.go | 8 +-- templates/account.sh | 26 ++++++++- .../common/system-account-assert.yaml | 3 +- .../common/system-account-secret.yaml | 1 + .../create-system-account/chainsaw-test.yaml | 18 +++++- .../system-account-update-assert.yaml | 22 +++++++ .../system-account-update-secret.yaml | 19 ++++++ .../kuttl/tests/account_create/05-assert.yaml | 2 +- 12 files changed, 158 insertions(+), 22 deletions(-) create mode 100644 tests/chainsaw/tests/create-system-account/system-account-update-assert.yaml create mode 100644 tests/chainsaw/tests/create-system-account/system-account-update-secret.yaml diff --git a/api/bases/mariadb.openstack.org_mariadbaccounts.yaml b/api/bases/mariadb.openstack.org_mariadbaccounts.yaml index 36599ddb..51dd7196 100644 --- 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 index ee3a07f4..dd5885e2 100644 --- a/api/v1beta1/mariadbaccount_types.go +++ b/api/v1beta1/mariadbaccount_types.go @@ -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 index 36599ddb..51dd7196 100644 --- 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 index c5d70fd7..ab2d330f 100644 --- a/controllers/mariadbaccount_controller.go +++ b/controllers/mariadbaccount_controller.go @@ -129,15 +129,15 @@ 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) } 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) { @@ -190,18 +190,27 @@ func (r *MariaDBAccountReconciler) reconcileCreate( var jobDef *batchv1.Job + var createOrUpdate string + if instance.Status.CurrentSecret != "" { + createOrUpdate = "update" + } else { + createOrUpdate = "create" + } + 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 { @@ -226,11 +235,24 @@ func (r *MariaDBAccountReconciler) reconcileCreate( } if accountCreateJob.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])) + log.Info(fmt.Sprintf("Job %s hash created or updated - %s", jobDef.Name, instance.Status.Hash[databasev1beta1.AccountCreateHash])) + + // 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 index 0dbf897f..15e9e87a 100644 --- a/pkg/mariadb/account.go +++ b/pkg/mariadb/account.go @@ -19,8 +19,8 @@ type accountCreateOrDeleteOptions struct { 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" @@ -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{ diff --git a/templates/account.sh b/templates/account.sh index dccd5aca..891ede76 100755 --- 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 <