configs+tofu: alternative check block style with error_messages

The original design for check blocks was a boolean argument "condition"
which returns true for valid and false for invalid, and then a separate
string argument "error_message" whose value is displayed as part of an
error diagnostic if the value is found to be invalid.

That design has worked well for simple primitive-typed variables, but it's
annoying for variables of collection types because in that case we
typically want to apply a validation rule to each element and then mention
in the error messages exactly which elements were invalid, which requires
both a complicated "condition" expression _and_ a very similar complicated
"error_message" expression to repeat essentially the same steps to find
out which elements were invalid.

This change is a prototype for an optional new design where the block
contains only a single argument error_messages that deals with both the
condition and the error message generation at the same time. Its expression
must return a list of strings where an empty list represents "valid" and
a non-empty list represents "invalid". In the invalid case, each element
is returned as a separate error diagnostic.

This means that module authors can use a "for" expression with an "if"
clause to filter out all of the valid elements and to project any invalid
elements into an error messages describing what is wrong with them.

In principle this could also be used with new provider-defined functions
that are designed to take a value and return a list of problems with that
value, such as in a hypothetical provider that implements JSON schema-based
validation of a data structure using an externally-provided schema.

This is only a prototype so the test coverage is rudimentary and it only
currently works for input variable validation. If we decide that we'd like
to implement something like this for real then we'll want to extend it to
work for all of the other kinds of checks -- test assertions, "check"
block assertions, and preconditions/postconditions -- since the language
design intent is for those to all appear to have essentially the same
treatment, despite not all of the code currently being shared between
them.

Signed-off-by: Martin Atkins <mart@degeneration.co.uk>

internal/configs/checks.go

@@ -27,6 +27,9 @@ type CheckRule struct {
 	// The available variables in a condition expression vary depending on what
 	// a check is attached to. For example, validation rules attached to
 	// input variables can only refer to the variable that is being validated.
+	//
+	// This field is nil whenever ErrorMessages is set, and is always set
+	// in conjunction with ErrorMessage.
 	Condition hcl.Expression
 
 	// ErrorMessage should be one or more full sentences, which should be in
@@ -37,8 +40,23 @@ type CheckRule struct {
 	//
 	// The error message expression has the same variables available for
 	// interpolation as the corresponding condition.
+	//
+	// This field is nil whenever ErrorMessages is set, and is always set
+	// in conjunction with Condition.
 	ErrorMessage hcl.Expression
 
+	// ErrorMessages is used for the multi-message variant of a validation
+	// rule, where the block contains only a single argument error_messages
+	// that incorporates both the condition and error message generation.
+	//
+	// In this case the expression must return something that can convert
+	// to a list of strings where an empty list represents that the value
+	// is valid while a non-empty list represents a list of problems with
+	// the value.
+	//
+	// This field is nil whenever Condition and ErrorMessage are set.
+	ErrorMessages hcl.Expression
+
 	DeclRange hcl.Range
 }
 
@@ -131,19 +149,43 @@ func decodeCheckRuleBlock(block *hcl.Block, override bool) (*CheckRule, hcl.Diag
 		cr.ErrorMessage = attr.Expr
 	}
 
+	if attr, exists := content.Attributes["error_messages"]; exists {
+		cr.ErrorMessages = attr.Expr
+	}
+
+	if cr.ErrorMessages != nil {
+		// error_messages is mutually-exclusive with condition+error_message.
+		if cr.Condition != nil || cr.ErrorMessage != nil {
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  fmt.Sprintf("Invalid %s block arguments", block.Type),
+				Detail:   fmt.Sprintf("Each %s block must have either the error_messages argument, or both the condition and error_message arguments.", block.Type),
+				Subject:  cr.DeclRange.Ptr(),
+			})
+		}
+	} else {
+		// condition and error_message must both be set together if error_messages isn't set
+		if cr.Condition == nil || cr.ErrorMessage == nil {
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  fmt.Sprintf("Invalid %s block arguments", block.Type),
+				Detail:   fmt.Sprintf("Each %s block must have either condition and error_message arguments together, or a single error_messages argument.", block.Type),
+				Subject:  cr.DeclRange.Ptr(),
+			})
+		}
+	}
+
 	return cr, diags
 }
 
 var checkRuleBlockSchema = &hcl.BodySchema{
 	Attributes: []hcl.AttributeSchema{
-		{
-			Name:     "condition",
-			Required: true,
-		},
-		{
-			Name:     "error_message",
-			Required: true,
-		},
+		// This initial schema treats these all as optional, but during decoding
+		// we required that either condition+error_message are both set xor
+		// error_messages is set.
+		{Name: "condition"},
+		{Name: "error_message"},
+		{Name: "error_messages"},
 	},
 }
 

internal/tofu/context_plan_test.go

@@ -13,6 +13,8 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
+	"regexp"
+	"slices"
 	"sort"
 	"strings"
 	"sync"
@@ -24,6 +26,7 @@ import (
 	"github.com/zclconf/go-cty/cty"
 
 	"github.com/opentofu/opentofu/internal/addrs"
+	"github.com/opentofu/opentofu/internal/checks"
 	"github.com/opentofu/opentofu/internal/configs/configschema"
 	"github.com/opentofu/opentofu/internal/configs/hcl2shim"
 	"github.com/opentofu/opentofu/internal/lang/marks"
@@ -6016,6 +6019,78 @@ resource "aws_instance" "foo" {
 	}
 }
 
+func TestContext2Validate_variableCustomValidationErrorMessages(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		`main.tf`: `
+			variable "test" {
+				type = list(string)
+
+				validation {
+					error_messages = [
+						for i, s in var.test : "Element ${i} must start with \"thingy-\"."
+						if !startswith(s, "thingy-")
+					]
+				}
+			}
+		`,
+	})
+	tofuCtx := testContext2(t, &ContextOpts{})
+
+	ctx := context.Background()
+	plan, diags := tofuCtx.Plan(ctx, m, nil, &PlanOpts{
+		SetVariables: InputValues{
+			"test": {
+				Value: cty.ListVal([]cty.Value{
+					cty.StringVal("thingy-0"),
+					cty.StringVal("thingy-1"),
+					cty.StringVal("thnigy-2"), // intentional typo for testing purposes
+					cty.StringVal("thingy-3"),
+					cty.UnknownVal(cty.String).Refine().StringPrefix("thingy-").NotNull().NewValue(),
+					cty.UnknownVal(cty.String).Refine().StringPrefix("thinngy-").NotNull().NewValue(), // intentional typo for testing purposes
+					cty.StringVal(""), // intentionally empty for testing purposes
+				}),
+				SourceType: ValueFromCaller,
+			},
+		},
+	})
+
+	// The input variable should have an associated check status that is marked as failed.
+	varAddr := addrs.InputVariable{Name: "test"}.Absolute(addrs.RootModuleInstance)
+	checkResult := plan.Checks.GetObjectResult(varAddr)
+	if checkResult == nil {
+		t.Errorf("%s has no check result", varAddr)
+	} else if got, want := checkResult.Status, checks.StatusFail; got != want {
+		t.Errorf("wrong check result for %s\ngot:  %s\nwant: %s", varAddr, got, want)
+	}
+
+	wantIndices := []string{
+		// These are the list indices from the test input above that do _not_
+		// conform to the validation rule and therefore produced error message
+		// strings in the error_messages argument.
+		"2", // definitely doesn't have the correct prefix
+		"5", // the refined string prefix tells us that the prefix is wrong even though the value isn't known
+		"6", // empty string does not start with the wanted prefix
+	}
+	var gotIndices []string
+	pattern := regexp.MustCompile(`Element (\d+) must start with "thingy-".`)
+	for _, diag := range diags {
+		if diag.Severity() != tfdiags.Error {
+			continue
+		}
+		detail := diag.Description().Detail
+		matches := pattern.FindStringSubmatch(detail)
+		if len(matches) == 0 {
+			continue
+		}
+		gotIndices = append(gotIndices, matches[1])
+	}
+	sort.Strings(gotIndices)
+
+	if !slices.Equal(gotIndices, wantIndices) {
+		t.Errorf("wrong indices reported as errors\ngot:  %#v\nwant: %#v\ndiagnostics:\n%s", gotIndices, wantIndices, diags.Err())
+	}
+}
+
 func TestContext2Plan_variableSensitivity(t *testing.T) {
 	m := testModule(t, "plan-variable-sensitivity")
 

internal/tofu/eval_variable.go

@@ -291,6 +291,16 @@ func evalVariableValidation(validation *configs.CheckRule, hclCtx *hcl.EvalConte
 	const errInvalidValue = "Invalid value for variable"
 	var diags tfdiags.Diagnostics
 
+	if validation.ErrorMessages != nil {
+		// This is a new-style validation rule with just a single error_messages expression
+		// that returns a list of zero or more problems, instead of the separate condition
+		// and error_message arguments.
+		// FIXME: This implementation approach is just for prototyping to minimize changes
+		// to the existing code. If we decide to implement this feature for real then
+		// we'll probably want to arrange things differently for better readability.
+		return evalVariableValidationWithErrorMessages(validation, hclCtx, addr, config, expr, ix)
+	}
+
 	result, moreDiags := validation.Condition.Value(hclCtx)
 	diags = diags.Append(moreDiags)
 	errorValue, errorDiags := validation.ErrorMessage.Value(hclCtx)
@@ -477,3 +487,112 @@ You can correct this by removing references to sensitive values, or by carefully
 		FailureMessage: errorMessage,
 	}, diags
 }
+
+func evalVariableValidationWithErrorMessages(validation *configs.CheckRule, hclCtx *hcl.EvalContext, addr addrs.AbsInputVariableInstance, _ *configs.Variable, _ hcl.Expression, _ int) (checkResult, tfdiags.Diagnostics) {
+	const invalidMessagesSummary = "Invalid variable validation error messages"
+	const invalidMessageSummary = "Invalid variable validation error message"
+	var diags tfdiags.Diagnostics
+
+	// GENERAL FIXME: At least some of the diagnostics returned from this function
+	// ought to be annotated with Extra: &addrs.CheckRuleDiagnosticExtra{ ... }
+	// to cooperate with the expectations of the "tofu test" harness.
+
+	messagesVal, moreDiags := validation.ErrorMessages.Value(hclCtx)
+	diags = diags.Append(moreDiags)
+	if moreDiags.HasErrors() {
+		return checkResult{Status: checks.StatusError}, diags
+	}
+
+	// FIXME: Need to do something about sensitive values here.
+	// Perhaps we just have a single check for whether there's any sensitivity
+	// anywhere in messagesVal, since that's invalid, or perhaps we'd prefer
+	// to try to render each error message individually and thus still return
+	// any of them that aren't sensitive, as long as the list as a whole isn't
+	// sensitive.
+
+	var convertErr error
+	messagesVal, convertErr = convert.Convert(messagesVal, cty.List(cty.String))
+	if convertErr != nil {
+		diags = diags.Append(&hcl.Diagnostic{
+			Severity:    hcl.DiagError,
+			Summary:     invalidMessagesSummary,
+			Detail:      fmt.Sprintf("Invalid result from error_messages expression: %s.", tfdiags.FormatError(convertErr)),
+			Subject:     validation.ErrorMessages.Range().Ptr(),
+			Expression:  validation.ErrorMessages,
+			EvalContext: hclCtx,
+		})
+		return checkResult{Status: checks.StatusError}, diags
+	}
+	if messagesVal.IsNull() {
+		diags = diags.Append(&hcl.Diagnostic{
+			Severity:    hcl.DiagError,
+			Summary:     invalidMessagesSummary,
+			Detail:      "Invalid result from error_messages expression: must not be null. Use an empty list to represent a valid value.",
+			Subject:     validation.ErrorMessages.Range().Ptr(),
+			Expression:  validation.ErrorMessages,
+			EvalContext: hclCtx,
+		})
+		return checkResult{Status: checks.StatusError}, diags
+	}
+
+	haveErrors := messagesVal.Length().NotEqual(cty.Zero)
+	if !haveErrors.IsKnown() {
+		log.Printf("[TRACE] evalVariableValidations: %s rule %s error_messages length is unknown, so skipping validation for now", addr, validation.DeclRange)
+		return checkResult{Status: checks.StatusUnknown}, diags // We'll wait until we've learned more, then.
+	}
+	if haveErrors.True() {
+		var errorMessages []string
+		for it := messagesVal.ElementIterator(); it.Next(); {
+			_, msgVal := it.Element()
+			if msgVal.IsNull() {
+				diags = diags.Append(&hcl.Diagnostic{
+					Severity:    hcl.DiagError,
+					Summary:     invalidMessageSummary,
+					Detail:      "Unsuitable value for error message: must not be null.",
+					Subject:     validation.ErrorMessages.Range().Ptr(),
+					Expression:  validation.ErrorMessages,
+					EvalContext: hclCtx,
+				})
+				continue
+			}
+			if !msgVal.IsKnown() {
+				diags = diags.Append(&hcl.Diagnostic{
+					Severity:    hcl.DiagError,
+					Summary:     "Invalid error message",
+					Detail:      "A value used in one of the error messages will not be known until the apply phase and so cannot be shown.",
+					Subject:     validation.ErrorMessages.Range().Ptr(),
+					Expression:  validation.ErrorMessages,
+					EvalContext: hclCtx,
+					Extra:       evalchecks.DiagnosticCausedByUnknown(true),
+				})
+				continue
+			}
+			// We've eliminated the null or unknown possibilities here.
+			// FIXME: We might need to deal with sensitive here if we don't decide
+			// to check for it across the entire list before we start above.
+			// That means we should definitely be holding a known, non-null string.
+			errorMessage := msgVal.AsString()
+			errorMessages = append(errorMessages, errorMessage)
+
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity:    hcl.DiagError,
+				Summary:     "Invalid value for variable",
+				Detail:      fmt.Sprintf("%s\n\nThis was checked by the validation rule at %s.", errorMessage, validation.DeclRange.String()),
+				Subject:     validation.ErrorMessages.Range().Ptr(),
+				Expression:  validation.ErrorMessages,
+				EvalContext: hclCtx,
+			})
+		}
+		return checkResult{
+			Status: checks.StatusFail,
+			// Since each check can only have one status and one failure message,
+			// we combine all of the error messages together here meaning that
+			// they'll get reported all together as a single failure in
+			// "tofu test" results, and in anything else that makes use of
+			// check results.
+			FailureMessage: strings.Join(errorMessages, "\n"),
+		}, diags
+	}
+
+	return checkResult{Status: checks.StatusPass}, diags
+}