Include sensitive metadata from the schema when building the json state output

[liamcervante]

This PR makes the jsonstate package include the sensitive metadata from the schema when building the sensitive_values entry in the JSON output.

Previously, we were only including information from the config (such as the sensitive()) function. This new behaviour matches the behaviour of the jsonplan package.

I am a little concerned that the tests seem to have been deliberately checking that this didn't happen, but it doesn't make sense to me that we wouldn't want this new behaviour. Basically, the tests were setting an attribute in the schema as sensitive and then were not including that attribute in the sensitive values. I'm hoping that was an oversight instead of something deliberate. I have updated the tests to reflect the new behaviour and they now require the sensitive metadata set from the schema.

Fixes #33055

Target Release

1.4.6

Draft CHANGELOG entry

BUG FIXES

• Fix a bug where the schemas sensitive metadata was being missed when creating the JSON state output for a terraform show command.

[apparentlymart]

I'm not 100% sure because this was quite a while ago now, but I think the odd treatment of statically-defined sensitive might be something related to hashicorp/terraform-plugin-sdk#201 , which ended up in a stalled state because of some disagreement about which layers ought to be dealing with and paying attention to these sensitivity declarations.

Unfortunately I don't have the full context for this anymore so I don't really have a definitive helpful answer here, but I'm mentioning it in case it triggers a stronger memory for a different reviewer who might be able to share some context about whatever decision caused us to be testing for the apparently-incorrect behavior as the expected behavior. 🤔

[jbardin]

My bet here is that as the tests were added in bulk when it was a new package, the tests were written to pass rather than for correctness. The way the values are handled internally led to attributes which are indicated as Sensitive by the schema only being marked during evaluation, hence they don't have marks going into and out of the state. I don't think that was done purposely, rather it was just an evolution of how they used to be handled before we had marks. I think this lead to the new renderer relying on the state representation which had incomplete information.

[jbardin]

@apparentlymart, I don't think that is going to be related. That issue was really about the lack of structural attributes, for which there is a solution now! I don't know if the legacy SDK can or even should be updated to change how the schema is generated, but it is at least possible now.

[apparentlymart]

Thanks for checking!

I don't think there are any plans to change how the legacy SDK behaves now, but indeed you're right that in principle the SDK could choose to present these computed-only objects as structured-type attributes instead of plain object type attributes, if someone did want to change it, with some risk of it subtly changing behavior.

[jbardin]

It's not needed for the issue at hand, but we should probably follow up to make sure the stored state also records sensitive schema values to prevent this from being able to diverge elsewhere.

[github-actions]

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

[github-actions]

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.