From e4a360630406323ffe8ac1c51e374ae1e934fe23 Mon Sep 17 00:00:00 2001 From: Alex Pilon Date: Tue, 29 Oct 2019 17:32:03 -0400 Subject: [PATCH] Merge pull request #22937 from hashicorp/jbardin/cbd-modules This cherry pick depends on hashicorp/terraform#22846 create_before_destroy across modules --- terraform/context_apply_test.go | 222 +++++++++++++ terraform/graph_builder_apply.go | 29 +- terraform/graph_builder_apply_test.go | 3 +- .../testdata/apply-destroy-data-cycle/main.tf | 10 + .../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 + .../main.tf | 11 + .../transform-cbd-destroy-edge-count/main.tf | 10 + .../transform-destroy-cbd-edge-basic/main.tf | 9 + terraform/transform_config_flat.go | 71 ----- terraform/transform_config_flat_test.go | 42 --- terraform/transform_destroy_cbd.go | 45 +-- terraform/transform_destroy_cbd_test.go | 293 +++++++++--------- terraform/transform_destroy_edge.go | 42 +++ terraform/transform_diff.go | 8 - 19 files changed, 541 insertions(+), 306 deletions(-) create mode 100644 terraform/testdata/apply-destroy-data-cycle/main.tf 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 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 delete mode 100644 terraform/transform_config_flat.go delete mode 100644 terraform/transform_config_flat_test.go diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index e231386e548..53f78f760f7 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -10513,3 +10513,225 @@ 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()) + } + }) + } +} + +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/graph_builder_apply.go b/terraform/graph_builder_apply.go index a4041d13376..91898761065 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,20 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer { // Connect references so ordering is correct &ReferenceTransformer{}, + // Destruction ordering + &DestroyEdgeTransformer{ + 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 // interpolations don't fail during destroy. diff --git a/terraform/graph_builder_apply_test.go b/terraform/graph_builder_apply_test.go index 91206fcc8d1..b85cd4c3297 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/testdata/apply-destroy-data-cycle/main.tf b/terraform/testdata/apply-destroy-data-cycle/main.tf new file mode 100644 index 00000000000..bd72a47e3a0 --- /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 +} 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 00000000000..6393231d685 --- /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 00000000000..2ade442bfd3 --- /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 00000000000..83fb1dcd467 --- /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 00000000000..6393231d685 --- /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 00000000000..2ade442bfd3 --- /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 00000000000..83fb1dcd467 --- /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) +} 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 00000000000..c19e78eaa2f --- /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 00000000000..775900fcdd8 --- /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 00000000000..a17d8b4e35c --- /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_config_flat.go b/terraform/transform_config_flat.go deleted file mode 100644 index 4dbdcb7424c..00000000000 --- a/terraform/transform_config_flat.go +++ /dev/null @@ -1,71 +0,0 @@ -package terraform - -import ( - "github.com/hashicorp/terraform-plugin-sdk/internal/configs" - "github.com/hashicorp/terraform-plugin-sdk/internal/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 2dcd57b253e..00000000000 --- a/terraform/transform_config_flat_test.go +++ /dev/null @@ -1,42 +0,0 @@ -package terraform - -import ( - "strings" - "testing" - - "github.com/hashicorp/terraform-plugin-sdk/internal/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 44c606407d4..98e088eee7e 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() { @@ -169,6 +176,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 +209,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 +251,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 +276,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 { diff --git a/terraform/transform_destroy_cbd_test.go b/terraform/transform_destroy_cbd_test.go index edb1184643e..53695c0b392 100644 --- a/terraform/transform_destroy_cbd_test.go +++ b/terraform/transform_destroy_cbd_test.go @@ -1,187 +1,198 @@ package terraform import ( + "regexp" "strings" "testing" "github.com/hashicorp/terraform-plugin-sdk/internal/addrs" + "github.com/hashicorp/terraform-plugin-sdk/internal/plans" ) -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") +func cbdTestGraph(t *testing.T, mod string, changes *plans.Changes) *Graph { + module := testModule(t, mod) - { - tf := &DestroyEdgeTransformer{ - Config: module, - Schemas: simpleTestSchemas(), - } - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } + 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) } - { - tf := &CBDEdgeTransformer{ - Config: module, - Schemas: simpleTestSchemas(), - } - if err := tf.Transform(&g); 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 } } - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(testTransformCBDEdgeBasicStr) - if actual != expected { - t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) + if !found { + panic("CBDEdgeTransformer not found") } + + return steps[:i+1] } -// 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) +// 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 +} - { - tf := &CBDEdgeTransformer{ - Config: module, - Schemas: simpleTestSchemas(), - } - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } +func TestCBDEdgeTransformer(t *testing.T) { + 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, + }, + }, + }, } + g := cbdTestGraph(t, "transform-destroy-cbd-edge-basic", changes) + g = filterInstances(g) + actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(testTransformCBDEdgeDepNonCBDStr) - 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) } } -*/ 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 -` diff --git a/terraform/transform_destroy_edge.go b/terraform/transform_destroy_edge.go index ab4ba156dfc..1d211570fc1 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 } diff --git a/terraform/transform_diff.go b/terraform/transform_diff.go index 773aad7c404..b7a237fce36 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)) - } } }