provider: Introduce shared IAM propagation timeout, refactor KMS Key creation to that timeout and add KMS Key deletion to internal waiter package #12863
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…creation to that timeout and add KMS Key deletion to internal waiter package Reference: #7646 Reference: #12840 The Terraform AWS Provider codebase contains many varied timeouts for handling IAM propagation retries. Here we introduce a shared constant for this amount of time. The choice of 2 minutes is based on that amount of time being: - Most widely used across resources - Based on lack of historical bug reports across those resources that implement that amount of time, most successful - Ensuring a reasonable user experience (not waiting too long) should there be an actual misconfiguration As an initial implementation of this IAM propagation timeout and further showing the potential waiter package refactoring, this fixes shorter IAM timeout implementations in the `aws_kms_key` and `aws_kms_external_key` resources, while also refactoring the pending deletion logic. This second change is designed as an inflection point for how we want to handle imports across multiple waiter packages, with the preference of this initial implementation to name the Go import of the outside service, `iamwaiter`, or generically SERVICEwaiter. If agreed, this will be added to the proposal and the refactoring documentation. NOTE: There is other `StateChangeConf` / `StateRefreshFunc` logic in these KMS resources, but this change is solely focused on highlighting the multiple import situation, and those will be handled later. Output from acceptance testing: ``` --- PASS: TestAccAWSKmsExternalKey_basic (19.53s) --- PASS: TestAccAWSKmsExternalKey_DeletionWindowInDays (31.61s) --- PASS: TestAccAWSKmsExternalKey_Description (32.11s) --- PASS: TestAccAWSKmsExternalKey_disappears (13.84s) --- PASS: TestAccAWSKmsExternalKey_Enabled (312.55s) --- PASS: TestAccAWSKmsExternalKey_KeyMaterialBase64 (104.29s) --- PASS: TestAccAWSKmsExternalKey_Policy (33.78s) --- PASS: TestAccAWSKmsExternalKey_Tags (43.70s) --- PASS: TestAccAWSKmsExternalKey_ValidTo (165.77s) --- PASS: TestAccAWSKmsKey_asymmetricKey (18.20s) --- PASS: TestAccAWSKmsKey_basic (21.13s) --- PASS: TestAccAWSKmsKey_disappears (13.92s) --- PASS: TestAccAWSKmsKey_isEnabled (236.91s) --- PASS: TestAccAWSKmsKey_policy (35.34s) --- PASS: TestAccAWSKmsKey_Policy_IamRole (34.14s) --- PASS: TestAccAWSKmsKey_Policy_IamServiceLinkedRole (44.80s) --- PASS: TestAccAWSKmsKey_tags (34.65s) ```
ghost
added
size/XL
bflad
removed
the
needs-triage
gdavison
approved these changes
Apr 29, 2020
There was a problem hiding this comment.
LGTM 🚀
--- PASS: TestAccAWSKmsKey_disappears (9.06s)
--- PASS: TestAccAWSKmsExternalKey_disappears (9.53s)
--- PASS: TestAccAWSKmsKey_asymmetricKey (11.22s)
--- PASS: TestAccAWSKmsExternalKey_basic (11.69s)
--- PASS: TestAccAWSKmsKey_basic (12.24s)
--- PASS: TestAccAWSKmsExternalKey_Policy (14.57s)
--- FAIL: TestAccAWSKmsExternalKey_Description (15.82s)
--- PASS: TestAccAWSKmsExternalKey_DeletionWindowInDays (16.21s)
--- PASS: TestAccAWSKmsKey_policy (16.63s)
--- PASS: TestAccAWSKmsKey_tags (16.94s)
--- PASS: TestAccAWSKmsExternalKey_Tags (21.42s)
--- PASS: TestAccAWSKmsKey_Policy_IamRole (28.56s)
--- PASS: TestAccAWSKmsKey_Policy_IamServiceLinkedRole (39.46s)
--- PASS: TestAccAWSKmsExternalKey_KeyMaterialBase64 (72.37s)
--- FAIL: TestAccAWSKmsExternalKey_ValidTo (203.10s)
--- PASS: TestAccAWSKmsExternalKey_Enabled (237.38s)
--- PASS: TestAccAWSKmsKey_isEnabled (245.15s)
Looks like it's hitting some pre-existing eventual consistency errors. Nothing introduced by this PR.
|
This has been released in version 2.60.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
adamdecaf
pushed a commit
to adamdecaf/terraform-provider-aws
that referenced
this pull request
May 28, 2020
…creation to that timeout and add KMS Key deletion to internal waiter package (hashicorp#12863) Reference: hashicorp#7646 Reference: hashicorp#12840 The Terraform AWS Provider codebase contains many varied timeouts for handling IAM propagation retries. Here we introduce a shared constant for this amount of time. The choice of 2 minutes is based on that amount of time being: - Most widely used across resources - Based on lack of historical bug reports across those resources that implement that amount of time, most successful - Ensuring a reasonable user experience (not waiting too long) should there be an actual misconfiguration As an initial implementation of this IAM propagation timeout and further showing the potential waiter package refactoring, this fixes shorter IAM timeout implementations in the `aws_kms_key` and `aws_kms_external_key` resources, while also refactoring the pending deletion logic. This second change is designed as an inflection point for how we want to handle imports across multiple waiter packages, with the preference of this initial implementation to name the Go import of the outside service, `iamwaiter`, or generically SERVICEwaiter. If agreed, this will be added to the proposal and the refactoring documentation. NOTE: There is other `StateChangeConf` / `StateRefreshFunc` logic in these KMS resources, but this change is solely focused on highlighting the multiple import situation, and those will be handled later. Output from acceptance testing: ``` --- PASS: TestAccAWSKmsExternalKey_basic (19.53s) --- PASS: TestAccAWSKmsExternalKey_DeletionWindowInDays (31.61s) --- PASS: TestAccAWSKmsExternalKey_Description (32.11s) --- PASS: TestAccAWSKmsExternalKey_disappears (13.84s) --- PASS: TestAccAWSKmsExternalKey_Enabled (312.55s) --- PASS: TestAccAWSKmsExternalKey_KeyMaterialBase64 (104.29s) --- PASS: TestAccAWSKmsExternalKey_Policy (33.78s) --- PASS: TestAccAWSKmsExternalKey_Tags (43.70s) --- PASS: TestAccAWSKmsExternalKey_ValidTo (165.77s) --- PASS: TestAccAWSKmsKey_asymmetricKey (18.20s) --- PASS: TestAccAWSKmsKey_basic (21.13s) --- PASS: TestAccAWSKmsKey_disappears (13.92s) --- PASS: TestAccAWSKmsKey_isEnabled (236.91s) --- PASS: TestAccAWSKmsKey_policy (35.34s) --- PASS: TestAccAWSKmsKey_Policy_IamRole (34.14s) --- PASS: TestAccAWSKmsKey_Policy_IamServiceLinkedRole (44.80s) --- PASS: TestAccAWSKmsKey_tags (34.65s) ```
|
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
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Community Note
Closes #7646
Reference: #12840
Release note for CHANGELOG:
The Terraform AWS Provider codebase contains many varied timeouts for handling IAM propagation retries. Here we introduce a shared constant for this amount of time. The choice of 2 minutes is based on that amount of time being:
As an initial implementation of this IAM propagation timeout and further showing the potential waiter package refactoring, this fixes shorter IAM timeout implementations in the
aws_kms_keyandaws_kms_external_keyresources, while also refactoring the pending deletion logic. This second change is designed as an inflection point for how we want to handle imports across multiple waiter packages, with the preference of this initial implementation to name the Go import of the outside service,iamwaiter, or generically SERVICEwaiter. If agreed, this will be added to the proposal and the refactoring documentation.NOTE: There is other
StateChangeConf/StateRefreshFunclogic in these KMS resources, but this change is solely focused on highlighting the multiple import situation, and those will be handled later.Output from acceptance testing: