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

"import" block validation is failing on false-positive "duplicates" #33390

Closed
DanielRis opened this issue Jun 19, 2023 · 7 comments · Fixed by #33434
Closed

"import" block validation is failing on false-positive "duplicates" #33390

DanielRis opened this issue Jun 19, 2023 · 7 comments · Fixed by #33434

Comments

@DanielRis
Copy link

DanielRis commented Jun 19, 2023

Terraform Version

v1.5.0

Terraform Configuration Files

module "us-east-1" {
  source = "./modules/name"
  provider = aws.us-east-1
}

module "us-west-2" {
  source = "./modules/name"
  provider = aws.us-west-2
}

import {
    id = "default"
    to = module.us-east-1.module.ebs.aws_ebs_encryption_by_default.default
}

import {
    id = "default"
    to = module.us-west-2.module.ebs.aws_ebs_encryption_by_default.default
}

Debug Output

N/A

Expected Behavior

Terraform should handle each import element individually.

Actual Behavior

Terraform is failing during the init stage:

Error: Duplicate import for ID "default"

  on _auto_import.tf line 181:
 181: import {

An import block for the ID "default" and a resource of type
"aws_ebs_encryption_by_default" was already declared at
_auto_import.tf:41,1-7. The same resource cannot be imported twice.

because it is comparing the id and the type of each import block individually instead of using the full resource identifier that would be required to support child modules.

Steps to Reproduce

  1. terraform init

Additional Context

No response

References

[No response]
https://github.com/hashicorp/terraform/blob/5ed38eb3fa6d44f167a48cea80a1bf2941011e3e/internal/configs/module.go#LL423C16-L423C16

@DanielRis DanielRis added bug new new issue not yet triaged labels Jun 19, 2023
@ghost
Copy link

ghost commented Jun 19, 2023

This can also happen if you have the same resource in multiple providers in the same state - for instance, creating the same resource in multiple AWS accounts. I was about to file this bug with a similar example:

terraform {
  required_version = "~> 1.5.0"
  required_providers {
    aws = {
      version = "~> 5.0"
      source  = "hashicorp/aws"
    }
  }
}

provider "aws" {
  alias  = "acc_a"
  region = "us-east-1"
  assume_role {
    role_arn     = "arn:aws:iam::${ID1}:role/OrganizationAccountAccessRole"
    session_name = "import_bug_test"
  }
}

provider "aws" {
  alias  = "acc_b"
  region = "us-east-1"
  assume_role {
    role_arn     = "arn:aws:iam::${ID2}:role/OrganizationAccountAccessRole"
    session_name = "import_bug_test"
  }
}

resource "aws_ebs_encryption_by_default" "ebs_encryption_a" {
  provider = aws.acc_a
  enabled  = true
}

resource "aws_ebs_encryption_by_default" "ebs_encryption_b" {
  provider = aws.acc_b
  enabled  = true
}

import {
    to = aws_ebs_encryption_by_default.ebs_encryption_a
    id = "default"
}

import {
    to = aws_ebs_encryption_by_default.ebs_encryption_b
    id = "default"
}

But in general, import feels like it should check uniqueness across both the id and the to, and the provider being used. It won't totally stop someone from doing something crazy and ending up with the same resource being "owned" by two Terraform resources, but that seems impossible to effectively accomplish.

For now, this is breaking valid import use-cases for resource types that have static import id values.

@jbardin
Copy link
Member

jbardin commented Jun 20, 2023

Thanks @DanielRis and @pshuman-heb!

The initial validation here assumed that id+"type" is always unique, but you're right about that not always being a valid assumption. The validation could be expanded to include the provider config too, but even then there could be exceptions with more abstract resource types that may not have any uniqueness constraints at all.

The validation may need to be relaxed to check only the target address, which is the unique key as far as Terraform in concerned.

@jbardin jbardin removed the new new issue not yet triaged label Jun 20, 2023
@ghost
Copy link

ghost commented Jun 20, 2023

@jbardin Thanks for the review! In general, it seems like maybe better to remove or significantly relax the validation for now and maybe try to put it back in later after some more review and with a more robust check?

The dangers of someone double-importing feels relatively low - worst case your plans/applies do something odd and you have to do a state rm later to fix it.

The best minimal validation I could think of is probably validating that you're not importing to the same resource multiple times, since that seems like obviously unintended? Trying to validate the id feels like it's making strong assumptions about how the downstream providers have implemented import IDs, which isn't something that seems to be suggested or enforced.

@kmoe kmoe self-assigned this Jun 26, 2023
@kmoe
Copy link
Member

kmoe commented Jun 27, 2023

It looks like individuating import blocks by id and provider in this validation will fix both the cases mentioned.

The dangers of double-importing are the same as any case in which a single remote object is bound to multiple resource addresses (docs 1 2). If my assumptions are correct then we can prevent with validation what was previously always a user error.

The validation could be expanded to include the provider config too, but even then there could be exceptions with more abstract resource types that may not have any uniqueness constraints at all.

@jbardin, do you have an example of this? Given that the ImportResourceState.Request sends only type_name and id to a provider, I'm not sure how sending an identical request to a provider could result in a different remote object.

@jbardin
Copy link
Member

jbardin commented Jun 27, 2023

@kmoe, because Terraform cannot enforce any uniqueness in resources, there is no requirement that identifiers themselves are unique. Multiple resource instances may have the exact same state, but with different addresses they are still separate unique values within a configuration.

Resource also don't need to each represent real physical objects, with some being more abstract in nature, and the identifying string may the same for each of these resources. These abstract resources are frequently duplicated between modules, for instance you could not import a time_sleep instance to avoid triggering side effects from changes if that resource were used in multiple places.

@kmoe
Copy link
Member

kmoe commented Jun 27, 2023

I see, time_sleep does rather break the assumption, because while the provider responds to identical ImportResourceState.Requests with identical Responses, these do not denote the same remote object (since there is no remote object), so it is safe to attach these identical states to distinct resource addresses.

We can relax this validation then and just check for duplicate to addresses (#33434).

Copy link

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.

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

Successfully merging a pull request may close this issue.

3 participants