From bc60f7aae415c30694d93b80c8c23ae108f74cab Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 19 Aug 2021 17:28:35 -0400 Subject: [PATCH 1/2] Extend CanChainFrom to handle relative modules CanChainFrom needs to be able to handle move statements from different relative modules, re-implementing with addrs.anyKey Add the anyKey InstanceKey value to the addrs package to simplify module path comparison. This allows all combinations of module path representation to be normalized into a ModuleInstance which can be compared directly, rather than dealing with multiple levels of different prefix types. --- internal/addrs/move_endpoint_module.go | 223 ++++++++++++-------- internal/addrs/move_endpoint_module_test.go | 115 +++++++++- 2 files changed, 249 insertions(+), 89 deletions(-) diff --git a/internal/addrs/move_endpoint_module.go b/internal/addrs/move_endpoint_module.go index 5d53ae770..654252ba9 100644 --- a/internal/addrs/move_endpoint_module.go +++ b/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 - // long as the path to the module where the move endpoint was defined. - if len(addr) < len(e.module) { + synthInst := e.synthModuleInstance() + + // 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)] - relPart := addr[len(e.module):] - - // The names of all of the steps that align with e.module must match, - // though the instance keys are wildcards for this part. - for i := range e.module { - if containerPart[i].Name != e.module[i] { - return false + for i, step := range synthInst { + switch step.InstanceKey { + case anyKey: + // we can match any key as long as the name matches + if step.Name != addr[i].Name { + return false + } + default: + if step != addr[i] { + return false + } } } + return true +} - // 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 len(relPart) != len(relMatch) { - return false - } - - for i := range relMatch { - if relPart[i] != relMatch[i] { - return false +// 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: - switch eSub := eSub.(type) { - case AbsModuleCall: - return eSub.Equal(oSub) - } - - case ModuleInstance: - switch eSub := eSub.(type) { - case ModuleInstance: - return eSub.Equal(oSub) + case AbsModuleCall, ModuleInstance: + switch eSub.(type) { + case AbsModuleCall, ModuleInstance: + // we already know the complete module path including any final + // module call name is equal. + return true } 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 { - // parent modules don't match at all - if !call.Module.IsAncestor(mod) { - return false - } - - rem := mod[len(call.Module):] - return rem[0].Name == call.Call.Name + switch eSub.(type) { + case AbsModuleCall: + // 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) } - // 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) - } + return true 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) + 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 + } + // 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) } } diff --git a/internal/addrs/move_endpoint_module_test.go b/internal/addrs/move_endpoint_module_test.go index 2dd61e261..6c85380b6 100644 --- a/internal/addrs/move_endpoint_module_test.go +++ b/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) } }, ) From 30afb492ab64120db76dae44aeb522552bb203ec Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 20 Aug 2021 14:22:58 -0400 Subject: [PATCH 2/2] fix ApplyMove test with nested modules working The addrs package can now correctly handle the combinations of nested module endpoints, which fixes these 2 tests. --- internal/refactoring/move_execute_test.go | 26 +++++++++++++++-------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/internal/refactoring/move_execute_test.go b/internal/refactoring/move_execute_test.go index 126a56ee7..63c54a7ad 100644 --- a/internal/refactoring/move_execute_test.go +++ b/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]"], - To: instAddrs["module.bar[0].foo.from[0]"], + From: 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]"], - To: instAddrs["module.bar[0].foo.from[0]"], + From: 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]`, }, }, }