sunchao commented on code in PR #5872:
URL: https://github.com/apache/datafusion-comet/pull/5872#discussion_r4017751296
##########
native/core/src/parquet/objectstore/s3.rs:
##########
@@ -239,24 +240,29 @@ 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(),
- );
- }
+ // 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"));
+ // The AWS SDK addresses a bucket whose name contains a dot path-style
over HTTPS, because
+ // the dotted host does not match S3's wildcard certificate. The default
AWS endpoint is
+ // HTTPS, and normalize_endpoint applies the same rule to a custom one by
its scheme.
+ let mut virtual_hosted_style_request =
+ !path_style_access && !bucket_needs_path_style_over_https(bucket);
Review Comment:
### Correctness
[P2] Apply the dotted-bucket guard after choosing the endpoint scheme
Could we limit this initial dotted-bucket fallback to the default HTTPS
endpoint? With `fs.s3a.endpoint=http://storage.example.test`, bucket
`review.dotted.bucket`, and `path.style.access` unset or false, this expression
already sets the flag to false. `normalize_endpoint` then returns at its first
path-style branch, before it can apply the scheme-sensitive rule. The native
configuration produces `http://storage.example.test/review.dotted.bucket`,
while Hadoop's AWS SDK resolver selects
`http://review.dotted.bucket.storage.example.test`. This breaks a custom HTTP
service that routes buckets by hostname. The new HTTP test calls
`normalize_endpoint(..., true)` directly, bypassing the caller that supplies
false. Please preserve virtual hosting for this HTTP case and cover it through
`extract_s3_config_options`, including the resulting URL.
##########
native/core/src/parquet/objectstore/s3.rs:
##########
@@ -814,11 +908,35 @@ impl CredentialProviderMetadata {
.build();
Ok(Arc::new(credential_provider))
}
- CredentialProviderMetadata::Profile => {
- let credential_provider =
ProfileFileCredentialsProvider::builder()
- .configure(&ProviderConfig::with_default_region().await)
- .build();
- Ok(Arc::new(credential_provider))
+ CredentialProviderMetadata::Profile {
+ name,
+ file,
+ credentials_only,
+ } => {
+ let mut builder = ProfileFileCredentialsProvider::builder()
+ .configure(&ProviderConfig::with_default_region().await);
+ if let Some(name) = name {
+ builder = builder.profile_name(name);
+ }
+ // Hadoop's ProfileAWSCredentialsProvider loads the configured
file, or the
+ // shared credentials file, as a credentials-format file and
reads nothing
+ // else, so a same-name role profile in the SDK's config file
never applies.
+ let credentials_file = match (file, credentials_only) {
+ (Some(file), _) => Some(file.clone()),
+ (None, true) => Some(default_shared_credentials_file(
+ std::env::var("AWS_SHARED_CREDENTIALS_FILE").ok(),
+ std::env::var("HOME").ok(),
+ )),
Review Comment:
### Correctness
[P2] Resolve Hadoop's default credentials path from JVM user.home
Could we pass Hadoop's resolved default file into this branch instead of
deriving it from the native process's `HOME`? When both
`fs.s3a.auth.profile.file` and `AWS_SHARED_CREDENTIALS_FILE` are unset,
[Hadoop's
provider](https://github.com/apache/hadoop/blob/22a6ad99379850e1fc43e3032a9d676fff7abbf2/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/ProfileAWSCredentialsProvider.java#L57-L73)
uses `SystemUtils.getUserHome()`, which reads the JVM `user.home` property.
For an executor launched with `-Duser.home=/synthetic/jvm-home` while
`HOME=/synthetic/env-home`, Hadoop selects
`/synthetic/jvm-home/.aws/credentials` but this code selects
`/synthetic/env-home/.aws/credentials`. If HOME is absent, it selects
`/.aws/credentials` even when the JVM has a valid home. A job can therefore
load a different profile or fail native reads after Hadoop successfully loads
its credentials. Please retain credentials-only loading while using the same
resolved file on both sides, wit
h a case where HOME and user.home differ.
--
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]
