From cd96520cc599f4f6a842fe2e0a578a15c63e517c Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Tue, 12 Mar 2024 15:15:29 -0400 Subject: [PATCH] apply finalizers to Secret for MariaDBAccount when a controller is reconciling the delete of its own CR as well as its MariaDBAccount, the lookup for MariaDBDatabase/MariaDBAccount must succeed so that db.DeleteFinalizer can be called. This uses GetDatabaseByNameAndAccount which is required to locate all three of MariaDBDatabase, MariaDBAccount, and Secret, else notfound is returned. therefore, prevent the Secret from being prematurely removed from the cluster by adding a calling CR finalizer to it, the same as it's added to the MariaDBAccount. Prior to this change, the GetDatabaseByNameAndAccount can return a not-found even though the MariaDBAccount exists, causing the calling controller to skip it and delete its own CR, leaving the MariaDBAccount and MariaDBDatabase dangling. the issue can be reproduced by building up an openstack env with keystone / galera / memcached / rabbitmq, then deleting the namespace. k8s will delete the secrets more quickly than it can remove the CRs since they are reconciling the delete. --- api/test/helpers/harnesses.go | 75 +++++++++++++++ api/v1beta1/mariadbdatabase_funcs.go | 134 +++++++++++++++++++++++---- 2 files changed, 189 insertions(+), 20 deletions(-) diff --git a/api/test/helpers/harnesses.go b/api/test/helpers/harnesses.go index f7d5f314..b3a78155 100644 --- a/api/test/helpers/harnesses.go +++ b/api/test/helpers/harnesses.go @@ -124,6 +124,12 @@ func (harness *MariaDBTestHarness) RunBasicSuite() { return mariadbAccount.Finalizers }, timeout, interval).Should(ContainElement(harness.finalizerName)) + // as well as in the secret + Eventually(func() []string { + dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: mariadbAccount.Spec.Secret, Namespace: mariadbAccount.Namespace}) + return dbSecret.Finalizers + }, timeout, interval).Should(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName))) + // mariaDBDatabaseName is set Expect(mariadbAccount.Labels["mariaDBDatabaseName"]).Should(Equal(harness.databaseName)) @@ -160,6 +166,12 @@ func (harness *MariaDBTestHarness) RunBasicSuite() { return mariadbAccount.Finalizers }, timeout, interval).Should(ContainElement(harness.finalizerName)) + // as well as in the secret + Eventually(func() []string { + dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: mariadbAccount.Spec.Secret, Namespace: mariadbAccount.Namespace}) + return dbSecret.Finalizers + }, timeout, interval).Should(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName))) + // mariaDBDatabaseName is set Expect(mariadbAccount.Labels["mariaDBDatabaseName"]).Should(Equal(harness.databaseName)) @@ -194,6 +206,13 @@ func (harness *MariaDBTestHarness) RunBasicSuite() { return oldMariadbAccount.Finalizers }, timeout, interval).Should(ContainElement(harness.finalizerName)) + // as well as in the secret + Eventually(func() []string { + oldMariadbAccount := mariaDBHelper.GetMariaDBAccount(oldAccountName) + dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: oldMariadbAccount.Spec.Secret, Namespace: oldMariadbAccount.Namespace}) + return dbSecret.Finalizers + }, timeout, interval).Should(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName))) + }) It("should ensure a new MariaDBAccount exists when accountname is changed", func() { mariaDBHelper, timeout, interval := harness.mariaDBHelper, harness.timeout, harness.interval @@ -228,12 +247,27 @@ func (harness *MariaDBTestHarness) RunBasicSuite() { return newMariadbAccount.Finalizers }, timeout, interval).Should(ContainElement(harness.finalizerName)) + // as well as in the secret + Eventually(func() []string { + newMariadbAccount := mariaDBHelper.GetMariaDBAccount(newAccountName) + dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: newMariadbAccount.Spec.Secret, Namespace: newMariadbAccount.Namespace}) + return dbSecret.Finalizers + }, timeout, interval).Should(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName))) + // old account retains the finalizer because we did not yet // complete the new MariaDBAccount Consistently(func() []string { oldMariadbAccount := mariaDBHelper.GetMariaDBAccount(oldAccountName) return oldMariadbAccount.Finalizers }, timeout, interval).Should(ContainElement(harness.finalizerName)) + + // as well as in the secret + Eventually(func() []string { + oldMariadbAccount := mariaDBHelper.GetMariaDBAccount(oldAccountName) + dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: oldMariadbAccount.Spec.Secret, Namespace: oldMariadbAccount.Namespace}) + return dbSecret.Finalizers + }, timeout, interval).Should(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName))) + }) It("should move the finalizer to a new MariaDBAccount when create is complete", func() { @@ -263,12 +297,26 @@ func (harness *MariaDBTestHarness) RunBasicSuite() { return newMariadbAccount.Finalizers }, timeout, interval).Should(ContainElement(harness.finalizerName)) + // as well as in the secret + Eventually(func() []string { + newMariadbAccount := mariaDBHelper.GetMariaDBAccount(newAccountName) + dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: newMariadbAccount.Spec.Secret, Namespace: newMariadbAccount.Namespace}) + return dbSecret.Finalizers + }, timeout, interval).Should(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName))) + // finalizer removed from old account Eventually(func() []string { oldMariadbAccount := mariaDBHelper.GetMariaDBAccount(oldAccountName) return oldMariadbAccount.Finalizers }, timeout, interval).ShouldNot(ContainElement(harness.finalizerName)) + // as well as in the secret + Eventually(func() []string { + oldMariadbAccount := mariaDBHelper.GetMariaDBAccount(oldAccountName) + dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: oldMariadbAccount.Spec.Secret, Namespace: oldMariadbAccount.Namespace}) + return dbSecret.Finalizers + }, timeout, interval).ShouldNot(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName))) + // CreateOrPatchDBByName will add a label referring to the database Eventually(func() string { mariadbAccount := mariaDBHelper.GetMariaDBAccount(newAccountName) @@ -311,11 +359,25 @@ func (harness *MariaDBTestHarness) RunBasicSuite() { return newMariadbAccount.Finalizers }, timeout, interval).Should(ContainElement(harness.finalizerName)) + // as well as in the secret + Eventually(func() []string { + newMariadbAccount := mariaDBHelper.GetMariaDBAccount(newAccountName) + dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: newMariadbAccount.Spec.Secret, Namespace: newMariadbAccount.Namespace}) + return dbSecret.Finalizers + }, timeout, interval).Should(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName))) + Eventually(func() []string { oldMariadbAccount := mariaDBHelper.GetMariaDBAccount(oldAccountName) return oldMariadbAccount.Finalizers }, timeout, interval).Should(ContainElement(harness.finalizerName)) + // as well as in the secret + Eventually(func() []string { + oldMariadbAccount := mariaDBHelper.GetMariaDBAccount(oldAccountName) + dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: oldMariadbAccount.Spec.Secret, Namespace: oldMariadbAccount.Namespace}) + return dbSecret.Finalizers + }, timeout, interval).Should(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName))) + // now delete the CR harness.DeleteCR() @@ -331,6 +393,19 @@ func (harness *MariaDBTestHarness) RunBasicSuite() { return oldMariadbAccount.Finalizers }, timeout, interval).ShouldNot(ContainElement(harness.finalizerName)) + // as well as in the secret + Eventually(func() []string { + oldMariadbAccount := mariaDBHelper.GetMariaDBAccount(oldAccountName) + dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: oldMariadbAccount.Spec.Secret, Namespace: oldMariadbAccount.Namespace}) + return dbSecret.Finalizers + }, timeout, interval).ShouldNot(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName))) + + Eventually(func() []string { + newMariadbAccount := mariaDBHelper.GetMariaDBAccount(newAccountName) + dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: newMariadbAccount.Spec.Secret, Namespace: newMariadbAccount.Namespace}) + return dbSecret.Finalizers + }, timeout, interval).ShouldNot(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName))) + }) }) diff --git a/api/v1beta1/mariadbdatabase_funcs.go b/api/v1beta1/mariadbdatabase_funcs.go index 95e3eb05..5919d766 100644 --- a/api/v1beta1/mariadbdatabase_funcs.go +++ b/api/v1beta1/mariadbdatabase_funcs.go @@ -356,18 +356,18 @@ func (d *Database) CreateOrPatchAll( return ctrl.Result{RequeueAfter: time.Second * 5}, nil } - opAcc, errAcc := CreateOrPatchAccount( - ctx, h, mariaDBAccount, + opAcc, err := createOrPatchAccountAndSecret( + ctx, h, mariaDBAccount, nil, map[string]string{ "mariaDBDatabaseName": d.name, }, ) - if errAcc != nil { + if err != nil { return ctrl.Result{}, util.WrapErrorForObject( fmt.Sprintf("Error creating or updating MariaDBAccount object %s", mariaDBAccount.Name), mariaDBAccount, - errAcc, + err, ) } @@ -585,12 +585,21 @@ func (d *Database) DeleteFinalizer( h *helper.Helper, ) error { + secretFinalizer := fmt.Sprintf("mariadb.openstack.org/%s", h.GetFinalizer()) + if d.secretObj != nil && controllerutil.RemoveFinalizer(d.secretObj, secretFinalizer) { + err := h.GetClient().Update(ctx, d.secretObj) + if err != nil && !k8s_errors.IsNotFound(err) { + return err + } + util.LogForObject(h, fmt.Sprintf("Removed finalizer %s from Secret %s", secretFinalizer, d.secretObj.Name), d.secretObj) + } + if d.account != nil && controllerutil.RemoveFinalizer(d.account, h.GetFinalizer()) { err := h.GetClient().Update(ctx, d.account) if err != nil && !k8s_errors.IsNotFound(err) { return err } - util.LogForObject(h, fmt.Sprintf("Removed finalizer %s from MariaDBAccount object", h.GetFinalizer()), d.account) + util.LogForObject(h, fmt.Sprintf("Removed finalizer %s from MariaDBAccount %s", h.GetFinalizer(), d.account.Name), d.account) // also do a delete for "unused" MariaDBAccounts, associated with // this MariaDBDatabase. @@ -678,13 +687,33 @@ func DeleteUnusedMariaDBAccountFinalizers( continue } + if mariaDBAccount.Spec.Secret != "" { + dbSecret, _, err := secret.GetSecret(ctx, h, mariaDBAccount.Spec.Secret, namespace) + if err != nil && !k8s_errors.IsNotFound(err) { + return err + } + + secretFinalizer := fmt.Sprintf("mariadb.openstack.org/%s", h.GetFinalizer()) + if dbSecret != nil && controllerutil.RemoveFinalizer(dbSecret, secretFinalizer) { + err := h.GetClient().Update(ctx, dbSecret) + if err != nil && !k8s_errors.IsNotFound(err) { + h.GetLogger().Error( + err, + fmt.Sprintf("Unable to remove finalizer %s from Secret %s", h.GetFinalizer(), dbSecret.Name)) + return err + } + util.LogForObject(h, fmt.Sprintf("Removed finalizer %s from Secret %s", h.GetFinalizer(), dbSecret.Name), dbSecret) + } + + } + if controllerutil.RemoveFinalizer(&mariaDBAccount, h.GetFinalizer()) { err := h.GetClient().Update(ctx, &mariaDBAccount) if err != nil && !k8s_errors.IsNotFound(err) { h.GetLogger().Error(err, fmt.Sprintf("Unable to remove finalizer %s from MariaDBAccount %s", h.GetFinalizer(), mariaDBAccount.Name)) return err } - util.LogForObject(h, fmt.Sprintf("Removed finalizer %s from MariaDBAccount", h.GetFinalizer()), &mariaDBAccount) + util.LogForObject(h, fmt.Sprintf("Removed finalizer %s from MariaDBAccount %s", h.GetFinalizer(), mariaDBAccount.Name), &mariaDBAccount) } } @@ -692,7 +721,83 @@ func DeleteUnusedMariaDBAccountFinalizers( } +// createOrPatchAccountAndSecret creates/updates a given MariaDBAccount / Secret CR. +func createOrPatchAccountAndSecret( + ctx context.Context, + h *helper.Helper, + account *MariaDBAccount, + accountSecret *corev1.Secret, + labels map[string]string, +) (controllerutil.OperationResult, error) { + opAcc, errAcc := controllerutil.CreateOrPatch(ctx, h.GetClient(), account, func() error { + account.Labels = util.MergeStringMaps( + account.GetLabels(), + labels, + ) + + err := controllerutil.SetControllerReference(h.GetBeforeObject(), account, h.GetScheme()) + if err != nil { + return err + } + + if account.Spec.UserName == "" { + return fmt.Errorf("no UserName field in account %s", account.Name) + } + if account.Spec.Secret == "" { + return fmt.Errorf("no secret field in account %s", account.Name) + } + + if accountSecret == nil { + accountSecret, _, err = secret.GetSecret(ctx, h, account.Spec.Secret, account.Namespace) + + if err != nil { + return fmt.Errorf("error loading secret %s for account %s", + account.Spec.Secret, + account.Name) + } + } + + _, errSecret := controllerutil.CreateOrPatch(ctx, h.GetClient(), accountSecret, func() error { + trueVal := true + + _, ok1 := accountSecret.Data[DatabasePasswordSelector] + _, ok2 := accountSecret.StringData[DatabasePasswordSelector] + if !ok1 && !ok2 { + err := fmt.Errorf("field %s not found in Secret %s", DatabasePasswordSelector, accountSecret.Name) + return err + } + + accountSecret.Immutable = &trueVal + + err := controllerutil.SetControllerReference(h.GetBeforeObject(), accountSecret, h.GetScheme()) + if err != nil { + return err + } + + // add calling CR finalizer to accountSecret first. controllers use + // 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, fmt.Sprintf("mariadb.openstack.org/%s", h.GetFinalizer())) + + return nil + }) + + if errSecret != nil { + return errSecret + } + + // then add calling CR finalizer to MariaDBAccount + controllerutil.AddFinalizer(account, h.GetFinalizer()) + + return nil + }) + + return opAcc, errAcc +} + // CreateOrPatchAccount creates/updates a given MariaDBAccount CR. +// deprecated; use CreateOrPatchAll func CreateOrPatchAccount( ctx context.Context, h *helper.Helper, @@ -844,22 +949,11 @@ func EnsureMariaDBAccount(ctx context.Context, DatabasePasswordSelector: dbPassword, }, } - - _, _, errSecret := secret.CreateOrPatchSecret( - ctx, - helper, - helper.GetBeforeObject(), - dbSecret, - ) - if errSecret != nil { - return nil, nil, errSecret - } - } - _, errAcc := CreateOrPatchAccount(ctx, helper, account, map[string]string{}) - if errAcc != nil { - return nil, nil, errAcc + _, err = createOrPatchAccountAndSecret(ctx, helper, account, dbSecret, map[string]string{}) + if err != nil { + return nil, nil, err } util.LogForObject(