fix: honor the S3 profile name and file and Hadoop's addressing mode for custom endpoints - #5872
dwsmith1983 wants to merge 14 commits into
Conversation
…for custom endpoints The profile credentials provider ignored fs.s3a.auth.profile.name and fs.s3a.auth.profile.file, and fs.s3a.path.style.access was applied inverted, so every custom endpoint was addressed path-style whatever the flag said and virtual-hosted addressing was never produced. Carry the profile name and file into the SDK builder, derive the virtual-hosted flag from the path-style setting the way Hadoop does, rebuild the endpoint as bucket.host for virtual-hosted addressing while forcing path-style for IP-literal hosts as the AWS SDK does, and return the effective mode with the endpoint so the two cannot disagree. Closes apache#4245 Closes apache#2802
sunchao
left a comment
There was a problem hiding this comment.
Reviewed 1917adca against base db790673. One verified P2 finding: the new default addressing mode breaks HTTPS buckets whose names contain dots.
Correctness
Previously, the native S3 configuration ignored the two profile override keys and inverted fs.s3a.path.style.access. This change carries the selected profile name/file through provider metadata, preserves bucket-specific precedence, and returns an endpoint together with its effective addressing mode. The maintained Spark 3.5/4.0 sources pass spark.hadoop.* values into Hadoop configuration. Comet's existing prefix-based extraction carries these S3 options through to native code. The new boolean parsing matches Hadoop's trimmed, case-insensitive true/false handling and default-false behavior.
The addressing change needs one correction before merge. Setting virtual_hosted_style_request = !path_style_access forces virtual hosting for dotted bucket names over HTTPS, including ordinary AWS endpoints with no override. For review.dotted.bucket, the resulting host is review.dotted.bucket.s3.us-east-1.amazonaws.com. The AWS SDK chooses the path-style URL instead, because the dotted hostname does not match S3's wildcard certificate. The inline P2 requests the same eligibility check for default and custom HTTPS endpoints.
I reproduced the endpoint difference without credentials or storage requests. A Rust harness using the HEAD configuration expressions and the local object_store 0.13.2 endpoint expression produced the dotted hostname. The actual offline AWS Java SDK 2.29.52 endpoint resolver, the version declared by Hadoop 3.4.2, selected path style. Normal bucket names, explicit path style and synthetic profile/bucket precedence checks behaved as expected. These are isolated configuration checks, not native/JNI or live S3 tests.
At 2026-09-12 20:03:22 UTC, only the label check succeeded. CI, CodeQL, the Delta gate and title validation were awaiting workflow approval. The cached merge has the assigned base/head parents and HEAD's tree, but no product CI execution can be credited. The author's reported 52 S3 tests/full-core pass was not rerun locally. Exact locked aws-config 1.12.0 and aws-runtime 1.9.2 source was unavailable locally, so SDK profile-file loading and refresh remain unverified. Maintained Spark 3.4/4.1 source gaps also remain. The final publication check confirmed that the head, base and discussion were unchanged after the temporary API rate limit cleared.
Performance
The added profile string handling and URL parsing occur during store/provider construction. The existing store cache includes the full configuration hash, so changing a profile name or file selects a different cached store. This PR does not add work to the object-read loop or alter credential expiry caching.
The endpoint result keeps normalization to one pass, and the profile description allocates only during construction-time logging. I found no separate verified performance issue. No benchmark or speedup claim is established by this review, and an expression microbenchmark is not applicable to this configuration change.
Design
Returning the normalized endpoint and addressing mode together is a useful safeguard against the original disagreement between those two values. The missing piece is deciding whether a bucket is eligible for virtual hosting before producing either result. That decision must also run when the endpoint is omitted, where object_store constructs the normal AWS URL.
Profile name and file are independently optional, with bucket values taking precedence over global values. The PR preserves the existing provider chain and expiry wrapper. Its metadata tests demonstrate option selection but do not establish actual SDK file precedence or credential renewal. The review therefore keeps those validation boundaries explicit.
Abstraction & complexity
The small NormalizedEndpoint type and blank-filtering helper are proportionate to the change. Configuration lookup remains centralized, and the new direct aws-runtime dependency supplies file-kind types already present in the dependency graph.
No broader provider or endpoint framework is needed. The actionable change is to extend the addressing decision with HTTPS bucket eligibility and test the final URL, rather than relying on a configuration-map assertion or a builder that has not issued a request.
| // 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; |
There was a problem hiding this comment.
Correctness
[P2] Preserve path-style addressing for dotted HTTPS buckets
Could we apply the AWS SDK's virtual-host eligibility rules before enabling this flag? With fs.s3a.endpoint.region=us-east-1 and path.style.access unset or false, a bucket such as review.dotted.bucket now becomes https://review.dotted.bucket.s3.us-east-1.amazonaws.com. BASE used path-style addressing, and the AWS SDK endpoint resolver still selects https://s3.us-east-1.amazonaws.com/review.dotted.bucket for this case. The dotted host does not match S3's wildcard TLS certificate, so this breaks native reads of otherwise valid buckets. The same eligibility issue exists for custom HTTPS endpoints. Please retain path-style addressing for dotted HTTPS buckets, including when no custom endpoint is configured, and add assertions on the resulting request URL. The current dotted-bucket test asserts the virtual-hosted string, which misses this regression.
andygrove
left a comment
There was a problem hiding this comment.
The two keys being read here are PROFILE_NAME and PROFILE_FILE on Hadoop's org.apache.hadoop.fs.s3a.auth.ProfileAWSCredentialsProvider, and Hadoop only applies them when fs.s3a.aws.credentials.provider names that class. When the provider is spelled software.amazon.awssdk.auth.credentials.ProfileCredentialsProvider, which is the spelling this arm matches, Hadoop instantiates it through S3AUtils.getInstanceFromReflection, which finds no (URI, Configuration) constructor and falls through to the SDK's static create(). The Hadoop config never reaches it. So the JVM side of the job resolves the SDK default profile while the native side now resolves the configured one, and the two halves of one job authenticate as different identities.
At the same time build_aws_credential_provider_metadata has no arm for org.apache.hadoop.fs.s3a.auth.ProfileAWSCredentialsProvider, so the one provider spelling that does honor these keys on the Hadoop side hits the _ => arm and fails the native scan with Unsupported credential provider. Would it make sense to accept that FQCN as a third alias here and add it to the datasources.md row, so the configuration Hadoop actually documents works on both sides?
On the addressing change, the new paragraph in datasources.md explains the rule well but does not mention that the default is changing. Today a custom fs.s3a.endpoint is addressed path-style whatever fs.s3a.path.style.access says, so a MinIO or Ceph RGW deployment that never set the flag works right now and will start sending requests to http://<bucket>.<host> once this lands. All the user sees is a DNS failure with nothing tying it back to this config. Could we add a sentence saying deployments that relied on the previous always-path-style behavior need to set fs.s3a.path.style.access=true?
For what it is worth, the object store cache key is fine. hash_object_store_configs hashes the whole forwarded map and extractObjectStoreOptions forwards every fs.s3a.* key by prefix, so two stores differing only in profile name or file get distinct hashes. Nothing logs credentials either.
…document the addressing change
Added
Added, right after the addressing paragraph. |
sunchao
left a comment
There was a problem hiding this comment.
Re-reviewed 3f66db90 against 1d0ce5fe. The Hadoop profile-provider spelling now reads the profile keys, the SDK spellings ignore them, and the custom-endpoint migration note is present.
The existing P2 dotted HTTPS bucket finding remains unresolved. The addressing code is unchanged, and rerunning the offline SDK endpoint resolver and configuration probe confirms the mismatch.
One new P2 is inline: with the Hadoop provider and no profile-file override, native code still merges the SDK config and credentials files, whereas Hadoop reads only the credentials file. A same-name role profile in the config file can therefore change native credential resolution.
The exact aws-config 1.12.0 and aws-runtime 1.9.2 source gaps from the first review are now closed using lockfile-checksummed archives. Validation included source tracing and an isolated probe of the SDK file-selection code. No account files, credential resolution, storage requests, full native/JNI tests, or benchmarks were used. The merge tree matches HEAD. At September 15, 05:56 UTC, CI, CodeQL, and the Delta gate awaited approval with zero jobs; only labeling passed.
| if let Some(name) = name { | ||
| builder = builder.profile_name(name); | ||
| } | ||
| if let Some(file) = file { |
There was a problem hiding this comment.
Correctness
[P2] Keep Hadoop's default profile source credentials-only
Could we preserve Hadoop's file selection when fs.s3a.auth.profile.file is unset too? The newly supported HADOOP_PROFILE arm reaches this branch with file: None, so it leaves the Rust SDK defaults in place. In the locked SDK those defaults merge ~/.aws/config with ~/.aws/credentials. Hadoop's provider instead selects only AWS_SHARED_CREDENTIALS_FILE or ~/.aws/credentials in this case.
For example, with auth.profile.name=analytics, static credentials in the credentials file and a same-name config profile containing role_arn plus source_profile=analytics, Hadoop uses the static identity while the native SDK merges in the role and assumes it. That can change the identity or fail native reads that Hadoop can perform. Please retain the Hadoop/SDK provider distinction and select a credentials-only default for the Hadoop spelling, while preserving normal SDK defaults for the SDK spellings. A test of the selected file set when the override is absent would cover this case.
…entials file for Hadoop's profile provider
Done. A bucket whose name contains a dot is addressed path-style whenever the endpoint is HTTPS, which covers the default AWS endpoint and a custom
Done. The profile metadata carries whether the provider is credentials-only; Hadoop's spelling is, the SDK spellings are not. With no file override, Hadoop's spelling now reads |
sunchao
left a comment
There was a problem hiding this comment.
Re-reviewed f9b1338a against a8e8157e. Both prior findings are addressed: dotted buckets now use path-style addressing over HTTPS, and Hadoop's profile provider now selects a credentials-only file without merging the SDK config.
Two new P2 findings are inline. The dotted-bucket guard also forces path style for custom HTTP endpoints because it runs before the scheme check. The credentials-only fallback uses native HOME instead of Hadoop's JVM user.home, so the two sides can select different files.
Validation used the current configuration functions in an isolated Rust probe, the offline AWS Java SDK 2.29.52 endpoint resolver, the locked SDK file-selection code, and Hadoop's declared Commons Lang 3.17.0 home-directory behavior. No account files, credential resolution, storage requests, full native/JNI tests, or benchmarks were used. Maintained Spark 3.5/4.0 configuration forwarding was checked. The 3.4/4.1 maintained-source gaps remain.
At September 15, 16:11 UTC, CI and CodeQL awaited approval with zero jobs. Only labeling passed. The current merge has the assigned base/head parents and the same tree as HEAD.
| let mut virtual_hosted_style_request = | ||
| !path_style_access && !bucket_needs_path_style_over_https(bucket); |
There was a problem hiding this 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.
| (None, true) => Some(default_shared_credentials_file( | ||
| std::env::var("AWS_SHARED_CREDENTIALS_FILE").ok(), | ||
| std::env::var("HOME").ok(), | ||
| )), |
There was a problem hiding this 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 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, with a case where HOME and user.home differ.
…rd Hadoop's default credentials path from the JVM
Done. The dotted-bucket rule now runs in
Done. |
sunchao
left a comment
There was a problem hiding this comment.
Re-reviewed 31fafaff against 4479e722. The dotted HTTP bucket finding is fixed: the extraction-caller probe now matches the scheme-aware rule, including the original failing case.
The existing P2 profile-path finding remains for distributed execution. extractObjectStoreOptions runs during driver-side scan planning, so the new key freezes the driver's user.home or AWS_SHARED_CREDENTIALS_FILE into the plan. The executor's native provider consumes that path unchanged. With different driver/executor homes, the isolated JVM checks select /synthetic/driver-home/.aws/credentials for native code where executor-side Hadoop selects /synthetic/executor-home/.aws/credentials. Could we resolve the omitted default in the executor JVM before creating the native provider, while preserving explicit file overrides?
Validation used the full NativeConfig object in isolated JVMs, exact-source Rust configuration probes with a bounded URL test double, the actual Hadoop path selector, and the offline AWS SDK endpoint resolver. No full Spark/JNI tests, credential loading, storage requests or benchmarks ran. Maintained Spark 3.4/4.1 source gaps remain. At September 15, 18:28 UTC, CI and CodeQL awaited approval with zero jobs. Only labeling passed.
Done. The driver no longer forwards anything for it. |
sunchao
left a comment
There was a problem hiding this comment.
Rechecked f3758e59 against 4479e722, including the change since 31fafaff.
The remaining profile-path finding is addressed. The implicit file is now resolved in the executor JVM before native plan creation, then applied to Parquet and CSV scan options. Explicit global and per-bucket profile files still take precedence, and the SDK profile aliases remain separate. The prior endpoint/addressing fixes are unchanged. I found no new or remaining P1/P2 issues.
The focused reproduction now selects the executor's file for both differing user.home values and differing AWS_SHARED_CREDENTIALS_FILE values. The previous head selects the driver's file. This used the full NativeConfig, the exact serializer method with small Spark/protobuf test doubles, isolated native overlay/provider-selection code, and Hadoop's actual path selector. It did not run a full Spark/JNI workload or authenticate to storage.
CI and CodeQL for this head are still awaiting approval with zero jobs. Only labeling has passed. Maintained Spark 3.5/4.0 sources were checked. Maintained 3.4/4.1 sources remain unavailable, so this review does not claim coverage for those versions.
sunchao
left a comment
There was a problem hiding this comment.
Rechecked 629d913b against 8c229a70. This is a base-only merge: all 11 PR files and the full authored diff are unchanged from f3758e59, and all 61 changed paths match the new base. The executor-side profile-path fix, explicit-file precedence, provider distinction and endpoint/addressing fixes remain intact. No new or remaining P1/P2 findings. Keeping the existing approval.
Validation checked source and dependency equivalence, including 14 complete files across the configuration path. The prior synthetic probes were not rerun. CI and CodeQL still await approval with zero jobs. Only labeling passed. No full Spark/JNI or cloud validation is claimed. Maintained Spark 3.4/4.1 source gaps remain.
sunchao
left a comment
There was a problem hiding this comment.
Follow-up on unchanged 629d913b / 8c229a70: attempt 2 Preflight now fails Markdown formatting in datasources.md. One new P2 is inline. Cached Prettier 3.9.6 passes the base file and fails the head file, with the difference confined to the changed credentials table. CI does not record its formatter version, so I am not claiming an exact version match.
The executed merge d89e12fa has the reviewed head's exact tree. Required Checks fails downstream of Preflight, and no product build or test jobs ran in this attempt. The newer advertised merge 4c39c21d is a different tree and gets no validation credit from these jobs.
The authored diff and prior source fixes are unchanged. Keeping the existing approval while the formatting blocker is corrected. No new runtime validation was performed.
| | `org.apache.hadoop.fs.s3a.auth.ProfileAWSCredentialsProvider` | Authenticate using a named profile from the local AWS credentials file | `fs.s3a.auth.profile.name` (optional), `fs.s3a.auth.profile.file` (optional); Hadoop applies both only to this provider | | ||
| | `com.amazonaws.auth.profile.ProfileCredentialsProvider`<br/>`software.amazon.awssdk.auth.credentials.ProfileCredentialsProvider` | Authenticate using the SDK's default profile; Hadoop constructs these without its configuration, so the profile keys are not applied on either side | None | |
There was a problem hiding this comment.
Correctness
[P2] Format the changed credentials table to unblock Preflight
Could you run prettier --write docs/source/user-guide/latest/datasources.md for this table change? Attempt 2 Preflight fails Check markdown formatting on this file. Its checked-out merge d89e12fa has the reviewed head's exact tree. A read-only check with cached Prettier 3.9.6 passes the base file but fails the head/CI file, and the only formatting diff adjusts this credentials table after the longer profile description. The downstream Required Checks failure comes from Preflight, and product build/test jobs were skipped. This needs formatting before CI can validate the implementation.
There was a problem hiding this comment.
Could you run
prettier --write docs/source/user-guide/latest/datasources.mdfor this table change?
Done in 7b95c8f. prettier --check "**/*.md" with prettier 3.9.7 now passes on the full tree, and the branch is merged with main.
sunchao
left a comment
There was a problem hiding this comment.
Rechecked 7b95c8fbb602cdfd39e859ed9faf80f4a989e768 against 36146a87bf9ca9ca9e211b4372628ed2f9d8c8c6. The Markdown-formatting finding is fixed: cached Prettier 3.9.6 reproduces the prior-head failure, while the base, current head, and advertised merge pass for datasources.md. The current document is byte-identical to the formatter output expected for the previous head.
The new authored change is table formatting. All ten non-documentation authored changes are unchanged from the previous review. The inherited base changes preserve executor-side profile-file resolution, JNI transport, scan-option overlays, and the earlier provider/addressing fixes. No remaining P1/P2 findings in this follow-up.
CI and CodeQL currently require approval and each has zero jobs. Only the label workflow passed. This approval covers the source review and focused local formatter check. I did not run full-tree formatting, native/JNI tests, or live object-store authentication at this head.
sunchao
left a comment
There was a problem hiding this comment.
Rechecked 1f2858eb against 58ab5f61 after the main merge. All 11 authored files are byte-identical to 7b95c8fb, and the six inherited file changes match the base update. The executor-local profile-file resolution, Hadoop credentials-only loading and dotted-bucket endpoint fixes remain intact. No new or remaining P1/P2 findings.
The document still matches the previously verified Prettier output. This round used source comparisons, with no product tests or formatter rerun. CI and CodeQL require approval and have run zero jobs. The successful label workflow checked out base/main and provides no product test evidence. My existing approval remains unchanged.
Which issue does this PR close?
Closes #4245, closes #2802.
Rationale for this change
Two gaps in how the native S3 store is configured from
fs.s3a.*settings.The profile credentials provider ignored
fs.s3a.auth.profile.nameandfs.s3a.auth.profile.file, so a job that selects a named profile or a non-default credentials file on the Hadoop side got the SDK defaults on the native side.fs.s3a.path.style.accesswas applied inverted:trueset object_store'svirtual_hosted_style_requestto true and then appended/bucketto the endpoint, which object_store, treating a virtual-hosted endpoint as already containing the bucket, sent as a path-style URL anyway. The net effect was that every custom endpoint was addressed path-style whatever the flag said, and virtual-hosted addressing (bucket.host) was never produced.What changes are included in this PR?
CredentialProviderMetadata::Profilecarriesnameandfile, read through the existing per-bucket config lookup with blanks treated as unset, and passed to the SDK builder. The file is loaded in credentials-file format, which is what Hadoop'sProfileAWSCredentialsProviderdoes; with no file the SDK default applies as before.aws-runtimebecomes a direct dependency because the file-kind types re-exported byaws_configare deprecated aliases; it was already in the tree.path.style.accessis parsed the way Hadoop'sConfiguration.getBooleanparses it (default false, non-boolean text falls back to the default),virtual_hosted_style_requestis its negation and is always passed, andnormalize_endpointreturns the endpoint together with the effective mode so the two cannot disagree: virtual-hosted rebuildsscheme://bucket.host[:port][/path], path-style leaves the endpoint alone for object_store to append the bucket, and an IP-literal host forces path-style the way the AWS SDK's endpoint rules do, sohttp://127.0.0.1:9000keeps working without the flag.localhostis not special-cased, matching Hadoop. Thes3.amazonaws.comskip is unchanged.Behavior change: a custom
fs.s3a.endpointwithfs.s3a.path.style.accessunset is now addressed virtual-hosted, as Hadoop S3A addresses it. Deployments on MinIO, Ceph RGW or similar services behind a hostname that relied on the previous always-path-style behavior needfs.s3a.path.style.access=true, which Hadoop already requires for those services; IP-address endpoints keep working either way. Vendor alias schemes are unaffected because the JVM side already synthesizes the flag for them.How are these changes tested?
52 unit tests in the S3 module, 11 of them written first and failing on the previous code (the profile metadata carried no name or file;
path.style.accessunset produced no flag; a hostname endpoint was never rewritten; an IP endpoint was rewritten tobucket.127.0.0.1). Coverage: the flag unset,true,false, mixed case with whitespace, and an invalid value; per-bucket override of the flag and of the endpoint, each against a global value set the other way; thes3.amazonaws.comskip in both modes; scheme-less,http://, port, trailing slash, path suffix, an AWS regional host and a dotted bucket name; IPv4 and IPv6 hosts with and without a port;create_storecalled with a custom endpoint in each mode and with an IP endpoint; profile name only, file only, both, neither, blank, trimmed and per-bucket, plus the provider chain building with the profile provider among others. Two existing tests that had encoded the inverted flag were replaced; one that asserted an empty config now asserts the endpoint key is absent, since the flag is always present.The
create_storecalls show object_store accepts each flag and endpoint pair but do not issue a request, since object_store parses the endpoint on first use. The four Scala tests that set an endpoint either set path-style access or are pure config-translation tests, so none needed changing. Full core crate suite passes, clippy and fmt clean.