fix(provision): support ${VAR} substitution in infra.deploymentStacks#9238
Open
vhvb1989 wants to merge 6 commits into
Open
fix(provision): support ${VAR} substitution in infra.deploymentStacks#9238vhvb1989 wants to merge 6 commits into
vhvb1989 wants to merge 6 commits into
Conversation
Replace the untyped `map[string]any` deploymentStacks field with a typed
`DeploymentStacksConfig` struct whose `denySettings.excludedActions` and
`denySettings.excludedPrincipals` leaves are `ExpandableString`, so `${VAR}`
environment-variable substitution now works there like the rest of azure.yaml
(fixes #9232).
Substitution is resolved lazily at deploy time, checking plan-time layer
outputs (VirtualEnv) first and then the azd environment; an unset variable is
treated as a misconfiguration and errors out. The resolved values are emitted
as a camelCase map consumed by the unchanged deployment-stacks API layer.
deploymentStacks is intentionally not forwarded to external (extension)
providers since it configures the Azure Deployment Stacks control-plane request
and is bicep-only; the proto field is retained for wire compatibility but left
empty.
The alpha schema tightens excludedActions/excludedPrincipals to string arrays
and documents ${VAR} support; the existing bicep-or-undefined gate is unchanged.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dd265349-d2a5-4c0b-95f0-990ceadeb6a0
|
Azure Pipelines: Successfully started running 1 pipeline(s). 21 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds environment-variable substitution for Bicep deployment-stack deny settings while preserving external-provider wire compatibility.
Changes:
- Introduces typed deployment-stack configuration.
- Resolves deny-setting variables from layer outputs or the azd environment.
- Updates schemas, integration paths, and unit tests.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
schemas/alpha/azure.yaml.json |
Documents substitution and string array types. |
cli/azd/pkg/infra/provisioning/provider.go |
Uses typed deployment-stack configuration. |
cli/azd/pkg/infra/provisioning/provider_test.go |
Updates option-defaulting tests. |
cli/azd/pkg/infra/provisioning/options_test.go |
Updates layered-config validation test. |
cli/azd/pkg/infra/provisioning/deployment_stacks_config.go |
Defines typed stack configuration. |
cli/azd/pkg/infra/provisioning/deployment_stacks_config_test.go |
Tests YAML unmarshalling. |
cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go |
Resolves substitutions and builds API options. |
cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go |
Tests resolution and precedence. |
cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go |
Integrates resolved options into deploy and destroy. |
cli/azd/internal/grpcserver/external_provisioning_provider.go |
Stops forwarding Bicep-only settings. |
cli/azd/internal/grpcserver/external_provisioning_provider_test.go |
Verifies settings remain omitted from gRPC. |
Gate deploymentStacks resolution on the deployment.stacks alpha feature so an
unavailable ${VAR} in an inactive deploymentStacks block can no longer fail an
otherwise-valid standard provision. Omit denySettings on the destroy path since
the stack delete APIs consume only actionOnUnmanage and the bypass flag, so
`azd down` can't be blocked by a deny-list variable that is no longer available.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Bypass the deployment-state shortcut when an active deploymentStacks
configuration is present. The state check hashes only the ARM template and
parameters, so a changed ${VAR}-resolved excluded principal/action would
otherwise be silently ignored (leaving stale deny settings on the stack) and
the early return would bypass the ${VAR} resolution/validation. Forcing a real
deployment in that case re-applies the settings and always runs validation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Honor shell-style default expressions (${VAR:-fallback}) in deploymentStacks
deny lists: reject only values that resolve to a blank string, so a reference
with a usable default is accepted instead of being flagged as unset.
- Test typed YAML unmarshal through the production braydonk/yaml parser used by
the azure.yaml loader, not gopkg.in/yaml.v3.
- Make the GetWithDefaults "merges deployment stacks" case a real merge (base
contributes the stack config, other contributes module/name) and assert the
nested pointer config survives.
- Guard map type assertions in the stacks tests with require to fail cleanly
instead of panicking and aborting the package.
- Add a fallback-expression regression test.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go:140
- [azd-code-reviewer] This only rejects an unset reference when the entire expanded entry is blank. An entry such as
Microsoft.Authorization/${MISSING}/writeor${TENANT}-${OBJECT_ID}remains nonblank after substitution and is sent to Azure with the missing segment removed, despite the promised unset-variable validation. Detect every missing reference that is not satisfied by a default expression, regardless of the final string, and add a regression test for an embedded missing variable.
if strings.TrimSpace(substituted) == "" {
if len(lookedUpEmpty) > 0 {
return nil, fmt.Errorf(
"deploymentStacks references unset environment variable(s): %s",
strings.Join(lookedUpEmpty, ", "))
}
Extract the deployment-state shortcut decision into a testable useDeploymentStateShortcut helper and add unit tests covering it, the management-groups actionOnUnmanage branch, and blank deny-list values. Add grpcserver tests for the broker-independent ExternalProvisioningProvider surface (factory, Name, reportProgress/stopProgress) so omitting deployment stacks from the proto options does not regress package coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dd265349-d2a5-4c0b-95f0-990ceadeb6a0
vhvb1989
marked this pull request as ready for review
July 24, 2026 19:55
vhvb1989
requested review from
JeffreyCA,
RickWinter,
hemarina,
richardpark-msft and
tg-msft
as code owners
July 24, 2026 19:55
📋 Prioritization NoteThanks for the contribution! The linked issue isn't in the current milestone yet. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). 21 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go:155
- [azd-code-reviewer] Unset references are rejected only when the entire expanded entry is blank. An entry such as
Microsoft.${UNSET}/*/writeremains nonblank after expansion and is sent to Azure even though this PR promises that every unset variable fails locally. Reject unresolved references embedded in nonblank values too, while still allowing references whose envsubst default actually supplied a value, and cover the composite case in a test.
if strings.TrimSpace(substituted) == "" {
if len(lookedUpEmpty) > 0 {
return nil, fmt.Errorf(
"deploymentStacks references unset environment variable(s): %s",
strings.Join(lookedUpEmpty, ", "))
}
| // deployment would otherwise leave stale deny settings on the stack and would also bypass the | ||
| // ${VAR} resolution/validation performed while building the options map. | ||
| func (p *BicepProvider) hasActiveDeploymentStacksConfig() bool { | ||
| return p.options.DeploymentStacks != nil && p.deploymentStacksEnabled() |
jongio
reviewed
Jul 24, 2026
jongio
left a comment
jongio
left a comment
Member
There was a problem hiding this comment.
A few things worth a look, none of them blocking.
Medium:
- The deployment-state shortcut gate in
deployment_stacks.gois both too broad and too narrow. Details inline.
Low:
- The alpha schema doesn't declare
actionOnUnmanage.managementGroupsand marksresourceGroups/resourcesas required, so the config shape one of the new tests exercises won't validate against the schema. resolveDeploymentStacksValuesonly errors when the whole value trims to blank, so a composite value with one unset variable passes through partially resolved.- The "merges deployment stacks" case in
TestOptions_GetWithDefaultsno longer merges twoDeploymentStacksvalues. - The new grpcserver test uses
context.Background()where the package and the rest of this PR uset.Context().
I ran the new tests plus a local probe of the mergo behavior for the new pointer type. The typed config, the destroy-path deny-settings omission, and the alpha-feature gating all behave the way the doc comments describe.
| // deployment would otherwise leave stale deny settings on the stack and would also bypass the | ||
| // ${VAR} resolution/validation performed while building the options map. | ||
| func (p *BicepProvider) hasActiveDeploymentStacksConfig() bool { | ||
| return p.options.DeploymentStacks != nil && p.deploymentStacksEnabled() |
Member
There was a problem hiding this comment.
This gate cuts both ways and I don't think either direction lands right.
Too broad: any project with a deploymentStacks block and the alpha feature on loses the deployment-state shortcut entirely, so every azd provision re-deploys even when template, parameters and deny settings are all unchanged. That's a user-visible behavior change for stacks users who never touch ${VAR}, and it isn't called out in the PR description.
Too narrow: as the other comment on this line points out, dropping the deploymentStacks block from azure.yaml makes this return false, so the shortcut kicks back in and the old deny assignment stays on the stack.
Folding the resolved stacks map into currentParamsHash covers both. Removing the block changes the hash so the stale-deny case redeploys, and an unchanged config keeps the shortcut instead of forcing a redeploy on every run.
| return nil, fmt.Errorf("resolving deploymentStacks value: %w", err) | ||
| } | ||
|
|
||
| if strings.TrimSpace(substituted) == "" { |
Member
There was a problem hiding this comment.
The unset-variable check only fires when the entire value trims to empty, so partial resolution slips through. Microsoft.Authorization/${UNSET_SCOPE}/write resolves to Microsoft.Authorization//write and goes to ARM, even though lookedUpEmpty recorded UNSET_SCOPE.
For excludedPrincipals the usual shape is a bare ${VAR} so this rarely bites, but excludedActions entries are more likely to be composite.
If you tighten it, len(lookedUpEmpty) > 0 on its own isn't a safe condition. The callback also records the name for ${VAR:-fallback} before Envsubst applies the default, so keying off it directly would break TestResolveDeploymentStacksMap_DefaultExpression.
| type ActionOnUnmanageConfig struct { | ||
| Resources string `yaml:"resources,omitempty" json:"resources,omitempty"` | ||
| ResourceGroups string `yaml:"resourceGroups,omitempty" json:"resourceGroups,omitempty"` | ||
| ManagementGroups string `yaml:"managementGroups,omitempty" json:"managementGroups,omitempty"` |
Member
There was a problem hiding this comment.
managementGroups has no counterpart in the alpha schema. deploymentStacksConfig.actionOnUnmanage declares only resourceGroups and resources and lists both as required, so the exact config TestResolveDeploymentStacksMap_ActionOnUnmanageManagementGroups exercises (managementGroups on its own) fails schema validation.
It predates this PR since the SDK type always had the field, but this is the change that makes it an explicit azd-owned field with a test behind it, and you're already editing that schema block, so it seems like the moment to add the property and relax the required list.
| DeploymentStacks: map[string]any{ | ||
| "stack2": "value2", | ||
| }, | ||
| Module: "custom-module", |
Member
There was a problem hiding this comment.
This case is still named "merges deployment stacks", but otherOptions no longer carries a DeploymentStacks value, so nothing about stacks merging gets asserted. It now exercises Module/Name merging with the stack config riding along unchanged from baseOptions.
The old version covered the map key-union behavior. I checked the new pointer semantics locally and mergo does deep-merge the struct field by field (actionOnUnmanage from base plus denySettings from other yields both), so the behavior is fine, it just isn't covered anymore. Putting a DeploymentStacks back on otherOptions with a different sub-field would restore the assertion.
| t.Run("progress with console", func(t *testing.T) { | ||
| provider := factory(mockinput.NewMockConsole()).(*ExternalProvisioningProvider) | ||
| // Should not panic and should route through the console spinner. | ||
| provider.reportProgress(context.Background(), "deploying") |
Member
There was a problem hiding this comment.
context.Background() here and on 141-142 and 148-149. This package uses t.Context() almost everywhere (335 occurrences against 6), AGENTS.md calls it out, and the other tests added in this PR use it, so these stand out.
Separately, this test covers the DI factory and the progress spinner helpers, neither of which this PR touches. No harm in keeping it, but it's unrelated to the ${VAR} change despite landing in the "raise coverage for deployment stacks changes" commit.
Contributor
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
jongio
approved these changes
Jul 25, 2026
jongio
left a comment
jongio
left a comment
Member
There was a problem hiding this comment.
Approving. Re-checked the full change against the current head: the typed DeploymentStacksConfig, the destroy-path deny-settings omission via deploymentOptionsMap(false), the alpha-feature gating, and dropping deploymentStacks from the external-provider proto path all behave the way the doc comments describe, and the CI checks are green.
The inline comments from my earlier review still stand, but none of them block. They're narrow edge cases in the alpha path (partial ${VAR} resolution when only part of a value is unset, and the deployment-state shortcut when a deploymentStacks block is removed) plus a couple of test and schema coverage notes. Reasonable to fold those into a follow-up.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #9232
Problem
Environment-variable substitution (
${VAR}) was applied across most ofazure.yaml, but not insideinfra.deploymentStacks. Values likedenySettings.excludedPrincipals: [${PIPELINE_SP_OBJECT_ID}]were passed verbatim (as the literal string${PIPELINE_SP_OBJECT_ID}) to the Azure Deployment Stacks API, so per-environment principal/action values couldn't be expressed portably.Change
map[string]anyDeploymentStacksoption with a typedDeploymentStacksConfigstruct (actionOnUnmanage,denySettings). TheexcludedActionsandexcludedPrincipalsleaves areosutil.ExpandableString, so${VAR}now works there like the rest ofazure.yaml.${VAR}is resolved at deploy time, checking plan-time layer outputs (VirtualEnv) first, then the azd environment. An unset variable errors out (a blank excluded principal is almost always a misconfiguration). Resolved values are emitted as a camelCasemap[string]anyconsumed by the unchanged deployment-stacks API layer.deploymentStacksis intentionally not forwarded to external (extension) providers — it configures the Deployment Stacks control-plane request and is bicep-only. The proto field (deployment_stacks) is retained for wire compatibility but left empty.excludedActions/excludedPrincipalsto string arrays and documents${VAR}support. The existing bicep-or-undefined gate is unchanged. v1.0 is untouched (deploymentStacks remains alpha-only).Notes
armdeploymentstacks v1.0.1is the latest; no bump needed. Its types have no struct tags and use custom JSON marshaling, so they can't be reused for YAML directly — hence the azd-owned yaml-tagged struct.Testing
${VAR}resolution,VirtualEnvprecedence, env fallback, unset-var error, options-map integration.go build ./...,go test(provisioning / bicep / grpcserver / azapi), gofmt, copyright, golangci-lint (0 issues), cspell — all pass.