Merge pull request #30232 from hashicorp/jbardin/module-move-re-index

Handle move blocks within a module which is changing the index

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
 }

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
+}

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() {

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.

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

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

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 {

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
+}

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"
+}