-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
[v2] Turn on checksum validation for CRT S3 client #8298
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## v2-s3-crt #8298 +/- ##
=============================================
- Coverage 93.19% 93.18% -0.01%
=============================================
Files 364 364
Lines 38506 38519 +13
Branches 6169 6171 +2
=============================================
+ Hits 35884 35893 +9
- Misses 1950 1954 +4
Partials 672 672 ☔ View full report in Codecov by Sentry. |
For uploads, the CRT S3 client will add CRC32 trailing checksums. For downloads, the CRT S3 client will validate checksums associated to the object when possible.
daea3a0
to
3a7e6e4
Compare
@@ -206,6 +206,7 @@ def upload(self, fileobj, bucket, key, extra_args=None, subscribers=None): | |||
extra_args = {} | |||
if subscribers is None: | |||
subscribers = {} | |||
self._validate_checksum_algorithm_supported(extra_args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
somewhat offtopic: Noticing that CRTTransferManager doesn't validate input as extensively as the classic TransferManager:
aws-cli/awscli/s3transfer/manager.py
Lines 318 to 319 in d61d077
self._validate_all_known_args(extra_args, self.ALLOWED_UPLOAD_ARGS) | |
self._validate_if_bucket_supported(bucket) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think we should follow up on validating extra args in another PR.
This is was made to keep parity with the pure Python transfer manager which accepts lowercase algorithm values.
[v2] Turn on checksum validation for CRT S3 client
[v2] Turn on checksum validation for CRT S3 client
[v2] Turn on checksum validation for CRT S3 client
For uploads, the CRT S3 client will add CRC32 trailing checksums. For downloads, the CRT S3 client will validate checksums associated to the object when possible. This is also inline with the proposed changes in this s3transfer PR: boto/s3transfer#277. The plan is backport this to s3transfer once merged.