[GLUTEN-12701][CORE][VL] Support Iceberg REST-catalog vended credentials in native scans - #12827
[GLUTEN-12701][CORE][VL] Support Iceberg REST-catalog vended credentials in native scans#12827guangyu-yang-rokt wants to merge 2 commits into
Conversation
…als in native scans
A REST catalog can be configured to vend credentials instead of letting the
compute use its own identity ('X-Iceberg-Access-Delegation: vended-credentials',
e.g. Apache Polaris). loadTable then returns credentials scoped to one table,
and only the JVM FileIO ever sees them: the native scan path receives file paths
only, so Velox's S3 client falls back to the process credential chain, which by
design has no access to the warehouse. Every native TableScan of such a table
fails with S3 403 while the same query reads fine on vanilla Spark.
Read the vended credentials from the scan table's FileIO at split planning time
and carry them, with the table location, in a new table-scoped
ReadRel.LocalFiles.read_properties map. A LocalFiles is single-table by
construction, so table granularity is split granularity. The Velox backend turns
the union of the task's scans into a file system token provider that resolves
credentials by longest-prefix match of the file path against the table
locations, so a query joining two vended-credential tables that live in one
bucket cannot mix them up.
FileIO.properties() is implemented by S3FileIO and ResolvingFileIO and defaults
to throwing, which is treated as "no credentials". Tables whose files the
process credentials can read attach nothing and are untouched.
New conf spark.gluten.sql.columnar.iceberg.enableVendedCredentials (default
true): when disabled, scans of tables read with vended credentials fail native
validation and fall back to vanilla Spark, which reads them correctly through
the JVM FileIO. Validation also requires the new backend capability
BackendSettingsApi.supportIcebergVendedCredentialsRead(), so a backend that does
not consume read_properties falls back instead of failing with access-denied
errors.
Credentials are snapshotted on the driver, so a scan has to start within the
lifetime of the credentials the catalog vended; executors cannot re-vend. A 403
on expiry stays a retriable task failure.
Fixes apache#12701
The native half of the previous commit needs the S3 file system to consume FileOptions::tokenProvider, which is facebookincubator/velox#18570 and not yet in the pinned Velox branch. Point UPSTREAM_VELOX_PR_ID at it so CI can build this PR, and drop this commit once the Velox change is merged and the pin is advanced past it.
ee043f6 to
557f86f
Compare
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds end-to-end support for Iceberg REST-catalog vended S3 credentials in the native (Velox) scan path by carrying table-scoped read properties through Substrait splits and installing a per-query S3 token provider.
Changes:
- Extend Substrait
ReadRel.LocalFileswith aread_propertiesmap and plumb it through JVM split planning into nativeSplitInfo. - Extract Iceberg FileIO S3 credentials + companions on the JVM and attach them to local file splits; validate/gate with a new Spark config flag.
- Implement
GlutenS3TokenProviderin C++ and install it into the VeloxQueryCtxso S3 opens use per-table credentials.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| gluten-substrait/src/main/scala/org/apache/gluten/backendsapi/BackendSettingsApi.scala | Adds backend capability flag for vended-credentials reads. |
| gluten-substrait/src/main/resources/substrait/proto/substrait/algebra.proto | Adds read_properties to ReadRel.LocalFiles for table-scoped properties. |
| gluten-substrait/src/main/java/org/apache/gluten/substrait/rel/LocalFilesNode.java | Serializes read_properties into Substrait LocalFiles payload. |
| gluten-iceberg/src/main/scala/org/apache/iceberg/spark/source/GlutenIcebergSourceUtil.scala | Extracts FileIO vended credentials and attaches them to split planning. |
| gluten-iceberg/src/main/scala/org/apache/gluten/execution/IcebergScanTransformer.scala | Validates/gates native scan when vended credentials are present. |
| gluten-iceberg/src/main/scala/org/apache/gluten/config/GlutenIcebergConfig.scala | Introduces spark.gluten.sql.columnar.iceberg.enableVendedCredentials. |
| gluten-iceberg/src/test/scala/org/apache/iceberg/spark/source/GlutenIcebergSourceUtilSuite.scala | Unit tests for credential extraction behavior. |
| gluten-iceberg/src/test/java/org/apache/gluten/substrait/rel/IcebergLocalFilesNodeReadPropertiesTest.java | Tests proto round-trip/omission of read_properties. |
| cpp/velox/substrait/SubstraitToVeloxPlan.h | Adds readProperties to native SplitInfo. |
| cpp/velox/compute/VeloxPlanConverter.cc | Copies LocalFiles.read_properties into SplitInfo. |
| cpp/velox/utils/GlutenS3TokenProvider.h | Declares per-table S3 token provider keyed by location prefix. |
| cpp/velox/utils/GlutenS3TokenProvider.cc | Implements longest-prefix token resolution and identity/hash. |
| cpp/velox/compute/WholeStageResultIterator.h | Declares fs token provider factory installed on QueryCtx. |
| cpp/velox/compute/WholeStageResultIterator.cc | Builds/installs token provider from scan read properties. |
| cpp/velox/tests/GlutenS3TokenProviderTest.cc | Adds ENABLE_S3 unit tests for provider resolution/identity. |
| cpp/velox/tests/CMakeLists.txt | Registers the new provider test under ENABLE_S3. |
| cpp/velox/CMakeLists.txt | Builds GlutenS3TokenProvider when ENABLE_S3 is on. |
| backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxBackend.scala | Advertises vended-credentials read support for Velox backend. |
| ep/build-velox/src/get-velox.sh | Temporarily pins Velox PR ID to pick up upstream support. |
| docs/get-started/VeloxIceberg.md | Documents new config and credential-vending behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # TODO: reset to "" once facebookincubator/velox#18570 (per-path S3 credentials | ||
| # from the query TokenProvider) is merged and the Velox pin above includes it. | ||
| UPSTREAM_VELOX_PR_ID="18570" |
| private[source] def extractVendedReadProperties( | ||
| ioProperties: JMap[String, String], | ||
| encodedTableLocation: String): JMap[String, String] = { | ||
| val accessKeyId = ioProperties.get(S3AccessKeyId) | ||
| val secretAccessKey = ioProperties.get(S3SecretAccessKey) | ||
| if (accessKeyId == null || secretAccessKey == null) { | ||
| return Collections.emptyMap() | ||
| } | ||
| val readProperties = new JHashMap[String, String]() | ||
| readProperties.put(S3AccessKeyId, accessKeyId) | ||
| readProperties.put(S3SecretAccessKey, secretAccessKey) | ||
| OptionalCredentialKeys.foreach { | ||
| key => | ||
| val value = ioProperties.get(key) | ||
| if (value != null) { | ||
| readProperties.put(key, value) | ||
| } | ||
| } | ||
| readProperties.put(LocationKey, encodedTableLocation) | ||
| readProperties | ||
| } |
| this.fileFormat = other.fileFormat; | ||
| this.preferredLocations.addAll(other.preferredLocations); | ||
| this.fileReadProperties = other.fileReadProperties; | ||
| this.readProperties = other.readProperties; | ||
| this.iterAsInput = other.iterAsInput; |
| const auto& path = s3Key->path(); | ||
| const S3TableCredentials* longestMatch = nullptr; | ||
| size_t longestMatchSize = 0; | ||
| for (const auto& [prefix, credentials] : credentialsByPrefix_) { | ||
| if (prefix.size() >= longestMatchSize && prefixMatches(path, prefix)) { | ||
| longestMatch = &credentials; | ||
| longestMatchSize = prefix.size(); | ||
| } | ||
| } | ||
| if (longestMatch == nullptr) { | ||
| return nullptr; | ||
| } | ||
| return std::make_shared<facebook::velox::filesystems::S3AccessToken>( | ||
| longestMatch->accessKeyId, longestMatch->secretAccessKey, longestMatch->sessionToken); | ||
| } |
| // Table-scoped storage properties the native reader needs to open the | ||
| // listed files, e.g. the S3 credentials an Iceberg REST catalog vends for | ||
| // one table. A LocalFiles is single-table by construction, so table | ||
| // granularity is split granularity. Emitted only when present. | ||
| map<string, string> read_properties = 11; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
gluten-substrait/src/main/java/org/apache/gluten/substrait/rel/LocalFilesNode.java:140
- Storing the caller-provided mutable Map by reference risks later mutation affecting serialization (and potentially sharing across nodes). Consider defensively copying (e.g., new HashMap<>(readProperties)) and/or wrapping as unmodifiable before storing.
public void setReadProperties(Map<String, String> readProperties) {
this.readProperties = readProperties;
}
gluten-substrait/src/main/java/org/apache/gluten/substrait/rel/LocalFilesNode.java:119
- The copy constructor assigns readProperties by reference, which can unintentionally share mutable state between instances. This should be cloned similarly to other collections (and ideally deep-copied for Maps) to avoid cross-instance mutation.
this.fileFormat = other.fileFormat;
this.preferredLocations.addAll(other.preferredLocations);
this.fileReadProperties = other.fileReadProperties;
this.readProperties = other.readProperties;
this.iterAsInput = other.iterAsInput;
this.fileSchema = other.fileSchema;
gluten-substrait/src/main/java/org/apache/gluten/substrait/rel/LocalFilesNode.java:65
- Having both fileReadProperties and readProperties is easy to confuse. Consider renaming the new field to something explicit like tableReadProperties (and matching setter) to make the split-scope vs table-scope distinction clearer.
private Map<String, String> fileReadProperties;
// Table-scoped storage properties required to open this split's files
// natively, e.g. Iceberg catalog-vended S3 credentials. Serialized into
// ReadRel.LocalFiles.read_properties when non-empty.
private Map<String, String> readProperties;
cpp/velox/utils/GlutenS3TokenProvider.cc:104
- getToken() does an O(N) scan over all table prefixes for every file open. If a query can involve many scans/tables, this can become a hot path. Consider precomputing an auxiliary vector of prefixes sorted by descending length (or another data structure for longest-prefix match) so you can stop at the first match and reduce average lookup cost.
for (const auto& [prefix, credentials] : credentialsByPrefix_) {
if (prefix.size() >= longestMatchSize && prefixMatches(path, prefix)) {
longestMatch = &credentials;
longestMatchSize = prefix.size();
}
}
cpp/velox/compute/WholeStageResultIterator.cc:299
- This copies every scanInfo->readProperties map into a new vector before building the provider. If these maps can be non-trivial, consider adjusting GlutenS3TokenProvider::create to accept references/views (e.g., const refs or a span of pointers) to avoid duplicating the maps.
std::shared_ptr<velox::filesystems::TokenProvider> WholeStageResultIterator::createFsTokenProvider() const {
#ifdef ENABLE_S3
std::vector<std::unordered_map<std::string, std::string>> readProperties;
readProperties.reserve(scanInfos_.size());
for (const auto& scanInfo : scanInfos_) {
readProperties.push_back(scanInfo->readProperties);
}
return GlutenS3TokenProvider::create(readProperties);
#else
return nullptr;
#endif
}
| # TODO: reset to "" once facebookincubator/velox#18570 (per-path S3 credentials | ||
| # from the query TokenProvider) is merged and the Velox pin above includes it. | ||
| UPSTREAM_VELOX_PR_ID="18570" |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
What changes were proposed in this pull request?
Fixes #12701: make native scans work for Iceberg tables read through a REST
catalog that vends credentials (
X-Iceberg-Access-Delegation: vended-credentials,e.g. Apache Polaris).
loadTablereturns credentials scoped to one table and only the JVMFileIOever sees them. The native scan path receives file paths only, so Velox's S3
client falls back to the process credential chain — which by design has no
access to the warehouse — and every native
TableScanof such a table dies withS3 403 (
S3ReadFile.cpp preadInternal: Failed to get S3 object due to: 'Access denied') while the same query reads fine on vanilla Spark.JVM side. At split planning, read the scan table's
FileIO.properties();when a vended access-key/secret pair is present, carry it (plus session
token/expiry/endpoint/region companions) together with the table location in a
new table-scoped
ReadRel.LocalFiles.read_propertiesmap. ALocalFilesissingle-table by construction, so table granularity is split granularity.
FileIO.properties()is implemented byS3FileIO/ResolvingFileIOand defaultsto throwing, which is treated as "no credentials". Tables whose files the process
credentials can read (e.g. Glue-catalog tables) attach nothing and are untouched.
Native side.
parseScanSplitInfocopies the map ontoSplitInfo, andWholeStageResultIteratorunions the task's scans into aGlutenS3TokenProviderinstalled on the
QueryCtxas the file system token provider. Resolution is asegment-boundary-safe longest-prefix match of the file path against the
normalized table locations, so a query joining two vended-credential tables that
live in the same bucket cannot mix them up, and a table nested inside another
table's location still gets its own credentials.
equals()/hash()cover allcredentials because they key Velox's file handle cache — a re-vended credential
set can never be served a handle opened with the set it replaced. When no scan
carries credentials the provider is
nullptr, i.e. behaviour is unchanged.Velox already threads
ConnectorQueryCtx::fsTokenProvider()into every data-fileand delete-file open through
FileHandleKeyandFileOptions.New config.
spark.gluten.sql.columnar.iceberg.enableVendedCredentials(default
true). When disabled, scans of tables read with vended credentialsfail native validation and fall back to vanilla Spark, which reads them
correctly through the JVM
FileIO— a working escape hatch rather than adegraded one.
Backend gate. Consuming
read_propertiesis a Velox-backend capability, sovalidation also requires the new
BackendSettingsApi.supportIcebergVendedCredentialsRead()(defaultfalse,truefor Velox). Without it a ClickHouse-backend scan of a vended-credentialtable would keep failing with access-denied errors; now it falls back to vanilla
Spark and reads correctly. Please shout if you would rather the ClickHouse
backend also implement this — the JVM half is backend-agnostic and already emits
the map.
Dependency on a Velox PR
The native half needs the S3 file system to consume
FileOptions::tokenProvider, which no file system does today (only ABFS and GCShave token-provider paths, and neither uses
FileOptions). That isfacebookincubator/velox#18570,
which adds
S3AccessTokenKey/S3AccessTokenand a per-credential client cachein
S3FileSystem.The second commit here (
[MINOR]) pointsUPSTREAM_VELOX_PR_IDat that PR soCI can build this one, and must be reverted before merge, once the Velox
change is merged and the pin is advanced past it. Happy to split this PR into
the JVM half and the native half if you would rather land them separately.
Notes for reviewers
plane as the broadcast Hadoop conf. Say the word if you would prefer the
config to default to
false(opt-in) instead.lifetime of the vended credentials; executors cannot re-vend. A 403 on expiry
stays a retriable task failure. Refresh-on-open is a follow-up — this is the
same staging
apache/datafusion-cometused for the same problem(comet#3523 static extraction, then comet#4309 pluggable refresh).
How was this patch tested?
New unit tests. The JVM ones pass locally on JDK 17 with
-Pbackends-velox,spark-3.5,icebergand the scalastyle/spotless gates live(
dev/format-scala-code.sh checkis clean):GlutenIcebergSourceUtilSuite(3 tests) — the credential set is extracted withits location and companions and nothing else; a table without a vended
access-key/secret pair extracts nothing; the session token is optional.
IcebergLocalFilesNodeReadPropertiesTest(2 tests) — the map round-trips intoReadRel.LocalFiles.read_properties, and a split without credentials emits noread_properties.GlutenS3TokenProviderTest(5 tests,ENABLE_S3, not run locally — no Veloxbuild environment on hand, so these are on CI) — no provider without
credentials or from an incomplete set; per-table resolution for two tables in
one bucket; longest-prefix wins for nested locations; a string-prefix table
name is not a match; identity covers all credentials; scheme normalization.
Also covered by the Velox PR's Minio-backed tests, which prove that
provider-supplied credentials are the ones used to open the file.
End to end, this patch set (backported to 1.6.0) has been running Iceberg reads
against Apache Polaris with STS credential vending on Spark 3.5.2 in production,
on both arm64 and x86_64: native
TableScanof vended-credential tables readswith the vended credentials instead of 403ing, and row-level parity against
vanilla Spark holds.