Merge pull request #25138 from hashicorp/jbardin/module-data-depends-on

Data source within modules using depends_on

terraform/context_apply_test.go

@@ -11121,17 +11121,36 @@ func TestContext2Apply_moduleDependsOn(t *testing.T) {
 	m := testModule(t, "apply-module-depends-on")
 
 	p := testProvider("test")
-	p.ReadDataSourceResponse = providers.ReadDataSourceResponse{
-		State: cty.ObjectVal(map[string]cty.Value{
-			"id":  cty.StringVal("data"),
-			"foo": cty.NullVal(cty.String),
-		}),
-	}
-	p.DiffFn = testDiffFn
 
 	// each instance being applied should happen in sequential order
 	applied := int64(0)
 
+	p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse {
+		cfg := req.Config.AsValueMap()
+		foo := cfg["foo"].AsString()
+		ord := atomic.LoadInt64(&applied)
+
+		resp := providers.ReadDataSourceResponse{
+			State: cty.ObjectVal(map[string]cty.Value{
+				"id":  cty.StringVal("data"),
+				"foo": cfg["foo"],
+			}),
+		}
+
+		if foo == "a" && ord < 4 {
+			// due to data source "a"'s module depending on instance 4, this
+			// should not be less than 4
+			resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("data source a read too early"))
+		}
+		if foo == "b" && ord < 1 {
+			// due to data source "b"'s module depending on instance 1, this
+			// should not be less than 1
+			resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("data source b read too early"))
+		}
+		return resp
+	}
+	p.DiffFn = testDiffFn
+
 	p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) (resp providers.ApplyResourceChangeResponse) {
 		state := req.PlannedState.AsValueMap()
 		num, _ := state["num"].AsBigFloat().Float64()
@@ -11154,7 +11173,12 @@ func TestContext2Apply_moduleDependsOn(t *testing.T) {
 		},
 	})
 
-	_, diags := ctx.Plan()
+	_, diags := ctx.Refresh()
+	if diags.HasErrors() {
+		t.Fatal(diags.ErrWithWarnings())
+	}
+
+	_, diags = ctx.Plan()
 	if diags.HasErrors() {
 		t.Fatal(diags.ErrWithWarnings())
 	}
@@ -11165,6 +11189,10 @@ func TestContext2Apply_moduleDependsOn(t *testing.T) {
 	}
 
 	// run the plan again to ensure that data sources are not going to be re-read
+	_, diags = ctx.Refresh()
+	if diags.HasErrors() {
+		t.Fatal(diags.ErrWithWarnings())
+	}
 	plan, diags := ctx.Plan()
 	if diags.HasErrors() {
 		t.Fatal(diags.ErrWithWarnings())

terraform/eval_read_data.go

@@ -52,6 +52,15 @@ type evalReadData struct {
 	// - If planned action is plans.Update, it indicates that the data source
 	// was read, and the result needs to be stored in state during apply.
 	OutputChange **plans.ResourceInstanceChange
+
+	// dependsOn stores the list of transitive resource addresses that any
+	// configuration depends_on references may resolve to. This is used to
+	// determine if there are any changes that will force this data sources to
+	// be deferred to apply.
+	dependsOn []addrs.ConfigResource
+	// forceDependsOn indicates that resources may be missing from dependsOn,
+	// but the parent module may have depends_on configured.
+	forceDependsOn bool
 }
 
 // readDataSource handles everything needed to call ReadDataSource on the provider.
@@ -235,7 +244,7 @@ func (n *evalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) {
 	// refresh, that we can read this resource. If there are dependency updates
 	// in the config, they we be discovered in plan and the data source will be
 	// read again.
-	if !configKnown || (priorVal.IsNull() && len(n.Config.DependsOn) > 0) {
+	if !configKnown || (priorVal.IsNull() && n.forcePlanRead()) {
 		if configKnown {
 			log.Printf("[TRACE] evalReadDataRefresh: %s configuration is fully known, but we're forcing a read plan to be created", absAddr)
 		} else {
@@ -291,3 +300,10 @@ func (n *evalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) {
 
 	return nil, diags.ErrWithWarnings()
 }
+
+// forcePlanRead determines if we need to override the usual behavior of
+// immediately reading from the data source where possible, instead forcing us
+// to generate a plan.
+func (n *evalReadDataRefresh) forcePlanRead() bool {
+	return len(n.Config.DependsOn) > 0 || len(n.dependsOn) > 0 || n.forceDependsOn
+}

terraform/eval_read_data_plan.go

@@ -6,7 +6,6 @@ import (
 
 	"github.com/zclconf/go-cty/cty"
 
-	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/plans"
 	"github.com/hashicorp/terraform/plans/objchange"
 	"github.com/hashicorp/terraform/states"
@@ -18,12 +17,6 @@ import (
 // or generating a plan to do so.
 type evalReadDataPlan struct {
 	evalReadData
-
-	// dependsOn stores the list of transitive resource addresses that any
-	// configuration depends_on references may resolve to. This is used to
-	// determine if there are any changes that will force this data sources to
-	// be deferred to apply.
-	dependsOn []addrs.ConfigResource
 }
 
 func (n *evalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) {

terraform/graph_builder_refresh.go

@@ -171,6 +171,7 @@ func (b *RefreshGraphBuilder) Steps() []GraphTransformer {
 		// have to connect again later for providers and so on.
 		&ReferenceTransformer{},
 		&AttachDependenciesTransformer{},
+		&attachDataResourceDependenciesTransformer{},
 
 		// Target
 		&TargetsTransformer{

terraform/node_data_refresh.go

@@ -121,6 +121,8 @@ func (n *NodeRefreshableDataResource) DynamicExpand(ctx EvalContext) (*Graph, er
 		a.Config = n.Config
 		a.ResolvedProvider = n.ResolvedProvider
 		a.ProviderMetas = n.ProviderMetas
+		a.dependsOn = n.dependsOn
+		a.forceDependsOn = n.forceDependsOn
 
 		return &NodeRefreshableDataResourceInstance{
 			NodeAbstractResourceInstance: a,
@@ -227,6 +229,8 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode {
 					ProviderSchema: &providerSchema,
 					OutputChange:   &change,
 					State:          &state,
+					dependsOn:      n.dependsOn,
+					forceDependsOn: n.forceDependsOn,
 				},
 			},
 

terraform/node_module_expand.go

@@ -47,18 +47,7 @@ func (n *nodeExpandModule) References() []*addrs.Reference {
 		return nil
 	}
 
-	for _, traversal := range n.ModuleCall.DependsOn {
-		ref, diags := addrs.ParseRef(traversal)
-		if diags.HasErrors() {
-			// We ignore this here, because this isn't a suitable place to return
-			// errors. This situation should be caught and rejected during
-			// validation.
-			log.Printf("[ERROR] Can't parse %#v from depends_on as reference: %s", traversal, diags.Err())
-			continue
-		}
-
-		refs = append(refs, ref)
-	}
+	refs = append(refs, n.DependsOn()...)
 
 	// Expansion only uses the count and for_each expressions, so this
 	// particular graph node only refers to those.
@@ -82,6 +71,28 @@ func (n *nodeExpandModule) References() []*addrs.Reference {
 	return appendResourceDestroyReferences(refs)
 }
 
+func (n *nodeExpandModule) DependsOn() []*addrs.Reference {
+	if n.ModuleCall == nil {
+		return nil
+	}
+
+	var refs []*addrs.Reference
+	for _, traversal := range n.ModuleCall.DependsOn {
+		ref, diags := addrs.ParseRef(traversal)
+		if diags.HasErrors() {
+			// We ignore this here, because this isn't a suitable place to return
+			// errors. This situation should be caught and rejected during
+			// validation.
+			log.Printf("[ERROR] Can't parse %#v from depends_on as reference: %s", traversal, diags.Err())
+			continue
+		}
+
+		refs = append(refs, ref)
+	}
+
+	return refs
+}
+
 // GraphNodeReferenceOutside
 func (n *nodeExpandModule) ReferenceOutside() (selfPath, referencePath addrs.Module) {
 	return n.Addr, n.Addr.Parent()

terraform/node_resource_abstract.go

@@ -61,8 +61,9 @@ type NodeAbstractResource struct {
 	// Set from GraphNodeTargetable
 	Targets []addrs.Targetable
 
-	// Set from GraphNodeDependsOn
-	dependsOn []addrs.ConfigResource
+	// Set from AttachResourceDependencies
+	dependsOn      []addrs.ConfigResource
+	forceDependsOn bool
 
 	// The address of the provider this resource will use
 	ResolvedProvider addrs.AbsProviderConfig
@@ -402,8 +403,9 @@ func (n *NodeAbstractResource) SetTargets(targets []addrs.Targetable) {
 }
 
 // graphNodeAttachResourceDependencies
-func (n *NodeAbstractResource) AttachResourceDependencies(deps []addrs.ConfigResource) {
+func (n *NodeAbstractResource) AttachResourceDependencies(deps []addrs.ConfigResource, force bool) {
 	n.dependsOn = deps
+	n.forceDependsOn = force
 }
 
 // GraphNodeAttachResourceState

terraform/node_resource_plan_instance.go

@@ -83,8 +83,8 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou
 					ProviderSchema: &providerSchema,
 					OutputChange:   &change,
 					State:          &state,
+					dependsOn:      n.dependsOn,
 				},
-				dependsOn: n.dependsOn,
 			},
 
 			&EvalWriteState{

terraform/testdata/apply-module-depends-on/moda/main.tf

@@ -3,6 +3,7 @@ resource "test_instance" "a" {
 }
 
 data "test_data_source" "a" {
+  foo = "a"
 }
 
 output "out" {

terraform/testdata/apply-module-depends-on/modb/main.tf

@@ -3,6 +3,7 @@ resource "test_instance" "b" {
 }
 
 data "test_data_source" "b" {
+  foo = "b"
 }
 
 output "out" {

terraform/transform_reference.go

@@ -43,6 +43,12 @@ type GraphNodeAttachDependencies interface {
 	AttachDependencies([]addrs.ConfigResource)
 }
 
+// graphNodeDependsOn is implemented by resources that need to expose any
+// references set via DependsOn in their configuration.
+type graphNodeDependsOn interface {
+	DependsOn() []*addrs.Reference
+}
+
 // graphNodeAttachResourceDependencies records all resources that are transitively
 // referenced through depends_on in the configuration. This is used by data
 // resources to determine if they can be read during the plan, or if they need
@@ -55,8 +61,16 @@ type GraphNodeAttachDependencies interface {
 // "perpetual diff"
 type graphNodeAttachResourceDependencies interface {
 	GraphNodeConfigResource
-	AttachResourceDependencies([]addrs.ConfigResource)
-	DependsOn() []*addrs.Reference
+	graphNodeDependsOn
+
+	// AttachResourceDependencies stored the discovered dependencies in the
+	// resource node for evaluation later.
+	//
+	// The force parameter indicates that even if there are no dependencies,
+	// force the data source to act as though there are for refresh purposes.
+	// This is needed because yet-to-be-created resources won't be in the
+	// initial refresh graph, but may still be referenced through depends_on.
+	AttachResourceDependencies(deps []addrs.ConfigResource, force bool)
 }
 
 // GraphNodeReferenceOutside is an interface that can optionally be implemented.
@@ -122,7 +136,7 @@ func (t *ReferenceTransformer) Transform(g *Graph) error {
 
 type depMap map[string]addrs.ConfigResource
 
-// addDep adds the vertex if it represents a resource in the
+// add stores the vertex if it represents a resource in the
 // graph.
 func (m depMap) add(v dag.Vertex) {
 	// we're only concerned with resources which may have changes that
@@ -155,34 +169,28 @@ func (t attachDataResourceDependenciesTransformer) Transform(g *Graph) error {
 		if !ok {
 			continue
 		}
-		selfAddr := depender.ResourceAddr()
 
 		// Only data need to attach depends_on, so they can determine if they
 		// are eligible to be read during plan.
-		if selfAddr.Resource.Mode != addrs.DataResourceMode {
+		if depender.ResourceAddr().Resource.Mode != addrs.DataResourceMode {
 			continue
 		}
 
-		// depMap will only add resource references and dedupe
-		m := make(depMap)
-
-		for _, dep := range refMap.DependsOn(v) {
+		// depMap will only add resource references then dedupe
+		deps := make(depMap)
+		dependsOnDeps, fromModule := refMap.dependsOn(g, depender)
+		for _, dep := range dependsOnDeps {
 			// any the dependency
-			m.add(dep)
-			// and check any ancestors
-			ans, _ := g.Ancestors(dep)
-			for _, v := range ans {
-				m.add(v)
-			}
+			deps.add(dep)
 		}
 
-		deps := make([]addrs.ConfigResource, 0, len(m))
-		for _, d := range m {
-			deps = append(deps, d)
+		res := make([]addrs.ConfigResource, 0, len(deps))
+		for _, d := range deps {
+			res = append(res, d)
 		}
 
-		log.Printf("[TRACE] AttachDependsOnTransformer: %s depends on %s", depender.ResourceAddr(), deps)
-		depender.AttachResourceDependencies(deps)
+		log.Printf("[TRACE] attachDataDependenciesTransformer: %s depends on %s", depender.ResourceAddr(), res)
+		depender.AttachResourceDependencies(res, fromModule)
 	}
 
 	return nil
@@ -254,6 +262,16 @@ func (t AttachDependenciesTransformer) Transform(g *Graph) error {
 	return nil
 }
 
+func isDependableResource(v dag.Vertex) bool {
+	switch v.(type) {
+	case GraphNodeResourceInstance:
+		return true
+	case GraphNodeConfigResource:
+		return true
+	}
+	return false
+}
+
 // ReferenceMap is a structure that can be used to efficiently check
 // for references on a graph, mapping internal reference keys (as produced by
 // the mapKey method) to one or more vertices that are identified by each key.
@@ -304,35 +322,87 @@ func (m ReferenceMap) References(v dag.Vertex) []dag.Vertex {
 	return matches
 }
 
-// DependsOn returns the set of vertices that the given vertex refers to from
-// the configured depends_on.
-func (m ReferenceMap) DependsOn(v dag.Vertex) []dag.Vertex {
-	depender, ok := v.(graphNodeAttachResourceDependencies)
-	if !ok {
-		return nil
+// dependsOn returns the set of vertices that the given vertex refers to from
+// the configured depends_on. The bool return value indicates if depends_on was
+// found in a parent module configuration.
+func (m ReferenceMap) dependsOn(g *Graph, depender graphNodeDependsOn) ([]dag.Vertex, bool) {
+	var res []dag.Vertex
+	fromModule := false
+
+	refs := depender.DependsOn()
+
+	// This is where we record that a module has depends_on configured.
+	if _, ok := depender.(*nodeExpandModule); ok && len(refs) > 0 {
+		fromModule = true
 	}
 
-	var matches []dag.Vertex
-
-	for _, ref := range depender.DependsOn() {
+	for _, ref := range refs {
 		subject := ref.Subject
 
-		key := m.referenceMapKey(v, subject)
+		key := m.referenceMapKey(depender, subject)
 		vertices, ok := m[key]
 		if !ok {
-			log.Printf("[WARN] DependOn: reference not found: %q", subject)
+			// the ReferenceMap generates all possible keys, so any warning
+			// here is probably not useful for this implementation.
 			continue
 		}
 		for _, rv := range vertices {
 			// don't include self-references
-			if rv == v {
+			if rv == depender {
 				continue
 			}
-			matches = append(matches, rv)
+			res = append(res, rv)
+
+			// and check any ancestors for transitive dependencies
+			ans, _ := g.Ancestors(rv)
+			for _, v := range ans {
+				if isDependableResource(v) {
+					res = append(res, v)
+				}
+			}
 		}
 	}
 
-	return matches
+	parentDeps, fromParentModule := m.parentModuleDependsOn(g, depender)
+	res = append(res, parentDeps...)
+
+	return res, fromModule || fromParentModule
+}
+
+// parentModuleDependsOn returns the set of vertices that a data sources parent
+// module references through the module call's depends_on. The bool return
+// value indicates if depends_on was found in a parent module configuration.
+func (n ReferenceMap) parentModuleDependsOn(g *Graph, depender graphNodeDependsOn) ([]dag.Vertex, bool) {
+	var res []dag.Vertex
+	fromModule := false
+
+	// Look for containing modules with DependsOn.
+	// This should be connected directly to the module node, so we only need to
+	// look one step away.
+	for _, v := range g.DownEdges(depender) {
+		// we're only concerned with module expansion nodes here.
+		mod, ok := v.(*nodeExpandModule)
+		if !ok {
+			continue
+		}
+
+		deps, fromParentModule := n.dependsOn(g, mod)
+		for _, dep := range deps {
+			// add the dependency
+			res = append(res, dep)
+
+			// and check any transitive resource dependencies for more resources
+			ans, _ := g.Ancestors(dep)
+			for _, v := range ans {
+				if isDependableResource(v) {
+					res = append(res, v)
+				}
+			}
+		}
+		fromModule = fromModule || fromParentModule
+	}
+
+	return res, fromModule
 }
 
 func (m *ReferenceMap) mapKey(path addrs.Module, addr addrs.Referenceable) string {