From 7c01d442976f4e0d02b96b0ea1aacf4bb8ed95aa 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 bb88699d2ee8..2c7025f9d5a4 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 2ca6a888bc99..b0bcd9f0040d 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 } }