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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement: error when resource exists in real infrastructure upon creation #1165

Closed
rickardl opened this issue Feb 29, 2024 · 6 comments 路 Fixed by #1241
Closed

Enhancement: error when resource exists in real infrastructure upon creation #1165

rickardl opened this issue Feb 29, 2024 · 6 comments 路 Fixed by #1241
Assignees

Comments

@rickardl
Copy link

rickardl commented Feb 29, 2024

Community Note

  • Please vote on this issue by adding a 馃憤 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform Version

Terraform v1.6.6 on darwin_amd64

APIC version and APIC Platform

  • V 2.13.2 and all.

Affected Resource(s)

  • aci_user_security_domain

Terraform Configuration Files

resource "aci_user_security_domain" "example1" {
  local_user_dn  = module.local_fgh001.id
  name  = "example"
  annotation = "orchestrator:terraform"
  name_alias = "example_name_alias"
  description = "from Terraform"
}

resource "aci_user_security_domain" "example2" {
  local_user_dn  = module.local_fgh001.id
  name  = "example"
  annotation = "orchestrator:terraform"
  name_alias = "example_name_alias"
  description = "from Terraform"
}

Debug Output

N/A

Panic Output

N/A

Expected Behavior

Terraform should prevent the creation of two aci_user_security_domain resources with identical attributes (specifically name and local_user_dn) or properly handle the conflict by notifying the user that a resource with these parameters already exists.

It should also not end up in a scenario, where the resources tracked by the provider creates a scenario here, removing one also deletes the other one.

The Terraform Cisco ACI and/or provider should either:

  • Prevent creation of duplicate aci_user_security_domain resources with the same name and local_user_dn, or
  • Properly handle the scenario, where ACI API's response when attempting to create a duplicate resource, ideally providing a meaningful error message.

Actual Behavior

Terraform plans and attempts to create two aci_user_security_domain resources with the same name and local_user_dn, leading to an ambiguous state where it seems both are created, but actually, only one exists in ACI. Subsequent Terraform runs show inconsistent behavior regarding the creation and destruction of these resources.

See the tracked ID's here

aci_user_security_domain.example1: Creation complete after 1s [id=uni/userext/user-fgh001/userdomain-example]
aci_user_security_domain.example2: Creation complete after 1s [id=uni/userext/user-fgh001/userdomain-example]

Steps to Reproduce

  1. Define two aci_user_security_domain resources in your Terraform configuration with identical name and local_user_dn attributes.
  2. Run terraform apply to apply the configuration. Notice that Terraform will attempt to create both resources without indicating any conflict.
  3. Observe that only one user security domain is actually created in ACI, but Terraform believes both exist.
  4. Remove or comment out one of the aci_user_security_domain resource definitions and re-run terraform apply. Terraform will now attempt to delete the supposedly existing resource, leading to inconsistencies.
  5. Re-add the previously removed aci_user_security_domain resource to the Terraform configuration and run terraform apply again. Terraform will attempt to create the resource anew, despite it supposedly having existed before.

Important Factoids

The behavior suggests a gap in the solutions handling of resource uniqueness and idempotency, particularly in how it interacts with the ACI API to check for existing resources before attempting to create new ones.

References

  • #0000
@akinross
Copy link
Collaborator

akinross commented Mar 1, 2024

Hi @rickardl,

Thank you for raising this issue. Apologies if you experienced any issues from this behaviour.

This behaviour is done by design, and was chosen because APIC allows us to create/replace the existing configuration and from that moment directly manage it in state. When you would like to validate the changes that you want to apply you could first import the resource into state.

I am aware that some other providers trigger an error message if the object is already existing in real infrastructure during create, but would argue that there is no hard rule defining this behaviour.

From terraform resources documentation, I see the following regarding creation of resources: "Create resources that exist in the configuration but are not associated with a real infrastructure object in the state."

Furthermore in terraform state documentation, the following is mentioned: "Terraform expects that each remote object is bound to only one resource instance in the configuration. If a remote object is bound to multiple resource instances, the mapping from configuration to the remote object in the state becomes ambiguous, and Terraform may behave unexpectedly. Terraform can guarantee a one-to-one mapping when it creates objects and records their identities in the state. When importing objects created outside of Terraform, you must make sure that each distinct object is imported to only one resource instance."

Our interpretation to this is that terraform "expects" each object to be unique in state but there is no definition that errors should be triggered upon creation of resources that are already existing in real infrastructure.

I do not think we should change the existing behaviour, since this would break existing behaviour of the provider. Having stated that, I do think this could be a valuable addition to consider as an enhancement to the provider configuration, where a flag could be set to validate if a DN is already existing in the real infrastructure and would error when it is existing. Would that be an option that could work for you?

@akinross akinross changed the title Duplicate aci_user_security_domain Resources with Identical Names and DNs Lead to Inconsistent Terraform State Management Enhancement: error when resource exists in real infrastructure upon creation Mar 1, 2024
@rickardl
Copy link
Author

rickardl commented Mar 1, 2024

Hi @akinross,

I truly appreciate the comprehensive explanation and the rationale behind the current design choice. Understanding that this behavior is intentionally designed and takes into consideration APIC's capabilities, provides significant clarity.

However, I'd like to emphasize our primary concern regarding the potential catastrophic outcomes stemming from the unintentional deletion of remote resources. Such outcomes may not be anticipated by users, which is troubling. Although Terraform's documentation doesn't explicitly require error messages for pre-existing objects in the real infrastructure, I believe the principle of least surprise should be a cornerstone of our design philosophy. Users generally expect that their actions within a declarative configuration framework should not inadvertently lead to destructive consequences unless explicitly intended.

Consider the following scenario:

An end-user creates two resources:

  • aci_user_security_domain.example1: Creation complete after 1s [id=uni/userext/user-fgh001/userdomain-example]
  • aci_user_security_domain.example2: Creation complete after 1s [id=uni/userext/user-fgh001/userdomain-example]

These resources, both tracking the same remote resource uni/userext/user-fgh001/userdomain-example, are created without any errors. This suggests a deviation from Terraform's guideline, which implies that no single resource under the same state management should be tracked by more than one resource. The aspect of a resource being "imported" or managed outside of the same state is a different matter and perhaps aligns more with your perspective.

However, when one resource is deleted:

  • aci_user_security_domain.example1: [id=uni/userext/user-fgh001/userdomain-example]
  • aci_user_security_domain.example1: Deleted [id=uni/userext/user-fgh001/userdomain-example]

It results in the removal of the remote resource tracked under uni/userext/user-fgh001/userdomain-example for example2, it inadvertently affecting the example1 resource as well.

Our configuration then only has

  • aci_user_security_domain.example1: [id=uni/userext/user-fgh001/userdomain-example]

Upon the next application, terraform will prompt for example1 to be recreated since the existing resource is no longer available, this would also perhaps mean that other resources are tainted as well. It's not likely in this scenario, but the current design opens up for that issue.

In a complex infrastructure setup, this could trigger a domino effect, impacting other interconnected resources. This scenario highlights the need for a design approach that minimizes unexpected and potentially disruptive outcomes, something we'd be happy to contribute towards, both in design and implementation.

Let me know your thoughts, our organization is large nordic customer and this has raised some barrier for us to adopt.

@akinross
Copy link
Collaborator

akinross commented Mar 1, 2024

Hi @rickardl, let me discuss this internally with the team. Do you mind sharing me the customer name, via email (akinross@cisco.com) I will get back to you early next week.

@rickardl
Copy link
Author

rickardl commented Mar 1, 2024

Absolutely I sent you an email, enjoy the weekend!

@bardahlm
Copy link

Is it possible to detect and warn about when multiple Terraform resources manage the same ACI resource? I got hit by a similar issue where I used aci_access_port_block instead of the correct aci_access_sub_port_block, with the result that all breakout ports where removed and not longer working. For me a warning would have helped a lot, as it would allow me to detect the error in plan and not in apply.

@akinross
Copy link
Collaborator

Hi @bardahlm,

Thank you for the suggestion we will take it into consideration when we start working on this issue. Follow the issue thread for any updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants