Address a few inconsistencies in how we process root module input variables vs. called module input variables #29959
Conversation
When going from a local backend to Terraform Cloud, if you skip the `terraform init` and run `terraform apply` this will give the user more clear instructions.
When going from a local backend to Terraform Cloud, if you skip the `terraform init` and run `terraform apply` this will give the user more clear instructions.
When going from a local backend to Terraform Cloud, if you skip the `terraform init` and run `terraform apply` this will give the user more clear instructions.
apparentlymart
force-pushed
the
b-root-variable-conversions
branch
2 times, most recently
from
29be269
to
2747969
Compare
| InputState: prevRunState, | ||
| Changes: changes, | ||
| MoveResults: moveResults, | ||
| RootVariableValues: rootVariables, |
There was a problem hiding this comment.
I believe that the rootVariables argument to the planWalk method is now unused, and unpicking that a bit further leads to removing it from plan, destroyPlan, and refreshOnlyPlan.
There was a problem hiding this comment.
I had indeed intended to remove that entirely here, so that the variable values are used directly only during graph construction and not during the walk. However, I can see now that I got a bit lost in the noise and neglected to clean up the top-level mentions of it, even though (as you noted) the deeper stuff isn't referring to it anymore. Thanks! I'll see about cleaning that up.
There was a problem hiding this comment.
Oh, looking closer I notice that the change you suggested is valid but not for the reason I explained in my previous comment: planWalk does include the graph construction and so it does still need the variables, but it now gets them from the opts *PlanOpts argument along with all of the other plan options, rather than needing a separate argument to carry them.
| func TestPrepareFinalInputVariableValue(t *testing.T) { | ||
| // This is just a concise way to define a bunch of *configs.Variable | ||
| // objects to use in our tests below. We're only going to decode this | ||
| // config, not fully evaluate it. |
This test seems to be a holdover from the many-moons-ago switch from one graph for all operations to separate graphs for plan and apply. It is effectively just a copy of a subset of the content of the Context.Validate function and is a maintainability hazard because it tends to lag behind updates to that function unless changes there happen to make it fail. This test doesn't cover anything that the other validate context tests don't exercise as an implementation detail of calling Context.Validate, so I've just removed it with no replacement.
Previously we had a significant discrepancy between these two situations: we wrote the raw root module variables directly into the EvalContext and then applied type conversions only at expression evaluation time, while for child modules we converted and validated the values while visiting the variable graph node and wrote only the _final_ value into the EvalContext. This confusion seems to have been the root cause for #29899, where validation rules for root module variables were being applied at the wrong point in the process, prior to type conversion. To fix that bug and also make similar mistakes less likely in the future, I've made the root module variable handling more like the child module variable handling in the following ways: - The "raw value" (exactly as given by the user) lives only in the graph node representing the variable, which mirrors how the _expression_ for a child module variable lives in its graph node. This means that the flow for the two is the same except that there's no expression evaluation step for root module variables, because they arrive as constant values from the caller. - The set of variable values in the EvalContext is always only "final" values, after type conversion is complete. That in turn means we no longer need to do "just in time" conversion in evaluationStateData.GetInputVariable, and can just return the value exactly as stored, which is consistent with how we handle all other references between objects. This diff is noisier than I'd like because of how much it takes to wire a new argument (the raw variable values) through to the plan graph builder, but those changes are pretty mechanical and the interesting logic lives inside the plan graph builder itself, in NodeRootVariable, and the shared helper functions in eval_variable.go. While here I also took the opportunity to fix a historical API wart in EvalContext, where SetModuleCallArguments was built to take a set of variable values all at once but our current caller always calls with only one at a time. That is now just SetModuleCallArgument singular, to match with the new SetRootModuleArgument to deal with root module variables.
|
@alisdair's note about the unused value As far as I can tell that extra call was relatively harmless but wasn't explicitly taking into account |
apparentlymart
force-pushed
the
b-root-variable-conversions
branch
2 times, most recently
from
93f6286
to
d2faa71
Compare
Previously we had three different layers all thinking they were responsible for substituting a default value for an unset root module variable: - the local backend, via logic in backend.ParseVariableValues - the context.Plan function (and other similar functions) trying to preprocess the input variables using terraform.mergeDefaultInputVariableValues . - the newer prepareFinalInputVariableValue, which aims to centralize all of the variable preparation logic so it can be common to both root and child module variables. The second of these was also trying to handle type constraint checking, which is also the responsibility of the central function and not something we need to handle so early. Only the last of these consistently handles both root and child module variables, and so is the one we ought to keep. The others are now redundant and are causing prepareFinalInputVariableValue to get a slightly corrupted view of the caller's chosen variable values. To rectify that, here we remove the two redundant layers altogether and have unset root variables pass through as cty.NilVal all the way to the central prepareFinalInputVariableValue function, which will then handle them in a suitable way which properly respects the "nullable" setting. This commit includes some test changes in the terraform package to make those tests no longer rely on the mergeDefaultInputVariableValues logic we've removed, and to instead explicitly set cty.NilVal for all unset variables to comply with our intended contract for PlanOpts.SetVariables, and similar. (This is so that we can more easily catch bugs in callers where they _don't_ correctly handle input variables; it allows us to distinguish between the caller explicitly marking a variable as unset vs. not describing it at all, where the latter is a bug in the caller.)
In earlier Terraform versions we had an extra validation step prior to the graph walk which tried to partially validate root module input variable values (just checking their type constraints) and then return error messages which specified as accurately as possible where the value had originally come from. We're now handling that sort of validation exclusively during the graph walk so that we can share the main logic between both root module and child module variable values, but previously that shared code wasn't able to generate such specific information about where the values had originated, because it was adapted from code originally written to only deal with child module variables. Here then we restore a similar level of detail as before, when we're processing root module variables. For child module variables, we use synthetic InputValue objects which state that the value was declared in the configuration, thus causing us to produce a similar sort of error message as we would've before which includes a source range covering the argument expression in the calling module block.
apparentlymart
force-pushed
the
b-root-variable-conversions
branch
from
26d341b
to
d1de8ba
Compare
|
Hi again, @alisdair! Thanks to you spotting that extra redundant argument in your previous review, I noticed that we had two other layers that were still partially trying to handle default values and type conversions for root module variables, but again (just like the problem I was originally trying to address here) were doing so non-comprehensively and thus just creating a muddled, uncertain situation for any code that followed about what condition the variable values might be in. Since my original mission here was to centralize all of the variable-final-value resolution logic into one place, I've added an additional pair of commits here which aims to remove those two extra partial variable processing attempts: one in the local backend and another in the I intentionally retained the perhaps-unexpected requirement that the caller into As part of doing that I noticed that the The original commits you already reviewed are unchanged here except for responding to the feedback you left before, so I'd suggest using the individual commit view to focus on the two new commits and thus avoid retreading the stuff you already looked at. |
| // three different places while child module variables were only handled | ||
| // during the Terraform Core graph walk. | ||
| // | ||
| // For Terraform v1.2 and later we rationalized that by having the Terraform |
There was a problem hiding this comment.
In #30312 I came to mostly the same functional conclusions you did here about module variables, but opted to unify handling in the module exec node. Dealing with the default feels very much outside the graph building responsibilities of creating and connecting the nodes, and I definitely found the fake expression which was injected before any evaluation very surprising.
If that's something which can be fixed but you want to leave that for later to keep this PR from growing, I can wait for this to land and do the refactoring later.
There was a problem hiding this comment.
I'm not exactly sure what you're referring to here, probably at least in part because I've not been looking at this PR for several weeks and so I no longer have the context loaded 😀 , but I would generally prefer to avoid growing the scope of this even more if it seems correct enough, since it already sprawled a lot more than I wanted and is at high risk of becoming conflicted as we start other work. If you do have ideas for further improvements then I'd love to discuss them in a followup PR though, assuming that would be motivated by maintainability rather than by correctness. (I say that just because my original intent here was to fix a bug, and I only ended up refactoring when I learned that the bug was caused by a design problem.)
I believe my thought process here was that we ought to never have included the default values in this JSON plan output at all, and that they only really ended up here as an unintended side-effect of a strange separation of concerns elsewhere. The place to look for default values for variables would be a hypothetical JSON serialization of the configuration, because default values belong to the configuration rather than to the plan. Ideally it should be possible to see from this JSON plan output whether a particular variable was set or not, and perhaps one day we'll find some way to opt-in to that (arguably) better behavior, and so this special case is here to keep this oddity isolated out here in the one system we'd be changing anyway if we were to "fix" this, rather than having this backward-compatibility shim effectively spread across multiple subsystems.
There was a problem hiding this comment.
That's totally reasonable. I can follow up and see if anything from 30312 is still applicable afterwards, but the changes here also seem to fix the processing order for module variables.
|
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. |
This should close #29899 along with just generally creating some better consistency in how we handle root module input variables as compared to called module input variables, to make sure we're following an equivalent workflow in each case.
The starting idea here is to make the user-provided value (prior to type conversion and validation) be attached to the graph node representing a root module variable, which corresponds with our strategy of including in a called module variable node the expression that will define that variable. We can then follow the same steps during evaluation aside from skipping the expression evaluation in the root module scenario, because root module variables always arrive as concrete values.
Another level of consistency which builds on the above is that the set of variables kept inside the
terraform.EvalContextis always the so-called "final" values, after type conversion and validation. Previously that was true for called module variables but not for root module variables, which I believe was the inconsistency that led to the bug described in #29899.This diff is noisier than I'd like because of how much it takes to wire a new argument (the raw variable values) through to the graph builders, and the need to collect the variable values a little earlier so they're ready to pass to the graph builders, but those changes are pretty mechanical and the interesting logic lives inside the plan graph builder itself, in
NodeRootVariable, and the shared helper functions ineval_variable.go. My main goal is for bothNodeRootVariableandNodeModuleVariableto have the same structure where the "source value" lives inside the node itself, and the graph node uses that to prepare the "final value" to save in theEvalContext.NodeRootVariablehas a constantcty.Value(wrapped insideterraform.InputValue) whereasNodeModuleVariablehas a dynamichcl.Expression, but the same general principle applies.While here I also took the opportunity to fix a historical API wart in
EvalContext, whereSetModuleCallArgumentswas built to take a set of variable values all at once but our current caller always calls with only one at a time. That is now justSetModuleCallArgumentsingular, to match with the newSetRootModuleArgumentto deal with root module variables.