Merge pull request #34647 from hashicorp/f-s3-express-tidyup

S3 Express post-release fixes

Author: ewbankkit

.changelog/34647.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+resource/aws_s3_directory_bucket: Fix `NotImplemented: This bucket does not support Object Versioning` errors on resource Delete when `force_destroy` is `true`
+```

go.mod

@@ -95,7 +95,6 @@ require (
 	github.com/aws/aws-sdk-go-v2/service/vpclattice v1.5.1
 	github.com/aws/aws-sdk-go-v2/service/workspaces v1.35.1
 	github.com/aws/aws-sdk-go-v2/service/xray v1.23.1
-	github.com/aws/smithy-go v1.18.1
 	github.com/beevik/etree v1.2.0
 	github.com/davecgh/go-spew v1.1.1
 	github.com/gertd/go-pluralize v0.2.1
@@ -158,6 +157,7 @@ require (
 	github.com/aws/aws-sdk-go-v2/service/internal/s3shared v1.16.7 // indirect
 	github.com/aws/aws-sdk-go-v2/service/sso v1.18.1 // indirect
 	github.com/aws/aws-sdk-go-v2/service/ssooidc v1.21.1 // indirect
+	github.com/aws/smithy-go v1.18.1 // indirect
 	github.com/bgentry/speakeasy v0.1.0 // indirect
 	github.com/boombuler/barcode v1.0.1 // indirect
 	github.com/bufbuild/protocompile v0.6.0 // indirect

internal/service/s3/bucket.go

@@ -23,7 +23,6 @@ import (
 	"github.com/aws/aws-sdk-go/aws/request"
 	"github.com/aws/aws-sdk-go/service/s3"
 	"github.com/aws/aws-sdk-go/service/s3/s3manager"
-	smithy "github.com/aws/smithy-go"
 	"github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr"
 	tfawserr_sdkv2 "github.com/hashicorp/aws-sdk-go-base/v2/tfawserr"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
@@ -1442,25 +1441,13 @@ func findBucket(ctx context.Context, conn *s3_sdkv2.Client, bucket string, optFn
 
 	_, err := conn.HeadBucket(ctx, input, optFns...)
 
-	if tfawserr_sdkv2.ErrHTTPStatusCodeEquals(err, http.StatusNotFound) || tfawserr_sdkv2.ErrCodeEquals(err, errCodeNoSuchBucket) {
+	if tfawserr_sdkv2.ErrHTTPStatusCodeEquals(err, http.StatusNotFound) || tfawserr_sdkv2.ErrCodeEquals(err, errCodeNoSuchBucket) || errs.Contains(err, errCodeNoSuchBucket) {
 		return &retry.NotFoundError{
 			LastError:   err,
 			LastRequest: input,
 		}
 	}
 
-	// FIXME Move to aws-sdk-go-base
-	// FIXME &smithy.OperationError{ServiceID:"S3", OperationName:"HeadBucket", Err:(*errors.errorString)(0xc00202bb60)}
-	// FIXME "operation error S3: HeadBucket, get identity: get credentials: operation error S3: CreateSession, https response error StatusCode: 404, RequestID: 0033eada6b00018c17de82890509d9eada65ba39, HostID: F31dBn, NoSuchBucket:"
-	if operationErr, ok := errs.As[*smithy.OperationError](err); ok {
-		if strings.Contains(operationErr.Err.Error(), errCodeNoSuchBucket) {
-			return &retry.NotFoundError{
-				LastError:   err,
-				LastRequest: input,
-			}
-		}
-	}
-
 	return err
 }
 

internal/service/s3/delete.go

@@ -13,16 +13,13 @@ import (
 	"github.com/aws/aws-sdk-go-v2/service/s3"
 	"github.com/aws/aws-sdk-go-v2/service/s3/types"
 	"github.com/hashicorp/aws-sdk-go-base/v2/tfawserr"
+	tfslices "github.com/hashicorp/terraform-provider-aws/internal/slices"
 )
 
-const (
-	listObjectVersionsMaxKeys = 1000
-)
-
-// emptyBucket empties the specified S3 bucket by deleting all object versions and delete markers.
+// emptyBucket empties the specified S3 general purpose 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.
+// Returns the number of object versions and delete markers deleted.
 func emptyBucket(ctx context.Context, conn *s3.Client, bucket string, force bool) (int64, error) {
 	nObjects, err := forEachObjectVersionsPage(ctx, conn, bucket, func(ctx context.Context, conn *s3.Client, bucket string, page *s3.ListObjectVersionsOutput) (int64, error) {
 		return deletePageOfObjectVersions(ctx, conn, bucket, force, page)
@@ -38,13 +35,18 @@ func emptyBucket(ctx context.Context, conn *s3.Client, bucket string, force bool
 	return nObjects, err
 }
 
+// emptyDirectoryBucket empties the specified S3 directory bucket by deleting all objects.
+// Returns the number of objects deleted.
+func emptyDirectoryBucket(ctx context.Context, conn *s3.Client, bucket string) (int64, error) {
+	return forEachObjectsPage(ctx, conn, bucket, deletePageOfObjects)
+}
+
 // forEachObjectVersionsPage calls the specified function for each page returned from the S3 ListObjectVersionsPages API.
 func forEachObjectVersionsPage(ctx context.Context, conn *s3.Client, bucket string, fn func(ctx context.Context, conn *s3.Client, bucket string, page *s3.ListObjectVersionsOutput) (int64, error)) (int64, error) {
 	var nObjects int64
 
 	input := &s3.ListObjectVersionsInput{
-		Bucket:  aws.String(bucket),
-		MaxKeys: aws.Int32(listObjectVersionsMaxKeys),
+		Bucket: aws.String(bucket),
 	}
 	var lastErr error
 
@@ -72,21 +74,52 @@ func forEachObjectVersionsPage(ctx context.Context, conn *s3.Client, bucket stri
 	return nObjects, nil
 }
 
+// forEachObjectsPage calls the specified function for each page returned from the S3 ListObjectsV2 API.
+func forEachObjectsPage(ctx context.Context, conn *s3.Client, bucket string, fn func(ctx context.Context, conn *s3.Client, bucket string, page *s3.ListObjectsV2Output) (int64, error)) (int64, error) {
+	var nObjects int64
+
+	input := &s3.ListObjectsV2Input{
+		Bucket: aws.String(bucket),
+	}
+	var lastErr error
+
+	pages := s3.NewListObjectsV2Paginator(conn, input)
+	for pages.HasMorePages() {
+		page, err := pages.NextPage(ctx)
+
+		if err != nil {
+			return nObjects, fmt.Errorf("listing S3 bucket (%s) objects: %w", bucket, err)
+		}
+
+		n, err := fn(ctx, conn, bucket, page)
+		nObjects += n
+
+		if err != nil {
+			lastErr = err
+			break
+		}
+	}
+
+	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.Client, bucket string, force bool, page *s3.ListObjectVersionsOutput) (int64, error) {
-	var nObjects int64
-
-	toDelete := make([]types.ObjectIdentifier, 0, len(page.Versions))
-	for _, v := range page.Versions {
-		toDelete = append(toDelete, types.ObjectIdentifier{
+	toDelete := tfslices.ApplyToAll(page.Versions, func(v types.ObjectVersion) types.ObjectIdentifier {
+		return types.ObjectIdentifier{
 			Key:       v.Key,
 			VersionId: v.VersionId,
-		})
-	}
+		}
+	})
 
+	var nObjects int64
 	if nObjects = int64(len(toDelete)); nObjects == 0 {
 		return nObjects, nil
 	}
@@ -109,16 +142,14 @@ func deletePageOfObjectVersions(ctx context.Context, conn *s3.Client, bucket str
 	}
 
 	if err != nil {
-		return nObjects, fmt.Errorf("deleting S3 bucket (%s) objects: %w", bucket, err)
+		return nObjects, fmt.Errorf("deleting S3 bucket (%s) object versions: %w", bucket, err)
 	}
 
 	nObjects -= int64(len(output.Errors))
 
-	var deleteErrs []error
-
+	var errs []error
 	for _, v := range output.Errors {
 		code := aws.ToString(v.Code)
-
 		if code == errCodeNoSuchKey {
 			continue
 		}
@@ -139,8 +170,8 @@ func deletePageOfObjectVersions(ctx context.Context, conn *s3.Client, bucket str
 
 			if err != nil {
 				// Add the original error and the new error.
-				deleteErrs = append(deleteErrs, newDeleteObjectVersionError(v))
-				deleteErrs = append(deleteErrs, fmt.Errorf("removing legal hold: %w", newObjectVersionError(key, versionID, err)))
+				errs = append(errs, newDeleteObjectVersionError(v))
+				errs = append(errs, fmt.Errorf("removing legal hold: %w", newObjectVersionError(key, versionID, err)))
 			} else {
 				// Attempt to delete the object once the legal hold has been removed.
 				_, err := conn.DeleteObject(ctx, &s3.DeleteObjectInput{
@@ -150,18 +181,18 @@ func deletePageOfObjectVersions(ctx context.Context, conn *s3.Client, bucket str
 				})
 
 				if err != nil {
-					deleteErrs = append(deleteErrs, fmt.Errorf("deleting: %w", newObjectVersionError(key, versionID, err)))
+					errs = append(errs, fmt.Errorf("deleting: %w", newObjectVersionError(key, versionID, err)))
 				} else {
 					nObjects++
 				}
 			}
 		} else {
-			deleteErrs = append(deleteErrs, newDeleteObjectVersionError(v))
+			errs = append(errs, newDeleteObjectVersionError(v))
 		}
 	}
 
-	if err := errors.Join(deleteErrs...); err != nil {
-		return nObjects, fmt.Errorf("deleting S3 bucket (%s) objects: %w", bucket, err)
+	if err := errors.Join(errs...); err != nil {
+		return nObjects, fmt.Errorf("deleting S3 bucket (%s) object versions: %w", bucket, err)
 	}
 
 	return nObjects, nil
@@ -170,16 +201,14 @@ func deletePageOfObjectVersions(ctx context.Context, conn *s3.Client, bucket str
 // deletePageOfDeleteMarkers deletes a page (<= 1000) of S3 object delete markers.
 // Returns the number of delete markers deleted.
 func deletePageOfDeleteMarkers(ctx context.Context, conn *s3.Client, bucket string, page *s3.ListObjectVersionsOutput) (int64, error) {
-	var nObjects int64
-
-	toDelete := make([]types.ObjectIdentifier, 0, len(page.Versions))
-	for _, v := range page.DeleteMarkers {
-		toDelete = append(toDelete, types.ObjectIdentifier{
+	toDelete := tfslices.ApplyToAll(page.Versions, func(v types.ObjectVersion) types.ObjectIdentifier {
+		return types.ObjectIdentifier{
 			Key:       v.Key,
 			VersionId: v.VersionId,
-		})
-	}
+		}
+	})
 
+	var nObjects int64
 	if nObjects = int64(len(toDelete)); nObjects == 0 {
 		return nObjects, nil
 	}
@@ -204,19 +233,64 @@ func deletePageOfDeleteMarkers(ctx context.Context, conn *s3.Client, bucket stri
 
 	nObjects -= int64(len(output.Errors))
 
-	var deleteErrs []error
-
+	var errs []error
 	for _, v := range output.Errors {
-		deleteErrs = append(deleteErrs, newDeleteObjectVersionError(v))
+		errs = append(errs, newDeleteObjectVersionError(v))
 	}
 
-	if err := errors.Join(deleteErrs...); err != nil {
+	if err := errors.Join(errs...); err != nil {
 		return nObjects, fmt.Errorf("deleting S3 bucket (%s) delete markers: %w", bucket, err)
 	}
 
 	return nObjects, nil
 }
 
+// deletePageOfObjects deletes a page (<= 1000) of S3 objects.
+// Returns the number of objects deleted.
+func deletePageOfObjects(ctx context.Context, conn *s3.Client, bucket string, page *s3.ListObjectsV2Output) (int64, error) {
+	toDelete := tfslices.ApplyToAll(page.Contents, func(v types.Object) types.ObjectIdentifier {
+		return types.ObjectIdentifier{
+			Key: v.Key,
+		}
+	})
+
+	var nObjects int64
+	if nObjects = int64(len(toDelete)); nObjects == 0 {
+		return nObjects, nil
+	}
+
+	input := &s3.DeleteObjectsInput{
+		Bucket: aws.String(bucket),
+		Delete: &types.Delete{
+			Objects: toDelete,
+			Quiet:   aws.Bool(true), // Only report errors.
+		},
+	}
+
+	output, err := conn.DeleteObjects(ctx, input)
+
+	if tfawserr.ErrCodeEquals(err, 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 errs []error
+	for _, v := range output.Errors {
+		errs = append(errs, newDeleteObjectVersionError(v))
+	}
+
+	if err := errors.Join(errs...); err != nil {
+		return nObjects, fmt.Errorf("deleting S3 bucket (%s) objects: %w", bucket, err)
+	}
+
+	return nObjects, nil
+}
+
 func newObjectVersionError(key, versionID string, err error) error {
 	if err == nil {
 		return nil
@@ -235,20 +309,21 @@ func newDeleteObjectVersionError(err types.Error) error {
 	return fmt.Errorf("deleting: %w", newObjectVersionError(aws.ToString(err.Key), aws.ToString(err.VersionId), s3Err))
 }
 
-// deleteAllObjectVersions deletes all versions of a specified key from an S3 bucket.
+// deleteAllObjectVersions deletes all versions of a specified key from an S3 general purpose bucket.
 // If key is empty then all versions of all objects are deleted.
-// Set force to true to override any S3 object lock protections on object lock enabled buckets.
+// Set `force` to `true` to override any S3 object lock protections on object lock enabled buckets.
 // Returns the number of objects deleted.
+// Use `emptyBucket` to delete all versions of all objects in a bucket.
 func deleteAllObjectVersions(ctx context.Context, conn *s3.Client, bucket, key string, force, ignoreObjectErrors bool) (int64, error) {
-	var nObjects int64
+	if key == "" {
+		return 0, errors.New("use `emptyBucket` to delete all versions of all objects in an S3 general purpose bucket")
+	}
 
 	input := &s3.ListObjectVersionsInput{
-		Bucket:  aws.String(bucket),
-		MaxKeys: aws.Int32(listObjectVersionsMaxKeys),
-	}
-	if key != "" {
-		input.Prefix = aws.String(key)
+		Bucket: aws.String(bucket),
+		Prefix: aws.String(key),
 	}
+	var nObjects int64
 	var lastErr error
 
 	pages := s3.NewListObjectVersionsPaginator(conn, input)
@@ -267,7 +342,7 @@ func deleteAllObjectVersions(ctx context.Context, conn *s3.Client, bucket, key s
 			objectKey := aws.ToString(objectVersion.Key)
 			objectVersionID := aws.ToString(objectVersion.VersionId)
 
-			if key != "" && key != objectKey {
+			if key != objectKey {
 				continue
 			}
 
@@ -358,7 +433,7 @@ func deleteAllObjectVersions(ctx context.Context, conn *s3.Client, bucket, key s
 			deleteMarkerKey := aws.ToString(deleteMarker.Key)
 			deleteMarkerVersionID := aws.ToString(deleteMarker.VersionId)
 
-			if key != "" && key != deleteMarkerKey {
+			if key != deleteMarkerKey {
 				continue
 			}
 
@@ -383,7 +458,7 @@ func deleteAllObjectVersions(ctx context.Context, conn *s3.Client, bucket, key s
 }
 
 // deleteObjectVersion deletes a specific object version.
-// Set force to true to override any S3 object lock protections.
+// Set `force` to `true` to override any S3 object lock protections.
 func deleteObjectVersion(ctx context.Context, conn *s3.Client, b, k, v string, force bool) error {
 	input := &s3.DeleteObjectInput{
 		Bucket: aws.String(b),

internal/service/s3/directory_bucket.go

@@ -254,7 +254,7 @@ func (r *directoryBucketResource) Delete(ctx context.Context, request resource.D
 	if tfawserr.ErrCodeEquals(err, errCodeBucketNotEmpty) {
 		if data.ForceDestroy.ValueBool() {
 			// Empty the bucket and try again.
-			_, err = emptyBucket(ctx, conn, data.ID.ValueString(), false)
+			_, err = emptyDirectoryBucket(ctx, conn, data.ID.ValueString())
 
 			if err != nil {
 				response.Diagnostics.AddError(fmt.Sprintf("emptying S3 Directory Bucket (%s)", data.ID.ValueString()), err.Error())