From 28c0432afd6d0bff3a8f0cacc4de6b1e206bf2f1 Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Fri, 12 Jun 2026 22:32:09 +0200 Subject: [PATCH] Do not restore cross-target variables into shared config in config-remote-sync Variable restoration in config-remote-sync runs with a single target selected, so the resolved variables include values that may only exist for that target (targets..variables assignments, BUNDLE_VAR_* environment variables, variable-overrides.json). The fallback lookup (matchAnyVariable) and sibling-based Add restoration (restoreFromSiblings) substituted such variables with no check that they resolve in other targets, writing e.g. ${var.dev_catalog} into the shared resources section and breaking every other target of the bundle. Add a cross-target guard, built from the configuration just before target selection (the only point where the full targets.*.variables map is still available): a ${var.X} reference may only be introduced at a position that didn't previously hold one when X has a root default or lookup, is assigned in every target, or the write destination is the current target's override section (which other targets never read). Single-target bundles are exempt since no other target can break. Restoration steps that re-emit a reference already present at the same position (matchOriginalRef, restoreCompoundInterpolation) are intentionally not gated. --- .../databricks.yml.tmpl | 55 ++++++ .../cross_target_variables/out.test.toml | 4 + .../cross_target_variables/output.txt | 57 ++++++ .../cross_target_variables/script | 49 +++++ .../cross_target_variables/test.toml | 10 + bundle/configsync/variables.go | 184 ++++++++++++++++-- bundle/configsync/variables_test.go | 181 ++++++++++++++++- 7 files changed, 521 insertions(+), 19 deletions(-) create mode 100644 acceptance/bundle/config-remote-sync/cross_target_variables/databricks.yml.tmpl create mode 100644 acceptance/bundle/config-remote-sync/cross_target_variables/out.test.toml create mode 100644 acceptance/bundle/config-remote-sync/cross_target_variables/output.txt create mode 100644 acceptance/bundle/config-remote-sync/cross_target_variables/script create mode 100644 acceptance/bundle/config-remote-sync/cross_target_variables/test.toml diff --git a/acceptance/bundle/config-remote-sync/cross_target_variables/databricks.yml.tmpl b/acceptance/bundle/config-remote-sync/cross_target_variables/databricks.yml.tmpl new file mode 100644 index 00000000000..20e82f1b8f6 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/cross_target_variables/databricks.yml.tmpl @@ -0,0 +1,55 @@ +bundle: + name: test-bundle-$UNIQUE_NAME + +variables: + # Root default: resolvable in every target, so sync may introduce + # references to it anywhere. + region: + default: us-west-1 + # No root default, but assigned in every target: also resolvable everywhere. + etl_schema: + description: Schema name, assigned in every target + # No root default and only assigned in dev: other targets cannot resolve it, + # so sync must not introduce ${var.dev_catalog} outside the dev section. + dev_catalog: + description: Catalog name, assigned only in the dev target + # Same shape as dev_catalog; referenced from the dev override section where + # dev-only variables are allowed. + dev_label: + description: Cost center tag, assigned only in the dev target + +resources: + jobs: + my_job: + name: test-job-$UNIQUE_NAME + parameters: + - name: region + default: ${var.region} + - name: schema + default: ${var.etl_schema} + - name: catalog + default: ${var.dev_catalog} + tasks: + - task_key: main + notebook_task: + notebook_path: /Users/{{workspace_user_name}}/notebook + new_cluster: + spark_version: $DEFAULT_SPARK_VERSION + node_type_id: $NODE_TYPE_ID + num_workers: 1 + +targets: + dev: + default: true + variables: + etl_schema: dev_schema + dev_catalog: dev_data + dev_label: dev-team + resources: + jobs: + my_job: + tags: + costcenter: ${var.dev_label} + prod: + variables: + etl_schema: prod_schema diff --git a/acceptance/bundle/config-remote-sync/cross_target_variables/out.test.toml b/acceptance/bundle/config-remote-sync/cross_target_variables/out.test.toml new file mode 100644 index 00000000000..579b1e4a3c9 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/cross_target_variables/out.test.toml @@ -0,0 +1,4 @@ +Local = true +Cloud = true +GOOS.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct", "terraform"] diff --git a/acceptance/bundle/config-remote-sync/cross_target_variables/output.txt b/acceptance/bundle/config-remote-sync/cross_target_variables/output.txt new file mode 100644 index 00000000000..5f76807d391 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/cross_target_variables/output.txt @@ -0,0 +1,57 @@ +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== Modify job remotely + +=== Detect and save changes +Detected changes in 1 resource(s): + +Resource: resources.jobs.my_job + parameters[name='catalog_copy']: add + parameters[name='data_region']: add + parameters[name='region'].default: replace + parameters[name='schema_copy']: add + tags['costcenter']: replace + + + +=== Configuration changes + +>>> diff.py databricks.yml.backup databricks.yml +--- databricks.yml.backup ++++ databricks.yml +@@ -25,9 +25,15 @@ + parameters: + - name: region +- default: ${var.region} ++ default: dev_data + - name: schema + default: ${var.etl_schema} + - name: catalog + default: ${var.dev_catalog} ++ - default: dev_data ++ name: catalog_copy ++ - default: ${var.region} ++ name: data_region ++ - default: ${var.etl_schema} ++ name: schema_copy + tasks: + - task_key: main +@@ -50,5 +56,5 @@ + my_job: + tags: +- costcenter: ${var.dev_label} ++ costcenter: ${var.dev_catalog} + prod: + variables: + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.jobs.my_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/dev + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/config-remote-sync/cross_target_variables/script b/acceptance/bundle/config-remote-sync/cross_target_variables/script new file mode 100644 index 00000000000..97cd6489de5 --- /dev/null +++ b/acceptance/bundle/config-remote-sync/cross_target_variables/script @@ -0,0 +1,49 @@ +#!/bin/bash + +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve +} +trap cleanup EXIT + +$CLI bundle deploy +job_id="$(read_id.py my_job)" + +title "Modify job remotely" +echo +edit_resource.py jobs $job_id < restored. +r["parameters"].append({"name": "data_region", "default": "us-west-1"}) +# "dev_schema" matches sibling \${var.etl_schema} (assigned in every target) +# -> restored. +r["parameters"].append({"name": "schema_copy", "default": "dev_schema"}) +# "dev_data" matches sibling \${var.dev_catalog}, which is only assigned in +# the dev target: restoring it into the shared resources section would break +# every other target -> stays hardcoded. +r["parameters"].append({"name": "catalog_copy", "default": "dev_data"}) + +# --- Replace fallback (re-targeting to another variable) --- +# "region" was \${var.region}; the new value uniquely matches dev-only +# \${var.dev_catalog} -> the shared definition keeps the hardcoded value. +for p in r["parameters"]: + if p["name"] == "region": + p["default"] = "dev_data" + +# --- Replace inside the current target's override section --- +# costcenter is defined under targets.dev; dev-only variables are allowed +# there, so the unique match \${var.dev_catalog} is restored. +r["tags"]["costcenter"] = "dev_data" +EOF + +title "Detect and save changes" +echo +cp databricks.yml databricks.yml.backup +$CLI bundle config-remote-sync --save + +title "Configuration changes" +echo +trace diff.py databricks.yml.backup databricks.yml +rm databricks.yml.backup diff --git a/acceptance/bundle/config-remote-sync/cross_target_variables/test.toml b/acceptance/bundle/config-remote-sync/cross_target_variables/test.toml new file mode 100644 index 00000000000..1067069b9fa --- /dev/null +++ b/acceptance/bundle/config-remote-sync/cross_target_variables/test.toml @@ -0,0 +1,10 @@ +Cloud = true + +RecordRequests = false +Ignore = [".databricks", "databricks.yml", "databricks.yml.backup"] + +[Env] +DATABRICKS_BUNDLE_ENABLE_EXPERIMENTAL_YAML_SYNC = "true" + +[EnvMatrix] +DATABRICKS_BUNDLE_ENGINE = ["direct", "terraform"] diff --git a/bundle/configsync/variables.go b/bundle/configsync/variables.go index 0745bfba43b..d50cbf46ab9 100644 --- a/bundle/configsync/variables.go +++ b/bundle/configsync/variables.go @@ -51,8 +51,14 @@ var varPrefix = dyn.NewPath(dyn.Key("var")) // view where ${var.X} and ${resources.X.Y.id} references are still literal // strings — enabling correct sibling lookup even for sequences split across // files via target overrides. +// +// Restoration steps that introduce a reference at a position that didn't +// previously hold one (the Replace fallback and sibling-based Add restoration) +// are additionally gated by crossTargetGuard: a ${var.X} reference is only +// introduced when X is resolvable in every target, or when the write +// destination is the current target's override section. func RestoreVariableReferences(ctx context.Context, b *bundle.Bundle, fieldChanges []FieldChange) error { - preResolved := loadPreResolvedConfig(ctx, b) + preResolved, guard := loadPreResolvedConfig(ctx, b) if !preResolved.IsValid() { return errors.New("pre-resolved config unavailable; variable-backed fields will be hardcoded") } @@ -87,6 +93,7 @@ func RestoreVariableReferences(ctx context.Context, b *bundle.Bundle, fieldChang for i := range fieldChanges { fc := &fieldChanges[i] + allow := guard.allowFor(fc.FieldCandidates) var newValue any switch fc.Change.Operation { @@ -95,13 +102,13 @@ func RestoreVariableReferences(ctx context.Context, b *bundle.Bundle, fieldChang if !ok { continue } - newValue = restoreOriginalRefs(fc.Change.Value, fieldValue, resolved) + newValue = restoreOriginalRefs(fc.Change.Value, fieldValue, resolved, allow) case OperationAdd: siblings, ok := sequenceSiblings(preResolved, fc.FieldCandidates) if !ok { continue } - newValue = restoreFromSiblings(fc.Change.Value, siblings, resolved) + newValue = restoreFromSiblings(fc.Change.Value, siblings, resolved, allow) case OperationUnknown, OperationRemove, OperationSkip: continue } @@ -119,18 +126,152 @@ func RestoreVariableReferences(ctx context.Context, b *bundle.Bundle, fieldChang // variable resolution. The resulting dyn.Value is fully merged across files // and targets, yet retains ${...} references as literal strings. Returns // InvalidValue if loading fails (restoration is then skipped). -func loadPreResolvedConfig(ctx context.Context, b *bundle.Bundle) dyn.Value { +// +// The cross-target guard is built just before target selection: that is the +// only point where the full targets.*.variables map is still available +// (SelectTarget merges the chosen target into the root and removes the +// targets section). +func loadPreResolvedConfig(ctx context.Context, b *bundle.Bundle) (dyn.Value, *crossTargetGuard) { fresh := &bundle.Bundle{ BundleRootPath: b.BundleRootPath, BundleRoot: b.BundleRoot, } mutator.DefaultMutators(ctx, fresh) + guard := newCrossTargetGuard(&fresh.Config) if target := b.Config.Bundle.Target; target != "" { if _, ok := fresh.Config.Targets[target]; ok { bundle.ApplyContext(ctx, fresh, mutator.SelectTarget(target)) } } - return fresh.Config.Value() + return fresh.Config.Value(), guard +} + +// crossTargetGuard decides whether restoration may introduce a ${var.X} +// reference at a position that didn't previously hold one. Restoration runs +// with a single target selected, so the resolved variables include values that +// may only exist for that target (targets..variables assignments, +// BUNDLE_VAR_* environment variables, variable-overrides.json). Writing such a +// reference into a shared part of the configuration breaks the other targets: +// a variable without a root default that some target doesn't assign makes that +// target fail to load. The guard only admits variables that are resolvable in +// every target — unless the write destination is the current target's override +// section, which other targets never read. +type crossTargetGuard struct { + // targetsRoot is the merged configuration before target selection, with + // the targets.* subtree still present. Used to classify write destinations. + targetsRoot dyn.Value + + // multiTarget is false when the bundle has at most one target; restoration + // can't create cross-target breakage then and all variables are allowed. + multiTarget bool + + // safe holds the variables that are resolvable in every target: a default + // or lookup in the root variables block, or an assignment in every + // target's variables block. + safe map[string]bool +} + +// newCrossTargetGuard captures variable declarations from a config that has +// not had a target selected yet. +func newCrossTargetGuard(cfg *config.Root) *crossTargetGuard { + g := &crossTargetGuard{ + targetsRoot: cfg.Value(), + multiTarget: len(cfg.Targets) > 1, + safe: map[string]bool{}, + } + // The nil checks below guard direct construction in unit tests; in the + // production path InitializeVariables and the loader produce non-nil + // entries. + for name, v := range cfg.Variables { + if v != nil && (v.HasDefault() || v.Lookup != nil) { + g.safe[name] = true + continue + } + assignedEverywhere := len(cfg.Targets) > 0 + for _, target := range cfg.Targets { + if target == nil { + assignedEverywhere = false + break + } + tv := target.Variables[name] + if tv == nil || (tv.Default == nil && tv.Lookup == nil) { + assignedEverywhere = false + break + } + } + g.safe[name] = assignedEverywhere + } + return g +} + +// allowFor returns the variable admission predicate for a field change with +// the given path candidates (plain path first, targets..-prefixed second; +// see ResolveChanges). +func (g *crossTargetGuard) allowFor(candidates []string) func(string) bool { + if !g.multiTarget || g.destinationInTarget(candidates) { + return allowAllVariables + } + return func(name string) bool { return g.safe[name] } +} + +func allowAllVariables(string) bool { return true } + +// destinationInTarget reports whether the change can only be written inside +// the current target's override section. applyChange tries the plain path +// first, so the destination is the target section only when the plain path +// does not exist in the pre-target-selection configuration while the +// targets..-prefixed path does. Classification failures fall back to false, +// which applies the stricter shared-destination rule. +// +// Known limitation: candidates carry file-local numeric indexes (see +// yamlFileIndex), so for a sequence element defined only in the target +// override section the plain path may collide with a different element of the +// shared sequence and classify as shared. That errs in the safe direction: +// the value stays hardcoded instead of becoming a target-only reference. +func (g *crossTargetGuard) destinationInTarget(candidates []string) bool { + if len(candidates) < 2 { + return false + } + return !patternExists(g.targetsRoot, candidates[0]) && patternExists(g.targetsRoot, candidates[1]) +} + +// patternExists reports whether the (possibly [*]-terminated) path pattern +// resolves in root. For new-element Adds the trailing [*] is dropped so the +// check applies to the parent sequence. +func patternExists(root dyn.Value, pattern string) bool { + node, err := structpath.ParsePattern(pattern) + if err != nil { + return false + } + if node.BracketStar() { + node = node.Parent() + } + p, err := dyn.NewPathFromString(node.String()) + if err != nil { + return false + } + _, err = dyn.GetByPath(root, p) + return err == nil +} + +// varRefsAllowed reports whether every ${var.X} reference in s names a +// variable admitted by allow. References outside the var namespace +// (${bundle.X}, ${workspace.X}, ${resources.X.Y.id}) are exempt. +func varRefsAllowed(s string, allow func(string) bool) bool { + ref, ok := dynvar.NewRef(dyn.V(s)) + if !ok { + return true + } + for _, key := range ref.References() { + p, err := dyn.NewPathFromString(key) + if err != nil || !p.HasPrefix(varPrefix) || len(p) < 2 { + continue + } + if !allow(p[1].Key()) { + return false + } + } + return true } // resourceIDLookup returns a function that resolves resource keys to their @@ -231,7 +372,11 @@ func resolveReferencePath(refStr string) (dyn.Path, bool) { // new value, falls back to a global lookup: if the new value uniquely matches // a different variable, that variable is used instead. The field's prior use // of a variable is the false-positive guard. -func restoreOriginalRefs(value any, preResolved, resolved dyn.Value) any { +// +// Only the fallback consults allow: the other two steps re-emit a reference +// that already exists at this exact position, which can't introduce a +// cross-target leak. +func restoreOriginalRefs(value any, preResolved, resolved dyn.Value, allow func(string) bool) any { switch v := value.(type) { case string, bool, int64: if ref, ok := matchOriginalRef(value, preResolved, resolved); ok { @@ -243,7 +388,7 @@ func restoreOriginalRefs(value any, preResolved, resolved dyn.Value) any { } } if isPureVarRef(preResolved) { - if ref, ok := matchAnyVariable(value, resolved); ok { + if ref, ok := matchAnyVariable(value, resolved); ok && varRefsAllowed(ref, allow) { return ref } } @@ -258,7 +403,7 @@ func restoreOriginalRefs(value any, preResolved, resolved dyn.Value) any { childPre = p.Value } } - v[key] = restoreOriginalRefs(val, childPre, resolved) + v[key] = restoreOriginalRefs(val, childPre, resolved, allow) } return v @@ -269,7 +414,7 @@ func restoreOriginalRefs(value any, preResolved, resolved dyn.Value) any { if i < len(preSeq) { childPre = preSeq[i] } - v[i] = restoreOriginalRefs(val, childPre, resolved) + v[i] = restoreOriginalRefs(val, childPre, resolved, allow) } return v @@ -283,11 +428,15 @@ func restoreOriginalRefs(value any, preResolved, resolved dyn.Value) any { // relative path: if exactly one unique pure variable reference across siblings // resolves to the leaf value, that reference is substituted. Multiple // different matching references are treated as ambiguous and skipped. -func restoreFromSiblings(value any, siblings []dyn.Value, resolved dyn.Value) any { - return restoreFromSiblingsAt(value, siblings, resolved, dyn.EmptyPath) +// +// Every restored reference is checked against allow: sibling references come +// from other positions (possibly other files or the target override section), +// so substituting one always introduces a reference at a new position. +func restoreFromSiblings(value any, siblings []dyn.Value, resolved dyn.Value, allow func(string) bool) any { + return restoreFromSiblingsAt(value, siblings, resolved, dyn.EmptyPath, allow) } -func restoreFromSiblingsAt(value any, siblings []dyn.Value, resolved dyn.Value, relPath dyn.Path) any { +func restoreFromSiblingsAt(value any, siblings []dyn.Value, resolved dyn.Value, relPath dyn.Path, allow func(string) bool) any { switch v := value.(type) { case string, bool, int64: refs := map[string]struct{}{} @@ -323,22 +472,27 @@ func restoreFromSiblingsAt(value any, siblings []dyn.Value, resolved dyn.Value, } } } + // The allow check runs after unique-match selection so that variables + // rejected by the guard still count toward ambiguity rather than + // promoting another candidate. if len(refs) == 1 { for ref := range refs { - return ref + if varRefsAllowed(ref, allow) { + return ref + } } } return value case map[string]any: for key, val := range v { - v[key] = restoreFromSiblingsAt(val, siblings, resolved, relPath.Append(dyn.Key(key))) + v[key] = restoreFromSiblingsAt(val, siblings, resolved, relPath.Append(dyn.Key(key)), allow) } return v case []any: for i, val := range v { - v[i] = restoreFromSiblingsAt(val, siblings, resolved, relPath.Append(dyn.Index(i))) + v[i] = restoreFromSiblingsAt(val, siblings, resolved, relPath.Append(dyn.Index(i)), allow) } return v diff --git a/bundle/configsync/variables_test.go b/bundle/configsync/variables_test.go index 9dd94c0cfb7..d4e1c0582a6 100644 --- a/bundle/configsync/variables_test.go +++ b/bundle/configsync/variables_test.go @@ -3,6 +3,8 @@ package configsync import ( "testing" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/variable" "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/assert" ) @@ -19,7 +21,7 @@ func TestRestoreOriginalRefs_HardcodedFieldNotRewritten(t *testing.T) { }) // Even though "main" matches ${var.region}, restoreOriginalRefs must NOT // rewrite it — the original was hardcoded. - result := restoreOriginalRefs("main", preResolved, resolved) + result := restoreOriginalRefs("main", preResolved, resolved, allowAllVariables) assert.Equal(t, "main", result) } @@ -44,7 +46,7 @@ func TestRestoreFromSiblings_ValueMatchesVariableButDifferentPath(t *testing.T) "task_key": "other", "min_retry_interval": int64(5), } - result := restoreFromSiblings(value, siblings, resolved).(map[string]any) + result := restoreFromSiblings(value, siblings, resolved, allowAllVariables).(map[string]any) assert.Equal(t, "other", result["task_key"]) assert.Equal(t, int64(5), result["min_retry_interval"]) } @@ -64,7 +66,7 @@ func TestRestoreFromSiblings_AmbiguousAcrossSiblings(t *testing.T) { }), }) value := map[string]any{"default": "raw_data"} - result := restoreFromSiblings(value, siblings, resolved).(map[string]any) + result := restoreFromSiblings(value, siblings, resolved, allowAllVariables).(map[string]any) assert.Equal(t, "raw_data", result["default"]) } @@ -108,8 +110,179 @@ func TestRestoreCompoundInterpolation(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := restoreOriginalRefs(tt.remote, dyn.V(tt.template), resolved) + result := restoreOriginalRefs(tt.remote, dyn.V(tt.template), resolved, allowAllVariables) assert.Equal(t, tt.want, result) }) } } + +func TestNewCrossTargetGuard_Safe(t *testing.T) { + strDefault := func(s string) *variable.TargetVariable { + return &variable.TargetVariable{Default: s} + } + cfg := &config.Root{ + Variables: map[string]*variable.Variable{ + "root_default": {Default: "v"}, + "root_lookup": {Lookup: &variable.Lookup{Cluster: "c"}}, + "all_targets": {}, + "dev_only": {}, + "env_or_file_fed": {}, + }, + Targets: map[string]*config.Target{ + "dev": {Variables: map[string]*variable.TargetVariable{ + "all_targets": strDefault("dev-v"), + "dev_only": strDefault("dev-v"), + }}, + "prod": {Variables: map[string]*variable.TargetVariable{ + "all_targets": strDefault("prod-v"), + }}, + }, + } + + g := newCrossTargetGuard(cfg) + assert.True(t, g.multiTarget) + assert.True(t, g.safe["root_default"]) + assert.True(t, g.safe["root_lookup"]) + assert.True(t, g.safe["all_targets"]) + assert.False(t, g.safe["dev_only"]) + assert.False(t, g.safe["env_or_file_fed"]) +} + +func TestCrossTargetGuard_AllowFor(t *testing.T) { + targetsRoot := dyn.V(map[string]dyn.Value{ + "resources": dyn.V(map[string]dyn.Value{ + "jobs": dyn.V(map[string]dyn.Value{ + "foo": dyn.V(map[string]dyn.Value{ + "tasks": dyn.V([]dyn.Value{dyn.V(map[string]dyn.Value{"task_key": dyn.V("main")})}), + }), + }), + }), + "targets": dyn.V(map[string]dyn.Value{ + "dev": dyn.V(map[string]dyn.Value{ + "resources": dyn.V(map[string]dyn.Value{ + "jobs": dyn.V(map[string]dyn.Value{ + "foo": dyn.V(map[string]dyn.Value{ + "tags": dyn.V(map[string]dyn.Value{"env": dyn.V("x")}), + "tasks": dyn.V([]dyn.Value{dyn.V(map[string]dyn.Value{"task_key": dyn.V("extra")})}), + }), + }), + }), + }), + }), + }) + + multi := &crossTargetGuard{targetsRoot: targetsRoot, multiTarget: true, safe: map[string]bool{"safe_var": true}} + single := &crossTargetGuard{targetsRoot: targetsRoot, multiTarget: false, safe: map[string]bool{}} + + tests := []struct { + name string + guard *crossTargetGuard + candidates []string + variable string + want bool + }{ + { + name: "single target allows everything", + guard: single, + candidates: []string{"resources.jobs.foo.tasks[*]"}, + variable: "dev_only", + want: true, + }, + { + name: "shared destination allows safe variable", + guard: multi, + candidates: []string{"resources.jobs.foo.tasks[*]", "targets.dev.resources.jobs.foo.tasks[*]"}, + variable: "safe_var", + want: true, + }, + { + name: "shared destination rejects unsafe variable", + guard: multi, + candidates: []string{"resources.jobs.foo.tasks[*]", "targets.dev.resources.jobs.foo.tasks[*]"}, + variable: "dev_only", + want: false, + }, + { + name: "target override destination allows unsafe variable", + guard: multi, + candidates: []string{"resources.jobs.foo.tags.env", "targets.dev.resources.jobs.foo.tags.env"}, + variable: "dev_only", + want: true, + }, + { + // Pins the documented destinationInTarget limitation: the + // file-local index of an element defined only in the target + // override collides with the shared sequence, so the change + // classifies as shared and the strict rule applies. + name: "indexed override element falls back to strict rule", + guard: multi, + candidates: []string{"resources.jobs.foo.tasks[0].task_key", "targets.dev.resources.jobs.foo.tasks[0].task_key"}, + variable: "dev_only", + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + allow := tt.guard.allowFor(tt.candidates) + assert.Equal(t, tt.want, allow(tt.variable)) + }) + } +} + +func TestVarRefsAllowed(t *testing.T) { + allow := func(name string) bool { return name == "safe_var" } + + tests := []struct { + s string + want bool + }{ + {"${var.safe_var}", true}, + {"${var.dev_only}", false}, + {"${bundle.target}", true}, + {"${resources.pipelines.p.id}", true}, + {"/mnt/${var.safe_var}/${bundle.target}/raw", true}, + {"/mnt/${var.safe_var}/${var.dev_only}/raw", false}, + {"no refs at all", true}, + } + for _, tt := range tests { + t.Run(tt.s, func(t *testing.T) { + assert.Equal(t, tt.want, varRefsAllowed(tt.s, allow)) + }) + } +} + +// TestRestoreFromSiblings_GuardBlocksCrossTargetVariable fences the Add-path +// guard: a unique sibling match naming a variable other targets can't resolve +// must stay hardcoded instead of leaking into a shared file. +func TestRestoreFromSiblings_GuardBlocksCrossTargetVariable(t *testing.T) { + siblings := []dyn.Value{ + dyn.V(map[string]dyn.Value{"default": dyn.V("${var.dev_only}")}), + } + resolved := dyn.V(map[string]dyn.Value{ + "variables": dyn.V(map[string]dyn.Value{ + "dev_only": dyn.V(map[string]dyn.Value{"value": dyn.V("dev_data")}), + }), + }) + allow := func(name string) bool { return false } + + value := map[string]any{"default": "dev_data"} + result := restoreFromSiblings(value, siblings, resolved, allow).(map[string]any) + assert.Equal(t, "dev_data", result["default"]) +} + +// TestRestoreOriginalRefs_GuardBlocksFallback fences the Replace-fallback +// guard: re-targeting a pure ${var.X} field to a variable other targets can't +// resolve must fall back to the hardcoded value. +func TestRestoreOriginalRefs_GuardBlocksFallback(t *testing.T) { + preResolved := dyn.V("${var.region}") + resolved := dyn.V(map[string]dyn.Value{ + "variables": dyn.V(map[string]dyn.Value{ + "region": dyn.V(map[string]dyn.Value{"value": dyn.V("us-west-1")}), + "dev_only": dyn.V(map[string]dyn.Value{"value": dyn.V("dev_data")}), + }), + }) + allow := func(name string) bool { return name == "region" } + + result := restoreOriginalRefs("dev_data", preResolved, resolved, allow) + assert.Equal(t, "dev_data", result) +}