Core: Pass storage credentials to ioBuilder-created FileIO in RESTSes…#15752
Open
kaveti wants to merge 1 commit intoapache:mainfrom
Open
Core: Pass storage credentials to ioBuilder-created FileIO in RESTSes…#15752kaveti wants to merge 1 commit intoapache:mainfrom
kaveti wants to merge 1 commit intoapache:mainfrom
Conversation
findinpath
reviewed
Mar 24, 2026
| if (null != ioBuilder) { | ||
| return ioBuilder.apply(context, properties); | ||
| FileIO fileIO = ioBuilder.apply(context, properties); | ||
| if (!storageCredentials.isEmpty() && fileIO instanceof SupportsStorageCredentials) { |
Contributor
There was a problem hiding this comment.
@nastra could you pls take a look at this change?
nastra
reviewed
Mar 24, 2026
| if (null != ioBuilder) { | ||
| return ioBuilder.apply(context, properties); | ||
| FileIO fileIO = ioBuilder.apply(context, properties); | ||
| if (!storageCredentials.isEmpty() && fileIO instanceof SupportsStorageCredentials) { |
Contributor
There was a problem hiding this comment.
Suggested change
| if (!storageCredentials.isEmpty() && fileIO instanceof SupportsStorageCredentials) { | |
| if (!storageCredentials.isEmpty() && fileIO instanceof SupportsStorageCredentials ioWithCredentials) { |
nastra
reviewed
Mar 24, 2026
| return ioBuilder.apply(context, properties); | ||
| FileIO fileIO = ioBuilder.apply(context, properties); | ||
| if (!storageCredentials.isEmpty() && fileIO instanceof SupportsStorageCredentials) { | ||
| ((SupportsStorageCredentials) fileIO) |
Contributor
There was a problem hiding this comment.
Suggested change
| ((SupportsStorageCredentials) fileIO) | |
| ioWithCredentials |
nastra
reviewed
Mar 24, 2026
| Consumer<Map<String, String>> responseHeaders) { | ||
| T response = | ||
| super.handleRequest(route, vars, httpRequest, responseType, responseHeaders); | ||
| if (route == Route.LOAD_TABLE && response instanceof LoadTableResponse) { |
Contributor
There was a problem hiding this comment.
Suggested change
| if (route == Route.LOAD_TABLE && response instanceof LoadTableResponse) { | |
| if (route == Route.LOAD_TABLE && response instanceof LoadTableResponse loadResponse) { |
nastra
reviewed
Mar 24, 2026
| T response = | ||
| super.handleRequest(route, vars, httpRequest, responseType, responseHeaders); | ||
| if (route == Route.LOAD_TABLE && response instanceof LoadTableResponse) { | ||
| LoadTableResponse loadResponse = (LoadTableResponse) response; |
Contributor
There was a problem hiding this comment.
Suggested change
| LoadTableResponse loadResponse = (LoadTableResponse) response; |
nastra
approved these changes
Mar 24, 2026
Contributor
nastra
left a comment
nastra
left a comment
There was a problem hiding this comment.
LGTM, left just a few small comments
…sionCatalog RESTSessionCatalog.newFileIO() has two code paths for creating a FileIO: 1. ioBuilder path - when a custom ioBuilder is provided (used by Trino) 2. Reflection path - when ioBuilder is null (uses CatalogUtil.loadFileIO()) The reflection path correctly passes storage credentials to FileIO implementations that implement SupportsStorageCredentials via setCredentials(). However, the ioBuilder path completely ignores the storageCredentials parameter, silently discarding vended credentials. After ioBuilder.apply() creates the FileIO, check if it implements SupportsStorageCredentials and call setCredentials() - matching the behavior of CatalogUtil.loadFileIO().
kaveti
force-pushed
the
fix-iobuilder-storage-credentials
branch
from
62f5b66 to
9f64efb
Compare
Author
|
@nastra i have addressed your review comments. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
RESTSessionCatalog.newFileIO() has two code paths for creating a
FileIO:ioBuilderis provided (e.g. by Trino)ioBuilderis null, delegates toCatalogUtil.loadFileIO()The reflection path correctly passes storage credentials (vended via LoadTableResponse) to
FileIOimplementations that implementSupportsStorageCredentials. TheioBuilderpath,however, completely ignores the
storageCredentialsparameter — 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
ioBuilderpath was missed because Trino iscurrently 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
ioBuildernever receivesthem. For Trino specifically, this blocks the ability to use vended credentials with their
custom
FileIO— which is the subject of ongoing work intrinodb/trino#28425.
confirmed as unintentional
The fix
After
ioBuilder.apply(context, properties)creates theFileIO, we now check if theinstance implements
SupportsStorageCredentialsand call setCredentials() with theconverted credentials — matching exactly what
CatalogUtil.loadFileIO()already does atlines 418–419.
The change is 8 lines of logic, non-breaking, and covers all 5 call sites that flow through
tableFileIO() → newFileIO():
Builder.load()