Skip to content

Commit b675426

Browse files
committed
fix: use order-insensitive comparison for user roles in updateRoles
MongoDB does not guarantee the order of roles returned by usersInfo command. The updateRoles function used reflect.DeepEqual to compare the desired roles from the CR spec with the roles returned by MongoDB. Since reflect.DeepEqual is order-sensitive, this caused an infinite reconciliation loop where the operator would repeatedly detect a "change" and call updateUser on every reconciliation cycle (~15s), even when the roles were semantically identical. This is the same class of bug that was fixed for custom roles comparison in rolesChanged() (K8SPSMDB-1146 / PR #1679), which correctly uses cmp.Equal with cmpopts.SortSlices. This commit applies the same fix to updateRoles() for user roles. Signed-off-by: Romain Acciari <romain.acciari+github@gmail.com>
1 parent f02530c commit b675426

2 files changed

Lines changed: 106 additions & 2 deletions

File tree

pkg/controller/perconaservermongodb/custom_users.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package perconaservermongodb
33
import (
44
"context"
55
"fmt"
6-
"reflect"
76
"strings"
87

98
"github.com/google/go-cmp/cmp"
@@ -357,7 +356,13 @@ func updateRoles(ctx context.Context, mongoCli mongo.Client, user *api.User, use
357356
roles = append(roles, mongo.Role{DB: role.DB, Role: role.Name})
358357
}
359358

360-
if reflect.DeepEqual(userInfo.Roles, roles) {
359+
sortRoles := cmpopts.SortSlices(func(a, b mongo.Role) bool {
360+
if a.DB != b.DB {
361+
return a.DB < b.DB
362+
}
363+
return a.Role < b.Role
364+
})
365+
if cmp.Equal(userInfo.Roles, roles, sortRoles) {
361366
return nil
362367
}
363368

pkg/controller/perconaservermongodb/custom_users_test.go

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,105 @@ import (
1616
"github.com/percona/percona-server-mongodb-operator/pkg/psmdb/mongo"
1717
)
1818

19+
// mockMongoClientForRoles is a minimal mock to test updateRoles behavior.
20+
type mockMongoClientForRoles struct {
21+
mongo.Client
22+
updateRolesCalled bool
23+
}
24+
25+
func (m *mockMongoClientForRoles) UpdateUserRoles(ctx context.Context, db, username string, roles []mongo.Role) error {
26+
m.updateRolesCalled = true
27+
return nil
28+
}
29+
30+
func TestUpdateRoles(t *testing.T) {
31+
tests := []struct {
32+
name string
33+
user *api.User
34+
userInfo *mongo.User
35+
expectUpdateCalled bool
36+
}{
37+
{
38+
name: "same roles same order - no update",
39+
user: &api.User{
40+
Name: "testuser",
41+
DB: "testdb",
42+
Roles: []api.UserRole{
43+
{Name: "readWrite", DB: "db1"},
44+
{Name: "read", DB: "db2"},
45+
},
46+
},
47+
userInfo: &mongo.User{
48+
Roles: []mongo.Role{
49+
{Role: "readWrite", DB: "db1"},
50+
{Role: "read", DB: "db2"},
51+
},
52+
},
53+
expectUpdateCalled: false,
54+
},
55+
{
56+
name: "same roles different order - no update",
57+
user: &api.User{
58+
Name: "testuser",
59+
DB: "testdb",
60+
Roles: []api.UserRole{
61+
{Name: "readWrite", DB: "db1"},
62+
{Name: "clusterMonitor", DB: "admin"},
63+
{Name: "readWrite", DB: "db2"},
64+
},
65+
},
66+
userInfo: &mongo.User{
67+
Roles: []mongo.Role{
68+
{Role: "clusterMonitor", DB: "admin"},
69+
{Role: "readWrite", DB: "db2"},
70+
{Role: "readWrite", DB: "db1"},
71+
},
72+
},
73+
expectUpdateCalled: false,
74+
},
75+
{
76+
name: "different roles - update called",
77+
user: &api.User{
78+
Name: "testuser",
79+
DB: "testdb",
80+
Roles: []api.UserRole{
81+
{Name: "readWrite", DB: "db1"},
82+
{Name: "read", DB: "db2"},
83+
},
84+
},
85+
userInfo: &mongo.User{
86+
Roles: []mongo.Role{
87+
{Role: "readWrite", DB: "db1"},
88+
{Role: "readWrite", DB: "db3"},
89+
},
90+
},
91+
expectUpdateCalled: true,
92+
},
93+
{
94+
name: "nil userInfo - no update",
95+
user: &api.User{
96+
Name: "testuser",
97+
DB: "testdb",
98+
Roles: []api.UserRole{
99+
{Name: "readWrite", DB: "db1"},
100+
},
101+
},
102+
userInfo: nil,
103+
expectUpdateCalled: false,
104+
},
105+
}
106+
107+
for _, tt := range tests {
108+
t.Run(tt.name, func(t *testing.T) {
109+
mock := &mockMongoClientForRoles{}
110+
err := updateRoles(t.Context(), mock, tt.user, tt.userInfo)
111+
assert.NoError(t, err)
112+
assert.Equal(t, tt.expectUpdateCalled, mock.updateRolesCalled,
113+
"UpdateUserRoles called = %v, want %v", mock.updateRolesCalled, tt.expectUpdateCalled)
114+
})
115+
}
116+
}
117+
19118
func TestRolesChanged(t *testing.T) {
20119
r2 := &mongo.Role{
21120
Privileges: []mongo.RolePrivilege{

0 commit comments

Comments
 (0)