From f3a62c694d2b0dabe0b261f867b5b8237f4a2579 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 2 Dec 2016 09:44:55 -0500 Subject: [PATCH 1/2] terraform: when promoting non-CBD to CBD, mark the config as such Fixes #10439 When a CBD resource depends on a non-CBD resource, the non-CBD resource is auto-promoted to CBD. This was done in cf3a259. This PR makes it so that we also set the config CBD to true. This causes the proper runtime execution behavior to occur where we depose state and so on. So in addition to simple graph edge tricks we also treat the non-CBD resources as CBD resources. --- terraform/context_apply_test.go | 67 +++++++++++++++++++ .../apply-cbd-depends-non-cbd/main.tf | 12 ++++ terraform/transform_destroy.go | 29 +++++++- 3 files changed, 105 insertions(+), 3 deletions(-) create mode 100644 terraform/test-fixtures/apply-cbd-depends-non-cbd/main.tf diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index f933301bc..c2a5674bf 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -881,6 +881,73 @@ func TestContext2Apply_createBeforeDestroyUpdate(t *testing.T) { } } +// This tests that when a CBD resource depends on a non-CBD resource, +// we can still properly apply changes that require new for both. +func TestContext2Apply_createBeforeDestroy_dependsNonCBD(t *testing.T) { + m := testModule(t, "apply-cbd-depends-non-cbd") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + state := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Resources: map[string]*ResourceState{ + "aws_instance.bar": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "bar", + Attributes: map[string]string{ + "require_new": "abc", + }, + }, + }, + + "aws_instance.foo": &ResourceState{ + Type: "aws_instance", + Primary: &InstanceState{ + ID: "foo", + Attributes: map[string]string{ + "require_new": "abc", + }, + }, + }, + }, + }, + }, + } + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + State: state, + }) + + if p, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } else { + t.Logf(p.String()) + } + + state, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + checkStateString(t, state, ` +aws_instance.bar: + ID = foo + require_new = yes + type = aws_instance + value = foo +aws_instance.foo: + ID = foo + require_new = yes + type = aws_instance + `) +} + func TestContext2Apply_createBeforeDestroy_hook(t *testing.T) { h := new(MockHook) m := testModule(t, "apply-good-create-before") diff --git a/terraform/test-fixtures/apply-cbd-depends-non-cbd/main.tf b/terraform/test-fixtures/apply-cbd-depends-non-cbd/main.tf new file mode 100644 index 000000000..6ba1b983f --- /dev/null +++ b/terraform/test-fixtures/apply-cbd-depends-non-cbd/main.tf @@ -0,0 +1,12 @@ +resource "aws_instance" "foo" { + require_new = "yes" +} + +resource "aws_instance" "bar" { + require_new = "yes" + value = "${aws_instance.foo.id}" + + lifecycle { + create_before_destroy = true + } +} diff --git a/terraform/transform_destroy.go b/terraform/transform_destroy.go index 47c42c0da..64e295805 100644 --- a/terraform/transform_destroy.go +++ b/terraform/transform_destroy.go @@ -1,6 +1,9 @@ package terraform -import "github.com/hashicorp/terraform/dag" +import ( + "github.com/hashicorp/terraform/config" + "github.com/hashicorp/terraform/dag" +) // GraphNodeDestroyable is the interface that nodes that can be destroyed // must implement. This is used to automatically handle the creation of @@ -151,8 +154,28 @@ func (t *CreateBeforeDestroyTransformer) Transform(g *Graph) error { } // If the node doesn't need to create before destroy, then continue - if !dn.CreateBeforeDestroy() && noCreateBeforeDestroyAncestors(g, dn) { - continue + if !dn.CreateBeforeDestroy() { + if noCreateBeforeDestroyAncestors(g, dn) { + continue + } + + // PURPOSELY HACKY FIX SINCE THIS TRANSFORM IS DEPRECATED. + // This is a hacky way to fix GH-10439. For a detailed description + // of the fix, see CBDEdgeTransformer, which is the equivalent + // transform used by the new graphs. + // + // This transform is deprecated because it is only used by the + // old graphs which are going to be removed. + var update *config.Resource + if dn, ok := v.(*graphNodeResourceDestroy); ok { + update = dn.Original.Resource + } + if dn, ok := v.(*graphNodeResourceDestroyFlat); ok { + update = dn.Original.Resource + } + if update != nil { + update.Lifecycle.CreateBeforeDestroy = true + } } // Get the creation side of this node From 95239a7fe62ce8f2adaf59798d48a6e0a21a12b6 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 2 Dec 2016 09:46:42 -0500 Subject: [PATCH 2/2] terraform: when promoting non-CBD to CBD, mark the config as such This brings the change for the new graph. See #10455 --- terraform/node_resource_destroy.go | 14 +++++++ terraform/transform_destroy_cbd.go | 47 +++++++++++++++++++++++- terraform/transform_destroy_cbd_test.go | 43 ++++++++++++++++++++++ terraform/transform_destroy_edge_test.go | 17 ++++++++- 4 files changed, 119 insertions(+), 2 deletions(-) diff --git a/terraform/node_resource_destroy.go b/terraform/node_resource_destroy.go index 23652200d..6b4a48b6b 100644 --- a/terraform/node_resource_destroy.go +++ b/terraform/node_resource_destroy.go @@ -30,6 +30,20 @@ func (n *NodeDestroyResource) CreateBeforeDestroy() bool { return n.Config.Lifecycle.CreateBeforeDestroy } +// GraphNodeDestroyerCBD +func (n *NodeDestroyResource) ModifyCreateBeforeDestroy(v bool) error { + // If we have no config, do nothing since it won't affect the + // create step anyways. + if n.Config == nil { + return nil + } + + // Set CBD to true + n.Config.Lifecycle.CreateBeforeDestroy = true + + return nil +} + // GraphNodeReferenceable, overriding NodeAbstractResource func (n *NodeDestroyResource) ReferenceableName() []string { result := n.NodeAbstractResource.ReferenceableName() diff --git a/terraform/transform_destroy_cbd.go b/terraform/transform_destroy_cbd.go index 4b3f45b1f..e1a525b88 100644 --- a/terraform/transform_destroy_cbd.go +++ b/terraform/transform_destroy_cbd.go @@ -1,6 +1,7 @@ package terraform import ( + "fmt" "log" "github.com/hashicorp/terraform/config/module" @@ -15,6 +16,11 @@ type GraphNodeDestroyerCBD interface { // CreateBeforeDestroy returns true if this node represents a node // that is doing a CBD. CreateBeforeDestroy() bool + + // ModifyCreateBeforeDestroy is called when the CBD state of a node + // is changed dynamically. This can return an error if this isn't + // allowed. + ModifyCreateBeforeDestroy(bool) error } // CBDEdgeTransformer modifies the edges of CBD nodes that went through @@ -48,7 +54,23 @@ func (t *CBDEdgeTransformer) Transform(g *Graph) error { } if !dn.CreateBeforeDestroy() { - continue + // If there are no CBD ancestors (dependent nodes), then we + // do nothing here. + if !t.hasCBDAncestor(g, v) { + continue + } + + // If this isn't naturally a CBD node, this means that an ancestor is + // and we need to auto-upgrade this node to CBD. We do this because + // a CBD node depending on non-CBD will result in cycles. To avoid this, + // we always attempt to upgrade it. + if err := dn.ModifyCreateBeforeDestroy(true); err != nil { + return fmt.Errorf( + "%s: must have create before destroy enabled because "+ + "a dependent resource has CBD enabled. However, when "+ + "attempting to automatically do this, an error occurred: %s", + dag.VertexName(v), err) + } } // Find the destroy edge. There should only be one. @@ -189,3 +211,26 @@ func (t *CBDEdgeTransformer) depMap( return depMap, nil } + +// hasCBDAncestor returns true if any ancestor (node that depends on this) +// has CBD set. +func (t *CBDEdgeTransformer) hasCBDAncestor(g *Graph, v dag.Vertex) bool { + s, _ := g.Ancestors(v) + if s == nil { + return true + } + + for _, v := range s.List() { + dn, ok := v.(GraphNodeDestroyerCBD) + if !ok { + continue + } + + if dn.CreateBeforeDestroy() { + // some ancestor is CreateBeforeDestroy, so we need to follow suit + return true + } + } + + return false +} diff --git a/terraform/transform_destroy_cbd_test.go b/terraform/transform_destroy_cbd_test.go index dad1d27bb..bb8fc24f8 100644 --- a/terraform/transform_destroy_cbd_test.go +++ b/terraform/transform_destroy_cbd_test.go @@ -36,6 +36,38 @@ func TestCBDEdgeTransformer(t *testing.T) { } } +func TestCBDEdgeTransformer_depNonCBD(t *testing.T) { + g := Graph{Path: RootModulePath} + g.Add(&graphNodeCreatorTest{AddrString: "test.A"}) + g.Add(&graphNodeCreatorTest{AddrString: "test.B"}) + g.Add(&graphNodeDestroyerTest{AddrString: "test.A"}) + g.Add(&graphNodeDestroyerTest{AddrString: "test.B", CBD: true}) + + module := testModule(t, "transform-destroy-edge-basic") + + { + tf := &DestroyEdgeTransformer{ + Module: module, + } + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + { + tf := &CBDEdgeTransformer{Module: module} + 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("bad:\n\n%s", actual) + } +} + const testTransformCBDEdgeBasicStr = ` test.A test.A (destroy) @@ -43,3 +75,14 @@ test.A (destroy) test.B test.B ` + +const testTransformCBDEdgeDepNonCBDStr = ` +test.A +test.A (destroy) (modified) + test.A + test.B + test.B (destroy) +test.B +test.B (destroy) + test.B +` diff --git a/terraform/transform_destroy_edge_test.go b/terraform/transform_destroy_edge_test.go index 873b01eac..cf8e4c704 100644 --- a/terraform/transform_destroy_edge_test.go +++ b/terraform/transform_destroy_edge_test.go @@ -113,10 +113,25 @@ func (n *graphNodeCreatorTest) CreateAddr() *ResourceAddress { type graphNodeDestroyerTest struct { AddrString string CBD bool + Modified bool +} + +func (n *graphNodeDestroyerTest) Name() string { + result := n.DestroyAddr().String() + " (destroy)" + if n.Modified { + result += " (modified)" + } + + return result } -func (n *graphNodeDestroyerTest) Name() string { return n.DestroyAddr().String() + " (destroy)" } func (n *graphNodeDestroyerTest) CreateBeforeDestroy() bool { return n.CBD } + +func (n *graphNodeDestroyerTest) ModifyCreateBeforeDestroy(v bool) error { + n.Modified = true + return nil +} + func (n *graphNodeDestroyerTest) DestroyAddr() *ResourceAddress { addr, err := ParseResourceAddress(n.AddrString) if err != nil {