You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "kaveti (via GitHub)" <gi...@apache.org> on 2026/03/24 15:19:51 UTC
[PR] Core: Pass storage credentials to ioBuilder-created FileIO in RESTSes… [iceberg]
kaveti opened a new pull request, #15752:
URL: https://github.com/apache/iceberg/pull/15752
## What
[RESTSessionCatalog.newFileIO()](cci:1://file:///Users/rkaveti/Documents/rkaveti/iceberg/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:1175:2-1197:3) has two code paths for creating a `FileIO`:
1. **ioBuilder path** — used when a custom `ioBuilder` is provided (e.g. by Trino)
2. **Reflection path** — used when `ioBuilder` is null, delegates to `CatalogUtil.loadFileIO()`
The reflection path correctly passes storage credentials (vended via [LoadTableResponse](cci:2://file:///Users/rkaveti/Documents/rkaveti/iceberg/core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java:40:0-141:1)) to
`FileIO` implementations that implement `SupportsStorageCredentials`. The `ioBuilder` path,
however, completely ignores the `storageCredentials` parameter — silently discarding them.
## Why this matters
This was introduced in #12591, which added storage credentials V3 support but only wired up
credential passing for the reflection path. The `ioBuilder` path was missed because Trino is
currently the only engine that uses it.
In practice, this means that when a REST catalog server vends storage credentials
(e.g. short-lived S3/GCS/ADLS tokens), any engine using a custom `ioBuilder` never receives
them. For Trino specifically, this blocks the ability to use vended credentials with their
custom `FileIO` — which is the subject of ongoing work in
[trinodb/trino#28425](https://github.com/trinodb/trino/pull/28425).
This was [confirmed as unintentional](https://github.com/trinodb/trino/pull/28425#discussion_r2980583819)
by @nastra.
## The fix
After `ioBuilder.apply(context, properties)` creates the `FileIO`, we now check if the
instance implements `SupportsStorageCredentials` and call [setCredentials()](cci:1://file:///Users/rkaveti/Documents/rkaveti/iceberg/core/src/test/java/org/apache/iceberg/TestCatalogUtil.java:572:4-575:5) with the
converted credentials — matching exactly what `CatalogUtil.loadFileIO()` already does at
lines 418–419.
The change is 8 lines of logic, non-breaking, and covers all 5 call sites that flow through
[tableFileIO()](cci:1://file:///Users/rkaveti/Documents/rkaveti/iceberg/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:1199:2-1208:3) → [newFileIO()](cci:1://file:///Users/rkaveti/Documents/rkaveti/iceberg/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:1175:2-1197:3):
- [loadTable()](cci:1://file:///Users/rkaveti/Documents/rkaveti/iceberg/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:445:2-558:3)
- [registerTable()](cci:1://file:///Users/rkaveti/Documents/rkaveti/iceberg/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:670:2-720:3)
- `Builder.load()`
- [Builder.createTransaction()](cci:1://file:///Users/rkaveti/Documents/rkaveti/iceberg/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:991:4-1019:5)
- [Builder.replaceTransaction()](cci:1://file:///Users/rkaveti/Documents/rkaveti/iceberg/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:1021:4-1084:5)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Pass storage credentials to ioBuilder-created FileIO in RESTSes… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #15752:
URL: https://github.com/apache/iceberg/pull/15752#discussion_r2982730755
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -1175,7 +1176,15 @@ private FileIO newFileIO(SessionContext context, Map<String, String> properties)
private FileIO newFileIO(
SessionContext context, Map<String, String> properties, List<Credential> storageCredentials) {
if (null != ioBuilder) {
- return ioBuilder.apply(context, properties);
+ FileIO fileIO = ioBuilder.apply(context, properties);
+ if (!storageCredentials.isEmpty() && fileIO instanceof SupportsStorageCredentials) {
+ ((SupportsStorageCredentials) fileIO)
Review Comment:
```suggestion
ioWithCredentials
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Pass storage credentials to ioBuilder-created FileIO in RESTSes… [iceberg]
Posted by "kaveti (via GitHub)" <gi...@apache.org>.
kaveti commented on PR #15752:
URL: https://github.com/apache/iceberg/pull/15752#issuecomment-4120036619
@nastra i have addressed your review comments.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Pass storage credentials to ioBuilder-created FileIO in RESTSes… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #15752:
URL: https://github.com/apache/iceberg/pull/15752#discussion_r2982729645
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -1175,7 +1176,15 @@ private FileIO newFileIO(SessionContext context, Map<String, String> properties)
private FileIO newFileIO(
SessionContext context, Map<String, String> properties, List<Credential> storageCredentials) {
if (null != ioBuilder) {
- return ioBuilder.apply(context, properties);
+ FileIO fileIO = ioBuilder.apply(context, properties);
+ if (!storageCredentials.isEmpty() && fileIO instanceof SupportsStorageCredentials) {
Review Comment:
```suggestion
if (!storageCredentials.isEmpty() && fileIO instanceof SupportsStorageCredentials ioWithCredentials) {
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Pass storage credentials to ioBuilder-created FileIO in RESTSes… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #15752:
URL: https://github.com/apache/iceberg/pull/15752#discussion_r2982747979
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3740,6 +3746,72 @@ public void testSequenceNumberConflictThrowsCommitFailed() {
.hasMessageContaining("Validation failed, please retry");
}
+ @Test
+ public void testIoBuilderReceivesStorageCredentials() {
+ Credential credential =
+ ImmutableCredential.builder()
+ .prefix("s3://test-bucket/")
+ .putConfig("s3.access-key-id", "test-access-key")
+ .putConfig("s3.secret-access-key", "test-secret-key")
+ .build();
+
+ // Adapter that injects storage credentials into LoadTableResponse
+ RESTCatalogAdapter adapter =
+ new RESTCatalogAdapter(backendCatalog) {
+ @SuppressWarnings("unchecked")
+ @Override
+ public <T extends RESTResponse> T handleRequest(
+ Route route,
+ Map<String, String> vars,
+ HTTPRequest httpRequest,
+ Class<T> responseType,
+ Consumer<Map<String, String>> responseHeaders) {
+ T response =
+ super.handleRequest(route, vars, httpRequest, responseType, responseHeaders);
+ if (route == Route.LOAD_TABLE && response instanceof LoadTableResponse) {
Review Comment:
```suggestion
if (route == Route.LOAD_TABLE && response instanceof LoadTableResponse loadResponse) {
```
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3740,6 +3746,72 @@ public void testSequenceNumberConflictThrowsCommitFailed() {
.hasMessageContaining("Validation failed, please retry");
}
+ @Test
+ public void testIoBuilderReceivesStorageCredentials() {
+ Credential credential =
+ ImmutableCredential.builder()
+ .prefix("s3://test-bucket/")
+ .putConfig("s3.access-key-id", "test-access-key")
+ .putConfig("s3.secret-access-key", "test-secret-key")
+ .build();
+
+ // Adapter that injects storage credentials into LoadTableResponse
+ RESTCatalogAdapter adapter =
+ new RESTCatalogAdapter(backendCatalog) {
+ @SuppressWarnings("unchecked")
+ @Override
+ public <T extends RESTResponse> T handleRequest(
+ Route route,
+ Map<String, String> vars,
+ HTTPRequest httpRequest,
+ Class<T> responseType,
+ Consumer<Map<String, String>> responseHeaders) {
+ T response =
+ super.handleRequest(route, vars, httpRequest, responseType, responseHeaders);
+ if (route == Route.LOAD_TABLE && response instanceof LoadTableResponse) {
+ LoadTableResponse loadResponse = (LoadTableResponse) response;
Review Comment:
```suggestion
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Pass storage credentials to ioBuilder-created FileIO in RESTSes… [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #15752:
URL: https://github.com/apache/iceberg/pull/15752
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Pass storage credentials to ioBuilder-created FileIO in RESTSes… [iceberg]
Posted by "findinpath (via GitHub)" <gi...@apache.org>.
findinpath commented on code in PR #15752:
URL: https://github.com/apache/iceberg/pull/15752#discussion_r2982447066
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -1175,7 +1176,15 @@ private FileIO newFileIO(SessionContext context, Map<String, String> properties)
private FileIO newFileIO(
SessionContext context, Map<String, String> properties, List<Credential> storageCredentials) {
if (null != ioBuilder) {
- return ioBuilder.apply(context, properties);
+ FileIO fileIO = ioBuilder.apply(context, properties);
+ if (!storageCredentials.isEmpty() && fileIO instanceof SupportsStorageCredentials) {
Review Comment:
@nastra could you pls take a look at this change?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Pass storage credentials to ioBuilder-created FileIO in RESTSes… [iceberg]
Posted by "findinpath (via GitHub)" <gi...@apache.org>.
findinpath commented on code in PR #15752:
URL: https://github.com/apache/iceberg/pull/15752#discussion_r3288574387
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -1175,7 +1176,15 @@ private FileIO newFileIO(SessionContext context, Map<String, String> properties)
private FileIO newFileIO(
SessionContext context, Map<String, String> properties, List<Credential> storageCredentials) {
if (null != ioBuilder) {
- return ioBuilder.apply(context, properties);
+ FileIO fileIO = ioBuilder.apply(context, properties);
+ if (!storageCredentials.isEmpty()
+ && fileIO instanceof SupportsStorageCredentials ioWithCredentials) {
+ ioWithCredentials.setCredentials(
+ storageCredentials.stream()
+ .map(c -> StorageCredential.create(c.prefix(), c.config()))
+ .collect(Collectors.toList()));
+ }
+ return fileIO;
Review Comment:
This approach is not right :(
With the statement
```
ioBuilder.apply(context, properties);
```
we already create a file system factory in Trino (and also wrap it into a ForwardingFileIo).
https://github.com/trinodb/trino/blob/ae8daa0a0a07d43c6c36ce8085f7bbe7dd599d03/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java#L117-L122
Ideally we would need to give the `storageCredentials` at the same time as the properties to the `ioBuilder`
e.g.
```
ioBuilder.apply(context, properties, storageCredentials);
```
(side note, there is no `TriFunction` in JDK and we'll likely need to introduce an interface if we agree on making such a change)
cc @nastra @kaveti
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org