Experimental support for arbitrary references in custom variable validation rules #34947
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(I ran out of time on this today, so I've pushed it even though the tests aren't quite all passing yet. I hope to work on this more soon.) |
As of this commit this experiment doesn't actually change any behavior at all; as usual, those changes will follow in subsequent commits.
apparentlymart
force-pushed
the
f-var-valid-interactions
branch
from
315c8fa
to
4d32997
Compare
So far we've handled custom validation rules as a part of the same graph
node that is responsible for deciding the final value for an input
variable, but that's bothersome for two reasons:
- There are two different pieces of code both trying to implement the
same rules: the root variable node and the module variable node. Those
have accidentally diverged in the past.
- Each graph node can evaluate expressions only in one module's scope,
and so the module variable node is set up to evaluate in the parent
module's scope because its purpose is to explicitly pass parent module
data into the child module. That is why we haven't yet allowed input
variable validation rules to refer to objects declared in the module
where the variable is declared.
This commit does not actually yet change any real behavior, because this
new graph node has no "Execute" implementation. The focus here is just to
make sure the graph nodes are inserted into the graph with the needed
edges so that a future commit can move the validation logic into here,
and hopefully eventually also allow the validation rules to refer to
objects declared in the child module.
The parent commit added a new node to the graph for each existing input variable node, but that node didn't do anything yet. Now those new nodes are actually responsible for checking the variable's validation rules, instead of that being included in the same graph node that decides the variable's final value. This also refines the validation node graph transformer to only insert a validation node if there's at least one rule to check, which should avoid bloating the graph for anyone who isn't actually using input variable validation. Therefore the plan graph builder tests revert back to their old expected result because that input configuration does not include any validation rules. For the moment this just moves around functionality that already existed and doesn't add anything new, but having the validation handled in a separate graph node means that we can evaluate the validation rules in the EvalContext of the module where the variable was declared instead of where it was defined, and so a future commit will allow -- initially just as an experiment -- validation rules that can refer to other objects in the module where the variable is declared, including the values of other input variables. As a nice bonus, this fixed a small bug in "terraform test" where problems with variable values defined in the test run were reported as problems with the variable declaration, rather than with the expression assigning the value to it. Since it's now NodeRootVariable that decides the definition source range, rather than the validation-rule-checking helper, it has access to the source range provided in a terraform.InputValue object, which was already populated correctly for per-test-run definitions.
When a module opts in to the variable_validation_crossref experiment, Terraform will treat variable validation rules in a similar way as any other dynamic expression, allowing it to refer to other objects that are declared in the same module as the variable. Because the check for a validation rule referring only to itself is now experiments-sensitive we need to do it only once the full module has been assembled, rather than during assembly of individual files, and so the "badref" test in package configs is now classified under "invalid-modules" rather than "invalid-files". It's otherwise unchanged, and the existing test that a validation rule must refer to at least _something_ is still present because that remains true even when opted in to the experiment. This is experimental for now because although it seems pretty plausible that it should be okay, this is allowing full evaluation in a new place where it wasn't allowed before and so we'd like to ask those interested in this feature to try it out with some of their realistic use-cases and make sure this solution is a good fit before we lock it in as a compatibility constraint.
apparentlymart
force-pushed
the
f-var-valid-interactions
branch
from
4d32997
to
0a27311
Compare
|
Okay! I got the few remaining test and check related wrinkles sorted. I think this is ready to go for a first round of alpha feedback now, assuming the changes seem plausible. |
jbardin
approved these changes
Apr 5, 2024
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Back when we first introduced
validationblocks for input variable declarations we ran into an architectural problem where Terraform wasn't built to allow a single graph node to evaluate expressions both in the parent module and in the child module at the same time, and so as an interim compromise we constrained input variable validations to be able to refer only to the variable being checked, and nothing else.It turned out that there were some additional problems in that area too, but #34316 has recently addressed the last of them (as far as I know!) and so this PR completes a little refactoring I started there by finally splitting the input variable processing into a pair of graph nodes for each variable, where the first is responsible for deciding the variable's "final value" and registering its checks, and then the second is responsible for checking the evaluated value against the validation rules.
The final commit here then adds an experimental new version of the validation-checking codepath which uses our normal expression evaluation strategy instead of the special constrained one we've been using so far, and thus allows the expressions in
validationblocks to refer to anything else that's declared in the same module where the variable is declared.This is experimental to start because although my instincts are telling me this should be fine -- this is not really much different than other places in the language where we evaluate dynamic expressions -- I'd prefer to first ask the folks who requested this feature (over in #25609) to try this out and make sure it does actually solve the problem, and more importantly to make sure it doesn't introduce any new problems.
In particular, I'd like to try some of the real examples we learn about in combination with
terraform testsince that leads some slightly different circumstances for variable evaluation that should hopefully be just fine, but I'd like to make sure before this new functionality becomes a compatibility commitment.If we do run into problems with this, I would then like to try for a compromise where we allow referring to other input variables but not to anything else. Hopefully it won't come to that -- again, it seems like this general approach should work -- but the experiment guard will give us the freedom to take that compromise if we need to, so that we might at least meet the main use-cases where it's just about interactions between different variables.
As usual I tried to keep all of the experiment-handling machinery in a single commit -- the final one in this series -- so it'll be easier to find it all and tear it back out later, whether we stabilize the new behavior or remove it and revert back to the previous behavior.