Various fixes for "terraform validate" command #18318
Merged
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
Since the intent of the validate command is to check config validity regardless of context (input variables, state, etc), we use unknown values of the requested type here, which will then allow us to complete type checking against the specified types of the variables without assuming any particular values.
Previously we had the evaluate methods accept directly an addrs.InstanceKey and had our evaluator infer a suitable value for count.index for it, but that prevents us from setting the index to be unknown in the validation scenario where we may not be able to predict the number of instances yet but we still want to be able to check that the configuration block is type-safe for all possible count values. To achieve this, we separate the concern of deciding on a value for count.index from the concern of evaluating it, which then allows for other implementations of this in future. For the purpose of this commit there is no change in behavior, with the count.index value being populated whenever the instance key is a number. This commit does a little more groundwork for the future implementation of the for_each feature (which'll support each.key and each.value) but still doesn't yet implement it, leaving it just stubbed out for the moment.
Previously we would attempt to DynamicExpand during the validate walk and then validate each expanded instance separately. However, this meant that we would not be able to validate the contents of a block where count = 0 or if count is not yet known. Here we instead do a more static validation pass against the resource configuration itself, setting count.index to cty.UnknownVal(cty.Number) so we can type-check everything inside the block as being correct regardless of the final count. This is another step towards repairing the "validate" command for our changed assumptions in a world where we have a more sophisticated type checker. This doesn't yet address the remaining problem that the expression evaluator can't, with the current state structures, distinguish between a completed resource with count = 0 and a resource that doesn't exist at all (during validate), and so we'll still get errors if an expression elsewhere in configuration refers to a dynamic index of a resource with "count" set. That's a pre-existing condition that's no longer being masked by _this_ problem, but can't be addressed until we've introduced the new state types (states.State, etc) and thus we _can_ distinguish these two situations. That will therefore be addressed in a later commit.
apparentlymart
force-pushed
the
f-hcl2-validate2
branch
from
e9955a3
to
b2aaa7d
Compare
jbardin
approved these changes
Jun 25, 2018
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
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.
The increased precision of the new type checker has caused some assumptions to shift for
terraform validate, and for the validate walk in general, causing it to be not fully functional at the moment.This PR doesn't quite get it all the way there, since some bigger work needs to be completed first, but this pushes the errors down the stack a couple steps:
Set root variables during
terraform validate: the early pass we did to getterraform validateworking against the new config loader got away with not setting variables because it wasn't actually walking the graph. However, now we are walking the graph we must provide values for all of the root variables in order to prevent a "this is a bug in terraform"-type error message. Here we set them to unknown values of the declared type, allowing us to do type checking of the uses of these values regardless of which particular values are finally set when runningplanorapply.Don't
DynamicExpandduring evaluate: doing a dynamic-expand in the validate walk creates a loophole where we will fail to validate the contents of a resource block which either hascount = 0or has count set to some value that won't be known until the plan walk. Here instead we just validate the configuration block itself, using an unknown number forcount.indexif appropriate, to determine of the configuration is valid for any count. More precise validation then happens during theDynamicExpandin the plan walk.The remaining step here to repair the validate walk is to swap in the new state types and then allow the evaluator to recognize the difference between a completed resource with
count = 0and a resource that has no state at all due to being in the validate walk. That will then allow expressions likenull_resource.example[0]to pass validation, sincenull_resource.examplewould in that case be an unknown list value ofnull_resource's implied object type, allowing type checking to proceed.Once this is working again, we'll be partially down the road towards #15895, though we'll probably want to do some further refactoring afterwards to more cleanly distinguish the different walks. The sequence of work that this PR belongs to is just about getting
terraform validateback into a working state when running against our new expression evaluator.This doesn't yet include any tests for the
terraform validatebreakage, with the intent that we'll do that once it's working end-to-end and thus the tests would actually pass.