Skip to content

Commit 91233d8

Browse files
lsierantlsierant
committed
CLOUDP-350197: allow to create a custom role with cluster: true
1 parent 2d7b9a0 commit 91233d8

File tree

8 files changed

+171
-118
lines changed

8 files changed

+171
-118
lines changed

‎api/v1/mdb/mongodb_roles_validation.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,7 @@ func validatePrivilege(privilege Privilege, mdbVersion string) v1.ValidationResu
246246
if !*privilege.Resource.Cluster {
247247
return v1.ValidationError("The only valid value for privilege.cluster, if set, is true")
248248
}
249-
if privilege.Resource.Collection != "" || privilege.Resource.Db != "" {
249+
if privilege.Resource.Collection != nil || privilege.Resource.Db != nil {
250250
return v1.ValidationError("Cluster: true is not compatible with setting db/collection")
251251
}
252252
if res := validateClusterPrivilegeActions(privilege.Actions, mdbVersion); res.Level == v1.ErrorLevel {

‎api/v1/mdb/mongodb_types.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -962,10 +962,10 @@ type AuthenticationRestriction struct {
962962

963963
type Resource struct {
964964
// +optional
965-
Db string `json:"db"`
965+
Db *string `json:"db,omitempty"`
966966
// +optional
967-
Collection string `json:"collection"`
968-
Cluster *bool `json:"cluster,omitempty"`
967+
Collection *string `json:"collection,omitempty"`
968+
Cluster *bool `json:"cluster,omitempty"`
969969
}
970970

971971
type Privilege struct {

‎controllers/operator/common_controller.go‎

Lines changed: 40 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"k8s.io/apimachinery/pkg/types"
1616
"k8s.io/client-go/kubernetes"
1717
"k8s.io/client-go/rest"
18+
"k8s.io/utils/ptr"
1819
"sigs.k8s.io/controller-runtime/pkg/client"
1920
"sigs.k8s.io/controller-runtime/pkg/reconcile"
2021

@@ -132,12 +133,24 @@ func (r *ReconcileCommonController) ensureRoles(ctx context.Context, db mdbv1.Db
132133
if reflect.DeepEqual(dRoles, roles) {
133134
return workflow.OK()
134135
}
135-
// HELP-20798: the agent deals correctly with a null value for
136-
// privileges only when creating a role, not when updating
137-
// we work around it by explicitly passing empty array
138-
for i, role := range roles {
139-
if role.Privileges == nil {
140-
roles[i].Privileges = []mdbv1.Privilege{}
136+
137+
// clone roles list to avoid mutating the spec in normalizePrivilegeResource
138+
newRoles := make([]mdbv1.MongoDBRole, len(roles))
139+
for i := range roles {
140+
newRoles[i] = *roles[i].DeepCopy()
141+
}
142+
roles = newRoles
143+
144+
for roleIdx := range roles {
145+
// HELP-20798: the agent deals correctly with a null value for
146+
// privileges only when creating a role, not when updating
147+
// we work around it by explicitly passing empty array
148+
if roles[roleIdx].Privileges == nil {
149+
roles[roleIdx].Privileges = []mdbv1.Privilege{}
150+
}
151+
152+
for privilegeIdx := range roles[roleIdx].Privileges {
153+
roles[roleIdx].Privileges[privilegeIdx].Resource = normalizePrivilegeResource(roles[roleIdx].Privileges[privilegeIdx].Resource)
141154
}
142155
}
143156

@@ -155,6 +168,27 @@ func (r *ReconcileCommonController) ensureRoles(ctx context.Context, db mdbv1.Db
155168
return workflow.OK()
156169
}
157170

171+
// normalizePrivilegeResource ensures that mutually exclusive fields are not passed at the same time and ensures backwards compatibility by
172+
// preserving empty strings for db and collection.
173+
// This function was introduced after we've changed db and collection fields to *string allowing to omit the field from serialization (CLOUDP-349078).
174+
func normalizePrivilegeResource(resource mdbv1.Resource) mdbv1.Resource {
175+
if resource.Cluster != nil && *resource.Cluster {
176+
// for cluster-wide privilege mongod is not accepting even empty strings in db and collection
177+
resource.Db = nil
178+
resource.Collection = nil
179+
} else {
180+
// for backwards compatibility we must convert "not specified" fields as empty strings
181+
if resource.Db == nil {
182+
resource.Db = ptr.To("")
183+
}
184+
if resource.Collection == nil {
185+
resource.Collection = ptr.To("")
186+
}
187+
}
188+
189+
return resource
190+
}
191+
158192
// getRoleRefs retrieves the roles from the referenced resources. It will return an error if any of the referenced resources are not found.
159193
// It will also add the referenced resources to the resource watcher, so that they are watched for changes.
160194
// The referenced resources are expected to be of kind ClusterMongoDBRole.

‎controllers/operator/common_controller_test.go‎

Lines changed: 49 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -419,22 +419,36 @@ func TestCheckEmptyStringsInPrivilegesEquivalentToNotPassingFields(t *testing.T)
419419
Privileges: []mdbv1.Privilege{
420420
{
421421
Resource: mdbv1.Resource{
422-
Db: "config",
423-
Collection: "", // Explicit empty string
422+
Db: ptr.To("config"),
423+
Collection: ptr.To(""), // Explicit empty string
424424
},
425425
Actions: []string{"find", "update", "insert", "remove"},
426426
},
427427
{
428428
Resource: mdbv1.Resource{
429-
Db: "users",
430-
Collection: "usersCollection",
429+
Db: ptr.To("users"),
430+
Collection: ptr.To("usersCollection"),
431431
},
432432
Actions: []string{"update", "insert", "remove"},
433433
},
434434
{
435435
Resource: mdbv1.Resource{
436-
Db: "", // Explicit empty string
437-
Collection: "", // Explicit empty string
436+
Db: ptr.To(""), // Explicit empty string
437+
Collection: ptr.To(""), // Explicit empty string
438+
},
439+
Actions: []string{"find"},
440+
},
441+
{
442+
Resource: mdbv1.Resource{
443+
Cluster: ptr.To(true),
444+
},
445+
Actions: []string{"find"},
446+
},
447+
{
448+
Resource: mdbv1.Resource{
449+
Cluster: ptr.To(true),
450+
Db: ptr.To(""),
451+
Collection: ptr.To(""),
438452
},
439453
Actions: []string{"find"},
440454
},
@@ -452,15 +466,15 @@ func TestCheckEmptyStringsInPrivilegesEquivalentToNotPassingFields(t *testing.T)
452466
Privileges: []mdbv1.Privilege{
453467
{
454468
Resource: mdbv1.Resource{
455-
Db: "config",
469+
Db: ptr.To("config"),
456470
// field not set, should pass ""
457471
},
458472
Actions: []string{"find", "update", "insert", "remove"},
459473
},
460474
{
461475
Resource: mdbv1.Resource{
462-
Db: "users",
463-
Collection: "usersCollection",
476+
Db: ptr.To("users"),
477+
Collection: ptr.To("usersCollection"),
464478
},
465479
Actions: []string{"update", "insert", "remove"},
466480
},
@@ -470,6 +484,12 @@ func TestCheckEmptyStringsInPrivilegesEquivalentToNotPassingFields(t *testing.T)
470484
},
471485
Actions: []string{"find"},
472486
},
487+
{
488+
Resource: mdbv1.Resource{
489+
Cluster: ptr.To(true),
490+
},
491+
Actions: []string{"find"},
492+
},
473493
},
474494
}
475495

@@ -486,16 +506,26 @@ func TestCheckEmptyStringsInPrivilegesEquivalentToNotPassingFields(t *testing.T)
486506
assert.True(t, ok)
487507
require.Len(t, roles, 2)
488508

489-
assert.Equal(t, "config", roles[0].Privileges[0].Resource.Db)
490-
assert.Equal(t, "", roles[0].Privileges[0].Resource.Collection)
491-
492-
assert.Equal(t, "users", roles[0].Privileges[1].Resource.Db)
493-
assert.Equal(t, "usersCollection", roles[0].Privileges[1].Resource.Collection)
494-
495-
assert.Equal(t, "", roles[0].Privileges[2].Resource.Db)
496-
assert.Equal(t, "", roles[0].Privileges[2].Resource.Collection)
497-
498-
assert.True(t, reflect.DeepEqual(roles[0].Privileges, roles[1].Privileges))
509+
// we iterate over two created privileges because both should end with the same result
510+
for i := range 2 {
511+
assert.Nil(t, roles[i].Privileges[0].Resource.Cluster)
512+
assert.Equal(t, ptr.To("config"), roles[i].Privileges[0].Resource.Db)
513+
// even if the db or collection field is not passed it must result in empty string
514+
assert.Equal(t, ptr.To(""), roles[i].Privileges[0].Resource.Collection)
515+
516+
assert.Nil(t, roles[i].Privileges[1].Resource.Cluster)
517+
assert.Equal(t, ptr.To("users"), roles[i].Privileges[1].Resource.Db)
518+
assert.Equal(t, ptr.To("usersCollection"), roles[i].Privileges[1].Resource.Collection)
519+
520+
assert.Nil(t, roles[i].Privileges[2].Resource.Cluster)
521+
assert.Equal(t, ptr.To(""), roles[i].Privileges[2].Resource.Db)
522+
assert.Equal(t, ptr.To(""), roles[i].Privileges[2].Resource.Collection)
523+
524+
require.NotNil(t, roles[i].Privileges[3].Resource.Cluster)
525+
assert.True(t, *roles[i].Privileges[3].Resource.Cluster)
526+
assert.Nil(t, roles[i].Privileges[3].Resource.Db)
527+
assert.Nil(t, roles[i].Privileges[3].Resource.Collection)
528+
}
499529
}
500530

501531
func TestSecretWatcherWithAllResources(t *testing.T) {

‎controllers/searchcontroller/mongodbsearch_reconcile_helper.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -490,7 +490,7 @@ func SearchCoordinatorRole() mdbv1.MongoDBRole {
490490
Privileges: []mdbv1.Privilege{
491491
{
492492
Resource: mdbv1.Resource{
493-
Db: "__mdb_internal_search",
493+
Db: ptr.To("__mdb_internal_search"),
494494
},
495495
Actions: []string{
496496
"changeStream", "collStats", "dbHash", "dbStats", "find",

‎docker/mongodb-kubernetes-tests/tests/authentication/fixtures/cluster-mongodb-role-with-empty-strings.yaml‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,6 @@ spec:
99
- db: "admin"
1010
role: "read"
1111
privileges:
12-
# - resource:
13-
# cluster: true
14-
# actions:
15-
# - "addShard"
1612
- resource:
1713
db: "config"
1814
collection: ""
@@ -33,6 +29,10 @@ spec:
3329
collection: ""
3430
actions:
3531
- "find"
32+
- resource:
33+
cluster: true
34+
actions:
35+
- "bypassWriteBlockingMode"
3636
authenticationRestrictions:
3737
- clientSource:
3838
- "127.0.0.0/8"

‎docker/mongodb-kubernetes-tests/tests/authentication/fixtures/cluster-mongodb-role-without-empty-strings.yaml‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,6 @@ spec:
99
- db: "admin"
1010
role: "read"
1111
privileges:
12-
# - resource:
13-
# cluster: true
14-
# actions:
15-
# - "addShard"
1612
- resource:
1713
db: "config"
1814
actions:
@@ -30,6 +26,10 @@ spec:
3026
- resource: {}
3127
actions:
3228
- "find"
29+
- resource:
30+
cluster: true
31+
actions:
32+
- "bypassWriteBlockingMode"
3333
authenticationRestrictions:
3434
- clientSource:
3535
- "127.0.0.0/8"

0 commit comments

Comments
 (0)