Merge pull request #26263 from hashicorp/jbardin/cbd-from-state

always check the CreateBeforeDestroy value from state

terraform/context_apply_test.go

@@ -11390,18 +11390,29 @@ variable "ct" {
 
 resource "test_instance" "a" {
   count = var.ct
+}
+
+resource "test_instance" "b" {
+  require_new = local.removable
   lifecycle {
 	create_before_destroy = true
   }
 }
 
-resource "test_instance" "b" {
-  foo = join(".", test_instance.a[*].id)
+resource "test_instance" "c" {
+  require_new = test_instance.b.id
+  lifecycle {
+	create_before_destroy = true
+  }
 }
 
 output "out" {
   value = join(".", test_instance.a[*].id)
 }
+
+locals {
+  removable = join(".", test_instance.a[*].id)
+}
 `})
 
 	state := states.NewState()
@@ -11409,27 +11420,43 @@ output "out" {
 	root.SetResourceInstanceCurrent(
 		mustResourceInstanceAddr("test_instance.a[0]").Resource,
 		&states.ResourceInstanceObjectSrc{
-			Status:       states.ObjectReady,
-			AttrsJSON:    []byte(`{"id":"a0"}`),
-			Dependencies: []addrs.ConfigResource{mustResourceAddr("module.child.aws_instance.child")},
+			Status:              states.ObjectReady,
+			AttrsJSON:           []byte(`{"id":"a0"}`),
+			Dependencies:        []addrs.ConfigResource{},
+			CreateBeforeDestroy: true,
 		},
 		mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
 	)
 	root.SetResourceInstanceCurrent(
 		mustResourceInstanceAddr("test_instance.a[1]").Resource,
 		&states.ResourceInstanceObjectSrc{
-			Status:       states.ObjectReady,
-			AttrsJSON:    []byte(`{"id":"a1"}`),
-			Dependencies: []addrs.ConfigResource{mustResourceAddr("module.child.aws_instance.child")},
+			Status:              states.ObjectReady,
+			AttrsJSON:           []byte(`{"id":"a1"}`),
+			Dependencies:        []addrs.ConfigResource{},
+			CreateBeforeDestroy: true,
 		},
 		mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
 	)
 	root.SetResourceInstanceCurrent(
 		mustResourceInstanceAddr("test_instance.b").Resource,
 		&states.ResourceInstanceObjectSrc{
-			Status:       states.ObjectReady,
-			AttrsJSON:    []byte(`{"id":"b", "foo":"old.old"}`),
-			Dependencies: []addrs.ConfigResource{mustResourceAddr("test_instance.a")},
+			Status:              states.ObjectReady,
+			AttrsJSON:           []byte(`{"id":"b", "require_new":"old.old"}`),
+			Dependencies:        []addrs.ConfigResource{mustResourceAddr("test_instance.a")},
+			CreateBeforeDestroy: true,
+		},
+		mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
+	)
+	root.SetResourceInstanceCurrent(
+		mustResourceInstanceAddr("test_instance.c").Resource,
+		&states.ResourceInstanceObjectSrc{
+			Status:    states.ObjectReady,
+			AttrsJSON: []byte(`{"id":"c", "require_new":"b"}`),
+			Dependencies: []addrs.ConfigResource{
+				mustResourceAddr("test_instance.a"),
+				mustResourceAddr("test_instance.b"),
+			},
+			CreateBeforeDestroy: true,
 		},
 		mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
 	)
@@ -11443,7 +11470,7 @@ output "out" {
 	ctx := testContext2(t, &ContextOpts{
 		Variables: InputValues{
 			"ct": &InputValue{
-				Value:      cty.NumberIntVal(1),
+				Value:      cty.NumberIntVal(0),
 				SourceType: ValueFromCaller,
 			},
 		},
@@ -11462,6 +11489,11 @@ output "out" {
 	// if resource b isn't going to apply correctly, we will get an error about
 	// an invalid plan value
 	state, diags = ctx.Apply()
+	errMsg := diags.ErrWithWarnings().Error()
+	if strings.Contains(errMsg, "Cycle") {
+		t.Fatal("test should not produce a cycle:\n", errMsg)
+	}
+
 	if !diags.HasErrors() {
 		// FIXME: this test is correct, but needs to wait until we no longer
 		// evaluate resourced that are pending destruction.

terraform/node_resource_apply_instance.go

@@ -21,7 +21,6 @@ import (
 type NodeApplyableResourceInstance struct {
 	*NodeAbstractResourceInstance
 
-	destroyNode      GraphNodeDestroyerCBD
 	graphNodeDeposer // implementation of GraphNodeDeposerConfig
 
 	// If this node is forced to be CreateBeforeDestroy, we need to record that
@@ -39,13 +38,7 @@ var (
 	_ GraphNodeAttachDependencies = (*NodeApplyableResourceInstance)(nil)
 )
 
-// GraphNodeAttachDestroyer
-func (n *NodeApplyableResourceInstance) AttachDestroyNode(d GraphNodeDestroyerCBD) {
-	n.destroyNode = d
-}
-
-// CreateBeforeDestroy checks this nodes config status and the status af any
-// companion destroy node for CreateBeforeDestroy.
+// CreateBeforeDestroy returns this node's CreateBeforeDestroy status.
 func (n *NodeApplyableResourceInstance) CreateBeforeDestroy() bool {
 	if n.ForceCreateBeforeDestroy {
 		return n.ForceCreateBeforeDestroy
@@ -55,10 +48,6 @@ func (n *NodeApplyableResourceInstance) CreateBeforeDestroy() bool {
 		return n.Config.Managed.CreateBeforeDestroy
 	}
 
-	if n.destroyNode != nil {
-		return n.destroyNode.CreateBeforeDestroy()
-	}
-
 	return false
 }
 

terraform/node_resource_destroy.go

@@ -21,8 +21,6 @@ type NodeDestroyResourceInstance struct {
 	// this node destroys a deposed object of the associated instance
 	// rather than its current object.
 	DeposedKey states.DeposedKey
-
-	CreateBeforeDestroyOverride *bool
 }
 
 var (
@@ -53,28 +51,25 @@ func (n *NodeDestroyResourceInstance) DestroyAddr() *addrs.AbsResourceInstance {
 
 // GraphNodeDestroyerCBD
 func (n *NodeDestroyResourceInstance) CreateBeforeDestroy() bool {
-	if n.CreateBeforeDestroyOverride != nil {
-		return *n.CreateBeforeDestroyOverride
-	}
-
-	// Config takes precedence
-	if n.Config != nil && n.Config.Managed != nil {
-		return n.Config.Managed.CreateBeforeDestroy
-	}
-
-	// Otherwise check the state for a stored destroy order
+	// State takes precedence during destroy.
+	// If the resource was removed, there is no config to check.
+	// If CBD was forced from descendent, it should be saved in the state
+	// already.
 	if s := n.instanceState; s != nil {
 		if s.Current != nil {
 			return s.Current.CreateBeforeDestroy
 		}
 	}
 
+	if n.Config != nil && n.Config.Managed != nil {
+		return n.Config.Managed.CreateBeforeDestroy
+	}
+
 	return false
 }
 
 // GraphNodeDestroyerCBD
 func (n *NodeDestroyResourceInstance) ModifyCreateBeforeDestroy(v bool) error {
-	n.CreateBeforeDestroyOverride = &v
 	return nil
 }
 

terraform/transform_destroy_cbd.go

@@ -23,20 +23,6 @@ type GraphNodeDestroyerCBD interface {
 	ModifyCreateBeforeDestroy(bool) error
 }
 
-// GraphNodeAttachDestroyer is implemented by applyable nodes that have a
-// companion destroy node. This allows the creation node to look up the status
-// of the destroy node and determine if it needs to depose the existing state,
-// or replace it.
-// If a node is not marked as create-before-destroy in the configuration, but a
-// dependency forces that status, only the destroy node will be aware of that
-// status.
-type GraphNodeAttachDestroyer interface {
-	// AttachDestroyNode takes a destroy node and saves a reference to that
-	// node in the receiver, so it can later check the status of
-	// CreateBeforeDestroy().
-	AttachDestroyNode(n GraphNodeDestroyerCBD)
-}
-
 // ForcedCBDTransformer detects when a particular CBD-able graph node has
 // dependencies with another that has create_before_destroy set that require
 // it to be forced on, and forces it on.

terraform/transform_destroy_edge.go

@@ -157,16 +157,6 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 				dag.VertexName(a), dag.VertexName(a_d))
 
 			g.Connect(dag.BasicEdge(a, a_d))
-
-			// Attach the destroy node to the creator
-			// There really shouldn't be more than one destroyer, but even if
-			// there are, any of them will represent the correct
-			// CreateBeforeDestroy status.
-			if n, ok := cn.(GraphNodeAttachDestroyer); ok {
-				if d, ok := d.(GraphNodeDestroyerCBD); ok {
-					n.AttachDestroyNode(d)
-				}
-			}
 		}
 	}