Skip to content
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

MoveState clarification and removed+import blocks alternative #1058

Open
lantoli opened this issue Nov 18, 2024 · 3 comments
Open

MoveState clarification and removed+import blocks alternative #1058

lantoli opened this issue Nov 18, 2024 · 3 comments
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Milestone

Comments

@lantoli
Copy link

lantoli commented Nov 18, 2024

Module version

go list -m github.com/hashicorp/terraform-plugin-framework/...
github.com/hashicorp/terraform-plugin-framework v1.13.0

...

Use-cases

Here at MongoDB we want to help customers migrate easily from an old resource type to the new resource type. It's important to note that many of the customers have these resource inside modules. Also they often have some security restrictions that prevent them from using Terraform commands that change file state directly such as "terraform state rm" or "terraform import".

Attempted Solutions

We have some resources in TPF, but these resources are in SDKv2, not in TPF yet. One solution would be to migrate the destination resource to TPF and implement MoveState. This will work but we'll need to introduce some breaking changes in the destination resource because the SDKv2 to TPF migration.
About MoveState, the transformation logic we need to do to go from the old resource schema to the new one is complex. Doing some tests we've found that Read is called in the destination resource just after MoveState (Read is probably called because the new resource being declared in the Terraform files) so the state written in MoveState is overwritten by Read. Can you confirm that we can skip doing the transformation logic, and just read the identifiers from the source schema and set them in the destination to set the attributes to identify the resource so Read can later fill the destination state? (the state must be valid, e.g. all values have to be known or null, but we've found that it's ok to use some fake values except for the id attributes as the state will be filled correctly by Read).

Another solution would be to use a combination of removed and import blocks so we don't need to upgrade resources to TPF. Can you confirm if this would be a valid alternative to MoveState? One issue we've found is that import can not be used inside modules.

Proposal

Allow use of import block inside modules. A problem we've found is that import can only be used from root module, do you know why this restriction exists? Would it be possible to improve Terraform to allow import inside module code? For instance removed and moved blocks work fine inside a module, don't know why import doesn't.
We get this error: "An import block was detected in "module.atlas_basic". Import blocks are only allowed in the root module"
However we can do the same thing having the import block in the the root module, importing the resource inside the module, problem is that this has to be done in each module client code, instead of the import block being centralised in the module code:

import {
  to = module.atlas_basic.mongodbatlas_advanced_cluster.cluster
  id = "${var.project_id}-${var.cluster_name}"
}

Also we propose to clarify MoveState doc to explain the MoveStat / Read operations and there is no need to do a full transformation logic provided this is working as in our assumptions.

Thanks.

References

@lantoli lantoli added the enhancement New feature or request label Nov 18, 2024
@austinvalle
Copy link
Member

austinvalle commented Nov 18, 2024

Hey there @lantoli 👋🏻 , thanks for filing the issue! I'll try to answer each question:

Doing some tests we've found that Read is called in the destination resource just after MoveState (Read is probably called because the new resource being declared in the Terraform files) so the state written in MoveState is overwritten by Read. Can you confirm that we can skip doing the transformation logic, and just read the identifiers from the source schema and set them in the destination to set the attributes to identify the resource so Read can later fill the destination state?

Yeah the moved operation happens outside (and before) Terraform's normal resource lifecycle, similar to the import block, so the first thing existing resources do is refresh (Read), so you should be able to fill in the data that you do know during MoveState, then fill the rest of the data in during Read. All of that makes sense to me 👍🏻

There is a related issue on the Terraform core side about configuration of providers that you might be interested in (a lot of it I'm pulling over here): hashicorp/terraform#35922

Another solution would be to use a combination of removed and import blocks so we don't need to upgrade resources to TPF. Can you confirm if this would be a valid alternative to MoveState?

I believe that would work in the exact same way as moved because they are also refactoring operations which happen outside of Terraform's graph, and import typically relies on the same refresh (Read) to fill data properly.

Allow use of import block inside modules. A problem we've found is that import can only be used from root module, do you know why this restriction exists? Would it be possible to improve Terraform to allow import inside module code?

I don't know the exact technical detail behind why import isn't allowed in child modules, but there is an open issue for it on the Terraform core side: hashicorp/terraform#33474

Also we propose to clarify MoveState doc to explain the MoveStat / Read operations and there is no need to do a full transformation logic provided this is working as in our assumptions.

I think that's a good idea to add to our docs, I'll mark that down to get that added to our next release.


Overall, I think the missing piece from your request that we can't solve on the provider side is the import inside of modules, which you can track/add your use-case to hashicorp/terraform#33474. I'd like to keep this issue open to track documentation fixes to be more explicit about this in our documentation 👍🏻

@austinvalle austinvalle added the documentation Improvements or additions to documentation label Nov 18, 2024
@austinvalle austinvalle added this to the v1.14.0 milestone Nov 18, 2024
@lantoli
Copy link
Author

lantoli commented Nov 19, 2024

thanks @austinvalle for your answers. Can you confirm if we should expect the same behavior in HCP (Terraform cloud offering) and TFE (Terraform on-premise offering)?

About hashicorp/terraform#33474, do you know if there is any estimate? This is critical for us.

@austinvalle
Copy link
Member

No problem!

Can you confirm if we should expect the same behavior in HCP (Terraform cloud offering) and TFE (Terraform on-premise offering)?

Yes I would expect the same to be true (refactoring operations running before the initial refresh) regardless of where your provider is executed. ATM, I can't think of any gotchas or caveats for HCP or TFE that would be a concern here.

About hashicorp/terraform#33474, do you know if there is any estimate? This is critical for us.

I don't know anything about the status of that unfortunately 🙁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants