From d2195b5ea3b094722e56cc0610062fea755dfe7a Mon Sep 17 00:00:00 2001 From: Lucas Maxwell Date: Tue, 16 Mar 2021 11:50:11 +1100 Subject: [PATCH 01/36] Add policy lockout check bypass flag --- aws/resource_aws_kms_key.go | 5 +++++ aws/resource_aws_kms_key_test.go | 12 ++++++------ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index 586a7839fb9..b9cca68c615 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -65,6 +65,11 @@ func resourceAwsKmsKey() *schema.Resource { ValidateFunc: validation.StringIsJSON, DiffSuppressFunc: suppressEquivalentAwsPolicyDiffs, }, + "bypass_policy_lockout_check": { + Type: schema.TypeBool, + Optional: true, + Default: false, + }, "is_enabled": { Type: schema.TypeBool, Optional: true, diff --git a/aws/resource_aws_kms_key_test.go b/aws/resource_aws_kms_key_test.go index fce4b5b8a3f..773beb81f90 100644 --- a/aws/resource_aws_kms_key_test.go +++ b/aws/resource_aws_kms_key_test.go @@ -94,7 +94,7 @@ func TestAccAWSKmsKey_basic(t *testing.T) { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"deletion_window_in_days"}, + ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_check"}, }, }, }) @@ -169,7 +169,7 @@ func TestAccAWSKmsKey_policy(t *testing.T) { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"deletion_window_in_days"}, + ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_check"}, }, { Config: testAccAWSKmsKey_removedPolicy(rName), @@ -202,7 +202,7 @@ func TestAccAWSKmsKey_Policy_IamRole(t *testing.T) { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"deletion_window_in_days"}, + ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_check"}, }, }, }) @@ -230,7 +230,7 @@ func TestAccAWSKmsKey_Policy_IamServiceLinkedRole(t *testing.T) { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"deletion_window_in_days"}, + ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_check"}, }, }, }) @@ -260,7 +260,7 @@ func TestAccAWSKmsKey_isEnabled(t *testing.T) { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"deletion_window_in_days"}, + ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_check"}, }, { Config: testAccAWSKmsKey_disabled(rName), @@ -306,7 +306,7 @@ func TestAccAWSKmsKey_tags(t *testing.T) { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"deletion_window_in_days"}, + ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_check"}, }, { Config: testAccAWSKmsKey(rName), From 10c4a11b83e89507e9a3aa1d580b73f101f825a8 Mon Sep 17 00:00:00 2001 From: Lucas Maxwell Date: Tue, 16 Mar 2021 11:52:17 +1100 Subject: [PATCH 02/36] Supply bypass flag when creating kms keys --- aws/resource_aws_kms_key.go | 5 ++- aws/resource_aws_kms_key_test.go | 71 ++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index b9cca68c615..9ea399132d4 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -98,8 +98,9 @@ func resourceAwsKmsKeyCreate(d *schema.ResourceData, meta interface{}) error { // Allow aws to chose default values if we don't pass them req := &kms.CreateKeyInput{ - CustomerMasterKeySpec: aws.String(d.Get("customer_master_key_spec").(string)), - KeyUsage: aws.String(d.Get("key_usage").(string)), + CustomerMasterKeySpec: aws.String(d.Get("customer_master_key_spec").(string)), + KeyUsage: aws.String(d.Get("key_usage").(string)), + BypassPolicyLockoutSafetyCheck: aws.Bool(d.Get("bypass_policy_lockout_check").(bool)), } if v, exists := d.GetOk("description"); exists { req.Description = aws.String(v.(string)) diff --git a/aws/resource_aws_kms_key_test.go b/aws/resource_aws_kms_key_test.go index 773beb81f90..249a2a99702 100644 --- a/aws/resource_aws_kms_key_test.go +++ b/aws/resource_aws_kms_key_test.go @@ -3,6 +3,7 @@ package aws import ( "fmt" "log" + "regexp" "testing" "github.com/aws/aws-sdk-go/aws" @@ -181,6 +182,37 @@ func TestAccAWSKmsKey_policy(t *testing.T) { }) } +func TestAccAWSKmsKey_policyBypass(t *testing.T) { + var key kms.KeyMetadata + rName := fmt.Sprintf("tf-testacc-kms-key-%s", acctest.RandString(13)) + resourceName := "aws_kms_key.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSKmsKeyDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSKmsKey_policyBypass(rName, false), + ExpectError: regexp.MustCompile(`The new key policy will not allow you to update the key policy in the future`), + }, + { + Config: testAccAWSKmsKey_policyBypass(rName, true), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSKmsKeyExists(resourceName, &key), + resource.TestCheckResourceAttr(resourceName, "bypass_policy_lockout_check", "true"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_check"}, + }, + }, + }) +} + func TestAccAWSKmsKey_Policy_IamRole(t *testing.T) { var key kms.KeyMetadata rName := acctest.RandomWithPrefix("tf-acc-test") @@ -469,6 +501,45 @@ POLICY `, rName) } +func testAccAWSKmsKey_policyBypass(rName string, bypassFlag bool) string { + return fmt.Sprintf(` +data "aws_caller_identity" "current" {} + +resource "aws_kms_key" "test" { + description = %[1]q + deletion_window_in_days = 7 + + bypass_policy_lockout_check = %t + policy = <<-POLICY + { + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": "${data.aws_caller_identity.current.arn}" + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:ScheduleKeyDeletion", + "kms:Describe*", + "kms:Get*", + "kms:List*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] + } + POLICY +} +`, rName, bypassFlag) +} + func testAccAWSKmsKeyConfigPolicyIamRole(rName string) string { return fmt.Sprintf(` data "aws_partition" "current" {} From a4a57d6a3a9f928a350cd091a8a2f77311a7dbaa Mon Sep 17 00:00:00 2001 From: Lucas Maxwell Date: Tue, 16 Mar 2021 11:52:35 +1100 Subject: [PATCH 03/36] Supply bypass flag when updating kms keys --- aws/resource_aws_kms_key.go | 10 ++++++---- aws/resource_aws_kms_key_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index 9ea399132d4..001ab93f39c 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -324,13 +324,15 @@ func resourceAwsKmsKeyPolicyUpdate(conn *kms.KMS, d *schema.ResourceData) error return fmt.Errorf("policy contains an invalid JSON: %w", err) } keyId := d.Get("key_id").(string) + bypassPolicyLockoutCheck := d.Get("bypass_policy_lockout_check").(bool) - log.Printf("[DEBUG] KMS key: %s, update policy: %s", keyId, policy) + log.Printf("[DEBUG] KMS key: %s, bypass policy lockout check: %t, update policy: %s", keyId, bypassPolicyLockoutCheck, policy) req := &kms.PutKeyPolicyInput{ - KeyId: aws.String(keyId), - Policy: aws.String(policy), - PolicyName: aws.String("default"), + KeyId: aws.String(keyId), + Policy: aws.String(policy), + PolicyName: aws.String("default"), + BypassPolicyLockoutSafetyCheck: aws.Bool(bypassPolicyLockoutCheck), } _, err = conn.PutKeyPolicy(req) diff --git a/aws/resource_aws_kms_key_test.go b/aws/resource_aws_kms_key_test.go index 249a2a99702..25d572230c7 100644 --- a/aws/resource_aws_kms_key_test.go +++ b/aws/resource_aws_kms_key_test.go @@ -213,6 +213,34 @@ func TestAccAWSKmsKey_policyBypass(t *testing.T) { }) } +func TestAccAWSKmsKey_policyBypassUpdate(t *testing.T) { + var before, after kms.KeyMetadata + rName := fmt.Sprintf("tf-testacc-kms-key-%s", acctest.RandString(13)) + resourceName := "aws_kms_key.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSKmsKeyDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSKmsKey(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSKmsKeyExists(resourceName, &before), + resource.TestCheckResourceAttr(resourceName, "bypass_policy_lockout_check", "false"), + ), + }, + { + Config: testAccAWSKmsKey_policyBypass(rName, true), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSKmsKeyExists(resourceName, &after), + resource.TestCheckResourceAttr(resourceName, "bypass_policy_lockout_check", "true"), + ), + }, + }, + }) +} + func TestAccAWSKmsKey_Policy_IamRole(t *testing.T) { var key kms.KeyMetadata rName := acctest.RandomWithPrefix("tf-acc-test") From a0324c4da9d64b4af5b810936226b99608f3e7e5 Mon Sep 17 00:00:00 2001 From: Lucas Maxwell Date: Tue, 16 Mar 2021 11:53:25 +1100 Subject: [PATCH 04/36] Add doco for bypass lockout check flag --- website/docs/r/kms_key.html.markdown | 1 + 1 file changed, 1 insertion(+) diff --git a/website/docs/r/kms_key.html.markdown b/website/docs/r/kms_key.html.markdown index 0a063d31a6b..de2eedd7f53 100644 --- a/website/docs/r/kms_key.html.markdown +++ b/website/docs/r/kms_key.html.markdown @@ -32,6 +32,7 @@ Valid values: `SYMMETRIC_DEFAULT`, `RSA_2048`, `RSA_3072`, `RSA_4096`, `ECC_NIS ~> **NOTE:** Note: All KMS keys must have a key policy. If a key policy is not specified, AWS gives the KMS key a [default key policy](https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html#key-policy-default) that gives all principals in the owning account unlimited access to all KMS operations for the key. This default key policy effectively delegates all access control to IAM policies and KMS grants. +* `bypass_policy_lockout_check` - (Optional) Specifies whether to disable the policy lockout check performed when creating or updating the key's policy. Setting this value to `true` increases the risk that the CMK becomes unmanageable. For more information, refer to the scenario in the [Default Key Policy](https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html#key-policy-default-allow-root-enable-iam) section in the AWS Key Management Service Developer Guide. Defaults to `false`. * `deletion_window_in_days` - (Optional) Duration in days after which the key is deleted after destruction of the resource, must be between 7 and 30 days. Defaults to 30 days. * `is_enabled` - (Optional) Specifies whether the key is enabled. Defaults to true. * `enable_key_rotation` - (Optional) Specifies whether [key rotation](http://docs.aws.amazon.com/kms/latest/developerguide/rotate-keys.html) is enabled. Defaults to false. From 7ae8f2f540fc7fc6c481634316702416112d93d5 Mon Sep 17 00:00:00 2001 From: Lucas Maxwell Date: Tue, 16 Mar 2021 17:31:15 +1100 Subject: [PATCH 05/36] Fix terraform formatting --- aws/resource_aws_kms_key_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws/resource_aws_kms_key_test.go b/aws/resource_aws_kms_key_test.go index 25d572230c7..067650ca0ff 100644 --- a/aws/resource_aws_kms_key_test.go +++ b/aws/resource_aws_kms_key_test.go @@ -538,7 +538,7 @@ resource "aws_kms_key" "test" { deletion_window_in_days = 7 bypass_policy_lockout_check = %t - policy = <<-POLICY + policy = <<-POLICY { "Version": "2012-10-17", "Id": "kms-tf-1", From 9d45932d67144015677f49ed350c613de6dcb8d6 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Sat, 17 Jul 2021 16:52:14 -0400 Subject: [PATCH 06/36] Add CHANGELOG entry. --- .changelog/18117.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/18117.txt diff --git a/.changelog/18117.txt b/.changelog/18117.txt new file mode 100644 index 00000000000..d229cf0ba97 --- /dev/null +++ b/.changelog/18117.txt @@ -0,0 +1,3 @@ +```release-note:enhancement +resource/aws_kms_key: Add `bypass_policy_lockout_check` argument +``` From ee55e9692a2d124cc1d5e8b63f1abf38da9112f4 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Sat, 17 Jul 2021 18:23:32 -0400 Subject: [PATCH 07/36] Add internal KMS finder package. --- aws/internal/service/kms/enum.go | 5 + aws/internal/service/kms/finder/finder.go | 74 ++++++++++ aws/internal/service/kms/waiter/status.go | 21 ++- aws/internal/service/kms/waiter/waiter.go | 54 ++++++++ aws/internal/tfresource/retry.go | 33 +++++ aws/internal/tfresource/retry_test.go | 62 +++++++++ aws/resource_aws_kms_key.go | 158 ++++++++++------------ 7 files changed, 312 insertions(+), 95 deletions(-) create mode 100644 aws/internal/service/kms/enum.go create mode 100644 aws/internal/service/kms/finder/finder.go diff --git a/aws/internal/service/kms/enum.go b/aws/internal/service/kms/enum.go new file mode 100644 index 00000000000..f75335b3d46 --- /dev/null +++ b/aws/internal/service/kms/enum.go @@ -0,0 +1,5 @@ +package kms + +const ( + PolicyNameDefault = "default" +) diff --git a/aws/internal/service/kms/finder/finder.go b/aws/internal/service/kms/finder/finder.go new file mode 100644 index 00000000000..10c616aa919 --- /dev/null +++ b/aws/internal/service/kms/finder/finder.go @@ -0,0 +1,74 @@ +package finder + +import ( + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/kms" + "github.com/hashicorp/aws-sdk-go-base/tfawserr" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" +) + +func KeyByID(conn *kms.KMS, id string) (*kms.KeyMetadata, error) { + input := &kms.DescribeKeyInput{ + KeyId: aws.String(id), + } + + output, err := conn.DescribeKey(input) + + if tfawserr.ErrCodeEquals(err, kms.ErrCodeNotFoundException) { + return nil, &resource.NotFoundError{ + LastError: err, + LastRequest: input, + } + } + + if err != nil { + return nil, err + } + + if output == nil || output.KeyMetadata == nil { + return nil, &resource.NotFoundError{ + Message: "Empty result", + LastRequest: input, + } + } + + keyMetadata := output.KeyMetadata + + if state := aws.StringValue(keyMetadata.KeyState); state == kms.KeyStatePendingDeletion { + return nil, &resource.NotFoundError{ + Message: state, + LastRequest: input, + } + } + + return keyMetadata, nil +} + +func KeyPolicyByKeyIDAndPolicyName(conn *kms.KMS, keyID, policyName string) (string, error) { + input := &kms.GetKeyPolicyInput{ + KeyId: aws.String(keyID), + PolicyName: aws.String(policyName), + } + + output, err := conn.GetKeyPolicy(input) + + if tfawserr.ErrCodeEquals(err, kms.ErrCodeNotFoundException) { + return "", &resource.NotFoundError{ + LastError: err, + LastRequest: input, + } + } + + if err != nil { + return "", err + } + + if output == nil || output.Policy == nil { + return "", &resource.NotFoundError{ + Message: "Empty result", + LastRequest: input, + } + } + + return aws.StringValue(output.Policy), nil +} diff --git a/aws/internal/service/kms/waiter/status.go b/aws/internal/service/kms/waiter/status.go index 194f73a9347..dbef1ad726d 100644 --- a/aws/internal/service/kms/waiter/status.go +++ b/aws/internal/service/kms/waiter/status.go @@ -4,25 +4,22 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/kms" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/finder" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) -// KeyState fetches the Key and its State -func KeyState(conn *kms.KMS, keyID string) resource.StateRefreshFunc { +func KeyState(conn *kms.KMS, id string) resource.StateRefreshFunc { return func() (interface{}, string, error) { - input := &kms.DescribeKeyInput{ - KeyId: aws.String(keyID), - } - - output, err := conn.DescribeKey(input) + output, err := finder.KeyByID(conn, id) - if err != nil { - return nil, kms.KeyStateUnavailable, err + if tfresource.NotFound(err) { + return nil, "", nil } - if output == nil || output.KeyMetadata == nil { - return output, kms.KeyStateUnavailable, nil + if err != nil { + return nil, "", err } - return output, aws.StringValue(output.KeyMetadata.KeyState), nil + return output, aws.StringValue(output.KeyState), nil } } diff --git a/aws/internal/service/kms/waiter/waiter.go b/aws/internal/service/kms/waiter/waiter.go index b1fbc22155c..baf0be8544f 100644 --- a/aws/internal/service/kms/waiter/waiter.go +++ b/aws/internal/service/kms/waiter/waiter.go @@ -4,14 +4,68 @@ import ( "time" "github.com/aws/aws-sdk-go/service/kms" + "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" + iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) const ( // Maximum amount of time to wait for KeyState to return PendingDeletion KeyStatePendingDeletionTimeout = 20 * time.Minute + + KeyDeletedTimeout = 20 * time.Minute ) +// IAMPropagation retries the specified function if the returned error indicates an IAM eventual consistency issue. +// If the retries time out the specified function is called one last time. +func IAMPropagation(f func() (interface{}, error)) (interface{}, error) { + var output interface{} + + err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError { + var err error + + output, err = f() + + if tfawserr.ErrCodeEquals(err, kms.ErrCodeMalformedPolicyDocumentException) { + return resource.RetryableError(err) + } + + if err != nil { + return resource.NonRetryableError(err) + } + + return nil + }) + + if tfresource.TimedOut(err) { + output, err = f() + } + + if err != nil { + return nil, err + } + + return output, nil +} + +func KeyDeleted(conn *kms.KMS, id string) (*kms.KeyMetadata, error) { + stateConf := &resource.StateChangeConf{ + Pending: []string{kms.KeyStateDisabled, kms.KeyStateEnabled}, + Target: []string{}, + Refresh: KeyState(conn, id), + Timeout: KeyDeletedTimeout, + } + + outputRaw, err := stateConf.WaitForState() + + if output, ok := outputRaw.(*kms.KeyMetadata); ok { + return output, err + } + + return nil, err +} + // KeyStatePendingDeletion waits for KeyState to return PendingDeletion func KeyStatePendingDeletion(conn *kms.KMS, keyID string) (*kms.DescribeKeyOutput, error) { stateConf := &resource.StateChangeConf{ diff --git a/aws/internal/tfresource/retry.go b/aws/internal/tfresource/retry.go index 56b891a4258..04993dd1064 100644 --- a/aws/internal/tfresource/retry.go +++ b/aws/internal/tfresource/retry.go @@ -19,6 +19,8 @@ func RetryWhenAwsErrCodeEquals(timeout time.Duration, f func() (interface{}, err output, err = f() + // https://github.com/hashicorp/aws-sdk-go-base/pull/55 has been merged. + // Once aws-sdk-go-base has been updated, use variadic version of ErrCodeEquals. for _, code := range codes { if tfawserr.ErrCodeEquals(err, code) { return resource.RetryableError(err) @@ -43,6 +45,37 @@ func RetryWhenAwsErrCodeEquals(timeout time.Duration, f func() (interface{}, err return output, nil } +// RetryWhenNotFound retries the specified function when it returns a resource.NotFoundError. +func RetryWhenNotFound(timeout time.Duration, f func() (interface{}, error)) (interface{}, error) { + var output interface{} + + err := resource.Retry(timeout, func() *resource.RetryError { + var err error + + output, err = f() + + if NotFound(err) { + return resource.RetryableError(err) + } + + if err != nil { + return resource.NonRetryableError(err) + } + + return nil + }) + + if TimedOut(err) { + output, err = f() + } + + if err != nil { + return nil, err + } + + return output, nil +} + // RetryConfigContext allows configuration of StateChangeConf's various time arguments. // This is especially useful for AWS services that are prone to throttling, such as Route53, where // the default durations cause problems. To not use a StateChangeConf argument and revert to the diff --git a/aws/internal/tfresource/retry_test.go b/aws/internal/tfresource/retry_test.go index 2edbf74723d..0dbb8abd2de 100644 --- a/aws/internal/tfresource/retry_test.go +++ b/aws/internal/tfresource/retry_test.go @@ -75,6 +75,68 @@ func TestRetryWhenAwsErrCodeEquals(t *testing.T) { } } +func TestRetryWhenNotFound(t *testing.T) { + var retryCount int32 + + testCases := []struct { + Name string + F func() (interface{}, error) + ExpectError bool + }{ + { + Name: "no error", + F: func() (interface{}, error) { + return nil, nil + }, + }, + { + Name: "non-retryable other error", + F: func() (interface{}, error) { + return nil, errors.New("TestCode") + }, + ExpectError: true, + }, + { + Name: "non-retryable AWS error", + F: func() (interface{}, error) { + return nil, awserr.New("Testing", "Testing", nil) + }, + ExpectError: true, + }, + { + Name: "retryable NotFoundError timeout", + F: func() (interface{}, error) { + return nil, &resource.NotFoundError{} + }, + ExpectError: true, + }, + { + Name: "retryable NotFoundError success", + F: func() (interface{}, error) { + if atomic.CompareAndSwapInt32(&retryCount, 0, 1) { + return nil, &resource.NotFoundError{} + } + + return nil, nil + }, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.Name, func(t *testing.T) { + retryCount = 0 + + _, err := tfresource.RetryWhenNotFound(5*time.Second, testCase.F) + + if testCase.ExpectError && err == nil { + t.Fatal("expected error") + } else if !testCase.ExpectError && err != nil { + t.Fatalf("unexpected error: %s", err) + } + }) + } +} + func TestRetryConfigContext_error(t *testing.T) { t.Parallel() diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index 001ab93f39c..c8c8a259ff0 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -12,8 +12,9 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure" "github.com/hashicorp/terraform-plugin-sdk/v2/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/finder" "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/waiter" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) func resourceAwsKmsKey() *schema.Resource { @@ -34,57 +35,67 @@ func resourceAwsKmsKey() *schema.Resource { Type: schema.TypeString, Computed: true, }, - "key_id": { - Type: schema.TypeString, - Computed: true, + + "bypass_policy_lockout_check": { + Type: schema.TypeBool, + Optional: true, + Default: false, }, - "description": { + + "customer_master_key_spec": { Type: schema.TypeString, Optional: true, - Computed: true, - ValidateFunc: validation.StringLenBetween(0, 8192), + ForceNew: true, + Default: kms.CustomerMasterKeySpecSymmetricDefault, + ValidateFunc: validation.StringInSlice(kms.CustomerMasterKeySpec_Values(), false), }, - "key_usage": { - Type: schema.TypeString, + + "deletion_window_in_days": { + Type: schema.TypeInt, Optional: true, - Default: kms.KeyUsageTypeEncryptDecrypt, - ForceNew: true, - ValidateFunc: validation.StringInSlice(kms.KeyUsageType_Values(), false), + ValidateFunc: validation.IntBetween(7, 30), }, - "customer_master_key_spec": { + + "description": { Type: schema.TypeString, Optional: true, - Default: kms.CustomerMasterKeySpecSymmetricDefault, - ForceNew: true, - ValidateFunc: validation.StringInSlice(kms.CustomerMasterKeySpec_Values(), false), - }, - "policy": { - Type: schema.TypeString, - Optional: true, - Computed: true, - ValidateFunc: validation.StringIsJSON, - DiffSuppressFunc: suppressEquivalentAwsPolicyDiffs, + Computed: true, + ValidateFunc: validation.StringLenBetween(0, 8192), }, - "bypass_policy_lockout_check": { + + "enable_key_rotation": { Type: schema.TypeBool, Optional: true, Default: false, }, + "is_enabled": { Type: schema.TypeBool, Optional: true, Default: true, }, - "enable_key_rotation": { - Type: schema.TypeBool, - Optional: true, - Default: false, + + "key_id": { + Type: schema.TypeString, + Computed: true, }, - "deletion_window_in_days": { - Type: schema.TypeInt, + + "key_usage": { + Type: schema.TypeString, Optional: true, - ValidateFunc: validation.IntBetween(7, 30), + ForceNew: true, + Default: kms.KeyUsageTypeEncryptDecrypt, + ValidateFunc: validation.StringInSlice(kms.KeyUsageType_Values(), false), }, + + "policy": { + Type: schema.TypeString, + Optional: true, + Computed: true, + ValidateFunc: validation.StringIsJSON, + DiffSuppressFunc: suppressEquivalentAwsPolicyDiffs, + }, + "tags": tagsSchema(), "tags_all": tagsSchemaComputed(), }, @@ -96,47 +107,42 @@ func resourceAwsKmsKeyCreate(d *schema.ResourceData, meta interface{}) error { defaultTagsConfig := meta.(*AWSClient).DefaultTagsConfig tags := defaultTagsConfig.MergeTags(keyvaluetags.New(d.Get("tags").(map[string]interface{}))) - // Allow aws to chose default values if we don't pass them - req := &kms.CreateKeyInput{ + input := &kms.CreateKeyInput{ + BypassPolicyLockoutSafetyCheck: aws.Bool(d.Get("bypass_policy_lockout_check").(bool)), CustomerMasterKeySpec: aws.String(d.Get("customer_master_key_spec").(string)), KeyUsage: aws.String(d.Get("key_usage").(string)), - BypassPolicyLockoutSafetyCheck: aws.Bool(d.Get("bypass_policy_lockout_check").(bool)), } - if v, exists := d.GetOk("description"); exists { - req.Description = aws.String(v.(string)) + + if v, ok := d.GetOk("description"); ok { + input.Description = aws.String(v.(string)) } - if v, exists := d.GetOk("policy"); exists { - req.Policy = aws.String(v.(string)) + + if v, ok := d.GetOk("policy"); ok { + input.Policy = aws.String(v.(string)) } + if len(tags) > 0 { - req.Tags = tags.IgnoreAws().KmsTags() + input.Tags = tags.IgnoreAws().KmsTags() } - var resp *kms.CreateKeyOutput // AWS requires any principal in the policy to exist before the key is created. // 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(iamwaiter.PropagationTimeout, func() *resource.RetryError { - var err error - resp, err = conn.CreateKey(req) - if isAWSErr(err, kms.ErrCodeMalformedPolicyDocumentException, "") { - return resource.RetryableError(err) - } - if err != nil { - return resource.NonRetryableError(err) - } - return nil + log.Printf("[DEBUG] Creating KMS Key: %s", input) + + outputRaw, err := waiter.IAMPropagation(func() (interface{}, error) { + return conn.CreateKey(input) }) - if isResourceTimeoutError(err) { - resp, err = conn.CreateKey(req) - } + if err != nil { - return err + return fmt.Errorf("error creating KMS Key: %w", err) } - d.SetId(aws.StringValue(resp.KeyMetadata.KeyId)) - d.Set("key_id", resp.KeyMetadata.KeyId) + output := outputRaw.(*kms.CreateKeyOutput) + + d.SetId(aws.StringValue(output.KeyMetadata.KeyId)) + d.Set("key_id", output.KeyMetadata.KeyId) if enableKeyRotation := d.Get("enable_key_rotation").(bool); enableKeyRotation { if err := updateKmsKeyRotationStatus(conn, d); err != nil { @@ -158,38 +164,24 @@ func resourceAwsKmsKeyRead(d *schema.ResourceData, meta interface{}) error { defaultTagsConfig := meta.(*AWSClient).DefaultTagsConfig ignoreTagsConfig := meta.(*AWSClient).IgnoreTagsConfig - req := &kms.DescribeKeyInput{ - KeyId: aws.String(d.Id()), - } - - var resp *kms.DescribeKeyOutput - var err error - if d.IsNewResource() { - var out interface{} - out, err = retryOnAwsCode(kms.ErrCodeNotFoundException, func() (interface{}, error) { - return conn.DescribeKey(req) - }) - resp, _ = out.(*kms.DescribeKeyOutput) - } else { - resp, err = conn.DescribeKey(req) - } - if err != nil { - return err - } - metadata := resp.KeyMetadata + output, err := finder.KeyByID(conn, d.Id()) - if aws.StringValue(metadata.KeyState) == kms.KeyStatePendingDeletion { - log.Printf("[WARN] Removing KMS key %s because it's already gone", d.Id()) + if !d.IsNewResource() && tfresource.NotFound(err) { + log.Printf("[WARN] KMS Key (%s) not found, removing from state", d.Id()) d.SetId("") return nil } - d.Set("arn", metadata.Arn) - d.Set("key_id", metadata.KeyId) - d.Set("description", metadata.Description) - d.Set("key_usage", metadata.KeyUsage) - d.Set("customer_master_key_spec", metadata.CustomerMasterKeySpec) - d.Set("is_enabled", metadata.Enabled) + if err != nil { + return fmt.Errorf("error reading KMS Key (%s): %w", d.Id(), err) + } + + d.Set("arn", output.Arn) + d.Set("customer_master_key_spec", output.CustomerMasterKeySpec) + d.Set("description", output.Description) + d.Set("is_enabled", output.Enabled) + d.Set("key_id", output.KeyId) + d.Set("key_usage", output.KeyUsage) pOut, err := retryOnAwsCode(kms.ErrCodeNotFoundException, func() (interface{}, error) { return conn.GetKeyPolicy(&kms.GetKeyPolicyInput{ From ceed49da868c76de4e3365f7fda3f263802ead26 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Sat, 17 Jul 2021 18:43:21 -0400 Subject: [PATCH 08/36] r/aws_kms_key: Some waiter simplification. --- aws/internal/service/kms/finder/finder.go | 28 ++++++++++++++++++ aws/internal/service/kms/waiter/waiter.go | 31 ++------------------ aws/resource_aws_kms_key.go | 35 ++++++++++++----------- 3 files changed, 50 insertions(+), 44 deletions(-) diff --git a/aws/internal/service/kms/finder/finder.go b/aws/internal/service/kms/finder/finder.go index 10c616aa919..6072dea0d7f 100644 --- a/aws/internal/service/kms/finder/finder.go +++ b/aws/internal/service/kms/finder/finder.go @@ -72,3 +72,31 @@ func KeyPolicyByKeyIDAndPolicyName(conn *kms.KMS, keyID, policyName string) (str return aws.StringValue(output.Policy), nil } + +func KeyRotationStatusByKeyID(conn *kms.KMS, keyID string) (bool, error) { + input := &kms.GetKeyRotationStatusInput{ + KeyId: aws.String(keyID), + } + + output, err := conn.GetKeyRotationStatus(input) + + if tfawserr.ErrCodeEquals(err, kms.ErrCodeNotFoundException) { + return false, &resource.NotFoundError{ + LastError: err, + LastRequest: input, + } + } + + if err != nil { + return false, err + } + + if output == nil || output.KeyRotationEnabled == nil { + return false, &resource.NotFoundError{ + Message: "Empty result", + LastRequest: input, + } + } + + return aws.BoolValue(output.KeyRotationEnabled), nil +} diff --git a/aws/internal/service/kms/waiter/waiter.go b/aws/internal/service/kms/waiter/waiter.go index baf0be8544f..7f5df7eba15 100644 --- a/aws/internal/service/kms/waiter/waiter.go +++ b/aws/internal/service/kms/waiter/waiter.go @@ -4,7 +4,6 @@ import ( "time" "github.com/aws/aws-sdk-go/service/kms" - "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter" "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" @@ -15,38 +14,14 @@ const ( KeyStatePendingDeletionTimeout = 20 * time.Minute KeyDeletedTimeout = 20 * time.Minute + + PropagationTimeout = 2 * time.Minute ) // IAMPropagation retries the specified function if the returned error indicates an IAM eventual consistency issue. // If the retries time out the specified function is called one last time. func IAMPropagation(f func() (interface{}, error)) (interface{}, error) { - var output interface{} - - err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError { - var err error - - output, err = f() - - if tfawserr.ErrCodeEquals(err, kms.ErrCodeMalformedPolicyDocumentException) { - return resource.RetryableError(err) - } - - if err != nil { - return resource.NonRetryableError(err) - } - - return nil - }) - - if tfresource.TimedOut(err) { - output, err = f() - } - - if err != nil { - return nil, err - } - - return output, nil + return tfresource.RetryWhenAwsErrCodeEquals(iamwaiter.PropagationTimeout, f, kms.ErrCodeMalformedPolicyDocumentException) } func KeyDeleted(conn *kms.KMS, id string) (*kms.KeyMetadata, error) { diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index c8c8a259ff0..3d7110d889a 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -12,6 +12,7 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags" + tfkms "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms" "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/finder" "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/waiter" "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" @@ -144,6 +145,10 @@ func resourceAwsKmsKeyCreate(d *schema.ResourceData, meta interface{}) error { d.SetId(aws.StringValue(output.KeyMetadata.KeyId)) d.Set("key_id", output.KeyMetadata.KeyId) + // TODO + // TODO Ensure that DescribeKey etc. return OK here, NOT during Read. + // TODO + if enableKeyRotation := d.Get("enable_key_rotation").(bool); enableKeyRotation { if err := updateKmsKeyRotationStatus(conn, d); err != nil { return err @@ -183,33 +188,31 @@ func resourceAwsKmsKeyRead(d *schema.ResourceData, meta interface{}) error { d.Set("key_id", output.KeyId) d.Set("key_usage", output.KeyUsage) - pOut, err := retryOnAwsCode(kms.ErrCodeNotFoundException, func() (interface{}, error) { - return conn.GetKeyPolicy(&kms.GetKeyPolicyInput{ - KeyId: aws.String(d.Id()), - PolicyName: aws.String("default"), - }) + outputRaw, err := tfresource.RetryWhenNotFound(waiter.PropagationTimeout, func() (interface{}, error) { + return finder.KeyPolicyByKeyIDAndPolicyName(conn, d.Id(), tfkms.PolicyNameDefault) }) + if err != nil { - return err + return fmt.Errorf("error reading KMS Key (%s) policy: %w", d.Id(), err) } - p := pOut.(*kms.GetKeyPolicyOutput) - policy, err := structure.NormalizeJsonString(*p.Policy) + policy, err := structure.NormalizeJsonString(outputRaw.(string)) + if err != nil { - return fmt.Errorf("policy contains an invalid JSON: %w", err) + return fmt.Errorf("policy contains invalid JSON: %w", err) } + d.Set("policy", policy) - out, err := retryOnAwsCode(kms.ErrCodeNotFoundException, func() (interface{}, error) { - return conn.GetKeyRotationStatus(&kms.GetKeyRotationStatusInput{ - KeyId: aws.String(d.Id()), - }) + outputRaw, err = tfresource.RetryWhenNotFound(waiter.PropagationTimeout, func() (interface{}, error) { + return finder.KeyRotationStatusByKeyID(conn, d.Id()) }) + if err != nil { - return err + return fmt.Errorf("error reading KMS Key (%s) rotation status: %w", d.Id(), err) } - krs, _ := out.(*kms.GetKeyRotationStatusOutput) - d.Set("enable_key_rotation", krs.KeyRotationEnabled) + + d.Set("enable_key_rotation", outputRaw.(bool)) var tags keyvaluetags.KeyValueTags err = resource.Retry(2*time.Minute, func() *resource.RetryError { From e6ceb125e731cb169c3bbd344be4a8705007cfe3 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Sun, 18 Jul 2021 17:00:00 -0400 Subject: [PATCH 09/36] Add 'tfresource.RetryWhen'. --- aws/internal/tfresource/retry.go | 55 +++++++---------- aws/internal/tfresource/retry_test.go | 87 +++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 32 deletions(-) diff --git a/aws/internal/tfresource/retry.go b/aws/internal/tfresource/retry.go index 04993dd1064..be253150148 100644 --- a/aws/internal/tfresource/retry.go +++ b/aws/internal/tfresource/retry.go @@ -10,8 +10,9 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" ) -// RetryWhenAwsErrCodeEquals retries the specified function when it returns one of the specified AWS error code. -func RetryWhenAwsErrCodeEquals(timeout time.Duration, f func() (interface{}, error), codes ...string) (interface{}, error) { +// RetryWhen retries the function `f` when the error it returns satisfies `predicate`. +// `f` is retried until `timeout` expires. +func RetryWhen(timeout time.Duration, f func() (interface{}, error), predicate func(error) bool) (interface{}, error) { var output interface{} err := resource.Retry(timeout, func() *resource.RetryError { @@ -19,12 +20,8 @@ func RetryWhenAwsErrCodeEquals(timeout time.Duration, f func() (interface{}, err output, err = f() - // https://github.com/hashicorp/aws-sdk-go-base/pull/55 has been merged. - // Once aws-sdk-go-base has been updated, use variadic version of ErrCodeEquals. - for _, code := range codes { - if tfawserr.ErrCodeEquals(err, code) { - return resource.RetryableError(err) - } + if predicate(err) { + return resource.RetryableError(err) } if err != nil { @@ -45,35 +42,29 @@ func RetryWhenAwsErrCodeEquals(timeout time.Duration, f func() (interface{}, err return output, nil } -// RetryWhenNotFound retries the specified function when it returns a resource.NotFoundError. -func RetryWhenNotFound(timeout time.Duration, f func() (interface{}, error)) (interface{}, error) { - var output interface{} - - err := resource.Retry(timeout, func() *resource.RetryError { - var err error - - output, err = f() - - if NotFound(err) { - return resource.RetryableError(err) - } - - if err != nil { - return resource.NonRetryableError(err) +// RetryWhenAwsErrCodeEquals retries the specified function when it returns one of the specified AWS error code. +func RetryWhenAwsErrCodeEquals(timeout time.Duration, f func() (interface{}, error), codes ...string) (interface{}, error) { + return RetryWhen(timeout, f, func(err error) bool { + // https://github.com/hashicorp/aws-sdk-go-base/pull/55 has been merged. + // Once aws-sdk-go-base has been updated, use variadic version of ErrCodeEquals. + for _, code := range codes { + if tfawserr.ErrCodeEquals(err, code) { + return true + } } - return nil + return false }) +} - if TimedOut(err) { - output, err = f() - } - - if err != nil { - return nil, err - } +// RetryWhenNotFound retries the specified function when it returns a resource.NotFoundError. +func RetryWhenNotFound(timeout time.Duration, f func() (interface{}, error)) (interface{}, error) { + return RetryWhen(timeout, f, func(err error) bool { return NotFound(err) }) +} - return output, nil +// RetryWhenNewResourceNotFound retries the specified function when it returns a resource.NotFoundError and `isNewResource` is true. +func RetryWhenNewResourceNotFound(timeout time.Duration, f func() (interface{}, error), isNewResource bool) (interface{}, error) { + return RetryWhen(timeout, f, func(err error) bool { return isNewResource && NotFound(err) }) } // RetryConfigContext allows configuration of StateChangeConf's various time arguments. diff --git a/aws/internal/tfresource/retry_test.go b/aws/internal/tfresource/retry_test.go index 0dbb8abd2de..d02c74c098a 100644 --- a/aws/internal/tfresource/retry_test.go +++ b/aws/internal/tfresource/retry_test.go @@ -75,6 +75,93 @@ func TestRetryWhenAwsErrCodeEquals(t *testing.T) { } } +func TestRetryWhenNewResourceNotFound(t *testing.T) { + var retryCount int32 + + testCases := []struct { + Name string + F func() (interface{}, error) + NewResource bool + ExpectError bool + }{ + { + Name: "no error", + F: func() (interface{}, error) { + return nil, nil + }, + }, + { + Name: "no error new resource", + F: func() (interface{}, error) { + return nil, nil + }, + NewResource: true, + }, + { + Name: "non-retryable other error", + F: func() (interface{}, error) { + return nil, errors.New("TestCode") + }, + ExpectError: true, + }, + { + Name: "non-retryable other error new resource", + F: func() (interface{}, error) { + return nil, errors.New("TestCode") + }, + NewResource: true, + ExpectError: true, + }, + { + Name: "non-retryable AWS error", + F: func() (interface{}, error) { + return nil, awserr.New("Testing", "Testing", nil) + }, + ExpectError: true, + }, + { + Name: "retryable NotFoundError not new resource", + F: func() (interface{}, error) { + return nil, &resource.NotFoundError{} + }, + ExpectError: true, + }, + { + Name: "retryable NotFoundError new resource timeout", + F: func() (interface{}, error) { + return nil, &resource.NotFoundError{} + }, + NewResource: true, + ExpectError: true, + }, + { + Name: "retryable NotFoundError success new resource", + F: func() (interface{}, error) { + if atomic.CompareAndSwapInt32(&retryCount, 0, 1) { + return nil, &resource.NotFoundError{} + } + + return nil, nil + }, + NewResource: true, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.Name, func(t *testing.T) { + retryCount = 0 + + _, err := tfresource.RetryWhenNotFound(5*time.Second, testCase.F) + + if testCase.ExpectError && err == nil { + t.Fatal("expected error") + } else if !testCase.ExpectError && err != nil { + t.Fatalf("unexpected error: %s", err) + } + }) + } +} + func TestRetryWhenNotFound(t *testing.T) { var retryCount int32 From 381e4d58ec0b95375902f92893308b6988bb3bde Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Sun, 18 Jul 2021 17:38:04 -0400 Subject: [PATCH 10/36] r/aws_kms_key: Simplify retry on Read. --- aws/internal/service/kms/finder/finder.go | 24 ++--- aws/resource_aws_kms_key.go | 106 ++++++++++------------ 2 files changed, 59 insertions(+), 71 deletions(-) diff --git a/aws/internal/service/kms/finder/finder.go b/aws/internal/service/kms/finder/finder.go index 6072dea0d7f..91070f34982 100644 --- a/aws/internal/service/kms/finder/finder.go +++ b/aws/internal/service/kms/finder/finder.go @@ -44,7 +44,7 @@ func KeyByID(conn *kms.KMS, id string) (*kms.KeyMetadata, error) { return keyMetadata, nil } -func KeyPolicyByKeyIDAndPolicyName(conn *kms.KMS, keyID, policyName string) (string, error) { +func KeyPolicyByKeyIDAndPolicyName(conn *kms.KMS, keyID, policyName string) (*string, error) { input := &kms.GetKeyPolicyInput{ KeyId: aws.String(keyID), PolicyName: aws.String(policyName), @@ -53,27 +53,27 @@ func KeyPolicyByKeyIDAndPolicyName(conn *kms.KMS, keyID, policyName string) (str output, err := conn.GetKeyPolicy(input) if tfawserr.ErrCodeEquals(err, kms.ErrCodeNotFoundException) { - return "", &resource.NotFoundError{ + return nil, &resource.NotFoundError{ LastError: err, LastRequest: input, } } if err != nil { - return "", err + return nil, err } - if output == nil || output.Policy == nil { - return "", &resource.NotFoundError{ + if output == nil { + return nil, &resource.NotFoundError{ Message: "Empty result", LastRequest: input, } } - return aws.StringValue(output.Policy), nil + return output.Policy, nil } -func KeyRotationStatusByKeyID(conn *kms.KMS, keyID string) (bool, error) { +func KeyRotationEnabledByKeyID(conn *kms.KMS, keyID string) (*bool, error) { input := &kms.GetKeyRotationStatusInput{ KeyId: aws.String(keyID), } @@ -81,22 +81,22 @@ func KeyRotationStatusByKeyID(conn *kms.KMS, keyID string) (bool, error) { output, err := conn.GetKeyRotationStatus(input) if tfawserr.ErrCodeEquals(err, kms.ErrCodeNotFoundException) { - return false, &resource.NotFoundError{ + return nil, &resource.NotFoundError{ LastError: err, LastRequest: input, } } if err != nil { - return false, err + return nil, err } - if output == nil || output.KeyRotationEnabled == nil { - return false, &resource.NotFoundError{ + if output == nil { + return nil, &resource.NotFoundError{ Message: "Empty result", LastRequest: input, } } - return aws.BoolValue(output.KeyRotationEnabled), nil + return output.KeyRotationEnabled, nil } diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index 3d7110d889a..0a3d3cfc5ca 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -140,14 +140,8 @@ func resourceAwsKmsKeyCreate(d *schema.ResourceData, meta interface{}) error { return fmt.Errorf("error creating KMS Key: %w", err) } - output := outputRaw.(*kms.CreateKeyOutput) - - d.SetId(aws.StringValue(output.KeyMetadata.KeyId)) - d.Set("key_id", output.KeyMetadata.KeyId) - - // TODO - // TODO Ensure that DescribeKey etc. return OK here, NOT during Read. - // TODO + d.SetId(aws.StringValue(outputRaw.(*kms.CreateKeyOutput).KeyMetadata.KeyId)) + d.Set("key_id", d.Id()) if enableKeyRotation := d.Get("enable_key_rotation").(bool); enableKeyRotation { if err := updateKmsKeyRotationStatus(conn, d); err != nil { @@ -169,76 +163,68 @@ func resourceAwsKmsKeyRead(d *schema.ResourceData, meta interface{}) error { defaultTagsConfig := meta.(*AWSClient).DefaultTagsConfig ignoreTagsConfig := meta.(*AWSClient).IgnoreTagsConfig - output, err := finder.KeyByID(conn, d.Id()) - - if !d.IsNewResource() && tfresource.NotFound(err) { - log.Printf("[WARN] KMS Key (%s) not found, removing from state", d.Id()) - d.SetId("") - return nil + type Key struct { + metadata *kms.KeyMetadata + policy string + rotation *bool + tags keyvaluetags.KeyValueTags } - if err != nil { - return fmt.Errorf("error reading KMS Key (%s): %w", d.Id(), err) - } - - d.Set("arn", output.Arn) - d.Set("customer_master_key_spec", output.CustomerMasterKeySpec) - d.Set("description", output.Description) - d.Set("is_enabled", output.Enabled) - d.Set("key_id", output.KeyId) - d.Set("key_usage", output.KeyUsage) - - outputRaw, err := tfresource.RetryWhenNotFound(waiter.PropagationTimeout, func() (interface{}, error) { - return finder.KeyPolicyByKeyIDAndPolicyName(conn, d.Id(), tfkms.PolicyNameDefault) - }) - - if err != nil { - return fmt.Errorf("error reading KMS Key (%s) policy: %w", d.Id(), err) - } + outputRaw, err := tfresource.RetryWhenNewResourceNotFound(waiter.PropagationTimeout, func() (interface{}, error) { + var err error + var key Key - policy, err := structure.NormalizeJsonString(outputRaw.(string)) + key.metadata, err = finder.KeyByID(conn, d.Id()) - if err != nil { - return fmt.Errorf("policy contains invalid JSON: %w", err) - } + if err != nil { + return nil, fmt.Errorf("error reading KMS Key (%s): %w", d.Id(), err) + } - d.Set("policy", policy) + policy, err := finder.KeyPolicyByKeyIDAndPolicyName(conn, d.Id(), tfkms.PolicyNameDefault) - outputRaw, err = tfresource.RetryWhenNotFound(waiter.PropagationTimeout, func() (interface{}, error) { - return finder.KeyRotationStatusByKeyID(conn, d.Id()) - }) + if err != nil { + return nil, fmt.Errorf("error reading KMS Key (%s) policy: %w", d.Id(), err) + } - if err != nil { - return fmt.Errorf("error reading KMS Key (%s) rotation status: %w", d.Id(), err) - } + key.policy, err = structure.NormalizeJsonString(aws.StringValue(policy)) - d.Set("enable_key_rotation", outputRaw.(bool)) + if err != nil { + return nil, fmt.Errorf("policy contains invalid JSON: %w", err) + } - var tags keyvaluetags.KeyValueTags - err = resource.Retry(2*time.Minute, func() *resource.RetryError { - var err error - tags, err = keyvaluetags.KmsListTags(conn, d.Id()) + key.rotation, err = finder.KeyRotationEnabledByKeyID(conn, d.Id()) - if d.IsNewResource() && isAWSErr(err, kms.ErrCodeNotFoundException, "") { - return resource.RetryableError(err) + if err != nil { + return nil, fmt.Errorf("error reading KMS Key (%s) rotation enabled: %w", d.Id(), err) } + key.tags, err = keyvaluetags.KmsListTags(conn, d.Id()) + if err != nil { - return resource.NonRetryableError(err) + return nil, fmt.Errorf("error listing tags for KMS Key (%s): %w", d.Id(), err) } - return nil - }) + return &key, nil + }, d.IsNewResource()) - if isResourceTimeoutError(err) { - tags, err = keyvaluetags.KmsListTags(conn, d.Id()) + if !d.IsNewResource() && tfresource.NotFound(err) { + log.Printf("[WARN] KMS Key (%s) not found, removing from state", d.Id()) + d.SetId("") + return nil } - if err != nil { - return fmt.Errorf("error listing tags for KMS Key (%s): %w", d.Id(), err) - } + key := outputRaw.(*Key) - tags = tags.IgnoreAws().IgnoreConfig(ignoreTagsConfig) + d.Set("arn", key.metadata.Arn) + d.Set("customer_master_key_spec", key.metadata.CustomerMasterKeySpec) + d.Set("description", key.metadata.Description) + d.Set("enable_key_rotation", key.rotation) + d.Set("is_enabled", key.metadata.Enabled) + d.Set("key_id", key.metadata.KeyId) + d.Set("key_usage", key.metadata.KeyUsage) + d.Set("policy", key.policy) + + tags := key.tags.IgnoreAws().IgnoreConfig(ignoreTagsConfig) //lintignore:AWSR002 if err := d.Set("tags", tags.RemoveDefaultConfig(defaultTagsConfig).Map()); err != nil { @@ -488,6 +474,8 @@ func resourceAwsKmsKeyDelete(d *schema.ResourceData, meta interface{}) error { return fmt.Errorf("error scheduling deletion for KMS Key (%s): %w", d.Id(), err) } + // Error: error scheduling deletion for KMS Key (a93d0bf5-ff4c-4323-9f9a-2151ac639254): KMSInvalidStateException: arn:aws:kms:us-west-2:346386234494:key/a93d0bf5-ff4c-4323-9f9a-2151ac639254 is pending deletion. + _, err = waiter.KeyStatePendingDeletion(conn, d.Id()) if isAWSErr(err, kms.ErrCodeNotFoundException, "") { From 352de328e1efaa0975068c0392606da167a36287 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Tue, 20 Jul 2021 17:11:06 -0400 Subject: [PATCH 11/36] Tweak 'RetryWhenNotFound'. --- aws/internal/tfresource/retry.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws/internal/tfresource/retry.go b/aws/internal/tfresource/retry.go index be253150148..1e694fdcd39 100644 --- a/aws/internal/tfresource/retry.go +++ b/aws/internal/tfresource/retry.go @@ -59,7 +59,7 @@ func RetryWhenAwsErrCodeEquals(timeout time.Duration, f func() (interface{}, err // RetryWhenNotFound retries the specified function when it returns a resource.NotFoundError. func RetryWhenNotFound(timeout time.Duration, f func() (interface{}, error)) (interface{}, error) { - return RetryWhen(timeout, f, func(err error) bool { return NotFound(err) }) + return RetryWhen(timeout, f, NotFound) } // RetryWhenNewResourceNotFound retries the specified function when it returns a resource.NotFoundError and `isNewResource` is true. From 740579a6f1a522926b5b6cb6105a1fef24b63cde Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Tue, 20 Jul 2021 17:14:52 -0400 Subject: [PATCH 12/36] Tweak 'SetLastError'. --- aws/internal/tfresource/errors.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/aws/internal/tfresource/errors.go b/aws/internal/tfresource/errors.go index 032ea8f42e9..407804df7d7 100644 --- a/aws/internal/tfresource/errors.go +++ b/aws/internal/tfresource/errors.go @@ -27,12 +27,9 @@ func TimedOut(err error) bool { // SetLastError sets the LastError field on the error if supported. // If lastErr is nil it is ignored. func SetLastError(err, lastErr error) { - var te *resource.TimeoutError - var use *resource.UnexpectedStateError - - if ok := errors.As(err, &te); ok && te.LastError == nil { + if te := (*resource.TimeoutError)(nil); errors.As(err, &te) && te.LastError == nil { te.LastError = lastErr - } else if ok := errors.As(err, &use); ok && use.LastError == nil { + } else if use := (*resource.UnexpectedStateError)(nil); errors.As(err, &use) && use.LastError == nil { use.LastError = lastErr } } From d0810a835fd6d2ad09189c4d515b8e193a5c5f96 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Wed, 21 Jul 2021 12:48:58 -0400 Subject: [PATCH 13/36] Add 'Context' versions of Retry functions. --- aws/internal/tfresource/retry.go | 35 +++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/aws/internal/tfresource/retry.go b/aws/internal/tfresource/retry.go index 1e694fdcd39..4dc26737c62 100644 --- a/aws/internal/tfresource/retry.go +++ b/aws/internal/tfresource/retry.go @@ -10,9 +10,9 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" ) -// RetryWhen retries the function `f` when the error it returns satisfies `predicate`. +// RetryWhenContext retries the function `f` when the error it returns satisfies `predicate`. // `f` is retried until `timeout` expires. -func RetryWhen(timeout time.Duration, f func() (interface{}, error), predicate func(error) bool) (interface{}, error) { +func RetryWhenContext(ctx context.Context, timeout time.Duration, f func() (interface{}, error), predicate func(error) bool) (interface{}, error) { var output interface{} err := resource.Retry(timeout, func() *resource.RetryError { @@ -42,9 +42,15 @@ func RetryWhen(timeout time.Duration, f func() (interface{}, error), predicate f return output, nil } -// RetryWhenAwsErrCodeEquals retries the specified function when it returns one of the specified AWS error code. -func RetryWhenAwsErrCodeEquals(timeout time.Duration, f func() (interface{}, error), codes ...string) (interface{}, error) { - return RetryWhen(timeout, f, func(err error) bool { +// RetryWhen retries the function `f` when the error it returns satisfies `predicate`. +// `f` is retried until `timeout` expires. +func RetryWhen(timeout time.Duration, f func() (interface{}, error), predicate func(error) bool) (interface{}, error) { + return RetryWhenContext(context.Background(), timeout, f, predicate) +} + +// RetryWhenAwsErrCodeEqualsContext retries the specified function when it returns one of the specified AWS error code. +func RetryWhenAwsErrCodeEqualsContext(ctx context.Context, timeout time.Duration, f func() (interface{}, error), codes ...string) (interface{}, error) { + return RetryWhenContext(ctx, timeout, f, func(err error) bool { // https://github.com/hashicorp/aws-sdk-go-base/pull/55 has been merged. // Once aws-sdk-go-base has been updated, use variadic version of ErrCodeEquals. for _, code := range codes { @@ -57,14 +63,29 @@ func RetryWhenAwsErrCodeEquals(timeout time.Duration, f func() (interface{}, err }) } +// RetryWhenAwsErrCodeEquals retries the specified function when it returns one of the specified AWS error code. +func RetryWhenAwsErrCodeEquals(timeout time.Duration, f func() (interface{}, error), codes ...string) (interface{}, error) { + return RetryWhenAwsErrCodeEqualsContext(context.Background(), timeout, f, codes...) +} + +// RetryWhenNotFoundContext retries the specified function when it returns a resource.NotFoundError. +func RetryWhenNotFoundContext(ctx context.Context, timeout time.Duration, f func() (interface{}, error)) (interface{}, error) { + return RetryWhenContext(ctx, timeout, f, NotFound) +} + // RetryWhenNotFound retries the specified function when it returns a resource.NotFoundError. func RetryWhenNotFound(timeout time.Duration, f func() (interface{}, error)) (interface{}, error) { - return RetryWhen(timeout, f, NotFound) + return RetryWhenNotFoundContext(context.Background(), timeout, f) +} + +// RetryWhenNewResourceNotFoundContext retries the specified function when it returns a resource.NotFoundError and `isNewResource` is true. +func RetryWhenNewResourceNotFoundContext(ctx context.Context, timeout time.Duration, f func() (interface{}, error), isNewResource bool) (interface{}, error) { + return RetryWhenContext(ctx, timeout, f, func(err error) bool { return isNewResource && NotFound(err) }) } // RetryWhenNewResourceNotFound retries the specified function when it returns a resource.NotFoundError and `isNewResource` is true. func RetryWhenNewResourceNotFound(timeout time.Duration, f func() (interface{}, error), isNewResource bool) (interface{}, error) { - return RetryWhen(timeout, f, func(err error) bool { return isNewResource && NotFound(err) }) + return RetryWhenNewResourceNotFoundContext(context.Background(), timeout, f, isNewResource) } // RetryConfigContext allows configuration of StateChangeConf's various time arguments. From bfe31d9d60159263b4f7988f493fc3131772f63e Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Wed, 21 Jul 2021 14:08:30 -0400 Subject: [PATCH 14/36] Add 'tfresource.WaitUntil'. --- aws/internal/tfresource/wait.go | 67 ++++++++++++++++++++++++++++ aws/internal/tfresource/wait_test.go | 65 +++++++++++++++++++++++++++ 2 files changed, 132 insertions(+) create mode 100644 aws/internal/tfresource/wait.go create mode 100644 aws/internal/tfresource/wait_test.go diff --git a/aws/internal/tfresource/wait.go b/aws/internal/tfresource/wait.go new file mode 100644 index 00000000000..6bf588a3c7a --- /dev/null +++ b/aws/internal/tfresource/wait.go @@ -0,0 +1,67 @@ +package tfresource + +import ( + "context" + "time" + + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" +) + +type WaitOpts struct { + ContinuousTargetOccurence int // Number of times the target state has to occur continuously. + Delay time.Duration // Wait this time before starting checks. + MinTimeout time.Duration // Smallest time to wait before refreshes. + PollInterval time.Duration // Override MinTimeout/backoff and only poll this often. +} + +const ( + targetStateError = "ERROR" + targetStateFalse = "FALSE" + targetStateTrue = "TRUE" +) + +// WaitUntilContext waits for the function `f` to return `true`. +// If `f` returns an error, return immediately with that error. +// If `timeout` is exceeded before `f` returns `true`, return an error. +// Waits between calls to `f` using exponential backoff, except when waiting for the target state to reoccur. +func WaitUntilContext(ctx context.Context, timeout time.Duration, f func() (bool, error), opts *WaitOpts) error { + refresh := func() (interface{}, string, error) { + done, err := f() + + if err != nil { + return nil, targetStateError, err + } + + if done { + return "", targetStateTrue, nil + } + + return "", targetStateFalse, nil + } + + stateConf := &resource.StateChangeConf{ + Pending: []string{targetStateFalse}, + Target: []string{targetStateTrue}, + Refresh: refresh, + Timeout: timeout, + } + + if opts != nil { + stateConf.ContinuousTargetOccurence = opts.ContinuousTargetOccurence + stateConf.Delay = opts.Delay + stateConf.MinTimeout = opts.MinTimeout + stateConf.PollInterval = opts.PollInterval + } + + _, err := stateConf.WaitForStateContext(ctx) + + return err +} + +// WaitUntil waits for the function `f` to return `true`. +// If `f` returns an error, return immediately with that error. +// If `timeout` is exceeded before `f` returns `true`, return an error. +// Waits between calls to `f` using exponential backoff, except when waiting for the target state to reoccur. +func WaitUntil(timeout time.Duration, f func() (bool, error), opts *WaitOpts) error { + return WaitUntilContext(context.Background(), timeout, f, opts) +} diff --git a/aws/internal/tfresource/wait_test.go b/aws/internal/tfresource/wait_test.go new file mode 100644 index 00000000000..606d0fa1a1b --- /dev/null +++ b/aws/internal/tfresource/wait_test.go @@ -0,0 +1,65 @@ +package tfresource_test + +import ( + "errors" + "sync/atomic" + "testing" + "time" + + "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" +) + +func TestWaitUntil(t *testing.T) { + var retryCount int32 + + testCases := []struct { + Name string + F func() (bool, error) + ExpectError bool + }{ + { + Name: "no error", + F: func() (bool, error) { + return true, nil + }, + }, + { + Name: "immediate error", + F: func() (bool, error) { + return false, errors.New("TestCode") + }, + ExpectError: true, + }, + { + Name: "never reaches state", + F: func() (bool, error) { + return false, nil + }, + ExpectError: true, + }, + { + Name: "retry then success", + F: func() (bool, error) { + if atomic.CompareAndSwapInt32(&retryCount, 0, 1) { + return true, nil + } + + return false, nil + }, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.Name, func(t *testing.T) { + retryCount = 0 + + err := tfresource.WaitUntil(5*time.Second, testCase.F, nil) + + if testCase.ExpectError && err == nil { + t.Fatal("expected error") + } else if !testCase.ExpectError && err != nil { + t.Fatalf("unexpected error: %s", err) + } + }) + } +} From 56a56975db39a84cbb499726b43652948744b2f3 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Wed, 21 Jul 2021 14:42:54 -0400 Subject: [PATCH 15/36] Add 'updateKmsKeyRotationEnabled'. --- aws/internal/service/kms/finder/finder.go | 1 + aws/internal/service/kms/waiter/waiter.go | 3 +- aws/internal/tfresource/wait.go | 4 +- aws/resource_aws_kms_key.go | 80 ++++++++++++++++++----- 4 files changed, 70 insertions(+), 18 deletions(-) diff --git a/aws/internal/service/kms/finder/finder.go b/aws/internal/service/kms/finder/finder.go index 91070f34982..b65c6389a61 100644 --- a/aws/internal/service/kms/finder/finder.go +++ b/aws/internal/service/kms/finder/finder.go @@ -34,6 +34,7 @@ func KeyByID(conn *kms.KMS, id string) (*kms.KeyMetadata, error) { keyMetadata := output.KeyMetadata + // Once the CMK is in the pending deletion state Terraform considers it logically deleted. if state := aws.StringValue(keyMetadata.KeyState); state == kms.KeyStatePendingDeletion { return nil, &resource.NotFoundError{ Message: state, diff --git a/aws/internal/service/kms/waiter/waiter.go b/aws/internal/service/kms/waiter/waiter.go index 7f5df7eba15..da70f146c1d 100644 --- a/aws/internal/service/kms/waiter/waiter.go +++ b/aws/internal/service/kms/waiter/waiter.go @@ -15,7 +15,8 @@ const ( KeyDeletedTimeout = 20 * time.Minute - PropagationTimeout = 2 * time.Minute + KeyRotationUpdatedTimeout = 20 * time.Minute + PropagationTimeout = 2 * time.Minute ) // IAMPropagation retries the specified function if the returned error indicates an IAM eventual consistency issue. diff --git a/aws/internal/tfresource/wait.go b/aws/internal/tfresource/wait.go index 6bf588a3c7a..75470d7afc2 100644 --- a/aws/internal/tfresource/wait.go +++ b/aws/internal/tfresource/wait.go @@ -16,8 +16,8 @@ type WaitOpts struct { const ( targetStateError = "ERROR" - targetStateFalse = "FALSE" - targetStateTrue = "TRUE" + targetStateFalse = "FALSE" + targetStateTrue = "TRUE" ) // WaitUntilContext waits for the function `f` to return `true`. diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index 0a3d3cfc5ca..b08a4c2e6aa 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -7,6 +7,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/kms" + "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure" @@ -144,7 +145,7 @@ func resourceAwsKmsKeyCreate(d *schema.ResourceData, meta interface{}) error { d.Set("key_id", d.Id()) if enableKeyRotation := d.Get("enable_key_rotation").(bool); enableKeyRotation { - if err := updateKmsKeyRotationStatus(conn, d); err != nil { + if err := updateKmsKeyRotationEnabled(conn, d.Id(), enableKeyRotation); err != nil { return err } } @@ -249,7 +250,7 @@ func resourceAwsKmsKeyUpdate(d *schema.ResourceData, meta interface{}) error { } if d.HasChange("enable_key_rotation") { - if err := updateKmsKeyRotationStatus(conn, d); err != nil { + if err := updateKmsKeyRotationEnabled(conn, d.Id(), d.Get("enable_key_rotation").(bool)); err != nil { return err } } @@ -456,34 +457,83 @@ func handleKeyRotation(conn *kms.KMS, shouldEnableRotation bool, keyId *string) func resourceAwsKmsKeyDelete(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).kmsconn - keyId := d.Get("key_id").(string) - req := &kms.ScheduleKeyDeletionInput{ - KeyId: aws.String(keyId), + input := &kms.ScheduleKeyDeletionInput{ + KeyId: aws.String(d.Id()), + } + + if v, ok := d.GetOk("deletion_window_in_days"); ok { + input.PendingWindowInDays = aws.Int64(int64(v.(int))) } - if v, exists := d.GetOk("deletion_window_in_days"); exists { - req.PendingWindowInDays = aws.Int64(int64(v.(int))) + + log.Printf("[DEBUG] Deleting KMS Key: (%s)", d.Id()) + _, err := conn.ScheduleKeyDeletion(input) + + if tfawserr.ErrCodeEquals(err, kms.ErrCodeNotFoundException) { + return nil } - _, err := conn.ScheduleKeyDeletion(req) - if isAWSErr(err, kms.ErrCodeNotFoundException, "") { + if tfawserr.ErrMessageContains(err, kms.ErrCodeInvalidStateException, "is pending deletion") { return nil } if err != nil { - return fmt.Errorf("error scheduling deletion for KMS Key (%s): %w", d.Id(), err) + return fmt.Errorf("error deleting KMS Key (%s): %w", d.Id(), err) + } + + if _, err := waiter.KeyDeleted(conn, d.Id()); err != nil { + return fmt.Errorf("error waiting for KMS Key (%s) to delete: %w", d.Id(), err) } - // Error: error scheduling deletion for KMS Key (a93d0bf5-ff4c-4323-9f9a-2151ac639254): KMSInvalidStateException: arn:aws:kms:us-west-2:346386234494:key/a93d0bf5-ff4c-4323-9f9a-2151ac639254 is pending deletion. + return nil +} - _, err = waiter.KeyStatePendingDeletion(conn, d.Id()) +func updateKmsKeyRotationEnabled(conn *kms.KMS, keyID string, enabled bool) error { + updateFunc := func() (interface{}, error) { + var err error - if isAWSErr(err, kms.ErrCodeNotFoundException, "") { - return nil + if enabled { + _, err = conn.EnableKeyRotation(&kms.EnableKeyRotationInput{ + KeyId: aws.String(keyID), + }) + } else { + _, err = conn.DisableKeyRotation(&kms.DisableKeyRotationInput{ + KeyId: aws.String(keyID), + }) + } + + return nil, err } + _, err := tfresource.RetryWhenAwsErrCodeEquals(waiter.KeyRotationUpdatedTimeout, updateFunc, kms.ErrCodeNotFoundException, kms.ErrCodeDisabledException) + + if err != nil { + return fmt.Errorf("error updating KMS Key (%s) key rotation enabled (%t): %w", keyID, enabled, err) + } + + // Wait for propagation since KMS is eventually consistent. + checkFunc := func() (bool, error) { + got, err := finder.KeyRotationEnabledByKeyID(conn, keyID) + + if tfresource.NotFound(err) { + return false, nil + } + + if err != nil { + return false, err + } + + return aws.BoolValue(got) == enabled, nil + } + opts := &tfresource.WaitOpts{ + ContinuousTargetOccurence: 5, + MinTimeout: 1 * time.Second, + } + + err = tfresource.WaitUntil(waiter.PropagationTimeout, checkFunc, opts) + if err != nil { - return fmt.Errorf("error waiting for KMS Key (%s) to schedule deletion: %w", d.Id(), err) + return fmt.Errorf("error waiting for KMS Key (%s) key rotation propagation: %w", keyID, err) } return nil From 5ad13b71f9b047975b08301bad6cca843c3fb449 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Wed, 21 Jul 2021 17:33:26 -0400 Subject: [PATCH 16/36] r/aws_kms_key: Tidy up eventual consistency checks. --- aws/internal/service/kms/waiter/waiter.go | 7 +- aws/resource_aws_kms_external_key.go | 134 +++++++++++ aws/resource_aws_kms_key.go | 262 +++++++--------------- aws/resource_aws_kms_key_test.go | 27 ++- 4 files changed, 232 insertions(+), 198 deletions(-) diff --git a/aws/internal/service/kms/waiter/waiter.go b/aws/internal/service/kms/waiter/waiter.go index da70f146c1d..eac97494d4b 100644 --- a/aws/internal/service/kms/waiter/waiter.go +++ b/aws/internal/service/kms/waiter/waiter.go @@ -13,10 +13,11 @@ const ( // Maximum amount of time to wait for KeyState to return PendingDeletion KeyStatePendingDeletionTimeout = 20 * time.Minute - KeyDeletedTimeout = 20 * time.Minute + KeyDeletedTimeout = 20 * time.Minute + KeyRotationUpdatedTimeout = 10 * time.Minute + KeyStatePropagationTimeout = 20 * time.Minute - KeyRotationUpdatedTimeout = 20 * time.Minute - PropagationTimeout = 2 * time.Minute + PropagationTimeout = 2 * time.Minute ) // IAMPropagation retries the specified function if the returned error indicates an IAM eventual consistency issue. diff --git a/aws/resource_aws_kms_external_key.go b/aws/resource_aws_kms_external_key.go index 933dd6e7dd7..921ac3ea748 100644 --- a/aws/resource_aws_kms_external_key.go +++ b/aws/resource_aws_kms_external_key.go @@ -462,3 +462,137 @@ func importKmsExternalKeyMaterial(conn *kms.KMS, keyID, keyMaterialBase64, valid return nil } + +func updateKmsKeyStatus(conn *kms.KMS, id string, shouldBeEnabled bool) error { + var err error + + if shouldBeEnabled { + log.Printf("[DEBUG] Enabling KMS key %q", id) + _, err = conn.EnableKey(&kms.EnableKeyInput{ + KeyId: aws.String(id), + }) + } else { + log.Printf("[DEBUG] Disabling KMS key %q", id) + _, err = conn.DisableKey(&kms.DisableKeyInput{ + KeyId: aws.String(id), + }) + } + + if err != nil { + return fmt.Errorf("Failed to set KMS key %q status to %t: %q", + id, shouldBeEnabled, err.Error()) + } + + // Wait for propagation since KMS is eventually consistent + wait := resource.StateChangeConf{ + Pending: []string{fmt.Sprintf("%t", !shouldBeEnabled)}, + Target: []string{fmt.Sprintf("%t", shouldBeEnabled)}, + Timeout: 20 * time.Minute, + MinTimeout: 2 * time.Second, + ContinuousTargetOccurence: 15, + Refresh: func() (interface{}, string, error) { + log.Printf("[DEBUG] Checking if KMS key %s enabled status is %t", + id, shouldBeEnabled) + resp, err := conn.DescribeKey(&kms.DescribeKeyInput{ + KeyId: aws.String(id), + }) + if err != nil { + if isAWSErr(err, kms.ErrCodeNotFoundException, "") { + return nil, fmt.Sprintf("%t", !shouldBeEnabled), nil + } + return resp, "FAILED", err + } + status := fmt.Sprintf("%t", *resp.KeyMetadata.Enabled) + log.Printf("[DEBUG] KMS key %s status received: %s, retrying", id, status) + + return resp, status, nil + }, + } + + _, err = wait.WaitForState() + if err != nil { + return fmt.Errorf("Failed setting KMS key status to %t: %w", shouldBeEnabled, err) + } + + return nil +} + +func updateKmsKeyRotationStatus(conn *kms.KMS, d *schema.ResourceData) error { + shouldEnableRotation := d.Get("enable_key_rotation").(bool) + + err := resource.Retry(10*time.Minute, func() *resource.RetryError { + err := handleKeyRotation(conn, shouldEnableRotation, aws.String(d.Id())) + + if err != nil { + if isAWSErr(err, kms.ErrCodeDisabledException, "") { + return resource.RetryableError(err) + } + if isAWSErr(err, kms.ErrCodeNotFoundException, "") { + return resource.RetryableError(err) + } + + return resource.NonRetryableError(err) + } + + return nil + }) + if isResourceTimeoutError(err) { + err = handleKeyRotation(conn, shouldEnableRotation, aws.String(d.Id())) + } + + if err != nil { + return fmt.Errorf("Failed to set key rotation for %q to %t: %q", + d.Id(), shouldEnableRotation, err.Error()) + } + + // Wait for propagation since KMS is eventually consistent + wait := resource.StateChangeConf{ + Pending: []string{fmt.Sprintf("%t", !shouldEnableRotation)}, + Target: []string{fmt.Sprintf("%t", shouldEnableRotation)}, + Timeout: 5 * time.Minute, + MinTimeout: 1 * time.Second, + ContinuousTargetOccurence: 5, + Refresh: func() (interface{}, string, error) { + log.Printf("[DEBUG] Checking if KMS key %s rotation status is %t", + d.Id(), shouldEnableRotation) + + out, err := retryOnAwsCode(kms.ErrCodeNotFoundException, func() (interface{}, error) { + return conn.GetKeyRotationStatus(&kms.GetKeyRotationStatusInput{ + KeyId: aws.String(d.Id()), + }) + }) + if err != nil { + return 42, "", err + } + resp, _ := out.(*kms.GetKeyRotationStatusOutput) + + status := fmt.Sprintf("%t", *resp.KeyRotationEnabled) + log.Printf("[DEBUG] KMS key %s rotation status received: %s, retrying", d.Id(), status) + + return resp, status, nil + }, + } + + _, err = wait.WaitForState() + if err != nil { + return fmt.Errorf("Failed setting KMS key rotation status to %t: %s", shouldEnableRotation, err) + } + + return nil +} + +func handleKeyRotation(conn *kms.KMS, shouldEnableRotation bool, keyId *string) error { + var err error + if shouldEnableRotation { + log.Printf("[DEBUG] Enabling key rotation for KMS key %q", *keyId) + _, err = conn.EnableKeyRotation(&kms.EnableKeyRotationInput{ + KeyId: keyId, + }) + } else { + log.Printf("[DEBUG] Disabling key rotation for KMS key %q", *keyId) + _, err = conn.DisableKeyRotation(&kms.DisableKeyRotationInput{ + KeyId: keyId, + }) + } + return err +} diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index b08a4c2e6aa..2cf3bbfa13f 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -8,7 +8,6 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/kms" "github.com/hashicorp/aws-sdk-go-base/tfawserr" - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" @@ -151,7 +150,7 @@ func resourceAwsKmsKeyCreate(d *schema.ResourceData, meta interface{}) error { } if enabled := d.Get("is_enabled").(bool); !enabled { - if err := updateKmsKeyStatus(conn, d.Id(), enabled); err != nil { + if err := updateKmsKeyEnabled(conn, d.Id(), enabled); err != nil { return err } } @@ -242,34 +241,58 @@ func resourceAwsKmsKeyRead(d *schema.ResourceData, meta interface{}) error { func resourceAwsKmsKeyUpdate(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).kmsconn - if d.HasChange("is_enabled") && d.Get("is_enabled").(bool) { - // Enable before any attributes will be modified - if err := updateKmsKeyStatus(conn, d.Id(), d.Get("is_enabled").(bool)); err != nil { + if hasChange, enabled := d.HasChange("is_enabled"), d.Get("is_enabled").(bool); hasChange && enabled { + // Enable before any attributes are modified. + if err := updateKmsKeyEnabled(conn, d.Id(), enabled); err != nil { return err } } - if d.HasChange("enable_key_rotation") { - if err := updateKmsKeyRotationEnabled(conn, d.Id(), d.Get("enable_key_rotation").(bool)); err != nil { + if hasChange, enableKeyRotation := d.HasChange("enable_key_rotation"), d.Get("enable_key_rotation").(bool); hasChange { + if err := updateKmsKeyRotationEnabled(conn, d.Id(), enableKeyRotation); err != nil { return err } } if d.HasChange("description") { - if err := resourceAwsKmsKeyDescriptionUpdate(conn, d); err != nil { - return err + input := &kms.UpdateKeyDescriptionInput{ + Description: aws.String(d.Get("description").(string)), + KeyId: aws.String(d.Id()), + } + + log.Printf("[DEBUG] Updating KMS Key description: %s", input) + _, err := conn.UpdateKeyDescription(input) + + if err != nil { + return fmt.Errorf("error updating KMS Key (%s) description: %w", d.Id(), err) } } + if d.HasChange("policy") { - if err := resourceAwsKmsKeyPolicyUpdate(conn, d); err != nil { - return err + policy, err := structure.NormalizeJsonString(d.Get("policy").(string)) + + if err != nil { + return fmt.Errorf("policy contains invalid JSON: %w", err) + } + + input := &kms.PutKeyPolicyInput{ + BypassPolicyLockoutSafetyCheck: aws.Bool(d.Get("bypass_policy_lockout_check").(bool)), + KeyId: aws.String(d.Id()), + Policy: aws.String(policy), + PolicyName: aws.String(tfkms.PolicyNameDefault), + } + + log.Printf("[DEBUG] Updating KMS Key policy: %s", input) + _, err = conn.PutKeyPolicy(input) + + if err != nil { + return fmt.Errorf("error updating KMS Key (%s) policy: %w", d.Id(), err) } } - if d.HasChange("is_enabled") && !d.Get("is_enabled").(bool) { - // Only disable when all attributes are modified - // because we cannot modify disabled keys - if err := updateKmsKeyStatus(conn, d.Id(), d.Get("is_enabled").(bool)); err != nil { + if hasChange, enabled := d.HasChange("is_enabled"), d.Get("is_enabled").(bool); hasChange && !enabled { + // Only disable after all attributes have been modified because we cannot modify disabled keys. + if err := updateKmsKeyEnabled(conn, d.Id(), enabled); err != nil { return err } } @@ -285,204 +308,80 @@ func resourceAwsKmsKeyUpdate(d *schema.ResourceData, meta interface{}) error { return resourceAwsKmsKeyRead(d, meta) } -func resourceAwsKmsKeyDescriptionUpdate(conn *kms.KMS, d *schema.ResourceData) error { - description := d.Get("description").(string) - keyId := d.Get("key_id").(string) +func resourceAwsKmsKeyDelete(d *schema.ResourceData, meta interface{}) error { + conn := meta.(*AWSClient).kmsconn - log.Printf("[DEBUG] KMS key: %s, update description: %s", keyId, description) + input := &kms.ScheduleKeyDeletionInput{ + KeyId: aws.String(d.Id()), + } - req := &kms.UpdateKeyDescriptionInput{ - Description: aws.String(description), - KeyId: aws.String(keyId), + if v, ok := d.GetOk("deletion_window_in_days"); ok { + input.PendingWindowInDays = aws.Int64(int64(v.(int))) } - _, err := conn.UpdateKeyDescription(req) - return err -} + log.Printf("[DEBUG] Deleting KMS Key: (%s)", d.Id()) + _, err := conn.ScheduleKeyDeletion(input) -func resourceAwsKmsKeyPolicyUpdate(conn *kms.KMS, d *schema.ResourceData) error { - policy, err := structure.NormalizeJsonString(d.Get("policy").(string)) - if err != nil { - return fmt.Errorf("policy contains an invalid JSON: %w", err) + if tfawserr.ErrCodeEquals(err, kms.ErrCodeNotFoundException) { + return nil } - keyId := d.Get("key_id").(string) - bypassPolicyLockoutCheck := d.Get("bypass_policy_lockout_check").(bool) - log.Printf("[DEBUG] KMS key: %s, bypass policy lockout check: %t, update policy: %s", keyId, bypassPolicyLockoutCheck, policy) + if tfawserr.ErrMessageContains(err, kms.ErrCodeInvalidStateException, "is pending deletion") { + return nil + } + + if err != nil { + return fmt.Errorf("error deleting KMS Key (%s): %w", d.Id(), err) + } - req := &kms.PutKeyPolicyInput{ - KeyId: aws.String(keyId), - Policy: aws.String(policy), - PolicyName: aws.String("default"), - BypassPolicyLockoutSafetyCheck: aws.Bool(bypassPolicyLockoutCheck), + if _, err := waiter.KeyDeleted(conn, d.Id()); err != nil { + return fmt.Errorf("error waiting for KMS Key (%s) to delete: %w", d.Id(), err) } - _, err = conn.PutKeyPolicy(req) - return err + return nil } -func updateKmsKeyStatus(conn *kms.KMS, id string, shouldBeEnabled bool) error { +func updateKmsKeyEnabled(conn *kms.KMS, keyID string, enabled bool) error { var err error - if shouldBeEnabled { - log.Printf("[DEBUG] Enabling KMS key %q", id) + log.Printf("[DEBUG] Updating KMS Key (%s) key enabled: %t", keyID, enabled) + if enabled { _, err = conn.EnableKey(&kms.EnableKeyInput{ - KeyId: aws.String(id), + KeyId: aws.String(keyID), }) } else { - log.Printf("[DEBUG] Disabling KMS key %q", id) _, err = conn.DisableKey(&kms.DisableKeyInput{ - KeyId: aws.String(id), + KeyId: aws.String(keyID), }) } if err != nil { - return fmt.Errorf("Failed to set KMS key %q status to %t: %q", - id, shouldBeEnabled, err.Error()) - } - - // Wait for propagation since KMS is eventually consistent - wait := resource.StateChangeConf{ - Pending: []string{fmt.Sprintf("%t", !shouldBeEnabled)}, - Target: []string{fmt.Sprintf("%t", shouldBeEnabled)}, - Timeout: 20 * time.Minute, - MinTimeout: 2 * time.Second, - ContinuousTargetOccurence: 15, - Refresh: func() (interface{}, string, error) { - log.Printf("[DEBUG] Checking if KMS key %s enabled status is %t", - id, shouldBeEnabled) - resp, err := conn.DescribeKey(&kms.DescribeKeyInput{ - KeyId: aws.String(id), - }) - if err != nil { - if isAWSErr(err, kms.ErrCodeNotFoundException, "") { - return nil, fmt.Sprintf("%t", !shouldBeEnabled), nil - } - return resp, "FAILED", err - } - status := fmt.Sprintf("%t", *resp.KeyMetadata.Enabled) - log.Printf("[DEBUG] KMS key %s status received: %s, retrying", id, status) - - return resp, status, nil - }, - } - - _, err = wait.WaitForState() - if err != nil { - return fmt.Errorf("Failed setting KMS key status to %t: %w", shouldBeEnabled, err) + return fmt.Errorf("error updating KMS Key (%s) key enabled (%t): %w", keyID, enabled, err) } - return nil -} - -func updateKmsKeyRotationStatus(conn *kms.KMS, d *schema.ResourceData) error { - shouldEnableRotation := d.Get("enable_key_rotation").(bool) + // Wait for propagation since KMS is eventually consistent. + checkFunc := func() (bool, error) { + output, err := finder.KeyByID(conn, keyID) - err := resource.Retry(10*time.Minute, func() *resource.RetryError { - err := handleKeyRotation(conn, shouldEnableRotation, aws.String(d.Id())) + if tfresource.NotFound(err) { + return false, nil + } if err != nil { - if isAWSErr(err, kms.ErrCodeDisabledException, "") { - return resource.RetryableError(err) - } - if isAWSErr(err, kms.ErrCodeNotFoundException, "") { - return resource.RetryableError(err) - } - - return resource.NonRetryableError(err) + return false, err } - return nil - }) - if isResourceTimeoutError(err) { - err = handleKeyRotation(conn, shouldEnableRotation, aws.String(d.Id())) - } - - if err != nil { - return fmt.Errorf("Failed to set key rotation for %q to %t: %q", - d.Id(), shouldEnableRotation, err.Error()) - } - - // Wait for propagation since KMS is eventually consistent - wait := resource.StateChangeConf{ - Pending: []string{fmt.Sprintf("%t", !shouldEnableRotation)}, - Target: []string{fmt.Sprintf("%t", shouldEnableRotation)}, - Timeout: 5 * time.Minute, - MinTimeout: 1 * time.Second, - ContinuousTargetOccurence: 5, - Refresh: func() (interface{}, string, error) { - log.Printf("[DEBUG] Checking if KMS key %s rotation status is %t", - d.Id(), shouldEnableRotation) - - out, err := retryOnAwsCode(kms.ErrCodeNotFoundException, func() (interface{}, error) { - return conn.GetKeyRotationStatus(&kms.GetKeyRotationStatusInput{ - KeyId: aws.String(d.Id()), - }) - }) - if err != nil { - return 42, "", err - } - resp, _ := out.(*kms.GetKeyRotationStatusOutput) - - status := fmt.Sprintf("%t", *resp.KeyRotationEnabled) - log.Printf("[DEBUG] KMS key %s rotation status received: %s, retrying", d.Id(), status) - - return resp, status, nil - }, - } - - _, err = wait.WaitForState() - if err != nil { - return fmt.Errorf("Failed setting KMS key rotation status to %t: %s", shouldEnableRotation, err) - } - - return nil -} - -func handleKeyRotation(conn *kms.KMS, shouldEnableRotation bool, keyId *string) error { - var err error - if shouldEnableRotation { - log.Printf("[DEBUG] Enabling key rotation for KMS key %q", *keyId) - _, err = conn.EnableKeyRotation(&kms.EnableKeyRotationInput{ - KeyId: keyId, - }) - } else { - log.Printf("[DEBUG] Disabling key rotation for KMS key %q", *keyId) - _, err = conn.DisableKeyRotation(&kms.DisableKeyRotationInput{ - KeyId: keyId, - }) + return aws.BoolValue(output.Enabled) == enabled, nil } - return err -} - -func resourceAwsKmsKeyDelete(d *schema.ResourceData, meta interface{}) error { - conn := meta.(*AWSClient).kmsconn - - input := &kms.ScheduleKeyDeletionInput{ - KeyId: aws.String(d.Id()), - } - - if v, ok := d.GetOk("deletion_window_in_days"); ok { - input.PendingWindowInDays = aws.Int64(int64(v.(int))) - } - - log.Printf("[DEBUG] Deleting KMS Key: (%s)", d.Id()) - _, err := conn.ScheduleKeyDeletion(input) - - if tfawserr.ErrCodeEquals(err, kms.ErrCodeNotFoundException) { - return nil + opts := &tfresource.WaitOpts{ + ContinuousTargetOccurence: 15, + MinTimeout: 2 * time.Second, } - if tfawserr.ErrMessageContains(err, kms.ErrCodeInvalidStateException, "is pending deletion") { - return nil - } + err = tfresource.WaitUntil(waiter.KeyStatePropagationTimeout, checkFunc, opts) if err != nil { - return fmt.Errorf("error deleting KMS Key (%s): %w", d.Id(), err) - } - - if _, err := waiter.KeyDeleted(conn, d.Id()); err != nil { - return fmt.Errorf("error waiting for KMS Key (%s) to delete: %w", d.Id(), err) + return fmt.Errorf("error waiting for KMS Key (%s) key state propagation: %w", keyID, err) } return nil @@ -492,6 +391,7 @@ func updateKmsKeyRotationEnabled(conn *kms.KMS, keyID string, enabled bool) erro updateFunc := func() (interface{}, error) { var err error + log.Printf("[DEBUG] Updating KMS Key (%s) key rotation enabled: %t", keyID, enabled) if enabled { _, err = conn.EnableKeyRotation(&kms.EnableKeyRotationInput{ KeyId: aws.String(keyID), @@ -513,7 +413,7 @@ func updateKmsKeyRotationEnabled(conn *kms.KMS, keyID string, enabled bool) erro // Wait for propagation since KMS is eventually consistent. checkFunc := func() (bool, error) { - got, err := finder.KeyRotationEnabledByKeyID(conn, keyID) + output, err := finder.KeyRotationEnabledByKeyID(conn, keyID) if tfresource.NotFound(err) { return false, nil @@ -523,7 +423,7 @@ func updateKmsKeyRotationEnabled(conn *kms.KMS, keyID string, enabled bool) erro return false, err } - return aws.BoolValue(got) == enabled, nil + return aws.BoolValue(output) == enabled, nil } opts := &tfresource.WaitOpts{ ContinuousTargetOccurence: 5, diff --git a/aws/resource_aws_kms_key_test.go b/aws/resource_aws_kms_key_test.go index 067650ca0ff..1d9edc4c2f2 100644 --- a/aws/resource_aws_kms_key_test.go +++ b/aws/resource_aws_kms_key_test.go @@ -12,6 +12,9 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" awspolicy "github.com/jen20/awspolicyequivalence" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/finder" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/waiter" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) func init() { @@ -423,19 +426,17 @@ func testAccCheckAWSKmsKeyDestroy(s *terraform.State) error { continue } - out, err := conn.DescribeKey(&kms.DescribeKeyInput{ - KeyId: aws.String(rs.Primary.ID), - }) + _, err := finder.KeyByID(conn, rs.Primary.ID) - if err != nil { - return err + if tfresource.NotFound(err) { + continue } - if *out.KeyMetadata.KeyState == "PendingDeletion" { - return nil + if err != nil { + return err } - return fmt.Errorf("KMS key still exists:\n%#v", out.KeyMetadata) + return fmt.Errorf("KMS Key %s still exists", rs.Primary.ID) } return nil @@ -454,17 +455,15 @@ func testAccCheckAWSKmsKeyExists(name string, key *kms.KeyMetadata) resource.Tes conn := testAccProvider.Meta().(*AWSClient).kmsconn - o, err := retryOnAwsCode("NotFoundException", func() (interface{}, error) { - return conn.DescribeKey(&kms.DescribeKeyInput{ - KeyId: aws.String(rs.Primary.ID), - }) + outputRaw, err := tfresource.RetryWhenNotFound(waiter.PropagationTimeout, func() (interface{}, error) { + return finder.KeyByID(conn, rs.Primary.ID) }) + if err != nil { return err } - out := o.(*kms.DescribeKeyOutput) - *key = *out.KeyMetadata + *key = *(outputRaw.(*kms.KeyMetadata)) return nil } From 121941b82001b9346993deeeeba2a9b4022662b2 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Thu, 22 Jul 2021 11:28:31 -0400 Subject: [PATCH 17/36] r/aws_key: Use standard random name in acceptance tests. --- aws/internal/tfresource/wait.go | 23 ++++++++++------------- aws/internal/tfresource/wait_test.go | 2 +- aws/resource_aws_kms_key.go | 4 ++-- aws/resource_aws_kms_key_test.go | 21 ++++++++++----------- 4 files changed, 23 insertions(+), 27 deletions(-) diff --git a/aws/internal/tfresource/wait.go b/aws/internal/tfresource/wait.go index 75470d7afc2..1a9ef7f89c4 100644 --- a/aws/internal/tfresource/wait.go +++ b/aws/internal/tfresource/wait.go @@ -24,7 +24,7 @@ const ( // If `f` returns an error, return immediately with that error. // If `timeout` is exceeded before `f` returns `true`, return an error. // Waits between calls to `f` using exponential backoff, except when waiting for the target state to reoccur. -func WaitUntilContext(ctx context.Context, timeout time.Duration, f func() (bool, error), opts *WaitOpts) error { +func WaitUntilContext(ctx context.Context, timeout time.Duration, f func() (bool, error), opts WaitOpts) error { refresh := func() (interface{}, string, error) { done, err := f() @@ -40,17 +40,14 @@ func WaitUntilContext(ctx context.Context, timeout time.Duration, f func() (bool } stateConf := &resource.StateChangeConf{ - Pending: []string{targetStateFalse}, - Target: []string{targetStateTrue}, - Refresh: refresh, - Timeout: timeout, - } - - if opts != nil { - stateConf.ContinuousTargetOccurence = opts.ContinuousTargetOccurence - stateConf.Delay = opts.Delay - stateConf.MinTimeout = opts.MinTimeout - stateConf.PollInterval = opts.PollInterval + Pending: []string{targetStateFalse}, + Target: []string{targetStateTrue}, + Refresh: refresh, + Timeout: timeout, + ContinuousTargetOccurence: opts.ContinuousTargetOccurence, + Delay: opts.Delay, + MinTimeout: opts.MinTimeout, + PollInterval: opts.PollInterval, } _, err := stateConf.WaitForStateContext(ctx) @@ -62,6 +59,6 @@ func WaitUntilContext(ctx context.Context, timeout time.Duration, f func() (bool // If `f` returns an error, return immediately with that error. // If `timeout` is exceeded before `f` returns `true`, return an error. // Waits between calls to `f` using exponential backoff, except when waiting for the target state to reoccur. -func WaitUntil(timeout time.Duration, f func() (bool, error), opts *WaitOpts) error { +func WaitUntil(timeout time.Duration, f func() (bool, error), opts WaitOpts) error { return WaitUntilContext(context.Background(), timeout, f, opts) } diff --git a/aws/internal/tfresource/wait_test.go b/aws/internal/tfresource/wait_test.go index 606d0fa1a1b..8062b0e14d5 100644 --- a/aws/internal/tfresource/wait_test.go +++ b/aws/internal/tfresource/wait_test.go @@ -53,7 +53,7 @@ func TestWaitUntil(t *testing.T) { t.Run(testCase.Name, func(t *testing.T) { retryCount = 0 - err := tfresource.WaitUntil(5*time.Second, testCase.F, nil) + err := tfresource.WaitUntil(5*time.Second, testCase.F, tfresource.WaitOpts{}) if testCase.ExpectError && err == nil { t.Fatal("expected error") diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index 2cf3bbfa13f..01cbda8ff71 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -373,7 +373,7 @@ func updateKmsKeyEnabled(conn *kms.KMS, keyID string, enabled bool) error { return aws.BoolValue(output.Enabled) == enabled, nil } - opts := &tfresource.WaitOpts{ + opts := tfresource.WaitOpts{ ContinuousTargetOccurence: 15, MinTimeout: 2 * time.Second, } @@ -425,7 +425,7 @@ func updateKmsKeyRotationEnabled(conn *kms.KMS, keyID string, enabled bool) erro return aws.BoolValue(output) == enabled, nil } - opts := &tfresource.WaitOpts{ + opts := tfresource.WaitOpts{ ContinuousTargetOccurence: 5, MinTimeout: 1 * time.Second, } diff --git a/aws/resource_aws_kms_key_test.go b/aws/resource_aws_kms_key_test.go index 1d9edc4c2f2..5569215e746 100644 --- a/aws/resource_aws_kms_key_test.go +++ b/aws/resource_aws_kms_key_test.go @@ -76,7 +76,7 @@ func testSweepKmsKeys(region string) error { func TestAccAWSKmsKey_basic(t *testing.T) { var key kms.KeyMetadata - rName := fmt.Sprintf("tf-testacc-kms-key-%s", acctest.RandString(13)) + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_key.test" resource.ParallelTest(t, resource.TestCase{ @@ -106,7 +106,7 @@ func TestAccAWSKmsKey_basic(t *testing.T) { func TestAccAWSKmsKey_asymmetricKey(t *testing.T) { var key kms.KeyMetadata - rName := fmt.Sprintf("tf-testacc-kms-key-%s", acctest.RandString(13)) + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_key.test" resource.ParallelTest(t, resource.TestCase{ @@ -129,7 +129,7 @@ func TestAccAWSKmsKey_asymmetricKey(t *testing.T) { func TestAccAWSKmsKey_disappears(t *testing.T) { var key kms.KeyMetadata - rName := fmt.Sprintf("tf-testacc-kms-key-%s", acctest.RandString(13)) + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_key.test" resource.ParallelTest(t, resource.TestCase{ @@ -152,7 +152,7 @@ func TestAccAWSKmsKey_disappears(t *testing.T) { func TestAccAWSKmsKey_policy(t *testing.T) { var key kms.KeyMetadata - rName := fmt.Sprintf("tf-testacc-kms-key-%s", acctest.RandString(13)) + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_key.test" expectedPolicyText := `{"Version":"2012-10-17","Id":"kms-tf-1","Statement":[{"Sid":"Enable IAM User Permissions","Effect":"Allow","Principal":{"AWS":"*"},"Action":"kms:*","Resource":"*"}]}` @@ -187,7 +187,7 @@ func TestAccAWSKmsKey_policy(t *testing.T) { func TestAccAWSKmsKey_policyBypass(t *testing.T) { var key kms.KeyMetadata - rName := fmt.Sprintf("tf-testacc-kms-key-%s", acctest.RandString(13)) + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_key.test" resource.ParallelTest(t, resource.TestCase{ @@ -218,7 +218,7 @@ func TestAccAWSKmsKey_policyBypass(t *testing.T) { func TestAccAWSKmsKey_policyBypassUpdate(t *testing.T) { var before, after kms.KeyMetadata - rName := fmt.Sprintf("tf-testacc-kms-key-%s", acctest.RandString(13)) + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_key.test" resource.ParallelTest(t, resource.TestCase{ @@ -301,7 +301,7 @@ func TestAccAWSKmsKey_Policy_IamServiceLinkedRole(t *testing.T) { func TestAccAWSKmsKey_isEnabled(t *testing.T) { var key1, key2, key3 kms.KeyMetadata - rName := fmt.Sprintf("tf-testacc-kms-key-%s", acctest.RandString(13)) + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_key.test" resource.ParallelTest(t, resource.TestCase{ @@ -349,7 +349,7 @@ func TestAccAWSKmsKey_isEnabled(t *testing.T) { func TestAccAWSKmsKey_tags(t *testing.T) { var key kms.KeyMetadata - rName := fmt.Sprintf("tf-testacc-kms-key-%s", acctest.RandString(13)) + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_key.test" resource.ParallelTest(t, resource.TestCase{ @@ -471,9 +471,8 @@ func testAccCheckAWSKmsKeyExists(name string, key *kms.KeyMetadata) resource.Tes func testAccCheckAWSKmsKeyIsEnabled(key *kms.KeyMetadata, isEnabled bool) resource.TestCheckFunc { return func(s *terraform.State) error { - if *key.Enabled != isEnabled { - return fmt.Errorf("Expected key %q to have is_enabled=%t, given %t", - *key.Arn, isEnabled, *key.Enabled) + if got, want := aws.BoolValue(key.Enabled), isEnabled; got != want { + return fmt.Errorf("Expected key %q to have is_enabled=%t, given %t", aws.StringValue(key.Arn), want, got) } return nil From 518bee6ec858e9b7b0fe8ca20a9452a0ef075808 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Thu, 22 Jul 2021 12:25:32 -0400 Subject: [PATCH 18/36] r/aws_kms_key: Wait for policy propagation. --- aws/resource_aws_kms_key.go | 121 ++++++++++++++++++------------------ 1 file changed, 60 insertions(+), 61 deletions(-) diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index 01cbda8ff71..6e2ab23ef13 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -3,7 +3,6 @@ package aws import ( "fmt" "log" - "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/kms" @@ -155,6 +154,13 @@ func resourceAwsKmsKeyCreate(d *schema.ResourceData, meta interface{}) error { } } + // Wait for propagation since KMS is eventually consistent. + if v, ok := d.GetOk("policy"); ok { + if err := waiter.KeyPolicyPropagated(conn, d.Id(), v.(string)); err != nil { + return fmt.Errorf("error waiting for KMS Key (%s) policy propagation: %w", d.Id(), err) + } + } + return resourceAwsKmsKeyRead(d, meta) } @@ -269,24 +275,8 @@ func resourceAwsKmsKeyUpdate(d *schema.ResourceData, meta interface{}) error { } if d.HasChange("policy") { - policy, err := structure.NormalizeJsonString(d.Get("policy").(string)) - - if err != nil { - return fmt.Errorf("policy contains invalid JSON: %w", err) - } - - input := &kms.PutKeyPolicyInput{ - BypassPolicyLockoutSafetyCheck: aws.Bool(d.Get("bypass_policy_lockout_check").(bool)), - KeyId: aws.String(d.Id()), - Policy: aws.String(policy), - PolicyName: aws.String(tfkms.PolicyNameDefault), - } - - log.Printf("[DEBUG] Updating KMS Key policy: %s", input) - _, err = conn.PutKeyPolicy(input) - - if err != nil { - return fmt.Errorf("error updating KMS Key (%s) policy: %w", d.Id(), err) + if err := updateKmsKeyPolicy(conn, d.Id(), d.Get("policy").(string), d.Get("bypass_policy_lockout_check").(bool)); err != nil { + return err } } @@ -342,46 +332,73 @@ func resourceAwsKmsKeyDelete(d *schema.ResourceData, meta interface{}) error { } func updateKmsKeyEnabled(conn *kms.KMS, keyID string, enabled bool) error { - var err error + updateFunc := func() (interface{}, error) { + var err error - log.Printf("[DEBUG] Updating KMS Key (%s) key enabled: %t", keyID, enabled) - if enabled { - _, err = conn.EnableKey(&kms.EnableKeyInput{ - KeyId: aws.String(keyID), - }) - } else { - _, err = conn.DisableKey(&kms.DisableKeyInput{ - KeyId: aws.String(keyID), - }) + log.Printf("[DEBUG] Updating KMS Key (%s) key enabled: %t", keyID, enabled) + if enabled { + _, err = conn.EnableKey(&kms.EnableKeyInput{ + KeyId: aws.String(keyID), + }) + } else { + _, err = conn.DisableKey(&kms.DisableKeyInput{ + KeyId: aws.String(keyID), + }) + } + + return nil, err } + _, err := tfresource.RetryWhenAwsErrCodeEquals(waiter.KeyRotationUpdatedTimeout, updateFunc, kms.ErrCodeNotFoundException) + if err != nil { return fmt.Errorf("error updating KMS Key (%s) key enabled (%t): %w", keyID, enabled, err) } // Wait for propagation since KMS is eventually consistent. - checkFunc := func() (bool, error) { - output, err := finder.KeyByID(conn, keyID) + err = waiter.KeyStatePropagated(conn, keyID, enabled) - if tfresource.NotFound(err) { - return false, nil - } + if err != nil { + return fmt.Errorf("error waiting for KMS Key (%s) key state propagation: %w", keyID, err) + } - if err != nil { - return false, err + return nil +} + +func updateKmsKeyPolicy(conn *kms.KMS, keyID string, policy string, bypassPolicyLockoutSafetyCheck bool) error { + policy, err := structure.NormalizeJsonString(policy) + + if err != nil { + return fmt.Errorf("policy contains invalid JSON: %w", err) + } + + updateFunc := func() (interface{}, error) { + var err error + + input := &kms.PutKeyPolicyInput{ + BypassPolicyLockoutSafetyCheck: aws.Bool(bypassPolicyLockoutSafetyCheck), + KeyId: aws.String(keyID), + Policy: aws.String(policy), + PolicyName: aws.String(tfkms.PolicyNameDefault), } - return aws.BoolValue(output.Enabled) == enabled, nil + log.Printf("[DEBUG] Updating KMS Key policy: %s", input) + _, err = conn.PutKeyPolicy(input) + + return nil, err } - opts := tfresource.WaitOpts{ - ContinuousTargetOccurence: 15, - MinTimeout: 2 * time.Second, + + _, err = tfresource.RetryWhenAwsErrCodeEquals(waiter.PropagationTimeout, updateFunc, kms.ErrCodeNotFoundException) + + if err != nil { + return fmt.Errorf("error updating KMS Key (%s) policy: %w", keyID, err) } - err = tfresource.WaitUntil(waiter.KeyStatePropagationTimeout, checkFunc, opts) + // Wait for propagation since KMS is eventually consistent. + err = waiter.KeyPolicyPropagated(conn, keyID, policy) if err != nil { - return fmt.Errorf("error waiting for KMS Key (%s) key state propagation: %w", keyID, err) + return fmt.Errorf("error waiting for KMS Key (%s) policy propagation: %w", keyID, err) } return nil @@ -412,25 +429,7 @@ func updateKmsKeyRotationEnabled(conn *kms.KMS, keyID string, enabled bool) erro } // Wait for propagation since KMS is eventually consistent. - checkFunc := func() (bool, error) { - output, err := finder.KeyRotationEnabledByKeyID(conn, keyID) - - if tfresource.NotFound(err) { - return false, nil - } - - if err != nil { - return false, err - } - - return aws.BoolValue(output) == enabled, nil - } - opts := tfresource.WaitOpts{ - ContinuousTargetOccurence: 5, - MinTimeout: 1 * time.Second, - } - - err = tfresource.WaitUntil(waiter.PropagationTimeout, checkFunc, opts) + err = waiter.KeyRotationEnabledPropagated(conn, keyID, enabled) if err != nil { return fmt.Errorf("error waiting for KMS Key (%s) key rotation propagation: %w", keyID, err) From b4a24fbfd008ff8491f31dcca9b86e87d1129ae2 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Thu, 22 Jul 2021 12:42:33 -0400 Subject: [PATCH 19/36] 'bypass_policy_lockout_check' -> 'bypass_policy_lockout_safety_check'. --- .changelog/18117.txt | 2 +- aws/internal/service/kms/waiter/waiter.go | 78 +++++++++++++++++++++++ aws/resource_aws_kms_key.go | 6 +- aws/resource_aws_kms_key_test.go | 77 ++++++++++++---------- website/docs/r/kms_key.html.markdown | 2 +- 5 files changed, 125 insertions(+), 40 deletions(-) diff --git a/.changelog/18117.txt b/.changelog/18117.txt index d229cf0ba97..f67d67f234a 100644 --- a/.changelog/18117.txt +++ b/.changelog/18117.txt @@ -1,3 +1,3 @@ ```release-note:enhancement -resource/aws_kms_key: Add `bypass_policy_lockout_check` argument +resource/aws_kms_key: Add `bypass_policy_lockout_safety_check` argument ``` diff --git a/aws/internal/service/kms/waiter/waiter.go b/aws/internal/service/kms/waiter/waiter.go index eac97494d4b..7146638f556 100644 --- a/aws/internal/service/kms/waiter/waiter.go +++ b/aws/internal/service/kms/waiter/waiter.go @@ -3,9 +3,13 @@ package waiter import ( "time" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/kms" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" + awspolicy "github.com/jen20/awspolicyequivalence" iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter" + tfkms "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/finder" "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) @@ -43,6 +47,80 @@ func KeyDeleted(conn *kms.KMS, id string) (*kms.KeyMetadata, error) { return nil, err } +func KeyPolicyPropagated(conn *kms.KMS, id, policy string) error { + checkFunc := func() (bool, error) { + output, err := finder.KeyPolicyByKeyIDAndPolicyName(conn, id, tfkms.PolicyNameDefault) + + if tfresource.NotFound(err) { + return false, nil + } + + if err != nil { + return false, err + } + + equivalent, err := awspolicy.PoliciesAreEquivalent(aws.StringValue(output), policy) + + if err != nil { + return false, err + } + + return equivalent, nil + } + opts := tfresource.WaitOpts{ + ContinuousTargetOccurence: 5, + MinTimeout: 1 * time.Second, + } + + return tfresource.WaitUntil(PropagationTimeout, checkFunc, opts) +} + +func KeyRotationEnabledPropagated(conn *kms.KMS, id string, enabled bool) error { + checkFunc := func() (bool, error) { + output, err := finder.KeyRotationEnabledByKeyID(conn, id) + + if tfresource.NotFound(err) { + return false, nil + } + + if err != nil { + return false, err + } + + return aws.BoolValue(output) == enabled, nil + } + opts := tfresource.WaitOpts{ + ContinuousTargetOccurence: 5, + MinTimeout: 1 * time.Second, + } + + return tfresource.WaitUntil(PropagationTimeout, checkFunc, opts) +} + +func KeyStatePropagated(conn *kms.KMS, id string, enabled bool) error { + checkFunc := func() (bool, error) { + output, err := finder.KeyByID(conn, id) + + if tfresource.NotFound(err) { + return false, nil + } + + if err != nil { + return false, err + } + + return aws.BoolValue(output.Enabled) == enabled, nil + } + opts := tfresource.WaitOpts{ + ContinuousTargetOccurence: 15, + MinTimeout: 2 * time.Second, + } + + return tfresource.WaitUntil(KeyStatePropagationTimeout, checkFunc, opts) +} + +// TODO: Remove + // KeyStatePendingDeletion waits for KeyState to return PendingDeletion func KeyStatePendingDeletion(conn *kms.KMS, keyID string) (*kms.DescribeKeyOutput, error) { stateConf := &resource.StateChangeConf{ diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index 6e2ab23ef13..432231670b5 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -36,7 +36,7 @@ func resourceAwsKmsKey() *schema.Resource { Computed: true, }, - "bypass_policy_lockout_check": { + "bypass_policy_lockout_safety_check": { Type: schema.TypeBool, Optional: true, Default: false, @@ -108,7 +108,7 @@ func resourceAwsKmsKeyCreate(d *schema.ResourceData, meta interface{}) error { tags := defaultTagsConfig.MergeTags(keyvaluetags.New(d.Get("tags").(map[string]interface{}))) input := &kms.CreateKeyInput{ - BypassPolicyLockoutSafetyCheck: aws.Bool(d.Get("bypass_policy_lockout_check").(bool)), + BypassPolicyLockoutSafetyCheck: aws.Bool(d.Get("bypass_policy_lockout_safety_check").(bool)), CustomerMasterKeySpec: aws.String(d.Get("customer_master_key_spec").(string)), KeyUsage: aws.String(d.Get("key_usage").(string)), } @@ -275,7 +275,7 @@ func resourceAwsKmsKeyUpdate(d *schema.ResourceData, meta interface{}) error { } if d.HasChange("policy") { - if err := updateKmsKeyPolicy(conn, d.Id(), d.Get("policy").(string), d.Get("bypass_policy_lockout_check").(bool)); err != nil { + if err := updateKmsKeyPolicy(conn, d.Id(), d.Get("policy").(string), d.Get("bypass_policy_lockout_safety_check").(bool)); err != nil { return err } } diff --git a/aws/resource_aws_kms_key_test.go b/aws/resource_aws_kms_key_test.go index 5569215e746..005523ddc00 100644 --- a/aws/resource_aws_kms_key_test.go +++ b/aws/resource_aws_kms_key_test.go @@ -98,7 +98,7 @@ func TestAccAWSKmsKey_basic(t *testing.T) { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_check"}, + ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_safety_check"}, }, }, }) @@ -173,7 +173,7 @@ func TestAccAWSKmsKey_policy(t *testing.T) { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_check"}, + ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_safety_check"}, }, { Config: testAccAWSKmsKey_removedPolicy(rName), @@ -203,14 +203,14 @@ func TestAccAWSKmsKey_policyBypass(t *testing.T) { Config: testAccAWSKmsKey_policyBypass(rName, true), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsKeyExists(resourceName, &key), - resource.TestCheckResourceAttr(resourceName, "bypass_policy_lockout_check", "true"), + resource.TestCheckResourceAttr(resourceName, "bypass_policy_lockout_safety_check", "true"), ), }, { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_check"}, + ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_safety_check"}, }, }, }) @@ -230,14 +230,14 @@ func TestAccAWSKmsKey_policyBypassUpdate(t *testing.T) { Config: testAccAWSKmsKey(rName), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsKeyExists(resourceName, &before), - resource.TestCheckResourceAttr(resourceName, "bypass_policy_lockout_check", "false"), + resource.TestCheckResourceAttr(resourceName, "bypass_policy_lockout_safety_check", "false"), ), }, { Config: testAccAWSKmsKey_policyBypass(rName, true), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsKeyExists(resourceName, &after), - resource.TestCheckResourceAttr(resourceName, "bypass_policy_lockout_check", "true"), + resource.TestCheckResourceAttr(resourceName, "bypass_policy_lockout_safety_check", "true"), ), }, }, @@ -265,7 +265,7 @@ func TestAccAWSKmsKey_Policy_IamRole(t *testing.T) { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_check"}, + ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_safety_check"}, }, }, }) @@ -293,7 +293,7 @@ func TestAccAWSKmsKey_Policy_IamServiceLinkedRole(t *testing.T) { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_check"}, + ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_safety_check"}, }, }, }) @@ -323,7 +323,7 @@ func TestAccAWSKmsKey_isEnabled(t *testing.T) { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_check"}, + ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_safety_check"}, }, { Config: testAccAWSKmsKey_disabled(rName), @@ -359,23 +359,34 @@ func TestAccAWSKmsKey_tags(t *testing.T) { CheckDestroy: testAccCheckAWSKmsKeyDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSKmsKey_tags(rName), + Config: testAccAWSKmsKeyConfigTags1(rName, "key1", "value1"), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsKeyExists(resourceName, &key), - resource.TestCheckResourceAttr(resourceName, "tags.%", "3"), + resource.TestCheckResourceAttr(resourceName, "tags.%", "1"), + resource.TestCheckResourceAttr(resourceName, "tags.key1", "value1"), ), }, { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_check"}, + ImportStateVerifyIgnore: []string{"deletion_window_in_days", "bypass_policy_lockout_safety_check"}, }, { - Config: testAccAWSKmsKey(rName), + Config: testAccAWSKmsKeyConfigTags2(rName, "key1", "value1updated", "key2", "value2"), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsKeyExists(resourceName, &key), - resource.TestCheckResourceAttr(resourceName, "tags.%", "0"), + resource.TestCheckResourceAttr(resourceName, "tags.%", "2"), + resource.TestCheckResourceAttr(resourceName, "tags.key1", "value1updated"), + resource.TestCheckResourceAttr(resourceName, "tags.key2", "value2"), + ), + }, + { + Config: testAccAWSKmsKeyConfigTags1(rName, "key2", "value2"), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSKmsKeyExists(resourceName, &key), + resource.TestCheckResourceAttr(resourceName, "tags.%", "1"), + resource.TestCheckResourceAttr(resourceName, "tags.key2", "value2"), ), }, }, @@ -535,8 +546,9 @@ resource "aws_kms_key" "test" { description = %[1]q deletion_window_in_days = 7 - bypass_policy_lockout_check = %t - policy = <<-POLICY + bypass_policy_lockout_safety_check = %[2]t + + policy = <<-POLICY { "Version": "2012-10-17", "Id": "kms-tf-1", @@ -679,10 +691,6 @@ func testAccAWSKmsKey_removedPolicy(rName string) string { resource "aws_kms_key" "test" { description = %[1]q deletion_window_in_days = 7 - - tags = { - Name = %[1]q - } } `, rName) } @@ -693,10 +701,6 @@ resource "aws_kms_key" "test" { description = %[1]q deletion_window_in_days = 7 enable_key_rotation = true - - tags = { - Name = %[1]q - } } `, rName) } @@ -708,10 +712,6 @@ resource "aws_kms_key" "test" { deletion_window_in_days = 7 enable_key_rotation = false is_enabled = false - - tags = { - Name = %[1]q - } } `, rName) } @@ -723,24 +723,31 @@ resource "aws_kms_key" "test" { deletion_window_in_days = 7 enable_key_rotation = true is_enabled = true +} +`, rName) +} + +func testAccAWSKmsKeyConfigTags1(rName, tagKey1, tagValue1 string) string { + return fmt.Sprintf(` +resource "aws_kms_key" "test" { + description = %[1]q tags = { - Name = %[1]q + %[2]q = %[3]q } } -`, rName) +`, rName, tagKey1, tagValue1) } -func testAccAWSKmsKey_tags(rName string) string { +func testAccAWSKmsKeyConfigTags2(rName, tagKey1, tagValue1, tagKey2, tagValue2 string) string { return fmt.Sprintf(` resource "aws_kms_key" "test" { description = %[1]q tags = { - Name = %[1]q - Key1 = "Value One" - Description = "Very interesting" + %[2]q = %[3]q + %[4]q = %[5]q } } -`, rName) +`, rName, tagKey1, tagValue1, tagKey2, tagValue2) } diff --git a/website/docs/r/kms_key.html.markdown b/website/docs/r/kms_key.html.markdown index de2eedd7f53..a167ca6c57d 100644 --- a/website/docs/r/kms_key.html.markdown +++ b/website/docs/r/kms_key.html.markdown @@ -32,7 +32,7 @@ Valid values: `SYMMETRIC_DEFAULT`, `RSA_2048`, `RSA_3072`, `RSA_4096`, `ECC_NIS ~> **NOTE:** Note: All KMS keys must have a key policy. If a key policy is not specified, AWS gives the KMS key a [default key policy](https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html#key-policy-default) that gives all principals in the owning account unlimited access to all KMS operations for the key. This default key policy effectively delegates all access control to IAM policies and KMS grants. -* `bypass_policy_lockout_check` - (Optional) Specifies whether to disable the policy lockout check performed when creating or updating the key's policy. Setting this value to `true` increases the risk that the CMK becomes unmanageable. For more information, refer to the scenario in the [Default Key Policy](https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html#key-policy-default-allow-root-enable-iam) section in the AWS Key Management Service Developer Guide. Defaults to `false`. +* `bypass_policy_lockout_safety_check` - (Optional) Specifies whether to disable the policy lockout check performed when creating or updating the key's policy. Setting this value to `true` increases the risk that the CMK becomes unmanageable. For more information, refer to the scenario in the [Default Key Policy](https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html#key-policy-default-allow-root-enable-iam) section in the AWS Key Management Service Developer Guide. Defaults to `false`. * `deletion_window_in_days` - (Optional) Duration in days after which the key is deleted after destruction of the resource, must be between 7 and 30 days. Defaults to 30 days. * `is_enabled` - (Optional) Specifies whether the key is enabled. Defaults to true. * `enable_key_rotation` - (Optional) Specifies whether [key rotation](http://docs.aws.amazon.com/kms/latest/developerguide/rotate-keys.html) is enabled. Defaults to false. From 668db80bc261ae7ace5cf03c29fc38b1d233358c Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Thu, 22 Jul 2021 14:28:00 -0400 Subject: [PATCH 20/36] Add 'KeyValueTags.Equal'. --- aws/internal/keyvaluetags/key_value_tags.go | 28 ++++ .../keyvaluetags/key_value_tags_test.go | 126 ++++++++++++++++++ 2 files changed, 154 insertions(+) diff --git a/aws/internal/keyvaluetags/key_value_tags.go b/aws/internal/keyvaluetags/key_value_tags.go index e22c61cac08..31d4834c54f 100644 --- a/aws/internal/keyvaluetags/key_value_tags.go +++ b/aws/internal/keyvaluetags/key_value_tags.go @@ -428,6 +428,34 @@ func (tags KeyValueTags) ContainsAll(target KeyValueTags) bool { return true } +// Equal returns whether or two sets of key-value tags are equal. +func (tags KeyValueTags) Equal(other KeyValueTags) bool { + if tags == nil && other == nil { + return true + } + + if tags == nil || other == nil { + return false + } + + if len(tags) != len(other) { + return false + } + + for k, v := range tags { + o, ok := other[k] + if !ok { + return false + } + + if !v.Equal(o) { + return false + } + } + + return true +} + // Hash returns a stable hash value. // The returned value may be negative (i.e. not suitable for a 'Set' function). func (tags KeyValueTags) Hash() int { diff --git a/aws/internal/keyvaluetags/key_value_tags_test.go b/aws/internal/keyvaluetags/key_value_tags_test.go index ef432d1afbc..1223ff6aa26 100644 --- a/aws/internal/keyvaluetags/key_value_tags_test.go +++ b/aws/internal/keyvaluetags/key_value_tags_test.go @@ -1824,6 +1824,132 @@ func TestKeyValueTagsContainsAll(t *testing.T) { } } +func TestKeyValueTagsEqual(t *testing.T) { + testCases := []struct { + name string + source KeyValueTags + target KeyValueTags + want bool + }{ + { + name: "nil", + source: nil, + target: nil, + want: true, + }, + { + name: "empty", + source: New(map[string]string{}), + target: New(map[string]string{}), + want: true, + }, + { + name: "source_nil", + source: nil, + target: New(map[string]string{ + "key1": "value1", + }), + want: false, + }, + { + name: "source_empty", + source: New(map[string]string{}), + target: New(map[string]string{ + "key1": "value1", + }), + want: false, + }, + { + name: "target_nil", + source: New(map[string]string{ + "key1": "value1", + }), + target: nil, + want: false, + }, + { + name: "target_empty", + source: New(map[string]string{ + "key1": "value1", + }), + target: New(map[string]string{}), + want: false, + }, + { + name: "nil value matches", + source: New(map[string]*string{ + "key1": nil, + }), + target: New(map[string]*string{ + "key1": nil, + }), + want: true, + }, + { + name: "exact_match", + source: New(map[string]string{ + "key1": "value1", + "key2": "value2", + }), + target: New(map[string]string{ + "key1": "value1", + "key2": "value2", + }), + want: true, + }, + { + name: "source_contains_all", + source: New(map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + }), + target: New(map[string]string{ + "key1": "value1", + "key3": "value3", + }), + want: false, + }, + { + name: "source_does_not_contain_all", + source: New(map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + }), + target: New(map[string]string{ + "key1": "value1", + "key4": "value4", + }), + want: false, + }, + { + name: "target_value_neq", + source: New(map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + }), + target: New(map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value4", + }), + want: false, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + got := testCase.source.Equal(testCase.target) + + if got != testCase.want { + t.Errorf("unexpected ContainsAll: %t", got) + } + }) + } +} + func TestKeyValueTagsHash(t *testing.T) { testCases := []struct { name string From 2452fd9634698f7c2d07824b987fa1fa20e12e90 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Thu, 22 Jul 2021 14:33:54 -0400 Subject: [PATCH 21/36] r/aws_kms_key: Wait for key-value tags propagation. Acceptance test output: % make testacc TESTARGS='-run=TestAccAWSKmsKey_' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSKmsKey_ -timeout 180m === RUN TestAccAWSKmsKey_basic === PAUSE TestAccAWSKmsKey_basic === RUN TestAccAWSKmsKey_asymmetricKey === PAUSE TestAccAWSKmsKey_asymmetricKey === RUN TestAccAWSKmsKey_disappears === PAUSE TestAccAWSKmsKey_disappears === RUN TestAccAWSKmsKey_policy === PAUSE TestAccAWSKmsKey_policy === RUN TestAccAWSKmsKey_policyBypass === PAUSE TestAccAWSKmsKey_policyBypass === RUN TestAccAWSKmsKey_policyBypassUpdate === PAUSE TestAccAWSKmsKey_policyBypassUpdate === RUN TestAccAWSKmsKey_Policy_IamRole === PAUSE TestAccAWSKmsKey_Policy_IamRole === RUN TestAccAWSKmsKey_Policy_IamServiceLinkedRole === PAUSE TestAccAWSKmsKey_Policy_IamServiceLinkedRole === RUN TestAccAWSKmsKey_isEnabled === PAUSE TestAccAWSKmsKey_isEnabled === RUN TestAccAWSKmsKey_tags === PAUSE TestAccAWSKmsKey_tags === CONT TestAccAWSKmsKey_basic === CONT TestAccAWSKmsKey_Policy_IamRole === CONT TestAccAWSKmsKey_policy === CONT TestAccAWSKmsKey_disappears === CONT TestAccAWSKmsKey_isEnabled === CONT TestAccAWSKmsKey_asymmetricKey === CONT TestAccAWSKmsKey_Policy_IamServiceLinkedRole === CONT TestAccAWSKmsKey_policyBypassUpdate === CONT TestAccAWSKmsKey_tags === CONT TestAccAWSKmsKey_policyBypass --- PASS: TestAccAWSKmsKey_disappears (17.91s) --- PASS: TestAccAWSKmsKey_asymmetricKey (20.53s) --- PASS: TestAccAWSKmsKey_basic (24.68s) --- PASS: TestAccAWSKmsKey_policyBypassUpdate (39.56s) --- PASS: TestAccAWSKmsKey_Policy_IamRole (40.32s) --- PASS: TestAccAWSKmsKey_policy (43.14s) --- PASS: TestAccAWSKmsKey_Policy_IamServiceLinkedRole (48.18s) --- PASS: TestAccAWSKmsKey_tags (78.25s) --- PASS: TestAccAWSKmsKey_policyBypass (148.52s) --- PASS: TestAccAWSKmsKey_isEnabled (267.43s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 274.824s --- .../keyvaluetags/key_value_tags_test.go | 2 +- aws/internal/service/kms/waiter/waiter.go | 26 ++++++++++++++++++- aws/resource_aws_kms_key.go | 15 +++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-) diff --git a/aws/internal/keyvaluetags/key_value_tags_test.go b/aws/internal/keyvaluetags/key_value_tags_test.go index 1223ff6aa26..9e3a4b20e4c 100644 --- a/aws/internal/keyvaluetags/key_value_tags_test.go +++ b/aws/internal/keyvaluetags/key_value_tags_test.go @@ -1944,7 +1944,7 @@ func TestKeyValueTagsEqual(t *testing.T) { got := testCase.source.Equal(testCase.target) if got != testCase.want { - t.Errorf("unexpected ContainsAll: %t", got) + t.Errorf("unexpected Equal: %t", got) } }) } diff --git a/aws/internal/service/kms/waiter/waiter.go b/aws/internal/service/kms/waiter/waiter.go index 7146638f556..cb571d760f4 100644 --- a/aws/internal/service/kms/waiter/waiter.go +++ b/aws/internal/service/kms/waiter/waiter.go @@ -5,8 +5,10 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/kms" + "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" awspolicy "github.com/jen20/awspolicyequivalence" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags" iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter" tfkms "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms" "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/finder" @@ -68,7 +70,7 @@ func KeyPolicyPropagated(conn *kms.KMS, id, policy string) error { return equivalent, nil } opts := tfresource.WaitOpts{ - ContinuousTargetOccurence: 5, + ContinuousTargetOccurence: 3, MinTimeout: 1 * time.Second, } @@ -119,6 +121,28 @@ func KeyStatePropagated(conn *kms.KMS, id string, enabled bool) error { return tfresource.WaitUntil(KeyStatePropagationTimeout, checkFunc, opts) } +func TagsPropagated(conn *kms.KMS, id string, tags keyvaluetags.KeyValueTags) error { + checkFunc := func() (bool, error) { + output, err := keyvaluetags.KmsListTags(conn, id) + + if tfawserr.ErrCodeEquals(err, kms.ErrCodeNotFoundException) { + return false, nil + } + + if err != nil { + return false, err + } + + return output.Equal(tags), nil + } + opts := tfresource.WaitOpts{ + ContinuousTargetOccurence: 3, + MinTimeout: 1 * time.Second, + } + + return tfresource.WaitUntil(PropagationTimeout, checkFunc, opts) +} + // TODO: Remove // KeyStatePendingDeletion waits for KeyState to return PendingDeletion diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index 432231670b5..a86fc6ed05d 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -7,6 +7,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/kms" "github.com/hashicorp/aws-sdk-go-base/tfawserr" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" @@ -161,6 +162,12 @@ func resourceAwsKmsKeyCreate(d *schema.ResourceData, meta interface{}) error { } } + if len(tags) > 0 { + if err := waiter.TagsPropagated(conn, d.Id(), tags); err != nil { + return fmt.Errorf("error waiting for KMS Key (%s) tag propagation: %w", d.Id(), err) + } + } + return resourceAwsKmsKeyRead(d, meta) } @@ -206,6 +213,10 @@ func resourceAwsKmsKeyRead(d *schema.ResourceData, meta interface{}) error { key.tags, err = keyvaluetags.KmsListTags(conn, d.Id()) + if tfawserr.ErrCodeEquals(err, kms.ErrCodeNotFoundException) { + return nil, &resource.NotFoundError{LastError: err} + } + if err != nil { return nil, fmt.Errorf("error listing tags for KMS Key (%s): %w", d.Id(), err) } @@ -293,6 +304,10 @@ func resourceAwsKmsKeyUpdate(d *schema.ResourceData, meta interface{}) error { if err := keyvaluetags.KmsUpdateTags(conn, d.Id(), o, n); err != nil { return fmt.Errorf("error updating KMS Key (%s) tags: %w", d.Id(), err) } + + if err := waiter.TagsPropagated(conn, d.Id(), keyvaluetags.New(n)); err != nil { + return fmt.Errorf("error waiting for KMS Key (%s) tag propagation: %w", d.Id(), err) + } } return resourceAwsKmsKeyRead(d, meta) From 95bcab6680e9481897f69bf88019c455af1b433b Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Thu, 22 Jul 2021 16:48:29 -0400 Subject: [PATCH 22/36] r/aws_kms_alias: Add and use 'finder.AliasByName'. --- aws/internal/service/kms/finder/finder.go | 31 +++++++++ aws/resource_aws_kms_alias.go | 76 ++++++++++++----------- 2 files changed, 72 insertions(+), 35 deletions(-) diff --git a/aws/internal/service/kms/finder/finder.go b/aws/internal/service/kms/finder/finder.go index b65c6389a61..a07d0459403 100644 --- a/aws/internal/service/kms/finder/finder.go +++ b/aws/internal/service/kms/finder/finder.go @@ -7,6 +7,37 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" ) +func AliasByName(conn *kms.KMS, name string) (*kms.AliasListEntry, error) { + input := &kms.ListAliasesInput{} + var output *kms.AliasListEntry + + err := conn.ListAliasesPages(input, func(page *kms.ListAliasesOutput, lastPage bool) bool { + if page == nil { + return !lastPage + } + + for _, alias := range page.Aliases { + if aws.StringValue(alias.AliasName) == name { + output = alias + + return false + } + } + + return !lastPage + }) + + if err != nil { + return nil, err + } + + if output == nil { + return nil, &resource.NotFoundError{} + } + + return output, nil +} + func KeyByID(conn *kms.KMS, id string) (*kms.KeyMetadata, error) { input := &kms.DescribeKeyInput{ KeyId: aws.String(id), diff --git a/aws/resource_aws_kms_alias.go b/aws/resource_aws_kms_alias.go index a47216d81b3..d9fcecde948 100644 --- a/aws/resource_aws_kms_alias.go +++ b/aws/resource_aws_kms_alias.go @@ -11,6 +11,10 @@ import ( "github.com/aws/aws-sdk-go/service/kms" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/naming" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/finder" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/waiter" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) func resourceAwsKmsAlias() *schema.Resource { @@ -29,6 +33,7 @@ func resourceAwsKmsAlias() *schema.Resource { Type: schema.TypeString, Computed: true, }, + "name": { Type: schema.TypeString, Optional: true, @@ -36,6 +41,7 @@ func resourceAwsKmsAlias() *schema.Resource { ConflictsWith: []string{"name_prefix"}, ValidateFunc: validateAwsKmsName, }, + "name_prefix": { Type: schema.TypeString, Optional: true, @@ -43,73 +49,73 @@ func resourceAwsKmsAlias() *schema.Resource { ConflictsWith: []string{"name"}, ValidateFunc: validateAwsKmsName, }, + + "target_key_arn": { + Type: schema.TypeString, + Computed: true, + }, + "target_key_id": { Type: schema.TypeString, Required: true, DiffSuppressFunc: suppressEquivalentTargetKeyIdAndARN, }, - "target_key_arn": { - Type: schema.TypeString, - Computed: true, - }, }, } } +const ( + kmsAliasDefaultNamePrefix = "alias/" +) + func resourceAwsKmsAliasCreate(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).kmsconn - var name string - if v, ok := d.GetOk("name"); ok { - name = v.(string) - } else if v, ok := d.GetOk("name_prefix"); ok { - name = resource.PrefixedUniqueId(v.(string)) - } else { - name = resource.PrefixedUniqueId("alias/") + namePrefix := d.Get("name_prefix").(string) + if namePrefix == "" { + namePrefix = kmsAliasDefaultNamePrefix } + name := naming.Generate(d.Get("name").(string), namePrefix) - targetKeyId := d.Get("target_key_id").(string) - - log.Printf("[DEBUG] KMS alias create name: %s, target_key: %s", name, targetKeyId) - - req := &kms.CreateAliasInput{ + input := &kms.CreateAliasInput{ AliasName: aws.String(name), - TargetKeyId: aws.String(targetKeyId), + TargetKeyId: aws.String(d.Get("target_key_id").(string)), } - // KMS is eventually consistent - _, err := retryOnAwsCode("NotFoundException", func() (interface{}, error) { - return conn.CreateAlias(req) - }) + // KMS is eventually consistent. + log.Printf("[DEBUG] Creating KMS Alias: %s", input) + + _, err := tfresource.RetryWhenAwsErrCodeEquals(waiter.KeyRotationUpdatedTimeout, func() (interface{}, error) { + return conn.CreateAlias(input) + }, kms.ErrCodeNotFoundException) + if err != nil { - return err + return fmt.Errorf("error creating KMS Alias (%s): %w", name, err) } + d.SetId(name) + return resourceAwsKmsAliasRead(d, meta) } func resourceAwsKmsAliasRead(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).kmsconn - var alias *kms.AliasListEntry - var err error - if d.IsNewResource() { - alias, err = retryFindKmsAliasByName(conn, d.Id()) - } else { - alias, err = findKmsAliasByName(conn, d.Id(), nil) - } - if err != nil { - return err - } - if alias == nil { - log.Printf("[DEBUG] Removing KMS Alias (%s) as it's already gone", d.Id()) + outputRaw, err := tfresource.RetryWhenNewResourceNotFound(waiter.PropagationTimeout, func() (interface{}, error) { + return finder.AliasByName(conn, d.Id()) + }, d.IsNewResource()) + + if !d.IsNewResource() && tfresource.NotFound(err) { + log.Printf("[WARN] KMS Alias (%s) not found, removing from state", d.Id()) d.SetId("") return nil } - log.Printf("[DEBUG] Found KMS Alias: %s", alias) + alias := outputRaw.(*kms.AliasListEntry) d.Set("arn", alias.AliasArn) + d.Set("name", alias.AliasName) + d.Set("name_prefix", naming.NamePrefixFromName(aws.StringValue(alias.AliasName))) d.Set("target_key_id", alias.TargetKeyId) aliasARN, err := arn.Parse(*alias.AliasArn) From 60b0404bbb7196dba5437c78fefb6196ccb27a67 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Thu, 22 Jul 2021 17:42:24 -0400 Subject: [PATCH 23/36] r/aws_kms_alias: Add 'AliasARNToKeyARN'. --- aws/internal/service/kms/arn.go | 36 ++++++++++++++ aws/internal/service/kms/arn_test.go | 60 +++++++++++++++++++++++ aws/resource_aws_kms_alias.go | 72 ++++++++++------------------ 3 files changed, 120 insertions(+), 48 deletions(-) create mode 100644 aws/internal/service/kms/arn.go create mode 100644 aws/internal/service/kms/arn_test.go diff --git a/aws/internal/service/kms/arn.go b/aws/internal/service/kms/arn.go new file mode 100644 index 00000000000..b12e9262be1 --- /dev/null +++ b/aws/internal/service/kms/arn.go @@ -0,0 +1,36 @@ +package kms + +import ( + "fmt" + "strings" + + "github.com/aws/aws-sdk-go/aws/arn" +) + +const ( + ARNSeparator = "/" + ARNService = "kms" +) + +// AliasARNToKeyARN converts an alias ARN to a CMK ARN. +func AliasARNToKeyARN(inputARN, keyID string) (string, error) { + parsedARN, err := arn.Parse(inputARN) + + if err != nil { + return "", fmt.Errorf("error parsing ARN (%s): %w", inputARN, err) + } + + if actual, expected := parsedARN.Service, ARNService; actual != expected { + return "", fmt.Errorf("expected service %s in ARN (%s), got: %s", expected, inputARN, actual) + } + + outputARN := arn.ARN{ + Partition: parsedARN.Partition, + Service: parsedARN.Service, + Region: parsedARN.Region, + AccountID: parsedARN.AccountID, + Resource: strings.Join([]string{"key", keyID}, ARNSeparator), + }.String() + + return outputARN, nil +} diff --git a/aws/internal/service/kms/arn_test.go b/aws/internal/service/kms/arn_test.go new file mode 100644 index 00000000000..2c137dcaa74 --- /dev/null +++ b/aws/internal/service/kms/arn_test.go @@ -0,0 +1,60 @@ +package kms_test + +import ( + "regexp" + "testing" + + tfkms "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms" +) + +func TestAliasARNToKeyARN(t *testing.T) { + testCases := []struct { + TestName string + InputARN string + ExpectedError *regexp.Regexp + ExpectedARN string + }{ + { + TestName: "empty ARN", + InputARN: "", + ExpectedError: regexp.MustCompile(`error parsing ARN`), + }, + { + TestName: "unparsable ARN", + InputARN: "test", + ExpectedError: regexp.MustCompile(`error parsing ARN`), + }, + { + TestName: "invalid ARN service", + InputARN: "arn:aws:ec2:us-west-2:123456789012:alias/test-alias", + ExpectedError: regexp.MustCompile(`expected service kms`), + }, + { + TestName: "valid ARN", + InputARN: "arn:aws:kms:us-west-2:123456789012:alias/test-alias", + ExpectedARN: "arn:aws:kms:us-west-2:123456789012:key/test-key", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.TestName, func(t *testing.T) { + got, err := tfkms.AliasARNToKeyARN(testCase.InputARN, "test-key") + + if err == nil && testCase.ExpectedError != nil { + t.Fatalf("expected error %s, got no error", testCase.ExpectedError.String()) + } + + if err != nil && testCase.ExpectedError == nil { + t.Fatalf("got unexpected error: %s", err) + } + + if err != nil && !testCase.ExpectedError.MatchString(err.Error()) { + t.Fatalf("expected error %s, got: %s", testCase.ExpectedError.String(), err) + } + + if got != testCase.ExpectedARN { + t.Errorf("got %s, expected %s", got, testCase.ExpectedARN) + } + }) + } +} diff --git a/aws/resource_aws_kms_alias.go b/aws/resource_aws_kms_alias.go index d9fcecde948..74f5eced914 100644 --- a/aws/resource_aws_kms_alias.go +++ b/aws/resource_aws_kms_alias.go @@ -4,14 +4,14 @@ import ( "fmt" "log" "strings" - "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/arn" "github.com/aws/aws-sdk-go/service/kms" - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" + "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/terraform-providers/terraform-provider-aws/aws/internal/naming" + tfkms "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms" "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/finder" "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/waiter" "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" @@ -25,7 +25,7 @@ func resourceAwsKmsAlias() *schema.Resource { Delete: resourceAwsKmsAliasDelete, Importer: &schema.ResourceImporter{ - State: resourceAwsKmsAliasImport, + State: schema.ImportStatePassthrough, }, Schema: map[string]*schema.Schema{ @@ -111,25 +111,24 @@ func resourceAwsKmsAliasRead(d *schema.ResourceData, meta interface{}) error { return nil } - alias := outputRaw.(*kms.AliasListEntry) + if err != nil { + return fmt.Errorf("error reading KMS Alias (%s): %w", d.Id(), err) + } - d.Set("arn", alias.AliasArn) - d.Set("name", alias.AliasName) - d.Set("name_prefix", naming.NamePrefixFromName(aws.StringValue(alias.AliasName))) - d.Set("target_key_id", alias.TargetKeyId) + alias := outputRaw.(*kms.AliasListEntry) + aliasARN := aws.StringValue(alias.AliasArn) + targetKeyID := aws.StringValue(alias.TargetKeyId) + targetKeyARN, err := tfkms.AliasARNToKeyARN(aliasARN, targetKeyID) - aliasARN, err := arn.Parse(*alias.AliasArn) if err != nil { return err } - targetKeyARN := arn.ARN{ - Partition: aliasARN.Partition, - Service: aliasARN.Service, - Region: aliasARN.Region, - AccountID: aliasARN.AccountID, - Resource: fmt.Sprintf("key/%s", *alias.TargetKeyId), - } - d.Set("target_key_arn", targetKeyARN.String()) + + d.Set("arn", aliasARN) + d.Set("name", alias.AliasName) + d.Set("name_prefix", naming.NamePrefixFromName(aws.StringValue(alias.AliasName))) + d.Set("target_key_arn", targetKeyARN) + d.Set("target_key_id", targetKeyID) return nil } @@ -165,38 +164,20 @@ func resourceAwsKmsAliasTargetUpdate(conn *kms.KMS, d *schema.ResourceData) erro func resourceAwsKmsAliasDelete(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).kmsconn - req := &kms.DeleteAliasInput{ + log.Printf("[DEBUG] Deleting KMS Alias: (%s)", d.Id()) + _, err := conn.DeleteAlias(&kms.DeleteAliasInput{ AliasName: aws.String(d.Id()), - } - _, err := conn.DeleteAlias(req) - if err != nil { - return err - } - - log.Printf("[DEBUG] KMS Alias: (%s) deleted.", d.Id()) - - return nil -} + }) -func retryFindKmsAliasByName(conn *kms.KMS, name string) (*kms.AliasListEntry, error) { - var resp *kms.AliasListEntry - err := resource.Retry(1*time.Minute, func() *resource.RetryError { - var err error - resp, err = findKmsAliasByName(conn, name, nil) - if err != nil { - return resource.NonRetryableError(err) - } - if resp == nil { - return resource.RetryableError(&resource.NotFoundError{}) - } + if tfawserr.ErrCodeEquals(err, kms.ErrCodeNotFoundException) { return nil - }) + } - if isResourceTimeoutError(err) { - resp, err = findKmsAliasByName(conn, name, nil) + if err != nil { + return fmt.Errorf("error deleting KMS Alias (%s): %w", d.Id(), err) } - return resp, err + return nil } // API by default limits results to 50 aliases @@ -229,11 +210,6 @@ func findKmsAliasByName(conn *kms.KMS, name string, marker *string) (*kms.AliasL return nil, nil } -func resourceAwsKmsAliasImport(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { - d.Set("name", d.Id()) - return []*schema.ResourceData{d}, nil -} - func suppressEquivalentTargetKeyIdAndARN(k, old, new string, d *schema.ResourceData) bool { newARN, err := arn.Parse(new) if err != nil { From 4130ed663be0f5c1f777b45dc9a8ef3eba3f4494 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Thu, 22 Jul 2021 17:42:48 -0400 Subject: [PATCH 24/36] Error check. --- aws/resource_aws_kms_key.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index a86fc6ed05d..441269c7356 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -230,6 +230,10 @@ func resourceAwsKmsKeyRead(d *schema.ResourceData, meta interface{}) error { return nil } + if err != nil { + return nil + } + key := outputRaw.(*Key) d.Set("arn", key.metadata.Arn) From 6acc07ef72f850883df32321f3ce46bb59846b0e Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Fri, 23 Jul 2021 10:03:23 -0400 Subject: [PATCH 25/36] Add 'KeyARNOrIDEqual'. --- aws/internal/service/kms/arn.go | 24 +++++++++ aws/internal/service/kms/arn_test.go | 75 ++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) diff --git a/aws/internal/service/kms/arn.go b/aws/internal/service/kms/arn.go index b12e9262be1..d54fd3bb334 100644 --- a/aws/internal/service/kms/arn.go +++ b/aws/internal/service/kms/arn.go @@ -34,3 +34,27 @@ func AliasARNToKeyARN(inputARN, keyID string) (string, error) { return outputARN, nil } + +// KeyARNOrIDEqual returns whether two CMK ARNs or IDs are equal. +func KeyARNOrIDEqual(arnOrID1, arnOrID2 string) bool { + if arnOrID1 == arnOrID2 { + return true + } + + // Key ARN: arn:aws:kms:us-east-2:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab + // Key ID: 1234abcd-12ab-34cd-56ef-1234567890ab + arn1, err := arn.Parse(arnOrID1) + firstIsARN := err == nil + arn2, err := arn.Parse(arnOrID2) + secondIsARN := err == nil + + if firstIsARN && !secondIsARN { + return arn1.Resource == "key/"+arnOrID2 + } + + if secondIsARN && !firstIsARN { + return arn2.Resource == "key/"+arnOrID1 + } + + return false +} diff --git a/aws/internal/service/kms/arn_test.go b/aws/internal/service/kms/arn_test.go index 2c137dcaa74..24766949025 100644 --- a/aws/internal/service/kms/arn_test.go +++ b/aws/internal/service/kms/arn_test.go @@ -58,3 +58,78 @@ func TestAliasARNToKeyARN(t *testing.T) { }) } } + +func TestKeyARNOrIDEqual(t *testing.T) { + testCases := []struct { + name string + first string + second string + want bool + }{ + { + name: "empty", + first: "", + second: "", + want: true, + }, + { + name: "equal IDs", + first: "1234abcd-12ab-34cd-56ef-1234567890ab", + second: "1234abcd-12ab-34cd-56ef-1234567890ab", + want: true, + }, + { + name: "not equal IDs", + first: "1234abcd-12ab-34cd-56ef-1234567890ab", + second: "1234abcd-12ab-34cd-56ef-1234567890ac", + }, + { + name: "equal ARNs", + first: "arn:aws:kms:us-east-2:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab", + second: "arn:aws:kms:us-east-2:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab", + want: true, + }, + { + name: "not equal ARNs", + first: "arn:aws:kms:us-east-2:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab", + second: "arn:aws:kms:us-east-2:111122224444:key/1234abcd-12ab-34cd-56ef-1234567890ab", + }, + { + name: "equal first ID, second ARN", + first: "1234abcd-12ab-34cd-56ef-1234567890ab", + second: "arn:aws:kms:us-east-2:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab", + want: true, + }, + { + name: "equal first ARN, second ID", + first: "arn:aws:kms:us-east-2:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab", + second: "1234abcd-12ab-34cd-56ef-1234567890ab", + want: true, + }, + { + name: "not equal first ID, second ARN", + first: "1234abcd-12ab-34cd-56ef-1234567890ab", + second: "arn:aws:kms:us-east-2:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ac", + }, + { + name: "not equal first ARN, second ID", + first: "arn:aws:kms:us-east-2:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab", + second: "1234abcd-12ab-34cd-56ef-1234567890ac", + }, + { + name: "not equal first ID, second incorrect ARN", + first: "1234abcd-12ab-34cd-56ef-1234567890ab", + second: "arn:aws:kms:us-east-2:111122223333:alias/1234abcd-12ab-34cd-56ef-1234567890ab", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + got := tfkms.KeyARNOrIDEqual(testCase.first, testCase.second) + + if got != testCase.want { + t.Errorf("unexpected Equal: %t", got) + } + }) + } +} From c449de0bb42b2a38b587260762a4f625fade753e Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Fri, 23 Jul 2021 11:12:56 -0400 Subject: [PATCH 26/36] r/aws_kms_alias: `name` and `name_prefix` are Computed. Acceptance test output: % make testacc TESTARGS='-run=TestAccAWSKmsAlias_\|TestAccAWSEBSDefaultKmsKey_basic' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSKmsAlias_\|TestAccAWSEBSDefaultKmsKey_basic -timeout 180m === RUN TestAccAWSEBSDefaultKmsKey_basic === PAUSE TestAccAWSEBSDefaultKmsKey_basic === RUN TestAccAWSKmsAlias_basic === PAUSE TestAccAWSKmsAlias_basic === RUN TestAccAWSKmsAlias_disappears === PAUSE TestAccAWSKmsAlias_disappears === RUN TestAccAWSKmsAlias_Name_Generated === PAUSE TestAccAWSKmsAlias_Name_Generated === RUN TestAccAWSKmsAlias_NamePrefix === PAUSE TestAccAWSKmsAlias_NamePrefix === RUN TestAccAWSKmsAlias_UpdateKeyID === PAUSE TestAccAWSKmsAlias_UpdateKeyID === RUN TestAccAWSKmsAlias_MultipleAliasesForSameKey === PAUSE TestAccAWSKmsAlias_MultipleAliasesForSameKey === RUN TestAccAWSKmsAlias_ArnDiffSuppress === PAUSE TestAccAWSKmsAlias_ArnDiffSuppress === CONT TestAccAWSEBSDefaultKmsKey_basic === CONT TestAccAWSKmsAlias_UpdateKeyID === CONT TestAccAWSKmsAlias_ArnDiffSuppress === CONT TestAccAWSKmsAlias_MultipleAliasesForSameKey === CONT TestAccAWSKmsAlias_Name_Generated === CONT TestAccAWSKmsAlias_disappears === CONT TestAccAWSKmsAlias_NamePrefix === CONT TestAccAWSKmsAlias_basic --- PASS: TestAccAWSKmsAlias_disappears (20.97s) --- PASS: TestAccAWSKmsAlias_Name_Generated (23.93s) --- PASS: TestAccAWSKmsAlias_basic (23.96s) --- PASS: TestAccAWSKmsAlias_NamePrefix (24.19s) --- PASS: TestAccAWSKmsAlias_MultipleAliasesForSameKey (24.28s) --- PASS: TestAccAWSEBSDefaultKmsKey_basic (24.39s) --- PASS: TestAccAWSKmsAlias_ArnDiffSuppress (31.42s) --- PASS: TestAccAWSKmsAlias_UpdateKeyID (35.92s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 39.542s --- aws/internal/naming/naming_test.go | 5 + aws/internal/service/kms/enum.go | 4 + aws/resource_aws_ebs_default_kms_key_test.go | 3 +- aws/resource_aws_kms_alias.go | 79 ++----- aws/resource_aws_kms_alias_test.go | 234 +++++++++++++------ 5 files changed, 192 insertions(+), 133 deletions(-) diff --git a/aws/internal/naming/naming_test.go b/aws/internal/naming/naming_test.go index 0242eb1265c..c3f0486279d 100644 --- a/aws/internal/naming/naming_test.go +++ b/aws/internal/naming/naming_test.go @@ -253,6 +253,11 @@ func TestNamePrefixFromName(t *testing.T) { Input: "terraform-20060102150405000000000001", Expected: strPtr("terraform-"), }, + { + TestName: "KMS alias prefix", + Input: "alias/20210723150229087000000002", + Expected: strPtr("alias/"), + }, } for _, testCase := range testCases { diff --git a/aws/internal/service/kms/enum.go b/aws/internal/service/kms/enum.go index f75335b3d46..a7e255d21bf 100644 --- a/aws/internal/service/kms/enum.go +++ b/aws/internal/service/kms/enum.go @@ -1,5 +1,9 @@ package kms +const ( + AliasNamePrefix = "alias/" +) + const ( PolicyNameDefault = "default" ) diff --git a/aws/resource_aws_ebs_default_kms_key_test.go b/aws/resource_aws_ebs_default_kms_key_test.go index 28c5d0f26f8..68b25d457d2 100644 --- a/aws/resource_aws_ebs_default_kms_key_test.go +++ b/aws/resource_aws_ebs_default_kms_key_test.go @@ -9,6 +9,7 @@ import ( "github.com/aws/aws-sdk-go/service/ec2" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" + kmsfinder "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/finder" ) func TestAccAWSEBSDefaultKmsKey_basic(t *testing.T) { @@ -94,7 +95,7 @@ func testAccCheckEbsDefaultKmsKey(name string) resource.TestCheckFunc { func testAccAwsEbsDefaultKmsKeyAwsManagedDefaultKey() (*arn.ARN, error) { conn := testAccProvider.Meta().(*AWSClient).kmsconn - alias, err := findKmsAliasByName(conn, "alias/aws/ebs", nil) + alias, err := kmsfinder.AliasByName(conn, "alias/aws/ebs") if err != nil { return nil, err } diff --git a/aws/resource_aws_kms_alias.go b/aws/resource_aws_kms_alias.go index 74f5eced914..c4229acc8ba 100644 --- a/aws/resource_aws_kms_alias.go +++ b/aws/resource_aws_kms_alias.go @@ -3,10 +3,8 @@ package aws import ( "fmt" "log" - "strings" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/aws/arn" "github.com/aws/aws-sdk-go/service/kms" "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" @@ -37,6 +35,7 @@ func resourceAwsKmsAlias() *schema.Resource { "name": { Type: schema.TypeString, Optional: true, + Computed: true, ForceNew: true, ConflictsWith: []string{"name_prefix"}, ValidateFunc: validateAwsKmsName, @@ -45,6 +44,7 @@ func resourceAwsKmsAlias() *schema.Resource { "name_prefix": { Type: schema.TypeString, Optional: true, + Computed: true, ForceNew: true, ConflictsWith: []string{"name"}, ValidateFunc: validateAwsKmsName, @@ -58,22 +58,18 @@ func resourceAwsKmsAlias() *schema.Resource { "target_key_id": { Type: schema.TypeString, Required: true, - DiffSuppressFunc: suppressEquivalentTargetKeyIdAndARN, + DiffSuppressFunc: suppressEquivalentKmsKeyARNOrID, }, }, } } -const ( - kmsAliasDefaultNamePrefix = "alias/" -) - func resourceAwsKmsAliasCreate(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).kmsconn namePrefix := d.Get("name_prefix").(string) if namePrefix == "" { - namePrefix = kmsAliasDefaultNamePrefix + namePrefix = tfkms.AliasNamePrefix } name := naming.Generate(d.Get("name").(string), namePrefix) @@ -137,28 +133,20 @@ func resourceAwsKmsAliasUpdate(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).kmsconn if d.HasChange("target_key_id") { - err := resourceAwsKmsAliasTargetUpdate(conn, d) - if err != nil { - return err + input := &kms.UpdateAliasInput{ + AliasName: aws.String(d.Id()), + TargetKeyId: aws.String(d.Get("target_key_id").(string)), } - return resourceAwsKmsAliasRead(d, meta) - } - return nil -} - -func resourceAwsKmsAliasTargetUpdate(conn *kms.KMS, d *schema.ResourceData) error { - name := d.Get("name").(string) - targetKeyId := d.Get("target_key_id").(string) - log.Printf("[DEBUG] KMS alias: %s, update target: %s", name, targetKeyId) + log.Printf("[DEBUG] Updating KMS Alias: %s", input) + _, err := conn.UpdateAlias(input) - req := &kms.UpdateAliasInput{ - AliasName: aws.String(name), - TargetKeyId: aws.String(targetKeyId), + if err != nil { + return fmt.Errorf("error updating KMS Alias (%s): %w", d.Id(), err) + } } - _, err := conn.UpdateAlias(req) - return err + return resourceAwsKmsAliasRead(d, meta) } func resourceAwsKmsAliasDelete(d *schema.ResourceData, meta interface{}) error { @@ -180,43 +168,6 @@ func resourceAwsKmsAliasDelete(d *schema.ResourceData, meta interface{}) error { return nil } -// API by default limits results to 50 aliases -// This is how we make sure we won't miss any alias -// See http://docs.aws.amazon.com/kms/latest/APIReference/API_ListAliases.html -func findKmsAliasByName(conn *kms.KMS, name string, marker *string) (*kms.AliasListEntry, error) { - req := kms.ListAliasesInput{ - Limit: aws.Int64(int64(100)), - } - if marker != nil { - req.Marker = marker - } - - log.Printf("[DEBUG] Listing KMS aliases: %s", req) - resp, err := conn.ListAliases(&req) - if err != nil { - return nil, err - } - - for _, entry := range resp.Aliases { - if *entry.AliasName == name { - return entry, nil - } - } - if *resp.Truncated { - log.Printf("[DEBUG] KMS alias list is truncated, listing more via %s", *resp.NextMarker) - return findKmsAliasByName(conn, name, resp.NextMarker) - } - - return nil, nil -} - -func suppressEquivalentTargetKeyIdAndARN(k, old, new string, d *schema.ResourceData) bool { - newARN, err := arn.Parse(new) - if err != nil { - log.Printf("[DEBUG] %q can not be parsed as an ARN: %q", new, err) - return false - } - - resource := strings.TrimPrefix(newARN.Resource, "key/") - return old == resource +func suppressEquivalentKmsKeyARNOrID(k, old, new string, d *schema.ResourceData) bool { + return tfkms.KeyARNOrIDEqual(old, new) } diff --git a/aws/resource_aws_kms_alias_test.go b/aws/resource_aws_kms_alias_test.go index 810ea7e511b..86f0be77b1c 100644 --- a/aws/resource_aws_kms_alias_test.go +++ b/aws/resource_aws_kms_alias_test.go @@ -2,19 +2,24 @@ package aws import ( "fmt" + "regexp" "testing" - "time" "github.com/aws/aws-sdk-go/service/kms" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/naming" + tfkms "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/finder" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) func TestAccAWSKmsAlias_basic(t *testing.T) { - rInt := acctest.RandInt() - kmsAliasTimestamp := time.Now().Format(time.RFC1123) + var alias kms.AliasListEntry + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_alias.test" + keyResourceName := "aws_kms_key.test" resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, @@ -23,10 +28,13 @@ func TestAccAWSKmsAlias_basic(t *testing.T) { CheckDestroy: testAccCheckAWSKmsAliasDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSKmsSingleAlias(rInt, kmsAliasTimestamp), + Config: testAccAWSKmsAliasConfigName(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckAWSKmsAliasExists(resourceName), - resource.TestCheckResourceAttrSet(resourceName, "target_key_arn"), + testAccCheckAWSKmsAliasExists(resourceName, &alias), + testAccMatchResourceAttrRegionalARN(resourceName, "arn", "kms", regexp.MustCompile(`alias/.+`)), + resource.TestCheckResourceAttr(resourceName, "name", tfkms.AliasNamePrefix+rName), + resource.TestCheckResourceAttrPair(resourceName, "target_key_arn", keyResourceName, "arn"), + resource.TestCheckResourceAttrPair(resourceName, "target_key_id", keyResourceName, "id"), ), }, { @@ -34,19 +42,36 @@ func TestAccAWSKmsAlias_basic(t *testing.T) { ImportState: true, ImportStateVerify: true, }, + }, + }) +} + +func TestAccAWSKmsAlias_disappears(t *testing.T) { + var alias kms.AliasListEntry + rName := acctest.RandomWithPrefix("tf-acc-test") + resourceName := "aws_kms_alias.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ErrorCheck: testAccErrorCheck(t, kms.EndpointsID), + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSKmsAliasDestroy, + Steps: []resource.TestStep{ { - Config: testAccAWSKmsSingleAlias_modified(rInt, kmsAliasTimestamp), + Config: testAccAWSKmsAliasConfigName(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckAWSKmsAliasExists(resourceName), + testAccCheckAWSKmsAliasExists(resourceName, &alias), + testAccCheckResourceDisappears(testAccProvider, resourceAwsKmsAlias(), resourceName), ), + ExpectNonEmptyPlan: true, }, }, }) } -func TestAccAWSKmsAlias_name_prefix(t *testing.T) { - rInt := acctest.RandInt() - kmsAliasTimestamp := time.Now().Format(time.RFC1123) +func TestAccAWSKmsAlias_Name_Generated(t *testing.T) { + var alias kms.AliasListEntry + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_alias.test" resource.ParallelTest(t, resource.TestCase{ @@ -56,10 +81,11 @@ func TestAccAWSKmsAlias_name_prefix(t *testing.T) { CheckDestroy: testAccCheckAWSKmsAliasDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSKmsSingleAlias(rInt, kmsAliasTimestamp), + Config: testAccAWSKmsAliasConfigNameGenerated(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckAWSKmsAliasExists("aws_kms_alias.name_prefix"), - resource.TestCheckResourceAttrSet("aws_kms_alias.name_prefix", "target_key_arn"), + testAccCheckAWSKmsAliasExists(resourceName, &alias), + resource.TestMatchResourceAttr(resourceName, "name", regexp.MustCompile(fmt.Sprintf("%s[[:xdigit:]]{%d}", tfkms.AliasNamePrefix, resource.UniqueIDSuffixLength))), + resource.TestCheckResourceAttr(resourceName, "name_prefix", tfkms.AliasNamePrefix), ), }, { @@ -71,9 +97,9 @@ func TestAccAWSKmsAlias_name_prefix(t *testing.T) { }) } -func TestAccAWSKmsAlias_no_name(t *testing.T) { - rInt := acctest.RandInt() - kmsAliasTimestamp := time.Now().Format(time.RFC1123) +func TestAccAWSKmsAlias_NamePrefix(t *testing.T) { + var alias kms.AliasListEntry + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_alias.test" resource.ParallelTest(t, resource.TestCase{ @@ -83,10 +109,11 @@ func TestAccAWSKmsAlias_no_name(t *testing.T) { CheckDestroy: testAccCheckAWSKmsAliasDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSKmsSingleAlias(rInt, kmsAliasTimestamp), + Config: testAccAWSKmsAliasConfigNamePrefix(rName, tfkms.AliasNamePrefix+"tf-acc-test-prefix-"), Check: resource.ComposeTestCheckFunc( - testAccCheckAWSKmsAliasExists("aws_kms_alias.nothing"), - resource.TestCheckResourceAttrSet("aws_kms_alias.nothing", "target_key_arn"), + testAccCheckAWSKmsAliasExists(resourceName, &alias), + naming.TestCheckResourceAttrNameFromPrefix(resourceName, "name", tfkms.AliasNamePrefix+"tf-acc-test-prefix-"), + resource.TestCheckResourceAttr(resourceName, "name_prefix", tfkms.AliasNamePrefix+"tf-acc-test-prefix-"), ), }, { @@ -98,10 +125,12 @@ func TestAccAWSKmsAlias_no_name(t *testing.T) { }) } -func TestAccAWSKmsAlias_multiple(t *testing.T) { - rInt := acctest.RandInt() - kmsAliasTimestamp := time.Now().Format(time.RFC1123) +func TestAccAWSKmsAlias_UpdateKeyID(t *testing.T) { + var alias kms.AliasListEntry + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_alias.test" + key1ResourceName := "aws_kms_key.test" + key2ResourceName := "aws_kms_key.test2" resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, @@ -110,12 +139,52 @@ func TestAccAWSKmsAlias_multiple(t *testing.T) { CheckDestroy: testAccCheckAWSKmsAliasDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSKmsMultipleAliases(rInt, kmsAliasTimestamp), + Config: testAccAWSKmsAliasConfigName(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckAWSKmsAliasExists("aws_kms_alias.test"), - resource.TestCheckResourceAttrSet("aws_kms_alias.test", "target_key_arn"), - testAccCheckAWSKmsAliasExists("aws_kms_alias.test2"), - resource.TestCheckResourceAttrSet("aws_kms_alias.test2", "target_key_arn"), + testAccCheckAWSKmsAliasExists(resourceName, &alias), + resource.TestCheckResourceAttrPair(resourceName, "target_key_arn", key1ResourceName, "arn"), + resource.TestCheckResourceAttrPair(resourceName, "target_key_id", key1ResourceName, "id"), + ), + }, + { + Config: testAccAWSKmsAliasConfigUpdatedKeyId(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSKmsAliasExists(resourceName, &alias), + resource.TestCheckResourceAttrPair(resourceName, "target_key_arn", key2ResourceName, "arn"), + resource.TestCheckResourceAttrPair(resourceName, "target_key_id", key2ResourceName, "id"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func TestAccAWSKmsAlias_MultipleAliasesForSameKey(t *testing.T) { + var alias kms.AliasListEntry + rName := acctest.RandomWithPrefix("tf-acc-test") + resourceName := "aws_kms_alias.test" + alias2ResourceName := "aws_kms_alias.test2" + keyResourceName := "aws_kms_key.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ErrorCheck: testAccErrorCheck(t, kms.EndpointsID), + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSKmsAliasDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSKmsAliasConfigMultiple(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSKmsAliasExists(resourceName, &alias), + resource.TestCheckResourceAttrPair(resourceName, "target_key_arn", keyResourceName, "arn"), + resource.TestCheckResourceAttrPair(resourceName, "target_key_id", keyResourceName, "id"), + testAccCheckAWSKmsAliasExists(alias2ResourceName, &alias), + resource.TestCheckResourceAttrPair(alias2ResourceName, "target_key_arn", keyResourceName, "arn"), + resource.TestCheckResourceAttrPair(alias2ResourceName, "target_key_id", keyResourceName, "id"), ), }, { @@ -128,8 +197,8 @@ func TestAccAWSKmsAlias_multiple(t *testing.T) { } func TestAccAWSKmsAlias_ArnDiffSuppress(t *testing.T) { - rInt := acctest.RandInt() - kmsAliasTimestamp := time.Now().Format(time.RFC1123) + var alias kms.AliasListEntry + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_alias.test" resource.ParallelTest(t, resource.TestCase{ @@ -139,10 +208,10 @@ func TestAccAWSKmsAlias_ArnDiffSuppress(t *testing.T) { CheckDestroy: testAccCheckAWSKmsAliasDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSKmsArnDiffSuppress(rInt, kmsAliasTimestamp), + Config: testAccAWSKmsAliasConfigDiffSuppress(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckAWSKmsAliasExists("aws_kms_alias.test"), - resource.TestCheckResourceAttrSet("aws_kms_alias.test", "target_key_arn"), + testAccCheckAWSKmsAliasExists(resourceName, &alias), + resource.TestCheckResourceAttrSet(resourceName, "target_key_arn"), ), }, { @@ -153,7 +222,7 @@ func TestAccAWSKmsAlias_ArnDiffSuppress(t *testing.T) { { ExpectNonEmptyPlan: false, PlanOnly: true, - Config: testAccAWSKmsArnDiffSuppress(rInt, kmsAliasTimestamp), + Config: testAccAWSKmsAliasConfigDiffSuppress(rName), }, }, }) @@ -167,107 +236,136 @@ func testAccCheckAWSKmsAliasDestroy(s *terraform.State) error { continue } - entry, err := findKmsAliasByName(conn, rs.Primary.ID, nil) + _, err := finder.AliasByName(conn, rs.Primary.ID) + + if tfresource.NotFound(err) { + continue + } + if err != nil { return err } - if entry != nil { - return fmt.Errorf("KMS alias still exists:\n%#v", entry) - } - return nil + return fmt.Errorf("KMS Alias %s still exists", rs.Primary.ID) } return nil } -func testAccCheckAWSKmsAliasExists(name string) resource.TestCheckFunc { +func testAccCheckAWSKmsAliasExists(name string, v *kms.AliasListEntry) resource.TestCheckFunc { return func(s *terraform.State) error { - _, ok := s.RootModule().Resources[name] + rs, ok := s.RootModule().Resources[name] if !ok { return fmt.Errorf("Not found: %s", name) } + if rs.Primary.ID == "" { + return fmt.Errorf("No KMS Alias ID is set") + } + + conn := testAccProvider.Meta().(*AWSClient).kmsconn + + output, err := finder.AliasByName(conn, rs.Primary.ID) + + if err != nil { + return err + } + + *v = *output + return nil } } -func testAccAWSKmsSingleAlias(rInt int, timestamp string) string { +func testAccAWSKmsAliasConfigName(rName string) string { return fmt.Sprintf(` resource "aws_kms_key" "test" { - description = "Terraform acc test One %s" + description = %[1]q deletion_window_in_days = 7 } -resource "aws_kms_key" "test2" { - description = "Terraform acc test Two %s" +resource "aws_kms_alias" "test" { + name = "alias/%[1]s" + target_key_id = aws_kms_key.test.id +} +`, rName) +} + +func testAccAWSKmsAliasConfigNameGenerated(rName string) string { + return fmt.Sprintf(` +resource "aws_kms_key" "test" { + description = %[1]q deletion_window_in_days = 7 } -resource "aws_kms_alias" "name_prefix" { - name_prefix = "alias/tf-acc-key-alias-%d" - target_key_id = aws_kms_key.test.key_id +resource "aws_kms_alias" "test" { + target_key_id = aws_kms_key.test.id +} +`, rName) } -resource "aws_kms_alias" "nothing" { - target_key_id = aws_kms_key.test.key_id +func testAccAWSKmsAliasConfigNamePrefix(rName, namePrefix string) string { + return fmt.Sprintf(` +resource "aws_kms_key" "test" { + description = %[1]q + deletion_window_in_days = 7 } resource "aws_kms_alias" "test" { - name = "alias/tf-acc-key-alias-%d" - target_key_id = aws_kms_key.test.key_id + name_prefix = %[2]q + target_key_id = aws_kms_key.test.id } -`, timestamp, timestamp, rInt, rInt) +`, rName, namePrefix) } -func testAccAWSKmsSingleAlias_modified(rInt int, timestamp string) string { +func testAccAWSKmsAliasConfigUpdatedKeyId(rName string) string { return fmt.Sprintf(` resource "aws_kms_key" "test" { - description = "Terraform acc test One %s" + description = %[1]q deletion_window_in_days = 7 } resource "aws_kms_key" "test2" { - description = "Terraform acc test Two %s" + description = "%[1]s-2" deletion_window_in_days = 7 } resource "aws_kms_alias" "test" { - name = "alias/tf-acc-key-alias-%d" - target_key_id = aws_kms_key.test2.key_id + name = "alias/%[1]s" + target_key_id = aws_kms_key.test2.id } -`, timestamp, timestamp, rInt) +`, rName) } -func testAccAWSKmsMultipleAliases(rInt int, timestamp string) string { +func testAccAWSKmsAliasConfigMultiple(rName string) string { return fmt.Sprintf(` resource "aws_kms_key" "test" { - description = "Terraform acc test One %s" + description = %[1]q deletion_window_in_days = 7 } resource "aws_kms_alias" "test" { - name = "alias/tf-acc-alias-test-%d" + name = "alias/%[1]s-1" target_key_id = aws_kms_key.test.key_id } resource "aws_kms_alias" "test2" { - name = "alias/tf-acc-alias-test2-%d" + name = "alias/%[1]s-2" target_key_id = aws_kms_key.test.key_id } -`, timestamp, rInt, rInt) +`, rName) } -func testAccAWSKmsArnDiffSuppress(rInt int, timestamp string) string { +func testAccAWSKmsAliasConfigDiffSuppress(rName string) string { return fmt.Sprintf(` resource "aws_kms_key" "test" { - description = "Terraform acc test test %s" + description = %[1]q deletion_window_in_days = 7 } resource "aws_kms_alias" "test" { - name = "alias/tf-acc-key-alias-%d" + name = "alias/%[1]s" target_key_id = aws_kms_key.test.arn } -`, timestamp, rInt) +`, rName) } From 97d68e627b5b5b93f491b6a5e20c722482630238 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Tue, 27 Jul 2021 13:00:17 -0400 Subject: [PATCH 27/36] d/aws_kms_alias: Use internal finder package. Acceptance test output: % make testacc TESTARGS='-run=TestAccDataSourceAwsKmsAlias_' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccDataSourceAwsKmsAlias_ -timeout 180m === RUN TestAccDataSourceAwsKmsAlias_AwsService === PAUSE TestAccDataSourceAwsKmsAlias_AwsService === RUN TestAccDataSourceAwsKmsAlias_CMK === PAUSE TestAccDataSourceAwsKmsAlias_CMK === CONT TestAccDataSourceAwsKmsAlias_AwsService === CONT TestAccDataSourceAwsKmsAlias_CMK --- PASS: TestAccDataSourceAwsKmsAlias_AwsService (12.56s) --- PASS: TestAccDataSourceAwsKmsAlias_CMK (21.23s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 24.461s --- aws/data_source_aws_kms_alias.go | 45 +++++++++------------------ aws/data_source_aws_kms_alias_test.go | 44 ++++++++++---------------- 2 files changed, 30 insertions(+), 59 deletions(-) diff --git a/aws/data_source_aws_kms_alias.go b/aws/data_source_aws_kms_alias.go index 3d8509a246f..21ee79abd00 100644 --- a/aws/data_source_aws_kms_alias.go +++ b/aws/data_source_aws_kms_alias.go @@ -2,26 +2,25 @@ package aws import ( "fmt" - "log" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/kms" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/finder" ) func dataSourceAwsKmsAlias() *schema.Resource { return &schema.Resource{ Read: dataSourceAwsKmsAliasRead, Schema: map[string]*schema.Schema{ + "arn": { + Type: schema.TypeString, + Computed: true, + }, "name": { Type: schema.TypeString, Required: true, ValidateFunc: validateAwsKmsName, }, - "arn": { - Type: schema.TypeString, - Computed: true, - }, "target_key_arn": { Type: schema.TypeString, Computed: true, @@ -36,27 +35,13 @@ func dataSourceAwsKmsAlias() *schema.Resource { func dataSourceAwsKmsAliasRead(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).kmsconn - params := &kms.ListAliasesInput{} - target := d.Get("name") - var alias *kms.AliasListEntry - log.Printf("[DEBUG] Reading KMS Alias: %s", params) - err := conn.ListAliasesPages(params, func(page *kms.ListAliasesOutput, lastPage bool) bool { - for _, entity := range page.Aliases { - if aws.StringValue(entity.AliasName) == target { - alias = entity - return false - } - } + target := d.Get("name").(string) - return true - }) - if err != nil { - return fmt.Errorf("Error fetch KMS alias list: %w", err) - } + alias, err := finder.AliasByName(conn, target) - if alias == nil { - return fmt.Errorf("No alias with name %q found in this region.", target) + if err != nil { + return fmt.Errorf("error reading KMS Alias (%s): %w", target, err) } d.SetId(aws.StringValue(alias.AliasArn)) @@ -73,16 +58,14 @@ func dataSourceAwsKmsAliasRead(d *schema.ResourceData, meta interface{}) error { // // https://docs.aws.amazon.com/kms/latest/APIReference/API_ListAliases.html - req := &kms.DescribeKeyInput{ - KeyId: aws.String(target.(string)), - } - resp, err := conn.DescribeKey(req) + keyMetadata, err := finder.KeyByID(conn, target) + if err != nil { - return fmt.Errorf("Error calling KMS DescribeKey: %w", err) + return fmt.Errorf("error reading KMS Key (%s): %w", target, err) } - d.Set("target_key_arn", resp.KeyMetadata.Arn) - d.Set("target_key_id", resp.KeyMetadata.KeyId) + d.Set("target_key_arn", keyMetadata.Arn) + d.Set("target_key_id", keyMetadata.KeyId) return nil } diff --git a/aws/data_source_aws_kms_alias_test.go b/aws/data_source_aws_kms_alias_test.go index f4eed540139..d35745474e9 100644 --- a/aws/data_source_aws_kms_alias_test.go +++ b/aws/data_source_aws_kms_alias_test.go @@ -8,11 +8,10 @@ import ( "github.com/aws/aws-sdk-go/service/kms" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" - "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" ) func TestAccDataSourceAwsKmsAlias_AwsService(t *testing.T) { - name := "alias/aws/s3" + rName := "alias/aws/s3" resourceName := "data.aws_kms_alias.test" resource.ParallelTest(t, resource.TestCase{ @@ -21,11 +20,10 @@ func TestAccDataSourceAwsKmsAlias_AwsService(t *testing.T) { Providers: testAccProviders, Steps: []resource.TestStep{ { - Config: testAccDataSourceAwsKmsAlias_name(name), + Config: testAccDataSourceAwsKmsAliasConfig(rName), Check: resource.ComposeTestCheckFunc( - testAccDataSourceAwsKmsAliasCheckExists(resourceName), - testAccCheckResourceAttrRegionalARN(resourceName, "arn", "kms", name), - resource.TestCheckResourceAttr(resourceName, "name", name), + testAccCheckResourceAttrRegionalARN(resourceName, "arn", "kms", rName), + resource.TestCheckResourceAttr(resourceName, "name", rName), testAccMatchResourceAttrRegionalARN(resourceName, "target_key_arn", "kms", regexp.MustCompile(`key/[a-z0-9]{8}-([a-z0-9]{4}-){3}[a-z0-9]{12}`)), resource.TestMatchResourceAttr(resourceName, "target_key_id", regexp.MustCompile("^[a-z0-9]{8}-([a-z0-9]{4}-){3}[a-z0-9]{12}$")), ), @@ -35,7 +33,7 @@ func TestAccDataSourceAwsKmsAlias_AwsService(t *testing.T) { } func TestAccDataSourceAwsKmsAlias_CMK(t *testing.T) { - rInt := acctest.RandInt() + rName := acctest.RandomWithPrefix("tf-acc-test") aliasResourceName := "aws_kms_alias.test" datasourceAliasResourceName := "data.aws_kms_alias.test" @@ -45,9 +43,8 @@ func TestAccDataSourceAwsKmsAlias_CMK(t *testing.T) { Providers: testAccProviders, Steps: []resource.TestStep{ { - Config: testAccDataSourceAwsKmsAlias_CMK(rInt), + Config: testAccDataSourceAwsKmsAliasConfigCMK(rName), Check: resource.ComposeTestCheckFunc( - testAccDataSourceAwsKmsAliasCheckExists(datasourceAliasResourceName), resource.TestCheckResourceAttrPair(datasourceAliasResourceName, "arn", aliasResourceName, "arn"), resource.TestCheckResourceAttrPair(datasourceAliasResourceName, "target_key_arn", aliasResourceName, "target_key_arn"), resource.TestCheckResourceAttrPair(datasourceAliasResourceName, "target_key_id", aliasResourceName, "target_key_id"), @@ -57,37 +54,28 @@ func TestAccDataSourceAwsKmsAlias_CMK(t *testing.T) { }) } -func testAccDataSourceAwsKmsAliasCheckExists(name string) resource.TestCheckFunc { - return func(s *terraform.State) error { - _, ok := s.RootModule().Resources[name] - if !ok { - return fmt.Errorf("root module has no resource called %s", name) - } - - return nil - } -} - -func testAccDataSourceAwsKmsAlias_name(name string) string { +func testAccDataSourceAwsKmsAliasConfig(rName string) string { return fmt.Sprintf(` data "aws_kms_alias" "test" { - name = "%s" + name = %[1]q } -`, name) +`, rName) } -func testAccDataSourceAwsKmsAlias_CMK(rInt int) string { +func testAccDataSourceAwsKmsAliasConfigCMK(rName string) string { return fmt.Sprintf(` resource "aws_kms_key" "test" { - description = "Terraform acc test" + description = %[1]q deletion_window_in_days = 7 } resource "aws_kms_alias" "test" { - name = "alias/tf-acc-key-alias-%d" + name = "alias/%[1]s" target_key_id = aws_kms_key.test.key_id } -%s -`, rInt, testAccDataSourceAwsKmsAlias_name("${aws_kms_alias.test.name}")) +data "aws_kms_alias" "test" { + name = aws_kms_alias.test.name +} +`, rName) } From 4398730abdeaaa6167248bf27486fd85c7a0d90f Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Tue, 27 Jul 2021 13:36:36 -0400 Subject: [PATCH 28/36] r/aws_kms_external_key: Use internal packages. --- aws/internal/service/kms/waiter/waiter.go | 23 ------- aws/resource_aws_kms_external_key.go | 84 ++++++++++++----------- aws/resource_aws_kms_external_key_test.go | 51 ++++---------- aws/resource_aws_kms_key.go | 2 +- 4 files changed, 59 insertions(+), 101 deletions(-) diff --git a/aws/internal/service/kms/waiter/waiter.go b/aws/internal/service/kms/waiter/waiter.go index cb571d760f4..f677ad07305 100644 --- a/aws/internal/service/kms/waiter/waiter.go +++ b/aws/internal/service/kms/waiter/waiter.go @@ -142,26 +142,3 @@ func TagsPropagated(conn *kms.KMS, id string, tags keyvaluetags.KeyValueTags) er return tfresource.WaitUntil(PropagationTimeout, checkFunc, opts) } - -// TODO: Remove - -// 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 921ac3ea748..4dc3f7f9ecb 100644 --- a/aws/resource_aws_kms_external_key.go +++ b/aws/resource_aws_kms_external_key.go @@ -12,12 +12,12 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/kms" + "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure" "github.com/hashicorp/terraform-plugin-sdk/v2/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" ) @@ -39,40 +39,55 @@ func resourceAwsKmsExternalKey() *schema.Resource { Type: schema.TypeString, Computed: true, }, + + // TODO + // "bypass_policy_lockout_safety_check": { + // Type: schema.TypeBool, + // Optional: true, + // Default: false, + // }, + "deletion_window_in_days": { Type: schema.TypeInt, Optional: true, Default: 30, ValidateFunc: validation.IntBetween(7, 30), }, + "description": { Type: schema.TypeString, Optional: true, ValidateFunc: validation.StringLenBetween(0, 8192), }, + "enabled": { Type: schema.TypeBool, Optional: true, Computed: true, }, + "expiration_model": { Type: schema.TypeString, Computed: true, }, + "key_material_base64": { Type: schema.TypeString, Optional: true, ForceNew: true, Sensitive: true, }, + "key_state": { Type: schema.TypeString, Computed: true, }, + "key_usage": { Type: schema.TypeString, Computed: true, }, + "policy": { Type: schema.TypeString, Optional: true, @@ -83,8 +98,10 @@ func resourceAwsKmsExternalKey() *schema.Resource { validation.StringIsJSON, ), }, + "tags": tagsSchema(), "tags_all": tagsSchemaComputed(), + "valid_to": { Type: schema.TypeString, Optional: true, @@ -116,46 +133,31 @@ func resourceAwsKmsExternalKeyCreate(d *schema.ResourceData, meta interface{}) e input.Tags = tags.IgnoreAws().KmsTags() } - var output *kms.CreateKeyOutput - err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError { - var err error - - output, err = conn.CreateKey(input) + // AWS requires any principal in the policy to exist before the key is created. + // The KMS service's awareness of principals is limited by "eventual consistency". + // KMS will report this error until it can validate the policy itself. + // They acknowledge this here: + // http://docs.aws.amazon.com/kms/latest/APIReference/API_CreateKey.html + log.Printf("[DEBUG] Creating KMS External Key: %s", input) - // AWS requires any principal in the policy to exist before the key is created. - // The KMS service's awareness of principals is limited by "eventual consistency". - // KMS will report this error until it can validate the policy itself. - // They acknowledge this here: - // http://docs.aws.amazon.com/kms/latest/APIReference/API_CreateKey.html - if isAWSErr(err, kms.ErrCodeMalformedPolicyDocumentException, "") { - return resource.RetryableError(err) - } - - if err != nil { - return resource.NonRetryableError(err) - } - - return nil + outputRaw, err := waiter.IAMPropagation(func() (interface{}, error) { + return conn.CreateKey(input) }) - if isResourceTimeoutError(err) { - output, err = conn.CreateKey(input) - } - if err != nil { - return fmt.Errorf("error creating KMS External Key: %s", err) + return fmt.Errorf("error creating KMS External Key: %w", err) } - d.SetId(aws.StringValue(output.KeyMetadata.KeyId)) + d.SetId(aws.StringValue(outputRaw.(*kms.CreateKeyOutput).KeyMetadata.KeyId)) if v, ok := d.GetOk("key_material_base64"); ok { if err := importKmsExternalKeyMaterial(conn, d.Id(), v.(string), d.Get("valid_to").(string)); err != nil { return fmt.Errorf("error importing KMS External Key (%s) material: %s", d.Id(), err) } - if !d.Get("enabled").(bool) { - if err := updateKmsKeyStatus(conn, d.Id(), false); err != nil { - return fmt.Errorf("error disabling KMS External Key (%s): %s", d.Id(), err) + if enabled := d.Get("enabled").(bool); !enabled { + if err := updateKmsKeyEnabled(conn, d.Id(), enabled); err != nil { + return err } } } @@ -337,28 +339,30 @@ func resourceAwsKmsExternalKeyDelete(d *schema.ResourceData, meta interface{}) e conn := meta.(*AWSClient).kmsconn input := &kms.ScheduleKeyDeletionInput{ - KeyId: aws.String(d.Id()), - PendingWindowInDays: aws.Int64(int64(d.Get("deletion_window_in_days").(int))), + KeyId: aws.String(d.Id()), } + if v, ok := d.GetOk("deletion_window_in_days"); ok { + input.PendingWindowInDays = aws.Int64(int64(v.(int))) + } + + log.Printf("[DEBUG] Deleting KMS External Key: (%s)", d.Id()) _, err := conn.ScheduleKeyDeletion(input) - if isAWSErr(err, kms.ErrCodeNotFoundException, "") { + if tfawserr.ErrCodeEquals(err, kms.ErrCodeNotFoundException) { return nil } - if err != nil { - 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, "") { + if tfawserr.ErrMessageContains(err, kms.ErrCodeInvalidStateException, "is pending deletion") { return nil } if err != nil { - return fmt.Errorf("error waiting for KMS Key (%s) to schedule deletion: %w", d.Id(), err) + return fmt.Errorf("error deleting KMS External Key (%s): %w", d.Id(), err) + } + + if _, err := waiter.KeyDeleted(conn, d.Id()); err != nil { + return fmt.Errorf("error waiting for KMS External Key (%s) to delete: %w", d.Id(), err) } return nil diff --git a/aws/resource_aws_kms_external_key_test.go b/aws/resource_aws_kms_external_key_test.go index dd5fda2094d..c4748c2d035 100644 --- a/aws/resource_aws_kms_external_key_test.go +++ b/aws/resource_aws_kms_external_key_test.go @@ -11,7 +11,9 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" awspolicy "github.com/jen20/awspolicyequivalence" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/finder" "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/waiter" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) func TestAccAWSKmsExternalKey_basic(t *testing.T) { @@ -67,7 +69,7 @@ func TestAccAWSKmsExternalKey_disappears(t *testing.T) { Config: testAccAWSKmsExternalKeyConfig(), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key1), - testAccCheckAWSKmsExternalKeyDisappears(&key1), + testAccCheckResourceDisappears(testAccProvider, resourceAwsKmsExternalKey(), resourceName), ), ExpectNonEmptyPlan: true, }, @@ -431,19 +433,17 @@ func testAccCheckAWSKmsExternalKeyDestroy(s *terraform.State) error { continue } - out, err := conn.DescribeKey(&kms.DescribeKeyInput{ - KeyId: aws.String(rs.Primary.ID), - }) + _, err := finder.KeyByID(conn, rs.Primary.ID) - if err != nil { - return err + if tfresource.NotFound(err) { + continue } - if aws.StringValue(out.KeyMetadata.KeyState) == kms.KeyStatePendingDeletion { - continue + if err != nil { + return err } - return fmt.Errorf("KMS key still exists:\n%#v", out.KeyMetadata) + return fmt.Errorf("KMS External Key %s still exists", rs.Primary.ID) } return nil @@ -457,45 +457,22 @@ func testAccCheckAWSKmsExternalKeyExists(name string, key *kms.KeyMetadata) reso } if rs.Primary.ID == "" { - return fmt.Errorf("No KMS Key ID is set") + return fmt.Errorf("No KMS External Key ID is set") } conn := testAccProvider.Meta().(*AWSClient).kmsconn - o, err := retryOnAwsCode("NotFoundException", func() (interface{}, error) { - return conn.DescribeKey(&kms.DescribeKeyInput{ - KeyId: aws.String(rs.Primary.ID), - }) + outputRaw, err := tfresource.RetryWhenNotFound(waiter.PropagationTimeout, func() (interface{}, error) { + return finder.KeyByID(conn, rs.Primary.ID) }) - if err != nil { - return err - } - out := o.(*kms.DescribeKeyOutput) - - *key = *out.KeyMetadata - - return nil - } -} - -func testAccCheckAWSKmsExternalKeyDisappears(key *kms.KeyMetadata) resource.TestCheckFunc { - return func(s *terraform.State) error { - conn := testAccProvider.Meta().(*AWSClient).kmsconn - - input := &kms.ScheduleKeyDeletionInput{ - KeyId: key.KeyId, - PendingWindowInDays: aws.Int64(int64(7)), - } - - _, err := conn.ScheduleKeyDeletion(input) if err != nil { return err } - _, err = waiter.KeyStatePendingDeletion(conn, aws.StringValue(key.KeyId)) + *key = *(outputRaw.(*kms.KeyMetadata)) - return err + return nil } } diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index 441269c7356..c31cd596012 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -93,8 +93,8 @@ func resourceAwsKmsKey() *schema.Resource { Type: schema.TypeString, Optional: true, Computed: true, - ValidateFunc: validation.StringIsJSON, DiffSuppressFunc: suppressEquivalentAwsPolicyDiffs, + ValidateFunc: validation.StringIsJSON, }, "tags": tagsSchema(), From c3bbe28bd60f84367b6f8f50f6ba2469679bb1c6 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Tue, 27 Jul 2021 13:55:41 -0400 Subject: [PATCH 29/36] Add and use 'findKmsKey'. --- aws/resource_aws_kms_external_key.go | 113 +++++++-------------------- aws/resource_aws_kms_key.go | 106 +++++++++++++------------ 2 files changed, 84 insertions(+), 135 deletions(-) diff --git a/aws/resource_aws_kms_external_key.go b/aws/resource_aws_kms_external_key.go index 4dc3f7f9ecb..590b767efec 100644 --- a/aws/resource_aws_kms_external_key.go +++ b/aws/resource_aws_kms_external_key.go @@ -19,6 +19,7 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags" "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/waiter" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) func resourceAwsKmsExternalKey() *schema.Resource { @@ -170,89 +171,32 @@ func resourceAwsKmsExternalKeyRead(d *schema.ResourceData, meta interface{}) err defaultTagsConfig := meta.(*AWSClient).DefaultTagsConfig ignoreTagsConfig := meta.(*AWSClient).IgnoreTagsConfig - input := &kms.DescribeKeyInput{ - KeyId: aws.String(d.Id()), - } - - var output *kms.DescribeKeyOutput - // Retry for KMS eventual consistency on creation - err := resource.Retry(2*time.Minute, func() *resource.RetryError { - var err error - - output, err = conn.DescribeKey(input) - - if d.IsNewResource() && isAWSErr(err, kms.ErrCodeNotFoundException, "") { - return resource.RetryableError(err) - } - - if err != nil { - return resource.NonRetryableError(err) - } - - return nil - }) + key, err := findKmsKey(conn, d.Id(), d.IsNewResource()) - if isResourceTimeoutError(err) { - output, err = conn.DescribeKey(input) - } - - if !d.IsNewResource() && isAWSErr(err, kms.ErrCodeNotFoundException, "") { + if !d.IsNewResource() && tfresource.NotFound(err) { log.Printf("[WARN] KMS External Key (%s) not found, removing from state", d.Id()) d.SetId("") return nil } if err != nil { - return fmt.Errorf("error describing KMS External Key (%s): %s", d.Id(), err) - } - - if output == nil || output.KeyMetadata == nil { - return fmt.Errorf("error describing KMS External Key (%s): empty response", d.Id()) - } - - metadata := output.KeyMetadata - - if aws.StringValue(metadata.KeyState) == kms.KeyStatePendingDeletion { - log.Printf("[WARN] KMS External Key (%s) not found, removing from state", d.Id()) - d.SetId("") return nil } - getKeyPolicyInput := &kms.GetKeyPolicyInput{ - KeyId: metadata.KeyId, - PolicyName: aws.String("default"), - } - - getKeyPolicyOutput, err := conn.GetKeyPolicy(getKeyPolicyInput) - - if err != nil { - return fmt.Errorf("error getting KMS External Key (%s) policy: %s", d.Id(), err) - } - - if getKeyPolicyOutput == nil { - return fmt.Errorf("error getting KMS External Key (%s) policy: empty response", d.Id()) - } - - policy, err := structure.NormalizeJsonString(aws.StringValue(getKeyPolicyOutput.Policy)) - - if err != nil { - return fmt.Errorf("error normalizing KMS External Key (%s) policy: %s", d.Id(), err) - } - - d.Set("arn", metadata.Arn) - d.Set("description", metadata.Description) - d.Set("enabled", metadata.Enabled) - d.Set("expiration_model", metadata.ExpirationModel) - d.Set("key_state", metadata.KeyState) - d.Set("key_usage", metadata.KeyUsage) - d.Set("policy", policy) - - tags, err := keyvaluetags.KmsListTags(conn, d.Id()) - if err != nil { - return fmt.Errorf("error listing tags for KMS Key (%s): %s", d.Id(), err) + d.Set("arn", key.metadata.Arn) + d.Set("description", key.metadata.Description) + d.Set("enabled", key.metadata.Enabled) + d.Set("expiration_model", key.metadata.ExpirationModel) + d.Set("key_state", key.metadata.KeyState) + d.Set("key_usage", key.metadata.KeyUsage) + d.Set("policy", key.policy) + if key.metadata.ValidTo != nil { + d.Set("valid_to", aws.TimeValue(key.metadata.ValidTo).Format(time.RFC3339)) + } else { + d.Set("valid_to", nil) } - tags = tags.IgnoreAws().IgnoreConfig(ignoreTagsConfig) + tags := key.tags.IgnoreAws().IgnoreConfig(ignoreTagsConfig) //lintignore:AWSR002 if err := d.Set("tags", tags.RemoveDefaultConfig(defaultTagsConfig).Map()); err != nil { @@ -263,20 +207,15 @@ func resourceAwsKmsExternalKeyRead(d *schema.ResourceData, meta interface{}) err return fmt.Errorf("error setting tags_all: %w", err) } - d.Set("valid_to", "") - if metadata.ValidTo != nil { - d.Set("valid_to", aws.TimeValue(metadata.ValidTo).Format(time.RFC3339)) - } - return nil } func resourceAwsKmsExternalKeyUpdate(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).kmsconn - if d.HasChange("enabled") && d.Get("enabled").(bool) && d.Get("key_state") != kms.KeyStatePendingImport { - // Enable before any attributes will be modified - if err := updateKmsKeyStatus(conn, d.Id(), d.Get("enabled").(bool)); err != nil { + if hasChange, enabled, state := d.HasChange("enabled"), d.Get("enabled").(bool), d.Get("key_state").(string); hasChange && enabled && state != kms.KeyStatePendingImport { + // Enable before any attributes are modified. + if err := updateKmsKeyEnabled(conn, d.Id(), enabled); err != nil { return err } } @@ -287,8 +226,11 @@ func resourceAwsKmsExternalKeyUpdate(d *schema.ResourceData, meta interface{}) e KeyId: aws.String(d.Id()), } - if _, err := conn.UpdateKeyDescription(input); err != nil { - return fmt.Errorf("error updating KMS External Key (%s) description: %s", d.Id(), err) + log.Printf("[DEBUG] Updating KMS External Key description: %s", input) + _, err := conn.UpdateKeyDescription(input) + + if err != nil { + return fmt.Errorf("error updating KMS External Key (%s) description: %w", d.Id(), err) } } @@ -316,10 +258,9 @@ func resourceAwsKmsExternalKeyUpdate(d *schema.ResourceData, meta interface{}) e } } - if d.HasChange("enabled") && !d.Get("enabled").(bool) && d.Get("key_state") != kms.KeyStatePendingImport { - // Only disable when all attributes are modified - // because we cannot modify disabled keys - if err := updateKmsKeyStatus(conn, d.Id(), d.Get("enabled").(bool)); err != nil { + if hasChange, enabled, state := d.HasChange("enabled"), d.Get("enabled").(bool), d.Get("key_state").(string); hasChange && !enabled && state != kms.KeyStatePendingImport { + // Only disable after all attributes have been modified because we cannot modify disabled keys. + if err := updateKmsKeyEnabled(conn, d.Id(), enabled); err != nil { return err } } @@ -328,7 +269,7 @@ func resourceAwsKmsExternalKeyUpdate(d *schema.ResourceData, meta interface{}) e o, n := d.GetChange("tags_all") if err := keyvaluetags.KmsUpdateTags(conn, d.Id(), o, n); err != nil { - return fmt.Errorf("error updating KMS External Key (%s) tags: %s", d.Id(), err) + return fmt.Errorf("error updating KMS External Key (%s) tags: %w", d.Id(), err) } } diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index c31cd596012..a3ce08b4e8b 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -176,53 +176,7 @@ func resourceAwsKmsKeyRead(d *schema.ResourceData, meta interface{}) error { defaultTagsConfig := meta.(*AWSClient).DefaultTagsConfig ignoreTagsConfig := meta.(*AWSClient).IgnoreTagsConfig - type Key struct { - metadata *kms.KeyMetadata - policy string - rotation *bool - tags keyvaluetags.KeyValueTags - } - - outputRaw, err := tfresource.RetryWhenNewResourceNotFound(waiter.PropagationTimeout, func() (interface{}, error) { - var err error - var key Key - - key.metadata, err = finder.KeyByID(conn, d.Id()) - - if err != nil { - return nil, fmt.Errorf("error reading KMS Key (%s): %w", d.Id(), err) - } - - policy, err := finder.KeyPolicyByKeyIDAndPolicyName(conn, d.Id(), tfkms.PolicyNameDefault) - - if err != nil { - return nil, fmt.Errorf("error reading KMS Key (%s) policy: %w", d.Id(), err) - } - - key.policy, err = structure.NormalizeJsonString(aws.StringValue(policy)) - - if err != nil { - return nil, fmt.Errorf("policy contains invalid JSON: %w", err) - } - - key.rotation, err = finder.KeyRotationEnabledByKeyID(conn, d.Id()) - - if err != nil { - return nil, fmt.Errorf("error reading KMS Key (%s) rotation enabled: %w", d.Id(), err) - } - - key.tags, err = keyvaluetags.KmsListTags(conn, d.Id()) - - if tfawserr.ErrCodeEquals(err, kms.ErrCodeNotFoundException) { - return nil, &resource.NotFoundError{LastError: err} - } - - if err != nil { - return nil, fmt.Errorf("error listing tags for KMS Key (%s): %w", d.Id(), err) - } - - return &key, nil - }, d.IsNewResource()) + key, err := findKmsKey(conn, d.Id(), d.IsNewResource()) if !d.IsNewResource() && tfresource.NotFound(err) { log.Printf("[WARN] KMS Key (%s) not found, removing from state", d.Id()) @@ -234,8 +188,6 @@ func resourceAwsKmsKeyRead(d *schema.ResourceData, meta interface{}) error { return nil } - key := outputRaw.(*Key) - d.Set("arn", key.metadata.Arn) d.Set("customer_master_key_spec", key.metadata.CustomerMasterKeySpec) d.Set("description", key.metadata.Description) @@ -350,6 +302,62 @@ func resourceAwsKmsKeyDelete(d *schema.ResourceData, meta interface{}) error { return nil } +type kmsKey struct { + metadata *kms.KeyMetadata + policy string + rotation *bool + tags keyvaluetags.KeyValueTags +} + +func findKmsKey(conn *kms.KMS, keyID string, isNewResource bool) (*kmsKey, error) { + outputRaw, err := tfresource.RetryWhenNewResourceNotFound(waiter.PropagationTimeout, func() (interface{}, error) { + var err error + var key kmsKey + + key.metadata, err = finder.KeyByID(conn, keyID) + + if err != nil { + return nil, fmt.Errorf("error reading KMS Key (%s): %w", keyID, err) + } + + policy, err := finder.KeyPolicyByKeyIDAndPolicyName(conn, keyID, tfkms.PolicyNameDefault) + + if err != nil { + return nil, fmt.Errorf("error reading KMS Key (%s) policy: %w", keyID, err) + } + + key.policy, err = structure.NormalizeJsonString(aws.StringValue(policy)) + + if err != nil { + return nil, fmt.Errorf("policy contains invalid JSON: %w", err) + } + + key.rotation, err = finder.KeyRotationEnabledByKeyID(conn, keyID) + + if err != nil { + return nil, fmt.Errorf("error reading KMS Key (%s) rotation enabled: %w", keyID, err) + } + + key.tags, err = keyvaluetags.KmsListTags(conn, keyID) + + if tfawserr.ErrCodeEquals(err, kms.ErrCodeNotFoundException) { + return nil, &resource.NotFoundError{LastError: err} + } + + if err != nil { + return nil, fmt.Errorf("error listing tags for KMS Key (%s): %w", keyID, err) + } + + return &key, nil + }, isNewResource) + + if err != nil { + return nil, err + } + + return outputRaw.(*kmsKey), nil +} + func updateKmsKeyEnabled(conn *kms.KMS, keyID string, enabled bool) error { updateFunc := func() (interface{}, error) { var err error From 2c10ec10a05a732bd7a41370efda4334b9921022 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Tue, 27 Jul 2021 15:37:36 -0400 Subject: [PATCH 30/36] r/aws_kms_external_key: Add `bypass_policy_lockout_safety_check` argument. Acceptance test output: % make testacc TESTARGS='-run=TestAccAWSKmsExternalKey_' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSKmsExternalKey_ -timeout 180m === RUN TestAccAWSKmsExternalKey_basic === PAUSE TestAccAWSKmsExternalKey_basic === RUN TestAccAWSKmsExternalKey_disappears === PAUSE TestAccAWSKmsExternalKey_disappears === RUN TestAccAWSKmsExternalKey_DeletionWindowInDays === PAUSE TestAccAWSKmsExternalKey_DeletionWindowInDays === RUN TestAccAWSKmsExternalKey_Description === PAUSE TestAccAWSKmsExternalKey_Description === RUN TestAccAWSKmsExternalKey_Enabled === PAUSE TestAccAWSKmsExternalKey_Enabled === RUN TestAccAWSKmsExternalKey_KeyMaterialBase64 === PAUSE TestAccAWSKmsExternalKey_KeyMaterialBase64 === RUN TestAccAWSKmsExternalKey_Policy === PAUSE TestAccAWSKmsExternalKey_Policy === RUN TestAccAWSKmsExternalKey_PolicyBypass === PAUSE TestAccAWSKmsExternalKey_PolicyBypass === RUN TestAccAWSKmsExternalKey_Tags === PAUSE TestAccAWSKmsExternalKey_Tags === RUN TestAccAWSKmsExternalKey_ValidTo === PAUSE TestAccAWSKmsExternalKey_ValidTo === CONT TestAccAWSKmsExternalKey_basic === CONT TestAccAWSKmsExternalKey_Policy === CONT TestAccAWSKmsExternalKey_Description === CONT TestAccAWSKmsExternalKey_Enabled === CONT TestAccAWSKmsExternalKey_disappears === CONT TestAccAWSKmsExternalKey_ValidTo === CONT TestAccAWSKmsExternalKey_Tags === CONT TestAccAWSKmsExternalKey_PolicyBypass === CONT TestAccAWSKmsExternalKey_KeyMaterialBase64 === CONT TestAccAWSKmsExternalKey_DeletionWindowInDays --- PASS: TestAccAWSKmsExternalKey_disappears (17.41s) --- PASS: TestAccAWSKmsExternalKey_basic (26.35s) --- PASS: TestAccAWSKmsExternalKey_PolicyBypass (26.47s) --- PASS: TestAccAWSKmsExternalKey_Description (38.35s) --- PASS: TestAccAWSKmsExternalKey_DeletionWindowInDays (38.49s) --- PASS: TestAccAWSKmsExternalKey_Policy (42.59s) --- PASS: TestAccAWSKmsExternalKey_Tags (50.91s) --- PASS: TestAccAWSKmsExternalKey_KeyMaterialBase64 (108.03s) --- PASS: TestAccAWSKmsExternalKey_ValidTo (179.40s) --- PASS: TestAccAWSKmsExternalKey_Enabled (205.79s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 210.650s --- .changelog/18117.txt | 4 ++ aws/resource_aws_kms_external_key.go | 35 ++++----- aws/resource_aws_kms_external_key_test.go | 71 +++++++++++++++++-- aws/resource_aws_kms_key.go | 8 ++- website/docs/r/kms_external_key.html.markdown | 1 + 5 files changed, 88 insertions(+), 31 deletions(-) diff --git a/.changelog/18117.txt b/.changelog/18117.txt index f67d67f234a..bbbc8e18b60 100644 --- a/.changelog/18117.txt +++ b/.changelog/18117.txt @@ -1,3 +1,7 @@ ```release-note:enhancement resource/aws_kms_key: Add `bypass_policy_lockout_safety_check` argument ``` + +```release-note:enhancement +resource/aws_kms_external_key: Add `bypass_policy_lockout_safety_check` argument +``` diff --git a/aws/resource_aws_kms_external_key.go b/aws/resource_aws_kms_external_key.go index 590b767efec..2eb603427a6 100644 --- a/aws/resource_aws_kms_external_key.go +++ b/aws/resource_aws_kms_external_key.go @@ -15,7 +15,6 @@ import ( "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags" "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/waiter" @@ -41,12 +40,11 @@ func resourceAwsKmsExternalKey() *schema.Resource { Computed: true, }, - // TODO - // "bypass_policy_lockout_safety_check": { - // Type: schema.TypeBool, - // Optional: true, - // Default: false, - // }, + "bypass_policy_lockout_safety_check": { + Type: schema.TypeBool, + Optional: true, + Default: false, + }, "deletion_window_in_days": { Type: schema.TypeInt, @@ -118,8 +116,9 @@ func resourceAwsKmsExternalKeyCreate(d *schema.ResourceData, meta interface{}) e tags := defaultTagsConfig.MergeTags(keyvaluetags.New(d.Get("tags").(map[string]interface{}))) input := &kms.CreateKeyInput{ - KeyUsage: aws.String(kms.KeyUsageTypeEncryptDecrypt), - Origin: aws.String(kms.OriginTypeExternal), + BypassPolicyLockoutSafetyCheck: aws.Bool(d.Get("bypass_policy_lockout_safety_check").(bool)), + KeyUsage: aws.String(kms.KeyUsageTypeEncryptDecrypt), + Origin: aws.String(kms.OriginTypeExternal), } if v, ok := d.GetOk("description"); ok { @@ -183,6 +182,8 @@ func resourceAwsKmsExternalKeyRead(d *schema.ResourceData, meta interface{}) err return nil } + log.Printf("[WARN] kmsKey: %v", key) + d.Set("arn", key.metadata.Arn) d.Set("description", key.metadata.Description) d.Set("enabled", key.metadata.Enabled) @@ -235,20 +236,8 @@ func resourceAwsKmsExternalKeyUpdate(d *schema.ResourceData, meta interface{}) e } if d.HasChange("policy") { - policy, err := structure.NormalizeJsonString(d.Get("policy").(string)) - - if err != nil { - return fmt.Errorf("error parsing KMS External Key (%s) policy JSON: %s", d.Id(), err) - } - - input := &kms.PutKeyPolicyInput{ - KeyId: aws.String(d.Id()), - Policy: aws.String(policy), - PolicyName: aws.String("default"), - } - - if _, err := conn.PutKeyPolicy(input); err != nil { - return fmt.Errorf("error updating KMS External Key (%s) policy: %s", d.Id(), err) + if err := updateKmsKeyPolicy(conn, d.Id(), d.Get("policy").(string), d.Get("bypass_policy_lockout_safety_check").(bool)); err != nil { + return err } } diff --git a/aws/resource_aws_kms_external_key_test.go b/aws/resource_aws_kms_external_key_test.go index c4748c2d035..c2834356085 100644 --- a/aws/resource_aws_kms_external_key_test.go +++ b/aws/resource_aws_kms_external_key_test.go @@ -8,6 +8,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/kms" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" awspolicy "github.com/jen20/awspolicyequivalence" @@ -31,6 +32,7 @@ func TestAccAWSKmsExternalKey_basic(t *testing.T) { Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key1), testAccMatchResourceAttrRegionalARN(resourceName, "arn", "kms", regexp.MustCompile(`key/.+`)), + resource.TestCheckResourceAttr(resourceName, "bypass_policy_lockout_safety_check", "false"), resource.TestCheckResourceAttr(resourceName, "deletion_window_in_days", "30"), resource.TestCheckResourceAttr(resourceName, "enabled", "false"), resource.TestCheckResourceAttr(resourceName, "expiration_model", ""), @@ -47,6 +49,7 @@ func TestAccAWSKmsExternalKey_basic(t *testing.T) { ImportState: true, ImportStateVerify: true, ImportStateVerifyIgnore: []string{ + "bypass_policy_lockout_safety_check", "deletion_window_in_days", "key_material_base64", }, @@ -99,6 +102,7 @@ func TestAccAWSKmsExternalKey_DeletionWindowInDays(t *testing.T) { ImportState: true, ImportStateVerify: true, ImportStateVerifyIgnore: []string{ + "bypass_policy_lockout_safety_check", "deletion_window_in_days", "key_material_base64", }, @@ -137,6 +141,7 @@ func TestAccAWSKmsExternalKey_Description(t *testing.T) { ImportState: true, ImportStateVerify: true, ImportStateVerifyIgnore: []string{ + "bypass_policy_lockout_safety_check", "deletion_window_in_days", "key_material_base64", }, @@ -175,6 +180,7 @@ func TestAccAWSKmsExternalKey_Enabled(t *testing.T) { ImportState: true, ImportStateVerify: true, ImportStateVerifyIgnore: []string{ + "bypass_policy_lockout_safety_check", "deletion_window_in_days", "key_material_base64", }, @@ -222,6 +228,7 @@ func TestAccAWSKmsExternalKey_KeyMaterialBase64(t *testing.T) { ImportState: true, ImportStateVerify: true, ImportStateVerifyIgnore: []string{ + "bypass_policy_lockout_safety_check", "deletion_window_in_days", "key_material_base64", }, @@ -241,6 +248,7 @@ func TestAccAWSKmsExternalKey_KeyMaterialBase64(t *testing.T) { func TestAccAWSKmsExternalKey_Policy(t *testing.T) { var key1, key2 kms.KeyMetadata + rName := acctest.RandomWithPrefix("tf-acc-test") policy1 := `{"Version":"2012-10-17","Id":"kms-tf-1","Statement":[{"Sid":"Enable IAM User Permissions 1","Effect":"Allow","Principal":{"AWS":"*"},"Action":"kms:*","Resource":"*"}]}` policy2 := `{"Version":"2012-10-17","Id":"kms-tf-1","Statement":[{"Sid":"Enable IAM User Permissions 2","Effect":"Allow","Principal":{"AWS":"*"},"Action":"kms:*","Resource":"*"}]}` resourceName := "aws_kms_external_key.test" @@ -252,7 +260,7 @@ func TestAccAWSKmsExternalKey_Policy(t *testing.T) { CheckDestroy: testAccCheckAWSKmsExternalKeyDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSKmsExternalKeyConfigPolicy(policy1), + Config: testAccAWSKmsExternalKeyConfigPolicy(rName, policy1), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key1), testAccCheckAWSKmsExternalKeyHasPolicy(resourceName, policy1), @@ -263,12 +271,13 @@ func TestAccAWSKmsExternalKey_Policy(t *testing.T) { ImportState: true, ImportStateVerify: true, ImportStateVerifyIgnore: []string{ + "bypass_policy_lockout_safety_check", "deletion_window_in_days", "key_material_base64", }, }, { - Config: testAccAWSKmsExternalKeyConfigPolicy(policy2), + Config: testAccAWSKmsExternalKeyConfigPolicy(rName, policy2), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key2), testAccCheckAWSKmsExternalKeyNotRecreated(&key1, &key2), @@ -279,6 +288,40 @@ func TestAccAWSKmsExternalKey_Policy(t *testing.T) { }) } +func TestAccAWSKmsExternalKey_PolicyBypass(t *testing.T) { + var key kms.KeyMetadata + rName := acctest.RandomWithPrefix("tf-acc-test") + policy := `{"Version":"2012-10-17","Id":"kms-tf-1","Statement":[{"Sid":"Enable IAM User Permissions 1","Effect":"Allow","Principal":{"AWS":"*"},"Action":"kms:*","Resource":"*"}]}` + resourceName := "aws_kms_external_key.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ErrorCheck: testAccErrorCheck(t, kms.EndpointsID), + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSKmsExternalKeyDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSKmsExternalKeyConfigPolicyBypass(rName, policy), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSKmsExternalKeyExists(resourceName, &key), + testAccCheckAWSKmsExternalKeyHasPolicy(resourceName, policy), + resource.TestCheckResourceAttr(resourceName, "bypass_policy_lockout_safety_check", "true"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{ + "bypass_policy_lockout_safety_check", + "deletion_window_in_days", + "key_material_base64", + }, + }, + }, + }) +} + func TestAccAWSKmsExternalKey_Tags(t *testing.T) { var key1, key2, key3 kms.KeyMetadata resourceName := "aws_kms_external_key.test" @@ -302,6 +345,7 @@ func TestAccAWSKmsExternalKey_Tags(t *testing.T) { ImportState: true, ImportStateVerify: true, ImportStateVerifyIgnore: []string{ + "bypass_policy_lockout_safety_check", "deletion_window_in_days", "key_material_base64", }, @@ -354,6 +398,7 @@ func TestAccAWSKmsExternalKey_ValidTo(t *testing.T) { ImportState: true, ImportStateVerify: true, ImportStateVerifyIgnore: []string{ + "bypass_policy_lockout_safety_check", "deletion_window_in_days", "key_material_base64", }, @@ -539,16 +584,32 @@ resource "aws_kms_external_key" "test" { `, keyMaterialBase64) } -func testAccAWSKmsExternalKeyConfigPolicy(policy string) string { +func testAccAWSKmsExternalKeyConfigPolicy(rName, policy string) string { + return fmt.Sprintf(` +resource "aws_kms_external_key" "test" { + description = %[1]q + deletion_window_in_days = 7 + + policy = < Date: Tue, 27 Jul 2021 17:57:56 -0400 Subject: [PATCH 31/36] r/aws_kms_key: Test completely empty configuration. --- aws/resource_aws_kms_key_test.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/aws/resource_aws_kms_key_test.go b/aws/resource_aws_kms_key_test.go index 005523ddc00..282c1f2e0dd 100644 --- a/aws/resource_aws_kms_key_test.go +++ b/aws/resource_aws_kms_key_test.go @@ -76,7 +76,6 @@ func testSweepKmsKeys(region string) error { func TestAccAWSKmsKey_basic(t *testing.T) { var key kms.KeyMetadata - rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_key.test" resource.ParallelTest(t, resource.TestCase{ @@ -86,7 +85,7 @@ func TestAccAWSKmsKey_basic(t *testing.T) { CheckDestroy: testAccCheckAWSKmsKeyDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSKmsKey(rName), + Config: testAccAWSKmsKeyConfig(), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsKeyExists(resourceName, &key), resource.TestCheckResourceAttr(resourceName, "customer_master_key_spec", "SYMMETRIC_DEFAULT"), @@ -139,7 +138,7 @@ func TestAccAWSKmsKey_disappears(t *testing.T) { CheckDestroy: testAccCheckAWSKmsKeyDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSKmsKey(rName), + Config: testAccAWSKmsKeyConfigName(rName), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsKeyExists(resourceName, &key), testAccCheckResourceDisappears(testAccProvider, resourceAwsKmsKey(), resourceName), @@ -227,7 +226,7 @@ func TestAccAWSKmsKey_policyBypassUpdate(t *testing.T) { CheckDestroy: testAccCheckAWSKmsKeyDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSKmsKey(rName), + Config: testAccAWSKmsKeyConfigName(rName), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsKeyExists(resourceName, &before), resource.TestCheckResourceAttr(resourceName, "bypass_policy_lockout_safety_check", "false"), @@ -490,7 +489,13 @@ func testAccCheckAWSKmsKeyIsEnabled(key *kms.KeyMetadata, isEnabled bool) resour } } -func testAccAWSKmsKey(rName string) string { +func testAccAWSKmsKeyConfig() string { + return ` +resource "aws_kms_key" "test" {} +` +} + +func testAccAWSKmsKeyConfigName(rName string) string { return fmt.Sprintf(` resource "aws_kms_key" "test" { description = %[1]q From dfc1480e888a9a294a20b99be91c157214935fd9 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Tue, 27 Jul 2021 17:58:30 -0400 Subject: [PATCH 32/36] r/aws_kms_external_key: Add 'waiter.KeyMaterialImported'. --- aws/internal/service/kms/waiter/waiter.go | 22 ++- aws/resource_aws_kms_external_key.go | 230 ++++------------------ aws/resource_aws_kms_external_key_test.go | 111 ++++++----- 3 files changed, 115 insertions(+), 248 deletions(-) diff --git a/aws/internal/service/kms/waiter/waiter.go b/aws/internal/service/kms/waiter/waiter.go index f677ad07305..89dee2ff0d2 100644 --- a/aws/internal/service/kms/waiter/waiter.go +++ b/aws/internal/service/kms/waiter/waiter.go @@ -20,6 +20,7 @@ const ( KeyStatePendingDeletionTimeout = 20 * time.Minute KeyDeletedTimeout = 20 * time.Minute + KeyMaterialImportedTimeout = 10 * time.Minute KeyRotationUpdatedTimeout = 10 * time.Minute KeyStatePropagationTimeout = 20 * time.Minute @@ -49,6 +50,23 @@ func KeyDeleted(conn *kms.KMS, id string) (*kms.KeyMetadata, error) { return nil, err } +func KeyMaterialImported(conn *kms.KMS, id string) (*kms.KeyMetadata, error) { + stateConf := &resource.StateChangeConf{ + Pending: []string{kms.KeyStatePendingImport}, + Target: []string{kms.KeyStateDisabled, kms.KeyStateEnabled}, + Refresh: KeyState(conn, id), + Timeout: KeyMaterialImportedTimeout, + } + + outputRaw, err := stateConf.WaitForState() + + if output, ok := outputRaw.(*kms.KeyMetadata); ok { + return output, err + } + + return nil, err +} + func KeyPolicyPropagated(conn *kms.KMS, id, policy string) error { checkFunc := func() (bool, error) { output, err := finder.KeyPolicyByKeyIDAndPolicyName(conn, id, tfkms.PolicyNameDefault) @@ -70,7 +88,7 @@ func KeyPolicyPropagated(conn *kms.KMS, id, policy string) error { return equivalent, nil } opts := tfresource.WaitOpts{ - ContinuousTargetOccurence: 3, + ContinuousTargetOccurence: 5, MinTimeout: 1 * time.Second, } @@ -136,7 +154,7 @@ func TagsPropagated(conn *kms.KMS, id string, tags keyvaluetags.KeyValueTags) er return output.Equal(tags), nil } opts := tfresource.WaitOpts{ - ContinuousTargetOccurence: 3, + ContinuousTargetOccurence: 5, MinTimeout: 1 * time.Second, } diff --git a/aws/resource_aws_kms_external_key.go b/aws/resource_aws_kms_external_key.go index 2eb603427a6..6135c26b7db 100644 --- a/aws/resource_aws_kms_external_key.go +++ b/aws/resource_aws_kms_external_key.go @@ -13,7 +13,6 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/kms" "github.com/hashicorp/aws-sdk-go-base/tfawserr" - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags" @@ -155,6 +154,10 @@ func resourceAwsKmsExternalKeyCreate(d *schema.ResourceData, meta interface{}) e return fmt.Errorf("error importing KMS External Key (%s) material: %s", d.Id(), err) } + if _, err := waiter.KeyMaterialImported(conn, d.Id()); err != nil { + return fmt.Errorf("error waiting for KMS External Key (%s) material import: %w", d.Id(), err) + } + if enabled := d.Get("enabled").(bool); !enabled { if err := updateKmsKeyEnabled(conn, d.Id(), enabled); err != nil { return err @@ -182,8 +185,6 @@ func resourceAwsKmsExternalKeyRead(d *schema.ResourceData, meta interface{}) err return nil } - log.Printf("[WARN] kmsKey: %v", key) - d.Set("arn", key.metadata.Arn) d.Set("description", key.metadata.Description) d.Set("enabled", key.metadata.Enabled) @@ -245,6 +246,10 @@ func resourceAwsKmsExternalKeyUpdate(d *schema.ResourceData, meta interface{}) e if err := importKmsExternalKeyMaterial(conn, d.Id(), d.Get("key_material_base64").(string), d.Get("valid_to").(string)); err != nil { return fmt.Errorf("error importing KMS External Key (%s) material: %s", d.Id(), err) } + + if _, err := waiter.KeyMaterialImported(conn, d.Id()); err != nil { + return fmt.Errorf("error waiting for KMS External Key (%s) material import: %w", d.Id(), err) + } } if hasChange, enabled, state := d.HasChange("enabled"), d.Get("enabled").(bool), d.Get("key_state").(string); hasChange && !enabled && state != kms.KeyStatePendingImport { @@ -260,6 +265,10 @@ func resourceAwsKmsExternalKeyUpdate(d *schema.ResourceData, meta interface{}) e if err := keyvaluetags.KmsUpdateTags(conn, d.Id(), o, n); err != nil { return fmt.Errorf("error updating KMS External Key (%s) tags: %w", d.Id(), err) } + + if err := waiter.TagsPropagated(conn, d.Id(), keyvaluetags.New(n)); err != nil { + return fmt.Errorf("error waiting for KMS External Key (%s) tag propagation: %w", d.Id(), err) + } } return resourceAwsKmsExternalKeyRead(d, meta) @@ -299,64 +308,42 @@ func resourceAwsKmsExternalKeyDelete(d *schema.ResourceData, meta interface{}) e } func importKmsExternalKeyMaterial(conn *kms.KMS, keyID, keyMaterialBase64, validTo string) error { - getParametersForImportInput := &kms.GetParametersForImportInput{ - KeyId: aws.String(keyID), - WrappingAlgorithm: aws.String(kms.AlgorithmSpecRsaesOaepSha256), - WrappingKeySpec: aws.String(kms.WrappingKeySpecRsa2048), - } - - var getParametersForImportOutput *kms.GetParametersForImportOutput - // Handle KMS eventual consistency - err := resource.Retry(1*time.Minute, func() *resource.RetryError { - var err error - - getParametersForImportOutput, err = conn.GetParametersForImport(getParametersForImportInput) - - if isAWSErr(err, kms.ErrCodeNotFoundException, "") { - return resource.RetryableError(err) - } - - if err != nil { - return resource.NonRetryableError(err) - } - - return nil - }) - - if isResourceTimeoutError(err) { - getParametersForImportOutput, err = conn.GetParametersForImport(getParametersForImportInput) - } + outputRaw, err := tfresource.RetryWhenAwsErrCodeEquals(waiter.PropagationTimeout, func() (interface{}, error) { + return conn.GetParametersForImport(&kms.GetParametersForImportInput{ + KeyId: aws.String(keyID), + WrappingAlgorithm: aws.String(kms.AlgorithmSpecRsaesOaepSha256), + WrappingKeySpec: aws.String(kms.WrappingKeySpecRsa2048), + }) + }, kms.ErrCodeNotFoundException) if err != nil { - return fmt.Errorf("error getting parameters for import: %s", err) + return fmt.Errorf("error getting parameters for import: %w", err) } - if getParametersForImportOutput == nil { - return fmt.Errorf("error getting parameters for import: empty response") - } + output := outputRaw.(*kms.GetParametersForImportOutput) keyMaterial, err := base64.StdEncoding.DecodeString(keyMaterialBase64) if err != nil { - return fmt.Errorf("error Base64 decoding key material: %s", err) + return fmt.Errorf("error Base64 decoding key material: %w", err) } - publicKey, err := x509.ParsePKIXPublicKey(getParametersForImportOutput.PublicKey) + publicKey, err := x509.ParsePKIXPublicKey(output.PublicKey) if err != nil { - return fmt.Errorf("error parsing public key: %s", err) + return fmt.Errorf("error parsing public key: %w", err) } encryptedKeyMaterial, err := rsa.EncryptOAEP(sha256.New(), rand.Reader, publicKey.(*rsa.PublicKey), keyMaterial, []byte{}) if err != nil { - return fmt.Errorf("error encrypting key material: %s", err) + return fmt.Errorf("error encrypting key material: %w", err) } - importKeyMaterialInput := &kms.ImportKeyMaterialInput{ + input := &kms.ImportKeyMaterialInput{ EncryptedKeyMaterial: encryptedKeyMaterial, ExpirationModel: aws.String(kms.ExpirationModelTypeKeyMaterialDoesNotExpire), - ImportToken: getParametersForImportOutput.ImportToken, + ImportToken: output.ImportToken, KeyId: aws.String(keyID), } @@ -364,169 +351,20 @@ func importKmsExternalKeyMaterial(conn *kms.KMS, keyID, keyMaterialBase64, valid t, err := time.Parse(time.RFC3339, validTo) if err != nil { - return fmt.Errorf("error parsing valid to timestamp: %s", err) + return fmt.Errorf("error parsing valid_to timestamp: %w", err) } - importKeyMaterialInput.ExpirationModel = aws.String(kms.ExpirationModelTypeKeyMaterialExpires) - importKeyMaterialInput.ValidTo = aws.Time(t) + input.ExpirationModel = aws.String(kms.ExpirationModelTypeKeyMaterialExpires) + input.ValidTo = aws.Time(t) } - // Handle KMS eventual consistency - err = resource.Retry(1*time.Minute, func() *resource.RetryError { - _, err := conn.ImportKeyMaterial(importKeyMaterialInput) - - if isAWSErr(err, kms.ErrCodeNotFoundException, "") { - return resource.RetryableError(err) - } - - if err != nil { - return resource.NonRetryableError(err) - } - - return nil - }) - - if isResourceTimeoutError(err) { - _, err = conn.ImportKeyMaterial(importKeyMaterialInput) - } + _, err = tfresource.RetryWhenAwsErrCodeEquals(waiter.PropagationTimeout, func() (interface{}, error) { + return conn.ImportKeyMaterial(input) + }, kms.ErrCodeNotFoundException) if err != nil { - return fmt.Errorf("error importing key material: %s", err) + return fmt.Errorf("error importing key material: %w", err) } return nil } - -func updateKmsKeyStatus(conn *kms.KMS, id string, shouldBeEnabled bool) error { - var err error - - if shouldBeEnabled { - log.Printf("[DEBUG] Enabling KMS key %q", id) - _, err = conn.EnableKey(&kms.EnableKeyInput{ - KeyId: aws.String(id), - }) - } else { - log.Printf("[DEBUG] Disabling KMS key %q", id) - _, err = conn.DisableKey(&kms.DisableKeyInput{ - KeyId: aws.String(id), - }) - } - - if err != nil { - return fmt.Errorf("Failed to set KMS key %q status to %t: %q", - id, shouldBeEnabled, err.Error()) - } - - // Wait for propagation since KMS is eventually consistent - wait := resource.StateChangeConf{ - Pending: []string{fmt.Sprintf("%t", !shouldBeEnabled)}, - Target: []string{fmt.Sprintf("%t", shouldBeEnabled)}, - Timeout: 20 * time.Minute, - MinTimeout: 2 * time.Second, - ContinuousTargetOccurence: 15, - Refresh: func() (interface{}, string, error) { - log.Printf("[DEBUG] Checking if KMS key %s enabled status is %t", - id, shouldBeEnabled) - resp, err := conn.DescribeKey(&kms.DescribeKeyInput{ - KeyId: aws.String(id), - }) - if err != nil { - if isAWSErr(err, kms.ErrCodeNotFoundException, "") { - return nil, fmt.Sprintf("%t", !shouldBeEnabled), nil - } - return resp, "FAILED", err - } - status := fmt.Sprintf("%t", *resp.KeyMetadata.Enabled) - log.Printf("[DEBUG] KMS key %s status received: %s, retrying", id, status) - - return resp, status, nil - }, - } - - _, err = wait.WaitForState() - if err != nil { - return fmt.Errorf("Failed setting KMS key status to %t: %w", shouldBeEnabled, err) - } - - return nil -} - -func updateKmsKeyRotationStatus(conn *kms.KMS, d *schema.ResourceData) error { - shouldEnableRotation := d.Get("enable_key_rotation").(bool) - - err := resource.Retry(10*time.Minute, func() *resource.RetryError { - err := handleKeyRotation(conn, shouldEnableRotation, aws.String(d.Id())) - - if err != nil { - if isAWSErr(err, kms.ErrCodeDisabledException, "") { - return resource.RetryableError(err) - } - if isAWSErr(err, kms.ErrCodeNotFoundException, "") { - return resource.RetryableError(err) - } - - return resource.NonRetryableError(err) - } - - return nil - }) - if isResourceTimeoutError(err) { - err = handleKeyRotation(conn, shouldEnableRotation, aws.String(d.Id())) - } - - if err != nil { - return fmt.Errorf("Failed to set key rotation for %q to %t: %q", - d.Id(), shouldEnableRotation, err.Error()) - } - - // Wait for propagation since KMS is eventually consistent - wait := resource.StateChangeConf{ - Pending: []string{fmt.Sprintf("%t", !shouldEnableRotation)}, - Target: []string{fmt.Sprintf("%t", shouldEnableRotation)}, - Timeout: 5 * time.Minute, - MinTimeout: 1 * time.Second, - ContinuousTargetOccurence: 5, - Refresh: func() (interface{}, string, error) { - log.Printf("[DEBUG] Checking if KMS key %s rotation status is %t", - d.Id(), shouldEnableRotation) - - out, err := retryOnAwsCode(kms.ErrCodeNotFoundException, func() (interface{}, error) { - return conn.GetKeyRotationStatus(&kms.GetKeyRotationStatusInput{ - KeyId: aws.String(d.Id()), - }) - }) - if err != nil { - return 42, "", err - } - resp, _ := out.(*kms.GetKeyRotationStatusOutput) - - status := fmt.Sprintf("%t", *resp.KeyRotationEnabled) - log.Printf("[DEBUG] KMS key %s rotation status received: %s, retrying", d.Id(), status) - - return resp, status, nil - }, - } - - _, err = wait.WaitForState() - if err != nil { - return fmt.Errorf("Failed setting KMS key rotation status to %t: %s", shouldEnableRotation, err) - } - - return nil -} - -func handleKeyRotation(conn *kms.KMS, shouldEnableRotation bool, keyId *string) error { - var err error - if shouldEnableRotation { - log.Printf("[DEBUG] Enabling key rotation for KMS key %q", *keyId) - _, err = conn.EnableKeyRotation(&kms.EnableKeyRotationInput{ - KeyId: keyId, - }) - } else { - log.Printf("[DEBUG] Disabling key rotation for KMS key %q", *keyId) - _, err = conn.DisableKeyRotation(&kms.DisableKeyRotationInput{ - KeyId: keyId, - }) - } - return err -} diff --git a/aws/resource_aws_kms_external_key_test.go b/aws/resource_aws_kms_external_key_test.go index c2834356085..92e0f78762a 100644 --- a/aws/resource_aws_kms_external_key_test.go +++ b/aws/resource_aws_kms_external_key_test.go @@ -12,13 +12,14 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" awspolicy "github.com/jen20/awspolicyequivalence" + tfkms "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms" "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/finder" "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/kms/waiter" "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) func TestAccAWSKmsExternalKey_basic(t *testing.T) { - var key1 kms.KeyMetadata + var key kms.KeyMetadata resourceName := "aws_kms_external_key.test" resource.ParallelTest(t, resource.TestCase{ @@ -30,7 +31,7 @@ func TestAccAWSKmsExternalKey_basic(t *testing.T) { { Config: testAccAWSKmsExternalKeyConfig(), Check: resource.ComposeTestCheckFunc( - testAccCheckAWSKmsExternalKeyExists(resourceName, &key1), + testAccCheckAWSKmsExternalKeyExists(resourceName, &key), testAccMatchResourceAttrRegionalARN(resourceName, "arn", "kms", regexp.MustCompile(`key/.+`)), resource.TestCheckResourceAttr(resourceName, "bypass_policy_lockout_safety_check", "false"), resource.TestCheckResourceAttr(resourceName, "deletion_window_in_days", "30"), @@ -59,7 +60,7 @@ func TestAccAWSKmsExternalKey_basic(t *testing.T) { } func TestAccAWSKmsExternalKey_disappears(t *testing.T) { - var key1 kms.KeyMetadata + var key kms.KeyMetadata resourceName := "aws_kms_external_key.test" resource.ParallelTest(t, resource.TestCase{ @@ -71,7 +72,7 @@ func TestAccAWSKmsExternalKey_disappears(t *testing.T) { { Config: testAccAWSKmsExternalKeyConfig(), Check: resource.ComposeTestCheckFunc( - testAccCheckAWSKmsExternalKeyExists(resourceName, &key1), + testAccCheckAWSKmsExternalKeyExists(resourceName, &key), testAccCheckResourceDisappears(testAccProvider, resourceAwsKmsExternalKey(), resourceName), ), ExpectNonEmptyPlan: true, @@ -82,6 +83,7 @@ func TestAccAWSKmsExternalKey_disappears(t *testing.T) { func TestAccAWSKmsExternalKey_DeletionWindowInDays(t *testing.T) { var key1, key2 kms.KeyMetadata + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_external_key.test" resource.ParallelTest(t, resource.TestCase{ @@ -91,7 +93,7 @@ func TestAccAWSKmsExternalKey_DeletionWindowInDays(t *testing.T) { CheckDestroy: testAccCheckAWSKmsExternalKeyDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSKmsExternalKeyConfigDeletionWindowInDays(8), + Config: testAccAWSKmsExternalKeyConfigDeletionWindowInDays(rName, 8), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key1), resource.TestCheckResourceAttr(resourceName, "deletion_window_in_days", "8"), @@ -108,7 +110,7 @@ func TestAccAWSKmsExternalKey_DeletionWindowInDays(t *testing.T) { }, }, { - Config: testAccAWSKmsExternalKeyConfigDeletionWindowInDays(7), + Config: testAccAWSKmsExternalKeyConfigDeletionWindowInDays(rName, 7), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key2), testAccCheckAWSKmsExternalKeyNotRecreated(&key1, &key2), @@ -121,6 +123,7 @@ func TestAccAWSKmsExternalKey_DeletionWindowInDays(t *testing.T) { func TestAccAWSKmsExternalKey_Description(t *testing.T) { var key1, key2 kms.KeyMetadata + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_external_key.test" resource.ParallelTest(t, resource.TestCase{ @@ -130,10 +133,10 @@ func TestAccAWSKmsExternalKey_Description(t *testing.T) { CheckDestroy: testAccCheckAWSKmsExternalKeyDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSKmsExternalKeyConfigDescription("description1"), + Config: testAccAWSKmsExternalKeyConfigDescription(rName + "-1"), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key1), - resource.TestCheckResourceAttr(resourceName, "description", "description1"), + resource.TestCheckResourceAttr(resourceName, "description", rName+"-1"), ), }, { @@ -147,11 +150,11 @@ func TestAccAWSKmsExternalKey_Description(t *testing.T) { }, }, { - Config: testAccAWSKmsExternalKeyConfigDescription("description2"), + Config: testAccAWSKmsExternalKeyConfigDescription(rName + "-2"), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key2), testAccCheckAWSKmsExternalKeyNotRecreated(&key1, &key2), - resource.TestCheckResourceAttr(resourceName, "description", "description2"), + resource.TestCheckResourceAttr(resourceName, "description", rName+"-2"), ), }, }, @@ -160,6 +163,7 @@ func TestAccAWSKmsExternalKey_Description(t *testing.T) { func TestAccAWSKmsExternalKey_Enabled(t *testing.T) { var key1, key2, key3 kms.KeyMetadata + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_external_key.test" resource.ParallelTest(t, resource.TestCase{ @@ -169,7 +173,7 @@ func TestAccAWSKmsExternalKey_Enabled(t *testing.T) { CheckDestroy: testAccCheckAWSKmsExternalKeyDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSKmsExternalKeyConfigEnabled(false), + Config: testAccAWSKmsExternalKeyConfigEnabled(rName, false), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key1), resource.TestCheckResourceAttr(resourceName, "enabled", "false"), @@ -186,7 +190,7 @@ func TestAccAWSKmsExternalKey_Enabled(t *testing.T) { }, }, { - Config: testAccAWSKmsExternalKeyConfigEnabled(true), + Config: testAccAWSKmsExternalKeyConfigEnabled(rName, true), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key2), testAccCheckAWSKmsExternalKeyNotRecreated(&key1, &key2), @@ -194,7 +198,7 @@ func TestAccAWSKmsExternalKey_Enabled(t *testing.T) { ), }, { - Config: testAccAWSKmsExternalKeyConfigEnabled(false), + Config: testAccAWSKmsExternalKeyConfigEnabled(rName, false), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key3), testAccCheckAWSKmsExternalKeyNotRecreated(&key2, &key3), @@ -207,6 +211,7 @@ func TestAccAWSKmsExternalKey_Enabled(t *testing.T) { func TestAccAWSKmsExternalKey_KeyMaterialBase64(t *testing.T) { var key1, key2 kms.KeyMetadata + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_external_key.test" resource.ParallelTest(t, resource.TestCase{ @@ -217,7 +222,7 @@ func TestAccAWSKmsExternalKey_KeyMaterialBase64(t *testing.T) { Steps: []resource.TestStep{ { // ACCEPTANCE TESTING ONLY -- NEVER EXPOSE YOUR KEY MATERIAL - Config: testAccAWSKmsExternalKeyConfigKeyMaterialBase64("Wblj06fduthWggmsT0cLVoIMOkeLbc2kVfMud77i/JY="), + Config: testAccAWSKmsExternalKeyConfigKeyMaterialBase64(rName, "Wblj06fduthWggmsT0cLVoIMOkeLbc2kVfMud77i/JY="), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key1), resource.TestCheckResourceAttr(resourceName, "key_material_base64", "Wblj06fduthWggmsT0cLVoIMOkeLbc2kVfMud77i/JY="), @@ -235,7 +240,7 @@ func TestAccAWSKmsExternalKey_KeyMaterialBase64(t *testing.T) { }, { // ACCEPTANCE TESTING ONLY -- NEVER EXPOSE YOUR KEY MATERIAL - Config: testAccAWSKmsExternalKeyConfigKeyMaterialBase64("O1zsg06cKRCsZnoT5oizMlwHEtnk0HoOmBLkFtwh2Vw="), + Config: testAccAWSKmsExternalKeyConfigKeyMaterialBase64(rName, "O1zsg06cKRCsZnoT5oizMlwHEtnk0HoOmBLkFtwh2Vw="), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key2), testAccCheckAWSKmsExternalKeyRecreated(&key1, &key2), @@ -324,6 +329,7 @@ func TestAccAWSKmsExternalKey_PolicyBypass(t *testing.T) { func TestAccAWSKmsExternalKey_Tags(t *testing.T) { var key1, key2, key3 kms.KeyMetadata + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_external_key.test" resource.ParallelTest(t, resource.TestCase{ @@ -333,7 +339,7 @@ func TestAccAWSKmsExternalKey_Tags(t *testing.T) { CheckDestroy: testAccCheckAWSKmsExternalKeyDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSKmsExternalKeyConfigTags1("value1"), + Config: testAccAWSKmsExternalKeyConfigTags1(rName, "key1", "value1"), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key1), resource.TestCheckResourceAttr(resourceName, "tags.%", "1"), @@ -351,7 +357,7 @@ func TestAccAWSKmsExternalKey_Tags(t *testing.T) { }, }, { - Config: testAccAWSKmsExternalKeyConfigTags2("value1updated", "value2"), + Config: testAccAWSKmsExternalKeyConfigTags2(rName, "key1", "value1updated", "key2", "value2"), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key2), testAccCheckAWSKmsExternalKeyNotRecreated(&key1, &key2), @@ -361,12 +367,12 @@ func TestAccAWSKmsExternalKey_Tags(t *testing.T) { ), }, { - Config: testAccAWSKmsExternalKeyConfigTags1("value1updated"), + Config: testAccAWSKmsExternalKeyConfigTags1(rName, "key2", "value2"), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key3), testAccCheckAWSKmsExternalKeyNotRecreated(&key2, &key3), resource.TestCheckResourceAttr(resourceName, "tags.%", "1"), - resource.TestCheckResourceAttr(resourceName, "tags.key1", "value1updated"), + resource.TestCheckResourceAttr(resourceName, "tags.key2", "value2"), ), }, }, @@ -375,6 +381,7 @@ func TestAccAWSKmsExternalKey_Tags(t *testing.T) { func TestAccAWSKmsExternalKey_ValidTo(t *testing.T) { var key1, key2, key3, key4 kms.KeyMetadata + rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_kms_external_key.test" validTo1 := time.Now().UTC().Add(1 * time.Hour).Format(time.RFC3339) validTo2 := time.Now().UTC().Add(2 * time.Hour).Format(time.RFC3339) @@ -386,7 +393,7 @@ func TestAccAWSKmsExternalKey_ValidTo(t *testing.T) { CheckDestroy: testAccCheckAWSKmsExternalKeyDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSKmsExternalKeyConfigValidTo(validTo1), + Config: testAccAWSKmsExternalKeyConfigValidTo(rName, validTo1), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key1), resource.TestCheckResourceAttr(resourceName, "expiration_model", "KEY_MATERIAL_EXPIRES"), @@ -404,7 +411,7 @@ func TestAccAWSKmsExternalKey_ValidTo(t *testing.T) { }, }, { - Config: testAccAWSKmsExternalKeyConfigEnabled(true), + Config: testAccAWSKmsExternalKeyConfigEnabled(rName, true), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key2), testAccCheckAWSKmsExternalKeyNotRecreated(&key1, &key2), @@ -413,7 +420,7 @@ func TestAccAWSKmsExternalKey_ValidTo(t *testing.T) { ), }, { - Config: testAccAWSKmsExternalKeyConfigValidTo(validTo1), + Config: testAccAWSKmsExternalKeyConfigValidTo(rName, validTo1), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key3), testAccCheckAWSKmsExternalKeyNotRecreated(&key2, &key3), @@ -422,7 +429,7 @@ func TestAccAWSKmsExternalKey_ValidTo(t *testing.T) { ), }, { - Config: testAccAWSKmsExternalKeyConfigValidTo(validTo2), + Config: testAccAWSKmsExternalKeyConfigValidTo(rName, validTo2), Check: resource.ComposeTestCheckFunc( testAccCheckAWSKmsExternalKeyExists(resourceName, &key4), testAccCheckAWSKmsExternalKeyNotRecreated(&key3, &key4), @@ -442,20 +449,18 @@ func testAccCheckAWSKmsExternalKeyHasPolicy(name string, expectedPolicyText stri } if rs.Primary.ID == "" { - return fmt.Errorf("No KMS Key ID is set") + return fmt.Errorf("No KMS External Key ID is set") } conn := testAccProvider.Meta().(*AWSClient).kmsconn - out, err := conn.GetKeyPolicy(&kms.GetKeyPolicyInput{ - KeyId: aws.String(rs.Primary.ID), - PolicyName: aws.String("default"), - }) + output, err := finder.KeyPolicyByKeyIDAndPolicyName(conn, rs.Primary.ID, tfkms.PolicyNameDefault) + if err != nil { return err } - actualPolicyText := *out.Policy + actualPolicyText := aws.StringValue(output) equivalent, err := awspolicy.PoliciesAreEquivalent(actualPolicyText, expectedPolicyText) if err != nil { @@ -547,41 +552,44 @@ resource "aws_kms_external_key" "test" {} ` } -func testAccAWSKmsExternalKeyConfigDeletionWindowInDays(deletionWindowInDays int) string { +func testAccAWSKmsExternalKeyConfigDeletionWindowInDays(rName string, deletionWindowInDays int) string { return fmt.Sprintf(` resource "aws_kms_external_key" "test" { - deletion_window_in_days = %[1]d + description = %[1]q + deletion_window_in_days = %[2]d } -`, deletionWindowInDays) +`, rName, deletionWindowInDays) } -func testAccAWSKmsExternalKeyConfigDescription(description string) string { +func testAccAWSKmsExternalKeyConfigDescription(rName string) string { return fmt.Sprintf(` resource "aws_kms_external_key" "test" { description = %[1]q deletion_window_in_days = 7 } -`, description) +`, rName) } -func testAccAWSKmsExternalKeyConfigEnabled(enabled bool) string { +func testAccAWSKmsExternalKeyConfigEnabled(rName string, enabled bool) string { return fmt.Sprintf(` # ACCEPTANCE TESTING ONLY -- NEVER EXPOSE YOUR KEY MATERIAL resource "aws_kms_external_key" "test" { + description = %[1]q deletion_window_in_days = 7 - enabled = %[1]t + enabled = %[2]t key_material_base64 = "Wblj06fduthWggmsT0cLVoIMOkeLbc2kVfMud77i/JY=" } -`, enabled) +`, rName, enabled) } -func testAccAWSKmsExternalKeyConfigKeyMaterialBase64(keyMaterialBase64 string) string { +func testAccAWSKmsExternalKeyConfigKeyMaterialBase64(rName, keyMaterialBase64 string) string { return fmt.Sprintf(` resource "aws_kms_external_key" "test" { + description = %[1]q deletion_window_in_days = 7 - key_material_base64 = %[1]q + key_material_base64 = %[2]q } -`, keyMaterialBase64) +`, rName, keyMaterialBase64) } func testAccAWSKmsExternalKeyConfigPolicy(rName, policy string) string { @@ -612,38 +620,41 @@ POLICY `, rName, policy) } -func testAccAWSKmsExternalKeyConfigTags1(value1 string) string { +func testAccAWSKmsExternalKeyConfigTags1(rName, tagKey1, tagValue1 string) string { return fmt.Sprintf(` resource "aws_kms_external_key" "test" { + description = %[1]q deletion_window_in_days = 7 tags = { - key1 = %[1]q + %[2]q = %[3]q } } -`, value1) +`, rName, tagKey1, tagValue1) } -func testAccAWSKmsExternalKeyConfigTags2(value1, value2 string) string { +func testAccAWSKmsExternalKeyConfigTags2(rName, tagKey1, tagValue1, tagKey2, tagValue2 string) string { return fmt.Sprintf(` resource "aws_kms_external_key" "test" { + description = %[1]q deletion_window_in_days = 7 tags = { - key1 = %[1]q - key2 = %[2]q + %[2]q = %[3]q + %[4]q = %[5]q } } -`, value1, value2) +`, rName, tagKey1, tagValue1, tagKey2, tagValue2) } -func testAccAWSKmsExternalKeyConfigValidTo(validTo string) string { +func testAccAWSKmsExternalKeyConfigValidTo(rName, validTo string) string { return fmt.Sprintf(` # ACCEPTANCE TESTING ONLY -- NEVER EXPOSE YOUR KEY MATERIAL resource "aws_kms_external_key" "test" { + description = %[1]q deletion_window_in_days = 7 key_material_base64 = "Wblj06fduthWggmsT0cLVoIMOkeLbc2kVfMud77i/JY=" - valid_to = %[1]q + valid_to = %[2]q } -`, validTo) +`, rName, validTo) } From adcb59f4aa5f09234f082ab712c12b3472601ee9 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Wed, 28 Jul 2021 12:02:13 -0400 Subject: [PATCH 33/36] Add 'updateKmsKeyDescription' and 'waiter.KeyValidToPropagated'. --- aws/internal/service/kms/waiter/waiter.go | 58 +++++++++++++++++++++-- aws/resource_aws_kms_external_key.go | 32 ++++++++----- aws/resource_aws_kms_key.go | 36 ++++++++++---- 3 files changed, 100 insertions(+), 26 deletions(-) diff --git a/aws/internal/service/kms/waiter/waiter.go b/aws/internal/service/kms/waiter/waiter.go index 89dee2ff0d2..7b4765a39fc 100644 --- a/aws/internal/service/kms/waiter/waiter.go +++ b/aws/internal/service/kms/waiter/waiter.go @@ -19,10 +19,12 @@ const ( // Maximum amount of time to wait for KeyState to return PendingDeletion KeyStatePendingDeletionTimeout = 20 * time.Minute - KeyDeletedTimeout = 20 * time.Minute - KeyMaterialImportedTimeout = 10 * time.Minute - KeyRotationUpdatedTimeout = 10 * time.Minute - KeyStatePropagationTimeout = 20 * time.Minute + KeyDeletedTimeout = 20 * time.Minute + KeyDescriptionPropagationTimeout = 5 * time.Minute + KeyMaterialImportedTimeout = 10 * time.Minute + KeyRotationUpdatedTimeout = 10 * time.Minute + KeyStatePropagationTimeout = 20 * time.Minute + KeyValidToPropagationTimeout = 5 * time.Minute PropagationTimeout = 2 * time.Minute ) @@ -50,6 +52,28 @@ func KeyDeleted(conn *kms.KMS, id string) (*kms.KeyMetadata, error) { return nil, err } +func KeyDescriptionPropagated(conn *kms.KMS, id string, description string) error { + checkFunc := func() (bool, error) { + output, err := finder.KeyByID(conn, id) + + if tfresource.NotFound(err) { + return false, nil + } + + if err != nil { + return false, err + } + + return aws.StringValue(output.Description) == description, nil + } + opts := tfresource.WaitOpts{ + ContinuousTargetOccurence: 5, + MinTimeout: 2 * time.Second, + } + + return tfresource.WaitUntil(KeyDescriptionPropagationTimeout, checkFunc, opts) +} + func KeyMaterialImported(conn *kms.KMS, id string) (*kms.KeyMetadata, error) { stateConf := &resource.StateChangeConf{ Pending: []string{kms.KeyStatePendingImport}, @@ -139,6 +163,32 @@ func KeyStatePropagated(conn *kms.KMS, id string, enabled bool) error { return tfresource.WaitUntil(KeyStatePropagationTimeout, checkFunc, opts) } +func KeyValidToPropagated(conn *kms.KMS, id string, validTo string) error { + checkFunc := func() (bool, error) { + output, err := finder.KeyByID(conn, id) + + if tfresource.NotFound(err) { + return false, nil + } + + if err != nil { + return false, err + } + + if output.ValidTo != nil { + return aws.TimeValue(output.ValidTo).Format(time.RFC3339) == validTo, nil + } + + return validTo == "", nil + } + opts := tfresource.WaitOpts{ + ContinuousTargetOccurence: 5, + MinTimeout: 2 * time.Second, + } + + return tfresource.WaitUntil(KeyValidToPropagationTimeout, checkFunc, opts) +} + func TagsPropagated(conn *kms.KMS, id string, tags keyvaluetags.KeyValueTags) error { checkFunc := func() (bool, error) { output, err := keyvaluetags.KmsListTags(conn, id) diff --git a/aws/resource_aws_kms_external_key.go b/aws/resource_aws_kms_external_key.go index 6135c26b7db..349c4f52bf2 100644 --- a/aws/resource_aws_kms_external_key.go +++ b/aws/resource_aws_kms_external_key.go @@ -150,7 +150,9 @@ func resourceAwsKmsExternalKeyCreate(d *schema.ResourceData, meta interface{}) e d.SetId(aws.StringValue(outputRaw.(*kms.CreateKeyOutput).KeyMetadata.KeyId)) if v, ok := d.GetOk("key_material_base64"); ok { - if err := importKmsExternalKeyMaterial(conn, d.Id(), v.(string), d.Get("valid_to").(string)); err != nil { + validTo := d.Get("valid_to").(string) + + if err := importKmsExternalKeyMaterial(conn, d.Id(), v.(string), validTo); err != nil { return fmt.Errorf("error importing KMS External Key (%s) material: %s", d.Id(), err) } @@ -158,6 +160,12 @@ func resourceAwsKmsExternalKeyCreate(d *schema.ResourceData, meta interface{}) e return fmt.Errorf("error waiting for KMS External Key (%s) material import: %w", d.Id(), err) } + if err := waiter.KeyValidToPropagated(conn, d.Id(), validTo); err != nil { + return fmt.Errorf("error waiting for KMS External Key (%s) valid_to propagation: %w", d.Id(), err) + } + + // The key can only be disabled if key material has been imported, else: + // "KMSInvalidStateException: arn:aws:kms:us-west-2:123456789012:key/47e3edc1-945f-413b-88b1-e7341c2d89f7 is pending import." if enabled := d.Get("enabled").(bool); !enabled { if err := updateKmsKeyEnabled(conn, d.Id(), enabled); err != nil { return err @@ -223,16 +231,8 @@ func resourceAwsKmsExternalKeyUpdate(d *schema.ResourceData, meta interface{}) e } if d.HasChange("description") { - input := &kms.UpdateKeyDescriptionInput{ - Description: aws.String(d.Get("description").(string)), - KeyId: aws.String(d.Id()), - } - - log.Printf("[DEBUG] Updating KMS External Key description: %s", input) - _, err := conn.UpdateKeyDescription(input) - - if err != nil { - return fmt.Errorf("error updating KMS External Key (%s) description: %w", d.Id(), err) + if err := updateKmsKeyDescription(conn, d.Id(), d.Get("description").(string)); err != nil { + return err } } @@ -243,13 +243,19 @@ func resourceAwsKmsExternalKeyUpdate(d *schema.ResourceData, meta interface{}) e } if d.HasChange("valid_to") { - if err := importKmsExternalKeyMaterial(conn, d.Id(), d.Get("key_material_base64").(string), d.Get("valid_to").(string)); err != nil { + validTo := d.Get("valid_to").(string) + + if err := importKmsExternalKeyMaterial(conn, d.Id(), d.Get("key_material_base64").(string), validTo); err != nil { return fmt.Errorf("error importing KMS External Key (%s) material: %s", d.Id(), err) } if _, err := waiter.KeyMaterialImported(conn, d.Id()); err != nil { return fmt.Errorf("error waiting for KMS External Key (%s) material import: %w", d.Id(), err) } + + if err := waiter.KeyValidToPropagated(conn, d.Id(), validTo); err != nil { + return fmt.Errorf("error waiting for KMS External Key (%s) valid_to propagation: %w", d.Id(), err) + } } if hasChange, enabled, state := d.HasChange("enabled"), d.Get("enabled").(bool), d.Get("key_state").(string); hasChange && !enabled && state != kms.KeyStatePendingImport { @@ -308,6 +314,7 @@ func resourceAwsKmsExternalKeyDelete(d *schema.ResourceData, meta interface{}) e } func importKmsExternalKeyMaterial(conn *kms.KMS, keyID, keyMaterialBase64, validTo string) error { + // Wait for propagation since KMS is eventually consistent. outputRaw, err := tfresource.RetryWhenAwsErrCodeEquals(waiter.PropagationTimeout, func() (interface{}, error) { return conn.GetParametersForImport(&kms.GetParametersForImportInput{ KeyId: aws.String(keyID), @@ -358,6 +365,7 @@ func importKmsExternalKeyMaterial(conn *kms.KMS, keyID, keyMaterialBase64, valid input.ValidTo = aws.Time(t) } + // Wait for propagation since KMS is eventually consistent. _, err = tfresource.RetryWhenAwsErrCodeEquals(waiter.PropagationTimeout, func() (interface{}, error) { return conn.ImportKeyMaterial(input) }, kms.ErrCodeNotFoundException) diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index 5f9059d24ee..7aa984e0c99 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -228,16 +228,8 @@ func resourceAwsKmsKeyUpdate(d *schema.ResourceData, meta interface{}) error { } if d.HasChange("description") { - input := &kms.UpdateKeyDescriptionInput{ - Description: aws.String(d.Get("description").(string)), - KeyId: aws.String(d.Id()), - } - - log.Printf("[DEBUG] Updating KMS Key description: %s", input) - _, err := conn.UpdateKeyDescription(input) - - if err != nil { - return fmt.Errorf("error updating KMS Key (%s) description: %w", d.Id(), err) + if err := updateKmsKeyDescription(conn, d.Id(), d.Get("description").(string)); err != nil { + return err } } @@ -310,6 +302,7 @@ type kmsKey struct { } func findKmsKey(conn *kms.KMS, keyID string, isNewResource bool) (*kmsKey, error) { + // Wait for propagation since KMS is eventually consistent. outputRaw, err := tfresource.RetryWhenNewResourceNotFound(waiter.PropagationTimeout, func() (interface{}, error) { var err error var key kmsKey @@ -360,6 +353,29 @@ func findKmsKey(conn *kms.KMS, keyID string, isNewResource bool) (*kmsKey, error return outputRaw.(*kmsKey), nil } +func updateKmsKeyDescription(conn *kms.KMS, keyID string, description string) error { + input := &kms.UpdateKeyDescriptionInput{ + Description: aws.String(description), + KeyId: aws.String(keyID), + } + + log.Printf("[DEBUG] Updating KMS Key description: %s", input) + _, err := conn.UpdateKeyDescription(input) + + if err != nil { + return fmt.Errorf("error updating KMS Key (%s) description: %w", keyID, err) + } + + // Wait for propagation since KMS is eventually consistent. + err = waiter.KeyDescriptionPropagated(conn, keyID, description) + + if err != nil { + return fmt.Errorf("error waiting for KMS Key (%s) description propagation: %w", keyID, err) + } + + return nil +} + func updateKmsKeyEnabled(conn *kms.KMS, keyID string, enabled bool) error { updateFunc := func() (interface{}, error) { var err error From c111d91e6e7d835aa04314ed9b4c70d8d994faaf Mon Sep 17 00:00:00 2001 From: Lucas Maxwell Date: Tue, 16 Mar 2021 11:52:35 +1100 Subject: [PATCH 34/36] Supply bypass flag when updating kms keys From bc1698d4607c2681587e21e46e01992a77265ac9 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Tue, 3 Aug 2021 10:08:59 -0400 Subject: [PATCH 35/36] Fix golangci-lint errors. --- aws/resource_aws_kms_external_key.go | 2 +- aws/resource_aws_kms_key.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/aws/resource_aws_kms_external_key.go b/aws/resource_aws_kms_external_key.go index 349c4f52bf2..81ea14ae22b 100644 --- a/aws/resource_aws_kms_external_key.go +++ b/aws/resource_aws_kms_external_key.go @@ -190,7 +190,7 @@ func resourceAwsKmsExternalKeyRead(d *schema.ResourceData, meta interface{}) err } if err != nil { - return nil + return err } d.Set("arn", key.metadata.Arn) diff --git a/aws/resource_aws_kms_key.go b/aws/resource_aws_kms_key.go index 7aa984e0c99..d683613ba14 100644 --- a/aws/resource_aws_kms_key.go +++ b/aws/resource_aws_kms_key.go @@ -185,7 +185,7 @@ func resourceAwsKmsKeyRead(d *schema.ResourceData, meta interface{}) error { } if err != nil { - return nil + return err } d.Set("arn", key.metadata.Arn) From 0b4a4c8db4a4d01efa9e12bf4f2ca8279fed4a5a Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Tue, 3 Aug 2021 10:09:33 -0400 Subject: [PATCH 36/36] Fix awsproviderlint 'XAT001: missing ErrorCheck'. --- aws/resource_aws_kms_key_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/aws/resource_aws_kms_key_test.go b/aws/resource_aws_kms_key_test.go index 282c1f2e0dd..d0fd6064cd6 100644 --- a/aws/resource_aws_kms_key_test.go +++ b/aws/resource_aws_kms_key_test.go @@ -191,6 +191,7 @@ func TestAccAWSKmsKey_policyBypass(t *testing.T) { resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, + ErrorCheck: testAccErrorCheck(t, kms.EndpointsID), Providers: testAccProviders, CheckDestroy: testAccCheckAWSKmsKeyDestroy, Steps: []resource.TestStep{ @@ -222,6 +223,7 @@ func TestAccAWSKmsKey_policyBypassUpdate(t *testing.T) { resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, + ErrorCheck: testAccErrorCheck(t, kms.EndpointsID), Providers: testAccProviders, CheckDestroy: testAccCheckAWSKmsKeyDestroy, Steps: []resource.TestStep{