core and backend: remove redundant handling of default variable values

Previously we had three different layers all thinking they were
responsible for substituting a default value for an unset root module
variable:
 - the local backend, via logic in backend.ParseVariableValues
 - the context.Plan function (and other similar functions) trying to
   preprocess the input variables using
   terraform.mergeDefaultInputVariableValues .
 - the newer prepareFinalInputVariableValue, which aims to centralize all
   of the variable preparation logic so it can be common to both root and
   child module variables.

The second of these was also trying to handle type constraint checking,
which is also the responsibility of the central function and not something
we need to handle so early.

Only the last of these consistently handles both root and child module
variables, and so is the one we ought to keep. The others are now
redundant and are causing prepareFinalInputVariableValue to get a slightly
corrupted view of the caller's chosen variable values.

To rectify that, here we remove the two redundant layers altogether and
have unset root variables pass through as cty.NilVal all the way to the
central prepareFinalInputVariableValue function, which will then handle
them in a suitable way which properly respects the "nullable" setting.

This commit includes some test changes in the terraform package to make
those tests no longer rely on the mergeDefaultInputVariableValues logic
we've removed, and to instead explicitly set cty.NilVal for all unset
variables to comply with our intended contract for PlanOpts.SetVariables,
and similar. (This is so that we can more easily catch bugs in callers
where they _don't_ correctly handle input variables; it allows us to
distinguish between the caller explicitly marking a variable as unset vs.
not describing it at all, where the latter is a bug in the caller.)

Author: apparentlymart

internal/backend/unparsed_value.go

@@ -164,13 +164,18 @@ func ParseVariableValues(vv map[string]UnparsedVariableValue, decls map[string]*
 
 	// By this point we should've gathered all of the required root module
 	// variables from one of the many possible sources. We'll now populate
-	// any we haven't gathered as their defaults and fail if any of the
-	// missing ones are required.
+	// any we haven't gathered as unset placeholders which Terraform Core
+	// can then react to.
 	for name, vc := range decls {
 		if isDefinedAny(name, ret, undeclared) {
 			continue
 		}
 
+		// This check is redundant with a check made in Terraform Core when
+		// processing undeclared variables, but allows us to generate a more
+		// specific error message which mentions -var and -var-file command
+		// line options, whereas the one in Terraform Core is more general
+		// due to supporting both root and child module variables.
 		if vc.Required() {
 			diags = diags.Append(&hcl.Diagnostic{
 				Severity: hcl.DiagError,
@@ -189,8 +194,14 @@ func ParseVariableValues(vv map[string]UnparsedVariableValue, decls map[string]*
 				SourceRange: tfdiags.SourceRangeFromHCL(vc.DeclRange),
 			}
 		} else {
+			// We're still required to put an entry for this variable
+			// in the mapping to be explicit to Terraform Core that we
+			// visited it, but its value will be cty.NilVal to represent
+			// that it wasn't set at all at this layer, and so Terraform Core
+			// should substitute a default if available, or generate an error
+			// if not.
 			ret[name] = &terraform.InputValue{
-				Value:       vc.Default,
+				Value:       cty.NilVal,
 				SourceType:  terraform.ValueFromConfig,
 				SourceRange: tfdiags.SourceRangeFromHCL(vc.DeclRange),
 			}

internal/backend/unparsed_value_test.go

@@ -204,7 +204,7 @@ func TestUnparsedValue(t *testing.T) {
 				},
 			},
 			"missing2": {
-				Value:      cty.StringVal("default for missing2"),
+				Value:      cty.NilVal, // Terraform Core handles substituting the default
 				SourceType: terraform.ValueFromConfig,
 				SourceRange: tfdiags.SourceRange{
 					Filename: "fake.tf",

internal/command/jsonplan/plan.go

@@ -118,7 +118,7 @@ func Marshal(
 	output := newPlan()
 	output.TerraformVersion = version.String()
 
-	err := output.marshalPlanVariables(p.VariableValues, schemas)
+	err := output.marshalPlanVariables(p.VariableValues, config.Module.Variables)
 	if err != nil {
 		return nil, fmt.Errorf("error in marshalPlanVariables: %s", err)
 	}
@@ -183,11 +183,7 @@ func Marshal(
 	return ret, err
 }
 
-func (p *plan) marshalPlanVariables(vars map[string]plans.DynamicValue, schemas *terraform.Schemas) error {
-	if len(vars) == 0 {
-		return nil
-	}
-
+func (p *plan) marshalPlanVariables(vars map[string]plans.DynamicValue, decls map[string]*configs.Variable) error {
 	p.Variables = make(variables, len(vars))
 
 	for k, v := range vars {
@@ -203,6 +199,41 @@ func (p *plan) marshalPlanVariables(vars map[string]plans.DynamicValue, schemas
 			Value: valJSON,
 		}
 	}
+
+	// In Terraform v1.1 and earlier we had some confusion about which subsystem
+	// of Terraform was the one responsible for substituting in default values
+	// for unset module variables, with root module variables being handled in
+	// three different places while child module variables were only handled
+	// during the Terraform Core graph walk.
+	//
+	// For Terraform v1.2 and later we rationalized that by having the Terraform
+	// Core graph walk always be responsible for selecting defaults regardless
+	// of root vs. child module, but unfortunately our earlier accidental
+	// misbehavior bled out into the public interface by making the defaults
+	// show up in the "vars" map to this function. Those are now correctly
+	// omitted (so that the plan file only records the variables _actually_
+	// set by the caller) but consumers of the JSON plan format may be depending
+	// on our old behavior and so we'll fake it here just in time so that
+	// outside consumers won't see a behavior change.
+	for name, decl := range decls {
+		if _, ok := p.Variables[name]; ok {
+			continue
+		}
+		if val := decl.Default; val != cty.NilVal {
+			valJSON, err := ctyjson.Marshal(val, val.Type())
+			if err != nil {
+				return err
+			}
+			p.Variables[name] = &variable{
+				Value: valJSON,
+			}
+		}
+	}
+
+	if len(p.Variables) == 0 {
+		p.Variables = nil // omit this property if there are no variables to describe
+	}
+
 	return nil
 }
 

internal/terraform/context_apply2_test.go

@@ -426,7 +426,7 @@ resource "test_resource" "b" {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, state, DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	_, diags = ctx.Apply(plan, m)
@@ -530,14 +530,14 @@ resource "test_object" "y" {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags := ctx.Apply(plan, m)
 	assertNoErrors(t, diags)
 
 	// FINAL PLAN:
-	plan, diags = ctx.Plan(m, state, DefaultPlanOpts)
+	plan, diags = ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	// make sure the same marks are compared in the next plan as well

internal/terraform/context_apply_test.go

@@ -517,7 +517,7 @@ func TestContext2Apply_mapVarBetweenModules(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags := ctx.Apply(plan, m)
@@ -2262,7 +2262,7 @@ func TestContext2Apply_countVariable(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags := ctx.Apply(plan, m)
@@ -2288,7 +2288,7 @@ func TestContext2Apply_countVariableRef(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags := ctx.Apply(plan, m)
@@ -2327,7 +2327,7 @@ func TestContext2Apply_provisionerInterpCount(t *testing.T) {
 		Provisioners: provisioners,
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	// We'll marshal and unmarshal the plan here, to ensure that we have
@@ -3682,7 +3682,7 @@ func TestContext2Apply_multiVarOrder(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags := ctx.Apply(plan, m)
@@ -3713,7 +3713,7 @@ func TestContext2Apply_multiVarOrderInterp(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags := ctx.Apply(plan, m)
@@ -4704,9 +4704,7 @@ func TestContext2Apply_provisionerDestroy(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, state, &PlanOpts{
-		Mode: plans.DestroyMode,
-	})
+	plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.DestroyMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags = ctx.Apply(plan, m)
@@ -4753,9 +4751,7 @@ func TestContext2Apply_provisionerDestroyFail(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, state, &PlanOpts{
-		Mode: plans.DestroyMode,
-	})
+	plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.DestroyMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags = ctx.Apply(plan, m)
@@ -5908,7 +5904,7 @@ func TestContext2Apply_destroyWithModuleVariableAndCountNested(t *testing.T) {
 		})
 
 		// First plan and apply a create operation
-		plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+		plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 		assertNoErrors(t, diags)
 
 		state, diags = ctx.Apply(plan, m)
@@ -5929,9 +5925,7 @@ func TestContext2Apply_destroyWithModuleVariableAndCountNested(t *testing.T) {
 		})
 
 		// First plan and apply a create operation
-		plan, diags := ctx.Plan(m, state, &PlanOpts{
-			Mode: plans.DestroyMode,
-		})
+		plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.DestroyMode, testInputValuesUnset(m.Module.Variables)))
 		if diags.HasErrors() {
 			t.Fatalf("destroy plan err: %s", diags.Err())
 		}
@@ -7561,6 +7555,12 @@ func TestContext2Apply_vars(t *testing.T) {
 			Value:      cty.StringVal("us-east-1"),
 			SourceType: ValueFromCaller,
 		},
+		"bar": &InputValue{
+			// This one is not explicitly set but that's okay because it
+			// has a declared default, which Terraform Core will use instead.
+			Value:      cty.NilVal,
+			SourceType: ValueFromCaller,
+		},
 		"test_list": &InputValue{
 			Value: cty.ListVal([]cty.Value{
 				cty.StringVal("Hello"),
@@ -7876,7 +7876,7 @@ func TestContext2Apply_issue7824(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("err: %s", diags.Err())
 	}
@@ -7932,7 +7932,7 @@ func TestContext2Apply_issue5254(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("err: %s", diags.Err())
 	}
@@ -7951,7 +7951,7 @@ func TestContext2Apply_issue5254(t *testing.T) {
 		},
 	})
 
-	plan, diags = ctx.Plan(m, state, DefaultPlanOpts)
+	plan, diags = ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("err: %s", diags.Err())
 	}
@@ -8845,7 +8845,7 @@ func TestContext2Apply_plannedInterpolatedCount(t *testing.T) {
 		Providers: Providers,
 	})
 
-	plan, diags := ctx.Plan(m, state, DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("plan failed: %s", diags.Err())
 	}
@@ -8904,9 +8904,7 @@ func TestContext2Apply_plannedDestroyInterpolatedCount(t *testing.T) {
 		Providers: providers,
 	})
 
-	plan, diags := ctx.Plan(m, state, &PlanOpts{
-		Mode: plans.DestroyMode,
-	})
+	plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.DestroyMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("plan failed: %s", diags.Err())
 	}
@@ -9674,7 +9672,7 @@ func TestContext2Apply_plannedConnectionRefs(t *testing.T) {
 		Hooks:        []Hook{hook},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	diags.HasErrors()
 	if diags.HasErrors() {
 		t.Fatalf("diags: %s", diags.Err())
@@ -11687,7 +11685,7 @@ resource "test_resource" "foo" {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags := ctx.Apply(plan, m)
@@ -11702,7 +11700,7 @@ resource "test_resource" "foo" {
 		},
 	})
 
-	plan, diags = ctx.Plan(m, state, DefaultPlanOpts)
+	plan, diags = ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	state, diags = ctx.Apply(plan, m)
@@ -11720,6 +11718,7 @@ resource "test_resource" "foo" {
 	plan, diags = ctx.Plan(m, state, &PlanOpts{
 		Mode: plans.NormalMode,
 		SetVariables: InputValues{
+			"sensitive_id": &InputValue{Value: cty.NilVal},
 			"sensitive_var": &InputValue{
 				Value: cty.StringVal("bar"),
 			},
@@ -11759,7 +11758,7 @@ resource "test_resource" "foo" {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("plan errors: %s", diags.Err())
 	}
@@ -11904,7 +11903,7 @@ resource "test_resource" "foo" {
 		)
 	})
 
-	plan, diags := ctx.Plan(m, state, DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 
 	addr := mustResourceInstanceAddr("test_resource.foo")
@@ -11954,7 +11953,7 @@ resource "test_resource" "foo" {
 	// but this seems rather suspicious and we should ideally figure out what
 	// this test was originally intending to do and make it do that.
 	oldPlan := plan
-	_, diags = ctx2.Plan(m2, state, DefaultPlanOpts)
+	_, diags = ctx2.Plan(m2, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	assertNoErrors(t, diags)
 	stateWithoutSensitive, diags := ctx.Apply(oldPlan, m)
 	assertNoErrors(t, diags)
@@ -12206,7 +12205,7 @@ func TestContext2Apply_dataSensitive(t *testing.T) {
 		},
 	})
 
-	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
 	if diags.HasErrors() {
 		t.Fatalf("diags: %s", diags.Err())
 	} else {

internal/terraform/context_eval.go

@@ -45,7 +45,7 @@ func (c *Context) Eval(config *configs.Config, state *states.State, moduleAddr a
 	state = state.DeepCopy()
 	var walker *ContextGraphWalker
 
-	variables := mergeDefaultInputVariableValues(opts.SetVariables, config.Module.Variables)
+	variables := opts.SetVariables
 
 	// By the time we get here, we should have values defined for all of
 	// the root module variables, even if some of them are "unknown". It's the

internal/terraform/context_eval_test.go

@@ -54,7 +54,9 @@ func TestContextEval(t *testing.T) {
 		},
 	})
 
-	scope, diags := ctx.Eval(m, states.NewState(), addrs.RootModuleInstance, &EvalOpts{})
+	scope, diags := ctx.Eval(m, states.NewState(), addrs.RootModuleInstance, &EvalOpts{
+		SetVariables: testInputValuesUnset(m.Module.Variables),
+	})
 	if diags.HasErrors() {
 		t.Fatalf("Eval errors: %s", diags.Err())
 	}

internal/terraform/context_import.go

@@ -53,7 +53,7 @@ func (c *Context) Import(config *configs.Config, prevRunState *states.State, opt
 
 	log.Printf("[DEBUG] Building and walking import graph")
 
-	variables := mergeDefaultInputVariableValues(opts.SetVariables, config.Module.Variables)
+	variables := opts.SetVariables
 
 	// Initialize our graph builder
 	builder := &ImportGraphBuilder{