From 36c4d4c241b3402d7fbf677e4e7850f6ab80e66f Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Mon, 20 Dec 2021 16:38:52 -0800 Subject: [PATCH] core and backend: remove redundant handling of default variable values Previously we had three different layers all thinking they were responsible for substituting a default value for an unset root module variable: - the local backend, via logic in backend.ParseVariableValues - the context.Plan function (and other similar functions) trying to preprocess the input variables using terraform.mergeDefaultInputVariableValues . - the newer prepareFinalInputVariableValue, which aims to centralize all of the variable preparation logic so it can be common to both root and child module variables. The second of these was also trying to handle type constraint checking, which is also the responsibility of the central function and not something we need to handle so early. Only the last of these consistently handles both root and child module variables, and so is the one we ought to keep. The others are now redundant and are causing prepareFinalInputVariableValue to get a slightly corrupted view of the caller's chosen variable values. To rectify that, here we remove the two redundant layers altogether and have unset root variables pass through as cty.NilVal all the way to the central prepareFinalInputVariableValue function, which will then handle them in a suitable way which properly respects the "nullable" setting. This commit includes some test changes in the terraform package to make those tests no longer rely on the mergeDefaultInputVariableValues logic we've removed, and to instead explicitly set cty.NilVal for all unset variables to comply with our intended contract for PlanOpts.SetVariables, and similar. (This is so that we can more easily catch bugs in callers where they _don't_ correctly handle input variables; it allows us to distinguish between the caller explicitly marking a variable as unset vs. not describing it at all, where the latter is a bug in the caller.) --- internal/backend/unparsed_value.go | 17 +- internal/backend/unparsed_value_test.go | 2 +- internal/command/jsonplan/plan.go | 43 +++++- internal/terraform/context_apply.go | 15 ++ internal/terraform/context_apply2_test.go | 6 +- internal/terraform/context_apply_test.go | 59 ++++--- internal/terraform/context_eval.go | 2 +- internal/terraform/context_eval_test.go | 4 +- internal/terraform/context_import.go | 2 +- internal/terraform/context_plan.go | 69 ++++++++- internal/terraform/context_plan2_test.go | 2 +- internal/terraform/context_plan_test.go | 15 +- internal/terraform/eval_variable.go | 10 +- internal/terraform/variables.go | 135 ++++++---------- internal/terraform/variables_test.go | 180 +++------------------- 15 files changed, 255 insertions(+), 306 deletions(-) diff --git a/internal/backend/unparsed_value.go b/internal/backend/unparsed_value.go index 91c982582..e7eadea9a 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 981c84a43..8807d243d 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 64d77c05a..06ed97961 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 42520b03d..d3cd9dc2d 100644 --- a/internal/terraform/context_apply.go +++ b/internal/terraform/context_apply.go @@ -87,6 +87,21 @@ func (c *Context) applyGraph(plan *plans.Plan, config *configs.Config, validate return nil, walkApply, diags } + // The plan.VariableValues field only records variables that were actually + // set by the caller in the PlanOpts, so we may need to provide + // placeholders for any other variables that the user didn't set, in + // which case Terraform will once again use the default value from the + // configuration when we visit these variables during the graph walk. + for name := range config.Module.Variables { + if _, ok := variables[name]; ok { + continue + } + variables[name] = &InputValue{ + Value: cty.NilVal, + SourceType: ValueFromPlan, + } + } + graph, moreDiags := (&ApplyGraphBuilder{ Config: config, Changes: plan.Changes, diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index 6b87d409d..512f3dab3 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 28c5c788b..46dcbd58b 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 c6af77635..f9d0f6493 100644 --- a/internal/terraform/context_eval.go +++ b/internal/terraform/context_eval.go @@ -45,7 +45,7 @@ func (c *Context) Eval(config *configs.Config, state *states.State, moduleAddr a state = state.DeepCopy() var walker *ContextGraphWalker - variables := mergeDefaultInputVariableValues(opts.SetVariables, config.Module.Variables) + variables := opts.SetVariables // By the time we get here, we should have values defined for all of // the root module variables, even if some of them are "unknown". It's the diff --git a/internal/terraform/context_eval_test.go b/internal/terraform/context_eval_test.go index dff687983..0bd752935 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 b5417405f..d809d6bb9 100644 --- a/internal/terraform/context_import.go +++ b/internal/terraform/context_import.go @@ -53,7 +53,7 @@ func (c *Context) Import(config *configs.Config, prevRunState *states.State, opt log.Printf("[DEBUG] Building and walking import graph") - variables := mergeDefaultInputVariableValues(opts.SetVariables, config.Module.Variables) + variables := opts.SetVariables // Initialize our graph builder builder := &ImportGraphBuilder{ diff --git a/internal/terraform/context_plan.go b/internal/terraform/context_plan.go index 0b3c97f14..f114b2a5f 100644 --- a/internal/terraform/context_plan.go +++ b/internal/terraform/context_plan.go @@ -21,10 +21,42 @@ import ( // PlanOpts are the various options that affect the details of how Terraform // will build a plan. type PlanOpts struct { - Mode plans.Mode - SkipRefresh bool + // Mode defines what variety of plan the caller wishes to create. + // Refer to the documentation of the plans.Mode type and its values + // for more information. + Mode plans.Mode + + // SkipRefresh specifies to trust that the current values for managed + // resource instances in the prior state are accurate and to therefore + // disable the usual step of fetching updated values for each resource + // instance using its corresponding provider. + SkipRefresh bool + + // SetVariables are the raw values for root module variables as provided + // by the user who is requesting the run, prior to any normalization or + // substitution of defaults. See the documentation for the InputValue + // type for more information on how to correctly populate this. SetVariables InputValues - Targets []addrs.Targetable + + // If Targets has a non-zero length then it activates targeted planning + // mode, where Terraform will take actions only for resource instances + // mentioned in this set and any other objects those resource instances + // depend on. + // + // Targeted planning mode is intended for exceptional use only, + // and so populating this field will cause Terraform to generate extra + // warnings as part of the planning result. + Targets []addrs.Targetable + + // ForceReplace is a set of resource instance addresses whose corresponding + // objects should be forced planned for replacement if the provider's + // plan would otherwise have been to either update the object in-place or + // to take no action on it at all. + // + // A typical use of this argument is to ask Terraform to replace an object + // which the user has determined is somehow degraded (via information from + // outside of Terraform), thereby hopefully replacing it with a + // fully-functional new object. ForceReplace []addrs.AbsResourceInstance } @@ -99,8 +131,6 @@ func (c *Context) Plan(config *configs.Config, prevRunState *states.State, opts return nil, diags } - variables := mergeDefaultInputVariableValues(opts.SetVariables, config.Module.Variables) - // By the time we get here, we should have values defined for all of // the root module variables, even if some of them are "unknown". It's the // caller's responsibility to have already handled the decoding of these @@ -108,7 +138,7 @@ func (c *Context) Plan(config *configs.Config, prevRunState *states.State, opts // user-friendly error messages if they are not all present, and so // the error message from checkInputVariables should never be seen and // includes language asking the user to report a bug. - varDiags := checkInputVariables(config.Module.Variables, variables) + varDiags := checkInputVariables(config.Module.Variables, opts.SetVariables) diags = diags.Append(varDiags) if len(opts.Targets) > 0 { @@ -139,8 +169,12 @@ The -target option is not for routine use, and is provided only for exceptional } // convert the variables into the format expected for the plan - varVals := make(map[string]plans.DynamicValue, len(variables)) - for k, iv := range variables { + varVals := make(map[string]plans.DynamicValue, len(opts.SetVariables)) + for k, iv := range opts.SetVariables { + if iv.Value == cty.NilVal { + continue // We only record values that the caller actually set + } + // We use cty.DynamicPseudoType here so that we'll save both the // value _and_ its dynamic type in the plan, so we can recover // exactly the same value later. @@ -172,6 +206,25 @@ var DefaultPlanOpts = &PlanOpts{ Mode: plans.NormalMode, } +// SimplePlanOpts is a constructor to help with creating "simple" values of +// PlanOpts which only specify a mode and input variables. +// +// This helper function is primarily intended for use in straightforward +// tests that don't need any of the more "esoteric" planning options. For +// handling real user requests to run Terraform, it'd probably be better +// to construct a *PlanOpts value directly and provide a way for the user +// to set values for all of its fields. +// +// The "mode" and "setVariables" arguments become the values of the "Mode" +// and "SetVariables" fields in the result. Refer to the PlanOpts type +// documentation to learn about the meanings of those fields. +func SimplePlanOpts(mode plans.Mode, setVariables InputValues) *PlanOpts { + return &PlanOpts{ + Mode: mode, + SetVariables: setVariables, + } +} + func (c *Context) plan(config *configs.Config, prevRunState *states.State, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 006e9e932..d1771b1f3 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 cfd51da8c..9cf7e875e 100644 --- a/internal/terraform/context_plan_test.go +++ b/internal/terraform/context_plan_test.go @@ -405,7 +405,7 @@ func TestContext2Plan_moduleExpand(t *testing.T) { }, }) - plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts) + plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))) if diags.HasErrors() { t.Fatalf("unexpected errors: %s", diags.Err()) } @@ -1175,7 +1175,7 @@ func TestContext2Plan_moduleProviderVar(t *testing.T) { }, }) - plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts) + plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))) if diags.HasErrors() { t.Fatalf("unexpected errors: %s", diags.Err()) } @@ -2242,7 +2242,7 @@ func TestContext2Plan_countModuleStatic(t *testing.T) { }, }) - plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts) + plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))) if diags.HasErrors() { t.Fatalf("unexpected errors: %s", diags.Err()) } @@ -2295,7 +2295,7 @@ func TestContext2Plan_countModuleStaticGrandchild(t *testing.T) { }, }) - plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts) + plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))) if diags.HasErrors() { t.Fatalf("unexpected errors: %s", diags.Err()) } @@ -3938,7 +3938,7 @@ func TestContext2Plan_taintDestroyInterpolatedCountRace(t *testing.T) { }, }) - plan, diags := ctx.Plan(m, state.DeepCopy(), DefaultPlanOpts) + plan, diags := ctx.Plan(m, state.DeepCopy(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))) if diags.HasErrors() { t.Fatalf("unexpected errors: %s", diags.Err()) } @@ -5481,7 +5481,7 @@ func TestContext2Plan_variableSensitivity(t *testing.T) { }, }) - plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts) + plan, diags := ctx.Plan(m, states.NewState(), SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))) if diags.HasErrors() { t.Fatalf("unexpected errors: %s", diags.Err()) } @@ -5544,6 +5544,7 @@ func TestContext2Plan_variableSensitivityModule(t *testing.T) { plan, diags := ctx.Plan(m, states.NewState(), &PlanOpts{ Mode: plans.NormalMode, SetVariables: InputValues{ + "sensitive_var": {Value: cty.NilVal}, "another_var": &InputValue{ Value: cty.StringVal("boop"), SourceType: ValueFromCaller, @@ -6657,7 +6658,7 @@ resource "test_resource" "foo" { }, ) }) - plan, diags := ctx.Plan(m, state, DefaultPlanOpts) + plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))) if diags.HasErrors() { t.Fatal(diags.Err()) } diff --git a/internal/terraform/eval_variable.go b/internal/terraform/eval_variable.go index 1c16069ad..bbafbe11c 100644 --- a/internal/terraform/eval_variable.go +++ b/internal/terraform/eval_variable.go @@ -45,9 +45,11 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c log.Printf("[TRACE] prepareFinalInputVariableValue: %s has no defined value", addr) if cfg.Required() { // NOTE: The CLI layer typically checks for itself whether all of - // the required _root_ module variables are not set, which would - // mask this error. We can get here for child module variables, - // though. + // the required _root_ module variables are set, which would + // mask this error with a more specific one that refers to the + // CLI features for setting such variables. We can get here for + // child module variables, though. + log.Printf("[ERROR] prepareFinalInputVariableValue: %s is required but is not set", addr) diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: `Required variable not set`, @@ -64,6 +66,7 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c val, err := convert.Convert(given, convertTy) if err != nil { + log.Printf("[ERROR] prepareFinalInputVariableValue: %s has unsuitable type\n got: %s\n want: %s", addr, given.Type(), convertTy) diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Invalid value for module argument", @@ -93,6 +96,7 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c if defaultVal != cty.NilVal { val = defaultVal } else { + log.Printf("[ERROR] prepareFinalInputVariableValue: %s is non-nullable but set to null, and is required", addr) diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: `Required variable not set`, diff --git a/internal/terraform/variables.go b/internal/terraform/variables.go index 7a6ace0ee..f5f03d156 100644 --- a/internal/terraform/variables.go +++ b/internal/terraform/variables.go @@ -3,18 +3,50 @@ package terraform import ( "fmt" - "github.com/hashicorp/hcl/v2" "github.com/zclconf/go-cty/cty" - "github.com/zclconf/go-cty/cty/convert" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/tfdiags" ) -// InputValue represents a value for a variable in the root module, provided -// as part of the definition of an operation. +// InputValue represents a raw value vor a root module input variable as +// provided by the external caller into a function like terraform.Context.Plan. +// +// InputValue should represent as directly as possible what the user set the +// variable to, without any attempt to convert the value to the variable's +// type constraint or substitute the configured default values for variables +// that wasn't set. Those adjustments will be handled by Terraform Core itself +// as part of performing the requested operation. +// +// A Terraform Core caller must provide an InputValue object for each of the +// variables declared in the root module, even if the end user didn't provide +// an explicit value for some of them. See the Value field documentation for +// how to handle that situation. type InputValue struct { - Value cty.Value + // Value is the raw value as provided by the user as part of the plan + // options, or a corresponding similar data structure for non-plan + // operations. + // + // If a particular variable declared in the root module is _not_ set by + // the user then the caller must still provide an InputValue for it but + // must set Value to cty.NilVal to represent the absense of a value. + // This requirement is to help detect situations where the caller isn't + // correctly detecting and handling all of the declared variables. + // + // For historical reasons it's important that callers distinguish the + // situation of the value not being set at all (cty.NilVal) from the + // situation of it being explicitly set to null (a cty.NullVal result): + // for "nullable" input variables that distinction unfortunately decides + // whether the final value will be the variable's default or will be + // explicitly null. + Value cty.Value + + // SourceType is a high-level category for where the value of Value + // came from, which Terraform Core uses to tailor some of its error + // messages to be more helpful to the user. + // + // Some SourceType values should be accompanied by a populated SourceRange + // value. See that field's documentation below for more information. SourceType ValueSourceType // SourceRange provides source location information for values whose @@ -129,23 +161,6 @@ func (vv InputValues) JustValues() map[string]cty.Value { return ret } -// DefaultVariableValues returns an InputValues map representing the default -// values specified for variables in the given configuration map. -func DefaultVariableValues(configs map[string]*configs.Variable) InputValues { - ret := make(InputValues) - for k, c := range configs { - if c.Default == cty.NilVal { - continue - } - ret[k] = &InputValue{ - Value: c.Default, - SourceType: ValueFromConfig, - SourceRange: tfdiags.SourceRangeFromHCL(c.DeclRange), - } - } - return ret -} - // SameValues returns true if the given InputValues has the same values as // the receiever, disregarding the source types and source ranges. // @@ -227,21 +242,15 @@ func (vv InputValues) Identical(other InputValues) bool { return true } -func mergeDefaultInputVariableValues(setVals InputValues, rootVarsConfig map[string]*configs.Variable) InputValues { - var variables InputValues - - // Default variables from the configuration seed our map. - variables = DefaultVariableValues(rootVarsConfig) - - // Variables provided by the caller (from CLI, environment, etc) can - // override the defaults. - variables = variables.Override(setVals) - - return variables -} - -// checkInputVariables ensures that variable values supplied at the UI conform -// to their corresponding declarations in configuration. +// checkInputVariables ensures that the caller provided an InputValue +// definition for each root module variable declared in the configuration. +// The caller must provide an InputVariables with keys exactly matching +// the declared variables, though some of them may be marked explicitly +// unset by their values being cty.NilVal. +// +// This doesn't perform any type checking, default value substitution, or +// validation checks. Those are all handled during a graph walk when we +// visit the graph nodes representing each root variable. // // The set of values is considered valid only if the returned diagnostics // does not contain errors. A valid set of values may still produce warnings, @@ -249,11 +258,12 @@ func mergeDefaultInputVariableValues(setVals InputValues, rootVarsConfig map[str func checkInputVariables(vcs map[string]*configs.Variable, vs InputValues) tfdiags.Diagnostics { var diags tfdiags.Diagnostics - for name, vc := range vcs { - val, isSet := vs[name] + for name := range vcs { + _, isSet := vs[name] if !isSet { - // Always an error, since the caller should already have included - // default values from the configuration in the values map. + // Always an error, since the caller should have produced an + // item with Value: cty.NilVal to be explicit that it offered + // an opportunity to set this variable. diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, "Unassigned variable", @@ -261,49 +271,6 @@ func checkInputVariables(vcs map[string]*configs.Variable, vs InputValues) tfdia )) continue } - - // A given value is valid if it can convert to the desired type. - _, err := convert.Convert(val.Value, vc.ConstraintType) - if err != nil { - switch val.SourceType { - case ValueFromConfig, ValueFromAutoFile, ValueFromNamedFile: - // We have source location information for these. - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid value for input variable", - Detail: fmt.Sprintf("The given value is not valid for variable %q: %s.", name, err), - Subject: val.SourceRange.ToHCL().Ptr(), - }) - case ValueFromEnvVar: - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Invalid value for input variable", - fmt.Sprintf("The environment variable TF_VAR_%s does not contain a valid value for variable %q: %s.", name, name, err), - )) - case ValueFromCLIArg: - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Invalid value for input variable", - fmt.Sprintf("The argument -var=\"%s=...\" does not contain a valid value for variable %q: %s.", name, name, err), - )) - case ValueFromInput: - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Invalid value for input variable", - fmt.Sprintf("The value entered for variable %q is not valid: %s.", name, err), - )) - default: - // The above gets us good coverage for the situations users - // are likely to encounter with their own inputs. The other - // cases are generally implementation bugs, so we'll just - // use a generic error for these. - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Invalid value for input variable", - fmt.Sprintf("The value provided for variable %q is not valid: %s.", name, err), - )) - } - } } // Check for any variables that are assigned without being configured. diff --git a/internal/terraform/variables_test.go b/internal/terraform/variables_test.go index 41decbae2..6e53a9575 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 +}