From 346418e31f9327c954b61e9354b7bf2d48c83632 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 21 Dec 2021 14:50:53 -0500 Subject: [PATCH 1/3] IsModuleMoveReIndex Add a method for checking if the From and To addresses in a move statement are only changing the indexes of modules relative to the statement module. This is needed because move statement nested within the module will be able to match against both the From and To addresses, causing cycles in the order of move operations. --- internal/addrs/module_instance_test.go | 6 +- internal/addrs/move_endpoint_module.go | 52 +++++++- internal/addrs/move_endpoint_module_test.go | 133 ++++++++++++++++++++ 3 files changed, 187 insertions(+), 4 deletions(-) diff --git a/internal/addrs/module_instance_test.go b/internal/addrs/module_instance_test.go index 4ad096cfc..393bcd57e 100644 --- a/internal/addrs/module_instance_test.go +++ b/internal/addrs/module_instance_test.go @@ -162,9 +162,9 @@ func TestModuleInstance_IsDeclaredByCall(t *testing.T) { } func mustParseModuleInstanceStr(str string) ModuleInstance { - mi, err := ParseModuleInstanceStr(str) - if err != nil { - panic(err) + mi, diags := ParseModuleInstanceStr(str) + if diags.HasErrors() { + panic(diags.ErrWithWarnings()) } return mi } diff --git a/internal/addrs/move_endpoint_module.go b/internal/addrs/move_endpoint_module.go index e2180f25a..7ff17621b 100644 --- a/internal/addrs/move_endpoint_module.go +++ b/internal/addrs/move_endpoint_module.go @@ -373,7 +373,7 @@ func (e *MoveEndpointInModule) CanChainFrom(other *MoveEndpointInModule) bool { return false } -// NestedWithin returns true if the reciever describes an address that is +// NestedWithin returns true if the receiver describes an address that is // contained within one of the objects that the given other address could // select. func (e *MoveEndpointInModule) NestedWithin(other *MoveEndpointInModule) bool { @@ -704,3 +704,53 @@ func (r AbsResourceInstance) MoveDestination(fromMatch, toMatch *MoveEndpointInM panic("unexpected object kind") } } + +// IsModuleMoveReIndex takes the from and to endpoints from a move statement, +// and returns true if the only changes are to module indexes, and all +// non-absolute paths remain the same. +func IsModuleMoveReIndex(from, to *MoveEndpointInModule) bool { + // The statements must originate from the same module. + if !from.module.Equal(to.module) { + panic("cannot compare move expressions from different modules") + } + + switch f := from.relSubject.(type) { + case AbsModuleCall: + switch t := to.relSubject.(type) { + case ModuleInstance: + if len(t) != 1 { + // An AbsModuleCall only ever has one segment, so the + // ModuleInstance length must match. + return false + } + + return f.Call.Name == t[0].Name + } + + case ModuleInstance: + switch t := to.relSubject.(type) { + case AbsModuleCall: + if len(f) != 1 { + return false + } + + return f[0].Name == t.Call.Name + + case ModuleInstance: + // We must have the same number of segments, and the names must all + // match in order for this to solely be an index change operation. + if len(f) != len(t) { + return false + } + + for i := range f { + if f[i].Name != t[i].Name { + return false + } + } + return true + } + } + + return false +} diff --git a/internal/addrs/move_endpoint_module_test.go b/internal/addrs/move_endpoint_module_test.go index bda37ca53..1e2758239 100644 --- a/internal/addrs/move_endpoint_module_test.go +++ b/internal/addrs/move_endpoint_module_test.go @@ -1584,6 +1584,139 @@ func TestSelectsResource(t *testing.T) { } } +func TestIsModuleMoveReIndex(t *testing.T) { + tests := []struct { + from, to AbsMoveable + expect bool + }{ + { + from: mustParseModuleInstanceStr(`module.bar`), + to: mustParseModuleInstanceStr(`module.bar`), + expect: true, + }, + { + from: mustParseModuleInstanceStr(`module.bar`), + to: mustParseModuleInstanceStr(`module.bar[0]`), + expect: true, + }, + { + from: AbsModuleCall{ + Call: ModuleCall{Name: "bar"}, + }, + to: mustParseModuleInstanceStr(`module.bar[0]`), + expect: true, + }, + { + from: mustParseModuleInstanceStr(`module.bar["a"]`), + to: AbsModuleCall{ + Call: ModuleCall{Name: "bar"}, + }, + expect: true, + }, + { + from: mustParseModuleInstanceStr(`module.foo`), + to: mustParseModuleInstanceStr(`module.bar`), + expect: false, + }, + { + from: mustParseModuleInstanceStr(`module.bar`), + to: mustParseModuleInstanceStr(`module.foo[0]`), + expect: false, + }, + { + from: AbsModuleCall{ + Call: ModuleCall{Name: "bar"}, + }, + to: mustParseModuleInstanceStr(`module.foo[0]`), + expect: false, + }, + { + from: mustParseModuleInstanceStr(`module.bar["a"]`), + to: AbsModuleCall{ + Call: ModuleCall{Name: "foo"}, + }, + expect: false, + }, + { + from: mustParseModuleInstanceStr(`module.bar.module.baz`), + to: mustParseModuleInstanceStr(`module.bar.module.baz`), + expect: true, + }, + { + from: mustParseModuleInstanceStr(`module.bar.module.baz`), + to: mustParseModuleInstanceStr(`module.bar.module.baz[0]`), + expect: true, + }, + { + from: mustParseModuleInstanceStr(`module.bar.module.baz`), + to: mustParseModuleInstanceStr(`module.baz.module.baz`), + expect: false, + }, + { + from: mustParseModuleInstanceStr(`module.bar.module.baz`), + to: mustParseModuleInstanceStr(`module.baz.module.baz[0]`), + expect: false, + }, + { + from: mustParseModuleInstanceStr(`module.bar.module.baz`), + to: mustParseModuleInstanceStr(`module.bar[0].module.baz`), + expect: true, + }, + { + from: mustParseModuleInstanceStr(`module.bar[0].module.baz`), + to: mustParseModuleInstanceStr(`module.bar.module.baz[0]`), + expect: true, + }, + { + from: mustParseModuleInstanceStr(`module.bar[0].module.baz`), + to: mustParseModuleInstanceStr(`module.bar[1].module.baz[0]`), + expect: true, + }, + { + from: AbsModuleCall{ + Call: ModuleCall{Name: "baz"}, + }, + to: mustParseModuleInstanceStr(`module.bar.module.baz[0]`), + expect: false, + }, + { + from: mustParseModuleInstanceStr(`module.bar.module.baz[0]`), + to: AbsModuleCall{ + Call: ModuleCall{Name: "baz"}, + }, + expect: false, + }, + { + from: mustParseModuleInstanceStr(`module.baz`), + to: mustParseModuleInstanceStr(`module.bar.module.baz[0]`), + expect: false, + }, + { + from: mustParseModuleInstanceStr(`module.bar.module.baz[0]`), + to: mustParseModuleInstanceStr(`module.baz`), + expect: false, + }, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("[%02d]IsModuleMoveReIndex(%s, %s)", i, test.from, test.to), + func(t *testing.T) { + from := &MoveEndpointInModule{ + relSubject: test.from, + } + + to := &MoveEndpointInModule{ + relSubject: test.to, + } + + if got := IsModuleMoveReIndex(from, to); got != test.expect { + t.Errorf("expected %t, got %t", test.expect, got) + } + }, + ) + } +} + func mustParseAbsResourceInstanceStr(s string) AbsResourceInstance { r, diags := ParseAbsResourceInstanceStr(s) if diags.HasErrors() { From e761117562ec0cbe15ac552089ee93112860898f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 21 Dec 2021 15:40:56 -0500 Subject: [PATCH 2/3] find implied moves in nested modules Implied moves in nested modules were being skipped --- internal/refactoring/move_statement.go | 2 +- internal/refactoring/move_statement_test.go | 47 +++++++++++++++++++ .../child/move-statement-implied.tf | 16 +++++++ .../move-statement-implied.tf | 4 ++ 4 files changed, 68 insertions(+), 1 deletion(-) create mode 100644 internal/refactoring/testdata/move-statement-implied/child/move-statement-implied.tf diff --git a/internal/refactoring/move_statement.go b/internal/refactoring/move_statement.go index a363602c3..08fffeb6f 100644 --- a/internal/refactoring/move_statement.go +++ b/internal/refactoring/move_statement.go @@ -149,7 +149,7 @@ func impliedMoveStatements(cfg *configs.Config, prevRunState *states.State, expl } for _, childCfg := range cfg.Children { - into = findMoveStatements(childCfg, into) + into = impliedMoveStatements(childCfg, prevRunState, explicitStmts, into) } return into diff --git a/internal/refactoring/move_statement_test.go b/internal/refactoring/move_statement_test.go index c6f7c2d79..249d7df7e 100644 --- a/internal/refactoring/move_statement_test.go +++ b/internal/refactoring/move_statement_test.go @@ -18,6 +18,15 @@ func TestImpliedMoveStatements(t *testing.T) { Name: name, }.Absolute(addrs.RootModuleInstance) } + + nestedResourceAddr := func(mod, name string) addrs.AbsResource { + return addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "foo", + Name: name, + }.Absolute(addrs.RootModuleInstance.Child(mod, addrs.NoKey)) + } + instObjState := func() *states.ResourceInstanceObjectSrc { return &states.ResourceInstanceObjectSrc{} } @@ -86,6 +95,19 @@ func TestImpliedMoveStatements(t *testing.T) { instObjState(), providerAddr, ) + + // Add two resource nested in a module to ensure we find these + // recursively. + s.SetResourceInstanceCurrent( + nestedResourceAddr("child", "formerly_count").Instance(addrs.IntKey(0)), + instObjState(), + providerAddr, + ) + s.SetResourceInstanceCurrent( + nestedResourceAddr("child", "now_count").Instance(addrs.NoKey), + instObjState(), + providerAddr, + ) }) explicitStmts := FindMoveStatements(rootCfg) @@ -101,6 +123,19 @@ func TestImpliedMoveStatements(t *testing.T) { End: tfdiags.SourcePos{Line: 5, Column: 32, Byte: 211}, }, }, + + // Found implied moves in a nested module, ignoring the explicit moves + { + From: addrs.ImpliedMoveStatementEndpoint(nestedResourceAddr("child", "formerly_count").Instance(addrs.IntKey(0)), tfdiags.SourceRange{}), + To: addrs.ImpliedMoveStatementEndpoint(nestedResourceAddr("child", "formerly_count").Instance(addrs.NoKey), tfdiags.SourceRange{}), + Implied: true, + DeclRange: tfdiags.SourceRange{ + Filename: "testdata/move-statement-implied/child/move-statement-implied.tf", + Start: tfdiags.SourcePos{Line: 5, Column: 1, Byte: 180}, + End: tfdiags.SourcePos{Line: 5, Column: 32, Byte: 211}, + }, + }, + { From: addrs.ImpliedMoveStatementEndpoint(resourceAddr("now_count").Instance(addrs.NoKey), tfdiags.SourceRange{}), To: addrs.ImpliedMoveStatementEndpoint(resourceAddr("now_count").Instance(addrs.IntKey(0)), tfdiags.SourceRange{}), @@ -112,6 +147,18 @@ func TestImpliedMoveStatements(t *testing.T) { }, }, + // Found implied moves in a nested module, ignoring the explicit moves + { + From: addrs.ImpliedMoveStatementEndpoint(nestedResourceAddr("child", "now_count").Instance(addrs.NoKey), tfdiags.SourceRange{}), + To: addrs.ImpliedMoveStatementEndpoint(nestedResourceAddr("child", "now_count").Instance(addrs.IntKey(0)), tfdiags.SourceRange{}), + Implied: true, + DeclRange: tfdiags.SourceRange{ + Filename: "testdata/move-statement-implied/child/move-statement-implied.tf", + Start: tfdiags.SourcePos{Line: 10, Column: 11, Byte: 282}, + End: tfdiags.SourcePos{Line: 10, Column: 12, Byte: 283}, + }, + }, + // We generate foo.ambiguous[0] to foo.ambiguous here, even though // there's already a foo.ambiguous in the state, because it's the // responsibility of the later ApplyMoves step to deal with the diff --git a/internal/refactoring/testdata/move-statement-implied/child/move-statement-implied.tf b/internal/refactoring/testdata/move-statement-implied/child/move-statement-implied.tf new file mode 100644 index 000000000..87d09c827 --- /dev/null +++ b/internal/refactoring/testdata/move-statement-implied/child/move-statement-implied.tf @@ -0,0 +1,16 @@ +# This fixture is useful only in conjunction with a previous run state that +# conforms to the statements encoded in the resource names. It's for +# TestImpliedMoveStatements only. + +resource "foo" "formerly_count" { + # but not count anymore +} + +resource "foo" "now_count" { + count = 1 +} + +moved { + from = foo.no_longer_present[1] + to = foo.no_longer_present +} diff --git a/internal/refactoring/testdata/move-statement-implied/move-statement-implied.tf b/internal/refactoring/testdata/move-statement-implied/move-statement-implied.tf index 498ead305..4ea628ea6 100644 --- a/internal/refactoring/testdata/move-statement-implied/move-statement-implied.tf +++ b/internal/refactoring/testdata/move-statement-implied/move-statement-implied.tf @@ -48,3 +48,7 @@ resource "foo" "ambiguous" { # set it up to have both no-key and zero-key instances in the # state. } + +module "child" { + source = "./child" +} From 75ef61c7833962038d8832a420a366721cfc2a02 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 21 Dec 2021 15:08:48 -0500 Subject: [PATCH 3/3] check for nested module index changes Changing only the index on a nested module will cause all nested moves to create cycles, since their full addresses will match both the From and To addresses. When building the dependency graph, check if the parent is only changing the index of the containing module, and prevent the backwards edge for the move. --- internal/addrs/move_endpoint_module.go | 40 +++++----------- internal/addrs/move_endpoint_module_test.go | 21 ++++++++- internal/refactoring/move_execute.go | 30 ++++++++++-- internal/refactoring/move_validate_test.go | 52 +++++++++++++++++++++ 4 files changed, 109 insertions(+), 34 deletions(-) diff --git a/internal/addrs/move_endpoint_module.go b/internal/addrs/move_endpoint_module.go index 7ff17621b..fdc8a5c25 100644 --- a/internal/addrs/move_endpoint_module.go +++ b/internal/addrs/move_endpoint_module.go @@ -705,10 +705,10 @@ func (r AbsResourceInstance) MoveDestination(fromMatch, toMatch *MoveEndpointInM } } -// IsModuleMoveReIndex takes the from and to endpoints from a move statement, -// and returns true if the only changes are to module indexes, and all -// non-absolute paths remain the same. -func IsModuleMoveReIndex(from, to *MoveEndpointInModule) bool { +// IsModuleReIndex takes the From and To endpoints from a single move +// statement, and returns true if the only changes are to module indexes, and +// all non-absolute paths remain the same. +func (from *MoveEndpointInModule) IsModuleReIndex(to *MoveEndpointInModule) bool { // The statements must originate from the same module. if !from.module.Equal(to.module) { panic("cannot compare move expressions from different modules") @@ -718,37 +718,21 @@ func IsModuleMoveReIndex(from, to *MoveEndpointInModule) bool { case AbsModuleCall: switch t := to.relSubject.(type) { case ModuleInstance: - if len(t) != 1 { - // An AbsModuleCall only ever has one segment, so the - // ModuleInstance length must match. - return false - } - - return f.Call.Name == t[0].Name + // Generate a synthetic module to represent the full address of + // the module call. We're not actually comparing indexes, so the + // instance doesn't matter. + callAddr := f.Instance(NoKey).Module() + return callAddr.Equal(t.Module()) } case ModuleInstance: switch t := to.relSubject.(type) { case AbsModuleCall: - if len(f) != 1 { - return false - } - - return f[0].Name == t.Call.Name + callAddr := t.Instance(NoKey).Module() + return callAddr.Equal(f.Module()) case ModuleInstance: - // We must have the same number of segments, and the names must all - // match in order for this to solely be an index change operation. - if len(f) != len(t) { - return false - } - - for i := range f { - if f[i].Name != t[i].Name { - return false - } - } - return true + return t.Module().Equal(f.Module()) } } diff --git a/internal/addrs/move_endpoint_module_test.go b/internal/addrs/move_endpoint_module_test.go index 1e2758239..c1643d44c 100644 --- a/internal/addrs/move_endpoint_module_test.go +++ b/internal/addrs/move_endpoint_module_test.go @@ -1686,6 +1686,25 @@ func TestIsModuleMoveReIndex(t *testing.T) { }, expect: false, }, + + { + from: AbsModuleCall{ + Module: mustParseModuleInstanceStr(`module.bar[0]`), + Call: ModuleCall{Name: "baz"}, + }, + to: mustParseModuleInstanceStr(`module.bar.module.baz[0]`), + expect: true, + }, + + { + from: mustParseModuleInstanceStr(`module.bar.module.baz[0]`), + to: AbsModuleCall{ + Module: mustParseModuleInstanceStr(`module.bar[0]`), + Call: ModuleCall{Name: "baz"}, + }, + expect: true, + }, + { from: mustParseModuleInstanceStr(`module.baz`), to: mustParseModuleInstanceStr(`module.bar.module.baz[0]`), @@ -1709,7 +1728,7 @@ func TestIsModuleMoveReIndex(t *testing.T) { relSubject: test.to, } - if got := IsModuleMoveReIndex(from, to); got != test.expect { + if got := from.IsModuleReIndex(to); got != test.expect { t.Errorf("expected %t, got %t", test.expect, got) } }, diff --git a/internal/refactoring/move_execute.go b/internal/refactoring/move_execute.go index 7f5fbae23..9d5b25520 100644 --- a/internal/refactoring/move_execute.go +++ b/internal/refactoring/move_execute.go @@ -238,11 +238,31 @@ func statementDependsOn(a, b *MoveStatement) bool { // // Since we are only interested in checking if A depends on B, we only need // to check the 4 possibilities above which result in B being executed - // first. - return a.From.NestedWithin(b.To) || - a.To.NestedWithin(b.To) || - b.From.NestedWithin(a.From) || - b.To.NestedWithin(a.From) + // first. If we're there's no dependency at all we can return immediately. + if !(a.From.NestedWithin(b.To) || a.To.NestedWithin(b.To) || + b.From.NestedWithin(a.From) || b.To.NestedWithin(a.From)) { + return false + } + + // If a nested move has a dependency, we need to rule out the possibility + // that this is a move inside a module only changing indexes. If an + // ancestor module is only changing the index of a nested module, any + // nested move statements are going to match both the From and To address + // when the base name is not changing, causing a cycle in the order of + // operations. + + // if A is not declared in an ancestor module, then we can't be nested + // within a module index change. + if len(a.To.Module()) >= len(b.To.Module()) { + return true + } + // We only want the nested move statement to depend on the outer module + // move, so we only test this in the reverse direction. + if a.From.IsModuleReIndex(a.To) { + return false + } + + return true } // MoveResults describes the outcome of an ApplyMoves call. diff --git a/internal/refactoring/move_validate_test.go b/internal/refactoring/move_validate_test.go index 53bbbe6c2..60122511f 100644 --- a/internal/refactoring/move_validate_test.go +++ b/internal/refactoring/move_validate_test.go @@ -404,6 +404,58 @@ Each resource can have moved from only one source resource.`, }, WantError: `Resource type mismatch: This statement declares a move from test.nonexist1[0] to other.single, which is a resource instance of a different type.`, }, + "crossing nested statements": { + // overlapping nested moves will result in a cycle. + Statements: []MoveStatement{ + makeTestMoveStmt(t, ``, + `module.nonexist.test.single`, + `module.count[0].test.count[0]`, + ), + makeTestMoveStmt(t, ``, + `module.nonexist`, + `module.count[0]`, + ), + }, + WantError: `Cyclic dependency in move statements: The following chained move statements form a cycle, and so there is no final location to move objects to: + - test:1,1: module.nonexist → module.count[0] + - test:1,1: module.nonexist.test.single → module.count[0].test.count[0] + +A chain of move statements must end with an address that doesn't appear in any other statements, and which typically also refers to an object still declared in the configuration.`, + }, + "fully contained nested statements": { + // we have to avoid a cycle because the nested moves appear in both + // the from and to address of the parent when only the module index + // is changing. + Statements: []MoveStatement{ + makeTestMoveStmt(t, `count`, + `test.count`, + `test.count[0]`, + ), + makeTestMoveStmt(t, ``, + `module.count`, + `module.count[0]`, + ), + }, + }, + "double fully contained nested statements": { + // we have to avoid a cycle because the nested moves appear in both + // the from and to address of the parent when only the module index + // is changing. + Statements: []MoveStatement{ + makeTestMoveStmt(t, `count`, + `module.count`, + `module.count[0]`, + ), + makeTestMoveStmt(t, `count.count`, + `test.count`, + `test.count[0]`, + ), + makeTestMoveStmt(t, ``, + `module.count`, + `module.count[0]`, + ), + }, + }, } for name, test := range tests {