Skip to content

ResourceIdentity: Validate that identities do not change after Terraform stores it #1137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 16, 2025

Conversation

austinvalle
Copy link
Member

@austinvalle austinvalle commented May 9, 2025

This PR re-introduces the validation that previously lived in Terraform core (removed in hashicorp/terraform#36989), which ensures that resource identities do not change after Terraform core stores them in state.

The one tricky part of this PR is ReadResource, which the protocol currently does not have sufficient information for us to determine if the state we are refreshing has already been stored (typical use-case) or if we are importing the resource. Rather than loosening how strict the validation is, I added a temporary key to a framework reserved private field, which Terraform passes between ImportResourceState -> ReadResource. Once we've read that field, we can skip validation of the identity, clear the private field, then following refreshes will validate as normal.

As we have the immutable validation in the SDK now, we can allow resources (such as RDS instances) to easily opt-out of that validation when necessary.

There are some corner tests over here which display the immutable error messaging: hashicorp/terraform-provider-corner#340

@austinvalle austinvalle added this to the v1.15.0 milestone May 9, 2025
@austinvalle austinvalle added the enhancement New feature or request label May 9, 2025
@austinvalle austinvalle marked this pull request as ready for review May 12, 2025 13:37
@austinvalle austinvalle requested a review from a team as a code owner May 12, 2025 13:37
@austinvalle austinvalle force-pushed the av/add-immutable-validation branch from c61b1b9 to 4e15fc4 Compare May 13, 2025 14:06
rainkwan
rainkwan previously approved these changes May 13, 2025
Copy link
Contributor

@rainkwan rainkwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to handle the problem of changing identities this way and have the SDK help out with validating the identity/or not when we need to.

ansgarm
ansgarm previously approved these changes May 15, 2025
Copy link
Member

@ansgarm ansgarm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@austinvalle austinvalle dismissed stale reviews from ansgarm and rainkwan via 90ad13b May 15, 2025 18:29
@austinvalle austinvalle force-pushed the av/add-immutable-validation branch from 90ad13b to 0c40969 Compare May 15, 2025 18:30
@austinvalle austinvalle force-pushed the av/add-immutable-validation branch from 0c40969 to d928925 Compare May 16, 2025 12:40
Copy link
Contributor

@SBGoods SBGoods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏾
I don't know if we want to add a note in the private state documentation about the reserved key, since it's highly unlikely that someone would accidentally use that key name. Maybe if we do something like this again, we can add a section about reserved key names.

@austinvalle
Copy link
Member Author

austinvalle commented May 16, 2025

... I don't know if we want to add a note in the private state documentation about the reserved key, since it's highly unlikely that someone would accidentally use that key name. Maybe if we do something like this again, we can add a section about reserved key names.

We're actually already covered because the framework already validates that the . prefix is off-limits 😌
https://developer.hashicorp.com/terraform/plugin/framework/resources/private-state#reserved-keys

SDKv2 doesn't have any documentation about private, but luckily it's blocked from direct usage, and the indirect usage we have control over (timeouts)

@austinvalle austinvalle merged commit fc240f1 into main May 16, 2025
35 checks passed
@austinvalle austinvalle deleted the av/add-immutable-validation branch May 16, 2025 15:06
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 15, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants