diff --git a/alzlib_test.go b/alzlib_test.go index ce52028..c21ded9 100644 --- a/alzlib_test.go +++ b/alzlib_test.go @@ -981,6 +981,43 @@ func TestAssignmentReferencedDefinitionHasParameter(t *testing.T) { assert.True(t, az.AssignmentReferencedDefinitionHasParameter(resID2, nil, "anyParam")) } +func TestPolicyAssignmentEffectiveDefinitionVersionRoundTrip(t *testing.T) { + az := NewAlzLib(&Options{ + AllowOverwrite: true, + Parallelism: defaultParallelism, + UniqueRoleDefinitions: defaultUniqueRoleDefinitions, + }) + + original := assets.NewPolicyAssignment(armpolicy.Assignment{ + Name: to.Ptr("Deploy-MCSB2-Monitoring"), + Properties: &armpolicy.AssignmentProperties{ + DisplayName: to.Ptr("Microsoft Cloud Security Benchmark v2"), + Description: to.Ptr("Microsoft Cloud Security Benchmark v2 policy initiative."), + PolicyDefinitionID: to.Ptr("/providers/Microsoft.Authorization/policySetDefinitions/e3ec7e09"), + DefinitionVersion: to.Ptr("1.*.*-preview"), + }, + }) + require.NoError(t, az.AddPolicyAssignments(original)) + + // Callers get a deep copy, so the effective version must survive being written back. + fetched := az.PolicyAssignment("Deploy-MCSB2-Monitoring") + require.NotNil(t, fetched) + require.Nil(t, fetched.EffectiveDefinitionVersion()) + + fetched.SetEffectiveDefinitionVersion(to.Ptr("1.2.0-preview")) + require.NoError(t, az.AddPolicyAssignments(fetched)) + + stored := az.PolicyAssignment("Deploy-MCSB2-Monitoring") + require.NotNil(t, stored) + require.NotNil(t, stored.EffectiveDefinitionVersion()) + assert.Equal(t, "1.2.0-preview", *stored.EffectiveDefinitionVersion()) + + _, ver, err := stored.ReferencedPolicyDefinitionResourceIDAndVersion() + require.NoError(t, err) + require.NotNil(t, ver) + assert.Equal(t, "1.2.0-preview", *ver) +} + func TestAddPolicyDefinitionsMultipleVersions(t *testing.T) { t.Parallel() diff --git a/assets/genericVersionCollection.go b/assets/genericVersionCollection.go index b150794..617885e 100644 --- a/assets/genericVersionCollection.go +++ b/assets/genericVersionCollection.go @@ -54,7 +54,7 @@ func (c *VersionedPolicyCollection[T]) Versions() []semver.Version { return vers } -// GetVersion returns a policy version based on the provided constraint string. +// GetVersion returns a policy version based on an exact version or policy version constraint. // If the constraint string is nil, it returns the versionless definition if it exists. // If the constraint string is nil and no versionless definition exists, it returns the latest // version. @@ -86,28 +86,62 @@ func (c *VersionedPolicyCollection[T]) GetVersion(constraintStr *string) (T, err return c.versionlessDefinition, nil } + // Exact lookup supports Azure's read-only effective definition version. Writable assignment + // constraints must continue to use a wildcard patch version. + if sv, err := semver.StrictNewVersion(*constraintStr); err == nil { + if pol, ok := c.versions[*sv]; ok { + return pol, nil + } + + return nil, errors.Join(ErrNoVersionFound, fmt.Errorf( + "version %s", + *constraintStr, + )) + } + constraint, err := policyVersionConstraintToSemVerConstraint(*constraintStr) if err != nil { return nil, err } - var resKey *semver.Version + // Azure Policy encodes state as a semver prerelease suffix: "-preview" or "-deprecated". + // A constraint carrying such a suffix must resolve to a version with the same suffix, not to a + // higher stable release or a different suffix. It falls back to the highest stable release when + // no same-suffix version matches (graduated out of preview), then to any other matching version + // as a last resort. See Azure/Azure-Landing-Zones#4190. + wantPrerelease := policyVersionConstraintPrerelease(*constraintStr) + + var bestMatch, bestStable, bestAny *semver.Version for v := range c.versions { if !constraint.Check(&v) { continue } - if resKey == nil { - resKey = &v - continue + if bestAny == nil || v.GreaterThan(bestAny) { + bestAny = &v } - if v.LessThan(resKey) { - continue + // When the constraint has no suffix, wantPrerelease is "" and the first case wins. + switch v.Prerelease() { + case wantPrerelease: + if bestMatch == nil || v.GreaterThan(bestMatch) { + bestMatch = &v + } + case "": + if bestStable == nil || v.GreaterThan(bestStable) { + bestStable = &v + } } + } - resKey = &v + resKey := bestMatch + if resKey == nil { + resKey = bestStable + } + + if resKey == nil { + resKey = bestAny } if resKey == nil { diff --git a/assets/genericVersionCollection_test.go b/assets/genericVersionCollection_test.go index 669b453..98aca7e 100644 --- a/assets/genericVersionCollection_test.go +++ b/assets/genericVersionCollection_test.go @@ -190,10 +190,17 @@ func TestVersionedPolicyCollection_GetVersion_Versioned(t *testing.T) { assert.Nil(t, got) }) - t.Run("invalid no wildcard patch", func(t *testing.T) { + t.Run("exact version resolves", func(t *testing.T) { constr := testVersion100 got, err := pdvs.GetVersion(&constr) - require.ErrorContains(t, err, "version constraint should have wildcard in patch version") + require.NoError(t, err) + assert.Equal(t, policy1, got) + }) + + t.Run("exact version not in collection", func(t *testing.T) { + constr := "3.0.0" + got, err := pdvs.GetVersion(&constr) + require.ErrorIs(t, err, ErrNoVersionFound) assert.Nil(t, got) }) } @@ -236,6 +243,97 @@ func TestVersionedPolicyCollection_GetVersion_PrereleaseVersionMatchOnNilVersion assert.NotNil(t, got) } +func TestVersionedPolicyCollection_GetVersion_PrereleaseAwareResolution(t *testing.T) { + newColl := func(versions ...string) *PolicyDefinitionVersions { + pdvs := NewPolicyDefinitionVersions() + for _, v := range versions { + require.NoError(t, pdvs.Add(fakePolicyDefinitionVersioned("MCSB2", v), false)) + } + + return pdvs + } + + fullSet := []string{"1.1.0-preview", "1.2.0-preview", "1.3.0-preview", "1.4.0"} + + tests := []struct { + name string + versions []string + constraint string + want string + }{ + { + name: "preview wildcard prefers highest preview not stable", + versions: fullSet, + constraint: "1.*.*-preview", + want: "1.3.0-preview", + }, + { + name: "preview pinned minor", + versions: fullSet, + constraint: "1.3.*-preview", + want: "1.3.0-preview", + }, + { + name: "clean wildcard resolves stable", + versions: fullSet, + constraint: "1.*.*", + want: "1.4.0", + }, + { + name: "graduated out of preview falls back to stable", + versions: []string{"1.4.0"}, + constraint: "1.*.*-preview", + want: "1.4.0", + }, + { + name: "preview only resolves highest preview", + versions: []string{"1.1.0-preview", "1.2.0-preview"}, + constraint: "1.*.*-preview", + want: "1.2.0-preview", + }, + { + name: "preview wildcard ignores deprecated and higher stable", + versions: []string{"1.2.0-preview", "1.3.0-preview", "1.4.0-deprecated", "1.5.0"}, + constraint: "1.*.*-preview", + want: "1.3.0-preview", + }, + { + name: "deprecated constraint resolves matching deprecated", + versions: []string{"1.3.0-preview", "1.4.0-deprecated", "1.5.0"}, + constraint: "1.*.*-deprecated", + want: "1.4.0-deprecated", + }, + { + name: "clean wildcard ignores deprecated", + versions: []string{"1.4.0-deprecated", "1.5.0"}, + constraint: "1.*.*", + want: "1.5.0", + }, + { + name: "only different suffix falls back to any match", + versions: []string{"1.4.0-deprecated"}, + constraint: "1.*.*-preview", + want: "1.4.0-deprecated", + }, + { + name: "exact preview version pins below the highest preview", + versions: fullSet, + constraint: "1.2.0-preview", + want: "1.2.0-preview", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pdvs := newColl(tt.versions...) + got, err := pdvs.GetVersion(to.Ptr(tt.constraint)) + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, tt.want, *got.GetVersion()) + }) + } +} + func TestVersionedPolicyCollection_Exists(t *testing.T) { t.Run("returns true when versionless definition present", func(t *testing.T) { pdvs := NewPolicyDefinitionVersions() diff --git a/assets/policyAssignment.go b/assets/policyAssignment.go index bdabe16..e2814b2 100644 --- a/assets/policyAssignment.go +++ b/assets/policyAssignment.go @@ -26,6 +26,10 @@ const ( // working with policy assignments. type PolicyAssignment struct { armpolicy.Assignment + + // Held outside the embedded SDK type so it is never serialized: Azure rejects this read-only + // property when an assignment is deployed. + effectiveDefinitionVersion *string } // NewPolicyAssignment creates a new PolicyAssignment instance from an armpolicy.Assignment. @@ -33,12 +37,15 @@ type PolicyAssignment struct { // Use either the UnmarshalJSON method, or the ValidatePolicyAssignment function to validate the // assignment. func NewPolicyAssignment(pa armpolicy.Assignment) *PolicyAssignment { - return &PolicyAssignment{pa} + paObj := &PolicyAssignment{Assignment: pa} + paObj.hoistReadOnlyDefinitionVersions() + + return paObj } // NewPolicyAssignmentValidate creates a new PolicyAssignment instance and validates it. func NewPolicyAssignmentValidate(pa armpolicy.Assignment) (*PolicyAssignment, error) { - paObj := &PolicyAssignment{pa} + paObj := NewPolicyAssignment(pa) if err := ValidatePolicyAssignment(paObj); err != nil { return nil, fmt.Errorf("NewPolicyAssignmentValidate: %w", err) } @@ -46,6 +53,48 @@ func NewPolicyAssignmentValidate(pa armpolicy.Assignment) (*PolicyAssignment, er return paObj, nil } +// hoistReadOnlyDefinitionVersions moves the effective version reported by Azure out of the embedded +// SDK type, so it can inform version resolution without being written to deployment artifacts, and +// drops the other read-only version field for the same reason. +func (pa *PolicyAssignment) hoistReadOnlyDefinitionVersions() { + pa.effectiveDefinitionVersion = nil + + if pa.Properties == nil { + return + } + + if v := pa.Properties.EffectiveDefinitionVersion; v != nil && *v != "" { + pa.effectiveDefinitionVersion = to.Ptr(*v) + } + + pa.Properties.EffectiveDefinitionVersion = nil + pa.Properties.LatestDefinitionVersion = nil +} + +// EffectiveDefinitionVersion returns the exact policy definition version that Azure reports as being +// in effect for this assignment, or nil when it is unknown. +func (pa *PolicyAssignment) EffectiveDefinitionVersion() *string { + if pa.effectiveDefinitionVersion == nil { + return nil + } + + return to.Ptr(*pa.effectiveDefinitionVersion) +} + +// SetEffectiveDefinitionVersion records the exact policy definition version that Azure reports as +// being in effect, so that role assignments are computed against the version the Policy RP actually +// enforces instead of re-resolving the definitionVersion constraint. Passing nil or an empty string +// clears it. +func (pa *PolicyAssignment) SetEffectiveDefinitionVersion(version *string) { + if version == nil || *version == "" { + pa.effectiveDefinitionVersion = nil + + return + } + + pa.effectiveDefinitionVersion = to.Ptr(*version) +} + // IdentityType returns the identity type of the policy assignment. func (pa *PolicyAssignment) IdentityType() armpolicy.ResourceIdentityType { return *pa.Identity.Type @@ -53,12 +102,19 @@ func (pa *PolicyAssignment) IdentityType() armpolicy.ResourceIdentityType { // ReferencedPolicyDefinitionResourceIDAndVersion returns the resource ID and version of the // policy definition referenced by the policy assignment. +// The effective version reported by Azure takes precedence over the definitionVersion constraint, +// so callers resolve the version the assignment actually runs rather than re-resolving the +// constraint to a newer one. See Azure/Azure-Landing-Zones#4190. func (pa *PolicyAssignment) ReferencedPolicyDefinitionResourceIDAndVersion() (*arm.ResourceID, *string, error) { id, err := arm.ParseResourceID(*pa.Properties.PolicyDefinitionID) if err != nil { return nil, nil, fmt.Errorf("PolicyAssignment.ReferencedPolicyDefinitionResourceID: %w", err) } + if pa.effectiveDefinitionVersion != nil { + return id, to.Ptr(*pa.effectiveDefinitionVersion), nil + } + return id, pa.Properties.DefinitionVersion, nil } @@ -105,6 +161,8 @@ func (pa *PolicyAssignment) UnmarshalJSON(data []byte) error { return fmt.Errorf("PolicyAssignment.UnmarshalJSON: %w", err) } + pa.hoistReadOnlyDefinitionVersions() + return ValidatePolicyAssignment(pa) } @@ -135,6 +193,16 @@ func ValidatePolicyAssignment(pa *PolicyAssignment) error { return NewErrPropertyMustNotBeNil("properties.policyDefinitionID") } + if pa.Properties.DefinitionVersion != nil { + if _, err := policyVersionConstraintToSemVerConstraint(*pa.Properties.DefinitionVersion); err != nil { + return fmt.Errorf( + "ValidatePolicyAssignment: invalid properties.definitionVersion `%s`: %w", + *pa.Properties.DefinitionVersion, + err, + ) + } + } + if pa.Properties.DisplayName == nil { return NewErrPropertyMustNotBeNil("properties.displayName") } diff --git a/assets/policyAssignment_test.go b/assets/policyAssignment_test.go index a9d89f2..707d403 100644 --- a/assets/policyAssignment_test.go +++ b/assets/policyAssignment_test.go @@ -4,6 +4,7 @@ package assets import ( + "encoding/json" "fmt" "reflect" "testing" @@ -11,6 +12,7 @@ import ( "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" + "github.com/brunoga/deep" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -30,6 +32,115 @@ func TestIdentityType(t *testing.T) { } } +func TestPolicyAssignmentEffectiveDefinitionVersion(t *testing.T) { + newAssignment := func() armpolicy.Assignment { + return armpolicy.Assignment{ + Name: to.Ptr("Deploy-MCSB2-Monitoring"), + Properties: &armpolicy.AssignmentProperties{ + PolicyDefinitionID: to.Ptr( + "/providers/Microsoft.Authorization/policySetDefinitions/e3ec7e09-768c-4b64-882c-fcada3772047", + ), + DefinitionVersion: to.Ptr("1.*.*-preview"), + }, + } + } + + t.Run("constraint is used when no effective version is known", func(t *testing.T) { + pa := NewPolicyAssignment(newAssignment()) + + _, ver, err := pa.ReferencedPolicyDefinitionResourceIDAndVersion() + require.NoError(t, err) + require.NotNil(t, ver) + assert.Equal(t, "1.*.*-preview", *ver) + assert.Nil(t, pa.EffectiveDefinitionVersion()) + }) + + t.Run("effective version takes precedence over the constraint", func(t *testing.T) { + pa := NewPolicyAssignment(newAssignment()) + pa.SetEffectiveDefinitionVersion(to.Ptr("1.2.0-preview")) + + _, ver, err := pa.ReferencedPolicyDefinitionResourceIDAndVersion() + require.NoError(t, err) + require.NotNil(t, ver) + assert.Equal(t, "1.2.0-preview", *ver) + }) + + t.Run("empty effective version clears it", func(t *testing.T) { + pa := NewPolicyAssignment(newAssignment()) + pa.SetEffectiveDefinitionVersion(to.Ptr("1.2.0-preview")) + pa.SetEffectiveDefinitionVersion(to.Ptr("")) + + _, ver, err := pa.ReferencedPolicyDefinitionResourceIDAndVersion() + require.NoError(t, err) + require.NotNil(t, ver) + assert.Equal(t, "1.*.*-preview", *ver) + }) + + t.Run("read-only versions are hoisted off the serialized type", func(t *testing.T) { + assignment := newAssignment() + assignment.Properties.EffectiveDefinitionVersion = to.Ptr("1.3.0-preview") + assignment.Properties.LatestDefinitionVersion = to.Ptr("1.4.0") + + pa := NewPolicyAssignment(assignment) + + require.NotNil(t, pa.EffectiveDefinitionVersion()) + assert.Equal(t, "1.3.0-preview", *pa.EffectiveDefinitionVersion()) + assert.Nil(t, pa.Properties.EffectiveDefinitionVersion) + assert.Nil(t, pa.Properties.LatestDefinitionVersion) + + b, err := json.Marshal(pa) + require.NoError(t, err) + assert.NotContains(t, string(b), "effectiveDefinitionVersion") + assert.NotContains(t, string(b), "latestDefinitionVersion") + }) + + t.Run("effective version survives a deep copy", func(t *testing.T) { + pa := NewPolicyAssignment(newAssignment()) + pa.SetEffectiveDefinitionVersion(to.Ptr("1.2.0-preview")) + + cp := deep.MustCopy(pa) + + require.NotNil(t, cp.EffectiveDefinitionVersion()) + assert.Equal(t, "1.2.0-preview", *cp.EffectiveDefinitionVersion()) + }) + + t.Run("reused assignment does not keep a stale effective version", func(t *testing.T) { + const payload = `{ + "name": "Deploy-MCSB2-Monitoring", + "properties": { + "policyDefinitionId": "/providers/Microsoft.Authorization/policySetDefinitions/e3ec7e09-768c-4b64-882c-fcada3772047", + "displayName": "display", + "description": "description", + "definitionVersion": "%s"%s + } + }` + + for name, second := range map[string]string{ + "absent": "", + "empty": `, "effectiveDefinitionVersion": ""`, + "null": `, "effectiveDefinitionVersion": null`, + } { + t.Run(name, func(t *testing.T) { + pa := new(PolicyAssignment) + require.NoError(t, json.Unmarshal( + []byte(fmt.Sprintf(payload, "1.*.*-preview", `, "effectiveDefinitionVersion": "1.2.0-preview"`)), + pa, + )) + require.NotNil(t, pa.EffectiveDefinitionVersion()) + + require.NoError(t, json.Unmarshal([]byte(fmt.Sprintf(payload, "2.*.*", second)), pa)) + + assert.Nil(t, pa.EffectiveDefinitionVersion()) + + _, ver, err := pa.ReferencedPolicyDefinitionResourceIDAndVersion() + require.NoError(t, err) + require.NotNil(t, ver) + assert.Equal(t, "2.*.*", *ver) + }) + } + }) +} + func TestReferencedPolicyDefinitionResourceId(t *testing.T) { pa := NewPolicyAssignment(armpolicy.Assignment{ Properties: &armpolicy.AssignmentProperties{ @@ -119,6 +230,66 @@ func TestValidatePolicyAssignment(t *testing.T) { }, expectedErr: "", }, + { + name: "Valid definition version autoingests minor and patch updates", + assignment: armpolicy.Assignment{ + Name: to.Ptr("validName"), + Properties: &armpolicy.AssignmentProperties{ + PolicyDefinitionID: to.Ptr( + "/subscriptions/123/resourceGroups/rg1/providers/Microsoft.Authorization/policyDefinitions/pd1", + ), + DefinitionVersion: to.Ptr("1.*.*-preview"), + DisplayName: to.Ptr("Valid Display Name"), + Description: to.Ptr("Valid Description"), + }, + }, + expectedErr: "", + }, + { + name: "Valid definition version pins a minor path", + assignment: armpolicy.Assignment{ + Name: to.Ptr("validName"), + Properties: &armpolicy.AssignmentProperties{ + PolicyDefinitionID: to.Ptr( + "/subscriptions/123/resourceGroups/rg1/providers/Microsoft.Authorization/policyDefinitions/pd1", + ), + DefinitionVersion: to.Ptr("1.3.*-preview"), + DisplayName: to.Ptr("Valid Display Name"), + Description: to.Ptr("Valid Description"), + }, + }, + expectedErr: "", + }, + { + name: "Invalid definition version pins a stable patch", + assignment: armpolicy.Assignment{ + Name: to.Ptr("validName"), + Properties: &armpolicy.AssignmentProperties{ + PolicyDefinitionID: to.Ptr( + "/subscriptions/123/resourceGroups/rg1/providers/Microsoft.Authorization/policyDefinitions/pd1", + ), + DefinitionVersion: to.Ptr("1.3.0"), + DisplayName: to.Ptr("Valid Display Name"), + Description: to.Ptr("Valid Description"), + }, + }, + expectedErr: "version constraint should have wildcard in patch version", + }, + { + name: "Invalid definition version pins a preview patch", + assignment: armpolicy.Assignment{ + Name: to.Ptr("validName"), + Properties: &armpolicy.AssignmentProperties{ + PolicyDefinitionID: to.Ptr( + "/subscriptions/123/resourceGroups/rg1/providers/Microsoft.Authorization/policyDefinitions/pd1", + ), + DefinitionVersion: to.Ptr("1.3.0-preview"), + DisplayName: to.Ptr("Valid Display Name"), + Description: to.Ptr("Valid Description"), + }, + }, + expectedErr: "version constraint should have wildcard in patch version", + }, { name: "Nil Name", assignment: armpolicy.Assignment{ diff --git a/assets/semver.go b/assets/semver.go index 0cdea9b..58f0ffa 100644 --- a/assets/semver.go +++ b/assets/semver.go @@ -54,3 +54,17 @@ func policyVersionConstraintToSemVerConstraint(constraint string) (*semver.Const return sv, nil } + +// policyVersionConstraintPrerelease returns the prerelease identifier carried by a policy version constraint string, +// e.g. "1.*.*-preview" -> "preview", "1.*.*-deprecated" -> "deprecated", "1.*.*" -> "". +// Azure Policy encodes preview/deprecated state as this suffix. +func policyVersionConstraintPrerelease(constraint string) string { + parts := strings.Split(constraint, ".") + if len(parts) != ExpectedVersionComponents { + return "" + } + + _, pre, _ := strings.Cut(parts[len(parts)-1], "-") + + return pre +} diff --git a/assets/semver_test.go b/assets/semver_test.go index 086a2e6..99cd759 100644 --- a/assets/semver_test.go +++ b/assets/semver_test.go @@ -120,3 +120,25 @@ func TestPolicyVersionConstraintToSemVerConstraint(t *testing.T) { }) } } + +func TestPolicyVersionConstraintPrerelease(t *testing.T) { + tests := []struct { + constraint string + want string + }{ + {"1.*.*-preview", "preview"}, + {"1.3.*-preview", "preview"}, + {"1.*.*-deprecated", "deprecated"}, + {">= 0.0.*-preview", "preview"}, + {"1.*.*", ""}, + {"1.2.*", ""}, + {">= 0.0.*", ""}, + {"1.2", ""}, + } + + for _, tt := range tests { + t.Run(tt.constraint, func(t *testing.T) { + assert.Equal(t, tt.want, policyVersionConstraintPrerelease(tt.constraint)) + }) + } +}