From c9e581e58a8b64f2ea3aa81eaafc884d33155551 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 9 Sep 2020 12:23:23 -0400 Subject: [PATCH] don't connect module closers to destroy nodes One of the tenants of the graph transformations is that resource destroy nodes can only be ordered relative to other resources, and can't be referenced directly. This was broken by the module close node which naively connected to all module nodes, creating cycles in some cases when edges are reversed from CreateBeforeDestroy. --- terraform/context_apply_test.go | 81 +++++++++++++++++++++++++ terraform/transform_module_expansion.go | 29 +++++---- 2 files changed, 98 insertions(+), 12 deletions(-) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index bb88699d2..2c7025f9d 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -11766,3 +11766,84 @@ output "outputs" { // Destroying again from the empty state should not cause any errors either destroy() } + +func TestContext2Apply_createBeforeDestroyWithModule(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +variable "v" {} + +module "mod" { + source = "./mod" + in = var.v +} + +resource "test_resource" "a" { + value = var.v + depends_on = [module.mod] + lifecycle { + create_before_destroy = true + } +} +`, + "mod/main.tf": ` +variable "in" {} + +resource "test_resource" "a" { + value = var.in +} +`}) + + p := testProvider("test") + p.ApplyFn = testApplyFn + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + proposed := req.ProposedNewState.AsValueMap() + proposed["id"] = cty.UnknownVal(cty.String) + return providers.PlanResourceChangeResponse{ + PlannedState: cty.ObjectVal(proposed), + RequiresReplace: []cty.Path{cty.Path{cty.GetAttrStep{Name: "value"}}}, + } + } + + ctx := testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + Variables: InputValues{ + "v": &InputValue{ + Value: cty.StringVal("A"), + }, + }, + }) + + 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()) + } + + ctx = testContext2(t, &ContextOpts{ + Config: m, + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + Variables: InputValues{ + "v": &InputValue{ + Value: cty.StringVal("B"), + }, + }, + State: state, + }) + + if _, diags := ctx.Plan(); diags.HasErrors() { + t.Fatalf("plan errors: %s", diags.Err()) + } + + _, diags = ctx.Apply() + if diags.HasErrors() { + t.Fatalf("apply errors: %s", diags.Err()) + } +} diff --git a/terraform/transform_module_expansion.go b/terraform/transform_module_expansion.go index 2ca6a888b..b0bcd9f00 100644 --- a/terraform/transform_module_expansion.go +++ b/terraform/transform_module_expansion.go @@ -43,8 +43,13 @@ func (t *ModuleExpansionTransformer) Transform(g *Graph) error { // handled by the RemovedModuleTransformer, and those module closers are in // the graph already, and need to be connected to their parent closers. for _, v := range g.Vertices() { - // skip closers so they don't attach to themselves - if _, ok := v.(*nodeCloseModule); ok { + switch v.(type) { + case GraphNodeDestroyer: + // Destroy nodes can only be ordered relative to other resource + // instances. + continue + case *nodeCloseModule: + // a module closer cannot connect to itself continue } @@ -84,17 +89,17 @@ func (t *ModuleExpansionTransformer) transform(g *Graph, c *configs.Config, pare Config: c.Module, ModuleCall: modCall, } - var v dag.Vertex = n + var expander dag.Vertex = n if t.Concrete != nil { - v = t.Concrete(n) + expander = t.Concrete(n) } - g.Add(v) - log.Printf("[TRACE] ModuleExpansionTransformer: Added %s as %T", c.Path, v) + g.Add(expander) + log.Printf("[TRACE] ModuleExpansionTransformer: Added %s as %T", c.Path, expander) if parentNode != nil { - log.Printf("[TRACE] ModuleExpansionTransformer: %s must wait for expansion of %s", dag.VertexName(v), dag.VertexName(parentNode)) - g.Connect(dag.BasicEdge(v, parentNode)) + log.Printf("[TRACE] ModuleExpansionTransformer: %s must wait for expansion of %s", dag.VertexName(expander), dag.VertexName(parentNode)) + g.Connect(dag.BasicEdge(expander, parentNode)) } // Add the closer (which acts as the root module node) to provide a @@ -103,12 +108,12 @@ func (t *ModuleExpansionTransformer) transform(g *Graph, c *configs.Config, pare Addr: c.Path, } g.Add(closer) - g.Connect(dag.BasicEdge(closer, v)) + g.Connect(dag.BasicEdge(closer, expander)) t.closers[c.Path.String()] = closer for _, childV := range g.Vertices() { // don't connect a node to itself - if childV == v { + if childV == expander { continue } @@ -126,13 +131,13 @@ func (t *ModuleExpansionTransformer) transform(g *Graph, c *configs.Config, pare if path.Equal(c.Path) { log.Printf("[TRACE] ModuleExpansionTransformer: %s must wait for expansion of %s", dag.VertexName(childV), c.Path) - g.Connect(dag.BasicEdge(childV, v)) + g.Connect(dag.BasicEdge(childV, expander)) } } // Also visit child modules, recursively. for _, cc := range c.Children { - if err := t.transform(g, cc, v); err != nil { + if err := t.transform(g, cc, expander); err != nil { return err } }