Skip to content

Commit 31acaf7

Browse files
mateigrigoremateigrigore
authored
CLOUDP-196371: Delete users removed from the resource (mongodb#1587)
1 parent 35e9ae1 commit 31acaf7

File tree

12 files changed

+486
-22
lines changed

12 files changed

+486
-22
lines changed

‎.action_templates/jobs/tests.yaml

+2
Original file line numberDiff line numberDiff line change
@@ -62,3 +62,5 @@ tests:
6262
distro: ubi
6363
- test-name: replica_set_x509
6464
distro: ubi
65+
- test-name: replica_set_remove_user
66+
distro: ubi

‎.github/workflows/e2e-fork.yml

+2
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,8 @@ jobs:
147147
distro: ubi
148148
- test-name: replica_set_x509
149149
distro: ubi
150+
- test-name: replica_set_remove_user
151+
distro: ubi
150152
steps:
151153
# template: .action_templates/steps/cancel-previous.yaml
152154
- name: Cancel Previous Runs

‎.github/workflows/e2e.yml

+2
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,8 @@ jobs:
153153
distro: ubi
154154
- test-name: replica_set_x509
155155
distro: ubi
156+
- test-name: replica_set_remove_user
157+
distro: ubi
156158
steps:
157159
# template: .action_templates/steps/cancel-previous.yaml
158160
- name: Cancel Previous Runs

‎controllers/mongodb_cleanup.go

+57-10
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,12 @@ import (
1010
)
1111

1212
// cleanupPemSecret cleans up the old pem secret generated for the agent certificate.
13-
func (r *ReplicaSetReconciler) cleanupPemSecret(ctx context.Context, currentMDB mdbv1.MongoDBCommunitySpec, lastAppliedMDBSpec mdbv1.MongoDBCommunitySpec, namespace string) {
14-
if currentMDB.GetAgentAuthMode() == lastAppliedMDBSpec.GetAgentAuthMode() {
13+
func (r *ReplicaSetReconciler) cleanupPemSecret(ctx context.Context, currentMDBSpec mdbv1.MongoDBCommunitySpec, lastAppliedMDBSpec mdbv1.MongoDBCommunitySpec, namespace string) {
14+
if currentMDBSpec.GetAgentAuthMode() == lastAppliedMDBSpec.GetAgentAuthMode() {
1515
return
1616
}
1717

18-
if !currentMDB.IsAgentX509() && lastAppliedMDBSpec.IsAgentX509() {
18+
if !currentMDBSpec.IsAgentX509() && lastAppliedMDBSpec.IsAgentX509() {
1919
agentCertSecret := lastAppliedMDBSpec.GetAgentCertificateRef()
2020
if err := r.client.DeleteSecret(ctx, types.NamespacedName{
2121
Namespace: namespace,
@@ -31,33 +31,50 @@ func (r *ReplicaSetReconciler) cleanupPemSecret(ctx context.Context, currentMDB
3131
}
3232

3333
// cleanupScramSecrets cleans up old scram secrets based on the last successful applied mongodb spec.
34-
func (r *ReplicaSetReconciler) cleanupScramSecrets(ctx context.Context, currentMDB mdbv1.MongoDBCommunitySpec, lastAppliedMDBSpec mdbv1.MongoDBCommunitySpec, namespace string) {
35-
secretsToDelete := getScramSecretsToDelete(currentMDB, lastAppliedMDBSpec)
34+
func (r *ReplicaSetReconciler) cleanupScramSecrets(ctx context.Context, currentMDBSpec mdbv1.MongoDBCommunitySpec, lastAppliedMDBSpec mdbv1.MongoDBCommunitySpec, namespace string) {
35+
secretsToDelete := getScramSecretsToDelete(currentMDBSpec, lastAppliedMDBSpec)
3636

3737
for _, s := range secretsToDelete {
3838
if err := r.client.DeleteSecret(ctx, types.NamespacedName{
3939
Name: s,
4040
Namespace: namespace,
4141
}); err != nil {
42-
r.log.Warnf("Could not cleanup old secret %s", s)
42+
r.log.Warnf("Could not cleanup old secret %s: %s", s, err)
4343
} else {
4444
r.log.Debugf("Sucessfully cleaned up secret: %s", s)
4545
}
4646
}
4747
}
4848

49-
func getScramSecretsToDelete(currentMDB mdbv1.MongoDBCommunitySpec, lastAppliedMDBSpec mdbv1.MongoDBCommunitySpec) []string {
49+
// cleanupConnectionStringSecrets cleans up old scram secrets based on the last successful applied mongodb spec.
50+
func (r *ReplicaSetReconciler) cleanupConnectionStringSecrets(ctx context.Context, currentMDBSpec mdbv1.MongoDBCommunitySpec, lastAppliedMDBSpec mdbv1.MongoDBCommunitySpec, namespace string, resourceName string) {
51+
secretsToDelete := getConnectionStringSecretsToDelete(currentMDBSpec, lastAppliedMDBSpec, resourceName)
52+
53+
for _, s := range secretsToDelete {
54+
if err := r.client.DeleteSecret(ctx, types.NamespacedName{
55+
Name: s,
56+
Namespace: namespace,
57+
}); err != nil {
58+
r.log.Warnf("Could not cleanup old secret %s: %s", s, err)
59+
} else {
60+
r.log.Debugf("Sucessfully cleaned up secret: %s", s)
61+
}
62+
}
63+
}
64+
65+
func getScramSecretsToDelete(currentMDBSpec mdbv1.MongoDBCommunitySpec, lastAppliedMDBSpec mdbv1.MongoDBCommunitySpec) []string {
5066
type user struct {
5167
db string
5268
name string
5369
}
5470
m := map[user]string{}
5571
var secretsToDelete []string
5672

57-
for _, mongoDBUser := range currentMDB.Users {
58-
if mongoDBUser.DB != constants.ExternalDB {
59-
m[user{db: mongoDBUser.DB, name: mongoDBUser.Name}] = mongoDBUser.GetScramCredentialsSecretName()
73+
for _, mongoDBUser := range currentMDBSpec.Users {
74+
if mongoDBUser.DB == constants.ExternalDB {
75+
continue
6076
}
77+
m[user{db: mongoDBUser.DB, name: mongoDBUser.Name}] = mongoDBUser.GetScramCredentialsSecretName()
6178
}
6279

6380
for _, mongoDBUser := range lastAppliedMDBSpec.Users {
@@ -73,3 +90,33 @@ func getScramSecretsToDelete(currentMDB mdbv1.MongoDBCommunitySpec, lastAppliedM
7390
}
7491
return secretsToDelete
7592
}
93+
94+
func getConnectionStringSecretsToDelete(currentMDBSpec mdbv1.MongoDBCommunitySpec, lastAppliedMDBSpec mdbv1.MongoDBCommunitySpec, resourceName string) []string {
95+
type user struct {
96+
db string
97+
name string
98+
}
99+
m := map[user]string{}
100+
var secretsToDelete []string
101+
102+
for _, mongoDBUser := range currentMDBSpec.Users {
103+
if mongoDBUser.DB == constants.ExternalDB {
104+
continue
105+
}
106+
m[user{db: mongoDBUser.DB, name: mongoDBUser.Name}] = mongoDBUser.GetConnectionStringSecretName(resourceName)
107+
}
108+
109+
for _, mongoDBUser := range lastAppliedMDBSpec.Users {
110+
if mongoDBUser.DB == constants.ExternalDB {
111+
continue
112+
}
113+
currentConnectionStringSecretName, ok := m[user{db: mongoDBUser.DB, name: mongoDBUser.Name}]
114+
if !ok { // user was removed
115+
secretsToDelete = append(secretsToDelete, mongoDBUser.GetConnectionStringSecretName(resourceName))
116+
} else if currentConnectionStringSecretName != mongoDBUser.GetConnectionStringSecretName(resourceName) {
117+
// this happens when a new ConnectionStringSecretName was set for the old user
118+
secretsToDelete = append(secretsToDelete, mongoDBUser.GetConnectionStringSecretName(resourceName))
119+
}
120+
}
121+
return secretsToDelete
122+
}

‎controllers/mongodb_cleanup_test.go

+89-4
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,7 @@ func TestReplicaSetReconcilerCleanupScramSecrets(t *testing.T) {
2222
t.Run("no change same resource", func(t *testing.T) {
2323
actual := getScramSecretsToDelete(lastApplied.Spec, lastApplied.Spec)
2424

25-
var expected []string
26-
assert.Equal(t, expected, actual)
25+
assert.Equal(t, []string(nil), actual)
2726
})
2827

2928
t.Run("new user new secret", func(t *testing.T) {
@@ -44,10 +43,9 @@ func TestReplicaSetReconcilerCleanupScramSecrets(t *testing.T) {
4443
},
4544
)
4645

47-
var expected []string
4846
actual := getScramSecretsToDelete(current.Spec, lastApplied.Spec)
4947

50-
assert.Equal(t, expected, actual)
48+
assert.Equal(t, []string(nil), actual)
5149
})
5250

5351
t.Run("old user new secret", func(t *testing.T) {
@@ -154,3 +152,90 @@ func TestReplicaSetReconcilerCleanupPemSecret(t *testing.T) {
154152
_, err = r.client.GetSecret(ctx, mdb.AgentCertificatePemSecretNamespacedName())
155153
assert.Error(t, err)
156154
}
155+
156+
func TestReplicaSetReconcilerCleanupConnectionStringSecrets(t *testing.T) {
157+
lastApplied := newScramReplicaSet(mdbv1.MongoDBUser{
158+
Name: "testUser",
159+
PasswordSecretRef: mdbv1.SecretKeyReference{
160+
Name: "password-secret-name",
161+
},
162+
ConnectionStringSecretName: "connection-string-secret",
163+
})
164+
165+
t.Run("no change same resource", func(t *testing.T) {
166+
actual := getConnectionStringSecretsToDelete(lastApplied.Spec, lastApplied.Spec, "my-rs")
167+
168+
assert.Equal(t, []string(nil), actual)
169+
})
170+
171+
t.Run("new user does not require existing user cleanup", func(t *testing.T) {
172+
current := newScramReplicaSet(
173+
mdbv1.MongoDBUser{
174+
Name: "testUser",
175+
PasswordSecretRef: mdbv1.SecretKeyReference{
176+
Name: "password-secret-name",
177+
},
178+
ConnectionStringSecretName: "connection-string-secret",
179+
},
180+
mdbv1.MongoDBUser{
181+
Name: "newUser",
182+
PasswordSecretRef: mdbv1.SecretKeyReference{
183+
Name: "password-secret-name",
184+
},
185+
ConnectionStringSecretName: "connection-string-secret-2",
186+
},
187+
)
188+
189+
actual := getConnectionStringSecretsToDelete(current.Spec, lastApplied.Spec, "my-rs")
190+
191+
assert.Equal(t, []string(nil), actual)
192+
})
193+
194+
t.Run("old user new secret", func(t *testing.T) {
195+
current := newScramReplicaSet(mdbv1.MongoDBUser{
196+
Name: "testUser",
197+
PasswordSecretRef: mdbv1.SecretKeyReference{
198+
Name: "password-secret-name",
199+
},
200+
ConnectionStringSecretName: "connection-string-secret-2",
201+
})
202+
203+
expected := []string{"connection-string-secret"}
204+
actual := getConnectionStringSecretsToDelete(current.Spec, lastApplied.Spec, "my-rs")
205+
206+
assert.Equal(t, expected, actual)
207+
})
208+
209+
t.Run("removed one user and changed secret of the other", func(t *testing.T) {
210+
lastApplied = newScramReplicaSet(
211+
mdbv1.MongoDBUser{
212+
Name: "testUser",
213+
PasswordSecretRef: mdbv1.SecretKeyReference{
214+
Name: "password-secret-name",
215+
},
216+
ConnectionStringSecretName: "connection-string-secret",
217+
},
218+
mdbv1.MongoDBUser{
219+
Name: "anotherUser",
220+
PasswordSecretRef: mdbv1.SecretKeyReference{
221+
Name: "password-secret-name",
222+
},
223+
ConnectionStringSecretName: "connection-string-secret-2",
224+
},
225+
)
226+
227+
current := newScramReplicaSet(mdbv1.MongoDBUser{
228+
Name: "testUser",
229+
PasswordSecretRef: mdbv1.SecretKeyReference{
230+
Name: "password-secret-name",
231+
},
232+
ConnectionStringSecretName: "connection-string-secret-1",
233+
})
234+
235+
expected := []string{"connection-string-secret", "connection-string-secret-2"}
236+
actual := getConnectionStringSecretsToDelete(current.Spec, lastApplied.Spec, "my-rs")
237+
238+
assert.Equal(t, expected, actual)
239+
})
240+
241+
}

‎controllers/mongodb_users.go

+3
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,9 @@ func (r ReplicaSetReconciler) updateConnectionStringSecrets(ctx context.Context,
8282
if err := secret.CreateOrUpdate(ctx, r.client, connectionStringSecret); err != nil {
8383
return err
8484
}
85+
86+
secretNamespacedName := types.NamespacedName{Name: connectionStringSecret.Name, Namespace: connectionStringSecret.Namespace}
87+
r.secretWatcher.Watch(ctx, secretNamespacedName, mdb.NamespacedName())
8588
}
8689

8790
return nil

‎controllers/replica_set_controller.go

+13-8
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ func (r ReplicaSetReconciler) Reconcile(ctx context.Context, request reconcile.R
173173
withFailedPhase())
174174
}
175175

176-
ready, err := r.deployMongoDBReplicaSet(ctx, mdb)
176+
ready, err := r.deployMongoDBReplicaSet(ctx, mdb, lastAppliedSpec)
177177
if err != nil {
178178
return status.Update(ctx, r.client.Status(), &mdb, statusOptions().
179179
withMessage(Error, fmt.Sprintf("Error deploying MongoDB ReplicaSet: %s", err)).
@@ -225,6 +225,7 @@ func (r ReplicaSetReconciler) Reconcile(ctx context.Context, request reconcile.R
225225
if lastAppliedSpec != nil {
226226
r.cleanupScramSecrets(ctx, mdb.Spec, *lastAppliedSpec, mdb.Namespace)
227227
r.cleanupPemSecret(ctx, mdb.Spec, *lastAppliedSpec, mdb.Namespace)
228+
r.cleanupConnectionStringSecrets(ctx, mdb.Spec, *lastAppliedSpec, mdb.Namespace, mdb.Name)
228229
}
229230

230231
if err := r.updateLastSuccessfulConfiguration(ctx, mdb); err != nil {
@@ -331,15 +332,15 @@ func (r *ReplicaSetReconciler) deployStatefulSet(ctx context.Context, mdb mdbv1.
331332

332333
// deployAutomationConfig deploys the AutomationConfig for the MongoDBCommunity resource.
333334
// The returned boolean indicates whether or not that Agents have all reached goal state.
334-
func (r *ReplicaSetReconciler) deployAutomationConfig(ctx context.Context, mdb mdbv1.MongoDBCommunity) (bool, error) {
335+
func (r *ReplicaSetReconciler) deployAutomationConfig(ctx context.Context, mdb mdbv1.MongoDBCommunity, lastAppliedSpec *mdbv1.MongoDBCommunitySpec) (bool, error) {
335336
r.log.Infof("Creating/Updating AutomationConfig")
336337

337338
sts, err := r.client.GetStatefulSet(ctx, mdb.NamespacedName())
338339
if err != nil && !apiErrors.IsNotFound(err) {
339340
return false, fmt.Errorf("failed to get StatefulSet: %s", err)
340341
}
341342

342-
ac, err := r.ensureAutomationConfig(mdb, ctx)
343+
ac, err := r.ensureAutomationConfig(mdb, ctx, lastAppliedSpec)
343344
if err != nil {
344345
return false, fmt.Errorf("failed to ensure AutomationConfig: %s", err)
345346
}
@@ -408,10 +409,10 @@ func (r *ReplicaSetReconciler) shouldRunInOrder(ctx context.Context, mdb mdbv1.M
408409
// deployMongoDBReplicaSet will ensure that both the AutomationConfig secret and backing StatefulSet
409410
// have been successfully created. A boolean is returned indicating if the process is complete
410411
// and an error if there was one.
411-
func (r *ReplicaSetReconciler) deployMongoDBReplicaSet(ctx context.Context, mdb mdbv1.MongoDBCommunity) (bool, error) {
412+
func (r *ReplicaSetReconciler) deployMongoDBReplicaSet(ctx context.Context, mdb mdbv1.MongoDBCommunity, lastAppliedSpec *mdbv1.MongoDBCommunitySpec) (bool, error) {
412413
return functions.RunSequentially(r.shouldRunInOrder(ctx, mdb),
413414
func() (bool, error) {
414-
return r.deployAutomationConfig(ctx, mdb)
415+
return r.deployAutomationConfig(ctx, mdb, lastAppliedSpec)
415416
},
416417
func() (bool, error) {
417418
return r.deployStatefulSet(ctx, mdb)
@@ -489,8 +490,8 @@ func (r *ReplicaSetReconciler) createOrUpdateStatefulSet(ctx context.Context, md
489490

490491
// ensureAutomationConfig makes sure the AutomationConfig secret has been successfully created. The automation config
491492
// that was updated/created is returned.
492-
func (r ReplicaSetReconciler) ensureAutomationConfig(mdb mdbv1.MongoDBCommunity, ctx context.Context) (automationconfig.AutomationConfig, error) {
493-
ac, err := r.buildAutomationConfig(ctx, mdb)
493+
func (r ReplicaSetReconciler) ensureAutomationConfig(mdb mdbv1.MongoDBCommunity, ctx context.Context, lastAppliedSpec *mdbv1.MongoDBCommunitySpec) (automationconfig.AutomationConfig, error) {
494+
ac, err := r.buildAutomationConfig(ctx, mdb, lastAppliedSpec)
494495
if err != nil {
495496
return automationconfig.AutomationConfig{}, fmt.Errorf("could not build automation config: %s", err)
496497
}
@@ -622,7 +623,7 @@ func getCustomRolesModification(mdb mdbv1.MongoDBCommunity) (automationconfig.Mo
622623
}, nil
623624
}
624625

625-
func (r ReplicaSetReconciler) buildAutomationConfig(ctx context.Context, mdb mdbv1.MongoDBCommunity) (automationconfig.AutomationConfig, error) {
626+
func (r ReplicaSetReconciler) buildAutomationConfig(ctx context.Context, mdb mdbv1.MongoDBCommunity, lastAppliedSpec *mdbv1.MongoDBCommunitySpec) (automationconfig.AutomationConfig, error) {
626627
tlsModification, err := getTLSConfigModification(ctx, r.client, r.client, mdb)
627628
if err != nil {
628629
return automationconfig.AutomationConfig{}, fmt.Errorf("could not configure TLS modification: %s", err)
@@ -643,6 +644,10 @@ func (r ReplicaSetReconciler) buildAutomationConfig(ctx context.Context, mdb mdb
643644
return automationconfig.AutomationConfig{}, err
644645
}
645646

647+
if lastAppliedSpec != nil {
648+
authentication.AddRemovedUsers(&auth, mdb, lastAppliedSpec)
649+
}
650+
646651
prometheusModification := automationconfig.NOOP()
647652
if mdb.Spec.Prometheus != nil {
648653
secretNamespacedName := types.NamespacedName{Name: mdb.Spec.Prometheus.PasswordSecretRef.Name, Namespace: mdb.Namespace}

‎pkg/authentication/authentication.go

+34
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package authentication
33
import (
44
"context"
55
"fmt"
6+
mdbv1 "github.com/mongodb/mongodb-kubernetes-operator/api/v1"
67

78
"k8s.io/apimachinery/pkg/types"
89

@@ -33,3 +34,36 @@ func Enable(ctx context.Context, auth *automationconfig.Auth, secretGetUpdateCre
3334
}
3435
return nil
3536
}
37+
38+
func AddRemovedUsers(auth *automationconfig.Auth, mdb mdbv1.MongoDBCommunity, lastAppliedSpec *mdbv1.MongoDBCommunitySpec) {
39+
deletedUsers := getRemovedUsersFromSpec(mdb.Spec, lastAppliedSpec)
40+
41+
auth.UsersDeleted = append(auth.UsersDeleted, deletedUsers...)
42+
}
43+
44+
func getRemovedUsersFromSpec(currentMDB mdbv1.MongoDBCommunitySpec, lastAppliedMDBSpec *mdbv1.MongoDBCommunitySpec) []automationconfig.DeletedUser {
45+
type user struct {
46+
db string
47+
name string
48+
}
49+
m := map[user]bool{}
50+
var deletedUsers []automationconfig.DeletedUser
51+
52+
for _, mongoDBUser := range currentMDB.Users {
53+
if mongoDBUser.DB == constants.ExternalDB {
54+
continue
55+
}
56+
m[user{db: mongoDBUser.DB, name: mongoDBUser.Name}] = true
57+
}
58+
59+
for _, mongoDBUser := range lastAppliedMDBSpec.Users {
60+
if mongoDBUser.DB == constants.ExternalDB {
61+
continue
62+
}
63+
_, ok := m[user{db: mongoDBUser.DB, name: mongoDBUser.Name}]
64+
if !ok {
65+
deletedUsers = append(deletedUsers, automationconfig.DeletedUser{User: mongoDBUser.Name, Dbs: []string{mongoDBUser.DB}})
66+
}
67+
}
68+
return deletedUsers
69+
}

0 commit comments

Comments
 (0)