From 912fb6f72998315f83a347e0585109c2e2991f11 Mon Sep 17 00:00:00 2001 From: Sharon Nam Date: Thu, 30 Nov 2023 15:07:52 -0800 Subject: [PATCH 1/3] add iam retry --- internal/service/backup/errors.go | 3 ++- internal/service/backup/vault_policy.go | 13 +++++++++++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/internal/service/backup/errors.go b/internal/service/backup/errors.go index b8dff1cad971..dd42e0594988 100644 --- a/internal/service/backup/errors.go +++ b/internal/service/backup/errors.go @@ -4,5 +4,6 @@ package backup const ( - errCodeAccessDeniedException = "AccessDeniedException" + errCodeAccessDeniedException = "AccessDeniedException" + errCodeInvalidVaultPolicyConfig = "InvalidVaultPolicyConfig" ) diff --git a/internal/service/backup/vault_policy.go b/internal/service/backup/vault_policy.go index 7a3240a7e044..8f8121e2c535 100644 --- a/internal/service/backup/vault_policy.go +++ b/internal/service/backup/vault_policy.go @@ -6,6 +6,7 @@ package backup import ( "context" "log" + "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/backup" @@ -60,6 +61,8 @@ func resourceVaultPolicyPut(ctx context.Context, d *schema.ResourceData, meta in var diags diag.Diagnostics conn := meta.(*conns.AWSClient).BackupConn(ctx) + iamPropagationTimeout := 2 * time.Minute + policy, err := structure.NormalizeJsonString(d.Get("policy").(string)) if err != nil { @@ -72,10 +75,16 @@ func resourceVaultPolicyPut(ctx context.Context, d *schema.ResourceData, meta in Policy: aws.String(policy), } - _, err = conn.PutBackupVaultAccessPolicyWithContext(ctx, input) + outputRaw, err := tfresource.RetryWhenAWSErrMessageContains(ctx, iamPropagationTimeout, + func() (interface{}, error) { + return conn.PutBackupVaultAccessPolicyWithContext(ctx, input) + }, + errCodeInvalidVaultPolicyConfig, "VaultPolicyyConfig.IamBackupRole", + ) if err != nil { - return sdkdiag.AppendErrorf(diags, "creating Backup Vault Policy (%s): %s", name, err) + return sdkdiag.AppendErrorf(diags, "creating Backup Vault Policy (%s): %s", outputRaw.(d.) + , err) } d.SetId(name) From 0b1b4c621136a78543966d3d55862e05ee10d290 Mon Sep 17 00:00:00 2001 From: Sharon Nam Date: Thu, 30 Nov 2023 18:43:21 -0800 Subject: [PATCH 2/3] add test --- internal/service/backup/vault_policy.go | 5 +- internal/service/backup/vault_policy_test.go | 65 ++++++++++++++++++++ 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/internal/service/backup/vault_policy.go b/internal/service/backup/vault_policy.go index 8f8121e2c535..5d8b7afe183a 100644 --- a/internal/service/backup/vault_policy.go +++ b/internal/service/backup/vault_policy.go @@ -75,7 +75,7 @@ func resourceVaultPolicyPut(ctx context.Context, d *schema.ResourceData, meta in Policy: aws.String(policy), } - outputRaw, err := tfresource.RetryWhenAWSErrMessageContains(ctx, iamPropagationTimeout, + _, err = tfresource.RetryWhenAWSErrMessageContains(ctx, iamPropagationTimeout, func() (interface{}, error) { return conn.PutBackupVaultAccessPolicyWithContext(ctx, input) }, @@ -83,8 +83,7 @@ func resourceVaultPolicyPut(ctx context.Context, d *schema.ResourceData, meta in ) if err != nil { - return sdkdiag.AppendErrorf(diags, "creating Backup Vault Policy (%s): %s", outputRaw.(d.) - , err) + return sdkdiag.AppendErrorf(diags, "creating Backup Vault Policy (%s): %s", name, err) } d.SetId(name) diff --git a/internal/service/backup/vault_policy_test.go b/internal/service/backup/vault_policy_test.go index e66e263e23bf..107e65cc1856 100644 --- a/internal/service/backup/vault_policy_test.go +++ b/internal/service/backup/vault_policy_test.go @@ -54,6 +54,41 @@ func TestAccBackupVaultPolicy_basic(t *testing.T) { }) } +func TestAccBackupVaultPolicy_eventual_consistency(t *testing.T) { + ctx := acctest.Context(t) + var vault backup.GetBackupVaultAccessPolicyOutput + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_backup_vault_policy.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t); testAccPreCheck(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, backup.EndpointsID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckVaultPolicyDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccVaultPolicyConfig_eventual_consistency(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckVaultPolicyExists(ctx, resourceName, &vault), + resource.TestMatchResourceAttr(resourceName, "policy", regexache.MustCompile("^{\"Id\":\"default\".+"))), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + { + Config: testAccVaultPolicyConfig_updated(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckVaultPolicyExists(ctx, resourceName, &vault), + resource.TestMatchResourceAttr(resourceName, "policy", regexache.MustCompile("^{\"Id\":\"default\".+")), + resource.TestMatchResourceAttr(resourceName, "policy", regexache.MustCompile("backup:ListRecoveryPointsByBackupVault")), + ), + }, + }, + }) +} + func TestAccBackupVaultPolicy_disappears(t *testing.T) { ctx := acctest.Context(t) var vault backup.GetBackupVaultAccessPolicyOutput @@ -285,3 +320,33 @@ resource "aws_backup_vault_policy" "test" { } `, rName) } + +func testAccVaultPolicyConfig_eventual_consistency(rName string) string { + return acctest.ConfigCompose( + testAccVaultPolicyConfig_basic(rName), + fmt.Sprintf(` +data "aws_partition" "current" {} + +resource "aws_iam_role" "test" { + name = %[1]q + assume_role_policy = jsonencode({ + Version = "2012-10-17" + Statement = [ + { + Action = "sts:AssumeRole" + Effect = "Allow" + Sid = "" + Principal = { + Service = "backup.amazonaws.com" + } + }, + ] + }) +} + +resource "aws_iam_role_policy_attachment" "test" { + role = aws_iam_role.test.name + policy_arn = "arn:${data.aws_partition.current.partition}:iam::${data.aws_partition.current.partition}:policy/service-role/AWSBackupServiceRolePolicyForBackup" +} +`, rName)) +} From cbf8d0496e745107c24857a5a967b3d16271bf0a Mon Sep 17 00:00:00 2001 From: Adrian Johnson Date: Mon, 4 Dec 2023 12:04:22 -0600 Subject: [PATCH 3/3] aws_backup_vault_policy: tweak acctest to use iam role --- internal/service/backup/consts.go | 6 +++ internal/service/backup/errors.go | 4 +- internal/service/backup/vault_policy.go | 5 +- internal/service/backup/vault_policy_test.go | 51 +++++++++++++------- 4 files changed, 43 insertions(+), 23 deletions(-) diff --git a/internal/service/backup/consts.go b/internal/service/backup/consts.go index ea4860cd65fc..573f338a20d8 100644 --- a/internal/service/backup/consts.go +++ b/internal/service/backup/consts.go @@ -3,6 +3,12 @@ package backup +import "time" + +const ( + iamPropagationTimeout = 2 * time.Minute +) + const ( frameworkStatusCompleted = "COMPLETED" frameworkStatusCreationInProgress = "CREATE_IN_PROGRESS" diff --git a/internal/service/backup/errors.go b/internal/service/backup/errors.go index dd42e0594988..38fd3926ae88 100644 --- a/internal/service/backup/errors.go +++ b/internal/service/backup/errors.go @@ -4,6 +4,6 @@ package backup const ( - errCodeAccessDeniedException = "AccessDeniedException" - errCodeInvalidVaultPolicyConfig = "InvalidVaultPolicyConfig" + errCodeAccessDeniedException = "AccessDeniedException" + errCodeInvalidParameterValueException = "InvalidParameterValueException" ) diff --git a/internal/service/backup/vault_policy.go b/internal/service/backup/vault_policy.go index 5d8b7afe183a..c7b84e4aefd7 100644 --- a/internal/service/backup/vault_policy.go +++ b/internal/service/backup/vault_policy.go @@ -6,7 +6,6 @@ package backup import ( "context" "log" - "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/backup" @@ -61,8 +60,6 @@ func resourceVaultPolicyPut(ctx context.Context, d *schema.ResourceData, meta in var diags diag.Diagnostics conn := meta.(*conns.AWSClient).BackupConn(ctx) - iamPropagationTimeout := 2 * time.Minute - policy, err := structure.NormalizeJsonString(d.Get("policy").(string)) if err != nil { @@ -79,7 +76,7 @@ func resourceVaultPolicyPut(ctx context.Context, d *schema.ResourceData, meta in func() (interface{}, error) { return conn.PutBackupVaultAccessPolicyWithContext(ctx, input) }, - errCodeInvalidVaultPolicyConfig, "VaultPolicyyConfig.IamBackupRole", + errCodeInvalidParameterValueException, "Provided principal is not valid", ) if err != nil { diff --git a/internal/service/backup/vault_policy_test.go b/internal/service/backup/vault_policy_test.go index 107e65cc1856..a9540f9b67f5 100644 --- a/internal/service/backup/vault_policy_test.go +++ b/internal/service/backup/vault_policy_test.go @@ -54,7 +54,7 @@ func TestAccBackupVaultPolicy_basic(t *testing.T) { }) } -func TestAccBackupVaultPolicy_eventual_consistency(t *testing.T) { +func TestAccBackupVaultPolicy_eventualConsistency(t *testing.T) { ctx := acctest.Context(t) var vault backup.GetBackupVaultAccessPolicyOutput rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) @@ -67,24 +67,11 @@ func TestAccBackupVaultPolicy_eventual_consistency(t *testing.T) { CheckDestroy: testAccCheckVaultPolicyDestroy(ctx), Steps: []resource.TestStep{ { - Config: testAccVaultPolicyConfig_eventual_consistency(rName), + Config: testAccVaultPolicyConfig_eventualConsistency(rName), Check: resource.ComposeTestCheckFunc( testAccCheckVaultPolicyExists(ctx, resourceName, &vault), resource.TestMatchResourceAttr(resourceName, "policy", regexache.MustCompile("^{\"Id\":\"default\".+"))), }, - { - ResourceName: resourceName, - ImportState: true, - ImportStateVerify: true, - }, - { - Config: testAccVaultPolicyConfig_updated(rName), - Check: resource.ComposeTestCheckFunc( - testAccCheckVaultPolicyExists(ctx, resourceName, &vault), - resource.TestMatchResourceAttr(resourceName, "policy", regexache.MustCompile("^{\"Id\":\"default\".+")), - resource.TestMatchResourceAttr(resourceName, "policy", regexache.MustCompile("backup:ListRecoveryPointsByBackupVault")), - ), - }, }, }) } @@ -321,9 +308,8 @@ resource "aws_backup_vault_policy" "test" { `, rName) } -func testAccVaultPolicyConfig_eventual_consistency(rName string) string { +func testAccVaultPolicyConfig_eventualConsistency(rName string) string { return acctest.ConfigCompose( - testAccVaultPolicyConfig_basic(rName), fmt.Sprintf(` data "aws_partition" "current" {} @@ -348,5 +334,36 @@ resource "aws_iam_role_policy_attachment" "test" { role = aws_iam_role.test.name policy_arn = "arn:${data.aws_partition.current.partition}:iam::${data.aws_partition.current.partition}:policy/service-role/AWSBackupServiceRolePolicyForBackup" } + +resource "aws_backup_vault" "test" { + name = %[1]q +} + +resource "aws_backup_vault_policy" "test" { + backup_vault_name = aws_backup_vault.test.name + + policy = jsonencode({ + Version = "2012-10-17" + Id = "default" + Statement = [{ + Sid = "default" + Effect = "Allow" + Principal = { + AWS = "${aws_iam_role.test.arn}" + } + Action = [ + "backup:DescribeBackupVault", + "backup:DeleteBackupVault", + "backup:PutBackupVaultAccessPolicy", + "backup:DeleteBackupVaultAccessPolicy", + "backup:GetBackupVaultAccessPolicy", + "backup:StartBackupJob", + "backup:GetBackupVaultNotifications", + "backup:PutBackupVaultNotifications", + ] + Resource = aws_backup_vault.test.arn + }] + }) +} `, rName)) }