From f46cf7b8bcaf0e6b580b1c16d496b64cf45a75fc Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 22 Dec 2021 17:32:19 -0500 Subject: [PATCH] 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(), ), )) }