Split variable evaluation / validation scope (#2199)

Signed-off-by: Christian Mesh <christianmesh1@gmail.com>

internal/tofu/context_functions_test.go

@@ -759,3 +759,88 @@ variable "obfmod" {
 		t.Fatalf("Expected function call")
 	}
 }
+
+// Functions used as variable values are evaluated correctly
+func TestContext2Functions_providerFunctionsVariableCustom(t *testing.T) {
+	p := testProvider("aws")
+	p.GetFunctionsResponse = &providers.GetFunctionsResponse{
+		Functions: map[string]providers.FunctionSpec{
+			"arn_parse_custom": providers.FunctionSpec{
+				Parameters: []providers.FunctionParameterSpec{{
+					Name: "arn",
+					Type: cty.String,
+				}},
+				Return: cty.Bool,
+			},
+		},
+	}
+	p.CallFunctionResponse = &providers.CallFunctionResponse{
+		Result: cty.True,
+	}
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+terraform {
+  required_providers {
+    aws = ">=5.70.0"
+  }
+}
+
+provider "aws" {
+  region="us-east-1"
+  alias = "primary"
+}
+
+module "mod" {
+  source = "./mod"
+  providers = {
+    aws = aws.primary
+  }
+}
+ `,
+		"mod/mod.tf": `
+terraform {
+  required_providers {
+    aws = ">=5.70.0"
+  }
+}
+
+module "mod2" {
+       source = "./mod2"
+       value = provider::aws::arn_parse_custom("foo")
+}
+`,
+		"mod/mod2/mod.tf": `
+variable "value" { }
+`,
+	})
+
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p),
+		},
+	})
+
+	diags := ctx.Validate(context.Background(), m)
+	if diags.HasErrors() {
+		t.Fatal(diags.Err())
+	}
+	if p.GetFunctionsCalled {
+		t.Fatalf("Unexpected function call")
+	}
+	if p.CallFunctionCalled {
+		t.Fatalf("Unexpected function call")
+	}
+
+	p.GetFunctionsCalled = false
+	p.CallFunctionCalled = false
+	_, diags = ctx.Plan(context.Background(), m, nil, nil)
+	if diags.HasErrors() {
+		t.Fatal(diags.Err())
+	}
+	if !p.GetFunctionsCalled {
+		t.Fatalf("Expected function call")
+	}
+	if !p.CallFunctionCalled {
+		t.Fatalf("Expected function call")
+	}
+}

internal/tofu/graph_builder_plan_test.go

@@ -250,7 +250,7 @@ func TestPlanGraphBuilder_forEach(t *testing.T) {
 const testPlanGraphBuilderStr = `
 aws_instance.web (expand)
   aws_security_group.firewall (expand)
-  var.foo
+  var.foo (expand, reference)
 aws_load_balancer.weblb (expand)
   aws_instance.web (expand)
 aws_security_group.firewall (expand)
@@ -273,6 +273,8 @@ root
   provider["registry.opentofu.org/hashicorp/aws"] (close)
   provider["registry.opentofu.org/hashicorp/openstack"] (close)
 var.foo
+var.foo (expand, reference)
+  var.foo
 `
 const testPlanGraphBuilderForEachStr = `
 aws_instance.bar (expand)

internal/tofu/node_module_variable.go

@@ -47,22 +47,9 @@ func (n *nodeExpandModuleVariable) temporaryValue() bool {
 func (n *nodeExpandModuleVariable) DynamicExpand(ctx EvalContext) (*Graph, error) {
 	var g Graph
 
-	// If this variable has preconditions, we need to report these checks now.
-	//
-	// We should only do this during planning as the apply phase starts with
-	// all the same checkable objects that were registered during the plan.
-	var checkableAddrs addrs.Set[addrs.Checkable]
-	if checkState := ctx.Checks(); checkState.ConfigHasChecks(n.Addr.InModule(n.Module)) {
-		checkableAddrs = addrs.MakeSet[addrs.Checkable]()
-	}
-
 	expander := ctx.InstanceExpander()
 	for _, module := range expander.ExpandModule(n.Module) {
 		addr := n.Addr.Absolute(module)
-		if checkableAddrs != nil {
-			checkableAddrs.Add(addr)
-		}
-
 		o := &nodeModuleVariable{
 			Addr:           addr,
 			Config:         n.Config,
@@ -73,15 +60,11 @@ func (n *nodeExpandModuleVariable) DynamicExpand(ctx EvalContext) (*Graph, error
 	}
 	addRootNodeToGraph(&g)
 
-	if checkableAddrs != nil {
-		ctx.Checks().ReportCheckableObjects(n.Addr.InModule(n.Module), checkableAddrs)
-	}
-
 	return &g, nil
 }
 
 func (n *nodeExpandModuleVariable) Name() string {
-	return fmt.Sprintf("%s.%s (expand)", n.Module, n.Addr.String())
+	return fmt.Sprintf("%s.%s (expand, input)", n.Module, n.Addr.String())
 }
 
 // GraphNodeModulePath
@@ -91,27 +74,15 @@ func (n *nodeExpandModuleVariable) ModulePath() addrs.Module {
 
 // GraphNodeReferencer
 func (n *nodeExpandModuleVariable) References() []*addrs.Reference {
-	var refs []*addrs.Reference
-	if n.Config != nil {
-		// These references will ignore GraphNodeReferenceOutside and are used by the ProviderFunctionTransformer and lang.Scope.evalContext
-		// It's an odd pattern, but it works
-		for _, validation := range n.Config.Validations {
-			condFuncs, _ := lang.ProviderFunctionsInExpr(addrs.ParseRef, validation.Condition)
-			refs = append(refs, condFuncs...)
-			errFuncs, _ := lang.ProviderFunctionsInExpr(addrs.ParseRef, validation.ErrorMessage)
-			refs = append(refs, errFuncs...)
-		}
-	}
-
 	// If we have no value expression, we cannot depend on anything.
 	if n.Expr == nil {
-		return refs
+		return nil
 	}
 
 	// Variables in the root don't depend on anything, because their values
 	// are gathered prior to the graph walk and recorded in the context.
 	if len(n.Module) == 0 {
-		return refs
+		return nil
 	}
 
 	// Otherwise, we depend on anything referenced by our value expression.
@@ -124,9 +95,7 @@ func (n *nodeExpandModuleVariable) References() []*addrs.Reference {
 	// where our associated variable was declared, which is correct because
 	// our value expression is assigned within a "module" block in the parent
 	// module.
-	outerRefs, _ := lang.ReferencesInExpr(addrs.ParseRef, n.Expr)
+	refs, _ := lang.ReferencesInExpr(addrs.ParseRef, n.Expr)
-	refs = append(refs, outerRefs...)
-
 	return refs
 }
 
@@ -165,7 +134,7 @@ func (n *nodeModuleVariable) temporaryValue() bool {
 }
 
 func (n *nodeModuleVariable) Name() string {
-	return n.Addr.String()
+	return n.Addr.String() + "(input)"
 }
 
 // GraphNodeModuleInstance
@@ -184,17 +153,8 @@ func (n *nodeModuleVariable) ModulePath() addrs.Module {
 func (n *nodeModuleVariable) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) {
 	log.Printf("[TRACE] nodeModuleVariable: evaluating %s", n.Addr)
 
-	var val cty.Value
+	val, err := n.evalModuleVariable(ctx, op == walkValidate)
-	var err error
+	diags = diags.Append(err)
-
-	switch op {
-	case walkValidate:
-		val, err = n.evalModuleVariable(ctx, true)
-		diags = diags.Append(err)
-	default:
-		val, err = n.evalModuleVariable(ctx, false)
-		diags = diags.Append(err)
-	}
 	if diags.HasErrors() {
 		return diags
 	}
@@ -203,8 +163,7 @@ func (n *nodeModuleVariable) Execute(ctx EvalContext, op walkOperation) (diags t
 	// during expression evaluation.
 	_, call := n.Addr.Module.CallInstance()
 	ctx.SetModuleCallArgument(call, n.Addr.Variable, val)
-
+	return diags
-	return evalVariableValidations(n.Addr, n.Config, n.Expr, ctx)
 }
 
 // dag.GraphNodeDotter impl.

internal/tofu/node_root_variable.go

@@ -13,7 +13,6 @@ import (
 	"github.com/opentofu/opentofu/internal/addrs"
 	"github.com/opentofu/opentofu/internal/configs"
 	"github.com/opentofu/opentofu/internal/dag"
-	"github.com/opentofu/opentofu/internal/lang"
 	"github.com/opentofu/opentofu/internal/tfdiags"
 )
 
@@ -54,23 +53,6 @@ func (n *NodeRootVariable) ReferenceableAddrs() []addrs.Referenceable {
 	return []addrs.Referenceable{n.Addr}
 }
 
-// GraphNodeReferencer
-func (n *NodeRootVariable) References() []*addrs.Reference {
-	// This is identical to nodeModuleVariable.References
-	var refs []*addrs.Reference
-
-	if n.Config != nil {
-		for _, validation := range n.Config.Validations {
-			condFuncs, _ := lang.ProviderFunctionsInExpr(addrs.ParseRef, validation.Condition)
-			refs = append(refs, condFuncs...)
-			errFuncs, _ := lang.ProviderFunctionsInExpr(addrs.ParseRef, validation.ErrorMessage)
-			refs = append(refs, errFuncs...)
-		}
-	}
-
-	return refs
-}
-
 // GraphNodeExecutable
 func (n *NodeRootVariable) Execute(ctx EvalContext, op walkOperation) tfdiags.Diagnostics {
 	// Root module variables are special in that they are provided directly
@@ -104,12 +86,6 @@ func (n *NodeRootVariable) Execute(ctx EvalContext, op walkOperation) tfdiags.Di
 		}
 	}
 
-	if checkState := ctx.Checks(); checkState.ConfigHasChecks(n.Addr.InModule(addrs.RootModule)) {
-		ctx.Checks().ReportCheckableObjects(
-			n.Addr.InModule(addrs.RootModule),
-			addrs.MakeSet[addrs.Checkable](n.Addr.Absolute(addrs.RootModuleInstance)))
-	}
-
 	finalVal, moreDiags := prepareFinalInputVariableValue(
 		addr,
 		givenVal,
@@ -124,13 +100,6 @@ func (n *NodeRootVariable) Execute(ctx EvalContext, op walkOperation) tfdiags.Di
 
 	ctx.SetRootModuleArgument(addr.Variable, finalVal)
 
-	moreDiags = evalVariableValidations(
-		addrs.RootModuleInstance.InputVariable(n.Addr.Name),
-		n.Config,
-		nil, // not set for root module variables
-		ctx,
-	)
-	diags = diags.Append(moreDiags)
 	return diags
 }
 

internal/tofu/node_root_variable_test.go

@@ -128,6 +128,11 @@ func TestNodeRootVariableExecute(t *testing.T) {
 			},
 		}
 
+		ref := &nodeVariableReference{
+			Addr:   n.Addr,
+			Config: n.Config,
+		}
+
 		ctx.ChecksState = checks.NewState(&configs.Config{
 			Module: &configs.Module{
 				Variables: map[string]*configs.Variable{
@@ -141,6 +146,20 @@ func TestNodeRootVariableExecute(t *testing.T) {
 			t.Fatalf("unexpected error: %s", diags.Err())
 		}
 
+		g, err := ref.DynamicExpand(ctx)
+		if err != nil {
+			t.Fatalf("unexpected error: %s", err)
+		}
+
+		for _, v := range g.Vertices() {
+			if ev, ok := v.(GraphNodeExecutable); ok {
+				diags = ev.Execute(ctx, walkApply)
+				if diags.HasErrors() {
+					t.Fatalf("unexpected error: %s", diags.Err())
+				}
+			}
+		}
+
 		if !ctx.SetRootModuleArgumentCalled {
 			t.Fatalf("ctx.SetRootModuleArgument wasn't called")
 		}

internal/tofu/node_variable_reference.go

@@ -0,0 +1,154 @@
+// Copyright (c) The OpenTofu Authors
+// SPDX-License-Identifier: MPL-2.0
+// Copyright (c) 2023 HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package tofu
+
+import (
+	"fmt"
+	"log"
+
+	"github.com/hashicorp/hcl/v2"
+	"github.com/opentofu/opentofu/internal/addrs"
+	"github.com/opentofu/opentofu/internal/configs"
+	"github.com/opentofu/opentofu/internal/dag"
+	"github.com/opentofu/opentofu/internal/lang"
+	"github.com/opentofu/opentofu/internal/tfdiags"
+)
+
+// nodeVariableReference is the placeholder for an variable reference that has not yet had
+// its module path expanded.  It is a dependency on the evaluation (in a different scope) of
+// the nodes which provide the actual variable value to the evaluation context.  This split
+// allows the evaluation and validation of the variable in the two different scopes required.
+type nodeVariableReference struct {
+	Addr   addrs.InputVariable
+	Module addrs.Module
+	Config *configs.Variable
+	Expr   hcl.Expression // Used for diagnostics only
+}
+
+var (
+	_ GraphNodeDynamicExpandable = (*nodeVariableReference)(nil)
+	_ GraphNodeReferenceable     = (*nodeVariableReference)(nil)
+	_ GraphNodeReferencer        = (*nodeVariableReference)(nil)
+	_ graphNodeExpandsInstances  = (*nodeVariableReference)(nil)
+)
+
+// graphNodeExpandsInstances
+func (n *nodeVariableReference) expandsInstances() {}
+
+// GraphNodeDynamicExpandable
+func (n *nodeVariableReference) DynamicExpand(ctx EvalContext) (*Graph, error) {
+	var g Graph
+
+	// If this variable has preconditions, we need to report these checks now.
+	//
+	// We should only do this during planning as the apply phase starts with
+	// all the same checkable objects that were registered during the plan.
+	var checkableAddrs addrs.Set[addrs.Checkable]
+	if checkState := ctx.Checks(); checkState.ConfigHasChecks(n.Addr.InModule(n.Module)) {
+		checkableAddrs = addrs.MakeSet[addrs.Checkable]()
+	}
+
+	expander := ctx.InstanceExpander()
+	for _, module := range expander.ExpandModule(n.Module) {
+		addr := n.Addr.Absolute(module)
+		if checkableAddrs != nil {
+			checkableAddrs.Add(addr)
+		}
+
+		o := &nodeVariableReferenceInstance{
+			Addr:   addr,
+			Config: n.Config,
+			Expr:   n.Expr,
+		}
+		g.Add(o)
+	}
+	addRootNodeToGraph(&g)
+
+	if checkableAddrs != nil {
+		ctx.Checks().ReportCheckableObjects(n.Addr.InModule(n.Module), checkableAddrs)
+	}
+
+	return &g, nil
+}
+
+func (n *nodeVariableReference) Name() string {
+	addrStr := n.Addr.String()
+	if len(n.Module) != 0 {
+		addrStr = n.Module.String() + "." + addrStr
+	}
+	return fmt.Sprintf("%s (expand, reference)", addrStr)
+}
+
+// GraphNodeModulePath
+func (n *nodeVariableReference) ModulePath() addrs.Module {
+	return n.Module
+}
+
+// GraphNodeReferencer
+func (n *nodeVariableReference) References() []*addrs.Reference {
+	var refs []*addrs.Reference
+	if n.Config != nil {
+		for _, validation := range n.Config.Validations {
+			condFuncs, _ := lang.ProviderFunctionsInExpr(addrs.ParseRef, validation.Condition)
+			refs = append(refs, condFuncs...)
+			errFuncs, _ := lang.ProviderFunctionsInExpr(addrs.ParseRef, validation.ErrorMessage)
+			refs = append(refs, errFuncs...)
+		}
+	}
+	return refs
+}
+
+// GraphNodeReferenceable
+func (n *nodeVariableReference) ReferenceableAddrs() []addrs.Referenceable {
+	return []addrs.Referenceable{n.Addr}
+}
+
+// nodeVariableReferenceInstance represents a module variable reference during
+// the apply step.
+type nodeVariableReferenceInstance struct {
+	Addr   addrs.AbsInputVariableInstance
+	Config *configs.Variable // Config is the var in the config
+	Expr   hcl.Expression    // Used for diagnostics only
+}
+
+// Ensure that we are implementing all of the interfaces we think we are
+// implementing.
+var (
+	_ GraphNodeModuleInstance = (*nodeVariableReferenceInstance)(nil)
+	_ GraphNodeExecutable     = (*nodeVariableReferenceInstance)(nil)
+	_ dag.GraphNodeDotter     = (*nodeVariableReferenceInstance)(nil)
+)
+
+func (n *nodeVariableReferenceInstance) Name() string {
+	return n.Addr.String() + " (reference)"
+}
+
+// GraphNodeModuleInstance
+func (n *nodeVariableReferenceInstance) Path() addrs.ModuleInstance {
+	return n.Addr.Module
+}
+
+// GraphNodeModulePath
+func (n *nodeVariableReferenceInstance) ModulePath() addrs.Module {
+	return n.Addr.Module.Module()
+}
+
+// GraphNodeExecutable
+func (n *nodeVariableReferenceInstance) Execute(ctx EvalContext, _ walkOperation) tfdiags.Diagnostics {
+	log.Printf("[TRACE] nodeVariableReferenceInstance: evaluating %s", n.Addr)
+	return evalVariableValidations(n.Addr, n.Config, n.Expr, ctx)
+}
+
+// dag.GraphNodeDotter impl.
+func (n *nodeVariableReferenceInstance) DotNode(name string, _ *dag.DotOpts) *dag.DotNode {
+	return &dag.DotNode{
+		Name: name,
+		Attrs: map[string]string{
+			"label": n.Name(),
+			"shape": "note",
+		},
+	}
+}

internal/tofu/transform_module_variable.go

@@ -11,6 +11,7 @@ import (
 	"github.com/zclconf/go-cty/cty"
 
 	"github.com/opentofu/opentofu/internal/addrs"
+	"github.com/opentofu/opentofu/internal/dag"
 	"github.com/opentofu/opentofu/internal/tfdiags"
 
 	"github.com/hashicorp/hcl/v2"
@@ -102,9 +103,10 @@ func (t *ModuleVariableTransformer) transformSingle(g *Graph, parent, c *configs
 			expr = attr.Expr
 		}
 
-		// Add a plannable node, as the variable may expand
+		// Add a plannable input, as the variable may expand
 		// during module expansion
-		node := &nodeExpandModuleVariable{
+		// It is evaluated in the "parent" module
+		input := &nodeExpandModuleVariable{
 			Addr: addrs.InputVariable{
 				Name: v.Name,
 			},
@@ -112,7 +114,21 @@ func (t *ModuleVariableTransformer) transformSingle(g *Graph, parent, c *configs
 			Config: v,
 			Expr:   expr,
 		}
-		g.Add(node)
+		g.Add(input)
+
+		// It is evaluated in the "child" module
+		ref := &nodeVariableReference{
+			Addr: addrs.InputVariable{
+				Name: v.Name,
+			},
+			Module: c.Path,
+			Config: v,
+			Expr:   expr,
+		}
+		g.Add(ref)
+
+		// Input must be available before reference is valid
+		g.Connect(dag.BasicEdge(ref, input))
 	}
 
 	return nil

internal/tofu/transform_module_variable_test.go

@@ -63,10 +63,16 @@ func TestModuleVariableTransformer_nested(t *testing.T) {
 }
 
 const testTransformModuleVarBasicStr = `
-module.child.var.value (expand)
+module.child.var.value (expand, input)
+module.child.var.value (expand, reference)
+  module.child.var.value (expand, input)
 `
 
 const testTransformModuleVarNestedStr = `
-module.child.module.child.var.value (expand)
+module.child.module.child.var.value (expand, input)
-module.child.var.value (expand)
+module.child.module.child.var.value (expand, reference)
+  module.child.module.child.var.value (expand, input)
+module.child.var.value (expand, input)
+module.child.var.value (expand, reference)
+  module.child.var.value (expand, input)
 `

internal/tofu/transform_provider.go

@@ -310,8 +310,14 @@ func (t *ProviderFunctionTransformer) Transform(g *Graph) error {
 		if nr, ok := v.(GraphNodeReferencer); ok && t.Config != nil {
 			for _, ref := range nr.References() {
 				if pf, ok := ref.Subject.(addrs.ProviderFunction); ok {
+					refPath := nr.ModulePath()
+
+					if outside, isOutside := v.(GraphNodeReferenceOutside); isOutside {
+						_, refPath = outside.ReferenceOutside()
+					}
+
 					key := ProviderFunctionReference{
-						ModulePath:    nr.ModulePath().String(),
+						ModulePath:    refPath.String(),
 						ProviderName:  pf.ProviderName,
 						ProviderAlias: pf.ProviderAlias,
 					}
@@ -324,13 +330,13 @@ func (t *ProviderFunctionTransformer) Transform(g *Graph) error {
 					}
 
 					// Find the config that this node belongs to
-					mc := t.Config.Descendent(nr.ModulePath())
+					mc := t.Config.Descendent(refPath)
 					if mc == nil {
 						// I don't think this is possible
 						diags = diags.Append(&hcl.Diagnostic{
 							Severity: hcl.DiagError,
 							Summary:  "Unknown Descendent Module",
-							Detail:   nr.ModulePath().String(),
+							Detail:   refPath.String(),
 							Subject:  ref.SourceRange.ToHCL().Ptr(),
 						})
 						continue
@@ -351,7 +357,7 @@ func (t *ProviderFunctionTransformer) Transform(g *Graph) error {
 					// Build fully qualified provider address
 					absPc := addrs.AbsProviderConfig{
 						Provider: pr.Type,
-						Module:   nr.ModulePath(),
+						Module:   refPath,
 						Alias:    pf.ProviderAlias,
 					}
 

internal/tofu/transform_variable.go

@@ -8,6 +8,7 @@ package tofu
 import (
 	"github.com/opentofu/opentofu/internal/addrs"
 	"github.com/opentofu/opentofu/internal/configs"
+	"github.com/opentofu/opentofu/internal/dag"
 )
 
 // RootVariableTransformer is a GraphTransformer that adds all the root
@@ -42,6 +43,17 @@ func (t *RootVariableTransformer) Transform(g *Graph) error {
 			RawValue: t.RawValues[v.Name],
 		}
 		g.Add(node)
+
+		ref := &nodeVariableReference{
+			Addr: addrs.InputVariable{
+				Name: v.Name,
+			},
+			Config: v,
+		}
+		g.Add(ref)
+
+		// Input must be available before reference is valid
+		g.Connect(dag.BasicEdge(ref, node))
 	}
 
 	return nil