Merge pull request #29437 from hashicorp/jbardin/move

refactoring: complete CanChainFrom and NestedWithin

internal/addrs/move_endpoint_module.go

@@ -5,8 +5,29 @@ import (
 	"strings"
 
 	"github.com/hashicorp/terraform/internal/tfdiags"
+	"github.com/zclconf/go-cty/cty"
 )
 
+// anyKeyImpl is the InstanceKey representation indicating a wildcard, which
+// matches all possible keys. This is only used internally for matching
+// combinations of address types, where only portions of the path contain key
+// information.
+type anyKeyImpl rune
+
+func (k anyKeyImpl) instanceKeySigil() {
+}
+
+func (k anyKeyImpl) String() string {
+	return fmt.Sprintf("[%s]", string(k))
+}
+
+func (k anyKeyImpl) Value() cty.Value {
+	return cty.StringVal(string(k))
+}
+
+// anyKey is the only valid value of anyKeyImpl
+var anyKey = anyKeyImpl('*')
+
 // MoveEndpointInModule annotates a MoveEndpoint with the address of the
 // module where it was declared, which is the form we use for resolving
 // whether move statements chain from or are nested within other move
@@ -149,6 +170,36 @@ func (e *MoveEndpointInModule) ModuleCallTraversals() (Module, []ModuleCall) {
 	return e.module, ret
 }
 
+// synthModuleInstance constructs a module instance out of the module path and
+// any module portion of the relSubject, substituting Module and Call segments
+// with ModuleInstanceStep using the anyKey value.
+// This is only used internally for comparison of these complete paths, but
+// does not represent how the individual parts are handled elsewhere in the
+// code.
+func (e *MoveEndpointInModule) synthModuleInstance() ModuleInstance {
+	var inst ModuleInstance
+
+	for _, mod := range e.module {
+		inst = append(inst, ModuleInstanceStep{Name: mod, InstanceKey: anyKey})
+	}
+
+	switch sub := e.relSubject.(type) {
+	case ModuleInstance:
+		inst = append(inst, sub...)
+	case AbsModuleCall:
+		inst = append(inst, sub.Module...)
+		inst = append(inst, ModuleInstanceStep{Name: sub.Call.Name, InstanceKey: anyKey})
+	case AbsResource:
+		inst = append(inst, sub.Module...)
+	case AbsResourceInstance:
+		inst = append(inst, sub.Module...)
+	default:
+		panic(fmt.Sprintf("unhandled relative address type %T", sub))
+	}
+
+	return inst
+}
+
 // SelectsModule returns true if the reciever directly selects either
 // the given module or a resource nested directly inside that module.
 //
@@ -158,60 +209,49 @@ func (e *MoveEndpointInModule) ModuleCallTraversals() (Module, []ModuleCall) {
 // resource move indicates that we should search each of the resources in
 // the given module to see if they match.
 func (e *MoveEndpointInModule) SelectsModule(addr ModuleInstance) bool {
-	// In order to match the given module path should be at least as
+	synthInst := e.synthModuleInstance()
-	// long as the path to the module where the move endpoint was defined.
+
-	if len(addr) < len(e.module) {
+	// In order to match the given module instance, our combined path must be
+	// equal in length.
+	if len(synthInst) != len(addr) {
 		return false
 	}
 
-	containerPart := addr[:len(e.module)]
+	for i, step := range synthInst {
-	relPart := addr[len(e.module):]
+		switch step.InstanceKey {
-
+		case anyKey:
-	// The names of all of the steps that align with e.module must match,
+			// we can match any key as long as the name matches
-	// though the instance keys are wildcards for this part.
+			if step.Name != addr[i].Name {
-	for i := range e.module {
-		if containerPart[i].Name != e.module[i] {
 				return false
 			}
-	}
-
-	// The remaining module address steps must match both name and key.
-	// The logic for all of these is similar but we will retrieve the
-	// module address differently for each type.
-	var relMatch ModuleInstance
-	switch relAddr := e.relSubject.(type) {
-	case ModuleInstance:
-		relMatch = relAddr
-	case AbsModuleCall:
-		// This one requires a little more fuss because the call effectively
-		// slices in two the final step of the module address.
-		if len(relPart) != len(relAddr.Module)+1 {
-			return false
-		}
-		callPart := relPart[len(relPart)-1]
-		if callPart.Name != relAddr.Call.Name {
-			return false
-		}
-
-		relMatch = relAddr.Module.Child(relAddr.Call.Name, callPart.InstanceKey)
-	case AbsResource:
-		relMatch = relAddr.Module
-	case AbsResourceInstance:
-		relMatch = relAddr.Module
 		default:
-		panic(fmt.Sprintf("unhandled relative address type %T", relAddr))
+			if step != addr[i] {
-	}
-
-	if len(relPart) != len(relMatch) {
-		return false
-	}
-
-	for i := range relMatch {
-		if relPart[i] != relMatch[i] {
 				return false
 			}
 		}
+	}
+	return true
+}
 
+// moduleInstanceCanMatch indicates that modA can match modB taking into
+// account steps with an anyKey InstanceKey as wildcards. The comparison of
+// wildcard steps is done symmetrically, because varying portions of either
+// instance's path could have been derived from configuration vs evaluation.
+// The length of modA must be equal or shorter than the length of modB.
+func moduleInstanceCanMatch(modA, modB ModuleInstance) bool {
+	for i, step := range modA {
+		switch {
+		case step.InstanceKey == anyKey || modB[i].InstanceKey == anyKey:
+			// we can match any key as long as the names match
+			if step.Name != modB[i].Name {
+				return false
+			}
+		default:
+			if step != modB[i] {
+				return false
+			}
+		}
+	}
 	return true
 }
 
@@ -222,32 +262,40 @@ func (e *MoveEndpointInModule) SelectsModule(addr ModuleInstance) bool {
 // the reciever is the "to" from one statement and the other given address
 // is the "from" of another statement.
 func (e *MoveEndpointInModule) CanChainFrom(other *MoveEndpointInModule) bool {
+	eMod := e.synthModuleInstance()
+	oMod := other.synthModuleInstance()
+
+	// if the complete paths are different lengths, these cannot refer to the
+	// same value.
+	if len(eMod) != len(oMod) {
+		return false
+	}
+	if !moduleInstanceCanMatch(oMod, eMod) {
+		return false
+	}
+
 	eSub := e.relSubject
 	oSub := other.relSubject
 
 	switch oSub := oSub.(type) {
-	case AbsModuleCall:
+	case AbsModuleCall, ModuleInstance:
-		switch eSub := eSub.(type) {
+		switch eSub.(type) {
-		case AbsModuleCall:
+		case AbsModuleCall, ModuleInstance:
-			return eSub.Equal(oSub)
+			// we already know the complete module path including any final
-		}
+			// module call name is equal.
-
+			return true
-	case ModuleInstance:
-		switch eSub := eSub.(type) {
-		case ModuleInstance:
-			return eSub.Equal(oSub)
 		}
 
 	case AbsResource:
 		switch eSub := eSub.(type) {
 		case AbsResource:
-			return eSub.Equal(oSub)
+			return eSub.Resource.Equal(oSub.Resource)
 		}
 
 	case AbsResourceInstance:
 		switch eSub := eSub.(type) {
 		case AbsResourceInstance:
-			return eSub.Equal(oSub)
+			return eSub.Resource.Equal(oSub.Resource)
 		}
 	}
 
@@ -258,49 +306,52 @@ func (e *MoveEndpointInModule) CanChainFrom(other *MoveEndpointInModule) bool {
 // contained within one of the objects that the given other address could
 // select.
 func (e *MoveEndpointInModule) NestedWithin(other *MoveEndpointInModule) bool {
+	eMod := e.synthModuleInstance()
+	oMod := other.synthModuleInstance()
+
+	// In order to be nested within the given endpoint, the module path must be
+	// shorter or equal.
+	if len(oMod) > len(eMod) {
+		return false
+	}
+
+	if !moduleInstanceCanMatch(oMod, eMod) {
+		return false
+	}
+
 	eSub := e.relSubject
 	oSub := other.relSubject
 
 	switch oSub := oSub.(type) {
 	case AbsModuleCall:
-		withinModuleCall := func(mod ModuleInstance, call AbsModuleCall) bool {
+		switch eSub.(type) {
-			// parent modules don't match at all
+		case AbsModuleCall:
-			if !call.Module.IsAncestor(mod) {
+			// we know the other endpoint selects our module, but if we are
+			// also a module call our path must be longer to be nested.
+			return len(eMod) > len(oMod)
+		}
+
+		return true
+
+	case ModuleInstance:
+		switch eSub.(type) {
+		case ModuleInstance, AbsModuleCall:
+			// a nested module must have a longer path
+			return len(eMod) > len(oMod)
+		}
+
+		return true
+
+	case AbsResource:
+		if len(eMod) != len(oMod) {
+			// these resources are from different modules
 			return false
 		}
 
-			rem := mod[len(call.Module):]
-			return rem[0].Name == call.Call.Name
-		}
-
-		// Module calls can contain module instances, resources, and resource
-		// instances.
-		switch eSub := eSub.(type) {
-		case AbsResource:
-			return withinModuleCall(eSub.Module, oSub)
-
-		case AbsResourceInstance:
-			return withinModuleCall(eSub.Module, oSub)
-
-		case ModuleInstance:
-			return withinModuleCall(eSub, oSub)
-		}
-
-	case ModuleInstance:
-		// Module instances can contain resources and resource instances.
-		switch eSub := eSub.(type) {
-		case AbsResource:
-			return eSub.Module.Equal(oSub) || oSub.IsAncestor(eSub.Module)
-
-		case AbsResourceInstance:
-			return eSub.Module.Equal(oSub) || oSub.IsAncestor(eSub.Module)
-		}
-
-	case AbsResource:
 		// A resource can only contain a resource instance.
 		switch eSub := eSub.(type) {
 		case AbsResourceInstance:
-			return eSub.ContainingResource().Equal(oSub)
+			return eSub.Resource.Resource.Equal(oSub.Resource)
 		}
 	}
 

internal/addrs/move_endpoint_module_test.go

@@ -1078,6 +1078,7 @@ func TestAbsResourceMoveDestination(t *testing.T) {
 func TestMoveEndpointChainAndNested(t *testing.T) {
 	tests := []struct {
 		Endpoint, Other            AbsMoveable
+		EndpointMod, OtherMod      Module
 		CanChainFrom, NestedWithin bool
 	}{
 		{
@@ -1140,7 +1141,7 @@ func TestMoveEndpointChainAndNested(t *testing.T) {
 			},
 			Other:        mustParseModuleInstanceStr("module.foo[2]"),
 			CanChainFrom: false,
-			NestedWithin: false,
+			NestedWithin: true,
 		},
 
 		{
@@ -1203,6 +1204,7 @@ func TestMoveEndpointChainAndNested(t *testing.T) {
 			Other:        mustParseAbsResourceInstanceStr("module.foo[2].resource.baz"),
 			CanChainFrom: false,
 		},
+
 		{
 			Endpoint:     mustParseModuleInstanceStr("module.foo[2]"),
 			Other:        mustParseAbsResourceInstanceStr("module.foo[2].resource.baz"),
@@ -1213,11 +1215,116 @@ func TestMoveEndpointChainAndNested(t *testing.T) {
 			Other:        mustParseAbsResourceInstanceStr("module.foo[2].resource.baz"),
 			CanChainFrom: false,
 		},
+
 		{
 			Endpoint:     mustParseAbsResourceInstanceStr("module.foo[2].resource.baz"),
 			Other:        mustParseAbsResourceInstanceStr("module.foo[2].resource.baz"),
 			CanChainFrom: true,
 		},
+
+		{
+			Endpoint:     mustParseAbsResourceInstanceStr("resource.baz"),
+			EndpointMod:  Module{"foo"},
+			Other:        mustParseAbsResourceInstanceStr("module.foo[2].resource.baz"),
+			CanChainFrom: true,
+		},
+
+		{
+			Endpoint:     mustParseAbsResourceInstanceStr("module.foo[2].resource.baz"),
+			Other:        mustParseAbsResourceInstanceStr("resource.baz"),
+			OtherMod:     Module{"foo"},
+			CanChainFrom: true,
+		},
+
+		{
+			Endpoint:     mustParseAbsResourceInstanceStr("resource.baz"),
+			EndpointMod:  Module{"foo"},
+			Other:        mustParseAbsResourceInstanceStr("resource.baz"),
+			OtherMod:     Module{"foo"},
+			CanChainFrom: true,
+		},
+
+		{
+			Endpoint:     mustParseAbsResourceInstanceStr("resource.baz").ContainingResource(),
+			EndpointMod:  Module{"foo"},
+			Other:        mustParseAbsResourceInstanceStr("module.foo[2].resource.baz").ContainingResource(),
+			CanChainFrom: true,
+		},
+
+		{
+			Endpoint:     mustParseModuleInstanceStr("module.foo[2].module.baz"),
+			Other:        mustParseModuleInstanceStr("module.baz"),
+			OtherMod:     Module{"foo"},
+			CanChainFrom: true,
+		},
+
+		{
+			Endpoint: AbsModuleCall{
+				Call: ModuleCall{Name: "bing"},
+			},
+			EndpointMod: Module{"foo", "baz"},
+			Other: AbsModuleCall{
+				Module: mustParseModuleInstanceStr("module.baz"),
+				Call:   ModuleCall{Name: "bing"},
+			},
+			OtherMod:     Module{"foo"},
+			CanChainFrom: true,
+		},
+
+		{
+			Endpoint:     mustParseAbsResourceInstanceStr("resource.baz"),
+			EndpointMod:  Module{"foo"},
+			Other:        mustParseAbsResourceInstanceStr("module.foo[2].resource.baz").ContainingResource(),
+			NestedWithin: true,
+		},
+
+		{
+			Endpoint:     mustParseAbsResourceInstanceStr("module.foo[2].resource.baz"),
+			Other:        mustParseAbsResourceInstanceStr("resource.baz").ContainingResource(),
+			OtherMod:     Module{"foo"},
+			NestedWithin: true,
+		},
+
+		{
+			Endpoint:     mustParseAbsResourceInstanceStr("resource.baz"),
+			EndpointMod:  Module{"foo"},
+			Other:        mustParseAbsResourceInstanceStr("resource.baz").ContainingResource(),
+			OtherMod:     Module{"foo"},
+			NestedWithin: true,
+		},
+
+		{
+			Endpoint:     mustParseAbsResourceInstanceStr("ressurce.baz").ContainingResource(),
+			EndpointMod:  Module{"foo"},
+			Other:        mustParseModuleInstanceStr("module.foo[2]"),
+			NestedWithin: true,
+		},
+
+		{
+			Endpoint: AbsModuleCall{
+				Call: ModuleCall{Name: "bang"},
+			},
+			EndpointMod: Module{"foo", "baz", "bing"},
+			Other: AbsModuleCall{
+				Module: mustParseModuleInstanceStr("module.baz"),
+				Call:   ModuleCall{Name: "bing"},
+			},
+			OtherMod:     Module{"foo"},
+			NestedWithin: true,
+		},
+
+		{
+			Endpoint: AbsModuleCall{
+				Module: mustParseModuleInstanceStr("module.bing"),
+				Call:   ModuleCall{Name: "bang"},
+			},
+			EndpointMod: Module{"foo", "baz"},
+			Other: AbsModuleCall{
+				Module: mustParseModuleInstanceStr("module.foo.module.baz"),
+				Call:   ModuleCall{Name: "bing"},
+			},
+			NestedWithin: true,
+		},
 	}
 
 	for i, test := range tests {
@@ -1225,18 +1332,20 @@ func TestMoveEndpointChainAndNested(t *testing.T) {
 			func(t *testing.T) {
 				endpoint := &MoveEndpointInModule{
 					relSubject: test.Endpoint,
+					module:     test.EndpointMod,
 				}
 
 				other := &MoveEndpointInModule{
 					relSubject: test.Other,
+					module:     test.OtherMod,
 				}
 
 				if endpoint.CanChainFrom(other) != test.CanChainFrom {
-					t.Errorf("expected %s CanChainFrom %s == %t", test.Endpoint, test.Other, test.CanChainFrom)
+					t.Errorf("expected %s CanChainFrom %s == %t", endpoint, other, test.CanChainFrom)
 				}
 
 				if endpoint.NestedWithin(other) != test.NestedWithin {
-					t.Errorf("expected %s NestedWithin %s == %t", test.Endpoint, test.Other, test.NestedWithin)
+					t.Errorf("expected %s NestedWithin %s == %t", endpoint, other, test.NestedWithin)
 				}
 			},
 		)

internal/refactoring/move_execute_test.go

@@ -324,10 +324,10 @@ func TestApplyMoves(t *testing.T) {
 			},
 		},
 
-		"move whole module to within indexed module and instance chained": {
+		"move whole module to indexed module and move instance chained": {
 			[]MoveStatement{
 				testMoveStatement(t, "", "module.boo", "module.bar[0]"),
-				testMoveStatement(t, "module.bar[0]", "foo.from[0]", "foo.too[0]"),
+				testMoveStatement(t, "bar", "foo.from[0]", "foo.to[0]"),
 			},
 			states.BuildState(func(s *states.SyncState) {
 				s.SetResourceInstanceCurrent(
@@ -345,19 +345,23 @@ func TestApplyMoves(t *testing.T) {
 					To:   instAddrs["module.bar[0].foo.from[0]"],
 				},
 				instAddrs["module.bar[0].foo.from[0]"].UniqueKey(): {
-					From: instAddrs["module.boo.foo.from[0]"],
+					From: instAddrs["module.bar[0].foo.from[0]"],
-					To:   instAddrs["module.bar[0].foo.from[0]"],
+					To:   instAddrs["module.bar[0].foo.to[0]"],
+				},
+				instAddrs["module.bar[0].foo.to[0]"].UniqueKey(): {
+					From: instAddrs["module.bar[0].foo.from[0]"],
+					To:   instAddrs["module.bar[0].foo.to[0]"],
 				},
 			},
 			[]string{
-				`module.bar[0].foo.from[0]`,
+				`module.bar[0].foo.to[0]`,
 			},
 		},
 
 		"move instance to indexed module and instance chained": {
 			[]MoveStatement{
 				testMoveStatement(t, "", "module.boo.foo.from[0]", "module.bar[0].foo.from[0]"),
-				testMoveStatement(t, "module.bar[0]", "foo.from[0]", "foo.too[0]"),
+				testMoveStatement(t, "bar", "foo.from[0]", "foo.to[0]"),
 			},
 			states.BuildState(func(s *states.SyncState) {
 				s.SetResourceInstanceCurrent(
@@ -375,12 +379,16 @@ func TestApplyMoves(t *testing.T) {
 					To:   instAddrs["module.bar[0].foo.from[0]"],
 				},
 				instAddrs["module.bar[0].foo.from[0]"].UniqueKey(): {
-					From: instAddrs["module.boo.foo.from[0]"],
+					From: instAddrs["module.bar[0].foo.from[0]"],
-					To:   instAddrs["module.bar[0].foo.from[0]"],
+					To:   instAddrs["module.bar[0].foo.to[0]"],
+				},
+				instAddrs["module.bar[0].foo.to[0]"].UniqueKey(): {
+					From: instAddrs["module.bar[0].foo.from[0]"],
+					To:   instAddrs["module.bar[0].foo.to[0]"],
 				},
 			},
 			[]string{
-				`module.bar[0].foo.from[0]`,
+				`module.bar[0].foo.to[0]`,
 			},
 		},
 	}