-
Notifications
You must be signed in to change notification settings - Fork 375
feat(aws_s3): Add S3 compatibility environment variables for non-AWS services #198
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
feat(aws_s3): Add S3 compatibility environment variables for non-AWS services #198
Conversation
Add AWS_S3_DISABLE_SSE and AWS_S3_DISABLE_CHECKSUMS environment variables to improve compatibility with S3-compatible storage services like MinIO and Rook Ceph RGW. Changes: - Add AWS_S3_DISABLE_SSE environment variable to optionally disable server-side encryption headers - Add AWS_S3_DISABLE_CHECKSUMS environment variable to optionally disable CRC32 checksum headers - Update multipart upload creation to conditionally add headers - Update individual part uploads to conditionally add checksums - Maintain backward compatibility (disabled by default) This resolves compatibility issues with S3-compatible services that don't support AWS-specific encryption or checksum features, while maintaining full compatibility with AWS S3. Fixes: Upload failures with InvalidArgument and NotImplemented errors on non-AWS S3 services
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.
Pull Request Overview
This PR adds environment variables to disable AWS-specific S3 headers for compatibility with non-AWS S3 services like MinIO and Rook Ceph RGW.
- Added
AWS_S3_DISABLE_SSE
andAWS_S3_DISABLE_CHECKSUMS
environment variables to control AWS-specific header inclusion - Modified S3 multipart upload operations to conditionally add server-side encryption and checksum headers
- Maintains backward compatibility by defaulting to include headers when environment variables are not set
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
crates/aws_utils/src/lib.rs | Added environment variable configuration and accessor functions for S3 compatibility settings |
crates/aws_s3/src/storage.rs | Updated multipart upload methods to conditionally add AWS-specific headers based on environment variables |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
- Extract configure_multipart_upload_builder helper method - Eliminates code duplication between start_upload and start_client_driven_upload - Addresses GitHub Copilot review feedback for improved maintainability
Thanks for the code review feedback! ✅ Issue Addressed: The logic duplication between Solution Implemented:
Commit: 423c57e - Future changes to the AWS header logic now only need to be made in one place! The code is cleaner and more maintainable while preserving all the original functionality and documentation. |
@@ -33,6 +33,20 @@ static AWS_S3_FORCE_PATH_STYLE: LazyLock<bool> = LazyLock::new(|| { | |||
.unwrap_or_default() | |||
}); | |||
|
|||
static AWS_S3_DISABLE_SSE: LazyLock<bool> = LazyLock::new(|| { |
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.
you'll also want to add them to self-hosted/docker/docker-compose.yml
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.
I'll take care of it
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.
Alright. Let me know if there is anything else you want me to fix or change.
@gautamg795 - can you review? I took a look and it seems reasonable to me, but would appreciate another set of eyes. Would solve #85 |
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.
lgtm, pretty clean solution
merged in 683d4bd |
Thanks @jovermier for the contribution! |
Summary
AWS_S3_DISABLE_SSE=true
disables server-side encryption headers (x-amz-server-side-encryption
)AWS_S3_DISABLE_CHECKSUMS=true
disables checksum algorithm headers (x-amz-checksum-algorithm
)Problem
The current Convex backend hard-codes AWS-specific headers in S3 multipart upload operations:
ServerSideEncryption::Aes256
ChecksumAlgorithm::Crc32
These headers cause compatibility issues with S3-compatible services that don't support AWS-specific features, leading to multipart upload failures.
Solution
aws_utils
crate for S3 compatibility configurationaws_s3
storage implementation to conditionally add AWS headers based on environment variablesFiles Changed
crates/aws_utils/src/lib.rs
: Added environment variable configuration and accessor functionscrates/aws_s3/src/storage.rs
: Updated multipart upload methods to conditionally add headersTesting
Benefits
🤖 Generated with Claude Code