core: Simplify our idea of "root node" and require it for DynamicExpand #31880
Conversation
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.
|
The fact that |
|
My focus here was on trying to make all of the existing implementations obey the documented contract rather than changing the contract, so I've not investigated what the consequences would be of changing the contract (if any). I don't personally feel confident in evaluating all of the possible problems that removing this root node might have, since this has been with us since long before I was working on Terraform and so I don't know what exactly depends on it. If we can feel sure that we're already exercising all of the relevant behaviors in our test suite then it could be interesting to give it a try, but I've been burned before by things that have been around long enough that we just take it for granted and don't test it thoroughly in our test suite. 🤔 |
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
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
RootTransformerbeing 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 exactlyterraform.rootNode.Many existing
DynamicExpandimplementations 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 theDynamicExpandreturn value before we try to walk that subgraph. For good measure we also verify that the root node is exactlyterraform.rootNode, even though that isn't strictly required by our graph walker, just to help us catch potential future bugs where aDynamicExpandimplementation neglects to add our singleton root node.This is not a user-facing change; if this alters Terraform's outwardly-visible behavior (apart from the detailed logs) then that's a bug.
Instead, it's addressing an implementation quirk we noticed in review of #31860, where the initial implementation was not producing a valid subgraph in all cases due to the combination of our "clever" behavior in
RootTransformerand the newGraph.Subsumemethod which didn't take that into account.This is therefore another attempt to remove some special cases and overall make Terraform Core's behavior easier to think about: we can now assume that all graphs will contain
terraform.rootNodeand that it will be the only node which has no dependencies itself. We can also assume that all graphs, including dynamic sub-graphs, will be "valid" per the definition in theValidatemethod.I'm intentionally not backporting this into the v1.3 branch because it isn't strictly necessary to fix the previously-discovered bug and so it's better to let this soak during the alpha and beta releases for v1.4 in case there's any unintended consequences we haven't considered yet.