Skip to content

Commit eb92de7

Browse files
jaredfholgatejaredfholgate
authored
feat: allow specified role definition id and name (#208)
* feat: allow specified role definition id and name * improve code and tests * improve test
1 parent 5976bab commit eb92de7

File tree

5 files changed

+166
-24
lines changed

5 files changed

+166
-24
lines changed

‎alzlib.go‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,9 @@ import (
2020
)
2121

2222
const (
23-
defaultParallelism = 10 // default number of parallel requests to make to Azure APIs
24-
defaultOverwrite = false
23+
defaultParallelism = 10 // default number of parallel requests to make to Azure APIs
24+
defaultOverwrite = false
25+
defaultUniqueRoleDefinitions = true // default to unique role definitions per management group
2526
)
2627

2728
// AlzLib is the structure that gets built from the the library files
@@ -48,8 +49,9 @@ type azureClients struct {
4849

4950
// AlzLibOptions are options for the AlzLib.
5051
type AlzLibOptions struct {
51-
AllowOverwrite bool // AllowOverwrite allows overwriting of existing policy assignments when processing additional libraries with AlzLib.Init().
52-
Parallelism int // Parallelism is the number of parallel requests to make to Azure APIs when getting policy definitions and policy set definitions.
52+
AllowOverwrite bool // AllowOverwrite allows overwriting of existing policy assignments when processing additional libraries with AlzLib.Init().
53+
Parallelism int // Parallelism is the number of parallel requests to make to Azure APIs when getting policy definitions and policy set definitions.
54+
UniqueRoleDefinitions bool // UniqueRoleDefinitions indicates whether to update the role definitions to be unique per management group. If this is not set, you may end up with conflicting role definition names.
5355
}
5456

5557
// NewAlzLib returns a new instance of the alzlib library, optionally using the supplied directory
@@ -77,8 +79,9 @@ func NewAlzLib(opts *AlzLibOptions) *AlzLib {
7779

7880
func defaultAlzLibOptions() *AlzLibOptions {
7981
return &AlzLibOptions{
80-
Parallelism: defaultParallelism,
81-
AllowOverwrite: defaultOverwrite,
82+
Parallelism: defaultParallelism,
83+
AllowOverwrite: defaultOverwrite,
84+
UniqueRoleDefinitions: defaultUniqueRoleDefinitions,
8285
}
8386
}
8487

‎alzlib_test.go‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,22 @@ import (
1818
"github.com/stretchr/testify/require"
1919
)
2020

21-
func TestNewAlzLibOptions(t *testing.T) {
21+
func TestNewAlzLibDefaultOptions(t *testing.T) {
2222
az := NewAlzLib(nil)
23+
assert.Equal(t, defaultOverwrite, az.Options.AllowOverwrite)
2324
assert.Equal(t, defaultParallelism, az.Options.Parallelism)
25+
assert.Equal(t, defaultUniqueRoleDefinitions, az.Options.UniqueRoleDefinitions)
26+
}
27+
28+
func TestNewAlzLibCustomOptions(t *testing.T) {
29+
az := NewAlzLib(&AlzLibOptions{
30+
AllowOverwrite: true,
31+
Parallelism: 25,
32+
UniqueRoleDefinitions: false,
33+
})
34+
assert.Equal(t, true, az.Options.AllowOverwrite)
35+
assert.Equal(t, 25, az.Options.Parallelism)
36+
assert.Equal(t, false, az.Options.UniqueRoleDefinitions)
2437
}
2538

2639
// Test_NewAlzLib_noDir tests the creation of a new AlzLib when supplied with a path

‎deployment/hierarchy.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,7 @@ func (h *Hierarchy) addManagementGroup(ctx context.Context, req managementGroupA
306306
h.mgs[req.id] = mg
307307

308308
// run Update to change all refs, etc.
309-
if err := h.mgs[req.id].update(); err != nil {
309+
if err := h.mgs[req.id].update(h.alzlib.Options.UniqueRoleDefinitions); err != nil {
310310
return nil, fmt.Errorf("Hierarchy.AddManagementGroup: adding `%s` error updating assets at scope %w", req.id, err)
311311
}
312312

‎deployment/managementgroup.go‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,7 @@ func (mg *HierarchyManagementGroup) generatePolicyAssignmentAdditionalRoleAssign
331331

332332
// update will update the AlzManagementGroup resources with the correct resource ids, references, etc.
333333
// Make sure to pass in any updates to the policy assignment parameter values.
334-
func (mg *HierarchyManagementGroup) update() error {
334+
func (mg *HierarchyManagementGroup) update(uniqueRoleDefinitions bool) error {
335335
pd2mg := mg.hierarchy.policyDefinitionToMg()
336336
psd2mg := mg.hierarchy.policySetDefinitionToMg()
337337

@@ -345,7 +345,7 @@ func (mg *HierarchyManagementGroup) update() error {
345345
}
346346

347347
// re-write the assignableScopes for the role definitions.
348-
updateRoleDefinitions(mg)
348+
updateRoleDefinitions(mg, uniqueRoleDefinitions)
349349

350350
if err := updatePolicyAsignments(mg, pd2mg, psd2mg); err != nil {
351351
return fmt.Errorf("HierarchyManagementGroup.update: error updating policy assignments: %w", err)
@@ -540,12 +540,15 @@ func updatePolicyAsignments(mg *HierarchyManagementGroup, pd2mg, psd2mg map[stri
540540
return nil
541541
}
542542

543-
func updateRoleDefinitions(alzmg *HierarchyManagementGroup) {
543+
func updateRoleDefinitions(alzmg *HierarchyManagementGroup, uniqueRoleDefinitions bool) {
544544
for _, roledef := range alzmg.roleDefinitions {
545-
u := uuidV5(alzmg.id, *roledef.Name)
546-
roledef.Name = to.Ptr(u.String())
547-
roledef.ID = to.Ptr(fmt.Sprintf(RoleDefinitionIdFmt, alzmg.id, u))
548-
roledef.Properties.RoleName = to.Ptr(fmt.Sprintf("%s (%s)", *roledef.Properties.RoleName, alzmg.id))
545+
if uniqueRoleDefinitions {
546+
u := uuidV5(alzmg.id, *roledef.Name)
547+
roledef.Name = to.Ptr(u.String())
548+
roledef.Properties.RoleName = to.Ptr(fmt.Sprintf("%s (%s)", *roledef.Properties.RoleName, alzmg.id))
549+
}
550+
roledef.ID = to.Ptr(fmt.Sprintf(RoleDefinitionIdFmt, alzmg.id, *roledef.Name))
551+
549552
if len(roledef.Properties.AssignableScopes) == 0 {
550553
roledef.Properties.AssignableScopes = make([]*string, 1)
551554
}

‎deployment/managementgroup_test.go‎

Lines changed: 132 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -413,10 +413,10 @@ func TestManagementGroupUpdate(t *testing.T) {
413413
h.mgs["mg1a"] = mg1a
414414
h.mgs["mg2"] = mg2
415415
mgRoot.children = mapset.NewThreadUnsafeSet(mg1, mg2)
416-
require.NoError(t, mgRoot.update())
417-
require.NoError(t, mg1.update())
418-
require.NoError(t, mg2.update())
419-
require.NoError(t, mg1a.update())
416+
require.NoError(t, mgRoot.update(true))
417+
require.NoError(t, mg1.update(true))
418+
require.NoError(t, mg2.update(true))
419+
require.NoError(t, mg1a.update(true))
420420

421421
// Check that the policy assignments reference the correct policy definitions.
422422
assert.Equal(t, fmt.Sprintf(PolicyDefinitionIdFmt, "mg1", "pdDeployedTwice"), *mg1a.policyAssignments["paAtMg1a"].Properties.PolicyDefinitionID)
@@ -445,7 +445,7 @@ func TestManagementGroupUpdate(t *testing.T) {
445445
roleDefinitions: map[string]*assets.RoleDefinition{},
446446
}
447447
h.mgs["mgOtherRoot"] = mgOtherRoot
448-
require.NoError(t, mgOtherRoot.update())
448+
require.NoError(t, mgOtherRoot.update(true))
449449

450450
mg1.policyAssignments["defNotInHierarchy"] = &assets.PolicyAssignment{
451451
Assignment: armpolicy.Assignment{
@@ -455,7 +455,130 @@ func TestManagementGroupUpdate(t *testing.T) {
455455
},
456456
},
457457
}
458-
assert.ErrorContains(t, mg1.update(), "policy assignment defNotInHierarchy has a policy definition pdOtherRoot that is not in the same hierarchy")
458+
assert.ErrorContains(t, mg1.update(true), "policy assignment defNotInHierarchy has a policy definition pdOtherRoot that is not in the same hierarchy")
459+
}
460+
461+
func TestManagementGroupUpdateWithUniqueRoleDefinitions(t *testing.T) {
462+
h := NewHierarchy(nil)
463+
mgRoot := &HierarchyManagementGroup{
464+
id: "mgRoot",
465+
parent: nil,
466+
level: 0,
467+
parentExternal: to.Ptr("external"),
468+
location: "changeme",
469+
hierarchy: h,
470+
policyAssignments: map[string]*assets.PolicyAssignment{},
471+
policyDefinitions: map[string]*assets.PolicyDefinition{},
472+
policySetDefinitions: map[string]*assets.PolicySetDefinition{},
473+
roleDefinitions: map[string]*assets.RoleDefinition{
474+
"rdRoot01": {
475+
RoleDefinition: armauthorization.RoleDefinition{
476+
Name: to.Ptr("8a60c97f-9cb6-536b-b5db-9c997ee1de03"),
477+
Properties: &armauthorization.RoleDefinitionProperties{
478+
RoleName: to.Ptr("[ALZ] Application-Owners"),
479+
Description: to.Ptr("Contributor role granted for application/operations team at resource group level"),
480+
},
481+
},
482+
},
483+
},
484+
}
485+
mg1 := &HierarchyManagementGroup{
486+
id: "mg1",
487+
parent: mgRoot,
488+
level: 1,
489+
location: "changeme",
490+
hierarchy: h,
491+
policyAssignments: map[string]*assets.PolicyAssignment{},
492+
policyDefinitions: map[string]*assets.PolicyDefinition{},
493+
policySetDefinitions: map[string]*assets.PolicySetDefinition{},
494+
roleDefinitions: map[string]*assets.RoleDefinition{
495+
"rdMg101": {
496+
RoleDefinition: armauthorization.RoleDefinition{
497+
Name: to.Ptr("8a60c97f-9cb6-536b-b5db-9c997ee1de03"),
498+
Properties: &armauthorization.RoleDefinitionProperties{
499+
RoleName: to.Ptr("[ALZ] Application-Owners"),
500+
Description: to.Ptr("Contributor role granted for application/operations team at resource group level"),
501+
},
502+
},
503+
},
504+
},
505+
}
506+
507+
h.mgs["mgRoot"] = mgRoot
508+
h.mgs["mg1"] = mg1
509+
510+
mgRoot.children = mapset.NewThreadUnsafeSet(mg1)
511+
require.NoError(t, mgRoot.update(true))
512+
require.NoError(t, mg1.update(true))
513+
514+
// Check that the role definitions are unique
515+
assert.NotEqual(t, *mgRoot.roleDefinitions["rdRoot01"].Name, *mg1.roleDefinitions["rdMg101"].Name, "Role definitions should not have the same ID")
516+
assert.NotEqual(t, *mgRoot.roleDefinitions["rdRoot01"].Properties.RoleName, *mg1.roleDefinitions["rdMg101"].Properties.RoleName, "Role definitions should not have the same ID")
517+
assert.NotEqual(t, *mgRoot.roleDefinitions["rdRoot01"].ID, "/providers/Microsoft.Management/managementGroups/mgRoot/providers/Microsoft.Authorization/roleDefinitions/8a60c97f-9cb6-536b-b5db-9c997ee1de03", "Role definitions should not have the same ID after update")
518+
assert.Equal(t, *mgRoot.roleDefinitions["rdRoot01"].ID, fmt.Sprintf("/providers/Microsoft.Management/managementGroups/mgRoot/providers/Microsoft.Authorization/roleDefinitions/%s", *mgRoot.roleDefinitions["rdRoot01"].Name), "Role definitions should have the same ID after update")
519+
assert.NotEqual(t, *mgRoot.roleDefinitions["rdRoot01"].Name, "8a60c97f-9cb6-536b-b5db-9c997ee1de03", "Role definitions should have the same Name after update")
520+
assert.Equal(t, *mgRoot.roleDefinitions["rdRoot01"].Properties.RoleName, fmt.Sprintf("[ALZ] Application-Owners (%s)", mgRoot.id), "Role definitions should have the same RoleName after update")
521+
}
522+
523+
func TestManagementGroupUpdateWithNonUniqueRoleDefinitions(t *testing.T) {
524+
h := NewHierarchy(nil)
525+
mgRoot := &HierarchyManagementGroup{
526+
id: "mgRoot",
527+
parent: nil,
528+
level: 0,
529+
parentExternal: to.Ptr("external"),
530+
location: "changeme",
531+
hierarchy: h,
532+
policyAssignments: map[string]*assets.PolicyAssignment{},
533+
policyDefinitions: map[string]*assets.PolicyDefinition{},
534+
policySetDefinitions: map[string]*assets.PolicySetDefinition{},
535+
roleDefinitions: map[string]*assets.RoleDefinition{
536+
"rdRoot01": {
537+
RoleDefinition: armauthorization.RoleDefinition{
538+
Name: to.Ptr("8a60c97f-9cb6-536b-b5db-9c997ee1de03"),
539+
Properties: &armauthorization.RoleDefinitionProperties{
540+
RoleName: to.Ptr("[ALZ] Application-Owners"),
541+
Description: to.Ptr("Contributor role granted for application/operations team at resource group level"),
542+
},
543+
},
544+
},
545+
},
546+
}
547+
mg1 := &HierarchyManagementGroup{
548+
id: "mg1",
549+
parent: mgRoot,
550+
level: 1,
551+
location: "changeme",
552+
hierarchy: h,
553+
policyAssignments: map[string]*assets.PolicyAssignment{},
554+
policyDefinitions: map[string]*assets.PolicyDefinition{},
555+
policySetDefinitions: map[string]*assets.PolicySetDefinition{},
556+
roleDefinitions: map[string]*assets.RoleDefinition{
557+
"rdMg101": {
558+
RoleDefinition: armauthorization.RoleDefinition{
559+
Name: to.Ptr("8a60c97f-9cb6-536b-b5db-9c997ee1de03"),
560+
Properties: &armauthorization.RoleDefinitionProperties{
561+
RoleName: to.Ptr("[ALZ] Application-Owners"),
562+
Description: to.Ptr("Contributor role granted for application/operations team at resource group level"),
563+
},
564+
},
565+
},
566+
},
567+
}
568+
569+
h.mgs["mgRoot"] = mgRoot
570+
h.mgs["mg1"] = mg1
571+
572+
mgRoot.children = mapset.NewThreadUnsafeSet(mg1)
573+
require.NoError(t, mgRoot.update(false))
574+
require.NoError(t, mg1.update(false))
575+
576+
// Check that the role definitions are still not unique after update
577+
assert.Equal(t, *mgRoot.roleDefinitions["rdRoot01"].Name, *mg1.roleDefinitions["rdMg101"].Name, "Role definitions should not have the same ID after update")
578+
assert.Equal(t, *mgRoot.roleDefinitions["rdRoot01"].Properties.RoleName, *mg1.roleDefinitions["rdMg101"].Properties.RoleName, "Role definitions should not have the same ID after update")
579+
assert.Equal(t, *mgRoot.roleDefinitions["rdRoot01"].ID, "/providers/Microsoft.Management/managementGroups/mgRoot/providers/Microsoft.Authorization/roleDefinitions/8a60c97f-9cb6-536b-b5db-9c997ee1de03", "Role definitions should have the same ID after update")
580+
assert.Equal(t, *mgRoot.roleDefinitions["rdRoot01"].Name, "8a60c97f-9cb6-536b-b5db-9c997ee1de03", "Role definitions should have the same Name after update")
581+
assert.Equal(t, *mgRoot.roleDefinitions["rdRoot01"].Properties.RoleName, "[ALZ] Application-Owners", "Role definitions should have the same RoleName after update")
459582
}
460583

461584
func TestModifyPolicyDefinitions(t *testing.T) {
@@ -589,7 +712,7 @@ func TestModifyRoleDefinitions(t *testing.T) {
589712
}),
590713
},
591714
}
592-
updateRoleDefinitions(alzmg)
715+
updateRoleDefinitions(alzmg, true)
593716
expected := fmt.Sprintf(RoleDefinitionIdFmt, "mg1", uuidV5("mg1", "role1"))
594717
assert.Equal(t, expected, *alzmg.roleDefinitions["rd1"].ID)
595718
assert.Len(t, alzmg.roleDefinitions["rd1"].Properties.AssignableScopes, 1)
@@ -616,7 +739,7 @@ func TestModifyRoleDefinitions(t *testing.T) {
616739
}),
617740
},
618741
}
619-
updateRoleDefinitions(alzmg)
742+
updateRoleDefinitions(alzmg, true)
620743
expected = fmt.Sprintf(RoleDefinitionIdFmt, "mg1", uuidV5("mg1", "role1"))
621744
assert.Equal(t, expected, *alzmg.roleDefinitions["rd1"].ID)
622745
assert.Len(t, alzmg.roleDefinitions["rd1"].Properties.AssignableScopes, 1)
@@ -633,7 +756,7 @@ func TestModifyRoleDefinitions(t *testing.T) {
633756
id: "mg1",
634757
roleDefinitions: map[string]*assets.RoleDefinition{},
635758
}
636-
updateRoleDefinitions(alzmg)
759+
updateRoleDefinitions(alzmg, true)
637760
assert.Empty(t, alzmg.roleDefinitions)
638761
}
639762

0 commit comments

Comments
 (0)