diff --git a/internal/terraform/graph.go b/internal/terraform/graph.go index cc99942093c6..242ac0429b7c 100644 --- a/internal/terraform/graph.go +++ b/internal/terraform/graph.go @@ -1,6 +1,7 @@ package terraform import ( + "fmt" "log" "strings" @@ -88,6 +89,28 @@ func (g *Graph) walk(walker GraphWalker) tfdiags.Diagnostics { return } if g != nil { + // The subgraph should always be valid, per our normal acyclic + // graph validation rules. + if err := g.Validate(); err != nil { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Graph node has invalid dynamic subgraph", + fmt.Sprintf("The internal logic for %q generated an invalid dynamic subgraph: %s.\n\nThis is a bug in Terraform. Please report it!", dag.VertexName(v), err), + )) + return + } + // If we passed validation then there is exactly one root node. + // That root node should always be "rootNode", the singleton + // root node value. + if n, err := g.Root(); err != nil || n != dag.Vertex(rootNode) { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Graph node has invalid dynamic subgraph", + fmt.Sprintf("The internal logic for %q generated an invalid dynamic subgraph: the root node is %T, which is not a suitable root node type.\n\nThis is a bug in Terraform. Please report it!", dag.VertexName(v), v), + )) + return + } + // Walk the subgraph log.Printf("[TRACE] vertex %q: entering dynamic subgraph", dag.VertexName(v)) subDiags := g.walk(walker) diff --git a/internal/terraform/node_local.go b/internal/terraform/node_local.go index 79b47576822c..f194b9cc82a2 100644 --- a/internal/terraform/node_local.go +++ b/internal/terraform/node_local.go @@ -73,6 +73,7 @@ func (n *nodeExpandLocal) DynamicExpand(ctx EvalContext) (*Graph, error) { log.Printf("[TRACE] Expanding local: adding %s as %T", o.Addr.String(), o) g.Add(o) } + addRootNodeToGraph(&g) return &g, nil } diff --git a/internal/terraform/node_module_variable.go b/internal/terraform/node_module_variable.go index c5e2294eaadb..6d5ae2af89cb 100644 --- a/internal/terraform/node_module_variable.go +++ b/internal/terraform/node_module_variable.go @@ -50,6 +50,7 @@ func (n *nodeExpandModuleVariable) DynamicExpand(ctx EvalContext) (*Graph, error } g.Add(o) } + addRootNodeToGraph(&g) return &g, nil } diff --git a/internal/terraform/node_output.go b/internal/terraform/node_output.go index 5ce32c244582..2a1f4fdb9999 100644 --- a/internal/terraform/node_output.go +++ b/internal/terraform/node_output.go @@ -96,6 +96,7 @@ func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) { log.Printf("[TRACE] Expanding output: adding %s as %T", o.Addr.String(), o) g.Add(o) } + addRootNodeToGraph(&g) if checkableAddrs != nil { checkState := ctx.Checks() diff --git a/internal/terraform/node_resource_apply.go b/internal/terraform/node_resource_apply.go index 3928bea0fdc5..6f7b46af6e9a 100644 --- a/internal/terraform/node_resource_apply.go +++ b/internal/terraform/node_resource_apply.go @@ -49,6 +49,7 @@ func (n *nodeExpandApplyableResource) DynamicExpand(ctx EvalContext) (*Graph, er Addr: n.Addr.Resource.Absolute(module), }) } + addRootNodeToGraph(&g) return &g, nil } diff --git a/internal/terraform/node_resource_import.go b/internal/terraform/node_resource_import.go index ecf39a07e033..93b14fdf18a6 100644 --- a/internal/terraform/node_resource_import.go +++ b/internal/terraform/node_resource_import.go @@ -176,11 +176,7 @@ func (n *graphNodeImportState) DynamicExpand(ctx EvalContext) (*Graph, error) { }) } - // Root transform for a single root - t := &RootTransformer{} - if err := t.Transform(g); err != nil { - return nil, err - } + addRootNodeToGraph(g) // Done! return g, diags.Err() diff --git a/internal/terraform/node_resource_plan.go b/internal/terraform/node_resource_plan.go index 18c97c7cb9a9..3c6e94d8bf9d 100644 --- a/internal/terraform/node_resource_plan.go +++ b/internal/terraform/node_resource_plan.go @@ -171,6 +171,8 @@ func (n *nodeExpandPlannableResource) DynamicExpand(ctx EvalContext) (*Graph, er checkState.ReportCheckableObjects(n.NodeAbstractResource.Addr, instAddrs) } + addRootNodeToGraph(&g) + return &g, diags.ErrWithWarnings() } diff --git a/internal/terraform/transform_root.go b/internal/terraform/transform_root.go index b22ef11fc0f7..e06ef5b414cf 100644 --- a/internal/terraform/transform_root.go +++ b/internal/terraform/transform_root.go @@ -10,41 +10,48 @@ const rootNodeName = "root" type RootTransformer struct{} func (t *RootTransformer) Transform(g *Graph) error { - // If we already have a good root, we're done - if _, err := g.Root(); err == nil { - return nil - } + addRootNodeToGraph(g) + return nil +} - // We intentionally add a graphNodeRoot value -- rather than a pointer to - // one -- so that all root nodes will coalesce together if two graphs - // are merged. Each distinct node value can only be in a graph once, - // so adding another graphNodeRoot value to the same graph later will - // be a no-op and all of the edges from root nodes will coalesce together - // under Graph.Subsume. +// addRootNodeToGraph modifies the given graph in-place so that it has a root +// node if it didn't already have one and so that any other node which doesn't +// already depend on something will depend on that root node. +// +// After this function returns, the graph will have only one node that doesn't +// depend on any other nodes. +func addRootNodeToGraph(g *Graph) { + // We always add the root node. This is a singleton so if it's already + // in the graph this will do nothing and just retain the existing root node. // - // It's important to retain this coalescing guarantee under future - // maintenence. - var root graphNodeRoot - g.Add(root) + // Note that rootNode is intentionally added by value and not by pointer + // so that all root nodes will be equal to one another and therefore + // coalesce when two valid graphs get merged together into a single graph. + g.Add(rootNode) - // We initially make the root node depend on every node except itself. - // If the caller subsequently runs transitive reduction on the graph then - // it's typical for some of these edges to then be removed. + // Everything that doesn't already depend on at least one other node will + // depend on the root node, except the root node itself. for _, v := range g.Vertices() { - if v == root { + if v == dag.Vertex(rootNode) { continue } if g.UpEdges(v).Len() == 0 { - g.Connect(dag.BasicEdge(root, v)) + g.Connect(dag.BasicEdge(rootNode, v)) } } - - return nil } type graphNodeRoot struct{} +// rootNode is the singleton value representing all root graph nodes. +// +// The root node for all graphs should be this value directly, and in particular +// _not_ a pointer to this value. Using the value directly here means that +// multiple root nodes will always coalesce together when subsuming one graph +// into another. +var rootNode graphNodeRoot + func (n graphNodeRoot) Name() string { return rootNodeName } diff --git a/internal/terraform/transform_root_test.go b/internal/terraform/transform_root_test.go index 4a426b5e7cc2..61f24a5f764a 100644 --- a/internal/terraform/transform_root_test.go +++ b/internal/terraform/transform_root_test.go @@ -8,50 +8,78 @@ import ( ) func TestRootTransformer(t *testing.T) { - mod := testModule(t, "transform-root-basic") + t.Run("many nodes", func(t *testing.T) { + mod := testModule(t, "transform-root-basic") - g := Graph{Path: addrs.RootModuleInstance} - { - tf := &ConfigTransformer{Config: mod} - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) + g := Graph{Path: addrs.RootModuleInstance} + { + tf := &ConfigTransformer{Config: mod} + if err := tf.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } } - } - { - transform := &MissingProviderTransformer{} - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) + { + transform := &MissingProviderTransformer{} + if err := transform.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } } - } - { - transform := &ProviderTransformer{} - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) + { + transform := &ProviderTransformer{} + if err := transform.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + { + transform := &RootTransformer{} + if err := transform.Transform(&g); err != nil { + t.Fatalf("err: %s", err) + } + } + + actual := strings.TrimSpace(g.String()) + expected := strings.TrimSpace(testTransformRootBasicStr) + if actual != expected { + t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) } - } - { - transform := &RootTransformer{} - if err := transform.Transform(&g); err != nil { + root, err := g.Root() + if err != nil { t.Fatalf("err: %s", err) } - } + if _, ok := root.(graphNodeRoot); !ok { + t.Fatalf("bad: %#v", root) + } + }) + + t.Run("only one initial node", func(t *testing.T) { + g := Graph{Path: addrs.RootModuleInstance} + g.Add("foo") + addRootNodeToGraph(&g) + got := strings.TrimSpace(g.String()) + want := strings.TrimSpace(` +foo +root + foo +`) + if got != want { + t.Errorf("wrong final graph\ngot:\n%s\nwant:\n%s", got, want) + } + }) - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(testTransformRootBasicStr) - if actual != expected { - t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected) - } + t.Run("graph initially empty", func(t *testing.T) { + g := Graph{Path: addrs.RootModuleInstance} + addRootNodeToGraph(&g) + got := strings.TrimSpace(g.String()) + want := `root` + if got != want { + t.Errorf("wrong final graph\ngot:\n%s\nwant:\n%s", got, want) + } + }) - root, err := g.Root() - if err != nil { - t.Fatalf("err: %s", err) - } - if _, ok := root.(graphNodeRoot); !ok { - t.Fatalf("bad: %#v", root) - } } const testTransformRootBasicStr = `