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

Support import blocks in config #31910

Closed
wants to merge 1 commit into from

Conversation

tmccombs
Copy link
Contributor

This adds a new block that looks like:

import {
    to = random_integer.foo
    id = "5,0,10"
 }

To import a resource, when the change is applied.

Fixes #22219

Target Release

1.4.x

Draft CHANGELOG entry

NEW FEATURES

  • Add support for import statements that allow you to import resources as part of a normal apply.

@@ -556,6 +566,7 @@ func (c *Context) planGraph(config *configs.Config, prevRunState *states.State,
RootVariableValues: opts.SetVariables,
Plugins: c.plugins,
Targets: opts.Targets,
ImportTargets: opts.ImportTargets,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping that simply adding the import targets here would be sufficient. However, while it appears that it imports it for the current plan, it doesn't seem to write it back to the state. I'm not sure why, and would probably have to spend a fair amount of time understanding how the planning and applying stages work better in order to fix it.

@tmccombs
Copy link
Contributor Author

Even, if this worked as I intended (it doesn't, quite). This implementation has a couple of limitations:

  1. import blocks are only allowed in the root module
  2. The id has to be a constant value (as opposed to say, letting you get it from a data source).

I think in order to remove 1 you would either also need to remove 2 (thus allowing an import to depend on data sources, variables, and possibly even other resources), or only allow it in submodules that don't use for_each or count anywhere in the path.

I think that removing those restrictions would make this a lot more valuable, because you wouldn't need a separate tool to look up the ids based on some other filter criteria in many cases. And in fact, unless you have that feature, I'm not sure that this has a lot more value than #22227 and probably adds more complexity.

@crw
Copy link
Contributor

crw commented Oct 3, 2022

Hi @tmccombs, thanks for working on this! Just a heads-up that this is a feature that potentially has a lot of overlap with Terraform Cloud/Enterprise, and as such may be difficult to review for a formal merge into main. It would (or will) require coordination and commitment with the product managers on that side of the house. I am hoping to set expectations correctly here, I don't want you to spend a lot of time working on something that may not be able to be merged. Please let us know if you have any questions, and thanks for the thought and effort you are putting into this! We do appreciate it.

@tmccombs
Copy link
Contributor Author

tmccombs commented Oct 3, 2022

Are you saying that there will be a bulk import feature that is specific to Terraform Cloud/Enterprise, and thus having bulk import functionality in the OSS version would interfere with Hashicorp's business, or that such a feature would require changes in the proprietary portions of Terraform Cloud/Enterprise as well, so for this to be merged, that team would also need to commit to working on making those changes?

@crw
Copy link
Contributor

crw commented Oct 3, 2022

As I understand it, the latter rather than the former.

@omarismail
Copy link
Contributor

omarismail commented Oct 3, 2022

Hey @tmccombs,

First off, thank you for putting thought and effort into this! My name is Omar and I'm the PM for Terraform.

To elaborate more on Craig's comment: we are thinking about import, and that there is a chance this PR may not get merged. The intent of this comment is more so as an fyi around that. To solve for bulk import is a big time commitment, and as an open source contributor, it wouldn't feel good to put in a lot of time into something that may not be accepted.

Are you saying that there will be a bulk import feature that is specific to Terraform Cloud/Enterprise, and thus having bulk import functionality in the OSS version would interfere with Hashicorp's business

No. However and whenever bulk import is solved in Terraform, it will be a capability for every Terraform user.

On a completely separate note, I would love to chat with you about import and Terraform in general. Feel free to email me oismail@hashicorp.com :).

@tmccombs
Copy link
Contributor Author

tmccombs commented Oct 4, 2022

Thank you for your responses.

This draft PR is primarily just an exploration of how feasible it would be to implement. I probably won't put any more effort into it at this time, especially given your comments.

@jgoldschrafe
Copy link
Contributor

@omarismail As an observer on this PR, I obviously don't understand the full slate of choices that are going into these decisions. Obviously, TFC/TFE are important to consider here. So what I will say with respect to Terraform Enterprise, as a TFE customer, is that it is now, and has always been, literally impossible to use imports safely with VCS-driven workflows. These workflows are extremely important to companies in regulated industries, and they do not work because this obvious feature, mostly implemented here in this PR, and analogous to the moved block that you have implemented, is missing.

There is no supported workflow in Terraform Enterprise that synchronizes code changes with state modifications like imports. There will always be a gap between the state modification and the code merge—often waiting on a human security reviewer for an arbitrary length of time—in which one of the following two things will happen if a run is triggered:

  • Terraform destroys live infrastructure.
  • Terraform duplicatively creates a resource that already exists and that the code author intended to import.

Because of how locking semantics work, there is no effective way to prevent another team member or repository automation (think Renovate Bot or Dependabot) from making changes while intended imports are in flight. There is literally no functioning process. Teams ask each other nicely not to make changes and hope that no automated systems trigger runs on the workspace.

Like you folks have recognized with resource moves, the only way to resolve this situation is to have Terraform-the-tool consider resource imports in its plan construction model, and render the values to state when a run is completed. As a customer leading a TFE-centric platform team that is bleeding user goodwill because of this mis-design, I strongly encourage not only permitting but prioritizing the inclusion of this feature.

It's extremely disheartening to see someone take a crack at solving this problem and be told that their feature might or might not be considered because of a feature someone is "thinking about" that might or might not exist in Terraform Cloud/Enterprise some day, for some other customer. Maybe the syntax or implementation needs to be stabilized; consider throwing it behind a feature gate. Import-related misbehaviors have cost my colleagues data loss and untold hours of production outages. My executive engineering leadership is taking notice of where Terraform is not fit-for-purpose for our engineering and governance workflows.

Happy to chat more about this - if you'd like to discuss further, Philip Lautman knows how to reach the team here.

@omarismail
Copy link
Contributor

Hey @jgoldschrafe, I have missed this original message and apologies for such a late reply. I appreciate everything you said and we actually see eye-to-eye on the big problems with import today and what is needed to address it.

My original message to the PR author was more so to say that we are working on this problem so as to avoid him spending extra effort that will likely not go anywhere.

As a customer leading a TFE-centric platform team that is bleeding user goodwill because of this mis-design, I strongly encourage not only permitting but prioritizing the inclusion of this feature

We are, and are actively working on something :). Stay tuned!

@crw
Copy link
Contributor

crw commented May 19, 2023

With the addition of #33085, it has come time to close this pull request. Thanks very much for your contribution @tmccombs! It is very appreciated.

@crw crw closed this May 19, 2023
@tmccombs tmccombs deleted the import-statement branch May 19, 2023 23:20
@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to import in bulk
4 participants