undeclared variables must be allowed during apply

Plan allows undeclared config variables with only warnings, and that behavior need to be carried forward to apply now that ephemeral variables can be passed in at apply time.
hashicorp · Nov 18, 2024 · c1f8152 · c1f8152
1 parent bf64925
commit c1f8152
Show file tree

Hide file tree

Showing 2 changed files with 136 additions and 73 deletions.
diff --git a/internal/backend/local/backend_apply.go b/internal/backend/local/backend_apply.go
@@ -252,7 +252,22 @@ func (b *Local) opApply(
 	}
 
 	if len(op.Variables) != 0 {
+		// Undeclared variables cause warnings during plan, but will show up
+		// again here during apply. Their handling is tricky though, because it
+		// depends on how they were declared, and is subject to compatibility
+		// constraints. Collect any suspect values as we go, and then use the
+		// same parsing logic from the plan to generate the diagnostics.
+		undeclaredVariables := map[string]backendrun.UnparsedVariableValue{}
+
 		for varName, rawV := range op.Variables {
+			decl, ok := lr.Config.Module.Variables[varName]
+			if !ok {
+				// 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
@@ -269,25 +284,6 @@ func (b *Local) opApply(
 				rng = v.SourceRange.ToHCL().Ptr()
 			}
 
-			decl, ok := lr.Config.Module.Variables[varName]
-
-			// If the variable isn't declared in config at all, take
-			// this opportunity to give the user a helpful error,
-			// rather than waiting for a less helpful one later.
-			// We are ok with over-supplying variables through environment variables
-			// since it would be a breaking change to disallow it.
-			if v.SourceType == terraform.ValueFromCLIArg || v.SourceType == terraform.ValueFromNamedFile {
-				if !ok {
-					diags = diags.Append(&hcl.Diagnostic{
-						Severity: hcl.DiagError,
-						Summary:  "Value for undeclared variable",
-						Detail:   fmt.Sprintf("The variable %s cannot be set using the -var and -var-file options because it is not declared in configuration.", varName),
-						Subject:  rng,
-					})
-					continue
-				}
-			}
-
 			// If the var is declared as ephemeral in config, go ahead and handle it
 			if ok && decl.Ephemeral {
 				// Determine whether this is an apply-time variable, i.e. an
@@ -345,12 +341,8 @@ func (b *Local) opApply(
 				// If a non-ephemeral variable is set differently between plan and apply, we should emit a diagnostic.
 				plannedVariableValue, ok := plan.VariableValues[varName]
 				if !ok {
-					diags = diags.Append(&hcl.Diagnostic{
-						Severity: hcl.DiagError,
-						Summary:  "Can't set 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 it is neither ephemeral nor has it been declared during the plan operation. To declare an ephemeral variable which is not saved in the plan file, use ephemeral = true.", varName),
-						Subject:  rng,
-					})
+					// We'll catch this with ParseUndeclaredVariableValues after
+					undeclaredVariables[varName] = rawV
 					continue
 				}
 
@@ -373,7 +365,15 @@ func (b *Local) opApply(
 					}
 				}
 			}
+
+		}
+		_, undeclaredDiags := backendrun.ParseUndeclaredVariableValues(undeclaredVariables, map[string]*configs.Variable{})
+		// always add hard errors here, and add warnings if we're not in a
+		// combined op which just emitted those same warnings already.
+		if undeclaredDiags.HasErrors() || !combinedPlanApply {
+			diags = diags.Append(undeclaredDiags)
 		}
+
 		if diags.HasErrors() {
 			op.ReportResult(runningOp, diags)
 			return

diff --git a/internal/command/apply_test.go b/internal/command/apply_test.go
@@ -906,59 +906,14 @@ func TestApply_planWithVarFile(t *testing.T) {
 	}
 }
 
-func TestApply_planWithVarFilePreviouslyUnset(t *testing.T) {
-	varFileDir := testTempDir(t)
-	varFilePath := filepath.Join(varFileDir, "terraform.tfvars")
-	if err := ioutil.WriteFile(varFilePath, []byte(applyVarFile), 0644); err != nil {
-		t.Fatalf("err: %s", err)
-	}
-
-	// The value of foo is not set
-	planPath := applyFixturePlanFile(t)
-	statePath := testTempFile(t)
-
-	cwd, err := os.Getwd()
-	if err != nil {
-		t.Fatalf("err: %s", err)
-	}
-	if err := os.Chdir(varFileDir); err != nil {
-		t.Fatalf("err: %s", err)
-	}
-	defer os.Chdir(cwd)
-
-	p := applyFixtureProvider()
-	view, done := testView(t)
-	c := &ApplyCommand{
-		Meta: Meta{
-			testingOverrides: metaOverridesForProvider(p),
-			View:             view,
-		},
-	}
-
-	args := []string{
-		"-state-out", statePath,
-		planPath,
-	}
-	code := c.Run(args)
-	output := done(t)
-	if code == 0 {
-		t.Fatalf("expected to fail, but succeeded. \n\n%s", output.All())
-	}
-
-	expectedTitle := "Can't set variable when applying a saved plan"
-	if !strings.Contains(output.Stderr(), expectedTitle) {
-		t.Fatalf("Expected stderr to contain %q, got %q", expectedTitle, output.Stderr())
-	}
-}
-
 func TestApply_planWithVarFileChangingVariableValue(t *testing.T) {
 	varFileDir := testTempDir(t)
 	varFilePath := filepath.Join(varFileDir, "terraform.tfvars")
-	if err := ioutil.WriteFile(varFilePath, []byte(applyVarFile), 0644); err != nil {
+	if err := os.WriteFile(varFilePath, []byte(applyVarFile), 0644); err != nil {
 		t.Fatalf("err: %s", err)
 	}
 
-	// The value of foo is differnet from the var file
+	// The value of foo is different from the var file
 	planPath := applyFixturePlanFileWithVariableValue(t, "lorem ipsum")
 	statePath := testTempFile(t)
 
@@ -1289,6 +1244,114 @@ foo = "bar"
 	}
 }
 
+// Variables can be passed to apply now for ephemeral usage, but we need to
+// ensure that the legacy handling of undeclared variables remains intact
+func TestApply_changedVars_applyTime(t *testing.T) {
+	t.Run("undeclared-config-var", func(t *testing.T) {
+		// an undeclared config variable is a warning, just like during plan
+		varFileDir := testTempDir(t)
+		varFilePath := filepath.Join(varFileDir, "terraform.tfvars")
+		if err := os.WriteFile(varFilePath, []byte(`undeclared = true`), 0644); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		// The value of foo is not set
+		planPath := applyFixturePlanFile(t)
+		statePath := testTempFile(t)
+
+		cwd, err := os.Getwd()
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+		if err := os.Chdir(varFileDir); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+		defer os.Chdir(cwd)
+
+		p := applyFixtureProvider()
+		view, done := testView(t)
+		c := &ApplyCommand{
+			Meta: Meta{
+				testingOverrides: metaOverridesForProvider(p),
+				View:             view,
+			},
+		}
+
+		args := []string{
+			"-state-out", statePath,
+			planPath,
+		}
+		code := c.Run(args)
+		output := done(t)
+		if code != 0 {
+			t.Fatalf("unexpected exit code %d:\n\n%s", code, output.All())
+		}
+
+		if !strings.Contains(output.All(), `Value for undeclared variable`) {
+			t.Fatalf("missing undeclared warning:\n%s", output.All())
+		}
+	})
+
+	t.Run("undeclared-cli-var", func(t *testing.T) {
+		// an undeclared cli variable is an error, just like during plan
+		planPath := applyFixturePlanFile(t)
+		statePath := testTempFile(t)
+
+		p := applyFixtureProvider()
+		view, done := testView(t)
+		c := &ApplyCommand{
+			Meta: Meta{
+				testingOverrides: metaOverridesForProvider(p),
+				View:             view,
+			},
+		}
+
+		args := []string{
+			"-var", "undeclared=true",
+			"-state-out", statePath,
+			planPath,
+		}
+		code := c.Run(args)
+		output := done(t)
+		if code != 1 {
+			t.Fatalf("unexpected exit code %d:\n\n%s", code, output.All())
+		}
+
+		if !strings.Contains(output.Stderr(), `Value for undeclared variable`) {
+			t.Fatalf("missing undeclared warning:\n%s", output.All())
+		}
+	})
+
+	t.Run("changed-cli-var", func(t *testing.T) {
+		planPath := applyFixturePlanFileWithVariableValue(t, "orig")
+		statePath := testTempFile(t)
+
+		p := applyFixtureProvider()
+		view, done := testView(t)
+		c := &ApplyCommand{
+			Meta: Meta{
+				testingOverrides: metaOverridesForProvider(p),
+				View:             view,
+			},
+		}
+
+		args := []string{
+			"-var", "foo=new",
+			"-state-out", statePath,
+			planPath,
+		}
+		code := c.Run(args)
+		output := done(t)
+		if code != 1 {
+			t.Fatalf("unexpected exit code %d:\n\n%s", code, output.All())
+		}
+
+		if !strings.Contains(output.Stderr(), `Can't change variable when applying a saved plan`) {
+			t.Fatalf("missing undeclared warning:\n%s", output.All())
+		}
+	})
+}
+
 // we should be able to apply a plan file with no other file dependencies
 func TestApply_planNoModuleFiles(t *testing.T) {
 	// temporary data directory which we can remove between commands
@@ -2595,7 +2658,7 @@ func applyFixturePlanFileMatchState(t *testing.T, stateMeta statemgr.SnapshotMet
 // a single change to create the test_instance.foo and a variable value that is included in the
 // "apply" test fixture, returning the location of that plan file.
 func applyFixturePlanFileWithVariableValue(t *testing.T, value string) string {
-	_, snap := testModuleWithSnapshot(t, "apply")
+	_, snap := testModuleWithSnapshot(t, "apply-vars")
 	plannedVal := cty.ObjectVal(map[string]cty.Value{
 		"id":  cty.UnknownVal(cty.String),
 		"ami": cty.StringVal("bar"),