make sure apply nodes are GraphNodeDestroyerCBD

If an instance was forced to be CreateBeforeDestroy due to a dependent,
and that dependent had no changes to apply, the dependent would not be
in the graph to force the CBD status on the change, and the result would
be lost from the state.

This rarely made any difference, because the status would be restored
during the next plan. However if the resource was immediately removed
from the configuration, the incorrect state would be the only source of
the destroy order, which could result in a cycle during the next apply.

While it would be better to use the destroy order calculated during
plan, when there is a newly created object its plan state is not
stored because the instance has a null state value. This means we still
need to recompute the CBD status again during apply until a new way to
transfer the information from plan to apply is developed.

While instances without changes are not present in the apply graph,
their resource expansion nodes do happen to be there and hold the
configuration (and while they were previously an implementation quirk of
the expansion system, they now play an important role in the ephemeral
resource evaluation). Those resource expansion nodes didn't implement
the `GraphNodeDestroyerCBD` interface though, which was why the CBD
status was not picked up during apply.

The fix is relatively easy, move the `GraphNodeDestroyerCBD`
implementation down to the abstract resource node, to make sure all
resources nodes implement the behavior. The nodes which need a different
implementation already have it, and thus will override the embedded
methods.

internal/terraform/context_apply2_test.go

@@ -3624,3 +3624,84 @@ resource "test_object" "c" {
 		}
 	}
 }
+
+func TestContext2Apply_updateForcedCreateBeforeDestroy(t *testing.T) {
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "test_object" "a" {
+}
+
+resource "test_object" "b" {
+  ref = test_object.a.id
+  update = "new"
+}
+
+resource "test_object" "c" {
+  ref = test_object.b.id
+  lifecycle {
+    create_before_destroy = true
+  }
+}
+`,
+	})
+
+	p := &testing_provider.MockProvider{}
+	p.GetProviderSchemaResponse = &providers.GetProviderSchemaResponse{
+		ResourceTypes: map[string]providers.Schema{
+			"test_object": {
+				Block: &configschema.Block{
+					Attributes: map[string]*configschema.Attribute{
+						"id": {
+							Type:     cty.String,
+							Computed: true,
+						},
+						"ref": {
+							Type:     cty.String,
+							Optional: true,
+						},
+						"update": {
+							Type:     cty.String,
+							Optional: true,
+						},
+					},
+				},
+			},
+		},
+	}
+
+	state := states.BuildState(func(s *states.SyncState) {
+		s.SetResourceInstanceCurrent(mustResourceInstanceAddr("test_object.a"), &states.ResourceInstanceObjectSrc{
+			AttrsJSON:           []byte(`{"id":"a"}`),
+			Status:              states.ObjectReady,
+			CreateBeforeDestroy: true,
+		}, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`))
+		s.SetResourceInstanceCurrent(mustResourceInstanceAddr("test_object.b"), &states.ResourceInstanceObjectSrc{
+			AttrsJSON:           []byte(`{"id":"b","ref":"a","update":"old"}`),
+			Status:              states.ObjectReady,
+			CreateBeforeDestroy: true,
+		}, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`))
+		s.SetResourceInstanceCurrent(mustResourceInstanceAddr("test_object.c"), &states.ResourceInstanceObjectSrc{
+			AttrsJSON:           []byte(`{"id":"c","ref":"b"}`),
+			Status:              states.ObjectReady,
+			CreateBeforeDestroy: true,
+		}, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`))
+	})
+
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+	})
+
+	plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables)))
+	assertNoErrors(t, diags)
+
+	state, diags = ctx.Apply(plan, m, nil)
+	assertNoErrors(t, diags)
+
+	for _, res := range state.RootModule().Resources {
+		if !res.Instances[addrs.NoKey].Current.CreateBeforeDestroy {
+			t.Errorf("%s should be create_before_destroy", res.Addr)
+		}
+	}
+}

internal/terraform/graph_builder_apply.go

@@ -194,6 +194,11 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer {
 
 		// Detect when create_before_destroy must be forced on for a particular
 		// node due to dependency edges, to avoid graph cycles during apply.
+		//
+		// FIXME: this should not need to be recalculated during apply.
+		// Currently however, the instance object which stores the planned
+		// information is lost for newly created instances because it contains
+		// no state value, and we end up recalculating CBD for all nodes.
 		&ForcedCBDTransformer{},
 
 		// Destruction ordering

internal/terraform/node_resource_abstract.go

@@ -86,6 +86,8 @@ type NodeAbstractResource struct {
 	// generateConfigPath tells this node which file to write generated config
 	// into. If empty, then config should not be generated.
 	generateConfigPath string
+
+	forceCreateBeforeDestroy bool
 }
 
 var (
@@ -102,6 +104,7 @@ var (
 	_ GraphNodeTargetable                  = (*NodeAbstractResource)(nil)
 	_ graphNodeAttachDataResourceDependsOn = (*NodeAbstractResource)(nil)
 	_ dag.GraphNodeDotter                  = (*NodeAbstractResource)(nil)
+	_ GraphNodeDestroyerCBD                = (*NodeAbstractResource)(nil)
 )
 
 // NewNodeAbstractResource creates an abstract resource graph node for
@@ -144,6 +147,24 @@ func (n *NodeAbstractResource) ReferenceableAddrs() []addrs.Referenceable {
 	return []addrs.Referenceable{n.Addr.Resource}
 }
 
+// CreateBeforeDestroy returns this node's CreateBeforeDestroy status.
+func (n *NodeAbstractResource) CreateBeforeDestroy() bool {
+	if n.forceCreateBeforeDestroy {
+		return n.forceCreateBeforeDestroy
+	}
+
+	if n.Config != nil && n.Config.Managed != nil {
+		return n.Config.Managed.CreateBeforeDestroy
+	}
+
+	return false
+}
+
+func (n *NodeAbstractResource) ModifyCreateBeforeDestroy(v bool) error {
+	n.forceCreateBeforeDestroy = v
+	return nil
+}
+
 // GraphNodeReferencer
 func (n *NodeAbstractResource) References() []*addrs.Reference {
 	var result []*addrs.Reference

internal/terraform/node_resource_abstract_instance.go

@@ -337,11 +337,7 @@ func (n *NodeAbstractResourceInstance) writeResourceInstanceStateImpl(ctx EvalCo
 		return nil
 	}
 
-	if obj != nil {
-		log.Printf("[TRACE] %s: writing state object for %s", logFuncName, absAddr)
-	} else {
-		log.Printf("[TRACE] %s: removing state object for %s", logFuncName, absAddr)
-	}
+	log.Printf("[TRACE] %s: writing state object for %s", logFuncName, absAddr)
 
 	schema, currentVersion := providerSchema.SchemaForResourceAddr(absAddr.ContainingResource().Resource)
 	if schema == nil {

internal/terraform/node_resource_apply_instance.go

@@ -29,10 +29,6 @@ type NodeApplyableResourceInstance struct {
 
 	graphNodeDeposer // implementation of GraphNodeDeposerConfig
 
-	// If this node is forced to be CreateBeforeDestroy, we need to record that
-	// in the state to.
-	ForceCreateBeforeDestroy bool
-
 	// forceReplace are resource instance addresses where the user wants to
 	// force generating a replace action. This set isn't pre-filtered, so
 	// it might contain addresses that have nothing to do with the resource
@@ -50,24 +46,6 @@ var (
 	_ GraphNodeAttachDependencies = (*NodeApplyableResourceInstance)(nil)
 )
 
-// CreateBeforeDestroy returns this node's CreateBeforeDestroy status.
-func (n *NodeApplyableResourceInstance) CreateBeforeDestroy() bool {
-	if n.ForceCreateBeforeDestroy {
-		return n.ForceCreateBeforeDestroy
-	}
-
-	if n.Config != nil && n.Config.Managed != nil {
-		return n.Config.Managed.CreateBeforeDestroy
-	}
-
-	return false
-}
-
-func (n *NodeApplyableResourceInstance) ModifyCreateBeforeDestroy(v bool) error {
-	n.ForceCreateBeforeDestroy = v
-	return nil
-}
-
 // GraphNodeCreator
 func (n *NodeApplyableResourceInstance) CreateAddr() *addrs.AbsResourceInstance {
 	addr := n.ResourceInstanceAddr()
@@ -330,6 +308,7 @@ func (n *NodeApplyableResourceInstance) managedResourceExecute(ctx EvalContext)
 	}
 
 	state, applyDiags := n.apply(ctx, state, diffApply, n.Config, repeatData, n.CreateBeforeDestroy())
+
 	diags = diags.Append(applyDiags)
 
 	// We clear the change out here so that future nodes don't see a change