r/aws_s3_bucket: Speed up deletion

.changelog/24020.txt

@@ -0,0 +1,3 @@
+```release-note:enhancement
+resource/aws_s3_bucket: Speed up resource deletion, especially when the S3 buckets contains a large number of objects and `force_destroy` is `true`
+```

internal/service/s3/bucket.go

@@ -19,6 +19,7 @@ import (
 	"github.com/aws/aws-sdk-go/service/s3"
 	"github.com/aws/aws-sdk-go/service/s3/s3manager"
 	"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/resource"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure"
@@ -32,10 +33,11 @@ import (
 
 func ResourceBucket() *schema.Resource {
 	return &schema.Resource{
-		Create: resourceBucketCreate,
-		Read:   resourceBucketRead,
-		Update: resourceBucketUpdate,
-		Delete: resourceBucketDelete,
+		Create:               resourceBucketCreate,
+		Read:                 resourceBucketRead,
+		Update:               resourceBucketUpdate,
+		DeleteWithoutTimeout: resourceBucketDelete,
+
 		Importer: &schema.ResourceImporter{
 			State: schema.ImportStatePassthrough,
 		},
@@ -1404,27 +1406,27 @@ func resourceBucketRead(d *schema.ResourceData, meta interface{}) error {
 	return nil
 }
 
-func resourceBucketDelete(d *schema.ResourceData, meta interface{}) error {
+func resourceBucketDelete(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
 	conn := meta.(*conns.AWSClient).S3Conn
 
-	log.Printf("[DEBUG] S3 Delete Bucket: %s", d.Id())
-	_, err := conn.DeleteBucket(&s3.DeleteBucketInput{
+	log.Printf("[DEBUG] Deleting S3 Bucket: %s", d.Id())
+	_, err := conn.DeleteBucketWithContext(ctx, &s3.DeleteBucketInput{
 		Bucket: aws.String(d.Id()),
 	})
 
 	if tfawserr.ErrCodeEquals(err, s3.ErrCodeNoSuchBucket) {
 		return nil
 	}
 
-	if tfawserr.ErrCodeEquals(err, "BucketNotEmpty") {
+	if tfawserr.ErrCodeEquals(err, ErrCodeBucketNotEmpty) {
 		if d.Get("force_destroy").(bool) {
 			// Use a S3 service client that can handle multiple slashes in URIs.
 			// While aws_s3_object resources cannot create these object
 			// keys, other AWS services and applications using the S3 Bucket can.
 			conn = meta.(*conns.AWSClient).S3ConnURICleaningDisabled
 
 			// bucket may have things delete them
-			log.Printf("[DEBUG] S3 Bucket attempting to forceDestroy %+v", err)
+			log.Printf("[DEBUG] S3 Bucket attempting to forceDestroy %s", err)
 
 			// Delete everything including locked objects.
 			// Don't ignore any object errors or we could recurse infinitely.
@@ -1433,19 +1435,20 @@ func resourceBucketDelete(d *schema.ResourceData, meta interface{}) error {
 			if objectLockConfiguration != nil {
 				objectLockEnabled = aws.StringValue(objectLockConfiguration.ObjectLockEnabled) == s3.ObjectLockEnabledEnabled
 			}
-			err = DeleteAllObjectVersions(conn, d.Id(), "", objectLockEnabled, false)
 
-			if err != nil {
-				return fmt.Errorf("error S3 Bucket force_destroy: %s", err)
+			if n, err := EmptyBucket(ctx, conn, d.Id(), objectLockEnabled); err != nil {
+				return diag.Errorf("emptying S3 Bucket (%s): %s", d.Id(), err)
+			} else {
+				log.Printf("[DEBUG] Deleted %d S3 objects", n)
 			}
 
 			// this line recurses until all objects are deleted or an error is returned
-			return resourceBucketDelete(d, meta)
+			return resourceBucketDelete(ctx, d, meta)
 		}
 	}
 
 	if err != nil {
-		return fmt.Errorf("error deleting S3 Bucket (%s): %s", d.Id(), err)
+		return diag.Errorf("deleting S3 Bucket (%s): %s", d.Id(), err)
 	}
 
 	return nil

internal/service/s3/bucket_object.go

@@ -390,7 +390,7 @@ func resourceBucketObjectDelete(d *schema.ResourceData, meta interface{}) error
 
 	var err error
 	if _, ok := d.GetOk("version_id"); ok {
-		err = DeleteAllObjectVersions(conn, bucket, key, d.Get("force_destroy").(bool), false)
+		_, err = DeleteAllObjectVersions(conn, bucket, key, d.Get("force_destroy").(bool), false)
 	} else {
 		err = deleteS3ObjectVersion(conn, bucket, key, "", false)
 	}

internal/service/s3/delete.go

@@ -0,0 +1,240 @@
+package s3
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/awserr"
+	"github.com/aws/aws-sdk-go/service/s3"
+	"github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr"
+	multierror "github.com/hashicorp/go-multierror"
+)
+
+// EmptyBucket empties the specified S3 bucket by deleting all object versions and delete markers.
+// If `force` is `true` then S3 Object Lock governance mode restrictions are bypassed and
+// an attempt is made to remove any S3 Object Lock legal holds.
+// Returns the number of objects deleted.
+func EmptyBucket(ctx context.Context, conn *s3.S3, bucket string, force bool) (int64, error) {
+	nObjects, err := forEachObjectVersionsPage(ctx, conn, bucket, func(ctx context.Context, conn *s3.S3, bucket string, page *s3.ListObjectVersionsOutput) (int64, error) {
+		return deletePageOfObjectVersions(ctx, conn, bucket, force, page)
+	})
+
+	if err != nil {
+		return nObjects, err
+	}
+
+	n, err := forEachObjectVersionsPage(ctx, conn, bucket, deletePageOfDeleteMarkers)
+	nObjects += n
+
+	if err != nil {
+		return nObjects, err
+	}
+
+	return nObjects, nil
+}
+
+// forEachObjectVersionsPage calls the specified function for each page returned from the S3 ListObjectVersionsPages API.
+func forEachObjectVersionsPage(ctx context.Context, conn *s3.S3, bucket string, fn func(ctx context.Context, conn *s3.S3, bucket string, page *s3.ListObjectVersionsOutput) (int64, error)) (int64, error) {
+	var nObjects int64
+
+	input := &s3.ListObjectVersionsInput{
+		Bucket: aws.String(bucket),
+	}
+	var lastErr error
+
+	err := conn.ListObjectVersionsPagesWithContext(ctx, input, func(page *s3.ListObjectVersionsOutput, lastPage bool) bool {
+		if page == nil {
+			return !lastPage
+		}
+
+		n, err := fn(ctx, conn, bucket, page)
+		nObjects += n
+
+		if err != nil {
+			lastErr = err
+
+			return false
+		}
+
+		return !lastPage
+	})
+
+	if err != nil {
+		return nObjects, fmt.Errorf("listing S3 Bucket (%s) object versions: %w", bucket, err)
+	}
+
+	if lastErr != nil {
+		return nObjects, lastErr
+	}
+
+	return nObjects, nil
+}
+
+// deletePageOfObjectVersions deletes a page (<= 1000) of S3 object versions.
+// If `force` is `true` then S3 Object Lock governance mode restrictions are bypassed and
+// an attempt is made to remove any S3 Object Lock legal holds.
+// Returns the number of objects deleted.
+func deletePageOfObjectVersions(ctx context.Context, conn *s3.S3, bucket string, force bool, page *s3.ListObjectVersionsOutput) (int64, error) {
+	var nObjects int64
+
+	toDelete := make([]*s3.ObjectIdentifier, 0, len(page.Versions))
+	for _, v := range page.Versions {
+		toDelete = append(toDelete, &s3.ObjectIdentifier{
+			Key:       v.Key,
+			VersionId: v.VersionId,
+		})
+	}
+
+	if nObjects = int64(len(toDelete)); nObjects == 0 {
+		return nObjects, nil
+	}
+
+	input := &s3.DeleteObjectsInput{
+		Bucket: aws.String(bucket),
+		Delete: &s3.Delete{
+			Objects: toDelete,
+			Quiet:   aws.Bool(true), // Only report errors.
+		},
+	}
+
+	if force {
+		input.BypassGovernanceRetention = aws.Bool(true)
+	}
+
+	output, err := conn.DeleteObjectsWithContext(ctx, input)
+
+	if tfawserr.ErrCodeEquals(err, s3.ErrCodeNoSuchBucket) {
+		return nObjects, nil
+	}
+
+	if err != nil {
+		return nObjects, fmt.Errorf("deleting S3 Bucket (%s) objects: %w", bucket, err)
+	}
+
+	nObjects -= int64(len(output.Errors))
+
+	var deleteErrs *multierror.Error
+
+	for _, v := range output.Errors {
+		code := aws.StringValue(v.Code)
+
+		if code == s3.ErrCodeNoSuchKey {
+			continue
+		}
+
+		// Attempt to remove any legal hold on the object.
+		if force && code == ErrCodeAccessDenied {
+			key := aws.StringValue(v.Key)
+			versionID := aws.StringValue(v.VersionId)
+
+			_, err := conn.PutObjectLegalHoldWithContext(ctx, &s3.PutObjectLegalHoldInput{
+				Bucket:    aws.String(bucket),
+				Key:       aws.String(key),
+				VersionId: aws.String(versionID),
+				LegalHold: &s3.ObjectLockLegalHold{
+					Status: aws.String(s3.ObjectLockLegalHoldStatusOff),
+				},
+			})
+
+			if err != nil {
+				// Add the original error and the new error.
+				deleteErrs = multierror.Append(deleteErrs, newDeleteS3ObjectVersionError(v))
+				deleteErrs = multierror.Append(deleteErrs, fmt.Errorf("removing legal hold: %w", newS3ObjectVersionError(key, versionID, err)))
+			} else {
+				// Attempt to delete the object once the legal hold has been removed.
+				_, err := conn.DeleteObjectWithContext(ctx, &s3.DeleteObjectInput{
+					Bucket:    aws.String(bucket),
+					Key:       aws.String(key),
+					VersionId: aws.String(versionID),
+				})
+
+				if err != nil {
+					deleteErrs = multierror.Append(deleteErrs, fmt.Errorf("deleting: %w", newS3ObjectVersionError(key, versionID, err)))
+				} else {
+					nObjects++
+				}
+			}
+		} else {
+			deleteErrs = multierror.Append(deleteErrs, newDeleteS3ObjectVersionError(v))
+		}
+	}
+
+	if err := deleteErrs.ErrorOrNil(); err != nil {
+		return nObjects, fmt.Errorf("deleting S3 Bucket (%s) objects: %w", bucket, err)
+	}
+
+	return nObjects, nil
+}
+
+// deletePageOfDeleteMarkers deletes a page (<= 1000) of S3 object delete markers.
+// Returns the number of delete markers deleted.
+func deletePageOfDeleteMarkers(ctx context.Context, conn *s3.S3, bucket string, page *s3.ListObjectVersionsOutput) (int64, error) {
+	var nObjects int64
+
+	toDelete := make([]*s3.ObjectIdentifier, 0, len(page.Versions))
+	for _, v := range page.DeleteMarkers {
+		toDelete = append(toDelete, &s3.ObjectIdentifier{
+			Key:       v.Key,
+			VersionId: v.VersionId,
+		})
+	}
+
+	if nObjects = int64(len(toDelete)); nObjects == 0 {
+		return nObjects, nil
+	}
+
+	input := &s3.DeleteObjectsInput{
+		Bucket: aws.String(bucket),
+		Delete: &s3.Delete{
+			Objects: toDelete,
+			Quiet:   aws.Bool(true), // Only report errors.
+		},
+	}
+
+	output, err := conn.DeleteObjectsWithContext(ctx, input)
+
+	if tfawserr.ErrCodeEquals(err, s3.ErrCodeNoSuchBucket) {
+		return nObjects, nil
+	}
+
+	if err != nil {
+		return nObjects, fmt.Errorf("deleting S3 Bucket (%s) delete markers: %w", bucket, err)
+	}
+
+	nObjects -= int64(len(output.Errors))
+
+	var deleteErrs *multierror.Error
+
+	for _, v := range output.Errors {
+		deleteErrs = multierror.Append(deleteErrs, newDeleteS3ObjectVersionError(v))
+	}
+
+	if err := deleteErrs.ErrorOrNil(); err != nil {
+		return nObjects, fmt.Errorf("deleting S3 Bucket (%s) delete markers: %w", bucket, err)
+	}
+
+	return nObjects, nil
+}
+
+func newS3ObjectVersionError(key, versionID string, err error) error {
+	if err == nil {
+		return nil
+	}
+
+	if versionID == "" {
+		return fmt.Errorf("S3 object (%s): %w", key, err)
+	}
+
+	return fmt.Errorf("S3 object (%s) version (%s): %w", key, versionID, err)
+}
+
+func newDeleteS3ObjectVersionError(err *s3.Error) error {
+	if err == nil {
+		return nil
+	}
+
+	awsErr := awserr.New(aws.StringValue(err.Code), aws.StringValue(err.Message), nil)
+
+	return fmt.Errorf("deleting: %w", newS3ObjectVersionError(aws.StringValue(err.Key), aws.StringValue(err.VersionId), awsErr))
+}

internal/service/s3/delete_test.go

@@ -0,0 +1,51 @@
+package s3_test
+
+import (
+	"context"
+	"flag"
+	"testing"
+
+	"github.com/aws/aws-sdk-go/aws/session"
+	"github.com/aws/aws-sdk-go/service/s3"
+	tfs3 "github.com/hashicorp/terraform-provider-aws/internal/service/s3"
+)
+
+// AWS_REGION=us-west-2 go test -v ./internal/service/s3 -run=TestEmptyBucket -b ewbankkit-test-empty-bucket-001 -f
+
+var bucket = flag.String("b", "", "bucket")
+var force = flag.Bool("f", false, "force")
+
+func TestEmptyBucket(t *testing.T) {
+	if *bucket == "" {
+		t.Skip("bucket not specified")
+	}
+
+	sess := session.Must(session.NewSession())
+	svc := s3.New(sess)
+	ctx := context.Background()
+
+	n, err := tfs3.EmptyBucket(ctx, svc, *bucket, *force)
+
+	if err != nil {
+		t.Fatalf("error emptying S3 bucket (%s): %s", *bucket, err)
+	}
+
+	t.Logf("%d S3 objects deleted", n)
+}
+
+func TestDeleteAllObjectVersions(t *testing.T) {
+	if *bucket == "" {
+		t.Skip("bucket not specified")
+	}
+
+	sess := session.Must(session.NewSession())
+	svc := s3.New(sess)
+
+	n, err := tfs3.DeleteAllObjectVersions(svc, *bucket, "", *force, false)
+
+	if err != nil {
+		t.Fatalf("error emptying S3 bucket (%s): %s", *bucket, err)
+	}
+
+	t.Logf("%d S3 objects deleted", n)
+}

internal/service/s3/errors.go

@@ -4,6 +4,8 @@ package s3
 // https://docs.aws.amazon.com/sdk-for-go/api/service/s3/#pkg-constants
 
 const (
+	ErrCodeAccessDenied                              = "AccessDenied"
+	ErrCodeBucketNotEmpty                            = "BucketNotEmpty"
 	ErrCodeInvalidBucketState                        = "InvalidBucketState"
 	ErrCodeInvalidRequest                            = "InvalidRequest"
 	ErrCodeMalformedPolicy                           = "MalformedPolicy"