core: Skip edges between resource instances in different module instances

Our reference transformer analyses and our destroy transformer analyses
are built around static (not-yet-expanded) addresses so that they can
correctly handle mixtures of expanded and not-yet-expanded objects in the
same graph.

However, this characteristic also makes them unnecessarily conservative
in their handling of references between resources within different
instances of the same module: we know they can never interact with each
other in practice because the dependencies for all instances of a module
are the same and so one instance cannot possibly depend on another.

As a compromise then, here we introduce a new helper function that can
recognize when a proposed edge is between two resource instances that
belong to different instances of the same module, and thus allow us to
skip actually creating those edges even though our imprecise analyses
believe them to be needed.

As well as significantly reducing the number of edges in situations where
multi-instance resources appear inside multi-instance modules, this also
fixes some potential cycles in situations where a single plan includes
both destroying an instance of a module and creating a new instance of the
same module: the dependencies between the objects in the instance being
destroyed and the objects in the instance being created can, if allowed
to connect, cause Terraform to believe that the create and the destroy
both depend on one another even though there is no need for that to be
true in practice.

This involves a very specialized helper function to encode the situation
where this exception applies. This function has an ugly name to reflect
how specialized it is; it's not intended to be of any use outside of these
three situations in particular.

terraform/node_resource_abstract.go

@@ -445,3 +445,37 @@ func (n *NodeAbstractResource) DotNode(name string, opts *dag.DotOpts) *dag.DotN
 		},
 	}
 }
+
+// graphNodesAreResourceInstancesInDifferentInstancesOfSameModule is an
+// annoyingly-task-specific helper function that returns true if and only if
+// the following conditions hold:
+// - Both of the given vertices represent specific resource instances, as
+//   opposed to unexpanded resources or any other non-resource-related object.
+// - The module instance addresses for both of the resource instances belong
+//   to the same static module.
+// - The module instance addresses for both of the resource instances are
+//   not equal, indicating that they belong to different instances of the
+//   same module.
+//
+// This result can be used as a way to compensate for the effects of
+// conservative analyses passes in our graph builders which make their
+// decisions based only on unexpanded addresses, often so that they can behave
+// correctly for interactions between expanded and not-yet-expanded objects.
+//
+// Callers of this helper function will typically skip adding an edge between
+// the two given nodes if this function returns true.
+func graphNodesAreResourceInstancesInDifferentInstancesOfSameModule(a, b dag.Vertex) bool {
+	aRI, aOK := a.(GraphNodeResourceInstance)
+	bRI, bOK := b.(GraphNodeResourceInstance)
+	if !(aOK && bOK) {
+		return false
+	}
+	aModInst := aRI.ResourceInstanceAddr().Module
+	bModInst := bRI.ResourceInstanceAddr().Module
+	aMod := aModInst.Module()
+	bMod := bModInst.Module()
+	if !aMod.Equal(bMod) {
+		return false
+	}
+	return !aModInst.Equal(bModInst)
+}

terraform/testdata/transform-destroy-edge-module-only/main.tf

@@ -1,3 +1,4 @@
 module "child" {
     source = "./child"
+    count  = 2
 }

terraform/transform_destroy_edge.go

@@ -105,9 +105,12 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 
 			for _, resAddr := range ri.StateDependencies() {
 				for _, desDep := range destroyersByResource[resAddr.String()] {
-					log.Printf("[TRACE] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(desDep), dag.VertexName(des))
-					g.Connect(dag.BasicEdge(desDep, des))
-
+					if !graphNodesAreResourceInstancesInDifferentInstancesOfSameModule(desDep, des) {
+						log.Printf("[TRACE] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(desDep), dag.VertexName(des))
+						g.Connect(dag.BasicEdge(desDep, des))
+					} else {
+						log.Printf("[TRACE] DestroyEdgeTransformer: skipping %s => %s inter-module-instance dependency\n", dag.VertexName(desDep), dag.VertexName(des))
+					}
 				}
 			}
 		}
@@ -122,9 +125,12 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 
 		for _, resAddr := range ri.StateDependencies() {
 			for _, desDep := range destroyersByResource[resAddr.String()] {
-				log.Printf("[TRACE] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(c), dag.VertexName(desDep))
-				g.Connect(dag.BasicEdge(c, desDep))
-
+				if !graphNodesAreResourceInstancesInDifferentInstancesOfSameModule(c, desDep) {
+					log.Printf("[TRACE] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(c), dag.VertexName(desDep))
+					g.Connect(dag.BasicEdge(c, desDep))
+				} else {
+					log.Printf("[TRACE] DestroyEdgeTransformer: skipping %s => %s inter-module-instance dependency\n", dag.VertexName(c), dag.VertexName(desDep))
+				}
 			}
 		}
 	}

terraform/transform_destroy_edge_test.go

@@ -1,6 +1,7 @@
 package terraform
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 
@@ -172,41 +173,46 @@ func TestDestroyEdgeTransformer_module(t *testing.T) {
 
 func TestDestroyEdgeTransformer_moduleOnly(t *testing.T) {
 	g := Graph{Path: addrs.RootModuleInstance}
-	g.Add(testDestroyNode("module.child[0].test_object.a"))
-	g.Add(testDestroyNode("module.child[0].test_object.b"))
-	g.Add(testDestroyNode("module.child[0].test_object.c"))
 
 	state := states.NewState()
-	child := state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.IntKey(0)))
-	child.SetResourceInstanceCurrent(
-		mustResourceInstanceAddr("test_object.a").Resource,
-		&states.ResourceInstanceObjectSrc{
-			Status:    states.ObjectReady,
-			AttrsJSON: []byte(`{"id":"a"}`),
-		},
-		mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
-	)
-	child.SetResourceInstanceCurrent(
-		mustResourceInstanceAddr("test_object.b").Resource,
-		&states.ResourceInstanceObjectSrc{
-			Status:       states.ObjectReady,
-			AttrsJSON:    []byte(`{"id":"b","test_string":"x"}`),
-			Dependencies: []addrs.ConfigResource{mustResourceAddr("module.child.test_object.a")},
-		},
-		mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
-	)
-	child.SetResourceInstanceCurrent(
-		mustResourceInstanceAddr("test_object.c").Resource,
-		&states.ResourceInstanceObjectSrc{
-			Status:    states.ObjectReady,
-			AttrsJSON: []byte(`{"id":"c","test_string":"x"}`),
-			Dependencies: []addrs.ConfigResource{
-				mustResourceAddr("module.child.test_object.a"),
-				mustResourceAddr("module.child.test_object.b"),
+	for moduleIdx := 0; moduleIdx < 2; moduleIdx++ {
+		g.Add(testDestroyNode(fmt.Sprintf("module.child[%d].test_object.a", moduleIdx)))
+		g.Add(testDestroyNode(fmt.Sprintf("module.child[%d].test_object.b", moduleIdx)))
+		g.Add(testDestroyNode(fmt.Sprintf("module.child[%d].test_object.c", moduleIdx)))
+
+		child := state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.IntKey(moduleIdx)))
+		child.SetResourceInstanceCurrent(
+			mustResourceInstanceAddr("test_object.a").Resource,
+			&states.ResourceInstanceObjectSrc{
+				Status:    states.ObjectReady,
+				AttrsJSON: []byte(`{"id":"a"}`),
 			},
-		},
-		mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
-	)
+			mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
+		)
+		child.SetResourceInstanceCurrent(
+			mustResourceInstanceAddr("test_object.b").Resource,
+			&states.ResourceInstanceObjectSrc{
+				Status:    states.ObjectReady,
+				AttrsJSON: []byte(`{"id":"b","test_string":"x"}`),
+				Dependencies: []addrs.ConfigResource{
+					mustResourceAddr("module.child.test_object.a"),
+				},
+			},
+			mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
+		)
+		child.SetResourceInstanceCurrent(
+			mustResourceInstanceAddr("test_object.c").Resource,
+			&states.ResourceInstanceObjectSrc{
+				Status:    states.ObjectReady,
+				AttrsJSON: []byte(`{"id":"c","test_string":"x"}`),
+				Dependencies: []addrs.ConfigResource{
+					mustResourceAddr("module.child.test_object.a"),
+					mustResourceAddr("module.child.test_object.b"),
+				},
+			},
+			mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
+		)
+	}
 
 	if err := (&AttachStateTransformer{State: state}).Transform(&g); err != nil {
 		t.Fatal(err)
@@ -220,6 +226,20 @@ func TestDestroyEdgeTransformer_moduleOnly(t *testing.T) {
 		t.Fatalf("err: %s", err)
 	}
 
+	// The analyses done in the destroy edge transformer are between
+	// not-yet-expanded objects, which is conservative and so it will generate
+	// edges that aren't strictly necessary. As a special case we filter out
+	// any edges that are between resources instances that are in different
+	// instances of the same module, because those edges are never needed
+	// (one instance of a module cannot depend on another instance of the
+	// same module) and including them can, in complex cases, cause cycles due
+	// to unnecessary interactions between destroyed and created module
+	// instances in the same plan.
+	//
+	// Therefore below we expect to see the dependencies within each instance
+	// of module.child reflected, but we should not see any dependencies
+	// _between_ instances of module.child.
+
 	actual := strings.TrimSpace(g.String())
 	expected := strings.TrimSpace(`
 module.child[0].test_object.a (destroy)
@@ -228,6 +248,12 @@ module.child[0].test_object.a (destroy)
 module.child[0].test_object.b (destroy)
   module.child[0].test_object.c (destroy)
 module.child[0].test_object.c (destroy)
+module.child[1].test_object.a (destroy)
+  module.child[1].test_object.b (destroy)
+  module.child[1].test_object.c (destroy)
+module.child[1].test_object.b (destroy)
+  module.child[1].test_object.c (destroy)
+module.child[1].test_object.c (destroy)
 `)
 	if actual != expected {
 		t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected)

terraform/transform_reference.go

@@ -123,7 +123,11 @@ func (t *ReferenceTransformer) Transform(g *Graph) error {
 			dag.VertexName(v), parentsDbg)
 
 		for _, parent := range parents {
-			g.Connect(dag.BasicEdge(v, parent))
+			if !graphNodesAreResourceInstancesInDifferentInstancesOfSameModule(v, parent) {
+				g.Connect(dag.BasicEdge(v, parent))
+			} else {
+				log.Printf("[TRACE] ReferenceTransformer: skipping %s => %s inter-module-instance dependency", v, parent)
+			}
 		}
 
 		if len(parents) > 0 {

terraform/transform_reference_test.go

@@ -89,6 +89,89 @@ func TestReferenceTransformer_path(t *testing.T) {
 	}
 }
 
+func TestReferenceTransformer_resourceInstances(t *testing.T) {
+	// Our reference analyses are all done based on unexpanded addresses
+	// so that we can use this transformer both in the plan graph (where things
+	// are not expanded yet) and the apply graph (where resource instances are
+	// pre-expanded but nothing else is.)
+	// However, that would make the result too conservative about instances
+	// of the same resource in different instances of the same module, so we
+	// make an exception for that situation in particular, keeping references
+	// between resource instances segregated by their containing module
+	// instance.
+	g := Graph{Path: addrs.RootModuleInstance}
+	moduleInsts := []addrs.ModuleInstance{
+		{
+			{
+				Name: "foo", InstanceKey: addrs.IntKey(0),
+			},
+		},
+		{
+			{
+				Name: "foo", InstanceKey: addrs.IntKey(1),
+			},
+		},
+	}
+	resourceAs := make([]addrs.AbsResourceInstance, len(moduleInsts))
+	for i, moduleInst := range moduleInsts {
+		resourceAs[i] = addrs.Resource{
+			Mode: addrs.ManagedResourceMode,
+			Type: "thing",
+			Name: "a",
+		}.Instance(addrs.NoKey).Absolute(moduleInst)
+	}
+	resourceBs := make([]addrs.AbsResourceInstance, len(moduleInsts))
+	for i, moduleInst := range moduleInsts {
+		resourceBs[i] = addrs.Resource{
+			Mode: addrs.ManagedResourceMode,
+			Type: "thing",
+			Name: "b",
+		}.Instance(addrs.NoKey).Absolute(moduleInst)
+	}
+	g.Add(&graphNodeFakeResourceInstance{
+		Addr: resourceAs[0],
+	})
+	g.Add(&graphNodeFakeResourceInstance{
+		Addr: resourceBs[0],
+		Refs: []*addrs.Reference{
+			{
+				Subject: resourceAs[0].Resource,
+			},
+		},
+	})
+	g.Add(&graphNodeFakeResourceInstance{
+		Addr: resourceAs[1],
+	})
+	g.Add(&graphNodeFakeResourceInstance{
+		Addr: resourceBs[1],
+		Refs: []*addrs.Reference{
+			{
+				Subject: resourceAs[1].Resource,
+			},
+		},
+	})
+
+	tf := &ReferenceTransformer{}
+	if err := tf.Transform(&g); err != nil {
+		t.Fatalf("unexpected error: %s", err)
+	}
+
+	// Resource B should be connected to resource A in each module instance,
+	// but there should be no connections between the two module instances.
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(`
+module.foo[0].thing.a
+module.foo[0].thing.b
+  module.foo[0].thing.a
+module.foo[1].thing.a
+module.foo[1].thing.b
+  module.foo[1].thing.a
+`)
+	if actual != expected {
+		t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected)
+	}
+}
+
 func TestReferenceMapReferences(t *testing.T) {
 	cases := map[string]struct {
 		Nodes  []dag.Vertex
@@ -187,6 +270,39 @@ func (n *graphNodeRefChildTest) ModulePath() addrs.Module {
 	return normalizeModulePath(n.PathValue).Module()
 }
 
+type graphNodeFakeResourceInstance struct {
+	Addr addrs.AbsResourceInstance
+	Refs []*addrs.Reference
+}
+
+var _ GraphNodeResourceInstance = (*graphNodeFakeResourceInstance)(nil)
+var _ GraphNodeReferenceable = (*graphNodeFakeResourceInstance)(nil)
+var _ GraphNodeReferencer = (*graphNodeFakeResourceInstance)(nil)
+
+func (n *graphNodeFakeResourceInstance) ResourceInstanceAddr() addrs.AbsResourceInstance {
+	return n.Addr
+}
+
+func (n *graphNodeFakeResourceInstance) ModulePath() addrs.Module {
+	return n.Addr.Module.Module()
+}
+
+func (n *graphNodeFakeResourceInstance) ReferenceableAddrs() []addrs.Referenceable {
+	return []addrs.Referenceable{n.Addr.Resource}
+}
+
+func (n *graphNodeFakeResourceInstance) References() []*addrs.Reference {
+	return n.Refs
+}
+
+func (n *graphNodeFakeResourceInstance) StateDependencies() []addrs.ConfigResource {
+	return nil
+}
+
+func (n *graphNodeFakeResourceInstance) String() string {
+	return n.Addr.String()
+}
+
 const testTransformRefBasicStr = `
 A
 B