From dd8ab5812ede0ac151aa37e3c112d735060cf957 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 12 Dec 2019 14:59:18 -0500 Subject: [PATCH 1/5] add CreateBeforeDestroy to state Added to src and object, but still need serialization and tests. --- states/instance_object.go | 7 +++++++ states/instance_object_src.go | 18 ++++++++++-------- states/state_deepcopy.go | 15 ++++++++------- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/states/instance_object.go b/states/instance_object.go index 78e1dda93..1642b4569 100644 --- a/states/instance_object.go +++ b/states/instance_object.go @@ -36,6 +36,13 @@ type ResourceInstanceObject struct { // altogether, or is now deposed. Dependencies []addrs.AbsResource + // CreateBeforeDestroy reflects the status of the lifecycle + // create_before_destroy option when this instance was last updated. + // Because create_before_destroy also effects the overall ordering of the + // destroy operations, we need to record the status to ensure a resource + // removed from the config will still be destroyed in the same manner. + CreateBeforeDestroy bool + // DependsOn corresponds to the deprecated `depends_on` field in the state. // This field contained the configuration `depends_on` values, and some of // the references from within a single module. diff --git a/states/instance_object_src.go b/states/instance_object_src.go index a18cf313c..ff56bcfb8 100644 --- a/states/instance_object_src.go +++ b/states/instance_object_src.go @@ -51,9 +51,10 @@ type ResourceInstanceObjectSrc struct { // These fields all correspond to the fields of the same name on // ResourceInstanceObject. - Private []byte - Status ObjectStatus - Dependencies []addrs.AbsResource + Private []byte + Status ObjectStatus + Dependencies []addrs.AbsResource + CreateBeforeDestroy bool // deprecated DependsOn []addrs.Referenceable } @@ -85,11 +86,12 @@ func (os *ResourceInstanceObjectSrc) Decode(ty cty.Type) (*ResourceInstanceObjec } return &ResourceInstanceObject{ - Value: val, - Status: os.Status, - Dependencies: os.Dependencies, - DependsOn: os.DependsOn, - Private: os.Private, + Value: val, + Status: os.Status, + Dependencies: os.Dependencies, + DependsOn: os.DependsOn, + Private: os.Private, + CreateBeforeDestroy: os.CreateBeforeDestroy, }, nil } diff --git a/states/state_deepcopy.go b/states/state_deepcopy.go index 7d7a7ef10..89082fb8e 100644 --- a/states/state_deepcopy.go +++ b/states/state_deepcopy.go @@ -166,13 +166,14 @@ func (obj *ResourceInstanceObjectSrc) DeepCopy() *ResourceInstanceObjectSrc { } return &ResourceInstanceObjectSrc{ - Status: obj.Status, - SchemaVersion: obj.SchemaVersion, - Private: private, - AttrsFlat: attrsFlat, - AttrsJSON: attrsJSON, - Dependencies: dependencies, - DependsOn: dependsOn, + Status: obj.Status, + SchemaVersion: obj.SchemaVersion, + Private: private, + AttrsFlat: attrsFlat, + AttrsJSON: attrsJSON, + Dependencies: dependencies, + DependsOn: dependsOn, + CreateBeforeDestroy: obj.CreateBeforeDestroy, } } From d2a9dd3cefbb524d3b1e86110f654444ccacc50b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 12 Dec 2019 15:00:47 -0500 Subject: [PATCH 2/5] don't override CreateBeforeDestroy from diff If the Diff is only a delete action, we can't override CreateBeforeDestroy, because it will always be false and prevent the stored state value from being used. --- terraform/transform_diff.go | 1 - 1 file changed, 1 deletion(-) diff --git a/terraform/transform_diff.go b/terraform/transform_diff.go index 23b6e2a75..637309fad 100644 --- a/terraform/transform_diff.go +++ b/terraform/transform_diff.go @@ -161,7 +161,6 @@ func (t *DiffTransformer) Transform(g *Graph) error { NodeAbstractResourceInstance: abstract, DeposedKey: dk, } - node.(*NodeDestroyResourceInstance).ModifyCreateBeforeDestroy(createBeforeDestroy) } else { node = &NodeDestroyDeposedResourceInstanceObject{ NodeAbstractResourceInstance: abstract, From a44cf03eaa665270d6da0e1aedb75752c9b855aa Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 12 Dec 2019 15:02:26 -0500 Subject: [PATCH 3/5] test for CBD instance being removed entirely Even though this is only the destroy half of CreateBeforeDestroy, the resource may still require the same destroy order. --- terraform/graph_builder_apply_test.go | 105 ++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index c0827d05b..8e04587c1 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -610,6 +610,111 @@ test_object.b } } +// Ensure that an update resulting from the removal of a resource happens before +// a CBD resource is destroyed. +func TestApplyGraphBuilder_updateFromCBDOrphan(t *testing.T) { + schemas := simpleTestSchemas() + instanceSchema := schemas.Providers[addrs.NewLegacyProvider("test")].ResourceTypes["test_object"] + + bBefore, _ := plans.NewDynamicValue( + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("b_id"), + "test_string": cty.StringVal("a_id"), + }), instanceSchema.ImpliedType()) + bAfter, _ := plans.NewDynamicValue( + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("b_id"), + "test_string": cty.StringVal("changed"), + }), instanceSchema.ImpliedType()) + + changes := &plans.Changes{ + Resources: []*plans.ResourceInstanceChangeSrc{ + { + Addr: mustResourceInstanceAddr("test_object.a"), + ChangeSrc: plans.ChangeSrc{ + Action: plans.Delete, + }, + }, + { + Addr: mustResourceInstanceAddr("test_object.b"), + ChangeSrc: plans.ChangeSrc{ + Action: plans.Update, + Before: bBefore, + After: bAfter, + }, + }, + }, + } + + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_object", + Name: "a", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"a_id"}`), + CreateBeforeDestroy: true, + }, + addrs.LocalProviderConfig{ + LocalName: "test", + }.Absolute(addrs.RootModuleInstance), + ) + root.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_object", + Name: "b", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"b_id","test_string":"a_id"}`), + Dependencies: []addrs.AbsResource{ + addrs.AbsResource{ + Resource: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_object", + Name: "a", + }, + Module: root.Addr, + }, + }, + }, + addrs.LocalProviderConfig{ + LocalName: "test", + }.Absolute(addrs.RootModuleInstance), + ) + + b := &ApplyGraphBuilder{ + Config: testModule(t, "graph-builder-apply-orphan-update"), + Changes: changes, + Components: simpleMockComponentFactory(), + Schemas: schemas, + State: state, + } + + g, err := b.Build(addrs.RootModuleInstance) + if err != nil { + t.Fatalf("err: %s", err) + } + + expected := strings.TrimSpace(` +test_object.a (destroy) + test_object.b +test_object.b +`) + + instanceGraph := filterInstances(g) + got := strings.TrimSpace(instanceGraph.String()) + + if got != expected { + t.Fatalf("expected:\n%s\ngot:\n%s", expected, got) + } +} + // The orphan clean up node should not be connected to a provider func TestApplyGraphBuilder_orphanedWithProvider(t *testing.T) { changes := &plans.Changes{ From 691bb6b9077483d3d78966f1d8cb8920879559cd Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 12 Dec 2019 15:03:32 -0500 Subject: [PATCH 4/5] use CreateBeforeDestroy from state If the resource was stored as CreateBeforeDestroy, use the same ordering regardless. This reversal will be taken care if more cleanly in state-only destroys, and with less risk. Don't use this commit as-is. --- terraform/node_resource_destroy.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index 0374d83dd..3619671d8 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -56,12 +56,21 @@ func (n *NodeDestroyResourceInstance) CreateBeforeDestroy() bool { return *n.CreateBeforeDestroyOverride } - // If we have no config, we just assume no - if n.Config == nil || n.Config.Managed == nil { - return false + // Config takes precedence + if n.Config != nil && n.Config.Managed != nil { + return n.Config.Managed.CreateBeforeDestroy } - return n.Config.Managed.CreateBeforeDestroy + // Otherwise check the state for a stored destroy order + if rs := n.ResourceState; rs != nil { + if s := rs.Instance(n.InstanceKey); s != nil { + if s.Current != nil { + return s.Current.CreateBeforeDestroy + } + } + } + + return false } // GraphNodeDestroyerCBD From 85ebaac8ce89d87bd04f881104018c282aba5502 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 13 Feb 2020 21:15:11 -0500 Subject: [PATCH 5/5] update provider types in tests --- terraform/graph_builder_apply_test.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index 8e04587c1..ede05e5de 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -659,9 +659,7 @@ func TestApplyGraphBuilder_updateFromCBDOrphan(t *testing.T) { AttrsJSON: []byte(`{"id":"a_id"}`), CreateBeforeDestroy: true, }, - addrs.LocalProviderConfig{ - LocalName: "test", - }.Absolute(addrs.RootModuleInstance), + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) root.SetResourceInstanceCurrent( addrs.Resource{ @@ -683,9 +681,7 @@ func TestApplyGraphBuilder_updateFromCBDOrphan(t *testing.T) { }, }, }, - addrs.LocalProviderConfig{ - LocalName: "test", - }.Absolute(addrs.RootModuleInstance), + mustProviderConfig(`provider["registry.terraform.io/-/test"]`), ) b := &ApplyGraphBuilder{