Field for the previous address of each resource instance in the plan

In order to expose the effect of any relevant "moved" statements we dealt
with prior to creating the plan, we'll record with each
ResourceInstanceChange both is current address and the address it was
tracked at for the previous run.

To save consumers of these objects from having to special-case the
situation where there _was_ no previous run (e.g. because this is a Create
change), we'll just pretend the previous run address was the same as the
current address in that case, the same as for an update without any
renaming in effect.

This includes a breaking change to the plan file format, but one that
doesn't require a version number increment because there is no ambiguity
between the two formats and so mismatched parsers will already fail with
an error message.

As of this commit we've just added the new field but not yet populated it
with any useful information: it always just matches Addr. A future commit
will wire this up to the result of applying the moves so that we can
populate it correctly. We also don't yet expose this new information
anywhere in the UI layer.

Author: apparentlymart

internal/plans/changes.go

@@ -147,6 +147,19 @@ type ResourceInstanceChange struct {
 	// will apply to.
 	Addr addrs.AbsResourceInstance
 
+	// PrevRunAddr is the absolute address that this resource instance had at
+	// the conclusion of a previous run.
+	//
+	// This will typically be the same as Addr, but can be different if the
+	// previous resource instance was subject to a "moved" block that we
+	// handled in the process of creating this plan.
+	//
+	// For the initial creation of a resource instance there isn't really any
+	// meaningful "previous run address", but PrevRunAddr will still be set
+	// equal to Addr in that case in order to simplify logic elsewhere which
+	// aims to detect and react to the movement of instances between addresses.
+	PrevRunAddr addrs.AbsResourceInstance
+
 	// DeposedKey is the identifier for a deposed object associated with the
 	// given instance, or states.NotDeposed if this change applies to the
 	// current object.
@@ -203,8 +216,15 @@ func (rc *ResourceInstanceChange) Encode(ty cty.Type) (*ResourceInstanceChangeSr
 	if err != nil {
 		return nil, err
 	}
+	prevRunAddr := rc.PrevRunAddr
+	if prevRunAddr.Resource.Resource.Type == "" {
+		// Suggests an old caller that hasn't been properly updated to
+		// populate this yet.
+		prevRunAddr = rc.Addr
+	}
 	return &ResourceInstanceChangeSrc{
 		Addr:            rc.Addr,
+		PrevRunAddr:     prevRunAddr,
 		DeposedKey:      rc.DeposedKey,
 		ProviderAddr:    rc.ProviderAddr,
 		ChangeSrc:       *cs,

internal/plans/changes_src.go

@@ -16,6 +16,19 @@ type ResourceInstanceChangeSrc struct {
 	// will apply to.
 	Addr addrs.AbsResourceInstance
 
+	// PrevRunAddr is the absolute address that this resource instance had at
+	// the conclusion of a previous run.
+	//
+	// This will typically be the same as Addr, but can be different if the
+	// previous resource instance was subject to a "moved" block that we
+	// handled in the process of creating this plan.
+	//
+	// For the initial creation of a resource instance there isn't really any
+	// meaningful "previous run address", but PrevRunAddr will still be set
+	// equal to Addr in that case in order to simplify logic elsewhere which
+	// aims to detect and react to the movement of instances between addresses.
+	PrevRunAddr addrs.AbsResourceInstance
+
 	// DeposedKey is the identifier for a deposed object associated with the
 	// given instance, or states.NotDeposed if this change applies to the
 	// current object.
@@ -66,8 +79,15 @@ func (rcs *ResourceInstanceChangeSrc) Decode(ty cty.Type) (*ResourceInstanceChan
 	if err != nil {
 		return nil, err
 	}
+	prevRunAddr := rcs.PrevRunAddr
+	if prevRunAddr.Resource.Resource.Type == "" {
+		// Suggests an old caller that hasn't been properly updated to
+		// populate this yet.
+		prevRunAddr = rcs.Addr
+	}
 	return &ResourceInstanceChange{
 		Addr:            rcs.Addr,
+		PrevRunAddr:     prevRunAddr,
 		DeposedKey:      rcs.DeposedKey,
 		ProviderAddr:    rcs.ProviderAddr,
 		Change:          *change,

internal/plans/internal/planproto/planfile.pb.go

@@ -129,34 +129,23 @@ enum ResourceInstanceActionReason {
 }
 
 message ResourceInstanceChange {
-    // module_path is an address to the module that defined this resource.
-    // module_path is omitted for resources in the root module. For descendent modules
-    // it is a string like module.foo.module.bar as would be seen at the beginning of a
-    // resource address. The format of this string is not yet frozen and so external
-    // callers should treat it as an opaque key for filtering purposes.
-    string module_path = 1;
-
-    // mode is the resource mode.
-    ResourceMode mode = 2;
-    enum ResourceMode {
-        managed = 0; // for "resource" blocks in configuration
-        data = 1;    // for "data" blocks in configuration
-    }
-
-    // type is the resource type name, like "aws_instance".
-    string type = 3;
+    // addr is a string representation of the resource instance address that
+    // this change will apply to.
+    string addr = 13;
 
-    // name is the logical name of the resource as defined in configuration.
-    // For example, in aws_instance.foo this would be "foo".
-    string name = 4;
+    // prev_run_addr is a string representation of the address at which
+    // this resource instance was tracked during the previous apply operation.
+    //
+    // This is populated only if it would be different from addr due to
+    // Terraform having reacted to refactoring annotations in the configuration.
+    // If empty, the previous run address is the same as the current address.
+    string prev_run_addr = 14;
 
-    // instance_key is either an integer index or a string key, depending on which iteration
-    // attributes ("count" or "for_each") are being used for this resource. If none
-    // are in use, this field is omitted.
-    oneof instance_key {
-        string str = 5;
-        int64 int = 6;
-    };
+    // NOTE: Earlier versions of this format had fields 1 through 6 describing
+    // various indivdual parts of "addr". We're now using our standard compact
+    // string representation to capture the same information. We don't support
+    // preserving plan files from one Terraform version to the next, so we
+    // no longer declare nor accept those fields.
 
     // deposed_key, if set, indicates that this change applies to a deposed
     // object for the indicated instance with the given deposed key. If not
@@ -169,8 +158,7 @@ message ResourceInstanceChange {
     string provider = 8;
 
     // Description of the proposed change. May use "create", "read", "update",
-    // "replace" and "delete" actions. "no-op" changes are not currently used here
-    // but consumers must accept and discard them to allow for future expansion.
+    // "replace", "delete" and "no-op" actions.
     Change change = 9;
 
     // raw blob value provided by the provider as additional context for the

internal/plans/internal/planproto/planfile.proto

@@ -12,7 +12,6 @@ import (
 	"github.com/hashicorp/terraform/internal/plans"
 	"github.com/hashicorp/terraform/internal/plans/internal/planproto"
 	"github.com/hashicorp/terraform/internal/states"
-	"github.com/hashicorp/terraform/internal/tfdiags"
 	"github.com/hashicorp/terraform/version"
 	"github.com/zclconf/go-cty/cty"
 )
@@ -157,12 +156,23 @@ func resourceChangeFromTfplan(rawChange *planproto.ResourceInstanceChange) (*pla
 
 	ret := &plans.ResourceInstanceChangeSrc{}
 
-	moduleAddr := addrs.RootModuleInstance
-	if rawChange.ModulePath != "" {
-		var diags tfdiags.Diagnostics
-		moduleAddr, diags = addrs.ParseModuleInstanceStr(rawChange.ModulePath)
+	if rawChange.Addr == "" {
+		// If "Addr" isn't populated then seems likely that this is a plan
+		// file created by an earlier version of Terraform, which had the
+		// same information spread over various other fields:
+		// ModulePath, Mode, Name, Type, and InstanceKey.
+		return nil, fmt.Errorf("no instance address for resource instance change; perhaps this plan was created by a different version of Terraform?")
+	}
+
+	instAddr, diags := addrs.ParseAbsResourceInstanceStr(rawChange.Addr)
+	if diags.HasErrors() {
+		return nil, fmt.Errorf("invalid resource instance address %q: %w", rawChange.Addr, diags.Err())
+	}
+	prevRunAddr := instAddr
+	if rawChange.PrevRunAddr != "" {
+		prevRunAddr, diags = addrs.ParseAbsResourceInstanceStr(rawChange.PrevRunAddr)
 		if diags.HasErrors() {
-			return nil, diags.Err()
+			return nil, fmt.Errorf("invalid resource instance previous run address %q: %w", rawChange.PrevRunAddr, diags.Err())
 		}
 	}
 
@@ -172,37 +182,8 @@ func resourceChangeFromTfplan(rawChange *planproto.ResourceInstanceChange) (*pla
 	}
 	ret.ProviderAddr = providerAddr
 
-	var mode addrs.ResourceMode
-	switch rawChange.Mode {
-	case planproto.ResourceInstanceChange_managed:
-		mode = addrs.ManagedResourceMode
-	case planproto.ResourceInstanceChange_data:
-		mode = addrs.DataResourceMode
-	default:
-		return nil, fmt.Errorf("resource has invalid mode %s", rawChange.Mode)
-	}
-
-	typeName := rawChange.Type
-	name := rawChange.Name
-
-	resAddr := addrs.Resource{
-		Mode: mode,
-		Type: typeName,
-		Name: name,
-	}
-
-	var instKey addrs.InstanceKey
-	switch rawTk := rawChange.InstanceKey.(type) {
-	case nil:
-	case *planproto.ResourceInstanceChange_Int:
-		instKey = addrs.IntKey(rawTk.Int)
-	case *planproto.ResourceInstanceChange_Str:
-		instKey = addrs.StringKey(rawTk.Str)
-	default:
-		return nil, fmt.Errorf("instance of %s has invalid key type %T", resAddr.Absolute(moduleAddr), rawChange.InstanceKey)
-	}
-
-	ret.Addr = resAddr.Instance(instKey).Absolute(moduleAddr)
+	ret.Addr = instAddr
+	ret.PrevRunAddr = prevRunAddr
 
 	if rawChange.DeposedKey != "" {
 		if len(rawChange.DeposedKey) != 8 {
@@ -454,35 +435,20 @@ func writeTfplan(plan *plans.Plan, w io.Writer) error {
 func resourceChangeToTfplan(change *plans.ResourceInstanceChangeSrc) (*planproto.ResourceInstanceChange, error) {
 	ret := &planproto.ResourceInstanceChange{}
 
-	ret.ModulePath = change.Addr.Module.String()
-
-	relAddr := change.Addr.Resource
-
-	switch relAddr.Resource.Mode {
-	case addrs.ManagedResourceMode:
-		ret.Mode = planproto.ResourceInstanceChange_managed
-	case addrs.DataResourceMode:
-		ret.Mode = planproto.ResourceInstanceChange_data
-	default:
-		return nil, fmt.Errorf("resource %s has unsupported mode %s", relAddr, relAddr.Resource.Mode)
+	if change.PrevRunAddr.Resource.Resource.Type == "" {
+		// Suggests that an old caller wasn't yet updated to populate this
+		// properly. All code that generates plans should populate this field,
+		// even if it's just to write in the same value as in change.Addr.
+		change.PrevRunAddr = change.Addr
 	}
 
-	ret.Type = relAddr.Resource.Type
-	ret.Name = relAddr.Resource.Name
-
-	switch tk := relAddr.Key.(type) {
-	case nil:
-		// Nothing to do, then.
-	case addrs.IntKey:
-		ret.InstanceKey = &planproto.ResourceInstanceChange_Int{
-			Int: int64(tk),
-		}
-	case addrs.StringKey:
-		ret.InstanceKey = &planproto.ResourceInstanceChange_Str{
-			Str: string(tk),
-		}
-	default:
-		return nil, fmt.Errorf("resource %s has unsupported instance key type %T", relAddr, relAddr.Key)
+	ret.Addr = change.Addr.String()
+	ret.PrevRunAddr = change.PrevRunAddr.String()
+	if ret.PrevRunAddr == ret.Addr {
+		// In the on-disk format we leave PrevRunAddr unpopulated in the common
+		// case where it's the same as Addr, and then fill it back in again on
+		// read.
+		ret.PrevRunAddr = ""
 	}
 
 	ret.DeposedKey = string(change.DeposedKey)
@@ -500,7 +466,7 @@ func resourceChangeToTfplan(change *plans.ResourceInstanceChangeSrc) (*planproto
 
 	valChange, err := changeToTfplan(&change.ChangeSrc)
 	if err != nil {
-		return nil, fmt.Errorf("failed to serialize resource %s change: %s", relAddr, err)
+		return nil, fmt.Errorf("failed to serialize resource %s change: %s", change.Addr, err)
 	}
 	ret.Change = valChange
 
@@ -514,7 +480,7 @@ func resourceChangeToTfplan(change *plans.ResourceInstanceChangeSrc) (*planproto
 	case plans.ResourceInstanceReplaceByRequest:
 		ret.ActionReason = planproto.ResourceInstanceActionReason_REPLACE_BY_REQUEST
 	default:
-		return nil, fmt.Errorf("resource %s has unsupported action reason %s", relAddr, change.ActionReason)
+		return nil, fmt.Errorf("resource %s has unsupported action reason %s", change.Addr, change.ActionReason)
 	}
 
 	if len(change.Private) > 0 {

internal/plans/planfile/tfplan.go

@@ -57,6 +57,11 @@ func TestTFPlanRoundTrip(t *testing.T) {
 						Type: "test_thing",
 						Name: "woot",
 					}.Instance(addrs.IntKey(0)).Absolute(addrs.RootModuleInstance),
+					PrevRunAddr: addrs.Resource{
+						Mode: addrs.ManagedResourceMode,
+						Type: "test_thing",
+						Name: "woot",
+					}.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance),
 					ProviderAddr: addrs.AbsProviderConfig{
 						Provider: addrs.NewDefaultProvider("test"),
 						Module:   addrs.RootModule,
@@ -93,7 +98,12 @@ func TestTFPlanRoundTrip(t *testing.T) {
 						Mode: addrs.ManagedResourceMode,
 						Type: "test_thing",
 						Name: "woot",
-					}.Instance(addrs.IntKey(0)).Absolute(addrs.RootModuleInstance),
+					}.Instance(addrs.IntKey(1)).Absolute(addrs.RootModuleInstance),
+					PrevRunAddr: addrs.Resource{
+						Mode: addrs.ManagedResourceMode,
+						Type: "test_thing",
+						Name: "woot",
+					}.Instance(addrs.IntKey(1)).Absolute(addrs.RootModuleInstance),
 					DeposedKey: "foodface",
 					ProviderAddr: addrs.AbsProviderConfig{
 						Provider: addrs.NewDefaultProvider("test"),
@@ -214,6 +224,11 @@ func TestTFPlanRoundTripDestroy(t *testing.T) {
 						Type: "test_thing",
 						Name: "woot",
 					}.Instance(addrs.IntKey(0)).Absolute(addrs.RootModuleInstance),
+					PrevRunAddr: addrs.Resource{
+						Mode: addrs.ManagedResourceMode,
+						Type: "test_thing",
+						Name: "woot",
+					}.Instance(addrs.IntKey(0)).Absolute(addrs.RootModuleInstance),
 					ProviderAddr: addrs.AbsProviderConfig{
 						Provider: addrs.NewDefaultProvider("test"),
 						Module:   addrs.RootModule,

internal/plans/planfile/tfplan_test.go

@@ -392,8 +392,9 @@ func (n *NodeAbstractResourceInstance) planDestroy(ctx EvalContext, currentState
 		// that we checked something and concluded no changes were needed
 		// vs. that something being entirely excluded e.g. due to -target.
 		noop := &plans.ResourceInstanceChange{
-			Addr:       absAddr,
-			DeposedKey: deposedKey,
+			Addr:        absAddr,
+			PrevRunAddr: absAddr, // TODO-PrevRunAddr: If this instance was moved/renamed in this run, record its old address
+			DeposedKey:  deposedKey,
 			Change: plans.Change{
 				Action: plans.NoOp,
 				Before: cty.NullVal(cty.DynamicPseudoType),
@@ -419,8 +420,9 @@ func (n *NodeAbstractResourceInstance) planDestroy(ctx EvalContext, currentState
 	// Plan is always the same for a destroy. We don't need the provider's
 	// help for this one.
 	plan := &plans.ResourceInstanceChange{
-		Addr:       absAddr,
-		DeposedKey: deposedKey,
+		Addr:        absAddr,
+		PrevRunAddr: absAddr, // TODO-PrevRunAddr: If this instance was moved/renamed in this run, record its old address
+		DeposedKey:  deposedKey,
 		Change: plans.Change{
 			Action: plans.Delete,
 			Before: currentState.Value,
@@ -444,7 +446,7 @@ func (n *NodeAbstractResourceInstance) planDestroy(ctx EvalContext, currentState
 	return plan, diags
 }
 
-// writeChange  saves a planned change for an instance object into the set of
+// writeChange saves a planned change for an instance object into the set of
 // global planned changes.
 func (n *NodeAbstractResourceInstance) writeChange(ctx EvalContext, change *plans.ResourceInstanceChange, deposedKey states.DeposedKey) error {
 	changes := ctx.Changes()
@@ -469,6 +471,16 @@ func (n *NodeAbstractResourceInstance) writeChange(ctx EvalContext, change *plan
 		// Should never happen, and indicates a bug in the caller.
 		panic("inconsistent address and/or deposed key in writeChange")
 	}
+	if change.PrevRunAddr.Resource.Resource.Type == "" {
+		// Should never happen, and indicates a bug in the caller.
+		// (The change.Encode function actually has its own fixup to just
+		// quietly make this match change.Addr in the incorrect case, but we
+		// intentionally panic here in order to catch incorrect callers where
+		// the stack trace will hopefully be actually useful. The tolerance
+		// at the next layer down is mainly to accommodate sloppy input in
+		// older tests.)
+		panic("unpopulated ResourceInstanceChange.PrevRunAddr in writeChange")
+	}
 
 	ri := n.Addr.Resource
 	schema, _ := providerSchema.SchemaForResourceAddr(ri.Resource)
@@ -1054,6 +1066,7 @@ func (n *NodeAbstractResourceInstance) plan(
 	// Update our return plan
 	plan = &plans.ResourceInstanceChange{
 		Addr:         n.Addr,
+		PrevRunAddr:  n.Addr, // TODO-PrevRunAddr: If this instance was moved/renamed in this run, record its old address
 		Private:      plannedPrivate,
 		ProviderAddr: n.ResolvedProvider,
 		Change: plans.Change{
@@ -1515,6 +1528,7 @@ func (n *NodeAbstractResourceInstance) planDataSource(ctx EvalContext, currentSt
 		// value containing unknowns from PlanDataResourceObject.
 		plannedChange := &plans.ResourceInstanceChange{
 			Addr:         n.Addr,
+			PrevRunAddr:  n.Addr, // data resources are not refactorable
 			ProviderAddr: n.ResolvedProvider,
 			Change: plans.Change{
 				Action: plans.Read,