From b06fe04621d7aa252ae616b209071a82b5d25dde Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Thu, 3 Feb 2022 14:14:21 -0500 Subject: [PATCH 1/4] 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. --- internal/configs/checks.go | 82 ++----------------- internal/configs/named_values.go | 25 ++++++ internal/configs/named_values_test.go | 33 -------- .../variable-validation-bad-msg.tf | 6 -- .../variable-validation-condition-badref.tf | 7 ++ .../valid-files/variable_validation.tf | 16 ++++ internal/terraform/context_plan2_test.go | 2 +- internal/terraform/eval_conditions.go | 51 +++++++++--- internal/terraform/eval_variable.go | 70 ++++++++++++---- internal/terraform/node_resource_abstract.go | 4 + internal/terraform/node_root_variable_test.go | 3 +- 11 files changed, 157 insertions(+), 142 deletions(-) delete mode 100644 internal/configs/named_values_test.go delete mode 100644 internal/configs/testdata/invalid-files/variable-validation-bad-msg.tf 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.")), }, }, }, From b59bffada68eec94b7d8741cce452b028c73543d Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Thu, 3 Feb 2022 14:14:21 -0500 Subject: [PATCH 2/4] core: Evaluate pre/postconditions during validate During the validation walk, we attempt to proactively evaluate check rule condition and error message expressions. This will help catch some errors as early as possible. At present, resource values in the validation walk are of dynamic type. This means that any references to resources will cause validation to be delayed, rather than presenting useful errors. Validation may still catch other errors, and any future changes which cause better type propagation will result in better validation too. --- internal/terraform/context_validate_test.go | 290 +++++++++++++++++++ internal/terraform/node_resource_validate.go | 26 ++ 2 files changed, 316 insertions(+) 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/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 From 45d0c04707777a9d25b8349c2f61caef5ccef16a Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Mon, 7 Feb 2022 14:46:31 -0500 Subject: [PATCH 3/4] core: Add fallback for JSON syntax error messages Custom variable validations specified using JSON syntax would always parse error messages as string literals, even if they included template expressions. We need to be as backwards compatible with this behaviour as possible, which results in this complex fallback logic. More detail about this in the extensive code comments. --- internal/terraform/eval_variable.go | 55 +++++++- internal/terraform/eval_variable_test.go | 154 +++++++++++++++++++++++ 2 files changed, 207 insertions(+), 2 deletions(-) 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()) + } + } + }) + } +} From f21d0e8bf60fd921772b783dacbba6be19629326 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Tue, 8 Feb 2022 10:42:50 -0500 Subject: [PATCH 4/4] website: Update docs for check rule error messages --- .../expressions/preconditions-postconditions.mdx | 14 +++++++++----- website/docs/language/values/variables.mdx | 5 ++++- 2 files changed, 13 insertions(+), 6 deletions(-) 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.