terraform: Stabilize the variable_validation_crossref experiment

Previously we introduced a language experiment that would permit variable
validation rules to refer to other objects declared in the same module
as the variable.

Now that experiment is concluded and its behavior is available for all
modules.

This final version deviates slightly from the experiment: we learned from
the experimental implementation that we accidentally made the "validate"
command able to validate constant-valued input variables in child modules
despite the usual rule that input variables are unknown during validation,
because the previous compromise bypassed the main expression evaluator
and built its own evaluation context directly.

Even though that behavior was not intended, it's a useful behavior that is
protected by our compatibility promises and so this commit includes a
slightly hacky emulation of that behavior, in eval_variable.go, that
fetches the variable value in the same way the old implementation would
have and then modifies the hcl evaluation context to include that value,
while preserving anything else that our standard evaluation context
builder put in there. That narrowly preserves the old behavior for
expressions that compare the variable value directly to a constant, while
treating all other references (which were previously totally invalid) in
the standard way. This quirk was already covered by the existing test
TestContext2Validate_variableCustomValidationsFail, which fails if the
special workaround is removed.

Author: apparentlymart

internal/configs/experiments.go

@@ -208,19 +208,5 @@ func checkModuleExperiments(m *Module) hcl.Diagnostics {
 		}
 	*/
 
-	if !m.ActiveExperiments.Has(experiments.VariableValidationCrossRef) {
-		// Without this experiment, validation rules are subject to the old
-		// rule that they can only refer to the variable whose value they
-		// are checking. This experiment removes that constraint, and makes
-		// the modules runtime responsible for validating and evaluating
-		// the conditions and error messages, just as we'd do for any other
-		// dynamic expression.
-		for varName, vc := range m.Variables {
-			for _, vv := range vc.Validations {
-				diags = append(diags, checkVariableValidationBlock(varName, vv)...)
-			}
-		}
-	}
-
 	return diags
 }

internal/configs/named_values.go

@@ -178,7 +178,7 @@ func decodeVariableBlock(block *hcl.Block, override bool) (*Variable, hcl.Diagno
 		switch block.Type {
 
 		case "validation":
-			vv, moreDiags := decodeVariableValidationBlock(block, override)
+			vv, moreDiags := decodeCheckRuleBlock(block, override)
 			diags = append(diags, moreDiags...)
 			v.Validations = append(v.Validations, vv)
 
@@ -324,77 +324,6 @@ func (m VariableParsingMode) Parse(name, value string) (cty.Value, hcl.Diagnosti
 	}
 }
 
-// decodeVariableValidationBlock is a wrapper around decodeCheckRuleBlock
-// that imposes the additional rule that the condition expression can refer
-// only to an input variable of the given name.
-func decodeVariableValidationBlock(block *hcl.Block, override bool) (*CheckRule, hcl.Diagnostics) {
-	return decodeCheckRuleBlock(block, override)
-}
-
-func checkVariableValidationBlock(varName string, vv *CheckRule) hcl.Diagnostics {
-	var diags hcl.Diagnostics
-
-	if vv.Condition != nil {
-		// The validation condition can only refer to the variable itself,
-		// to ensure that the variable declaration can't create additional
-		// edges in the dependency graph.
-		goodRefs := 0
-		for _, traversal := range vv.Condition.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 condition for variable %q can only refer to the variable itself, using var.%s.", varName, varName),
-				Subject:  traversal.SourceRange().Ptr(),
-			})
-		}
-		if goodRefs < 1 {
-			diags = diags.Append(&hcl.Diagnostic{
-				Severity: hcl.DiagError,
-				Summary:  "Invalid variable validation condition",
-				Detail:   fmt.Sprintf("The condition for variable %q must refer to var.%s in order to test incoming values.", varName, varName),
-				Subject:  vv.Condition.Range().Ptr(),
-			})
-		}
-	}
-
-	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 diags
-}
-
 // Output represents an "output" block in a module or file.
 type Output struct {
 	Name        string

internal/configs/testdata/invalid-modules/variable-validation-condition-badref/variable-validation-condition-badref.tf renamed to internal/configs/testdata/valid-files/variable-validation-condition-crossref.tf

@@ -5,14 +5,14 @@ locals {
 
 variable "validation" {
   validation {
-    condition     = local.foo == var.validation # ERROR: Invalid reference in variable validation
+    condition     = local.foo == var.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
+    error_message = "Cannot equal ${local.foo}."
   }
 }

internal/experiments/experiment.go

@@ -31,7 +31,7 @@ func init() {
 	// a current or a concluded experiment.
 	registerConcludedExperiment(UnknownInstances, "Unknown instances are being rolled into a larger feature for deferring unready resources and modules.")
 	registerConcludedExperiment(VariableValidation, "Custom variable validation can now be used by default, without enabling an experiment.")
-	registerCurrentExperiment(VariableValidationCrossRef)
+	registerConcludedExperiment(VariableValidationCrossRef, "Input variable validation rules may now refer to other objects in the same module without enabling any experiment.")
 	registerConcludedExperiment(SuppressProviderSensitiveAttrs, "Provider-defined sensitive attributes are now redacted by default, without enabling an experiment.")
 	registerCurrentExperiment(TemplateStringFunc)
 	registerConcludedExperiment(ConfigDrivenMove, "Declarations of moved resource instances using \"moved\" blocks can now be used by default, without enabling an experiment.")

internal/terraform/context_plan_test.go

@@ -6921,8 +6921,6 @@ resource "test_resource" "foo" {
 }
 
 func TestContext2Plan_variableCustomValidationsSimple(t *testing.T) {
-	// This test is dealing with validation rules that refer to other objects
-	// in the same module.
 	m := testModuleInline(t, map[string]string{
 		"main.tf": `
 			variable "a" {
@@ -6978,11 +6976,6 @@ func TestContext2Plan_variableCustomValidationsCrossRef(t *testing.T) {
 	// in the same module.
 	m := testModuleInline(t, map[string]string{
 		"main.tf": `
-			# Validation cross-references are currently experimental
-			terraform {
-				experiments = [variable_validation_crossref]
-			}
-
 			variable "a" {
 				type = string
 			}

internal/terraform/eval_variable.go

@@ -215,67 +215,6 @@ func evalVariableValidations(addr addrs.AbsInputVariableInstance, ctx EvalContex
 		return diags
 	}
 
-	// Validation expressions are statically validated (during configuration
-	// loading) to refer only to the variable being validated, so we can
-	// bypass our usual evaluation machinery here and just produce a minimal
-	// evaluation context containing just the required value.
-	val := ctx.NamedValues().GetInputVariableValue(addr)
-	if val == cty.NilVal {
-		diags = diags.Append(&hcl.Diagnostic{
-			Severity: hcl.DiagError,
-			Summary:  "No final value for variable",
-			Detail:   fmt.Sprintf("Terraform doesn't have a final value for %s during validation. This is a bug in Terraform; please report it!", addr),
-		})
-		return diags
-	}
-	hclCtx := &hcl.EvalContext{
-		Variables: map[string]cty.Value{
-			"var": cty.ObjectVal(map[string]cty.Value{
-				addr.Variable.Name: val,
-			}),
-		},
-		Functions: ctx.EvaluationScope(nil, nil, EvalDataForNoInstanceKey).Functions(),
-	}
-
-	for ix, validation := range rules {
-		result, ruleDiags := evalVariableValidation(validation, hclCtx, valueRng, addr, ix)
-		diags = diags.Append(ruleDiags)
-
-		log.Printf("[TRACE] evalVariableValidations: %s status is now %s", addr, result.Status)
-		if result.Status == checks.StatusFail {
-			checkState.ReportCheckFailure(addr, addrs.InputValidation, ix, result.FailureMessage)
-		} else {
-			checkState.ReportCheckResult(addr, addrs.InputValidation, ix, result.Status)
-		}
-	}
-
-	return diags
-}
-
-// evalVariableValidationsCrossRef is an experimental variant of
-// [evalVariableValidations] that allows arbitrary references to any object
-// declared in the same module as the variable.
-//
-// If the experiment is successful, this function should replace
-// [evalVariableValidations], but it's currently written separately to minimize
-// the risk of the experiment impacting non-opted modules.
-func evalVariableValidationsCrossRef(addr addrs.AbsInputVariableInstance, ctx EvalContext, rules []*configs.CheckRule, valueRng hcl.Range) (diags tfdiags.Diagnostics) {
-	if len(rules) == 0 {
-		log.Printf("[TRACE] evalVariableValidations: no validation rules declared for %s, so skipping", addr)
-		return nil
-	}
-	log.Printf("[TRACE] evalVariableValidations: validating %s", addr)
-
-	checkState := ctx.Checks()
-	if !checkState.ConfigHasChecks(addr.ConfigCheckable()) {
-		// We have nothing to do if this object doesn't have any checks,
-		// but the "rules" slice should agree that we don't.
-		if ct := len(rules); ct != 0 {
-			panic(fmt.Sprintf("check state says that %s should have no rules, but it has %d", addr, ct))
-		}
-		return diags
-	}
-
 	// We'll build just one evaluation context covering the data needed by
 	// all of the rules together, since that'll minimize lock contention
 	// on the state, plan, etc.
@@ -300,6 +239,46 @@ func evalVariableValidationsCrossRef(addr addrs.AbsInputVariableInstance, ctx Ev
 		return diags
 	}
 
+	// HACK: Historically we manually built a very constrained hcl.EvalContext
+	// here, which only included the value of the one specific input variable
+	// we're validating, since we didn't yet support referring to anything
+	// else. That accidentally bypassed our rule that input variables are
+	// always unknown during the validate walk, and thus accidentally created
+	// a useful behavior of actually checking constant-only values against
+	// their validation rules just during "terraform validate", rather than
+	// having to run "terraform plan".
+	//
+	// Although that behavior was accidental, it makes simple validation rules
+	// more useful and is protected by compatibility promises, and so we'll
+	// fake it here by overwriting the unknown value that scope.EvalContext
+	// will have inserted with a possibly-more-known value using the same
+	// strategy our special code used to use.
+	ourVal := ctx.NamedValues().GetInputVariableValue(addr)
+	if ourVal != cty.NilVal {
+		// (it would be weird for ourVal to be nil here, but we'll tolerate it
+		// because it was scope.EvalContext's responsibility to check for the
+		// absent final value, and even if it didn't we'll just get an
+		// evaluation error when evaluating the expressions below anyway.)
+
+		// Our goal here is to make sure that a reference to the variable
+		// we're checking will evaluate to ourVal, regardless of what else
+		// scope.EvalContext might have put in the variables table.
+		if hclCtx.Variables == nil {
+			hclCtx.Variables = make(map[string]cty.Value)
+		}
+		if varsVal, ok := hclCtx.Variables["var"]; ok {
+			// Unfortunately we need to unpack and repack the object here,
+			// because cty values are immutable.
+			attrs := varsVal.AsValueMap()
+			attrs[addr.Variable.Name] = ourVal
+			hclCtx.Variables["var"] = cty.ObjectVal(attrs)
+		} else {
+			hclCtx.Variables["var"] = cty.ObjectVal(map[string]cty.Value{
+				addr.Variable.Name: ourVal,
+			})
+		}
+	}
+
 	for ix, validation := range rules {
 		result, ruleDiags := evalVariableValidation(validation, hclCtx, valueRng, addr, ix)
 		diags = diags.Append(ruleDiags)

internal/terraform/eval_variable_test.go

@@ -1173,7 +1173,13 @@ func TestEvalVariableValidations_jsonErrorMessageEdgeCase(t *testing.T) {
 
 			// We need a minimal scope to allow basic functions to be passed to
 			// the HCL scope
-			ctx.EvaluationScopeScope = &lang.Scope{}
+			ctx.EvaluationScopeScope = &lang.Scope{
+				Data: &fakeEvaluationData{
+					inputVariables: map[addrs.InputVariable]cty.Value{
+						varAddr.Variable: test.given,
+					},
+				},
+			}
 			ctx.NamedValuesState = namedvals.NewState()
 			ctx.NamedValuesState.SetInputVariableValue(varAddr, test.given)
 			ctx.ChecksState = checks.NewState(cfg)
@@ -1322,13 +1328,19 @@ variable "bar" {
 
 			// We need a minimal scope to allow basic functions to be passed to
 			// the HCL scope
-			ctx.EvaluationScopeScope = &lang.Scope{}
-			ctx.NamedValuesState = namedvals.NewState()
+			varVal := test.given
 			if varCfg.Sensitive {
-				ctx.NamedValuesState.SetInputVariableValue(varAddr, test.given.Mark(marks.Sensitive))
-			} else {
-				ctx.NamedValuesState.SetInputVariableValue(varAddr, test.given)
+				varVal = varVal.Mark(marks.Sensitive)
 			}
+			ctx.EvaluationScopeScope = &lang.Scope{
+				Data: &fakeEvaluationData{
+					inputVariables: map[addrs.InputVariable]cty.Value{
+						varAddr.Variable: varVal,
+					},
+				},
+			}
+			ctx.NamedValuesState = namedvals.NewState()
+			ctx.NamedValuesState.SetInputVariableValue(varAddr, varVal)
 			ctx.ChecksState = checks.NewState(cfg)
 			ctx.ChecksState.ReportCheckableObjects(varAddr.ConfigCheckable(), addrs.MakeSet[addrs.Checkable](varAddr))
 

internal/terraform/graph_builder_apply.go

@@ -125,9 +125,7 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer {
 			Config:       b.Config,
 			DestroyApply: b.Operation == walkDestroy,
 		},
-		&variableValidationTransformer{
-			config: b.Config,
-		},
+		&variableValidationTransformer{},
 		&LocalTransformer{Config: b.Config},
 		&OutputTransformer{
 			Config:     b.Config,

internal/terraform/graph_builder_eval.go

@@ -76,9 +76,7 @@ func (b *EvalGraphBuilder) Steps() []GraphTransformer {
 		// Add dynamic values
 		&RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues, Planning: true},
 		&ModuleVariableTransformer{Config: b.Config, Planning: true},
-		&variableValidationTransformer{
-			config: b.Config,
-		},
+		&variableValidationTransformer{},
 		&LocalTransformer{Config: b.Config},
 		&OutputTransformer{
 			Config:   b.Config,

internal/terraform/graph_builder_plan.go

@@ -159,9 +159,7 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer {
 			Planning:     true,
 			DestroyApply: false, // always false for planning
 		},
-		&variableValidationTransformer{
-			config: b.Config,
-		},
+		&variableValidationTransformer{},
 		&LocalTransformer{Config: b.Config},
 		&OutputTransformer{
 			Config:      b.Config,