-
Notifications
You must be signed in to change notification settings - Fork 546
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
Open
nadav-govari
wants to merge
12
commits into
main
Choose a base branch
from
nadav/crc32
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
207cf76
Use CRC32C for checksums instead of MD5
nadav-govari 600281b
Only apply crc32c on non-S3 flavors
nadav-govari 78d6275
lints
nadav-govari f7f6a45
Make crc32 default and a turn-off button for it
nadav-govari 92d1fbf
lints
nadav-govari 9f2cfdf
Make it an enum
nadav-govari ed54ce6
restore the md5 computation since its a noop on the aws sdk
nadav-govari 4169a3a
rename to checksum algorithm, update docs, backwards compat
nadav-govari 44198f8
lints
nadav-govari 2f7b61a
make compute_md5 blocking
nadav-govari 50aeb53
fix docs layout
nadav-govari 157fe62
one more change
nadav-govari File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,30 @@ 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, | ||
| } | ||
|
|
||
| impl ChecksumAlgorithm { | ||
| pub fn is_md5(&self) -> bool { | ||
| matches!(self, Self::Md5) | ||
| } | ||
|
|
||
| pub fn is_disabled(&self) -> bool { | ||
| matches!(self, Self::Disabled) | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Copy, Eq, PartialEq, Ord, PartialOrd, Hash, Serialize, Deserialize)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum StorageBackendFlavor { | ||
|
|
@@ -330,7 +354,10 @@ pub struct S3StorageConfig { | |
| #[serde(default)] | ||
| pub disable_multipart_upload: bool, | ||
| #[serde(default)] | ||
| pub disable_checksums: bool, | ||
| pub checksum_algorithm: ChecksumAlgorithm, | ||
| /// Deprecated: applies into `checksum_algorithm: disabled`. | ||
| #[serde(default, skip_serializing)] | ||
|
Collaborator
Author
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. remove the option |
||
| pub disable_checksums: Option<bool>, | ||
| #[serde(default)] | ||
| pub disable_stalled_stream_protection_upload: bool, | ||
| #[serde(default)] | ||
|
|
@@ -343,25 +370,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 +433,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 +676,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 +689,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() | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
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.
Nadav, you can make this backward compatible with some serde trick. We can use an alias and then deserialize
falseto disabled and true to the new default.