Merge pull request #25261 from hashicorp/jbardin/validate-depends-on

Validate depends_on in modules and outputs

terraform/context_validate_test.go

@@ -1632,3 +1632,83 @@ output "out" {
 		t.Fatal(diags.ErrWithWarnings())
 	}
 }
+
+func TestContext2Validate_invalidModuleDependsOn(t *testing.T) {
+	// validate module and output depends_on
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+module "mod1" {
+  source = "./mod"
+  depends_on = [resource_foo.bar.baz]
+}
+
+module "mod2" {
+  source = "./mod"
+  depends_on = [resource_foo.bar.baz]
+}
+`,
+		"mod/main.tf": `
+output "out" {
+  value = "foo"
+}
+`,
+	})
+
+	diags := testContext2(t, &ContextOpts{
+		Config: m,
+	}).Validate()
+	if !diags.HasErrors() {
+		t.Fatal("succeeded; want errors")
+	}
+
+	if len(diags) != 2 {
+		t.Fatalf("wanted 2 diagnostic errors, got %q", diags)
+	}
+
+	for _, d := range diags {
+		des := d.Description().Summary
+		if !strings.Contains(des, "Invalid depends_on reference") {
+			t.Fatalf(`expected "Invalid depends_on reference", got %q`, des)
+		}
+	}
+}
+
+func TestContext2Validate_invalidOutputDependsOn(t *testing.T) {
+	// validate module and output depends_on
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+module "mod1" {
+  source = "./mod"
+}
+
+output "out" {
+  value = "bar"
+  depends_on = [resource_foo.bar.baz]
+}
+`,
+		"mod/main.tf": `
+output "out" {
+  value = "bar"
+  depends_on = [resource_foo.bar.baz]
+}
+`,
+	})
+
+	diags := testContext2(t, &ContextOpts{
+		Config: m,
+	}).Validate()
+	if !diags.HasErrors() {
+		t.Fatal("succeeded; want errors")
+	}
+
+	if len(diags) != 2 {
+		t.Fatalf("wanted 2 diagnostic errors, got %q", diags)
+	}
+
+	for _, d := range diags {
+		des := d.Description().Summary
+		if !strings.Contains(des, "Invalid depends_on reference") {
+			t.Fatalf(`expected "Invalid depends_on reference", got %q`, des)
+		}
+	}
+}

terraform/eval_output.go

@@ -4,10 +4,10 @@ import (
 	"fmt"
 	"log"
 
-	"github.com/hashicorp/hcl/v2"
 	"github.com/zclconf/go-cty/cty"
 
 	"github.com/hashicorp/terraform/addrs"
+	"github.com/hashicorp/terraform/configs"
 	"github.com/hashicorp/terraform/plans"
 	"github.com/hashicorp/terraform/states"
 )
@@ -32,9 +32,8 @@ func (n *EvalDeleteOutput) Eval(ctx EvalContext) (interface{}, error) {
 // EvalWriteOutput is an EvalNode implementation that writes the output
 // for the given name to the current state.
 type EvalWriteOutput struct {
-	Addr      addrs.OutputValue
-	Sensitive bool
-	Expr      hcl.Expression
+	Addr   addrs.OutputValue
+	Config *configs.Output
 	// ContinueOnErr allows interpolation to fail during Input
 	ContinueOnErr bool
 }
@@ -45,9 +44,13 @@ func (n *EvalWriteOutput) Eval(ctx EvalContext) (interface{}, error) {
 
 	// This has to run before we have a state lock, since evaluation also
 	// reads the state
-	val, diags := ctx.EvaluateExpr(n.Expr, cty.DynamicPseudoType, nil)
+	val, diags := ctx.EvaluateExpr(n.Config.Expr, cty.DynamicPseudoType, nil)
 	// We'll handle errors below, after we have loaded the module.
 
+	// Outputs don't have a separate mode for validation, so validate
+	// depends_on expressions here too
+	diags = diags.Append(validateDependsOn(ctx, n.Config.DependsOn))
+
 	state := ctx.State()
 	if state == nil {
 		return nil, nil
@@ -80,7 +83,7 @@ func (n *EvalWriteOutput) setValue(addr addrs.AbsOutputValue, state *states.Sync
 		// changeset below, if we have one on this graph walk.
 		log.Printf("[TRACE] EvalWriteOutput: Saving value for %s in state", addr)
 		stateVal := cty.UnknownAsNull(val)
-		state.SetOutputValue(addr, stateVal, n.Sensitive)
+		state.SetOutputValue(addr, stateVal, n.Config.Sensitive)
 	} else {
 		log.Printf("[TRACE] EvalWriteOutput: Removing %s from state (it is now null)", addr)
 		state.RemoveOutputValue(addr)
@@ -100,7 +103,7 @@ func (n *EvalWriteOutput) setValue(addr addrs.AbsOutputValue, state *states.Sync
 		if !val.IsNull() {
 			change = &plans.OutputChange{
 				Addr:      addr,
-				Sensitive: n.Sensitive,
+				Sensitive: n.Config.Sensitive,
 				Change: plans.Change{
 					Action: plans.Create,
 					Before: cty.NullVal(cty.DynamicPseudoType),
@@ -110,7 +113,7 @@ func (n *EvalWriteOutput) setValue(addr addrs.AbsOutputValue, state *states.Sync
 		} else {
 			change = &plans.OutputChange{
 				Addr:      addr,
-				Sensitive: n.Sensitive,
+				Sensitive: n.Config.Sensitive,
 				Change: plans.Change{
 					// This is just a weird placeholder delete action since
 					// we don't have an actual prior value to indicate.

terraform/eval_output_test.go

@@ -3,6 +3,7 @@ package terraform
 import (
 	"testing"
 
+	"github.com/hashicorp/terraform/configs"
 	"github.com/hashicorp/terraform/states"
 
 	"github.com/hashicorp/terraform/addrs"
@@ -44,7 +45,8 @@ func TestEvalWriteMapOutput(t *testing.T) {
 
 	for _, tc := range cases {
 		evalNode := &EvalWriteOutput{
-			Addr: addrs.OutputValue{Name: tc.name},
+			Config: &configs.Output{},
+			Addr:   addrs.OutputValue{Name: tc.name},
 		}
 		ctx.EvaluateExprResult = tc.val
 		t.Run(tc.name, func(t *testing.T) {

terraform/eval_validate.go

@@ -401,29 +401,7 @@ func (n *EvalValidateResource) Eval(ctx EvalContext) (interface{}, error) {
 		diags = diags.Append(forEachDiags)
 	}
 
-	for _, traversal := range n.Config.DependsOn {
-		ref, refDiags := addrs.ParseRef(traversal)
-		diags = diags.Append(refDiags)
-		if !refDiags.HasErrors() && len(ref.Remaining) != 0 {
-			diags = diags.Append(&hcl.Diagnostic{
-				Severity: hcl.DiagError,
-				Summary:  "Invalid depends_on reference",
-				Detail:   "References in depends_on must be to a whole object (resource, etc), not to an attribute of an object.",
-				Subject:  ref.Remaining.SourceRange().Ptr(),
-			})
-		}
-
-		// The ref must also refer to something that exists. To test that,
-		// we'll just eval it and count on the fact that our evaluator will
-		// detect references to non-existent objects.
-		if !diags.HasErrors() {
-			scope := ctx.EvaluationScope(nil, EvalDataForNoInstanceKey)
-			if scope != nil { // sometimes nil in tests, due to incomplete mocks
-				_, refDiags = scope.EvalReference(ref, cty.DynamicPseudoType)
-				diags = diags.Append(refDiags)
-			}
-		}
-	}
+	diags = diags.Append(validateDependsOn(ctx, n.Config.DependsOn))
 
 	// Validate the provider_meta block for the provider this resource
 	// belongs to, if there is one.
@@ -622,3 +600,30 @@ func (n *EvalValidateResource) validateForEach(ctx EvalContext, expr hcl.Express
 
 	return diags
 }
+
+func validateDependsOn(ctx EvalContext, dependsOn []hcl.Traversal) (diags tfdiags.Diagnostics) {
+	for _, traversal := range dependsOn {
+		ref, refDiags := addrs.ParseRef(traversal)
+		diags = diags.Append(refDiags)
+		if !refDiags.HasErrors() && len(ref.Remaining) != 0 {
+			diags = diags.Append(&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Invalid depends_on reference",
+				Detail:   "References in depends_on must be to a whole object (resource, etc), not to an attribute of an object.",
+				Subject:  ref.Remaining.SourceRange().Ptr(),
+			})
+		}
+
+		// The ref must also refer to something that exists. To test that,
+		// we'll just eval it and count on the fact that our evaluator will
+		// detect references to non-existent objects.
+		if !diags.HasErrors() {
+			scope := ctx.EvaluationScope(nil, EvalDataForNoInstanceKey)
+			if scope != nil { // sometimes nil in tests, due to incomplete mocks
+				_, refDiags = scope.EvalReference(ref, cty.DynamicPseudoType)
+				diags = diags.Append(refDiags)
+			}
+		}
+	}
+	return diags
+}

terraform/node_module_expand.go

@@ -7,6 +7,7 @@ import (
 	"github.com/hashicorp/terraform/configs"
 	"github.com/hashicorp/terraform/dag"
 	"github.com/hashicorp/terraform/lang"
+	"github.com/hashicorp/terraform/tfdiags"
 )
 
 type ConcreteModuleNodeFunc func(n *nodeExpandModule) dag.Vertex
@@ -271,6 +272,7 @@ type evalValidateModule struct {
 
 func (n *evalValidateModule) Eval(ctx EvalContext) (interface{}, error) {
 	_, call := n.Addr.Call()
+	var diags tfdiags.Diagnostics
 	expander := ctx.InstanceExpander()
 
 	// Modules all evaluate to single instances during validation, only to
@@ -285,20 +287,23 @@ func (n *evalValidateModule) Eval(ctx EvalContext) (interface{}, error) {
 		// a full expansion, presuming these errors will be caught in later steps
 		switch {
 		case n.ModuleCall.Count != nil:
-			_, diags := evaluateCountExpressionValue(n.ModuleCall.Count, ctx)
-			if diags.HasErrors() {
-				return nil, diags.Err()
-			}
+			_, countDiags := evaluateCountExpressionValue(n.ModuleCall.Count, ctx)
+			diags = diags.Append(countDiags)
 
 		case n.ModuleCall.ForEach != nil:
-			_, diags := evaluateForEachExpressionValue(n.ModuleCall.ForEach, ctx)
-			if diags.HasErrors() {
-				return nil, diags.Err()
-			}
+			_, forEachDiags := evaluateForEachExpressionValue(n.ModuleCall.ForEach, ctx)
+			diags = diags.Append(forEachDiags)
 		}
 
+		diags = diags.Append(validateDependsOn(ctx, n.ModuleCall.DependsOn))
+
 		// now set our own mode to single
 		expander.SetModuleSingle(module, call)
 	}
+
+	if diags.HasErrors() {
+		return nil, diags.ErrWithWarnings()
+	}
+
 	return nil, nil
 }

terraform/node_output.go

@@ -228,9 +228,8 @@ func (n *NodeApplyableOutput) EvalTree() EvalNode {
 			&EvalOpFilter{
 				Ops: []walkOperation{walkEval, walkRefresh, walkPlan, walkApply, walkValidate, walkDestroy, walkPlanDestroy},
 				Node: &EvalWriteOutput{
-					Addr:      n.Addr.OutputValue,
-					Sensitive: n.Config.Sensitive,
-					Expr:      n.Config.Expr,
+					Addr:   n.Addr.OutputValue,
+					Config: n.Config,
 				},
 			},
 		},