Skip to content

Commit

Permalink
core and backend: remove redundant handling of default variable values
Browse files Browse the repository at this point in the history 
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.

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.)
  • Loading branch information
@apparentlymart
apparentlymart committed Dec 21, 2021
1 parent 72bff83 commit 93f6286
Show file tree
Hide file tree
Showing 7 changed files with 20 additions and 198 deletions.
17 changes: 14 additions & 3 deletions internal/backend/unparsed_value.go
View file
Expand Up @@ -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,
Expand All @@ -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),
}
Expand Down
2 changes: 1 addition & 1 deletion internal/backend/unparsed_value_test.go
View file
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion internal/terraform/context_eval.go
View file
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion internal/terraform/context_import.go
View file
Expand Up @@ -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{
Expand Down
8 changes: 3 additions & 5 deletions internal/terraform/context_plan.go
View file
Expand Up @@ -99,16 +99,14 @@ 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
// from the various ways the CLI allows them to be set and to produce
// 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 {
Expand Down Expand Up @@ -139,8 +137,8 @@ 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 {
// 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.
Expand Down
30 changes: 0 additions & 30 deletions internal/terraform/variables.go
View file
Expand Up @@ -129,23 +129,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.
//
Expand Down Expand Up @@ -227,19 +210,6 @@ 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.
//
Expand Down
157 changes: 0 additions & 157 deletions internal/terraform/variables_test.go
View file
Expand Up @@ -3,166 +3,9 @@ package terraform
import (
"testing"

"github.com/davecgh/go-spew/spew"
"github.com/hashicorp/terraform/internal/tfdiags"

"github.com/go-test/deep"
"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")

Expand Down

0 comments on commit 93f6286

Please sign in to comment.