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

resource/aws_cloudhsm2_hsm: Prevent creation of extra HSMs #12435

Closed
wants to merge 1 commit into from

Conversation

mattburgess
Copy link
Collaborator

From https://aws.amazon.com/cloudhsm/faqs/:

Amazon monitors and maintains the HSM and network for availability and error
conditions. If an HSM fails or loses network connectivity, the HSM will be
automatically replaced

When this happens, the replacement HSM joins the cluster with a new HSM ID
but attached to the same ENI ID as the failed HSM. So, track HSM instances
using their ENI ID rather than their HSM ID.

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment 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 pull request followers and do not help prioritize the request

Closes #8648

Release note for CHANGELOG:

resource/aws_cloudhsmv2_hsm: Prevent additional HSMs from being created after the HSM service replaces a faulty HSM [GH-8648]

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

Closes hashicorp#8648

From https://aws.amazon.com/cloudhsm/faqs/:

> Amazon monitors and maintains the HSM and network for availability and error
> conditions. If an HSM fails or loses network connectivity, the HSM will be
> automatically replaced

When this happens, the replacement HSM joins the cluster with a new HSM ID
but attached to the same ENI ID as the failed HSM. So, track HSM instances
using their ENI ID rather than their HSM ID.
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. labels Mar 17, 2020
@mattburgess
Copy link
Collaborator Author

Draft PR raised because:

  1. I've not run the acceptance tests yet; tomorrow hopefully!
  2. I've no idea if https:/terraform-providers/terraform-provider-aws/pull/12435/files#diff-86eb031620ea6d677edc58c55baf9457R93-R99 is the right pattern for handling the fact that the IDs used to track the resources have completed changed

@fprimex
Copy link

fprimex commented May 13, 2020

Hi @mattburgess , how is progress on this PR going?

@mhnsm
Copy link

mhnsm commented Aug 21, 2020

What's needed to push this further forward? Anything I can do?

We've also experienced this issue, over the last weeks, 4 HSM's have been replaced causing drift.

@mattburgess
Copy link
Collaborator Author

Apologies for the lack of progress on this PR folks. I'm struggling to find time to do much with it. But what I can say is that I think that at least https:/hashicorp/terraform-provider-aws/pull/12435/files#diff-b66a2150b744c0017e8e529c2901542700367d503c590df4508efa5ed1ddc86cR93-R97 is wrong.

I think what is needed here is a migration function, so that current TF state that tracks HSMs by ID is migrated, instead, to track HSMs by ENI ID. But I haven't delved enough into how TF migration functions work to implement that yet, nor how to adequately unit test it. Hopefully this is enough to let the next person pick up the baton and run with this. I may come back to it at some point but really can't commit to any timescales.

Base automatically changed from master to main January 23, 2021 00:57
@bflad
Copy link
Contributor

bflad commented Apr 6, 2021

Hi @mattburgess 👋 Thank you for submitting this.

Since changing the resource identifier (id attribute) and resource import identifier are considered breaking changes, we have opted to implement this in a backwards compatible manner by matching via either the HSM ID or ENI ID: #18580

Since the fix will be covered with a different approach, I'm going to close this pull request. Please track the other one for further updates and thanks again.

@bflad bflad closed this Apr 6, 2021
@ghost
Copy link

ghost commented May 6, 2021

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators May 6, 2021
@breathingdust breathingdust removed the needs-triage Waiting for first response or review from a maintainer. label Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
size/XS Managed by automation to categorize the size of a PR.
Projects
None yet
5 participants