Do not restore cross-target variables into shared config in config-remote-sync#5593
Open
ilyakuz-db wants to merge 1 commit into
Open
Do not restore cross-target variables into shared config in config-remote-sync#5593ilyakuz-db wants to merge 1 commit into
ilyakuz-db wants to merge 1 commit into
Conversation
…mote-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.<t>.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.
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Contributor
Approval status: pending
|
Collaborator
Integration test reportCommit: 28c0432
22 interesting tests: 15 SKIP, 7 KNOWN
Top 29 slowest tests (at least 2 minutes):
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
bundle config-remote-syncrestores hardcoded remote values back to${var.X}references. Two restoration paths could introduce a reference into shared configuration that names a variable only resolvable for the currently selected target:matchAnyVariable), which re-targets a${var.X}field to any uniquely matching variable, andrestoreFromSiblings), which copies references from sibling sequence elements.Both consult the post-target-selection config, where
targets.<t>.variablesassignments and invocation-scoped values (BUNDLE_VAR_*,variable-overrides.json) are already merged in. Writing such a reference into the sharedresourcessection breaks every other target of the bundle.This PR adds a cross-target guard, built from the configuration just before
SelectTargetruns (the only point where the fulltargets.*.variablesmap is still available). A${var.X}reference may only be introduced at a position that didn't previously hold one when:Xhas a default or lookup in the rootvariablesblock, orXis assigned a default or lookup in every target'svariablesblock, orapplyChangetries them in).Restoration steps that re-emit a reference already present at the same position (
matchOriginalRef,restoreCompoundInterpolationon Replace) are intentionally not gated, and guard-rejected variables still count toward the existing ambiguity rule rather than promoting another candidate.Why
Without the guard, syncing a remote edit on one target could rewrite a shared field such as
default: ${var.region}todefault: ${var.dev_catalog}(or copy${var.dev_catalog}into a newly added element) when the edited value happened to match that variable's dev value. Sincedev_cataloghas no root default and is only assigned fordev, every other target then fails to load. The guard makes this structurally impossible while keeping restoration intact for variables that resolve everywhere and for edits that land in the current target's own override section.Tests
varRefsAllowed, and both gated restoration paths).acceptance/bundle/config-remote-sync/cross_target_variablescovering: Add restored to a root-default variable, Add restored to a variable assigned in every target, Add of a dev-only match staying hardcoded, Replace fallback to a dev-only variable staying hardcoded, and a Replace insidetargets.devstill restoring the dev-only variable. Before the fix, the first run of this test showed${var.dev_catalog}written into the sharedresourcessection in both leak paths.config-remote-syncacceptance suite passes locally; the new test plusresolve_variablesandtarget_overridealso pass against a real workspace on both engines (direct and terraform).