From 3d769b7282614db7ccf1a33ec2cdefa0e1465f7c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 21 Dec 2021 14:50:53 -0500 Subject: [PATCH 1/4] 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 deb82daf2bbb28ccaf6d52142b4e74428ca21e72 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 21 Dec 2021 15:40:56 -0500 Subject: [PATCH 2/4] 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 22dc685052178184e998902bf3e881f618b079fb Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 21 Dec 2021 15:08:48 -0500 Subject: [PATCH 3/4] 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 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_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 { From f46cf7b8bcaf0e6b580b1c16d496b64cf45a75fc Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 22 Dec 2021 17:32:19 -0500 Subject: [PATCH 4/4] cleanup some move graph handling Create a separate `validateMoveStatementGraph` function so that `ValidateMoves` and `ApplyMoves` both check the same conditions. Since we're not using the builtin `graph.Validate` method, because we may have multiple roots and want better cycle diagnostics, we need to add checks for self references too. While multiple roots are an error enforced by `Validate` for the concurrent walk, they are OK when using `TransitiveReduction` and `ReverseDepthFirstWalk`, so we can skip that check. Apply moves must first use `TransitiveReduction` to reduce the graph, otherwise nodes may be skipped if they are passed over by a transitive edge. --- internal/refactoring/move_execute.go | 17 +++++-- internal/refactoring/move_validate.go | 69 +++++++++++++++++++-------- 2 files changed, 62 insertions(+), 24 deletions(-) diff --git a/internal/refactoring/move_execute.go b/internal/refactoring/move_execute.go index 97a9d5c7f..db62152f3 100644 --- a/internal/refactoring/move_execute.go +++ b/internal/refactoring/move_execute.go @@ -31,6 +31,10 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults { Blocked: make(map[addrs.UniqueKey]MoveBlocked), } + if len(stmts) == 0 { + return ret + } + // The methodology here is to construct a small graph of all of the move // statements where the edges represent where a particular statement // is either chained from or nested inside the effect of another statement. @@ -39,13 +43,18 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults { g := buildMoveStatementGraph(stmts) - // If there are any cycles in the graph then we'll not take any action - // at all. The separate validation step should detect this and return - // an error. - if len(g.Cycles()) != 0 { + // If the graph is not valid the we will not take any action at all. The + // separate validation step should detect this and return an error. + if diags := validateMoveStatementGraph(g); diags.HasErrors() { + log.Printf("[ERROR] ApplyMoves: %s", diags.ErrWithWarnings()) return ret } + // The graph must be reduced in order for ReverseDepthFirstWalk to work + // correctly, since it is built from following edges and can skip over + // dependencies if there is a direct edge to a transitive dependency. + g.TransitiveReduction() + // The starting nodes are the ones that don't depend on any other nodes. startNodes := make(dag.Set, len(stmts)) for _, v := range g.Vertices() { diff --git a/internal/refactoring/move_validate.go b/internal/refactoring/move_validate.go index 133698608..eedf00414 100644 --- a/internal/refactoring/move_validate.go +++ b/internal/refactoring/move_validate.go @@ -8,6 +8,7 @@ import ( "github.com/hashicorp/hcl/v2" "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/configs" + "github.com/hashicorp/terraform/internal/dag" "github.com/hashicorp/terraform/internal/instances" "github.com/hashicorp/terraform/internal/tfdiags" ) @@ -31,6 +32,10 @@ import ( func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts instances.Set) tfdiags.Diagnostics { var diags tfdiags.Diagnostics + if len(stmts) == 0 { + return diags + } + g := buildMoveStatementGraph(stmts) // We need to track the absolute versions of our endpoint addresses in @@ -200,30 +205,54 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts // validation rules above where we can make better suggestions, and so // we'll use a cycle report only as a last resort. if !diags.HasErrors() { - for _, cycle := range g.Cycles() { - // Reporting cycles is awkward because there isn't any definitive - // way to decide which of the objects in the cycle is the cause of - // the problem. Therefore we'll just list them all out and leave - // the user to figure it out. :( - stmtStrs := make([]string, 0, len(cycle)) - for _, stmtI := range cycle { - // move statement graph nodes are pointers to move statements - stmt := stmtI.(*MoveStatement) - stmtStrs = append(stmtStrs, fmt.Sprintf( - "\n - %s: %s → %s", - stmt.DeclRange.StartString(), - stmt.From.String(), - stmt.To.String(), - )) - } - sort.Strings(stmtStrs) // just to make the order deterministic + diags = diags.Append(validateMoveStatementGraph(g)) + } + return diags +} + +func validateMoveStatementGraph(g *dag.AcyclicGraph) tfdiags.Diagnostics { + var diags tfdiags.Diagnostics + for _, cycle := range g.Cycles() { + // Reporting cycles is awkward because there isn't any definitive + // way to decide which of the objects in the cycle is the cause of + // the problem. Therefore we'll just list them all out and leave + // the user to figure it out. :( + stmtStrs := make([]string, 0, len(cycle)) + for _, stmtI := range cycle { + // move statement graph nodes are pointers to move statements + stmt := stmtI.(*MoveStatement) + stmtStrs = append(stmtStrs, fmt.Sprintf( + "\n - %s: %s → %s", + stmt.DeclRange.StartString(), + stmt.From.String(), + stmt.To.String(), + )) + } + sort.Strings(stmtStrs) // just to make the order deterministic + + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Cyclic dependency in move statements", + fmt.Sprintf( + "The following chained move statements form a cycle, and so there is no final location to move objects to:%s\n\nA 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.", + strings.Join(stmtStrs, ""), + ), + )) + } + + // Look for cycles to self. + // A user shouldn't be able to create self-references, but we cannot + // correctly process a graph with them. + for _, e := range g.Edges() { + src := e.Source() + if src == e.Target() { diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, - "Cyclic dependency in move statements", + "Self reference in move statements", fmt.Sprintf( - "The following chained move statements form a cycle, and so there is no final location to move objects to:%s\n\nA 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.", - strings.Join(stmtStrs, ""), + "The move statement %s refers to itself the move dependency graph, which is invalid. This is a bug in Terraform; please report it!", + src.(*MoveStatement).Name(), ), )) }