Skip to content

fix: apply later-library policy (set) definitions when library overwrite is enabled - #252

Open
Yingjie Chen (Y1ngJ1eChen) wants to merge 1 commit into
mainfrom
user-yingjie/fix-issue-4295-lib
Open

Yingjie Chen (Y1ngJ1eChen) wants to merge 1 commit into
mainfrom
user-yingjie/fix-issue-4295-lib

Conversation

@Y1ngJ1eChen

Copy link
Copy Markdown
Contributor

Summary

Fixes a regression where AllowOverwrite (library_overwrite_enabled in terraform-provider-alz, --library-overwrite-enabled in alzlibtool) was silently ignored for policy definitions and policy set definitions. With this change, a later library that redefines one of these assets replaces the earlier version again, and versions added by a later library are retained.

Fixes Azure/Azure-Landing-Zones#4295. Follows up on #249, which documented the previous behaviour as a limitation pending a decision on the intended contract. The ALZ documentation, the engine's behaviour before #222, and the Upsert contract all show that the later library is meant to win.

Problem

Redefining a library asset with the same name in a custom library is the documented way to replace it:

provider "alz" {
  library_overwrite_enabled = true
  library_references = [
    { path = "platform/alz", ref = "2025.02.0" },
    { custom_url = "${path.root}/lib" } # contains a modified Enforce-Guardrails-Network
  ]
}

Since policy versioning was introduced in #222 (alzlib v0.29.0, provider v0.20.0), the custom version of a policy (set) definition was discarded and the library version was deployed. No error or warning was raised. Deployments that relied on such overrides had them reverted on the next apply after a provider upgrade:

~ subnetNsg = { ~ defaultValue = "Audit" -> "Deny" }

Two further symptoms had the same cause:

  • A new version of a definition added by a later library was dropped, even without the overwrite flag.
  • AddPolicyDefinitions swallowed the real error and returned 0 errors occurred:.

Root cause

addPolicyAndRoleAssets called Upsert with the receiver and argument swapped. Upsert merges its argument into the receiver, so the stored collection was merged into the incoming one, and the incoming collection was then discarded:

if psdv, exists := az.policySetDefinitions[k]; exists {
    v.Upsert(psdv, az.Options.AllowOverwrite) // stored merged into incoming
    continue                                  // incoming discarded, map still holds stored
}

Separately, AddPolicyDefinitions called pdvc.Upsert(pdvc, ...) on failure. That upserts the collection into itself and loses the original error. AddPolicySetDefinitions already handled this correctly.

How it is resolved

The incoming collection is now merged into the stored one, so the library processed last wins version by version:

- if err := v.Upsert(pdv, az.Options.AllowOverwrite); err != nil {
+ if err := pdv.Upsert(v, az.Options.AllowOverwrite); err != nil {

The same change is applied to policy set definitions. AddPolicyDefinitions now appends the actual Add error. No changes were needed in assets; Upsert already had the intended semantics.

Merging per version, rather than replacing the whole collection, keeps earlier versions that the later library does not redefine. Assignments pinned to an older definitionVersion therefore still resolve.

Changes

File Change
alzlib.go Swap the Upsert receiver and argument for policy (set) definitions; return the real error from AddPolicyDefinitions
alzlib_test.go Add TestAddPolicyAndRoleAssetsLaterLibraryDefinitions (9 cases × policy definition / policy set definition) and TestAddPolicyDefinitionsReturnsAddError
cmd/alzlibtool/command/{check,generate,document,cache}/*.go Replace the help text and stderr warnings added in #249 ("the dependency version is kept") with the per-version replacement semantics
library_test.go Test_libraryCmd_policyDefinitionOverrideNotApplied → ..._OverrideApplied: now expects exit code 0
architecture_overwrite_test.go outputAssetContract now expects the member's policy definition in the generated artifact

Behaviour after the fix

Same-name definition in a later library AllowOverwrite = false AllowOverwrite = true
Versionless Error (unchanged) Later library wins (was: earlier kept)
Same version, different content Error (unchanged) Later library wins (was: earlier kept)
Same version, identical content No-op (unchanged) No-op (unchanged)
New version Both versions kept (was: new version dropped) Both versions kept (was: new version dropped)
Versionless ↔ versioned Error (unchanged) Error (unchanged)

Redefined assets are still replaced in full, not merged field by field. Behaviour for policy assignments, role definitions, archetypes, architectures and policy default values is unchanged. Duplicate archetype override names remain an error.

Out of scope

Testing

All tests were run locally.

In-repo: go test ./... (including integrationtest) and golangci-lint pass. The new unit tests fail on main and pass with this fix.

Local verification against main (d5b42ab) and this branch, covering:

  • Issue scenario: override Enforce-Guardrails-Network in real platform/alz 2025.02.0 and 2026.04.2. main keeps Deny; this PR applies Audit.
  • Override types: versionless, same version, and a new version added alongside the existing one.
  • Library order: provider-style library_references, a three-library chain, and reversed order. The last library processed wins.
  • Deployment output: policy set parameter defaults, the latest version deployed, and policy role assignments all follow the later library.
  • No regression: a library that redefines nothing produces byte-identical output to main; with redefinitions, only the redefined assets differ. Conflicts without the flag are still rejected.

Backwards compatibility and upgrade impact

  • Default (AllowOverwrite = false): conflicts are rejected exactly as before. The only change is that a new version added by a later library is now retained instead of silently dropped.
  • Libraries that redefine nothing: generated output is byte-identical to main.
  • Users who enabled overwrite and redefine policy (set) definitions: their custom versions take effect again. This is the documented behaviour, but it surfaces as a plan diff on upgrade. A stale custom copy could now revert later upstream fixes, so users should compare their copies against the current library version before applying.
  • No public API changes.

Suggested release note

Bug fix: policy definitions and policy set definitions redefined by a later library are applied again when library overwrite is enabled, and versions added by a later library are retained. Since v0.29.0 these were silently ignored (Azure/Azure-Landing-Zones#4295).

Behaviour change: if you enabled library overwrite and redefine policy (set) definitions in a custom library, your custom versions now take effect. Review the plan or generated output before applying.

@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Oct 6, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 70.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.61%. Comparing base (d5b42ab) to head (7bea846).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
cmd/alzlibtool/command/cache/create.go 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #252      +/-   ##
==========================================
+ Coverage   49.54%   49.61%   +0.07%     
==========================================
  Files          54       54              
  Lines        4967     4962       -5     
==========================================
+ Hits         2461     2462       +1     
+ Misses       2210     2207       -3     
+ Partials      296      293       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This branch was successfully deployed

1 active deployment
test — 7bea8465 Deployed Oct 6, 2026 by Y1ngJ1eChen via test (ubuntu-latest) #682
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants