fix(provision): support ${VAR} substitution in infra.deploymentStacks#9238
Draft
vhvb1989 wants to merge 6 commits into
Draft
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
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
|
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.