From b4608b74ec97d3a627c6fa11325168e6de235d03 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Tue, 21 Jul 2026 21:01:26 +0000 Subject: [PATCH 1/5] fix(provision): support ${VAR} substitution in infra.deploymentStacks 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 --- .../external_provisioning_provider.go | 12 +- .../external_provisioning_provider_test.go | 11 +- .../provisioning/bicep/bicep_provider.go | 4 +- .../provisioning/bicep/deployment_stacks.go | 139 ++++++++++++++++ .../bicep/deployment_stacks_test.go | 148 ++++++++++++++++++ .../provisioning/deployment_stacks_config.go | 47 ++++++ .../deployment_stacks_config_test.go | 62 ++++++++ .../pkg/infra/provisioning/options_test.go | 4 +- cli/azd/pkg/infra/provisioning/provider.go | 12 +- .../pkg/infra/provisioning/provider_test.go | 25 ++- schemas/alpha/azure.yaml.json | 12 +- 11 files changed, 434 insertions(+), 42 deletions(-) create mode 100644 cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go create mode 100644 cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go create mode 100644 cli/azd/pkg/infra/provisioning/deployment_stacks_config.go create mode 100644 cli/azd/pkg/infra/provisioning/deployment_stacks_config_test.go diff --git a/cli/azd/internal/grpcserver/external_provisioning_provider.go b/cli/azd/internal/grpcserver/external_provisioning_provider.go index 9cd1970a581..12db1ca3d1e 100644 --- a/cli/azd/internal/grpcserver/external_provisioning_provider.go +++ b/cli/azd/internal/grpcserver/external_provisioning_provider.go @@ -334,18 +334,14 @@ func (p *ExternalProvisioningProvider) PlannedOutputs( func convertToProtoOptions( options provisioning.Options, ) (*azdext.ProvisioningOptions, error) { - deploymentStacks := make( - map[string]string, len(options.DeploymentStacks), - ) - for k, v := range options.DeploymentStacks { - deploymentStacks[k] = fmt.Sprintf("%v", v) - } - + // deploymentStacks is intentionally not forwarded to external providers: it configures the + // Azure Deployment Stacks control-plane request and is only valid for the built-in Bicep + // provider. The proto field (deployment_stacks) is retained for wire compatibility but left + // empty for extension providers. protoOptions := &azdext.ProvisioningOptions{ Provider: string(options.Provider), Path: options.Path, Module: options.Module, - DeploymentStacks: deploymentStacks, IgnoreDeploymentState: options.IgnoreDeploymentState, Name: options.Name, } diff --git a/cli/azd/internal/grpcserver/external_provisioning_provider_test.go b/cli/azd/internal/grpcserver/external_provisioning_provider_test.go index 57d6a974088..634400feaa2 100644 --- a/cli/azd/internal/grpcserver/external_provisioning_provider_test.go +++ b/cli/azd/internal/grpcserver/external_provisioning_provider_test.go @@ -35,10 +35,8 @@ func Test_convertToProtoOptions(t *testing.T) { Path: "/infra", Module: "main", Name: "layer1", - DeploymentStacks: map[string]any{ - "stackName": "my-stack", - "intVal": 42, - "boolVal": true, + DeploymentStacks: &provisioning.DeploymentStacksConfig{ + DenySettings: &provisioning.DenySettingsConfig{Mode: "denyDelete"}, }, IgnoreDeploymentState: true, Config: map[string]any{ @@ -56,9 +54,8 @@ func Test_convertToProtoOptions(t *testing.T) { assert.Equal(t, "main", result.Module) assert.Equal(t, "layer1", result.Name) assert.True(t, result.IgnoreDeploymentState) - assert.Equal(t, "my-stack", result.DeploymentStacks["stackName"]) - assert.Equal(t, "42", result.DeploymentStacks["intVal"]) - assert.Equal(t, "true", result.DeploymentStacks["boolVal"]) + // deploymentStacks is bicep-only and intentionally not forwarded to extensions. + assert.Empty(t, result.DeploymentStacks) require.NotNil(t, result.Config) assert.Equal(t, "value1", result.Config.Fields["key1"].GetStringValue()) nested := result.Config.Fields["nested"].GetStructValue() diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go index 6161f459c1a..84ecb2590d1 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go @@ -898,7 +898,7 @@ func (p *BicepProvider) Deploy(ctx context.Context) (*provisioning.DeployResult, deploymentTags[azure.TagKeyAzdDeploymentStateParamHashName] = new(currentParamsHash) } - optionsMap, err := convert.ToMap(p.options) + optionsMap, err := p.deploymentOptionsMap() if err != nil { return nil, err } @@ -1660,7 +1660,7 @@ func (p *BicepProvider) destroyDeployment( p.console.StopSpinner(ctx, progressMessage.Message, input.StepFailed) } }, func(progress *async.Progress[azapi.DeleteDeploymentProgress]) error { - optionsMap, err := convert.ToMap(p.options) + optionsMap, err := p.deploymentOptionsMap() if err != nil { return err } diff --git a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go new file mode 100644 index 00000000000..9ab7cebc17a --- /dev/null +++ b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go @@ -0,0 +1,139 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package bicep + +import ( + "fmt" + "strings" + + "github.com/azure/azure-dev/cli/azd/pkg/convert" + "github.com/azure/azure-dev/cli/azd/pkg/osutil" +) + +// resolveDeploymentStacksMap resolves the typed deployment-stacks configuration into a +// camelCase map[string]any consumable by the deployment-stacks API layer +// (azapi.parseDeploymentStackOptions). It performs ${VAR} environment-variable substitution +// on denySettings.excludedPrincipals and denySettings.excludedActions, resolving values from +// plan-time layer outputs (VirtualEnv) first and then the azd environment. +// +// It returns nil when no deployment-stacks configuration is present, in which case the caller +// should omit the DeploymentStacks key entirely so the API layer applies its defaults. +func (p *BicepProvider) resolveDeploymentStacksMap() (map[string]any, error) { + cfg := p.options.DeploymentStacks + if cfg == nil { + return nil, nil + } + + result := map[string]any{} + + if cfg.ActionOnUnmanage != nil { + actionOnUnmanage := map[string]any{} + if cfg.ActionOnUnmanage.Resources != "" { + actionOnUnmanage["resources"] = cfg.ActionOnUnmanage.Resources + } + if cfg.ActionOnUnmanage.ResourceGroups != "" { + actionOnUnmanage["resourceGroups"] = cfg.ActionOnUnmanage.ResourceGroups + } + if cfg.ActionOnUnmanage.ManagementGroups != "" { + actionOnUnmanage["managementGroups"] = cfg.ActionOnUnmanage.ManagementGroups + } + result["actionOnUnmanage"] = actionOnUnmanage + } + + if cfg.DenySettings != nil { + denySettings := map[string]any{} + if cfg.DenySettings.Mode != "" { + denySettings["mode"] = cfg.DenySettings.Mode + } + if cfg.DenySettings.ApplyToChildScopes != nil { + denySettings["applyToChildScopes"] = *cfg.DenySettings.ApplyToChildScopes + } + + excludedActions, err := p.resolveDeploymentStacksValues(cfg.DenySettings.ExcludedActions) + if err != nil { + return nil, err + } + if excludedActions != nil { + denySettings["excludedActions"] = excludedActions + } + + excludedPrincipals, err := p.resolveDeploymentStacksValues(cfg.DenySettings.ExcludedPrincipals) + if err != nil { + return nil, err + } + if excludedPrincipals != nil { + denySettings["excludedPrincipals"] = excludedPrincipals + } + + result["denySettings"] = denySettings + } + + return result, nil +} + +// resolveDeploymentStacksValues evaluates ${VAR} references in each value, resolving from the +// plan-time layer outputs (VirtualEnv) first and then the azd environment. It returns an error +// if any referenced environment variable is unset, since a blank value in deny settings (for +// example, an empty excluded principal) is almost always a misconfiguration. +func (p *BicepProvider) resolveDeploymentStacksValues(values []osutil.ExpandableString) ([]string, error) { + if len(values) == 0 { + return nil, nil + } + + resolved := make([]string, 0, len(values)) + for _, value := range values { + var unset []string + substituted, err := value.Envsubst(func(name string) string { + if p.options.VirtualEnv != nil { + if v, has := p.options.VirtualEnv[name]; has { + return v + } + } + + v, ok := p.env.LookupEnv(name) + if !ok { + unset = append(unset, name) + } + return v + }) + if err != nil { + return nil, fmt.Errorf("resolving deploymentStacks value: %w", err) + } + + if len(unset) > 0 { + return nil, fmt.Errorf( + "deploymentStacks references unset environment variable(s): %s", strings.Join(unset, ", ")) + } + + resolved = append(resolved, substituted) + } + + return resolved, nil +} + +// deploymentOptionsMap builds the generic options map handed to the deployment API layer, +// with the deployment-stacks configuration resolved (including ${VAR} substitution). Standard +// (non-stack) deployments ignore this map; stack deployments read only the DeploymentStacks key. +func (p *BicepProvider) deploymentOptionsMap() (map[string]any, error) { + optionsMap, err := convert.ToMap(p.options) + if err != nil { + return nil, err + } + + stacks, err := p.resolveDeploymentStacksMap() + if err != nil { + return nil, err + } + + if stacks == nil { + // The typed DeploymentStacksConfig is not JSON-serializable in a form the API layer can + // consume (ExpandableString has no exported fields), so drop whatever convertOptionsToMap + // produced for it and let the API layer apply its defaults. + delete(optionsMap, "DeploymentStacks") + } else { + optionsMap["DeploymentStacks"] = stacks + } + + return optionsMap, nil +} diff --git a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go new file mode 100644 index 00000000000..e1c678633fe --- /dev/null +++ b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go @@ -0,0 +1,148 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package bicep + +import ( + "testing" + + "github.com/azure/azure-dev/cli/azd/pkg/azure" + "github.com/azure/azure-dev/cli/azd/pkg/infra/provisioning" + "github.com/azure/azure-dev/cli/azd/pkg/osutil" + "github.com/azure/azure-dev/cli/azd/test/mocks" + "github.com/stretchr/testify/require" +) + +func expandableStrings(values ...string) []osutil.ExpandableString { + result := make([]osutil.ExpandableString, 0, len(values)) + for _, value := range values { + result = append(result, osutil.NewExpandableString(value)) + } + return result +} + +func minimalArmTemplate() azure.ArmTemplate { + return azure.ArmTemplate{ + Schema: "https://schema.management.azure.com/schemas/2018-05-01/subscriptionDeploymentTemplate.json#", + ContentVersion: "1.0.0.0", + Parameters: azure.ArmTemplateParameterDefinitions{}, + Outputs: azure.ArmTemplateOutputs{}, + } +} + +func TestResolveDeploymentStacksMap_Nil(t *testing.T) { + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), nil) + provider.options.DeploymentStacks = nil + + stacks, err := provider.resolveDeploymentStacksMap() + require.NoError(t, err) + require.Nil(t, stacks) + + // deploymentOptionsMap must omit the DeploymentStacks key entirely so the API layer + // applies its own defaults. + optionsMap, err := provider.deploymentOptionsMap() + require.NoError(t, err) + require.NotContains(t, optionsMap, "DeploymentStacks") +} + +func TestResolveDeploymentStacksMap_ResolvesEnvSubstitution(t *testing.T) { + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), map[string]string{ + "OPERATOR_OBJECT_ID": "11111111-1111-1111-1111-111111111111", + }) + + // Plan-time layer output takes precedence over the azd environment. + provider.options.VirtualEnv = map[string]string{ + "PIPELINE_SP_OBJECT_ID": "22222222-2222-2222-2222-222222222222", + } + + provider.options.DeploymentStacks = &provisioning.DeploymentStacksConfig{ + ActionOnUnmanage: &provisioning.ActionOnUnmanageConfig{ + Resources: "delete", + ResourceGroups: "delete", + }, + DenySettings: &provisioning.DenySettingsConfig{ + Mode: "denyDelete", + ApplyToChildScopes: new(true), + ExcludedActions: expandableStrings("Microsoft.Authorization/*/write"), + ExcludedPrincipals: expandableStrings("${PIPELINE_SP_OBJECT_ID}", "${OPERATOR_OBJECT_ID}"), + }, + } + + stacks, err := provider.resolveDeploymentStacksMap() + require.NoError(t, err) + + actionOnUnmanage, ok := stacks["actionOnUnmanage"].(map[string]any) + require.True(t, ok) + require.Equal(t, "delete", actionOnUnmanage["resources"]) + require.Equal(t, "delete", actionOnUnmanage["resourceGroups"]) + + denySettings, ok := stacks["denySettings"].(map[string]any) + require.True(t, ok) + require.Equal(t, "denyDelete", denySettings["mode"]) + require.Equal(t, true, denySettings["applyToChildScopes"]) + require.Equal(t, []string{"Microsoft.Authorization/*/write"}, denySettings["excludedActions"]) + require.Equal(t, []string{ + "22222222-2222-2222-2222-222222222222", + "11111111-1111-1111-1111-111111111111", + }, denySettings["excludedPrincipals"]) +} + +func TestResolveDeploymentStacksMap_UnsetVarErrors(t *testing.T) { + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), nil) + + provider.options.DeploymentStacks = &provisioning.DeploymentStacksConfig{ + DenySettings: &provisioning.DenySettingsConfig{ + Mode: "denyDelete", + ExcludedPrincipals: expandableStrings("${DOES_NOT_EXIST}"), + }, + } + + _, err := provider.resolveDeploymentStacksMap() + require.Error(t, err) + require.Contains(t, err.Error(), "DOES_NOT_EXIST") +} + +func TestResolveDeploymentStacksMap_EnvFallback(t *testing.T) { + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), map[string]string{ + "OPERATOR_OBJECT_ID": "33333333-3333-3333-3333-333333333333", + }) + + provider.options.DeploymentStacks = &provisioning.DeploymentStacksConfig{ + DenySettings: &provisioning.DenySettingsConfig{ + Mode: "denyWriteAndDelete", + ExcludedPrincipals: expandableStrings("${OPERATOR_OBJECT_ID}"), + }, + } + + stacks, err := provider.resolveDeploymentStacksMap() + require.NoError(t, err) + + denySettings := stacks["denySettings"].(map[string]any) + require.Equal(t, []string{"33333333-3333-3333-3333-333333333333"}, denySettings["excludedPrincipals"]) +} + +func TestDeploymentOptionsMap_IncludesResolvedStacks(t *testing.T) { + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), map[string]string{ + "OPERATOR_OBJECT_ID": "44444444-4444-4444-4444-444444444444", + }) + + provider.options.DeploymentStacks = &provisioning.DeploymentStacksConfig{ + DenySettings: &provisioning.DenySettingsConfig{ + Mode: "denyDelete", + ExcludedPrincipals: expandableStrings("${OPERATOR_OBJECT_ID}"), + }, + } + + optionsMap, err := provider.deploymentOptionsMap() + require.NoError(t, err) + + stacks, ok := optionsMap["DeploymentStacks"].(map[string]any) + require.True(t, ok) + denySettings := stacks["denySettings"].(map[string]any) + require.Equal(t, []string{"44444444-4444-4444-4444-444444444444"}, denySettings["excludedPrincipals"]) +} diff --git a/cli/azd/pkg/infra/provisioning/deployment_stacks_config.go b/cli/azd/pkg/infra/provisioning/deployment_stacks_config.go new file mode 100644 index 00000000000..13e79334b38 --- /dev/null +++ b/cli/azd/pkg/infra/provisioning/deployment_stacks_config.go @@ -0,0 +1,47 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package provisioning + +import "github.com/azure/azure-dev/cli/azd/pkg/osutil" + +// DeploymentStacksConfig is the typed representation of the `infra.deploymentStacks` +// block in azure.yaml. It configures the Azure Deployment Stacks control-plane request +// (not the IaC template inputs) and is only valid when the provider is Bicep. +// +// The JSON tags intentionally match the camelCase keys expected by the +// armdeploymentstacks SDK (via its custom JSON marshaling) so the resolved +// configuration can be handed to the deployment-stacks API layer unchanged. +type DeploymentStacksConfig struct { + // ActionOnUnmanage defines the behavior of resources that are no longer managed + // after the deployment stack is updated or deleted. + ActionOnUnmanage *ActionOnUnmanageConfig `yaml:"actionOnUnmanage,omitempty" json:"actionOnUnmanage,omitempty"` + // DenySettings defines how resources deployed by the stack are locked. + DenySettings *DenySettingsConfig `yaml:"denySettings,omitempty" json:"denySettings,omitempty"` +} + +// ActionOnUnmanageConfig defines the unmanage behavior for each resource scope. +// Valid values are "delete" and "detach". +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"` +} + +// DenySettingsConfig defines the deny-assignment (lock) configuration for a deployment stack. +// +// ExcludedActions and ExcludedPrincipals support ${VAR} environment-variable substitution, +// resolved from the azd environment at provision time. This makes per-environment values +// (for example, pipeline service principal or operator object IDs) expressible in a portable +// template instead of being committed as literals. +type DenySettingsConfig struct { + // Mode defines the denied actions. One of "none", "denyDelete", "denyWriteAndDelete". + Mode string `yaml:"mode,omitempty" json:"mode,omitempty"` + // ApplyToChildScopes applies the deny settings to child resource scopes of every managed + // resource with a deny assignment. + ApplyToChildScopes *bool `yaml:"applyToChildScopes,omitempty" json:"applyToChildScopes,omitempty"` + // ExcludedActions is the list of role-based management operations excluded from the deny settings. + ExcludedActions []osutil.ExpandableString `yaml:"excludedActions,omitempty" json:"excludedActions,omitempty"` + // ExcludedPrincipals is the list of Entra ID principal IDs excluded from the lock. + ExcludedPrincipals []osutil.ExpandableString `yaml:"excludedPrincipals,omitempty" json:"excludedPrincipals,omitempty"` +} diff --git a/cli/azd/pkg/infra/provisioning/deployment_stacks_config_test.go b/cli/azd/pkg/infra/provisioning/deployment_stacks_config_test.go new file mode 100644 index 00000000000..14358efc6b3 --- /dev/null +++ b/cli/azd/pkg/infra/provisioning/deployment_stacks_config_test.go @@ -0,0 +1,62 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package provisioning + +import ( + "testing" + + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" +) + +func TestDeploymentStacksConfigUnmarshalYAML(t *testing.T) { + const source = ` +provider: bicep +deploymentStacks: + actionOnUnmanage: + resources: delete + resourceGroups: detach + denySettings: + mode: denyDelete + applyToChildScopes: true + excludedActions: + - Microsoft.Authorization/*/write + excludedPrincipals: + - ${PIPELINE_SP_OBJECT_ID} + - literal-principal-id +` + + var options Options + require.NoError(t, yaml.Unmarshal([]byte(source), &options)) + + require.NotNil(t, options.DeploymentStacks) + require.NotNil(t, options.DeploymentStacks.ActionOnUnmanage) + require.Equal(t, "delete", options.DeploymentStacks.ActionOnUnmanage.Resources) + require.Equal(t, "detach", options.DeploymentStacks.ActionOnUnmanage.ResourceGroups) + + denySettings := options.DeploymentStacks.DenySettings + require.NotNil(t, denySettings) + require.Equal(t, "denyDelete", denySettings.Mode) + require.NotNil(t, denySettings.ApplyToChildScopes) + require.True(t, *denySettings.ApplyToChildScopes) + + require.Len(t, denySettings.ExcludedActions, 1) + action, err := denySettings.ExcludedActions[0].Envsubst(func(string) string { return "" }) + require.NoError(t, err) + require.Equal(t, "Microsoft.Authorization/*/write", action) + + require.Len(t, denySettings.ExcludedPrincipals, 2) + resolved, err := denySettings.ExcludedPrincipals[0].Envsubst(func(name string) string { + if name == "PIPELINE_SP_OBJECT_ID" { + return "resolved-sp-id" + } + return "" + }) + require.NoError(t, err) + require.Equal(t, "resolved-sp-id", resolved) + + literal, err := denySettings.ExcludedPrincipals[1].Envsubst(func(string) string { return "" }) + require.NoError(t, err) + require.Equal(t, "literal-principal-id", literal) +} diff --git a/cli/azd/pkg/infra/provisioning/options_test.go b/cli/azd/pkg/infra/provisioning/options_test.go index 67229b3058b..26fac07a768 100644 --- a/cli/azd/pkg/infra/provisioning/options_test.go +++ b/cli/azd/pkg/infra/provisioning/options_test.go @@ -218,7 +218,9 @@ func TestOptionsValidate(t *testing.T) { "layers with DeploymentStacks set at top level is invalid", func(t *testing.T) { opts := &Options{ - DeploymentStacks: map[string]any{"k": "v"}, + DeploymentStacks: &DeploymentStacksConfig{ + DenySettings: &DenySettingsConfig{Mode: "none"}, + }, Layers: []Options{ {Name: "l1", Path: "infra/l1"}, }, diff --git a/cli/azd/pkg/infra/provisioning/provider.go b/cli/azd/pkg/infra/provisioning/provider.go index 86fd15b504f..b2366624d46 100644 --- a/cli/azd/pkg/infra/provisioning/provider.go +++ b/cli/azd/pkg/infra/provisioning/provider.go @@ -37,12 +37,12 @@ const ( // Options for a provisioning provider. type Options struct { - Provider ProviderKind `yaml:"provider,omitempty"` - Path string `yaml:"path,omitempty"` - Module string `yaml:"module,omitempty"` - Name string `yaml:"name,omitempty"` - Hooks HooksConfig `yaml:"hooks,omitempty"` - DeploymentStacks map[string]any `yaml:"deploymentStacks,omitempty"` + Provider ProviderKind `yaml:"provider,omitempty"` + Path string `yaml:"path,omitempty"` + Module string `yaml:"module,omitempty"` + Name string `yaml:"name,omitempty"` + Hooks HooksConfig `yaml:"hooks,omitempty"` + DeploymentStacks *DeploymentStacksConfig `yaml:"deploymentStacks,omitempty"` // Config holds provider-specific configuration options Config map[string]any `yaml:"config,omitempty"` // DependsOn lists the names of other layers this layer must wait for diff --git a/cli/azd/pkg/infra/provisioning/provider_test.go b/cli/azd/pkg/infra/provisioning/provider_test.go index 3ec26764390..a294323ffba 100644 --- a/cli/azd/pkg/infra/provisioning/provider_test.go +++ b/cli/azd/pkg/infra/provisioning/provider_test.go @@ -122,24 +122,17 @@ func TestOptions_GetWithDefaults(t *testing.T) { name: "merges deployment stacks", baseOptions: Options{ Provider: Bicep, - DeploymentStacks: map[string]any{ - "stack1": "value1", - }, - }, - otherOptions: []Options{ - { - DeploymentStacks: map[string]any{ - "stack2": "value2", - }, + DeploymentStacks: &DeploymentStacksConfig{ + DenySettings: &DenySettingsConfig{Mode: "denyDelete"}, }, }, + otherOptions: nil, expectedResult: Options{ Provider: Bicep, Module: "main", Path: "infra", - DeploymentStacks: map[string]any{ - "stack1": "value1", - "stack2": "value2", + DeploymentStacks: &DeploymentStacksConfig{ + DenySettings: &DenySettingsConfig{Mode: "denyDelete"}, }, }, expectError: false, @@ -202,8 +195,8 @@ func TestOptions_GetWithDefaults(t *testing.T) { Path: "custom-path", Module: "custom-module", Name: "custom-name", - DeploymentStacks: map[string]any{ - "key": "value", + DeploymentStacks: &DeploymentStacksConfig{ + DenySettings: &DenySettingsConfig{Mode: "none"}, }, IgnoreDeploymentState: true, }, @@ -213,8 +206,8 @@ func TestOptions_GetWithDefaults(t *testing.T) { Path: "custom-path", Module: "custom-module", Name: "custom-name", - DeploymentStacks: map[string]any{ - "key": "value", + DeploymentStacks: &DeploymentStacksConfig{ + DenySettings: &DenySettingsConfig{Mode: "none"}, }, IgnoreDeploymentState: true, }, diff --git a/schemas/alpha/azure.yaml.json b/schemas/alpha/azure.yaml.json index 398df6e49d7..0f37ce21046 100644 --- a/schemas/alpha/azure.yaml.json +++ b/schemas/alpha/azure.yaml.json @@ -1682,11 +1682,19 @@ }, "excludedActions": { "type": "array", - "title": "List of role-based management operations that are excluded from the denySettings." + "title": "List of role-based management operations that are excluded from the denySettings.", + "description": "Each entry supports ${VAR} environment variable substitution, resolved at deployment time.", + "items": { + "type": "string" + } }, "excludedPrincipals": { "type": "array", - "title": "List of Entra ID principal IDs excluded from the lock. Up to 5 principals are permitted." + "title": "List of Entra ID principal IDs excluded from the lock. Up to 5 principals are permitted.", + "description": "Each entry supports ${VAR} environment variable substitution, resolved at deployment time.", + "items": { + "type": "string" + } } } } From 4d5d3d11dab5c293f4f95e2b693497dca024ebc8 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Tue, 21 Jul 2026 21:14:21 +0000 Subject: [PATCH 2/5] fix: address Copilot review feedback (iteration 1) 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> --- .../provisioning/bicep/bicep_provider.go | 4 +- .../provisioning/bicep/deployment_stacks.go | 48 ++++++++++--- .../bicep/deployment_stacks_test.go | 67 +++++++++++++++++-- 3 files changed, 101 insertions(+), 18 deletions(-) diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go index 84ecb2590d1..2a73f5fc9e2 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go @@ -898,7 +898,7 @@ func (p *BicepProvider) Deploy(ctx context.Context) (*provisioning.DeployResult, deploymentTags[azure.TagKeyAzdDeploymentStateParamHashName] = new(currentParamsHash) } - optionsMap, err := p.deploymentOptionsMap() + optionsMap, err := p.deploymentOptionsMap(true) if err != nil { return nil, err } @@ -1660,7 +1660,7 @@ func (p *BicepProvider) destroyDeployment( p.console.StopSpinner(ctx, progressMessage.Message, input.StepFailed) } }, func(progress *async.Progress[azapi.DeleteDeploymentProgress]) error { - optionsMap, err := p.deploymentOptionsMap() + optionsMap, err := p.deploymentOptionsMap(false) if err != nil { return err } diff --git a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go index 9ab7cebc17a..08d51c34c05 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go +++ b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go @@ -7,19 +7,38 @@ import ( "fmt" "strings" + "github.com/azure/azure-dev/cli/azd/pkg/alpha" + "github.com/azure/azure-dev/cli/azd/pkg/azapi" "github.com/azure/azure-dev/cli/azd/pkg/convert" "github.com/azure/azure-dev/cli/azd/pkg/osutil" ) +// deploymentStacksEnabled reports whether the Deployment Stacks alpha feature is active. This +// mirrors the deployment-service selection in cmd/container.go: only when the feature is enabled +// is the stacks deployment service used (and the deployment-stacks options actually consumed). +func (p *BicepProvider) deploymentStacksEnabled() bool { + var featureManager *alpha.FeatureManager + if err := p.serviceLocator.Resolve(&featureManager); err != nil { + return false + } + + return featureManager.IsEnabled(azapi.FeatureDeploymentStacks) +} + // resolveDeploymentStacksMap resolves the typed deployment-stacks configuration into a // camelCase map[string]any consumable by the deployment-stacks API layer // (azapi.parseDeploymentStackOptions). It performs ${VAR} environment-variable substitution // on denySettings.excludedPrincipals and denySettings.excludedActions, resolving values from // plan-time layer outputs (VirtualEnv) first and then the azd environment. // +// When includeDenySettings is false the denySettings block is omitted entirely (and therefore +// not resolved). The stack delete APIs only consume actionOnUnmanage and the bypass flag, so the +// destroy path passes false to avoid failing `azd down` when a ${VAR} referenced only by the deny +// lists is no longer available. +// // It returns nil when no deployment-stacks configuration is present, in which case the caller // should omit the DeploymentStacks key entirely so the API layer applies its defaults. -func (p *BicepProvider) resolveDeploymentStacksMap() (map[string]any, error) { +func (p *BicepProvider) resolveDeploymentStacksMap(includeDenySettings bool) (map[string]any, error) { cfg := p.options.DeploymentStacks if cfg == nil { return nil, nil @@ -41,7 +60,7 @@ func (p *BicepProvider) resolveDeploymentStacksMap() (map[string]any, error) { result["actionOnUnmanage"] = actionOnUnmanage } - if cfg.DenySettings != nil { + if includeDenySettings && cfg.DenySettings != nil { denySettings := map[string]any{} if cfg.DenySettings.Mode != "" { denySettings["mode"] = cfg.DenySettings.Mode @@ -115,23 +134,32 @@ func (p *BicepProvider) resolveDeploymentStacksValues(values []osutil.Expandable // deploymentOptionsMap builds the generic options map handed to the deployment API layer, // with the deployment-stacks configuration resolved (including ${VAR} substitution). Standard // (non-stack) deployments ignore this map; stack deployments read only the DeploymentStacks key. -func (p *BicepProvider) deploymentOptionsMap() (map[string]any, error) { +// +// Resolution is skipped entirely when the Deployment Stacks alpha feature is inactive, so an +// otherwise-valid standard provision can't be failed by an unavailable ${VAR} in an inactive +// deploymentStacks block. includeDenySettings is false on the destroy path, where the deny lists +// are not consumed by the stack delete APIs. +func (p *BicepProvider) deploymentOptionsMap(includeDenySettings bool) (map[string]any, error) { optionsMap, err := convert.ToMap(p.options) if err != nil { return nil, err } - stacks, err := p.resolveDeploymentStacksMap() + // The typed DeploymentStacksConfig is not JSON-serializable in a form the API layer can + // consume (ExpandableString has no exported fields), so always drop whatever convert.ToMap + // produced for it and only re-add a resolved map when stacks are actually in play. + delete(optionsMap, "DeploymentStacks") + + if !p.deploymentStacksEnabled() { + return optionsMap, nil + } + + stacks, err := p.resolveDeploymentStacksMap(includeDenySettings) if err != nil { return nil, err } - if stacks == nil { - // The typed DeploymentStacksConfig is not JSON-serializable in a form the API layer can - // consume (ExpandableString has no exported fields), so drop whatever convertOptionsToMap - // produced for it and let the API layer apply its defaults. - delete(optionsMap, "DeploymentStacks") - } else { + if stacks != nil { optionsMap["DeploymentStacks"] = stacks } diff --git a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go index e1c678633fe..03562d94302 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go +++ b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go @@ -30,18 +30,25 @@ func minimalArmTemplate() azure.ArmTemplate { } } +// enableDeploymentStacks turns on the deployment.stacks alpha feature for the duration of the test +// so deploymentOptionsMap resolves the stacks configuration. +func enableDeploymentStacks(t *testing.T) { + t.Setenv("AZD_ALPHA_ENABLE_DEPLOYMENT_STACKS", "true") +} + func TestResolveDeploymentStacksMap_Nil(t *testing.T) { mockContext := mocks.NewMockContext(t.Context()) provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), nil) provider.options.DeploymentStacks = nil - stacks, err := provider.resolveDeploymentStacksMap() + stacks, err := provider.resolveDeploymentStacksMap(true) require.NoError(t, err) require.Nil(t, stacks) // deploymentOptionsMap must omit the DeploymentStacks key entirely so the API layer // applies its own defaults. - optionsMap, err := provider.deploymentOptionsMap() + enableDeploymentStacks(t) + optionsMap, err := provider.deploymentOptionsMap(true) require.NoError(t, err) require.NotContains(t, optionsMap, "DeploymentStacks") } @@ -70,7 +77,7 @@ func TestResolveDeploymentStacksMap_ResolvesEnvSubstitution(t *testing.T) { }, } - stacks, err := provider.resolveDeploymentStacksMap() + stacks, err := provider.resolveDeploymentStacksMap(true) require.NoError(t, err) actionOnUnmanage, ok := stacks["actionOnUnmanage"].(map[string]any) @@ -100,11 +107,36 @@ func TestResolveDeploymentStacksMap_UnsetVarErrors(t *testing.T) { }, } - _, err := provider.resolveDeploymentStacksMap() + _, err := provider.resolveDeploymentStacksMap(true) require.Error(t, err) require.Contains(t, err.Error(), "DOES_NOT_EXIST") } +// TestResolveDeploymentStacksMap_OmitsDenySettingsOnDestroy verifies that the destroy path +// (includeDenySettings=false) never resolves the deny lists, so an unavailable ${VAR} referenced +// only by denySettings can't fail `azd down`. The stack delete APIs consume only actionOnUnmanage. +func TestResolveDeploymentStacksMap_OmitsDenySettingsOnDestroy(t *testing.T) { + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), nil) + + provider.options.DeploymentStacks = &provisioning.DeploymentStacksConfig{ + ActionOnUnmanage: &provisioning.ActionOnUnmanageConfig{ + Resources: "delete", + ResourceGroups: "delete", + }, + DenySettings: &provisioning.DenySettingsConfig{ + Mode: "denyDelete", + // This variable is intentionally unset; it must not be resolved on the destroy path. + ExcludedPrincipals: expandableStrings("${DOES_NOT_EXIST}"), + }, + } + + stacks, err := provider.resolveDeploymentStacksMap(false) + require.NoError(t, err) + require.Contains(t, stacks, "actionOnUnmanage") + require.NotContains(t, stacks, "denySettings") +} + func TestResolveDeploymentStacksMap_EnvFallback(t *testing.T) { mockContext := mocks.NewMockContext(t.Context()) provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), map[string]string{ @@ -118,7 +150,7 @@ func TestResolveDeploymentStacksMap_EnvFallback(t *testing.T) { }, } - stacks, err := provider.resolveDeploymentStacksMap() + stacks, err := provider.resolveDeploymentStacksMap(true) require.NoError(t, err) denySettings := stacks["denySettings"].(map[string]any) @@ -126,6 +158,8 @@ func TestResolveDeploymentStacksMap_EnvFallback(t *testing.T) { } func TestDeploymentOptionsMap_IncludesResolvedStacks(t *testing.T) { + enableDeploymentStacks(t) + mockContext := mocks.NewMockContext(t.Context()) provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), map[string]string{ "OPERATOR_OBJECT_ID": "44444444-4444-4444-4444-444444444444", @@ -138,7 +172,7 @@ func TestDeploymentOptionsMap_IncludesResolvedStacks(t *testing.T) { }, } - optionsMap, err := provider.deploymentOptionsMap() + optionsMap, err := provider.deploymentOptionsMap(true) require.NoError(t, err) stacks, ok := optionsMap["DeploymentStacks"].(map[string]any) @@ -146,3 +180,24 @@ func TestDeploymentOptionsMap_IncludesResolvedStacks(t *testing.T) { denySettings := stacks["denySettings"].(map[string]any) require.Equal(t, []string{"44444444-4444-4444-4444-444444444444"}, denySettings["excludedPrincipals"]) } + +// TestDeploymentOptionsMap_SkipsResolutionWhenStacksDisabled verifies that when the deployment +// stacks alpha feature is inactive, the DeploymentStacks key is omitted and no ${VAR} resolution +// happens, so an unavailable variable in an inactive deploymentStacks block can't fail an +// otherwise-valid standard provision. +func TestDeploymentOptionsMap_SkipsResolutionWhenStacksDisabled(t *testing.T) { + // Deployment stacks alpha feature is NOT enabled here. + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), nil) + + provider.options.DeploymentStacks = &provisioning.DeploymentStacksConfig{ + DenySettings: &provisioning.DenySettingsConfig{ + Mode: "denyDelete", + ExcludedPrincipals: expandableStrings("${DOES_NOT_EXIST}"), + }, + } + + optionsMap, err := provider.deploymentOptionsMap(true) + require.NoError(t, err) + require.NotContains(t, optionsMap, "DeploymentStacks") +} From ed0f030645551cb8498503d3c71972988b265879 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Tue, 21 Jul 2026 21:23:54 +0000 Subject: [PATCH 3/5] fix: address Copilot review feedback (iteration 2) 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> --- .../provisioning/bicep/bicep_provider.go | 8 ++++- .../provisioning/bicep/deployment_stacks.go | 10 ++++++ .../bicep/deployment_stacks_test.go | 34 +++++++++++++++++++ 3 files changed, 51 insertions(+), 1 deletion(-) diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go index 2a73f5fc9e2..ba9220cf1e7 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go @@ -833,7 +833,13 @@ func (p *BicepProvider) Deploy(ctx context.Context) (*provisioning.DeployResult, logDS("%s", parametersHashErr.Error()) } - if !p.ignoreDeploymentState && parametersHashErr == nil { + useDeploymentState := !p.ignoreDeploymentState && parametersHashErr == nil + if useDeploymentState && p.hasActiveDeploymentStacksConfig() { + logDS("deployment stacks configuration present; bypassing deployment-state shortcut") + useDeploymentState = false + } + + if useDeploymentState { deploymentState, stateErr := p.deploymentState(ctx, planned, deployment, currentParamsHash) if stateErr == nil { // As a heuristic, we also check the existence of all resource groups diff --git a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go index 08d51c34c05..f83c72a6015 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go +++ b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go @@ -25,6 +25,16 @@ func (p *BicepProvider) deploymentStacksEnabled() bool { return featureManager.IsEnabled(azapi.FeatureDeploymentStacks) } +// hasActiveDeploymentStacksConfig reports whether an effective deployment-stacks configuration is +// present (config supplied AND the alpha feature enabled). When true, the deployment-state shortcut +// must be bypassed: the stacks deny/unmanage settings — including ${VAR}-resolved deny lists — can +// change independently of the ARM template and parameters that the state hash covers. Skipping the +// 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() +} + // resolveDeploymentStacksMap resolves the typed deployment-stacks configuration into a // camelCase map[string]any consumable by the deployment-stacks API layer // (azapi.parseDeploymentStackOptions). It performs ${VAR} environment-variable substitution diff --git a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go index 03562d94302..e3d683bb9f8 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go +++ b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go @@ -201,3 +201,37 @@ func TestDeploymentOptionsMap_SkipsResolutionWhenStacksDisabled(t *testing.T) { require.NoError(t, err) require.NotContains(t, optionsMap, "DeploymentStacks") } + +// TestHasActiveDeploymentStacksConfig guards the deployment-state bypass: when an effective +// deployment-stacks configuration is present, Deploy must skip the state shortcut so a changed +// ${VAR}-resolved deny principal/action is re-applied (rather than silently ignored because the +// ARM template and parameters are unchanged) and the ${VAR} validation always runs. +func TestHasActiveDeploymentStacksConfig(t *testing.T) { + t.Run("no config", func(t *testing.T) { + enableDeploymentStacks(t) + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), nil) + provider.options.DeploymentStacks = nil + require.False(t, provider.hasActiveDeploymentStacksConfig()) + }) + + t.Run("config present but feature disabled", func(t *testing.T) { + // deployment.stacks alpha feature is NOT enabled. + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), nil) + provider.options.DeploymentStacks = &provisioning.DeploymentStacksConfig{ + DenySettings: &provisioning.DenySettingsConfig{Mode: "denyDelete"}, + } + require.False(t, provider.hasActiveDeploymentStacksConfig()) + }) + + t.Run("config present and feature enabled", func(t *testing.T) { + enableDeploymentStacks(t) + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), nil) + provider.options.DeploymentStacks = &provisioning.DeploymentStacksConfig{ + DenySettings: &provisioning.DenySettingsConfig{Mode: "denyDelete"}, + } + require.True(t, provider.hasActiveDeploymentStacksConfig()) + }) +} From b6cd306529c64013b832829593a79dfea7eddd88 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Tue, 21 Jul 2026 21:36:32 +0000 Subject: [PATCH 4/5] fix: address Copilot review feedback (iteration 3) - 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> --- .../provisioning/bicep/deployment_stacks.go | 24 ++++++++++------ .../bicep/deployment_stacks_test.go | 28 +++++++++++++++++-- .../deployment_stacks_config_test.go | 2 +- .../pkg/infra/provisioning/provider_test.go | 16 ++++++++--- 4 files changed, 54 insertions(+), 16 deletions(-) diff --git a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go index f83c72a6015..701382e0866 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go +++ b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go @@ -102,9 +102,11 @@ func (p *BicepProvider) resolveDeploymentStacksMap(includeDenySettings bool) (ma } // resolveDeploymentStacksValues evaluates ${VAR} references in each value, resolving from the -// plan-time layer outputs (VirtualEnv) first and then the azd environment. It returns an error -// if any referenced environment variable is unset, since a blank value in deny settings (for -// example, an empty excluded principal) is almost always a misconfiguration. +// plan-time layer outputs (VirtualEnv) first and then the azd environment. A value that resolves +// to an empty string is treated as a misconfiguration (a blank deny-list entry, for example an +// empty excluded principal, is almost always a mistake) and returns an error. Shell-style default +// expressions such as ${VAR:-fallback} are honored: Envsubst applies the default before this check, +// so a reference with a usable default yields a non-blank value and is accepted. func (p *BicepProvider) resolveDeploymentStacksValues(values []osutil.ExpandableString) ([]string, error) { if len(values) == 0 { return nil, nil @@ -112,7 +114,7 @@ func (p *BicepProvider) resolveDeploymentStacksValues(values []osutil.Expandable resolved := make([]string, 0, len(values)) for _, value := range values { - var unset []string + var lookedUpEmpty []string substituted, err := value.Envsubst(func(name string) string { if p.options.VirtualEnv != nil { if v, has := p.options.VirtualEnv[name]; has { @@ -121,8 +123,8 @@ func (p *BicepProvider) resolveDeploymentStacksValues(values []osutil.Expandable } v, ok := p.env.LookupEnv(name) - if !ok { - unset = append(unset, name) + if !ok || v == "" { + lookedUpEmpty = append(lookedUpEmpty, name) } return v }) @@ -130,9 +132,13 @@ func (p *BicepProvider) resolveDeploymentStacksValues(values []osutil.Expandable return nil, fmt.Errorf("resolving deploymentStacks value: %w", err) } - if len(unset) > 0 { - return nil, fmt.Errorf( - "deploymentStacks references unset environment variable(s): %s", strings.Join(unset, ", ")) + if strings.TrimSpace(substituted) == "" { + if len(lookedUpEmpty) > 0 { + return nil, fmt.Errorf( + "deploymentStacks references unset environment variable(s): %s", + strings.Join(lookedUpEmpty, ", ")) + } + return nil, fmt.Errorf("deploymentStacks value resolved to an empty string") } resolved = append(resolved, substituted) diff --git a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go index e3d683bb9f8..1a52b8bee14 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go +++ b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go @@ -153,7 +153,8 @@ func TestResolveDeploymentStacksMap_EnvFallback(t *testing.T) { stacks, err := provider.resolveDeploymentStacksMap(true) require.NoError(t, err) - denySettings := stacks["denySettings"].(map[string]any) + denySettings, ok := stacks["denySettings"].(map[string]any) + require.True(t, ok) require.Equal(t, []string{"33333333-3333-3333-3333-333333333333"}, denySettings["excludedPrincipals"]) } @@ -177,7 +178,8 @@ func TestDeploymentOptionsMap_IncludesResolvedStacks(t *testing.T) { stacks, ok := optionsMap["DeploymentStacks"].(map[string]any) require.True(t, ok) - denySettings := stacks["denySettings"].(map[string]any) + denySettings, ok := stacks["denySettings"].(map[string]any) + require.True(t, ok) require.Equal(t, []string{"44444444-4444-4444-4444-444444444444"}, denySettings["excludedPrincipals"]) } @@ -235,3 +237,25 @@ func TestHasActiveDeploymentStacksConfig(t *testing.T) { require.True(t, provider.hasActiveDeploymentStacksConfig()) }) } + +// TestResolveDeploymentStacksMap_DefaultExpression verifies that shell-style default expressions +// (${VAR:-fallback}) are honored: an unset variable with a usable default resolves to the default +// and is accepted, rather than being rejected as an unset reference. +func TestResolveDeploymentStacksMap_DefaultExpression(t *testing.T) { + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), nil) + + provider.options.DeploymentStacks = &provisioning.DeploymentStacksConfig{ + DenySettings: &provisioning.DenySettingsConfig{ + Mode: "denyDelete", + ExcludedPrincipals: expandableStrings("${UNSET_PRINCIPAL:-55555555-5555-5555-5555-555555555555}"), + }, + } + + stacks, err := provider.resolveDeploymentStacksMap(true) + require.NoError(t, err) + + denySettings, ok := stacks["denySettings"].(map[string]any) + require.True(t, ok) + require.Equal(t, []string{"55555555-5555-5555-5555-555555555555"}, denySettings["excludedPrincipals"]) +} diff --git a/cli/azd/pkg/infra/provisioning/deployment_stacks_config_test.go b/cli/azd/pkg/infra/provisioning/deployment_stacks_config_test.go index 14358efc6b3..a06b5e2e0f3 100644 --- a/cli/azd/pkg/infra/provisioning/deployment_stacks_config_test.go +++ b/cli/azd/pkg/infra/provisioning/deployment_stacks_config_test.go @@ -6,8 +6,8 @@ package provisioning import ( "testing" + "github.com/braydonk/yaml" "github.com/stretchr/testify/require" - "gopkg.in/yaml.v3" ) func TestDeploymentStacksConfigUnmarshalYAML(t *testing.T) { diff --git a/cli/azd/pkg/infra/provisioning/provider_test.go b/cli/azd/pkg/infra/provisioning/provider_test.go index a294323ffba..eab0e64b849 100644 --- a/cli/azd/pkg/infra/provisioning/provider_test.go +++ b/cli/azd/pkg/infra/provisioning/provider_test.go @@ -123,16 +123,24 @@ func TestOptions_GetWithDefaults(t *testing.T) { baseOptions: Options{ Provider: Bicep, DeploymentStacks: &DeploymentStacksConfig{ - DenySettings: &DenySettingsConfig{Mode: "denyDelete"}, + ActionOnUnmanage: &ActionOnUnmanageConfig{Resources: "delete"}, + DenySettings: &DenySettingsConfig{Mode: "denyDelete"}, + }, + }, + otherOptions: []Options{ + { + Module: "custom-module", + Name: "custom-name", }, }, - otherOptions: nil, expectedResult: Options{ Provider: Bicep, - Module: "main", + Module: "custom-module", Path: "infra", + Name: "custom-name", DeploymentStacks: &DeploymentStacksConfig{ - DenySettings: &DenySettingsConfig{Mode: "denyDelete"}, + ActionOnUnmanage: &ActionOnUnmanageConfig{Resources: "delete"}, + DenySettings: &DenySettingsConfig{Mode: "denyDelete"}, }, }, expectError: false, From 2c6548a533e43009ed133f21c472a346d90f4326 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Tue, 21 Jul 2026 23:36:22 +0000 Subject: [PATCH 5/5] test: raise coverage for deployment stacks changes 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 --- .../external_provisioning_provider_test.go | 30 +++++++ .../provisioning/bicep/bicep_provider.go | 7 +- .../provisioning/bicep/deployment_stacks.go | 15 ++++ .../bicep/deployment_stacks_test.go | 84 +++++++++++++++++++ 4 files changed, 133 insertions(+), 3 deletions(-) diff --git a/cli/azd/internal/grpcserver/external_provisioning_provider_test.go b/cli/azd/internal/grpcserver/external_provisioning_provider_test.go index 634400feaa2..af36602ef24 100644 --- a/cli/azd/internal/grpcserver/external_provisioning_provider_test.go +++ b/cli/azd/internal/grpcserver/external_provisioning_provider_test.go @@ -20,6 +20,7 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/azdext" "github.com/azure/azure-dev/cli/azd/pkg/infra/provisioning" "github.com/azure/azure-dev/cli/azd/pkg/prompt" + "github.com/azure/azure-dev/cli/azd/test/mocks/mockinput" ) func Test_convertToProtoOptions(t *testing.T) { @@ -121,6 +122,35 @@ func Test_convertToProtoOptions(t *testing.T) { } } +// Test_ExternalProvisioningProvider_NameAndProgress covers the broker-independent surface of the +// provider: the DI factory, Name(), and the reportProgress/stopProgress spinner helpers (including +// their nil-console fallbacks). The broker-backed forwarders (Deploy/Preview/Destroy/etc.) require +// a live gRPC broker and are tracked separately in issue #7480. +func Test_ExternalProvisioningProvider_NameAndProgress(t *testing.T) { + factory := NewExternalProvisioningProviderFactory("my-provider", nil, nil) + + t.Run("Name returns provider name", func(t *testing.T) { + provider := factory(mockinput.NewMockConsole()) + assert.Equal(t, "my-provider", provider.Name()) + }) + + 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") + provider.stopProgress(context.Background(), nil) + provider.stopProgress(context.Background(), assert.AnError) + }) + + t.Run("progress with nil console", func(t *testing.T) { + provider := &ExternalProvisioningProvider{providerName: "no-console"} + // Falls back to a debug log / no-op without a console. + provider.reportProgress(context.Background(), "deploying") + provider.stopProgress(context.Background(), nil) + assert.Equal(t, "no-console", provider.Name()) + }) +} + func Test_convertFromProtoStateResult(t *testing.T) { tests := []struct { name string diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go index ba9220cf1e7..d5de25aef49 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go @@ -833,10 +833,11 @@ func (p *BicepProvider) Deploy(ctx context.Context) (*provisioning.DeployResult, logDS("%s", parametersHashErr.Error()) } - useDeploymentState := !p.ignoreDeploymentState && parametersHashErr == nil - if useDeploymentState && p.hasActiveDeploymentStacksConfig() { + useDeploymentState := p.useDeploymentStateShortcut(parametersHashErr) + if !useDeploymentState && !p.ignoreDeploymentState && parametersHashErr == nil { + // The only remaining reason the shortcut is disabled here is an active deployment-stacks + // configuration; surface it in the deployment-stacks debug log. logDS("deployment stacks configuration present; bypassing deployment-state shortcut") - useDeploymentState = false } if useDeploymentState { diff --git a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go index 701382e0866..3def63a28c7 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go +++ b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks.go @@ -35,6 +35,21 @@ func (p *BicepProvider) hasActiveDeploymentStacksConfig() bool { return p.options.DeploymentStacks != nil && p.deploymentStacksEnabled() } +// useDeploymentStateShortcut reports whether the deployment-state shortcut (which skips a +// redeploy when the template and parameters are unchanged) may be used. It returns false when +// deployment-state tracking is disabled (--no-state), when the parameters hash could not be +// computed, or when an active deployment-stacks configuration is present. In the stacks case the +// deny/unmanage settings — including ${VAR}-resolved deny lists — can change independently of the +// template+parameters the state hash covers, so the shortcut must be bypassed to avoid leaving +// stale settings on the stack and to preserve ${VAR} resolution/validation. +func (p *BicepProvider) useDeploymentStateShortcut(parametersHashErr error) bool { + if p.ignoreDeploymentState || parametersHashErr != nil { + return false + } + + return !p.hasActiveDeploymentStacksConfig() +} + // resolveDeploymentStacksMap resolves the typed deployment-stacks configuration into a // camelCase map[string]any consumable by the deployment-stacks API layer // (azapi.parseDeploymentStackOptions). It performs ${VAR} environment-variable substitution diff --git a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go index 1a52b8bee14..f524927682f 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go +++ b/cli/azd/pkg/infra/provisioning/bicep/deployment_stacks_test.go @@ -4,6 +4,7 @@ package bicep import ( + "errors" "testing" "github.com/azure/azure-dev/cli/azd/pkg/azure" @@ -259,3 +260,86 @@ func TestResolveDeploymentStacksMap_DefaultExpression(t *testing.T) { require.True(t, ok) require.Equal(t, []string{"55555555-5555-5555-5555-555555555555"}, denySettings["excludedPrincipals"]) } + +// TestUseDeploymentStateShortcut exercises every branch of the shortcut decision: it is disabled +// when deployment-state tracking is off, when the parameters hash failed, and when an active +// deployment-stacks configuration is present; otherwise it is enabled. +func TestUseDeploymentStateShortcut(t *testing.T) { + t.Run("enabled by default", func(t *testing.T) { + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), nil) + require.True(t, provider.useDeploymentStateShortcut(nil)) + }) + + t.Run("disabled when deployment state ignored", func(t *testing.T) { + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), nil) + provider.ignoreDeploymentState = true + require.False(t, provider.useDeploymentStateShortcut(nil)) + }) + + t.Run("disabled when parameters hash failed", func(t *testing.T) { + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), nil) + require.False(t, provider.useDeploymentStateShortcut(errors.New("hash failed"))) + }) + + t.Run("disabled when active deployment stacks config present", func(t *testing.T) { + enableDeploymentStacks(t) + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), nil) + provider.options.DeploymentStacks = &provisioning.DeploymentStacksConfig{ + DenySettings: &provisioning.DenySettingsConfig{Mode: "denyDelete"}, + } + require.False(t, provider.useDeploymentStateShortcut(nil)) + }) + + t.Run("enabled when stacks config present but feature disabled", func(t *testing.T) { + // deployment.stacks alpha feature is NOT enabled, so the config is inert. + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), nil) + provider.options.DeploymentStacks = &provisioning.DeploymentStacksConfig{ + DenySettings: &provisioning.DenySettingsConfig{Mode: "denyDelete"}, + } + require.True(t, provider.useDeploymentStateShortcut(nil)) + }) +} + +// TestResolveDeploymentStacksMap_ActionOnUnmanageManagementGroups covers the management-groups +// branch of actionOnUnmanage and confirms an empty ActionOnUnmanage still produces the (empty) map. +func TestResolveDeploymentStacksMap_ActionOnUnmanageManagementGroups(t *testing.T) { + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), nil) + + provider.options.DeploymentStacks = &provisioning.DeploymentStacksConfig{ + ActionOnUnmanage: &provisioning.ActionOnUnmanageConfig{ + ManagementGroups: "detach", + }, + } + + stacks, err := provider.resolveDeploymentStacksMap(true) + require.NoError(t, err) + + actionOnUnmanage, ok := stacks["actionOnUnmanage"].(map[string]any) + require.True(t, ok) + require.Equal(t, "detach", actionOnUnmanage["managementGroups"]) + require.NotContains(t, stacks, "denySettings") +} + +// TestResolveDeploymentStacksValues_BlankLiteralErrors verifies that a literal blank deny-list +// entry (no ${VAR} reference at all) is rejected as a misconfiguration. +func TestResolveDeploymentStacksValues_BlankLiteralErrors(t *testing.T) { + mockContext := mocks.NewMockContext(t.Context()) + provider := createBicepProviderWithEnv(t, mockContext, minimalArmTemplate(), nil) + + provider.options.DeploymentStacks = &provisioning.DeploymentStacksConfig{ + DenySettings: &provisioning.DenySettingsConfig{ + Mode: "denyDelete", + ExcludedActions: expandableStrings(" "), + }, + } + + _, err := provider.resolveDeploymentStacksMap(true) + require.Error(t, err) + require.Contains(t, err.Error(), "empty string") +}