From 0027d16705efe81dc2aca5bc4fb36295320873b9 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 27 Nov 2024 12:57:57 -0500 Subject: [PATCH 1/2] correcly parse env variables during apply Input variables taken from the environment have no type information, so are initially stored as strings. When we compare those for validation during apply, we have to make sure to use the same parsing logic used in the plan in order to compare them with the stored variables. --- internal/backend/local/backend_apply.go | 47 +++++++++---------------- 1 file changed, 16 insertions(+), 31 deletions(-) diff --git a/internal/backend/local/backend_apply.go b/internal/backend/local/backend_apply.go index 142369dd91..e52e7b0d46 100644 --- a/internal/backend/local/backend_apply.go +++ b/internal/backend/local/backend_apply.go @@ -259,33 +259,26 @@ func (b *Local) opApply( // same parsing logic from the plan to generate the diagnostics. undeclaredVariables := map[string]backendrun.UnparsedVariableValue{} - for varName, rawV := range op.Variables { + parsedVars, _ := backendrun.ParseVariableValues(op.Variables, lr.Config.Module.Variables) + + for varName := range op.Variables { + parsedVar, parsed := parsedVars[varName] + decl, ok := lr.Config.Module.Variables[varName] - if !ok { + if !ok || !parsed { // We'll try to parse this and handle diagnostics for missing // variables with ParseUndeclaredVariableValues after. - undeclaredVariables[varName] = rawV - continue - } - - // We're "parsing" only to get the resulting value's SourceType, - // so we'll use configs.VariableParseLiteral just because it's - // the most liberal interpretation and so least likely to - // fail with an unrelated error. - v, _ := rawV.ParseVariableValue(configs.VariableParseLiteral) - if v == nil { - // We'll ignore any that don't parse at all, because - // they'll fail elsewhere in this process anyway. + undeclaredVariables[varName] = op.Variables[varName] continue } var rng *hcl.Range - if v.HasSourceRange() { - rng = v.SourceRange.ToHCL().Ptr() + if parsedVar.HasSourceRange() { + rng = parsedVar.SourceRange.ToHCL().Ptr() } // If the var is declared as ephemeral in config, go ahead and handle it - if ok && decl.Ephemeral { + if decl.Ephemeral { // Determine whether this is an apply-time variable, i.e. an // ephemeral variable that was set (non-null) during the // planning phase. @@ -311,17 +304,9 @@ func (b *Local) opApply( continue } - // Get the value of the variable, because we'll need it for - // the next two steps. - val, valDiags := rawV.ParseVariableValue(decl.ParsingMode) - diags = diags.Append(valDiags) - if valDiags.HasErrors() { - continue - } - // If this is an apply-time variable, the user must supply a // value during apply: it can't be null. - if applyTimeVar && val.Value.IsNull() { + if applyTimeVar && parsedVar.Value.IsNull() { diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Ephemeral variable must be set for apply", @@ -336,17 +321,17 @@ func (b *Local) opApply( // If we get here, we are in possession of a non-null // ephemeral apply-time input variable, and need only pass // its value on to the ApplyOpts. - applyTimeValues[varName] = val + applyTimeValues[varName] = parsedVar } else { // If a non-ephemeral variable is set differently between plan and apply, we should emit a diagnostic. plannedVariableValue, ok := plan.VariableValues[varName] if !ok { // We'll catch this with ParseUndeclaredVariableValues after - undeclaredVariables[varName] = rawV + undeclaredVariables[varName] = op.Variables[varName] continue } - val, err := plannedVariableValue.Decode(cty.DynamicPseudoType) + plannedVar, err := plannedVariableValue.Decode(cty.DynamicPseudoType) if err != nil { diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, @@ -355,11 +340,11 @@ func (b *Local) opApply( Subject: rng, }) } else { - if v.Value.Equals(val).False() { + if parsedVar.Value.Equals(plannedVar).False() { diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Can't change variable when applying a saved plan", - Detail: fmt.Sprintf("The variable %s cannot be set using the -var and -var-file options when applying a saved plan file, because a saved plan includes the variable values that were set when it was created. The saved plan specifies %s as the value whereas during apply the value %s was %s. To declare an ephemeral variable which is not saved in the plan file, use ephemeral = true.", varName, viewsjson.CompactValueStr(v.Value), viewsjson.CompactValueStr(val), v.SourceType.DiagnosticLabel()), + Detail: fmt.Sprintf("The variable %s cannot be set using the -var and -var-file options when applying a saved plan file, because a saved plan includes the variable values that were set when it was created. The saved plan specifies %s as the value whereas during apply the value %s was %s. To declare an ephemeral variable which is not saved in the plan file, use ephemeral = true.", varName, viewsjson.CompactValueStr(parsedVar.Value), viewsjson.CompactValueStr(plannedVar), parsedVar.SourceType.DiagnosticLabel()), Subject: rng, }) } From 4883499a8931931b71bfc4fbc088e1c2aecfec1f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 27 Nov 2024 13:00:53 -0500 Subject: [PATCH 2/2] test non-string env var input variables Non-string input variables taken from the environment initially need to be parsed and stored as string, since there is no type associated type information. Make sure these are correctly handled when validated during apply. --- internal/command/apply_test.go | 17 +++++------------ .../testdata/apply-ephemeral-variable/main.tf | 5 +++++ 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/internal/command/apply_test.go b/internal/command/apply_test.go index 89449163a7..14bac07a54 100644 --- a/internal/command/apply_test.go +++ b/internal/command/apply_test.go @@ -964,16 +964,10 @@ func TestApply_planWithVarFileChangingVariableValue(t *testing.T) { } } -func TestApply_planVars(t *testing.T) { - // This test ensures that it isn't allowed to set non-ephemeral input - // variables when applying from a saved plan file, since in that case the - // variable values come from the saved plan file. - // - // This situation was originally checked by the apply command itself, - // and that's what this test was originally exercising. This rule - // is now enforced by the "local" backend instead, but this test - // is still valid since the command instance delegates to the - // local backend. +func TestApply_planUndeclaredVars(t *testing.T) { + // This test ensures that it isn't allowed to set undeclared input variables + // when applying from a saved plan file, since in that case the variable + // values come from the saved plan file. planPath := applyFixturePlanFile(t) statePath := testTempFile(t) @@ -1076,7 +1070,6 @@ foo = "bar" "with planfile passing ephemeral variable through environment variable": func(t *testing.T, c *ApplyCommand, statePath, planPath string, done func(*testing.T) *terminal.TestOutput) { t.Setenv("TF_VAR_foo", "bar") - defer t.Setenv("TF_VAR_foo", "") args := []string{ "-state", statePath, @@ -1168,7 +1161,7 @@ foo = "bar" "without planfile passing ephemeral variable through environment variable": func(t *testing.T, c *ApplyCommand, statePath, planPath string, done func(*testing.T) *terminal.TestOutput) { t.Setenv("TF_VAR_foo", "bar") - defer t.Setenv("TF_VAR_foo", "") + t.Setenv("TF_VAR_unused", `{key:"val"}`) args := []string{ "-state", statePath, diff --git a/internal/command/testdata/apply-ephemeral-variable/main.tf b/internal/command/testdata/apply-ephemeral-variable/main.tf index f1b83b2052..7961a3b9a1 100644 --- a/internal/command/testdata/apply-ephemeral-variable/main.tf +++ b/internal/command/testdata/apply-ephemeral-variable/main.tf @@ -9,6 +9,11 @@ variable "bar" { ephemeral = true } +variable "unused" { + type = map(string) + default = null +} + resource "test_instance" "foo" { ami = "bar" }