From 23112e198aeae433cb9572f07a78503a288a5bfa Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 19 Nov 2019 17:30:27 -0500 Subject: [PATCH 1/4] fix missing deposed key --- terraform/node_resource_destroy_deposed.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/terraform/node_resource_destroy_deposed.go b/terraform/node_resource_destroy_deposed.go index 67c46913f..e0d5db836 100644 --- a/terraform/node_resource_destroy_deposed.go +++ b/terraform/node_resource_destroy_deposed.go @@ -178,7 +178,7 @@ var ( ) func (n *NodeDestroyDeposedResourceInstanceObject) Name() string { - return fmt.Sprintf("%s (destroy deposed %s)", n.Addr.String(), n.DeposedKey) + return fmt.Sprintf("%s (destroy deposed %s)", n.ResourceInstanceAddr(), n.DeposedKey) } func (n *NodeDestroyDeposedResourceInstanceObject) DeposedInstanceObjectKey() states.DeposedKey { From c47f100e5675cd0f68d9c3e5e3cc3aaa589c8db9 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 19 Nov 2019 17:41:45 -0500 Subject: [PATCH 2/4] update test states that need dependency info A number of tests had no, or incomplete state for the transformations they wanted to test. Add states state with the correct dependencies for these tests. --- terraform/context_apply_test.go | 123 +++++++++++-------- terraform/graph_builder_apply_test.go | 21 ++++ terraform/terraform_test.go | 16 +++ terraform/transform_destroy_cbd_test.go | 149 +++++++++++++++++++++--- 4 files changed, 248 insertions(+), 61 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index c61cd8881..f53393a7e 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -9175,33 +9175,48 @@ func TestContext2Apply_createBefore_depends(t *testing.T) { p := testProvider("aws") p.ApplyFn = testApplyFn p.DiffFn = testDiffFn - state := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.web": &ResourceState{ + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "web", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"bar","require_new":"ami-old"}`), + }, + addrs.ProviderConfig{ + Type: "aws", + }.Absolute(addrs.RootModuleInstance), + ) + + root.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "lb", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"baz","instance":"bar"}`), + Dependencies: []addrs.AbsResource{ + addrs.AbsResource{ + Resource: addrs.Resource{ + Mode: addrs.ManagedResourceMode, Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - Attributes: map[string]string{ - "require_new": "ami-old", - }, - }, - }, - "aws_instance.lb": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "baz", - Attributes: map[string]string{ - "instance": "bar", - }, - }, + Name: "web", }, + Module: addrs.RootModuleInstance, }, }, }, - }) + addrs.ProviderConfig{ + Type: "aws", + }.Absolute(addrs.RootModuleInstance), + ) + ctx := testContext2(t, &ContextOpts{ Config: m, Hooks: []Hook{h}, @@ -9241,17 +9256,18 @@ func TestContext2Apply_createBefore_depends(t *testing.T) { // Test that things were managed _in the right order_ order := h.States + diffs := h.Diffs if !order[0].IsNull() || diffs[0].Action == plans.Delete { t.Fatalf("should create new instance first: %#v", order) } if order[1].GetAttr("id").AsString() != "baz" { - t.Fatalf("update must happen after create: %#v", order) + t.Fatalf("update must happen after create: %#v", order[1]) } if order[2].GetAttr("id").AsString() != "bar" || diffs[2].Action != plans.Delete { - t.Fatalf("destroy must happen after update: %#v", order) + t.Fatalf("destroy must happen after update: %#v", order[2]) } } @@ -9290,33 +9306,48 @@ func TestContext2Apply_singleDestroy(t *testing.T) { return testApplyFn(info, s, d) } p.DiffFn = testDiffFn - state := MustShimLegacyState(&State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.web": &ResourceState{ + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "web", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"bar","require_new":"ami-old"}`), + }, + addrs.ProviderConfig{ + Type: "aws", + }.Absolute(addrs.RootModuleInstance), + ) + + root.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "lb", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"baz","instance":"bar"}`), + Dependencies: []addrs.AbsResource{ + addrs.AbsResource{ + Resource: addrs.Resource{ + Mode: addrs.ManagedResourceMode, Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - Attributes: map[string]string{ - "require_new": "ami-old", - }, - }, - }, - "aws_instance.lb": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "baz", - Attributes: map[string]string{ - "instance": "bar", - }, - }, + Name: "web", }, + Module: addrs.RootModuleInstance, }, }, }, - }) + addrs.ProviderConfig{ + Type: "aws", + }.Absolute(addrs.RootModuleInstance), + ) + ctx := testContext2(t, &ContextOpts{ Config: m, Hooks: []Hook{h}, diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index 1d098a63b..92908e583 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -89,11 +89,32 @@ func TestApplyGraphBuilder_depCbd(t *testing.T) { }, } + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.A").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"A"}`), + }, + mustProviderConfig("provider.test"), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.B").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"B","test_list":["x"]}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + }, + mustProviderConfig("provider.test"), + ) + b := &ApplyGraphBuilder{ Config: testModule(t, "graph-builder-apply-dep-cbd"), Changes: changes, Components: simpleMockComponentFactory(), Schemas: simpleTestSchemas(), + State: state, } g, err := b.Build(addrs.RootModuleInstance) diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index 0baa2a204..845881977 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -210,6 +210,22 @@ func mustResourceInstanceAddr(s string) addrs.AbsResourceInstance { return addr } +func mustResourceAddr(s string) addrs.AbsResource { + addr, diags := addrs.ParseAbsResourceStr(s) + if diags.HasErrors() { + panic(diags.Err()) + } + return addr +} + +func mustProviderConfig(s string) addrs.AbsProviderConfig { + p, diags := addrs.ParseAbsProviderConfigStr(s) + if diags.HasErrors() { + panic(diags.Err()) + } + return p +} + func instanceObjectIdForTests(obj *states.ResourceInstanceObject) string { v := obj.Value if v.IsNull() || !v.IsKnown() { diff --git a/terraform/transform_destroy_cbd_test.go b/terraform/transform_destroy_cbd_test.go index 13deaa336..d4dcb90e0 100644 --- a/terraform/transform_destroy_cbd_test.go +++ b/terraform/transform_destroy_cbd_test.go @@ -7,9 +7,10 @@ import ( "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/plans" + "github.com/hashicorp/terraform/states" ) -func cbdTestGraph(t *testing.T, mod string, changes *plans.Changes) *Graph { +func cbdTestGraph(t *testing.T, mod string, changes *plans.Changes, state *states.State) *Graph { module := testModule(t, mod) applyBuilder := &ApplyGraphBuilder{ @@ -17,6 +18,7 @@ func cbdTestGraph(t *testing.T, mod string, changes *plans.Changes) *Graph { Changes: changes, Components: simpleMockComponentFactory(), Schemas: simpleTestSchemas(), + State: state, } g, err := (&BasicGraphBuilder{ Steps: cbdTestSteps(applyBuilder.Steps()), @@ -77,7 +79,27 @@ func TestCBDEdgeTransformer(t *testing.T) { }, } - g := cbdTestGraph(t, "transform-destroy-cbd-edge-basic", changes) + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.A").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"A"}`), + }, + mustProviderConfig("provider.test"), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.B").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"B","test_list":["x"]}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + }, + mustProviderConfig("provider.test"), + ) + + g := cbdTestGraph(t, "transform-destroy-cbd-edge-basic", changes, state) g = filterInstances(g) actual := strings.TrimSpace(g.String()) @@ -119,7 +141,38 @@ func TestCBDEdgeTransformerMulti(t *testing.T) { }, } - g := cbdTestGraph(t, "transform-destroy-cbd-edge-multi", changes) + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.A").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"A"}`), + }, + mustProviderConfig("provider.test"), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.B").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"B"}`), + }, + mustProviderConfig("provider.test"), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.C").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"C","test_list":["x"]}`), + Dependencies: []addrs.AbsResource{ + mustResourceAddr("test_object.A"), + mustResourceAddr("test_object.B"), + }, + }, + mustProviderConfig("provider.test"), + ) + + g := cbdTestGraph(t, "transform-destroy-cbd-edge-multi", changes, state) g = filterInstances(g) actual := strings.TrimSpace(g.String()) @@ -166,7 +219,36 @@ func TestCBDEdgeTransformer_depNonCBDCount(t *testing.T) { }, } - g := cbdTestGraph(t, "transform-cbd-destroy-edge-count", changes) + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.A").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"A"}`), + }, + mustProviderConfig("provider.test"), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.B[0]").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"B","test_list":["x"]}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + }, + mustProviderConfig("provider.test"), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.B[1]").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"B","test_list":["x"]}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + }, + mustProviderConfig("provider.test"), + ) + + g := cbdTestGraph(t, "transform-cbd-destroy-edge-count", changes, state) actual := strings.TrimSpace(g.String()) expected := regexp.MustCompile(strings.TrimSpace(` @@ -215,22 +297,59 @@ func TestCBDEdgeTransformer_depNonCBDCountBoth(t *testing.T) { }, } - g := cbdTestGraph(t, "transform-cbd-destroy-edge-both-count", changes) + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.A[0]").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"A"}`), + }, + mustProviderConfig("provider.test"), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.A[1]").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"A"}`), + }, + mustProviderConfig("provider.test"), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.B[0]").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"B","test_list":["x"]}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + }, + mustProviderConfig("provider.test"), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.B[1]").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"B","test_list":["x"]}`), + Dependencies: []addrs.AbsResource{mustResourceAddr("test_object.A")}, + }, + mustProviderConfig("provider.test"), + ) + + g := cbdTestGraph(t, "transform-cbd-destroy-edge-both-count", changes, state) actual := strings.TrimSpace(g.String()) expected := regexp.MustCompile(strings.TrimSpace(` -test_object.A \(destroy deposed \w+\) - test_object.A\[0\] - test_object.A\[1\] - test_object.B\[0\] - test_object.B\[1\] -test_object.A \(destroy deposed \w+\) - test_object.A\[0\] - test_object.A\[1\] - test_object.B\[0\] - test_object.B\[1\] test_object.A\[0\] +test_object.A\[0\] \(destroy deposed \w+\) + test_object.A\[0\] + test_object.A\[1\] + test_object.B\[0\] + test_object.B\[1\] test_object.A\[1\] +test_object.A\[1\] \(destroy deposed \w+\) + test_object.A\[0\] + test_object.A\[1\] + test_object.B\[0\] + test_object.B\[1\] test_object.B\[0\] test_object.A\[0\] test_object.A\[1\] From 8510aa81cab09d97317b074a8f3b9e39f2d1a3ff Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 21 Nov 2019 10:33:00 -0500 Subject: [PATCH 3/4] make sure to get a ResourceAddr for destroy refs addr.Resource is sometimes a resource, except when it's an instance. Make sure to always get the underlying resource. --- terraform/transform_destroy_edge.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index df3872a1d..bc3c9619d 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -81,7 +81,7 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { destroyers[key] = append(destroyers[key], n) destroyerAddrs[key] = addr - resAddr := addr.Resource.Absolute(addr.Module).String() + resAddr := addr.Resource.Resource.Absolute(addr.Module).String() destroyersByResource[resAddr] = append(destroyersByResource[resAddr], n) case GraphNodeCreator: addr := n.CreateAddr() From 5ed7d172652dfbe0f83a8bcb7a0dc4471b805fac Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 21 Nov 2019 10:34:22 -0500 Subject: [PATCH 4/4] remove incorrect comment The CreateBeforeDestroy transformer correctly handles the edge referred to in the comment, and going forward it will probably be easier to use the knowledge of this edge for CBD anyway. --- terraform/transform_destroy_edge.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index bc3c9619d..f52429229 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -122,12 +122,6 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { for _, resAddr := range ri.StateDependencies() { for _, desDep := range destroyersByResource[resAddr.String()] { - // TODO: don't connect this if c is CreateBeforeDestroy. - // This will require getting the actual change action at - // this point, since the lifecycle may have been forced - // by a dependent. This should prevent needing to prune - // the edge back out in CBDEdgeTransformer, and allow - // non-CBD nodes to depend on CBD destroys directly. log.Printf("[TRACE] DestroyEdgeTransformer: %s has stored dependency of %s\n", dag.VertexName(c), dag.VertexName(desDep)) g.Connect(dag.BasicEdge(c, desDep))