-
Notifications
You must be signed in to change notification settings - Fork 544
Use CRC32C for checksums instead of MD5 #6442
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
base: main
Are you sure you want to change the base?
Changes from all commits
207cf76
600281b
78d6275
f7f6a45
92d1fbf
9f2cfdf
ed54ce6
4169a3a
44198f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,20 @@ pub enum StorageBackend { | |
| S3, | ||
| } | ||
|
|
||
| /// Strategy used to checksum object-storage uploads. | ||
| #[derive(Debug, Clone, Copy, Default, Eq, PartialEq, Serialize, Deserialize)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum ChecksumAlgorithm { | ||
| /// CRC32C, computed and validated by the AWS SDK. Native S3 default. | ||
| #[default] | ||
| Crc32c, | ||
| /// MD5 (Content-MD5 header), computed client-side. Used by S3-compatible | ||
| /// implementations that predate the SDK's `x-amz-checksum-*` headers. | ||
| Md5, | ||
| /// No upload checksum is sent and no response checksum is validated. | ||
| Disabled, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Copy, Eq, PartialEq, Ord, PartialOrd, Hash, Serialize, Deserialize)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum StorageBackendFlavor { | ||
|
|
@@ -330,7 +344,10 @@ pub struct S3StorageConfig { | |
| #[serde(default)] | ||
| pub disable_multipart_upload: bool, | ||
| #[serde(default)] | ||
| pub disable_checksums: bool, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that does not look backward compatible. I think noone uses it, and they will get a clear error message, so it is probably ok if it is a calculated risk.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nadav, you can make this backward compatible with some serde trick. We can use an alias and then deserialize |
||
| pub checksum_algorithm: ChecksumAlgorithm, | ||
| /// Deprecated: applies into `checksum_algorithm: disabled`. | ||
| #[serde(default, skip_serializing)] | ||
| pub disable_checksums: Option<bool>, | ||
| #[serde(default)] | ||
| pub disable_stalled_stream_protection_upload: bool, | ||
| #[serde(default)] | ||
|
|
@@ -343,25 +360,27 @@ impl S3StorageConfig { | |
| Some(StorageBackendFlavor::DigitalOcean) => { | ||
| self.force_path_style_access = true; | ||
| self.disable_multi_object_delete = true; | ||
| self.disable_checksums = true; | ||
| } | ||
| Some(StorageBackendFlavor::Garage) => { | ||
| self.region = Some("garage".to_string()); | ||
| self.force_path_style_access = true; | ||
| self.disable_checksums = true; | ||
| } | ||
| Some(StorageBackendFlavor::Gcs) => { | ||
| self.disable_multi_object_delete = true; | ||
| self.disable_multipart_upload = true; | ||
| self.disable_checksums = true; | ||
| // doesnt support CRC32C via the S3 SDK | ||
| self.checksum_algorithm = ChecksumAlgorithm::Disabled; | ||
| } | ||
| Some(StorageBackendFlavor::MinIO) => { | ||
| self.region = Some("minio".to_string()); | ||
| self.force_path_style_access = true; | ||
| self.disable_checksums = true; | ||
| } | ||
| _ => {} | ||
| } | ||
| // Legacy: honor `disable_checksums: true` from older configs. | ||
| if matches!(self.disable_checksums, Some(true)) { | ||
| self.checksum_algorithm = ChecksumAlgorithm::Disabled; | ||
| } | ||
| } | ||
|
|
||
| pub fn redact(&mut self) { | ||
|
|
@@ -404,7 +423,7 @@ impl fmt::Debug for S3StorageConfig { | |
| &self.disable_multi_object_delete, | ||
| ) | ||
| .field("disable_multipart_upload", &self.disable_multipart_upload) | ||
| .field("disable_checksums", &self.disable_checksums) | ||
| .field("checksum_algorithm", &self.checksum_algorithm) | ||
| .field( | ||
| "disable_stalled_stream_protection_upload", | ||
| &self.disable_stalled_stream_protection_upload, | ||
|
|
@@ -647,7 +666,7 @@ mod tests { | |
| force_path_style_access: true | ||
| disable_multi_object_delete_requests: true | ||
| disable_multipart_upload: true | ||
| disable_checksums: true | ||
| checksum_algorithm: disabled | ||
| disable_stalled_stream_protection_upload: true | ||
| disable_stalled_stream_protection_download: true | ||
| "#; | ||
|
|
@@ -660,7 +679,7 @@ mod tests { | |
| force_path_style_access: true, | ||
| disable_multi_object_delete: true, | ||
| disable_multipart_upload: true, | ||
| disable_checksums: true, | ||
| checksum_algorithm: ChecksumAlgorithm::Disabled, | ||
| disable_stalled_stream_protection_upload: true, | ||
| disable_stalled_stream_protection_download: true, | ||
| ..Default::default() | ||
|
|
||
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.
The new formatting of the table is gross and makes the diff harder to review.