Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle older S3 packages for operations that require MD5 or checksums #3642

Merged
merged 2 commits into from
Feb 7, 2025

Conversation

dscpinheiro
Copy link
Contributor

@dscpinheiro dscpinheiro commented Feb 7, 2025

Fixes #3641

Description

In the ChecksumUtils class (the component responsible for calculating the request checksums), I added some logic to handle newer Core packages + older S3 packages, however I only checked the multi-part upload scenario (which I knew it was going to break).

However, the approach I chose (to skip checksums entirely) breaks other use cases, such as DeleteObjects which will fail if there's no Content-MD5 OR x-amz-checksum-* headers in the request.

This PR expands the workaround we have (which has already been removed from V4), to use MD5 when needed.

Testing

  • Dry-run (in progress): DRY_RUN-198ffecc-a108-4f13-92a1-87e14c9238ab
  • Ran a console app locally using latest Core and S3 before the data integrity changes:
var s3 = new AmazonS3Client(RegionEndpoint.USWest2);

// Works
var deleteObjectsRequest = new DeleteObjectsRequest
{
    BucketName = "my-bucket",
    Objects = [new KeyVersion() { Key = "partitions.json" }]
};
await s3.DeleteObjectsAsync(deleteObjectsRequest);

// The original workaround was in place for directory buckets and MPUs, confirmed it still works as expected
var tu = new TransferUtility(s3);
await tu.UploadAsync(new TransferUtilityUploadRequest
{
    BucketName = "test-usw2-az1--x-s3",
    Key = "test.zip",
    FilePath = @"C:\Users\dspin\Downloads\net472.zip",
});

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • My code follows the code style of this project
  • All new and existing tests passed

License

  • I confirm that this pull request can be released under the Apache 2 license

@dscpinheiro dscpinheiro marked this pull request as ready for review February 7, 2025 00:34
@afroz429 afroz429 self-requested a review February 7, 2025 00:45
@dscpinheiro
Copy link
Contributor Author

dscpinheiro commented Feb 7, 2025

@boblodgett @normj @afroz429 I had to add another commit (780ed4e), as it looks like the previous fix we made for S3 (#3631) also broke PutObject calls when mixing old S3 and new Core. That was because previous versions of S3 would wrap the customer input into an MD5Stream (which does not support seeking), and an exception would be thrown when trying to access contentStream.Position:

AmazonS3Client 35|2025-02-06T21:17:38.634Z|ERROR|An exception of type NotSupportedException was handled in ErrorHandler. --> System.NotSupportedException: HashStream does not support seeking
AmazonS3Client 36|2025-02-06T21:17:38.864Z|ERROR|NotSupportedException making request PutObjectRequest to https://dspin-test-accelerate.s3.us-west-2.amazonaws.com/. Attempt 1. --> System.NotSupportedException: HashStream does not support seeking

I did more manual testing (in both .NET 8 and .NET Framework) and my app succeeded, but can you please take a look at the changes again?

var bucketName = "dspin-test-accelerate";

var s3 = new AmazonS3Client(RegionEndpoint.USWest2);
await s3.PutObjectAsync(new PutObjectRequest
{
    BucketName = bucketName,
    Key = "partitions.json",
    FilePath = @"C:\Users\dspin\Downloads\partitions.json",
});
await s3.DeleteObjectsAsync(new DeleteObjectsRequest
{
    BucketName = bucketName,
    Objects = [new KeyVersion() { Key = "partitions.json" }]
});

var tu = new TransferUtility(s3);
await tu.UploadAsync(@"C:\Users\dspin\Downloads\partitions.json", bucketName, "test-tu.json");
await tu.UploadAsync(new TransferUtilityUploadRequest
{
    BucketName = "test-dspin--usw2-az1--x-s3",
    Key = "test.zip",
    FilePath = @"C:\Users\dspin\Downloads\net472.zip",
});

Edit: I also checked that the scenario reported on the other issue (a memory stream with a position greater than 0 and AutoResetStreamPosition set to false) still works when only updating Core.

@dscpinheiro dscpinheiro merged commit 9cd49b6 into main-staging Feb 7, 2025
2 checks passed
@dscpinheiro dscpinheiro deleted the dspin/gh-3641 branch February 7, 2025 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants