From a5fadd88363ea53d9367cdcc2318e9ad162df5ab Mon Sep 17 00:00:00 2001 From: Yingjie Date: Thu, 3 Sep 2026 14:35:42 +0800 Subject: [PATCH] Match policy parameter names case-insensitively Azure Resource Manager treats template function parameters as case-insensitive and Azure Policy artifacts rely on this. The built-in Windows AMA/DCR initiative 9575b8b7-78ab-4281-b53b-d3c1ace2260b passes `DcrResourceId` to policy definition eab1f514-22e3-42e3-9a1f-e1dc9199355c, which declares `dcrResourceId`. alzlib resolved parameter names with exact Go map key lookups and therefore rejected an artifact that Azure itself accepts. Two defects sit on that path: * Hierarchy.AddManagementGroup returned a hard error, aborting `terraform plan`, `alzlibtool check library` and `generate architecture` before anything was computed. * parseArmFunctionInPolicySetParameter could not resolve the initiative reference key from the member definition's canonical parameter name, so the assignPermissions role assignment for the data collection rule scope was dropped. terraform-provider-alz downgrades the resulting PolicyRoleAssignmentErrors to a warning and still emits a partial policy_role_assignments set, so the first defect masked the second. Add internal/parametername, a generic resolver that prefers an exact match and falls back to a unique strings.EqualFold match, returning the canonical key declared by the artifact. A fallback that matches more than one key returns an explicit error listing the sorted candidates, so the outcome never depends on Go map iteration order. Route the Azure artifact parameter-name lookups through the resolver: * assets: add PolicyDefinition.ResolveParameter and delegate Parameter to it; apply the same contract to PolicySetDefinition.Parameter, PolicyAssignment.ParameterValueAsString, ParameterIsOptional and Set/UnsetAssignPermissionsOnParameter, which previously no-opped silently on a case-only variant. * deployment: distinguish missing from ambiguous in the hierarchy validation error; resolve the member reference key in parseArmFunctionInPolicySetParameter and guard against a nil parameter value; map assignment parameters onto the canonical initiative parameter before evaluating the ARM expression so a case-only variant overrides the initiative default; write assignment parameters under the casing already present in the assignment so WithParameters cannot emit two parameters that differ only in case. Remove the dead branch in generatePolicyAssignmentAdditionalRoleAssignments that looked up a key in the map it was iterating and could never fail. Parameters that do not exist in any casing still fail hard, preserving the guardrail added in #127. Fixes Azure/Azure-Landing-Zones#4248 --- assets/policyAssignment.go | 16 +- assets/policyDefinition.go | 62 ++++-- assets/policyDefinition_test.go | 94 ++++++++ assets/policySetDefinition.go | 9 +- deployment/hierarchy.go | 15 +- deployment/managementgroup.go | 70 +++--- deployment/parameterNameCasing_test.go | 292 +++++++++++++++++++++++++ internal/parametername/resolve.go | 60 +++++ internal/parametername/resolve_test.go | 87 ++++++++ 9 files changed, 644 insertions(+), 61 deletions(-) create mode 100644 deployment/parameterNameCasing_test.go create mode 100644 internal/parametername/resolve.go create mode 100644 internal/parametername/resolve_test.go diff --git a/assets/policyAssignment.go b/assets/policyAssignment.go index bdabe16..28a853f 100644 --- a/assets/policyAssignment.go +++ b/assets/policyAssignment.go @@ -7,6 +7,7 @@ import ( "fmt" "unicode/utf8" + "github.com/Azure/alzlib/internal/parametername" "github.com/Azure/alzlib/to" "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy" @@ -63,12 +64,22 @@ func (pa *PolicyAssignment) ReferencedPolicyDefinitionResourceIDAndVersion() (*a } // ParameterValueAsString returns the value of a policy assignment parameter. +// The parameter name is matched exactly first, then case-insensitively, as Azure Resource Manager +// treats parameter names as case-insensitive. // We always expect the value to be a string as it's used in calculating the additional role // assignments for // policy parameters with the assignPermissions metadata set to true. // Therefore the value should be an ARM resourceId. func (pa *PolicyAssignment) ParameterValueAsString(paramName string) (string, error) { - paParamVal, ok := pa.Properties.Parameters[paramName] + match, ok, err := parametername.Resolve(pa.Properties.Parameters, paramName) + if err != nil { + return "", fmt.Errorf( + "PolicyAssignment.ParameterValueAsString: policy assignment %s: %w", + *pa.Name, + err, + ) + } + if !ok { return "", fmt.Errorf( "PolicyAssignment.ParameterValueAsString: parameter %s not found in policy assignment %s", @@ -77,7 +88,8 @@ func (pa *PolicyAssignment) ParameterValueAsString(paramName string) (string, er ) } - if paParamVal.Value == nil { + paParamVal := match.Value + if paParamVal == nil || paParamVal.Value == nil { return "", fmt.Errorf( "PolicyAssignment.ParameterValueAsString: parameter %s value field in policy assignment %s is nil", paramName, diff --git a/assets/policyDefinition.go b/assets/policyDefinition.go index 2ca494e..ba679e2 100644 --- a/assets/policyDefinition.go +++ b/assets/policyDefinition.go @@ -9,6 +9,7 @@ import ( "fmt" "unicode/utf8" + "github.com/Azure/alzlib/internal/parametername" "github.com/Azure/alzlib/to" "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy" @@ -242,6 +243,7 @@ func (pd *PolicyDefinition) AssignPermissionsParameterNames() ([]string, error) } // ParameterIsOptional checks if the parameter with the given name is optional in the policy definition. +// The name is matched case-insensitively if there is no exact match. func (pd *PolicyDefinition) ParameterIsOptional(name string) (bool, error) { if pd == nil || pd.Properties == nil || pd.Properties.Parameters == nil { return false, errors.New( @@ -249,8 +251,12 @@ func (pd *PolicyDefinition) ParameterIsOptional(name string) (bool, error) { ) } - param, ok := pd.Properties.Parameters[name] - if !ok { + _, param, found, err := pd.ResolveParameter(name) + if err != nil { + return false, err + } + + if !found { return false, fmt.Errorf( "PolicyDefinition.ParameterIsOptional: parameter %s not found in policy definition", name, @@ -264,30 +270,45 @@ func (pd *PolicyDefinition) ParameterIsOptional(name string) (bool, error) { return true, nil } -// Parameter returns the parameter with the given name from the policy definition. -func (pd *PolicyDefinition) Parameter(name string) *armpolicy.ParameterDefinitionsValue { +// ResolveParameter returns the canonical name and value of the parameter matching the given name. +// Names are matched exactly first, then case-insensitively, as Azure Resource Manager treats +// parameter names as case-insensitive. +// The boolean return value reports whether a matching parameter was declared. An error is returned +// if the name matches more than one parameter when compared case-insensitively. +func (pd *PolicyDefinition) ResolveParameter( + name string, +) (string, *armpolicy.ParameterDefinitionsValue, bool, error) { if pd == nil || pd.Properties == nil || pd.Properties.Parameters == nil { - return nil + return "", nil, false, nil } - ret, ok := pd.Properties.Parameters[name] - if !ok { + match, found, err := parametername.Resolve(pd.Properties.Parameters, name) + if err != nil { + return "", nil, false, fmt.Errorf("PolicyDefinition.ResolveParameter: %w", err) + } + + return match.Key, match.Value, found, nil +} + +// Parameter returns the parameter with the given name from the policy definition. +// The name is matched case-insensitively if there is no exact match. +// It returns nil if the parameter is not found, or if the name is ambiguous. +func (pd *PolicyDefinition) Parameter(name string) *armpolicy.ParameterDefinitionsValue { + _, param, found, err := pd.ResolveParameter(name) + if err != nil || !found { return nil } - return ret + return param } // SetAssignPermissionsOnParameter sets the AssignPermissions metadata field to true for the // parameter with the given // name. +// The name is matched case-insensitively if there is no exact match. func (pd *PolicyDefinition) SetAssignPermissionsOnParameter(parameterName string) { - if pd == nil || pd.Properties == nil || pd.Properties.Parameters == nil { - return - } - - param, ok := pd.Properties.Parameters[parameterName] - if !ok { + param := pd.Parameter(parameterName) + if param == nil { return } @@ -300,17 +321,10 @@ func (pd *PolicyDefinition) SetAssignPermissionsOnParameter(parameterName string // UnsetAssignPermissionsOnParameter removes the AssignPermissions metadata field for the parameter // with the given name. +// The name is matched case-insensitively if there is no exact match. func (pd *PolicyDefinition) UnsetAssignPermissionsOnParameter(parameterName string) { - if pd == nil || pd.Properties == nil || pd.Properties.Parameters == nil { - return - } - - param, ok := pd.Properties.Parameters[parameterName] - if !ok { - return - } - - if param.Metadata == nil { + param := pd.Parameter(parameterName) + if param == nil || param.Metadata == nil { return } diff --git a/assets/policyDefinition_test.go b/assets/policyDefinition_test.go index a22746f..d8d309a 100644 --- a/assets/policyDefinition_test.go +++ b/assets/policyDefinition_test.go @@ -85,6 +85,100 @@ func TestUnsetAssignPermissionsOnParameter(t *testing.T) { assert.Nil(t, pd.Properties.Parameters["test"].Metadata.AssignPermissions) } +// TestResolveParameterCaseInsensitive covers the built-in Windows AMA/DCR shape, where the +// initiative passes `DcrResourceId` to a definition declaring `dcrResourceId`. +func TestResolveParameterCaseInsensitive(t *testing.T) { + pd := &PolicyDefinition{ + Definition: armpolicy.Definition{ + Properties: &armpolicy.DefinitionProperties{ + Parameters: map[string]*armpolicy.ParameterDefinitionsValue{ + "dcrResourceId": {Type: to.Ptr(armpolicy.ParameterTypeString)}, + }, + }, + }, + } + + name, param, found, err := pd.ResolveParameter("dcrResourceId") + require.NoError(t, err) + assert.True(t, found) + assert.Equal(t, "dcrResourceId", name) + assert.NotNil(t, param) + + name, param, found, err = pd.ResolveParameter("DcrResourceId") + require.NoError(t, err) + assert.True(t, found) + assert.Equal(t, "dcrResourceId", name) + assert.NotNil(t, param) + assert.NotNil(t, pd.Parameter("DcrResourceId")) + + _, _, found, err = pd.ResolveParameter("minPort") + require.NoError(t, err) + assert.False(t, found) + assert.Nil(t, pd.Parameter("minPort")) +} + +func TestResolveParameterAmbiguous(t *testing.T) { + pd := &PolicyDefinition{ + Definition: armpolicy.Definition{ + Properties: &armpolicy.DefinitionProperties{ + Parameters: map[string]*armpolicy.ParameterDefinitionsValue{ + "dcrResourceId": {Type: to.Ptr(armpolicy.ParameterTypeString)}, + "DCRRESOURCEID": {Type: to.Ptr(armpolicy.ParameterTypeString)}, + }, + }, + }, + } + + _, _, found, err := pd.ResolveParameter("DcrResourceId") + require.Error(t, err) + assert.False(t, found) + require.ErrorContains(t, err, "is ambiguous") + assert.Nil(t, pd.Parameter("DcrResourceId")) +} + +// TestAssignPermissionsAndOptionalParameterCaseInsensitive asserts that the parameter-name based +// helpers agree with Parameter() on casing, so a case-only variant does not silently no-op. +func TestAssignPermissionsAndOptionalParameterCaseInsensitive(t *testing.T) { + pd := &PolicyDefinition{ + Definition: armpolicy.Definition{ + Properties: &armpolicy.DefinitionProperties{ + Parameters: map[string]*armpolicy.ParameterDefinitionsValue{ + "dcrResourceId": {Type: to.Ptr(armpolicy.ParameterTypeString)}, + }, + }, + }, + } + + optional, err := pd.ParameterIsOptional("DcrResourceId") + require.NoError(t, err) + assert.False(t, optional) + + pd.SetAssignPermissionsOnParameter("DcrResourceId") + require.NotNil(t, pd.Properties.Parameters["dcrResourceId"].Metadata) + assert.True(t, *pd.Properties.Parameters["dcrResourceId"].Metadata.AssignPermissions) + + pd.UnsetAssignPermissionsOnParameter("DCRRESOURCEID") + assert.Nil(t, pd.Properties.Parameters["dcrResourceId"].Metadata.AssignPermissions) + + _, err = pd.ParameterIsOptional("minPort") + require.ErrorContains(t, err, "not found in policy definition") +} + +func TestResolveParameterNilReceiverAndProperties(t *testing.T) { + var pd *PolicyDefinition + + _, _, found, err := pd.ResolveParameter("dcrResourceId") + require.NoError(t, err) + assert.False(t, found) + assert.Nil(t, pd.Parameter("dcrResourceId")) + + pd = &PolicyDefinition{Definition: armpolicy.Definition{}} + + _, _, found, err = pd.ResolveParameter("dcrResourceId") + require.NoError(t, err) + assert.False(t, found) +} + func TestNewPolicyDefinitionFromVersionSuccess(t *testing.T) { versionID := "/subscriptions/00000000-0000-0000-0000-000000000000/providers/" + "Microsoft.Authorization/policyDefinitions/myPolicy/versions/1.0.0" diff --git a/assets/policySetDefinition.go b/assets/policySetDefinition.go index 3b6031e..d7a31e2 100644 --- a/assets/policySetDefinition.go +++ b/assets/policySetDefinition.go @@ -9,6 +9,7 @@ import ( "fmt" "unicode/utf8" + "github.com/Azure/alzlib/internal/parametername" "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy" ) @@ -152,17 +153,19 @@ func (psd *PolicySetDefinition) PolicyDefinitionReferences() []*armpolicy.Defini } // Parameter returns the parameter definition for the given name. +// The name is matched case-insensitively if there is no exact match. +// It returns nil if the parameter is not found, or if the name is ambiguous. func (psd *PolicySetDefinition) Parameter(name string) *armpolicy.ParameterDefinitionsValue { if psd == nil || psd.Properties == nil || psd.Properties.Parameters == nil { return nil } - ret, ok := psd.Properties.Parameters[name] - if !ok { + match, found, err := parametername.Resolve(psd.Properties.Parameters, name) + if err != nil || !found { return nil } - return ret + return match.Value } // GetVersion returns the version of the policy definition, if it exists. diff --git a/deployment/hierarchy.go b/deployment/hierarchy.go index 3b67a34..72e8c54 100644 --- a/deployment/hierarchy.go +++ b/deployment/hierarchy.go @@ -432,7 +432,20 @@ func (h *Hierarchy) addManagementGroup( } for param := range rf.Parameters { - if pd.Parameter(param) == nil { + _, _, found, err := pd.ResolveParameter(param) + if err != nil { + return nil, fmt.Errorf( + "Hierarchy.AddManagementGroup(): parameter `%s` in policy set definition `%s` "+ + "is ambiguous in referenced definition `%s` in management group `%s`: %w", + param, + *psd.Name, + alzlib.JoinNameAndVersion(*pd.Name, rf.DefinitionVersion), + req.id, + err, + ) + } + + if !found { return nil, fmt.Errorf( "Hierarchy.AddManagementGroup(): parameter `%s` in policy set definition `%s` "+ "does not match a parameter in referenced definition `%s` in management group `%s`", diff --git a/deployment/managementgroup.go b/deployment/managementgroup.go index ea96f9f..2a92a6f 100644 --- a/deployment/managementgroup.go +++ b/deployment/managementgroup.go @@ -11,6 +11,7 @@ import ( "github.com/Azure/alzlib" "github.com/Azure/alzlib/assets" + "github.com/Azure/alzlib/internal/parametername" "github.com/Azure/alzlib/to" "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armpolicy" @@ -547,32 +548,6 @@ func (mg *HierarchyManagementGroup) generatePolicyAssignmentAdditionalRoleAssign !*paramVal.Metadata.AssignPermissions { continue } - // get the parameter value from the policy reference within the set definition. - if _, ok := pd.Properties.Parameters[paramName]; !ok { - if errs == nil { - errs = NewPolicyRoleAssignmentErrors() - } - - errs.Add( - NewPolicyRoleAssignmentError( - paName, - mg.id, - paramName, - *pdRef.PolicyDefinitionReferenceID, - rdids, - fmt.Errorf( - "ManagementGroup.GeneratePolicyAssignmentAdditionalRoleAssignments: "+ - "assignment `%s` for policy set `%s`, parameter `%s` not found in refernced policy definition `%s`", - paName, - *psd.Name, - paramName, - *pd.Name, - ), - ), - ) - - continue - } // use goarmfunctions to evaluate the ARM expression in the parameter value in the set // definition reference. scope, err := parseArmFunctionInPolicySetParameter( @@ -743,13 +718,24 @@ func WithParameters(parameters map[string]*armpolicy.ParameterValuesValue) Modif ) } - pa.Properties.Parameters[k] = v + pa.Properties.Parameters[canonicalAssignmentParameterName(pa, k)] = v } return nil } } +// canonicalAssignmentParameterName returns the casing already used by the assignment for name, so +// that a case-only variant does not add a second parameter that Azure would treat as a duplicate. +func canonicalAssignmentParameterName(pa *assets.PolicyAssignment, name string) string { + match, found, err := parametername.Resolve(pa.Properties.Parameters, name) + if err != nil || !found { + return name + } + + return match.Key +} + // WithEnforcementMode sets the enforcement mode for the policy assignment. func WithEnforcementMode(enforcementMode *armpolicy.EnforcementMode) ModifyPolicyAssignmentOption { return func(mg *HierarchyManagementGroup, name string) error { @@ -884,7 +870,15 @@ func parseArmFunctionInPolicySetParameter( } for k, v := range ass.Properties.Parameters { - resultantParams[k] = v.Value + // Assignment parameter names are case-insensitive, so map them onto the canonical set + // definition parameter name, otherwise the default value would not be overridden. + key := k + if match, found, err := parametername.Resolve(setDef.Properties.Parameters, k); err == nil && + found { + key = match.Key + } + + resultantParams[key] = v.Value } var toParse string @@ -894,8 +888,17 @@ func parseArmFunctionInPolicySetParameter( continue } - p, ok := def.Parameters[paramName] - if !ok { + match, found, err := parametername.Resolve(def.Parameters, paramName) + if err != nil { + return nil, fmt.Errorf( + "parseArmFunctionInPolicySetParameter: paramName %s in %s: %w", + paramName, + *def.PolicyDefinitionReferenceID, + err, + ) + } + + if !found { return nil, fmt.Errorf( "parseArmFunctionInPolicySetParameter: paramName %s not found in %s", paramName, @@ -903,7 +906,12 @@ func parseArmFunctionInPolicySetParameter( ) } - pStr, ok := p.Value.(string) + var value any + if match.Value != nil { + value = match.Value.Value + } + + pStr, ok := value.(string) if !ok { return nil, fmt.Errorf( "parseArmFunctionInPolicySetParameter: paramName %s in %s is not a string", diff --git a/deployment/parameterNameCasing_test.go b/deployment/parameterNameCasing_test.go new file mode 100644 index 0000000..1f188cb --- /dev/null +++ b/deployment/parameterNameCasing_test.go @@ -0,0 +1,292 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package deployment + +import ( + "context" + "testing" + + "github.com/Azure/alzlib" + "github.com/Azure/alzlib/assets" + "github.com/Azure/alzlib/to" + "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" +) + +// The following fixtures reproduce the shape of the official Azure built-in Windows AMA/DCR +// initiative `9575b8b7-78ab-4281-b53b-d3c1ace2260b`, which passes `DcrResourceId` to the policy +// definition `eab1f514-22e3-42e3-9a1f-e1dc9199355c` that declares `dcrResourceId`. +// See https://github.com/Azure/Azure-Landing-Zones/issues/4248. +const ( + dcrMemberDefinitionName = "eab1f514-22e3-42e3-9a1f-e1dc9199355c" + dcrInitiativeName = "9575b8b7-78ab-4281-b53b-d3c1ace2260b" + dcrAssignmentName = "Deploy-VM-Monitoring" + dcrMemberParameterName = "dcrResourceId" + dcrInitiativeParamName = "DcrResourceId" + dcrRoleDefinitionID = "/providers/Microsoft.Authorization/roleDefinitions/749f88d5-cbae-40b8-bcfc-e573ddc772fa" + dcrResourceID = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/rg-ama/" + + "providers/Microsoft.Insights/dataCollectionRules/dcr-windows" +) + +// newDcrMemberDefinition returns a policy definition declaring memberParamName with +// assignPermissions set to true. +func newDcrMemberDefinition(memberParamName string) *assets.PolicyDefinition { + return assets.NewPolicyDefinition(armpolicy.Definition{ + Name: to.Ptr(dcrMemberDefinitionName), + Type: to.Ptr("Microsoft.Authorization/policyDefinitions"), + Properties: &armpolicy.DefinitionProperties{ + PolicyRule: map[string]any{ + "then": map[string]any{ + "details": map[string]any{ + "roleDefinitionIds": []any{dcrRoleDefinitionID}, + }, + }, + }, + Parameters: map[string]*armpolicy.ParameterDefinitionsValue{ + memberParamName: { + Type: to.Ptr(armpolicy.ParameterTypeString), + Metadata: &armpolicy.ParameterDefinitionsValueMetadata{ + AssignPermissions: to.Ptr(true), + }, + }, + }, + }, + }) +} + +// newDcrInitiative returns an initiative that passes refParamName to the member definition. +func newDcrInitiative(refParamName string) *assets.PolicySetDefinition { + return assets.NewPolicySetDefinition(armpolicy.SetDefinition{ + Name: to.Ptr(dcrInitiativeName), + Type: to.Ptr("Microsoft.Authorization/policySetDefinitions"), + Properties: &armpolicy.SetDefinitionProperties{ + Parameters: map[string]*armpolicy.ParameterDefinitionsValue{ + dcrInitiativeParamName: {Type: to.Ptr(armpolicy.ParameterTypeString)}, + }, + PolicyDefinitions: []*armpolicy.DefinitionReference{ + { + PolicyDefinitionReferenceID: to.Ptr("WindowsDcrAssociation"), + PolicyDefinitionID: to.Ptr( + "/providers/Microsoft.Authorization/policyDefinitions/" + dcrMemberDefinitionName, + ), + Parameters: map[string]*armpolicy.ParameterValuesValue{ + refParamName: {Value: "[parameters('" + dcrInitiativeParamName + "')]"}, + }, + }, + }, + }, + }) +} + +// newDcrAssignment returns an assignment of the initiative supplying assignmentParamName. +func newDcrAssignment(assignmentParamName string) *assets.PolicyAssignment { + return assets.NewPolicyAssignment(armpolicy.Assignment{ + Name: to.Ptr(dcrAssignmentName), + Type: to.Ptr("Microsoft.Authorization/policyAssignments"), + Identity: &armpolicy.Identity{Type: to.Ptr(armpolicy.ResourceIdentityTypeSystemAssigned)}, + Properties: &armpolicy.AssignmentProperties{ + PolicyDefinitionID: to.Ptr( + "/providers/Microsoft.Authorization/policySetDefinitions/" + dcrInitiativeName, + ), + Parameters: map[string]*armpolicy.ParameterValuesValue{ + assignmentParamName: {Value: dcrResourceID}, + }, + }, + }) +} + +// newDcrArchetype returns an archetype referencing the initiative assignment. +func newDcrArchetype() *alzlib.Archetype { + arch := alzlib.NewArchetype("dcr") + arch.PolicyAssignments.Add(dcrAssignmentName) + + return arch +} + +func newDcrManagementGroupAddRequest() managementGroupAddRequest { + return managementGroupAddRequest{ + id: "mg1", + displayName: "mg1", + parentID: "external", + parentIsExternal: true, + archetypes: []*alzlib.Archetype{newDcrArchetype()}, + location: eastUSLocation, + } +} + +// TestAddManagementGroupParameterNameCasing asserts that the hierarchy accepts an initiative that +// passes a case-only variant of a member definition parameter name, as Azure does, while a +// genuinely missing parameter still fails. +func TestAddManagementGroupParameterNameCasing(t *testing.T) { + t.Parallel() + + tcs := []struct { + name string + memberParamName string + refParamName string + errContains string + }{ + { + name: "exact match", + memberParamName: dcrMemberParameterName, + refParamName: dcrMemberParameterName, + }, + { + name: "case only difference", + memberParamName: dcrMemberParameterName, + refParamName: dcrInitiativeParamName, + }, + { + name: "genuinely missing parameter", + memberParamName: "minPort", + refParamName: dcrInitiativeParamName, + errContains: "does not match a parameter in referenced definition", + }, + } + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + az := alzlib.NewAlzLib(nil) + require.NoError(t, az.AddPolicyDefinitions(newDcrMemberDefinition(tc.memberParamName))) + require.NoError(t, az.AddPolicySetDefinitions(newDcrInitiative(tc.refParamName))) + require.NoError(t, az.AddPolicyAssignments(newDcrAssignment(dcrInitiativeParamName))) + + h := NewHierarchy(az) + + mg, err := h.addManagementGroup(context.Background(), newDcrManagementGroupAddRequest()) + if tc.errContains != "" { + require.Error(t, err) + assert.ErrorContains(t, err, tc.errContains) + + return + } + + require.NoError(t, err) + require.NotNil(t, mg) + assert.Contains(t, mg.policyAssignments, dcrAssignmentName) + }) + } +} + +// TestAddManagementGroupAmbiguousParameterName asserts that a parameter name matching more than one +// member definition parameter case-insensitively is rejected deterministically. +func TestAddManagementGroupAmbiguousParameterName(t *testing.T) { + t.Parallel() + + pd := newDcrMemberDefinition(dcrMemberParameterName) + pd.Properties.Parameters["DCRRESOURCEID"] = &armpolicy.ParameterDefinitionsValue{ + Type: to.Ptr(armpolicy.ParameterTypeString), + } + + az := alzlib.NewAlzLib(nil) + require.NoError(t, az.AddPolicyDefinitions(pd)) + require.NoError(t, az.AddPolicySetDefinitions(newDcrInitiative(dcrInitiativeParamName))) + require.NoError(t, az.AddPolicyAssignments(newDcrAssignment(dcrInitiativeParamName))) + + h := NewHierarchy(az) + + mg, err := h.addManagementGroup(context.Background(), newDcrManagementGroupAddRequest()) + require.Error(t, err) + assert.Nil(t, mg) + assert.ErrorContains(t, err, "is ambiguous") +} + +// TestWithParametersDoesNotDuplicateCaseVariantKeys asserts that supplying a case-only variant of a +// parameter the assignment already sets overwrites it, rather than emitting two parameters that +// Azure would treat as duplicates. +func TestWithParametersDoesNotDuplicateCaseVariantKeys(t *testing.T) { + t.Parallel() + + az := alzlib.NewAlzLib(nil) + require.NoError(t, az.AddPolicyDefinitions(newDcrMemberDefinition(dcrMemberParameterName))) + require.NoError(t, az.AddPolicySetDefinitions(newDcrInitiative(dcrMemberParameterName))) + + pa := newDcrAssignment(dcrInitiativeParamName) + require.NoError(t, az.AddPolicyAssignments(pa)) + + h := NewHierarchy(az) + mg := &HierarchyManagementGroup{ + id: "mg1", + hierarchy: h, + policyAssignments: map[string]*assets.PolicyAssignment{*pa.Name: pa}, + } + h.mgs["mg1"] = mg + + const overriddenID = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/rg-ama/" + + "providers/Microsoft.Insights/dataCollectionRules/dcr-other" + + require.NoError(t, mg.ModifyPolicyAssignment(*pa.Name, WithParameters( + map[string]*armpolicy.ParameterValuesValue{"dcrresourceid": {Value: overriddenID}}, + ))) + + assert.Len(t, pa.Properties.Parameters, 1) + require.Contains(t, pa.Properties.Parameters, dcrInitiativeParamName) + assert.Equal(t, overriddenID, pa.Properties.Parameters[dcrInitiativeParamName].Value) +} + +// TestPolicyRoleAssignmentsParameterNameCasing asserts that the additional role assignment for the +// assignPermissions parameter is still scoped to the supplied data collection rule when the +// initiative and the member definition disagree on parameter name casing. +func TestPolicyRoleAssignmentsParameterNameCasing(t *testing.T) { + t.Parallel() + + tcs := []struct { + name string + refParamName string + assignmentParamName string + }{ + { + name: "exact match", + refParamName: dcrMemberParameterName, + assignmentParamName: dcrInitiativeParamName, + }, + { + name: "reference key differs in case", + refParamName: dcrInitiativeParamName, + assignmentParamName: dcrInitiativeParamName, + }, + { + name: "assignment key differs in case", + refParamName: dcrInitiativeParamName, + assignmentParamName: "dcrresourceid", + }, + } + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + az := alzlib.NewAlzLib(nil) + require.NoError(t, az.AddPolicyDefinitions(newDcrMemberDefinition(dcrMemberParameterName))) + require.NoError(t, az.AddPolicySetDefinitions(newDcrInitiative(tc.refParamName))) + + pa := newDcrAssignment(tc.assignmentParamName) + require.NoError(t, az.AddPolicyAssignments(pa)) + + h := NewHierarchy(az) + h.mgs["mg1"] = &HierarchyManagementGroup{ + id: "mg1", + hierarchy: h, + policyRoleAssignments: mapset.NewThreadUnsafeSet[PolicyRoleAssignment](), + policyDefinitions: make(map[string]*assets.PolicyDefinition), + policySetDefinitions: make(map[string]*assets.PolicySetDefinition), + policyAssignments: map[string]*assets.PolicyAssignment{*pa.Name: pa}, + } + + res, err := h.PolicyRoleAssignments(context.Background()) + require.NoError(t, err) + + assert.True(t, res.Contains(PolicyRoleAssignment{ + AssignmentName: dcrAssignmentName, + RoleDefinitionID: dcrRoleDefinitionID, + Scope: dcrResourceID, + ManagementGroupID: "mg1", + }), "expected a role assignment scoped to the supplied data collection rule, got %v", res.ToSlice()) + }) + } +} diff --git a/internal/parametername/resolve.go b/internal/parametername/resolve.go new file mode 100644 index 0000000..3d6b2e1 --- /dev/null +++ b/internal/parametername/resolve.go @@ -0,0 +1,60 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +// Package parametername resolves Azure Policy parameter names against the maps that contain them. +// +// Azure Resource Manager treats template function parameters as case-insensitive and Azure Policy +// artifacts rely on this, e.g. the built-in Windows AMA/DCR initiative passes `DcrResourceId` to a +// policy definition that declares `dcrResourceId`. Go maps are case-sensitive, so an exact key +// lookup rejects artifacts that Azure itself accepts. +package parametername + +import ( + "fmt" + "slices" + "strings" +) + +// Match is a successfully resolved parameter. +// Key is the canonical key as declared by the source artifact, which may differ in case from the +// requested name. +type Match[V any] struct { + Key string + Value V +} + +// Resolve looks up name in values, preferring an exact match and falling back to a unique +// case-insensitive match. The boolean return value reports whether a match was found. +// +// An error is returned when the case-insensitive fallback matches more than one key, as the +// result would otherwise depend on Go map iteration order. +func Resolve[V any](values map[string]V, name string) (Match[V], bool, error) { + if value, ok := values[name]; ok { + return Match[V]{Key: name, Value: value}, true, nil + } + + var candidates []string + + for key := range values { + if strings.EqualFold(key, name) { + candidates = append(candidates, key) + } + } + + switch len(candidates) { + case 0: + return Match[V]{}, false, nil + case 1: + key := candidates[0] + + return Match[V]{Key: key, Value: values[key]}, true, nil + } + + slices.Sort(candidates) + + return Match[V]{}, false, fmt.Errorf( + "parameter name `%s` is ambiguous, it matches `%s` when compared case-insensitively", + name, + strings.Join(candidates, "`, `"), + ) +} diff --git a/internal/parametername/resolve_test.go b/internal/parametername/resolve_test.go new file mode 100644 index 0000000..b5830d5 --- /dev/null +++ b/internal/parametername/resolve_test.go @@ -0,0 +1,87 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package parametername + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestResolveExactMatch(t *testing.T) { + t.Parallel() + + values := map[string]int{"dcrResourceId": 1, "DcrResourceId": 2} + + match, found, err := Resolve(values, "DcrResourceId") + + require.NoError(t, err) + assert.True(t, found) + assert.Equal(t, "DcrResourceId", match.Key) + assert.Equal(t, 2, match.Value) +} + +func TestResolveCaseInsensitiveFallback(t *testing.T) { + t.Parallel() + + values := map[string]int{"dcrResourceId": 1} + + match, found, err := Resolve(values, "DcrResourceId") + + require.NoError(t, err) + assert.True(t, found) + assert.Equal(t, "dcrResourceId", match.Key, "the canonical key of the source artifact is returned") + assert.Equal(t, 1, match.Value) +} + +func TestResolveNotFound(t *testing.T) { + t.Parallel() + + values := map[string]int{"dcrResourceId": 1} + + match, found, err := Resolve(values, "minPort") + + require.NoError(t, err) + assert.False(t, found) + assert.Empty(t, match.Key) +} + +func TestResolveNilAndEmptyMap(t *testing.T) { + t.Parallel() + + for name, values := range map[string]map[string]int{ + "nil": nil, + "empty": {}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + _, found, err := Resolve(values, "dcrResourceId") + + require.NoError(t, err) + assert.False(t, found) + }) + } +} + +func TestResolveAmbiguousIsDeterministic(t *testing.T) { + t.Parallel() + + values := map[string]int{"dcrResourceId": 1, "DCRRESOURCEID": 2} + + // Repeat to make sure the outcome does not depend on map iteration order. + for range 100 { + _, found, err := Resolve(values, "DcrResourceId") + + require.Error(t, err) + assert.False(t, found) + assert.Equal( + t, + "parameter name `DcrResourceId` is ambiguous, "+ + "it matches `DCRRESOURCEID`, `dcrResourceId` when compared case-insensitively", + err.Error(), + ) + } +}