dwsmith1983 commented on code in PR #5872:
URL: https://github.com/apache/datafusion-comet/pull/5872#discussion_r4267511136
##########
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() {
+ Ok(store) => {
+ store
+ .signed_url(
+ reqwest::Method::GET,
+ &Path::from("object"),
+ Duration::from_secs(60),
+ )
+ .await
+ }
+ Err(e) => Err(e),
+ };
+ match signed {
+ Ok(mut url) => {
+ url.set_query(None);
+ url.to_string()
+ }
+ Err(e) => format!("error: {e}"),
+ }
+ }
+
+ #[tokio::test]
+ #[cfg_attr(miri, ignore)] // object_store calls foreign functions
+ async fn test_bucket_the_sdk_cannot_virtual_host_stays_path_style() {
+ // The AWS SDK virtual-hosts a bucket only when its name is a DNS
label: 3 to 63
+ // lowercase letters, digits and hyphens that start and end with a
letter or digit.
+ // Other names, such as the mixed-case ones US East accepted before
March 2018, go
+ // path-style, since a hostname would lowercase the bucket into a
different one.
+ let default_endpoint =
TestConfigBuilder::new().with_region("us-east-1").build();
+ let http_endpoint = TestConfigBuilder::new()
+ .with_region("us-east-1")
+ .with_property("endpoint", "http://storage.example.test";)
+ .build();
+ let long = "a".repeat(64);
+ let cases = [
(
- "custom.endpoint.com",
- "https://custom.endpoint.com/test-bucket";,
+ &default_endpoint,
+ "LegacyBucket",
+
"https://s3.us-east-1.amazonaws.com/LegacyBucket/object".to_string(),
+ ),
+ (
+ &default_endpoint,
+ "legacy_bucket",
+
"https://s3.us-east-1.amazonaws.com/legacy_bucket/object".to_string(),
+ ),
+ (
+ &default_endpoint,
+ "-legacy-bucket",
+
"https://s3.us-east-1.amazonaws.com/-legacy-bucket/object".to_string(),
),
(
- "https://custom.endpoint.com";,
- "https://custom.endpoint.com/test-bucket";,
+ &default_endpoint,
+ "legacy-bucket-",
+
"https://s3.us-east-1.amazonaws.com/legacy-bucket-/object".to_string(),
),
(
+ &default_endpoint,
+ long.as_str(),
+ format!("https://s3.us-east-1.amazonaws.com/{long}/object";),
+ ),
+ (
+ &default_endpoint,
+ "legacy-bucket",
+
"https://legacy-bucket.s3.us-east-1.amazonaws.com/object".to_string(),
+ ),
+ // Over plain HTTP the SDK also accepts dots, but not an
IPv4-shaped name, a dot next
+ // to a hyphen or an uppercase letter
+ (
+ &http_endpoint,
+ "192.168.10.12",
+ "http://storage.example.test/192.168.10.12/object".to_string(),
+ ),
+ (
+ &http_endpoint,
+ "legacy-.bucket",
+
"http://storage.example.test/legacy-.bucket/object".to_string(),
+ ),
+ (
+ &http_endpoint,
+ "LegacyBucket",
+ "http://storage.example.test/LegacyBucket/object".to_string(),
+ ),
+ ];
+ let mut actual = Vec::new();
+ let mut expected = Vec::new();
+ for (configs, bucket, url) in cases {
+ actual.push(format!("{bucket}: {}", final_url(bucket,
configs).await));
+ expected.push(format!("{bucket}: {url}"));
+ }
+ assert_eq!(actual, expected);
+ }
+
+ #[test]
+ fn test_normalize_endpoint_path_style() {
+ // Path-style leaves the endpoint as configured apart from the https
default, since
+ // object_store appends the bucket itself
+ let cases = [
+ ("custom.endpoint.com", "https://custom.endpoint.com";),
+ ("http://custom.endpoint.com";, "http://custom.endpoint.com";),
+ (
+ "https://custom.endpoint.com/";,
"https://custom.endpoint.com/";,
- "https://custom.endpoint.com/test-bucket";,
+ ),
+ ("http://minio.internal:9000";, "http://minio.internal:9000";),
+ (
+ "https://custom.endpoint.com:8443/";,
+ "https://custom.endpoint.com:8443/";,
),
(
"https://custom.endpoint.com/path/to/resource";,
- "https://custom.endpoint.com/path/to/resource/test-bucket";,
+ "https://custom.endpoint.com/path/to/resource";,
),
(
- "https://custom.endpoint.com/path/to/resource/";,
- "https://custom.endpoint.com/path/to/resource/test-bucket";,
+ "s3.us-west-2.amazonaws.com",
+ "https://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());
- configs.insert("fs.s3a.path.style.access".to_string(),
"true".to_string());
+ for (endpoint, expected) in cases {
+ assert_eq!(
+ normalize_endpoint(endpoint, "test-bucket", false),
+ Some(NormalizedEndpoint {
+ endpoint: expected.to_string(),
+ virtual_hosted_style_request: false,
+ }),
+ "endpoint {endpoint}"
+ );
+ }
+
+ assert_eq!(
+ normalize_endpoint("custom.endpoint.com", "my.dotted.bucket",
false),
+ Some(NormalizedEndpoint {
+ endpoint: "https://custom.endpoint.com".to_string(),
+ virtual_hosted_style_request: false,
+ })
+ );
+ }
+
+ #[test]
+ fn test_normalize_endpoint_ip_host_forces_path_style() {
+ // The AWS SDK endpoint rules address IP-literal hosts path-style
whatever the
+ // configuration says, since `bucket.127.0.0.1` is not a valid host
+ let cases = [
+ ("http://127.0.0.1:9000";, "http://127.0.0.1:9000";),
+ ("http://127.0.0.1";, "http://127.0.0.1";),
+ ("127.0.0.1:9000", "https://127.0.0.1:9000";),
+ ("http://[::1]:9000";, "http://[::1]:9000";),
+ ("https://[::1]";, "https://[::1]";),
+ ("[::1]:9000", "https://[::1]:9000";),
+ ];
+ for (endpoint, expected) in cases {
+ for virtual_hosted_style_request in [true, false] {
+ assert_eq!(
+ normalize_endpoint(endpoint, "test-bucket",
virtual_hosted_style_request),
+ Some(NormalizedEndpoint {
+ endpoint: expected.to_string(),
+ virtual_hosted_style_request: false,
+ }),
+ "endpoint {endpoint}, requested virtual-hosted
{virtual_hosted_style_request}"
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_normalize_endpoint_skips_default_aws_endpoint() {
+ for virtual_hosted_style_request in [true, false] {
+ assert_eq!(
+ normalize_endpoint(
+ "s3.amazonaws.com",
+ "test-bucket",
+ virtual_hosted_style_request
+ ),
+ None
+ );
+ assert_eq!(
+ normalize_endpoint("", "test-bucket",
virtual_hosted_style_request),
+ None
+ );
+ }
+ }
+
+ #[test]
+ fn test_extract_s3_config_path_style_access() {
+ // Hadoop defaults fs.s3a.path.style.access to false (virtual-hosted)
and, like
+ // Configuration.getBoolean, falls back to that default for
non-boolean text
+ let cases = [
+ (None, "true", "https://test-bucket.custom.endpoint.com";),
+ (
+ Some("false"),
+ "true",
+ "https://test-bucket.custom.endpoint.com";,
+ ),
+ (
+ Some("yes"),
+ "true",
+ "https://test-bucket.custom.endpoint.com";,
+ ),
+ (Some("true"), "false", "https://custom.endpoint.com";),
+ (Some(" TRUE "), "false", "https://custom.endpoint.com";),
+ ];
+ for (path_style_access, expected_flag, expected_endpoint) in cases {
+ let mut builder =
+ TestConfigBuilder::new().with_property("endpoint",
"custom.endpoint.com");
+ if let Some(value) = path_style_access {
+ builder = builder.with_property("path.style.access", value);
+ }
+ let s3_configs = extract_s3_config_options(&builder.build(),
"test-bucket");
+ assert_eq!(
+ s3_configs.get(&AmazonS3ConfigKey::VirtualHostedStyleRequest),
+ Some(&expected_flag.to_string()),
+ "path.style.access {path_style_access:?}"
+ );
+ assert_eq!(
+ s3_configs.get(&AmazonS3ConfigKey::Endpoint),
+ Some(&expected_endpoint.to_string()),
+ "path.style.access {path_style_access:?}"
+ );
+ }
+ }
+
+ #[test]
+ fn test_extract_s3_config_ip_endpoint_forces_path_style() {
Review Comment:
Done. The `extract_s3_config_*` tests are now one table,
`test_extract_s3_config_final_url`, right after the `normalize_endpoint` tests.
Each row is (properties, bucket, expected URL). Together they cover the flag
unset, `true`, `false` and invalid, per-bucket overrides in both directions, IP
hosts, HTTP, and the default endpoint with dotted and legacy bucket names.
`test_create_store_with_custom_endpoint` and the IP, HTTP and dotted duplicates
are gone. `test_extract_s3_config_options` stays, because it checks the
requester-pays option, which the URL does not show.
--
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]
