Address a few inconsistencies in how we process root module input variables vs. called module input variables

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_apply.go

@@ -30,30 +30,11 @@ func (c *Context) Apply(plan *plans.Plan, config *configs.Config) (*states.State
 		return nil, diags
 	}
 
-	variables := InputValues{}
-	for name, dyVal := range plan.VariableValues {
-		val, err := dyVal.Decode(cty.DynamicPseudoType)
-		if err != nil {
-			diags = diags.Append(tfdiags.Sourceless(
-				tfdiags.Error,
-				"Invalid variable value in plan",
-				fmt.Sprintf("Invalid value for variable %q recorded in plan file: %s.", name, err),
-			))
-			continue
-		}
-
-		variables[name] = &InputValue{
-			Value:      val,
-			SourceType: ValueFromPlan,
-		}
-	}
-
 	workingState := plan.PriorState.DeepCopy()
 	walker, walkDiags := c.walk(graph, operation, &graphWalkOpts{
-		Config:             config,
-		InputState:         workingState,
-		Changes:            plan.Changes,
-		RootVariableValues: variables,
+		Config:     config,
+		InputState: workingState,
+		Changes:    plan.Changes,
 	})
 	diags = diags.Append(walker.NonFatalDiagnostics)
 	diags = diags.Append(walkDiags)
@@ -83,15 +64,58 @@ Note that the -target option is not suitable for routine use, and is provided on
 }
 
 func (c *Context) applyGraph(plan *plans.Plan, config *configs.Config, validate bool) (*Graph, walkOperation, tfdiags.Diagnostics) {
-	graph, diags := (&ApplyGraphBuilder{
-		Config:       config,
-		Changes:      plan.Changes,
-		State:        plan.PriorState,
-		Plugins:      c.plugins,
-		Targets:      plan.TargetAddrs,
-		ForceReplace: plan.ForceReplaceAddrs,
-		Validate:     validate,
+	var diags tfdiags.Diagnostics
+
+	variables := InputValues{}
+	for name, dyVal := range plan.VariableValues {
+		val, err := dyVal.Decode(cty.DynamicPseudoType)
+		if err != nil {
+			diags = diags.Append(tfdiags.Sourceless(
+				tfdiags.Error,
+				"Invalid variable value in plan",
+				fmt.Sprintf("Invalid value for variable %q recorded in plan file: %s.", name, err),
+			))
+			continue
+		}
+
+		variables[name] = &InputValue{
+			Value:      val,
+			SourceType: ValueFromPlan,
+		}
+	}
+	if diags.HasErrors() {
+		return nil, walkApply, diags
+	}
+
+	// The plan.VariableValues field only records variables that were actually
+	// set by the caller in the PlanOpts, so we may need to provide
+	// placeholders for any other variables that the user didn't set, in
+	// which case Terraform will once again use the default value from the
+	// configuration when we visit these variables during the graph walk.
+	for name := range config.Module.Variables {
+		if _, ok := variables[name]; ok {
+			continue
+		}
+		variables[name] = &InputValue{
+			Value:      cty.NilVal,
+			SourceType: ValueFromPlan,
+		}
+	}
+
+	graph, moreDiags := (&ApplyGraphBuilder{
+		Config:             config,
+		Changes:            plan.Changes,
+		State:              plan.PriorState,
+		RootVariableValues: variables,
+		Plugins:            c.plugins,
+		Targets:            plan.TargetAddrs,
+		ForceReplace:       plan.ForceReplaceAddrs,
+		Validate:           validate,
 	}).Build(addrs.RootModuleInstance)
+	diags = diags.Append(moreDiags)
+	if moreDiags.HasErrors() {
+		return nil, walkApply, diags
+	}
 
 	operation := walkApply
 	if plan.UIMode == plans.DestroyMode {

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