builtin/providers/terraform: Disable remote state file version checks #26692
Conversation
Codecov Report
|
|
This is interesting (and works great! I played with it using consul to write some state with v0.11)! I had assumed - without looking into it, to be clear - that we would handle this by checking compatibility with the statefile version instead of the terraform version. The current version of terraform could for e.g. read a v4 statefile but error on anything less. I definitely am not saying that is the "right" or even "better" way, but I'm curious if you considered it? (I also think that might be the future plan so this is probably pretty ignorable) |
There was a problem hiding this comment.
I left a couple of random thoughts. I think this is really good, and I'm in favor - I know @apparentlymart was going to take a look as well and I think it's better if he gives the 👍 so I'm just going to leave my comments for now.
Yes, I think it makes more sense for 0.15 (and maybe 0.14) to drop the Terraform version check on statefiles altogether, and rely on the statefile format version check. That means that if we need to change the format or its semantics, we will need to bump the version from v4 to v5, but now might be a reasonable time to do that. After opening this PR, I think it's only likely to be worthwhile as a possible change for a late 0.13 release. This would allow 0.13 users to read remote state for all future Terraform versions until we need a v5 statefile version. (The reason we can't remove the Terraform version check in an 0.13 patch release is because sensitivity is changing the format in 0.14.) |
Instead of always checking the Terraform version associated with a state file when reading it, we add a CheckTerraformVersion method and call it from locations where we care about enforcing this check. For this commit, the check has been retained at all call sites for states/statefile.Read, with these exceptions: - Unit tests, which shouldn't care about the state file version; - E2E test runner which should always be using valid state files; - terraform.ShimLegacyState, where the check is pointless as the state file was just created by the current Terraform version.
Add RefreshStateWithoutCheckVersion method to the statemgr.Persistent interface, allowing callers to refresh state from the backend without raising errors if the state's Terraform version is thought to be not fully compatible. This enables use cases where we can be extremely confident that any state file we can read is suitable, such as the Terraform provider's remote state data source, which only reads outputs.
alisdair
force-pushed
the
alisdair/builtin-terraform-provider-disable-version-checks
branch
from
1dd2abd
to
4f367c5
Compare
Allow users of backends to initialize a state manager instance without checking the Terraform version of any state files which are retrieved during this process. Many backends call RefreshState as part of initialization, and this new method instead calls the new RefreshStateWithoutCheckVersion method to prevent version checking.
The builtin Terraform provider's remote state data source uses a configured backend to fetch a given state, in order to allow access to its root module outputs. Until this commit, this was only possible with remote states which are from the current Terraform version or older, forcing multi-state users to carefully orchestrate Terraform upgrades. Building on previous commits in this branch, we now disable this version check, and allow any Terraform state file that the current Terraform version can parse. Since we are only ever accessing root module outputs, this is very likely to be safe for the foreseeable future.
alisdair
force-pushed
the
alisdair/builtin-terraform-provider-disable-version-checks
branch
from
4f367c5
to
7c905c5
Compare
|
Initial review feedback addressed, and rebased/retargeted to the v0.13 branch. |
alisdair
deleted the
alisdair/builtin-terraform-provider-disable-version-checks
branch
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked as resolved and limited conversation to collaborators
The builtin Terraform provider's remote state data source uses a configured backend to fetch a given state, in order to allow access to its root module outputs. Until this change, this was only possible with remote states which are from the current Terraform version or older, forcing multi-state users to carefully orchestrate Terraform upgrades.
We can now disable this version check, and allow any Terraform state file that the current Terraform version can parse. Since we are only ever accessing root module outputs, this is very likely to be safe for the foreseeable future.
Notes to reviewers 📝
I believe this PR will be easier to review one commit at a time. There is some detail in each commit message which may be worth reading. Each commit except the last one should be a no-op, and existing tests pass.
This specific implementation of this idea was intended to be as mechanically simple as possible, because it covers so many files which are difficult to test. The ambition is for reviewers to be confident enough in this change to approve back-porting it to earlier Terraform versions.
The addition of
RefreshStateWithoutCheckVersionandStateMgrWithoutCheckVersionmake me a bit uncomfortable, as does the need to remember to callCheckVersionafter reading a state file wherever appropriate. Ideally I'd like to find another way to achieve this end result before merging this commit, so that we can have a cleaner interface for at least 0.15. That approach could be more invasive, as it would not need to be back-ported. I don't currently have any concrete ideas for this, though—suggestions very welcome!