core: Check rule error message expressions

Error messages for preconditions, postconditions, and custom variable
validations have until now been string literals. This commit changes
this to treat the field as an HCL expression, which must evaluate to a
string. Most commonly this will either be a string literal or a template
expression.

When the check rule condition is evaluated, we also evaluate the error
message. This means that the error message should always evaluate to a
string value, even if the condition passes. If it does not, this will
result in an error diagnostic.

If the condition fails, and the error message also fails to evaluate, we
fall back to a default error message. This means that the check rule
failure will still be reported, alongside diagnostics explaining why the
custom error message failed to render.

As part of this change, we also necessarily remove the heuristic about
the error message format. This guidance can be readded in future as part
of a configuration hint system.

Author: alisdair

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{
 		{

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
 }
 

internal/configs/named_values_test.go

@@ -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
+  }
+}

internal/configs/testdata/invalid-files/variable-validation-bad-msg.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)."
+  }
+}

internal/configs/testdata/invalid-files/variable-validation-condition-badref.tf

@@ -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",
 		},
 	}
 

internal/configs/testdata/valid-files/variable_validation.tf

@@ -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,24 +100,46 @@ 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,
 			})
 			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