From a6776eaa94a84659c8c5ccb1e8434355033f2caf Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 18 Aug 2020 16:34:44 -0400 Subject: [PATCH 1/2] completely prune inter-module dependencies There was a missing outer loop for catching inverse module dependencies when pruning nodes for destroy. Since the need to "register" the fully destroyed modules no longer exists, the extra complication of pruning the modules as a whole from the leaves inward is no longer required. While it is technically still a valid optimization to reduce iterations, the extra comparisons required to backtrack for transitive dependencies don't amount to much, and having a single nested loop is much easier to maintain. --- terraform/transform_destroy_edge.go | 67 ++--------------------------- 1 file changed, 4 insertions(+), 63 deletions(-) diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index 4a3245eca..ac935f5ed 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -2,7 +2,6 @@ package terraform import ( "log" - "sort" "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/states" @@ -188,69 +187,9 @@ func (t *pruneUnusedNodesTransformer) Transform(g *Graph) error { // dependencies from child modules, freeing up nodes in the parent module // to also be removed. - // First collect the nodes into their respective modules based on - // configuration path. - moduleMap := make(map[string]pruneUnusedNodesMod) - for _, v := range g.Vertices() { - var path addrs.Module - switch v := v.(type) { - case GraphNodeModulePath: - path = v.ModulePath() - default: - continue - } - m := moduleMap[path.String()] - m.addr = path - m.nodes = append(m.nodes, v) + nodes := g.Vertices() - moduleMap[path.String()] = m - } - - // now we need to restructure the modules so we can sort them - var modules []pruneUnusedNodesMod - - for _, mod := range moduleMap { - modules = append(modules, mod) - } - - // Sort them by path length, longest first, so that we start with the - // deepest modules. The order of modules at the same tree level doesn't - // matter, we just need to ensure that child modules are processed before - // parent modules. - sort.Slice(modules, func(i, j int) bool { - return len(modules[i].addr) > len(modules[j].addr) - }) - - for _, mod := range modules { - mod.removeUnused(g) - } - - return nil -} - -// pruneUnusedNodesMod is a container to hold the nodes that belong to a -// particular configuration module for the pruneUnusedNodesTransformer -type pruneUnusedNodesMod struct { - addr addrs.Module - nodes []dag.Vertex -} - -// Remove any unused locals, variables, outputs and expanders. Since module -// closers can also lookup expansion info to detect orphaned instances, disable -// them if their associated expander is removed. -func (m *pruneUnusedNodesMod) removeUnused(g *Graph) { - // We modify the nodes slice during processing here. - // Make a copy so no one is surprised by this changing in the future. - nodes := make([]dag.Vertex, len(m.nodes)) - copy(nodes, m.nodes) - - // since we have no defined structure within the module, just cycle through - // the nodes in each module until there are no more removals - removed := true - for { - if !removed { - return - } + for removed := true; removed; { removed = false for i := 0; i < len(nodes); i++ { @@ -307,4 +246,6 @@ func (m *pruneUnusedNodesMod) removeUnused(g *Graph) { }() } } + + return nil } From b68ab92392ba52d145e4f145068ec8b0573b8c0f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 19 Aug 2020 10:59:19 -0400 Subject: [PATCH 2/2] more complicated for_each destroy --- terraform/context_apply_test.go | 109 ++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index ee5d7622d..d67019f65 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -11645,3 +11645,112 @@ output "output" { t.Fatalf("destroy apply errors: %s", diags.Err()) } } + +// Destroying properly requires pruning out all unneeded config nodes to +// prevent incorrect expansion evaluation. +func TestContext2Apply_destroyInterModuleExpansion(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +data "test_data_source" "a" { + for_each = { + one = "thing" + } +} + +locals { + module_input = { + for k, v in data.test_data_source.a : k => v.id + } +} + +module "mod1" { + source = "./mod" + input = local.module_input +} + +module "mod2" { + source = "./mod" + input = module.mod1.outputs +} + +resource "test_instance" "bar" { + for_each = module.mod2.outputs +} + +output "module_output" { + value = module.mod2.outputs +} +output "test_instances" { + value = test_instance.bar +} +`, + "mod/main.tf": ` +variable "input" { +} + +data "test_data_source" "foo" { + for_each = var.input +} + +output "outputs" { + value = data.test_data_source.foo +} +`}) + + p := testProvider("test") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse { + return providers.ReadDataSourceResponse{ + State: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("data_source"), + "foo": cty.StringVal("output"), + }), + } + } + + ctx := testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + if _, diags := ctx.Plan(); diags.HasErrors() { + t.Fatalf("plan errors: %s", diags.Err()) + } + + state, diags := ctx.Apply() + if diags.HasErrors() { + t.Fatalf("apply errors: %s", diags.Err()) + } + + destroy := func() { + ctx = testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + + State: state, + Destroy: true, + }) + + if _, diags := ctx.Refresh(); diags.HasErrors() { + t.Fatalf("destroy plan errors: %s", diags.Err()) + } + + if _, diags := ctx.Plan(); diags.HasErrors() { + t.Fatalf("destroy plan errors: %s", diags.Err()) + } + + state, diags = ctx.Apply() + if diags.HasErrors() { + t.Fatalf("destroy apply errors: %s", diags.Err()) + } + } + + destroy() + // Destroying again from the empty state should not cause any errors either + destroy() +}