From 28d5a5bf6304179070f03b4c55a0dfd5d3aaac07 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 20 Oct 2022 10:59:08 -0400 Subject: [PATCH] NoOp nodes should not have destroy edges NoOp changes should not participate in a destroy sequence, but because they are included as normal update nodes the usual connections were still being made. --- internal/terraform/graph_builder_apply.go | 4 +- internal/terraform/transform_destroy_edge.go | 27 +++++++- .../terraform/transform_destroy_edge_test.go | 69 +++++++++++++++++++ 3 files changed, 96 insertions(+), 4 deletions(-) diff --git a/internal/terraform/graph_builder_apply.go b/internal/terraform/graph_builder_apply.go index 40a2979c70..4d7763d2a5 100644 --- a/internal/terraform/graph_builder_apply.go +++ b/internal/terraform/graph_builder_apply.go @@ -142,7 +142,9 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { &ForcedCBDTransformer{}, // Destruction ordering - &DestroyEdgeTransformer{}, + &DestroyEdgeTransformer{ + Changes: b.Changes, + }, &CBDEdgeTransformer{ Config: b.Config, State: b.State, diff --git a/internal/terraform/transform_destroy_edge.go b/internal/terraform/transform_destroy_edge.go index 14b421b758..aac39fd4d7 100644 --- a/internal/terraform/transform_destroy_edge.go +++ b/internal/terraform/transform_destroy_edge.go @@ -5,6 +5,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/dag" + "github.com/hashicorp/terraform/internal/plans" ) // GraphNodeDestroyer must be implemented by nodes that destroy resources. @@ -37,7 +38,15 @@ type GraphNodeCreator interface { // dependent resources will block parent resources from deleting. Concrete // example: VPC with subnets, the VPC can't be deleted while there are // still subnets. -type DestroyEdgeTransformer struct{} +type DestroyEdgeTransformer struct { + // FIXME: GraphNodeCreators are not always applying changes, and should not + // participate in the destroy graph if there are no operations which could + // interract with destroy nodes. We need Changes for now to detect the + // action type, but perhaps this should be indicated somehow by the + // DiffTransformer which was intended to be the only transformer operating + // from the change set. + Changes *plans.Changes +} // tryInterProviderDestroyEdge checks if we're inserting a destroy edge // across a provider boundary, and only adds the edge if it results in no cycles. @@ -144,8 +153,20 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { resAddr := addr.ContainingResource().Config().String() destroyersByResource[resAddr] = append(destroyersByResource[resAddr], n) case GraphNodeCreator: - addr := n.CreateAddr().ContainingResource().Config().String() - creators[addr] = append(creators[addr], n) + addr := n.CreateAddr() + cfgAddr := addr.ContainingResource().Config().String() + + if t.Changes == nil { + // unit tests may not have changes + creators[cfgAddr] = append(creators[cfgAddr], n) + break + } + + // NoOp changes should not participate in the destroy dependencies. + rc := t.Changes.ResourceInstance(*addr) + if rc != nil && rc.Action != plans.NoOp { + creators[cfgAddr] = append(creators[cfgAddr], n) + } } } diff --git a/internal/terraform/transform_destroy_edge_test.go b/internal/terraform/transform_destroy_edge_test.go index fbb2c5d452..b4fc351be2 100644 --- a/internal/terraform/transform_destroy_edge_test.go +++ b/internal/terraform/transform_destroy_edge_test.go @@ -440,6 +440,75 @@ func TestPruneUnusedNodesTransformer_rootModuleOutputValues(t *testing.T) { } } +// NoOp changes should not be participating in the destroy sequence +func TestDestroyEdgeTransformer_noOp(t *testing.T) { + g := Graph{Path: addrs.RootModuleInstance} + g.Add(testDestroyNode("test_object.A")) + g.Add(testUpdateNode("test_object.B")) + g.Add(testDestroyNode("test_object.C")) + + 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["registry.terraform.io/hashicorp/test"]`), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.B").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"B","test_string":"x"}`), + Dependencies: []addrs.ConfigResource{mustConfigResourceAddr("test_object.A")}, + }, + mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + root.SetResourceInstanceCurrent( + mustResourceInstanceAddr("test_object.C").Resource, + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"C","test_string":"x"}`), + Dependencies: []addrs.ConfigResource{mustConfigResourceAddr("test_object.A"), + mustConfigResourceAddr("test_object.B")}, + }, + mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`), + ) + + if err := (&AttachStateTransformer{State: state}).Transform(&g); err != nil { + t.Fatal(err) + } + + tf := &DestroyEdgeTransformer{ + // We only need a minimal object to indicate GraphNodeCreator change is + // a NoOp here. + Changes: &plans.Changes{ + Resources: []*plans.ResourceInstanceChangeSrc{ + { + Addr: mustResourceInstanceAddr("test_object.B"), + ChangeSrc: plans.ChangeSrc{Action: plans.NoOp}, + }, + }, + }, + } + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + + expected := strings.TrimSpace(` +test_object.A (destroy) + test_object.C (destroy) +test_object.B +test_object.C (destroy)`) + + actual := strings.TrimSpace(g.String()) + if actual != expected { + t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) + } +} + func testDestroyNode(addrString string) GraphNodeDestroyer { instAddr := mustResourceInstanceAddr(addrString) inst := NewNodeAbstractResourceInstance(instAddr)