Merge pull request #10455 from hashicorp/b-non-cbd-promote

terraform: when promoting non-CBD to CBD, mark the config as such

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")

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()

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
+  }
+}

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

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
+}

terraform/transform_destroy_cbd_test.go

@@ -36,10 +36,53 @@ 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)
   test.A
   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
+`

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 {