diff --git a/internal/terraform/eval_variable.go b/internal/terraform/eval_variable.go index 4ac53ed33..629da7051 100644 --- a/internal/terraform/eval_variable.go +++ b/internal/terraform/eval_variable.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/gohcl" "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/tfdiags" @@ -211,12 +212,62 @@ func evalVariableValidations(addr addrs.AbsInputVariableInstance, config *config result, moreDiags := validation.Condition.Value(hclCtx) ruleDiags = ruleDiags.Append(moreDiags) errorValue, errorDiags := validation.ErrorMessage.Value(hclCtx) - ruleDiags = ruleDiags.Append(errorDiags) + + // The following error handling is a workaround to preserve backwards + // compatibility. Due to an implementation quirk, all prior versions of + // Terraform would treat error messages specified using JSON + // configuration syntax (.tf.json) as string literals, even if they + // contained the "${" template expression operator. This behaviour did + // not match that of HCL configuration syntax, where a template + // expression would result in a validation error. + // + // As a result, users writing or generating JSON configuration syntax + // may have specified error messages which are invalid template + // expressions. As we add support for error message expressions, we are + // unable to perfectly distinguish between these two cases. + // + // To ensure that we don't break backwards compatibility, we have the + // below fallback logic if the error message fails to evaluate. This + // should only have any effect for JSON configurations. The gohcl + // DecodeExpression function behaves differently when the source of the + // expression is a JSON configuration file and a nil context is passed. + if errorDiags.HasErrors() { + // Attempt to decode the expression as a string literal. Passing + // nil as the context forces a JSON syntax string value to be + // interpreted as a string literal. + var errorString string + moreErrorDiags := gohcl.DecodeExpression(validation.ErrorMessage, nil, &errorString) + if !moreErrorDiags.HasErrors() { + // Decoding succeeded, meaning that this is a JSON syntax + // string value. We rewrap that as a cty value to allow later + // decoding to succeed. + errorValue = cty.StringVal(errorString) + + // This warning diagnostic explains this odd behaviour, while + // giving us an escape hatch to change this to a hard failure + // in some future Terraform 1.x version. + errorDiags = hcl.Diagnostics{ + &hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Validation error message expression is invalid", + Detail: fmt.Sprintf("The error message provided could not be evaluated as an expression, so Terraform is interpreting it as a string literal.\n\nIn future versions of Terraform, this will be considered an error. Please file a GitHub issue if this would break your workflow.\n\n%s", errorDiags.Error()), + Subject: validation.ErrorMessage.Range().Ptr(), + Context: validation.DeclRange.Ptr(), + Expression: validation.ErrorMessage, + EvalContext: hclCtx, + }, + } + } + + // We want to either report the original diagnostics if the + // fallback failed, or the warning generated above if it succeeded. + ruleDiags = ruleDiags.Append(errorDiags) + } diags = diags.Append(ruleDiags) if ruleDiags.HasErrors() { - log.Printf("[TRACE] evalVariableValidations: %s rule %s condition expression failed: %s", addr, validation.DeclRange, ruleDiags.Err().Error()) + log.Printf("[TRACE] evalVariableValidations: %s rule %s check rule evaluation failed: %s", addr, validation.DeclRange, ruleDiags.Err().Error()) } if !result.IsKnown() { log.Printf("[TRACE] evalVariableValidations: %s rule %s condition value is unknown, so skipping validation for now", addr, validation.DeclRange) diff --git a/internal/terraform/eval_variable_test.go b/internal/terraform/eval_variable_test.go index 0ebea982f..c84a969e8 100644 --- a/internal/terraform/eval_variable_test.go +++ b/internal/terraform/eval_variable_test.go @@ -2,12 +2,14 @@ package terraform import ( "fmt" + "strings" "testing" "github.com/hashicorp/hcl/v2" "github.com/zclconf/go-cty/cty" "github.com/hashicorp/terraform/internal/addrs" + "github.com/hashicorp/terraform/internal/lang" "github.com/hashicorp/terraform/internal/tfdiags" ) @@ -561,3 +563,155 @@ func TestPrepareFinalInputVariableValue(t *testing.T) { } }) } + +// These tests cover the JSON syntax configuration edge case handling, +// the background of which is described in detail in comments in the +// evalVariableValidations function. Future versions of Terraform may +// be able to remove this behaviour altogether. +func TestEvalVariableValidations_jsonErrorMessageEdgeCase(t *testing.T) { + cfgSrc := `{ + "variable": { + "valid": { + "type": "string", + "validation": { + "condition": "${var.valid != \"bar\"}", + "error_message": "Valid template string ${var.valid}" + } + }, + "invalid": { + "type": "string", + "validation": { + "condition": "${var.invalid != \"bar\"}", + "error_message": "Invalid template string ${" + } + } + } +} +` + cfg := testModuleInline(t, map[string]string{ + "main.tf.json": 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.json" + for _, v := range vc.Validations { + v.DeclRange.Filename = "main.tf.json" + } + } + + tests := []struct { + varName string + given cty.Value + wantErr []string + wantWarn []string + }{ + // Valid variable validation declaration, assigned value which passes + // the condition generates no diagnostics. + { + varName: "valid", + given: cty.StringVal("foo"), + }, + // Assigning a value which fails the condition generates an error + // message with the expression successfully evaluated. + { + varName: "valid", + given: cty.StringVal("bar"), + wantErr: []string{ + "Invalid value for variable", + "Valid template string bar", + }, + }, + // Invalid variable validation declaration due to an unparseable + // template string. Assigning a value which passes the condition + // results in a warning about the error message. + { + varName: "invalid", + given: cty.StringVal("foo"), + wantWarn: []string{ + "Validation error message expression is invalid", + "Missing expression; Expected the start of an expression, but found the end of the file.", + }, + }, + // Assigning a value which fails the condition generates an error + // message including the configured string interpreted as a literal + // value, and the same warning diagnostic as above. + { + varName: "invalid", + given: cty.StringVal("bar"), + wantErr: []string{ + "Invalid value for variable", + "Invalid template string ${", + }, + wantWarn: []string{ + "Validation error message expression is invalid", + "Missing expression; Expected the start of an expression, but found the end of the file.", + }, + }, + } + + 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) + } + + // Build a mock context to allow the function under test to + // retrieve the variable value and evaluate the expressions + ctx := &MockEvalContext{} + + // We need a minimal scope to allow basic functions to be passed to + // the HCL scope + ctx.EvaluationScopeScope = &lang.Scope{} + ctx.GetVariableValueFunc = func(addr addrs.AbsInputVariableInstance) cty.Value { + if got, want := addr.String(), varAddr.String(); got != want { + t.Errorf("incorrect argument to GetVariableValue: got %s, want %s", got, want) + } + return test.given + } + + gotDiags := evalVariableValidations( + varAddr, varCfg, nil, ctx, + ) + + if len(test.wantErr) == 0 && len(test.wantWarn) == 0 { + if len(gotDiags) > 0 { + t.Errorf("no diags expected, got %s", gotDiags.Err().Error()) + } + } else { + wantErrs: + for _, want := range test.wantErr { + for _, diag := range gotDiags { + if diag.Severity() != tfdiags.Error { + continue + } + desc := diag.Description() + if strings.Contains(desc.Summary, want) || strings.Contains(desc.Detail, want) { + continue wantErrs + } + } + t.Errorf("no error diagnostics found containing %q\ngot: %s", want, gotDiags.Err().Error()) + } + + wantWarns: + for _, want := range test.wantWarn { + for _, diag := range gotDiags { + if diag.Severity() != tfdiags.Warning { + continue + } + desc := diag.Description() + if strings.Contains(desc.Summary, want) || strings.Contains(desc.Detail, want) { + continue wantWarns + } + } + t.Errorf("no warning diagnostics found containing %q\ngot: %s", want, gotDiags.Err().Error()) + } + } + }) + } +}