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..fdc8a5c25 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,37 @@ func (r AbsResourceInstance) MoveDestination(fromMatch, toMatch *MoveEndpointInM panic("unexpected object kind") } } + +// 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") + } + + switch f := from.relSubject.(type) { + case AbsModuleCall: + switch t := to.relSubject.(type) { + case ModuleInstance: + // 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: + callAddr := t.Instance(NoKey).Module() + return callAddr.Equal(f.Module()) + + case ModuleInstance: + return t.Module().Equal(f.Module()) + } + } + + return false +} diff --git a/internal/addrs/move_endpoint_module_test.go b/internal/addrs/move_endpoint_module_test.go index bda37ca53..c1643d44c 100644 --- a/internal/addrs/move_endpoint_module_test.go +++ b/internal/addrs/move_endpoint_module_test.go @@ -1584,6 +1584,158 @@ 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: 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]`), + 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 := from.IsModuleReIndex(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() { diff --git a/internal/refactoring/move_execute.go b/internal/refactoring/move_execute.go index b99da1072..97a9d5c7f 100644 --- a/internal/refactoring/move_execute.go +++ b/internal/refactoring/move_execute.go @@ -242,11 +242,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_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/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 { 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" +}