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 e5d2702bc5dc..d3cd9dc2d575 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,58 @@ Note that the -target option is not suitable for routine use, and is provided on } func (c *Context) applyGraph(plan *plans.Plan, config *configs.Config, validate bool) (*Graph, walkOperation, tfdiags.Diagnostics) { - graph, diags := (&ApplyGraphBuilder{ - Config: config, - Changes: plan.Changes, - State: plan.PriorState, - Plugins: c.plugins, - Targets: plan.TargetAddrs, - ForceReplace: plan.ForceReplaceAddrs, - Validate: validate, + var diags tfdiags.Diagnostics + + variables := InputValues{} + for name, dyVal := range plan.VariableValues { + val, err := dyVal.Decode(cty.DynamicPseudoType) + if err != nil { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Invalid variable value in plan", + fmt.Sprintf("Invalid value for variable %q recorded in plan file: %s.", name, err), + )) + continue + } + + variables[name] = &InputValue{ + Value: val, + SourceType: ValueFromPlan, + } + } + if diags.HasErrors() { + return nil, walkApply, diags + } + + // The plan.VariableValues field only records variables that were actually + // set by the caller in the PlanOpts, so we may need to provide + // placeholders for any other variables that the user didn't set, in + // which case Terraform will once again use the default value from the + // configuration when we visit these variables during the graph walk. + for name := range config.Module.Variables { + if _, ok := variables[name]; ok { + continue + } + variables[name] = &InputValue{ + Value: cty.NilVal, + SourceType: ValueFromPlan, + } + } + + graph, moreDiags := (&ApplyGraphBuilder{ + Config: config, + Changes: plan.Changes, + State: plan.PriorState, + RootVariableValues: variables, + Plugins: c.plugins, + Targets: plan.TargetAddrs, + ForceReplace: plan.ForceReplaceAddrs, + Validate: validate, }).Build(addrs.RootModuleInstance) + diags = diags.Append(moreDiags) + if moreDiags.HasErrors() { + return nil, walkApply, diags + } operation := walkApply if plan.UIMode == plans.DestroyMode { 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 efc24767c205..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 @@ -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_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 af17cbd62dc8..d809d6bb9dca 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 := opts.SetVariables + // 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..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 { @@ -125,11 +155,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)) } @@ -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,14 +206,33 @@ var DefaultPlanOpts = &PlanOpts{ Mode: plans.NormalMode, } -func (c *Context) plan(config *configs.Config, prevRunState *states.State, rootVariables InputValues, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) { +// 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 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 +247,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 +288,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 +313,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 +344,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 +445,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 +472,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 +521,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_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/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_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": ` 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..fd57a136f290 100644 --- a/internal/terraform/eval_variable.go +++ b/internal/terraform/eval_variable.go @@ -12,6 +12,156 @@ import ( "github.com/zclconf/go-cty/cty/convert" ) +func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, raw *InputValue, 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 + } + } + + 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() { + // NOTE: The CLI layer typically checks for itself whether all of + // 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`, + Detail: fmt.Sprintf(`The variable %q is required, but is not set.`, addr.Variable.Name), + Subject: cfg.DeclRange.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 { + log.Printf("[ERROR] prepareFinalInputVariableValue: %s has unsuitable type\n got: %s\n want: %s", addr, given.Type(), convertTy) + 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 + } + + // 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 { + log.Printf("[ERROR] prepareFinalInputVariableValue: %s is non-nullable but set to null, and is required", addr) + 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) + } + } + + return val, diags +} + // evalVariableValidations ensures that all of the configured custom validations // for a variable are passing. // @@ -20,9 +170,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 +185,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..0ebea982f57b --- /dev/null +++ b/internal/terraform/eval_variable_test.go @@ -0,0 +1,563 @@ +package terraform + +import ( + "fmt" + "testing" + + "github.com/hashicorp/hcl/v2" + "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 + + // 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 + 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: Unsuitable value for var.required set from outside of the configuration: required variable may not be set to 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: Unsuitable value for var.constrained_string_required set from outside of the configuration: required variable may not be set to 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, + ) + + rawVal := &InputValue{ + Value: test.given, + SourceType: ValueFromCaller, + } + + got, diags := prepareFinalInputVariableValue( + varAddr, rawVal, 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) + } + }) + } + + 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/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..ae3450be528e 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,54 @@ 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) + 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 } - // 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) + // 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, } - vals := make(map[string]cty.Value) - vals[name] = val + finalVal, moreDiags := prepareFinalInputVariableValue(n.Addr, rawVal, 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..33f439d7cd9a 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,59 @@ 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 + 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. 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, + } } - return evalVariableValidations( + finalVal, moreDiags := prepareFinalInputVariableValue( + addr, + givenVal, + 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) } diff --git a/internal/terraform/variables.go b/internal/terraform/variables.go index 7a6ace0eee5c..a60f187003dd 100644 --- a/internal/terraform/variables.go +++ b/internal/terraform/variables.go @@ -3,23 +3,61 @@ 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 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 +// 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. +// +// 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 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 - // 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 } @@ -74,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) } @@ -129,23 +185,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 +266,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 +282,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 +295,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 +}