Repository navigation
fix: handle empty built-in policy version listings instead of panicking - #254
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #254 +/- ##
==========================================
+ Coverage 50.79% 55.13% +4.34%
==========================================
Files 55 56 +1
Lines 4999 5213 +214
==========================================
+ Hits 2539 2874 +335
+ Misses 2169 2022 -147
- Partials 291 317 +26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| return err | ||
| } | ||
|
|
||
| if err := az.AddPolicyDefinitions(pd); err != nil { |
There was a problem hiding this comment.
Fallback makes version selection depend on reference order
The fallback stores only the version selected for the current constraint. Subsequent references use PolicyDefinitionExists, so any cached matching version suppresses further resolution—even when a newer matching version exists.
Reproduced: a policy set references the same definition first with 1.0., then 1..*, with available versions 1.0.2 and 1.2.0:
Healthy listing: the broader constraint resolves to 1.2.0.
Empty listing with fallback: it incorrectly reuses 1.0.2.
This can produce different parameter or role-assignment behavior in affected regions. Track resolved constraints or otherwise ensure a partial fallback result does not suppress resolving a broader constraint.
There was a problem hiding this comment.
Thanks. Confirmed, with your example. For a member of a built-in policy set, the listing stores every listed version, but the fallback stored only the one selected for the first reference, so PolicyDefinitionExists treated a later, broader constraint as satisfied. Fixed in bd46ebb: the fallback now gets every version the definition lists (skipping versions already stored, and adding none unless all are got), so it stores exactly what the listing would have, whatever the reference order. Added TestVersionFallbackIndependentOfReferenceOrder for both orders; it fails without the fix. Offline, reading the default alz architecture with the regional fault injected now stores the same built-in definitions as a healthy read (exported caches identical), at 1,241 requests instead of 1,175 (healthy: 1,026).
A related order dependence is pre-existing and unchanged here. For directly referenced definitions and policy set definitions, the listing path itself stores only the selected version. So on main, in any region, a direct 1.0.* processed before a built-in set's 1.. resolves the latter to 1.0.2. If the versions differ, that can fail Hierarchy.AddManagementGroup with a parameter mismatch, or compute policy role assignments from the older version. It is latent for the ALZ library: its constraints are all N.. / N..-preview, and the two definitions referenced with both resolve to the same version. The fallback now behaves exactly like the listing there, so affected regions are no worse off. Fixing it changes healthy-path behaviour, so I'll open a separate issue rather than widen this hotfix.
| err = c.Add(got, true) | ||
| } | ||
|
|
||
| if err != nil { |
There was a problem hiding this comment.
Duplicate recovery swallows context cancellation
The recovery path handles every GET error by logging a warning and returning nil, including cancellation and deadline errors.
Reproduced: cancelling during duplicate recovery on the final historical-list item makes NewCacheFromAzure return a non-nil cache with no error, despite ctx.Err() being context.Canceled. Callers can therefore report success and persist a cache after cancellation.
Propagate cancellation/deadline errors before applying the best-effort “keep first” behavior.
There was a problem hiding this comment.
Thanks. Confirmed: if the recovery GET is the last request, NewCacheFromAzure returned a cache with a nil error after cancellation. addListedVersion now returns the context error when the GET fails and ctx is done. Other GET failures still keep the version listed first, with a warning. Added TestNewCacheFromAzureReturnsCancellationDuringRecovery for exactly this case; it fails without the fix.
Summary
Reading an
alz_architecturecrashes terraform-provider-alz with a nil pointer dereference when Azure's built-in policy definition version listing returns no version for a definition that a built-in policy set references with adefinitionVersion. Since around 27 October 2026, Azure returns exactly that in several regions for every Resource Provider mode definition, so planning the default ALZ architecture fails for everyone whose requests are served there.This PR fixes it in two commits, best reviewed one at a time:
VersionedPolicyCollection.Versions()is nil-safe, so the existing descriptive error is returned instead of a panic. This commit stands on its own if the second one needs more review.properties.versionswith the existingGetVersion, and gets that version. A healthy listing costs no extra request.Refs Azure/Azure-Landing-Zones#4327. Not "Fixes": the issue stays open until a provider release with this change is confirmed by the reporters.
Problem
In UK West, France Central, Spain Central, West Central US, Jio India Central and the two EUAP regions,
GET /providers/Microsoft.Authorization/policyDefinitions/{name}/versionsanswers200 OKwith"value": []for every Resource Provider mode built-in definition (Microsoft.*.Data), whatever the API version. Everything else alzlib needs still works there: the definition GET (includingproperties.versions), the version GET and the policy set definition version listing. ARM serves requests from a nearby region, so only some users are affected.Reading the default
alzarchitecture (platform/alz 2026.10.0) needs 73 such definitions, 68 of them through Microsoft cloud security benchmark v2. In an affected region, v0.32.2 fails with:Root cause
Since #222 (v0.29.0), alzlib resolves
definitionVersionconstraints itself and lists all versions of every referenced built-in definition. When nothing is listed under the requested name,fetchReferencedPolicyDefinitionVersionsbuilds its error message withaz.policyDefinitions[name].Versions(). The definition was never stored, so the map returns a nil collection andVersions()dereferences it. The error turns into a panic, and the provider plugin exits ("Plugin did not respond"). Affected: alzlib v0.29.0 to v0.32.2, terraform-provider-alz v0.20.0 to v0.22.x.Without the panic the read still cannot succeed: in those regions the versions are never listed, and retrying the listing returns the same empty list.
How it is resolved
Commit 1: make
VersionedPolicyCollection.Versionsnil-safeVersions()returns no versions for a nil receiver, so the read now fails with:Fixing it in
assetsrather than at the call site protects every caller, and keeps this PR free of conflicts with #253, which rewrites that call site.Commit 2: fall back to the definition when no listed version matches
When a version listing yields no version that satisfies the constraint, because it is empty or because none matches, alzlib:
DefinitionsClient.GetBuiltInorSetDefinitionsClient.GetBuiltIn) and takes the versions it lists inproperties.versionsandproperties.version;GetVersion, on a collection of placeholders that carry only the version. The selection, including-previewhandling, is therefore the one a healthy listing gives, and it follows future changes toGetVersionsuch as Resolve policy definition versions to the version the assignment actually runs #248;DefinitionVersionsClient.GetBuiltInorSetDefinitionVersionsClient.GetBuiltIn), validates it, checks that Azure returned the requested name and version, and stores it under the requested name.This covers every place that lists versions: definitions referenced by built-in policy sets (
fetchReferencedPolicyDefinitionVersions), definitions referenced directly (addVersionedPolicyDefinition), and policy set definitions (addVersionedPolicySetDefinition). Each fallback makes two requests. If it fails too, the error reports what the listing returned and why the fallback failed. A cancelled context stops it before any request. The fallback gets only the selected version, so a definition referenced with two different constraints can be listed and resolved twice; this happens once in the defaultalzarchitecture.Also in this commit:
cache.NewCacheFromAzure(alzlibtool cache createwithout--library): a version that Azure lists twice with different content no longer aborts cache creation. The version is got on its own and stored, with a warning. Only if that fails is the version listed first kept, also with a warning. Keeping either copy blindly could cache stale content, for example differentroleDefinitionIds, and silently change policy role assignments.Alzlib.getBuiltInPoliciesinstead ofAlzlib.getBuiltInPolicySets.No retry is added: in the affected regions the listing is consistently empty, so retrying does not help. The fallback lives in a new file with small hooks in
alzlib.go, to keep the merge surface with #253 small.Changes
assets/genericVersionCollection.goVersions()is nil-safeassets/genericVersionCollection_nil_test.goalzlib_missing_referenced_version_test.gobuiltin_version_fallback.goalzlib.gocollectPolicy(Set)DefinitionVersionsalso return what was listed; error prefixbuiltin_version_fallback_test.goalzlib_policy_fetch_test.gocache/azure.goNewCacheFromAzurecache/azure_duplicates_test.godocs/cache.mdcache createBehaviour after the fix
cache createin full mode, a version listed twice with different contentTesting
All tests were run locally.
In-repo:
go build ./...,go vet ./...andgo test ./...pass, with the 7 tests that need Azure credentials skipped.golangci-lintwithGOTOOLCHAIN=go1.25.2, as in CI, reports no new issues. The new tests also passed 20 runs in a row. They use the SDK fakes and cover: a healthy listing with no extra request; an empty listing; versions listed under another case; versions of another definition; the definition not found; no version satisfying the constraint; the policy set path with the1.*.*-previewconstraint MCSB v2 is assigned with; cancellation; and, for preview constraints, the same selection as a healthy listing. Commit 1's tests panic onmain. An end-to-end suite outside the repo ran the scenarios above againstmain, commit 1 only and this PR, including terraform-provider-alz; every outcome matched the table.Live, against Azure on 2026-10-30 (read-only). A test-only pipeline policy pinned every ARM request to a regional endpoint,
https://{region}.management.azure.com, andx-ms-routing-request-idconfirmed the region of every response.Region scan at 13:12 UTC, 62 regions: the listing is empty in West Central US, UK West, France Central, Spain Central, Jio India Central, East US 2 EUAP and Central US EUAP. An Indexed control definition is listed in all of them.
Reading platform/alz 2026.10.0
alzas the provider does:main(v0.32.2)mainIn UK West, 74 listings were empty. The fallback added 74 definition GETs and 74 version GETs, plus one listing:
1a5b4dca-0b6f-4cf5-907c-56316bc1bf3dis referenced with8.*.*and, by a library initiative, with9.*.*, so it was listed and resolved once for each. A cache exported after the read in UK West (ExportBuiltInCache) reproduced the same output without any request.Golden comparison:
alzlibtool generate architectureoutput ofmainand of this PR is byte-identical for platform/alz 2026.04.2 (319 files), 2026.08.0 (319 files) and 2026.10.0 (320 files).terraform-provider-alz v0.22.0 built against this branch with
replace:terraform planofdata.alz_architecture, pinned to UK West, succeeds with the same values as UK South (12 management groups, 240 policy role assignments). The released v0.22.0 fails with "Plugin did not respond" and the panic above; built with commit 1 only, it fails with a readable diagnostic. The data source read timeout was raised to 30m because every request from the test machine crossed to the UK and the fetch is serial (fix: honor Options.Parallelism when fetching built-in policy definitions #253).Open PRs (heads of 2026-10-30): this branch merges without conflicts with #253 (
8c42f3f), #248 (ce35388) and #245 (1495111), and with #253 and #248 together; build, vet and tests pass on each result. #253 and #245 conflict with each other ingo.sum, with or without this PR.Backwards compatibility and upgrade impact
alzarchitecture.getBuiltInPolicies.cache createin full mode no longer fails on a conflicting duplicate. In an affected region it stores only the latest version of Resource Provider mode definitions, because their history listings are empty; a cache miss falls back to Azure. Limited mode (--library --architecture), which follows the provider's path, is complete.Out of scope
fetchReferencedPolicyDefinitionVersions: fix: honor Options.Parallelism when fetching built-in policy definitions #253 replaces it with a locked helper, and merges cleanly with this PR.terraform-provider-alz: the alzlib bump follows in a separate PR after release.Suggested release note