Fix private networking deployment by dropping defaultOutboundAccess

[Roland Krummenacher (RolandKrummenacher)]

Fixes #2258

🐛 Problem

Deploying with enablePublicAccess = false fails on dev at the first deployment script:

DeploymentScriptStorageAccountWithServiceEndpointEnabled
Storage account '<script storage>' has firewall settings enabled which are not supported for
deployment scripts.

The shipped v14 template deploys the same scenario successfully, so this is a regression in the current cycle. Both private network modes are affected, since privateRouting is !enablePublicAccess.

The error message is misleading. The script storage account it names is configured identically in the working v14 deployment:

v14  (deploys)   publicNetworkAccess=Enabled  defaultAction=Deny  bypass=AzureServices  vnetRules=[script-subnet]
dev  (fails)     publicNetworkAccess=Enabled  defaultAction=Deny  bypass=AzureServices  vnetRules=[script-subnet]

🔍 Cause

defaultOutboundAccess is now set explicitly on all three subnets. Setting it — at any value, on any subnet in the virtual network — makes the Deployment Scripts service reject the script storage account.

🔧 Solution

Drop the property from the three subnet definitions. Attaching the NAT Gateway is what routes outbound traffic, so the explicit setting is redundant and the feature is unaffected.

A comment above the NAT Gateway block records why the property must not come back, including the fact that the Azure error points at the storage account rather than the subnet.

🧪 Validation

Nine deployments on a test subscription (storage-only, same region):

#	Template / mode	property set	NAT Gateway	Result
1	dev, private	yes (all)	yes	❌
2	dev, vnet	yes (all)	no	❌
3	v14 shipped	no	no	✅
4	dev, same params as #3	yes (all)	no	❌
5	dev, property removed	no	no	✅
6	dev, removed on script subnet only	yes (2)	yes	❌
7	dev, script subnet untouched	yes (2)	yes	❌
8	repeat of #5	no	no	✅
9	dev, property removed, NAT on	no	yes	✅

Failure occurs exactly when the property is set, without exception. #3 vs #4 differ in nothing else. #5/#8 show the successes reproduce. #6/#7 rule out a script-subnet-only explanation. #9 was run as a falsifiable prediction and cleared the NAT Gateway.

This branch: rebuilt from source and deployed with enableNatGateway: true and enablePublicAccess: false — succeeded in 18 minutes, all 7 deployment scripts ran, NAT Gateway attached to the script and Data Explorer subnets, property absent everywhere.

Single-file template change; no PowerShell, workbook or query code is touched.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Fixes a regression in the FinOps hub private networking deployment (when enablePublicAccess = false) by removing an Azure subnet property that causes Deployment Scripts to fail even though the script storage account configuration is unchanged.

Changes:

• Removed defaultOutboundAccess from all three subnet definitions to prevent Deployment Scripts from rejecting the script storage account.
• Updated the NAT Gateway section comment to document the deployment-script failure mode and why defaultOutboundAccess must not be reintroduced.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[Michael Flanakin (flanakin)]

🤖 [AI][Claude] PR Review

Summary: This is a well-diagnosed, well-validated fix — removing defaultOutboundAccess from the three subnet definitions resolves a real deployment regression where the Deployment Scripts service rejects the script storage account whenever that property is set on any subnet in the vNet. The Bicep change itself is correct, minimal, and builds cleanly, and the comment explaining why the property must not come back is a nice touch. No blockers.

The only gap: this PR only touches infrastructure.bicep, but three other files still describe the old defaultOutboundAccess behavior that no longer exists, so they're now inaccurate. None of these lines are part of this PR's diff, so I can't attach inline comments — listing them here instead.

⚠️ Should fix (3) — stale references to the removed property

1. src/powershell/Public/Deploy-FinOpsHub.ps1 (NetworkMode parameter help text) — currently says private mode gives you "private endpoints + NAT Gateway, subnets locked down with defaultOutboundAccess=false." That property is gone now. Suggest something like: "private endpoints + NAT Gateway for controlled outbound access — required when the 'Subnets should be private' policy is enforced."
2. src/templates/finops-hub/modules/fx/hub-types.bicep (natGateway option description) — says "When enabled, subnets disable Azure default outbound access and route through the NAT Gateway." The first half is no longer true; only the NAT Gateway attachment happens now. Suggest: "When enabled, the script and Data Explorer subnets route outbound traffic through the NAT Gateway."
3. src/templates/finops-hub/createUiDefinition.json (enableNatGateway checkbox label) — this is customer-facing Azure Portal text and currently reads "Deploy NAT Gateway and disable default outbound access," which no longer matches what the checkbox does. Suggest: "Deploy NAT Gateway for controlled outbound access."

💡 Suggestions (2)

1. src/scripts/Deploy-Hub.ps1 — private mode help text says "subnets locked down," which still implies the removed behavior even though it doesn't name the property directly. Worth tightening for consistency with whatever wording is chosen for Deploy-FinOpsHub.ps1, though lower priority since it's not explicitly incorrect.
2. Changelog — no entry added. Defensible: the feature that introduced the bug (NAT Gateway for private subnets, #2163) is only in the commented-out "Unreleased" section of the changelog, not a released version, so this is arguably fixing pre-release work rather than a shipped regression. Worth a quick check with maintainers on whether the #2163 changelog entry (once uncommented for release) should just reflect the corrected behavior directly, with no separate "Fixed" line needed.

[Roland Krummenacher (RolandKrummenacher)]

Thanks — all three "should fix" items and the Deploy-Hub.ps1 suggestion are addressed in acad6d3, along with two more instances the review missed.

Fixed (6 strings)

File	Was	Now
src/powershell/Public/Deploy-FinOpsHub.ps1	"subnets locked down with defaultOutboundAccess=false"	"NAT Gateway for controlled outbound access"
src/scripts/Deploy-Hub.ps1	"subnets locked down"	same wording as above, for consistency
src/templates/finops-hub/createUiDefinition.json	"Deploy NAT Gateway and disable default outbound access"	"Deploy NAT Gateway for controlled outbound access"
src/templates/finops-hub/modules/fx/hub-types.bicep	"subnets disable Azure default outbound access and route through the NAT Gateway"	"the script and Data Explorer subnets route outbound traffic through the NAT Gateway"
src/templates/finops-hub/main.bicep	same stale sentence	same correction
src/templates/finops-hub/modules/hub.bicep	same stale sentence	same correction

The last two are the enableNatGateway @description() strings. They matter more than the others, because they render into the generated ARM template and the published parameter reference — the incorrect text would have reached template consumers, not just contributors reading source.

I used "the script and Data Explorer subnets" rather than a bare "subnets" because only those two are attached to the NAT Gateway; the private endpoint subnet is not.

Deploy-Hub.ps1 line 92 was left alone deliberately: it documents -Private (equivalent to vnet mode), and "default outbound access" is still accurate there — that mode genuinely relies on Azure default outbound.

On the changelog

I've left it out. The #2163 entry is still inside the commented-out Unreleased block, and its text is "Added VNet and private network modes, including opt-in NAT Gateway support for private mode" — it never described the defaultOutboundAccess behavior, so it stays accurate as written and needs no correction when it's uncommented for release. Happy to add a "Fixed" line if you'd rather have the bug recorded explicitly.

Validation

• bicep build clean on both main.bicep and modules/hub.bicep (only the two pre-existing no-unused-params warnings in Recommendations/app.bicep)
• createUiDefinition.json parses; both .ps1 files parse
• Lint.Tests.ps1 — 2400/2400 pass

[Brett Wilson (MSBrett)]

The current change removes a policy-required subnet setting. NAT Gateway attachment and Azure Policy compliance are separate requirements. The deployment-script storage path must change so both requirements can be met.

[Brett Wilson (MSBrett)]

Findings and test evidence

NAT Gateway attachment is necessary for outbound traffic, but it does not meet the Azure Policy requirement by itself. Policy 7bca8353-aa3b-429b-904a-9229c4385837 requires each private mode subnet to set defaultOutboundAccess to false. The original PR removed that setting from all three subnets, so policy could deny the deployment.

The Deployment Scripts conflict comes from the Storage service-endpoint design. The fix keeps defaultOutboundAccess: false in private mode, keeps NAT on the script and Data Explorer subnets, and replaces the Storage service endpoint with the documented Azure Files private endpoint and privatelink.file.core.windows.net private DNS zone.

Requirements traceability matrix

Requirement	Evidence	Result
Private mode subnets set defaultOutboundAccess: false; VNet mode retains default outbound access	Compiled-ARM regression test checks all three conditional subnet expressions	Pass
Script and Data Explorer subnets use NAT	Compiled-ARM regression test checks two NAT associations	Pass
Deployment-script storage does not use a service endpoint	Regression test checks that the storage subnet firewall rule is absent	Pass
Deployment-script storage uses Azure Files privately	Regression test checks the file private endpoint, disabled public access, and privatelink.file.core.windows.net	Pass
Public Bicep entry points compile	src/templates/finops-hub/main.bicep and src/templates/finops-hub/modules/hub.bicep compiled locally	Pass
Focused networking regression test with Bicep installed	HubsPrivateNetworking.Tests.ps1: 1 passed, 0 failed	Pass
Focused networking regression test without Bicep	Existing repository skip convention exercised: 1 skipped, 0 failed	Pass
Review wording matches conditional behavior	Source comment and changelog now say private mode subnets	Pass
Existing FDPO hub upgrades in place through the canonical repository script	Deploy-Toolkit.ps1 -Build started, but the process was stopped before it returned a final deployment state	Incomplete; no live success claimed

The fix and review remediation are pushed through commit 264e5463.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Core/infrastructure.bicep:28

• The comment says Azure Policy requires private subnets to set defaultOutboundAccess to false explicitly, but the subnet definitions set defaultOutboundAccess: !hub.options.natGateway (which evaluates to true when NAT Gateway is disabled). This is misleading and may cause incorrect assumptions about policy compliance; please reword to describe the actual conditional behavior.

// Azure Policy requires private subnets to set defaultOutboundAccess to false explicitly.

docs-mslearn/toolkit/changelog.md:36

• This changelog entry says private subnets now set defaultOutboundAccess to false, but the template sets defaultOutboundAccess: !hub.options.natGateway (false only when NAT is enabled). Please update the wording so the release note matches the actual behavior.

  - Fixed private-network deployments that Azure Policy blocked when `defaultOutboundAccess` was omitted. Private subnets now set it to `false`, while an Azure Files private endpoint supports deployment-script storage and the NAT Gateway provides required container egress ([#2258](https://github.com/microsoft/finops-toolkit/issues/2258), [#2259](https://github.com/microsoft/finops-toolkit/pull/2259)).

src/powershell/Tests/Unit/HubsPrivateNetworking.Tests.ps1:35

• This test unconditionally runs bicep build in BeforeAll. Other unit tests in this repo skip when the bicep CLI isn't available (e.g., HubsIngestionQueries.Tests.ps1), but this test will fail hard in environments where bicep isn't installed. Please add a Get-Command bicep guard and skip the test when the CLI is missing.

    BeforeAll {
        $repoRoot = (Resolve-Path "$PSScriptRoot/../../../..").Path
        $bicepPath = Join-Path $repoRoot 'src/templates/finops-hub/modules/Microsoft.FinOpsHubs/Core/infrastructure.bicep'
        $templatePath = Join-Path ([System.IO.Path]::GetTempPath()) "finops-hub-infrastructure-$([guid]::NewGuid()).json"

        bicep build $bicepPath --outfile $templatePath
        if ($LASTEXITCODE -ne 0)
        {
            throw 'Failed to compile the FinOps hub infrastructure template.'
        }

[Brett Wilson (MSBrett)]

Addressed the three suppressed suggestions from review 4973971190 in commit $(264e5463d08eb30a2707c6d7ca6c5bb42e614cf0.Substring(0,8)):

Requirement	Change	Result
Match the conditional subnet behavior	The source comment now says private mode subnets set defaultOutboundAccess to alse	Addressed
Keep the changelog accurate	The changelog now uses the same private mode subnets wording	Addressed
Do not fail unit tests when Bicep is unavailable	The focused test follows the existing Get-Command bicep and Set-ItResult -Skipped pattern	Addressed

Validation: both public Bicep entry points compiled; the focused test passed with Bicep installed and skipped cleanly with Bicep absent.

[Roland Krummenacher (RolandKrummenacher)]

Live validation of this branch

Deployed the current branch head end to end on a disposable test subscription, in both modes affected by the regression. This closes the one row marked Incomplete in the traceability matrix above.

Check	private (NAT on)	vnet (NAT off)
Deployment result	Succeeded	Succeeded
Deployment scripts executed	7 / 7	7 / 7
defaultOutboundAccess on all three subnets	false	true (!natGateway)
Service endpoint on script subnet	removed	removed
Script storage public access	Disabled	Disabled
Script storage virtual network rules	0	0
privatelink.file.core.windows.net zone + vNet link	present	present

Storage-only (dataExplorerName: ''), westus, template built from source on this branch.

The 7/7 is the part that matters. In all five failing runs from the original investigation, the deployment aborted at the first deployment script, before any container group started — so "Succeeded" alone would not have been sufficient evidence. Here the container groups actually ran, and the private mode ends up with defaultOutboundAccess: false on all three subnets in the deployed resources, not just in the compiled ARM.

For completeness: the compiled-ARM regression tests in HubsPrivateNetworking.Tests.ps1 verify the template says the right thing; the original failure only surfaced at execution time, on a template that was already valid. The two checks complement each other.

One observation, not a blocker

In vnet mode the subnets end up with defaultOutboundAccess: true, since the expression is !natGateway. Policy 7bca8353-aa3b-429b-904a-9229c4385837 requires false, so vnet mode is not policy-compliant — only private mode is. That matches how the two modes are documented, but it does mean "VNet mode" alone will not satisfy the Subnets should be private policy. Worth a separate look if that was not the intent; nothing to change here.

On the original approach in this PR

Removing defaultOutboundAccess was the wrong half of the fix — the policy flags the property being absent as well as being true, so it would have traded the deployment failure for a policy violation. The service-endpoint change addresses the same conflict from the side that is actually optional. In the nine runs behind #2258, the service endpoint on the script storage was the one variable never altered, which is why it did not surface as a candidate.

🤖 Generated with Claude Code