From 46dbb3dde55659c3bd6481b0d7644d672f236dc9 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 30 Oct 2019 15:59:34 -0400 Subject: [PATCH] use Dependencies to connect creator and destroyer The DestroyEdgeTransformer cannot determine ordering from the graph when the destroyers are from orphaned resources, because there are no references to resolve. The new stored Dependencies provides what we need to connect the instances in this case. We also add the StateDependencies method directly in the GraphNodeResourceInstance interface, since all instances already implement this, and we don't need another optional interface to check. The old code in DestroyEdgeTransformer may no longer be needed in the long run, but that can be determined separately, since too many of the tests start with an incomplete state and rely on the Dependencies being determined from the configuration alone. --- command/command_test.go | 6 ++- terraform/context_apply_test.go | 5 +- terraform/graph_builder_apply_test.go | 6 ++- terraform/node_resource_abstract.go | 4 ++ terraform/transform_destroy_edge.go | 70 ++++++++++++++++----------- terraform/transform_reference.go | 6 ++- 6 files changed, 63 insertions(+), 34 deletions(-) diff --git a/command/command_test.go b/command/command_test.go index 58b817c7a..f87ad7554 100644 --- a/command/command_test.go +++ b/command/command_test.go @@ -260,8 +260,10 @@ func testState() *states.State { // The weird whitespace here is reflective of how this would // get written out in a real state file, due to the indentation // of all of the containing wrapping objects and arrays. - AttrsJSON: []byte("{\n \"id\": \"bar\"\n }"), - Status: states.ObjectReady, + AttrsJSON: []byte("{\n \"id\": \"bar\"\n }"), + Status: states.ObjectReady, + Dependencies: []addrs.AbsResource{}, + DependsOn: []addrs.Referenceable{}, }, addrs.ProviderConfig{ Type: "test", diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 5df64f381..516aa0a8f 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -26,6 +26,7 @@ import ( "github.com/hashicorp/terraform/providers" "github.com/hashicorp/terraform/provisioners" "github.com/hashicorp/terraform/states" + "github.com/hashicorp/terraform/states/statefile" "github.com/hashicorp/terraform/tfdiags" "github.com/zclconf/go-cty/cty" ) @@ -2984,7 +2985,9 @@ func TestContext2Apply_orphanResource(t *testing.T) { AttrsJSON: []byte(`{}`), }, providerAddr) }) - if !cmp.Equal(state, want) { + + // compare the marshaled form to easily remove empty and nil slices + if !statefile.StatesMarshalEqual(state, want) { t.Fatalf("wrong state after step 1\n%s", cmp.Diff(want, state)) } diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index 444aad8f4..1d098a63b 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -572,8 +572,10 @@ test_object.b `) instanceGraph := filterInstances(g) - if instanceGraph.String() != expected { - t.Fatalf("expected:\n%s\ngot:\n%s", expected, instanceGraph) + got := strings.TrimSpace(instanceGraph.String()) + + if got != expected { + t.Fatalf("expected:\n%s\ngot:\n%s", expected, got) } } diff --git a/terraform/node_resource_abstract.go b/terraform/node_resource_abstract.go index a1a4fc0b5..bdf62ab26 100644 --- a/terraform/node_resource_abstract.go +++ b/terraform/node_resource_abstract.go @@ -34,6 +34,10 @@ type ConcreteResourceInstanceNodeFunc func(*NodeAbstractResourceInstance) dag.Ve // configuration. type GraphNodeResourceInstance interface { ResourceInstanceAddr() addrs.AbsResourceInstance + + // StateDependencies returns any inter-resource dependencies that are + // stored in the state. + StateDependencies() []addrs.AbsResource } // NodeAbstractResource represents a resource that has no associated diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index 0ad66a713..c25bdce7e 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -58,30 +58,35 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { destroyers := make(map[string][]GraphNodeDestroyer) destroyerAddrs := make(map[string]addrs.AbsResourceInstance) + // Record the creators, which will need to depend on the destroyers if they + // are only being updated. + creators := make(map[string]GraphNodeCreator) + // destroyersByResource records each destroyer by the AbsResourceAddress. // We use this because dependencies are only referenced as resources, but we // will want to connect all the individual instances for correct ordering. destroyersByResource := make(map[string][]GraphNodeDestroyer) for _, v := range g.Vertices() { - dn, ok := v.(GraphNodeDestroyer) - if !ok { - continue + switch n := v.(type) { + case GraphNodeDestroyer: + addrP := n.DestroyAddr() + if addrP == nil { + log.Printf("[WARN] DestroyEdgeTransformer: %q (%T) has no destroy address", dag.VertexName(n), v) + continue + } + addr := *addrP + + key := addr.String() + log.Printf("[TRACE] DestroyEdgeTransformer: %q (%T) destroys %s", dag.VertexName(n), v, key) + destroyers[key] = append(destroyers[key], n) + destroyerAddrs[key] = addr + + resAddr := addr.Resource.Absolute(addr.Module).String() + destroyersByResource[resAddr] = append(destroyersByResource[resAddr], n) + case GraphNodeCreator: + addr := n.CreateAddr() + creators[addr.String()] = n } - - addrP := dn.DestroyAddr() - if addrP == nil { - log.Printf("[WARN] DestroyEdgeTransformer: %q (%T) has no destroy address", dag.VertexName(dn), v) - continue - } - addr := *addrP - - key := addr.String() - log.Printf("[TRACE] DestroyEdgeTransformer: %q (%T) destroys %s", dag.VertexName(dn), v, key) - destroyers[key] = append(destroyers[key], dn) - destroyerAddrs[key] = addr - - resAddr := addr.Resource.Absolute(addr.Module).String() - destroyersByResource[resAddr] = append(destroyersByResource[resAddr], dn) } // If we aren't destroying anything, there will be no edges to make @@ -90,22 +95,17 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { return nil } - type withDeps interface { - StateDependencies() []addrs.AbsResource - } - - // Connect destroy dependencies directly as stored in the state in case - // there's no configuration to create the edges otherwise. + // Connect destroy despendencies as stored in the state for _, ds := range destroyers { for _, des := range ds { - ri, ok := des.(withDeps) + ri, ok := des.(GraphNodeResourceInstance) if !ok { continue } for _, resAddr := range ri.StateDependencies() { for _, desDep := range destroyersByResource[resAddr.String()] { - log.Printf("[TRACE] DestroyEdgeTransformer: %s depends on %s\n", dag.VertexName(des), dag.VertexName(desDep)) + log.Printf("[TRACE] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(desDep), dag.VertexName(des)) g.Connect(dag.BasicEdge(desDep, des)) } @@ -113,6 +113,22 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { } } + // connect creators to any destroyers on which they may depend + for _, c := range creators { + ri, ok := c.(GraphNodeResourceInstance) + if !ok { + continue + } + + 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)) + + } + } + } + // Go through and connect creators to destroyers. Going along with // our example, this makes: A_d => A for _, v := range g.Vertices() { @@ -149,8 +165,6 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { } } - // FIXME: connect StateDependencies in here somewhere - // This is strange but is the easiest way to get the dependencies // of a node that is being destroyed. We use another graph to make sure // the resource is in the graph and ask for references. We have to do this diff --git a/terraform/transform_reference.go b/terraform/transform_reference.go index 11d261e33..25e544996 100644 --- a/terraform/transform_reference.go +++ b/terraform/transform_reference.go @@ -3,6 +3,7 @@ package terraform import ( "fmt" "log" + "sort" "github.com/hashicorp/hcl/v2" "github.com/hashicorp/terraform/addrs" @@ -158,11 +159,14 @@ func (t AttachDependenciesTransformer) Transform(g *Graph) error { for _, d := range depMap { deps = append(deps, d) } + sort.Slice(deps, func(i, j int) bool { + return deps[i].String() < deps[j].String() + }) log.Printf("[TRACE] AttachDependenciesTransformer: %s depends on %s", attacher.ResourceAddr(), deps) attacher.AttachDependencies(deps) - } + return nil }