You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "jackye1995 (via GitHub)" <gi...@apache.org> on 2025/08/23 04:40:26 UTC
[PR] feat(services/s3): Add missing S3 features based on object_store implementation [opendal]
jackye1995 opened a new pull request, #6531:
URL: https://github.com/apache/opendal/pull/6531
## Summary
This PR implements six new S3 service features based on the object_store library's Azure implementation to improve compatibility and functionality with various S3-compatible services.
### New Features Added
#### ✅ DefaultRegion
- **Purpose**: Provides a fallback region when auto-detection fails
- **Implementation**: Added `default_region` config option with fallback logic in region resolution
- **Usage**: `S3Builder::new().default_region("us-west-2")`
#### ✅ ImdsV1Fallback
- **Purpose**: Enable IMDSv1 for credential retrieval from instance metadata
- **Implementation**: Config option prepared (full implementation may require reqsign library updates)
- **Usage**: `S3Builder::new().enable_imdsv1_fallback()`
#### ✅ UnsignedPayload
- **Purpose**: Skip payload checksum computation for improved performance on large uploads
- **Implementation**: Sets `x-amz-content-sha256: UNSIGNED-PAYLOAD` header when enabled
- **Usage**: `S3Builder::new().enable_unsigned_payload()`
#### ✅ SkipSignature
- **Purpose**: Disable request signing for public buckets or when using pre-signed URLs
- **Implementation**: Early return in signing methods when enabled
- **Usage**: `S3Builder::new().enable_skip_signature()`
#### ✅ DisableTagging
- **Purpose**: Prepared infrastructure for disabling S3 tagging (for future tagging implementation)
- **Implementation**: Config flag ready for when S3 tagging is implemented
- **Usage**: `S3Builder::new().enable_disable_tagging()`
#### ✅ BucketKeyEnabled
- **Purpose**: Control S3 bucket key usage for KMS encryption cost optimization
- **Implementation**: Sets `x-amz-server-side-encryption-bucket-key-enabled` header
- **Usage**: `S3Builder::new().enable_bucket_key()` or `.disable_bucket_key()`
## Implementation Details
- **Config Structure**: All new options added to `S3Config` with proper defaults
- **Builder Pattern**: Consistent builder methods following OpenDAL patterns
- **Core Integration**: Proper integration with signing logic and request building
- **Testing**: Comprehensive unit tests for all new features
- **Documentation**: Inline documentation for all new configuration options
## Testing
- ✅ All existing S3 tests continue to pass
- ✅ New comprehensive unit tests for all features
- ✅ Clippy passes with `-D warnings`
- ✅ Code properly formatted with `cargo fmt`
- ✅ No compilation warnings or errors
## Compatibility
- **Breaking Changes**: None - all features are opt-in with sensible defaults
- **S3 Compatible Services**: Improves compatibility with various S3-compatible services
- **Performance**: UnsignedPayload and BucketKeyEnabled can improve performance and reduce costs
## Test Plan
```bash
# Run S3 service tests
cargo test --features services-s3 --lib services::s3::backend::tests
# Run clippy with strict warnings
cargo clippy --all-targets --features services-s3 -- -D warnings
# Format code
cargo fmt
```
All features have been tested locally and pass all quality checks.
🤖 Generated with [Claude Code](https://claude.ai/code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/s3): support default_region, server_side_encryption_bucket_key and skip_signature for Arrow object_store compatibility [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #6531:
URL: https://github.com/apache/opendal/pull/6531#discussion_r2315586195
##########
core/src/services/s3/backend.rs:
##########
@@ -600,6 +611,19 @@ impl S3Builder {
self
}
+ /// Enable the use of S3 bucket keys for server-side encryption.
+ /// This can reduce costs when using KMS encryption by using fewer KMS API calls.
+ pub fn enable_server_side_encryption_bucket_key(mut self) -> Self {
Review Comment:
I'm hesitant about this because OpenDAL tends to maintain configuration values in an orthogonal way, meaning each configuration value controls only a single feature. Users don't need to worry about the interactions between configuration values.
But I do understand the value of be compatible to those object_store options. Is it a good idea to add those mapping in to `object_store_opendal`? What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/s3): Add missing S3 features based on object_store implementation [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #6531:
URL: https://github.com/apache/opendal/pull/6531#discussion_r2298677770
##########
core/src/services/s3/backend.rs:
##########
@@ -600,6 +611,39 @@ impl S3Builder {
self
}
+ /// Enable IMDSv1 fallback for retrieving credentials from instance metadata.
Review Comment:
`reqsign` doesn't support this so far.
##########
core/src/services/s3/backend.rs:
##########
@@ -600,6 +611,39 @@ impl S3Builder {
self
}
+ /// Enable IMDSv1 fallback for retrieving credentials from instance metadata.
+ /// By default, only IMDSv2 is used as AWS recommends against IMDSv1 for security reasons.
+ pub fn enable_imdsv1_fallback(mut self) -> Self {
+ self.config.imdsv1_fallback = true;
+ self
+ }
+
+ /// Enable unsigned payload for request signing to avoid computing body checksum.
+ /// This can improve performance for large uploads but may not be suitable for all use cases.
+ pub fn enable_unsigned_payload(mut self) -> Self {
Review Comment:
We are using `unsigned_payload` by default, if we do need this, we might use `disable_unsigned_payload`. However, this can also have overlap with the checksum support we want to add in opendal: https://github.com/apache/opendal/issues/5549
##########
core/src/services/s3/backend.rs:
##########
@@ -600,6 +611,39 @@ impl S3Builder {
self
}
+ /// Enable IMDSv1 fallback for retrieving credentials from instance metadata.
+ /// By default, only IMDSv2 is used as AWS recommends against IMDSv1 for security reasons.
+ pub fn enable_imdsv1_fallback(mut self) -> Self {
+ self.config.imdsv1_fallback = true;
+ self
+ }
+
+ /// Enable unsigned payload for request signing to avoid computing body checksum.
+ /// This can improve performance for large uploads but may not be suitable for all use cases.
+ pub fn enable_unsigned_payload(mut self) -> Self {
+ self.config.unsigned_payload = true;
+ self
+ }
+
+ /// Skip request signing. Useful for public buckets or when using pre-signed URLs.
+ pub fn enable_skip_signature(mut self) -> Self {
+ self.config.skip_signature = true;
+ self
+ }
+
+ /// Enable the use of S3 bucket keys for server-side encryption.
+ /// This can reduce costs when using KMS encryption by using fewer KMS API calls.
+ pub fn enable_bucket_key(mut self) -> Self {
+ self.config.bucket_key_enabled = Some(true);
+ self
+ }
+
+ /// Disable the use of S3 bucket keys for server-side encryption.
+ pub fn disable_bucket_key(mut self) -> Self {
Review Comment:
This is `server_side_encryption`
##########
core/src/services/s3/backend.rs:
##########
@@ -600,6 +611,39 @@ impl S3Builder {
self
}
+ /// Enable IMDSv1 fallback for retrieving credentials from instance metadata.
+ /// By default, only IMDSv2 is used as AWS recommends against IMDSv1 for security reasons.
+ pub fn enable_imdsv1_fallback(mut self) -> Self {
+ self.config.imdsv1_fallback = true;
+ self
+ }
+
+ /// Enable unsigned payload for request signing to avoid computing body checksum.
+ /// This can improve performance for large uploads but may not be suitable for all use cases.
+ pub fn enable_unsigned_payload(mut self) -> Self {
+ self.config.unsigned_payload = true;
+ self
+ }
+
+ /// Skip request signing. Useful for public buckets or when using pre-signed URLs.
+ pub fn enable_skip_signature(mut self) -> Self {
+ self.config.skip_signature = true;
+ self
+ }
+
+ /// Enable the use of S3 bucket keys for server-side encryption.
+ /// This can reduce costs when using KMS encryption by using fewer KMS API calls.
+ pub fn enable_bucket_key(mut self) -> Self {
Review Comment:
This is `server_side_encryption`
##########
core/src/services/s3/backend.rs:
##########
@@ -600,6 +611,39 @@ impl S3Builder {
self
}
+ /// Enable IMDSv1 fallback for retrieving credentials from instance metadata.
+ /// By default, only IMDSv2 is used as AWS recommends against IMDSv1 for security reasons.
+ pub fn enable_imdsv1_fallback(mut self) -> Self {
+ self.config.imdsv1_fallback = true;
+ self
+ }
+
+ /// Enable unsigned payload for request signing to avoid computing body checksum.
+ /// This can improve performance for large uploads but may not be suitable for all use cases.
+ pub fn enable_unsigned_payload(mut self) -> Self {
+ self.config.unsigned_payload = true;
+ self
+ }
+
+ /// Skip request signing. Useful for public buckets or when using pre-signed URLs.
+ pub fn enable_skip_signature(mut self) -> Self {
Review Comment:
This is `allow_anonymous`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/s3): support default_region, server_side_encryption_bucket_key and skip_signature for Arrow object_store compatibility [opendal]
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6531:
URL: https://github.com/apache/opendal/pull/6531#discussion_r2316636431
##########
core/src/services/s3/backend.rs:
##########
@@ -600,6 +611,19 @@ impl S3Builder {
self
}
+ /// Enable the use of S3 bucket keys for server-side encryption.
+ /// This can reduce costs when using KMS encryption by using fewer KMS API calls.
+ pub fn enable_server_side_encryption_bucket_key(mut self) -> Self {
Review Comment:
I think for the 3 configs here, they are a bit different:
- `enable_server_side_encryption_bucket_key` seems to be a missing feature currently in OpenDAL S3, it adds the header `x-amz-server-side-encryption-bucket-key-enabled` to the request, where user can directly configure a bucket key for the entire bucket, and then use this API key to use that pre-configured key.
- `default_region` gives a default in region resolution process, so it is only used when there is no other environment configurations that can resolve a region
These 2 are independent configs orthogonal to other existing configurations in OpenDAL-S3.
`skip_signature` as you suggested is just an alias of `allow_anonymous` so it was just a missed configuration mapping in the last PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/s3): Add missing S3 features based on object_store implementation [opendal]
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6531:
URL: https://github.com/apache/opendal/pull/6531#discussion_r2299237450
##########
core/src/services/s3/backend.rs:
##########
@@ -600,6 +611,39 @@ impl S3Builder {
self
}
+ /// Enable IMDSv1 fallback for retrieving credentials from instance metadata.
Review Comment:
will remove, created a tracking issue at https://github.com/Xuanwo/reqsign/issues/599
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org