CLOUDP-355710 Fix deletion of external roles (#625)

# 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 `ClusterMongoDBRole`
resource. 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 `ensureRoles` method 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

- [x] Have you linked a jira ticket and/or is the ticket in the title?
- [x] Have you checked whether your jira ticket required DOCSP changes?
- [x] Have you added changelog file?
    - use `skip-changelog` label if not needed
- refer to [Changelog files and Release
Notes](https://github.com/mongodb/mongodb-kubernetes/blob/master/CONTRIBUTING.md#changelog-files-and-release-notes)
section in CONTRIBUTING.md for more details

Author: lucian-tosa

api/v1/mdb/mongodb_types.go

@@ -1201,6 +1201,20 @@ func (m *MongoDB) GetLastSpec() (*MongoDbSpec, error) {
 	return &lastSpec, nil
 }
 
+func (m *MongoDB) GetLastConfiguredRoles() ([]string, error) {
+	lastRolesStr := annotations.GetAnnotation(m, util.LastConfiguredRoles)
+	if lastRolesStr == "" {
+		return nil, nil
+	}
+
+	lastRoles := []string{}
+	if err := json.Unmarshal([]byte(lastRolesStr), &lastRoles); err != nil {
+		return nil, err
+	}
+
+	return lastRoles, nil
+}
+
 func (m *MongoDB) ServiceName() string {
 	if m.Spec.StatefulSetConfiguration != nil {
 		svc := m.Spec.StatefulSetConfiguration.SpecWrapper.Spec.ServiceName

api/v1/mdbmulti/mongodbmultibuilder.go

@@ -73,6 +73,14 @@ func (m *MultiReplicaSetBuilder) SetSecurity(s *mdbv1.Security) *MultiReplicaSet
 	return m
 }
 
+func (m *MultiReplicaSetBuilder) SetRoles(roles []mdbv1.MongoDBRole) *MultiReplicaSetBuilder {
+	if m.Spec.Security == nil {
+		m.Spec.Security = &mdbv1.Security{}
+	}
+	m.Spec.Security.Roles = roles
+	return m
+}
+
 func (m *MultiReplicaSetBuilder) SetRoleRefs(roleRefs []mdbv1.MongoDBRoleRef) *MultiReplicaSetBuilder {
 	if m.Spec.Security == nil {
 		m.Spec.Security = &mdbv1.Security{}

changelog/20251204_fix_roles_configured_via_ops_manager_ui_or_api_will_no.md

@@ -0,0 +1,6 @@
+---
+kind: fix
+date: 2025-12-04
+---
+
+* Roles configured via Ops Manager UI or API will no longer be removed by the operator

controllers/om/mockedomclient.go

@@ -933,6 +933,16 @@ func (oc *MockedOmConnection) AddPreferredHostname(agentApiKey string, value str
 	return nil
 }
 
+func (oc *MockedOmConnection) AddRole(role mdbv1.MongoDBRole) {
+	roles := oc.deployment.GetRoles()
+	roles = append(roles, role)
+	oc.deployment.SetRoles(roles)
+}
+
+func (oc *MockedOmConnection) GetRoles() []mdbv1.MongoDBRole {
+	return oc.deployment.GetRoles()
+}
+
 // updateAutoAuthMechanism simulates the changes made by Ops Manager and the agents in deciding which
 // mechanism will be specified as the "autoAuthMechanisms"
 func updateAutoAuthMechanism(ac *AutomationConfig) {

controllers/operator/common_controller.go

@@ -100,44 +100,85 @@ func NewReconcileCommonController(ctx context.Context, client client.Client) *Re
 	}
 }
 
-// ensureRoles will first check if both roles and roleRefs are populated. If both are, it will return an error, which is inline with the webhook validation rules.
-// Otherwise, if roles is populated, then it will extract the list of roles and check if they are already set in Ops Manager. If they are not, it will update the roles in Ops Manager.
-// If roleRefs is populated, it will extract the list of roles from the referenced resources and check if they are already set in Ops Manager. If they are not, it will update the roles in Ops Manager.
-func (r *ReconcileCommonController) ensureRoles(ctx context.Context, db mdbv1.DbCommonSpec, enableClusterMongoDBRoles bool, conn om.Connection, mongodbResourceNsName types.NamespacedName, log *zap.SugaredLogger) workflow.Status {
+func (r *ReconcileCommonController) getRoleAnnotation(ctx context.Context, db mdbv1.DbCommonSpec, enableClusterMongoDBRoles bool, mongodbResourceNsName types.NamespacedName) (map[string]string, []string, error) {
+	previousRoles, err := r.getRoleStrings(ctx, db, enableClusterMongoDBRoles, mongodbResourceNsName)
+	if err != nil {
+		return nil, nil, xerrors.Errorf("Error retrieving configured roles: %w", err)
+	}
+
+	annotationToAdd := make(map[string]string)
+	rolesBytes, err := json.Marshal(previousRoles)
+	if err != nil {
+		return nil, nil, err
+	}
+	annotationToAdd[util.LastConfiguredRoles] = string(rolesBytes)
+
+	return annotationToAdd, previousRoles, nil
+}
+
+func (r *ReconcileCommonController) getRoleStrings(ctx context.Context, db mdbv1.DbCommonSpec, enableClusterMongoDBRoles bool, mongodbResourceNsName types.NamespacedName) ([]string, error) {
+	roles, err := r.getRoles(ctx, db, enableClusterMongoDBRoles, mongodbResourceNsName)
+	if err != nil {
+		return []string{}, err
+	}
+
+	roleStrings := make([]string, len(roles))
+	for i, r := range roles {
+		roleStrings[i] = fmt.Sprintf("%s@%s", r.Role, r.Db)
+	}
+
+	return roleStrings, nil
+}
+
+func (r *ReconcileCommonController) getRoles(ctx context.Context, db mdbv1.DbCommonSpec, enableClusterMongoDBRoles bool, mongodbResourceNsName types.NamespacedName) ([]mdbv1.MongoDBRole, error) {
 	localRoles := db.GetSecurity().Roles
 	roleRefs := db.GetSecurity().RoleRefs
 
 	if len(localRoles) > 0 && len(roleRefs) > 0 {
-		return workflow.Failed(xerrors.Errorf("At most one one of roles or roleRefs can be non-empty."))
+		return nil, xerrors.Errorf("At most one of roles or roleRefs can be non-empty.")
 	}
 
 	var roles []mdbv1.MongoDBRole
 	if len(roleRefs) > 0 {
 		if !enableClusterMongoDBRoles {
-			return workflow.Failed(xerrors.Errorf("RoleRefs are not supported when ClusterMongoDBRoles are disabled. Please enable ClusterMongoDBRoles in the operator configuration. This can be done by setting the operator.enableClusterMongoDBRoles to true in the helm values file, which will automatically installed the necessary RBAC. Alternatively, it can be enabled by adding -watch-resource=clustermongodbroles flag to the operator deployment, and manually creating the necessary RBAC."))
+			return nil, xerrors.Errorf("RoleRefs are not supported when ClusterMongoDBRoles are disabled. Please enable ClusterMongoDBRoles in the operator configuration. This can be done by setting the operator.enableClusterMongoDBRoles to true in the helm values file, which will automatically installed the necessary RBAC. Alternatively, it can be enabled by adding -watch-resource=clustermongodbroles flag to the operator deployment, and manually creating the necessary RBAC.")
 		}
 		var err error
 		roles, err = r.getRoleRefs(ctx, roleRefs, mongodbResourceNsName, db.Version)
 		if err != nil {
-			return workflow.Failed(err)
+			return nil, err
 		}
 	} else {
 		roles = localRoles
 	}
 
+	return roles, nil
+}
+
+// ensureRoles will first check if both roles and roleRefs are populated. If both are, it will return an error, which is inline with the webhook validation rules.
+// Otherwise, if roles is populated, then it will extract the list of roles and check if they are already set in Ops Manager. If they are not, it will update the roles in Ops Manager.
+// If roleRefs is populated, it will extract the list of roles from the referenced resources and check if they are already set in Ops Manager. If they are not, it will update the roles in Ops Manager.
+func (r *ReconcileCommonController) ensureRoles(ctx context.Context, db mdbv1.DbCommonSpec, enableClusterMongoDBRoles bool, conn om.Connection, mongodbResourceNsName types.NamespacedName, previousRoles []string, log *zap.SugaredLogger) workflow.Status {
+	roles, err := r.getRoles(ctx, db, enableClusterMongoDBRoles, mongodbResourceNsName)
+	if err != nil {
+		return workflow.Failed(err)
+	}
+
 	d, err := conn.ReadDeployment()
 	if err != nil {
 		return workflow.Failed(err)
 	}
 	dRoles := d.GetRoles()
-	if reflect.DeepEqual(dRoles, roles) {
+	mergedRoles := mergeRoles(dRoles, roles, previousRoles)
+
+	if reflect.DeepEqual(dRoles, mergedRoles) {
 		return workflow.OK()
 	}
 
 	// clone roles list to avoid mutating the spec in normalizePrivilegeResource
-	newRoles := make([]mdbv1.MongoDBRole, len(roles))
-	for i := range roles {
-		newRoles[i] = *roles[i].DeepCopy()
+	newRoles := make([]mdbv1.MongoDBRole, len(mergedRoles))
+	for i := range mergedRoles {
+		newRoles[i] = *mergedRoles[i].DeepCopy()
 	}
 	roles = newRoles
 
@@ -189,6 +230,33 @@ func normalizePrivilegeResource(resource mdbv1.Resource) mdbv1.Resource {
 	return resource
 }
 
+// mergeRoles merges the deployed roles with the current roles and previously configured roles.
+// It ensures that roles configured outside the operator are not removed.
+// 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 {
+	roleMap := make(map[string]struct{})
+	for _, r := range current {
+		roleMap[r.Role+"@"+r.Db] = struct{}{}
+	}
+
+	for _, r := range previous {
+		roleMap[r] = struct{}{}
+	}
+
+	mergedRoles := make([]mdbv1.MongoDBRole, 0)
+	for _, r := range deployed {
+		key := r.Role + "@" + r.Db
+		if _, ok := roleMap[key]; !ok {
+			mergedRoles = append(mergedRoles, r)
+		}
+	}
+
+	mergedRoles = append(mergedRoles, current...)
+	return mergedRoles
+}
+
 // getRoleRefs retrieves the roles from the referenced resources. It will return an error if any of the referenced resources are not found.
 // It will also add the referenced resources to the resource watcher, so that they are watched for changes.
 // The referenced resources are expected to be of kind ClusterMongoDBRole.

controllers/operator/common_controller_test.go

@@ -290,7 +290,7 @@ func TestFailWhenRoleAndRoleRefsAreConfigured(t *testing.T) {
 	controller := NewReconcileCommonController(ctx, kubeClient)
 	mockOm, _ := prepareConnection(ctx, controller, omConnectionFactory.GetConnectionFunc, t)
 
-	result := controller.ensureRoles(ctx, rs.Spec.DbCommonSpec, true, mockOm, kube.ObjectKeyFromApiObject(rs), zap.S())
+	result := controller.ensureRoles(ctx, rs.Spec.DbCommonSpec, true, mockOm, kube.ObjectKeyFromApiObject(rs), nil, zap.S())
 	assert.False(t, result.IsOK())
 	assert.Equal(t, status.PhaseFailed, result.Phase())
 
@@ -318,7 +318,7 @@ func TestRoleRefsAreAdded(t *testing.T) {
 
 	_ = kubeClient.Create(ctx, roleResource)
 
-	controller.ensureRoles(ctx, rs.Spec.DbCommonSpec, true, mockOm, kube.ObjectKeyFromApiObject(rs), zap.S())
+	controller.ensureRoles(ctx, rs.Spec.DbCommonSpec, true, mockOm, kube.ObjectKeyFromApiObject(rs), nil, zap.S())
 
 	ac, err := mockOm.ReadAutomationConfig()
 	assert.NoError(t, err)
@@ -345,7 +345,7 @@ func TestErrorWhenRoleRefIsWrong(t *testing.T) {
 
 	_ = kubeClient.Create(ctx, roleResource)
 
-	result := controller.ensureRoles(ctx, rs.Spec.DbCommonSpec, true, mockOm, kube.ObjectKeyFromApiObject(rs), zap.S())
+	result := controller.ensureRoles(ctx, rs.Spec.DbCommonSpec, true, mockOm, kube.ObjectKeyFromApiObject(rs), nil, zap.S())
 	assert.False(t, result.IsOK())
 	assert.Equal(t, status.PhaseFailed, result.Phase())
 
@@ -371,7 +371,7 @@ func TestErrorWhenRoleDoesNotExist(t *testing.T) {
 	controller := NewReconcileCommonController(ctx, kubeClient)
 	mockOm, _ := prepareConnection(ctx, controller, omConnectionFactory.GetConnectionFunc, t)
 
-	result := controller.ensureRoles(ctx, rs.Spec.DbCommonSpec, true, mockOm, kube.ObjectKeyFromApiObject(rs), zap.S())
+	result := controller.ensureRoles(ctx, rs.Spec.DbCommonSpec, true, mockOm, kube.ObjectKeyFromApiObject(rs), nil, zap.S())
 	assert.False(t, result.IsOK())
 	assert.Equal(t, status.PhaseFailed, result.Phase())
 
@@ -398,7 +398,7 @@ func TestDontSendNilPrivileges(t *testing.T) {
 	kubeClient, omConnectionFactory := mock.NewDefaultFakeClient()
 	controller := NewReconcileCommonController(ctx, kubeClient)
 	mockOm, _ := prepareConnection(ctx, controller, omConnectionFactory.GetConnectionFunc, t)
-	controller.ensureRoles(ctx, rs.Spec.DbCommonSpec, true, mockOm, kube.ObjectKeyFromApiObject(rs), zap.S())
+	controller.ensureRoles(ctx, rs.Spec.DbCommonSpec, true, mockOm, kube.ObjectKeyFromApiObject(rs), nil, zap.S())
 	ac, err := mockOm.ReadAutomationConfig()
 	assert.NoError(t, err)
 	roles, ok := ac.Deployment["roles"].([]mdbv1.MongoDBRole)
@@ -498,7 +498,7 @@ func TestCheckEmptyStringsInPrivilegesEquivalentToNotPassingFields(t *testing.T)
 	controller := NewReconcileCommonController(ctx, kubeClient)
 	mockOm, _ := prepareConnection(ctx, controller, omConnectionFactory.GetConnectionFunc, t)
 
-	controller.ensureRoles(ctx, rs.Spec.DbCommonSpec, true, mockOm, kube.ObjectKeyFromApiObject(rs), zap.S())
+	controller.ensureRoles(ctx, rs.Spec.DbCommonSpec, true, mockOm, kube.ObjectKeyFromApiObject(rs), nil, zap.S())
 
 	ac, err := mockOm.ReadAutomationConfig()
 	assert.NoError(t, err)
@@ -528,6 +528,149 @@ func TestCheckEmptyStringsInPrivilegesEquivalentToNotPassingFields(t *testing.T)
 	}
 }
 
+func TestMergeRoles(t *testing.T) {
+	externalRole := mdbv1.MongoDBRole{
+		Role: "ext_role",
+		Db:   "admin",
+		Roles: []mdbv1.InheritedRole{{
+			Db:   "admin",
+			Role: "read",
+		}},
+	}
+	role1 := mdbv1.MongoDBRole{
+		Role: "role1",
+		Db:   "admin",
+		Roles: []mdbv1.InheritedRole{{
+			Db:   "admin",
+			Role: "readWrite",
+		}},
+	}
+
+	role2 := mdbv1.MongoDBRole{
+		Role: "role2",
+		Db:   "admin",
+		Roles: []mdbv1.InheritedRole{{
+			Db:   "admin",
+			Role: "readWrite",
+		}},
+	}
+
+	tests := []struct {
+		name          string
+		deployedRoles []mdbv1.MongoDBRole
+		currentRoles  []mdbv1.MongoDBRole
+		previousRoles []string
+		expectedRoles []mdbv1.MongoDBRole
+	}{
+		// externalRole was added via UI
+		// role1 and role2 were defined in the CR
+		// role2 was removed from the CR
+		{
+			name:          "Removing role from resource",
+			deployedRoles: []mdbv1.MongoDBRole{externalRole, role1, role2},
+			currentRoles:  []mdbv1.MongoDBRole{role1},
+			previousRoles: []string{"role1@admin", "role2@admin"},
+			expectedRoles: []mdbv1.MongoDBRole{externalRole, role1},
+		},
+		// externalRole was added via UI
+		// role1 was defined in the CR
+		// role2 was added in the CR
+		{
+			name:          "Adding role in resource",
+			deployedRoles: []mdbv1.MongoDBRole{externalRole, role1},
+			currentRoles:  []mdbv1.MongoDBRole{role1, role2},
+			previousRoles: []string{"role1@admin"},
+			expectedRoles: []mdbv1.MongoDBRole{externalRole, role1, role2},
+		},
+		{
+			name:          "Idempotency",
+			deployedRoles: []mdbv1.MongoDBRole{externalRole, role1, role2},
+			currentRoles:  []mdbv1.MongoDBRole{role1, role2},
+			previousRoles: []string{"role1@admin", "role2@admin"},
+			expectedRoles: []mdbv1.MongoDBRole{externalRole, role1, role2},
+		},
+		{
+			name:          "Nil previous roles - adding all defined roles",
+			deployedRoles: []mdbv1.MongoDBRole{externalRole, role1, role2},
+			currentRoles:  []mdbv1.MongoDBRole{role1, role2},
+			previousRoles: nil,
+			expectedRoles: []mdbv1.MongoDBRole{externalRole, role1, role2},
+		},
+		{
+			name:          "Nil current roles - removing all defined roles",
+			deployedRoles: []mdbv1.MongoDBRole{externalRole, role1, role2},
+			currentRoles:  nil,
+			previousRoles: []string{"role1@admin", "role2@admin"},
+			expectedRoles: []mdbv1.MongoDBRole{externalRole},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			mergedRoles := mergeRoles(tc.deployedRoles, tc.currentRoles, tc.previousRoles)
+
+			require.Len(t, mergedRoles, len(tc.expectedRoles))
+			for _, r := range tc.expectedRoles {
+				assert.Contains(t, mergedRoles, r)
+			}
+		})
+	}
+}
+
+func TestExternalRoleIsNotRemoved(t *testing.T) {
+	ctx := context.Background()
+
+	role := mdbv1.MongoDBRole{
+		Role: "embedded-role",
+		Db:   "admin",
+		Roles: []mdbv1.InheritedRole{{
+			Db:   "admin",
+			Role: "read",
+		}},
+	}
+
+	rs := DefaultReplicaSetBuilder().SetRoles([]mdbv1.MongoDBRole{role}).Build()
+	kubeClient, omConnectionFactory := mock.NewDefaultFakeClient()
+	controller := NewReconcileCommonController(ctx, kubeClient)
+	mockOm, _ := prepareConnection(ctx, controller, omConnectionFactory.GetConnectionFunc, t)
+
+	// Create deployment with one embedded role
+	controller.ensureRoles(ctx, rs.Spec.DbCommonSpec, true, mockOm, kube.ObjectKeyFromApiObject(rs), nil, zap.S())
+
+	roles := mockOm.GetRoles()
+	require.Len(t, roles, 1)
+
+	// Add external role directly to OM (via UI/API)
+	externalRole := mdbv1.MongoDBRole{
+		Role: "external-role",
+		Db:   "admin",
+		Roles: []mdbv1.InheritedRole{{
+			Db:   "admin",
+			Role: "read",
+		}},
+	}
+	mockOm.AddRole(externalRole)
+
+	// Ensure external role is added
+	roles = mockOm.GetRoles()
+	require.Len(t, roles, 2)
+
+	// Reconcile again - role created from the UI should still be there
+	roleStrings, _ := controller.getRoleStrings(ctx, rs.Spec.DbCommonSpec, true, kube.ObjectKeyFromApiObject(rs))
+	controller.ensureRoles(ctx, rs.Spec.DbCommonSpec, true, mockOm, kube.ObjectKeyFromApiObject(rs), roleStrings, zap.S())
+
+	roles = mockOm.GetRoles()
+	require.Len(t, roles, 2)
+
+	// Delete embedded role, only the external should remain
+	rs.Spec.Security.Roles = nil
+	controller.ensureRoles(ctx, rs.Spec.DbCommonSpec, true, mockOm, kube.ObjectKeyFromApiObject(rs), roleStrings, zap.S())
+
+	roles = mockOm.GetRoles()
+	require.Len(t, roles, 1)
+	assert.Equal(t, roles[0].Role, "external-role")
+}
+
 func TestSecretWatcherWithAllResources(t *testing.T) {
 	ctx := context.Background()
 	caName := "custom-ca"