andygrove opened a new pull request, #6323: URL: https://github.com/apache/datafusion-comet/pull/6323
Backport of #6025 to `branch-1.1`. Cherry-picked from `65a0cda1cf62877a37ec1a1f5f9ebc9dd1405ebd`. Two files conflicted, `iceberg_common.rs` and `operators/mod.rs`. Both conflicts come from #6106, the per-executor `FileIO` cache, which is on `main` but not on `branch-1.1` (#6306 was closed). On `main`, #6106 changed `build_s3_credential_loader` to return `(Option<loader>, cacheable)`. On `branch-1.1` it still returns a bare `Option`. The adaptations: - `iceberg_common.rs`: the take-over returns `Ok(take_over_if_irsa(...).map(CustomAwsCredentialLoader::new))` rather than `Ok((..., true))`, and the comment next to it says `Ok(None)` rather than `Ok((None, true))`. - `operators/mod.rs`: `iceberg_common` becomes `pub(crate)` as upstream, but without #6106's `clear_file_io_cache` re-export. - `web_identity.rs`: `iceberg_wiring_reads_s3_prefixed_keys` drops its three `.0` tuple accesses. The rest of the patch matches upstream's hunks. That covers the new `web_identity.rs` apart from that test, `Cargo.toml`, `Cargo.lock`, `credential_bridge.rs` and both docs. Only line offsets differ, plus one context line in `credential_bridge.rs` that #6106 reworded on `main`. The protection does not depend on #6106. On `branch-1.1` a `FileIO` is still built per task, but the provider keeps its credentials in a process-wide registry keyed by identity and settings. So a startup burst still makes one STS call per executor. ## Which issue does this PR close? Closes #6024 on `branch-1.1`. #6025 already closed it on `main`. ## Rationale for this change #6025 merged after `branch-1.1` was cut at 36ab57c68. Without it, 1.1.0 ships the failure in #6024. On EKS with IRSA, a concurrent startup burst throttles STS `AssumeRoleWithWebIdentity`. On the native Iceberg path, 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. 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 writes, and the Parquet path is unchanged. A catalog can opt out with `s3.comet.credential.webIdentity.enabled=false`. ## What changes are included in this PR? The original change plus the adaptations above; see #6025 for the details. In short: - `native/core/src/cloud/s3/web_identity.rs`: a process-wide cached `AssumeRoleWithWebIdentity` provider. It builds its STS client from the AWS SDK's resolved config with raised retries. It never falls back to the instance role, and refreshes are single-flighted and jittered. - `iceberg_common.rs`: `build_s3_credential_loader` installs the provider when no provider class is configured and IRSA is detected, unless the catalog configures static keys or an assume-role ARN. - `aws-sdk-sts` (default features off) and `aws-smithy-runtime-api` become direct dependencies, plus three test-only dev-dependencies. All of them were already in the dependency graph, so the lockfile change is dependency edges only. - The S3 credential providers guide gains an "EKS / IRSA" section listing the four `s3.comet.credential.webIdentity.*` catalog properties. The design doc explains why this provider caches when the bridge does not. ## How are these changes tested? The original PR's tests, run locally on this branch: - Rust: all 30 `cloud::s3::web_identity` tests and the 5 `iceberg_common` tests pass, including the adapted `iceberg_wiring_reads_s3_prefixed_keys`. With the take-over replaced by the old `return Ok(None)`, that test fails with "IRSA with nothing configured must install the web-identity loader", so it still exercises the `branch-1.1` wiring. - `cargo fmt --check` and workspace clippy with `--all-targets -D warnings` are clean. `prettier --check` passes on both docs. - This branch merges cleanly with #6318, the #6023 backport, which edits the same two docs. With both applied, the user guide, the Cargo files and `web_identity.rs` match `main`, except for the test adaptation. CI sets no IRSA variables, so the suites confirm that nothing changes when IRSA is absent. Against `branch-1.1`, the changed paths route this pull request to every suite except Spark 3.4's SQL job, the PyArrow UDF job and the benchmark check. -- 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]
