diff --git a/api/v1/mdb/mongodb_roles_validation.go b/api/v1/mdb/mongodb_roles_validation.go index 749ce8c04..c123ee520 100644 --- a/api/v1/mdb/mongodb_roles_validation.go +++ b/api/v1/mdb/mongodb_roles_validation.go @@ -246,7 +246,7 @@ func validatePrivilege(privilege Privilege, mdbVersion string) v1.ValidationResu if !*privilege.Resource.Cluster { return v1.ValidationError("The only valid value for privilege.cluster, if set, is true") } - if privilege.Resource.Collection != "" || privilege.Resource.Db != "" { + if privilege.Resource.Collection != nil || privilege.Resource.Db != nil { return v1.ValidationError("Cluster: true is not compatible with setting db/collection") } if res := validateClusterPrivilegeActions(privilege.Actions, mdbVersion); res.Level == v1.ErrorLevel { diff --git a/api/v1/mdb/mongodb_types.go b/api/v1/mdb/mongodb_types.go index 199146431..42bab17a2 100644 --- a/api/v1/mdb/mongodb_types.go +++ b/api/v1/mdb/mongodb_types.go @@ -965,10 +965,10 @@ type AuthenticationRestriction struct { type Resource struct { // +optional - Db string `json:"db"` + Db *string `json:"db,omitempty"` // +optional - Collection string `json:"collection"` - Cluster *bool `json:"cluster,omitempty"` + Collection *string `json:"collection,omitempty"` + Cluster *bool `json:"cluster,omitempty"` } type Privilege struct { diff --git a/changelog/20251027_fix_cloudp_350197_allow_to_create_a_custom_role_with.md b/changelog/20251027_fix_cloudp_350197_allow_to_create_a_custom_role_with.md new file mode 100644 index 000000000..4268ecd13 --- /dev/null +++ b/changelog/20251027_fix_cloudp_350197_allow_to_create_a_custom_role_with.md @@ -0,0 +1,6 @@ +--- +kind: fix +date: 2025-10-27 +--- + +* Fixed inability to specify cluster-wide privileges in custom roles. diff --git a/controllers/operator/common_controller.go b/controllers/operator/common_controller.go index e76cf4e81..356b7a592 100644 --- a/controllers/operator/common_controller.go +++ b/controllers/operator/common_controller.go @@ -15,6 +15,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -132,12 +133,24 @@ func (r *ReconcileCommonController) ensureRoles(ctx context.Context, db mdbv1.Db if reflect.DeepEqual(dRoles, roles) { return workflow.OK() } - // HELP-20798: the agent deals correctly with a null value for - // privileges only when creating a role, not when updating - // we work around it by explicitly passing empty array - for i, role := range roles { - if role.Privileges == nil { - roles[i].Privileges = []mdbv1.Privilege{} + + // 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() + } + roles = newRoles + + for roleIdx := range roles { + // HELP-20798: the agent deals correctly with a null value for + // privileges only when creating a role, not when updating + // we work around it by explicitly passing empty array + if roles[roleIdx].Privileges == nil { + roles[roleIdx].Privileges = []mdbv1.Privilege{} + } + + for privilegeIdx := range roles[roleIdx].Privileges { + roles[roleIdx].Privileges[privilegeIdx].Resource = normalizePrivilegeResource(roles[roleIdx].Privileges[privilegeIdx].Resource) } } @@ -155,6 +168,27 @@ func (r *ReconcileCommonController) ensureRoles(ctx context.Context, db mdbv1.Db return workflow.OK() } +// normalizePrivilegeResource ensures that mutually exclusive fields are not passed at the same time and ensures backwards compatibility by +// preserving empty strings for db and collection. +// This function was introduced after we've changed db and collection fields to *string allowing to omit the field from serialization (CLOUDP-349078). +func normalizePrivilegeResource(resource mdbv1.Resource) mdbv1.Resource { + if resource.Cluster != nil && *resource.Cluster { + // for cluster-wide privilege mongod is not accepting even empty strings in db and collection + resource.Db = nil + resource.Collection = nil + } else { + // for backwards compatibility we must convert "not specified" fields as empty strings + if resource.Db == nil { + resource.Db = ptr.To("") + } + if resource.Collection == nil { + resource.Collection = ptr.To("") + } + } + + return resource +} + // 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. diff --git a/controllers/operator/common_controller_test.go b/controllers/operator/common_controller_test.go index fd3ae4871..608a80bb9 100644 --- a/controllers/operator/common_controller_test.go +++ b/controllers/operator/common_controller_test.go @@ -419,22 +419,36 @@ func TestCheckEmptyStringsInPrivilegesEquivalentToNotPassingFields(t *testing.T) Privileges: []mdbv1.Privilege{ { Resource: mdbv1.Resource{ - Db: "config", - Collection: "", // Explicit empty string + Db: ptr.To("config"), + Collection: ptr.To(""), // Explicit empty string }, Actions: []string{"find", "update", "insert", "remove"}, }, { Resource: mdbv1.Resource{ - Db: "users", - Collection: "usersCollection", + Db: ptr.To("users"), + Collection: ptr.To("usersCollection"), }, Actions: []string{"update", "insert", "remove"}, }, { Resource: mdbv1.Resource{ - Db: "", // Explicit empty string - Collection: "", // Explicit empty string + Db: ptr.To(""), // Explicit empty string + Collection: ptr.To(""), // Explicit empty string + }, + Actions: []string{"find"}, + }, + { + Resource: mdbv1.Resource{ + Cluster: ptr.To(true), + }, + Actions: []string{"find"}, + }, + { + Resource: mdbv1.Resource{ + Cluster: ptr.To(true), + Db: ptr.To(""), + Collection: ptr.To(""), }, Actions: []string{"find"}, }, @@ -452,15 +466,15 @@ func TestCheckEmptyStringsInPrivilegesEquivalentToNotPassingFields(t *testing.T) Privileges: []mdbv1.Privilege{ { Resource: mdbv1.Resource{ - Db: "config", + Db: ptr.To("config"), // field not set, should pass "" }, Actions: []string{"find", "update", "insert", "remove"}, }, { Resource: mdbv1.Resource{ - Db: "users", - Collection: "usersCollection", + Db: ptr.To("users"), + Collection: ptr.To("usersCollection"), }, Actions: []string{"update", "insert", "remove"}, }, @@ -470,6 +484,12 @@ func TestCheckEmptyStringsInPrivilegesEquivalentToNotPassingFields(t *testing.T) }, Actions: []string{"find"}, }, + { + Resource: mdbv1.Resource{ + Cluster: ptr.To(true), + }, + Actions: []string{"find"}, + }, }, } @@ -486,16 +506,26 @@ func TestCheckEmptyStringsInPrivilegesEquivalentToNotPassingFields(t *testing.T) assert.True(t, ok) require.Len(t, roles, 2) - assert.Equal(t, "config", roles[0].Privileges[0].Resource.Db) - assert.Equal(t, "", roles[0].Privileges[0].Resource.Collection) - - assert.Equal(t, "users", roles[0].Privileges[1].Resource.Db) - assert.Equal(t, "usersCollection", roles[0].Privileges[1].Resource.Collection) - - assert.Equal(t, "", roles[0].Privileges[2].Resource.Db) - assert.Equal(t, "", roles[0].Privileges[2].Resource.Collection) - - assert.True(t, reflect.DeepEqual(roles[0].Privileges, roles[1].Privileges)) + // we iterate over two created privileges because both should end with the same result + for i := range 2 { + assert.Nil(t, roles[i].Privileges[0].Resource.Cluster) + assert.Equal(t, ptr.To("config"), roles[i].Privileges[0].Resource.Db) + // even if the db or collection field is not passed it must result in empty string + assert.Equal(t, ptr.To(""), roles[i].Privileges[0].Resource.Collection) + + assert.Nil(t, roles[i].Privileges[1].Resource.Cluster) + assert.Equal(t, ptr.To("users"), roles[i].Privileges[1].Resource.Db) + assert.Equal(t, ptr.To("usersCollection"), roles[i].Privileges[1].Resource.Collection) + + assert.Nil(t, roles[i].Privileges[2].Resource.Cluster) + assert.Equal(t, ptr.To(""), roles[i].Privileges[2].Resource.Db) + assert.Equal(t, ptr.To(""), roles[i].Privileges[2].Resource.Collection) + + require.NotNil(t, roles[i].Privileges[3].Resource.Cluster) + assert.True(t, *roles[i].Privileges[3].Resource.Cluster) + assert.Nil(t, roles[i].Privileges[3].Resource.Db) + assert.Nil(t, roles[i].Privileges[3].Resource.Collection) + } } func TestSecretWatcherWithAllResources(t *testing.T) { diff --git a/docker/mongodb-kubernetes-tests/kubetester/mongotester.py b/docker/mongodb-kubernetes-tests/kubetester/mongotester.py index 23f0d083a..c44ef9bb0 100644 --- a/docker/mongodb-kubernetes-tests/kubetester/mongotester.py +++ b/docker/mongodb-kubernetes-tests/kubetester/mongotester.py @@ -645,7 +645,7 @@ def run(self): self.health_function(**self.health_function_params) consecutive_failure = 0 except Exception as e: - print(f"Error in {self.__class__.__name__}: {e})") + logging.error(f"Error in {self.__class__.__name__}: {e})") self.last_exception = e consecutive_failure = consecutive_failure + 1 self.max_consecutive_failure = max(self.max_consecutive_failure, consecutive_failure) diff --git a/docker/mongodb-kubernetes-tests/tests/authentication/fixtures/cluster-mongodb-role-with-empty-strings.yaml b/docker/mongodb-kubernetes-tests/tests/authentication/fixtures/cluster-mongodb-role-with-empty-strings.yaml index a00d1c3da..b8f3219ce 100644 --- a/docker/mongodb-kubernetes-tests/tests/authentication/fixtures/cluster-mongodb-role-with-empty-strings.yaml +++ b/docker/mongodb-kubernetes-tests/tests/authentication/fixtures/cluster-mongodb-role-with-empty-strings.yaml @@ -29,6 +29,10 @@ spec: collection: "" actions: - "find" + - resource: + cluster: true + actions: + - "bypassWriteBlockingMode" authenticationRestrictions: - clientSource: - "127.0.0.0/8" diff --git a/docker/mongodb-kubernetes-tests/tests/authentication/fixtures/cluster-mongodb-role-without-empty-strings.yaml b/docker/mongodb-kubernetes-tests/tests/authentication/fixtures/cluster-mongodb-role-without-empty-strings.yaml index fddf16832..2da1f5b7a 100644 --- a/docker/mongodb-kubernetes-tests/tests/authentication/fixtures/cluster-mongodb-role-without-empty-strings.yaml +++ b/docker/mongodb-kubernetes-tests/tests/authentication/fixtures/cluster-mongodb-role-without-empty-strings.yaml @@ -26,6 +26,10 @@ spec: - resource: {} actions: - "find" + - resource: + cluster: true + actions: + - "bypassWriteBlockingMode" authenticationRestrictions: - clientSource: - "127.0.0.0/8" diff --git a/docker/mongodb-kubernetes-tests/tests/authentication/mongodb_custom_roles.py b/docker/mongodb-kubernetes-tests/tests/authentication/mongodb_custom_roles.py index 142a04e2d..9665e6169 100644 --- a/docker/mongodb-kubernetes-tests/tests/authentication/mongodb_custom_roles.py +++ b/docker/mongodb-kubernetes-tests/tests/authentication/mongodb_custom_roles.py @@ -57,6 +57,14 @@ def get_expected_role(role_name: str) -> dict: "find" ] }, + { + "resource": { + "cluster": True + }, + "actions": [ + "bypassWriteBlockingMode" + ] + } ], "authenticationRestrictions": [ { diff --git a/scripts/evergreen/e2e/dump_diagnostic_information.sh b/scripts/evergreen/e2e/dump_diagnostic_information.sh index 4f46546c2..bd5a7a242 100755 --- a/scripts/evergreen/e2e/dump_diagnostic_information.sh +++ b/scripts/evergreen/e2e/dump_diagnostic_information.sh @@ -206,6 +206,11 @@ dump_pod_readiness_state() { kubectl --context="${context}" cp -c "mongodb-agent" "${namespace}/${pod}:/var/log/mongodb-mms-automation/agent-health-status.json" "${agent_health_status}" &> /dev/null ([[ -f "${agent_health_status}" ]] && jq . < "${agent_health_status}" > tmpfile && mv tmpfile "${agent_health_status}") + if [[ ! -f "${agent_health_status}" ]]; then + kubectl --context="${context}" cp -c "mongodb-enterprise-database" "${namespace}/${pod}:/var/log/mongodb-mms-automation/agent-health-status.json" "${agent_health_status}" &> /dev/null + ([[ -f "${agent_health_status}" ]] && jq . < "${agent_health_status}" > tmpfile && mv tmpfile "${agent_health_status}") + fi + if [[ ! -f "${agent_health_status}" ]]; then echo "Agent health status not found; trying community health status: " kubectl --context="${context}" cp -c "mongodb-agent" "${namespace}/${pod}:/var/log/mongodb-mms-automation/healthstatus/agent-health-status.json" "${agent_health_status}" &> /dev/null