fix invalid null blocks during refresh #32424
Conversation
Legacy providers may return null values for nested blocks during refresh. Because the ReadResource call needs to accept any value to allow the provider to report external changes, we allowed all changes to the value as long as the underlying cty.Type was correct, allowing null block values to be inserted into the state. While technically invalid, we needed to accept these null values for compatibility, and they were mostly seen as a nuisance, causing noise in external changes and plan output. These null block values however can be inserted into the effective configuration with the use of `ignore_changes`, which can cause problems where the configuration is assumed to be completely valid. Rather than accept the null values, we can insert empty container values for these blocks when refreshing the instance, which will prevent any invalid values from entering state at all. Because these must still be accepted for compatibility, we can only log the difference as a warning. Currently the NormalizeObjectFromLegacySDK does not report which specific blocks it fixed, so we just log a generic message.
There was a problem hiding this comment.
This seems good to me but given that it's in the critical path I suggest we land it only in the main branch and not backport it, so it will get more opportunity to soak in prereleases.
(I see you weren't proposing to backport it anyway but just pre-registering an opinion on that in case the question comes up downstream somewhere.)
|
Is it worth discussing whether this sort of logic should also occur with terraform-plugin-framework, since currently the type system and provider response checking does not prevent list/set/single blocks from being stored as null? |
|
From what I can see purely via thought experiment (i.e. I've not actually tested this) I don't think it's super important that the framework respect this rule when it is generating responses, although I imagine it also wouldn't hurt for the framework to respect this rule for consistency with the Terraform Core behavior. So far this idea that nested block types can never appear as null or unknown has been a promise from Terraform Core to providers rather than the other way around. It was motivated by the fact that the legacy SDK (by which I specifically mean the Aside from this gap where we ended up passing a value returned by a provider directly back to the same provider without modification, thereby effectively breaking the promise, I don't think Terraform Core actually worries too much about nested block types appearing as null. From Terraform Core's perspective all of this stuff is just (That is, for example, why it was not a hugely big deal to add support for object types with block-like schema behaviors in protocol version 6; to Terraform Core they don't really look much different than nested block types, aside from the relaxation of the rule that they can never be null or unknown when sent to a provider.) |
|
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. |
Legacy providers may return null values for nested blocks during refresh. Because the ReadResource call needs to accept any value to allow the provider to report external changes, we allowed all changes to the value as long as the underlying cty.Type was correct, allowing null block values to be inserted into the state.
While technically invalid, we needed to accept these null values for compatibility, and they were mostly seen as a nuisance, causing noise in external changes and plan output. These null block values however can be inserted into the effective configuration with the use of
ignore_changes, which can cause problems where the configuration is assumed to be completely valid.Rather than accept the null values, we can insert empty container values for these blocks when refreshing the instance, which will prevent any invalid values from entering state at all. Because these must still be accepted for compatibility, we can only log the difference as a warning. Currently the NormalizeObjectFromLegacySDK does not report which specific blocks it fixed, so we just log a generic message.