Skip to content

Conversation

@SarahFrench
Copy link
Member

Fixes a bug that was introduced in #37777

That PR introduced some changes to how state_store config is hashed, mainly expanding the inputs that influence the hash value. One of the new inputs is the version of the provider used for PSS.

In the past we have also agreed that the provider version value can be nil when using a builtin or reattached provider.

As a result of these two things, the Hash method could panic when using a builtin or reattached provider resulted in the method receiving a nil pointer for a version.Version. This PR addresses that panic and adds test coverage of nil pointers being tolerated.

Target Release

N/A

Rollback Plan

  • If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

CHANGELOG entry

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

@SarahFrench SarahFrench marked this pull request as ready for review October 31, 2025 15:10
@SarahFrench SarahFrench requested a review from a team as a code owner October 31, 2025 15:10
@SarahFrench SarahFrench added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Oct 31, 2025
radeksimko
radeksimko previously approved these changes Oct 31, 2025
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

👍🏻 good catch

@SarahFrench SarahFrench requested review from a team as code owners November 3, 2025 10:58
@SarahFrench
Copy link
Member Author

I don't know why, but the code consistency check on this PR said that I need to run make syncdeps. This PR is based on the latest commit on main and make syncdeps doesn't result in diffs when run on main 🤷🏻

@SarahFrench SarahFrench marked this pull request as draft November 3, 2025 11:09
@SarahFrench SarahFrench marked this pull request as ready for review November 3, 2025 11:14
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

LGTM, As mentioned in Slack, I narrowed down the dependency changes to the introduction of github.com/hashicorp/terraform/internal/getproviders/reattach import which basically introduces go-plugin as a transitive dependency.

My guess is that the configs package is already a transitive dependency of the remote backend implementations - I just wasn't able to find through which exact package. I can only find configs/configschema which itself does not seem to depend on configs.

So I don't know the exact reason behind it but with backends becoming pluggable I think the idea of them now depending on go-plugin seems generally right.

@SarahFrench
Copy link
Member Author

This PR doesn't really change any dependencies but due to changes in indirect dependencies in backends I ran tests against the 3 major CSP backends:

gcs is ok

% go test github.com/hashicorp/terraform/internal/backend/remote-state/gcs                                              
ok      github.com/hashicorp/terraform/internal/backend/remote-state/gcs        82.361s

s3 is ok

% go test github.com/hashicorp/terraform/internal/backend/remote-state/s3   
ok      github.com/hashicorp/terraform/internal/backend/remote-state/s3 13.791s

azure is ok*

  • Majority of tests pass fine, but as I've noted before there are some tests that I can't run as I don't have permissions to interact with Active Directory. These aren't related to this PR though.

@SarahFrench SarahFrench merged commit f4d0ec5 into main Nov 5, 2025
14 of 15 checks passed
@SarahFrench SarahFrench deleted the pss/fix-hashing-without-provider-version branch November 5, 2025 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog-needed Add this to your PR if the change does not require a changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants