From 7c703b1bbfc273b471a4970a9e8fa7fcf55298ea Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 25 Sep 2019 13:20:24 -0400 Subject: [PATCH 1/7] apply edge transforamtions after references We can't correctly resolve the destroy ordering if all references haven't been assigned to each node. --- terraform/graph_builder_apply.go | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 7182dd7db..9382c8aba 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -127,21 +127,6 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // Attach the state &AttachStateTransformer{State: b.State}, - // Destruction ordering - &DestroyEdgeTransformer{ - Config: b.Config, - State: b.State, - Schemas: b.Schemas, - }, - GraphTransformIf( - func() bool { return !b.Destroy }, - &CBDEdgeTransformer{ - Config: b.Config, - State: b.State, - Schemas: b.Schemas, - }, - ), - // Provisioner-related transformations &MissingProvisionerTransformer{Provisioners: b.Components.ResourceProvisioners()}, &ProvisionerTransformer{}, @@ -171,6 +156,21 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // Connect references so ordering is correct &ReferenceTransformer{}, + // Destruction ordering + &DestroyEdgeTransformer{ + Config: b.Config, + State: b.State, + Schemas: b.Schemas, + }, + GraphTransformIf( + func() bool { return !b.Destroy }, + &CBDEdgeTransformer{ + Config: b.Config, + State: b.State, + Schemas: b.Schemas, + }, + ), + // Handle destroy time transformations for output and local values. // Reverse the edges from outputs and locals, so that // interpolations don't fail during destroy. From 8f359847911953b839f23c57226ca6ecd7c6cffb Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 25 Sep 2019 21:31:17 -0400 Subject: [PATCH 2/7] Use Descendants rather than UpEdges in CBD When looking for dependencies to fix when handling create_before_destroy, we need to look past more than one edge, as dependencies may appear transitively through outputs and variables. Use Descendants rather than UpEdges. We have the full graph to use for the CBD transformation, so there's no longer any need to create a temporary graph, which may differ from the original. --- terraform/transform_config_flat.go | 71 ------------------------- terraform/transform_config_flat_test.go | 42 --------------- terraform/transform_destroy_cbd.go | 38 ++++--------- 3 files changed, 10 insertions(+), 141 deletions(-) delete mode 100644 terraform/transform_config_flat.go delete mode 100644 terraform/transform_config_flat_test.go diff --git a/terraform/transform_config_flat.go b/terraform/transform_config_flat.go deleted file mode 100644 index 866c91759..000000000 --- a/terraform/transform_config_flat.go +++ /dev/null @@ -1,71 +0,0 @@ -package terraform - -import ( - "github.com/hashicorp/terraform/configs" - "github.com/hashicorp/terraform/dag" -) - -// FlatConfigTransformer is a GraphTransformer that adds the configuration -// to the graph. The module used to configure this transformer must be -// the root module. -// -// This transform adds the nodes but doesn't connect any of the references. -// The ReferenceTransformer should be used for that. -// -// NOTE: In relation to ConfigTransformer: this is a newer generation config -// transformer. It puts the _entire_ config into the graph (there is no -// "flattening" step as before). -type FlatConfigTransformer struct { - Concrete ConcreteResourceNodeFunc // What to turn resources into - - Config *configs.Config -} - -func (t *FlatConfigTransformer) Transform(g *Graph) error { - // We have nothing to do if there is no configuration. - if t.Config == nil { - return nil - } - - return t.transform(g, t.Config) -} - -func (t *FlatConfigTransformer) transform(g *Graph, config *configs.Config) error { - // If we have no configuration then there's nothing to do. - if config == nil { - return nil - } - - // Transform all the children. - for _, c := range config.Children { - if err := t.transform(g, c); err != nil { - return err - } - } - - module := config.Module - // For now we assume that each module call produces only one module - // instance with no key, since we don't yet support "count" and "for_each" - // on modules. - // FIXME: As part of supporting "count" and "for_each" on modules, rework - // this so that we'll "expand" the module call first and then create graph - // nodes for each module instance separately. - instPath := config.Path.UnkeyedInstanceShim() - - for _, r := range module.ManagedResources { - addr := r.Addr().Absolute(instPath) - abstract := &NodeAbstractResource{ - Addr: addr, - Config: r, - } - // Grab the address for this resource - var node dag.Vertex = abstract - if f := t.Concrete; f != nil { - node = f(abstract) - } - - g.Add(node) - } - - return nil -} diff --git a/terraform/transform_config_flat_test.go b/terraform/transform_config_flat_test.go deleted file mode 100644 index 79fc09367..000000000 --- a/terraform/transform_config_flat_test.go +++ /dev/null @@ -1,42 +0,0 @@ -package terraform - -import ( - "strings" - "testing" - - "github.com/hashicorp/terraform/addrs" -) - -func TestFlatConfigTransformer_nilModule(t *testing.T) { - g := Graph{Path: addrs.RootModuleInstance} - tf := &FlatConfigTransformer{} - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - - if len(g.Vertices()) > 0 { - t.Fatal("graph should be empty") - } -} - -func TestFlatConfigTransformer(t *testing.T) { - g := Graph{Path: addrs.RootModuleInstance} - tf := &FlatConfigTransformer{ - Config: testModule(t, "transform-flat-config-basic"), - } - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(testTransformFlatConfigBasicStr) - if actual != expected { - t.Fatalf("bad:\n\n%s", actual) - } -} - -const testTransformFlatConfigBasicStr = ` -aws_instance.bar -aws_instance.foo -module.child.aws_instance.baz -` diff --git a/terraform/transform_destroy_cbd.go b/terraform/transform_destroy_cbd.go index 2f4d5edeb..4b954facc 100644 --- a/terraform/transform_destroy_cbd.go +++ b/terraform/transform_destroy_cbd.go @@ -169,6 +169,7 @@ func (t *CBDEdgeTransformer) Transform(g *Graph) error { applyNode := de.Source() destroyNode := de.Target() g.Connect(&DestroyEdge{S: destroyNode, T: applyNode}) + break } // If the address has an index, we strip that. Our depMap creation @@ -201,12 +202,7 @@ func (t *CBDEdgeTransformer) Transform(g *Graph) error { // They key here is that B happens before A is destroyed. This is to // facilitate the primary purpose for CBD: making sure that downstreams // are properly updated to avoid downtime before the resource is destroyed. - // - // We can't trust that the resource being destroyed or anything that - // depends on it is actually in our current graph so we make a new - // graph in order to determine those dependencies and add them in. - log.Printf("[TRACE] CBDEdgeTransformer: building graph to find dependencies...") - depMap, err := t.depMap(destroyMap) + depMap, err := t.depMap(g, destroyMap) if err != nil { return err } @@ -248,26 +244,10 @@ func (t *CBDEdgeTransformer) Transform(g *Graph) error { return nil } -func (t *CBDEdgeTransformer) depMap(destroyMap map[string][]dag.Vertex) (map[string][]dag.Vertex, error) { - // Build the graph of our config, this ensures that all resources - // are present in the graph. - g, diags := (&BasicGraphBuilder{ - Steps: []GraphTransformer{ - &FlatConfigTransformer{Config: t.Config}, - &AttachResourceConfigTransformer{Config: t.Config}, - &AttachStateTransformer{State: t.State}, - &AttachSchemaTransformer{Schemas: t.Schemas}, - &ReferenceTransformer{}, - }, - Name: "CBDEdgeTransformer", - }).Build(nil) - if diags.HasErrors() { - return nil, diags.Err() - } - - // Using this graph, build the list of destroy nodes that each resource - // address should depend on. For example, when we find B, we map the - // address of B to A_d in the "depMap" variable below. +func (t *CBDEdgeTransformer) depMap(g *Graph, destroyMap map[string][]dag.Vertex) (map[string][]dag.Vertex, error) { + // Build the list of destroy nodes that each resource address should depend + // on. For example, when we find B, we map the address of B to A_d in the + // "depMap" variable below. depMap := make(map[string][]dag.Vertex) for _, v := range g.Vertices() { // We're looking for resources. @@ -289,8 +269,10 @@ func (t *CBDEdgeTransformer) depMap(destroyMap map[string][]dag.Vertex) (map[str } // Get the nodes that depend on this on. In the example above: - // finding B in A => B. - for _, v := range g.UpEdges(v).List() { + // finding B in A => B. Since dependencies can span modules, walk all + // descendents of the resource. + des, _ := g.Descendents(v) + for _, v := range des.List() { // We're looking for resources. rn, ok := v.(GraphNodeResource) if !ok { From 470362b0511d44db071f2979009f0e20423f216e Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 26 Sep 2019 12:36:44 -0400 Subject: [PATCH 3/7] fix CBD tests to work on real data The CBDEdgeTransformer tests worked on fake data structures, with a synthetic graph, and configs that didn't match. Update them to generate a more complete graph, with real node implementations, from real configs. The output graph is filtered down to instances, and the results still functionally match the original expected test results, with some minor additions due to using the real implementation. --- terraform/graph_builder_apply.go | 15 +- .../main.tf | 11 + .../transform-cbd-destroy-edge-count/main.tf | 10 + .../transform-destroy-cbd-edge-basic/main.tf | 9 + terraform/transform_destroy_cbd.go | 7 + terraform/transform_destroy_cbd_test.go | 324 +++++++++--------- 6 files changed, 206 insertions(+), 170 deletions(-) create mode 100644 terraform/testdata/transform-cbd-destroy-edge-both-count/main.tf create mode 100644 terraform/testdata/transform-cbd-destroy-edge-count/main.tf create mode 100644 terraform/testdata/transform-destroy-cbd-edge-basic/main.tf diff --git a/terraform/graph_builder_apply.go b/terraform/graph_builder_apply.go index 9382c8aba..73b6b9a56 100644 --- a/terraform/graph_builder_apply.go +++ b/terraform/graph_builder_apply.go @@ -162,14 +162,13 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { State: b.State, Schemas: b.Schemas, }, - GraphTransformIf( - func() bool { return !b.Destroy }, - &CBDEdgeTransformer{ - Config: b.Config, - State: b.State, - Schemas: b.Schemas, - }, - ), + + &CBDEdgeTransformer{ + Config: b.Config, + State: b.State, + Schemas: b.Schemas, + Destroy: b.Destroy, + }, // Handle destroy time transformations for output and local values. // Reverse the edges from outputs and locals, so that diff --git a/terraform/testdata/transform-cbd-destroy-edge-both-count/main.tf b/terraform/testdata/transform-cbd-destroy-edge-both-count/main.tf new file mode 100644 index 000000000..c19e78eaa --- /dev/null +++ b/terraform/testdata/transform-cbd-destroy-edge-both-count/main.tf @@ -0,0 +1,11 @@ +resource "test_object" "A" { + count = 2 + lifecycle { + create_before_destroy = true + } +} + +resource "test_object" "B" { + count = 2 + test_string = test_object.A[*].test_string[count.index] +} diff --git a/terraform/testdata/transform-cbd-destroy-edge-count/main.tf b/terraform/testdata/transform-cbd-destroy-edge-count/main.tf new file mode 100644 index 000000000..775900fcd --- /dev/null +++ b/terraform/testdata/transform-cbd-destroy-edge-count/main.tf @@ -0,0 +1,10 @@ +resource "test_object" "A" { + lifecycle { + create_before_destroy = true + } +} + +resource "test_object" "B" { + count = 2 + test_string = test_object.A.test_string +} diff --git a/terraform/testdata/transform-destroy-cbd-edge-basic/main.tf b/terraform/testdata/transform-destroy-cbd-edge-basic/main.tf new file mode 100644 index 000000000..a17d8b4e3 --- /dev/null +++ b/terraform/testdata/transform-destroy-cbd-edge-basic/main.tf @@ -0,0 +1,9 @@ +resource "test_object" "A" { + lifecycle { + create_before_destroy = true + } +} + +resource "test_object" "B" { + test_string = "${test_object.A.id}" +} diff --git a/terraform/transform_destroy_cbd.go b/terraform/transform_destroy_cbd.go index 4b954facc..21b75c141 100644 --- a/terraform/transform_destroy_cbd.go +++ b/terraform/transform_destroy_cbd.go @@ -134,9 +134,16 @@ type CBDEdgeTransformer struct { // obtain schema information from providers and provisioners so we can // properly resolve implicit dependencies. Schemas *Schemas + + // If the operation is a simple destroy, no transformation is done. + Destroy bool } func (t *CBDEdgeTransformer) Transform(g *Graph) error { + if t.Destroy { + return nil + } + // Go through and reverse any destroy edges destroyMap := make(map[string][]dag.Vertex) for _, v := range g.Vertices() { diff --git a/terraform/transform_destroy_cbd_test.go b/terraform/transform_destroy_cbd_test.go index 665d81647..9d320d776 100644 --- a/terraform/transform_destroy_cbd_test.go +++ b/terraform/transform_destroy_cbd_test.go @@ -1,198 +1,198 @@ package terraform import ( + "regexp" "strings" "testing" "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/plans" ) +func cbdTestGraph(t *testing.T, mod string, changes *plans.Changes) *Graph { + module := testModule(t, mod) + + applyBuilder := &ApplyGraphBuilder{ + Config: module, + Changes: changes, + Components: simpleMockComponentFactory(), + Schemas: simpleTestSchemas(), + } + g, err := (&BasicGraphBuilder{ + Steps: cbdTestSteps(applyBuilder.Steps()), + Name: "ApplyGraphBuilder", + }).Build(addrs.RootModuleInstance) + if err != nil { + t.Fatalf("err: %s", err) + } + + return filterInstances(g) +} + +// override the apply graph builder to halt the process after CBD +func cbdTestSteps(steps []GraphTransformer) []GraphTransformer { + found := false + var i int + var t GraphTransformer + for i, t = range steps { + if _, ok := t.(*CBDEdgeTransformer); ok { + found = true + break + } + } + + if !found { + panic("CBDEdgeTransformer not found") + } + + return steps[:i+1] +} + +// remove extra nodes for easier test comparisons +func filterInstances(g *Graph) *Graph { + for _, v := range g.Vertices() { + if _, ok := v.(GraphNodeResourceInstance); !ok { + g.Remove(v) + } + + } + return g +} + func TestCBDEdgeTransformer(t *testing.T) { - g := Graph{Path: addrs.RootModuleInstance} - g.Add(&graphNodeCreatorTest{AddrString: "test_object.A"}) - g.Add(&graphNodeCreatorTest{AddrString: "test_object.B"}) - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.A", CBD: true}) - - module := testModule(t, "transform-destroy-edge-basic") - - { - tf := &DestroyEdgeTransformer{ - Config: module, - Schemas: simpleTestSchemas(), - } - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } + changes := &plans.Changes{ + Resources: []*plans.ResourceInstanceChangeSrc{ + { + Addr: mustResourceInstanceAddr("test_object.A"), + ChangeSrc: plans.ChangeSrc{ + Action: plans.CreateThenDelete, + }, + }, + { + Addr: mustResourceInstanceAddr("test_object.B"), + ChangeSrc: plans.ChangeSrc{ + Action: plans.Update, + }, + }, + }, } - { - tf := &CBDEdgeTransformer{ - Config: module, - Schemas: simpleTestSchemas(), - } - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } + g := cbdTestGraph(t, "transform-destroy-cbd-edge-basic", changes) + g = filterInstances(g) actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(testTransformCBDEdgeBasicStr) - if actual != expected { + expected := regexp.MustCompile(strings.TrimSpace(` +(?m)test_object.A +test_object.A \(destroy deposed \w+\) + test_object.A + test_object.B +test_object.B + test_object.A +`)) + + if !expected.MatchString(actual) { t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) } } -// FIXME: see if there is a worthwhile test to create from this. -// CBD is marked on created nodes during the plan phase now, and the -// CBDEdgeTransformer only takes care of the final edge reversal. -/* -func TestCBDEdgeTransformer_depNonCBD(t *testing.T) { - g := Graph{Path: addrs.RootModuleInstance} - g.Add(&graphNodeCreatorTest{AddrString: "test_object.A"}) - g.Add(&graphNodeCreatorTest{AddrString: "test_object.B"}) - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.A"}) - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.B", CBD: true}) - - module := testModule(t, "transform-destroy-edge-basic") - - { - tf := &DestroyEdgeTransformer{ - Config: module, - Schemas: simpleTestSchemas(), - } - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - { - tf := &CBDEdgeTransformer{ - Config: module, - Schemas: simpleTestSchemas(), - } - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(testTransformCBDEdgeDepNonCBDStr) - if actual != expected { - t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) - } -} -*/ - func TestCBDEdgeTransformer_depNonCBDCount(t *testing.T) { - g := Graph{Path: addrs.RootModuleInstance} - g.Add(&graphNodeCreatorTest{AddrString: "test_object.A"}) - g.Add(&graphNodeCreatorTest{AddrString: "test_object.B[0]"}) - g.Add(&graphNodeCreatorTest{AddrString: "test_object.B[1]"}) - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.A", CBD: true}) - - module := testModule(t, "transform-destroy-edge-splat") - - { - tf := &DestroyEdgeTransformer{ - Config: module, - Schemas: simpleTestSchemas(), - } - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } + changes := &plans.Changes{ + Resources: []*plans.ResourceInstanceChangeSrc{ + { + Addr: mustResourceInstanceAddr("test_object.A"), + ChangeSrc: plans.ChangeSrc{ + Action: plans.CreateThenDelete, + }, + }, + { + Addr: mustResourceInstanceAddr("test_object.B[0]"), + ChangeSrc: plans.ChangeSrc{ + Action: plans.Update, + }, + }, + { + Addr: mustResourceInstanceAddr("test_object.B[1]"), + ChangeSrc: plans.ChangeSrc{ + Action: plans.Update, + }, + }, + }, } - { - tf := &CBDEdgeTransformer{ - Config: module, - Schemas: simpleTestSchemas(), - } - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } + g := cbdTestGraph(t, "transform-cbd-destroy-edge-count", changes) actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(` -test_object.A -test_object.A (destroy) + expected := regexp.MustCompile(strings.TrimSpace(` +(?m)test_object.A +test_object.A \(destroy deposed \w+\) test_object.A - test_object.B[0] - test_object.B[1] -test_object.B[0] -test_object.B[1] - `) - if actual != expected { + test_object.B\[0\] + test_object.B\[1\] +test_object.B\[0\] + test_object.A +test_object.B\[1\] + test_object.A`)) + + if !expected.MatchString(actual) { t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) } } func TestCBDEdgeTransformer_depNonCBDCountBoth(t *testing.T) { - g := Graph{Path: addrs.RootModuleInstance} - g.Add(&graphNodeCreatorTest{AddrString: "test_object.A[0]"}) - g.Add(&graphNodeCreatorTest{AddrString: "test_object.A[1]"}) - g.Add(&graphNodeCreatorTest{AddrString: "test_object.B[0]"}) - g.Add(&graphNodeCreatorTest{AddrString: "test_object.B[1]"}) - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.A[0]", CBD: true}) - g.Add(&graphNodeDestroyerTest{AddrString: "test_object.A[1]", CBD: true}) - - module := testModule(t, "transform-destroy-edge-splat") - - { - tf := &DestroyEdgeTransformer{ - Config: module, - Schemas: simpleTestSchemas(), - } - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } + changes := &plans.Changes{ + Resources: []*plans.ResourceInstanceChangeSrc{ + { + Addr: mustResourceInstanceAddr("test_object.A[0]"), + ChangeSrc: plans.ChangeSrc{ + Action: plans.CreateThenDelete, + }, + }, + { + Addr: mustResourceInstanceAddr("test_object.A[1]"), + ChangeSrc: plans.ChangeSrc{ + Action: plans.CreateThenDelete, + }, + }, + { + Addr: mustResourceInstanceAddr("test_object.B[0]"), + ChangeSrc: plans.ChangeSrc{ + Action: plans.Update, + }, + }, + { + Addr: mustResourceInstanceAddr("test_object.B[1]"), + ChangeSrc: plans.ChangeSrc{ + Action: plans.Update, + }, + }, + }, } - { - tf := &CBDEdgeTransformer{ - Config: module, - Schemas: simpleTestSchemas(), - } - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } + g := cbdTestGraph(t, "transform-cbd-destroy-edge-both-count", changes) actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(` -test_object.A[0] -test_object.A[0] (destroy) - test_object.A[0] - test_object.B[0] - test_object.B[1] -test_object.A[1] -test_object.A[1] (destroy) - test_object.A[1] - test_object.B[0] - test_object.B[1] -test_object.B[0] -test_object.B[1] - `) - if actual != expected { + expected := regexp.MustCompile(strings.TrimSpace(` +test_object.A \(destroy deposed \w+\) + test_object.A\[0\] + test_object.A\[1\] + test_object.B\[0\] + test_object.B\[1\] +test_object.A \(destroy deposed \w+\) + test_object.A\[0\] + test_object.A\[1\] + test_object.B\[0\] + test_object.B\[1\] +test_object.A\[0\] +test_object.A\[1\] +test_object.B\[0\] + test_object.A\[0\] + test_object.A\[1\] +test_object.B\[1\] + test_object.A\[0\] + test_object.A\[1\] +`)) + + if !expected.MatchString(actual) { t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) } } - -const testTransformCBDEdgeBasicStr = ` -test_object.A -test_object.A (destroy) - test_object.A - test_object.B -test_object.B -` - -const testTransformCBDEdgeDepNonCBDStr = ` -test_object.A -test_object.A (destroy) (modified) - test_object.A - test_object.B - test_object.B (destroy) -test_object.B -test_object.B (destroy) - test_object.B -` From 3eb0e74ef91f3f046434c6b8a6fa675956b1b334 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 27 Sep 2019 16:26:33 -0400 Subject: [PATCH 4/7] failing test for module instance replace cycle Destroy-time references are not correctly or fully inverted when crossing module boundaries, causing cycle during apply. --- terraform/context_apply_test.go | 145 ++++++++++++++++++ .../apply-module-replace-cycle-cbd/main.tf | 8 + .../mod1/main.tf | 10 ++ .../mod2/main.tf | 8 + .../apply-module-replace-cycle/main.tf | 8 + .../apply-module-replace-cycle/mod1/main.tf | 10 ++ .../apply-module-replace-cycle/mod2/main.tf | 8 + 7 files changed, 197 insertions(+) create mode 100644 terraform/testdata/apply-module-replace-cycle-cbd/main.tf create mode 100644 terraform/testdata/apply-module-replace-cycle-cbd/mod1/main.tf create mode 100644 terraform/testdata/apply-module-replace-cycle-cbd/mod2/main.tf create mode 100644 terraform/testdata/apply-module-replace-cycle/main.tf create mode 100644 terraform/testdata/apply-module-replace-cycle/mod1/main.tf create mode 100644 terraform/testdata/apply-module-replace-cycle/mod2/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index c389812c1..0be3375bb 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -10602,3 +10602,148 @@ func TestContext2Apply_invalidIndexRef(t *testing.T) { t.Fatalf("missing expected error\ngot: %s\n\nwant: error containing %q", gotErr, wantErr) } } + +func TestContext2Apply_moduleReplaceCycle(t *testing.T) { + for _, mode := range []string{"normal", "cbd"} { + var m *configs.Config + + switch mode { + case "normal": + m = testModule(t, "apply-module-replace-cycle") + case "cbd": + m = testModule(t, "apply-module-replace-cycle-cbd") + } + + p := testProvider("aws") + p.DiffFn = testDiffFn + p.ApplyFn = testApplyFn + + instanceSchema := &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Computed: true}, + "require_new": {Type: cty.String, Optional: true}, + }, + } + + p.GetSchemaReturn = &ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "aws_instance": instanceSchema, + }, + } + + state := states.NewState() + modA := state.EnsureModule(addrs.RootModuleInstance.Child("a", addrs.NoKey)) + modA.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "a", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"a","require_new":"old"}`), + }, + addrs.ProviderConfig{ + Type: "aws", + }.Absolute(addrs.RootModuleInstance), + ) + + modB := state.EnsureModule(addrs.RootModuleInstance.Child("b", addrs.NoKey)) + modB.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "b", + }.Instance(addrs.IntKey(0)), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"b","require_new":"old"}`), + }, + addrs.ProviderConfig{ + Type: "aws", + }.Absolute(addrs.RootModuleInstance), + ) + + aBefore, _ := plans.NewDynamicValue( + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("a"), + "require_new": cty.StringVal("old"), + }), instanceSchema.ImpliedType()) + aAfter, _ := plans.NewDynamicValue( + cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "require_new": cty.StringVal("new"), + }), instanceSchema.ImpliedType()) + bBefore, _ := plans.NewDynamicValue( + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("b"), + "require_new": cty.StringVal("old"), + }), instanceSchema.ImpliedType()) + bAfter, _ := plans.NewDynamicValue( + cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "require_new": cty.UnknownVal(cty.String), + }), instanceSchema.ImpliedType()) + + var aAction plans.Action + switch mode { + case "normal": + aAction = plans.DeleteThenCreate + case "cbd": + aAction = plans.CreateThenDelete + } + + changes := &plans.Changes{ + Resources: []*plans.ResourceInstanceChangeSrc{ + { + Addr: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "a", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance.Child("a", addrs.NoKey)), + ProviderAddr: addrs.ProviderConfig{ + Type: "aws", + }.Absolute(addrs.RootModuleInstance), + ChangeSrc: plans.ChangeSrc{ + Action: aAction, + Before: aBefore, + After: aAfter, + }, + }, + { + Addr: addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "aws_instance", + Name: "b", + }.Instance(addrs.IntKey(0)).Absolute(addrs.RootModuleInstance.Child("b", addrs.NoKey)), + ProviderAddr: addrs.ProviderConfig{ + Type: "aws", + }.Absolute(addrs.RootModuleInstance), + ChangeSrc: plans.ChangeSrc{ + Action: plans.DeleteThenCreate, + Before: bBefore, + After: bAfter, + }, + }, + }, + } + + ctx := testContext2(t, &ContextOpts{ + Config: m, + ProviderResolver: providers.ResolverFixed( + map[string]providers.Factory{ + "aws": testProviderFuncFixed(p), + }, + ), + State: state, + Changes: changes, + }) + + t.Run(mode, func(t *testing.T) { + _, diags := ctx.Apply() + if diags.HasErrors() { + t.Fatal(diags.Err()) + } + }) + } +} diff --git a/terraform/testdata/apply-module-replace-cycle-cbd/main.tf b/terraform/testdata/apply-module-replace-cycle-cbd/main.tf new file mode 100644 index 000000000..6393231d6 --- /dev/null +++ b/terraform/testdata/apply-module-replace-cycle-cbd/main.tf @@ -0,0 +1,8 @@ +module "a" { + source = "./mod1" +} + +module "b" { + source = "./mod2" + ids = module.a.ids +} diff --git a/terraform/testdata/apply-module-replace-cycle-cbd/mod1/main.tf b/terraform/testdata/apply-module-replace-cycle-cbd/mod1/main.tf new file mode 100644 index 000000000..2ade442bf --- /dev/null +++ b/terraform/testdata/apply-module-replace-cycle-cbd/mod1/main.tf @@ -0,0 +1,10 @@ +resource "aws_instance" "a" { + require_new = "new" + lifecycle { + create_before_destroy = true + } +} + +output "ids" { + value = [aws_instance.a.id] +} diff --git a/terraform/testdata/apply-module-replace-cycle-cbd/mod2/main.tf b/terraform/testdata/apply-module-replace-cycle-cbd/mod2/main.tf new file mode 100644 index 000000000..83fb1dcd4 --- /dev/null +++ b/terraform/testdata/apply-module-replace-cycle-cbd/mod2/main.tf @@ -0,0 +1,8 @@ +resource "aws_instance" "b" { + count = length(var.ids) + require_new = var.ids[count.index] +} + +variable "ids" { + type = list(string) +} diff --git a/terraform/testdata/apply-module-replace-cycle/main.tf b/terraform/testdata/apply-module-replace-cycle/main.tf new file mode 100644 index 000000000..6393231d6 --- /dev/null +++ b/terraform/testdata/apply-module-replace-cycle/main.tf @@ -0,0 +1,8 @@ +module "a" { + source = "./mod1" +} + +module "b" { + source = "./mod2" + ids = module.a.ids +} diff --git a/terraform/testdata/apply-module-replace-cycle/mod1/main.tf b/terraform/testdata/apply-module-replace-cycle/mod1/main.tf new file mode 100644 index 000000000..2ade442bf --- /dev/null +++ b/terraform/testdata/apply-module-replace-cycle/mod1/main.tf @@ -0,0 +1,10 @@ +resource "aws_instance" "a" { + require_new = "new" + lifecycle { + create_before_destroy = true + } +} + +output "ids" { + value = [aws_instance.a.id] +} diff --git a/terraform/testdata/apply-module-replace-cycle/mod2/main.tf b/terraform/testdata/apply-module-replace-cycle/mod2/main.tf new file mode 100644 index 000000000..83fb1dcd4 --- /dev/null +++ b/terraform/testdata/apply-module-replace-cycle/mod2/main.tf @@ -0,0 +1,8 @@ +resource "aws_instance" "b" { + count = length(var.ids) + require_new = var.ids[count.index] +} + +variable "ids" { + type = list(string) +} From eb6e7bba2ec59c3751256b4783c30f8622204fe8 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 27 Sep 2019 21:47:03 -0400 Subject: [PATCH 5/7] do not connect destroy and resource nodes Destroy nodes do not need to be connected to the resource (prepare state) node when adding them to the graph. Destroy nodes already have a complete state in the graph (which is being destroyed), any references will be added in the ReferenceTransformer, and the proper connection to the create node will be added in the DestroyEdgeTransformer. Under normal circumstances this makes no difference, as create and destroy nodes always have an dependency, so having the prepare state handled before both only linearizes the operation slightly in the normal destroy-then-create scenario. However if there is a dependency on a resource being replaced in another module, there will be a dependency between the destroy nodes in each module (to complete the destroy ordering), while the resource node will depend on the variable->output->resource chain. If both the destroy and create nodes depend on the resource node, there will be a cycle. --- terraform/graph_builder_apply_test.go | 3 ++- terraform/transform_diff.go | 8 -------- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index ac65751e6..c0f987423 100644 --- a/terraform/graph_builder_apply_test.go +++ b/terraform/graph_builder_apply_test.go @@ -522,8 +522,9 @@ root test_object.A (prepare state) provider.test test_object.A[1] (destroy) - test_object.A (prepare state) + provider.test test_object.B + test_object.A (prepare state) test_object.A[1] (destroy) test_object.B (prepare state) test_object.B (prepare state) diff --git a/terraform/transform_diff.go b/terraform/transform_diff.go index 6fb915f87..23b6e2a75 100644 --- a/terraform/transform_diff.go +++ b/terraform/transform_diff.go @@ -174,14 +174,6 @@ func (t *DiffTransformer) Transform(g *Graph) error { log.Printf("[TRACE] DiffTransformer: %s deposed object %s will be represented for destruction by %s", addr, dk, dag.VertexName(node)) } g.Add(node) - rsrcAddr := addr.ContainingResource().String() - for _, rsrcNode := range resourceNodes[rsrcAddr] { - // We connect this edge "forwards" (even though destroy dependencies - // are often inverted) because evaluating the resource node - // after the destroy node could cause an unnecessary husk of - // a resource state to be re-added. - g.Connect(dag.BasicEdge(node, rsrcNode)) - } } } From 710856022870882fe31efc7acccb4fe189f8fb9d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 2 Oct 2019 11:55:27 -0400 Subject: [PATCH 6/7] failing test with destroy cycle after the previous commit the cycle is broken, but we can't evaluate resource counts that depends on data sources being destroyed. --- terraform/context_apply_test.go | 77 +++++++++++++++++++ .../testdata/apply-destroy-data-cycle/main.tf | 10 +++ 2 files changed, 87 insertions(+) create mode 100644 terraform/testdata/apply-destroy-data-cycle/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 0be3375bb..902e0dd5a 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -10747,3 +10747,80 @@ func TestContext2Apply_moduleReplaceCycle(t *testing.T) { }) } } + +func TestContext2Apply_destroyDataCycle(t *testing.T) { + m, snap := testModuleWithSnapshot(t, "apply-destroy-data-cycle") + p := testProvider("null") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + + state := states.NewState() + root := state.EnsureModule(addrs.RootModuleInstance) + root.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "null_resource", + Name: "a", + }.Instance(addrs.IntKey(0)), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"a"}`), + }, + addrs.ProviderConfig{ + Type: "null", + }.Absolute(addrs.RootModuleInstance), + ) + root.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.DataResourceMode, + Type: "null_data_source", + Name: "d", + }.Instance(addrs.NoKey), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"data"}`), + }, + addrs.ProviderConfig{ + Type: "null", + }.Absolute(addrs.RootModuleInstance), + ) + + providerResolver := providers.ResolverFixed( + map[string]providers.Factory{ + "null": testProviderFuncFixed(p), + }, + ) + + hook := &testHook{} + ctx := testContext2(t, &ContextOpts{ + Config: m, + ProviderResolver: providerResolver, + State: state, + Destroy: true, + Hooks: []Hook{hook}, + }) + + plan, diags := ctx.Plan() + diags.HasErrors() + if diags.HasErrors() { + t.Fatalf("diags: %s", diags.Err()) + } + + // We'll marshal and unmarshal the plan here, to ensure that we have + // a clean new context as would be created if we separately ran + // terraform plan -out=tfplan && terraform apply tfplan + ctxOpts, err := contextOptsForPlanViaFile(snap, state, plan) + if err != nil { + t.Fatal(err) + } + ctxOpts.ProviderResolver = providerResolver + ctx, diags = NewContext(ctxOpts) + if diags.HasErrors() { + t.Fatalf("failed to create context for plan: %s", diags.Err()) + } + + _, diags = ctx.Apply() + if diags.HasErrors() { + t.Fatalf("diags: %s", diags.Err()) + } +} diff --git a/terraform/testdata/apply-destroy-data-cycle/main.tf b/terraform/testdata/apply-destroy-data-cycle/main.tf new file mode 100644 index 000000000..bd72a47e3 --- /dev/null +++ b/terraform/testdata/apply-destroy-data-cycle/main.tf @@ -0,0 +1,10 @@ +locals { + l = data.null_data_source.d.id +} + +data "null_data_source" "d" { +} + +resource "null_resource" "a" { + count = local.l == "NONE" ? 1 : 0 +} From f766bb8380e370502b1c2c0d3ef75396811d159c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Sat, 28 Sep 2019 20:34:55 -0400 Subject: [PATCH 7/7] prune unused resources from apply If a resource is only destroying instances, there is no reason to prepare the state and we can remove the Resource (prepare state) nodes. They normally have pose no issue, but if the instances are being destroyed along with their dependencies, the resource node may fail to evaluate due to the missing dependencies (since destroy happens in the reverse order). These failures were previously blocked by there being a cycle when the destroy nodes were directly attached to the resource nodes. --- terraform/transform_destroy_edge.go | 42 +++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index 7fb415bdf..be1540ffe 100644 --- a/terraform/transform_destroy_edge.go +++ b/terraform/transform_destroy_edge.go @@ -277,5 +277,47 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error { } } + return t.pruneResources(g) +} + +// If there are only destroy instances for a particular resource, there's no +// reason for the resource node to prepare the state. Remove Resource nodes so +// that they don't fail by trying to evaluate a resource that is only being +// destroyed along with its dependencies. +func (t *DestroyEdgeTransformer) pruneResources(g *Graph) error { + for _, v := range g.Vertices() { + n, ok := v.(*NodeApplyableResource) + if !ok { + continue + } + + // if there are only destroy dependencies, we don't need this node + des, err := g.Descendents(n) + if err != nil { + return err + } + + descendents := des.List() + nonDestroyInstanceFound := false + for _, v := range descendents { + if _, ok := v.(*NodeApplyableResourceInstance); ok { + nonDestroyInstanceFound = true + break + } + } + + if nonDestroyInstanceFound { + continue + } + + // connect all the through-edges, then delete the node + for _, d := range g.DownEdges(n).List() { + for _, u := range g.UpEdges(n).List() { + g.Connect(dag.BasicEdge(u, d)) + } + } + log.Printf("DestroyEdgeTransformer: pruning unused resource node %s", dag.VertexName(n)) + g.Remove(n) + } return nil }