comphead commented on code in PR #5872:
URL: https://github.com/apache/datafusion-comet/pull/5872#discussion_r4265558925
##########
native/core/src/parquet/objectstore/s3.rs:
##########
@@ -352,24 +354,33 @@ fn extract_s3_config_options(
s3_configs.insert(AmazonS3ConfigKey::Region, region.to_string());
}
- // Extract and handle path style access (virtual hosted style)
- let mut virtual_hosted_style_request = false;
- if let Some(path_style) = get_config_trimmed(configs, bucket,
"path.style.access") {
- virtual_hosted_style_request = path_style.to_lowercase() == "true";
- s3_configs.insert(
- AmazonS3ConfigKey::VirtualHostedStyleRequest,
- virtual_hosted_style_request.to_string(),
- );
- }
-
- // Extract endpoint configuration and modify if virtual hosted style is
enabled
- if let Some(endpoint) = get_config_trimmed(configs, bucket, "endpoint") {
- let normalized_endpoint =
- normalize_endpoint(endpoint, bucket, virtual_hosted_style_request);
- if let Some(endpoint) = normalized_endpoint {
- s3_configs.insert(AmazonS3ConfigKey::Endpoint, endpoint);
+ // Hadoop defaults fs.s3a.path.style.access to false, which means
virtual-hosted addressing,
+ // and treats non-boolean text as that default. object_store expects the
inverse flag.
+ let path_style_access = get_config_trimmed(configs, bucket,
"path.style.access")
+ .is_some_and(|value| value.eq_ignore_ascii_case("true"));
+ let mut virtual_hosted_style_request = !path_style_access;
+
+ // Extract endpoint configuration and shape it for the selected addressing
style. The flag is
+ // taken from the normalized result so the endpoint and the flag never
disagree. A custom
+ // endpoint decides the bucket-name rule by its own scheme inside
normalize_endpoint; the
+ // default AWS endpoint is HTTPS, so the rule applies to it here without
dots.
+ let custom_endpoint = get_config_trimmed(configs, bucket, "endpoint")
+ .and_then(|endpoint| normalize_endpoint(endpoint, bucket,
virtual_hosted_style_request));
+ match custom_endpoint {
+ Some(normalized) => {
+ virtual_hosted_style_request =
normalized.virtual_hosted_style_request;
+ s3_configs.insert(AmazonS3ConfigKey::Endpoint,
normalized.endpoint);
+ }
+ None => {
Review Comment:
This arm is the default AWS endpoint, but when `fs.s3a.endpoint.region` is
not set either, `S3StoreTemplate::new` first calls `resolve_bucket_region`,
which sends HEAD to `https://{bucket}.s3.amazonaws.com` (line 301). That probe
is always virtual-hosted, so it does not follow the rule added here. I expect,
not run, that a dotted bucket fails TLS on that request since the wildcard
certificate covers one label, and that `LegacyBucket` is lowercased by URL
parsing and so probes a different bucket name (a `Bucket not found` error, or
the region of someone else's bucket). That would mean the dotted and mixed-case
fixes only work when the region is configured, and every test for them sets
one. Could the probe reuse `is_virtual_hostable_bucket(bucket, false)` and use
a path-style probe URL when it is false, with a test of the probe URL? If not,
the data sources paragraph should say these buckets need
`fs.s3a.endpoint.region`.
##########
native/core/src/parquet/objectstore/s3.rs:
##########
@@ -1414,6 +1540,159 @@ mod tests {
assert_eq!(lookup_provider_class(&configs, "public-data"), None);
}
+ #[test]
+ fn test_hadoop_profile_provider_routes_through_the_adapter() {
Review Comment:
About half of the 18 rows duplicate tests that exist already. The global
class, per-bucket class, empty and blank class rows repeat
`test_empty_per_bucket_provider_class_opts_out`, and the per-bucket list rows
exercise `get_config_trimmed` precedence, which `test_get_config_trimmed`
covers. The rows specific to this change are the list matching (padded entries,
exact names, whole entries, SDK names) plus one row where a configured class
wins and one where a blank class opts out. Could the table keep those and drop
the rest?
##########
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:
Several of the new tests repeat each other. This one uses the same four IP
endpoints as `test_normalize_endpoint_ip_host_forces_path_style`, and the flag
it checks is the `virtual_hosted_style_request` that `normalize_endpoint`
already returns. `test_extract_s3_config_http_endpoint_keeps_scheme` repeats
the `http://minio.internal:9000` rows of the virtual-hosted and path-style
tables, dotted-bucket expectations appear in three tests, and
`test_create_store_with_custom_endpoint` only asserts that `create_store`
returns `Ok`, which every `final_url` case already needs. Since `final_url`
pins what object_store really sends, could the `extract_s3_config_*` tests
become one table of (properties, bucket, expected URL) next to the
`normalize_endpoint` unit tests, covering the flag unset, `true`, `false` and
invalid, per-bucket overrides, IP hosts, HTTP and the default endpoint? I
expect that removes a few hundred lines with the same coverage.
##########
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` rebuilds what `S3StoreTemplate::build` does (`with_url`,
`with_allow_http(true)`, then each extracted option), so a later change to
`build` would not be seen by these tests.
`builds_from_a_template_inside_the_runtime` already shows the pattern:
`S3StoreTemplate::new(&url, configs,
bucket)?.build(S3Credentials::Provider(..))` with a static credential provider,
which gives a store that can presign. Could `final_url` use that so the pinned
URL comes from the production build path?
--
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]
