Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

terraform: Stabilize the variable_validation_crossref experiment #34955

Merged
merged 2 commits into from
May 22, 2024

Conversation

apparentlymart
Copy link
Member

@apparentlymart apparentlymart commented Apr 5, 2024

In #34947 we introduced a language experiment that would permit variable validation rules to refer to other objects declared in the same module as the variable.

Now that experiment is concluded and its behavior is available for all modules.

This closes #25609.


This final version deviates slightly from the experiment: we learned from the experimental implementation that we accidentally made the terraform validate command able to validate constant-valued input variables in child modules
despite the usual rule that input variables are unknown during validation, because the previous compromise bypassed the main expression evaluator and built its own evaluation context directly.

Even though that behavior was not intended, it's a useful behavior that is protected by our compatibility promises and so this commit includes a slightly hacky emulation of that behavior, in eval_variable.go, that fetches the variable value in the same way the old implementation would have and then modifies the HCL evaluation context to include that value, while preserving anything else that our standard evaluation context builder put in there.

That narrowly preserves the old behavior for expressions that compare the variable value directly to a constant, while treating all other references (which were previously totally invalid) in the standard way. This quirk was already covered by the existing test TestContext2Validate_variableCustomValidationsFail, which fails if the special workaround is removed.

@apparentlymart
Copy link
Member Author

I've prepared this today in the hope that the experiment feedback is positive and we choose to move forward with stabilization, because it's easier to write a stabilization changeset with the experience of implementing the experiment still fresh in mind.

However, I'm going to leave this in draft until we've actually included the experiment in at least one alpha release and gotten some satisfying feedback about it. If feedback suggests changing approach then this I'll close this PR and replace it with another that implements another round of the experiment.

Our real implementation of the lang.Data interface depends on just about
everything else in the modules runtime to implement its behavior, and so
it's not really appropriate to use in unit tests.

This new implementation is considerably simpler, just returning values
predefined in a set of fixed tables. This is appropriate for unit tests of
features that perform expression evaluation as a side-effect of their work
but that don't contribute their own values to the real evaluation data
implementation.
@apparentlymart apparentlymart force-pushed the f-variable-validation-crossref-stablize branch 2 times, most recently from e271e71 to 850825c Compare May 21, 2024 18:15
@apparentlymart apparentlymart requested a review from a team May 21, 2024 22:20
Copy link
Member

@kmoe kmoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One extremely subjective suggestion.

I don't think this needs further documentation as it's something users will try naturally.

@@ -29,9 +29,9 @@ For more information on when to use certain custom conditions, see [Choosing Bet

## Input Variable Validation

-> **Note:** Input variable validation is available in Terraform v0.13.0 and later.
-> **Note:** Input variable validation is available in Terraform v0.13.0 and later. Before Terraform v1.9.0, validation rules can refer only to the variable being validated.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-> **Note:** Input variable validation is available in Terraform v0.13.0 and later. Before Terraform v1.9.0, validation rules can refer only to the variable being validated.
-> **Note:** Input variable validation is available in Terraform v0.13.0 and later. Before Terraform v1.9.0, validation rules can refer only to the variable being validated, not other variables.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable!

I kept this short both because we typically use these callouts mainly to nudge a reader that they might want to refer to the equivalent docs in an older version (now that the docs site supports that) and because older versions of Terraform will generate a suitable error message if someone tries to refer to anything else and so this is here mainly to answer the question "why did the example I found on the internet suggest trying something that returns an error?".

However, "not other variables" is a very reasonable addition that clarifies the point without adding too many more words. I'll update this before I merge.

Previously we introduced a language experiment that would permit variable
validation rules to refer to other objects declared in the same module
as the variable.

Now that experiment is concluded and its behavior is available for all
modules.

This final version deviates slightly from the experiment: we learned from
the experimental implementation that we accidentally made the "validate"
command able to validate constant-valued input variables in child modules
despite the usual rule that input variables are unknown during validation,
because the previous compromise bypassed the main expression evaluator
and built its own evaluation context directly.

Even though that behavior was not intended, it's a useful behavior that is
protected by our compatibility promises and so this commit includes a
slightly hacky emulation of that behavior, in eval_variable.go, that
fetches the variable value in the same way the old implementation would
have and then modifies the hcl evaluation context to include that value,
while preserving anything else that our standard evaluation context
builder put in there. That narrowly preserves the old behavior for
expressions that compare the variable value directly to a constant, while
treating all other references (which were previously totally invalid) in
the standard way. This quirk was already covered by the existing test
TestContext2Validate_variableCustomValidationsFail, which fails if the
special workaround is removed.
@apparentlymart apparentlymart force-pushed the f-variable-validation-crossref-stablize branch from 850825c to 8456322 Compare May 22, 2024 15:09
@apparentlymart apparentlymart merged commit 3b620bf into main May 22, 2024
11 checks passed
@apparentlymart apparentlymart deleted the f-variable-validation-crossref-stablize branch May 22, 2024 15:14
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow variable validation conditions to refer to other variables
2 participants