feature(s3/manager): add option to control default checksums #3151
Conversation
stanhu
force-pushed
the
sh-fix-issue-3007-checksums-try2
branch
from
d755c3b to
b32d2b6
Compare
stanhu
force-pushed
the
sh-fix-issue-3007-checksums-try2
branch
from
b32d2b6 to
a6b9cb3
Compare
feature/s3/manager/upload.go
Outdated
| // isS3ExpressBucket returns true if the bucket has the S3 Express suffix. | ||
| func (u *multiuploader) isS3ExpressBucket() bool { | ||
| bucketName := aws.ToString(u.in.Bucket) | ||
| return strings.HasSuffix(bucketName, "--x-s3") |
There was a problem hiding this comment.
I don't believe this is actually necessary -- if you look at line 336 in this file, we add a post-endpoint-resolution check that defaults the checksum if the endpoint resolution process determined that we were using an express bucket. You should be able to just verify this with an additional unit test.
There was a problem hiding this comment.
@lucix-aws Thanks. I'm not quite sure how to add a unit test here because that checksum is added in HandleFinalize, and the mock request doesn't seem to get there.
There was a problem hiding this comment.
the mock request doesn't seem to get there
I don't really understand what this means. No you're right actually, because that middleware is added in the low-level s3 client which the tests in this package would all be mocking. So this would have to be done in an integration test, which we as the SDK team would have to handle.
There was a problem hiding this comment.
I will try to find some time to take care of that next week.
lucix-aws
added
the
response-requested
|
Greetings! It looks like this PR hasn’t been active in longer than a week, add a comment or an upvote to prevent automatic closure, or if the issue is already closed, please feel free to open a new one. |
stanhu
force-pushed
the
sh-fix-issue-3007-checksums-try2
branch
from
a6b9cb3 to
36f5784
Compare
As announced in aws#2960, AWS SDK for Go v2 service/s3 v1.73.0 shipped a change (aws#2808) that adopted new default integrity protections, automatically calculating CRC32 checksums for operations like PutObject and UploadPart. While it is possible to revert to the previous behavior by setting `AWS_REQUEST_CHECKSUM_CALCULATION=when_required`, this config setting did not apply to the S3 Manager multipart uploader, which always enabled CRC32 checksums by default regardless of the global setting. This commit adds a new `RequestChecksumCalculation` field to the Uploader struct that allows users to control checksum behavior: - `RequestChecksumCalculationWhenSupported` (default): Always calculates CRC32 checksums for multipart uploads - `RequestChecksumCalculationWhenRequired`: Only calculates checksums when explicitly set by the user, preserving backwards compatibility For example: ```go uploader := manager.NewUploader(client, func(u *manager.Uploader) { u.RequestChecksumCalculation = aws.RequestChecksumCalculationWhenRequired }) ``` S3 Express One Zone buckets always require CRC32 checksums regardless of this setting, as mandated by the S3 Express service requirements. The uploader automatically detects S3 Express buckets (names ending with `--x-s3`) and applies CRC32 checksums unconditionally. Fixes aws#3007 Signed-off-by: Stan Hu <[email protected]>
stanhu
force-pushed
the
sh-fix-issue-3007-checksums-try2
branch
from
24798f7 to
fb4dd5a
Compare
lucix-aws
added
response-requested
|
Patch looks fine, approval pending integration tests but I'll run CI now. Ignore failing codegen and integration tests, those don't work in forks. |
As announced in #2960, AWS SDK for Go v2 service/s3 v1.73.0 shipped a change (#2808) that adopted new default integrity protections, automatically calculating CRC32 checksums for operations like PutObject and UploadPart.
While it is possible to revert to the previous behavior by setting
AWS_REQUEST_CHECKSUM_CALCULATION=when_required, this config setting did not apply to the S3 Manager multipart uploader, which always enabled CRC32 checksums by default regardless of the global setting.This commit adds a new
RequestChecksumCalculationfield to the Uploader struct that allows users to control checksum behavior:RequestChecksumCalculationWhenSupported(default): Always calculates CRC32 checksums for multipart uploadsRequestChecksumCalculationWhenRequired: Only calculates checksums when explicitly set by the user, preserving backwards compatibilityFor example:
S3 Express One Zone buckets always require CRC32 checksums regardless of this setting, as mandated by the S3 Express service requirements. The uploader automatically detects S3 Express buckets (names ending with
--x-s3) and applies CRC32 checksums unconditionally.Fixes #3007