Manual backport of core: Use the new namedvals.State API to track input variables, local values, and output values #34477
Conversation
This replaces the direct manipulation of a map shared between three different components, encapsulating that manipulation now inside a single wrapping API that itself ensures safe concurrent access. In future commits we'll do the same for local values and output values, but for now those part of namedvals.State remain unused.
Now that we have the separate namedvals.State type to encapsulate all of the named-value tracking we can simplify the EvalContext API to just return that object directly. This removes the slightly odd evolved API for setting and retrieving input variable values, instead now just calling directly into the relevant namedvals.State methods. It also slightly simplifies some of our test code because there's no longer any need to mock accesses to what is just a temporary in-memory data store anyway.
For a very long time we've had an annoying discrepancy between the in-memory state model and our state snapshot format where the in-memory format stores output values for all modules whereas the snapshot format only tracks the root module output values because those are all we actually need to preserve between runs. That design wart was a result of us using the state both as an internal and an external artifact, due to having nowhere else to store the transient values of non-root module output values while Terraform Core does its work. We now have namedvals.State to internally track all of the throwaway results from named values that don't need to persist between runs, so now we'll use that for our internal work instead and reserve the states.State model only for the data that we will preserve between runs in state snapshots. The namedvals internal model isn't really designed to support enumerating all of the output values for a particular module call, but our expression evaluator currently depends on being able to do that and so we have a temporary inefficient implementation of that which just scans the entire table of values as a stopgap just to avoid this commit growing even larger than it already is. In a future commit we'll rework the evaluator to support the PartialEval mode and at the same time move the responsiblity for enumerating all of the output values into the evaluator itself, since it should be able to determine what it's expecting by analyzing the configuration rather than just by trusting that earlier evaluation has completed correctly. Because our legacy state string serialization previously included output values for all modules, some of our context tests were accidentally depending on the implementation detail of how those got stored internally. Those tests are updated here to test only the data that is a real part of Terraform Core's result, by ensuring that the relevant data appears somewhere either in a root output value or in a resource attribute.
Back when we added local values (a long time ago now!) we put their results in state mainly just because it was the only suitable shared data structure to keep them in. They are a bit ideosyncratic there because we intentionally discard them when serializing state to a snapshot, and that's just fine because they never need to be retained between runs anyway. We now have namedvals.State for all of our named value result storage needs, so we can remove the local-value-related fields of states.Module and use the relevant map inside the local value state instead. As of this commit, states.State now tracks only the data that we actually persist between runs in state snapshots, which will hopefully avoid future bugs resulting from the former difference in fidelity between a freshly-created in-memory state vs. one loaded from a snapshot.
|
🤔 I am slightly concerned here that we'd intentionally waited until after the 1.7 branch was cut because this is a change that touches many parts of the system and we wanted to expose it to further testing during the 1.8 development period. The fact that I apparently accidentally fixed a bug is handy but also an indicator that this set of commits did change the behavior of at least one thing I wasn't intending to change. That of course doesn't mean that there are more examples of that (good or bad), but it does give me pause. I think given how close we are to the 1.7 release (we already reached release candidates) I would be more comfortable with a more surgical fix just for the bug that we're aware of, though I do acknowledge your point about that potentially making backports more challenging. There will be some further changes building on this set (actually implementing the deferred changes idea this was building towards) that will further distance the 1.7 tree from the 1.8 tree, so I think that this would at best delay the backporting challenges only by a month or two. |
|
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. |
Manual backport of #34343 into
v1.7branch. The pre-requisites for the original PR are already in thev1.7branch as they were merged before thev1.7branch was created, which makes this a safe and easy backport operation.This PR had the side effect of fixing the linked issue which means the issue is already fixed in
main. Rather than trying to hack a quick fix into thev1.7branch directly (making future changes and backports potentially harder), I thought it was easiest just to capture this follow up PR as well.I will do another PR that adds a specific test for the issue into main to make sure the fix is captured, which I will backport separately after this backport has been merged.
Fixes #34476
Target Release
1.7.0
Draft CHANGELOG entry
BUG FIXES
terraform test: Fix bug where outputs in "empty" modules were not available to the assertions from Terraform test files.