feat: Add crc32c checksums to S3 Service #4533
Conversation
|
This has me scratching my head a bit... I tried to remove the checksum header from multipart upload requests, but then I get the error Could it be that multipartuplaod only supports the |
We need to specify |
core/src/services/s3/core.rs
Outdated
| @@ -821,6 +846,17 @@ pub struct OutputCommonPrefix { | |||
| pub prefix: String, | |||
| } | |||
|
|
|||
| pub enum S3ChecksumAlgorithm { | |||
core/src/services/s3/core.rs
Outdated
| if let Some(checksum_algorithm) = self.checksum_algorithm.as_ref() { | ||
| let checksum = match checksum_algorithm { | ||
| S3ChecksumAlgorithm::Crc32c => { | ||
| BASE64_STANDARD.encode(crc32c::crc32c(body.to_vec().as_slice()).to_be_bytes()) |
There was a problem hiding this comment.
Please don't call body.to_vec() which invovles extra allocation and copy. Please call crc32c_append in loop instead.
| BASE64_STANDARD.encode(crc32c::crc32c(body.to_vec().as_slice()).to_be_bytes()) | ||
| ChecksumAlgorithm::Crc32c => { | ||
| let mut crc = 0u32; | ||
| body.clone() |
There was a problem hiding this comment.
This clone should be okay, since it does not do any allocation (?)
core/src/services/s3/core.rs
Outdated
| Crc32c, | ||
| } | ||
| impl ChecksumAlgorithm { | ||
| pub fn to_header_key(&self) -> &str { |
There was a problem hiding this comment.
How about using to_header_name()? And I think we can use HeaderName here directly.
core/src/services/s3/core.rs
Outdated
| req = req.header( | ||
| "x-amz-checksum-algorithm", | ||
| match checksum_algorithm { | ||
| ChecksumAlgorithm::Crc32c => "CRC32C", |
There was a problem hiding this comment.
We can implement Display for ChecksumAlgorithm
Xuanwo
changed the title
|
Awesome, Thank you too! |
| @@ -350,6 +350,7 @@ prometheus-client = { version = "0.22.2", optional = true } | |||
| tracing = { version = "0.1", optional = true } | |||
| # for layers-dtrace | |||
| probe = { version = "0.5.1", optional = true } | |||
| crc32c = "0.6.5" | |||
There was a problem hiding this comment.
This dependency doesn't seem to be grouped cleanly like others.
There was a problem hiding this comment.
Nice catch, would like to send a PR to fix this? Also, I think this dep should be hidden under services-s3.
For #4527