andygrove opened a new pull request, #6325:
URL: https://github.com/apache/datafusion-comet/pull/6325
Backport of #6025 to `branch-1.0`.
Cherry-picked from `65a0cda1cf62877a37ec1a1f5f9ebc9dd1405ebd`, but this is a
port rather than a clean pick. `branch-1.0` has no `iceberg_common.rs`, no
native Iceberg writer, and older AWS SDK and reqsign crates. The provider
itself, `web_identity.rs`, is unchanged apart from two comments and one test.
The adaptations are listed under "What changes are included" below.
## Which issue does this PR close?
Closes #6024 on `branch-1.0`. #6025 already closed it on `main`.
## Rationale for this change
1.0.0 ships the failure in #6024. On EKS with IRSA, a concurrent startup
burst throttles STS `AssumeRoleWithWebIdentity`. On the native Iceberg scan,
opendal's default credential chain does not retry the throttle and falls
through to the node instance role, so reads fail with a hard 403 even though
the throttle was transient.
`branch-1.0` is at least as exposed to the burst as `main`. iceberg-rust
builds a new S3 operator for every file operation, and reqsign-core 3.2.0's
signer does not hold its lock while it loads, so concurrent signs on a cold
operator each call STS. The take-over keeps one credential per executor process
and single-flights its refreshes, which covers both.
The take-over is on by default, but it only engages when all of the
following hold:
- both `AWS_WEB_IDENTITY_TOKEN_FILE` and `AWS_ROLE_ARN` are set,
- a region is set, and
- nothing that outranks web identity is configured: no bridge class, catalog
static keys or `client.assume-role.arn`, static environment credentials, or
profile or config file.
It covers native Iceberg reads, and the Parquet path is unchanged. A catalog
can opt out with `s3.comet.credential.webIdentity.enabled=false`.
On `branch-1.0` the take-over also changes which STS endpoint some regions
use, and the change is a fix. The old chain, reqsign-aws-v4 3.0.3, sends every
region outside China to the global `sts.amazonaws.com` unless
`AWS_STS_REGIONAL_ENDPOINTS=regional` is set. That includes GovCloud, ISO and
EUSC regions, which have no `sts.amazonaws.com` endpoint. The take-over uses
the global endpoint only for commercial regions, as on `main`, and the SDK's
regional endpoint everywhere else.
## What changes are included in this PR?
The provider is the original one, so see #6025 for the details. The port:
- `iceberg_scan.rs` gets what #6025 added to `iceberg_common.rs`: the
take-over in `build_s3_credential_loader`, which becomes `pub(crate)`, plus
`has_explicit_s3_credentials` and its test. On `branch-1.0` the loader takes no
`AccessMode` and returns a bare `Option`. Its provider-class lookup becomes a
`let ... else`, which gives the take-over a place to go. `iceberg_scan` becomes
`pub(crate)` for the wiring test, as `iceberg_common` did on `main`.
- `web_identity.rs`: `iceberg_wiring_reads_s3_prefixed_keys` calls the
three-argument loader. The module doc names `iceberg_scan`. The stand-aside log
comment says `load_file_io` runs per scan task, where upstream says per scan
and write task.
- Dependencies are pinned to the versions already in `branch-1.0`'s
lockfile: `aws-sdk-sts` 1.110.0 rather than 1.114.0, `aws-smithy-runtime-api`
1.14.0 rather than 1.16.0, and `aws-smithy-types` 1.6.1 rather than 1.6.3. The
lockfile gains the same five dependency edges as on `main`, and no crate
versions change.
- The docs say reads rather than reads and writes, because `branch-1.0` has
no native Iceberg writer. The design doc also names
`iceberg_scan.rs::build_s3_credential_loader`.
`credential_bridge.rs` and `cloud/s3/mod.rs` get upstream's hunks unchanged.
## How are these changes tested?
The original PR's tests, run locally on this branch against the `branch-1.0`
lockfile:
- Rust: all 30 `cloud::s3::web_identity` tests and the 7 `iceberg_scan`
tests pass, including the new `explicit_s3_credentials_detected`. The endpoint
tests drive the real aws-sdk-sts 1.110.0 resolver through a canned HTTP client.
The signing round-trip goes through reqsign-aws-v4 3.0.3 and reqsign-core
3.2.0. So the endpoint and signer behavior is checked at this branch's versions.
- Three mutations each fail exactly their own test. Returning `None` in
place of the take-over fails `iceberg_wiring_reads_s3_prefixed_keys`. Dropping
the `AWS_ENDPOINT_URL_STS` guard fails `custom_sts_endpoint_env_is_honored`,
and dropping the generic `endpoint_url` guard fails
`generic_endpoint_url_env_is_honored`.
- The version-sensitive behavior was also read in the pinned sources:
- opendal-service-s3 0.57.0 replaces its whole default chain with a custom
one, as on `main`. So a throttle that outlasts the retries still cannot fall
through to the instance role.
- reqsign-aws-v4 3.0.3 has the same 120 s refresh and 10 s signing margins
that `web_identity.rs` is built around.
- aws-sdk-sts 1.110.0 resolves `AWS_ENDPOINT_URL_STS` inside
`Builder::from(&SdkConfig)` and does not read `AWS_STS_REGIONAL_ENDPOINTS`. Its
commercial-partition regex is the one `is_commercial_partition` mirrors.
- `cargo fmt --check` and workspace clippy with `--all-targets -D warnings`
are clean. `prettier --check` passes on both docs.
CI sets no IRSA variables, so the Iceberg suites confirm that nothing
changes when IRSA is absent. On `branch-1.0` they run only with the
`run-iceberg-tests` label.
--
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]
