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/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..4ac53ed33 100644 --- a/internal/terraform/eval_variable.go +++ b/internal/terraform/eval_variable.go @@ -3,6 +3,7 @@ package terraform import ( "fmt" "log" + "strings" "github.com/hashicorp/hcl/v2" "github.com/hashicorp/terraform/internal/addrs" @@ -205,11 +206,17 @@ 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) + 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()) } if !result.IsKnown() { log.Printf("[TRACE] evalVariableValidations: %s rule %s condition value is unknown, so skipping validation for now", addr, validation.DeclRange) @@ -245,26 +252,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/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_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.")), }, }, },