diff --git a/aws/internal/service/iam/waiter/waiter.go b/aws/internal/service/iam/waiter/waiter.go new file mode 100644 index 000000000000..7706bb6f2e5c --- /dev/null +++ b/aws/internal/service/iam/waiter/waiter.go @@ -0,0 +1,14 @@ +package waiter + +import ( + "time" +) + +const ( + // Maximum amount of time to wait for IAM changes to propagate + // This timeout should not be increased without strong consideration + // as this will negatively impact user experience when configurations + // have incorrect references or permissions. + // Reference: https://docs.aws.amazon.com/IAM/latest/UserGuide/troubleshoot_general.html#troubleshoot_general_eventual-consistency + PropagationTimeout = 2 * time.Minute +) diff --git a/aws/internal/service/kms/waiter/status.go b/aws/internal/service/kms/waiter/status.go new file mode 100644 index 000000000000..d0021759c228 --- /dev/null +++ b/aws/internal/service/kms/waiter/status.go @@ -0,0 +1,28 @@ +package waiter + +import ( + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/kms" + "github.com/hashicorp/terraform-plugin-sdk/helper/resource" +) + +// KeyState fetches the Key and its State +func KeyState(conn *kms.KMS, keyID string) resource.StateRefreshFunc { + return func() (interface{}, string, error) { + input := &kms.DescribeKeyInput{ + KeyId: aws.String(keyID), + } + + output, err := conn.DescribeKey(input) + + if err != nil { + return nil, kms.KeyStateUnavailable, err + } + + if output == nil || output.KeyMetadata == nil { + return output, kms.KeyStateUnavailable, nil + } + + return output, aws.StringValue(output.KeyMetadata.KeyState), nil + } +} diff --git a/aws/internal/service/kms/waiter/waiter.go b/aws/internal/service/kms/waiter/waiter.go new file mode 100644 index 000000000000..741178145c50 --- /dev/null +++ b/aws/internal/service/kms/waiter/waiter.go @@ -0,0 +1,34 @@ +package waiter + +import ( + "time" + + "github.com/aws/aws-sdk-go/service/kms" + "github.com/hashicorp/terraform-plugin-sdk/helper/resource" +) + +const ( + // Maximum amount of time to wait for KeyState to return PendingDeletion + KeyStatePendingDeletionTimeout = 20 * time.Minute +) + +// KeyStatePendingDeletion waits for KeyState to return PendingDeletion +func KeyStatePendingDeletion(conn *kms.KMS, keyID string) (*kms.DescribeKeyOutput, error) { + stateConf := &resource.StateChangeConf{ + Pending: []string{ + kms.KeyStateDisabled, + kms.KeyStateEnabled, + }, + Target: []string{kms.KeyStatePendingDeletion}, + Refresh: KeyState(conn, keyID), + Timeout: KeyStatePendingDeletionTimeout, + } + + outputRaw, err := stateConf.WaitForState() + + if output, ok := outputRaw.(*kms.DescribeKeyOutput); ok { + return output, err + } + + return nil, err +} diff --git a/aws/resource_aws_kms_external_key.go b/aws/resource_aws_kms_external_key.go index 981bfffd4b18..33931cfb2047 100644 --- a/aws/resource_aws_kms_external_key.go +++ b/aws/resource_aws_kms_external_key.go @@ -17,6 +17,8 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/helper/structure" "github.com/hashicorp/terraform-plugin-sdk/helper/validation" "github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags" + iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/waiter" ) func resourceAwsKmsExternalKey() *schema.Resource { @@ -110,7 +112,7 @@ func resourceAwsKmsExternalKeyCreate(d *schema.ResourceData, meta interface{}) e } var output *kms.CreateKeyOutput - err := resource.Retry(1*time.Minute, func() *resource.RetryError { + err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError { var err error output, err = conn.CreateKey(input) @@ -332,11 +334,17 @@ func resourceAwsKmsExternalKeyDelete(d *schema.ResourceData, meta interface{}) e } if err != nil { - return fmt.Errorf("error scheduling KMS External Key (%s) deletion: %s", d.Id(), err) + return fmt.Errorf("error scheduling deletion for KMS Key (%s): %w", d.Id(), err) + } + + _, err = waiter.KeyStatePendingDeletion(conn, d.Id()) + + if isAWSErr(err, kms.ErrCodeNotFoundException, "") { + return nil } - if err := waitForKmsKeyScheduleDeletion(conn, d.Id()); err != nil { - return fmt.Errorf("error waiting for KMS External Key (%s) deletion scheduling: %s", d.Id(), err) + if err != nil { + return fmt.Errorf("error waiting for KMS Key (%s) to schedule deletion: %w", d.Id(), err) } return nil @@ -440,39 +448,3 @@ func importKmsExternalKeyMaterial(conn *kms.KMS, keyID, keyMaterialBase64, valid return nil } - -func waitForKmsKeyScheduleDeletion(conn *kms.KMS, keyID string) error { - // Wait for propagation since KMS is eventually consistent - input := &kms.DescribeKeyInput{ - KeyId: aws.String(keyID), - } - - wait := resource.StateChangeConf{ - Pending: []string{kms.KeyStateDisabled, kms.KeyStateEnabled}, - Target: []string{kms.KeyStatePendingDeletion}, - Timeout: 20 * time.Minute, - MinTimeout: 2 * time.Second, - ContinuousTargetOccurence: 10, - Refresh: func() (interface{}, string, error) { - output, err := conn.DescribeKey(input) - - if isAWSErr(err, kms.ErrCodeNotFoundException, "") { - return 42, kms.KeyStatePendingDeletion, nil - } - - if err != nil { - return nil, kms.KeyStateUnavailable, err - } - - if output == nil || output.KeyMetadata == nil { - return 42, kms.KeyStatePendingDeletion, nil - } - - return output, aws.StringValue(output.KeyMetadata.KeyState), nil - }, - } - - _, err := wait.WaitForState() - - return err -} diff --git a/aws/resource_aws_kms_external_key_test.go b/aws/resource_aws_kms_external_key_test.go index a7f4e61e9848..10c309cbad25 100644 --- a/aws/resource_aws_kms_external_key_test.go +++ b/aws/resource_aws_kms_external_key_test.go @@ -10,7 +10,8 @@ import ( "github.com/aws/aws-sdk-go/service/kms" "github.com/hashicorp/terraform-plugin-sdk/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/terraform" - "github.com/jen20/awspolicyequivalence" + awspolicy "github.com/jen20/awspolicyequivalence" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/waiter" ) func TestAccAWSKmsExternalKey_basic(t *testing.T) { @@ -483,7 +484,9 @@ func testAccCheckAWSKmsExternalKeyDisappears(key *kms.KeyMetadata) resource.Test return err } - return waitForKmsKeyScheduleDeletion(conn, aws.StringValue(key.KeyId)) + _, err = waiter.KeyStatePendingDeletion(conn, aws.StringValue(key.KeyId)) + + return err } } diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index 16817a799f02..ed8186945e3c 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -13,6 +13,8 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/helper/structure" "github.com/hashicorp/terraform-plugin-sdk/helper/validation" "github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags" + iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/waiter" ) func resourceAwsKmsKey() *schema.Resource { @@ -116,7 +118,7 @@ func resourceAwsKmsKeyCreate(d *schema.ResourceData, meta interface{}) error { // The KMS service's awareness of principals is limited by "eventual consistency". // They acknowledge this here: // http://docs.aws.amazon.com/kms/latest/APIReference/API_CreateKey.html - err := resource.Retry(30*time.Second, func() *resource.RetryError { + err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError { var err error resp, err = conn.CreateKey(req) if isAWSErr(err, kms.ErrCodeMalformedPolicyDocumentException, "") { @@ -471,39 +473,24 @@ func resourceAwsKmsKeyDelete(d *schema.ResourceData, meta interface{}) error { req.PendingWindowInDays = aws.Int64(int64(v.(int))) } _, err := conn.ScheduleKeyDeletion(req) - if err != nil { - return err + + if isAWSErr(err, kms.ErrCodeNotFoundException, "") { + return nil } - // Wait for propagation since KMS is eventually consistent - wait := resource.StateChangeConf{ - Pending: []string{kms.KeyStateEnabled, kms.KeyStateDisabled}, - Target: []string{kms.KeyStatePendingDeletion}, - Timeout: 20 * time.Minute, - MinTimeout: 2 * time.Second, - ContinuousTargetOccurence: 10, - Refresh: func() (interface{}, string, error) { - log.Printf("[DEBUG] Checking if KMS key %s state is PendingDeletion", keyId) - resp, err := conn.DescribeKey(&kms.DescribeKeyInput{ - KeyId: aws.String(keyId), - }) - if err != nil { - return resp, "Failed", err - } + if err != nil { + return fmt.Errorf("error scheduling deletion for KMS Key (%s): %w", d.Id(), err) + } - metadata := *resp.KeyMetadata - log.Printf("[DEBUG] KMS key %s state is %s, retrying", keyId, *metadata.KeyState) + _, err = waiter.KeyStatePendingDeletion(conn, d.Id()) - return resp, *metadata.KeyState, nil - }, + if isAWSErr(err, kms.ErrCodeNotFoundException, "") { + return nil } - _, err = wait.WaitForState() if err != nil { - return fmt.Errorf("Failed deactivating KMS key %s: %s", keyId, err) + return fmt.Errorf("error waiting for KMS Key (%s) to schedule deletion: %w", d.Id(), err) } - log.Printf("[DEBUG] KMS Key %s deactivated.", keyId) - return nil } diff --git a/aws/resource_aws_kms_key_test.go b/aws/resource_aws_kms_key_test.go index 3272c7dda248..3aab3fea5ae0 100644 --- a/aws/resource_aws_kms_key_test.go +++ b/aws/resource_aws_kms_key_test.go @@ -10,7 +10,7 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/helper/acctest" "github.com/hashicorp/terraform-plugin-sdk/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/terraform" - "github.com/jen20/awspolicyequivalence" + awspolicy "github.com/jen20/awspolicyequivalence" ) func init() { @@ -173,6 +173,59 @@ func TestAccAWSKmsKey_policy(t *testing.T) { }) } +func TestAccAWSKmsKey_Policy_IamRole(t *testing.T) { + var key kms.KeyMetadata + rName := acctest.RandomWithPrefix("tf-acc-test") + resourceName := "aws_kms_key.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSKmsKeyDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSKmsKeyConfigPolicyIamRole(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSKmsKeyExists(resourceName, &key), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"deletion_window_in_days"}, + }, + }, + }) +} + +// Reference: https://github.com/terraform-providers/terraform-provider-aws/issues/7646 +func TestAccAWSKmsKey_Policy_IamServiceLinkedRole(t *testing.T) { + var key kms.KeyMetadata + rName := acctest.RandomWithPrefix("tf-acc-test") + resourceName := "aws_kms_key.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSKmsKeyDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSKmsKeyConfigPolicyIamServiceLinkedRole(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSKmsKeyExists(resourceName, &key), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"deletion_window_in_days"}, + }, + }, + }) +} + func TestAccAWSKmsKey_isEnabled(t *testing.T) { var key1, key2, key3 kms.KeyMetadata rName := fmt.Sprintf("tf-testacc-kms-key-%s", acctest.RandStringFromCharSet(13, acctest.CharSetAlphaNum)) @@ -417,6 +470,110 @@ POLICY `, rName) } +func testAccAWSKmsKeyConfigPolicyIamRole(rName string) string { + return fmt.Sprintf(` +data "aws_partition" "current" {} + +resource "aws_iam_role" "test" { + name = %[1]q + + assume_role_policy = jsonencode({ + Statement = [{ + Action = "sts:AssumeRole" + Effect = "Allow" + Principal = { + Service = "ec2.${data.aws_partition.current.dns_suffix}" + } + }] + Version = "2012-10-17" + }) +} + +resource "aws_kms_key" "test" { + description = %[1]q + deletion_window_in_days = 7 + + policy = jsonencode({ + Id = "kms-tf-1" + Statement = [ + { + Action = "kms:*" + Effect = "Allow" + Principal = { + AWS = "*" + } + Resource = "*" + Sid= "Enable IAM User Permissions" + }, + { + Action = [ + "kms:Encrypt", + "kms:Decrypt", + "kms:ReEncrypt*", + "kms:GenerateDataKey*", + "kms:DescribeKey", + ] + Effect = "Allow" + Principal = { + AWS = [aws_iam_role.test.arn] + } + Resource = "*" + Sid = "Enable IAM User Permissions" + }, + ] + Version = "2012-10-17" +}) +} +`, rName) +} + +func testAccAWSKmsKeyConfigPolicyIamServiceLinkedRole(rName string) string { + return fmt.Sprintf(` +data "aws_partition" "current" {} + +resource "aws_iam_service_linked_role" "test" { + aws_service_name = "autoscaling.${data.aws_partition.current.dns_suffix}" + custom_suffix = %[1]q +} + +resource "aws_kms_key" "test" { + description = %[1]q + deletion_window_in_days = 7 + + policy = jsonencode({ + Id = "kms-tf-1" + Statement = [ + { + Action = "kms:*" + Effect = "Allow" + Principal = { + AWS = "*" + } + Resource = "*" + Sid= "Enable IAM User Permissions" + }, + { + Action = [ + "kms:Encrypt", + "kms:Decrypt", + "kms:ReEncrypt*", + "kms:GenerateDataKey*", + "kms:DescribeKey", + ] + Effect = "Allow" + Principal = { + AWS = [aws_iam_service_linked_role.test.arn] + } + Resource = "*" + Sid = "Enable IAM User Permissions" + }, + ] + Version = "2012-10-17" +}) +} +`, rName) +} + func testAccAWSKmsKey_removedPolicy(rName string) string { return fmt.Sprintf(` resource "aws_kms_key" "test" {