From ec044be5f532c09a05ef3de2082a4e465e6e5161 Mon Sep 17 00:00:00 2001 From: Igor Karpukhin Date: Tue, 7 Jan 2025 16:02:57 +0100 Subject: [PATCH 1/8] Fixed unintended removals of custom roles --- .../controller/atlasproject/custom_roles.go | 33 +++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/internal/controller/atlasproject/custom_roles.go b/internal/controller/atlasproject/custom_roles.go index a53e1b6536..de0428797c 100644 --- a/internal/controller/atlasproject/custom_roles.go +++ b/internal/controller/atlasproject/custom_roles.go @@ -50,16 +50,29 @@ func getLastAppliedCustomRoles(atlasProject *akov2.AtlasProject) ([]akov2.Custom return lastAppliedSpec.CustomRoles, nil } -func findRolesToDelete(prevSpec, atlasRoles []customroles.CustomRole) map[string]customroles.CustomRole { +func findRolesToDelete(prevSpec, akoRoles, atlasRoles []customroles.CustomRole) map[string]customroles.CustomRole { result := map[string]customroles.CustomRole{} - for atlasRoleIdx := range atlasRoles { - for specRoleIdx := range prevSpec { - if atlasRoles[atlasRoleIdx].Name == prevSpec[specRoleIdx].Name { - result[prevSpec[specRoleIdx].Name] = prevSpec[specRoleIdx] - continue - } + combinedAkoRoles := map[string]customroles.CustomRole{} + atlasRolesMap := mapCustomRolesByName(atlasRoles) + + // Get roles from the previous spec + for _, customRole := range prevSpec { + combinedAkoRoles[customRole.Name] = customRole + } + // Get roles from the current spec and remove the ones that are in the previous spec + for _, customRole := range akoRoles { + if _, exists := combinedAkoRoles[customRole.Name]; exists { + delete(combinedAkoRoles, customRole.Name) } } + + // Compare combinedAkoRoles with the current Atlas roles + for _, role := range combinedAkoRoles { + if _, exists := atlasRolesMap[role.Name]; exists { + result[role.Name] = role + } + } + return result } @@ -94,7 +107,7 @@ func ensureCustomRoles(workflowCtx *workflow.Context, project *akov2.AtlasProjec service: customroles.NewCustomRoles(workflowCtx.SdkClient.CustomDatabaseRolesApi), } - currentCustomRoles, err := r.service.List(r.ctx.Context, r.project.ID()) + currentAtlasCustomRoles, err := r.service.List(r.ctx.Context, r.project.ID()) if err != nil { return workflow.Terminate(workflow.ProjectCustomRolesReady, err.Error()) } @@ -104,12 +117,12 @@ func ensureCustomRoles(workflowCtx *workflow.Context, project *akov2.AtlasProjec akoRoles[i] = customroles.NewCustomRole(&project.Spec.CustomRoles[i]) } - ops := calculateChanges(currentCustomRoles, akoRoles) + ops := calculateChanges(currentAtlasCustomRoles, akoRoles) var deleteStatus map[string]status.CustomRole if len(lastAppliedCustomRoles) > 0 { deleteStatus = r.deleteCustomRoles(workflowCtx, project.ID(), - findRolesToDelete(convertToInternalRoles(lastAppliedCustomRoles), currentCustomRoles)) + findRolesToDelete(convertToInternalRoles(lastAppliedCustomRoles), akoRoles, currentAtlasCustomRoles)) } updateStatus := r.updateCustomRoles(workflowCtx, project.ID(), ops.Update) createStatus := r.createCustomRoles(workflowCtx, project.ID(), ops.Create) From 34b7d3203a11fe96f9a3200ca2d022aadbadcdee Mon Sep 17 00:00:00 2001 From: Igor Karpukhin Date: Tue, 7 Jan 2025 16:17:23 +0100 Subject: [PATCH 2/8] Make linter happy --- internal/controller/atlasproject/custom_roles.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/internal/controller/atlasproject/custom_roles.go b/internal/controller/atlasproject/custom_roles.go index de0428797c..98b6ea13c3 100644 --- a/internal/controller/atlasproject/custom_roles.go +++ b/internal/controller/atlasproject/custom_roles.go @@ -61,9 +61,7 @@ func findRolesToDelete(prevSpec, akoRoles, atlasRoles []customroles.CustomRole) } // Get roles from the current spec and remove the ones that are in the previous spec for _, customRole := range akoRoles { - if _, exists := combinedAkoRoles[customRole.Name]; exists { - delete(combinedAkoRoles, customRole.Name) - } + delete(combinedAkoRoles, customRole.Name) } // Compare combinedAkoRoles with the current Atlas roles From 8bc78212f3157e7c5449adf1492c71d4a7aa42b7 Mon Sep 17 00:00:00 2001 From: Igor Karpukhin Date: Tue, 7 Jan 2025 20:31:59 +0100 Subject: [PATCH 3/8] Added missing unit-test --- .../atlasproject/custom_roles_test.go | 131 ++++++++++++++++++ 1 file changed, 131 insertions(+) diff --git a/internal/controller/atlasproject/custom_roles_test.go b/internal/controller/atlasproject/custom_roles_test.go index 7855c68cfd..24a6bb3cc5 100644 --- a/internal/controller/atlasproject/custom_roles_test.go +++ b/internal/controller/atlasproject/custom_roles_test.go @@ -214,6 +214,137 @@ func TestEnsureCustomRoles(t *testing.T) { }(), isOK: true, }, + { + name: "Roles in AKO and in last applied config. Delete only those that were deleted from the spec", + projectAnnotations: map[string]string{ + customresource.AnnotationLastAppliedConfiguration: func() string { + d, _ := json.Marshal(&akov2.AtlasProjectSpec{ + CustomRoles: []akov2.CustomRole{ + { + Name: "test-role", + InheritedRoles: []akov2.Role{ + {Name: "role", Database: "db"}, + }, + Actions: []akov2.Action{ + { + Name: "action", + Resources: []akov2.Resource{ + { + Database: pointer.MakePtr("db"), + Cluster: pointer.MakePtr(true), + Collection: pointer.MakePtr("test-collection"), + }, + }, + }, + }, + }, + { + Name: "test-role-2", + InheritedRoles: []akov2.Role{ + {Name: "role2", Database: "db2"}, + }, + Actions: []akov2.Action{ + { + Name: "action2", + Resources: []akov2.Resource{ + {Database: pointer.MakePtr("db2")}, + }, + }, + }, + }, + }, + }) + return string(d) + }(), + }, + roles: []akov2.CustomRole{ + { + Name: "test-role", + InheritedRoles: []akov2.Role{ + {Name: "role", Database: "db"}, + }, + Actions: []akov2.Action{ + { + Name: "action", + Resources: []akov2.Resource{ + { + Database: pointer.MakePtr("db"), + Cluster: pointer.MakePtr(true), + Collection: pointer.MakePtr("test-collection"), + }, + }, + }, + }, + }, + }, + roleAPI: func() *mockadmin.CustomDatabaseRolesApi { + roleAPI := mockadmin.NewCustomDatabaseRolesApi(t) + roleAPI.EXPECT().ListCustomDatabaseRoles(context.Background(), ""). + Return(admin.ListCustomDatabaseRolesApiRequest{ApiService: roleAPI}) + roleAPI.EXPECT().ListCustomDatabaseRolesExecute(mock.Anything). + Return( + []admin.UserCustomDBRole{ + { + RoleName: "test-role", + InheritedRoles: &[]admin.DatabaseInheritedRole{ + {Role: "role", Db: "db"}, + }, + Actions: &[]admin.DatabasePrivilegeAction{ + { + Action: "action", + Resources: &[]admin.DatabasePermittedNamespaceResource{ + { + Db: "db", + Collection: "test-collection", + Cluster: true, + }, + }, + }, + }, + }, + { + RoleName: "test-role-1", + InheritedRoles: &[]admin.DatabaseInheritedRole{ + {Role: "role1", Db: "db1"}, + }, + Actions: &[]admin.DatabasePrivilegeAction{ + { + Action: "action1", + Resources: &[]admin.DatabasePermittedNamespaceResource{ + {Db: "db1"}, + }, + }, + }, + }, + { + RoleName: "test-role-2", + InheritedRoles: &[]admin.DatabaseInheritedRole{ + {Role: "role2", Db: "db2"}, + }, + Actions: &[]admin.DatabasePrivilegeAction{ + { + Action: "action2", + Resources: &[]admin.DatabasePermittedNamespaceResource{ + {Db: "db2"}, + }, + }, + }, + }, + }, + &http.Response{}, + nil, + ) + roleAPI.EXPECT().DeleteCustomDatabaseRole(context.Background(), "", "test-role-2"). + Return(admin.DeleteCustomDatabaseRoleApiRequest{ApiService: roleAPI}) + roleAPI.EXPECT().DeleteCustomDatabaseRoleExecute(mock.Anything). + Return( + &http.Response{}, + nil, + ) + return roleAPI + }(), + isOK: true, + }, { name: "Roles not in AKO but are in Atlas (Do not Delete) and NO previous in AKO", roleAPI: func() *mockadmin.CustomDatabaseRolesApi { From 23777ad63947b8d5ac8a8ff39b10720efa25fe00 Mon Sep 17 00:00:00 2001 From: Igor Karpukhin Date: Wed, 8 Jan 2025 13:21:49 +0100 Subject: [PATCH 4/8] Fixed naming --- internal/controller/atlasproject/custom_roles.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/controller/atlasproject/custom_roles.go b/internal/controller/atlasproject/custom_roles.go index 98b6ea13c3..f82345ce53 100644 --- a/internal/controller/atlasproject/custom_roles.go +++ b/internal/controller/atlasproject/custom_roles.go @@ -52,20 +52,20 @@ func getLastAppliedCustomRoles(atlasProject *akov2.AtlasProject) ([]akov2.Custom func findRolesToDelete(prevSpec, akoRoles, atlasRoles []customroles.CustomRole) map[string]customroles.CustomRole { result := map[string]customroles.CustomRole{} - combinedAkoRoles := map[string]customroles.CustomRole{} + deletionCandidates := map[string]customroles.CustomRole{} atlasRolesMap := mapCustomRolesByName(atlasRoles) // Get roles from the previous spec for _, customRole := range prevSpec { - combinedAkoRoles[customRole.Name] = customRole + deletionCandidates[customRole.Name] = customRole } // Get roles from the current spec and remove the ones that are in the previous spec for _, customRole := range akoRoles { - delete(combinedAkoRoles, customRole.Name) + delete(deletionCandidates, customRole.Name) } // Compare combinedAkoRoles with the current Atlas roles - for _, role := range combinedAkoRoles { + for _, role := range deletionCandidates { if _, exists := atlasRolesMap[role.Name]; exists { result[role.Name] = role } From 67965bdfdd392ffca8871671fb2fb3d62b9bd1aa Mon Sep 17 00:00:00 2001 From: Igor Karpukhin Date: Wed, 8 Jan 2025 14:59:04 +0100 Subject: [PATCH 5/8] Changes according to review --- api/v1/atlascustomrole_types.go | 1 - internal/controller/atlasproject/custom_roles.go | 9 --------- 2 files changed, 10 deletions(-) diff --git a/api/v1/atlascustomrole_types.go b/api/v1/atlascustomrole_types.go index 28289f26c9..bb6ca62074 100644 --- a/api/v1/atlascustomrole_types.go +++ b/api/v1/atlascustomrole_types.go @@ -17,7 +17,6 @@ func init() { // +kubebuilder:object:root=true // +kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].status` // +kubebuilder:printcolumn:name="Name",type=string,JSONPath=`.spec.role.name` -// +kubebuilder:printcolumn:name="Project ID",type=string,JSONPath=`.spec.projectIDRef.id` // +kubebuilder:subresource:status // +groupName:=atlas.mongodb.com // +kubebuilder:resource:categories=atlas,shortName=acr diff --git a/internal/controller/atlasproject/custom_roles.go b/internal/controller/atlasproject/custom_roles.go index f82345ce53..28346c1384 100644 --- a/internal/controller/atlasproject/custom_roles.go +++ b/internal/controller/atlasproject/custom_roles.go @@ -229,22 +229,13 @@ func mapCustomRolesByName(customRoles []customroles.CustomRole) map[string]custo type CustomRolesOperations struct { Create map[string]customroles.CustomRole Update map[string]customroles.CustomRole - Delete map[string]customroles.CustomRole } func calculateChanges(currentCustomRoles []customroles.CustomRole, desiredCustomRoles []customroles.CustomRole) CustomRolesOperations { currentCustomRolesByName := mapCustomRolesByName(currentCustomRoles) - desiredCustomRolesByName := mapCustomRolesByName(desiredCustomRoles) ops := CustomRolesOperations{ Create: map[string]customroles.CustomRole{}, Update: map[string]customroles.CustomRole{}, - Delete: map[string]customroles.CustomRole{}, - } - - for _, currentCustomRole := range currentCustomRoles { - if _, ok := desiredCustomRolesByName[currentCustomRole.Name]; !ok { - ops.Delete[currentCustomRole.Name] = currentCustomRole - } } for _, desiredCustomRole := range desiredCustomRoles { From 358d4b8986572983b254d47439f99cbd70788430 Mon Sep 17 00:00:00 2001 From: Igor Karpukhin Date: Wed, 8 Jan 2025 15:01:12 +0100 Subject: [PATCH 6/8] Applied suggestion --- .../controller/atlasproject/custom_roles.go | 23 +++++++------------ 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/internal/controller/atlasproject/custom_roles.go b/internal/controller/atlasproject/custom_roles.go index 28346c1384..20e11029c8 100644 --- a/internal/controller/atlasproject/custom_roles.go +++ b/internal/controller/atlasproject/custom_roles.go @@ -52,22 +52,15 @@ func getLastAppliedCustomRoles(atlasProject *akov2.AtlasProject) ([]akov2.Custom func findRolesToDelete(prevSpec, akoRoles, atlasRoles []customroles.CustomRole) map[string]customroles.CustomRole { result := map[string]customroles.CustomRole{} - deletionCandidates := map[string]customroles.CustomRole{} + lastAppliedRolesMap := mapCustomRolesByName(prevSpec) + akoRolesMap := mapCustomRolesByName(akoRoles) atlasRolesMap := mapCustomRolesByName(atlasRoles) - - // Get roles from the previous spec - for _, customRole := range prevSpec { - deletionCandidates[customRole.Name] = customRole - } - // Get roles from the current spec and remove the ones that are in the previous spec - for _, customRole := range akoRoles { - delete(deletionCandidates, customRole.Name) - } - - // Compare combinedAkoRoles with the current Atlas roles - for _, role := range deletionCandidates { - if _, exists := atlasRolesMap[role.Name]; exists { - result[role.Name] = role + + for atlasIx, atlasRole := range atlasRolesMap { + _, inAKO := akoRolesMap[atlasIx] + _, inLastApplied := lastAppliedRolesMap[atlasIx] + if !inAKO && inLastApplied { + result[atlasIx] = atlasRole } } From b89578a079326b0df4a9246513a8a7e6f3fd9adb Mon Sep 17 00:00:00 2001 From: Igor Karpukhin Date: Wed, 8 Jan 2025 15:18:07 +0100 Subject: [PATCH 7/8] Fixed lint --- internal/controller/atlasproject/custom_roles.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/atlasproject/custom_roles.go b/internal/controller/atlasproject/custom_roles.go index 20e11029c8..eea53f9985 100644 --- a/internal/controller/atlasproject/custom_roles.go +++ b/internal/controller/atlasproject/custom_roles.go @@ -55,7 +55,7 @@ func findRolesToDelete(prevSpec, akoRoles, atlasRoles []customroles.CustomRole) lastAppliedRolesMap := mapCustomRolesByName(prevSpec) akoRolesMap := mapCustomRolesByName(akoRoles) atlasRolesMap := mapCustomRolesByName(atlasRoles) - + for atlasIx, atlasRole := range atlasRolesMap { _, inAKO := akoRolesMap[atlasIx] _, inLastApplied := lastAppliedRolesMap[atlasIx] From 085307200028aee819f4db4b59b206b85bbfe8bb Mon Sep 17 00:00:00 2001 From: Igor Karpukhin Date: Thu, 9 Jan 2025 09:36:47 +0100 Subject: [PATCH 8/8] Update internal/controller/atlasproject/custom_roles.go Co-authored-by: josvaz --- internal/controller/atlasproject/custom_roles.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/controller/atlasproject/custom_roles.go b/internal/controller/atlasproject/custom_roles.go index eea53f9985..27886298bc 100644 --- a/internal/controller/atlasproject/custom_roles.go +++ b/internal/controller/atlasproject/custom_roles.go @@ -56,11 +56,11 @@ func findRolesToDelete(prevSpec, akoRoles, atlasRoles []customroles.CustomRole) akoRolesMap := mapCustomRolesByName(akoRoles) atlasRolesMap := mapCustomRolesByName(atlasRoles) - for atlasIx, atlasRole := range atlasRolesMap { - _, inAKO := akoRolesMap[atlasIx] - _, inLastApplied := lastAppliedRolesMap[atlasIx] + for atlasName, atlasRole := range atlasRolesMap { + _, inAKO := akoRolesMap[atlasName] + _, inLastApplied := lastAppliedRolesMap[atlasName] if !inAKO && inLastApplied { - result[atlasIx] = atlasRole + result[atlasName] = atlasRole } }