From c1867f98e3a42dd55fffe069f86f3aca93e8ba7c Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 27 Sep 2022 16:25:04 -0700 Subject: [PATCH] core: Simplify our idea of "root node" and require it for DynamicExpand The graph walking mechanism is specified as requiring a graph with a single root, which in practice means there's exactly one node in the graph which doesn't have any dependencies. However, we previously weren't verifying that invariant is true for subgraphs returned from DynamicExpand. It was working anyway, but it's not ideal to be relying on a behavior that isn't guaranteed by our underlying infrastructure. We also previously had the RootTransformer being a bit clever and trying to avoid adding a new node if there is already only a single graph with no dependencies. That special case isn't particularly valuable since there's no harm in turning a one-node graph into a two-node graph with an explicit separate root node, and doing that allows us to assume that the root node is always present and is always exactly terraform.rootNode. Many existing DynamicExpand implementations were not producing valid graphs and were previously getting away with it. All of them now produce properly-rooted graphs that should pass validation, and we will guarantee that with an explicit check of the DynamicExpand return value before we try to walk that subgraph. For good measure we also verify that the root node is exactly terraform.rootNode, even though that isn't strictly required by our graph walker, just to help us catch potential future bugs where a DynamicExpand implementation neglects to add our singleton root node. --- internal/terraform/graph.go | 23 ++++++ internal/terraform/node_local.go | 1 + internal/terraform/node_module_variable.go | 1 + internal/terraform/node_output.go | 1 + internal/terraform/node_resource_apply.go | 1 + internal/terraform/node_resource_import.go | 6 +- internal/terraform/node_resource_plan.go | 2 + internal/terraform/transform_root.go | 49 ++++++----- internal/terraform/transform_root_test.go | 94 ++++++++++++++-------- 9 files changed, 119 insertions(+), 59 deletions(-) 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 = `