diff --git a/terraform/node_resource_abstract.go b/terraform/node_resource_abstract.go index 051c7f4440..35c6ce27ff 100644 --- a/terraform/node_resource_abstract.go +++ b/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) +} diff --git a/terraform/testdata/transform-destroy-edge-module-only/main.tf b/terraform/testdata/transform-destroy-edge-module-only/main.tf index 0f6991c536..919351443d 100644 --- a/terraform/testdata/transform-destroy-edge-module-only/main.tf +++ b/terraform/testdata/transform-destroy-edge-module-only/main.tf @@ -1,3 +1,4 @@ module "child" { source = "./child" + count = 2 } diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index 1deba04277..4a3245eca2 100644 --- a/terraform/transform_destroy_edge.go +++ b/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)) + } } } } diff --git a/terraform/transform_destroy_edge_test.go b/terraform/transform_destroy_edge_test.go index c47632974a..0fd1d6d339 100644 --- a/terraform/transform_destroy_edge_test.go +++ b/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) diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index 6a0ee98faa..ee6069651b 100644 --- a/terraform/transform_reference.go +++ b/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 { diff --git a/terraform/transform_reference_test.go b/terraform/transform_reference_test.go index 073d02d347..069645e9eb 100644 --- a/terraform/transform_reference_test.go +++ b/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