From 06a0796943d89b105912ddf8f2cca8ee0bb7e20c Mon Sep 17 00:00:00 2001 From: Matt White <16320656+matt-FFFFFF@users.noreply.github.com> Date: Wed, 10 Jul 2024 15:39:31 +0100 Subject: [PATCH] fix: do not create policy role assignment is parameter has no value and is optional (#138) --- alzlib.go | 17 +++++++++++++++++ alzlib_test.go | 14 ++++++++++++++ assets/policyDefinition.go | 14 ++++++++++++++ deployment/managementgroup.go | 11 +++++++++-- integrationtest/alzlib_test.go | 12 +++++++++--- integrationtest/examples_test.go | 5 ----- .../simple/simple.alz_archetype_definition.json | 15 --------------- .../simple/simple.alz_archetype_definition.yaml | 10 ++++++++++ .../simple.alz_architecture_definition.yaml | 14 ++++++++++++++ .../simple/simpleo.alz_archetype_override.json | 6 ------ .../simple/simpleo.alz_archetype_override.yaml | 7 +++++++ testdata/simple/test.alz_policy_assignment.json | 2 +- .../simple/test.alz_policy_default_value.yml | 6 ++++++ 13 files changed, 101 insertions(+), 32 deletions(-) delete mode 100644 testdata/simple/simple.alz_archetype_definition.json create mode 100644 testdata/simple/simple.alz_archetype_definition.yaml create mode 100644 testdata/simple/simple.alz_architecture_definition.yaml delete mode 100644 testdata/simple/simpleo.alz_archetype_override.json create mode 100644 testdata/simple/simpleo.alz_archetype_override.yaml create mode 100644 testdata/simple/test.alz_policy_default_value.yml diff --git a/alzlib.go b/alzlib.go index 16fc77b..77a51df 100644 --- a/alzlib.go +++ b/alzlib.go @@ -11,6 +11,7 @@ import ( "net/url" "os" "path/filepath" + "slices" "strings" "sync" @@ -173,6 +174,7 @@ func (az *AlzLib) PolicyAssignments() []string { for k := range az.policyAssignments { result = append(result, k) } + slices.Sort(result) return result } @@ -184,6 +186,7 @@ func (az *AlzLib) PolicyDefinitions() []string { for k := range az.policyDefinitions { result = append(result, k) } + slices.Sort(result) return result } @@ -195,6 +198,7 @@ func (az *AlzLib) PolicySetDefinitions() []string { for k := range az.policySetDefinitions { result = append(result, k) } + slices.Sort(result) return result } @@ -206,6 +210,7 @@ func (az *AlzLib) RoleDefinitions() []string { for k := range az.roleDefinitions { result = append(result, k) } + slices.Sort(result) return result } @@ -217,6 +222,7 @@ func (az *AlzLib) Archetypes() []string { for k := range az.archetypes { result = append(result, k) } + slices.Sort(result) return result } @@ -241,6 +247,17 @@ func (az *AlzLib) Architectures() []string { return result } +func (az *AlzLib) PolicyDefaultValues() []string { + az.mu.RLock() + defer az.mu.RUnlock() + result := make([]string, 0, len(az.defaultPolicyAssignmentValues)) + for k := range az.defaultPolicyAssignmentValues { + result = append(result, k) + } + slices.Sort(result) + return result +} + // Architecture returns the requested architecture. func (az *AlzLib) Architecture(name string) (*Architecture, error) { az.mu.RLock() diff --git a/alzlib_test.go b/alzlib_test.go index 8e3c9c3..7163b58 100644 --- a/alzlib_test.go +++ b/alzlib_test.go @@ -15,6 +15,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy" mapset "github.com/deckarep/golang-set/v2" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNewAlzLibOptions(t *testing.T) { @@ -454,3 +455,16 @@ func TestAddDefaultPolicyValues(t *testing.T) { err = az.addDefaultPolicyAssignmentValues(res) assert.ErrorContains(t, err, "assignment `assignment1` and parameter `param1` already exists in defaults") } + +func TestInitSimple(t *testing.T) { + az := NewAlzLib(nil) + ctx := context.Background() + dirfs := os.DirFS("./testdata/simple") + require.NoError(t, az.Init(ctx, dirfs)) + assert.Equal(t, []string{"empty", "simple", "simpleoverride"}, az.Archetypes()) + assert.Equal(t, []string{"test-policy-definition"}, az.PolicyDefinitions()) + assert.Equal(t, []string{"test-policy-set-definition"}, az.PolicySetDefinitions()) + assert.Equal(t, []string{"test-role-definition"}, az.RoleDefinitions()) + assert.Equal(t, []string{"override-policy-assignment", "test-policy-assignment"}, az.PolicyAssignments()) + assert.Equal(t, []string{"test"}, az.PolicyDefaultValues()) +} diff --git a/assets/policyDefinition.go b/assets/policyDefinition.go index 95a028e..565e3fb 100644 --- a/assets/policyDefinition.go +++ b/assets/policyDefinition.go @@ -91,6 +91,20 @@ func (pd *PolicyDefinition) AssignPermissionsParameterNames() ([]string, error) return names, nil } +func (pd *PolicyDefinition) ParameterIsOptional(name string) (bool, error) { + if pd == nil || pd.Properties == nil || pd.Properties.Parameters == nil { + return false, errors.New("PolicyDefinition.ParameterIsOptional: policy definition is nil, missing properties or parameters") + } + param, ok := pd.Properties.Parameters[name] + if !ok { + return false, fmt.Errorf("PolicyDefinition.ParameterIsOptional: parameter %s not found in policy definition", name) + } + if param.DefaultValue == nil { + return false, nil + } + return true, nil +} + func (pd *PolicyDefinition) Parameter(name string) *armpolicy.ParameterDefinitionsValue { if pd == nil || pd.Properties == nil || pd.Properties.Parameters == nil { return nil diff --git a/deployment/managementgroup.go b/deployment/managementgroup.go index dc9619e..a4e6ea3 100644 --- a/deployment/managementgroup.go +++ b/deployment/managementgroup.go @@ -201,11 +201,18 @@ func (mg *HierarchyManagementGroup) generatePolicyAssignmentAdditionalRoleAssign return fmt.Errorf("ManagementGroup.GeneratePolicyAssignmentAdditionalRoleAssignments: error getting assign permissions parameter names for policy definition `%s`: %w", *pd.Name, err) } for _, paramName := range assignPermissionParams { - - paParamVal, err := pa.ParameterValueAsString(paramName) + paramIsOptional, err := pd.ParameterIsOptional(paramName) if err != nil { + return fmt.Errorf("ManagementGroup.GeneratePolicyAssignmentAdditionalRoleAssignments: error getting parameter %s optional status for policy definition `%s`: %w", paramName, *pd.Name, err) + } + paParamVal, err := pa.ParameterValueAsString(paramName) + if err != nil && !paramIsOptional { return fmt.Errorf("ManagementGroup.GeneratePolicyAssignmentAdditionalRoleAssignments: error getting parameter value for parameter `%s` in policy assignment `%s`: %w", paramName, paName, err) } + // We should assign permissions but the parameter os optional and doesn't have a value in the assignment, so skip. + if err != nil && paramIsOptional { + continue + } if paParamVal == "" { continue } diff --git a/integrationtest/alzlib_test.go b/integrationtest/alzlib_test.go index b1ddd03..db57611 100644 --- a/integrationtest/alzlib_test.go +++ b/integrationtest/alzlib_test.go @@ -13,6 +13,11 @@ import ( "github.com/stretchr/testify/require" ) +const ( + alzLibraryTag = "2024.07.02" + alzLibraryMember = "platform/alz" +) + func TestNewAlzLibOptionsError(t *testing.T) { az := new(alzlib.AlzLib) ctx, cancel := context.WithCancel(context.Background()) @@ -33,11 +38,12 @@ func TestInitMultiLib(t *testing.T) { dirfs := os.DirFS("../testdata/simple") err = az.Init(ctx, remoteLib, dirfs) require.NoError(t, err) - assert.Equal(t, 12, len(az.Archetypes())) + assert.Equal(t, 13, len(az.Archetypes())) // Test root archetype has been overridden arch, _ := az.Archetype("root") - assert.Equal(t, 1, arch.PolicyDefinitions.Cardinality()) - arch, _ = az.Archetype("simpleo") + assert.Equal(t, 158, arch.PolicyDefinitions.Cardinality()) + arch, err = az.Archetype("simpleoverride") + require.NoError(t, err) assert.Equal(t, 1, arch.PolicyDefinitions.Cardinality()) assert.Equal(t, 1, arch.PolicyAssignments.Cardinality()) } diff --git a/integrationtest/examples_test.go b/integrationtest/examples_test.go index aadb160..761e0a1 100644 --- a/integrationtest/examples_test.go +++ b/integrationtest/examples_test.go @@ -14,11 +14,6 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy" ) -const ( - alzLibraryTag = "2024.07.01" - alzLibraryMember = "platform/alz" -) - // Example_deploymentNewHierarchy tests the ALZ reference architecture creation in full. func Example_deploymentNewHierarchy() { az := alzlib.NewAlzLib(nil) diff --git a/testdata/simple/simple.alz_archetype_definition.json b/testdata/simple/simple.alz_archetype_definition.json deleted file mode 100644 index a7ba37b..0000000 --- a/testdata/simple/simple.alz_archetype_definition.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "name": "root", - "policy_assignments": [ - "test-policy-assignment" - ], - "policy_definitions": [ - "test-policy-definition" - ], - "policy_set_definitions": [ - "test-policy-set-definition" - ], - "role_definitions": [ - "test-role-definition" - ] -} diff --git a/testdata/simple/simple.alz_archetype_definition.yaml b/testdata/simple/simple.alz_archetype_definition.yaml new file mode 100644 index 0000000..5d17aa7 --- /dev/null +++ b/testdata/simple/simple.alz_archetype_definition.yaml @@ -0,0 +1,10 @@ +--- +name: simple +policy_assignments: + - test-policy-assignment +policy_definitions: + - test-policy-definition +policy_set_definitions: + - test-policy-set-definition +role_definitions: + - test-role-definition diff --git a/testdata/simple/simple.alz_architecture_definition.yaml b/testdata/simple/simple.alz_architecture_definition.yaml new file mode 100644 index 0000000..03fd1fd --- /dev/null +++ b/testdata/simple/simple.alz_architecture_definition.yaml @@ -0,0 +1,14 @@ +management_groups: + - archetypes: + - simple + display_name: simple + exists: false + id: simple + parent_id: null + - archetypes: + - simpleoverride + display_name: simple overide + exists: false + id: simpleoverride + parent_id: null +name: simple diff --git a/testdata/simple/simpleo.alz_archetype_override.json b/testdata/simple/simpleo.alz_archetype_override.json deleted file mode 100644 index 4ce30f6..0000000 --- a/testdata/simple/simpleo.alz_archetype_override.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "base_archetype": "root", - "name": "simpleo", - "policy_assignments_to_add": ["override-policy-assignment"], - "policy_assignments_to_remove": ["test-policy-assignment"] -} diff --git a/testdata/simple/simpleo.alz_archetype_override.yaml b/testdata/simple/simpleo.alz_archetype_override.yaml new file mode 100644 index 0000000..5ed1e2b --- /dev/null +++ b/testdata/simple/simpleo.alz_archetype_override.yaml @@ -0,0 +1,7 @@ +--- +base_archetype: simple +name: simpleoverride +policy_assignments_to_add: + - override-policy-assignment +policy_assignments_to_remove: + - test-policy-assignment diff --git a/testdata/simple/test.alz_policy_assignment.json b/testdata/simple/test.alz_policy_assignment.json index d10b022..ab52512 100644 --- a/testdata/simple/test.alz_policy_assignment.json +++ b/testdata/simple/test.alz_policy_assignment.json @@ -17,7 +17,7 @@ ], "notScopes": [], "parameters": {}, - "policyDefinitionId": "/providers/Microsoft.Authorization/policySetDefinitions/test-policy-set-definition", + "policyDefinitionId": "/providers/Microsoft.Authorization/policySetDefinitions/test-policy-definition", "scope": "/providers/Microsoft.Management/managementGroups/PLACEHOLDER" }, "type": "Microsoft.Authorization/policyAssignments" diff --git a/testdata/simple/test.alz_policy_default_value.yml b/testdata/simple/test.alz_policy_default_value.yml new file mode 100644 index 0000000..c517213 --- /dev/null +++ b/testdata/simple/test.alz_policy_default_value.yml @@ -0,0 +1,6 @@ +--- +name: test +policy_assignments: + - policy_assignment_name: test-policy-assignment + parameter_names: + - effect