diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs.go index c4920873f7a..72981a317e6 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs.go @@ -4,230 +4,31 @@ package cmd import ( - "fmt" - "regexp" - "strings" - - "github.com/azure/azure-dev/cli/azd/pkg/foundry" + "azureaiagent/internal/synthesis" ) -// Escape handling must match the expander that owns each field. -// Fields resolved by foundry.ExpandEnv take -// honorEnvironmentEscaping: it collapses '$' pairs, so $${VAR} -// stays literal, and it reserves ${{...}} spans for Foundry. -// The three project network fields (network.agentSubnet.vnet, -// network.peSubnet.vnet, network.dns.subscription) take -// ignoreEnvironmentEscaping because resolveVars in the projects -// synthesizer is a plain regex replace with no '$$' handling, -// so $${VAR} does expand there. The split mirrors that existing -// divergence rather than choosing two policies; it collapses -// once resolveVars moves to foundry.ExpandEnv. +// environmentReference and findEnvironmentReferences are this package's view of +// the single azd ${VAR} scanner. See [synthesis.FindEnvReferences] for the +// discovery rules; the policy layered on top lives in the callers +// (collectAzureYamlEnvironmentReferences for init prompting, +// collectStringEnvironmentTemplates for the generated service env block). // -// resolveVars also diverges on ':-'. Its pattern matches only -// ${NAME}, so a ${NAME:-default} on one of those three fields is -// never substituted and no error is raised; the literal then -// fails the field's own ARM id or subscription validation. No -// escaping flag can mirror that, so the scanner still reports -// the name and the gap is tracked upstream. -// See: https://github.com/Azure/azure-dev/issues/9350 -const ( - honorEnvironmentEscaping = true - ignoreEnvironmentEscaping = false -) - -// environmentReferencePrefix parses only the reference prefix. -// Balanced defaults remain the scanner's responsibility. -var environmentReferencePrefix = regexp.MustCompile( - `^\$\{([A-Za-z_][A-Za-z0-9_]*)(\}|:-)`, -) - -// environmentReference is one azd ${VAR} occurrence in a string. -// Start and End bound the whole reference, including any :- -// default, so a caller can resume scanning at End. -type environmentReference struct { - Name string - Start int - End int - HasDefault bool -} - -// findEnvironmentReferences returns the azd ${VAR} references in -// value, in order of appearance. It is the single scanner for the -// package: callers layer their own policy on the result rather -// than reimplementing discovery. init prompting skips references -// with a default because the expander supplies the fallback, -// while the generated service env block records them so the -// owning extension can re-apply the default. +// The implementation lives in internal/synthesis because that package is the +// other consumer — resolveVars derives its unresolved-variable guard from the +// same scan — and its two byte-identical copies (this extension and +// azure.ai.projects) can only share code through an import path both spell +// identically. pkg/foundry, next to ExpandEnv, is the natural home, but both +// extensions consume azd core at a pinned release, so moving it there needs a +// core release plus a go.mod bump in both modules. Tracked by +// https://github.com/Azure/azure-dev/issues/9427. // -// References the expander would not resolve are dropped: escaped -// ones and any reserved by a Foundry ${{...}} span. honorEscaping -// must match the expander that owns the field. -// -// A reference inside a :- default is not reported: nested azd -// references are unsupported by design, so ${OUTER:-${NESTED}} -// yields OUTER only. foundry.ExpandEnv still resolves NESTED at -// deploy, but nothing discovers it, so init never prompts for it -// and it gets no entry in the generated service env block. It -// then resolves only where the consumer keeps an azd environment -// fallback, and to empty where a declared env: drops it. Keep -// defaults literal. -func findEnvironmentReferences(value string, honorEscaping bool) []environmentReference { - candidates := environmentReferenceCandidates(value, honorEscaping) - if !honorEscaping || len(candidates) == 0 { - return candidates - } - - protected := protectedEnvironmentReferences(value, candidates) - references := make([]environmentReference, 0, len(candidates)) - for i, candidate := range candidates { - if protected[i] { - continue - } - references = append(references, candidate) - } - if len(references) == 0 { - return nil - } - return references -} - -// environmentReferenceCandidates scans value left to right for -// ${NAME} and ${NAME:-default} occurrences. drone/envsubst, which -// backs foundry.ExpandEnv, collapses a '$' pair into a literal -// '$' and keeps reading, so an escape only neutralizes the '${' -// it precedes: the text after it, including a default, still -// holds live references. Membership of a ${{...}} span is left to -// findEnvironmentReferences. Scanning resumes at the end of a -// match, so a default span is never scanned again; that is what -// keeps nested references out. -func environmentReferenceCandidates(value string, honorEscaping bool) []environmentReference { - var references []environmentReference - for index := 0; index < len(value); { - if value[index] != '$' { - index++ - continue - } - if honorEscaping && strings.HasPrefix(value[index:], "$$") { - index += 2 - continue - } - - reference, found := environmentReferenceAt(value, index) - if !found { - index++ - continue - } - - references = append(references, reference) - index = reference.End - } - return references -} - -// environmentReferenceAt parses the reference opening at start. -// The anchored prefix keeps a bare '$' from being read as one. -// Balanced defaults still need the stateful end scanner below. -func environmentReferenceAt(value string, start int) (environmentReference, bool) { - if start < 0 || start >= len(value) { - return environmentReference{}, false - } - - match := environmentReferencePrefix.FindStringSubmatch(value[start:]) - if match == nil { - return environmentReference{}, false - } - - name := match[1] - prefixEnd := start + len(match[0]) - if match[2] == "}" { - return environmentReference{ - Name: name, - Start: start, - End: prefixEnd, - }, true - } - - end, found := environmentReferenceEnd(value, prefixEnd) - if !found { - return environmentReference{}, false - } - return environmentReference{ - Name: name, - Start: start, - End: end, - HasDefault: true, - }, true -} - -// environmentReferenceEnd finds the '}' closing a :- default. It -// counts nested ${...} and steps over Foundry ${{...}} spans, -// which are legal default values, so the reported span covers the -// whole reference. -func environmentReferenceEnd(value string, index int) (int, bool) { - depth := 1 - for index < len(value) { - if strings.HasPrefix(value[index:], "${{") { - end := strings.Index(value[index+3:], "}}") - if end < 0 { - return 0, false - } - index += end + 5 - continue - } - if strings.HasPrefix(value[index:], "${") { - depth++ - index += 2 - continue - } - if value[index] == '}' { - depth-- - index++ - if depth == 0 { - return index, true - } - continue - } - index++ - } - return 0, false -} - -// protectedEnvironmentReferences reports which candidates sit -// inside a server-side ${{...}} span. Each candidate is replaced -// with a unique probe before running [foundry.ExpandEnv]; probes -// left verbatim are reserved by the shared expander. This keeps -// discovery linked to the owning implementation without ambiguous -// name-based occurrence counting. -func protectedEnvironmentReferences(value string, references []environmentReference) []bool { - protected := make([]bool, len(references)) - if len(references) == 0 { - return protected - } - - probePrefix := "AZD_ENV_REFERENCE_PROBE_" - for strings.Contains(value, probePrefix) { - probePrefix += "_" - } - - probeRefs := make([]string, len(references)) - var probed strings.Builder - last := 0 - for i, reference := range references { - probed.WriteString(value[last:reference.Start]) - probeRefs[i] = fmt.Sprintf("${%s%d}", probePrefix, i) - probed.WriteString(probeRefs[i]) - last = reference.End - } - probed.WriteString(value[last:]) - - expanded, err := foundry.ExpandEnv(probed.String(), func(name string) string { - return "expanded_" + name - }) - if err != nil { - return protected - } - for i, probeRef := range probeRefs { - protected[i] = strings.Contains(expanded, probeRef) - } - return protected +// Escape handling is no longer a per-field choice. Every Foundry field, +// including the three project network values (network.agentSubnet.vnet, +// network.peSubnet.vnet, network.dns.subscription), resolves through +// foundry.ExpandEnv, which collapses '$' pairs so $${VAR} stays literal and +// reserves ${{...}} spans for Foundry. +type environmentReference = synthesis.EnvReference + +func findEnvironmentReferences(value string) []environmentReference { + return synthesis.FindEnvReferences(value) } diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs_test.go index 99f2fded480..81985020070 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/env_refs_test.go @@ -18,7 +18,7 @@ import ( func TestFindEnvironmentReferencesEscapedOuterKeepsInnerLive(t *testing.T) { t.Parallel() - got := findEnvironmentReferences("$${A:-${B}}", honorEnvironmentEscaping) + got := findEnvironmentReferences("$${A:-${B}}") require.Equal(t, []environmentReference{{Name: "B", Start: 6, End: 10}}, got) } @@ -26,29 +26,25 @@ func TestFindEnvironmentReferences(t *testing.T) { t.Parallel() tests := []struct { - name string - value string - honorEscaping bool - want []environmentReference + name string + value string + want []environmentReference }{ { - name: "bare reference", - value: "${PLAIN}", - honorEscaping: honorEnvironmentEscaping, - want: []environmentReference{{Name: "PLAIN", Start: 0, End: 8}}, + name: "bare reference", + value: "${PLAIN}", + want: []environmentReference{{Name: "PLAIN", Start: 0, End: 8}}, }, { - name: "reference with default", - value: "${NAME:-fallback}", - honorEscaping: honorEnvironmentEscaping, + name: "reference with default", + value: "${NAME:-fallback}", want: []environmentReference{ {Name: "NAME", Start: 0, End: 17, HasDefault: true}, }, }, { - name: "multiple references keep order", - value: "prefix ${ONE} mid ${TWO:-x} suffix", - honorEscaping: honorEnvironmentEscaping, + name: "multiple references keep order", + value: "prefix ${ONE} mid ${TWO:-x} suffix", want: []environmentReference{ {Name: "ONE", Start: 7, End: 13}, {Name: "TWO", Start: 18, End: 27, HasDefault: true}, @@ -57,55 +53,35 @@ func TestFindEnvironmentReferences(t *testing.T) { { // drone/envsubst collapses '$' pairs, so one leading '$' // escapes the reference. - name: "single escape is dropped", - value: "$${VAR}", - honorEscaping: honorEnvironmentEscaping, - want: nil, + name: "single escape is dropped", + value: "$${VAR}", + want: nil, }, { // Two leading '$' collapse to a literal '$' and the // reference still expands. - name: "double escape still expands", - value: "$$${VAR}", - honorEscaping: honorEnvironmentEscaping, - want: []environmentReference{{Name: "VAR", Start: 2, End: 8}}, + name: "double escape still expands", + value: "$$${VAR}", + want: []environmentReference{{Name: "VAR", Start: 2, End: 8}}, }, { - name: "triple escape is dropped", - value: "$$$${VAR}", - honorEscaping: honorEnvironmentEscaping, - want: nil, + name: "triple escape is dropped", + value: "$$$${VAR}", + want: nil, }, { - name: "escapes ignored when the owner does not honor them", - value: "$${VAR}", - honorEscaping: ignoreEnvironmentEscaping, - want: []environmentReference{{Name: "VAR", Start: 1, End: 7}}, + name: "foundry expression yields nothing", + value: "${{connections.store.credentials.key}}", + want: nil, }, { - name: "foundry expression yields nothing", - value: "${{connections.store.credentials.key}}", - honorEscaping: honorEnvironmentEscaping, - want: nil, + name: "reference inside a foundry expression is reserved", + value: "${{ tools.${INNER} }}", + want: nil, }, { - name: "reference inside a foundry expression is reserved", - value: "${{ tools.${INNER} }}", - honorEscaping: honorEnvironmentEscaping, - want: nil, - }, - { - // Protection rides on the same switch as escaping so the - // scan matches whichever expander owns the field. - name: "foundry expression unprotected when escaping ignored", - value: "${{ tools.${INNER} }}", - honorEscaping: ignoreEnvironmentEscaping, - want: []environmentReference{{Name: "INNER", Start: 10, End: 18}}, - }, - { - name: "foundry expression as a default value", - value: "${MISSING:-${{event.body}}}", - honorEscaping: honorEnvironmentEscaping, + name: "foundry expression as a default value", + value: "${MISSING:-${{event.body}}}", want: []environmentReference{ {Name: "MISSING", Start: 0, End: 27, HasDefault: true}, }, @@ -114,9 +90,8 @@ func TestFindEnvironmentReferences(t *testing.T) { // Nested references are unsupported by design. The span // covers the default so scanning resumes after it and // NESTED is never reported. - name: "nested default is spanned, inner name unsupported", - value: "${OUTER:-${NESTED}} ${AFTER}", - honorEscaping: honorEnvironmentEscaping, + name: "nested default is spanned, inner name unsupported", + value: "${OUTER:-${NESTED}} ${AFTER}", want: []environmentReference{ {Name: "OUTER", Start: 0, End: 19, HasDefault: true}, {Name: "AFTER", Start: 20, End: 28}, @@ -126,36 +101,31 @@ func TestFindEnvironmentReferences(t *testing.T) { // A '$' the expander ignores must stay ignored here, // or a literal like "costs $price} today" writes a // phantom rice: ${rice} into the service env block. - name: "bare dollar is not a reference", - value: "$foo}", - honorEscaping: honorEnvironmentEscaping, - want: nil, + name: "bare dollar is not a reference", + value: "$foo}", + want: nil, }, { // The phantom would span the whole string and carry // HasDefault, hiding REAL from init prompting. - name: "bare dollar keeps a later reference live", - value: "$ab:-${REAL}}", - honorEscaping: honorEnvironmentEscaping, - want: []environmentReference{{Name: "REAL", Start: 5, End: 12}}, + name: "bare dollar keeps a later reference live", + value: "$ab:-${REAL}}", + want: []environmentReference{{Name: "REAL", Start: 5, End: 12}}, }, { - name: "invalid name yields nothing", - value: "${1BAD}", - honorEscaping: honorEnvironmentEscaping, - want: nil, + name: "invalid name yields nothing", + value: "${1BAD}", + want: nil, }, { - name: "unterminated reference yields nothing", - value: "${UNCLOSED", - honorEscaping: honorEnvironmentEscaping, - want: nil, + name: "unterminated reference yields nothing", + value: "${UNCLOSED", + want: nil, }, { - name: "plain text yields nothing", - value: "no references here", - honorEscaping: honorEnvironmentEscaping, - want: nil, + name: "plain text yields nothing", + value: "no references here", + want: nil, }, } @@ -163,7 +133,7 @@ func TestFindEnvironmentReferences(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := findEnvironmentReferences(tt.value, tt.honorEscaping) + got := findEnvironmentReferences(tt.value) require.Equal(t, tt.want, got) }) } @@ -186,7 +156,6 @@ func TestFindEnvironmentReferencesPolicies(t *testing.T) { collectAzureYamlEnvironmentReferences( value, false, - honorEnvironmentEscaping, &references, map[string]int{}, ) @@ -223,7 +192,6 @@ func TestNestedDefaultIsNotDiscovered(t *testing.T) { collectAzureYamlEnvironmentReferences( value, false, - honorEnvironmentEscaping, &references, map[string]int{}, ) @@ -238,14 +206,12 @@ func TestCollectAzureYamlEnvironmentReferencesUpgradesSecret(t *testing.T) { collectAzureYamlEnvironmentReferences( "${SHARED}", false, - honorEnvironmentEscaping, &references, indexByName, ) collectAzureYamlEnvironmentReferences( "${SHARED}", true, - honorEnvironmentEscaping, &references, indexByName, ) @@ -298,7 +264,7 @@ func TestFindEnvironmentReferencesMatchesExpander(t *testing.T) { }) require.NoError(t, err) - for _, reference := range findEnvironmentReferences(value, honorEnvironmentEscaping) { + for _, reference := range findEnvironmentReferences(value) { require.Truef( t, lookedUp[reference.Name], diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env.go index 54b87aeb4c5..59f153f8b6f 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env.go @@ -348,7 +348,6 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( variable.Value, false, - honorEnvironmentEscaping, references, indexByName, ) @@ -361,7 +360,6 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Target, false, - honorEnvironmentEscaping, references, indexByName, ) @@ -391,7 +389,6 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Network.AgentSubnet.VNet, false, - ignoreEnvironmentEscaping, references, indexByName, ) @@ -400,7 +397,6 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Network.PESubnet.VNet, false, - ignoreEnvironmentEscaping, references, indexByName, ) @@ -409,7 +405,6 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Network.DNS.Subscription, false, - ignoreEnvironmentEscaping, references, indexByName, ) @@ -438,7 +433,6 @@ func collectAzureYamlServiceEnvironmentReferences( collectAzureYamlEnvironmentReferences( config.Endpoint, false, - honorEnvironmentEscaping, references, indexByName, ) @@ -501,7 +495,6 @@ func collectAzureYamlEnvironmentReferencesFromNode( collectAzureYamlEnvironmentReferences( node.Value, secret, - honorEnvironmentEscaping, references, indexByName, ) @@ -511,11 +504,10 @@ func collectAzureYamlEnvironmentReferencesFromNode( func collectAzureYamlEnvironmentReferences( value string, secret bool, - honorEscaping bool, references *[]azureYamlEnvironmentReference, indexByName map[string]int, ) { - for _, reference := range findEnvironmentReferences(value, honorEscaping) { + for _, reference := range findEnvironmentReferences(value) { // A ${VAR:-default} needs no environment value because the // runtime expander supplies the fallback. if reference.HasDefault { diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env_test.go index 41018fcb2df..a49755a3a5e 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_env_test.go @@ -135,7 +135,7 @@ services: }, }, { - name: "project scans only expanded network fields", + name: "project scans only expanded network fields and honors escaping", content: `name: sample services: project: @@ -151,12 +151,14 @@ services: agentSubnet: vnet: $${AGENT_VNET_ID} peSubnet: - vnet: $${PE_VNET_ID} + vnet: ${PE_VNET_ID} dns: subscription: ${DNS_SUBSCRIPTION_ID} `, + // The network fields expand through foundry.ExpandEnv, so an escaped + // $${VAR} stays literal and needs no environment value; only the + // unescaped references are required. want: []azureYamlEnvironmentReference{ - {Name: "AGENT_VNET_ID"}, {Name: "PE_VNET_ID"}, {Name: "DNS_SUBSCRIPTION_ID"}, }, diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/resource_services.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/resource_services.go index 524a10d5966..83b5c8ae13b 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/resource_services.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/resource_services.go @@ -337,7 +337,7 @@ func collectEnvironmentTemplates(value any, environment map[string]string) { } func collectStringEnvironmentTemplates(value string, environment map[string]string) { - for _, reference := range findEnvironmentReferences(value, honorEnvironmentEscaping) { + for _, reference := range findEnvironmentReferences(value) { // env is keyed by name, so store one canonical ${NAME}. // A ${NAME:-default} default is re-applied by the owning // extension against the raw config at deploy, so the env section diff --git a/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go b/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go new file mode 100644 index 00000000000..5afddf74645 --- /dev/null +++ b/cli/azd/extensions/azure.ai.agents/internal/synthesis/envrefs.go @@ -0,0 +1,317 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package synthesis + +import ( + "fmt" + "regexp" + "strings" + + "github.com/azure/azure-dev/cli/azd/pkg/foundry" +) + +// EnvReference is one azd ${VAR} occurrence in a string. Start and End bound the +// whole reference, including any :- default, so a caller can resume scanning at +// End. +type EnvReference struct { + Name string + Start int + End int + HasDefault bool +} + +// envReferencePrefix parses only the reference prefix. Balanced defaults remain +// the scanner's responsibility. +var envReferencePrefix = regexp.MustCompile(`^\$\{([A-Za-z_][A-Za-z0-9_]*)(\}|:-)`) + +// FindEnvReferences returns the azd ${VAR} references in value that +// [foundry.ExpandEnv] actually resolves, in order of appearance. +// +// This is the single scanner for azd references in a Foundry value. Every +// consumer layers its own policy on the result rather than reimplementing +// discovery: init prompting skips references with a default because the +// expander supplies the fallback, the generated service env block records them +// so the owning extension can re-apply the default, and resolveVars treats the +// ones without a default as the names that must resolve. Re-deriving the escape +// and ${{...}} rules per consumer is what lets them drift away from the +// expander. +// +// References the expander would not resolve are dropped: escaped ones, and any +// reserved by a Foundry ${{...}} span. +// +// A reference inside a :- default is not reported: nested azd references are +// unsupported by design, so ${OUTER:-${NESTED}} yields OUTER only. +// [foundry.ExpandEnv] still resolves NESTED at deploy, but nothing discovers it, +// so init never prompts for it and it gets no entry in the generated service env +// block. It then resolves only where the consumer keeps an azd environment +// fallback, and to empty where a declared env: drops it. Keep defaults literal. +func FindEnvReferences(value string) []EnvReference { + candidates := envReferenceCandidates(value) + if len(candidates) == 0 { + return nil + } + + protected := protectedEnvReferences(value, candidates) + references := make([]EnvReference, 0, len(candidates)) + for i, candidate := range candidates { + if protected[i] { + continue + } + references = append(references, candidate) + } + if len(references) == 0 { + return nil + } + return references +} + +// ValidateEnvReferences reports an error when value carries a '$' form that +// [foundry.ExpandEnv] would act on but [FindEnvReferences] does not report. +// +// drone/envsubst, which backs the expander, implements the full shell parameter +// grammar: ${VAR:=default}, ${VAR:+alt}, ${VAR:?message}, ${VAR#prefix} and +// ${VAR:0:3} all expand. None of them are shapes the scanner above reports, so +// without this check they slip past the unresolved-variable guard and quietly +// rewrite a value that the caller then validates as if the user had written it. +// Typing ':=' instead of ':-' is a one character slip that would otherwise +// succeed while skipping the very guard it looks like it is using. +// +// A reference nested in a :- default is refused too — and that is a shape which +// resolves correctly today whenever the nested name is set: +// ${VNET_ID:-${FALLBACK_VNET_ID}} works on a project that has FALLBACK_VNET_ID +// in its environment. It is withdrawn rather than fixed because `required` is +// computed statically, and whether the nested name has to resolve depends on +// whether the outer one does — which is not known when the value is scanned. +// Reporting the nested name would raise a false unresolved-variable error every +// time the outer name IS set; not reporting it leaves ${A:-${B}} with neither +// set expanding to empty, so the caller blames the empty value instead of naming +// B. Neither half is right, so the shape goes. +// +// Where this runs, it makes [FindEnvReferences] complete: every occurrence the +// expander acts on is one the scanner saw. That is a property of the *call*, +// not of the scanner — only callers that invoke this get it. Today that is the +// three project network fields (network.agentSubnet.vnet, network.peSubnet.vnet, +// network.dns.subscription). Discovery-only consumers, such as init prompting +// and the generated service env block, still scan values that were never +// validated; extending the check to them is tracked by +// https://github.com/Azure/azure-dev/issues/9428. +func ValidateEnvReferences(value string) error { + // Non-zero while scanning the inside of a :- default. Nesting is refused on + // sight, so one boundary is enough — there is never a second level. + defaultEnd := 0 + for index := 0; index < len(value); { + if defaultEnd > 0 && index >= defaultEnd { + defaultEnd = 0 + } + if value[index] != '$' { + index++ + continue + } + // Outside a default, a '$' pair collapses to a literal '$', + // neutralizing only the '${' it precedes. Inside a :- default, + // envsubst re-parses the default text and the second '$' can still open + // a live ${VAR}, so leave it for the nested-reference check below. + // A Foundry span is masked before envsubst sees the pair in either case. + if defaultEnd == 0 && + strings.HasPrefix(value[index:], "$$") && + !strings.HasPrefix(value[index+1:], "${{") { + index += 2 + continue + } + // A Foundry span is reserved verbatim for the service to resolve. Legal + // as a default value, so this stays allowed inside one. + if strings.HasPrefix(value[index:], "${{") { + end := strings.Index(value[index+3:], "}}") + if end < 0 { + return fmt.Errorf("%q is missing the closing }} of a Foundry expression", + unsupportedEnvFragment(value, index)) + } + index += end + 5 + continue + } + // A bare '$' is not a reference: envsubst expands only the braced form, + // so "$VAR" and "costs $5" survive expansion untouched. + if !strings.HasPrefix(value[index:], "${") { + index++ + continue + } + reference, found := envReferenceAt(value, index) + if defaultEnd > 0 && found { + return fmt.Errorf( + "%q nests an environment variable reference inside a :- default, which azd "+ + "cannot check: whether the nested name is required depends on whether the "+ + "outer one resolves, and that is not known when the value is scanned. Use a "+ + "single ${VAR} and set it in the azd environment, or give the default a "+ + "literal value", + value[reference.Start:reference.End]) + } + if !found { + return fmt.Errorf( + "%q is not a supported environment variable reference; use ${VAR} or "+ + "${VAR:-default}, $${VAR} to keep it literal, or ${{...}} for a Foundry expression", + unsupportedEnvFragment(value, index)) + } + if reference.HasDefault { + // Step into the default rather than over it. FindEnvReferences stops + // at the default because nested references are not *discovered*; + // envsubst still *expands* whatever is in there, so both an + // unsupported form and a nested reference have to be caught here. + defaultEnd = reference.End + index = reference.Start + len("${") + len(reference.Name) + len(":-") + continue + } + index = reference.End + } + return nil +} + +// unsupportedEnvFragment returns the reference-looking fragment starting at +// index so an error can quote the offending text rather than the whole value. It +// stops at the first '}' because an unsupported form is by definition one the +// span scanner cannot bound. +func unsupportedEnvFragment(value string, index int) string { + rest := value[index:] + if end := strings.IndexByte(rest, '}'); end >= 0 { + return rest[:end+1] + } + return rest +} + +// envReferenceCandidates scans value left to right for ${NAME} and +// ${NAME:-default} occurrences. drone/envsubst, which backs +// [foundry.ExpandEnv], collapses a '$' pair into a literal '$' and keeps +// reading, so an escape only neutralizes the '${' it precedes: the text after +// it, including a default, still holds live references. Membership of a ${{...}} +// span is left to [FindEnvReferences]. Scanning resumes at the end of a match, +// so a default span is never scanned again; that is what keeps nested references +// out. +func envReferenceCandidates(value string) []EnvReference { + var references []EnvReference + for index := 0; index < len(value); { + if value[index] != '$' { + index++ + continue + } + if strings.HasPrefix(value[index:], "$$") { + index += 2 + continue + } + + reference, found := envReferenceAt(value, index) + if !found { + index++ + continue + } + + references = append(references, reference) + index = reference.End + } + return references +} + +// envReferenceAt parses the reference opening at start. The anchored prefix +// keeps a bare '$' from being read as one. Balanced defaults still need the +// stateful end scanner below. +func envReferenceAt(value string, start int) (EnvReference, bool) { + if start < 0 || start >= len(value) { + return EnvReference{}, false + } + + match := envReferencePrefix.FindStringSubmatch(value[start:]) + if match == nil { + return EnvReference{}, false + } + + name := match[1] + prefixEnd := start + len(match[0]) + if match[2] == "}" { + return EnvReference{ + Name: name, + Start: start, + End: prefixEnd, + }, true + } + + end, found := envReferenceEnd(value, prefixEnd) + if !found { + return EnvReference{}, false + } + return EnvReference{ + Name: name, + Start: start, + End: end, + HasDefault: true, + }, true +} + +// envReferenceEnd finds the '}' closing a :- default. It counts nested ${...} +// and steps over Foundry ${{...}} spans, which are legal default values, so the +// reported span covers the whole reference. +func envReferenceEnd(value string, index int) (int, bool) { + depth := 1 + for index < len(value) { + if strings.HasPrefix(value[index:], "${{") { + end := strings.Index(value[index+3:], "}}") + if end < 0 { + return 0, false + } + index += end + 5 + continue + } + if strings.HasPrefix(value[index:], "${") { + depth++ + index += 2 + continue + } + if value[index] == '}' { + depth-- + index++ + if depth == 0 { + return index, true + } + continue + } + index++ + } + return 0, false +} + +// protectedEnvReferences reports which candidates sit inside a server-side +// ${{...}} span. Each candidate is replaced with a unique probe before running +// [foundry.ExpandEnv]; probes left verbatim are reserved by the shared expander. +// This keeps discovery linked to the owning implementation without ambiguous +// name-based occurrence counting. +func protectedEnvReferences(value string, references []EnvReference) []bool { + protected := make([]bool, len(references)) + if len(references) == 0 { + return protected + } + + probePrefix := "AZD_ENV_REFERENCE_PROBE_" + for strings.Contains(value, probePrefix) { + probePrefix += "_" + } + + probeRefs := make([]string, len(references)) + var probed strings.Builder + last := 0 + for i, reference := range references { + probed.WriteString(value[last:reference.Start]) + probeRefs[i] = fmt.Sprintf("${%s%d}", probePrefix, i) + probed.WriteString(probeRefs[i]) + last = reference.End + } + probed.WriteString(value[last:]) + + expanded, err := foundry.ExpandEnv(probed.String(), func(name string) string { + return "expanded_" + name + }) + if err != nil { + return protected + } + for i, probeRef := range probeRefs { + protected[i] = strings.Contains(expanded, probeRef) + } + return protected +} diff --git a/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go b/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go index 4e68a9bdb67..2179527eb67 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go +++ b/cli/azd/extensions/azure.ai.agents/internal/synthesis/synthesizer.go @@ -902,9 +902,6 @@ var guidPattern = regexp.MustCompile( // rgNamePattern matches a valid Azure resource group name. var rgNamePattern = regexp.MustCompile(`^[-\w._()]{1,90}$`) -// varRefPattern matches a ${VAR} reference. -var varRefPattern = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)\}`) - // synthesizeNetwork validates the network: block and returns the bicep // parameter set plus the telemetry mode. When net is nil the returned // params disable network isolation and the output is byte-identical to the @@ -1017,6 +1014,12 @@ func synthesizeNetwork( params["dnsZonesResourceGroup"] = rg } if sub := strings.TrimSpace(net.DNS.Subscription); sub != "" { + // Rejected before either path reads it: an unsupported '$' form is + // silently rewritten by the expander on the provision path, and + // written verbatim into the ejected template on the other. + if err := ValidateEnvReferences(sub); err != nil { + return nil, "", fmt.Errorf("%s.dns.subscription: %w", fp(""), err) + } if resolve { resolved, err := resolveVars(sub, env) if err != nil { @@ -1024,9 +1027,12 @@ func synthesizeNetwork( } sub = resolved } - // Normalize to a bare GUID only when concrete; an unexpanded ${VAR} - // (eject path) is normalized at provision time. - if containsVarRef(sub) { + // Normalize to a bare GUID only when the value is final. On the eject + // path an unexpanded ${VAR} is normalized at provision time; once + // resolveVars has run there is nothing left to expand, so anything + // still shaped like a reference (an escaped $${VAR} resolves to a + // literal ${VAR}) is a subscription id that never will be. + if !resolve && containsVarRef(sub) { params["dnsZonesSubscription"] = sub } else { guid, err := normalizeSubscription(sub) @@ -1052,8 +1058,9 @@ func synthesizeNetwork( // vnet + name + prefix -> create subnet with that CIDR (create=true) // // vnet and name are required; ${VAR} references in vnet are expanded when -// resolve is true and validated as a Microsoft.Network/virtualNetworks id only -// when fully concrete. +// resolve is true. The Microsoft.Network/virtualNetworks id shape is then +// checked, except on the eject path (resolve false), where an unexpanded +// reference is left for provision time to validate. func resolveSubnet( s *subnetSpec, fieldPath string, env map[string]string, resolve bool, ) (vnetID, name, prefix string, create bool, err error) { @@ -1070,6 +1077,11 @@ func resolveSubnet( if name == "" { return "", "", "", false, fmt.Errorf("%s.name: required", fieldPath) } + // Rejected on both paths: see the dns.subscription call for why this cannot + // wait for resolveVars. + if err := ValidateEnvReferences(vnetID); err != nil { + return "", "", "", false, fmt.Errorf("%s.vnet: %w", fieldPath, err) + } if resolve { resolved, rerr := resolveVars(vnetID, env) if rerr != nil { @@ -1077,9 +1089,12 @@ func resolveSubnet( } vnetID = resolved } - // Validate the ARM id shape only when fully concrete; an unexpanded ${VAR} - // (eject path) is validated at provision time. - if !containsVarRef(vnetID) && !vnetIDPattern.MatchString(vnetID) { + // Validate the ARM id shape unless the value can still change: on the eject + // path an unexpanded ${VAR} is validated at provision time. After + // resolveVars there is nothing left to expand, so a leftover ${VAR} (what an + // escaped $${VAR} resolves to) is checked now rather than deferred to a + // provision that can only fail. + if (resolve || !containsVarRef(vnetID)) && !vnetIDPattern.MatchString(vnetID) { return "", "", "", false, fmt.Errorf( "%s.vnet: %q is not a well-formed Microsoft.Network/virtualNetworks id", fieldPath, vnetID) } @@ -1104,29 +1119,57 @@ func sameVNet(a, b string) bool { return strings.EqualFold(a, b) } -// containsVarRef reports whether s still contains a ${VAR} reference. +// containsVarRef reports whether s still carries an azd ${VAR} reference the +// expander will resolve, including the ${VAR:-default} form. +// +// Escaped references and names reserved by a Foundry ${{...}} span do not count: +// [foundry.ExpandEnv] leaves those alone, so the value is already as concrete as +// it will ever be and the caller's own shape validation should run on it. func containsVarRef(s string) bool { - return varRefPattern.MatchString(s) + return len(FindEnvReferences(s)) > 0 } // resolveVars expands ${VAR} references in s using env first, then the // process environment. An unresolved reference is an error naming the // variable. +// +// Expansion routes through foundry.ExpandEnv, the shared expander every other +// Foundry field uses, so ${VAR:-default} and the $${VAR} escape behave here +// exactly as they do elsewhere. +// +// ExpandEnv resolves through a mapping callback that only receives the variable +// name, so the names that must resolve are collected up front from +// [FindEnvReferences]: a name is required only where it occurs at least once +// without a :- default, in a position the expander will actually act on. Reusing +// that scanner is what keeps an escaped or ${{...}} reserved occurrence from +// making a live, defaulted occurrence of the same name look unresolvable. +// +// Callers validate the value with [ValidateEnvReferences] first, so every +// occurrence the expander acts on is one the scanner saw. func resolveVars(s string, env map[string]string) (string, error) { + required := map[string]struct{}{} + for _, reference := range FindEnvReferences(s) { + if !reference.HasDefault { + required[reference.Name] = struct{}{} + } + } + var unresolved string - out := varRefPattern.ReplaceAllStringFunc(s, func(match string) string { - name := varRefPattern.FindStringSubmatch(match)[1] + out, err := foundry.ExpandEnv(s, func(name string) string { if v, ok := env[name]; ok { return v } if v, ok := os.LookupEnv(name); ok { return v } - if unresolved == "" { + if _, ok := required[name]; ok && unresolved == "" { unresolved = name } - return match + return "" }) + if err != nil { + return "", err + } if unresolved != "" { return "", fmt.Errorf("unresolved environment variable ${%s}", unresolved) } diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go new file mode 100644 index 00000000000..5afddf74645 --- /dev/null +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/envrefs.go @@ -0,0 +1,317 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package synthesis + +import ( + "fmt" + "regexp" + "strings" + + "github.com/azure/azure-dev/cli/azd/pkg/foundry" +) + +// EnvReference is one azd ${VAR} occurrence in a string. Start and End bound the +// whole reference, including any :- default, so a caller can resume scanning at +// End. +type EnvReference struct { + Name string + Start int + End int + HasDefault bool +} + +// envReferencePrefix parses only the reference prefix. Balanced defaults remain +// the scanner's responsibility. +var envReferencePrefix = regexp.MustCompile(`^\$\{([A-Za-z_][A-Za-z0-9_]*)(\}|:-)`) + +// FindEnvReferences returns the azd ${VAR} references in value that +// [foundry.ExpandEnv] actually resolves, in order of appearance. +// +// This is the single scanner for azd references in a Foundry value. Every +// consumer layers its own policy on the result rather than reimplementing +// discovery: init prompting skips references with a default because the +// expander supplies the fallback, the generated service env block records them +// so the owning extension can re-apply the default, and resolveVars treats the +// ones without a default as the names that must resolve. Re-deriving the escape +// and ${{...}} rules per consumer is what lets them drift away from the +// expander. +// +// References the expander would not resolve are dropped: escaped ones, and any +// reserved by a Foundry ${{...}} span. +// +// A reference inside a :- default is not reported: nested azd references are +// unsupported by design, so ${OUTER:-${NESTED}} yields OUTER only. +// [foundry.ExpandEnv] still resolves NESTED at deploy, but nothing discovers it, +// so init never prompts for it and it gets no entry in the generated service env +// block. It then resolves only where the consumer keeps an azd environment +// fallback, and to empty where a declared env: drops it. Keep defaults literal. +func FindEnvReferences(value string) []EnvReference { + candidates := envReferenceCandidates(value) + if len(candidates) == 0 { + return nil + } + + protected := protectedEnvReferences(value, candidates) + references := make([]EnvReference, 0, len(candidates)) + for i, candidate := range candidates { + if protected[i] { + continue + } + references = append(references, candidate) + } + if len(references) == 0 { + return nil + } + return references +} + +// ValidateEnvReferences reports an error when value carries a '$' form that +// [foundry.ExpandEnv] would act on but [FindEnvReferences] does not report. +// +// drone/envsubst, which backs the expander, implements the full shell parameter +// grammar: ${VAR:=default}, ${VAR:+alt}, ${VAR:?message}, ${VAR#prefix} and +// ${VAR:0:3} all expand. None of them are shapes the scanner above reports, so +// without this check they slip past the unresolved-variable guard and quietly +// rewrite a value that the caller then validates as if the user had written it. +// Typing ':=' instead of ':-' is a one character slip that would otherwise +// succeed while skipping the very guard it looks like it is using. +// +// A reference nested in a :- default is refused too — and that is a shape which +// resolves correctly today whenever the nested name is set: +// ${VNET_ID:-${FALLBACK_VNET_ID}} works on a project that has FALLBACK_VNET_ID +// in its environment. It is withdrawn rather than fixed because `required` is +// computed statically, and whether the nested name has to resolve depends on +// whether the outer one does — which is not known when the value is scanned. +// Reporting the nested name would raise a false unresolved-variable error every +// time the outer name IS set; not reporting it leaves ${A:-${B}} with neither +// set expanding to empty, so the caller blames the empty value instead of naming +// B. Neither half is right, so the shape goes. +// +// Where this runs, it makes [FindEnvReferences] complete: every occurrence the +// expander acts on is one the scanner saw. That is a property of the *call*, +// not of the scanner — only callers that invoke this get it. Today that is the +// three project network fields (network.agentSubnet.vnet, network.peSubnet.vnet, +// network.dns.subscription). Discovery-only consumers, such as init prompting +// and the generated service env block, still scan values that were never +// validated; extending the check to them is tracked by +// https://github.com/Azure/azure-dev/issues/9428. +func ValidateEnvReferences(value string) error { + // Non-zero while scanning the inside of a :- default. Nesting is refused on + // sight, so one boundary is enough — there is never a second level. + defaultEnd := 0 + for index := 0; index < len(value); { + if defaultEnd > 0 && index >= defaultEnd { + defaultEnd = 0 + } + if value[index] != '$' { + index++ + continue + } + // Outside a default, a '$' pair collapses to a literal '$', + // neutralizing only the '${' it precedes. Inside a :- default, + // envsubst re-parses the default text and the second '$' can still open + // a live ${VAR}, so leave it for the nested-reference check below. + // A Foundry span is masked before envsubst sees the pair in either case. + if defaultEnd == 0 && + strings.HasPrefix(value[index:], "$$") && + !strings.HasPrefix(value[index+1:], "${{") { + index += 2 + continue + } + // A Foundry span is reserved verbatim for the service to resolve. Legal + // as a default value, so this stays allowed inside one. + if strings.HasPrefix(value[index:], "${{") { + end := strings.Index(value[index+3:], "}}") + if end < 0 { + return fmt.Errorf("%q is missing the closing }} of a Foundry expression", + unsupportedEnvFragment(value, index)) + } + index += end + 5 + continue + } + // A bare '$' is not a reference: envsubst expands only the braced form, + // so "$VAR" and "costs $5" survive expansion untouched. + if !strings.HasPrefix(value[index:], "${") { + index++ + continue + } + reference, found := envReferenceAt(value, index) + if defaultEnd > 0 && found { + return fmt.Errorf( + "%q nests an environment variable reference inside a :- default, which azd "+ + "cannot check: whether the nested name is required depends on whether the "+ + "outer one resolves, and that is not known when the value is scanned. Use a "+ + "single ${VAR} and set it in the azd environment, or give the default a "+ + "literal value", + value[reference.Start:reference.End]) + } + if !found { + return fmt.Errorf( + "%q is not a supported environment variable reference; use ${VAR} or "+ + "${VAR:-default}, $${VAR} to keep it literal, or ${{...}} for a Foundry expression", + unsupportedEnvFragment(value, index)) + } + if reference.HasDefault { + // Step into the default rather than over it. FindEnvReferences stops + // at the default because nested references are not *discovered*; + // envsubst still *expands* whatever is in there, so both an + // unsupported form and a nested reference have to be caught here. + defaultEnd = reference.End + index = reference.Start + len("${") + len(reference.Name) + len(":-") + continue + } + index = reference.End + } + return nil +} + +// unsupportedEnvFragment returns the reference-looking fragment starting at +// index so an error can quote the offending text rather than the whole value. It +// stops at the first '}' because an unsupported form is by definition one the +// span scanner cannot bound. +func unsupportedEnvFragment(value string, index int) string { + rest := value[index:] + if end := strings.IndexByte(rest, '}'); end >= 0 { + return rest[:end+1] + } + return rest +} + +// envReferenceCandidates scans value left to right for ${NAME} and +// ${NAME:-default} occurrences. drone/envsubst, which backs +// [foundry.ExpandEnv], collapses a '$' pair into a literal '$' and keeps +// reading, so an escape only neutralizes the '${' it precedes: the text after +// it, including a default, still holds live references. Membership of a ${{...}} +// span is left to [FindEnvReferences]. Scanning resumes at the end of a match, +// so a default span is never scanned again; that is what keeps nested references +// out. +func envReferenceCandidates(value string) []EnvReference { + var references []EnvReference + for index := 0; index < len(value); { + if value[index] != '$' { + index++ + continue + } + if strings.HasPrefix(value[index:], "$$") { + index += 2 + continue + } + + reference, found := envReferenceAt(value, index) + if !found { + index++ + continue + } + + references = append(references, reference) + index = reference.End + } + return references +} + +// envReferenceAt parses the reference opening at start. The anchored prefix +// keeps a bare '$' from being read as one. Balanced defaults still need the +// stateful end scanner below. +func envReferenceAt(value string, start int) (EnvReference, bool) { + if start < 0 || start >= len(value) { + return EnvReference{}, false + } + + match := envReferencePrefix.FindStringSubmatch(value[start:]) + if match == nil { + return EnvReference{}, false + } + + name := match[1] + prefixEnd := start + len(match[0]) + if match[2] == "}" { + return EnvReference{ + Name: name, + Start: start, + End: prefixEnd, + }, true + } + + end, found := envReferenceEnd(value, prefixEnd) + if !found { + return EnvReference{}, false + } + return EnvReference{ + Name: name, + Start: start, + End: end, + HasDefault: true, + }, true +} + +// envReferenceEnd finds the '}' closing a :- default. It counts nested ${...} +// and steps over Foundry ${{...}} spans, which are legal default values, so the +// reported span covers the whole reference. +func envReferenceEnd(value string, index int) (int, bool) { + depth := 1 + for index < len(value) { + if strings.HasPrefix(value[index:], "${{") { + end := strings.Index(value[index+3:], "}}") + if end < 0 { + return 0, false + } + index += end + 5 + continue + } + if strings.HasPrefix(value[index:], "${") { + depth++ + index += 2 + continue + } + if value[index] == '}' { + depth-- + index++ + if depth == 0 { + return index, true + } + continue + } + index++ + } + return 0, false +} + +// protectedEnvReferences reports which candidates sit inside a server-side +// ${{...}} span. Each candidate is replaced with a unique probe before running +// [foundry.ExpandEnv]; probes left verbatim are reserved by the shared expander. +// This keeps discovery linked to the owning implementation without ambiguous +// name-based occurrence counting. +func protectedEnvReferences(value string, references []EnvReference) []bool { + protected := make([]bool, len(references)) + if len(references) == 0 { + return protected + } + + probePrefix := "AZD_ENV_REFERENCE_PROBE_" + for strings.Contains(value, probePrefix) { + probePrefix += "_" + } + + probeRefs := make([]string, len(references)) + var probed strings.Builder + last := 0 + for i, reference := range references { + probed.WriteString(value[last:reference.Start]) + probeRefs[i] = fmt.Sprintf("${%s%d}", probePrefix, i) + probed.WriteString(probeRefs[i]) + last = reference.End + } + probed.WriteString(value[last:]) + + expanded, err := foundry.ExpandEnv(probed.String(), func(name string) string { + return "expanded_" + name + }) + if err != nil { + return protected + } + for i, probeRef := range probeRefs { + protected[i] = strings.Contains(expanded, probeRef) + } + return protected +} diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go index 4e68a9bdb67..2179527eb67 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer.go @@ -902,9 +902,6 @@ var guidPattern = regexp.MustCompile( // rgNamePattern matches a valid Azure resource group name. var rgNamePattern = regexp.MustCompile(`^[-\w._()]{1,90}$`) -// varRefPattern matches a ${VAR} reference. -var varRefPattern = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)\}`) - // synthesizeNetwork validates the network: block and returns the bicep // parameter set plus the telemetry mode. When net is nil the returned // params disable network isolation and the output is byte-identical to the @@ -1017,6 +1014,12 @@ func synthesizeNetwork( params["dnsZonesResourceGroup"] = rg } if sub := strings.TrimSpace(net.DNS.Subscription); sub != "" { + // Rejected before either path reads it: an unsupported '$' form is + // silently rewritten by the expander on the provision path, and + // written verbatim into the ejected template on the other. + if err := ValidateEnvReferences(sub); err != nil { + return nil, "", fmt.Errorf("%s.dns.subscription: %w", fp(""), err) + } if resolve { resolved, err := resolveVars(sub, env) if err != nil { @@ -1024,9 +1027,12 @@ func synthesizeNetwork( } sub = resolved } - // Normalize to a bare GUID only when concrete; an unexpanded ${VAR} - // (eject path) is normalized at provision time. - if containsVarRef(sub) { + // Normalize to a bare GUID only when the value is final. On the eject + // path an unexpanded ${VAR} is normalized at provision time; once + // resolveVars has run there is nothing left to expand, so anything + // still shaped like a reference (an escaped $${VAR} resolves to a + // literal ${VAR}) is a subscription id that never will be. + if !resolve && containsVarRef(sub) { params["dnsZonesSubscription"] = sub } else { guid, err := normalizeSubscription(sub) @@ -1052,8 +1058,9 @@ func synthesizeNetwork( // vnet + name + prefix -> create subnet with that CIDR (create=true) // // vnet and name are required; ${VAR} references in vnet are expanded when -// resolve is true and validated as a Microsoft.Network/virtualNetworks id only -// when fully concrete. +// resolve is true. The Microsoft.Network/virtualNetworks id shape is then +// checked, except on the eject path (resolve false), where an unexpanded +// reference is left for provision time to validate. func resolveSubnet( s *subnetSpec, fieldPath string, env map[string]string, resolve bool, ) (vnetID, name, prefix string, create bool, err error) { @@ -1070,6 +1077,11 @@ func resolveSubnet( if name == "" { return "", "", "", false, fmt.Errorf("%s.name: required", fieldPath) } + // Rejected on both paths: see the dns.subscription call for why this cannot + // wait for resolveVars. + if err := ValidateEnvReferences(vnetID); err != nil { + return "", "", "", false, fmt.Errorf("%s.vnet: %w", fieldPath, err) + } if resolve { resolved, rerr := resolveVars(vnetID, env) if rerr != nil { @@ -1077,9 +1089,12 @@ func resolveSubnet( } vnetID = resolved } - // Validate the ARM id shape only when fully concrete; an unexpanded ${VAR} - // (eject path) is validated at provision time. - if !containsVarRef(vnetID) && !vnetIDPattern.MatchString(vnetID) { + // Validate the ARM id shape unless the value can still change: on the eject + // path an unexpanded ${VAR} is validated at provision time. After + // resolveVars there is nothing left to expand, so a leftover ${VAR} (what an + // escaped $${VAR} resolves to) is checked now rather than deferred to a + // provision that can only fail. + if (resolve || !containsVarRef(vnetID)) && !vnetIDPattern.MatchString(vnetID) { return "", "", "", false, fmt.Errorf( "%s.vnet: %q is not a well-formed Microsoft.Network/virtualNetworks id", fieldPath, vnetID) } @@ -1104,29 +1119,57 @@ func sameVNet(a, b string) bool { return strings.EqualFold(a, b) } -// containsVarRef reports whether s still contains a ${VAR} reference. +// containsVarRef reports whether s still carries an azd ${VAR} reference the +// expander will resolve, including the ${VAR:-default} form. +// +// Escaped references and names reserved by a Foundry ${{...}} span do not count: +// [foundry.ExpandEnv] leaves those alone, so the value is already as concrete as +// it will ever be and the caller's own shape validation should run on it. func containsVarRef(s string) bool { - return varRefPattern.MatchString(s) + return len(FindEnvReferences(s)) > 0 } // resolveVars expands ${VAR} references in s using env first, then the // process environment. An unresolved reference is an error naming the // variable. +// +// Expansion routes through foundry.ExpandEnv, the shared expander every other +// Foundry field uses, so ${VAR:-default} and the $${VAR} escape behave here +// exactly as they do elsewhere. +// +// ExpandEnv resolves through a mapping callback that only receives the variable +// name, so the names that must resolve are collected up front from +// [FindEnvReferences]: a name is required only where it occurs at least once +// without a :- default, in a position the expander will actually act on. Reusing +// that scanner is what keeps an escaped or ${{...}} reserved occurrence from +// making a live, defaulted occurrence of the same name look unresolvable. +// +// Callers validate the value with [ValidateEnvReferences] first, so every +// occurrence the expander acts on is one the scanner saw. func resolveVars(s string, env map[string]string) (string, error) { + required := map[string]struct{}{} + for _, reference := range FindEnvReferences(s) { + if !reference.HasDefault { + required[reference.Name] = struct{}{} + } + } + var unresolved string - out := varRefPattern.ReplaceAllStringFunc(s, func(match string) string { - name := varRefPattern.FindStringSubmatch(match)[1] + out, err := foundry.ExpandEnv(s, func(name string) string { if v, ok := env[name]; ok { return v } if v, ok := os.LookupEnv(name); ok { return v } - if unresolved == "" { + if _, ok := required[name]; ok && unresolved == "" { unresolved = name } - return match + return "" }) + if err != nil { + return "", err + } if unresolved != "" { return "", fmt.Errorf("unresolved environment variable ${%s}", unresolved) } diff --git a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go index 297e2581004..59371b092a0 100644 --- a/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go +++ b/cli/azd/extensions/azure.ai.projects/internal/synthesis/synthesizer_test.go @@ -6,6 +6,7 @@ package synthesis import ( "encoding/json" "errors" + "fmt" "os" "path/filepath" "testing" @@ -1692,3 +1693,340 @@ services: }) } } + +// TestResolveVars_MatchesFoundryExpandEnv locks in that the three project +// network fields resolved through resolveVars use the same expander semantics +// as every other Foundry field: ${VAR:-default} falls back, $${VAR} stays +// literal, and a reference with neither a value nor a default is still a +// load-bearing error naming the variable. +func TestResolveVars_MatchesFoundryExpandEnv(t *testing.T) { + env := map[string]string{ + "SET_VAR": "set-value", + "EMPTY_VAR": "", + } + + tests := []struct { + name string + in string + want string + wantErr string + }{ + {name: "plain reference", in: "${SET_VAR}", want: "set-value"}, + {name: "default is unused when set", in: "${SET_VAR:-fallback}", want: "set-value"}, + {name: "default fills in when unset", in: "${MISSING_VAR_XYZ:-fallback}", want: "fallback"}, + {name: "empty default is allowed", in: "${MISSING_VAR_XYZ:-}", want: ""}, + {name: "empty env value takes the default", in: "${EMPTY_VAR:-fallback}", want: "fallback"}, + {name: "escaped reference stays literal", in: "$${MISSING_VAR_XYZ}", want: "${MISSING_VAR_XYZ}"}, + { + // required is derived from the same scanner the expander drives, so + // an occurrence the expander never resolves cannot make a live, + // defaulted occurrence of the same name look unresolvable. + name: "escaped reference does not make a defaulted one required", + in: "$${MISSING_VAR_XYZ} ${MISSING_VAR_XYZ:-fallback}", + want: "${MISSING_VAR_XYZ} fallback", + }, + { + name: "a name in a Foundry span does not make a defaulted one required", + in: "${{connections.${MISSING_VAR_XYZ}.key}} ${MISSING_VAR_XYZ:-fallback}", + want: "${{connections.${MISSING_VAR_XYZ}.key}} fallback", + }, + {name: "no references", in: "/subscriptions/abc", want: "/subscriptions/abc"}, + { + name: "default inside a resource id", + in: "${MISSING_VAR_XYZ:-/subscriptions/s/resourceGroups/rg}", + want: "/subscriptions/s/resourceGroups/rg", + }, + { + name: "unresolved reference errors", + in: "${MISSING_VAR_XYZ}", + wantErr: "unresolved environment variable ${MISSING_VAR_XYZ}", + }, + { + name: "first unresolved reference is named", + in: "${MISSING_A_XYZ}/${MISSING_B_XYZ}", + wantErr: "unresolved environment variable ${MISSING_A_XYZ}", + }, + { + name: "a default elsewhere does not excuse a bare reference", + in: "${MISSING_VAR_XYZ:-ok}/${MISSING_VAR_XYZ}", + wantErr: "unresolved environment variable ${MISSING_VAR_XYZ}", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := resolveVars(tt.in, env) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +// TestContainsVarRef_RecognizesDefaults guards the eject path: a reference with +// a default must be recognized as still-unresolved so the value is kept +// verbatim and the ARM-shape checks are deferred to provision time, instead of +// being rejected as a malformed resource id. +// +// The converse matters too. An escaped reference and a name reserved by a +// Foundry ${{...}} span are never expanded, so the value is already as concrete +// as it will ever be and the shape checks have to run on it now rather than +// being deferred to a provision that can only fail. +func TestContainsVarRef_RecognizesDefaults(t *testing.T) { + tests := []struct { + in string + want bool + }{ + {in: "${VNET}", want: true}, + {in: "${VNET:-/subscriptions/s}", want: true}, + {in: "/subscriptions/s/resourceGroups/rg", want: false}, + {in: "$${VNET}", want: false}, + {in: "${{connections.store.key}}", want: false}, + {in: "$${VNET} ${OTHER}", want: true}, + {in: "", want: false}, + } + + for _, tt := range tests { + t.Run(tt.in, func(t *testing.T) { + assert.Equal(t, tt.want, containsVarRef(tt.in)) + }) + } +} + +// TestValidateEnvReferences_RejectsUnsupportedForms pins the guard on +// drone/envsubst's wider grammar. Every rejected form below is one envsubst +// expands and the scanner does not report, so without this check it slips past +// the unresolved-variable guard: ${MISSING:=x} silently resolves, ${MISSING#x} +// silently becomes "", and the caller then validates the rewritten value as if +// the user had typed it. Typing ':=' for ':-' is a one character slip. +func TestValidateEnvReferences_RejectsUnsupportedForms(t *testing.T) { + t.Parallel() + + supported := []string{ + "${VAR}", + "${VAR:-default}", + "${VAR:-}", + "$${VAR}", + "${{connections.store.credentials.key}}", + "${{ tools.${INNER} }}", + "${MISSING:-${{event.body}}}", + // An escaped Foundry span: ExpandEnv masks the span starting at the + // second '$', so nothing inside it reaches envsubst and the '$' pair is + // never an escape. + "$${{ tools.${INNER} }}", + "/subscriptions/s/resourceGroups/rg", + // Bare '$' forms survive expansion untouched: envsubst only expands the + // braced shape, so these need no rejection. + "$VAR", + "costs $5 today", + "a$b", + "", + } + for _, value := range supported { + t.Run("ok/"+value, func(t *testing.T) { + t.Parallel() + assert.NoError(t, ValidateEnvReferences(value)) + }) + } + + unsupported := []string{ + "${MISSING:=default}", + "${MISSING:+alt}", + "${MISSING:?boom}", + "${MISSING#prefix}", + "${MISSING%suffix}", + "${MISSING:0:3}", + "${MISSING-nodefault}", + "${1BAD}", + "prefix ${MISSING:=x} suffix", + "${OUTER:-${INNER:=x}}", + "${A:-${9BAD}}", + } + for _, value := range unsupported { + t.Run("rejected/"+value, func(t *testing.T) { + t.Parallel() + err := ValidateEnvReferences(value) + require.Error(t, err) + assert.Contains(t, err.Error(), "is not a supported environment variable reference") + assert.Contains(t, err.Error(), "${VAR:-default}", + "the message has to name the shape the user probably meant") + }) + } + + // A nested reference is expanded but never discovered. Refusing it withdraws + // a shape that works today when the nested name is set, because `required` + // is static: reporting the nested name would fail whenever the outer one + // resolves, and not reporting it lets ${A:-${B}} with neither set expand to + // empty, so the field's own shape check blames the empty value. + nested := map[string]string{ + "${OUTER:-${NESTED}}": "${NESTED}", + "${OUTER:-prefix-${NESTED}-suffix}": "${NESTED}", + "${OUTER:-${NESTED:-inner}}": "${NESTED:-inner}", + "${OUTER:-$${NESTED}}": "${NESTED}", + "${OUTER:-prefix-$${NESTED}-suffix}": "${NESTED}", + // The quoted fragment has to be the nested reference's real span; a + // truncate-at-the-first-'}' fragment would come out unbalanced here. + "${A:-${B:-${C}}}": "${B:-${C}}", + } + for value, fragment := range nested { + t.Run("nested/"+value, func(t *testing.T) { + t.Parallel() + err := ValidateEnvReferences(value) + require.Error(t, err) + assert.Contains(t, err.Error(), "nests an environment variable reference inside a :- default") + assert.Contains(t, err.Error(), fmt.Sprintf("%q", fragment), + "the message has to quote the nested reference's real span") + }) + } + + t.Run("rejected/unterminated foundry span", func(t *testing.T) { + t.Parallel() + err := ValidateEnvReferences("${{connections.store.key}") + require.Error(t, err) + assert.Contains(t, err.Error(), "missing the closing }}") + }) +} + +// TestSynthesize_NetworkRejectsUnsupportedVarSyntax covers the guard end to end +// on both paths. envsubst would expand these, so on the provision path the +// value is silently rewritten before the ARM id / subscription checks see it, +// and on the eject path it is written into the template verbatim and rewritten +// at provision. Either way the user never learns their ':=' did not mean ':-'. +func TestSynthesize_NetworkRejectsUnsupportedVarSyntax(t *testing.T) { + tests := []struct { + name string + yaml string + field string + }{ + { + name: "subnet vnet", + field: "peSubnet.vnet", + yaml: ` +services: + my-project: + host: azure.ai.project + network: + peSubnet: {vnet: "${MISSING_VNET_XYZ:=/subscriptions/s}", name: pe-subnet} +`, + }, + { + name: "dns subscription", + field: "dns.subscription", + yaml: ` +services: + my-project: + host: azure.ai.project + network: + peSubnet: + vnet: /subscriptions/s/resourceGroups/rg/providers/Microsoft.Network/virtualNetworks/v + name: pe-subnet + dns: + subscription: "${MISSING_SUB_XYZ#prefix}" +`, + }, + } + + for _, tt := range tests { + for _, preserve := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/preserveVarRefs=%v", tt.name, preserve), func(t *testing.T) { + _, err := Synthesize(Input{ + RawAzureYAML: []byte(tt.yaml), + ServiceName: "my-project", + AcceptedHosts: []string{"azure.ai.project"}, + PreserveVarRefs: preserve, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "is not a supported environment variable reference") + assert.Contains(t, err.Error(), tt.field, + "the error has to name the offending field") + }) + } + } +} + +// TestSynthesize_NetworkEscapedRefIsValidatedOnBothPaths pins that an escaped +// reference is final on both paths. $${VNET} resolves to the literal ${VNET}, +// which is not a vnet id and never becomes one, so deferring the shape check to +// provision only moves the failure somewhere less useful — and made eject and +// provision disagree about the same azure.yaml. +func TestSynthesize_NetworkEscapedRefIsValidatedOnBothPaths(t *testing.T) { + const yaml = ` +services: + my-project: + host: azure.ai.project + network: + peSubnet: {vnet: "$${VNET_XYZ}", name: pe-subnet} +` + for _, preserve := range []bool{false, true} { + t.Run(fmt.Sprintf("preserveVarRefs=%v", preserve), func(t *testing.T) { + _, err := Synthesize(Input{ + RawAzureYAML: []byte(yaml), + ServiceName: "my-project", + AcceptedHosts: []string{"azure.ai.project"}, + PreserveVarRefs: preserve, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "is not a well-formed Microsoft.Network/virtualNetworks id") + }) + } +} + +// TestSynthesize_NetworkVarRefDefaults covers the reported bug end to end: +// ${VAR:-default} on the three network fields previously fell through +// resolveVars unchanged and was then rejected by the ARM id / subscription +// shape checks, blaming the resource id instead of the unsupported syntax. +func TestSynthesize_NetworkVarRefDefaults(t *testing.T) { + const ( + fallbackVNet = "/subscriptions/00000000-0000-0000-0000-000000000000" + + "/resourceGroups/rg/providers/Microsoft.Network/virtualNetworks/default" + fallbackSub = "11111111-1111-1111-1111-111111111111" + ) + + yaml := ` +services: + my-project: + host: azure.ai.project + network: + peSubnet: {vnet: "${MISSING_VNET_XYZ:-` + fallbackVNet + `}", name: pe-subnet} + dns: + subscription: "${MISSING_SUB_XYZ:-` + fallbackSub + `}" +` + res, err := Synthesize(Input{ + RawAzureYAML: []byte(yaml), + ServiceName: "my-project", + AcceptedHosts: []string{"azure.ai.project"}, + }) + require.NoError(t, err) + require.NotNil(t, res) + assert.Equal(t, fallbackVNet, res.Parameters["vnetId"]) + assert.Equal(t, fallbackSub, res.Parameters["dnsZonesSubscription"]) +} + +// TestSynthesize_NetworkPreserveVarRefsWithDefault is the eject-path half of the +// same bug: a defaulted reference must survive verbatim rather than being +// rejected as a malformed VNet id. +func TestSynthesize_NetworkPreserveVarRefsWithDefault(t *testing.T) { + const ref = "${AZURE_VNET_ID:-/subscriptions/s/resourceGroups/rg" + + "/providers/Microsoft.Network/virtualNetworks/default}" + + yaml := ` +services: + my-project: + host: azure.ai.project + network: + peSubnet: {vnet: "` + ref + `", name: pe-subnet} +` + res, err := Synthesize(Input{ + RawAzureYAML: []byte(yaml), + ServiceName: "my-project", + AcceptedHosts: []string{"azure.ai.project"}, + PreserveVarRefs: true, + }) + require.NoError(t, err, "a defaulted ${VAR} must not fail on the eject path") + require.NotNil(t, res) + assert.Equal(t, ref, res.Parameters["vnetId"]) +}