From 8e58f59e62b44841dbdaa59ccfc85cc0d0a8d93b Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 2 Dec 2021 17:41:06 -0800 Subject: [PATCH 1/4] core: Remove TestContext2Validate_PlanGraphBuilder This test seems to be a holdover from the many-moons-ago switch from one graph for all operations to separate graphs for plan and apply. It is effectively just a copy of a subset of the content of the Context.Validate function and is a maintainability hazard because it tends to lag behind updates to that function unless changes there happen to make it fail. This test doesn't cover anything that the other validate context tests don't exercise as an implementation detail of calling Context.Validate, so I've just removed it with no replacement. --- internal/terraform/context_validate_test.go | 26 --------------------- 1 file changed, 26 deletions(-) diff --git a/internal/terraform/context_validate_test.go b/internal/terraform/context_validate_test.go index 1ed5ad4253f2..a02d85cdd9f2 100644 --- a/internal/terraform/context_validate_test.go +++ b/internal/terraform/context_validate_test.go @@ -1187,32 +1187,6 @@ resource "aws_instance" "foo" { } } -// Manually validate using the new PlanGraphBuilder -func TestContext2Validate_PlanGraphBuilder(t *testing.T) { - fixture := contextFixtureApplyVars(t) - opts := fixture.ContextOpts() - c := testContext2(t, opts) - - graph, diags := ValidateGraphBuilder(&PlanGraphBuilder{ - Config: fixture.Config, - State: states.NewState(), - Plugins: c.plugins, - }).Build(addrs.RootModuleInstance) - if diags.HasErrors() { - t.Fatalf("errors from PlanGraphBuilder: %s", diags.Err()) - } - defer c.acquireRun("validate-test")() - walker, diags := c.walk(graph, walkValidate, &graphWalkOpts{ - Config: fixture.Config, - }) - if diags.HasErrors() { - t.Fatal(diags.Err()) - } - if len(walker.NonFatalDiagnostics) > 0 { - t.Fatal(walker.NonFatalDiagnostics.Err()) - } -} - func TestContext2Validate_invalidOutput(t *testing.T) { m := testModuleInline(t, map[string]string{ "main.tf": ` From 72bff83406d521ae60989181c5ceb463caacb9d2 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 10 Nov 2021 17:29:45 -0800 Subject: [PATCH 2/4] core: Handle root and child module input variables consistently Previously we had a significant discrepancy between these two situations: we wrote the raw root module variables directly into the EvalContext and then applied type conversions only at expression evaluation time, while for child modules we converted and validated the values while visiting the variable graph node and wrote only the _final_ value into the EvalContext. This confusion seems to have been the root cause for #29899, where validation rules for root module variables were being applied at the wrong point in the process, prior to type conversion. To fix that bug and also make similar mistakes less likely in the future, I've made the root module variable handling more like the child module variable handling in the following ways: - The "raw value" (exactly as given by the user) lives only in the graph node representing the variable, which mirrors how the _expression_ for a child module variable lives in its graph node. This means that the flow for the two is the same except that there's no expression evaluation step for root module variables, because they arrive as constant values from the caller. - The set of variable values in the EvalContext is always only "final" values, after type conversion is complete. That in turn means we no longer need to do "just in time" conversion in evaluationStateData.GetInputVariable, and can just return the value exactly as stored, which is consistent with how we handle all other references between objects. This diff is noisier than I'd like because of how much it takes to wire a new argument (the raw variable values) through to the plan graph builder, but those changes are pretty mechanical and the interesting logic lives inside the plan graph builder itself, in NodeRootVariable, and the shared helper functions in eval_variable.go. While here I also took the opportunity to fix a historical API wart in EvalContext, where SetModuleCallArguments was built to take a set of variable values all at once but our current caller always calls with only one at a time. That is now just SetModuleCallArgument singular, to match with the new SetRootModuleArgument to deal with root module variables. --- internal/terraform/context_apply.go | 69 +-- internal/terraform/context_eval.go | 12 +- internal/terraform/context_import.go | 16 +- internal/terraform/context_plan.go | 74 +-- internal/terraform/context_validate.go | 24 +- internal/terraform/context_walk.go | 24 +- internal/terraform/eval_context.go | 22 +- internal/terraform/eval_context_builtin.go | 29 +- internal/terraform/eval_context_mock.go | 38 +- internal/terraform/eval_variable.go | 107 ++++- internal/terraform/eval_variable_test.go | 426 ++++++++++++++++++ internal/terraform/evaluate.go | 40 +- internal/terraform/graph_builder_apply.go | 7 +- .../terraform/graph_builder_destroy_plan.go | 5 + internal/terraform/graph_builder_eval.go | 7 +- internal/terraform/graph_builder_import.go | 7 +- internal/terraform/graph_builder_plan.go | 7 +- internal/terraform/node_module_variable.go | 129 ++---- internal/terraform/node_root_variable.go | 59 ++- internal/terraform/node_root_variable_test.go | 166 ++++++- internal/terraform/transform_variable.go | 5 +- 21 files changed, 1014 insertions(+), 259 deletions(-) create mode 100644 internal/terraform/eval_variable_test.go diff --git a/internal/terraform/context_apply.go b/internal/terraform/context_apply.go index e5d2702bc5dc..42520b03dd83 100644 --- a/internal/terraform/context_apply.go +++ b/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,43 @@ 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 + } + + 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 { diff --git a/internal/terraform/context_eval.go b/internal/terraform/context_eval.go index efc24767c205..c6af77635fc3 100644 --- a/internal/terraform/context_eval.go +++ b/internal/terraform/context_eval.go @@ -60,9 +60,10 @@ func (c *Context) Eval(config *configs.Config, state *states.State, moduleAddr a log.Printf("[DEBUG] Building and walking 'eval' graph") graph, moreDiags := (&EvalGraphBuilder{ - Config: config, - State: state, - Plugins: c.plugins, + Config: config, + State: state, + RootVariableValues: variables, + Plugins: c.plugins, }).Build(addrs.RootModuleInstance) diags = diags.Append(moreDiags) if moreDiags.HasErrors() { @@ -70,9 +71,8 @@ func (c *Context) Eval(config *configs.Config, state *states.State, moduleAddr a } walkOpts := &graphWalkOpts{ - InputState: state, - Config: config, - RootVariableValues: variables, + InputState: state, + Config: config, } walker, moreDiags = c.walk(graph, walkEval, walkOpts) diff --git a/internal/terraform/context_import.go b/internal/terraform/context_import.go index af17cbd62dc8..b5417405fbb0 100644 --- a/internal/terraform/context_import.go +++ b/internal/terraform/context_import.go @@ -53,11 +53,14 @@ 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) + // Initialize our graph builder builder := &ImportGraphBuilder{ - ImportTargets: opts.Targets, - Config: config, - Plugins: c.plugins, + ImportTargets: opts.Targets, + Config: config, + RootVariableValues: variables, + Plugins: c.plugins, } // Build the graph @@ -67,13 +70,10 @@ func (c *Context) Import(config *configs.Config, prevRunState *states.State, opt return state, diags } - variables := mergeDefaultInputVariableValues(opts.SetVariables, config.Module.Variables) - // Walk it walker, walkDiags := c.walk(graph, walkImport, &graphWalkOpts{ - Config: config, - InputState: state, - RootVariableValues: variables, + Config: config, + InputState: state, }) diags = diags.Append(walkDiags) if walkDiags.HasErrors() { diff --git a/internal/terraform/context_plan.go b/internal/terraform/context_plan.go index 3f860ef1bd1f..0b3c97f14559 100644 --- a/internal/terraform/context_plan.go +++ b/internal/terraform/context_plan.go @@ -125,11 +125,11 @@ The -target option is not for routine use, and is provided only for exceptional var planDiags tfdiags.Diagnostics switch opts.Mode { case plans.NormalMode: - plan, planDiags = c.plan(config, prevRunState, variables, opts) + plan, planDiags = c.plan(config, prevRunState, opts) case plans.DestroyMode: - plan, planDiags = c.destroyPlan(config, prevRunState, variables, opts) + plan, planDiags = c.destroyPlan(config, prevRunState, opts) case plans.RefreshOnlyMode: - plan, planDiags = c.refreshOnlyPlan(config, prevRunState, variables, opts) + plan, planDiags = c.refreshOnlyPlan(config, prevRunState, opts) default: panic(fmt.Sprintf("unsupported plan mode %s", opts.Mode)) } @@ -172,14 +172,14 @@ var DefaultPlanOpts = &PlanOpts{ Mode: plans.NormalMode, } -func (c *Context) plan(config *configs.Config, prevRunState *states.State, rootVariables InputValues, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) { +func (c *Context) plan(config *configs.Config, prevRunState *states.State, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics if opts.Mode != plans.NormalMode { panic(fmt.Sprintf("called Context.plan with %s", opts.Mode)) } - plan, walkDiags := c.planWalk(config, prevRunState, rootVariables, opts) + plan, walkDiags := c.planWalk(config, prevRunState, opts) diags = diags.Append(walkDiags) if diags.HasErrors() { return nil, diags @@ -194,14 +194,14 @@ func (c *Context) plan(config *configs.Config, prevRunState *states.State, rootV return plan, diags } -func (c *Context) refreshOnlyPlan(config *configs.Config, prevRunState *states.State, rootVariables InputValues, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) { +func (c *Context) refreshOnlyPlan(config *configs.Config, prevRunState *states.State, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics if opts.Mode != plans.RefreshOnlyMode { panic(fmt.Sprintf("called Context.refreshOnlyPlan with %s", opts.Mode)) } - plan, walkDiags := c.planWalk(config, prevRunState, rootVariables, opts) + plan, walkDiags := c.planWalk(config, prevRunState, opts) diags = diags.Append(walkDiags) if diags.HasErrors() { return nil, diags @@ -235,7 +235,7 @@ func (c *Context) refreshOnlyPlan(config *configs.Config, prevRunState *states.S return plan, diags } -func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State, rootVariables InputValues, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) { +func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics pendingPlan := &plans.Plan{} @@ -260,7 +260,7 @@ func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State log.Printf("[TRACE] Context.destroyPlan: calling Context.plan to get the effect of refreshing the prior state") normalOpts := *opts normalOpts.Mode = plans.NormalMode - refreshPlan, refreshDiags := c.plan(config, prevRunState, rootVariables, &normalOpts) + refreshPlan, refreshDiags := c.plan(config, prevRunState, &normalOpts) if refreshDiags.HasErrors() { // NOTE: Normally we'd append diagnostics regardless of whether // there are errors, just in case there are warnings we'd want to @@ -291,7 +291,7 @@ func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State priorState = pendingPlan.PriorState } - destroyPlan, walkDiags := c.planWalk(config, priorState, rootVariables, opts) + destroyPlan, walkDiags := c.planWalk(config, priorState, opts) diags = diags.Append(walkDiags) if walkDiags.HasErrors() { return nil, diags @@ -392,7 +392,7 @@ func (c *Context) postPlanValidateMoves(config *configs.Config, stmts []refactor return refactoring.ValidateMoves(stmts, config, allInsts) } -func (c *Context) planWalk(config *configs.Config, prevRunState *states.State, rootVariables InputValues, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) { +func (c *Context) planWalk(config *configs.Config, prevRunState *states.State, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics log.Printf("[DEBUG] Building and walking plan graph for %s", opts.Mode) @@ -419,11 +419,10 @@ func (c *Context) planWalk(config *configs.Config, prevRunState *states.State, r // we can now walk. changes := plans.NewChanges() walker, walkDiags := c.walk(graph, walkOp, &graphWalkOpts{ - Config: config, - InputState: prevRunState, - Changes: changes, - MoveResults: moveResults, - RootVariableValues: rootVariables, + Config: config, + InputState: prevRunState, + Changes: changes, + MoveResults: moveResults, }) diags = diags.Append(walker.NonFatalDiagnostics) diags = diags.Append(walkDiags) @@ -469,34 +468,37 @@ func (c *Context) planGraph(config *configs.Config, prevRunState *states.State, switch mode := opts.Mode; mode { case plans.NormalMode: graph, diags := (&PlanGraphBuilder{ - Config: config, - State: prevRunState, - Plugins: c.plugins, - Targets: opts.Targets, - ForceReplace: opts.ForceReplace, - Validate: validate, - skipRefresh: opts.SkipRefresh, + Config: config, + State: prevRunState, + RootVariableValues: opts.SetVariables, + Plugins: c.plugins, + Targets: opts.Targets, + ForceReplace: opts.ForceReplace, + Validate: validate, + skipRefresh: opts.SkipRefresh, }).Build(addrs.RootModuleInstance) return graph, walkPlan, diags case plans.RefreshOnlyMode: graph, diags := (&PlanGraphBuilder{ - Config: config, - State: prevRunState, - Plugins: c.plugins, - Targets: opts.Targets, - Validate: validate, - skipRefresh: opts.SkipRefresh, - skipPlanChanges: true, // this activates "refresh only" mode. + Config: config, + State: prevRunState, + RootVariableValues: opts.SetVariables, + Plugins: c.plugins, + Targets: opts.Targets, + Validate: validate, + skipRefresh: opts.SkipRefresh, + skipPlanChanges: true, // this activates "refresh only" mode. }).Build(addrs.RootModuleInstance) return graph, walkPlan, diags case plans.DestroyMode: graph, diags := (&DestroyPlanGraphBuilder{ - Config: config, - State: prevRunState, - Plugins: c.plugins, - Targets: opts.Targets, - Validate: validate, - skipRefresh: opts.SkipRefresh, + Config: config, + State: prevRunState, + RootVariableValues: opts.SetVariables, + Plugins: c.plugins, + Targets: opts.Targets, + Validate: validate, + skipRefresh: opts.SkipRefresh, }).Build(addrs.RootModuleInstance) return graph, walkPlanDestroy, diags default: diff --git a/internal/terraform/context_validate.go b/internal/terraform/context_validate.go index fb54be420221..4fb02f767164 100644 --- a/internal/terraform/context_validate.go +++ b/internal/terraform/context_validate.go @@ -37,17 +37,6 @@ func (c *Context) Validate(config *configs.Config) tfdiags.Diagnostics { log.Printf("[DEBUG] Building and walking validate graph") - graph, moreDiags := ValidateGraphBuilder(&PlanGraphBuilder{ - Config: config, - Plugins: c.plugins, - Validate: true, - State: states.NewState(), - }).Build(addrs.RootModuleInstance) - diags = diags.Append(moreDiags) - if moreDiags.HasErrors() { - return diags - } - // Validate is to check if the given module is valid regardless of // input values, current state, etc. Therefore we populate all of the // input values with unknown values of the expected type, allowing us @@ -66,9 +55,20 @@ func (c *Context) Validate(config *configs.Config) tfdiags.Diagnostics { } } - walker, walkDiags := c.walk(graph, walkValidate, &graphWalkOpts{ + graph, moreDiags := ValidateGraphBuilder(&PlanGraphBuilder{ Config: config, + Plugins: c.plugins, + Validate: true, + State: states.NewState(), RootVariableValues: varValues, + }).Build(addrs.RootModuleInstance) + diags = diags.Append(moreDiags) + if moreDiags.HasErrors() { + return diags + } + + walker, walkDiags := c.walk(graph, walkValidate, &graphWalkOpts{ + Config: config, }) diags = diags.Append(walker.NonFatalDiagnostics) diags = diags.Append(walkDiags) diff --git a/internal/terraform/context_walk.go b/internal/terraform/context_walk.go index 166341513cce..e8b506314ede 100644 --- a/internal/terraform/context_walk.go +++ b/internal/terraform/context_walk.go @@ -23,8 +23,7 @@ type graphWalkOpts struct { Changes *plans.Changes Config *configs.Config - RootVariableValues InputValues - MoveResults refactoring.MoveResults + MoveResults refactoring.MoveResults } func (c *Context) walk(graph *Graph, operation walkOperation, opts *graphWalkOpts) (*ContextGraphWalker, tfdiags.Diagnostics) { @@ -98,16 +97,15 @@ func (c *Context) graphWalker(operation walkOperation, opts *graphWalkOpts) *Con } return &ContextGraphWalker{ - Context: c, - State: state, - Config: opts.Config, - RefreshState: refreshState, - PrevRunState: prevRunState, - Changes: changes.SyncWrapper(), - InstanceExpander: instances.NewExpander(), - MoveResults: opts.MoveResults, - Operation: operation, - StopContext: c.runContext, - RootVariableValues: opts.RootVariableValues, + Context: c, + State: state, + Config: opts.Config, + RefreshState: refreshState, + PrevRunState: prevRunState, + Changes: changes.SyncWrapper(), + InstanceExpander: instances.NewExpander(), + MoveResults: opts.MoveResults, + Operation: operation, + StopContext: c.runContext, } } diff --git a/internal/terraform/eval_context.go b/internal/terraform/eval_context.go index 4b5a3a5c2cac..8a5958ceb0d4 100644 --- a/internal/terraform/eval_context.go +++ b/internal/terraform/eval_context.go @@ -121,12 +121,24 @@ type EvalContext interface { // addresses in this context. EvaluationScope(self addrs.Referenceable, keyData InstanceKeyEvalData) *lang.Scope - // SetModuleCallArguments defines values for the variables of a particular - // child module call. + // SetRootModuleArgument defines the value for one variable of the root + // module. The caller must ensure that given value is a suitable + // "final value" for the variable, which means that it's already converted + // and validated to match any configured constraints and validation rules. // - // Calling this function multiple times has merging behavior, keeping any - // previously-set keys that are not present in the new map. - SetModuleCallArguments(addrs.ModuleCallInstance, map[string]cty.Value) + // Calling this function multiple times with the same variable address + // will silently overwrite the value provided by a previous call. + SetRootModuleArgument(addrs.InputVariable, cty.Value) + + // SetModuleCallArgument defines the value for one input variable of a + // particular child module call. The caller must ensure that the given + // value is a suitable "final value" for the variable, which means that + // it's already converted and validated to match any configured + // constraints and validation rules. + // + // Calling this function multiple times with the same variable address + // will silently overwrite the value provided by a previous call. + SetModuleCallArgument(addrs.ModuleCallInstance, addrs.InputVariable, cty.Value) // GetVariableValue returns the value provided for the input variable with // the given address, or cty.DynamicVal if the variable hasn't been assigned diff --git a/internal/terraform/eval_context_builtin.go b/internal/terraform/eval_context_builtin.go index ecbac446e7cc..35170bcd6ab3 100644 --- a/internal/terraform/eval_context_builtin.go +++ b/internal/terraform/eval_context_builtin.go @@ -313,7 +313,21 @@ func (ctx *BuiltinEvalContext) Path() addrs.ModuleInstance { return ctx.PathValue } -func (ctx *BuiltinEvalContext) SetModuleCallArguments(n addrs.ModuleCallInstance, vals map[string]cty.Value) { +func (ctx *BuiltinEvalContext) SetRootModuleArgument(addr addrs.InputVariable, v cty.Value) { + ctx.VariableValuesLock.Lock() + defer ctx.VariableValuesLock.Unlock() + + log.Printf("[TRACE] BuiltinEvalContext: Storing final value for variable %s", addr.Absolute(addrs.RootModuleInstance)) + key := addrs.RootModuleInstance.String() + args := ctx.VariableValues[key] + if args == nil { + args = make(map[string]cty.Value) + ctx.VariableValues[key] = args + } + args[addr.Name] = v +} + +func (ctx *BuiltinEvalContext) SetModuleCallArgument(callAddr addrs.ModuleCallInstance, varAddr addrs.InputVariable, v cty.Value) { ctx.VariableValuesLock.Lock() defer ctx.VariableValuesLock.Unlock() @@ -321,18 +335,15 @@ func (ctx *BuiltinEvalContext) SetModuleCallArguments(n addrs.ModuleCallInstance panic("context path not set") } - childPath := n.ModuleInstance(ctx.PathValue) + childPath := callAddr.ModuleInstance(ctx.PathValue) + log.Printf("[TRACE] BuiltinEvalContext: Storing final value for variable %s", varAddr.Absolute(childPath)) key := childPath.String() - args := ctx.VariableValues[key] if args == nil { - ctx.VariableValues[key] = vals - return - } - - for k, v := range vals { - args[k] = v + args = make(map[string]cty.Value) + ctx.VariableValues[key] = args } + args[varAddr.Name] = v } func (ctx *BuiltinEvalContext) GetVariableValue(addr addrs.AbsInputVariableInstance) cty.Value { diff --git a/internal/terraform/eval_context_mock.go b/internal/terraform/eval_context_mock.go index edcdaac6b62c..8dd6ec3343d4 100644 --- a/internal/terraform/eval_context_mock.go +++ b/internal/terraform/eval_context_mock.go @@ -111,13 +111,21 @@ type MockEvalContext struct { PathCalled bool PathPath addrs.ModuleInstance - SetModuleCallArgumentsCalled bool - SetModuleCallArgumentsModule addrs.ModuleCallInstance - SetModuleCallArgumentsValues map[string]cty.Value + SetRootModuleArgumentCalled bool + SetRootModuleArgumentAddr addrs.InputVariable + SetRootModuleArgumentValue cty.Value + SetRootModuleArgumentFunc func(addr addrs.InputVariable, v cty.Value) + + SetModuleCallArgumentCalled bool + SetModuleCallArgumentModuleCall addrs.ModuleCallInstance + SetModuleCallArgumentVariable addrs.InputVariable + SetModuleCallArgumentValue cty.Value + SetModuleCallArgumentFunc func(callAddr addrs.ModuleCallInstance, varAddr addrs.InputVariable, v cty.Value) GetVariableValueCalled bool GetVariableValueAddr addrs.AbsInputVariableInstance GetVariableValueValue cty.Value + GetVariableValueFunc func(addr addrs.AbsInputVariableInstance) cty.Value // supersedes GetVariableValueValue ChangesCalled bool ChangesChanges *plans.ChangesSync @@ -321,15 +329,31 @@ func (c *MockEvalContext) Path() addrs.ModuleInstance { return c.PathPath } -func (c *MockEvalContext) SetModuleCallArguments(n addrs.ModuleCallInstance, values map[string]cty.Value) { - c.SetModuleCallArgumentsCalled = true - c.SetModuleCallArgumentsModule = n - c.SetModuleCallArgumentsValues = values +func (c *MockEvalContext) SetRootModuleArgument(addr addrs.InputVariable, v cty.Value) { + c.SetRootModuleArgumentCalled = true + c.SetRootModuleArgumentAddr = addr + c.SetRootModuleArgumentValue = v + if c.SetRootModuleArgumentFunc != nil { + c.SetRootModuleArgumentFunc(addr, v) + } +} + +func (c *MockEvalContext) SetModuleCallArgument(callAddr addrs.ModuleCallInstance, varAddr addrs.InputVariable, v cty.Value) { + c.SetModuleCallArgumentCalled = true + c.SetModuleCallArgumentModuleCall = callAddr + c.SetModuleCallArgumentVariable = varAddr + c.SetModuleCallArgumentValue = v + if c.SetModuleCallArgumentFunc != nil { + c.SetModuleCallArgumentFunc(callAddr, varAddr, v) + } } func (c *MockEvalContext) GetVariableValue(addr addrs.AbsInputVariableInstance) cty.Value { c.GetVariableValueCalled = true c.GetVariableValueAddr = addr + if c.GetVariableValueFunc != nil { + return c.GetVariableValueFunc(addr) + } return c.GetVariableValueValue } diff --git a/internal/terraform/eval_variable.go b/internal/terraform/eval_variable.go index bdfadd2e9c70..1c16069ad4c1 100644 --- a/internal/terraform/eval_variable.go +++ b/internal/terraform/eval_variable.go @@ -12,6 +12,102 @@ import ( "github.com/zclconf/go-cty/cty/convert" ) +func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given cty.Value, valRange tfdiags.SourceRange, cfg *configs.Variable) (cty.Value, tfdiags.Diagnostics) { + var diags tfdiags.Diagnostics + + convertTy := cfg.ConstraintType + log.Printf("[TRACE] prepareFinalInputVariableValue: preparing %s", addr) + + var defaultVal cty.Value + if cfg.Default != cty.NilVal { + log.Printf("[TRACE] prepareFinalInputVariableValue: %s has a default value", addr) + var err error + defaultVal, err = convert.Convert(cfg.Default, convertTy) + if err != nil { + // Validation of the declaration should typically catch this, + // but we'll check it here too to be robust. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid default value for module argument", + Detail: fmt.Sprintf( + "The default value for variable %q is incompatible with its type constraint: %s.", + cfg.Name, err, + ), + Subject: &cfg.DeclRange, + }) + // We'll return a placeholder unknown value to avoid producing + // redundant downstream errors. + return cty.UnknownVal(cfg.Type), diags + } + } + + if given == cty.NilVal { // The variable wasn't set at all (even to null) + log.Printf("[TRACE] prepareFinalInputVariableValue: %s has no defined value", addr) + if cfg.Required() { + // NOTE: The CLI layer typically checks for itself whether all of + // the required _root_ module variables are not set, which would + // mask this error. We can get here for child module variables, + // though. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Required variable not set`, + Detail: fmt.Sprintf(`The variable %q is required, but is not set.`, addr.Variable.Name), + Subject: valRange.ToHCL().Ptr(), + }) + // We'll return a placeholder unknown value to avoid producing + // redundant downstream errors. + return cty.UnknownVal(cfg.Type), diags + } + + given = defaultVal // must be set, because we checked above that the variable isn't required + } + + val, err := convert.Convert(given, convertTy) + if err != nil { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid value for module argument", + Detail: fmt.Sprintf( + "The given value is not suitable for child module variable %q defined at %s: %s.", + cfg.Name, cfg.DeclRange.String(), err, + ), + Subject: valRange.ToHCL().Ptr(), + }) + // We'll return a placeholder unknown value to avoid producing + // redundant downstream errors. + return cty.UnknownVal(cfg.Type), diags + } + + // By the time we get here, we know: + // - val matches the variable's type constraint + // - val is definitely not cty.NilVal, but might be a null value if the given was already null. + // + // That means we just need to handle the case where the value is null, + // which might mean we need to use the default value, or produce an error. + // + // For historical reasons we do this only for a "non-nullable" variable. + // Nullable variables just appear as null if they were set to null, + // regardless of any default value. + if val.IsNull() && !cfg.Nullable { + log.Printf("[TRACE] prepareFinalInputVariableValue: %s is defined as null", addr) + if defaultVal != cty.NilVal { + val = defaultVal + } else { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Required variable not set`, + Detail: fmt.Sprintf(`The variable %q is required, but the given value is null.`, addr.Variable.Name), + Subject: valRange.ToHCL().Ptr(), + }) + // Stub out our return value so that the semantic checker doesn't + // produce redundant downstream errors. + val = cty.UnknownVal(cfg.Type) + } + } + + return val, diags +} + // evalVariableValidations ensures that all of the configured custom validations // for a variable are passing. // @@ -20,9 +116,10 @@ import ( // EvalModuleCallArgument for variables in descendent modules. func evalVariableValidations(addr addrs.AbsInputVariableInstance, config *configs.Variable, expr hcl.Expression, ctx EvalContext) (diags tfdiags.Diagnostics) { if config == nil || len(config.Validations) == 0 { - log.Printf("[TRACE] evalVariableValidations: not active for %s, so skipping", addr) + log.Printf("[TRACE] evalVariableValidations: no validation rules declared for %s, so skipping", addr) return nil } + log.Printf("[TRACE] evalVariableValidations: validating %s", addr) // Variable nodes evaluate in the parent module to where they were declared // because the value expression (n.Expr, if set) comes from the calling @@ -34,6 +131,14 @@ func evalVariableValidations(addr addrs.AbsInputVariableInstance, config *config // evaluation context containing just the required value, and thus avoid // the problem that ctx's evaluation functions refer to the wrong module. val := ctx.GetVariableValue(addr) + if val == cty.NilVal { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "No final value for variable", + Detail: fmt.Sprintf("Terraform doesn't have a final value for %s during validation. This is a bug in Terraform; please report it!", addr), + }) + return diags + } hclCtx := &hcl.EvalContext{ Variables: map[string]cty.Value{ "var": cty.ObjectVal(map[string]cty.Value{ diff --git a/internal/terraform/eval_variable_test.go b/internal/terraform/eval_variable_test.go new file mode 100644 index 000000000000..fa4048fed41f --- /dev/null +++ b/internal/terraform/eval_variable_test.go @@ -0,0 +1,426 @@ +package terraform + +import ( + "fmt" + "testing" + + "github.com/zclconf/go-cty/cty" + + "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/tfdiags" +) + +func TestPrepareFinalInputVariableValue(t *testing.T) { + // This is just a concise way to define a bunch of *configs.Variable + // objects to use in our tests below. We're only going to decode this + // config, not fully evaluate it. + cfgSrc := ` + variable "nullable_required" { + } + variable "nullable_optional_default_string" { + default = "hello" + } + variable "nullable_optional_default_null" { + default = null + } + variable "constrained_string_nullable_required" { + type = string + } + variable "constrained_string_nullable_optional_default_string" { + type = string + default = "hello" + } + variable "constrained_string_nullable_optional_default_bool" { + type = string + default = true + } + variable "constrained_string_nullable_optional_default_null" { + type = string + default = null + } + variable "required" { + nullable = false + } + variable "optional_default_string" { + nullable = false + default = "hello" + } + variable "constrained_string_required" { + nullable = false + type = string + } + variable "constrained_string_optional_default_string" { + nullable = false + type = string + default = "hello" + } + variable "constrained_string_optional_default_bool" { + nullable = false + type = string + default = true + } + ` + cfg := testModuleInline(t, map[string]string{ + "main.tf": cfgSrc, + }) + variableConfigs := cfg.Module.Variables + + tests := []struct { + varName string + given cty.Value + want cty.Value + wantErr string + }{ + // nullable_required + { + "nullable_required", + cty.NilVal, + cty.UnknownVal(cty.DynamicPseudoType), + `Required variable not set: The variable "nullable_required" is required, but is not set.`, + }, + { + "nullable_required", + cty.NullVal(cty.DynamicPseudoType), + cty.NullVal(cty.DynamicPseudoType), + ``, // "required" for a nullable variable means only that it must be set, even if it's set to null + }, + { + "nullable_required", + cty.StringVal("ahoy"), + cty.StringVal("ahoy"), + ``, + }, + { + "nullable_required", + cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String), + ``, + }, + + // nullable_optional_default_string + { + "nullable_optional_default_string", + cty.NilVal, + cty.StringVal("hello"), // the declared default value + ``, + }, + { + "nullable_optional_default_string", + cty.NullVal(cty.DynamicPseudoType), + cty.NullVal(cty.DynamicPseudoType), // nullable variables can be really set to null, masking the default + ``, + }, + { + "nullable_optional_default_string", + cty.StringVal("ahoy"), + cty.StringVal("ahoy"), + ``, + }, + { + "nullable_optional_default_string", + cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String), + ``, + }, + + // nullable_optional_default_null + { + "nullable_optional_default_null", + cty.NilVal, + cty.NullVal(cty.DynamicPseudoType), // the declared default value + ``, + }, + { + "nullable_optional_default_null", + cty.NullVal(cty.String), + cty.NullVal(cty.String), // nullable variables can be really set to null, masking the default + ``, + }, + { + "nullable_optional_default_null", + cty.StringVal("ahoy"), + cty.StringVal("ahoy"), + ``, + }, + { + "nullable_optional_default_null", + cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String), + ``, + }, + + // constrained_string_nullable_required + { + "constrained_string_nullable_required", + cty.NilVal, + cty.UnknownVal(cty.String), + `Required variable not set: The variable "constrained_string_nullable_required" is required, but is not set.`, + }, + { + "constrained_string_nullable_required", + cty.NullVal(cty.DynamicPseudoType), + cty.NullVal(cty.String), // the null value still gets converted to match the type constraint + ``, // "required" for a nullable variable means only that it must be set, even if it's set to null + }, + { + "constrained_string_nullable_required", + cty.StringVal("ahoy"), + cty.StringVal("ahoy"), + ``, + }, + { + "constrained_string_nullable_required", + cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String), + ``, + }, + + // constrained_string_nullable_optional_default_string + { + "constrained_string_nullable_optional_default_string", + cty.NilVal, + cty.StringVal("hello"), // the declared default value + ``, + }, + { + "constrained_string_nullable_optional_default_string", + cty.NullVal(cty.DynamicPseudoType), + cty.NullVal(cty.String), // nullable variables can be really set to null, masking the default + ``, + }, + { + "constrained_string_nullable_optional_default_string", + cty.StringVal("ahoy"), + cty.StringVal("ahoy"), + ``, + }, + { + "constrained_string_nullable_optional_default_string", + cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String), + ``, + }, + + // constrained_string_nullable_optional_default_bool + { + "constrained_string_nullable_optional_default_bool", + cty.NilVal, + cty.StringVal("true"), // the declared default value, automatically converted to match type constraint + ``, + }, + { + "constrained_string_nullable_optional_default_bool", + cty.NullVal(cty.DynamicPseudoType), + cty.NullVal(cty.String), // nullable variables can be really set to null, masking the default + ``, + }, + { + "constrained_string_nullable_optional_default_bool", + cty.StringVal("ahoy"), + cty.StringVal("ahoy"), + ``, + }, + { + "constrained_string_nullable_optional_default_bool", + cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String), + ``, + }, + + // constrained_string_nullable_optional_default_null + { + "constrained_string_nullable_optional_default_null", + cty.NilVal, + cty.NullVal(cty.String), + ``, + }, + { + "constrained_string_nullable_optional_default_null", + cty.NullVal(cty.DynamicPseudoType), + cty.NullVal(cty.String), + ``, + }, + { + "constrained_string_nullable_optional_default_null", + cty.StringVal("ahoy"), + cty.StringVal("ahoy"), + ``, + }, + { + "constrained_string_nullable_optional_default_null", + cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String), + ``, + }, + + // required + { + "required", + cty.NilVal, + cty.UnknownVal(cty.DynamicPseudoType), + `Required variable not set: The variable "required" is required, but is not set.`, + }, + { + "required", + cty.NullVal(cty.DynamicPseudoType), + cty.UnknownVal(cty.DynamicPseudoType), + `Required variable not set: The variable "required" is required, but the given value is null.`, + }, + { + "required", + cty.StringVal("ahoy"), + cty.StringVal("ahoy"), + ``, + }, + { + "required", + cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String), + ``, + }, + + // optional_default_string + { + "optional_default_string", + cty.NilVal, + cty.StringVal("hello"), // the declared default value + ``, + }, + { + "optional_default_string", + cty.NullVal(cty.DynamicPseudoType), + cty.StringVal("hello"), // the declared default value + ``, + }, + { + "optional_default_string", + cty.StringVal("ahoy"), + cty.StringVal("ahoy"), + ``, + }, + { + "optional_default_string", + cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String), + ``, + }, + + // constrained_string_required + { + "constrained_string_required", + cty.NilVal, + cty.UnknownVal(cty.String), + `Required variable not set: The variable "constrained_string_required" is required, but is not set.`, + }, + { + "constrained_string_required", + cty.NullVal(cty.DynamicPseudoType), + cty.UnknownVal(cty.String), + `Required variable not set: The variable "constrained_string_required" is required, but the given value is null.`, + }, + { + "constrained_string_required", + cty.StringVal("ahoy"), + cty.StringVal("ahoy"), + ``, + }, + { + "constrained_string_required", + cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String), + ``, + }, + + // constrained_string_optional_default_string + { + "constrained_string_optional_default_string", + cty.NilVal, + cty.StringVal("hello"), // the declared default value + ``, + }, + { + "constrained_string_optional_default_string", + cty.NullVal(cty.DynamicPseudoType), + cty.StringVal("hello"), // the declared default value + ``, + }, + { + "constrained_string_optional_default_string", + cty.StringVal("ahoy"), + cty.StringVal("ahoy"), + ``, + }, + { + "constrained_string_optional_default_string", + cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String), + ``, + }, + + // constrained_string_optional_default_bool + { + "constrained_string_optional_default_bool", + cty.NilVal, + cty.StringVal("true"), // the declared default value, automatically converted to match type constraint + ``, + }, + { + "constrained_string_optional_default_bool", + cty.NullVal(cty.DynamicPseudoType), + cty.StringVal("true"), // the declared default value, automatically converted to match type constraint + ``, + }, + { + "constrained_string_optional_default_bool", + cty.StringVal("ahoy"), + cty.StringVal("ahoy"), + ``, + }, + { + "constrained_string_optional_default_bool", + cty.UnknownVal(cty.String), + cty.UnknownVal(cty.String), + ``, + }, + } + + for _, test := range tests { + t.Run(fmt.Sprintf("%s %#v", test.varName, test.given), func(t *testing.T) { + varAddr := addrs.InputVariable{Name: test.varName}.Absolute(addrs.RootModuleInstance) + varCfg := variableConfigs[test.varName] + if varCfg == nil { + t.Fatalf("invalid variable name %q", test.varName) + } + + t.Logf( + "test case\nvariable: %s\nconstraint: %#v\ndefault: %#v\nnullable: %#v\ngiven value: %#v", + varAddr, + varCfg.Type, + varCfg.Default, + varCfg.Nullable, + test.given, + ) + + got, diags := prepareFinalInputVariableValue( + varAddr, test.given, tfdiags.SourceRangeFromHCL(varCfg.DeclRange), varCfg, + ) + + if test.wantErr != "" { + if !diags.HasErrors() { + t.Errorf("unexpected success\nwant error: %s", test.wantErr) + } else if got, want := diags.Err().Error(), test.wantErr; got != want { + t.Errorf("wrong error\ngot: %s\nwant: %s", got, want) + } + } else { + if diags.HasErrors() { + t.Errorf("unexpected error\ngot: %s", diags.Err().Error()) + } + } + + // NOTE: should still have returned some reasonable value even if there was an error + if !test.want.RawEquals(got) { + t.Fatalf("wrong result\ngot: %#v\nwant: %#v", got, test.want) + } + }) + } +} diff --git a/internal/terraform/evaluate.go b/internal/terraform/evaluate.go index 322ef6fda4d0..243335df2642 100644 --- a/internal/terraform/evaluate.go +++ b/internal/terraform/evaluate.go @@ -10,7 +10,6 @@ import ( "github.com/agext/levenshtein" "github.com/hashicorp/hcl/v2" "github.com/zclconf/go-cty/cty" - "github.com/zclconf/go-cty/cty/convert" "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" @@ -248,7 +247,7 @@ func (d *evaluationStateData) GetInputVariable(addr addrs.InputVariable, rng tfd // This is important because otherwise the validation walk will tend to be // overly strict, requiring expressions throughout the configuration to // be complicated to accommodate all possible inputs, whereas returning - // known here allows for simpler patterns like using input values as + // unknown here allows for simpler patterns like using input values as // guards to broadly enable/disable resources, avoid processing things // that are disabled, etc. Terraform's static validation leans towards // being liberal in what it accepts because the subsequent plan walk has @@ -267,28 +266,27 @@ func (d *evaluationStateData) GetInputVariable(addr addrs.InputVariable, rng tfd return cty.UnknownVal(config.Type), diags } - val, isSet := vals[addr.Name] - switch { - case !isSet: - // The config loader will ensure there is a default if the value is not - // set at all. - val = config.Default - - case val.IsNull() && !config.Nullable && config.Default != cty.NilVal: - // If nullable=false a null value will use the configured default. - val = config.Default - } + // d.Evaluator.VariableValues should always contain valid "final values" + // for variables, which is to say that they have already had type + // conversions, validations, and default value handling applied to them. + // Those are the responsibility of the graph notes representing the + // variable declarations. Therefore here we just trust that we already + // have a correct value. - var err error - val, err = convert.Convert(val, config.ConstraintType) - if err != nil { - // We should never get here because this problem should've been caught - // during earlier validation, but we'll do something reasonable anyway. + val, isSet := vals[addr.Name] + if !isSet { + // We should not be able to get here without having a valid value + // for every variable, so this always indicates a bug in either + // the graph builder (not including all the needed nodes) or in + // the graph nodes representing variables. diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, - Summary: `Incorrect variable type`, - Detail: fmt.Sprintf(`The resolved value of variable %q is not appropriate: %s.`, addr.Name, err), - Subject: &config.DeclRange, + Summary: `Reference to unresolved input variable`, + Detail: fmt.Sprintf( + `The final value for %s is missing in Terraform's evaluation context. This is a bug in Terraform; please report it!`, + addr.Absolute(d.ModulePath), + ), + Subject: rng.ToHCL().Ptr(), }) val = cty.UnknownVal(config.Type) } diff --git a/internal/terraform/graph_builder_apply.go b/internal/terraform/graph_builder_apply.go index 75f9d3d4ad25..86d825560276 100644 --- a/internal/terraform/graph_builder_apply.go +++ b/internal/terraform/graph_builder_apply.go @@ -26,6 +26,11 @@ type ApplyGraphBuilder struct { // State is the current state State *states.State + // RootVariableValues are the root module input variables captured as + // part of the plan object, which we must reproduce in the apply step + // to get a consistent result. + RootVariableValues InputValues + // Plugins is a library of the plug-in components (providers and // provisioners) available for use. Plugins *contextPlugins @@ -88,7 +93,7 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { }, // Add dynamic values - &RootVariableTransformer{Config: b.Config}, + &RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues}, &ModuleVariableTransformer{Config: b.Config}, &LocalTransformer{Config: b.Config}, &OutputTransformer{Config: b.Config, Changes: b.Changes}, diff --git a/internal/terraform/graph_builder_destroy_plan.go b/internal/terraform/graph_builder_destroy_plan.go index 0bac6305e08f..def1aa3739aa 100644 --- a/internal/terraform/graph_builder_destroy_plan.go +++ b/internal/terraform/graph_builder_destroy_plan.go @@ -23,6 +23,11 @@ type DestroyPlanGraphBuilder struct { // State is the current state State *states.State + // RootVariableValues are the raw input values for root input variables + // given by the caller, which we'll resolve into final values as part + // of the plan walk. + RootVariableValues InputValues + // Plugins is a library of plug-in components (providers and // provisioners) available for use. Plugins *contextPlugins diff --git a/internal/terraform/graph_builder_eval.go b/internal/terraform/graph_builder_eval.go index ee9d6b8e83de..78031e21f9f3 100644 --- a/internal/terraform/graph_builder_eval.go +++ b/internal/terraform/graph_builder_eval.go @@ -30,6 +30,11 @@ type EvalGraphBuilder struct { // State is the current state State *states.State + // RootVariableValues are the raw input values for root input variables + // given by the caller, which we'll resolve into final values as part + // of the plan walk. + RootVariableValues InputValues + // Plugins is a library of plug-in components (providers and // provisioners) available for use. Plugins *contextPlugins @@ -60,7 +65,7 @@ func (b *EvalGraphBuilder) Steps() []GraphTransformer { }, // Add dynamic values - &RootVariableTransformer{Config: b.Config}, + &RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues}, &ModuleVariableTransformer{Config: b.Config}, &LocalTransformer{Config: b.Config}, &OutputTransformer{Config: b.Config}, diff --git a/internal/terraform/graph_builder_import.go b/internal/terraform/graph_builder_import.go index 9910354cf5f9..d8d609ebaa85 100644 --- a/internal/terraform/graph_builder_import.go +++ b/internal/terraform/graph_builder_import.go @@ -17,6 +17,11 @@ type ImportGraphBuilder struct { // Module is a configuration to build the graph from. See ImportOpts.Config. Config *configs.Config + // RootVariableValues are the raw input values for root input variables + // given by the caller, which we'll resolve into final values as part + // of the plan walk. + RootVariableValues InputValues + // Plugins is a library of plug-in components (providers and // provisioners) available for use. Plugins *contextPlugins @@ -53,7 +58,7 @@ func (b *ImportGraphBuilder) Steps() []GraphTransformer { &ConfigTransformer{Config: config}, // Add dynamic values - &RootVariableTransformer{Config: b.Config}, + &RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues}, &ModuleVariableTransformer{Config: b.Config}, &LocalTransformer{Config: b.Config}, &OutputTransformer{Config: b.Config}, diff --git a/internal/terraform/graph_builder_plan.go b/internal/terraform/graph_builder_plan.go index 709b917b6733..1b8ce58338a9 100644 --- a/internal/terraform/graph_builder_plan.go +++ b/internal/terraform/graph_builder_plan.go @@ -28,6 +28,11 @@ type PlanGraphBuilder struct { // State is the current state State *states.State + // RootVariableValues are the raw input values for root input variables + // given by the caller, which we'll resolve into final values as part + // of the plan walk. + RootVariableValues InputValues + // Plugins is a library of plug-in components (providers and // provisioners) available for use. Plugins *contextPlugins @@ -95,7 +100,7 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer { }, // Add dynamic values - &RootVariableTransformer{Config: b.Config}, + &RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues}, &ModuleVariableTransformer{Config: b.Config}, &LocalTransformer{Config: b.Config}, &OutputTransformer{Config: b.Config}, diff --git a/internal/terraform/node_module_variable.go b/internal/terraform/node_module_variable.go index 9f15587eca1b..321cd874845f 100644 --- a/internal/terraform/node_module_variable.go +++ b/internal/terraform/node_module_variable.go @@ -12,7 +12,6 @@ import ( "github.com/hashicorp/terraform/internal/lang" "github.com/hashicorp/terraform/internal/tfdiags" "github.com/zclconf/go-cty/cty" - "github.com/zclconf/go-cty/cty/convert" ) // nodeExpandModuleVariable is the placeholder for an variable that has not yet had @@ -143,35 +142,27 @@ func (n *nodeModuleVariable) ModulePath() addrs.Module { // GraphNodeExecutable func (n *nodeModuleVariable) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) { - // If we have no value, do nothing - if n.Expr == nil { - return nil - } + log.Printf("[TRACE] nodeModuleVariable: evaluating %s", n.Addr) - // Otherwise, interpolate the value of this variable and set it - // within the variables mapping. - var vals map[string]cty.Value + var val cty.Value var err error switch op { case walkValidate: - vals, err = n.evalModuleCallArgument(ctx, true) + val, err = n.evalModuleCallArgument(ctx, true) diags = diags.Append(err) - if diags.HasErrors() { - return diags - } default: - vals, err = n.evalModuleCallArgument(ctx, false) + val, err = n.evalModuleCallArgument(ctx, false) diags = diags.Append(err) - if diags.HasErrors() { - return diags - } + } + if diags.HasErrors() { + return diags } // Set values for arguments of a child module call, for later retrieval // during expression evaluation. _, call := n.Addr.Module.CallInstance() - ctx.SetModuleCallArguments(call, vals) + ctx.SetModuleCallArgument(call, n.Addr.Variable, val) return evalVariableValidations(n.Addr, n.Config, n.Expr, ctx) } @@ -199,77 +190,45 @@ func (n *nodeModuleVariable) DotNode(name string, opts *dag.DotOpts) *dag.DotNod // validateOnly indicates that this evaluation is only for config // validation, and we will not have any expansion module instance // repetition data. -func (n *nodeModuleVariable) evalModuleCallArgument(ctx EvalContext, validateOnly bool) (map[string]cty.Value, error) { - name := n.Addr.Variable.Name - expr := n.Expr - - if expr == nil { - // Should never happen, but we'll bail out early here rather than - // crash in case it does. We set no value at all in this case, - // making a subsequent call to EvalContext.SetModuleCallArguments - // a no-op. - log.Printf("[ERROR] attempt to evaluate %s with nil expression", n.Addr.String()) - return nil, nil - } - - var moduleInstanceRepetitionData instances.RepetitionData - - switch { - case validateOnly: - // the instance expander does not track unknown expansion values, so we - // have to assume all RepetitionData is unknown. - moduleInstanceRepetitionData = instances.RepetitionData{ - CountIndex: cty.UnknownVal(cty.Number), - EachKey: cty.UnknownVal(cty.String), - EachValue: cty.DynamicVal, +func (n *nodeModuleVariable) evalModuleCallArgument(ctx EvalContext, validateOnly bool) (cty.Value, error) { + var diags tfdiags.Diagnostics + var givenVal cty.Value + var errSourceRange tfdiags.SourceRange + if expr := n.Expr; expr != nil { + var moduleInstanceRepetitionData instances.RepetitionData + + switch { + case validateOnly: + // the instance expander does not track unknown expansion values, so we + // have to assume all RepetitionData is unknown. + moduleInstanceRepetitionData = instances.RepetitionData{ + CountIndex: cty.UnknownVal(cty.Number), + EachKey: cty.UnknownVal(cty.String), + EachValue: cty.DynamicVal, + } + + default: + // Get the repetition data for this module instance, + // so we can create the appropriate scope for evaluating our expression + moduleInstanceRepetitionData = ctx.InstanceExpander().GetModuleInstanceRepetitionData(n.ModuleInstance) } - default: - // Get the repetition data for this module instance, - // so we can create the appropriate scope for evaluating our expression - moduleInstanceRepetitionData = ctx.InstanceExpander().GetModuleInstanceRepetitionData(n.ModuleInstance) - } - - scope := ctx.EvaluationScope(nil, moduleInstanceRepetitionData) - val, diags := scope.EvalExpr(expr, cty.DynamicPseudoType) - - // We intentionally passed DynamicPseudoType to EvalExpr above because - // now we can do our own local type conversion and produce an error message - // with better context if it fails. - var convErr error - val, convErr = convert.Convert(val, n.Config.ConstraintType) - if convErr != nil { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid value for module argument", - Detail: fmt.Sprintf( - "The given value is not suitable for child module variable %q defined at %s: %s.", - name, n.Config.DeclRange.String(), convErr, - ), - Subject: expr.Range().Ptr(), - }) - // We'll return a placeholder unknown value to avoid producing - // redundant downstream errors. - val = cty.UnknownVal(n.Config.Type) - } - - // If there is no default, we have to ensure that a null value is allowed - // for this variable. - if n.Config.Default == cty.NilVal && !n.Config.Nullable && val.IsNull() { - // The value cannot be null, and there is no configured default. - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: `Invalid variable value`, - Detail: fmt.Sprintf(`The variable %q is required, but the given value is null.`, n.Addr), - Subject: &n.Config.DeclRange, - }) - // Stub out our return value so that the semantic checker doesn't - // produce redundant downstream errors. - val = cty.UnknownVal(n.Config.Type) + scope := ctx.EvaluationScope(nil, moduleInstanceRepetitionData) + val, moreDiags := scope.EvalExpr(expr, cty.DynamicPseudoType) + diags = diags.Append(moreDiags) + if moreDiags.HasErrors() { + return cty.DynamicVal, diags.ErrWithWarnings() + } + givenVal = val + errSourceRange = tfdiags.SourceRangeFromHCL(expr.Range()) + } else { + // We'll use cty.NilVal to represent the variable not being set at all. + givenVal = cty.NilVal + errSourceRange = tfdiags.SourceRangeFromHCL(n.Config.DeclRange) // we use the declaration range as a fallback for an undefined variable } - vals := make(map[string]cty.Value) - vals[name] = val + finalVal, moreDiags := prepareFinalInputVariableValue(n.Addr, givenVal, errSourceRange, n.Config) + diags = diags.Append(moreDiags) - return vals, diags.ErrWithWarnings() + return finalVal, diags.ErrWithWarnings() } diff --git a/internal/terraform/node_root_variable.go b/internal/terraform/node_root_variable.go index 56ee5149a571..d023be3500ba 100644 --- a/internal/terraform/node_root_variable.go +++ b/internal/terraform/node_root_variable.go @@ -1,16 +1,26 @@ package terraform import ( + "log" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/dag" "github.com/hashicorp/terraform/internal/tfdiags" + "github.com/zclconf/go-cty/cty" ) // NodeRootVariable represents a root variable input. type NodeRootVariable struct { Addr addrs.InputVariable Config *configs.Variable + + // RawValue is the value for the variable set from outside Terraform + // Core, such as on the command line, or from an environment variable, + // or similar. This is the raw value that was provided, not yet + // converted or validated, and can be nil for a variable that isn't + // set at all. + RawValue *InputValue } var ( @@ -38,21 +48,56 @@ func (n *NodeRootVariable) ReferenceableAddrs() []addrs.Referenceable { // GraphNodeExecutable func (n *NodeRootVariable) Execute(ctx EvalContext, op walkOperation) tfdiags.Diagnostics { - // We don't actually need to _evaluate_ a root module variable, because - // its value is always constant and already stashed away in our EvalContext. - // However, we might need to run some user-defined validation rules against - // the value. + // Root module variables are special in that they are provided directly + // by the caller (usually, the CLI layer) and so we don't really need to + // evaluate them in the usual sense, but we do need to process the raw + // values given by the caller to match what the module is expecting, and + // make sure the values are valid. + var diags tfdiags.Diagnostics + + addr := addrs.RootModuleInstance.InputVariable(n.Addr.Name) + log.Printf("[TRACE] NodeRootVariable: evaluating %s", addr) + + if n.Config == nil { + // Because we build NodeRootVariable from configuration in the normal + // case it's strange to get here, but we tolerate it to allow for + // tests that might not populate the inputs fully. + return nil + } - if n.Config == nil || len(n.Config.Validations) == 0 { - return nil // nothing to do + var givenVal cty.Value + if n.RawValue != nil { + givenVal = n.RawValue.Value + } else { + // We'll use cty.NilVal to represent the variable not being set at + // all, which for historical reasons is unfortunately different than + // explicitly setting it to null in some cases. + givenVal = cty.NilVal } - return evalVariableValidations( + finalVal, moreDiags := prepareFinalInputVariableValue( + addr, + givenVal, + tfdiags.SourceRangeFromHCL(n.Config.DeclRange), + n.Config, + ) + diags = diags.Append(moreDiags) + if moreDiags.HasErrors() { + // No point in proceeding to validations then, because they'll + // probably fail trying to work with a value of the wrong type. + return diags + } + + ctx.SetRootModuleArgument(addr.Variable, finalVal) + + moreDiags = evalVariableValidations( addrs.RootModuleInstance.InputVariable(n.Addr.Name), n.Config, nil, // not set for root module variables ctx, ) + diags = diags.Append(moreDiags) + return diags } // dag.GraphNodeDotter impl. diff --git a/internal/terraform/node_root_variable_test.go b/internal/terraform/node_root_variable_test.go index bd3d9c2d65c4..aecb7428a203 100644 --- a/internal/terraform/node_root_variable_test.go +++ b/internal/terraform/node_root_variable_test.go @@ -3,26 +3,164 @@ package terraform import ( "testing" + "github.com/hashicorp/hcl/v2" + "github.com/zclconf/go-cty/cty" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" - "github.com/zclconf/go-cty/cty" + "github.com/hashicorp/terraform/internal/lang" ) func TestNodeRootVariableExecute(t *testing.T) { - ctx := new(MockEvalContext) - - n := &NodeRootVariable{ - Addr: addrs.InputVariable{Name: "foo"}, - Config: &configs.Variable{ - Name: "foo", - Type: cty.String, - ConstraintType: cty.String, - }, - } + t.Run("type conversion", func(t *testing.T) { + ctx := new(MockEvalContext) + + n := &NodeRootVariable{ + Addr: addrs.InputVariable{Name: "foo"}, + Config: &configs.Variable{ + Name: "foo", + Type: cty.String, + ConstraintType: cty.String, + }, + RawValue: &InputValue{ + Value: cty.True, + SourceType: ValueFromUnknown, + }, + } + + diags := n.Execute(ctx, walkApply) + if diags.HasErrors() { + t.Fatalf("unexpected error: %s", diags.Err()) + } + + if !ctx.SetRootModuleArgumentCalled { + t.Fatalf("ctx.SetRootModuleArgument wasn't called") + } + if got, want := ctx.SetRootModuleArgumentAddr.String(), "var.foo"; got != want { + t.Errorf("wrong address for ctx.SetRootModuleArgument\ngot: %s\nwant: %s", got, want) + } + if got, want := ctx.SetRootModuleArgumentValue, cty.StringVal("true"); !want.RawEquals(got) { + // NOTE: The given value was cty.Bool but the type constraint was + // cty.String, so it was NodeRootVariable's responsibility to convert + // as part of preparing the "final value". + t.Errorf("wrong value for ctx.SetRootModuleArgument\ngot: %#v\nwant: %#v", got, want) + } + }) + t.Run("validation", func(t *testing.T) { + ctx := new(MockEvalContext) + + // The variable validation function gets called with Terraform's + // built-in functions available, so we need a minimal scope just for + // it to get the functions from. + ctx.EvaluationScopeScope = &lang.Scope{} + + // We need to reimplement a _little_ bit of EvalContextBuiltin logic + // here to get a similar effect with EvalContextMock just to get the + // value to flow through here in a realistic way that'll make this test + // useful. + var finalVal cty.Value + ctx.SetRootModuleArgumentFunc = func(addr addrs.InputVariable, v cty.Value) { + if addr.Name == "foo" { + t.Logf("set %s to %#v", addr.String(), v) + finalVal = v + } + } + ctx.GetVariableValueFunc = func(addr addrs.AbsInputVariableInstance) cty.Value { + if addr.String() != "var.foo" { + return cty.NilVal + } + t.Logf("reading final val for %s (%#v)", addr.String(), finalVal) + return finalVal + } + + n := &NodeRootVariable{ + Addr: addrs.InputVariable{Name: "foo"}, + Config: &configs.Variable{ + Name: "foo", + Type: cty.Number, + ConstraintType: cty.Number, + Validations: []*configs.VariableValidation{ + { + Condition: fakeHCLExpressionFunc(func(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { + // This returns true only if the given variable value + // is exactly cty.Number, which allows us to verify + // that we were given the value _after_ type + // conversion. + // This had previously not been handled correctly, + // as reported in: + // https://github.com/hashicorp/terraform/issues/29899 + vars := ctx.Variables["var"] + if vars == cty.NilVal || !vars.Type().IsObjectType() || !vars.Type().HasAttribute("foo") { + t.Logf("var.foo isn't available") + return cty.False, nil + } + val := vars.GetAttr("foo") + if val == cty.NilVal || val.Type() != cty.Number { + t.Logf("var.foo is %#v; want a number", val) + return cty.False, nil + } + return cty.True, nil + }), + ErrorMessage: "Must be a number.", + }, + }, + }, + RawValue: &InputValue{ + // Note: This is a string, but the variable's type constraint + // is number so it should be converted before use. + Value: cty.StringVal("5"), + SourceType: ValueFromUnknown, + }, + } - diags := n.Execute(ctx, walkApply) - if diags.HasErrors() { - t.Fatalf("unexpected error: %s", diags.Err()) + diags := n.Execute(ctx, walkApply) + if diags.HasErrors() { + t.Fatalf("unexpected error: %s", diags.Err()) + } + + if !ctx.SetRootModuleArgumentCalled { + t.Fatalf("ctx.SetRootModuleArgument wasn't called") + } + if got, want := ctx.SetRootModuleArgumentAddr.String(), "var.foo"; got != want { + t.Errorf("wrong address for ctx.SetRootModuleArgument\ngot: %s\nwant: %s", got, want) + } + if got, want := ctx.SetRootModuleArgumentValue, cty.NumberIntVal(5); !want.RawEquals(got) { + // NOTE: The given value was cty.Bool but the type constraint was + // cty.String, so it was NodeRootVariable's responsibility to convert + // as part of preparing the "final value". + t.Errorf("wrong value for ctx.SetRootModuleArgument\ngot: %#v\nwant: %#v", got, want) + } + }) +} + +// fakeHCLExpressionFunc is a fake implementation of hcl.Expression that just +// directly produces a value with direct Go code. +// +// An expression of this type has no references and so it cannot access any +// variables from the EvalContext unless something else arranges for them +// to be guaranteed available. For example, custom variable validations just +// unconditionally have access to the variable they are validating regardless +// of references. +type fakeHCLExpressionFunc func(*hcl.EvalContext) (cty.Value, hcl.Diagnostics) + +var _ hcl.Expression = fakeHCLExpressionFunc(nil) + +func (f fakeHCLExpressionFunc) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) { + return f(ctx) +} + +func (f fakeHCLExpressionFunc) Variables() []hcl.Traversal { + return nil +} + +func (f fakeHCLExpressionFunc) Range() hcl.Range { + return hcl.Range{ + Filename: "fake", + Start: hcl.InitialPos, + End: hcl.InitialPos, } +} +func (f fakeHCLExpressionFunc) StartRange() hcl.Range { + return f.Range() } diff --git a/internal/terraform/transform_variable.go b/internal/terraform/transform_variable.go index 86bd6a981aff..4262ea3d6db0 100644 --- a/internal/terraform/transform_variable.go +++ b/internal/terraform/transform_variable.go @@ -13,6 +13,8 @@ import ( // reach them. type RootVariableTransformer struct { Config *configs.Config + + RawValues InputValues } func (t *RootVariableTransformer) Transform(g *Graph) error { @@ -31,7 +33,8 @@ func (t *RootVariableTransformer) Transform(g *Graph) error { Addr: addrs.InputVariable{ Name: v.Name, }, - Config: v, + Config: v, + RawValue: t.RawValues[v.Name], } g.Add(node) } From 630500e7e9148f3c1fe7f439a9bdd1227809ca13 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Mon, 20 Dec 2021 16:38:52 -0800 Subject: [PATCH 3/4] 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.) --- internal/backend/unparsed_value.go | 17 +- internal/backend/unparsed_value_test.go | 2 +- internal/command/jsonplan/plan.go | 43 +++++- internal/terraform/context_apply.go | 15 ++ internal/terraform/context_apply2_test.go | 6 +- internal/terraform/context_apply_test.go | 59 ++++--- internal/terraform/context_eval.go | 2 +- internal/terraform/context_eval_test.go | 4 +- internal/terraform/context_import.go | 2 +- internal/terraform/context_plan.go | 69 ++++++++- internal/terraform/context_plan2_test.go | 2 +- internal/terraform/context_plan_test.go | 15 +- internal/terraform/eval_variable.go | 10 +- internal/terraform/variables.go | 135 ++++++---------- internal/terraform/variables_test.go | 180 +++------------------- 15 files changed, 255 insertions(+), 306 deletions(-) diff --git a/internal/backend/unparsed_value.go b/internal/backend/unparsed_value.go index 91c9825826e2..e7eadea9a1f8 100644 --- a/internal/backend/unparsed_value.go +++ b/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), } diff --git a/internal/backend/unparsed_value_test.go b/internal/backend/unparsed_value_test.go index 981c84a43e7a..8807d243d782 100644 --- a/internal/backend/unparsed_value_test.go +++ b/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", diff --git a/internal/command/jsonplan/plan.go b/internal/command/jsonplan/plan.go index 64d77c05a54e..06ed97961012 100644 --- a/internal/command/jsonplan/plan.go +++ b/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 } diff --git a/internal/terraform/context_apply.go b/internal/terraform/context_apply.go index 42520b03dd83..d3cd9dc2d575 100644 --- a/internal/terraform/context_apply.go +++ b/internal/terraform/context_apply.go @@ -87,6 +87,21 @@ func (c *Context) applyGraph(plan *plans.Plan, config *configs.Config, validate 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, diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index 6b87d409d5d2..512f3dab3c9c 100644 --- a/internal/terraform/context_apply2_test.go +++ b/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 diff --git a/internal/terraform/context_apply_test.go b/internal/terraform/context_apply_test.go index 28c5c788b502..46dcbd58b8b9 100644 --- a/internal/terraform/context_apply_test.go +++ b/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 { diff --git a/internal/terraform/context_eval.go b/internal/terraform/context_eval.go index c6af77635fc3..f9d0f649338b 100644 --- a/internal/terraform/context_eval.go +++ b/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 diff --git a/internal/terraform/context_eval_test.go b/internal/terraform/context_eval_test.go index dff6879833e7..0bd752935ecb 100644 --- a/internal/terraform/context_eval_test.go +++ b/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()) } diff --git a/internal/terraform/context_import.go b/internal/terraform/context_import.go index b5417405fbb0..d809d6bb9dca 100644 --- a/internal/terraform/context_import.go +++ b/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{ diff --git a/internal/terraform/context_plan.go b/internal/terraform/context_plan.go index 0b3c97f14559..f114b2a5f7dc 100644 --- a/internal/terraform/context_plan.go +++ b/internal/terraform/context_plan.go @@ -21,10 +21,42 @@ import ( // PlanOpts are the various options that affect the details of how Terraform // will build a plan. type PlanOpts struct { - Mode plans.Mode - SkipRefresh bool + // Mode defines what variety of plan the caller wishes to create. + // Refer to the documentation of the plans.Mode type and its values + // for more information. + Mode plans.Mode + + // SkipRefresh specifies to trust that the current values for managed + // resource instances in the prior state are accurate and to therefore + // disable the usual step of fetching updated values for each resource + // instance using its corresponding provider. + SkipRefresh bool + + // SetVariables are the raw values for root module variables as provided + // by the user who is requesting the run, prior to any normalization or + // substitution of defaults. See the documentation for the InputValue + // type for more information on how to correctly populate this. SetVariables InputValues - Targets []addrs.Targetable + + // If Targets has a non-zero length then it activates targeted planning + // mode, where Terraform will take actions only for resource instances + // mentioned in this set and any other objects those resource instances + // depend on. + // + // Targeted planning mode is intended for exceptional use only, + // and so populating this field will cause Terraform to generate extra + // warnings as part of the planning result. + Targets []addrs.Targetable + + // ForceReplace is a set of resource instance addresses whose corresponding + // objects should be forced planned for replacement if the provider's + // plan would otherwise have been to either update the object in-place or + // to take no action on it at all. + // + // A typical use of this argument is to ask Terraform to replace an object + // which the user has determined is somehow degraded (via information from + // outside of Terraform), thereby hopefully replacing it with a + // fully-functional new object. ForceReplace []addrs.AbsResourceInstance } @@ -99,8 +131,6 @@ func (c *Context) Plan(config *configs.Config, prevRunState *states.State, opts return nil, diags } - variables := mergeDefaultInputVariableValues(opts.SetVariables, config.Module.Variables) - // 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 // caller's responsibility to have already handled the decoding of these @@ -108,7 +138,7 @@ func (c *Context) Plan(config *configs.Config, prevRunState *states.State, opts // user-friendly error messages if they are not all present, and so // the error message from checkInputVariables should never be seen and // includes language asking the user to report a bug. - varDiags := checkInputVariables(config.Module.Variables, variables) + varDiags := checkInputVariables(config.Module.Variables, opts.SetVariables) diags = diags.Append(varDiags) if len(opts.Targets) > 0 { @@ -139,8 +169,12 @@ The -target option is not for routine use, and is provided only for exceptional } // convert the variables into the format expected for the plan - varVals := make(map[string]plans.DynamicValue, len(variables)) - for k, iv := range variables { + varVals := make(map[string]plans.DynamicValue, len(opts.SetVariables)) + for k, iv := range opts.SetVariables { + if iv.Value == cty.NilVal { + continue // We only record values that the caller actually set + } + // We use cty.DynamicPseudoType here so that we'll save both the // value _and_ its dynamic type in the plan, so we can recover // exactly the same value later. @@ -172,6 +206,25 @@ var DefaultPlanOpts = &PlanOpts{ Mode: plans.NormalMode, } +// SimplePlanOpts is a constructor to help with creating "simple" values of +// PlanOpts which only specify a mode and input variables. +// +// This helper function is primarily intended for use in straightforward +// tests that don't need any of the more "esoteric" planning options. For +// handling real user requests to run Terraform, it'd probably be better +// to construct a *PlanOpts value directly and provide a way for the user +// to set values for all of its fields. +// +// The "mode" and "setVariables" arguments become the values of the "Mode" +// and "SetVariables" fields in the result. Refer to the PlanOpts type +// documentation to learn about the meanings of those fields. +func SimplePlanOpts(mode plans.Mode, setVariables InputValues) *PlanOpts { + return &PlanOpts{ + Mode: mode, + SetVariables: setVariables, + } +} + func (c *Context) plan(config *configs.Config, prevRunState *states.State, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 006e9e932f68..d1771b1f397f 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -205,7 +205,7 @@ data "test_data_source" "foo" { }, }) - plan, diags := ctx.Plan(m, state, DefaultPlanOpts) + plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))) assertNoErrors(t, diags) for _, res := range plan.Changes.Resources { diff --git a/internal/terraform/context_plan_test.go b/internal/terraform/context_plan_test.go index cfd51da8cc43..9cf7e875ebe1 100644 --- a/internal/terraform/context_plan_test.go +++ b/internal/terraform/context_plan_test.go @@ -405,7 +405,7 @@ func TestContext2Plan_moduleExpand(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("unexpected errors: %s", diags.Err()) } @@ -1175,7 +1175,7 @@ func TestContext2Plan_moduleProviderVar(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("unexpected errors: %s", diags.Err()) } @@ -2242,7 +2242,7 @@ func TestContext2Plan_countModuleStatic(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("unexpected errors: %s", diags.Err()) } @@ -2295,7 +2295,7 @@ func TestContext2Plan_countModuleStaticGrandchild(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("unexpected errors: %s", diags.Err()) } @@ -3938,7 +3938,7 @@ func TestContext2Plan_taintDestroyInterpolatedCountRace(t *testing.T) { }, }) - plan, diags := ctx.Plan(m, state.DeepCopy(), DefaultPlanOpts) + plan, diags := ctx.Plan(m, state.DeepCopy(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))) if diags.HasErrors() { t.Fatalf("unexpected errors: %s", diags.Err()) } @@ -5481,7 +5481,7 @@ func TestContext2Plan_variableSensitivity(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("unexpected errors: %s", diags.Err()) } @@ -5544,6 +5544,7 @@ func TestContext2Plan_variableSensitivityModule(t *testing.T) { plan, diags := ctx.Plan(m, states.NewState(), &PlanOpts{ Mode: plans.NormalMode, SetVariables: InputValues{ + "sensitive_var": {Value: cty.NilVal}, "another_var": &InputValue{ Value: cty.StringVal("boop"), SourceType: ValueFromCaller, @@ -6657,7 +6658,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))) if diags.HasErrors() { t.Fatal(diags.Err()) } diff --git a/internal/terraform/eval_variable.go b/internal/terraform/eval_variable.go index 1c16069ad4c1..bbafbe11c250 100644 --- a/internal/terraform/eval_variable.go +++ b/internal/terraform/eval_variable.go @@ -45,9 +45,11 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c log.Printf("[TRACE] prepareFinalInputVariableValue: %s has no defined value", addr) if cfg.Required() { // NOTE: The CLI layer typically checks for itself whether all of - // the required _root_ module variables are not set, which would - // mask this error. We can get here for child module variables, - // though. + // the required _root_ module variables are set, which would + // mask this error with a more specific one that refers to the + // CLI features for setting such variables. We can get here for + // child module variables, though. + log.Printf("[ERROR] prepareFinalInputVariableValue: %s is required but is not set", addr) diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: `Required variable not set`, @@ -64,6 +66,7 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c val, err := convert.Convert(given, convertTy) if err != nil { + log.Printf("[ERROR] prepareFinalInputVariableValue: %s has unsuitable type\n got: %s\n want: %s", addr, given.Type(), convertTy) diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Invalid value for module argument", @@ -93,6 +96,7 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c if defaultVal != cty.NilVal { val = defaultVal } else { + log.Printf("[ERROR] prepareFinalInputVariableValue: %s is non-nullable but set to null, and is required", addr) diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: `Required variable not set`, diff --git a/internal/terraform/variables.go b/internal/terraform/variables.go index 7a6ace0eee5c..f5f03d156bf6 100644 --- a/internal/terraform/variables.go +++ b/internal/terraform/variables.go @@ -3,18 +3,50 @@ package terraform import ( "fmt" - "github.com/hashicorp/hcl/v2" "github.com/zclconf/go-cty/cty" - "github.com/zclconf/go-cty/cty/convert" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/tfdiags" ) -// InputValue represents a value for a variable in the root module, provided -// as part of the definition of an operation. +// InputValue represents a raw value vor a root module input variable as +// provided by the external caller into a function like terraform.Context.Plan. +// +// InputValue should represent as directly as possible what the user set the +// variable to, without any attempt to convert the value to the variable's +// type constraint or substitute the configured default values for variables +// that wasn't set. Those adjustments will be handled by Terraform Core itself +// as part of performing the requested operation. +// +// A Terraform Core caller must provide an InputValue object for each of the +// variables declared in the root module, even if the end user didn't provide +// an explicit value for some of them. See the Value field documentation for +// how to handle that situation. type InputValue struct { - Value cty.Value + // Value is the raw value as provided by the user as part of the plan + // options, or a corresponding similar data structure for non-plan + // operations. + // + // If a particular variable declared in the root module is _not_ set by + // the user then the caller must still provide an InputValue for it but + // must set Value to cty.NilVal to represent the absense of a value. + // This requirement is to help detect situations where the caller isn't + // correctly detecting and handling all of the declared variables. + // + // For historical reasons it's important that callers distinguish the + // situation of the value not being set at all (cty.NilVal) from the + // situation of it being explicitly set to null (a cty.NullVal result): + // for "nullable" input variables that distinction unfortunately decides + // whether the final value will be the variable's default or will be + // explicitly null. + Value cty.Value + + // SourceType is a high-level category for where the value of Value + // came from, which Terraform Core uses to tailor some of its error + // messages to be more helpful to the user. + // + // Some SourceType values should be accompanied by a populated SourceRange + // value. See that field's documentation below for more information. SourceType ValueSourceType // SourceRange provides source location information for values whose @@ -129,23 +161,6 @@ func (vv InputValues) JustValues() map[string]cty.Value { return ret } -// DefaultVariableValues returns an InputValues map representing the default -// values specified for variables in the given configuration map. -func DefaultVariableValues(configs map[string]*configs.Variable) InputValues { - ret := make(InputValues) - for k, c := range configs { - if c.Default == cty.NilVal { - continue - } - ret[k] = &InputValue{ - Value: c.Default, - SourceType: ValueFromConfig, - SourceRange: tfdiags.SourceRangeFromHCL(c.DeclRange), - } - } - return ret -} - // SameValues returns true if the given InputValues has the same values as // the receiever, disregarding the source types and source ranges. // @@ -227,21 +242,15 @@ func (vv InputValues) Identical(other InputValues) bool { return true } -func mergeDefaultInputVariableValues(setVals InputValues, rootVarsConfig map[string]*configs.Variable) InputValues { - var variables InputValues - - // Default variables from the configuration seed our map. - variables = DefaultVariableValues(rootVarsConfig) - - // Variables provided by the caller (from CLI, environment, etc) can - // override the defaults. - variables = variables.Override(setVals) - - return variables -} - -// checkInputVariables ensures that variable values supplied at the UI conform -// to their corresponding declarations in configuration. +// checkInputVariables ensures that the caller provided an InputValue +// definition for each root module variable declared in the configuration. +// The caller must provide an InputVariables with keys exactly matching +// the declared variables, though some of them may be marked explicitly +// unset by their values being cty.NilVal. +// +// This doesn't perform any type checking, default value substitution, or +// validation checks. Those are all handled during a graph walk when we +// visit the graph nodes representing each root variable. // // The set of values is considered valid only if the returned diagnostics // does not contain errors. A valid set of values may still produce warnings, @@ -249,11 +258,12 @@ func mergeDefaultInputVariableValues(setVals InputValues, rootVarsConfig map[str func checkInputVariables(vcs map[string]*configs.Variable, vs InputValues) tfdiags.Diagnostics { var diags tfdiags.Diagnostics - for name, vc := range vcs { - val, isSet := vs[name] + for name := range vcs { + _, isSet := vs[name] if !isSet { - // Always an error, since the caller should already have included - // default values from the configuration in the values map. + // Always an error, since the caller should have produced an + // item with Value: cty.NilVal to be explicit that it offered + // an opportunity to set this variable. diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, "Unassigned variable", @@ -261,49 +271,6 @@ func checkInputVariables(vcs map[string]*configs.Variable, vs InputValues) tfdia )) continue } - - // A given value is valid if it can convert to the desired type. - _, err := convert.Convert(val.Value, vc.ConstraintType) - if err != nil { - switch val.SourceType { - case ValueFromConfig, ValueFromAutoFile, ValueFromNamedFile: - // We have source location information for these. - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid value for input variable", - Detail: fmt.Sprintf("The given value is not valid for variable %q: %s.", name, err), - Subject: val.SourceRange.ToHCL().Ptr(), - }) - case ValueFromEnvVar: - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Invalid value for input variable", - fmt.Sprintf("The environment variable TF_VAR_%s does not contain a valid value for variable %q: %s.", name, name, err), - )) - case ValueFromCLIArg: - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Invalid value for input variable", - fmt.Sprintf("The argument -var=\"%s=...\" does not contain a valid value for variable %q: %s.", name, name, err), - )) - case ValueFromInput: - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Invalid value for input variable", - fmt.Sprintf("The value entered for variable %q is not valid: %s.", name, err), - )) - default: - // The above gets us good coverage for the situations users - // are likely to encounter with their own inputs. The other - // cases are generally implementation bugs, so we'll just - // use a generic error for these. - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Invalid value for input variable", - fmt.Sprintf("The value provided for variable %q is not valid: %s.", name, err), - )) - } - } } // Check for any variables that are assigned without being configured. diff --git a/internal/terraform/variables_test.go b/internal/terraform/variables_test.go index 41decbae2a38..6e53a95750a2 100644 --- a/internal/terraform/variables_test.go +++ b/internal/terraform/variables_test.go @@ -3,166 +3,10 @@ package terraform import ( "testing" - "github.com/davecgh/go-spew/spew" - "github.com/hashicorp/terraform/internal/tfdiags" - - "github.com/go-test/deep" + "github.com/hashicorp/terraform/internal/configs" "github.com/zclconf/go-cty/cty" ) -func TestVariables(t *testing.T) { - tests := map[string]struct { - Module string - Override map[string]cty.Value - Want InputValues - }{ - "config only": { - "vars-basic", - nil, - InputValues{ - "a": &InputValue{ - Value: cty.StringVal("foo"), - SourceType: ValueFromConfig, - SourceRange: tfdiags.SourceRange{ - Filename: "testdata/vars-basic/main.tf", - Start: tfdiags.SourcePos{Line: 1, Column: 1, Byte: 0}, - End: tfdiags.SourcePos{Line: 1, Column: 13, Byte: 12}, - }, - }, - "b": &InputValue{ - Value: cty.ListValEmpty(cty.String), - SourceType: ValueFromConfig, - SourceRange: tfdiags.SourceRange{ - Filename: "testdata/vars-basic/main.tf", - Start: tfdiags.SourcePos{Line: 6, Column: 1, Byte: 55}, - End: tfdiags.SourcePos{Line: 6, Column: 13, Byte: 67}, - }, - }, - "c": &InputValue{ - Value: cty.MapValEmpty(cty.String), - SourceType: ValueFromConfig, - SourceRange: tfdiags.SourceRange{ - Filename: "testdata/vars-basic/main.tf", - Start: tfdiags.SourcePos{Line: 11, Column: 1, Byte: 113}, - End: tfdiags.SourcePos{Line: 11, Column: 13, Byte: 125}, - }, - }, - }, - }, - - "override": { - "vars-basic", - map[string]cty.Value{ - "a": cty.StringVal("bar"), - "b": cty.ListVal([]cty.Value{ - cty.StringVal("foo"), - cty.StringVal("bar"), - }), - "c": cty.MapVal(map[string]cty.Value{ - "foo": cty.StringVal("bar"), - }), - }, - InputValues{ - "a": &InputValue{ - Value: cty.StringVal("bar"), - SourceType: ValueFromCaller, - }, - "b": &InputValue{ - Value: cty.ListVal([]cty.Value{ - cty.StringVal("foo"), - cty.StringVal("bar"), - }), - SourceType: ValueFromCaller, - }, - "c": &InputValue{ - Value: cty.MapVal(map[string]cty.Value{ - "foo": cty.StringVal("bar"), - }), - SourceType: ValueFromCaller, - }, - }, - }, - - "bools: config only": { - "vars-basic-bool", - nil, - InputValues{ - "a": &InputValue{ - Value: cty.True, - SourceType: ValueFromConfig, - SourceRange: tfdiags.SourceRange{ - Filename: "testdata/vars-basic-bool/main.tf", - Start: tfdiags.SourcePos{Line: 4, Column: 1, Byte: 177}, - End: tfdiags.SourcePos{Line: 4, Column: 13, Byte: 189}, - }, - }, - "b": &InputValue{ - Value: cty.False, - SourceType: ValueFromConfig, - SourceRange: tfdiags.SourceRange{ - Filename: "testdata/vars-basic-bool/main.tf", - Start: tfdiags.SourcePos{Line: 8, Column: 1, Byte: 214}, - End: tfdiags.SourcePos{Line: 8, Column: 13, Byte: 226}, - }, - }, - }, - }, - - "bools: override with string": { - "vars-basic-bool", - map[string]cty.Value{ - "a": cty.StringVal("foo"), - "b": cty.StringVal("bar"), - }, - InputValues{ - "a": &InputValue{ - Value: cty.StringVal("foo"), - SourceType: ValueFromCaller, - }, - "b": &InputValue{ - Value: cty.StringVal("bar"), - SourceType: ValueFromCaller, - }, - }, - }, - - "bools: override with bool": { - "vars-basic-bool", - map[string]cty.Value{ - "a": cty.False, - "b": cty.True, - }, - InputValues{ - "a": &InputValue{ - Value: cty.False, - SourceType: ValueFromCaller, - }, - "b": &InputValue{ - Value: cty.True, - SourceType: ValueFromCaller, - }, - }, - }, - } - - for name, test := range tests { - // Wrapped in a func so we can get defers to work - t.Run(name, func(t *testing.T) { - m := testModule(t, test.Module) - fromConfig := DefaultVariableValues(m.Module.Variables) - overrides := InputValuesFromCaller(test.Override) - got := fromConfig.Override(overrides) - - if !got.Identical(test.Want) { - t.Errorf("wrong result\ngot: %swant: %s", spew.Sdump(got), spew.Sdump(test.Want)) - } - for _, problem := range deep.Equal(got, test.Want) { - t.Errorf(problem) - } - }) - } -} - func TestCheckInputVariables(t *testing.T) { c := testModule(t, "input-variables") @@ -280,3 +124,25 @@ func TestCheckInputVariables(t *testing.T) { } }) } + +// testInputValuesUnset is a helper for constructing InputValues values for +// situations where all of the root module variables are optional and a +// test case intends to just use those default values and not override them +// at all. +// +// In other words, this constructs an InputValues with one entry per given +// input variable declaration where all of them are declared as unset. +func testInputValuesUnset(decls map[string]*configs.Variable) InputValues { + if len(decls) == 0 { + return nil + } + + ret := make(InputValues, len(decls)) + for name := range decls { + ret[name] = &InputValue{ + Value: cty.NilVal, + SourceType: ValueFromUnknown, + } + } + return ret +} From d1de8bab2767e4f260da64f46ae5a20a639bba21 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 21 Dec 2021 18:04:24 -0800 Subject: [PATCH 4/4] core: More accurate error message for invalid variable values In earlier Terraform versions we had an extra validation step prior to the graph walk which tried to partially validate root module input variable values (just checking their type constraints) and then return error messages which specified as accurately as possible where the value had originally come from. We're now handling that sort of validation exclusively during the graph walk so that we can share the main logic between both root module and child module variable values, but previously that shared code wasn't able to generate such specific information about where the values had originated, because it was adapted from code originally written to only deal with child module variables. Here then we restore a similar level of detail as before, when we're processing root module variables. For child module variables, we use synthetic InputValue objects which state that the value was declared in the configuration, thus causing us to produce a similar sort of error message as we would've before which includes a source range covering the argument expression in the calling module block. --- internal/terraform/eval_variable.go | 84 +++++++++--- internal/terraform/eval_variable_test.go | 143 ++++++++++++++++++++- internal/terraform/node_module_variable.go | 11 +- internal/terraform/node_root_variable.go | 17 ++- internal/terraform/variables.go | 30 ++++- 5 files changed, 254 insertions(+), 31 deletions(-) diff --git a/internal/terraform/eval_variable.go b/internal/terraform/eval_variable.go index bbafbe11c250..fd57a136f290 100644 --- a/internal/terraform/eval_variable.go +++ b/internal/terraform/eval_variable.go @@ -12,7 +12,7 @@ import ( "github.com/zclconf/go-cty/cty/convert" ) -func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given cty.Value, valRange tfdiags.SourceRange, cfg *configs.Variable) (cty.Value, tfdiags.Diagnostics) { +func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, raw *InputValue, cfg *configs.Variable) (cty.Value, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics convertTy := cfg.ConstraintType @@ -41,6 +41,29 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c } } + var sourceRange tfdiags.SourceRange + var nonFileSource string + if raw.HasSourceRange() { + sourceRange = raw.SourceRange + } else { + // If the value came from a place that isn't a file and thus doesn't + // have its own source range, we'll use the declaration range as + // our source range and generate some slightly different error + // messages. + sourceRange = tfdiags.SourceRangeFromHCL(cfg.DeclRange) + switch raw.SourceType { + case ValueFromCLIArg: + nonFileSource = fmt.Sprintf("set using -var=\"%s=...\"", addr.Variable.Name) + case ValueFromEnvVar: + nonFileSource = fmt.Sprintf("set using the TF_VAR_%s environment variable", addr.Variable.Name) + case ValueFromInput: + nonFileSource = "set using an interactive prompt" + default: + nonFileSource = "set from outside of the configuration" + } + } + + given := raw.Value if given == cty.NilVal { // The variable wasn't set at all (even to null) log.Printf("[TRACE] prepareFinalInputVariableValue: %s has no defined value", addr) if cfg.Required() { @@ -54,7 +77,7 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c Severity: hcl.DiagError, Summary: `Required variable not set`, Detail: fmt.Sprintf(`The variable %q is required, but is not set.`, addr.Variable.Name), - Subject: valRange.ToHCL().Ptr(), + Subject: cfg.DeclRange.Ptr(), }) // We'll return a placeholder unknown value to avoid producing // redundant downstream errors. @@ -67,15 +90,27 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c val, err := convert.Convert(given, convertTy) if err != nil { log.Printf("[ERROR] prepareFinalInputVariableValue: %s has unsuitable type\n got: %s\n want: %s", addr, given.Type(), convertTy) - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid value for module argument", - Detail: fmt.Sprintf( - "The given value is not suitable for child module variable %q defined at %s: %s.", - cfg.Name, cfg.DeclRange.String(), err, - ), - Subject: valRange.ToHCL().Ptr(), - }) + if nonFileSource != "" { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid value for input variable", + Detail: fmt.Sprintf( + "Unsuitable value for %s %s: %s.", + addr, nonFileSource, err, + ), + Subject: cfg.DeclRange.Ptr(), + }) + } else { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid value for input variable", + Detail: fmt.Sprintf( + "The given value is not suitable for %s declared at %s: %s.", + addr, cfg.DeclRange.String(), err, + ), + Subject: sourceRange.ToHCL().Ptr(), + }) + } // We'll return a placeholder unknown value to avoid producing // redundant downstream errors. return cty.UnknownVal(cfg.Type), diags @@ -97,12 +132,27 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c val = defaultVal } else { log.Printf("[ERROR] prepareFinalInputVariableValue: %s is non-nullable but set to null, and is required", addr) - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: `Required variable not set`, - Detail: fmt.Sprintf(`The variable %q is required, but the given value is null.`, addr.Variable.Name), - Subject: valRange.ToHCL().Ptr(), - }) + if nonFileSource != "" { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Required variable not set`, + Detail: fmt.Sprintf( + "Unsuitable value for %s %s: required variable may not be set to null.", + addr, nonFileSource, + ), + Subject: cfg.DeclRange.Ptr(), + }) + } else { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Required variable not set`, + Detail: fmt.Sprintf( + "The given value is not suitable for %s defined at %s: required variable may not be set to null.", + addr, cfg.DeclRange.String(), + ), + Subject: sourceRange.ToHCL().Ptr(), + }) + } // Stub out our return value so that the semantic checker doesn't // produce redundant downstream errors. val = cty.UnknownVal(cfg.Type) diff --git a/internal/terraform/eval_variable_test.go b/internal/terraform/eval_variable_test.go index fa4048fed41f..0ebea982f57b 100644 --- a/internal/terraform/eval_variable_test.go +++ b/internal/terraform/eval_variable_test.go @@ -4,6 +4,7 @@ import ( "fmt" "testing" + "github.com/hashicorp/hcl/v2" "github.com/zclconf/go-cty/cty" "github.com/hashicorp/terraform/internal/addrs" @@ -65,6 +66,13 @@ func TestPrepareFinalInputVariableValue(t *testing.T) { }) variableConfigs := cfg.Module.Variables + // Because we loaded our pseudo-module from a temporary file, the + // declaration source ranges will have unpredictable filenames. We'll + // fix that here just to make things easier below. + for _, vc := range variableConfigs { + vc.DeclRange.Filename = "main.tf" + } + tests := []struct { varName string given cty.Value @@ -264,7 +272,7 @@ func TestPrepareFinalInputVariableValue(t *testing.T) { "required", cty.NullVal(cty.DynamicPseudoType), cty.UnknownVal(cty.DynamicPseudoType), - `Required variable not set: The variable "required" is required, but the given value is null.`, + `Required variable not set: Unsuitable value for var.required set from outside of the configuration: required variable may not be set to null.`, }, { "required", @@ -316,7 +324,7 @@ func TestPrepareFinalInputVariableValue(t *testing.T) { "constrained_string_required", cty.NullVal(cty.DynamicPseudoType), cty.UnknownVal(cty.String), - `Required variable not set: The variable "constrained_string_required" is required, but the given value is null.`, + `Required variable not set: Unsuitable value for var.constrained_string_required set from outside of the configuration: required variable may not be set to null.`, }, { "constrained_string_required", @@ -401,8 +409,13 @@ func TestPrepareFinalInputVariableValue(t *testing.T) { test.given, ) + rawVal := &InputValue{ + Value: test.given, + SourceType: ValueFromCaller, + } + got, diags := prepareFinalInputVariableValue( - varAddr, test.given, tfdiags.SourceRangeFromHCL(varCfg.DeclRange), varCfg, + varAddr, rawVal, varCfg, ) if test.wantErr != "" { @@ -423,4 +436,128 @@ func TestPrepareFinalInputVariableValue(t *testing.T) { } }) } + + t.Run("SourceType error message variants", func(t *testing.T) { + tests := []struct { + SourceType ValueSourceType + SourceRange tfdiags.SourceRange + WantTypeErr string + WantNullErr string + }{ + { + ValueFromUnknown, + tfdiags.SourceRange{}, + `Invalid value for input variable: Unsuitable value for var.constrained_string_required set from outside of the configuration: string required.`, + `Required variable not set: Unsuitable value for var.constrained_string_required set from outside of the configuration: required variable may not be set to null.`, + }, + { + ValueFromConfig, + tfdiags.SourceRange{ + Filename: "example.tf", + Start: tfdiags.SourcePos(hcl.InitialPos), + End: tfdiags.SourcePos(hcl.InitialPos), + }, + `Invalid value for input variable: The given value is not suitable for var.constrained_string_required declared at main.tf:32,3-41: string required.`, + `Required variable not set: The given value is not suitable for var.constrained_string_required defined at main.tf:32,3-41: required variable may not be set to null.`, + }, + { + ValueFromAutoFile, + tfdiags.SourceRange{ + Filename: "example.auto.tfvars", + Start: tfdiags.SourcePos(hcl.InitialPos), + End: tfdiags.SourcePos(hcl.InitialPos), + }, + `Invalid value for input variable: The given value is not suitable for var.constrained_string_required declared at main.tf:32,3-41: string required.`, + `Required variable not set: The given value is not suitable for var.constrained_string_required defined at main.tf:32,3-41: required variable may not be set to null.`, + }, + { + ValueFromNamedFile, + tfdiags.SourceRange{ + Filename: "example.tfvars", + Start: tfdiags.SourcePos(hcl.InitialPos), + End: tfdiags.SourcePos(hcl.InitialPos), + }, + `Invalid value for input variable: The given value is not suitable for var.constrained_string_required declared at main.tf:32,3-41: string required.`, + `Required variable not set: The given value is not suitable for var.constrained_string_required defined at main.tf:32,3-41: required variable may not be set to null.`, + }, + { + ValueFromCLIArg, + tfdiags.SourceRange{}, + `Invalid value for input variable: Unsuitable value for var.constrained_string_required set using -var="constrained_string_required=...": string required.`, + `Required variable not set: Unsuitable value for var.constrained_string_required set using -var="constrained_string_required=...": required variable may not be set to null.`, + }, + { + ValueFromEnvVar, + tfdiags.SourceRange{}, + `Invalid value for input variable: Unsuitable value for var.constrained_string_required set using the TF_VAR_constrained_string_required environment variable: string required.`, + `Required variable not set: Unsuitable value for var.constrained_string_required set using the TF_VAR_constrained_string_required environment variable: required variable may not be set to null.`, + }, + { + ValueFromInput, + tfdiags.SourceRange{}, + `Invalid value for input variable: Unsuitable value for var.constrained_string_required set using an interactive prompt: string required.`, + `Required variable not set: Unsuitable value for var.constrained_string_required set using an interactive prompt: required variable may not be set to null.`, + }, + { + // NOTE: This isn't actually a realistic case for this particular + // function, because if we have a value coming from a plan then + // we must be in the apply step, and we shouldn't be able to + // get past the plan step if we have invalid variable values, + // and during planning we'll always have other source types. + ValueFromPlan, + tfdiags.SourceRange{}, + `Invalid value for input variable: Unsuitable value for var.constrained_string_required set from outside of the configuration: string required.`, + `Required variable not set: Unsuitable value for var.constrained_string_required set from outside of the configuration: required variable may not be set to null.`, + }, + { + ValueFromCaller, + tfdiags.SourceRange{}, + `Invalid value for input variable: Unsuitable value for var.constrained_string_required set from outside of the configuration: string required.`, + `Required variable not set: Unsuitable value for var.constrained_string_required set from outside of the configuration: required variable may not be set to null.`, + }, + } + + for _, test := range tests { + t.Run(fmt.Sprintf("%s %s", test.SourceType, test.SourceRange.StartString()), func(t *testing.T) { + varAddr := addrs.InputVariable{Name: "constrained_string_required"}.Absolute(addrs.RootModuleInstance) + varCfg := variableConfigs[varAddr.Variable.Name] + t.Run("type error", func(t *testing.T) { + rawVal := &InputValue{ + Value: cty.EmptyObjectVal, + SourceType: test.SourceType, + SourceRange: test.SourceRange, + } + + _, diags := prepareFinalInputVariableValue( + varAddr, rawVal, varCfg, + ) + if !diags.HasErrors() { + t.Fatalf("unexpected success; want error") + } + + if got, want := diags.Err().Error(), test.WantTypeErr; got != want { + t.Errorf("wrong error\ngot: %s\nwant: %s", got, want) + } + }) + t.Run("null error", func(t *testing.T) { + rawVal := &InputValue{ + Value: cty.NullVal(cty.DynamicPseudoType), + SourceType: test.SourceType, + SourceRange: test.SourceRange, + } + + _, diags := prepareFinalInputVariableValue( + varAddr, rawVal, varCfg, + ) + if !diags.HasErrors() { + t.Fatalf("unexpected success; want error") + } + + if got, want := diags.Err().Error(), test.WantNullErr; got != want { + t.Errorf("wrong error\ngot: %s\nwant: %s", got, want) + } + }) + }) + } + }) } diff --git a/internal/terraform/node_module_variable.go b/internal/terraform/node_module_variable.go index 321cd874845f..ae3450be528e 100644 --- a/internal/terraform/node_module_variable.go +++ b/internal/terraform/node_module_variable.go @@ -227,7 +227,16 @@ func (n *nodeModuleVariable) evalModuleCallArgument(ctx EvalContext, validateOnl errSourceRange = tfdiags.SourceRangeFromHCL(n.Config.DeclRange) // we use the declaration range as a fallback for an undefined variable } - finalVal, moreDiags := prepareFinalInputVariableValue(n.Addr, givenVal, errSourceRange, n.Config) + // We construct a synthetic InputValue here to pretend as if this were + // a root module variable set from outside, just as a convenience so we + // can reuse the InputValue type for this. + rawVal := &InputValue{ + Value: givenVal, + SourceType: ValueFromConfig, + SourceRange: errSourceRange, + } + + finalVal, moreDiags := prepareFinalInputVariableValue(n.Addr, rawVal, n.Config) diags = diags.Append(moreDiags) return finalVal, diags.ErrWithWarnings() diff --git a/internal/terraform/node_root_variable.go b/internal/terraform/node_root_variable.go index d023be3500ba..33f439d7cd9a 100644 --- a/internal/terraform/node_root_variable.go +++ b/internal/terraform/node_root_variable.go @@ -65,20 +65,23 @@ func (n *NodeRootVariable) Execute(ctx EvalContext, op walkOperation) tfdiags.Di return nil } - var givenVal cty.Value - if n.RawValue != nil { - givenVal = n.RawValue.Value - } else { + givenVal := n.RawValue + if givenVal == nil { // We'll use cty.NilVal to represent the variable not being set at // all, which for historical reasons is unfortunately different than - // explicitly setting it to null in some cases. - givenVal = cty.NilVal + // explicitly setting it to null in some cases. In normal code we + // should never get here because all variables should have raw + // values, but we can get here in some historical tests that call + // in directly and don't necessarily obey the rules. + givenVal = &InputValue{ + Value: cty.NilVal, + SourceType: ValueFromUnknown, + } } finalVal, moreDiags := prepareFinalInputVariableValue( addr, givenVal, - tfdiags.SourceRangeFromHCL(n.Config.DeclRange), n.Config, ) diags = diags.Append(moreDiags) diff --git a/internal/terraform/variables.go b/internal/terraform/variables.go index f5f03d156bf6..a60f187003dd 100644 --- a/internal/terraform/variables.go +++ b/internal/terraform/variables.go @@ -9,7 +9,7 @@ import ( "github.com/hashicorp/terraform/internal/tfdiags" ) -// InputValue represents a raw value vor a root module input variable as +// InputValue represents a raw value for a root module input variable as // provided by the external caller into a function like terraform.Context.Plan. // // InputValue should represent as directly as possible what the user set the @@ -22,6 +22,11 @@ import ( // variables declared in the root module, even if the end user didn't provide // an explicit value for some of them. See the Value field documentation for // how to handle that situation. +// +// Terraform Core also internally uses InputValue to represent the raw value +// provided for a variable in a child module call, following the same +// conventions. However, that's an implementation detail not visible to +// outside callers. type InputValue struct { // Value is the raw value as provided by the user as part of the plan // options, or a corresponding similar data structure for non-plan @@ -50,8 +55,9 @@ type InputValue struct { SourceType ValueSourceType // SourceRange provides source location information for values whose - // SourceType is either ValueFromConfig or ValueFromFile. It is not - // populated for other source types, and so should not be used. + // SourceType is either ValueFromConfig, ValueFromNamedFile, or + // ValueForNormalFile. It is not populated for other source types, and so + // should not be used. SourceRange tfdiags.SourceRange } @@ -106,6 +112,24 @@ func (v *InputValue) GoString() string { } } +// HasSourceRange returns true if the reciever has a source type for which +// we expect the SourceRange field to be populated with a valid range. +func (v *InputValue) HasSourceRange() bool { + return v.SourceType.HasSourceRange() +} + +// HasSourceRange returns true if the reciever is one of the source types +// that is used along with a valid SourceRange field when appearing inside an +// InputValue object. +func (v ValueSourceType) HasSourceRange() bool { + switch v { + case ValueFromConfig, ValueFromAutoFile, ValueFromNamedFile: + return true + default: + return false + } +} + func (v ValueSourceType) GoString() string { return fmt.Sprintf("terraform.%s", v) }