From 3b87b53562bd6fc84e637c1d03a2912704c0f9d6 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Wed, 20 Dec 2023 17:27:30 -0800 Subject: [PATCH 1/3] Fixes bug where changes to `rotation_rules.automatically_after_days` was ignored if `rotation_rules.schedule_expression` was set --- .../service/secretsmanager/secret_rotation.go | 18 +++---- .../secretsmanager/secret_rotation_test.go | 54 +++++++++++++++++++ 2 files changed, 62 insertions(+), 10 deletions(-) diff --git a/internal/service/secretsmanager/secret_rotation.go b/internal/service/secretsmanager/secret_rotation.go index 15ba3a5ae4c6..b6be0ce716d7 100644 --- a/internal/service/secretsmanager/secret_rotation.go +++ b/internal/service/secretsmanager/secret_rotation.go @@ -12,6 +12,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/secretsmanager" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/id" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/hashicorp/terraform-provider-aws/internal/conns" @@ -53,12 +54,7 @@ func ResourceSecretRotation() *schema.Resource { Optional: true, ConflictsWith: []string{"rotation_rules.0.schedule_expression"}, ExactlyOneOf: []string{"rotation_rules.0.automatically_after_days", "rotation_rules.0.schedule_expression"}, - DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool { - _, exists := d.GetOk("rotation_rules.0.schedule_expression") - return exists - }, - DiffSuppressOnRefresh: true, - ValidateFunc: validation.IntBetween(1, 1000), + ValidateFunc: validation.IntBetween(1, 1000), }, "duration": { Type: schema.TypeString, @@ -90,8 +86,9 @@ func resourceSecretRotationCreate(ctx context.Context, d *schema.ResourceData, m secretID := d.Get("secret_id").(string) input := &secretsmanager.RotateSecretInput{ - RotationRules: expandRotationRules(d.Get("rotation_rules").([]interface{})), - SecretId: aws.String(secretID), + ClientRequestToken: aws.String(id.UniqueId()), // Needed because we're handling our own retries + RotationRules: expandRotationRules(d.Get("rotation_rules").([]interface{})), + SecretId: aws.String(secretID), } if v, ok := d.GetOk("rotation_lambda_arn"); ok { @@ -154,8 +151,9 @@ func resourceSecretRotationUpdate(ctx context.Context, d *schema.ResourceData, m if d.HasChanges("rotation_lambda_arn", "rotation_rules") { input := &secretsmanager.RotateSecretInput{ - RotationRules: expandRotationRules(d.Get("rotation_rules").([]interface{})), - SecretId: aws.String(secretID), + ClientRequestToken: aws.String(id.UniqueId()), // Needed because we're handling our own retries + RotationRules: expandRotationRules(d.Get("rotation_rules").([]interface{})), + SecretId: aws.String(secretID), } if v, ok := d.GetOk("rotation_lambda_arn"); ok { diff --git a/internal/service/secretsmanager/secret_rotation_test.go b/internal/service/secretsmanager/secret_rotation_test.go index b13328f24136..fc1d29c1f935 100644 --- a/internal/service/secretsmanager/secret_rotation_test.go +++ b/internal/service/secretsmanager/secret_rotation_test.go @@ -41,6 +41,8 @@ func TestAccSecretsManagerSecretRotation_basic(t *testing.T) { resource.TestCheckResourceAttrPair(resourceName, "rotation_lambda_arn", lambdaFunctionResourceName, "arn"), resource.TestCheckResourceAttr(resourceName, "rotation_rules.#", "1"), resource.TestCheckResourceAttr(resourceName, "rotation_rules.0.automatically_after_days", strconv.Itoa(days)), + resource.TestCheckResourceAttr(resourceName, "rotation_rules.0.duration", ""), + resource.TestCheckResourceAttr(resourceName, "rotation_rules.0.schedule_expression", ""), ), }, { @@ -95,6 +97,53 @@ func TestAccSecretsManagerSecretRotation_scheduleExpression(t *testing.T) { }) } +func TestAccSecretsManagerSecretRotation_scheduleExpressionToDays(t *testing.T) { + ctx := acctest.Context(t) + var secret secretsmanager.DescribeSecretOutput + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + const ( + resourceName = "aws_secretsmanager_secret_rotation.test" + lambdaFunctionResourceName = "aws_lambda_function.test" + scheduleExpression = "rate(10 days)" + days = 7 + ) + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t); testAccPreCheck(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, secretsmanager.EndpointsID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckSecretRotationDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccSecretRotationConfig_scheduleExpression(rName, scheduleExpression), + Check: resource.ComposeTestCheckFunc( + testAccCheckSecretRotationExists(ctx, resourceName, &secret), + resource.TestCheckResourceAttr(resourceName, "rotation_enabled", "true"), + resource.TestCheckResourceAttrPair(resourceName, "rotation_lambda_arn", lambdaFunctionResourceName, "arn"), + resource.TestCheckResourceAttr(resourceName, "rotation_rules.#", "1"), + resource.TestCheckResourceAttr(resourceName, "rotation_rules.0.schedule_expression", scheduleExpression), + resource.TestCheckResourceAttr(resourceName, "rotation_rules.0.automatically_after_days", "0"), + ), + }, + { + Config: testAccSecretRotationConfig_basic(rName, days), + Check: resource.ComposeTestCheckFunc( + testAccCheckSecretRotationExists(ctx, resourceName, &secret), + resource.TestCheckResourceAttr(resourceName, "rotation_enabled", "true"), + resource.TestCheckResourceAttrPair(resourceName, "rotation_lambda_arn", lambdaFunctionResourceName, "arn"), + resource.TestCheckResourceAttr(resourceName, "rotation_rules.#", "1"), + resource.TestCheckResourceAttr(resourceName, "rotation_rules.0.automatically_after_days", strconv.Itoa(days)), + resource.TestCheckResourceAttr(resourceName, "rotation_rules.0.schedule_expression", ""), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + func TestAccSecretsManagerSecretRotation_scheduleExpressionHours(t *testing.T) { ctx := acctest.Context(t) var secret secretsmanager.DescribeSecretOutput @@ -295,6 +344,11 @@ resource "aws_secretsmanager_secret" "test" { name = %[1]q } +resource "aws_secretsmanager_secret_version" "test" { + secret_id = aws_secretsmanager_secret.test.id + secret_string = "test-string" +} + resource "aws_secretsmanager_secret_rotation" "test" { secret_id = aws_secretsmanager_secret.test.id rotation_lambda_arn = aws_lambda_function.test.arn From 13ba5b3dc48052bf51b7f949079a35b89d244103 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Wed, 20 Dec 2023 17:35:22 -0800 Subject: [PATCH 2/3] Sets `ClientRequestToken` for idempotent operations --- internal/service/secretsmanager/secret.go | 7 +++++-- internal/service/secretsmanager/secret_version.go | 4 +++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/internal/service/secretsmanager/secret.go b/internal/service/secretsmanager/secret.go index d2fa147abbf0..97e0da3fac87 100644 --- a/internal/service/secretsmanager/secret.go +++ b/internal/service/secretsmanager/secret.go @@ -15,6 +15,7 @@ import ( "github.com/aws/aws-sdk-go/service/secretsmanager" "github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/id" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure" @@ -142,6 +143,7 @@ func resourceSecretCreate(ctx context.Context, d *schema.ResourceData, meta inte secretName := create.Name(d.Get("name").(string), d.Get("name_prefix").(string)) input := &secretsmanager.CreateSecretInput{ + ClientRequestToken: aws.String(id.UniqueId()), // Needed because we're handling our own retries Description: aws.String(d.Get("description").(string)), ForceOverwriteReplicaSecret: aws.Bool(d.Get("force_overwrite_replica_secret").(bool)), Name: aws.String(secretName), @@ -313,8 +315,9 @@ func resourceSecretUpdate(ctx context.Context, d *schema.ResourceData, meta inte if d.HasChanges("description", "kms_key_id") { input := &secretsmanager.UpdateSecretInput{ - Description: aws.String(d.Get("description").(string)), - SecretId: aws.String(d.Id()), + ClientRequestToken: aws.String(id.UniqueId()), // Needed because we're handling our own retries + Description: aws.String(d.Get("description").(string)), + SecretId: aws.String(d.Id()), } if v, ok := d.GetOk("kms_key_id"); ok { diff --git a/internal/service/secretsmanager/secret_version.go b/internal/service/secretsmanager/secret_version.go index e088430315ba..57694b949509 100644 --- a/internal/service/secretsmanager/secret_version.go +++ b/internal/service/secretsmanager/secret_version.go @@ -14,6 +14,7 @@ import ( "github.com/aws/aws-sdk-go/service/secretsmanager" "github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/id" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-provider-aws/internal/conns" @@ -78,7 +79,8 @@ func resourceSecretVersionCreate(ctx context.Context, d *schema.ResourceData, me secretID := d.Get("secret_id").(string) input := &secretsmanager.PutSecretValueInput{ - SecretId: aws.String(secretID), + ClientRequestToken: aws.String(id.UniqueId()), // Needed because we're handling our own retries + SecretId: aws.String(secretID), } if v, ok := d.GetOk("secret_string"); ok { From 92c991e5404ecd5b0c73eca66728c900170babdc Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Wed, 20 Dec 2023 18:06:22 -0800 Subject: [PATCH 3/3] Adds CHANGELOG entry --- .changelog/35024.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/35024.txt diff --git a/.changelog/35024.txt b/.changelog/35024.txt new file mode 100644 index 000000000000..f4b6a94f92b0 --- /dev/null +++ b/.changelog/35024.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_secretsmanager_secret_rotation: No longer ignores changes to `rotation_rules.automatically_after_days` when `rotation_rules.schedule_expression` is set. +```