Warn about potential errors when output interpolation expressions use a counted resource like a singleton #16735
Conversation
Validation is the best time to return detailed diagnostics to the user since we're much more likely to have source location information, etc than we are in later operations. This change doesn't actually add any detail to the messages yet, but it changes the interface so that we can gradually introduce more detailed diagnostics over time. While here there are some minor adjustments to some of the messages to improve their consistency with terminology we use elsewhere.
A common pattern is to conditionally assign to "count" in a resource in order to decide dynamically whether it should be created. In that situation it's necessary to refer to attributes of the resource using the splat syntax, but historically we didn't show errors in output expressions and so people "got away with" incorrect usage in that context. The intent of this warning is to catch potentially-problematic usage of attributes on such resources even if the count happens to be currently set dynamically to 1, which would not generate the error. Then the user can quickly locate and fix the incorrect usage regardless of the current value.
|
LGTM! I can follow up with the opt-out flag to make those new validation warning conditional. |
| // a count might dynamically be set to something | ||
| // other than 1 and thus splat syntax is still needed | ||
| // to be safe. | ||
| if r.RawCount != nil && r.RawCount.Raw != nil && r.RawCount.Raw["count"] != "1" { |
There was a problem hiding this comment.
Since we're only going to conditionally returns errors and can be a little overly ambitious, do we want to catch this with a literal "1" too since that is also something the user could change? I think of there's a count value at all, it indicates that it could be changed, otherwise it would have been left out.
There was a problem hiding this comment.
Oh I see, that's going to always have a raw value. I guess we should skip that rather than mess with the loader too.
There was a problem hiding this comment.
If it were straightforward to recognize presence vs. absense of count then I would've done that here, but this compromise seems okay because the goal of this warning is to catch cases where an error might result from a change of input variables, rather than from a change to the module's own configuration.
As you saw, right now we can't distinguish these cases due to a default applied in the loader, and indeed I didn't want to fuss with that right now since I know from earlier HCL2 work that there is logic in core depending on that default.
|
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
When
countis set to anything other than1on a resource it is necessary to access attributes of the resource via the splat syntax to get a list of all of the results.This has always been an error, but due to the earlier bug that errors were swallowed for outputs there are some existing configs that have erroneous outputs that now fail in 0.11.0. While in some cases this problem is immediately obvious due to Terraform failing to even plan, it's problematic when the dynamic value of the count happens to be
1on the first plan after an upgrade, but then later becomes some other value in a subsequent change: now something that used to work doesn't anymore, and you've already updated your state to 0.11.0 and so downgrading isn't an option.This change is intended as part of the fix for #16726: it emits a warning when an output references a resource with
countusing the singleton form, even if the current dynamic value ofcountis1. Omitting thecountaltogether or specifying the literal value1(as opposed to an expression that evaluates to1) allows this usage without the warning. This means that Terraform will give an early warning about the problematic usage so that it can be corrected immediately, rather than becoming a surprising gotcha after it's already too late to downgrade again.Unfortunately Terraform has never needed to produce warnings at this stage in validation before, so we had no mechanism available to do so. Thus a big part of this PR is a reorganization of the validation code to use our new "diagnostics" abstraction. This was originally intended to come as part of integrating the new config language parser, but we're expediting this change in order to allow for this warning.