Skip to content

Commit cd96520

Browse files
zzzeekzzzeek
committed
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.
1 parent 6b04e3e commit cd96520

File tree

2 files changed

+189
-20
lines changed

2 files changed

+189
-20
lines changed

‎api/test/helpers/harnesses.go

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,12 @@ func (harness *MariaDBTestHarness) RunBasicSuite() {
124124
return mariadbAccount.Finalizers
125125
}, timeout, interval).Should(ContainElement(harness.finalizerName))
126126

127+
// as well as in the secret
128+
Eventually(func() []string {
129+
dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: mariadbAccount.Spec.Secret, Namespace: mariadbAccount.Namespace})
130+
return dbSecret.Finalizers
131+
}, timeout, interval).Should(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName)))
132+
127133
// mariaDBDatabaseName is set
128134
Expect(mariadbAccount.Labels["mariaDBDatabaseName"]).Should(Equal(harness.databaseName))
129135

@@ -160,6 +166,12 @@ func (harness *MariaDBTestHarness) RunBasicSuite() {
160166
return mariadbAccount.Finalizers
161167
}, timeout, interval).Should(ContainElement(harness.finalizerName))
162168

169+
// as well as in the secret
170+
Eventually(func() []string {
171+
dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: mariadbAccount.Spec.Secret, Namespace: mariadbAccount.Namespace})
172+
return dbSecret.Finalizers
173+
}, timeout, interval).Should(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName)))
174+
163175
// mariaDBDatabaseName is set
164176
Expect(mariadbAccount.Labels["mariaDBDatabaseName"]).Should(Equal(harness.databaseName))
165177

@@ -194,6 +206,13 @@ func (harness *MariaDBTestHarness) RunBasicSuite() {
194206
return oldMariadbAccount.Finalizers
195207
}, timeout, interval).Should(ContainElement(harness.finalizerName))
196208

209+
// as well as in the secret
210+
Eventually(func() []string {
211+
oldMariadbAccount := mariaDBHelper.GetMariaDBAccount(oldAccountName)
212+
dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: oldMariadbAccount.Spec.Secret, Namespace: oldMariadbAccount.Namespace})
213+
return dbSecret.Finalizers
214+
}, timeout, interval).Should(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName)))
215+
197216
})
198217
It("should ensure a new MariaDBAccount exists when accountname is changed", func() {
199218
mariaDBHelper, timeout, interval := harness.mariaDBHelper, harness.timeout, harness.interval
@@ -228,12 +247,27 @@ func (harness *MariaDBTestHarness) RunBasicSuite() {
228247
return newMariadbAccount.Finalizers
229248
}, timeout, interval).Should(ContainElement(harness.finalizerName))
230249

250+
// as well as in the secret
251+
Eventually(func() []string {
252+
newMariadbAccount := mariaDBHelper.GetMariaDBAccount(newAccountName)
253+
dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: newMariadbAccount.Spec.Secret, Namespace: newMariadbAccount.Namespace})
254+
return dbSecret.Finalizers
255+
}, timeout, interval).Should(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName)))
256+
231257
// old account retains the finalizer because we did not yet
232258
// complete the new MariaDBAccount
233259
Consistently(func() []string {
234260
oldMariadbAccount := mariaDBHelper.GetMariaDBAccount(oldAccountName)
235261
return oldMariadbAccount.Finalizers
236262
}, timeout, interval).Should(ContainElement(harness.finalizerName))
263+
264+
// as well as in the secret
265+
Eventually(func() []string {
266+
oldMariadbAccount := mariaDBHelper.GetMariaDBAccount(oldAccountName)
267+
dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: oldMariadbAccount.Spec.Secret, Namespace: oldMariadbAccount.Namespace})
268+
return dbSecret.Finalizers
269+
}, timeout, interval).Should(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName)))
270+
237271
})
238272

239273
It("should move the finalizer to a new MariaDBAccount when create is complete", func() {
@@ -263,12 +297,26 @@ func (harness *MariaDBTestHarness) RunBasicSuite() {
263297
return newMariadbAccount.Finalizers
264298
}, timeout, interval).Should(ContainElement(harness.finalizerName))
265299

300+
// as well as in the secret
301+
Eventually(func() []string {
302+
newMariadbAccount := mariaDBHelper.GetMariaDBAccount(newAccountName)
303+
dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: newMariadbAccount.Spec.Secret, Namespace: newMariadbAccount.Namespace})
304+
return dbSecret.Finalizers
305+
}, timeout, interval).Should(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName)))
306+
266307
// finalizer removed from old account
267308
Eventually(func() []string {
268309
oldMariadbAccount := mariaDBHelper.GetMariaDBAccount(oldAccountName)
269310
return oldMariadbAccount.Finalizers
270311
}, timeout, interval).ShouldNot(ContainElement(harness.finalizerName))
271312

313+
// as well as in the secret
314+
Eventually(func() []string {
315+
oldMariadbAccount := mariaDBHelper.GetMariaDBAccount(oldAccountName)
316+
dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: oldMariadbAccount.Spec.Secret, Namespace: oldMariadbAccount.Namespace})
317+
return dbSecret.Finalizers
318+
}, timeout, interval).ShouldNot(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName)))
319+
272320
// CreateOrPatchDBByName will add a label referring to the database
273321
Eventually(func() string {
274322
mariadbAccount := mariaDBHelper.GetMariaDBAccount(newAccountName)
@@ -311,11 +359,25 @@ func (harness *MariaDBTestHarness) RunBasicSuite() {
311359
return newMariadbAccount.Finalizers
312360
}, timeout, interval).Should(ContainElement(harness.finalizerName))
313361

362+
// as well as in the secret
363+
Eventually(func() []string {
364+
newMariadbAccount := mariaDBHelper.GetMariaDBAccount(newAccountName)
365+
dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: newMariadbAccount.Spec.Secret, Namespace: newMariadbAccount.Namespace})
366+
return dbSecret.Finalizers
367+
}, timeout, interval).Should(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName)))
368+
314369
Eventually(func() []string {
315370
oldMariadbAccount := mariaDBHelper.GetMariaDBAccount(oldAccountName)
316371
return oldMariadbAccount.Finalizers
317372
}, timeout, interval).Should(ContainElement(harness.finalizerName))
318373

374+
// as well as in the secret
375+
Eventually(func() []string {
376+
oldMariadbAccount := mariaDBHelper.GetMariaDBAccount(oldAccountName)
377+
dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: oldMariadbAccount.Spec.Secret, Namespace: oldMariadbAccount.Namespace})
378+
return dbSecret.Finalizers
379+
}, timeout, interval).Should(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName)))
380+
319381
// now delete the CR
320382
harness.DeleteCR()
321383

@@ -331,6 +393,19 @@ func (harness *MariaDBTestHarness) RunBasicSuite() {
331393
return oldMariadbAccount.Finalizers
332394
}, timeout, interval).ShouldNot(ContainElement(harness.finalizerName))
333395

396+
// as well as in the secret
397+
Eventually(func() []string {
398+
oldMariadbAccount := mariaDBHelper.GetMariaDBAccount(oldAccountName)
399+
dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: oldMariadbAccount.Spec.Secret, Namespace: oldMariadbAccount.Namespace})
400+
return dbSecret.Finalizers
401+
}, timeout, interval).ShouldNot(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName)))
402+
403+
Eventually(func() []string {
404+
newMariadbAccount := mariaDBHelper.GetMariaDBAccount(newAccountName)
405+
dbSecret := harness.mariaDBHelper.GetSecret(types.NamespacedName{Name: newMariadbAccount.Spec.Secret, Namespace: newMariadbAccount.Namespace})
406+
return dbSecret.Finalizers
407+
}, timeout, interval).ShouldNot(ContainElement(fmt.Sprintf("mariadb.openstack.org/%s", harness.finalizerName)))
408+
334409
})
335410

336411
})

‎api/v1beta1/mariadbdatabase_funcs.go

Lines changed: 114 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -356,18 +356,18 @@ func (d *Database) CreateOrPatchAll(
356356
return ctrl.Result{RequeueAfter: time.Second * 5}, nil
357357
}
358358

359-
opAcc, errAcc := CreateOrPatchAccount(
360-
ctx, h, mariaDBAccount,
359+
opAcc, err := createOrPatchAccountAndSecret(
360+
ctx, h, mariaDBAccount, nil,
361361
map[string]string{
362362
"mariaDBDatabaseName": d.name,
363363
},
364364
)
365365

366-
if errAcc != nil {
366+
if err != nil {
367367
return ctrl.Result{}, util.WrapErrorForObject(
368368
fmt.Sprintf("Error creating or updating MariaDBAccount object %s", mariaDBAccount.Name),
369369
mariaDBAccount,
370-
errAcc,
370+
err,
371371
)
372372
}
373373

@@ -585,12 +585,21 @@ func (d *Database) DeleteFinalizer(
585585
h *helper.Helper,
586586
) error {
587587

588+
secretFinalizer := fmt.Sprintf("mariadb.openstack.org/%s", h.GetFinalizer())
589+
if d.secretObj != nil && controllerutil.RemoveFinalizer(d.secretObj, secretFinalizer) {
590+
err := h.GetClient().Update(ctx, d.secretObj)
591+
if err != nil && !k8s_errors.IsNotFound(err) {
592+
return err
593+
}
594+
util.LogForObject(h, fmt.Sprintf("Removed finalizer %s from Secret %s", secretFinalizer, d.secretObj.Name), d.secretObj)
595+
}
596+
588597
if d.account != nil && controllerutil.RemoveFinalizer(d.account, h.GetFinalizer()) {
589598
err := h.GetClient().Update(ctx, d.account)
590599
if err != nil && !k8s_errors.IsNotFound(err) {
591600
return err
592601
}
593-
util.LogForObject(h, fmt.Sprintf("Removed finalizer %s from MariaDBAccount object", h.GetFinalizer()), d.account)
602+
util.LogForObject(h, fmt.Sprintf("Removed finalizer %s from MariaDBAccount %s", h.GetFinalizer(), d.account.Name), d.account)
594603

595604
// also do a delete for "unused" MariaDBAccounts, associated with
596605
// this MariaDBDatabase.
@@ -678,21 +687,117 @@ func DeleteUnusedMariaDBAccountFinalizers(
678687
continue
679688
}
680689

690+
if mariaDBAccount.Spec.Secret != "" {
691+
dbSecret, _, err := secret.GetSecret(ctx, h, mariaDBAccount.Spec.Secret, namespace)
692+
if err != nil && !k8s_errors.IsNotFound(err) {
693+
return err
694+
}
695+
696+
secretFinalizer := fmt.Sprintf("mariadb.openstack.org/%s", h.GetFinalizer())
697+
if dbSecret != nil && controllerutil.RemoveFinalizer(dbSecret, secretFinalizer) {
698+
err := h.GetClient().Update(ctx, dbSecret)
699+
if err != nil && !k8s_errors.IsNotFound(err) {
700+
h.GetLogger().Error(
701+
err,
702+
fmt.Sprintf("Unable to remove finalizer %s from Secret %s", h.GetFinalizer(), dbSecret.Name))
703+
return err
704+
}
705+
util.LogForObject(h, fmt.Sprintf("Removed finalizer %s from Secret %s", h.GetFinalizer(), dbSecret.Name), dbSecret)
706+
}
707+
708+
}
709+
681710
if controllerutil.RemoveFinalizer(&mariaDBAccount, h.GetFinalizer()) {
682711
err := h.GetClient().Update(ctx, &mariaDBAccount)
683712
if err != nil && !k8s_errors.IsNotFound(err) {
684713
h.GetLogger().Error(err, fmt.Sprintf("Unable to remove finalizer %s from MariaDBAccount %s", h.GetFinalizer(), mariaDBAccount.Name))
685714
return err
686715
}
687-
util.LogForObject(h, fmt.Sprintf("Removed finalizer %s from MariaDBAccount", h.GetFinalizer()), &mariaDBAccount)
716+
util.LogForObject(h, fmt.Sprintf("Removed finalizer %s from MariaDBAccount %s", h.GetFinalizer(), mariaDBAccount.Name), &mariaDBAccount)
688717
}
689718

690719
}
691720
return nil
692721

693722
}
694723

724+
// createOrPatchAccountAndSecret creates/updates a given MariaDBAccount / Secret CR.
725+
func createOrPatchAccountAndSecret(
726+
ctx context.Context,
727+
h *helper.Helper,
728+
account *MariaDBAccount,
729+
accountSecret *corev1.Secret,
730+
labels map[string]string,
731+
) (controllerutil.OperationResult, error) {
732+
opAcc, errAcc := controllerutil.CreateOrPatch(ctx, h.GetClient(), account, func() error {
733+
account.Labels = util.MergeStringMaps(
734+
account.GetLabels(),
735+
labels,
736+
)
737+
738+
err := controllerutil.SetControllerReference(h.GetBeforeObject(), account, h.GetScheme())
739+
if err != nil {
740+
return err
741+
}
742+
743+
if account.Spec.UserName == "" {
744+
return fmt.Errorf("no UserName field in account %s", account.Name)
745+
}
746+
if account.Spec.Secret == "" {
747+
return fmt.Errorf("no secret field in account %s", account.Name)
748+
}
749+
750+
if accountSecret == nil {
751+
accountSecret, _, err = secret.GetSecret(ctx, h, account.Spec.Secret, account.Namespace)
752+
753+
if err != nil {
754+
return fmt.Errorf("error loading secret %s for account %s",
755+
account.Spec.Secret,
756+
account.Name)
757+
}
758+
}
759+
760+
_, errSecret := controllerutil.CreateOrPatch(ctx, h.GetClient(), accountSecret, func() error {
761+
trueVal := true
762+
763+
_, ok1 := accountSecret.Data[DatabasePasswordSelector]
764+
_, ok2 := accountSecret.StringData[DatabasePasswordSelector]
765+
if !ok1 && !ok2 {
766+
err := fmt.Errorf("field %s not found in Secret %s", DatabasePasswordSelector, accountSecret.Name)
767+
return err
768+
}
769+
770+
accountSecret.Immutable = &trueVal
771+
772+
err := controllerutil.SetControllerReference(h.GetBeforeObject(), accountSecret, h.GetScheme())
773+
if err != nil {
774+
return err
775+
}
776+
777+
// add calling CR finalizer to accountSecret first. controllers use
778+
// GetDatabaseByNameAndAccount to locate the Database which is how
779+
// they remove finalizers. this will return not found if secret
780+
// is not present, so finalizer will keep it around
781+
controllerutil.AddFinalizer(accountSecret, fmt.Sprintf("mariadb.openstack.org/%s", h.GetFinalizer()))
782+
783+
return nil
784+
})
785+
786+
if errSecret != nil {
787+
return errSecret
788+
}
789+
790+
// then add calling CR finalizer to MariaDBAccount
791+
controllerutil.AddFinalizer(account, h.GetFinalizer())
792+
793+
return nil
794+
})
795+
796+
return opAcc, errAcc
797+
}
798+
695799
// CreateOrPatchAccount creates/updates a given MariaDBAccount CR.
800+
// deprecated; use CreateOrPatchAll
696801
func CreateOrPatchAccount(
697802
ctx context.Context,
698803
h *helper.Helper,
@@ -844,22 +949,11 @@ func EnsureMariaDBAccount(ctx context.Context,
844949
DatabasePasswordSelector: dbPassword,
845950
},
846951
}
847-
848-
_, _, errSecret := secret.CreateOrPatchSecret(
849-
ctx,
850-
helper,
851-
helper.GetBeforeObject(),
852-
dbSecret,
853-
)
854-
if errSecret != nil {
855-
return nil, nil, errSecret
856-
}
857-
858952
}
859953

860-
_, errAcc := CreateOrPatchAccount(ctx, helper, account, map[string]string{})
861-
if errAcc != nil {
862-
return nil, nil, errAcc
954+
_, err = createOrPatchAccountAndSecret(ctx, helper, account, dbSecret, map[string]string{})
955+
if err != nil {
956+
return nil, nil, err
863957
}
864958

865959
util.LogForObject(

0 commit comments

Comments
 (0)