r/aws_s3_bucket: Allow deletion of objects with non-XML-safe bytes in their keys

[ravron]

Description

This PR teaches aws_s3_bucket and aws_s3_directory_bucket to handle non-XML-safe characters in object keys when emptying bucket contents on destroy because force_destroy is true. This PR addresses #40489, which contains more details on the issue, its cause, and the resolution.

The fundamental problem is that S3 object keys can contain any Unicode characters, but XML cannot encode all Unicode characters (see section 2.2 of the XML spec for allowed characters). For example, U+10 is a disallowed character. As a result, ListObjectVersions can return XML containing disallowed characters when listing objects whose keys themselves contain those characters, and the AWS Go SDK correctly refuses to parse the XML.

This is a well-known issue; see the references. The solution is to tell ListObjectVersions to URL-encode returned keys, which is easily done by setting EncodingType: types.EncodingTypeUrl, then URL-decode the keys on the client side. This works, but causes a new issue: DeleteObjectVersions will silently fail to delete any object whose key contains these invalid characters. This is because DeleteObjectVersions uses an XML request body, and Go will replace invalid characters with � (U+FFFD, the replacement character) in the request body.

This PR modifies several functions in internal/service/s3/delete.go with two goals:

1. Request URL-escaped object keys in ListObjectVersions and ListObjectsV2 responses, and unescape those keys after they are returned.
2. Split keys to be deleted into two groups: those that are XML-safe, and those that are not. XML-safe keys can be deleted in batches of 1,000 as before, with DeleteObjects. Non-XML-safe keys must be deleted individually, with DeleteObject.

While here, I consolidated the implementations of deletePageOfObjectVersions, deletePageOfDeleteMarkers, and deletePageOfObjects.

Relations

Closes #40489.

References

Here are some issues about the challenges of listing and bulk-deleting objects whose keys contain non-XML-safe characters:

• If the key of the object contains control characters, delete () of s3.Bucket.objectsCollection fails. boto/boto3#2005
• Can't multi-delete in S3 keys with XML characters in them aws/aws-sdk-php#1106
• service/s3: ListObjectsV2Pages breaks on keys containing illegal characters aws/aws-sdk-go#1914

Output from Acceptance Testing

$ make testacc TESTS='TestAccS3Bucket_Basic_forceDestroy|TestAccS3DirectoryBucket_forceDestroy' PKG=s3
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.3 test ./internal/service/s3/... -v -count 1 -parallel 20 -run='TestAccS3Bucket_Basic_forceDestroy|TestAccS3DirectoryBucket_forceDestroy'  -timeout 360m
2024/12/11 15:29:56 Initializing Terraform AWS Provider...
=== RUN   TestAccS3Bucket_Basic_forceDestroy
=== PAUSE TestAccS3Bucket_Basic_forceDestroy
=== RUN   TestAccS3Bucket_Basic_forceDestroyWithUnusualKeyBytes
=== PAUSE TestAccS3Bucket_Basic_forceDestroyWithUnusualKeyBytes
=== RUN   TestAccS3Bucket_Basic_forceDestroyWithObjectVersions
=== PAUSE TestAccS3Bucket_Basic_forceDestroyWithObjectVersions
=== RUN   TestAccS3Bucket_Basic_forceDestroyWithObjectVersionsUnusualKeyBytes
=== PAUSE TestAccS3Bucket_Basic_forceDestroyWithObjectVersionsUnusualKeyBytes
=== RUN   TestAccS3Bucket_Basic_forceDestroyWithEmptyPrefixes
=== PAUSE TestAccS3Bucket_Basic_forceDestroyWithEmptyPrefixes
=== RUN   TestAccS3Bucket_Basic_forceDestroyWithObjectLockEnabled
=== PAUSE TestAccS3Bucket_Basic_forceDestroyWithObjectLockEnabled
=== RUN   TestAccS3DirectoryBucket_forceDestroy
=== PAUSE TestAccS3DirectoryBucket_forceDestroy
=== RUN   TestAccS3DirectoryBucket_forceDestroyWithUnusualKeyBytes
=== PAUSE TestAccS3DirectoryBucket_forceDestroyWithUnusualKeyBytes
=== CONT  TestAccS3Bucket_Basic_forceDestroy
=== CONT  TestAccS3Bucket_Basic_forceDestroyWithEmptyPrefixes
=== CONT  TestAccS3Bucket_Basic_forceDestroyWithObjectVersions
=== CONT  TestAccS3DirectoryBucket_forceDestroy
=== CONT  TestAccS3Bucket_Basic_forceDestroyWithObjectLockEnabled
=== CONT  TestAccS3Bucket_Basic_forceDestroyWithObjectVersionsUnusualKeyBytes
=== CONT  TestAccS3Bucket_Basic_forceDestroyWithUnusualKeyBytes
=== CONT  TestAccS3DirectoryBucket_forceDestroyWithUnusualKeyBytes
--- PASS: TestAccS3Bucket_Basic_forceDestroyWithUnusualKeyBytes (29.32s)
--- PASS: TestAccS3Bucket_Basic_forceDestroy (30.95s)
--- PASS: TestAccS3Bucket_Basic_forceDestroyWithEmptyPrefixes (30.96s)
--- PASS: TestAccS3DirectoryBucket_forceDestroy (31.52s)
--- PASS: TestAccS3DirectoryBucket_forceDestroyWithUnusualKeyBytes (32.00s)
--- PASS: TestAccS3Bucket_Basic_forceDestroyWithObjectLockEnabled (33.76s)
--- PASS: TestAccS3Bucket_Basic_forceDestroyWithObjectVersionsUnusualKeyBytes (33.90s)
--- PASS: TestAccS3Bucket_Basic_forceDestroyWithObjectVersions (34.28s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/s3	43.652s

[github-actions]

Community Note

Voting for Prioritization

• Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
• Please see our prioritization guide for information on how we prioritize.
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

For Submitters

• Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
• For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
• Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

[github-actions]

Welcome @ravron 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTOR guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

[ewbankkit]

LGTM 🚀.

% make testacc TESTARGS='-run=TestAccS3Bucket_Basic_forceDestroy\|TestAccS3DirectoryBucket_forceDestroy' PKG=s3 ACCTEST_PARALLELISM=3
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.3 test ./internal/service/s3/... -v -count 1 -parallel 3  -run=TestAccS3Bucket_Basic_forceDestroy\|TestAccS3DirectoryBucket_forceDestroy -timeout 360m
2024/12/18 15:57:47 Initializing Terraform AWS Provider...
=== RUN   TestAccS3Bucket_Basic_forceDestroy
=== PAUSE TestAccS3Bucket_Basic_forceDestroy
=== RUN   TestAccS3Bucket_Basic_forceDestroyWithUnusualKeyBytes
=== PAUSE TestAccS3Bucket_Basic_forceDestroyWithUnusualKeyBytes
=== RUN   TestAccS3Bucket_Basic_forceDestroyWithObjectVersions
=== PAUSE TestAccS3Bucket_Basic_forceDestroyWithObjectVersions
=== RUN   TestAccS3Bucket_Basic_forceDestroyWithObjectVersionsUnusualKeyBytes
=== PAUSE TestAccS3Bucket_Basic_forceDestroyWithObjectVersionsUnusualKeyBytes
=== RUN   TestAccS3Bucket_Basic_forceDestroyWithEmptyPrefixes
=== PAUSE TestAccS3Bucket_Basic_forceDestroyWithEmptyPrefixes
=== RUN   TestAccS3Bucket_Basic_forceDestroyWithObjectLockEnabled
=== PAUSE TestAccS3Bucket_Basic_forceDestroyWithObjectLockEnabled
=== RUN   TestAccS3DirectoryBucket_forceDestroy
=== PAUSE TestAccS3DirectoryBucket_forceDestroy
=== RUN   TestAccS3DirectoryBucket_forceDestroyWithUnusualKeyBytes
=== PAUSE TestAccS3DirectoryBucket_forceDestroyWithUnusualKeyBytes
=== CONT  TestAccS3Bucket_Basic_forceDestroy
=== CONT  TestAccS3Bucket_Basic_forceDestroyWithEmptyPrefixes
=== CONT  TestAccS3Bucket_Basic_forceDestroyWithObjectVersions
--- PASS: TestAccS3Bucket_Basic_forceDestroy (16.17s)
=== CONT  TestAccS3Bucket_Basic_forceDestroyWithObjectVersionsUnusualKeyBytes
--- PASS: TestAccS3Bucket_Basic_forceDestroyWithEmptyPrefixes (16.45s)
=== CONT  TestAccS3DirectoryBucket_forceDestroy
--- PASS: TestAccS3Bucket_Basic_forceDestroyWithObjectVersions (20.52s)
=== CONT  TestAccS3DirectoryBucket_forceDestroyWithUnusualKeyBytes
--- PASS: TestAccS3DirectoryBucket_forceDestroy (14.69s)
=== CONT  TestAccS3Bucket_Basic_forceDestroyWithObjectLockEnabled
--- PASS: TestAccS3Bucket_Basic_forceDestroyWithObjectVersionsUnusualKeyBytes (17.80s)
=== CONT  TestAccS3Bucket_Basic_forceDestroyWithUnusualKeyBytes
--- PASS: TestAccS3DirectoryBucket_forceDestroyWithUnusualKeyBytes (14.24s)
--- PASS: TestAccS3Bucket_Basic_forceDestroyWithUnusualKeyBytes (14.33s)
--- PASS: TestAccS3Bucket_Basic_forceDestroyWithObjectLockEnabled (18.77s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/s3	55.276s

[jar-b]

LGTM 🚀

[ewbankkit]

@ravron Thanks for the contribution 🎉 👏.

[github-actions]

This functionality has been released in v5.82.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.