comphead opened a new pull request, #7604:
URL: https://github.com/apache/datafusion-comet/pull/7604
## Which issue does this PR close?
Part of #6957: its second part (credentials) and the encryption half of its
third part. #6957 stays open for ACLs, storage class, requester pays and
DSSE-KMS. The pinned iceberg-rust S3 configuration cannot express those, and
its `S3Config` is crate-private, so Comet's storage factory cannot set them
either.
Builds on #7603, which this branch contains. Its first commit is #7603's, so
review the second commit here until #7603 merges.
## Rationale for this change
For a `HadoopFileIO` table on S3, iceberg-java's writer signs with exactly
the providers that `fs.s3a.aws.credentials.provider` lists. The native writer
uses its own chain instead (Comet's IRSA provider or opendal's default chain),
which can resolve a different identity. So the gate declines the key, and every
write falls back on clusters that need it, for example IRSA through S3A's
web-identity provider, or assumed roles. S3A server-side encryption settings
fall back the same way, although iceberg-rust can apply SSE-S3, SSE-KMS and
SSE-C.
## What changes are included in this PR?
- **Credentials.** `usesS3ACredentialAdapter` picks Comet's existing
`HadoopS3ACredentialProviderAdapter` for the write when two things hold. The
FileIO's Hadoop configuration names the providers explicitly (globally, or for
the data bucket). The FileIO properties name no
`s3.comet.credential.provider.class` of their own. The write proto then names
the adapter as `s3.comet.credential.provider.class` and forwards the FileIO's
explicit `fs.s3a.*` settings, including catalog `hadoop.*` overrides.
- The adapter builds the list with S3A's own provider factory on the
executor, so the native writer signs as iceberg-java's writer would, for any
provider class.
- The gate then admits the provider list and the assumed-role and
credential-store settings. A list naming an anonymous provider, and S3A
delegation tokens, still fall back, because the adapter refuses both.
- No Rust change is needed. `build_s3_access` already routes a configured
provider class through the JVM bridge for writes, and fails the task if the
provider cannot initialize. opendal 0.58's custom credential chain replaces
static keys and assume-role, so the translated `s3.access-key-id` cannot
override the adapter.
- This departs from #6957's proposal to map well-known provider classes
natively, as the Parquet scan does in `s3.rs`. Delegating to S3A covers every
provider class with S3A's own semantics and leaves `iceberg_common.rs` alone
(#7363 is reworking `build_s3_access`). The cost is a JNI call per credential
refresh. A provider that reports no expiry is treated as valid for five
minutes, and the executor logs a one-time warning. Static-key chains and every
provider on the Spark 3.x builds (SDK v1) report no expiry. Such writes also
use S3A's web-identity provider rather than Comet's native IRSA provider.
- **Encryption.** `hadoopS3EncryptionProperties` translates S3A server-side
encryption into iceberg-rust's `s3.sse.*`:
- `AES256` (SSE-S3), SSE-KMS with or without a key, and SSE-C with the
key's MD5, which iceberg-rust does not derive
- precedence and value handling mirror `S3AUtils.buildEncryptionSecrets`
- DSSE-KMS, client-side encryption, SSE-S3 with a key and SSE-C without
one fall back
- so do encryption settings when a Hadoop credential store is configured,
because the store can supply the values instead
- The gate and `buildIcebergWriteProto` call the same functions on the same
configuration.
- User guide, S3 credential-provider guide and contributor guide are updated.
## How are these changes tested?
`CometIcebergWriteDetectionSuite`:
- Encryption: the translation, including precedence, deprecated names, the
SSE-C MD5 and every fallback case. The gate admits translated encryption and
rejects client-side encryption.
- Credentials: credential settings are admitted only with the adapter. The
adapter's selection rules (explicit list, data bucket, Comet provider class,
anonymous provider, core-default) are covered.
- Proto: the planned write proto names the adapter and carries the forwarded
`fs.s3a.*` settings and `s3.sse.*`.
- Fallbacks: an S3A list beside a Comet provider class still falls back.
- Default path: the existing proto test now checks that no adapter is named
without a list.
- Four existing tests used SSE-KMS as their stock unsupported value. They
now use DSSE-KMS.
`IcebergReadFromS3Suite` (manual MinIO suite): a native write through an
explicit `SimpleAWSCredentialsProvider` list, set only as a catalog `hadoop.*`
override, is compared with the same rows written by iceberg-java.
--
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]
