Merge pull request #19854 from GavinWu1991/f-aws-backup-vault-policy-…

…sweeper

r/aws_backup_vault_policy: sweeper should not fail on AccessDeniedException

.changelog/19749.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+resource/aws_backup_vault_policy: Correctly handle reading policy of deleted vault
+```

.changelog/19854.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+resource/aws_backup_vault_policy: Correctly handle deleting policy of deleted vault
+```

aws/internal/service/backup/errors.go

@@ -0,0 +1,5 @@
+package backup
+
+const (
+	ErrCodeAccessDeniedException = "AccessDeniedException"
+)

aws/internal/service/backup/finder/finder.go

@@ -0,0 +1,37 @@
+package finder
+
+import (
+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/service/backup"
+	"github.com/hashicorp/aws-sdk-go-base/tfawserr"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
+	tfbackup "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/backup"
+)
+
+func BackupVaultAccessPolicyByName(conn *backup.Backup, name string) (*backup.GetBackupVaultAccessPolicyOutput, error) {
+	input := &backup.GetBackupVaultAccessPolicyInput{
+		BackupVaultName: aws.String(name),
+	}
+
+	output, err := conn.GetBackupVaultAccessPolicy(input)
+
+	if tfawserr.ErrCodeEquals(err, backup.ErrCodeResourceNotFoundException) || tfawserr.ErrCodeEquals(err, tfbackup.ErrCodeAccessDeniedException) {
+		return nil, &resource.NotFoundError{
+			LastError:   err,
+			LastRequest: input,
+		}
+	}
+
+	if err != nil {
+		return nil, err
+	}
+
+	if output == nil {
+		return nil, &resource.NotFoundError{
+			Message:     "Empty result",
+			LastRequest: input,
+		}
+	}
+
+	return output, nil
+}

aws/resource_aws_backup_vault_policy.go

@@ -6,8 +6,12 @@ import (
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/backup"
+	"github.com/hashicorp/aws-sdk-go-base/tfawserr"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
+	tfbackup "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/backup"
+	"github.com/terraform-providers/terraform-provider-aws/aws/internal/service/backup/finder"
+	"github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource"
 )
 
 func resourceAwsBackupVaultPolicy() *schema.Resource {
@@ -21,6 +25,10 @@ func resourceAwsBackupVaultPolicy() *schema.Resource {
 		},
 
 		Schema: map[string]*schema.Schema{
+			"backup_vault_arn": {
+				Type:     schema.TypeString,
+				Computed: true,
+			},
 			"backup_vault_name": {
 				Type:     schema.TypeString,
 				Required: true,
@@ -32,68 +40,65 @@ func resourceAwsBackupVaultPolicy() *schema.Resource {
 				ValidateFunc:     validation.StringIsJSON,
 				DiffSuppressFunc: suppressEquivalentAwsPolicyDiffs,
 			},
-			"backup_vault_arn": {
-				Type:     schema.TypeString,
-				Computed: true,
-			},
 		},
 	}
 }
 
 func resourceAwsBackupVaultPolicyPut(d *schema.ResourceData, meta interface{}) error {
 	conn := meta.(*AWSClient).backupconn
 
+	name := d.Get("backup_vault_name").(string)
 	input := &backup.PutBackupVaultAccessPolicyInput{
-		BackupVaultName: aws.String(d.Get("backup_vault_name").(string)),
+		BackupVaultName: aws.String(name),
 		Policy:          aws.String(d.Get("policy").(string)),
 	}
 
 	_, err := conn.PutBackupVaultAccessPolicy(input)
+
 	if err != nil {
-		return fmt.Errorf("error creating Backup Vault Policy (%s): %w", d.Id(), err)
+		return fmt.Errorf("error creating Backup Vault Policy (%s): %w", name, err)
 	}
 
-	d.SetId(d.Get("backup_vault_name").(string))
+	d.SetId(name)
 
 	return resourceAwsBackupVaultPolicyRead(d, meta)
 }
 
 func resourceAwsBackupVaultPolicyRead(d *schema.ResourceData, meta interface{}) error {
 	conn := meta.(*AWSClient).backupconn
 
-	input := &backup.GetBackupVaultAccessPolicyInput{
-		BackupVaultName: aws.String(d.Id()),
-	}
+	output, err := finder.BackupVaultAccessPolicyByName(conn, d.Id())
 
-	resp, err := conn.GetBackupVaultAccessPolicy(input)
-	if isAWSErr(err, backup.ErrCodeResourceNotFoundException, "") {
-		log.Printf("[WARN] Backup Vault Policy %s not found, removing from state", d.Id())
+	if !d.IsNewResource() && tfresource.NotFound(err) {
+		log.Printf("[WARN] Backup Vault Policy (%s) not found, removing from state", d.Id())
 		d.SetId("")
 		return nil
 	}
 
 	if err != nil {
 		return fmt.Errorf("error reading Backup Vault Policy (%s): %w", d.Id(), err)
 	}
-	d.Set("backup_vault_name", resp.BackupVaultName)
-	d.Set("policy", resp.Policy)
-	d.Set("backup_vault_arn", resp.BackupVaultArn)
+
+	d.Set("backup_vault_arn", output.BackupVaultArn)
+	d.Set("backup_vault_name", output.BackupVaultName)
+	d.Set("policy", output.Policy)
 
 	return nil
 }
 
 func resourceAwsBackupVaultPolicyDelete(d *schema.ResourceData, meta interface{}) error {
 	conn := meta.(*AWSClient).backupconn
 
-	input := &backup.DeleteBackupVaultAccessPolicyInput{
+	log.Printf("[DEBUG] Deleting Backup Vault Policy (%s)", d.Id())
+	_, err := conn.DeleteBackupVaultAccessPolicy(&backup.DeleteBackupVaultAccessPolicyInput{
 		BackupVaultName: aws.String(d.Id()),
+	})
+
+	if tfawserr.ErrCodeEquals(err, backup.ErrCodeResourceNotFoundException) || tfawserr.ErrCodeEquals(err, tfbackup.ErrCodeAccessDeniedException) {
+		return nil
 	}
 
-	_, err := conn.DeleteBackupVaultAccessPolicy(input)
 	if err != nil {
-		if isAWSErr(err, backup.ErrCodeResourceNotFoundException, "") {
-			return nil
-		}
 		return fmt.Errorf("error deleting Backup Vault Policy (%s): %w", d.Id(), err)
 	}
 

aws/resource_aws_backup_vault_policy_test.go

@@ -8,10 +8,12 @@ import (
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/backup"
-	"github.com/hashicorp/go-multierror"
+	multierror "github.com/hashicorp/go-multierror"
 	"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/service/backup/finder"
+	"github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource"
 )
 
 func init() {
@@ -27,55 +29,47 @@ func testSweepBackupVaultPolicies(region string) error {
 		return fmt.Errorf("Error getting client: %w", err)
 	}
 	conn := client.(*AWSClient).backupconn
-	var sweeperErrs *multierror.Error
-
 	input := &backup.ListBackupVaultsInput{}
+	var sweeperErrs *multierror.Error
+	sweepResources := make([]*testSweepResource, 0)
 
-	for {
-		output, err := conn.ListBackupVaults(input)
-		if err != nil {
-			if testSweepSkipSweepError(err) {
-				log.Printf("[WARN] Skipping Backup Vault Policies sweep for %s: %s", region, err)
-				return nil
-			}
-			sweeperErrs = multierror.Append(sweeperErrs, fmt.Errorf("error retrieving Backup Vault Policies: %w", err))
-			return sweeperErrs.ErrorOrNil()
+	err = conn.ListBackupVaultsPages(input, func(page *backup.ListBackupVaultsOutput, lastPage bool) bool {
+		if page == nil {
+			return !lastPage
 		}
 
-		if len(output.BackupVaultList) == 0 {
-			log.Print("[DEBUG] No Backup Vault Policies to sweep")
-			return nil
-		}
+		for _, vault := range page.BackupVaultList {
+			r := resourceAwsBackupVaultPolicy()
+			d := r.Data(nil)
+			d.SetId(aws.StringValue(vault.BackupVaultName))
 
-		for _, rule := range output.BackupVaultList {
-			name := aws.StringValue(rule.BackupVaultName)
-
-			log.Printf("[INFO] Deleting Backup Vault Policies %s", name)
-			_, err := conn.DeleteBackupVaultAccessPolicy(&backup.DeleteBackupVaultAccessPolicyInput{
-				BackupVaultName: aws.String(name),
-			})
-			if err != nil {
-				sweeperErr := fmt.Errorf("error deleting Backup Vault Policies %s: %w", name, err)
-				log.Printf("[ERROR] %s", sweeperErr)
-				sweeperErrs = multierror.Append(sweeperErrs, sweeperErr)
-				continue
-			}
+			sweepResources = append(sweepResources, NewTestSweepResource(r, d, client))
 		}
 
-		if output.NextToken == nil {
-			break
-		}
-		input.NextToken = output.NextToken
+		return !lastPage
+	})
+
+	if testSweepSkipSweepError(err) {
+		log.Printf("[WARN] Skipping Backup Vault Policies sweep for %s: %s", region, err)
+		return sweeperErrs.ErrorOrNil()
+	}
+
+	if err != nil {
+		sweeperErrs = multierror.Append(sweeperErrs, fmt.Errorf("error listing Backup Vaults for %s: %w", region, err))
+	}
+
+	if err := testSweepResourceOrchestrator(sweepResources); err != nil {
+		sweeperErrs = multierror.Append(sweeperErrs, fmt.Errorf("error sweeping Backup Vault Policies for %s: %w", region, err))
 	}
 
 	return sweeperErrs.ErrorOrNil()
 }
 
 func TestAccAwsBackupVaultPolicy_basic(t *testing.T) {
 	var vault backup.GetBackupVaultAccessPolicyOutput
-
 	rName := acctest.RandomWithPrefix("tf-acc-test")
 	resourceName := "aws_backup_vault_policy.test"
+
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t); testAccPreCheckAWSBackup(t) },
 		ErrorCheck:   testAccErrorCheck(t, backup.EndpointsID),
@@ -107,9 +101,9 @@ func TestAccAwsBackupVaultPolicy_basic(t *testing.T) {
 
 func TestAccAwsBackupVaultPolicy_disappears(t *testing.T) {
 	var vault backup.GetBackupVaultAccessPolicyOutput
-
 	rName := acctest.RandomWithPrefix("tf-acc-test")
 	resourceName := "aws_backup_vault_policy.test"
+
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t); testAccPreCheckAWSBackup(t) },
 		ErrorCheck:   testAccErrorCheck(t, backup.EndpointsID),
@@ -128,24 +122,49 @@ func TestAccAwsBackupVaultPolicy_disappears(t *testing.T) {
 	})
 }
 
+func TestAccAwsBackupVaultPolicy_disappears_vault(t *testing.T) {
+	var vault backup.GetBackupVaultAccessPolicyOutput
+	rName := acctest.RandomWithPrefix("tf-acc-test")
+	resourceName := "aws_backup_vault_policy.test"
+	vaultResourceName := "aws_backup_vault.test"
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t); testAccPreCheckAWSBackup(t) },
+		ErrorCheck:   testAccErrorCheck(t, backup.EndpointsID),
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckAwsBackupVaultPolicyDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccBackupVaultPolicyConfig(rName),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAwsBackupVaultPolicyExists(resourceName, &vault),
+					testAccCheckResourceDisappears(testAccProvider, resourceAwsBackupVault(), vaultResourceName),
+				),
+				ExpectNonEmptyPlan: true,
+			},
+		},
+	})
+}
+
 func testAccCheckAwsBackupVaultPolicyDestroy(s *terraform.State) error {
 	conn := testAccProvider.Meta().(*AWSClient).backupconn
+
 	for _, rs := range s.RootModule().Resources {
 		if rs.Type != "aws_backup_vault_policy" {
 			continue
 		}
 
-		input := &backup.GetBackupVaultAccessPolicyInput{
-			BackupVaultName: aws.String(rs.Primary.ID),
-		}
+		_, err := finder.BackupVaultAccessPolicyByName(conn, rs.Primary.ID)
 
-		resp, err := conn.GetBackupVaultAccessPolicy(input)
+		if tfresource.NotFound(err) {
+			continue
+		}
 
-		if err == nil {
-			if aws.StringValue(resp.BackupVaultName) == rs.Primary.ID {
-				return fmt.Errorf("Backup Plan Policies '%s' was not deleted properly", rs.Primary.ID)
-			}
+		if err != nil {
+			return err
 		}
+
+		return fmt.Errorf("Backup Vault Policy %s still exists", rs.Primary.ID)
 	}
 
 	return nil
@@ -158,16 +177,19 @@ func testAccCheckAwsBackupVaultPolicyExists(name string, vault *backup.GetBackup
 			return fmt.Errorf("Not found: %s", name)
 		}
 
-		conn := testAccProvider.Meta().(*AWSClient).backupconn
-		params := &backup.GetBackupVaultAccessPolicyInput{
-			BackupVaultName: aws.String(rs.Primary.ID),
+		if rs.Primary.ID == "" {
+			return fmt.Errorf("No Backup Vault Policy ID is set")
 		}
-		resp, err := conn.GetBackupVaultAccessPolicy(params)
+
+		conn := testAccProvider.Meta().(*AWSClient).backupconn
+
+		output, err := finder.BackupVaultAccessPolicyByName(conn, rs.Primary.ID)
+
 		if err != nil {
 			return err
 		}
 
-		*vault = *resp
+		*vault = *output
 
 		return nil
 	}