diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 27f89da18..1601a0acf 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/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/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 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 {