Skip to content

Commit 1911189

Browse files
gkechhors
gkech
and
hors
authored
K8SPSMDB-1610 handle user updates when no changes happen (#2435)
* K8SPSMDB-1610 handle user updates when no changes happen * cr: address security comment * cr: add check for empty password * cr: add const admin * cr: adminDBName --------- Co-authored-by: Viacheslav Sarzhan <slava.sarzhan@percona.com>
1 parent 87cc227 commit 1911189

2 files changed

Lines changed: 184 additions & 48 deletions

File tree

‎pkg/controller/perconaservermongodbclustersync/psmdb_clustersync_controller_test.go‎

Lines changed: 106 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@ import (
66
"testing"
77
"time"
88

9-
"github.com/percona/percona-server-mongodb-operator/pkg/psmdb"
109
"github.com/percona/percona-server-mongodb-operator/pkg/psmdb/clustersync/client"
1110
"github.com/percona/percona-server-mongodb-operator/pkg/psmdb/mongo"
1211
"github.com/stretchr/testify/assert"
1312
"github.com/stretchr/testify/require"
13+
corev1 "k8s.io/api/core/v1"
1414
"k8s.io/apimachinery/pkg/api/meta"
1515
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1616
"k8s.io/apimachinery/pkg/types"
@@ -351,52 +351,139 @@ func (m *recordingMongoClient) UpdateUserRoles(_ context.Context, _, _ string, r
351351
}
352352

353353
func TestEnsureTargetMongoUser(t *testing.T) {
354-
creds := psmdb.Credentials{Username: "clustersync-cr", Password: "new-password"}
354+
const (
355+
username = "clustersync-cr"
356+
password = "new-password"
357+
)
358+
passwordMAC := passwordAnnotationMAC([]byte(password))
359+
360+
secretWith := func(annotations map[string]string) *corev1.Secret {
361+
return &corev1.Secret{
362+
ObjectMeta: metav1.ObjectMeta{Annotations: annotations},
363+
Data: map[string][]byte{
364+
targetUserSecretUsernameKey: []byte(username),
365+
targetUserSecretPasswordKey: []byte(password),
366+
},
367+
}
368+
}
369+
existingWithDesiredRoles := &mongo.User{DB: "admin", Roles: append([]mongo.Role(nil), syncTargetUserRoles...)}
355370

356371
tests := map[string]struct {
357-
mc *recordingMongoClient
358-
wantErr string
359-
assert func(t *testing.T, m *recordingMongoClient)
372+
mc *recordingMongoClient
373+
sec *corev1.Secret
374+
wantErr string
375+
wantMutated bool
376+
assert func(t *testing.T, m *recordingMongoClient, s *corev1.Secret)
360377
}{
361-
"missing user is created with secret password": {
362-
mc: &recordingMongoClient{getUserInfoResp: nil},
363-
assert: func(t *testing.T, m *recordingMongoClient) {
378+
"missing user is created and password hash annotation is set": {
379+
mc: &recordingMongoClient{getUserInfoResp: nil},
380+
sec: secretWith(nil),
381+
wantMutated: true,
382+
assert: func(t *testing.T, m *recordingMongoClient, s *corev1.Secret) {
364383
assert.Equal(t, 1, m.createUserCalls)
365-
assert.Equal(t, "new-password", m.lastCreatePass)
384+
assert.Equal(t, password, m.lastCreatePass)
366385
assert.Zero(t, m.updateUserPassCalls)
367386
assert.Zero(t, m.updateRolesCalls)
387+
assert.Equal(t, passwordMAC, s.Annotations[targetUserPasswordMACAnnotation])
368388
},
369389
},
370-
"existing user has password and roles re-applied": {
371-
mc: &recordingMongoClient{getUserInfoResp: &mongo.User{DB: "admin"}},
372-
assert: func(t *testing.T, m *recordingMongoClient) {
390+
"steady state converged does no writes": {
391+
mc: &recordingMongoClient{getUserInfoResp: existingWithDesiredRoles},
392+
sec: secretWith(map[string]string{targetUserPasswordMACAnnotation: passwordMAC}),
393+
wantMutated: false,
394+
assert: func(t *testing.T, m *recordingMongoClient, s *corev1.Secret) {
373395
assert.Zero(t, m.createUserCalls)
396+
assert.Zero(t, m.updateUserPassCalls)
397+
assert.Zero(t, m.updateRolesCalls)
398+
},
399+
},
400+
"password hash mismatch triggers UpdateUserPass and annotation refresh": {
401+
mc: &recordingMongoClient{getUserInfoResp: existingWithDesiredRoles},
402+
sec: secretWith(map[string]string{targetUserPasswordMACAnnotation: "stale"}),
403+
wantMutated: true,
404+
assert: func(t *testing.T, m *recordingMongoClient, s *corev1.Secret) {
374405
assert.Equal(t, 1, m.updateUserPassCalls)
375-
assert.Equal(t, "new-password", m.lastUpdatePass)
406+
assert.Equal(t, password, m.lastUpdatePass)
407+
assert.Zero(t, m.updateRolesCalls)
408+
assert.Equal(t, passwordMAC, s.Annotations[targetUserPasswordMACAnnotation])
409+
},
410+
},
411+
"recreated secret with existing mongo user triggers UpdateUserPass and creates annotation map": {
412+
mc: &recordingMongoClient{getUserInfoResp: existingWithDesiredRoles},
413+
sec: secretWith(nil),
414+
wantMutated: true,
415+
assert: func(t *testing.T, m *recordingMongoClient, s *corev1.Secret) {
416+
assert.Equal(t, 1, m.updateUserPassCalls)
417+
assert.Equal(t, password, m.lastUpdatePass)
418+
assert.Zero(t, m.updateRolesCalls)
419+
require.NotNil(t, s.Annotations)
420+
assert.Equal(t, passwordMAC, s.Annotations[targetUserPasswordMACAnnotation])
421+
},
422+
},
423+
"new cr reconcile against stale mongo user with drifted roles applies both password and roles": {
424+
mc: &recordingMongoClient{
425+
getUserInfoResp: &mongo.User{DB: "admin", Roles: []mongo.Role{{Role: "clusterMonitor", DB: "admin"}}},
426+
},
427+
sec: secretWith(nil),
428+
wantMutated: true,
429+
assert: func(t *testing.T, m *recordingMongoClient, s *corev1.Secret) {
430+
assert.Equal(t, 1, m.updateUserPassCalls)
431+
assert.Equal(t, password, m.lastUpdatePass)
432+
assert.Equal(t, 1, m.updateRolesCalls)
433+
assert.Equal(t, syncTargetUserRoles, m.lastUpdateRoles)
434+
assert.Equal(t, passwordMAC, s.Annotations[targetUserPasswordMACAnnotation])
435+
},
436+
},
437+
"role drift triggers UpdateUserRoles without touching password": {
438+
mc: &recordingMongoClient{
439+
getUserInfoResp: &mongo.User{DB: "admin", Roles: []mongo.Role{{Role: "clusterMonitor", DB: "admin"}}},
440+
},
441+
sec: secretWith(map[string]string{targetUserPasswordMACAnnotation: passwordMAC}),
442+
wantMutated: false,
443+
assert: func(t *testing.T, m *recordingMongoClient, s *corev1.Secret) {
444+
assert.Zero(t, m.updateUserPassCalls)
376445
assert.Equal(t, 1, m.updateRolesCalls)
377446
assert.Equal(t, syncTargetUserRoles, m.lastUpdateRoles)
378447
},
379448
},
449+
"roles in a different order are treated as equal": {
450+
mc: &recordingMongoClient{
451+
getUserInfoResp: &mongo.User{DB: "admin", Roles: []mongo.Role{
452+
{Role: "readWriteAnyDatabase", DB: "admin"},
453+
{Role: "clusterManager", DB: "admin"},
454+
{Role: "clusterMonitor", DB: "admin"},
455+
{Role: "restore", DB: "admin"},
456+
}},
457+
},
458+
sec: secretWith(map[string]string{targetUserPasswordMACAnnotation: passwordMAC}),
459+
wantMutated: false,
460+
assert: func(t *testing.T, m *recordingMongoClient, s *corev1.Secret) {
461+
assert.Zero(t, m.updateRolesCalls)
462+
},
463+
},
380464
"GetUserInfo error short-circuits with no writes": {
381465
mc: &recordingMongoClient{getUserInfoErr: errors.New("error")},
466+
sec: secretWith(nil),
382467
wantErr: "look up target user",
383-
assert: func(t *testing.T, m *recordingMongoClient) {
468+
assert: func(t *testing.T, m *recordingMongoClient, s *corev1.Secret) {
384469
assert.Zero(t, m.createUserCalls)
385470
assert.Zero(t, m.updateUserPassCalls)
386471
assert.Zero(t, m.updateRolesCalls)
387472
},
388473
},
389474
"CreateUser error is wrapped": {
390475
mc: &recordingMongoClient{getUserInfoResp: nil, createUserErr: errors.New("error")},
476+
sec: secretWith(nil),
391477
wantErr: "create target user",
392478
},
393479
"UpdateUserPass error stops before UpdateUserRoles": {
394480
mc: &recordingMongoClient{
395481
getUserInfoResp: &mongo.User{DB: "admin"},
396482
updateUserPassErr: errors.New("error"),
397483
},
484+
sec: secretWith(nil),
398485
wantErr: "sync target user password",
399-
assert: func(t *testing.T, m *recordingMongoClient) {
486+
assert: func(t *testing.T, m *recordingMongoClient, s *corev1.Secret) {
400487
assert.Zero(t, m.updateRolesCalls)
401488
},
402489
},
@@ -405,21 +492,23 @@ func TestEnsureTargetMongoUser(t *testing.T) {
405492
getUserInfoResp: &mongo.User{DB: "admin"},
406493
updateRolesErr: errors.New("error"),
407494
},
495+
sec: secretWith(map[string]string{targetUserPasswordMACAnnotation: passwordMAC}),
408496
wantErr: "sync target user roles",
409497
},
410498
}
411499

412500
for name, tc := range tests {
413501
t.Run(name, func(t *testing.T) {
414-
err := ensureTargetMongoUser(t.Context(), tc.mc, creds)
502+
mutated, err := ensureTargetMongoUser(t.Context(), tc.mc, tc.sec)
415503
if tc.wantErr != "" {
416504
require.Error(t, err)
417505
assert.Contains(t, err.Error(), tc.wantErr)
418506
} else {
419507
require.NoError(t, err)
508+
assert.Equal(t, tc.wantMutated, mutated)
420509
}
421510
if tc.assert != nil {
422-
tc.assert(t, tc.mc)
511+
tc.assert(t, tc.mc, tc.sec)
423512
}
424513
})
425514
}

‎pkg/controller/perconaservermongodbclustersync/target_user.go‎

Lines changed: 78 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,13 @@ package perconaservermongodbclustersync
22

33
import (
44
"context"
5+
"crypto/hmac"
6+
"crypto/sha256"
7+
"encoding/hex"
58
"fmt"
69

10+
"github.com/google/go-cmp/cmp"
11+
"github.com/google/go-cmp/cmp/cmpopts"
712
"github.com/pkg/errors"
813
corev1 "k8s.io/api/core/v1"
914
k8serrors "k8s.io/apimachinery/pkg/api/errors"
@@ -23,6 +28,11 @@ import (
2328
const (
2429
targetUserSecretUsernameKey = "username"
2530
targetUserSecretPasswordKey = "password"
31+
32+
adminDBName = "admin"
33+
34+
targetUserPasswordMACAnnotation = "psmdb.percona.com/target-user-password-mac"
35+
targetUserPasswordMACKey = "psmdb-target-user-password-annotation-v1"
2636
)
2737

2838
var syncTargetUserRoles = []mongo.Role{
@@ -32,6 +42,13 @@ var syncTargetUserRoles = []mongo.Role{
3242
{Role: "readWriteAnyDatabase", DB: "admin"},
3343
}
3444

45+
var sortRolesOpt = cmpopts.SortSlices(func(a, b mongo.Role) bool {
46+
if a.DB != b.DB {
47+
return a.DB < b.DB
48+
}
49+
return a.Role < b.Role
50+
})
51+
3552
// Scoped to the CR name so a new CR after a finalized one gets its own user.
3653
func syncTargetUsername(cr *psmdbv1.PerconaServerMongoDBClusterSync) string {
3754
return fmt.Sprintf("clustersync-%s", cr.Name)
@@ -45,7 +62,7 @@ func (r *ReconcilePerconaServerMongoDBClusterSync) ensureSyncTargetUser(
4562
target *psmdbv1.PerconaServerMongoDB,
4663
) (psmdb.Credentials, error) {
4764
log := logf.FromContext(ctx)
48-
creds, err := r.ensureTargetUserSecret(ctx, cr)
65+
sec, err := r.ensureTargetUserSecret(ctx, cr)
4966
if err != nil {
5067
return psmdb.Credentials{}, errors.Wrap(err, "ensure target user secret")
5168
}
@@ -61,35 +78,44 @@ func (r *ReconcilePerconaServerMongoDBClusterSync) ensureSyncTargetUser(
6178
}
6279
}(mc, ctx)
6380

64-
if err := ensureTargetMongoUser(ctx, mc, creds); err != nil {
81+
mutated, err := ensureTargetMongoUser(ctx, mc, sec)
82+
if err != nil {
6583
return psmdb.Credentials{}, errors.Wrap(err, "ensure target mongo user")
6684
}
67-
return creds, nil
85+
if mutated {
86+
if err := r.client.Update(ctx, sec); err != nil {
87+
return psmdb.Credentials{}, errors.Wrap(err, "update target user secret annotations")
88+
}
89+
}
90+
return psmdb.Credentials{
91+
Username: string(sec.Data[targetUserSecretUsernameKey]),
92+
Password: string(sec.Data[targetUserSecretPasswordKey]),
93+
}, nil
6894
}
6995

7096
func (r *ReconcilePerconaServerMongoDBClusterSync) ensureTargetUserSecret(
7197
ctx context.Context,
7298
cr *psmdbv1.PerconaServerMongoDBClusterSync,
73-
) (psmdb.Credentials, error) {
99+
) (*corev1.Secret, error) {
74100
nn := types.NamespacedName{Name: clustersync.TargetUserSecretName(cr), Namespace: cr.Namespace}
75101
existing := &corev1.Secret{}
76102
err := r.client.Get(ctx, nn, existing)
77103
switch {
78104
case err == nil:
79-
username := string(existing.Data[targetUserSecretUsernameKey])
80-
password := string(existing.Data[targetUserSecretPasswordKey])
81-
if username == "" || password == "" {
82-
return psmdb.Credentials{}, errors.Errorf("target user secret %s missing %s/%s keys",
83-
nn, targetUserSecretUsernameKey, targetUserSecretPasswordKey)
105+
if v, ok := existing.Data[targetUserSecretUsernameKey]; !ok || len(v) == 0 {
106+
return nil, errors.Errorf("target user secret %s missing %s key", nn, targetUserSecretUsernameKey)
107+
}
108+
if v, ok := existing.Data[targetUserSecretPasswordKey]; !ok || len(v) == 0 {
109+
return nil, errors.Errorf("target user secret %s missing %s key", nn, targetUserSecretPasswordKey)
84110
}
85-
return psmdb.Credentials{Username: username, Password: password}, nil
111+
return existing, nil
86112
case !k8serrors.IsNotFound(err):
87-
return psmdb.Credentials{}, errors.Wrapf(err, "get target user secret %s", nn)
113+
return nil, errors.Wrapf(err, "get target user secret %s", nn)
88114
}
89115

90116
password, err := secret.GeneratePassword()
91117
if err != nil {
92-
return psmdb.Credentials{}, errors.Wrap(err, "generate target user password")
118+
return nil, errors.Wrap(err, "generate target user password")
93119
}
94120
username := syncTargetUsername(cr)
95121

@@ -106,37 +132,58 @@ func (r *ReconcilePerconaServerMongoDBClusterSync) ensureTargetUserSecret(
106132
},
107133
}
108134
if err := controllerutil.SetControllerReference(cr, newSecret, r.scheme); err != nil {
109-
return psmdb.Credentials{}, errors.Wrap(err, "set owner reference on target user secret")
135+
return nil, errors.Wrap(err, "set owner reference on target user secret")
110136
}
111137
if err := r.client.Create(ctx, newSecret); err != nil {
112-
return psmdb.Credentials{}, errors.Wrapf(err, "create target user secret %s", nn)
138+
return nil, errors.Wrapf(err, "create target user secret %s", nn)
113139
}
114-
return psmdb.Credentials{Username: username, Password: string(password)}, nil
140+
return newSecret, nil
115141
}
116142

117-
// ensureTargetMongoUser converges the sync user on the target cluster: it
118-
// creates the user when missing, and re-applies password and roles when it
119-
// already exists. The latter heals the case where the CR (and its owned
120-
// Secret) was deleted and recreated while the Mongo user, which has no owner
121-
// ref, kept its old password.
122-
func ensureTargetMongoUser(ctx context.Context, mc mongo.Client, creds psmdb.Credentials) error {
123-
existing, err := mc.GetUserInfo(ctx, creds.Username, "admin")
143+
func ensureTargetMongoUser(ctx context.Context, mc mongo.Client, sec *corev1.Secret) (bool, error) {
144+
username := string(sec.Data[targetUserSecretUsernameKey])
145+
password := sec.Data[targetUserSecretPasswordKey]
146+
147+
existing, err := mc.GetUserInfo(ctx, username, adminDBName)
124148
if err != nil {
125-
return errors.Wrap(err, "look up target user")
149+
return false, errors.Wrap(err, "look up target user")
126150
}
127151
if existing == nil {
128-
if err := mc.CreateUser(ctx, "admin", creds.Username, creds.Password, syncTargetUserRoles...); err != nil {
129-
return errors.Wrapf(err, "create target user %q", creds.Username)
152+
if err := mc.CreateUser(ctx, adminDBName, username, string(password), syncTargetUserRoles...); err != nil {
153+
return false, errors.Wrapf(err, "create target user %q", username)
130154
}
131-
return nil
155+
setPasswordHashAnnotation(sec, password)
156+
return true, nil
132157
}
133-
if err := mc.UpdateUserPass(ctx, "admin", creds.Username, creds.Password); err != nil {
134-
return errors.Wrapf(err, "sync target user password %q", creds.Username)
158+
159+
mutated := false
160+
newMAC := passwordAnnotationMAC(password)
161+
if sec.Annotations[targetUserPasswordMACAnnotation] != newMAC {
162+
if err := mc.UpdateUserPass(ctx, adminDBName, username, string(password)); err != nil {
163+
return false, errors.Wrapf(err, "sync target user password %q", username)
164+
}
165+
setPasswordHashAnnotation(sec, password)
166+
mutated = true
135167
}
136-
if err := mc.UpdateUserRoles(ctx, "admin", creds.Username, syncTargetUserRoles); err != nil {
137-
return errors.Wrapf(err, "sync target user roles %q", creds.Username)
168+
if !cmp.Equal(existing.Roles, syncTargetUserRoles, sortRolesOpt) {
169+
if err := mc.UpdateUserRoles(ctx, adminDBName, username, syncTargetUserRoles); err != nil {
170+
return false, errors.Wrapf(err, "sync target user roles %q", username)
171+
}
138172
}
139-
return nil
173+
return mutated, nil
174+
}
175+
176+
func setPasswordHashAnnotation(sec *corev1.Secret, password []byte) {
177+
if sec.Annotations == nil {
178+
sec.Annotations = make(map[string]string)
179+
}
180+
sec.Annotations[targetUserPasswordMACAnnotation] = passwordAnnotationMAC(password)
181+
}
182+
183+
func passwordAnnotationMAC(data []byte) string {
184+
mac := hmac.New(sha256.New, []byte(targetUserPasswordMACKey))
185+
mac.Write(data)
186+
return hex.EncodeToString(mac.Sum(nil))
140187
}
141188

142189
func defaultTargetMongoClient(ctx context.Context, cl client.Client, target *psmdbv1.PerconaServerMongoDB) (mongo.Client, error) {

0 commit comments

Comments
 (0)