CLOUDP-355710 Fix deletion of external roles #625
Conversation
MCK 1.6.1 Release NotesBug Fixes
|
| } | ||
|
|
||
| // Set annotation and state for previously configured roles | ||
| roleAnnotation, roleStrings, err := r.commonController.getRoleAnnotation(ctx, r.sc.Spec.DbCommonSpec, r.enableClusterMongoDBRoles, kube.ObjectKeyFromApiObject(r.sc)) |
There was a problem hiding this comment.
why do we rely on annotation where we have deployment state available? I understand this is necessary for mongodb and mdbmc as we don't have the state yet, but here?
There was a problem hiding this comment.
OK, I see that we actually rely on the LastConfiguredRoles in the logic, so the annotation is to align with other controllers?
There was a problem hiding this comment.
Yes. I can remove the annotation from the sharded cluster, I was unsure whether I should add it to both the state and the annotation
There was a problem hiding this comment.
I removed the annotation
| roles = mockOm.GetRoles() | ||
| require.Len(t, roles, 2) | ||
|
|
||
| // Delete embedded role, only the external should remain |
There was a problem hiding this comment.
q: why did the embedded role get removed?
There was a problem hiding this comment.
re-initialized rs to a replicaset without roles on line 573
There was a problem hiding this comment.
ahhhh "deleting" by re-initialisation. Can you update the comment to clarify that ?
There was a problem hiding this comment.
no, you're right, I'll just set the roles field to an empty array to make it clear
| // This is achieved by removing currently configured roles from the deployed roles. | ||
| // To ensure that roles removed from the spec are also removed from OM, we also remove the previously configured roles. | ||
| // Finally, we add back the currently configured roles. | ||
| func mergeRoles(deployed []mdbv1.MongoDBRole, current []mdbv1.MongoDBRole, previous []string) []mdbv1.MongoDBRole { |
There was a problem hiding this comment.
blocking: lets add a dedicated unit test for mergeRoles
| }}, | ||
| } | ||
| mockOm.AddRole(externalRole) | ||
|
|
There was a problem hiding this comment.
can you - for the sake of clarity - add here:
roles = mockOm.GetRoles()
require.Len(t, roles, 2)
as well?
| --- | ||
| kind: fix | ||
| date: 2025-12-04 | ||
| --- | ||
|
|
||
| * Roles configured via Ops Manager UI or API will no longer be removed by the operator |
Summary
This PR fixes an issue where roles added to an OM project outside the operator (through UI or the API) are overwritten by the operator every reconciliation. This is now consistent with the user behaviour, where users can be defined in the UI and will not be removed.
To accomplish this, we are now tracking the roles we configure with the operator in an annotation, and state configmap (where applicable). This was necessary due to possible usage of the
ClusterMongoDBRoleresource. If the reference to a role resource is removed at the same time as removing the resource itself, then it is not possible to determine which role needs to be removed from the automation config without knowing the previous set of roles.Proof of Work
Unit tests have been added to verify that the
ensureRolesmethod which is reused across all controllers behaves correctly. Additionally, the unit tests verify that each controller keeps track of the previously configured roles in an annotation.Checklist
skip-changeloglabel if not needed