diff --git a/internal/configs/checks.go b/internal/configs/checks.go index 0980ec16c..10ad62b69 100644 --- a/internal/configs/checks.go +++ b/internal/configs/checks.go @@ -2,10 +2,8 @@ package configs import ( "fmt" - "unicode" "github.com/hashicorp/hcl/v2" - "github.com/hashicorp/hcl/v2/gohcl" "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/lang" ) @@ -24,12 +22,15 @@ type CheckRule struct { // input variables can only refer to the variable that is being validated. Condition hcl.Expression - // ErrorMessage is one or more full sentences, which would need to be in + // ErrorMessage should be one or more full sentences, which should be in // English for consistency with the rest of the error message output but - // can in practice be in any language as long as it ends with a period. - // The message should describe what is required for the condition to return - // true in a way that would make sense to a caller of the module. - ErrorMessage string + // can in practice be in any language. The message should describe what is + // required for the condition to return true in a way that would make sense + // to a caller of the module. + // + // The error message expression has the same variables available for + // interpolation as the corresponding condition. + ErrorMessage hcl.Expression DeclRange hcl.Range } @@ -111,77 +112,12 @@ func decodeCheckRuleBlock(block *hcl.Block, override bool) (*CheckRule, hcl.Diag } if attr, exists := content.Attributes["error_message"]; exists { - moreDiags := gohcl.DecodeExpression(attr.Expr, nil, &cr.ErrorMessage) - diags = append(diags, moreDiags...) - if !moreDiags.HasErrors() { - const errSummary = "Invalid validation error message" - switch { - case cr.ErrorMessage == "": - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: errSummary, - Detail: "An empty string is not a valid nor useful error message.", - Subject: attr.Expr.Range().Ptr(), - }) - case !looksLikeSentences(cr.ErrorMessage): - // Because we're going to include this string verbatim as part - // of a bigger error message written in our usual style in - // English, we'll require the given error message to conform - // to that. We might relax this in future if e.g. we start - // presenting these error messages in a different way, or if - // Terraform starts supporting producing error messages in - // other human languages, etc. - // For pragmatism we also allow sentences ending with - // exclamation points, but we don't mention it explicitly here - // because that's not really consistent with the Terraform UI - // writing style. - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: errSummary, - Detail: "The validation error message must be at least one full sentence starting with an uppercase letter and ending with a period or question mark.\n\nYour given message will be included as part of a larger Terraform error message, written as English prose. For broadly-shared modules we suggest using a similar writing style so that the overall result will be consistent.", - Subject: attr.Expr.Range().Ptr(), - }) - } - } + cr.ErrorMessage = attr.Expr } return cr, diags } -// looksLikeSentence is a simple heuristic that encourages writing error -// messages that will be presentable when included as part of a larger -// Terraform error diagnostic whose other text is written in the Terraform -// UI writing style. -// -// This is intentionally not a very strong validation since we're assuming -// that module authors want to write good messages and might just need a nudge -// about Terraform's specific style, rather than that they are going to try -// to work around these rules to write a lower-quality message. -func looksLikeSentences(s string) bool { - if len(s) < 1 { - return false - } - runes := []rune(s) // HCL guarantees that all strings are valid UTF-8 - first := runes[0] - last := runes[len(runes)-1] - - // If the first rune is a letter then it must be an uppercase letter. - // (This will only see the first rune in a multi-rune combining sequence, - // but the first rune is generally the letter if any are, and if not then - // we'll just ignore it because we're primarily expecting English messages - // right now anyway, for consistency with all of Terraform's other output.) - if unicode.IsLetter(first) && !unicode.IsUpper(first) { - return false - } - - // The string must be at least one full sentence, which implies having - // sentence-ending punctuation. - // (This assumes that if a sentence ends with quotes then the period - // will be outside the quotes, which is consistent with Terraform's UI - // writing style.) - return last == '.' || last == '?' || last == '!' -} - var checkRuleBlockSchema = &hcl.BodySchema{ Attributes: []hcl.AttributeSchema{ { diff --git a/internal/configs/named_values.go b/internal/configs/named_values.go index 970656511..10882ac08 100644 --- a/internal/configs/named_values.go +++ b/internal/configs/named_values.go @@ -345,6 +345,31 @@ func decodeVariableValidationBlock(varName string, block *hcl.Block, override bo } } + if vv.ErrorMessage != nil { + // The same applies to the validation error message, except that + // references are not required. A string literal is a valid error + // message. + goodRefs := 0 + for _, traversal := range vv.ErrorMessage.Variables() { + ref, moreDiags := addrs.ParseRef(traversal) + if !moreDiags.HasErrors() { + if addr, ok := ref.Subject.(addrs.InputVariable); ok { + if addr.Name == varName { + goodRefs++ + continue // Reference is valid + } + } + } + // If we fall out here then the reference is invalid. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid reference in variable validation", + Detail: fmt.Sprintf("The error message for variable %q can only refer to the variable itself, using var.%s.", varName, varName), + Subject: traversal.SourceRange().Ptr(), + }) + } + } + return vv, diags } diff --git a/internal/configs/named_values_test.go b/internal/configs/named_values_test.go deleted file mode 100644 index 3a44438a4..000000000 --- a/internal/configs/named_values_test.go +++ /dev/null @@ -1,33 +0,0 @@ -package configs - -import ( - "testing" -) - -func Test_looksLikeSentences(t *testing.T) { - tests := map[string]struct { - args string - want bool - }{ - "empty sentence": { - args: "", - want: false, - }, - "valid sentence": { - args: "A valid sentence.", - want: true, - }, - "valid sentence with an accent": { - args: `A Valid sentence with an accent "é".`, - want: true, - }, - } - - for name, tt := range tests { - t.Run(name, func(t *testing.T) { - if got := looksLikeSentences(tt.args); got != tt.want { - t.Errorf("looksLikeSentences() = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/internal/configs/testdata/invalid-files/variable-validation-bad-msg.tf b/internal/configs/testdata/invalid-files/variable-validation-bad-msg.tf deleted file mode 100644 index 65df45579..000000000 --- a/internal/configs/testdata/invalid-files/variable-validation-bad-msg.tf +++ /dev/null @@ -1,6 +0,0 @@ -variable "validation" { - validation { - condition = var.validation != 4 - error_message = "not four" # ERROR: Invalid validation error message - } -} diff --git a/internal/configs/testdata/invalid-files/variable-validation-condition-badref.tf b/internal/configs/testdata/invalid-files/variable-validation-condition-badref.tf index 42f4aa5f2..9b9e93576 100644 --- a/internal/configs/testdata/invalid-files/variable-validation-condition-badref.tf +++ b/internal/configs/testdata/invalid-files/variable-validation-condition-badref.tf @@ -9,3 +9,10 @@ variable "validation" { error_message = "Must be five." } } + +variable "validation_error_expression" { + validation { + condition = var.validation_error_expression != 1 + error_message = "Cannot equal ${local.foo}." # ERROR: Invalid reference in variable validation + } +} diff --git a/internal/configs/testdata/valid-files/variable_validation.tf b/internal/configs/testdata/valid-files/variable_validation.tf index 0a0c2dd50..20e227e06 100644 --- a/internal/configs/testdata/valid-files/variable_validation.tf +++ b/internal/configs/testdata/valid-files/variable_validation.tf @@ -4,3 +4,19 @@ variable "validation" { error_message = "Must be five." } } + +variable "validation_function" { + type = list(string) + validation { + condition = length(var.validation_function) > 0 + error_message = "Must not be empty." + } +} + +variable "validation_error_expression" { + type = list(string) + validation { + condition = length(var.validation_error_expression) < 10 + error_message = "Too long (${length(var.validation_error_expression)} is greater than 10)." + } +} diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index 622c4c803..ba153230c 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -2561,7 +2561,7 @@ func TestContext2Plan_preconditionErrors(t *testing.T) { { "test_resource.c.value", "Invalid condition result", - "Invalid validation condition result value: a bool is required", + "Invalid condition result value: a bool is required", }, } diff --git a/internal/terraform/context_validate_test.go b/internal/terraform/context_validate_test.go index 1f0491f1a..c049b17ce 100644 --- a/internal/terraform/context_validate_test.go +++ b/internal/terraform/context_validate_test.go @@ -2095,3 +2095,293 @@ func TestContext2Validate_nonNullableVariableDefaultValidation(t *testing.T) { t.Fatal(diags.ErrWithWarnings()) } } + +func TestContext2Validate_precondition_good(t *testing.T) { + p := testProvider("aws") + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "aws_instance": { + Attributes: map[string]*configschema.Attribute{ + "foo": {Type: cty.String, Optional: true}, + }, + }, + }, + }) + m := testModuleInline(t, map[string]string{ + "main.tf": ` +terraform { + experiments = [preconditions_postconditions] +} + +variable "input" { + type = string + default = "foo" +} + +resource "aws_instance" "test" { + foo = var.input + + lifecycle { + precondition { + condition = length(var.input) > 0 + error_message = "Input cannot be empty." + } + } +} + `, + }) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), + }, + }) + + diags := ctx.Validate(m) + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } +} + +func TestContext2Validate_precondition_badCondition(t *testing.T) { + p := testProvider("aws") + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "aws_instance": { + Attributes: map[string]*configschema.Attribute{ + "foo": {Type: cty.String, Optional: true}, + }, + }, + }, + }) + m := testModuleInline(t, map[string]string{ + "main.tf": ` +terraform { + experiments = [preconditions_postconditions] +} + +variable "input" { + type = string + default = "foo" +} + +resource "aws_instance" "test" { + foo = var.input + + lifecycle { + precondition { + condition = length(one(var.input)) == 1 + error_message = "You can't do that." + } + } +} + `, + }) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), + }, + }) + + diags := ctx.Validate(m) + if !diags.HasErrors() { + t.Fatalf("succeeded; want error") + } + if got, want := diags.Err().Error(), "Invalid function argument"; !strings.Contains(got, want) { + t.Errorf("unexpected error.\ngot: %s\nshould contain: %q", got, want) + } +} + +func TestContext2Validate_precondition_badErrorMessage(t *testing.T) { + p := testProvider("aws") + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "aws_instance": { + Attributes: map[string]*configschema.Attribute{ + "foo": {Type: cty.String, Optional: true}, + }, + }, + }, + }) + m := testModuleInline(t, map[string]string{ + "main.tf": ` +terraform { + experiments = [preconditions_postconditions] +} + +variable "input" { + type = string + default = "foo" +} + +resource "aws_instance" "test" { + foo = var.input + + lifecycle { + precondition { + condition = var.input != "foo" + error_message = "This is a bad use of a function: ${one(var.input)}." + } + } +} + `, + }) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), + }, + }) + + diags := ctx.Validate(m) + if !diags.HasErrors() { + t.Fatalf("succeeded; want error") + } + if got, want := diags.Err().Error(), "Invalid function argument"; !strings.Contains(got, want) { + t.Errorf("unexpected error.\ngot: %s\nshould contain: %q", got, want) + } +} + +func TestContext2Validate_postcondition_good(t *testing.T) { + p := testProvider("aws") + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "aws_instance": { + Attributes: map[string]*configschema.Attribute{ + "foo": {Type: cty.String, Optional: true}, + }, + }, + }, + }) + m := testModuleInline(t, map[string]string{ + "main.tf": ` +terraform { + experiments = [preconditions_postconditions] +} + +resource "aws_instance" "test" { + foo = "foo" + + lifecycle { + postcondition { + condition = length(self.foo) > 0 + error_message = "Input cannot be empty." + } + } +} + `, + }) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), + }, + }) + + diags := ctx.Validate(m) + if diags.HasErrors() { + t.Fatal(diags.ErrWithWarnings()) + } +} + +func TestContext2Validate_postcondition_badCondition(t *testing.T) { + p := testProvider("aws") + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "aws_instance": { + Attributes: map[string]*configschema.Attribute{ + "foo": {Type: cty.String, Optional: true}, + }, + }, + }, + }) + // This postcondition's condition expression does not refer to self, which + // is unrealistic. This is because at the time of writing the test, self is + // always an unknown value of dynamic type during validation. As a result, + // validation of conditions which refer to resource arguments is not + // possible until plan time. For now we exercise the code by referring to + // an input variable. + m := testModuleInline(t, map[string]string{ + "main.tf": ` +terraform { + experiments = [preconditions_postconditions] +} + +variable "input" { + type = string + default = "foo" +} + +resource "aws_instance" "test" { + foo = var.input + + lifecycle { + postcondition { + condition = length(one(var.input)) == 1 + error_message = "You can't do that." + } + } +} + `, + }) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), + }, + }) + + diags := ctx.Validate(m) + if !diags.HasErrors() { + t.Fatalf("succeeded; want error") + } + if got, want := diags.Err().Error(), "Invalid function argument"; !strings.Contains(got, want) { + t.Errorf("unexpected error.\ngot: %s\nshould contain: %q", got, want) + } +} + +func TestContext2Validate_postcondition_badErrorMessage(t *testing.T) { + p := testProvider("aws") + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "aws_instance": { + Attributes: map[string]*configschema.Attribute{ + "foo": {Type: cty.String, Optional: true}, + }, + }, + }, + }) + m := testModuleInline(t, map[string]string{ + "main.tf": ` +terraform { + experiments = [preconditions_postconditions] +} + +resource "aws_instance" "test" { + foo = "foo" + + lifecycle { + postcondition { + condition = self.foo != "foo" + error_message = "This is a bad use of a function: ${one("foo")}." + } + } +} + `, + }) + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p), + }, + }) + + diags := ctx.Validate(m) + if !diags.HasErrors() { + t.Fatalf("succeeded; want error") + } + if got, want := diags.Err().Error(), "Invalid function argument"; !strings.Contains(got, want) { + t.Errorf("unexpected error.\ngot: %s\nshould contain: %q", got, want) + } +} diff --git a/internal/terraform/eval_conditions.go b/internal/terraform/eval_conditions.go index 302f88160..ad4d0cc4d 100644 --- a/internal/terraform/eval_conditions.go +++ b/internal/terraform/eval_conditions.go @@ -3,6 +3,7 @@ package terraform import ( "fmt" "log" + "strings" "github.com/hashicorp/hcl/v2" "github.com/zclconf/go-cty/cty" @@ -59,12 +60,20 @@ func evalCheckRules(typ checkType, rules []*configs.CheckRule, ctx EvalContext, refs, moreDiags := lang.ReferencesInExpr(rule.Condition) ruleDiags = ruleDiags.Append(moreDiags) + moreRefs, moreDiags := lang.ReferencesInExpr(rule.ErrorMessage) + ruleDiags = ruleDiags.Append(moreDiags) + refs = append(refs, moreRefs...) + scope := ctx.EvaluationScope(self, keyData) hclCtx, moreDiags := scope.EvalContext(refs) ruleDiags = ruleDiags.Append(moreDiags) result, hclDiags := rule.Condition.Value(hclCtx) ruleDiags = ruleDiags.Append(hclDiags) + + errorValue, errorDiags := rule.ErrorMessage.Value(hclCtx) + ruleDiags = ruleDiags.Append(errorDiags) + diags = diags.Append(ruleDiags) if ruleDiags.HasErrors() { @@ -91,7 +100,7 @@ func evalCheckRules(typ checkType, rules []*configs.CheckRule, ctx EvalContext, diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: errInvalidCondition, - Detail: fmt.Sprintf("Invalid validation condition result value: %s.", tfdiags.FormatError(err)), + Detail: fmt.Sprintf("Invalid condition result value: %s.", tfdiags.FormatError(err)), Subject: rule.Condition.Range().Ptr(), Expression: rule.Condition, EvalContext: hclCtx, @@ -99,16 +108,38 @@ func evalCheckRules(typ checkType, rules []*configs.CheckRule, ctx EvalContext, continue } - if result.False() { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: typ.FailureSummary(), - Detail: rule.ErrorMessage, - Subject: rule.Condition.Range().Ptr(), - Expression: rule.Condition, - EvalContext: hclCtx, - }) + if result.True() { + continue } + + var errorMessage string + if !errorDiags.HasErrors() && errorValue.IsKnown() && !errorValue.IsNull() { + var err error + errorValue, err = convert.Convert(errorValue, cty.String) + if err != nil { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid error message", + Detail: fmt.Sprintf("Unsuitable value for error message: %s.", tfdiags.FormatError(err)), + Subject: rule.ErrorMessage.Range().Ptr(), + Expression: rule.ErrorMessage, + EvalContext: hclCtx, + }) + } else { + errorMessage = strings.TrimSpace(errorValue.AsString()) + } + } + if errorMessage == "" { + errorMessage = "Failed to evaluate condition error message." + } + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: typ.FailureSummary(), + Detail: errorMessage, + Subject: rule.Condition.Range().Ptr(), + Expression: rule.Condition, + EvalContext: hclCtx, + }) } return diags diff --git a/internal/terraform/eval_variable.go b/internal/terraform/eval_variable.go index fd57a136f..629da7051 100644 --- a/internal/terraform/eval_variable.go +++ b/internal/terraform/eval_variable.go @@ -3,8 +3,10 @@ package terraform import ( "fmt" "log" + "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" @@ -205,11 +207,67 @@ func evalVariableValidations(addr addrs.AbsInputVariableInstance, config *config for _, validation := range config.Validations { const errInvalidCondition = "Invalid variable validation result" const errInvalidValue = "Invalid value for variable" + var ruleDiags tfdiags.Diagnostics result, moreDiags := validation.Condition.Value(hclCtx) - diags = diags.Append(moreDiags) - if moreDiags.HasErrors() { - log.Printf("[TRACE] evalVariableValidations: %s rule %s condition expression failed: %s", addr, validation.DeclRange, diags.Err().Error()) + ruleDiags = ruleDiags.Append(moreDiags) + errorValue, errorDiags := validation.ErrorMessage.Value(hclCtx) + + // 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 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) @@ -245,26 +303,53 @@ func evalVariableValidations(addr addrs.AbsInputVariableInstance, config *config // we discard the marks now. result, _ = result.Unmark() - if result.False() { - if expr != nil { + if result.True() { + continue + } + + var errorMessage string + if !errorDiags.HasErrors() && errorValue.IsKnown() && !errorValue.IsNull() { + var err error + errorValue, err = convert.Convert(errorValue, cty.String) + if err != nil { diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: errInvalidValue, - Detail: fmt.Sprintf("%s\n\nThis was checked by the validation rule at %s.", validation.ErrorMessage, validation.DeclRange.String()), - Subject: expr.Range().Ptr(), + Severity: hcl.DiagError, + Summary: "Invalid error message", + Detail: fmt.Sprintf("Unsuitable value for error message: %s.", tfdiags.FormatError(err)), + Subject: validation.ErrorMessage.Range().Ptr(), + Expression: validation.ErrorMessage, + EvalContext: hclCtx, }) } else { - // Since we don't have a source expression for a root module - // variable, we'll just report the error from the perspective - // of the variable declaration itself. - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: errInvalidValue, - Detail: fmt.Sprintf("%s\n\nThis was checked by the validation rule at %s.", validation.ErrorMessage, validation.DeclRange.String()), - Subject: config.DeclRange.Ptr(), - }) + errorMessage = strings.TrimSpace(errorValue.AsString()) } } + if errorMessage == "" { + errorMessage = "Failed to evaluate condition error message." + } + + if expr != nil { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: errInvalidValue, + Detail: fmt.Sprintf("%s\n\nThis was checked by the validation rule at %s.", errorMessage, validation.DeclRange.String()), + Subject: expr.Range().Ptr(), + Expression: validation.Condition, + EvalContext: hclCtx, + }) + } else { + // Since we don't have a source expression for a root module + // variable, we'll just report the error from the perspective + // of the variable declaration itself. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: errInvalidValue, + Detail: fmt.Sprintf("%s\n\nThis was checked by the validation rule at %s.", errorMessage, validation.DeclRange.String()), + Subject: config.DeclRange.Ptr(), + Expression: validation.Condition, + EvalContext: hclCtx, + }) + } } return diags 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()) + } + } + }) + } +} diff --git a/internal/terraform/node_resource_abstract.go b/internal/terraform/node_resource_abstract.go index bc22d4be7..a15214b79 100644 --- a/internal/terraform/node_resource_abstract.go +++ b/internal/terraform/node_resource_abstract.go @@ -176,10 +176,14 @@ func (n *NodeAbstractResource) References() []*addrs.Reference { for _, check := range c.Preconditions { refs, _ := lang.ReferencesInExpr(check.Condition) result = append(result, refs...) + refs, _ = lang.ReferencesInExpr(check.ErrorMessage) + result = append(result, refs...) } for _, check := range c.Postconditions { refs, _ := lang.ReferencesInExpr(check.Condition) result = append(result, refs...) + refs, _ = lang.ReferencesInExpr(check.ErrorMessage) + result = append(result, refs...) } return result diff --git a/internal/terraform/node_resource_validate.go b/internal/terraform/node_resource_validate.go index f3eda571e..4f316906d 100644 --- a/internal/terraform/node_resource_validate.go +++ b/internal/terraform/node_resource_validate.go @@ -41,6 +41,18 @@ func (n *NodeValidatableResource) Path() addrs.ModuleInstance { func (n *NodeValidatableResource) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) { diags = diags.Append(n.validateResource(ctx)) + var self addrs.Referenceable + switch { + case n.Config.Count != nil: + self = n.Addr.Resource.Instance(addrs.IntKey(0)) + case n.Config.ForEach != nil: + self = n.Addr.Resource.Instance(addrs.StringKey("")) + default: + self = n.Addr.Resource.Instance(addrs.NoKey) + } + diags = diags.Append(validateCheckRules(ctx, n.Config.Preconditions, nil)) + diags = diags.Append(validateCheckRules(ctx, n.Config.Postconditions, self)) + if managed := n.Config.Managed; managed != nil { hasCount := n.Config.Count != nil hasForEach := n.Config.ForEach != nil @@ -466,6 +478,20 @@ func (n *NodeValidatableResource) validateResource(ctx EvalContext) tfdiags.Diag return diags } +func validateCheckRules(ctx EvalContext, crs []*configs.CheckRule, self addrs.Referenceable) tfdiags.Diagnostics { + var diags tfdiags.Diagnostics + + for _, cr := range crs { + _, conditionDiags := ctx.EvaluateExpr(cr.Condition, cty.Bool, self) + diags = diags.Append(conditionDiags) + + _, errorMessageDiags := ctx.EvaluateExpr(cr.ErrorMessage, cty.String, self) + diags = diags.Append(errorMessageDiags) + } + + return diags +} + func validateCount(ctx EvalContext, expr hcl.Expression) (diags tfdiags.Diagnostics) { val, countDiags := evaluateCountExpressionValue(expr, ctx) // If the value isn't known then that's the best we can do for now, but diff --git a/internal/terraform/node_root_variable_test.go b/internal/terraform/node_root_variable_test.go index ccf3b3e41..537cecce9 100644 --- a/internal/terraform/node_root_variable_test.go +++ b/internal/terraform/node_root_variable_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/hcltest" "github.com/zclconf/go-cty/cty" "github.com/hashicorp/terraform/internal/addrs" @@ -101,7 +102,7 @@ func TestNodeRootVariableExecute(t *testing.T) { } return cty.True, nil }), - ErrorMessage: "Must be a number.", + ErrorMessage: hcltest.MockExprLiteral(cty.StringVal("Must be a number.")), }, }, }, diff --git a/website/docs/language/expressions/preconditions-postconditions.mdx b/website/docs/language/expressions/preconditions-postconditions.mdx index 1aebff3b7..635cb1b63 100644 --- a/website/docs/language/expressions/preconditions-postconditions.mdx +++ b/website/docs/language/expressions/preconditions-postconditions.mdx @@ -335,11 +335,15 @@ Error: Resource postcondition failed The selected AMI must be tagged with the Component value "nomad-server". ``` -The `error_message` argument must always be a literal string, and should -typically be written as a full sentence in a style similar to Terraform's own -error messages. Terraform will show the given message alongside the name -of the resource that detected the problem and any outside values used as part -of the condition expression. +The `error_message` argument can be any expression which evaluates to a string. +This includes literal strings, heredocs, and template expressions. Multi-line +error messages are supported, and lines with leading whitespace will not be +word wrapped. + +Error message should typically be written as one or more full sentences in a +style similar to Terraform's own error messages. Terraform will show the given +message alongside the name of the resource that detected the problem and any +outside values used as part of the condition expression. ## Preconditions or Postconditions? diff --git a/website/docs/language/values/variables.mdx b/website/docs/language/values/variables.mdx index 4bfe6e053..434c88a24 100644 --- a/website/docs/language/values/variables.mdx +++ b/website/docs/language/values/variables.mdx @@ -198,10 +198,13 @@ variable "image_id" { ``` If `condition` evaluates to `false`, Terraform will produce an error message -that includes the sentences given in `error_message`. The error message string +that includes the result of the `error_message` expression. The error message should be at least one full sentence explaining the constraint that failed, using a sentence structure similar to the above examples. +Error messages can be literal strings, heredocs, or template expressions. The +only valid reference in an error message is the variable under validation. + Multiple `validation` blocks can be declared in which case error messages will be returned for _all_ failed conditions.