From d1de8bab2767e4f260da64f46ae5a20a639bba21 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 21 Dec 2021 18:04:24 -0800 Subject: [PATCH] core: More accurate error message for invalid variable values In earlier Terraform versions we had an extra validation step prior to the graph walk which tried to partially validate root module input variable values (just checking their type constraints) and then return error messages which specified as accurately as possible where the value had originally come from. We're now handling that sort of validation exclusively during the graph walk so that we can share the main logic between both root module and child module variable values, but previously that shared code wasn't able to generate such specific information about where the values had originated, because it was adapted from code originally written to only deal with child module variables. Here then we restore a similar level of detail as before, when we're processing root module variables. For child module variables, we use synthetic InputValue objects which state that the value was declared in the configuration, thus causing us to produce a similar sort of error message as we would've before which includes a source range covering the argument expression in the calling module block. --- internal/terraform/eval_variable.go | 84 +++++++++--- internal/terraform/eval_variable_test.go | 143 ++++++++++++++++++++- internal/terraform/node_module_variable.go | 11 +- internal/terraform/node_root_variable.go | 17 ++- internal/terraform/variables.go | 30 ++++- 5 files changed, 254 insertions(+), 31 deletions(-) diff --git a/internal/terraform/eval_variable.go b/internal/terraform/eval_variable.go index bbafbe11c250..fd57a136f290 100644 --- a/internal/terraform/eval_variable.go +++ b/internal/terraform/eval_variable.go @@ -12,7 +12,7 @@ import ( "github.com/zclconf/go-cty/cty/convert" ) -func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given cty.Value, valRange tfdiags.SourceRange, cfg *configs.Variable) (cty.Value, tfdiags.Diagnostics) { +func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, raw *InputValue, cfg *configs.Variable) (cty.Value, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics convertTy := cfg.ConstraintType @@ -41,6 +41,29 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c } } + var sourceRange tfdiags.SourceRange + var nonFileSource string + if raw.HasSourceRange() { + sourceRange = raw.SourceRange + } else { + // If the value came from a place that isn't a file and thus doesn't + // have its own source range, we'll use the declaration range as + // our source range and generate some slightly different error + // messages. + sourceRange = tfdiags.SourceRangeFromHCL(cfg.DeclRange) + switch raw.SourceType { + case ValueFromCLIArg: + nonFileSource = fmt.Sprintf("set using -var=\"%s=...\"", addr.Variable.Name) + case ValueFromEnvVar: + nonFileSource = fmt.Sprintf("set using the TF_VAR_%s environment variable", addr.Variable.Name) + case ValueFromInput: + nonFileSource = "set using an interactive prompt" + default: + nonFileSource = "set from outside of the configuration" + } + } + + given := raw.Value if given == cty.NilVal { // The variable wasn't set at all (even to null) log.Printf("[TRACE] prepareFinalInputVariableValue: %s has no defined value", addr) if cfg.Required() { @@ -54,7 +77,7 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c Severity: hcl.DiagError, Summary: `Required variable not set`, Detail: fmt.Sprintf(`The variable %q is required, but is not set.`, addr.Variable.Name), - Subject: valRange.ToHCL().Ptr(), + Subject: cfg.DeclRange.Ptr(), }) // We'll return a placeholder unknown value to avoid producing // redundant downstream errors. @@ -67,15 +90,27 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c val, err := convert.Convert(given, convertTy) if err != nil { log.Printf("[ERROR] prepareFinalInputVariableValue: %s has unsuitable type\n got: %s\n want: %s", addr, given.Type(), convertTy) - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid value for module argument", - Detail: fmt.Sprintf( - "The given value is not suitable for child module variable %q defined at %s: %s.", - cfg.Name, cfg.DeclRange.String(), err, - ), - Subject: valRange.ToHCL().Ptr(), - }) + if nonFileSource != "" { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid value for input variable", + Detail: fmt.Sprintf( + "Unsuitable value for %s %s: %s.", + addr, nonFileSource, err, + ), + Subject: cfg.DeclRange.Ptr(), + }) + } else { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid value for input variable", + Detail: fmt.Sprintf( + "The given value is not suitable for %s declared at %s: %s.", + addr, cfg.DeclRange.String(), err, + ), + Subject: sourceRange.ToHCL().Ptr(), + }) + } // We'll return a placeholder unknown value to avoid producing // redundant downstream errors. return cty.UnknownVal(cfg.Type), diags @@ -97,12 +132,27 @@ func prepareFinalInputVariableValue(addr addrs.AbsInputVariableInstance, given c val = defaultVal } else { log.Printf("[ERROR] prepareFinalInputVariableValue: %s is non-nullable but set to null, and is required", addr) - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: `Required variable not set`, - Detail: fmt.Sprintf(`The variable %q is required, but the given value is null.`, addr.Variable.Name), - Subject: valRange.ToHCL().Ptr(), - }) + if nonFileSource != "" { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Required variable not set`, + Detail: fmt.Sprintf( + "Unsuitable value for %s %s: required variable may not be set to null.", + addr, nonFileSource, + ), + Subject: cfg.DeclRange.Ptr(), + }) + } else { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: `Required variable not set`, + Detail: fmt.Sprintf( + "The given value is not suitable for %s defined at %s: required variable may not be set to null.", + addr, cfg.DeclRange.String(), + ), + Subject: sourceRange.ToHCL().Ptr(), + }) + } // Stub out our return value so that the semantic checker doesn't // produce redundant downstream errors. val = cty.UnknownVal(cfg.Type) diff --git a/internal/terraform/eval_variable_test.go b/internal/terraform/eval_variable_test.go index fa4048fed41f..0ebea982f57b 100644 --- a/internal/terraform/eval_variable_test.go +++ b/internal/terraform/eval_variable_test.go @@ -4,6 +4,7 @@ import ( "fmt" "testing" + "github.com/hashicorp/hcl/v2" "github.com/zclconf/go-cty/cty" "github.com/hashicorp/terraform/internal/addrs" @@ -65,6 +66,13 @@ func TestPrepareFinalInputVariableValue(t *testing.T) { }) variableConfigs := cfg.Module.Variables + // Because we loaded our pseudo-module from a temporary file, the + // declaration source ranges will have unpredictable filenames. We'll + // fix that here just to make things easier below. + for _, vc := range variableConfigs { + vc.DeclRange.Filename = "main.tf" + } + tests := []struct { varName string given cty.Value @@ -264,7 +272,7 @@ func TestPrepareFinalInputVariableValue(t *testing.T) { "required", cty.NullVal(cty.DynamicPseudoType), cty.UnknownVal(cty.DynamicPseudoType), - `Required variable not set: The variable "required" is required, but the given value is null.`, + `Required variable not set: Unsuitable value for var.required set from outside of the configuration: required variable may not be set to null.`, }, { "required", @@ -316,7 +324,7 @@ func TestPrepareFinalInputVariableValue(t *testing.T) { "constrained_string_required", cty.NullVal(cty.DynamicPseudoType), cty.UnknownVal(cty.String), - `Required variable not set: The variable "constrained_string_required" is required, but the given value is null.`, + `Required variable not set: Unsuitable value for var.constrained_string_required set from outside of the configuration: required variable may not be set to null.`, }, { "constrained_string_required", @@ -401,8 +409,13 @@ func TestPrepareFinalInputVariableValue(t *testing.T) { test.given, ) + rawVal := &InputValue{ + Value: test.given, + SourceType: ValueFromCaller, + } + got, diags := prepareFinalInputVariableValue( - varAddr, test.given, tfdiags.SourceRangeFromHCL(varCfg.DeclRange), varCfg, + varAddr, rawVal, varCfg, ) if test.wantErr != "" { @@ -423,4 +436,128 @@ func TestPrepareFinalInputVariableValue(t *testing.T) { } }) } + + t.Run("SourceType error message variants", func(t *testing.T) { + tests := []struct { + SourceType ValueSourceType + SourceRange tfdiags.SourceRange + WantTypeErr string + WantNullErr string + }{ + { + ValueFromUnknown, + tfdiags.SourceRange{}, + `Invalid value for input variable: Unsuitable value for var.constrained_string_required set from outside of the configuration: string required.`, + `Required variable not set: Unsuitable value for var.constrained_string_required set from outside of the configuration: required variable may not be set to null.`, + }, + { + ValueFromConfig, + tfdiags.SourceRange{ + Filename: "example.tf", + Start: tfdiags.SourcePos(hcl.InitialPos), + End: tfdiags.SourcePos(hcl.InitialPos), + }, + `Invalid value for input variable: The given value is not suitable for var.constrained_string_required declared at main.tf:32,3-41: string required.`, + `Required variable not set: The given value is not suitable for var.constrained_string_required defined at main.tf:32,3-41: required variable may not be set to null.`, + }, + { + ValueFromAutoFile, + tfdiags.SourceRange{ + Filename: "example.auto.tfvars", + Start: tfdiags.SourcePos(hcl.InitialPos), + End: tfdiags.SourcePos(hcl.InitialPos), + }, + `Invalid value for input variable: The given value is not suitable for var.constrained_string_required declared at main.tf:32,3-41: string required.`, + `Required variable not set: The given value is not suitable for var.constrained_string_required defined at main.tf:32,3-41: required variable may not be set to null.`, + }, + { + ValueFromNamedFile, + tfdiags.SourceRange{ + Filename: "example.tfvars", + Start: tfdiags.SourcePos(hcl.InitialPos), + End: tfdiags.SourcePos(hcl.InitialPos), + }, + `Invalid value for input variable: The given value is not suitable for var.constrained_string_required declared at main.tf:32,3-41: string required.`, + `Required variable not set: The given value is not suitable for var.constrained_string_required defined at main.tf:32,3-41: required variable may not be set to null.`, + }, + { + ValueFromCLIArg, + tfdiags.SourceRange{}, + `Invalid value for input variable: Unsuitable value for var.constrained_string_required set using -var="constrained_string_required=...": string required.`, + `Required variable not set: Unsuitable value for var.constrained_string_required set using -var="constrained_string_required=...": required variable may not be set to null.`, + }, + { + ValueFromEnvVar, + tfdiags.SourceRange{}, + `Invalid value for input variable: Unsuitable value for var.constrained_string_required set using the TF_VAR_constrained_string_required environment variable: string required.`, + `Required variable not set: Unsuitable value for var.constrained_string_required set using the TF_VAR_constrained_string_required environment variable: required variable may not be set to null.`, + }, + { + ValueFromInput, + tfdiags.SourceRange{}, + `Invalid value for input variable: Unsuitable value for var.constrained_string_required set using an interactive prompt: string required.`, + `Required variable not set: Unsuitable value for var.constrained_string_required set using an interactive prompt: required variable may not be set to null.`, + }, + { + // NOTE: This isn't actually a realistic case for this particular + // function, because if we have a value coming from a plan then + // we must be in the apply step, and we shouldn't be able to + // get past the plan step if we have invalid variable values, + // and during planning we'll always have other source types. + ValueFromPlan, + tfdiags.SourceRange{}, + `Invalid value for input variable: Unsuitable value for var.constrained_string_required set from outside of the configuration: string required.`, + `Required variable not set: Unsuitable value for var.constrained_string_required set from outside of the configuration: required variable may not be set to null.`, + }, + { + ValueFromCaller, + tfdiags.SourceRange{}, + `Invalid value for input variable: Unsuitable value for var.constrained_string_required set from outside of the configuration: string required.`, + `Required variable not set: Unsuitable value for var.constrained_string_required set from outside of the configuration: required variable may not be set to null.`, + }, + } + + for _, test := range tests { + t.Run(fmt.Sprintf("%s %s", test.SourceType, test.SourceRange.StartString()), func(t *testing.T) { + varAddr := addrs.InputVariable{Name: "constrained_string_required"}.Absolute(addrs.RootModuleInstance) + varCfg := variableConfigs[varAddr.Variable.Name] + t.Run("type error", func(t *testing.T) { + rawVal := &InputValue{ + Value: cty.EmptyObjectVal, + SourceType: test.SourceType, + SourceRange: test.SourceRange, + } + + _, diags := prepareFinalInputVariableValue( + varAddr, rawVal, varCfg, + ) + if !diags.HasErrors() { + t.Fatalf("unexpected success; want error") + } + + if got, want := diags.Err().Error(), test.WantTypeErr; got != want { + t.Errorf("wrong error\ngot: %s\nwant: %s", got, want) + } + }) + t.Run("null error", func(t *testing.T) { + rawVal := &InputValue{ + Value: cty.NullVal(cty.DynamicPseudoType), + SourceType: test.SourceType, + SourceRange: test.SourceRange, + } + + _, diags := prepareFinalInputVariableValue( + varAddr, rawVal, varCfg, + ) + if !diags.HasErrors() { + t.Fatalf("unexpected success; want error") + } + + if got, want := diags.Err().Error(), test.WantNullErr; got != want { + t.Errorf("wrong error\ngot: %s\nwant: %s", got, want) + } + }) + }) + } + }) } diff --git a/internal/terraform/node_module_variable.go b/internal/terraform/node_module_variable.go index 321cd874845f..ae3450be528e 100644 --- a/internal/terraform/node_module_variable.go +++ b/internal/terraform/node_module_variable.go @@ -227,7 +227,16 @@ func (n *nodeModuleVariable) evalModuleCallArgument(ctx EvalContext, validateOnl errSourceRange = tfdiags.SourceRangeFromHCL(n.Config.DeclRange) // we use the declaration range as a fallback for an undefined variable } - finalVal, moreDiags := prepareFinalInputVariableValue(n.Addr, givenVal, errSourceRange, n.Config) + // We construct a synthetic InputValue here to pretend as if this were + // a root module variable set from outside, just as a convenience so we + // can reuse the InputValue type for this. + rawVal := &InputValue{ + Value: givenVal, + SourceType: ValueFromConfig, + SourceRange: errSourceRange, + } + + finalVal, moreDiags := prepareFinalInputVariableValue(n.Addr, rawVal, n.Config) diags = diags.Append(moreDiags) return finalVal, diags.ErrWithWarnings() diff --git a/internal/terraform/node_root_variable.go b/internal/terraform/node_root_variable.go index d023be3500ba..33f439d7cd9a 100644 --- a/internal/terraform/node_root_variable.go +++ b/internal/terraform/node_root_variable.go @@ -65,20 +65,23 @@ func (n *NodeRootVariable) Execute(ctx EvalContext, op walkOperation) tfdiags.Di return nil } - var givenVal cty.Value - if n.RawValue != nil { - givenVal = n.RawValue.Value - } else { + givenVal := n.RawValue + if givenVal == nil { // We'll use cty.NilVal to represent the variable not being set at // all, which for historical reasons is unfortunately different than - // explicitly setting it to null in some cases. - givenVal = cty.NilVal + // explicitly setting it to null in some cases. In normal code we + // should never get here because all variables should have raw + // values, but we can get here in some historical tests that call + // in directly and don't necessarily obey the rules. + givenVal = &InputValue{ + Value: cty.NilVal, + SourceType: ValueFromUnknown, + } } finalVal, moreDiags := prepareFinalInputVariableValue( addr, givenVal, - tfdiags.SourceRangeFromHCL(n.Config.DeclRange), n.Config, ) diags = diags.Append(moreDiags) diff --git a/internal/terraform/variables.go b/internal/terraform/variables.go index f5f03d156bf6..a60f187003dd 100644 --- a/internal/terraform/variables.go +++ b/internal/terraform/variables.go @@ -9,7 +9,7 @@ import ( "github.com/hashicorp/terraform/internal/tfdiags" ) -// InputValue represents a raw value vor a root module input variable as +// InputValue represents a raw value for a root module input variable as // provided by the external caller into a function like terraform.Context.Plan. // // InputValue should represent as directly as possible what the user set the @@ -22,6 +22,11 @@ import ( // variables declared in the root module, even if the end user didn't provide // an explicit value for some of them. See the Value field documentation for // how to handle that situation. +// +// Terraform Core also internally uses InputValue to represent the raw value +// provided for a variable in a child module call, following the same +// conventions. However, that's an implementation detail not visible to +// outside callers. type InputValue struct { // Value is the raw value as provided by the user as part of the plan // options, or a corresponding similar data structure for non-plan @@ -50,8 +55,9 @@ type InputValue struct { SourceType ValueSourceType // SourceRange provides source location information for values whose - // SourceType is either ValueFromConfig or ValueFromFile. It is not - // populated for other source types, and so should not be used. + // SourceType is either ValueFromConfig, ValueFromNamedFile, or + // ValueForNormalFile. It is not populated for other source types, and so + // should not be used. SourceRange tfdiags.SourceRange } @@ -106,6 +112,24 @@ func (v *InputValue) GoString() string { } } +// HasSourceRange returns true if the reciever has a source type for which +// we expect the SourceRange field to be populated with a valid range. +func (v *InputValue) HasSourceRange() bool { + return v.SourceType.HasSourceRange() +} + +// HasSourceRange returns true if the reciever is one of the source types +// that is used along with a valid SourceRange field when appearing inside an +// InputValue object. +func (v ValueSourceType) HasSourceRange() bool { + switch v { + case ValueFromConfig, ValueFromAutoFile, ValueFromNamedFile: + return true + default: + return false + } +} + func (v ValueSourceType) GoString() string { return fmt.Sprintf("terraform.%s", v) }