Show which input values are unknown when for_each fails due to being unknown #26747
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
Our diagnostics model allows for optionally annotating an error or warning with information about the expression and eval context it was generated from, which the diagnostic renderer for the UI will then use to give the user some additional hints about what values may have contributed to the error. We previously didn't have those annotations on the results of evaluating for_each expressions though, because in that case we were using the helper function to evaluate an expression in one shot and thus we didn't ever have a reference to the EvalContext in order to include it in the diagnostic values. Now, at the expense of having to handle the evaluation at a slightly lower level of abstraction, we'll annotate all of the for_each error messages with source expression information. This is valuable because we see users often confused as to how their complex for_each expressions ended up being invalid, and hopefully giving some information about what the inputs were will allow more users to self-solve.
Previously when printing the relevant variables involved in a failed expression evaluation we would just skip over unknown values entirely. There are some errors, though, which are _caused by_ a value being unknown, in which case it's helpful to show which of the inputs to that expression were known vs. unknown so that the user can limit their further investigation only to the unknown ones. While here I also added a special case for sensitive values that overrides all other display, because we don't know what about a value is sensitive and so better to give nothing away at the expense of a slightly less helpful error message.
Codecov Report
|
alisdair
approved these changes
Oct 29, 2020
Co-authored-by: Alisdair McDiarmid <[email protected]>
|
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 as resolved 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.
(This is another installment of "I had a little time to kill and this has been annoying me.")
When writing complex
for_eachexpressions that collect values from various other places in the configuration, the error message about the value being invalid due to being unknown is particularly frustrating because it has historically not given any guidance as to what might be contributing to the value being unknown.We typically annotate expression evaluation errors with information about the values that might have contributed to them, but we've not been doing that for the
for_eachexpressions because our evaluation codepath was using a sort of "all in one" shortcut method to evaluate the expression and thus didn't have enough information (thehcl.EvalContext, to be specific) to properly annotate the errors.Questions of the form "why is my
for_eachnot known during plan?" come up pretty often, so I think it's worth some slight additional complexity of hopping down to the layer below so we can get hold of thehcl.EvalContextand improve these error messages, and so that's what this PR does. My focus was on the specific example I've described above, but this actually generalizes to any error message involving an expression making use of unknown values from elsewhere, and may therefore prove useful in some other cases by at least letting the user know what the type of each unknown value is.While I was in the neighborhood I also noticed that in the case of sensitive values this would potentially disclose that the value is unknown or null, which might (in rare cases) inadvertently indirectly disclose something important in the logs. For that reason, I added an extra check right up front to disable all further presentation for any value that is sensitive. Finally, I realized that we didn't actually have any tests for this formatting codepath (a result of it starting its life as a "shipped prototype", I guess) and so I took this opportunity to write out some basic test cases too, along with verifying the new behaviors I introduced here.
(There's probably some opportunities to do something similar for the corresponding message for unknown
countvalues, but I've run out of time to work on this today and so I'd like to save that for another time; my sense is thatfor_eachexpressions tend to be the more complicated ones, whilecountexpressions are often simpler and thus less likely to raise questions about what exactly is unknown in the inputs.)