dwsmith1983 commented on code in PR #5872:
URL: https://github.com/apache/datafusion-comet/pull/5872#discussion_r4267518486
##########
native/core/src/parquet/objectstore/s3.rs:
##########
@@ -2357,75 +2656,497 @@ mod tests {
}
#[test]
- fn test_extract_s3_config_custom_endpoint() {
- let cases = vec![
- ("custom.endpoint.com", "https://custom.endpoint.com";),
- ("https://custom.endpoint.com";, "https://custom.endpoint.com";),
+ fn test_normalize_endpoint_virtual_hosted_style() {
+ // Virtual-hosted addressing inserts the bucket as the leading host
label. The scheme,
+ // port and any path suffix are preserved and a trailing slash is
dropped.
+ let cases = [
+ (
+ "custom.endpoint.com",
+ "https://test-bucket.custom.endpoint.com";,
+ ),
+ (
+ "http://custom.endpoint.com";,
+ "http://test-bucket.custom.endpoint.com";,
+ ),
+ (
+ "https://custom.endpoint.com/";,
+ "https://test-bucket.custom.endpoint.com";,
+ ),
+ (
+ "http://minio.internal:9000";,
+ "http://test-bucket.minio.internal:9000";,
+ ),
+ (
+ "https://custom.endpoint.com:8443/";,
+ "https://test-bucket.custom.endpoint.com:8443";,
+ ),
(
"https://custom.endpoint.com/path/to/resource";,
- "https://custom.endpoint.com/path/to/resource";,
+ "https://test-bucket.custom.endpoint.com/path/to/resource";,
+ ),
+ (
+ "https://custom.endpoint.com/path/to/resource/";,
+ "https://test-bucket.custom.endpoint.com/path/to/resource";,
+ ),
+ (
+ "s3.us-west-2.amazonaws.com",
+ "https://test-bucket.s3.us-west-2.amazonaws.com";,
),
];
- for (endpoint, configured_endpoint) in cases {
- let mut configs = HashMap::new();
- configs.insert("fs.s3a.endpoint".to_string(),
endpoint.to_string());
- let s3_configs = extract_s3_config_options(&configs,
"test-bucket");
+ for (endpoint, expected) in cases {
assert_eq!(
- s3_configs.get(&AmazonS3ConfigKey::Endpoint),
- Some(&configured_endpoint.to_string())
+ normalize_endpoint(endpoint, "test-bucket", true),
+ Some(NormalizedEndpoint {
+ endpoint: expected.to_string(),
+ virtual_hosted_style_request: true,
+ }),
+ "endpoint {endpoint}"
);
}
+
+ // A dotted bucket over HTTPS stays path-style, as the AWS SDK
addresses it, since the
+ // dotted host falls outside S3's wildcard certificate; over HTTP it
is virtual-hosted.
+ assert_eq!(
+ normalize_endpoint("custom.endpoint.com", "my.dotted.bucket",
true),
+ Some(NormalizedEndpoint {
+ endpoint: "https://custom.endpoint.com".to_string(),
+ virtual_hosted_style_request: false,
+ })
+ );
+ assert_eq!(
+ normalize_endpoint("http://custom.endpoint.com";,
"my.dotted.bucket", true),
+ Some(NormalizedEndpoint {
+ endpoint:
"http://my.dotted.bucket.custom.endpoint.com".to_string(),
+ virtual_hosted_style_request: true,
+ })
+ );
}
#[test]
- fn test_extract_s3_config_custom_endpoint_with_virtual_hosted_style() {
- let cases = vec![
+ fn
test_extract_s3_config_dotted_bucket_stays_path_style_on_default_endpoint() {
+ // No custom endpoint means the HTTPS AWS endpoint, where a dotted
bucket must be
+ // addressed path-style whatever the flag says.
+ let configs =
TestConfigBuilder::new().with_region("us-east-1").build();
+ let s3_configs = extract_s3_config_options(&configs,
"review.dotted.bucket");
+ assert_eq!(
+ s3_configs.get(&AmazonS3ConfigKey::VirtualHostedStyleRequest),
+ Some(&"false".to_string())
+ );
+ assert!(!s3_configs.contains_key(&AmazonS3ConfigKey::Endpoint));
+
+ let configs = TestConfigBuilder::new()
+ .with_region("us-east-1")
+ .with_property("endpoint", "https://s3.us-east-1.amazonaws.com";)
+ .build();
+ let s3_configs = extract_s3_config_options(&configs,
"review.dotted.bucket");
+ assert_eq!(
+ s3_configs.get(&AmazonS3ConfigKey::VirtualHostedStyleRequest),
+ Some(&"false".to_string())
+ );
+ assert_eq!(
+ s3_configs.get(&AmazonS3ConfigKey::Endpoint),
+ Some(&"https://s3.us-east-1.amazonaws.com".to_string())
+ );
+
+ let s3_configs = extract_s3_config_options(&configs, "plainbucket");
+ assert_eq!(
+ s3_configs.get(&AmazonS3ConfigKey::VirtualHostedStyleRequest),
+ Some(&"true".to_string())
+ );
+
+ // A custom HTTP endpoint keeps virtual hosting for a dotted bucket,
as the SDK does,
+ // since the certificate rule only applies to HTTPS.
+ let configs = TestConfigBuilder::new()
+ .with_property("endpoint", "http://storage.example.test";)
+ .build();
+ let s3_configs = extract_s3_config_options(&configs,
"review.dotted.bucket");
+ assert_eq!(
+ s3_configs.get(&AmazonS3ConfigKey::VirtualHostedStyleRequest),
+ Some(&"true".to_string())
+ );
+ assert_eq!(
+ s3_configs.get(&AmazonS3ConfigKey::Endpoint),
+
Some(&"http://review.dotted.bucket.storage.example.test".to_string())
+ );
+ }
+
+ /// The URL object_store sends a GET for `s3a://<bucket>/object` to under
the options Comet
+ /// derives from `configs`, read from a presigned URL so no request leaves
the test.
+ async fn final_url(bucket: &str, configs: &HashMap<String, String>) ->
String {
+ use object_store::signer::Signer;
+
+ let mut builder = AmazonS3Builder::new()
+ .with_url(format!("s3a://{bucket}/object"))
+ .with_allow_http(true)
+ .with_access_key_id("test_access_key")
+ .with_secret_access_key("test_secret_key");
+ for (key, value) in extract_s3_config_options(configs, bucket) {
+ builder = builder.with_config(key, value);
+ }
+ let signed = match builder.build() {
Review Comment:
`final_url` now goes through `S3StoreTemplate::new(&url, configs,
bucket)?.build(S3Credentials::Provider(..))` with a static credential provider,
so the pinned URL comes from `build`. It first asserts that a region is
configured, so no row can reach the network probe.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
