Skip to content

K8SPSMDB-1607: use order-insensitive comparison for user roles in updateRoles#2256

Open
racciari wants to merge 2 commits intopercona:mainfrom
racciari:fix/user-roles-order-sensitive-comparison
Open

K8SPSMDB-1607: use order-insensitive comparison for user roles in updateRoles#2256
racciari wants to merge 2 commits intopercona:mainfrom
racciari:fix/user-roles-order-sensitive-comparison

Conversation

@racciari
Copy link
Copy Markdown

Summary

The updateRoles() function in custom_users.go uses reflect.DeepEqual to compare user roles from the CR spec with roles returned by MongoDB's usersInfo command. Since MongoDB does not guarantee the order of roles in its response, this causes an infinite reconciliation loop where the operator repeatedly detects a "change" and calls updateUser on every reconciliation cycle (~15 seconds), even when the roles are 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 PR applies the same fix to updateRoles() for user roles.

Root Cause

In updateRoles() (line 360):

if reflect.DeepEqual(userInfo.Roles, roles) {
    return nil
}

reflect.DeepEqual compares slices element-by-element in order. When MongoDB returns roles in a different order than specified in the CR, the comparison fails and triggers an unnecessary updateUser call. MongoDB then returns the roles in yet another order, perpetuating the loop.

Fix

Replace reflect.DeepEqual with cmp.Equal using cmpopts.SortSlices, sorting by (DB, Role) — consistent with the approach already used in rolesChanged():

sortRoles := cmpopts.SortSlices(func(a, b mongo.Role) bool {
    if a.DB != b.DB {
        return a.DB < b.DB
    }
    return a.Role < b.Role
})
if cmp.Equal(userInfo.Roles, roles, sortRoles) {
    return nil
}

Observed Impact

  • Operator logs show "User roles changed, updating them." every ~15 seconds for affected users
  • Unnecessary updateUser MongoDB commands on every reconciliation
  • Tested on Percona Operator 1.22.0 with MongoDB 8.0.12-4 across 10 namespaces: 2 actively looping, 8 converged by chance (role order happened to match)

Testing

Added TestUpdateRoles with 4 test cases:

  • Same roles, same order → no update
  • Same roles, different order → no update (the bug scenario)
  • Different roles → update called
  • Nil userInfo → no update

All existing tests pass.

Related

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Feb 25, 2026

CLA assistant check
All committers have signed the CLA.

@egegunes egegunes added this to the v1.23.0 milestone Feb 25, 2026
egegunes
egegunes previously approved these changes Feb 26, 2026
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
mock := &mockMongoClientForRoles{}
err := updateRoles(context.Background(), mock, tt.user, tt.userInfo)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can consider using t.Context() here

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks!

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 percona#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>
@racciari racciari force-pushed the fix/user-roles-order-sensitive-comparison branch from 8eb5d48 to b675426 Compare February 27, 2026 09:42
@egegunes egegunes changed the title fix: use order-insensitive comparison for user roles in updateRoles K8SPSMDB-1607: use order-insensitive comparison for user roles in updateRoles Mar 9, 2026
@JNKPercona
Copy link
Copy Markdown
Collaborator

Test Name Result Time
arbiter passed 00:00:00
balancer passed 00:00:00
cross-site-sharded passed 00:00:00
custom-replset-name passed 00:00:00
custom-tls passed 00:00:00
custom-users-roles passed 00:00:00
custom-users-roles-sharded passed 00:00:00
data-at-rest-encryption passed 00:00:00
data-sharded passed 00:00:00
demand-backup passed 00:00:00
demand-backup-eks-credentials-irsa passed 00:00:00
demand-backup-fs passed 00:00:00
demand-backup-if-unhealthy passed 00:00:00
demand-backup-incremental-aws passed 00:00:00
demand-backup-incremental-azure passed 00:00:00
demand-backup-incremental-gcp-native passed 00:00:00
demand-backup-incremental-gcp-s3 passed 00:00:00
demand-backup-incremental-minio passed 00:00:00
demand-backup-incremental-sharded-aws passed 00:00:00
demand-backup-incremental-sharded-azure passed 00:00:00
demand-backup-incremental-sharded-gcp-native passed 00:00:00
demand-backup-incremental-sharded-gcp-s3 passed 00:00:00
demand-backup-incremental-sharded-minio passed 00:00:00
demand-backup-logical-minio-native-tls passed 00:00:00
demand-backup-physical-parallel passed 00:00:00
demand-backup-physical-aws passed 00:00:00
demand-backup-physical-azure passed 00:00:00
demand-backup-physical-gcp-s3 passed 00:00:00
demand-backup-physical-gcp-native passed 00:00:00
demand-backup-physical-minio passed 00:00:00
demand-backup-physical-minio-native passed 00:00:00
demand-backup-physical-minio-native-tls passed 00:00:00
demand-backup-physical-sharded-parallel passed 00:00:00
demand-backup-physical-sharded-aws passed 00:00:00
demand-backup-physical-sharded-azure passed 00:00:00
demand-backup-physical-sharded-gcp-native passed 00:00:00
demand-backup-physical-sharded-minio passed 00:00:00
demand-backup-physical-sharded-minio-native passed 00:00:00
demand-backup-sharded passed 00:00:00
demand-backup-snapshot passed 00:00:00
demand-backup-snapshot-vault passed 00:00:00
disabled-auth passed 00:00:00
expose-sharded passed 00:32:55
finalizer passed 00:00:00
ignore-labels-annotations passed 00:00:00
init-deploy passed 00:00:00
ldap passed 00:00:00
ldap-tls passed 00:00:00
limits passed 00:00:00
liveness passed 00:00:00
mongod-major-upgrade passed 00:00:00
mongod-major-upgrade-sharded passed 00:00:00
monitoring-2-0 passed 00:00:00
monitoring-pmm3 passed 00:00:00
multi-cluster-service passed 00:00:00
multi-storage passed 00:00:00
non-voting-and-hidden passed 00:00:00
one-pod passed 00:00:00
operator-self-healing-chaos passed 00:00:00
pitr passed 00:00:00
pitr-physical passed 00:00:00
pitr-sharded passed 00:00:00
pitr-to-new-cluster passed 00:00:00
pitr-physical-backup-source passed 00:00:00
preinit-updates passed 00:00:00
pvc-auto-resize passed 00:00:00
pvc-resize passed 00:00:00
recover-no-primary passed 00:00:00
replset-overrides passed 00:00:00
replset-remapping passed 00:00:00
replset-remapping-sharded passed 00:00:00
rs-shard-migration passed 00:00:00
scaling passed 00:00:00
scheduled-backup passed 00:00:00
security-context passed 00:00:00
self-healing-chaos passed 00:00:00
service-per-pod passed 00:00:00
serviceless-external-nodes passed 00:00:00
smart-update passed 00:00:00
split-horizon passed 00:00:00
stable-resource-version passed 00:00:00
storage passed 00:00:00
tls-issue-cert-manager passed 00:00:00
unsafe-psa passed 00:00:00
upgrade passed 00:00:00
upgrade-consistency passed 00:00:00
upgrade-consistency-sharded-tls passed 00:00:00
upgrade-sharded passed 00:00:00
upgrade-partial-backup passed 00:00:00
users passed 00:00:00
users-vault passed 00:00:00
version-service passed 00:00:00
Summary Value
Tests Run 92/92
Job Duration 00:58:23
Total Test Time 00:32:55

commit: 6d23a02
image: perconalab/percona-server-mongodb-operator:PR-2256-6d23a024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community size/L 100-499 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants