Validate write-only attributes returned from providers during the UpgradeResourceState RPC

[SarahFrench]

This PR add validation to ensure providers don't return values for write-only attributes during UpgradeResourceState. Terraform core doesn't know if a provider has been updated since the last time the state was updated, and this RPC allows providers to upgrade state to match any schema changes for a particular resource.

The relevant changes for WO attributes are:

1. A field has been changed to become write-only
2. A field has changed to stop being write-only

In case number 2 there is no concern. The state will contain null from when the field was write-only, and then the provider may return a new non-null value on state upgrade.

For case number 1 we don't want a value to be returned to TF core. That is what this PR's changes protects against.

Target Release

1.11.x

CHANGELOG entry

• This change is user-facing and I added a changelog entry.
• This change is not user-facing.

This change is for a feature that is not yet released; no changelog needed

[liamcervante]

Just to make sure I'm not missing something - Terraform should already be storing write-only attributes as null in the state file right? I agree validating that the provider isn't doing something silly is a good idea, but the changes to the mock provider might not be necessary (or even detrimental).

The mock provider returning only the existing state, combined with the check that this isn't allowed, should actually verify that Terraform isn't ever writing real values for write-only attributes. If the mock provider ever fails in a test because it returned a non-null write-only attribute, then we know that there's a bug elsewhere that wrote a non-null write-only attribute in the first place (I think).

[radeksimko]

The mock provider returning only the existing state, combined with the check that this isn't allowed, should actually verify that Terraform isn't ever writing real values for write-only attributes. If the mock provider ever fails in a test because it returned a non-null write-only attribute, then we know that there's a bug elsewhere that wrote a non-null write-only attribute in the first place (I think).

Just to clarify - are you implying that the mock provider should behave the same way as SDK/framework, i.e. always omit write-only attribute values, so then the mocking of the various provider methods wouldn't be necessary as part of each test?

We considered that but initially felt like it would make the tests a bit too smart but I suppose it's fine.

[liamcervante]

Sorry! I misread the PR before!

I don't think I was trying to be so complicated - my understanding is that the state should never be written with any write-only attributes set. So, I'd imagine that both Terraform and the provider SDK/framework validate this (or the provider framework removes the attributes and Terraform validates it).

I had thought the changes on line 641-650 were making it so that for all write-only tests the mock provider was always stripping out the write-only attributes, and I was concerned this might conceal another bug elsewhere if Terraform was for example writing write-only attributes into state when it shouldn't have been. I have read it properly now and I see that it's only been set for exactly one test and not all tests, so that seems fine to me.

(side note, I wish it wouldn't take away the review from everyone else when I only had a question. I'll leave this for someone with more context to actually approve)

[radeksimko]

I had thought the changes on line 641-650 were making it so that for all write-only tests the mock provider was always stripping out the write-only attributes, and I was concerned this might conceal another bug elsewhere if Terraform was for example writing write-only attributes into state when it shouldn't have been. I have read it properly now and I see that it's only been set for exactly one test and not all tests, so that seems fine to me.

Yep, exactly - as mentioned - we considered what you probably thought we did here (i.e. make the MockProvider always behave like in the modified test) - but decided against it for the reasons mentioned. 😄

[SarahFrench]

Thanks @DanielMSchmidt!