Skip to content

Support Iceberg REST catalog storage credentials in ForwardingFileIo #28425

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kaveti wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Support Iceberg REST catalog storage credentials in ForwardingFileIo #28425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f433dbc
Update Iceberg to 1.11.0
ebyhr Sep 16, 2025
4f8d230
Support vended credentials from Iceberg REST catalog
kaveti Apr 9, 2026
afccbc6
Support Iceberg V3 storage credentials in REST catalog
kaveti Apr 9, 2026
16530f9
Reshape REST storage credentials flow for Iceberg 1.11.
kaveti Apr 9, 2026
7feb28b
Harden storage-credential routing test coverage.
kaveti Apr 9, 2026
c4e7643
Document REST fixture credentials behavior with a real backend test.
kaveti Apr 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 43 additions & 6 deletions ...rg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@
package io.trino.plugin.iceberg.catalog.rest;

import com.google.common.cache.Cache;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
import com.google.errorprone.annotations.concurrent.GuardedBy;
import com.google.inject.Inject;
import io.airlift.units.Duration;
import io.trino.cache.EvictableCacheBuilder;
import io.trino.plugin.iceberg.IcebergConfig;
import io.trino.plugin.iceberg.IcebergFileSystemFactory;
Expand All @@ -33,14 +35,18 @@
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.rest.HTTPClient;
import org.apache.iceberg.rest.RESTCatalogProperties;
import org.apache.iceberg.rest.RESTSessionCatalog;
import org.apache.iceberg.rest.RESTUtil;

import java.net.URI;
import java.util.Map;
import java.util.Optional;
import java.util.Set;

import static java.util.Objects.requireNonNull;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static org.apache.iceberg.CatalogProperties.AUTH_SESSION_TIMEOUT_MS;
import static org.apache.iceberg.rest.auth.OAuth2Properties.CREDENTIAL;
import static org.apache.iceberg.rest.auth.OAuth2Properties.TOKEN;

Expand All @@ -51,12 +57,18 @@ public class TrinoIcebergRestCatalogFactory
private final ForwardingFileIoFactory fileIoFactory;
private final CatalogName catalogName;
private final String trinoVersion;
private final URI serverUri;
private final Optional<String> prefix;
private final Optional<String> warehouse;
private final boolean nestedNamespaceEnabled;
private final Security security;
private final SessionType sessionType;
private final Optional<Duration> connectionTimeout;
private final Optional<Duration> socketTimeout;
private final Duration sessionTimeout;
private final boolean vendedCredentialsEnabled;
private final boolean viewEndpointsEnabled;
private final SecurityProperties securityProperties;
private final IcebergRestCatalogPropertiesProvider catalogPropertiesProvider;
private final boolean uniqueTableLocation;
private final TypeManager typeManager;
private final boolean caseInsensitiveNameMatching;
Expand All @@ -73,7 +85,6 @@ public TrinoIcebergRestCatalogFactory(
CatalogName catalogName,
IcebergRestCatalogConfig restConfig,
SecurityProperties securityProperties,
IcebergRestCatalogPropertiesProvider catalogPropertiesProvider,
IcebergConfig icebergConfig,
TypeManager typeManager,
NodeVersion nodeVersion)
Expand All @@ -83,12 +94,18 @@ public TrinoIcebergRestCatalogFactory(
this.catalogName = requireNonNull(catalogName, "catalogName is null");
this.trinoVersion = requireNonNull(nodeVersion, "nodeVersion is null").toString();
requireNonNull(restConfig, "restConfig is null");
this.serverUri = restConfig.getBaseUri();
this.prefix = restConfig.getPrefix();
this.warehouse = restConfig.getWarehouse();
this.nestedNamespaceEnabled = restConfig.isNestedNamespaceEnabled();
this.security = restConfig.getSecurity();
this.sessionType = restConfig.getSessionType();
this.connectionTimeout = restConfig.getConnectionTimeout();
this.socketTimeout = restConfig.getSocketTimeout();
this.sessionTimeout = restConfig.getSessionTimeout();
this.vendedCredentialsEnabled = restConfig.isVendedCredentialsEnabled();
this.viewEndpointsEnabled = restConfig.isViewEndpointsEnabled();
this.securityProperties = requireNonNull(securityProperties, "securityProperties is null");
this.catalogPropertiesProvider = requireNonNull(catalogPropertiesProvider, "catalogPropertiesProvider is null");
requireNonNull(icebergConfig, "icebergConfig is null");
this.uniqueTableLocation = icebergConfig.isUniqueTableLocation();
this.typeManager = requireNonNull(typeManager, "typeManager is null");
Expand All @@ -109,6 +126,21 @@ public synchronized TrinoCatalog create(ConnectorIdentity identity)
// Creation of the RESTSessionCatalog is lazy due to required network calls
// for authorization and config route
if (icebergCatalog == null) {
ImmutableMap.Builder<String, String> properties = ImmutableMap.builder();
properties.put(CatalogProperties.URI, serverUri.toString());
warehouse.ifPresent(location -> properties.put(CatalogProperties.WAREHOUSE_LOCATION, location));
prefix.ifPresent(prefix -> properties.put("prefix", prefix));
properties.put(RESTCatalogProperties.VIEW_ENDPOINTS_SUPPORTED, Boolean.toString(viewEndpointsEnabled));
properties.put("trino-version", trinoVersion);
properties.put(AUTH_SESSION_TIMEOUT_MS, String.valueOf(sessionTimeout.toMillis()));
connectionTimeout.ifPresent(duration -> properties.put("rest.client.connection-timeout-ms", String.valueOf(duration.toMillis())));
socketTimeout.ifPresent(duration -> properties.put("rest.client.socket-timeout-ms", String.valueOf(duration.toMillis())));
properties.putAll(securityProperties.get());

if (vendedCredentialsEnabled) {
properties.put("header.X-Iceberg-Access-Delegation", "vended-credentials");
}

RESTSessionCatalog icebergCatalogInstance = new RESTSessionCatalog(
config -> HTTPClient.builder(config)
.uri(config.get(CatalogProperties.URI))
Expand All @@ -117,10 +149,15 @@ public synchronized TrinoCatalog create(ConnectorIdentity identity)
(context, config) -> {
Comment thread
kaveti marked this conversation as resolved.
ConnectorIdentity currentIdentity = (context.wrappedIdentity() != null)
Copy link
Copy Markdown
Contributor

@findinpath findinpath Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the changes from apache/iceberg#12591 disregarded the fact that the ioBuilder is not receiving now the storageCredentials.

@nastra was it intentional to skip it during the implementation phase?

Copy link
Copy Markdown
Contributor

@nastra nastra Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@findinpath this was not intentional. I believe Trino is the only engine that goes through that path. Can you please open a PR with a fix?

Copy link
Copy Markdown
Contributor Author

@kaveti kaveti Mar 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opened PR: apache/iceberg#15752 , cc @nastra

? ((ConnectorIdentity) context.wrappedIdentity())
: ConnectorIdentity.ofUser("fake");
return fileIoFactory.create(fileSystemFactory.create(currentIdentity, config), true, config);
: ConnectorIdentity.ofUser("trino");
return fileIoFactory.create(
fileSystemFactory.create(currentIdentity, config),
true,
config,
fileSystemFactory,
currentIdentity);
});
icebergCatalogInstance.initialize(catalogName.toString(), catalogPropertiesProvider.catalogProperties());
icebergCatalogInstance.initialize(catalogName.toString(), properties.buildOrThrow());

icebergCatalog = icebergCatalogInstance;
}
Expand Down
31 changes: 5 additions & 26 deletions ...in/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,27 +204,16 @@ private List<String> collectNamespaces(ConnectorSession session, Namespace paren
{
try {
return restSessionCatalog.listNamespaces(convert(session), parentNamespace).stream()
.flatMap(childNamespace -> collectNamespaceIfExists(session, childNamespace).stream())
.flatMap(childNamespace -> Stream.concat(
Stream.of(childNamespace.toString()),
collectNamespaces(session, childNamespace).stream()))
.collect(toImmutableList());
}
catch (RESTException e) {
throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list namespaces", e);
}
}

private List<String> collectNamespaceIfExists(ConnectorSession session, Namespace namespace)
{
try {
return Stream.concat(
Stream.of(namespace.toString()),
collectNamespaces(session, namespace).stream())
.collect(toImmutableList());
}
catch (NoSuchNamespaceException e) {
return ImmutableList.of();
}
}

@Override
public void dropNamespace(ConnectorSession session, String namespace)
{
Expand Down Expand Up @@ -892,7 +881,7 @@ private SessionCatalog.SessionContext convert(ConnectorSession session)
return switch (sessionType) {
case NONE -> new SessionContext(randomUUID().toString(), null, credentials, ImmutableMap.of(), session.getIdentity());
case USER -> {
String sessionId = format("%s-%s-%s", session.getUser(), session.getQueryId(), session.getSource().orElse("default"));
String sessionId = format("%s-%s", session.getUser(), session.getSource().orElse("default"));

Map<String, String> properties = ImmutableMap.of(
"user", session.getUser(),
Expand Down Expand Up @@ -1067,17 +1056,7 @@ private List<Namespace> listNamespaces(ConnectorSession session, Namespace paren
catch (RESTException e) {
throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list namespaces", e);
}
return childNamespaces.stream().flatMap(childNamespace -> listNamespaceIfExists(session, childNamespace).stream()).toList();
}

private List<Namespace> listNamespaceIfExists(ConnectorSession session, Namespace namespace)
{
try {
return Stream.concat(Stream.of(namespace), listNamespaces(session, namespace).stream()).toList();
}
catch (NoSuchNamespaceException e) {
return ImmutableList.of();
}
return childNamespaces.stream().flatMap(childNamespace -> Stream.concat(Stream.of(childNamespace), listNamespaces(session, childNamespace).stream())).toList();
}

private static Namespace toTrinoNamespace(Namespace namespace)
Expand Down
89 changes: 84 additions & 5 deletions plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,23 +14,30 @@
package io.trino.plugin.iceberg.fileio;

import com.google.common.annotations.VisibleForTesting;
Comment thread
kaveti marked this conversation as resolved.
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
Comment thread
kaveti marked this conversation as resolved.
import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
import io.trino.filesystem.Location;
import io.trino.filesystem.TrinoFileSystem;
import io.trino.plugin.iceberg.IcebergFileSystemFactory;
import io.trino.spi.TrinoException;
import io.trino.spi.security.ConnectorIdentity;
import org.apache.iceberg.DataFile;
import org.apache.iceberg.DeleteFile;
import org.apache.iceberg.ManifestFile;
import org.apache.iceberg.ManifestListFile;
import org.apache.iceberg.io.BulkDeletionFailureException;
import org.apache.iceberg.io.InputFile;
import org.apache.iceberg.io.OutputFile;
import org.apache.iceberg.io.StorageCredential;
Comment thread
kaveti marked this conversation as resolved.
import org.apache.iceberg.io.SupportsBulkOperations;
import org.apache.iceberg.io.SupportsStorageCredentials;

import java.io.IOException;
import java.io.UncheckedIOException;
import java.util.ArrayList;
import java.util.IdentityHashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.Callable;
Expand All @@ -46,7 +53,7 @@
import static java.util.stream.Collectors.joining;

public class ForwardingFileIo
implements SupportsBulkOperations
implements SupportsBulkOperations, SupportsStorageCredentials
Copy link
Copy Markdown
Contributor

@findinpath findinpath Mar 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking at S3FileIO

https://github.com/apache/iceberg/blob/main/aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java#L114

https://github.com/apache/iceberg/blob/main/aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java#L102

and am rather thinking that we should have a prefixed map of TrinoFileSystem actually (and probably the default one we used to have as fallback).

{
private static final int DELETE_BATCH_SIZE = 1000;
private static final int BATCH_DELETE_PATHS_MESSAGE_LIMIT = 5;
Expand All @@ -55,6 +62,11 @@ public class ForwardingFileIo
private final Map<String, String> properties;
private final boolean useFileSizeFromMetadata;
private final ExecutorService deleteExecutor;
private final IcebergFileSystemFactory storageCredentialFileSystemFactory;
private final ConnectorIdentity storageCredentialIdentity;

private volatile List<StorageCredential> storageCredentials = ImmutableList.of();
private volatile Map<String, TrinoFileSystem> prefixedFileSystems = ImmutableMap.of();

@VisibleForTesting
public ForwardingFileIo(TrinoFileSystem fileSystem, boolean useFileSizeFromMetadata)
Expand All @@ -63,22 +75,36 @@ public ForwardingFileIo(TrinoFileSystem fileSystem, boolean useFileSizeFromMetad
}

public ForwardingFileIo(TrinoFileSystem fileSystem, Map<String, String> properties, boolean useFileSizeFromMetadata, ExecutorService deleteExecutor)
{
this(fileSystem, properties, useFileSizeFromMetadata, deleteExecutor, null, null);
}

public ForwardingFileIo(
TrinoFileSystem fileSystem,
Map<String, String> properties,
boolean useFileSizeFromMetadata,
ExecutorService deleteExecutor,
IcebergFileSystemFactory storageCredentialFileSystemFactory,
ConnectorIdentity storageCredentialIdentity)
{
this.fileSystem = requireNonNull(fileSystem, "fileSystem is null");
this.deleteExecutor = requireNonNull(deleteExecutor, "executorService is null");
this.properties = ImmutableMap.copyOf(requireNonNull(properties, "properties is null"));
this.useFileSizeFromMetadata = useFileSizeFromMetadata;
this.storageCredentialFileSystemFactory = storageCredentialFileSystemFactory;
this.storageCredentialIdentity = storageCredentialIdentity;
}

@Override
public InputFile newInputFile(String path)
{
return new ForwardingInputFile(fileSystem.newInputFile(Location.of(path)));
return new ForwardingInputFile(fileSystemForPath(path).newInputFile(Location.of(path)));
}

@Override
public InputFile newInputFile(String path, long length)
{
TrinoFileSystem fileSystem = fileSystemForPath(path);
if (!useFileSizeFromMetadata) {
return new ForwardingInputFile(fileSystem.newInputFile(Location.of(path)));
}
Expand All @@ -89,14 +115,14 @@ public InputFile newInputFile(String path, long length)
@Override
public OutputFile newOutputFile(String path)
{
return new ForwardingOutputFile(fileSystem, Location.of(path));
return new ForwardingOutputFile(fileSystemForPath(path), Location.of(path));
}

@Override
public void deleteFile(String path)
{
try {
fileSystem.deleteFile(Location.of(path));
fileSystemForPath(path).deleteFile(Location.of(path));
}
catch (IOException e) {
throw new UncheckedIOException("Failed to delete file: " + path, e);
Expand Down Expand Up @@ -159,7 +185,15 @@ public InputFile newInputFile(ManifestListFile manifestList)
private void deleteBatch(List<String> filesToDelete)
{
try {
fileSystem.deleteFiles(filesToDelete.stream().map(Location::of).toList());
Map<TrinoFileSystem, List<Location>> locationsByFileSystem = new IdentityHashMap<>();
for (String path : filesToDelete) {
TrinoFileSystem fileSystem = fileSystemForPath(path);
locationsByFileSystem.computeIfAbsent(fileSystem, ignored -> new ArrayList<>())
.add(Location.of(path));
}
for (Map.Entry<TrinoFileSystem, List<Location>> entry : locationsByFileSystem.entrySet()) {
entry.getKey().deleteFiles(entry.getValue());
}
}
catch (IOException e) {
throw new UncheckedIOException(
Expand All @@ -179,6 +213,51 @@ public Map<String, String> properties()
return properties;
}

@Override
public void setCredentials(List<StorageCredential> credentials)
{
storageCredentials = ImmutableList.copyOf(requireNonNull(credentials, "credentials is null"));
rebuildPrefixedFileSystems();
}

@Override
public List<StorageCredential> credentials()
{
return storageCredentials;
}

private TrinoFileSystem fileSystemForPath(String path)
{
TrinoFileSystem matchingFileSystem = fileSystem;
int matchingPrefixLength = -1;
for (Map.Entry<String, TrinoFileSystem> prefixedFileSystem : prefixedFileSystems.entrySet()) {
String prefix = prefixedFileSystem.getKey();
if (path.startsWith(prefix) && prefix.length() > matchingPrefixLength) {
matchingPrefixLength = prefix.length();
matchingFileSystem = prefixedFileSystem.getValue();
}
}
return matchingFileSystem;
}

private void rebuildPrefixedFileSystems()
{
if (storageCredentials.isEmpty() || storageCredentialFileSystemFactory == null || storageCredentialIdentity == null) {
prefixedFileSystems = ImmutableMap.of();
return;
}

ImmutableMap.Builder<String, TrinoFileSystem> rebuiltFileSystems = ImmutableMap.builder();
for (StorageCredential storageCredential : storageCredentials) {
Map<String, String> mergedProperties = ImmutableMap.<String, String>builder()
.putAll(properties)
.putAll(storageCredential.config())
.buildKeepingLast();
rebuiltFileSystems.put(storageCredential.prefix(), storageCredentialFileSystemFactory.create(storageCredentialIdentity, mergedProperties));
}
prefixedFileSystems = rebuiltFileSystems.buildKeepingLast();
}

@Override
public void initialize(Map<String, String> properties)
{
Expand Down
18 changes: 18 additions & 0 deletions ...n/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIoFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
import com.google.inject.Inject;
import io.trino.filesystem.TrinoFileSystem;
import io.trino.plugin.iceberg.ForIcebergFileDelete;
import io.trino.plugin.iceberg.IcebergFileSystemFactory;
import io.trino.spi.security.ConnectorIdentity;
import org.apache.iceberg.io.FileIO;

import java.util.Map;
Expand Down Expand Up @@ -48,4 +50,20 @@ public FileIO create(TrinoFileSystem fileSystem, boolean useFileSizeFromMetadata
{
return new ForwardingFileIo(fileSystem, properties, useFileSizeFromMetadata, deleteExecutor);
}

public FileIO create(
TrinoFileSystem fileSystem,
boolean useFileSizeFromMetadata,
Map<String, String> properties,
IcebergFileSystemFactory storageCredentialFileSystemFactory,
ConnectorIdentity storageCredentialIdentity)
{
return new ForwardingFileIo(
fileSystem,
properties,
useFileSizeFromMetadata,
deleteExecutor,
storageCredentialFileSystemFactory,
storageCredentialIdentity);
}
}
Loading
Loading