From b7a09db9030ea00a1cf251033a5d39657093583d Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Tue, 16 Sep 2025 11:01:07 +0900 Subject: [PATCH 1/9] Update Iceberg to 1.11.0 --- .../java/io/trino/plugin/iceberg/IcebergMetadata.java | 2 +- .../java/io/trino/plugin/iceberg/IcebergSplitSource.java | 2 +- .../io/trino/plugin/iceberg/delete/DeleteManager.java | 2 +- .../io/trino/plugin/iceberg/fileio/ForwardingFileIo.java | 7 +++++++ .../TestIcebergPolarisCatalogConnectorSmokeTest.java | 4 ++-- ...bergRestCatalogNestedNamespaceConnectorSmokeTest.java | 2 +- .../rest/TestIcebergS3TablesConnectorSmokeTest.java | 2 +- .../TestIcebergTrinoRestCatalogConnectorSmokeTest.java | 2 +- .../java/org/apache/iceberg/rest/RestCatalogServlet.java | 1 - pom.xml | 9 ++++++++- 10 files changed, 23 insertions(+), 10 deletions(-) diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java index fe94a28f4dcd..001adf64ef01 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java @@ -3632,7 +3632,7 @@ private void finishWrite(ConnectorSession session, IcebergTableHandle table, Col switch (task.content()) { case DATA -> dataTasks.add(task); case POSITION_DELETES -> deleteTasks.add(task); - case EQUALITY_DELETES -> throw new UnsupportedOperationException("Unsupported task content: " + task.content()); + case EQUALITY_DELETES, DATA_MANIFEST, DELETE_MANIFEST -> throw new UnsupportedOperationException("Unsupported task content: " + task.content()); } } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitSource.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitSource.java index d5e50213e495..24329350a1e0 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitSource.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitSource.java @@ -361,7 +361,7 @@ private synchronized Iterator prepareFileTasksIterator(L } yield isUnconstrainedPathAndTimeDomain(); } - case DATA -> throw new IllegalStateException("Unexpected delete file: " + deleteFile); + case DATA, DATA_MANIFEST, DELETE_MANIFEST -> throw new IllegalStateException("Unexpected delete file: " + deleteFile); }) .collect(toImmutableList()); scannedFiles.add(new DataFileWithDeleteFiles(wholeFileTask.file(), fullyAppliedDeletes)); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/delete/DeleteManager.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/delete/DeleteManager.java index 3fcc2d94d12a..7823f3a60865 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/delete/DeleteManager.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/delete/DeleteManager.java @@ -85,7 +85,7 @@ public Optional getDeletePredicate( } } case EQUALITY_DELETES -> equalityDeleteFiles.add(deleteFile); - case DATA -> throw new VerifyException("DATA is not delete file type"); + case DATA, DATA_MANIFEST, DELETE_MANIFEST -> throw new VerifyException("DATA is not delete file type"); } } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java index 81e58b6b9c2d..dd92a6cc532a 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java @@ -23,6 +23,7 @@ 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; @@ -149,6 +150,12 @@ public InputFile newInputFile(DeleteFile file) return SupportsBulkOperations.super.newInputFile(file); } + @Override + public InputFile newInputFile(ManifestListFile manifestList) + { + return SupportsBulkOperations.super.newInputFile(manifestList); + } + private void deleteBatch(List filesToDelete) { try { diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergPolarisCatalogConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergPolarisCatalogConnectorSmokeTest.java index 6257a4b7aa70..7291415f393b 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergPolarisCatalogConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergPolarisCatalogConnectorSmokeTest.java @@ -218,7 +218,7 @@ public void testCreateTableWithTrailingSpaceInLocation() public void testDropTableWithMissingMetadataFile() { assertThatThrownBy(super::testDropTableWithMissingMetadataFile) - .hasMessageMatching(".* Table '.*' does not exist"); + .hasMessageMatching("Failed to load table: (.*)"); } @Test @@ -242,7 +242,7 @@ public void testDropTableWithMissingManifestListFile() public void testDropTableWithNonExistentTableLocation() { assertThatThrownBy(super::testDropTableWithNonExistentTableLocation) - .hasMessageMatching(".* Table '.*' does not exist"); + .hasMessageMatching("Failed to load table: (.*)"); } @Test diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergRestCatalogNestedNamespaceConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergRestCatalogNestedNamespaceConnectorSmokeTest.java index e98f0e2daea4..61e5a40f13be 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergRestCatalogNestedNamespaceConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergRestCatalogNestedNamespaceConnectorSmokeTest.java @@ -234,7 +234,7 @@ public void testDropTableWithMissingSnapshotFile() assertThatThrownBy(super::testDropTableWithMissingSnapshotFile) .isInstanceOf(QueryFailedException.class) .cause() - .hasMessageContaining("Failed to drop table") + .hasMessageMatching("Failed to open input stream for file: .*avro") .hasNoCause(); } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3TablesConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3TablesConnectorSmokeTest.java index 469ccbf13d8c..9f5401ed5f12 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3TablesConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3TablesConnectorSmokeTest.java @@ -180,7 +180,7 @@ public void testCreateTableWithTrailingSpaceInLocation() public void testRenameTable() { assertThatThrownBy(super::testRenameTable) - .hasStackTraceContaining("Unable to process: RenameTable endpoint is not supported for Glue Catalog"); + .hasStackTraceContaining("RenameTable endpoint is not supported for Glue Catalog"); } @Test diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergTrinoRestCatalogConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergTrinoRestCatalogConnectorSmokeTest.java index ccc9fa724fc5..b3557784d931 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergTrinoRestCatalogConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergTrinoRestCatalogConnectorSmokeTest.java @@ -230,7 +230,7 @@ public void testDropTableWithMissingSnapshotFile() assertThatThrownBy(super::testDropTableWithMissingSnapshotFile) .isInstanceOf(QueryFailedException.class) .cause() - .hasMessageContaining("Failed to drop table") + .hasMessageMatching("Failed to open input stream for file: .*avro") .hasNoCause(); } diff --git a/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/RestCatalogServlet.java b/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/RestCatalogServlet.java index e50c3fb58709..0f38e5ab0711 100644 --- a/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/RestCatalogServlet.java +++ b/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/RestCatalogServlet.java @@ -23,7 +23,6 @@ import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.apache.iceberg.relocated.com.google.common.io.CharStreams; import org.apache.iceberg.rest.HTTPRequest.HTTPMethod; -import org.apache.iceberg.rest.RESTCatalogAdapter.Route; import org.apache.iceberg.rest.responses.ErrorResponse; import org.apache.iceberg.util.Pair; diff --git a/pom.xml b/pom.xml index bab49244c09e..8444bf339f44 100644 --- a/pom.xml +++ b/pom.xml @@ -198,7 +198,7 @@ v24.12.0 11.7.0 5.4.2 - 1.10.1 + 1.11.0-SNAPSHOT 5.18.1 0.13.0 1.20.0 @@ -2369,6 +2369,13 @@ + + + apache-snapshots + https://repository.apache.org/content/repositories/snapshots + + + From d2a4f53f80686d4e5a7d0b9dc8bcbf77e6a940f8 Mon Sep 17 00:00:00 2001 From: chenjian2664 Date: Tue, 28 Oct 2025 13:06:18 +0900 Subject: [PATCH 2/9] Test time travel with schema evolution in Iceberg --- .../iceberg/BaseIcebergConnectorTest.java | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java index d7be3c34682e..16aaf665a3ca 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java @@ -7927,6 +7927,77 @@ public void testPreparedStatementWithParameterizedVersionedTable() assertQueryFails(session, "DESCRIBE OUTPUT my_query", ".* DESCRIBE is not supported if a versioned table uses parameters"); } + @Test + public void testTimeTravelWithFilterOnRenamedColumn() + { + testTimeTravelWithFilterOnRenamedColumn(false); + testTimeTravelWithFilterOnRenamedColumn(true); + } + + private void testTimeTravelWithFilterOnRenamedColumn(boolean partitioned) + { + String partition = partitioned ? "WITH (partitioning = ARRAY['part'])" : ""; + try (TestTable table = newTrinoTable("time_travel_with_filter_on_rename_", "(x int, y int, part int)" + partition)) { + assertUpdate("INSERT INTO " + table.getName() + " VALUES (1, 1, 1), (1, 2, 2), (2, 2, 2)", 3); + assertThat(query("SELECT * FROM " + table.getName())) + .matches("VALUES (1, 1, 1), (1, 2, 2), (2, 2, 2)"); + long firstSnapshotId = getCurrentSnapshotId(table.getName()); + + assertUpdate("ALTER TABLE " + table.getName() + " RENAME COLUMN x TO renamed_x"); + + // generate a new version + assertUpdate("INSERT INTO " + table.getName() + " VALUES (1, 2, 3)", 1); + + assertThat(query("SELECT * FROM " + table.getName() + " FOR VERSION AS OF " + firstSnapshotId + " WHERE x = 1")) + .matches("VALUES (1, 1, 1), (1, 2, 2)"); + } + } + + @Test + public void testTimeTravelWithFilterOnDroppedColumn() + { + testTimeTravelWithFilterOnDroppedColumn(false); + testTimeTravelWithFilterOnDroppedColumn(true); + } + + private void testTimeTravelWithFilterOnDroppedColumn(boolean partitioned) + { + String partition = partitioned ? "WITH (partitioning = ARRAY['part'])" : ""; + try (TestTable table = newTrinoTable("time_travel_with_filter_on_drop_", "(x int, y int, part int)" + partition)) { + assertUpdate("INSERT INTO " + table.getName() + " VALUES (1, 1, 1), (1, 2, 2), (2, 2, 2)", 3); + assertThat(query("SELECT * FROM " + table.getName())) + .matches("VALUES (1, 1, 1), (1, 2, 2), (2, 2, 2)"); + long firstSnapshotId = getCurrentSnapshotId(table.getName()); + + assertUpdate("ALTER TABLE " + table.getName() + " DROP COLUMN x"); + + // generate a new version + assertUpdate("INSERT INTO " + table.getName() + " VALUES (1, 2)", 1); + + assertThat(query("SELECT * FROM " + table.getName() + " FOR VERSION AS OF " + firstSnapshotId + " WHERE x = 1")) + .matches("VALUES (1, 1, 1), (1, 2, 2)"); + } + } + + @Test + public void testTimeTravelWithFilterOnRenamedPartitionColumn() + { + try (TestTable table = newTrinoTable("time_travel_with_filter_on_drop_", "(x int, part1 int, part2 int) WITH (partitioning = ARRAY['part1', 'part2'])")) { + assertUpdate("INSERT INTO " + table.getName() + " VALUES (1, 1, 1), (1, 1, 2), (2, 2, 2)", 3); + assertThat(query("SELECT * FROM " + table.getName())) + .matches("VALUES (1, 1, 1), (1, 1, 2), (2, 2, 2)"); + long firstSnapshotId = getCurrentSnapshotId(table.getName()); + + assertUpdate("ALTER TABLE " + table.getName() + " RENAME COLUMN part1 TO renamed_part"); + + // generate a new version + assertUpdate("INSERT INTO " + table.getName() + " VALUES (1, 1, 3)", 1); + + assertThat(query("SELECT * FROM " + table.getName() + " FOR VERSION AS OF " + firstSnapshotId + " WHERE part1 = 1")) + .matches("VALUES (1, 1, 1), (1, 1, 2)"); + } + } + @Test public void testDeleteRetainsTableHistory() { From 7535bd4bb77a55c0aad49a990682fe54d3921b8f Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Thu, 13 Nov 2025 16:53:22 +0900 Subject: [PATCH 3/9] Test incorrect result by required field in Iceberg --- .../trino/plugin/iceberg/TestIcebergV2.java | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java index c240fd7ea3a6..4cc1973bcedc 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java @@ -1754,6 +1754,45 @@ void testAnalyzeNoSnapshot() catalog.dropTable(SESSION, schemaTableName); } + @Test // regression test for https://github.com/trinodb/trino/issues/20511 + void testRequiredField() + { + testRequiredField(true); + testRequiredField(false); + } + + private void testRequiredField(boolean projectionPushdown) + { + Session projectionPushdownEnabled = Session.builder(getSession()) + .setCatalogSessionProperty("iceberg", "projection_pushdown_enabled", Boolean.toString(projectionPushdown)) + .build(); + + String table = "test_required_field" + randomNameSuffix(); + SchemaTableName schemaTableName = new SchemaTableName("tpch", table); + + catalog.newCreateTableTransaction( + SESSION, + schemaTableName, + new Schema( + Types.NestedField.optional(1, "id", Types.IntegerType.get()), + Types.NestedField.optional(2, "struct", Types.StructType.of( + Types.NestedField.required(3, "field", Types.IntegerType.get())))), + PartitionSpec.unpartitioned(), + SortOrder.unsorted(), + Optional.ofNullable(catalog.defaultTableLocation(SESSION, schemaTableName)), + ImmutableMap.of()) + .commitTransaction(); + + assertUpdate("INSERT INTO " + table + " VALUES (1, row(10)), (2, NULL)", 2); + + assertThat(query(projectionPushdownEnabled, "SELECT id FROM " + table + " WHERE struct.field IS NOT NULL")) + .matches("VALUES 1"); + assertThat(query(projectionPushdownEnabled, "SELECT id FROM " + table + " WHERE struct.field IS NULL")) + .matches("VALUES 2"); + + catalog.dropTable(SESSION, schemaTableName); + } + private void testHighlyNestedFieldPartitioningWithTimestampTransform(String partitioning, String partitionDirectoryRegex, Set expectedPartitionDirectories) { String tableName = "test_highly_nested_field_partitioning_with_timestamp_transform_" + randomNameSuffix(); From 1548fc3eec05560f9f172a6dc2dc0ce46ff5fe77 Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Sun, 29 Mar 2026 10:17:07 +0900 Subject: [PATCH 4/9] Use MetricsConfig.forPositionDelete() --- .../io/trino/plugin/iceberg/IcebergFileWriterFactory.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergFileWriterFactory.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergFileWriterFactory.java index 332b3117ecdc..68b23b38685b 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergFileWriterFactory.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergFileWriterFactory.java @@ -87,14 +87,12 @@ import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED; import static java.lang.String.format; import static java.util.Objects.requireNonNull; -import static org.apache.iceberg.TableProperties.DEFAULT_WRITE_METRICS_MODE; import static org.apache.iceberg.io.DeleteSchemaUtil.pathPosSchema; import static org.apache.iceberg.parquet.ParquetSchemaUtil.convert; public class IcebergFileWriterFactory { private static final Schema POSITION_DELETE_SCHEMA = pathPosSchema(); - private static final MetricsConfig FULL_METRICS_CONFIG = MetricsConfig.fromProperties(ImmutableMap.of(DEFAULT_WRITE_METRICS_MODE, "full")); private static final Splitter COLUMN_NAMES_SPLITTER = Splitter.on(',').trimResults().omitEmptyStrings(); private final TypeManager typeManager; @@ -151,8 +149,8 @@ public IcebergFileWriter createPositionDeleteWriter( Map storageProperties) { return switch (fileFormat) { - case PARQUET -> createParquetWriter(FULL_METRICS_CONFIG, fileSystem, outputPath, POSITION_DELETE_SCHEMA, session, storageProperties); - case ORC -> createOrcWriter(FULL_METRICS_CONFIG, fileSystem, outputPath, POSITION_DELETE_SCHEMA, session, storageProperties, DataSize.ofBytes(Integer.MAX_VALUE)); + case PARQUET -> createParquetWriter(MetricsConfig.forPositionDelete(), fileSystem, outputPath, POSITION_DELETE_SCHEMA, session, storageProperties); + case ORC -> createOrcWriter(MetricsConfig.forPositionDelete(), fileSystem, outputPath, POSITION_DELETE_SCHEMA, session, storageProperties, DataSize.ofBytes(Integer.MAX_VALUE)); case AVRO -> createAvroWriter(fileSystem, outputPath, POSITION_DELETE_SCHEMA, storageProperties); }; } From 08583ff07da02c64c74bf2b3c353823b948241f2 Mon Sep 17 00:00:00 2001 From: kaveti Date: Thu, 9 Apr 2026 16:31:37 +0530 Subject: [PATCH 5/9] Support vended credentials from Iceberg REST catalog Made-with: Cursor --- .../sphinx/object-storage/file-system-s3.md | 15 +- .../main/sphinx/object-storage/metastores.md | 7 +- .../filesystem/s3/S3CredentialsMapper.java | 28 + .../filesystem/s3/S3FileSystemFactory.java | 59 +- .../filesystem/s3/S3FileSystemLoader.java | 88 ++- .../filesystem/s3/S3FileSystemModule.java | 2 + .../s3/S3FileSystemSecurityMapper.java | 29 + .../s3/S3SecurityMappingProvider.java | 5 +- .../s3/S3SecurityMappingResult.java | 6 +- ...ileSystemFactoryWithVendedCredentials.java | 581 ++++++++++++++++++ ...FileSystemLoaderWithCredentialsMapper.java | 546 ++++++++++++++++ .../filesystem/s3/S3FileSystemConstants.java | 3 + .../rest/IcebergRestCatalogModule.java | 7 +- .../rest/IcebergVendedCredentialsMapper.java | 89 +++ .../rest/IcebergVendedCredentialsModule.java | 32 + .../rest/TestIcebergS3VendingRestCatalog.java | 345 +++++++++++ .../TestIcebergVendedCredentialsMapper.java | 287 +++++++++ .../S3CredentialVendingCatalogAdapter.java | 54 ++ 18 files changed, 2153 insertions(+), 30 deletions(-) create mode 100644 lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3CredentialsMapper.java create mode 100644 lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemSecurityMapper.java create mode 100644 lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3FileSystemFactoryWithVendedCredentials.java create mode 100644 lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3FileSystemLoaderWithCredentialsMapper.java create mode 100644 plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergVendedCredentialsMapper.java create mode 100644 plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergVendedCredentialsModule.java create mode 100644 plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3VendingRestCatalog.java create mode 100644 plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergVendedCredentialsMapper.java create mode 100644 plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/S3CredentialVendingCatalogAdapter.java diff --git a/docs/src/main/sphinx/object-storage/file-system-s3.md b/docs/src/main/sphinx/object-storage/file-system-s3.md index d53824de822f..2618e3009238 100644 --- a/docs/src/main/sphinx/object-storage/file-system-s3.md +++ b/docs/src/main/sphinx/object-storage/file-system-s3.md @@ -25,11 +25,20 @@ support: - Activate the native implementation for S3 storage support. Defaults to `false`. Set to `true` to use S3 and enable all other properties. * - `s3.endpoint` - - S3 service endpoint URL to communicate with. + - S3 service endpoint URL to communicate with. When using the Iceberg REST catalog + with `iceberg.rest-catalog.vended-credentials-enabled=true`, this property can + be provided by the REST catalog via `s3.endpoint`. Static catalog configuration + takes precedence over vended values. * - `s3.region` - - S3 region to communicate with. + - S3 region to communicate with. When using the Iceberg REST catalog with + `iceberg.rest-catalog.vended-credentials-enabled=true`, this property can be + provided by the REST catalog via `client.region`. Static catalog configuration + takes precedence over vended values. * - `s3.cross-region-access` - - Enable cross region access. Defaults to `false`. + - Enable cross region access. Defaults to `false`. When using the Iceberg REST catalog + with `iceberg.rest-catalog.vended-credentials-enabled=true`, this property can be + provided by the REST catalog via `s3.cross-region-access-enabled`. Static catalog + configuration takes precedence over vended values. * - `s3.path-style-access` - Use path-style access for all requests to S3 * - `s3.storage-class` diff --git a/docs/src/main/sphinx/object-storage/metastores.md b/docs/src/main/sphinx/object-storage/metastores.md index 3d5e98e3a1ba..ce407d6b9624 100644 --- a/docs/src/main/sphinx/object-storage/metastores.md +++ b/docs/src/main/sphinx/object-storage/metastores.md @@ -519,8 +519,11 @@ following properties: - Controls whether to use the token exchange flow to acquire new tokens. Defaults to `true` * - `iceberg.rest-catalog.vended-credentials-enabled` - - Use credentials provided by the REST backend for file system access. - Defaults to `false`. + - Use credentials and configuration provided by the REST backend for file system access. + When enabled, the REST catalog can provide S3 credentials (`s3.access-key-id`, + `s3.secret-access-key`, `s3.session-token`) and configuration properties + (`client.region`, `s3.endpoint`, `s3.cross-region-access-enabled`). Static catalog + configuration takes precedence over vended properties. Defaults to `false`. * - `iceberg.rest-catalog.nested-namespace-enabled` - Support querying objects under nested namespace. Defaults to `false`. diff --git a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3CredentialsMapper.java b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3CredentialsMapper.java new file mode 100644 index 000000000000..805716429926 --- /dev/null +++ b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3CredentialsMapper.java @@ -0,0 +1,28 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.filesystem.s3; + +import io.trino.filesystem.Location; +import io.trino.spi.security.ConnectorIdentity; + +import java.util.Optional; + +public interface S3CredentialsMapper +{ + /** + * Returns S3 credentials and configuration for the given identity and location. + * Empty result indicates cluster defaults should be used. + */ + Optional getMapping(ConnectorIdentity identity, Location location); +} diff --git a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemFactory.java b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemFactory.java index dc9bd68ddfeb..29c8cb0e91f2 100644 --- a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemFactory.java +++ b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemFactory.java @@ -19,9 +19,13 @@ import io.trino.filesystem.TrinoFileSystemFactory; import io.trino.spi.security.ConnectorIdentity; import jakarta.annotation.PreDestroy; +import software.amazon.awssdk.auth.credentials.AwsCredentials; import software.amazon.awssdk.services.s3.S3Client; import software.amazon.awssdk.services.s3.presigner.S3Presigner; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executor; import static io.trino.filesystem.s3.S3FileSystemUtils.createS3PreSigner; @@ -34,6 +38,10 @@ public final class S3FileSystemFactory private final S3Context context; private final Executor uploadExecutor; private final S3Presigner preSigner; + private final String staticRegion; + private final String staticEndpoint; + private final boolean staticCrossRegionAccessEnabled; + private final Map dynamicClients; @Inject public S3FileSystemFactory(OpenTelemetry openTelemetry, S3FileSystemConfig config, S3FileSystemStats stats) @@ -43,19 +51,56 @@ public S3FileSystemFactory(OpenTelemetry openTelemetry, S3FileSystemConfig confi this.preSigner = createS3PreSigner(config, client); this.context = loader.context(); this.uploadExecutor = loader.uploadExecutor(); + this.staticRegion = config.getRegion(); + this.staticEndpoint = config.getEndpoint(); + this.staticCrossRegionAccessEnabled = config.isCrossRegionAccessEnabled(); + this.dynamicClients = new ConcurrentHashMap<>(); } - @PreDestroy - public void destroy() + @Override + public TrinoFileSystem create(ConnectorIdentity identity) { - try (client) { - loader.destroy(); + Optional vendedCredentials = S3FileSystemLoader.extractCredentialsFromIdentity(identity); + String region = S3FileSystemLoader.extractRegionFromIdentity(identity).orElse(staticRegion); + String endpoint = S3FileSystemLoader.extractEndpointFromIdentity(identity).orElse(staticEndpoint); + + boolean crossRegionAccessEnabled; + if (staticCrossRegionAccessEnabled) { + crossRegionAccessEnabled = true; } + else { + crossRegionAccessEnabled = S3FileSystemLoader.extractCrossRegionAccessEnabledFromIdentity(identity).orElse(false); + } + + boolean hasOverrides = vendedCredentials.isPresent() + || (region != null && !region.equals(staticRegion)) + || (endpoint != null && !endpoint.equals(staticEndpoint)) + || crossRegionAccessEnabled != staticCrossRegionAccessEnabled; + + if (hasOverrides) { + ClientKey key = new ClientKey(vendedCredentials.orElse(null), region, endpoint, crossRegionAccessEnabled); + S3Client s3Client = dynamicClients.computeIfAbsent(key, _ -> + loader.createClientWithOverrides(vendedCredentials, region, endpoint, crossRegionAccessEnabled)); + return new S3FileSystem(uploadExecutor, s3Client, preSigner, context.withCredentials(identity)); + } + + return new S3FileSystem(uploadExecutor, client, preSigner, context.withCredentials(identity)); } - @Override - public TrinoFileSystem create(ConnectorIdentity identity) + @PreDestroy + public void destroy() { - return new S3FileSystem(uploadExecutor, client, preSigner, context.withCredentials(identity)); + for (S3Client dynamicClient : dynamicClients.values()) { + try (var _ = dynamicClient) { + // Resource automatically closed + } + } + dynamicClients.clear(); + try (var _ = client) { + // Resource automatically closed + } + loader.destroy(); } + + private record ClientKey(AwsCredentials credentials, String region, String endpoint, boolean crossRegionAccessEnabled) {} } diff --git a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemLoader.java b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemLoader.java index 5c7d253844a6..a3418d083e68 100644 --- a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemLoader.java +++ b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemLoader.java @@ -19,8 +19,11 @@ import io.trino.filesystem.Location; import io.trino.filesystem.TrinoFileSystemFactory; import io.trino.filesystem.s3.S3Context.S3SseContext; +import io.trino.spi.security.ConnectorIdentity; import jakarta.annotation.PreDestroy; +import software.amazon.awssdk.auth.credentials.AwsCredentials; import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; +import software.amazon.awssdk.auth.credentials.AwsSessionCredentials; import software.amazon.awssdk.auth.credentials.WebIdentityTokenFileCredentialsProvider; import software.amazon.awssdk.core.checksums.RequestChecksumCalculation; import software.amazon.awssdk.core.client.config.ClientOverrideConfiguration; @@ -46,6 +49,12 @@ import static com.google.common.base.Preconditions.checkState; import static io.airlift.concurrent.Threads.daemonThreadsNamed; import static io.trino.filesystem.s3.S3FileSystemConfig.RetryMode.getRetryStrategy; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_ENDPOINT_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_REGION_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY; import static io.trino.filesystem.s3.S3FileSystemUtils.createS3PreSigner; import static io.trino.filesystem.s3.S3FileSystemUtils.createStaticCredentialsProvider; import static io.trino.filesystem.s3.S3FileSystemUtils.createStsClient; @@ -58,7 +67,7 @@ final class S3FileSystemLoader implements Function { - private final Optional mappingProvider; + private final Optional mappingProvider; private final SdkHttpClient httpClient; private final S3ClientFactory clientFactory; private final S3FileSystemConfig config; @@ -68,17 +77,7 @@ final class S3FileSystemLoader private final Map, S3Presigner> preSigners = new ConcurrentHashMap<>(); @Inject - public S3FileSystemLoader(S3SecurityMappingProvider mappingProvider, OpenTelemetry openTelemetry, S3FileSystemConfig config, S3FileSystemStats stats) - { - this(Optional.of(mappingProvider), openTelemetry, config, stats); - } - - S3FileSystemLoader(OpenTelemetry openTelemetry, S3FileSystemConfig config, S3FileSystemStats stats) - { - this(Optional.empty(), openTelemetry, config, stats); - } - - private S3FileSystemLoader(Optional mappingProvider, OpenTelemetry openTelemetry, S3FileSystemConfig config, S3FileSystemStats stats) + public S3FileSystemLoader(Optional mappingProvider, OpenTelemetry openTelemetry, S3FileSystemConfig config, S3FileSystemStats stats) { this.mappingProvider = requireNonNull(mappingProvider, "mappingProvider is null"); this.httpClient = createHttpClient(config); @@ -100,6 +99,11 @@ private S3FileSystemLoader(Optional mappingProvider, config.getCannedAcl()); } + S3FileSystemLoader(OpenTelemetry openTelemetry, S3FileSystemConfig config, S3FileSystemStats stats) + { + this(Optional.empty(), openTelemetry, config, stats); + } + @Override public TrinoFileSystemFactory apply(Location location) { @@ -126,6 +130,18 @@ public TrinoFileSystemFactory apply(Location location) @PreDestroy public void destroy() { + for (S3Client client : clients.values()) { + try (var _ = client) { + // Resource automatically closed + } + } + clients.clear(); + for (S3Presigner preSigner : preSigners.values()) { + try (var _ = preSigner) { + // Resource automatically closed + } + } + preSigners.clear(); try (httpClient) { uploadExecutor.shutdownNow(); } @@ -146,6 +162,48 @@ Executor uploadExecutor() return uploadExecutor; } + static Optional extractCredentialsFromIdentity(ConnectorIdentity identity) + { + String accessKey = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY); + String secretKey = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY); + String sessionToken = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY); + if (accessKey != null && secretKey != null && sessionToken != null) { + return Optional.of(AwsSessionCredentials.create(accessKey, secretKey, sessionToken)); + } + return Optional.empty(); + } + + static Optional extractRegionFromIdentity(ConnectorIdentity identity) + { + return Optional.ofNullable(identity.getExtraCredentials().get(EXTRA_CREDENTIALS_REGION_PROPERTY)); + } + + static Optional extractEndpointFromIdentity(ConnectorIdentity identity) + { + return Optional.ofNullable(identity.getExtraCredentials().get(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY)); + } + + static Optional extractCrossRegionAccessEnabledFromIdentity(ConnectorIdentity identity) + { + String value = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY); + return value != null ? Optional.of(Boolean.parseBoolean(value)) : Optional.empty(); + } + + S3Client createClientWithOverrides(Optional credentials, String region, String endpoint, Boolean crossRegionAccessEnabled) + { + S3SecurityMappingResult customMapping = new S3SecurityMappingResult( + credentials, + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.ofNullable(endpoint), + Optional.ofNullable(region), + Optional.ofNullable(crossRegionAccessEnabled)); + + return clientFactory.create(Optional.of(customMapping)); + } + private static S3ClientFactory s3ClientFactory(SdkHttpClient httpClient, OpenTelemetry openTelemetry, S3FileSystemConfig config, MetricPublisher metricPublisher) { ClientOverrideConfiguration overrideConfiguration = createOverrideConfiguration(openTelemetry, config, metricPublisher); @@ -170,9 +228,13 @@ private static S3ClientFactory s3ClientFactory(SdkHttpClient httpClient, OpenTel Optional iamRole = mapping.flatMap(S3SecurityMappingResult::iamRole).or(() -> staticIamRole); String roleSessionName = mapping.flatMap(S3SecurityMappingResult::roleSessionName).orElse(staticRoleSessionName); + boolean crossRegionAccessEnabled = mapping + .flatMap(S3SecurityMappingResult::crossRegionAccessEnabled) + .orElse(config.isCrossRegionAccessEnabled()); + S3ClientBuilder s3 = S3Client.builder(); s3.overrideConfiguration(overrideConfiguration); - s3.crossRegionAccessEnabled(config.isCrossRegionAccessEnabled()); + s3.crossRegionAccessEnabled(crossRegionAccessEnabled); s3.httpClient(httpClient); s3.responseChecksumValidation(WHEN_REQUIRED); s3.requestChecksumCalculation(RequestChecksumCalculation.WHEN_REQUIRED); diff --git a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemModule.java b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemModule.java index a7db2f289634..cc8fb87b7fdf 100644 --- a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemModule.java +++ b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemModule.java @@ -29,6 +29,7 @@ import java.util.function.Supplier; import static com.google.inject.Scopes.SINGLETON; +import static com.google.inject.multibindings.OptionalBinder.newOptionalBinder; import static io.airlift.configuration.ConfigBinder.configBinder; import static io.airlift.http.client.HttpClientBinder.httpClientBinder; import static java.lang.annotation.ElementType.FIELD; @@ -67,6 +68,7 @@ protected void setup(Binder binder) S3SecurityMappingConfig config = buildConfigObject(S3SecurityMappingConfig.class); binder.bind(S3SecurityMappingProvider.class).in(SINGLETON); + newOptionalBinder(binder, S3CredentialsMapper.class).setBinding().to(S3SecurityMappingProvider.class).in(SINGLETON); binder.bind(S3FileSystemLoader.class).in(SINGLETON); var mappingsBinder = binder.bind(new Key>() {}); diff --git a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemSecurityMapper.java b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemSecurityMapper.java new file mode 100644 index 000000000000..2033f325941a --- /dev/null +++ b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemSecurityMapper.java @@ -0,0 +1,29 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.filesystem.s3; + +import io.trino.filesystem.Location; +import io.trino.spi.security.ConnectorIdentity; + +import java.util.Optional; + +public interface S3FileSystemSecurityMapper +{ + /** + * Get security mapping for the given identity and location. + * + * @return Empty optional if cluster defaults should be used, otherwise the security mapping result + */ + Optional getMapping(ConnectorIdentity identity, Location location); +} diff --git a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingProvider.java b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingProvider.java index 008fb835f6bf..bc6d06202706 100644 --- a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingProvider.java +++ b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingProvider.java @@ -29,6 +29,7 @@ import static java.util.concurrent.TimeUnit.MILLISECONDS; final class S3SecurityMappingProvider + implements S3CredentialsMapper { private final Supplier mappingsProvider; private final Optional roleCredentialName; @@ -60,6 +61,7 @@ public S3SecurityMappingProvider( this.colonReplacement = requireNonNull(colonReplacement, "colonReplacement is null"); } + @Override public Optional getMapping(ConnectorIdentity identity, Location location) { S3SecurityMapping mapping = mappingsProvider.get().getMapping(identity, new S3Location(location)) @@ -76,7 +78,8 @@ public Optional getMapping(ConnectorIdentity identity, selectKmsKeyId(mapping, identity), getSseCustomerKey(mapping, identity), mapping.endpoint(), - mapping.region())); + mapping.region(), + Optional.empty())); } private Optional selectRole(S3SecurityMapping mapping, ConnectorIdentity identity) diff --git a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingResult.java b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingResult.java index 2d2ef7dc2e92..aecffc986713 100644 --- a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingResult.java +++ b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingResult.java @@ -21,14 +21,15 @@ import static java.util.Objects.requireNonNull; -record S3SecurityMappingResult( +public record S3SecurityMappingResult( Optional credentials, Optional iamRole, Optional roleSessionName, Optional kmsKeyId, Optional sseCustomerKey, Optional endpoint, - Optional region) + Optional region, + Optional crossRegionAccessEnabled) { public S3SecurityMappingResult { @@ -39,6 +40,7 @@ record S3SecurityMappingResult( requireNonNull(sseCustomerKey, "sseCustomerKey is null"); requireNonNull(endpoint, "endpoint is null"); requireNonNull(region, "region is null"); + requireNonNull(crossRegionAccessEnabled, "crossRegionAccessEnabled is null"); } public Optional credentialsProvider() diff --git a/lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3FileSystemFactoryWithVendedCredentials.java b/lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3FileSystemFactoryWithVendedCredentials.java new file mode 100644 index 000000000000..ce8cafa98459 --- /dev/null +++ b/lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3FileSystemFactoryWithVendedCredentials.java @@ -0,0 +1,581 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.filesystem.s3; + +import com.google.common.collect.ImmutableMap; +import io.opentelemetry.api.OpenTelemetry; +import io.trino.filesystem.TrinoFileSystem; +import io.trino.spi.security.ConnectorIdentity; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import software.amazon.awssdk.services.s3.S3Client; + +import java.lang.reflect.Field; +import java.util.Map; + +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_ENDPOINT_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_REGION_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY; +import static org.assertj.core.api.Assertions.assertThat; + +final class TestS3FileSystemFactoryWithVendedCredentials +{ + private S3FileSystemFactory factory; + + @AfterEach + void tearDown() + { + if (factory != null) { + factory.destroy(); + } + } + + @Test + void testBackwardCompatibility() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setAwsAccessKey("test-access") + .setAwsSecretKey("test-secret") + .setRegion("us-east-1"); + + factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); + + ConnectorIdentity identity = ConnectorIdentity.ofUser("test"); + TrinoFileSystem fs = factory.create(identity); + + assertThat(fs).isNotNull(); + } + + @Test + void testVendedRegionWhenNoStaticRegion() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setAwsAccessKey("test-access") + .setAwsSecretKey("test-secret") + .setRegion("us-east-1"); + + factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-2") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + TrinoFileSystem fs = factory.create(identity); + assertThat(fs).isNotNull(); + } + + @Test + void testStaticRegionTakesPrecedenceOverVended() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setAwsAccessKey("test-access") + .setAwsSecretKey("test-secret") + .setRegion("us-east-1"); + + factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-2") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + TrinoFileSystem fs = factory.create(identity); + assertThat(fs).isNotNull(); + } + + @Test + void testVendedEndpointWhenNoStaticEndpoint() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setAwsAccessKey("test-access") + .setAwsSecretKey("test-secret") + .setRegion("us-east-1"); + + factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://minio.example.com") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + TrinoFileSystem fs = factory.create(identity); + assertThat(fs).isNotNull(); + } + + @Test + void testStaticEndpointTakesPrecedenceOverVended() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setAwsAccessKey("test-access") + .setAwsSecretKey("test-secret") + .setRegion("us-east-1") + .setEndpoint("https://s3.amazonaws.com"); + + factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://minio.example.com") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + TrinoFileSystem fs = factory.create(identity); + assertThat(fs).isNotNull(); + } + + @Test + void testVendedCrossRegionAccessWhenStaticIsDisabled() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setAwsAccessKey("test-access") + .setAwsSecretKey("test-secret") + .setRegion("us-east-1") + .setCrossRegionAccessEnabled(false); + + factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "true") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + TrinoFileSystem fs = factory.create(identity); + assertThat(fs).isNotNull(); + } + + @Test + void testStaticCrossRegionAccessTakesPrecedence() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setAwsAccessKey("test-access") + .setAwsSecretKey("test-secret") + .setRegion("us-east-1") + .setCrossRegionAccessEnabled(true); + + factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "false") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + TrinoFileSystem fs = factory.create(identity); + assertThat(fs).isNotNull(); + } + + @Test + void testAllThreeOverrides() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setAwsAccessKey("test-access") + .setAwsSecretKey("test-secret") + .setRegion("us-east-1"); + + factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "eu-west-1") + .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://minio.example.com") + .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "true") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + TrinoFileSystem fs = factory.create(identity); + assertThat(fs).isNotNull(); + } + + @Test + void testMultipleUsersWithSameVendedRegion() + throws Exception + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setAwsAccessKey("test-access") + .setAwsSecretKey("test-secret") + .setRegion("us-east-1"); + + factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-2") + .buildOrThrow(); + + ConnectorIdentity identity1 = ConnectorIdentity.forUser("user1") + .withExtraCredentials(extraCredentials) + .build(); + + ConnectorIdentity identity2 = ConnectorIdentity.forUser("user2") + .withExtraCredentials(extraCredentials) + .build(); + + TrinoFileSystem fs1 = factory.create(identity1); + TrinoFileSystem fs2 = factory.create(identity2); + + assertThat(fs1).isNotNull(); + assertThat(fs2).isNotNull(); + + S3Client client1 = getClientFromFileSystem(fs1); + S3Client client2 = getClientFromFileSystem(fs2); + + assertThat(client1).isSameAs(client2); + } + + @Test + void testDifferentVendedRegions() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setAwsAccessKey("test-access") + .setAwsSecretKey("test-secret") + .setRegion("us-east-1"); + + factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); + + Map extraCredentials1 = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-1") + .buildOrThrow(); + + Map extraCredentials2 = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-2") + .buildOrThrow(); + + ConnectorIdentity identity1 = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials1) + .build(); + + ConnectorIdentity identity2 = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials2) + .build(); + + TrinoFileSystem fs1 = factory.create(identity1); + TrinoFileSystem fs2 = factory.create(identity2); + + assertThat(fs1).isNotNull(); + assertThat(fs2).isNotNull(); + } + + @Test + void testNoOverridesUsesDefaultClient() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setAwsAccessKey("test-access") + .setAwsSecretKey("test-secret") + .setRegion("us-east-1"); + + factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); + + ConnectorIdentity identity = ConnectorIdentity.ofUser("test"); + TrinoFileSystem fs = factory.create(identity); + + assertThat(fs).isNotNull(); + } + + @Test + void testOnlyCredentialsNoOverridesUsesDefaultClient() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setAwsAccessKey("test-access") + .setAwsSecretKey("test-secret") + .setRegion("us-east-1"); + + factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + TrinoFileSystem fs = factory.create(identity); + assertThat(fs).isNotNull(); + } + + @Test + void testExtractionMethods() + { + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-2") + .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://minio.local") + .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "true") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + assertThat(S3FileSystemLoader.extractRegionFromIdentity(identity)) + .isPresent() + .hasValue("us-west-2"); + + assertThat(S3FileSystemLoader.extractEndpointFromIdentity(identity)) + .isPresent() + .hasValue("https://minio.local"); + + assertThat(S3FileSystemLoader.extractCrossRegionAccessEnabledFromIdentity(identity)) + .isPresent() + .hasValue(true); + } + + @Test + void testExtractionMethodsWithMissingValues() + { + ConnectorIdentity identity = ConnectorIdentity.ofUser("test"); + + assertThat(S3FileSystemLoader.extractRegionFromIdentity(identity)) + .isEmpty(); + + assertThat(S3FileSystemLoader.extractEndpointFromIdentity(identity)) + .isEmpty(); + + assertThat(S3FileSystemLoader.extractCrossRegionAccessEnabledFromIdentity(identity)) + .isEmpty(); + } + + @Test + void testCrossRegionAccessBooleanParsing() + { + Map extraCredentialsTrue = ImmutableMap.of( + EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "true"); + + Map extraCredentialsFalse = ImmutableMap.of( + EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "false"); + + ConnectorIdentity identityTrue = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentialsTrue) + .build(); + + ConnectorIdentity identityFalse = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentialsFalse) + .build(); + + assertThat(S3FileSystemLoader.extractCrossRegionAccessEnabledFromIdentity(identityTrue)) + .isPresent() + .hasValue(true); + + assertThat(S3FileSystemLoader.extractCrossRegionAccessEnabledFromIdentity(identityFalse)) + .isPresent() + .hasValue(false); + } + + @Test + void testVendedCrossRegionAccessWhenStaticIsFalse() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setAwsAccessKey("test-access") + .setAwsSecretKey("test-secret") + .setRegion("us-east-1") + .setCrossRegionAccessEnabled(false); + + factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "false") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + TrinoFileSystem fs = factory.create(identity); + assertThat(fs).isNotNull(); + } + + @Test + void testVendedCrossRegionAccessFalseWhenStaticIsFalse() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setAwsAccessKey("test-access") + .setAwsSecretKey("test-secret") + .setRegion("us-east-1") + .setCrossRegionAccessEnabled(false); + + factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "true") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + TrinoFileSystem fs = factory.create(identity); + assertThat(fs).isNotNull(); + } + + @Test + void testClientCachingWithSameConfiguration() + throws Exception + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setAwsAccessKey("test-access") + .setAwsSecretKey("test-secret") + .setRegion("us-east-1"); + + factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-2") + .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://minio.example.com") + .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "true") + .buildOrThrow(); + + ConnectorIdentity identity1 = ConnectorIdentity.forUser("user1") + .withExtraCredentials(extraCredentials) + .build(); + + ConnectorIdentity identity2 = ConnectorIdentity.forUser("user2") + .withExtraCredentials(extraCredentials) + .build(); + + TrinoFileSystem fs1 = factory.create(identity1); + TrinoFileSystem fs2 = factory.create(identity2); + + assertThat(fs1).isNotNull(); + assertThat(fs2).isNotNull(); + + S3Client client1 = getClientFromFileSystem(fs1); + S3Client client2 = getClientFromFileSystem(fs2); + + assertThat(client1).isSameAs(client2); + } + + private static S3Client getClientFromFileSystem(TrinoFileSystem fileSystem) + throws Exception + { + assertThat(fileSystem).isInstanceOf(S3FileSystem.class); + Field clientField = S3FileSystem.class.getDeclaredField("client"); + clientField.setAccessible(true); + return (S3Client) clientField.get(fileSystem); + } + + @Test + void testVendedEndpointWhenStaticEndpointIsNull() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setAwsAccessKey("test-access") + .setAwsSecretKey("test-secret") + .setRegion("us-east-1"); + + factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-2") + .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://minio.example.com") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + TrinoFileSystem fs = factory.create(identity); + assertThat(fs).isNotNull(); + } + + @Test + void testCombinationRegionEndpointAndCrossRegionAccess() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setAwsAccessKey("test-access") + .setAwsSecretKey("test-secret") + .setRegion("us-east-1") + .setCrossRegionAccessEnabled(false); + + factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "eu-central-1") + .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://custom-s3.example.com") + .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "true") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + TrinoFileSystem fs = factory.create(identity); + assertThat(fs).isNotNull(); + } +} diff --git a/lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3FileSystemLoaderWithCredentialsMapper.java b/lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3FileSystemLoaderWithCredentialsMapper.java new file mode 100644 index 000000000000..2c988871ad55 --- /dev/null +++ b/lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3FileSystemLoaderWithCredentialsMapper.java @@ -0,0 +1,546 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.filesystem.s3; + +import io.opentelemetry.api.OpenTelemetry; +import io.trino.filesystem.Location; +import io.trino.filesystem.TrinoFileSystem; +import io.trino.filesystem.TrinoFileSystemFactory; +import io.trino.spi.security.ConnectorIdentity; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import software.amazon.awssdk.auth.credentials.AwsSessionCredentials; +import software.amazon.awssdk.services.s3.S3Client; + +import java.lang.reflect.Field; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.assertj.core.api.Assertions.assertThat; + +final class TestS3FileSystemLoaderWithCredentialsMapper +{ + private S3FileSystemLoader loader; + private S3FileSystemConfig config; + private S3FileSystemStats stats; + private TestCredentialsMapper credentialsMapper; + + @BeforeEach + void setUp() + throws Exception + { + config = new S3FileSystemConfig() + .setAwsAccessKey("test-access-key") + .setAwsSecretKey("test-secret-key") + .setRegion("us-east-1") + .setCrossRegionAccessEnabled(false); + + stats = new S3FileSystemStats(); + credentialsMapper = new TestCredentialsMapper(); + + // Use the public constructor directly + loader = new S3FileSystemLoader( + Optional.of(credentialsMapper), + OpenTelemetry.noop(), + config, + stats); + } + + @AfterEach + void tearDown() + { + if (loader != null) { + loader.destroy(); + } + } + + @Test + void testClientCachingForSameCredentials() + throws Exception + { + // Arrange: Same credentials + ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); + Location location = Location.of("s3://test-bucket/path"); + + S3SecurityMappingResult mapping = new S3SecurityMappingResult( + Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.of("https://s3.us-east-1.amazonaws.com"), + Optional.of("us-east-1"), + Optional.of(true)); + + credentialsMapper.setMapping(mapping); + + // Act: Create file systems multiple times with same credentials + TrinoFileSystemFactory factory = loader.apply(location); + TrinoFileSystem fs1 = factory.create(identity); + TrinoFileSystem fs2 = factory.create(identity); + TrinoFileSystem fs3 = factory.create(identity); + + // Assert: Same S3Client is reused + S3Client client1 = extractS3Client(fs1); + S3Client client2 = extractS3Client(fs2); + S3Client client3 = extractS3Client(fs3); + + assertThat(client1).isSameAs(client2); + assertThat(client2).isSameAs(client3); + + // Verify cache contains only ONE entry + Map, S3Client> clientsCache = getClientsCache(); + assertThat(clientsCache).hasSize(1); + } + + @Test + void testDifferentClientsForDifferentCredentials() + throws Exception + { + // Arrange: Different credentials + ConnectorIdentity identity1 = ConnectorIdentity.ofUser("user1"); + ConnectorIdentity identity2 = ConnectorIdentity.ofUser("user2"); + Location location = Location.of("s3://test-bucket/path"); + + S3SecurityMappingResult mapping1 = new S3SecurityMappingResult( + Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.of("https://s3.us-east-1.amazonaws.com"), + Optional.of("us-east-1"), + Optional.of(true)); + + S3SecurityMappingResult mapping2 = new S3SecurityMappingResult( + Optional.of(AwsSessionCredentials.create("access2", "secret2", "token2")), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.of("https://s3.us-west-2.amazonaws.com"), + Optional.of("us-west-2"), + Optional.of(false)); + + TrinoFileSystemFactory factory = loader.apply(location); + + // Act: Create file systems with different credentials + credentialsMapper.setMapping(mapping1); + TrinoFileSystem fs1 = factory.create(identity1); + + credentialsMapper.setMapping(mapping2); + TrinoFileSystem fs2 = factory.create(identity2); + + // Assert: Different S3Clients are created + S3Client client1 = extractS3Client(fs1); + S3Client client2 = extractS3Client(fs2); + + assertThat(client1).isNotSameAs(client2); + + // Verify cache contains TWO entries + Map, S3Client> clientsCache = getClientsCache(); + assertThat(clientsCache).hasSize(2); + } + + @Test + void testDifferentClientsForDifferentRegions() + throws Exception + { + // Arrange: Same credentials, different regions + ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); + Location location = Location.of("s3://test-bucket/path"); + + S3SecurityMappingResult mapping1 = new S3SecurityMappingResult( + Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.of("us-east-1"), + Optional.empty()); + + S3SecurityMappingResult mapping2 = new S3SecurityMappingResult( + Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.of("eu-west-1"), + Optional.empty()); + + TrinoFileSystemFactory factory = loader.apply(location); + + // Act: Create file systems with different regions + credentialsMapper.setMapping(mapping1); + TrinoFileSystem fs1 = factory.create(identity); + + credentialsMapper.setMapping(mapping2); + TrinoFileSystem fs2 = factory.create(identity); + + // Assert: Different S3Clients for different regions + S3Client client1 = extractS3Client(fs1); + S3Client client2 = extractS3Client(fs2); + + assertThat(client1).isNotSameAs(client2); + + // Verify cache contains TWO entries + Map, S3Client> clientsCache = getClientsCache(); + assertThat(clientsCache).hasSize(2); + } + + @Test + void testDifferentClientsForDifferentEndpoints() + throws Exception + { + // Arrange: Same credentials, different endpoints + ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); + Location location = Location.of("s3://test-bucket/path"); + + S3SecurityMappingResult mapping1 = new S3SecurityMappingResult( + Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.of("https://s3.us-east-1.amazonaws.com"), + Optional.of("us-east-1"), + Optional.empty()); + + S3SecurityMappingResult mapping2 = new S3SecurityMappingResult( + Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.of("https://custom-s3-endpoint.example.com"), + Optional.of("us-east-1"), + Optional.empty()); + + TrinoFileSystemFactory factory = loader.apply(location); + + // Act: Create file systems with different endpoints + credentialsMapper.setMapping(mapping1); + TrinoFileSystem fs1 = factory.create(identity); + + credentialsMapper.setMapping(mapping2); + TrinoFileSystem fs2 = factory.create(identity); + + // Assert: Different S3Clients for different endpoints + S3Client client1 = extractS3Client(fs1); + S3Client client2 = extractS3Client(fs2); + + assertThat(client1).isNotSameAs(client2); + + // Verify cache contains TWO entries + Map, S3Client> clientsCache = getClientsCache(); + assertThat(clientsCache).hasSize(2); + } + + @Test + void testDifferentClientsForDifferentCrossRegionAccess() + throws Exception + { + // Arrange: Same credentials, different cross-region settings + ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); + Location location = Location.of("s3://test-bucket/path"); + + S3SecurityMappingResult mapping1 = new S3SecurityMappingResult( + Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.of("us-east-1"), + Optional.of(true)); + + S3SecurityMappingResult mapping2 = new S3SecurityMappingResult( + Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.of("us-east-1"), + Optional.of(false)); + + TrinoFileSystemFactory factory = loader.apply(location); + + // Act: Create file systems with different cross-region settings + credentialsMapper.setMapping(mapping1); + TrinoFileSystem fs1 = factory.create(identity); + + credentialsMapper.setMapping(mapping2); + TrinoFileSystem fs2 = factory.create(identity); + + // Assert: Different S3Clients for different cross-region settings + S3Client client1 = extractS3Client(fs1); + S3Client client2 = extractS3Client(fs2); + + assertThat(client1).isNotSameAs(client2); + + // Verify cache contains TWO entries + Map, S3Client> clientsCache = getClientsCache(); + assertThat(clientsCache).hasSize(2); + } + + @Test + void testEmptyMappingUsesClusterDefaults() + throws Exception + { + // Arrange: No credentials mapping + ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); + Location location = Location.of("s3://test-bucket/path"); + + credentialsMapper.setMapping(null); // Return empty + + // Act: Create file system + TrinoFileSystemFactory factory = loader.apply(location); + TrinoFileSystem fs = factory.create(identity); + + // Assert: Client is created with cluster defaults + S3Client client = extractS3Client(fs); + assertThat(client).isNotNull(); + + // Verify cache contains ONE entry for empty mapping + Map, S3Client> clientsCache = getClientsCache(); + assertThat(clientsCache).hasSize(1); + } + + @Test + void testConcurrentClientCreationIsSafe() + throws Exception + { + // Arrange: Multiple threads creating clients with same credentials + ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); + Location location = Location.of("s3://test-bucket/path"); + + S3SecurityMappingResult mapping = new S3SecurityMappingResult( + Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.of("https://s3.us-east-1.amazonaws.com"), + Optional.of("us-east-1"), + Optional.of(true)); + + credentialsMapper.setMapping(mapping); + + int threadCount = 10; + CountDownLatch startLatch = new CountDownLatch(1); + CountDownLatch doneLatch = new CountDownLatch(threadCount); + ExecutorService executor = Executors.newFixedThreadPool(threadCount); + AtomicInteger clientCount = new AtomicInteger(0); + + TrinoFileSystemFactory factory = loader.apply(location); + + // Act: Create clients concurrently + for (int i = 0; i < threadCount; i++) { + executor.submit(() -> { + try { + startLatch.await(); // Wait for all threads to be ready + TrinoFileSystem fs = factory.create(identity); + S3Client client = extractS3Client(fs); + if (client != null) { + clientCount.incrementAndGet(); + } + } + catch (Exception e) { + throw new RuntimeException(e); + } + finally { + doneLatch.countDown(); + } + }); + } + + startLatch.countDown(); // Start all threads at once + doneLatch.await(10, TimeUnit.SECONDS); + executor.shutdown(); + + // Assert: All threads succeeded + assertThat(clientCount.get()).isEqualTo(threadCount); + + // Assert: Only ONE client created despite concurrent access + Map, S3Client> clientsCache = getClientsCache(); + assertThat(clientsCache).hasSize(1); + } + + @Test + void testClientCacheGrowsBounded() + throws Exception + { + // Arrange: Create clients with many different credential combinations + Location location = Location.of("s3://test-bucket/path"); + TrinoFileSystemFactory factory = loader.apply(location); + + int uniqueCredentialSets = 5; + + // Act: Create file systems with different credentials + for (int i = 0; i < uniqueCredentialSets; i++) { + ConnectorIdentity identity = ConnectorIdentity.ofUser("user" + i); + + S3SecurityMappingResult mapping = new S3SecurityMappingResult( + Optional.of(AwsSessionCredentials.create("access" + i, "secret" + i, "token" + i)), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.of("https://s3.us-east-1.amazonaws.com"), + Optional.of("us-east-1"), + Optional.of(true)); + + credentialsMapper.setMapping(mapping); + factory.create(identity); + } + + // Assert: Cache size equals number of unique credential sets (bounded growth) + Map, S3Client> clientsCache = getClientsCache(); + assertThat(clientsCache).hasSize(uniqueCredentialSets); + } + + @Test + void testKmsKeyIdIsAppliedToContext() + throws Exception + { + // Arrange: Mapping with KMS key ID + ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); + Location location = Location.of("s3://test-bucket/path"); + + S3SecurityMappingResult mapping = new S3SecurityMappingResult( + Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), + Optional.empty(), + Optional.empty(), + Optional.of("arn:aws:kms:us-east-1:123456789012:key/test-key"), + Optional.empty(), + Optional.empty(), + Optional.of("us-east-1"), + Optional.empty()); + + credentialsMapper.setMapping(mapping); + + // Act: Create file system + TrinoFileSystemFactory factory = loader.apply(location); + TrinoFileSystem fs = factory.create(identity); + + // Assert: File system created successfully (KMS key would be validated at runtime) + assertThat(fs).isNotNull(); + } + + @Test + void testSseCustomerKeyIsAppliedToContext() + throws Exception + { + // Arrange: Mapping with SSE customer key + ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); + Location location = Location.of("s3://test-bucket/path"); + + S3SecurityMappingResult mapping = new S3SecurityMappingResult( + Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.of("YmFzZTY0LWVuY29kZWQta2V5"), // base64 encoded key + Optional.empty(), + Optional.of("us-east-1"), + Optional.empty()); + + credentialsMapper.setMapping(mapping); + + // Act: Create file system + TrinoFileSystemFactory factory = loader.apply(location); + TrinoFileSystem fs = factory.create(identity); + + // Assert: File system created successfully + assertThat(fs).isNotNull(); + } + + @Test + void testComplexCredentialCombination() + throws Exception + { + // Arrange: Mapping with multiple fields populated + ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); + Location location = Location.of("s3://test-bucket/path"); + + S3SecurityMappingResult mapping = new S3SecurityMappingResult( + Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), + Optional.of("arn:aws:iam::123456789012:role/test-role"), + Optional.of("test-session"), + Optional.empty(), + Optional.empty(), + Optional.of("https://custom-endpoint.example.com"), + Optional.of("eu-west-1"), + Optional.of(true)); + + credentialsMapper.setMapping(mapping); + + // Act: Create file system + TrinoFileSystemFactory factory = loader.apply(location); + TrinoFileSystem fs = factory.create(identity); + + // Assert: File system created successfully + assertThat(fs).isNotNull(); + + S3Client client = extractS3Client(fs); + assertThat(client).isNotNull(); + } + + // Helper methods + + private S3Client extractS3Client(TrinoFileSystem fileSystem) + throws Exception + { + Field clientField = fileSystem.getClass().getDeclaredField("client"); + clientField.setAccessible(true); + return (S3Client) clientField.get(fileSystem); + } + + @SuppressWarnings("unchecked") + private Map, S3Client> getClientsCache() + throws Exception + { + Field clientsField = loader.getClass().getDeclaredField("clients"); + clientsField.setAccessible(true); + return (ConcurrentHashMap, S3Client>) clientsField.get(loader); + } + + // Test implementation of S3CredentialsMapper + private static class TestCredentialsMapper + implements S3CredentialsMapper + { + private S3SecurityMappingResult mapping; + + public void setMapping(S3SecurityMappingResult mapping) + { + this.mapping = mapping; + } + + @Override + public Optional getMapping(ConnectorIdentity identity, Location location) + { + return Optional.ofNullable(mapping); + } + } +} diff --git a/lib/trino-filesystem/src/main/java/io/trino/filesystem/s3/S3FileSystemConstants.java b/lib/trino-filesystem/src/main/java/io/trino/filesystem/s3/S3FileSystemConstants.java index 1155026292a6..e0e52c1186f7 100644 --- a/lib/trino-filesystem/src/main/java/io/trino/filesystem/s3/S3FileSystemConstants.java +++ b/lib/trino-filesystem/src/main/java/io/trino/filesystem/s3/S3FileSystemConstants.java @@ -18,6 +18,9 @@ public final class S3FileSystemConstants public static final String EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY = "internal$s3_aws_access_key"; public static final String EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY = "internal$s3_aws_secret_key"; public static final String EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY = "internal$s3_aws_session_token"; + public static final String EXTRA_CREDENTIALS_REGION_PROPERTY = "internal$s3_region"; + public static final String EXTRA_CREDENTIALS_ENDPOINT_PROPERTY = "internal$s3_endpoint"; + public static final String EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY = "internal$s3_cross_region_access_enabled"; private S3FileSystemConstants() {} } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogModule.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogModule.java index d6709e718739..8178a7c23ea9 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogModule.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogModule.java @@ -44,8 +44,11 @@ protected void setup(Binder binder) IcebergConfig icebergConfig = buildConfigObject(IcebergConfig.class); IcebergRestCatalogConfig restCatalogConfig = buildConfigObject(IcebergRestCatalogConfig.class); - if (restCatalogConfig.isVendedCredentialsEnabled() && icebergConfig.isRegisterTableProcedureEnabled()) { - throw new TrinoException(NOT_SUPPORTED, "Using the `register_table` procedure with vended credentials is currently not supported"); + if (restCatalogConfig.isVendedCredentialsEnabled()) { + if (icebergConfig.isRegisterTableProcedureEnabled()) { + throw new TrinoException(NOT_SUPPORTED, "Using the `register_table` procedure with vended credentials is currently not supported"); + } + install(new IcebergVendedCredentialsModule()); } } } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergVendedCredentialsMapper.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergVendedCredentialsMapper.java new file mode 100644 index 000000000000..965e7b3b8646 --- /dev/null +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergVendedCredentialsMapper.java @@ -0,0 +1,89 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.iceberg.catalog.rest; + +import com.google.inject.Inject; +import io.trino.filesystem.Location; +import io.trino.filesystem.s3.S3CredentialsMapper; +import io.trino.filesystem.s3.S3FileSystemConfig; +import io.trino.filesystem.s3.S3SecurityMappingResult; +import io.trino.spi.security.ConnectorIdentity; +import software.amazon.awssdk.auth.credentials.AwsSessionCredentials; + +import java.util.Optional; + +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_ENDPOINT_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_REGION_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY; +import static java.util.Objects.requireNonNull; + +final class IcebergVendedCredentialsMapper + implements S3CredentialsMapper +{ + private final Optional staticRegion; + private final Optional staticEndpoint; + private final boolean staticCrossRegionAccessEnabled; + + @Inject + public IcebergVendedCredentialsMapper(S3FileSystemConfig config) + { + requireNonNull(config, "config is null"); + this.staticRegion = Optional.ofNullable(config.getRegion()); + this.staticEndpoint = Optional.ofNullable(config.getEndpoint()); + this.staticCrossRegionAccessEnabled = config.isCrossRegionAccessEnabled(); + } + + @Override + public Optional getMapping(ConnectorIdentity identity, Location location) + { + String accessKey = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY); + String secretKey = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY); + String sessionToken = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY); + + if (accessKey == null || secretKey == null || sessionToken == null) { + return Optional.empty(); + } + + String vendedRegion = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_REGION_PROPERTY); + String vendedEndpoint = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY); + String vendedCrossRegionAccess = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY); + + Optional region = Optional.ofNullable(vendedRegion).or(() -> staticRegion); + Optional endpoint = Optional.ofNullable(vendedEndpoint).or(() -> staticEndpoint); + + Optional crossRegionAccessEnabled; + if (staticCrossRegionAccessEnabled) { + crossRegionAccessEnabled = Optional.of(true); + } + else if (vendedCrossRegionAccess != null) { + crossRegionAccessEnabled = Optional.of(Boolean.parseBoolean(vendedCrossRegionAccess)); + } + else { + crossRegionAccessEnabled = Optional.empty(); + } + + return Optional.of(new S3SecurityMappingResult( + Optional.of(AwsSessionCredentials.create(accessKey, secretKey, sessionToken)), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + endpoint, + region, + crossRegionAccessEnabled)); + } +} diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergVendedCredentialsModule.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergVendedCredentialsModule.java new file mode 100644 index 000000000000..9c7e1396803a --- /dev/null +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergVendedCredentialsModule.java @@ -0,0 +1,32 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.iceberg.catalog.rest; + +import com.google.inject.Binder; +import com.google.inject.Scopes; +import io.airlift.configuration.AbstractConfigurationAwareModule; +import io.trino.filesystem.s3.S3CredentialsMapper; + +import static com.google.inject.multibindings.OptionalBinder.newOptionalBinder; + +public class IcebergVendedCredentialsModule + extends AbstractConfigurationAwareModule +{ + @Override + protected void setup(Binder binder) + { + binder.bind(IcebergVendedCredentialsMapper.class).in(Scopes.SINGLETON); + newOptionalBinder(binder, S3CredentialsMapper.class).setBinding().to(IcebergVendedCredentialsMapper.class).in(Scopes.SINGLETON); + } +} diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3VendingRestCatalog.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3VendingRestCatalog.java new file mode 100644 index 000000000000..3d84c68073aa --- /dev/null +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3VendingRestCatalog.java @@ -0,0 +1,345 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.iceberg.catalog.rest; + +import com.google.common.collect.ImmutableMap; +import io.airlift.http.server.HttpServerConfig; +import io.airlift.http.server.HttpServerInfo; +import io.airlift.http.server.ServerFeature; +import io.airlift.http.server.testing.TestingHttpServer; +import io.airlift.node.NodeInfo; +import io.trino.plugin.iceberg.IcebergQueryRunner; +import io.trino.testing.AbstractTestQueryFramework; +import io.trino.testing.QueryRunner; +import io.trino.testing.containers.Minio; +import org.apache.iceberg.CatalogProperties; +import org.apache.iceberg.jdbc.JdbcCatalog; +import org.apache.iceberg.rest.RestCatalogServlet; +import org.apache.iceberg.rest.S3CredentialVendingCatalogAdapter; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Map; + +import static io.trino.testing.TestingNames.randomNameSuffix; +import static io.trino.testing.containers.Minio.MINIO_REGION; +import static io.trino.testing.containers.Minio.MINIO_ROOT_PASSWORD; +import static io.trino.testing.containers.Minio.MINIO_ROOT_USER; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS; + +@TestInstance(PER_CLASS) +class TestIcebergS3VendingRestCatalog + extends AbstractTestQueryFramework +{ + private static final String BUCKET_NAME = "test-iceberg-s3-vending-" + randomNameSuffix(); + + private Minio minio; + private JdbcCatalog backendCatalog; + private TestingHttpServer restServer; + + @SuppressWarnings("resource") + @Override + protected QueryRunner createQueryRunner() + throws Exception + { + minio = Minio.builder().build(); + minio.start(); + minio.createBucket(BUCKET_NAME); + + String minioAddress = minio.getMinioAddress(); + + backendCatalog = createBackendCatalog(minioAddress); + + // S3 properties that the REST catalog will vend to Trino + Map vendedS3Properties = ImmutableMap.builder() + .put("s3.access-key-id", MINIO_ROOT_USER) + .put("s3.secret-access-key", MINIO_ROOT_PASSWORD) + .put("s3.session-token", "test-session-token") + .put("client.region", MINIO_REGION) + .put("s3.endpoint", minioAddress) + .put("s3.cross-region-access-enabled", "true") + .buildOrThrow(); + + S3CredentialVendingCatalogAdapter adapter = new S3CredentialVendingCatalogAdapter(backendCatalog, vendedS3Properties); + RestCatalogServlet servlet = new RestCatalogServlet(adapter); + + NodeInfo nodeInfo = new NodeInfo("test"); + HttpServerConfig httpConfig = new HttpServerConfig() + .setHttpPort(0) + .setHttpEnabled(true); + HttpServerInfo httpServerInfo = new HttpServerInfo(httpConfig, nodeInfo); + restServer = new TestingHttpServer("s3-vending-rest-catalog", httpServerInfo, nodeInfo, httpConfig, servlet, ServerFeature.builder() + .withLegacyUriCompliance(true) + .build()); + restServer.start(); + + return IcebergQueryRunner.builder() + .setIcebergProperties(ImmutableMap.builder() + .put("iceberg.catalog.type", "rest") + .put("iceberg.rest-catalog.uri", restServer.getBaseUrl().toString()) + .put("iceberg.rest-catalog.vended-credentials-enabled", "true") + .put("fs.native-s3.enabled", "true") + .put("s3.region", MINIO_REGION) + .put("s3.endpoint", minioAddress) + .put("s3.path-style-access", "true") + .buildOrThrow()) + .build(); + } + + @AfterAll + public void tearDown() + { + if (restServer != null) { + try { + restServer.stop(); + } + catch (Exception e) { + // ignore + } + } + if (backendCatalog != null) { + try { + backendCatalog.close(); + } + catch (Exception e) { + // ignore + } + } + if (minio != null) { + minio.close(); + } + } + + @Test + void testCreateTableInsertAndSelect() + { + String tableName = "test_s3_vending_" + randomNameSuffix(); + assertUpdate("CREATE TABLE " + tableName + " (id INTEGER, name VARCHAR)"); + assertUpdate("INSERT INTO " + tableName + " VALUES (1, 'alice'), (2, 'bob')", 2); + assertQuery("SELECT * FROM " + tableName, "VALUES (1, 'alice'), (2, 'bob')"); + assertUpdate("DROP TABLE " + tableName); + } + + @Test + void testCreateTableAsSelect() + { + String tableName = "test_s3_ctas_" + randomNameSuffix(); + assertUpdate("CREATE TABLE " + tableName + " AS SELECT 1 AS id, VARCHAR 'hello' AS greeting", 1); + assertQuery("SELECT * FROM " + tableName, "VALUES (1, 'hello')"); + assertUpdate("DROP TABLE " + tableName); + } + + @Test + void testTableLocationIsOnS3() + { + String tableName = "test_s3_location_" + randomNameSuffix(); + assertUpdate("CREATE TABLE " + tableName + " AS SELECT 1 AS id", 1); + + String filePath = (String) computeScalar("SELECT file_path FROM \"" + tableName + "$files\" LIMIT 1"); + assertThat(filePath).startsWith("s3://" + BUCKET_NAME + "/"); + assertUpdate("DROP TABLE " + tableName); + } + + @Test + void testSchemaOperations() + { + String schemaName = "test_s3_schema_" + randomNameSuffix(); + assertUpdate("CREATE SCHEMA " + schemaName); + assertThat(computeActual("SHOW SCHEMAS").getOnlyColumnAsSet()).contains(schemaName); + + String tableName = schemaName + ".test_table_" + randomNameSuffix(); + assertUpdate("CREATE TABLE " + tableName + " (id INTEGER)"); + assertUpdate("INSERT INTO " + tableName + " VALUES (1)", 1); + assertQuery("SELECT * FROM " + tableName, "VALUES 1"); + + assertUpdate("DROP TABLE " + tableName); + assertUpdate("DROP SCHEMA " + schemaName); + } + + @Test + void testUpdateAndDelete() + { + String tableName = "test_s3_update_delete_" + randomNameSuffix(); + assertUpdate("CREATE TABLE " + tableName + " (id INTEGER, value VARCHAR)"); + assertUpdate("INSERT INTO " + tableName + " VALUES (1, 'original'), (2, 'keep'), (3, 'remove')", 3); + + assertUpdate("UPDATE " + tableName + " SET value = 'updated' WHERE id = 1", 1); + assertQuery("SELECT value FROM " + tableName + " WHERE id = 1", "VALUES 'updated'"); + + assertUpdate("DELETE FROM " + tableName + " WHERE id = 3", 1); + assertQuery("SELECT * FROM " + tableName + " ORDER BY id", "VALUES (1, 'updated'), (2, 'keep')"); + + assertUpdate("DROP TABLE " + tableName); + } + + @Test + void testPartitionedTable() + { + String tableName = "test_s3_partitioned_" + randomNameSuffix(); + assertUpdate("CREATE TABLE " + tableName + " (id INTEGER, region VARCHAR) WITH (partitioning = ARRAY['region'])"); + assertUpdate("INSERT INTO " + tableName + " VALUES (1, 'us'), (2, 'eu'), (3, 'us')", 3); + + assertQuery("SELECT id FROM " + tableName + " WHERE region = 'us' ORDER BY id", "VALUES 1, 3"); + assertQuery("SELECT id FROM " + tableName + " WHERE region = 'eu'", "VALUES 2"); + + long partitionCount = (long) computeScalar("SELECT count(*) FROM \"" + tableName + "$partitions\""); + assertThat(partitionCount).isEqualTo(2L); + + assertUpdate("DROP TABLE " + tableName); + } + + @Test + void testSnapshotsAndTimeTravel() + { + String tableName = "test_s3_snapshots_" + randomNameSuffix(); + assertUpdate("CREATE TABLE " + tableName + " (id INTEGER)"); + + assertUpdate("INSERT INTO " + tableName + " VALUES (1)", 1); + long snapshotAfterFirstInsert = (long) computeScalar("SELECT snapshot_id FROM \"" + tableName + "$snapshots\" ORDER BY committed_at DESC LIMIT 1"); + + assertUpdate("INSERT INTO " + tableName + " VALUES (2)", 1); + assertQuery("SELECT * FROM " + tableName + " ORDER BY id", "VALUES 1, 2"); + + // Time travel to first snapshot + assertQuery("SELECT * FROM " + tableName + " FOR VERSION AS OF " + snapshotAfterFirstInsert, "VALUES 1"); + + assertUpdate("DROP TABLE " + tableName); + } + + @Test + void testMetadataTables() + { + String tableName = "test_s3_metadata_" + randomNameSuffix(); + assertUpdate("CREATE TABLE " + tableName + " (id INTEGER, part VARCHAR) WITH (partitioning = ARRAY['part'])"); + assertUpdate("INSERT INTO " + tableName + " VALUES (1, 'a')", 1); + assertUpdate("INSERT INTO " + tableName + " VALUES (2, 'b')", 1); + + long snapshotCount = (long) computeScalar("SELECT count(*) FROM \"" + tableName + "$snapshots\""); + assertThat(snapshotCount).isGreaterThanOrEqualTo(2L); + + long fileCount = (long) computeScalar("SELECT count(*) FROM \"" + tableName + "$files\""); + assertThat(fileCount).isEqualTo(2L); + + long historyCount = (long) computeScalar("SELECT count(*) FROM \"" + tableName + "$history\""); + assertThat(historyCount).isGreaterThanOrEqualTo(2L); + + long partitionCount = (long) computeScalar("SELECT count(*) FROM \"" + tableName + "$partitions\""); + assertThat(partitionCount).isEqualTo(2L); + + // Verify file paths are on S3 + String filePath = (String) computeScalar("SELECT file_path FROM \"" + tableName + "$files\" LIMIT 1"); + assertThat(filePath).startsWith("s3://" + BUCKET_NAME + "/"); + + assertUpdate("DROP TABLE " + tableName); + } + + @Test + void testMultipleDataTypes() + { + String tableName = "test_s3_types_" + randomNameSuffix(); + assertUpdate("CREATE TABLE " + tableName + " (" + + "c_integer INTEGER, " + + "c_bigint BIGINT, " + + "c_real REAL, " + + "c_double DOUBLE, " + + "c_decimal DECIMAL(10,2), " + + "c_varchar VARCHAR, " + + "c_boolean BOOLEAN, " + + "c_date DATE, " + + "c_timestamp TIMESTAMP(6))"); + + assertUpdate("INSERT INTO " + tableName + " VALUES (" + + "42, " + + "BIGINT '9999999999', " + + "REAL '3.14', " + + "DOUBLE '2.718281828', " + + "DECIMAL '12345.67', " + + "VARCHAR 'hello world', " + + "true, " + + "DATE '2024-01-15', " + + "TIMESTAMP '2024-01-15 10:30:00.123456')", 1); + + assertQuery("SELECT c_integer, c_bigint, c_varchar, c_boolean FROM " + tableName, + "VALUES (42, 9999999999, 'hello world', true)"); + + assertUpdate("DROP TABLE " + tableName); + } + + @Test + void testCreateOrReplaceTable() + { + String tableName = "test_s3_create_or_replace_" + randomNameSuffix(); + assertUpdate("CREATE TABLE " + tableName + " AS SELECT 1 AS id, VARCHAR 'v1' AS version", 1); + assertQuery("SELECT * FROM " + tableName, "VALUES (1, 'v1')"); + + long v1SnapshotId = (long) computeScalar("SELECT snapshot_id FROM \"" + tableName + "$snapshots\" ORDER BY committed_at DESC LIMIT 1"); + + assertUpdate("CREATE OR REPLACE TABLE " + tableName + " AS SELECT 2 AS id, VARCHAR 'v2' AS version", 1); + assertQuery("SELECT * FROM " + tableName, "VALUES (2, 'v2')"); + + // Time travel to original version + assertQuery("SELECT * FROM " + tableName + " FOR VERSION AS OF " + v1SnapshotId, "VALUES (1, 'v1')"); + + assertUpdate("DROP TABLE " + tableName); + } + + @Test + void testAlterTable() + { + String tableName = "test_s3_alter_" + randomNameSuffix(); + assertUpdate("CREATE TABLE " + tableName + " (id INTEGER)"); + assertUpdate("INSERT INTO " + tableName + " VALUES (1)", 1); + + assertUpdate("ALTER TABLE " + tableName + " ADD COLUMN name VARCHAR"); + assertUpdate("INSERT INTO " + tableName + " VALUES (2, 'alice')", 1); + + assertQuery("SELECT * FROM " + tableName + " ORDER BY id", "VALUES (1, NULL), (2, 'alice')"); + + assertUpdate("ALTER TABLE " + tableName + " RENAME COLUMN name TO full_name"); + assertQuery("SELECT id, full_name FROM " + tableName + " WHERE id = 2", "VALUES (2, 'alice')"); + + assertUpdate("DROP TABLE " + tableName); + } + + private static JdbcCatalog createBackendCatalog(String minioAddress) + throws IOException + { + Path tempFile = Files.createTempFile("iceberg-s3-test-jdbc", null); + tempFile.toFile().deleteOnExit(); + + ImmutableMap.Builder properties = ImmutableMap.builder(); + properties.put(CatalogProperties.URI, "jdbc:h2:file:" + tempFile.toAbsolutePath()); + properties.put(JdbcCatalog.PROPERTY_PREFIX + "username", "user"); + properties.put(JdbcCatalog.PROPERTY_PREFIX + "password", "password"); + properties.put(JdbcCatalog.PROPERTY_PREFIX + "schema-version", "V1"); + properties.put(CatalogProperties.WAREHOUSE_LOCATION, "s3://" + BUCKET_NAME + "/warehouse"); + properties.put(CatalogProperties.FILE_IO_IMPL, "org.apache.iceberg.aws.s3.S3FileIO"); + properties.put("s3.access-key-id", MINIO_ROOT_USER); + properties.put("s3.secret-access-key", MINIO_ROOT_PASSWORD); + properties.put("s3.endpoint", minioAddress); + properties.put("s3.path-style-access", "true"); + properties.put("client.region", MINIO_REGION); + + JdbcCatalog catalog = new JdbcCatalog(); + catalog.setConf(new org.apache.hadoop.conf.Configuration()); + catalog.initialize("s3_backend", properties.buildOrThrow()); + + return catalog; + } +} diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergVendedCredentialsMapper.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergVendedCredentialsMapper.java new file mode 100644 index 000000000000..c43957b465e2 --- /dev/null +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergVendedCredentialsMapper.java @@ -0,0 +1,287 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.iceberg.catalog.rest; + +import com.google.common.collect.ImmutableMap; +import io.trino.filesystem.Location; +import io.trino.filesystem.s3.S3FileSystemConfig; +import io.trino.filesystem.s3.S3SecurityMappingResult; +import io.trino.spi.security.ConnectorIdentity; +import org.junit.jupiter.api.Test; +import software.amazon.awssdk.auth.credentials.AwsCredentials; +import software.amazon.awssdk.identity.spi.AwsCredentialsIdentity; + +import java.util.Map; +import java.util.Optional; + +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_ENDPOINT_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_REGION_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY; +import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY; +import static org.assertj.core.api.Assertions.assertThat; + +final class TestIcebergVendedCredentialsMapper +{ + private static final Location DEFAULT_LOCATION = Location.of("s3://test-bucket/path"); + + @Test + void testNoVendedCredentials() + { + IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(new S3FileSystemConfig()); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test").build(); + + Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); + + assertThat(result).isEmpty(); + } + + @Test + void testVendedCredentialsOnly() + { + IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(new S3FileSystemConfig()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); + + assertThat(result).isPresent(); + assertThat(result.get().credentials()).isPresent(); + assertThat(result.get().credentials().get()).isInstanceOf(AwsCredentials.class); + AwsCredentialsIdentity credentials = (AwsCredentialsIdentity) result.get().credentials().get(); + assertThat(credentials.accessKeyId()).isEqualTo("vended-access"); + assertThat(credentials.secretAccessKey()).isEqualTo("vended-secret"); + } + + @Test + void testVendedRegionTakesPrecedenceOverStatic() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setRegion("us-east-1"); + + IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(config); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-2") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); + + assertThat(result).isPresent(); + assertThat(result.get().region()).hasValue("us-west-2"); + } + + @Test + void testVendedEndpointTakesPrecedenceOverStatic() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setEndpoint("https://s3.amazonaws.com"); + + IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(config); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://minio.example.com") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); + + assertThat(result).isPresent(); + assertThat(result.get().endpoint()).hasValue("https://minio.example.com"); + } + + @Test + void testStaticCrossRegionAccessTrueAlwaysWins() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setCrossRegionAccessEnabled(true); + + IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(config); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "false") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); + + assertThat(result).isPresent(); + assertThat(result.get().crossRegionAccessEnabled()).hasValue(true); + } + + @Test + void testVendedCrossRegionAccessWhenStaticIsFalse() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setCrossRegionAccessEnabled(false); + + IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(config); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "true") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); + + assertThat(result).isPresent(); + assertThat(result.get().crossRegionAccessEnabled()).hasValue(true); + } + + @Test + void testIncompleteCredentialsReturnsEmpty() + { + IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(new S3FileSystemConfig()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); + + assertThat(result).isEmpty(); + } + + @Test + void testVendedRegionUsedWhenNoStatic() + { + IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(new S3FileSystemConfig()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "ap-south-1") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); + + assertThat(result).isPresent(); + assertThat(result.get().region()).hasValue("ap-south-1"); + } + + @Test + void testVendedEndpointUsedWhenNoStatic() + { + IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(new S3FileSystemConfig()); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://custom-s3.example.com") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); + + assertThat(result).isPresent(); + assertThat(result.get().endpoint()).hasValue("https://custom-s3.example.com"); + } + + @Test + void testStaticRegionUsedWhenVendedNotProvided() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setRegion("us-east-1"); + + IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(config); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); + + assertThat(result).isPresent(); + assertThat(result.get().region()).hasValue("us-east-1"); + } + + @Test + void testStaticEndpointUsedWhenVendedNotProvided() + { + S3FileSystemConfig config = new S3FileSystemConfig() + .setEndpoint("https://s3.amazonaws.com"); + + IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(config); + + Map extraCredentials = ImmutableMap.builder() + .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") + .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") + .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") + .buildOrThrow(); + + ConnectorIdentity identity = ConnectorIdentity.forUser("test") + .withExtraCredentials(extraCredentials) + .build(); + + Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); + + assertThat(result).isPresent(); + assertThat(result.get().endpoint()).hasValue("https://s3.amazonaws.com"); + } +} diff --git a/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/S3CredentialVendingCatalogAdapter.java b/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/S3CredentialVendingCatalogAdapter.java new file mode 100644 index 000000000000..25968b5f3952 --- /dev/null +++ b/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/S3CredentialVendingCatalogAdapter.java @@ -0,0 +1,54 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.iceberg.rest; + +import org.apache.iceberg.catalog.Catalog; +import org.apache.iceberg.rest.responses.ErrorResponse; +import org.apache.iceberg.rest.responses.LoadTableResponse; + +import java.util.Map; +import java.util.function.Consumer; + +import static java.util.Objects.requireNonNull; + +/** + * A REST catalog adapter that injects S3 vended credential properties + * into the LoadTableResponse config, simulating a REST catalog server + * that vends S3 credentials with region, endpoint, and cross-region access. + */ +public class S3CredentialVendingCatalogAdapter + extends RESTCatalogAdapter +{ + private final Map vendedS3Properties; + + public S3CredentialVendingCatalogAdapter(Catalog backendCatalog, Map vendedS3Properties) + { + super(backendCatalog); + this.vendedS3Properties = requireNonNull(vendedS3Properties, "vendedS3Properties is null"); + } + + @Override + protected T execute( + HTTPRequest request, + Class responseType, + Consumer errorHandler, + Consumer> responseHeaders) + { + T response = super.execute(request, responseType, errorHandler, responseHeaders); + if (response instanceof LoadTableResponse loadTableResponse) { + loadTableResponse.config().putAll(vendedS3Properties); + } + return response; + } +} From 66a19cc7077e8a4d206fbf0676be51e0cc41f5d5 Mon Sep 17 00:00:00 2001 From: kaveti Date: Thu, 9 Apr 2026 16:32:30 +0530 Subject: [PATCH 6/9] Support Iceberg V3 storage credentials in REST catalog Made-with: Cursor --- .../sphinx/object-storage/file-system-s3.md | 15 +- .../main/sphinx/object-storage/metastores.md | 7 +- .../filesystem/s3/S3CredentialsMapper.java | 28 - .../filesystem/s3/S3FileSystemFactory.java | 59 +- .../filesystem/s3/S3FileSystemLoader.java | 88 +-- .../filesystem/s3/S3FileSystemModule.java | 2 - .../s3/S3FileSystemSecurityMapper.java | 29 - .../s3/S3SecurityMappingProvider.java | 5 +- .../s3/S3SecurityMappingResult.java | 6 +- ...ileSystemFactoryWithVendedCredentials.java | 581 ------------------ ...FileSystemLoaderWithCredentialsMapper.java | 546 ---------------- .../filesystem/s3/S3FileSystemConstants.java | 3 - .../rest/IcebergRestCatalogModule.java | 7 +- .../rest/IcebergVendedCredentialsMapper.java | 89 --- .../rest/IcebergVendedCredentialsModule.java | 32 - .../rest/TrinoIcebergRestCatalogFactory.java | 32 +- .../catalog/rest/TrinoRestCatalog.java | 255 ++++---- .../iceberg/fileio/ForwardingFileIo.java | 118 +++- .../rest/TestIcebergS3VendingRestCatalog.java | 345 ----------- ...tIcebergStorageCredentialsRestCatalog.java | 110 ++++ .../TestIcebergVendedCredentialsMapper.java | 287 --------- .../iceberg/fileio/TestForwardingFileIo.java | 134 ++++ .../rest/DelegatingRestSessionCatalog.java | 51 +- .../S3CredentialVendingCatalogAdapter.java | 54 -- 24 files changed, 593 insertions(+), 2290 deletions(-) delete mode 100644 lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3CredentialsMapper.java delete mode 100644 lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemSecurityMapper.java delete mode 100644 lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3FileSystemFactoryWithVendedCredentials.java delete mode 100644 lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3FileSystemLoaderWithCredentialsMapper.java delete mode 100644 plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergVendedCredentialsMapper.java delete mode 100644 plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergVendedCredentialsModule.java delete mode 100644 plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3VendingRestCatalog.java create mode 100644 plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergStorageCredentialsRestCatalog.java delete mode 100644 plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergVendedCredentialsMapper.java delete mode 100644 plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/S3CredentialVendingCatalogAdapter.java diff --git a/docs/src/main/sphinx/object-storage/file-system-s3.md b/docs/src/main/sphinx/object-storage/file-system-s3.md index 2618e3009238..d53824de822f 100644 --- a/docs/src/main/sphinx/object-storage/file-system-s3.md +++ b/docs/src/main/sphinx/object-storage/file-system-s3.md @@ -25,20 +25,11 @@ support: - Activate the native implementation for S3 storage support. Defaults to `false`. Set to `true` to use S3 and enable all other properties. * - `s3.endpoint` - - S3 service endpoint URL to communicate with. When using the Iceberg REST catalog - with `iceberg.rest-catalog.vended-credentials-enabled=true`, this property can - be provided by the REST catalog via `s3.endpoint`. Static catalog configuration - takes precedence over vended values. + - S3 service endpoint URL to communicate with. * - `s3.region` - - S3 region to communicate with. When using the Iceberg REST catalog with - `iceberg.rest-catalog.vended-credentials-enabled=true`, this property can be - provided by the REST catalog via `client.region`. Static catalog configuration - takes precedence over vended values. + - S3 region to communicate with. * - `s3.cross-region-access` - - Enable cross region access. Defaults to `false`. When using the Iceberg REST catalog - with `iceberg.rest-catalog.vended-credentials-enabled=true`, this property can be - provided by the REST catalog via `s3.cross-region-access-enabled`. Static catalog - configuration takes precedence over vended values. + - Enable cross region access. Defaults to `false`. * - `s3.path-style-access` - Use path-style access for all requests to S3 * - `s3.storage-class` diff --git a/docs/src/main/sphinx/object-storage/metastores.md b/docs/src/main/sphinx/object-storage/metastores.md index ce407d6b9624..3d5e98e3a1ba 100644 --- a/docs/src/main/sphinx/object-storage/metastores.md +++ b/docs/src/main/sphinx/object-storage/metastores.md @@ -519,11 +519,8 @@ following properties: - Controls whether to use the token exchange flow to acquire new tokens. Defaults to `true` * - `iceberg.rest-catalog.vended-credentials-enabled` - - Use credentials and configuration provided by the REST backend for file system access. - When enabled, the REST catalog can provide S3 credentials (`s3.access-key-id`, - `s3.secret-access-key`, `s3.session-token`) and configuration properties - (`client.region`, `s3.endpoint`, `s3.cross-region-access-enabled`). Static catalog - configuration takes precedence over vended properties. Defaults to `false`. + - Use credentials provided by the REST backend for file system access. + Defaults to `false`. * - `iceberg.rest-catalog.nested-namespace-enabled` - Support querying objects under nested namespace. Defaults to `false`. diff --git a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3CredentialsMapper.java b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3CredentialsMapper.java deleted file mode 100644 index 805716429926..000000000000 --- a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3CredentialsMapper.java +++ /dev/null @@ -1,28 +0,0 @@ -/* - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.trino.filesystem.s3; - -import io.trino.filesystem.Location; -import io.trino.spi.security.ConnectorIdentity; - -import java.util.Optional; - -public interface S3CredentialsMapper -{ - /** - * Returns S3 credentials and configuration for the given identity and location. - * Empty result indicates cluster defaults should be used. - */ - Optional getMapping(ConnectorIdentity identity, Location location); -} diff --git a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemFactory.java b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemFactory.java index 29c8cb0e91f2..dc9bd68ddfeb 100644 --- a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemFactory.java +++ b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemFactory.java @@ -19,13 +19,9 @@ import io.trino.filesystem.TrinoFileSystemFactory; import io.trino.spi.security.ConnectorIdentity; import jakarta.annotation.PreDestroy; -import software.amazon.awssdk.auth.credentials.AwsCredentials; import software.amazon.awssdk.services.s3.S3Client; import software.amazon.awssdk.services.s3.presigner.S3Presigner; -import java.util.Map; -import java.util.Optional; -import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executor; import static io.trino.filesystem.s3.S3FileSystemUtils.createS3PreSigner; @@ -38,10 +34,6 @@ public final class S3FileSystemFactory private final S3Context context; private final Executor uploadExecutor; private final S3Presigner preSigner; - private final String staticRegion; - private final String staticEndpoint; - private final boolean staticCrossRegionAccessEnabled; - private final Map dynamicClients; @Inject public S3FileSystemFactory(OpenTelemetry openTelemetry, S3FileSystemConfig config, S3FileSystemStats stats) @@ -51,56 +43,19 @@ public S3FileSystemFactory(OpenTelemetry openTelemetry, S3FileSystemConfig confi this.preSigner = createS3PreSigner(config, client); this.context = loader.context(); this.uploadExecutor = loader.uploadExecutor(); - this.staticRegion = config.getRegion(); - this.staticEndpoint = config.getEndpoint(); - this.staticCrossRegionAccessEnabled = config.isCrossRegionAccessEnabled(); - this.dynamicClients = new ConcurrentHashMap<>(); - } - - @Override - public TrinoFileSystem create(ConnectorIdentity identity) - { - Optional vendedCredentials = S3FileSystemLoader.extractCredentialsFromIdentity(identity); - String region = S3FileSystemLoader.extractRegionFromIdentity(identity).orElse(staticRegion); - String endpoint = S3FileSystemLoader.extractEndpointFromIdentity(identity).orElse(staticEndpoint); - - boolean crossRegionAccessEnabled; - if (staticCrossRegionAccessEnabled) { - crossRegionAccessEnabled = true; - } - else { - crossRegionAccessEnabled = S3FileSystemLoader.extractCrossRegionAccessEnabledFromIdentity(identity).orElse(false); - } - - boolean hasOverrides = vendedCredentials.isPresent() - || (region != null && !region.equals(staticRegion)) - || (endpoint != null && !endpoint.equals(staticEndpoint)) - || crossRegionAccessEnabled != staticCrossRegionAccessEnabled; - - if (hasOverrides) { - ClientKey key = new ClientKey(vendedCredentials.orElse(null), region, endpoint, crossRegionAccessEnabled); - S3Client s3Client = dynamicClients.computeIfAbsent(key, _ -> - loader.createClientWithOverrides(vendedCredentials, region, endpoint, crossRegionAccessEnabled)); - return new S3FileSystem(uploadExecutor, s3Client, preSigner, context.withCredentials(identity)); - } - - return new S3FileSystem(uploadExecutor, client, preSigner, context.withCredentials(identity)); } @PreDestroy public void destroy() { - for (S3Client dynamicClient : dynamicClients.values()) { - try (var _ = dynamicClient) { - // Resource automatically closed - } + try (client) { + loader.destroy(); } - dynamicClients.clear(); - try (var _ = client) { - // Resource automatically closed - } - loader.destroy(); } - private record ClientKey(AwsCredentials credentials, String region, String endpoint, boolean crossRegionAccessEnabled) {} + @Override + public TrinoFileSystem create(ConnectorIdentity identity) + { + return new S3FileSystem(uploadExecutor, client, preSigner, context.withCredentials(identity)); + } } diff --git a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemLoader.java b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemLoader.java index a3418d083e68..5c7d253844a6 100644 --- a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemLoader.java +++ b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemLoader.java @@ -19,11 +19,8 @@ import io.trino.filesystem.Location; import io.trino.filesystem.TrinoFileSystemFactory; import io.trino.filesystem.s3.S3Context.S3SseContext; -import io.trino.spi.security.ConnectorIdentity; import jakarta.annotation.PreDestroy; -import software.amazon.awssdk.auth.credentials.AwsCredentials; import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; -import software.amazon.awssdk.auth.credentials.AwsSessionCredentials; import software.amazon.awssdk.auth.credentials.WebIdentityTokenFileCredentialsProvider; import software.amazon.awssdk.core.checksums.RequestChecksumCalculation; import software.amazon.awssdk.core.client.config.ClientOverrideConfiguration; @@ -49,12 +46,6 @@ import static com.google.common.base.Preconditions.checkState; import static io.airlift.concurrent.Threads.daemonThreadsNamed; import static io.trino.filesystem.s3.S3FileSystemConfig.RetryMode.getRetryStrategy; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_ENDPOINT_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_REGION_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY; import static io.trino.filesystem.s3.S3FileSystemUtils.createS3PreSigner; import static io.trino.filesystem.s3.S3FileSystemUtils.createStaticCredentialsProvider; import static io.trino.filesystem.s3.S3FileSystemUtils.createStsClient; @@ -67,7 +58,7 @@ final class S3FileSystemLoader implements Function { - private final Optional mappingProvider; + private final Optional mappingProvider; private final SdkHttpClient httpClient; private final S3ClientFactory clientFactory; private final S3FileSystemConfig config; @@ -77,7 +68,17 @@ final class S3FileSystemLoader private final Map, S3Presigner> preSigners = new ConcurrentHashMap<>(); @Inject - public S3FileSystemLoader(Optional mappingProvider, OpenTelemetry openTelemetry, S3FileSystemConfig config, S3FileSystemStats stats) + public S3FileSystemLoader(S3SecurityMappingProvider mappingProvider, OpenTelemetry openTelemetry, S3FileSystemConfig config, S3FileSystemStats stats) + { + this(Optional.of(mappingProvider), openTelemetry, config, stats); + } + + S3FileSystemLoader(OpenTelemetry openTelemetry, S3FileSystemConfig config, S3FileSystemStats stats) + { + this(Optional.empty(), openTelemetry, config, stats); + } + + private S3FileSystemLoader(Optional mappingProvider, OpenTelemetry openTelemetry, S3FileSystemConfig config, S3FileSystemStats stats) { this.mappingProvider = requireNonNull(mappingProvider, "mappingProvider is null"); this.httpClient = createHttpClient(config); @@ -99,11 +100,6 @@ public S3FileSystemLoader(Optional mappingProvider, OpenTel config.getCannedAcl()); } - S3FileSystemLoader(OpenTelemetry openTelemetry, S3FileSystemConfig config, S3FileSystemStats stats) - { - this(Optional.empty(), openTelemetry, config, stats); - } - @Override public TrinoFileSystemFactory apply(Location location) { @@ -130,18 +126,6 @@ public TrinoFileSystemFactory apply(Location location) @PreDestroy public void destroy() { - for (S3Client client : clients.values()) { - try (var _ = client) { - // Resource automatically closed - } - } - clients.clear(); - for (S3Presigner preSigner : preSigners.values()) { - try (var _ = preSigner) { - // Resource automatically closed - } - } - preSigners.clear(); try (httpClient) { uploadExecutor.shutdownNow(); } @@ -162,48 +146,6 @@ Executor uploadExecutor() return uploadExecutor; } - static Optional extractCredentialsFromIdentity(ConnectorIdentity identity) - { - String accessKey = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY); - String secretKey = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY); - String sessionToken = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY); - if (accessKey != null && secretKey != null && sessionToken != null) { - return Optional.of(AwsSessionCredentials.create(accessKey, secretKey, sessionToken)); - } - return Optional.empty(); - } - - static Optional extractRegionFromIdentity(ConnectorIdentity identity) - { - return Optional.ofNullable(identity.getExtraCredentials().get(EXTRA_CREDENTIALS_REGION_PROPERTY)); - } - - static Optional extractEndpointFromIdentity(ConnectorIdentity identity) - { - return Optional.ofNullable(identity.getExtraCredentials().get(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY)); - } - - static Optional extractCrossRegionAccessEnabledFromIdentity(ConnectorIdentity identity) - { - String value = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY); - return value != null ? Optional.of(Boolean.parseBoolean(value)) : Optional.empty(); - } - - S3Client createClientWithOverrides(Optional credentials, String region, String endpoint, Boolean crossRegionAccessEnabled) - { - S3SecurityMappingResult customMapping = new S3SecurityMappingResult( - credentials, - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.ofNullable(endpoint), - Optional.ofNullable(region), - Optional.ofNullable(crossRegionAccessEnabled)); - - return clientFactory.create(Optional.of(customMapping)); - } - private static S3ClientFactory s3ClientFactory(SdkHttpClient httpClient, OpenTelemetry openTelemetry, S3FileSystemConfig config, MetricPublisher metricPublisher) { ClientOverrideConfiguration overrideConfiguration = createOverrideConfiguration(openTelemetry, config, metricPublisher); @@ -228,13 +170,9 @@ private static S3ClientFactory s3ClientFactory(SdkHttpClient httpClient, OpenTel Optional iamRole = mapping.flatMap(S3SecurityMappingResult::iamRole).or(() -> staticIamRole); String roleSessionName = mapping.flatMap(S3SecurityMappingResult::roleSessionName).orElse(staticRoleSessionName); - boolean crossRegionAccessEnabled = mapping - .flatMap(S3SecurityMappingResult::crossRegionAccessEnabled) - .orElse(config.isCrossRegionAccessEnabled()); - S3ClientBuilder s3 = S3Client.builder(); s3.overrideConfiguration(overrideConfiguration); - s3.crossRegionAccessEnabled(crossRegionAccessEnabled); + s3.crossRegionAccessEnabled(config.isCrossRegionAccessEnabled()); s3.httpClient(httpClient); s3.responseChecksumValidation(WHEN_REQUIRED); s3.requestChecksumCalculation(RequestChecksumCalculation.WHEN_REQUIRED); diff --git a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemModule.java b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemModule.java index cc8fb87b7fdf..a7db2f289634 100644 --- a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemModule.java +++ b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemModule.java @@ -29,7 +29,6 @@ import java.util.function.Supplier; import static com.google.inject.Scopes.SINGLETON; -import static com.google.inject.multibindings.OptionalBinder.newOptionalBinder; import static io.airlift.configuration.ConfigBinder.configBinder; import static io.airlift.http.client.HttpClientBinder.httpClientBinder; import static java.lang.annotation.ElementType.FIELD; @@ -68,7 +67,6 @@ protected void setup(Binder binder) S3SecurityMappingConfig config = buildConfigObject(S3SecurityMappingConfig.class); binder.bind(S3SecurityMappingProvider.class).in(SINGLETON); - newOptionalBinder(binder, S3CredentialsMapper.class).setBinding().to(S3SecurityMappingProvider.class).in(SINGLETON); binder.bind(S3FileSystemLoader.class).in(SINGLETON); var mappingsBinder = binder.bind(new Key>() {}); diff --git a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemSecurityMapper.java b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemSecurityMapper.java deleted file mode 100644 index 2033f325941a..000000000000 --- a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemSecurityMapper.java +++ /dev/null @@ -1,29 +0,0 @@ -/* - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.trino.filesystem.s3; - -import io.trino.filesystem.Location; -import io.trino.spi.security.ConnectorIdentity; - -import java.util.Optional; - -public interface S3FileSystemSecurityMapper -{ - /** - * Get security mapping for the given identity and location. - * - * @return Empty optional if cluster defaults should be used, otherwise the security mapping result - */ - Optional getMapping(ConnectorIdentity identity, Location location); -} diff --git a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingProvider.java b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingProvider.java index bc6d06202706..008fb835f6bf 100644 --- a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingProvider.java +++ b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingProvider.java @@ -29,7 +29,6 @@ import static java.util.concurrent.TimeUnit.MILLISECONDS; final class S3SecurityMappingProvider - implements S3CredentialsMapper { private final Supplier mappingsProvider; private final Optional roleCredentialName; @@ -61,7 +60,6 @@ public S3SecurityMappingProvider( this.colonReplacement = requireNonNull(colonReplacement, "colonReplacement is null"); } - @Override public Optional getMapping(ConnectorIdentity identity, Location location) { S3SecurityMapping mapping = mappingsProvider.get().getMapping(identity, new S3Location(location)) @@ -78,8 +76,7 @@ public Optional getMapping(ConnectorIdentity identity, selectKmsKeyId(mapping, identity), getSseCustomerKey(mapping, identity), mapping.endpoint(), - mapping.region(), - Optional.empty())); + mapping.region())); } private Optional selectRole(S3SecurityMapping mapping, ConnectorIdentity identity) diff --git a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingResult.java b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingResult.java index aecffc986713..2d2ef7dc2e92 100644 --- a/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingResult.java +++ b/lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingResult.java @@ -21,15 +21,14 @@ import static java.util.Objects.requireNonNull; -public record S3SecurityMappingResult( +record S3SecurityMappingResult( Optional credentials, Optional iamRole, Optional roleSessionName, Optional kmsKeyId, Optional sseCustomerKey, Optional endpoint, - Optional region, - Optional crossRegionAccessEnabled) + Optional region) { public S3SecurityMappingResult { @@ -40,7 +39,6 @@ public record S3SecurityMappingResult( requireNonNull(sseCustomerKey, "sseCustomerKey is null"); requireNonNull(endpoint, "endpoint is null"); requireNonNull(region, "region is null"); - requireNonNull(crossRegionAccessEnabled, "crossRegionAccessEnabled is null"); } public Optional credentialsProvider() diff --git a/lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3FileSystemFactoryWithVendedCredentials.java b/lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3FileSystemFactoryWithVendedCredentials.java deleted file mode 100644 index ce8cafa98459..000000000000 --- a/lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3FileSystemFactoryWithVendedCredentials.java +++ /dev/null @@ -1,581 +0,0 @@ -/* - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.trino.filesystem.s3; - -import com.google.common.collect.ImmutableMap; -import io.opentelemetry.api.OpenTelemetry; -import io.trino.filesystem.TrinoFileSystem; -import io.trino.spi.security.ConnectorIdentity; -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.Test; -import software.amazon.awssdk.services.s3.S3Client; - -import java.lang.reflect.Field; -import java.util.Map; - -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_ENDPOINT_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_REGION_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY; -import static org.assertj.core.api.Assertions.assertThat; - -final class TestS3FileSystemFactoryWithVendedCredentials -{ - private S3FileSystemFactory factory; - - @AfterEach - void tearDown() - { - if (factory != null) { - factory.destroy(); - } - } - - @Test - void testBackwardCompatibility() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setAwsAccessKey("test-access") - .setAwsSecretKey("test-secret") - .setRegion("us-east-1"); - - factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); - - ConnectorIdentity identity = ConnectorIdentity.ofUser("test"); - TrinoFileSystem fs = factory.create(identity); - - assertThat(fs).isNotNull(); - } - - @Test - void testVendedRegionWhenNoStaticRegion() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setAwsAccessKey("test-access") - .setAwsSecretKey("test-secret") - .setRegion("us-east-1"); - - factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-2") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - TrinoFileSystem fs = factory.create(identity); - assertThat(fs).isNotNull(); - } - - @Test - void testStaticRegionTakesPrecedenceOverVended() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setAwsAccessKey("test-access") - .setAwsSecretKey("test-secret") - .setRegion("us-east-1"); - - factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-2") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - TrinoFileSystem fs = factory.create(identity); - assertThat(fs).isNotNull(); - } - - @Test - void testVendedEndpointWhenNoStaticEndpoint() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setAwsAccessKey("test-access") - .setAwsSecretKey("test-secret") - .setRegion("us-east-1"); - - factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://minio.example.com") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - TrinoFileSystem fs = factory.create(identity); - assertThat(fs).isNotNull(); - } - - @Test - void testStaticEndpointTakesPrecedenceOverVended() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setAwsAccessKey("test-access") - .setAwsSecretKey("test-secret") - .setRegion("us-east-1") - .setEndpoint("https://s3.amazonaws.com"); - - factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://minio.example.com") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - TrinoFileSystem fs = factory.create(identity); - assertThat(fs).isNotNull(); - } - - @Test - void testVendedCrossRegionAccessWhenStaticIsDisabled() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setAwsAccessKey("test-access") - .setAwsSecretKey("test-secret") - .setRegion("us-east-1") - .setCrossRegionAccessEnabled(false); - - factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "true") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - TrinoFileSystem fs = factory.create(identity); - assertThat(fs).isNotNull(); - } - - @Test - void testStaticCrossRegionAccessTakesPrecedence() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setAwsAccessKey("test-access") - .setAwsSecretKey("test-secret") - .setRegion("us-east-1") - .setCrossRegionAccessEnabled(true); - - factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "false") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - TrinoFileSystem fs = factory.create(identity); - assertThat(fs).isNotNull(); - } - - @Test - void testAllThreeOverrides() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setAwsAccessKey("test-access") - .setAwsSecretKey("test-secret") - .setRegion("us-east-1"); - - factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "eu-west-1") - .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://minio.example.com") - .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "true") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - TrinoFileSystem fs = factory.create(identity); - assertThat(fs).isNotNull(); - } - - @Test - void testMultipleUsersWithSameVendedRegion() - throws Exception - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setAwsAccessKey("test-access") - .setAwsSecretKey("test-secret") - .setRegion("us-east-1"); - - factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-2") - .buildOrThrow(); - - ConnectorIdentity identity1 = ConnectorIdentity.forUser("user1") - .withExtraCredentials(extraCredentials) - .build(); - - ConnectorIdentity identity2 = ConnectorIdentity.forUser("user2") - .withExtraCredentials(extraCredentials) - .build(); - - TrinoFileSystem fs1 = factory.create(identity1); - TrinoFileSystem fs2 = factory.create(identity2); - - assertThat(fs1).isNotNull(); - assertThat(fs2).isNotNull(); - - S3Client client1 = getClientFromFileSystem(fs1); - S3Client client2 = getClientFromFileSystem(fs2); - - assertThat(client1).isSameAs(client2); - } - - @Test - void testDifferentVendedRegions() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setAwsAccessKey("test-access") - .setAwsSecretKey("test-secret") - .setRegion("us-east-1"); - - factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); - - Map extraCredentials1 = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-1") - .buildOrThrow(); - - Map extraCredentials2 = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-2") - .buildOrThrow(); - - ConnectorIdentity identity1 = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials1) - .build(); - - ConnectorIdentity identity2 = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials2) - .build(); - - TrinoFileSystem fs1 = factory.create(identity1); - TrinoFileSystem fs2 = factory.create(identity2); - - assertThat(fs1).isNotNull(); - assertThat(fs2).isNotNull(); - } - - @Test - void testNoOverridesUsesDefaultClient() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setAwsAccessKey("test-access") - .setAwsSecretKey("test-secret") - .setRegion("us-east-1"); - - factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); - - ConnectorIdentity identity = ConnectorIdentity.ofUser("test"); - TrinoFileSystem fs = factory.create(identity); - - assertThat(fs).isNotNull(); - } - - @Test - void testOnlyCredentialsNoOverridesUsesDefaultClient() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setAwsAccessKey("test-access") - .setAwsSecretKey("test-secret") - .setRegion("us-east-1"); - - factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - TrinoFileSystem fs = factory.create(identity); - assertThat(fs).isNotNull(); - } - - @Test - void testExtractionMethods() - { - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-2") - .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://minio.local") - .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "true") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - assertThat(S3FileSystemLoader.extractRegionFromIdentity(identity)) - .isPresent() - .hasValue("us-west-2"); - - assertThat(S3FileSystemLoader.extractEndpointFromIdentity(identity)) - .isPresent() - .hasValue("https://minio.local"); - - assertThat(S3FileSystemLoader.extractCrossRegionAccessEnabledFromIdentity(identity)) - .isPresent() - .hasValue(true); - } - - @Test - void testExtractionMethodsWithMissingValues() - { - ConnectorIdentity identity = ConnectorIdentity.ofUser("test"); - - assertThat(S3FileSystemLoader.extractRegionFromIdentity(identity)) - .isEmpty(); - - assertThat(S3FileSystemLoader.extractEndpointFromIdentity(identity)) - .isEmpty(); - - assertThat(S3FileSystemLoader.extractCrossRegionAccessEnabledFromIdentity(identity)) - .isEmpty(); - } - - @Test - void testCrossRegionAccessBooleanParsing() - { - Map extraCredentialsTrue = ImmutableMap.of( - EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "true"); - - Map extraCredentialsFalse = ImmutableMap.of( - EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "false"); - - ConnectorIdentity identityTrue = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentialsTrue) - .build(); - - ConnectorIdentity identityFalse = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentialsFalse) - .build(); - - assertThat(S3FileSystemLoader.extractCrossRegionAccessEnabledFromIdentity(identityTrue)) - .isPresent() - .hasValue(true); - - assertThat(S3FileSystemLoader.extractCrossRegionAccessEnabledFromIdentity(identityFalse)) - .isPresent() - .hasValue(false); - } - - @Test - void testVendedCrossRegionAccessWhenStaticIsFalse() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setAwsAccessKey("test-access") - .setAwsSecretKey("test-secret") - .setRegion("us-east-1") - .setCrossRegionAccessEnabled(false); - - factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "false") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - TrinoFileSystem fs = factory.create(identity); - assertThat(fs).isNotNull(); - } - - @Test - void testVendedCrossRegionAccessFalseWhenStaticIsFalse() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setAwsAccessKey("test-access") - .setAwsSecretKey("test-secret") - .setRegion("us-east-1") - .setCrossRegionAccessEnabled(false); - - factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "true") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - TrinoFileSystem fs = factory.create(identity); - assertThat(fs).isNotNull(); - } - - @Test - void testClientCachingWithSameConfiguration() - throws Exception - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setAwsAccessKey("test-access") - .setAwsSecretKey("test-secret") - .setRegion("us-east-1"); - - factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-2") - .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://minio.example.com") - .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "true") - .buildOrThrow(); - - ConnectorIdentity identity1 = ConnectorIdentity.forUser("user1") - .withExtraCredentials(extraCredentials) - .build(); - - ConnectorIdentity identity2 = ConnectorIdentity.forUser("user2") - .withExtraCredentials(extraCredentials) - .build(); - - TrinoFileSystem fs1 = factory.create(identity1); - TrinoFileSystem fs2 = factory.create(identity2); - - assertThat(fs1).isNotNull(); - assertThat(fs2).isNotNull(); - - S3Client client1 = getClientFromFileSystem(fs1); - S3Client client2 = getClientFromFileSystem(fs2); - - assertThat(client1).isSameAs(client2); - } - - private static S3Client getClientFromFileSystem(TrinoFileSystem fileSystem) - throws Exception - { - assertThat(fileSystem).isInstanceOf(S3FileSystem.class); - Field clientField = S3FileSystem.class.getDeclaredField("client"); - clientField.setAccessible(true); - return (S3Client) clientField.get(fileSystem); - } - - @Test - void testVendedEndpointWhenStaticEndpointIsNull() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setAwsAccessKey("test-access") - .setAwsSecretKey("test-secret") - .setRegion("us-east-1"); - - factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-2") - .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://minio.example.com") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - TrinoFileSystem fs = factory.create(identity); - assertThat(fs).isNotNull(); - } - - @Test - void testCombinationRegionEndpointAndCrossRegionAccess() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setAwsAccessKey("test-access") - .setAwsSecretKey("test-secret") - .setRegion("us-east-1") - .setCrossRegionAccessEnabled(false); - - factory = new S3FileSystemFactory(OpenTelemetry.noop(), config, new S3FileSystemStats()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "eu-central-1") - .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://custom-s3.example.com") - .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "true") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - TrinoFileSystem fs = factory.create(identity); - assertThat(fs).isNotNull(); - } -} diff --git a/lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3FileSystemLoaderWithCredentialsMapper.java b/lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3FileSystemLoaderWithCredentialsMapper.java deleted file mode 100644 index 2c988871ad55..000000000000 --- a/lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3FileSystemLoaderWithCredentialsMapper.java +++ /dev/null @@ -1,546 +0,0 @@ -/* - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.trino.filesystem.s3; - -import io.opentelemetry.api.OpenTelemetry; -import io.trino.filesystem.Location; -import io.trino.filesystem.TrinoFileSystem; -import io.trino.filesystem.TrinoFileSystemFactory; -import io.trino.spi.security.ConnectorIdentity; -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import software.amazon.awssdk.auth.credentials.AwsSessionCredentials; -import software.amazon.awssdk.services.s3.S3Client; - -import java.lang.reflect.Field; -import java.util.Map; -import java.util.Optional; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; - -import static org.assertj.core.api.Assertions.assertThat; - -final class TestS3FileSystemLoaderWithCredentialsMapper -{ - private S3FileSystemLoader loader; - private S3FileSystemConfig config; - private S3FileSystemStats stats; - private TestCredentialsMapper credentialsMapper; - - @BeforeEach - void setUp() - throws Exception - { - config = new S3FileSystemConfig() - .setAwsAccessKey("test-access-key") - .setAwsSecretKey("test-secret-key") - .setRegion("us-east-1") - .setCrossRegionAccessEnabled(false); - - stats = new S3FileSystemStats(); - credentialsMapper = new TestCredentialsMapper(); - - // Use the public constructor directly - loader = new S3FileSystemLoader( - Optional.of(credentialsMapper), - OpenTelemetry.noop(), - config, - stats); - } - - @AfterEach - void tearDown() - { - if (loader != null) { - loader.destroy(); - } - } - - @Test - void testClientCachingForSameCredentials() - throws Exception - { - // Arrange: Same credentials - ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); - Location location = Location.of("s3://test-bucket/path"); - - S3SecurityMappingResult mapping = new S3SecurityMappingResult( - Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.of("https://s3.us-east-1.amazonaws.com"), - Optional.of("us-east-1"), - Optional.of(true)); - - credentialsMapper.setMapping(mapping); - - // Act: Create file systems multiple times with same credentials - TrinoFileSystemFactory factory = loader.apply(location); - TrinoFileSystem fs1 = factory.create(identity); - TrinoFileSystem fs2 = factory.create(identity); - TrinoFileSystem fs3 = factory.create(identity); - - // Assert: Same S3Client is reused - S3Client client1 = extractS3Client(fs1); - S3Client client2 = extractS3Client(fs2); - S3Client client3 = extractS3Client(fs3); - - assertThat(client1).isSameAs(client2); - assertThat(client2).isSameAs(client3); - - // Verify cache contains only ONE entry - Map, S3Client> clientsCache = getClientsCache(); - assertThat(clientsCache).hasSize(1); - } - - @Test - void testDifferentClientsForDifferentCredentials() - throws Exception - { - // Arrange: Different credentials - ConnectorIdentity identity1 = ConnectorIdentity.ofUser("user1"); - ConnectorIdentity identity2 = ConnectorIdentity.ofUser("user2"); - Location location = Location.of("s3://test-bucket/path"); - - S3SecurityMappingResult mapping1 = new S3SecurityMappingResult( - Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.of("https://s3.us-east-1.amazonaws.com"), - Optional.of("us-east-1"), - Optional.of(true)); - - S3SecurityMappingResult mapping2 = new S3SecurityMappingResult( - Optional.of(AwsSessionCredentials.create("access2", "secret2", "token2")), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.of("https://s3.us-west-2.amazonaws.com"), - Optional.of("us-west-2"), - Optional.of(false)); - - TrinoFileSystemFactory factory = loader.apply(location); - - // Act: Create file systems with different credentials - credentialsMapper.setMapping(mapping1); - TrinoFileSystem fs1 = factory.create(identity1); - - credentialsMapper.setMapping(mapping2); - TrinoFileSystem fs2 = factory.create(identity2); - - // Assert: Different S3Clients are created - S3Client client1 = extractS3Client(fs1); - S3Client client2 = extractS3Client(fs2); - - assertThat(client1).isNotSameAs(client2); - - // Verify cache contains TWO entries - Map, S3Client> clientsCache = getClientsCache(); - assertThat(clientsCache).hasSize(2); - } - - @Test - void testDifferentClientsForDifferentRegions() - throws Exception - { - // Arrange: Same credentials, different regions - ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); - Location location = Location.of("s3://test-bucket/path"); - - S3SecurityMappingResult mapping1 = new S3SecurityMappingResult( - Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.of("us-east-1"), - Optional.empty()); - - S3SecurityMappingResult mapping2 = new S3SecurityMappingResult( - Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.of("eu-west-1"), - Optional.empty()); - - TrinoFileSystemFactory factory = loader.apply(location); - - // Act: Create file systems with different regions - credentialsMapper.setMapping(mapping1); - TrinoFileSystem fs1 = factory.create(identity); - - credentialsMapper.setMapping(mapping2); - TrinoFileSystem fs2 = factory.create(identity); - - // Assert: Different S3Clients for different regions - S3Client client1 = extractS3Client(fs1); - S3Client client2 = extractS3Client(fs2); - - assertThat(client1).isNotSameAs(client2); - - // Verify cache contains TWO entries - Map, S3Client> clientsCache = getClientsCache(); - assertThat(clientsCache).hasSize(2); - } - - @Test - void testDifferentClientsForDifferentEndpoints() - throws Exception - { - // Arrange: Same credentials, different endpoints - ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); - Location location = Location.of("s3://test-bucket/path"); - - S3SecurityMappingResult mapping1 = new S3SecurityMappingResult( - Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.of("https://s3.us-east-1.amazonaws.com"), - Optional.of("us-east-1"), - Optional.empty()); - - S3SecurityMappingResult mapping2 = new S3SecurityMappingResult( - Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.of("https://custom-s3-endpoint.example.com"), - Optional.of("us-east-1"), - Optional.empty()); - - TrinoFileSystemFactory factory = loader.apply(location); - - // Act: Create file systems with different endpoints - credentialsMapper.setMapping(mapping1); - TrinoFileSystem fs1 = factory.create(identity); - - credentialsMapper.setMapping(mapping2); - TrinoFileSystem fs2 = factory.create(identity); - - // Assert: Different S3Clients for different endpoints - S3Client client1 = extractS3Client(fs1); - S3Client client2 = extractS3Client(fs2); - - assertThat(client1).isNotSameAs(client2); - - // Verify cache contains TWO entries - Map, S3Client> clientsCache = getClientsCache(); - assertThat(clientsCache).hasSize(2); - } - - @Test - void testDifferentClientsForDifferentCrossRegionAccess() - throws Exception - { - // Arrange: Same credentials, different cross-region settings - ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); - Location location = Location.of("s3://test-bucket/path"); - - S3SecurityMappingResult mapping1 = new S3SecurityMappingResult( - Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.of("us-east-1"), - Optional.of(true)); - - S3SecurityMappingResult mapping2 = new S3SecurityMappingResult( - Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.of("us-east-1"), - Optional.of(false)); - - TrinoFileSystemFactory factory = loader.apply(location); - - // Act: Create file systems with different cross-region settings - credentialsMapper.setMapping(mapping1); - TrinoFileSystem fs1 = factory.create(identity); - - credentialsMapper.setMapping(mapping2); - TrinoFileSystem fs2 = factory.create(identity); - - // Assert: Different S3Clients for different cross-region settings - S3Client client1 = extractS3Client(fs1); - S3Client client2 = extractS3Client(fs2); - - assertThat(client1).isNotSameAs(client2); - - // Verify cache contains TWO entries - Map, S3Client> clientsCache = getClientsCache(); - assertThat(clientsCache).hasSize(2); - } - - @Test - void testEmptyMappingUsesClusterDefaults() - throws Exception - { - // Arrange: No credentials mapping - ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); - Location location = Location.of("s3://test-bucket/path"); - - credentialsMapper.setMapping(null); // Return empty - - // Act: Create file system - TrinoFileSystemFactory factory = loader.apply(location); - TrinoFileSystem fs = factory.create(identity); - - // Assert: Client is created with cluster defaults - S3Client client = extractS3Client(fs); - assertThat(client).isNotNull(); - - // Verify cache contains ONE entry for empty mapping - Map, S3Client> clientsCache = getClientsCache(); - assertThat(clientsCache).hasSize(1); - } - - @Test - void testConcurrentClientCreationIsSafe() - throws Exception - { - // Arrange: Multiple threads creating clients with same credentials - ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); - Location location = Location.of("s3://test-bucket/path"); - - S3SecurityMappingResult mapping = new S3SecurityMappingResult( - Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.of("https://s3.us-east-1.amazonaws.com"), - Optional.of("us-east-1"), - Optional.of(true)); - - credentialsMapper.setMapping(mapping); - - int threadCount = 10; - CountDownLatch startLatch = new CountDownLatch(1); - CountDownLatch doneLatch = new CountDownLatch(threadCount); - ExecutorService executor = Executors.newFixedThreadPool(threadCount); - AtomicInteger clientCount = new AtomicInteger(0); - - TrinoFileSystemFactory factory = loader.apply(location); - - // Act: Create clients concurrently - for (int i = 0; i < threadCount; i++) { - executor.submit(() -> { - try { - startLatch.await(); // Wait for all threads to be ready - TrinoFileSystem fs = factory.create(identity); - S3Client client = extractS3Client(fs); - if (client != null) { - clientCount.incrementAndGet(); - } - } - catch (Exception e) { - throw new RuntimeException(e); - } - finally { - doneLatch.countDown(); - } - }); - } - - startLatch.countDown(); // Start all threads at once - doneLatch.await(10, TimeUnit.SECONDS); - executor.shutdown(); - - // Assert: All threads succeeded - assertThat(clientCount.get()).isEqualTo(threadCount); - - // Assert: Only ONE client created despite concurrent access - Map, S3Client> clientsCache = getClientsCache(); - assertThat(clientsCache).hasSize(1); - } - - @Test - void testClientCacheGrowsBounded() - throws Exception - { - // Arrange: Create clients with many different credential combinations - Location location = Location.of("s3://test-bucket/path"); - TrinoFileSystemFactory factory = loader.apply(location); - - int uniqueCredentialSets = 5; - - // Act: Create file systems with different credentials - for (int i = 0; i < uniqueCredentialSets; i++) { - ConnectorIdentity identity = ConnectorIdentity.ofUser("user" + i); - - S3SecurityMappingResult mapping = new S3SecurityMappingResult( - Optional.of(AwsSessionCredentials.create("access" + i, "secret" + i, "token" + i)), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.of("https://s3.us-east-1.amazonaws.com"), - Optional.of("us-east-1"), - Optional.of(true)); - - credentialsMapper.setMapping(mapping); - factory.create(identity); - } - - // Assert: Cache size equals number of unique credential sets (bounded growth) - Map, S3Client> clientsCache = getClientsCache(); - assertThat(clientsCache).hasSize(uniqueCredentialSets); - } - - @Test - void testKmsKeyIdIsAppliedToContext() - throws Exception - { - // Arrange: Mapping with KMS key ID - ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); - Location location = Location.of("s3://test-bucket/path"); - - S3SecurityMappingResult mapping = new S3SecurityMappingResult( - Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), - Optional.empty(), - Optional.empty(), - Optional.of("arn:aws:kms:us-east-1:123456789012:key/test-key"), - Optional.empty(), - Optional.empty(), - Optional.of("us-east-1"), - Optional.empty()); - - credentialsMapper.setMapping(mapping); - - // Act: Create file system - TrinoFileSystemFactory factory = loader.apply(location); - TrinoFileSystem fs = factory.create(identity); - - // Assert: File system created successfully (KMS key would be validated at runtime) - assertThat(fs).isNotNull(); - } - - @Test - void testSseCustomerKeyIsAppliedToContext() - throws Exception - { - // Arrange: Mapping with SSE customer key - ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); - Location location = Location.of("s3://test-bucket/path"); - - S3SecurityMappingResult mapping = new S3SecurityMappingResult( - Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.of("YmFzZTY0LWVuY29kZWQta2V5"), // base64 encoded key - Optional.empty(), - Optional.of("us-east-1"), - Optional.empty()); - - credentialsMapper.setMapping(mapping); - - // Act: Create file system - TrinoFileSystemFactory factory = loader.apply(location); - TrinoFileSystem fs = factory.create(identity); - - // Assert: File system created successfully - assertThat(fs).isNotNull(); - } - - @Test - void testComplexCredentialCombination() - throws Exception - { - // Arrange: Mapping with multiple fields populated - ConnectorIdentity identity = ConnectorIdentity.ofUser("test-user"); - Location location = Location.of("s3://test-bucket/path"); - - S3SecurityMappingResult mapping = new S3SecurityMappingResult( - Optional.of(AwsSessionCredentials.create("access1", "secret1", "token1")), - Optional.of("arn:aws:iam::123456789012:role/test-role"), - Optional.of("test-session"), - Optional.empty(), - Optional.empty(), - Optional.of("https://custom-endpoint.example.com"), - Optional.of("eu-west-1"), - Optional.of(true)); - - credentialsMapper.setMapping(mapping); - - // Act: Create file system - TrinoFileSystemFactory factory = loader.apply(location); - TrinoFileSystem fs = factory.create(identity); - - // Assert: File system created successfully - assertThat(fs).isNotNull(); - - S3Client client = extractS3Client(fs); - assertThat(client).isNotNull(); - } - - // Helper methods - - private S3Client extractS3Client(TrinoFileSystem fileSystem) - throws Exception - { - Field clientField = fileSystem.getClass().getDeclaredField("client"); - clientField.setAccessible(true); - return (S3Client) clientField.get(fileSystem); - } - - @SuppressWarnings("unchecked") - private Map, S3Client> getClientsCache() - throws Exception - { - Field clientsField = loader.getClass().getDeclaredField("clients"); - clientsField.setAccessible(true); - return (ConcurrentHashMap, S3Client>) clientsField.get(loader); - } - - // Test implementation of S3CredentialsMapper - private static class TestCredentialsMapper - implements S3CredentialsMapper - { - private S3SecurityMappingResult mapping; - - public void setMapping(S3SecurityMappingResult mapping) - { - this.mapping = mapping; - } - - @Override - public Optional getMapping(ConnectorIdentity identity, Location location) - { - return Optional.ofNullable(mapping); - } - } -} diff --git a/lib/trino-filesystem/src/main/java/io/trino/filesystem/s3/S3FileSystemConstants.java b/lib/trino-filesystem/src/main/java/io/trino/filesystem/s3/S3FileSystemConstants.java index e0e52c1186f7..1155026292a6 100644 --- a/lib/trino-filesystem/src/main/java/io/trino/filesystem/s3/S3FileSystemConstants.java +++ b/lib/trino-filesystem/src/main/java/io/trino/filesystem/s3/S3FileSystemConstants.java @@ -18,9 +18,6 @@ public final class S3FileSystemConstants public static final String EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY = "internal$s3_aws_access_key"; public static final String EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY = "internal$s3_aws_secret_key"; public static final String EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY = "internal$s3_aws_session_token"; - public static final String EXTRA_CREDENTIALS_REGION_PROPERTY = "internal$s3_region"; - public static final String EXTRA_CREDENTIALS_ENDPOINT_PROPERTY = "internal$s3_endpoint"; - public static final String EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY = "internal$s3_cross_region_access_enabled"; private S3FileSystemConstants() {} } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogModule.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogModule.java index 8178a7c23ea9..d6709e718739 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogModule.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogModule.java @@ -44,11 +44,8 @@ protected void setup(Binder binder) IcebergConfig icebergConfig = buildConfigObject(IcebergConfig.class); IcebergRestCatalogConfig restCatalogConfig = buildConfigObject(IcebergRestCatalogConfig.class); - if (restCatalogConfig.isVendedCredentialsEnabled()) { - if (icebergConfig.isRegisterTableProcedureEnabled()) { - throw new TrinoException(NOT_SUPPORTED, "Using the `register_table` procedure with vended credentials is currently not supported"); - } - install(new IcebergVendedCredentialsModule()); + if (restCatalogConfig.isVendedCredentialsEnabled() && icebergConfig.isRegisterTableProcedureEnabled()) { + throw new TrinoException(NOT_SUPPORTED, "Using the `register_table` procedure with vended credentials is currently not supported"); } } } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergVendedCredentialsMapper.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergVendedCredentialsMapper.java deleted file mode 100644 index 965e7b3b8646..000000000000 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergVendedCredentialsMapper.java +++ /dev/null @@ -1,89 +0,0 @@ -/* - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.trino.plugin.iceberg.catalog.rest; - -import com.google.inject.Inject; -import io.trino.filesystem.Location; -import io.trino.filesystem.s3.S3CredentialsMapper; -import io.trino.filesystem.s3.S3FileSystemConfig; -import io.trino.filesystem.s3.S3SecurityMappingResult; -import io.trino.spi.security.ConnectorIdentity; -import software.amazon.awssdk.auth.credentials.AwsSessionCredentials; - -import java.util.Optional; - -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_ENDPOINT_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_REGION_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY; -import static java.util.Objects.requireNonNull; - -final class IcebergVendedCredentialsMapper - implements S3CredentialsMapper -{ - private final Optional staticRegion; - private final Optional staticEndpoint; - private final boolean staticCrossRegionAccessEnabled; - - @Inject - public IcebergVendedCredentialsMapper(S3FileSystemConfig config) - { - requireNonNull(config, "config is null"); - this.staticRegion = Optional.ofNullable(config.getRegion()); - this.staticEndpoint = Optional.ofNullable(config.getEndpoint()); - this.staticCrossRegionAccessEnabled = config.isCrossRegionAccessEnabled(); - } - - @Override - public Optional getMapping(ConnectorIdentity identity, Location location) - { - String accessKey = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY); - String secretKey = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY); - String sessionToken = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY); - - if (accessKey == null || secretKey == null || sessionToken == null) { - return Optional.empty(); - } - - String vendedRegion = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_REGION_PROPERTY); - String vendedEndpoint = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY); - String vendedCrossRegionAccess = identity.getExtraCredentials().get(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY); - - Optional region = Optional.ofNullable(vendedRegion).or(() -> staticRegion); - Optional endpoint = Optional.ofNullable(vendedEndpoint).or(() -> staticEndpoint); - - Optional crossRegionAccessEnabled; - if (staticCrossRegionAccessEnabled) { - crossRegionAccessEnabled = Optional.of(true); - } - else if (vendedCrossRegionAccess != null) { - crossRegionAccessEnabled = Optional.of(Boolean.parseBoolean(vendedCrossRegionAccess)); - } - else { - crossRegionAccessEnabled = Optional.empty(); - } - - return Optional.of(new S3SecurityMappingResult( - Optional.of(AwsSessionCredentials.create(accessKey, secretKey, sessionToken)), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - endpoint, - region, - crossRegionAccessEnabled)); - } -} diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergVendedCredentialsModule.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergVendedCredentialsModule.java deleted file mode 100644 index 9c7e1396803a..000000000000 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergVendedCredentialsModule.java +++ /dev/null @@ -1,32 +0,0 @@ -/* - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.trino.plugin.iceberg.catalog.rest; - -import com.google.inject.Binder; -import com.google.inject.Scopes; -import io.airlift.configuration.AbstractConfigurationAwareModule; -import io.trino.filesystem.s3.S3CredentialsMapper; - -import static com.google.inject.multibindings.OptionalBinder.newOptionalBinder; - -public class IcebergVendedCredentialsModule - extends AbstractConfigurationAwareModule -{ - @Override - protected void setup(Binder binder) - { - binder.bind(IcebergVendedCredentialsMapper.class).in(Scopes.SINGLETON); - newOptionalBinder(binder, S3CredentialsMapper.class).setBinding().to(IcebergVendedCredentialsMapper.class).in(Scopes.SINGLETON); - } -} diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java index 321597effc74..89deaa4d143e 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java @@ -20,17 +20,19 @@ import com.google.inject.Inject; import io.airlift.units.Duration; import io.trino.cache.EvictableCacheBuilder; +import io.trino.plugin.iceberg.ForIcebergFileDelete; import io.trino.plugin.iceberg.IcebergConfig; import io.trino.plugin.iceberg.IcebergFileSystemFactory; import io.trino.plugin.iceberg.catalog.TrinoCatalog; import io.trino.plugin.iceberg.catalog.TrinoCatalogFactory; import io.trino.plugin.iceberg.catalog.rest.IcebergRestCatalogConfig.Security; import io.trino.plugin.iceberg.catalog.rest.IcebergRestCatalogConfig.SessionType; -import io.trino.plugin.iceberg.fileio.ForwardingFileIoFactory; +import io.trino.plugin.iceberg.fileio.ForwardingFileIo; import io.trino.spi.NodeVersion; import io.trino.spi.catalog.CatalogName; import io.trino.spi.security.ConnectorIdentity; import io.trino.spi.type.TypeManager; +import jakarta.annotation.PreDestroy; import org.apache.iceberg.CatalogProperties; import org.apache.iceberg.catalog.Namespace; import org.apache.iceberg.catalog.TableIdentifier; @@ -42,10 +44,12 @@ import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.concurrent.ExecutorService; 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.CatalogProperties.FILE_IO_IMPL; import static org.apache.iceberg.rest.auth.OAuth2Properties.CREDENTIAL; import static org.apache.iceberg.rest.auth.OAuth2Properties.TOKEN; @@ -53,7 +57,7 @@ public class TrinoIcebergRestCatalogFactory implements TrinoCatalogFactory { private final IcebergFileSystemFactory fileSystemFactory; - private final ForwardingFileIoFactory fileIoFactory; + private final ExecutorService deleteExecutor; private final CatalogName catalogName; private final String trinoVersion; private final URI serverUri; @@ -80,7 +84,7 @@ public class TrinoIcebergRestCatalogFactory @Inject public TrinoIcebergRestCatalogFactory( IcebergFileSystemFactory fileSystemFactory, - ForwardingFileIoFactory fileIoFactory, + @ForIcebergFileDelete ExecutorService deleteExecutor, CatalogName catalogName, IcebergRestCatalogConfig restConfig, SecurityProperties securityProperties, @@ -89,7 +93,7 @@ public TrinoIcebergRestCatalogFactory( NodeVersion nodeVersion) { this.fileSystemFactory = requireNonNull(fileSystemFactory, "fileSystemFactory is null"); - this.fileIoFactory = requireNonNull(fileIoFactory, "fileIoFactory is null"); + this.deleteExecutor = requireNonNull(deleteExecutor, "deleteExecutor is null"); this.catalogName = requireNonNull(catalogName, "catalogName is null"); this.trinoVersion = requireNonNull(nodeVersion, "nodeVersion is null").toString(); requireNonNull(restConfig, "restConfig is null"); @@ -119,6 +123,12 @@ public TrinoIcebergRestCatalogFactory( .build(); } + @PreDestroy + public void shutdown() + { + ForwardingFileIo.deregisterContext(catalogName.toString()); + } + @Override public synchronized TrinoCatalog create(ConnectorIdentity identity) { @@ -140,17 +150,19 @@ public synchronized TrinoCatalog create(ConnectorIdentity identity) properties.put("header.X-Iceberg-Access-Delegation", "vended-credentials"); } + properties.put(FILE_IO_IMPL, ForwardingFileIo.class.getName()); + properties.put(ForwardingFileIo.TRINO_CATALOG_NAME, catalogName.toString()); + ForwardingFileIo.registerContext(catalogName.toString(), fileSystemFactory, deleteExecutor); + + // ioBuilder is null so RESTSessionCatalog creates FileIO via reflection, + // which allows SupportsStorageCredentials.setCredentials() to be called + // with vended credentials before FileIO.initialize() RESTSessionCatalog icebergCatalogInstance = new RESTSessionCatalog( config -> HTTPClient.builder(config) .uri(config.get(CatalogProperties.URI)) .withHeaders(RESTUtil.configHeaders(config)) .build(), - (context, config) -> { - ConnectorIdentity currentIdentity = (context.wrappedIdentity() != null) - ? ((ConnectorIdentity) context.wrappedIdentity()) - : ConnectorIdentity.ofUser("fake"); - return fileIoFactory.create(fileSystemFactory.create(currentIdentity, config), true, config); - }); + null); icebergCatalogInstance.initialize(catalogName.toString(), properties.buildOrThrow()); icebergCatalog = icebergCatalogInstance; diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java index 600b4a59aa39..1009049f9d23 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java @@ -33,6 +33,7 @@ import io.trino.plugin.iceberg.catalog.TrinoCatalog; import io.trino.plugin.iceberg.catalog.rest.IcebergRestCatalogConfig.Security; import io.trino.plugin.iceberg.catalog.rest.IcebergRestCatalogConfig.SessionType; +import io.trino.plugin.iceberg.fileio.ForwardingFileIo; import io.trino.spi.TrinoException; import io.trino.spi.catalog.CatalogName; import io.trino.spi.connector.CatalogSchemaTableName; @@ -176,8 +177,8 @@ public Optional getNamespaceSeparator() @Override public boolean namespaceExists(ConnectorSession session, String namespace) { - try { - return restSessionCatalog.namespaceExists(convert(session), toRemoteNamespace(session, toNamespace(namespace))); + try (var scope = convert(session)) { + return restSessionCatalog.namespaceExists(scope.context(), toRemoteNamespace(session, toNamespace(namespace))); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to check namespace '%s'".formatted(namespace), e); @@ -190,8 +191,8 @@ public List listNamespaces(ConnectorSession session) if (nestedNamespaceEnabled) { return collectNamespaces(session, Namespace.empty()); } - try { - return restSessionCatalog.listNamespaces(convert(session)).stream() + try (var scope = convert(session)) { + return restSessionCatalog.listNamespaces(scope.context()).stream() .map(this::toSchemaName) .collect(toImmutableList()); } @@ -202,8 +203,8 @@ public List listNamespaces(ConnectorSession session) private List collectNamespaces(ConnectorSession session, Namespace parentNamespace) { - try { - return restSessionCatalog.listNamespaces(convert(session), parentNamespace).stream() + try (var scope = convert(session)) { + return restSessionCatalog.listNamespaces(scope.context(), parentNamespace).stream() .flatMap(childNamespace -> Stream.concat( Stream.of(childNamespace.toString()), collectNamespaces(session, childNamespace).stream())) @@ -217,8 +218,8 @@ private List collectNamespaces(ConnectorSession session, Namespace paren @Override public void dropNamespace(ConnectorSession session, String namespace) { - try { - restSessionCatalog.dropNamespace(convert(session), toRemoteNamespace(session, toNamespace(namespace))); + try (var scope = convert(session)) { + restSessionCatalog.dropNamespace(scope.context(), toRemoteNamespace(session, toNamespace(namespace))); } catch (NoSuchNamespaceException e) { throw new SchemaNotFoundException(namespace); @@ -234,9 +235,9 @@ public void dropNamespace(ConnectorSession session, String namespace) @Override public Map loadNamespaceMetadata(ConnectorSession session, String namespace) { - try { + try (var scope = convert(session)) { // Return immutable metadata as direct modifications will not be reflected on the namespace - return restSessionCatalog.loadNamespaceMetadata(convert(session), toRemoteNamespace(session, toNamespace(namespace))).entrySet().stream() + return restSessionCatalog.loadNamespaceMetadata(scope.context(), toRemoteNamespace(session, toNamespace(namespace))).entrySet().stream() .filter(property -> SUPPORTED_SCHEMA_PROPERTIES.contains(property.getKey())) .collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)); } @@ -258,9 +259,9 @@ public Optional getNamespacePrincipal(ConnectorSession session, @Override public void createNamespace(ConnectorSession session, String namespace, Map properties, TrinoPrincipal owner) { - try { + try (var scope = convert(session)) { restSessionCatalog.createNamespace( - convert(session), + scope.context(), toNamespace(namespace), Maps.transformValues(properties, property -> { if (property instanceof String stringProperty) { @@ -289,57 +290,59 @@ public void renameNamespace(ConnectorSession session, String source, String targ @Override public List listTables(ConnectorSession session, Optional namespace) { - SessionContext sessionContext = convert(session); - List namespaces = listNamespaces(session, namespace); + try (var scope = convert(session)) { + List namespaces = listNamespaces(session, namespace); - ImmutableList.Builder tables = ImmutableList.builder(); - for (Namespace restNamespace : namespaces) { - listTableIdentifiers(restNamespace, () -> { - try { - return restSessionCatalog.listTables(sessionContext, toRemoteNamespace(session, restNamespace)); - } - catch (RESTException e) { - throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list tables", e); - } - }).stream() - .map(id -> new TableInfo(SchemaTableName.schemaTableName(toSchemaName(id.namespace()), id.name()), TableInfo.ExtendedRelationType.TABLE)) - .forEach(tables::add); - if (viewEndpointsEnabled) { + ImmutableList.Builder tables = ImmutableList.builder(); + for (Namespace restNamespace : namespaces) { listTableIdentifiers(restNamespace, () -> { try { - return restSessionCatalog.listViews(sessionContext, toRemoteNamespace(session, restNamespace)); + return restSessionCatalog.listTables(scope.context(), toRemoteNamespace(session, restNamespace)); } catch (RESTException e) { - throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list views", e); + throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list tables", e); } }).stream() - .map(id -> new TableInfo(SchemaTableName.schemaTableName(toSchemaName(id.namespace()), id.name()), TableInfo.ExtendedRelationType.OTHER_VIEW)) + .map(id -> new TableInfo(SchemaTableName.schemaTableName(toSchemaName(id.namespace()), id.name()), TableInfo.ExtendedRelationType.TABLE)) .forEach(tables::add); + if (viewEndpointsEnabled) { + listTableIdentifiers(restNamespace, () -> { + try { + return restSessionCatalog.listViews(scope.context(), toRemoteNamespace(session, restNamespace)); + } + catch (RESTException e) { + throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list views", e); + } + }).stream() + .map(id -> new TableInfo(SchemaTableName.schemaTableName(toSchemaName(id.namespace()), id.name()), TableInfo.ExtendedRelationType.OTHER_VIEW)) + .forEach(tables::add); + } } + return tables.build(); } - return tables.build(); } @Override public List listIcebergTables(ConnectorSession session, Optional namespace) { - SessionContext sessionContext = convert(session); - List namespaces = listNamespaces(session, namespace); + try (var scope = convert(session)) { + List namespaces = listNamespaces(session, namespace); - ImmutableList.Builder tables = ImmutableList.builder(); - for (Namespace restNamespace : namespaces) { - listTableIdentifiers(restNamespace, () -> { - try { - return restSessionCatalog.listTables(sessionContext, toRemoteNamespace(session, restNamespace)); - } - catch (RESTException e) { - throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list tables", e); - } - }).stream() - .map(id -> SchemaTableName.schemaTableName(toSchemaName(id.namespace()), id.name())) - .forEach(tables::add); + ImmutableList.Builder tables = ImmutableList.builder(); + for (Namespace restNamespace : namespaces) { + listTableIdentifiers(restNamespace, () -> { + try { + return restSessionCatalog.listTables(scope.context(), toRemoteNamespace(session, restNamespace)); + } + catch (RESTException e) { + throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list tables", e); + } + }).stream() + .map(id -> SchemaTableName.schemaTableName(toSchemaName(id.namespace()), id.name())) + .forEach(tables::add); + } + return tables.build(); } - return tables.build(); } @Override @@ -349,23 +352,24 @@ public List listViews(ConnectorSession session, Optional namespaces = listNamespaces(session, namespace); + try (var scope = convert(session)) { + List namespaces = listNamespaces(session, namespace); - ImmutableList.Builder viewNames = ImmutableList.builder(); - for (Namespace restNamespace : namespaces) { - listTableIdentifiers(restNamespace, () -> { - try { - return restSessionCatalog.listViews(sessionContext, toRemoteNamespace(session, restNamespace)); - } - catch (RESTException e) { - throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list views", e); - } - }).stream() - .map(id -> SchemaTableName.schemaTableName(id.namespace().toString(), id.name())) - .forEach(viewNames::add); + ImmutableList.Builder viewNames = ImmutableList.builder(); + for (Namespace restNamespace : namespaces) { + listTableIdentifiers(restNamespace, () -> { + try { + return restSessionCatalog.listViews(scope.context(), toRemoteNamespace(session, restNamespace)); + } + catch (RESTException e) { + throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list views", e); + } + }).stream() + .map(id -> SchemaTableName.schemaTableName(id.namespace().toString(), id.name())) + .forEach(viewNames::add); + } + return viewNames.build(); } - return viewNames.build(); } private static List listTableIdentifiers(Namespace restNamespace, Supplier> tableIdentifiersProvider) @@ -415,8 +419,8 @@ public Transaction newCreateTableTransaction( Optional location, Map properties) { - try { - Catalog.TableBuilder tableBuilder = restSessionCatalog.buildTable(convert(session), toRemoteTable(session, schemaTableName, true), schema) + try (var scope = convert(session)) { + Catalog.TableBuilder tableBuilder = restSessionCatalog.buildTable(scope.context(), toRemoteTable(session, schemaTableName, true), schema) .withPartitionSpec(partitionSpec) .withSortOrder(sortOrder) .withProperties(properties); @@ -441,8 +445,8 @@ public Transaction newCreateOrReplaceTableTransaction( String location, Map properties) { - try { - return restSessionCatalog.buildTable(convert(session), toRemoteTable(session, schemaTableName, true), schema) + try (var scope = convert(session)) { + return restSessionCatalog.buildTable(scope.context(), toRemoteTable(session, schemaTableName, true), schema) .withPartitionSpec(partitionSpec) .withSortOrder(sortOrder) .withLocation(location) @@ -458,8 +462,8 @@ public Transaction newCreateOrReplaceTableTransaction( public void registerTable(ConnectorSession session, SchemaTableName tableName, TableMetadata tableMetadata) { TableIdentifier tableIdentifier = TableIdentifier.of(toRemoteNamespace(session, toNamespace(tableName.getSchemaName())), tableName.getTableName()); - try { - restSessionCatalog.registerTable(convert(session), tableIdentifier, tableMetadata.metadataFileLocation()); + try (var scope = convert(session)) { + restSessionCatalog.registerTable(scope.context(), tableIdentifier, tableMetadata.metadataFileLocation()); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to register table '%s'".formatted(tableName.getTableName()), e); @@ -469,8 +473,8 @@ public void registerTable(ConnectorSession session, SchemaTableName tableName, T @Override public void unregisterTable(ConnectorSession session, SchemaTableName tableName) { - try { - if (!restSessionCatalog.dropTable(convert(session), toRemoteTable(session, tableName, true))) { + try (var scope = convert(session)) { + if (!restSessionCatalog.dropTable(scope.context(), toRemoteTable(session, tableName, true))) { throw new TableNotFoundException(tableName); } } @@ -522,8 +526,8 @@ private static void deleteTableDirectory(TrinoFileSystem fileSystem, SchemaTable private void purgeTable(ConnectorSession session, SchemaTableName schemaTableName) { - try { - if (!restSessionCatalog.purgeTable(convert(session), toRemoteTable(session, schemaTableName, true))) { + try (var scope = convert(session)) { + if (!restSessionCatalog.purgeTable(scope.context(), toRemoteTable(session, schemaTableName, true))) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to drop table '%s'".formatted(schemaTableName)); } } @@ -543,8 +547,8 @@ public void dropCorruptedTable(ConnectorSession session, SchemaTableName schemaT @Override public void renameTable(ConnectorSession session, SchemaTableName from, SchemaTableName to) { - try { - restSessionCatalog.renameTable(convert(session), toRemoteTable(session, from, true), toRemoteTable(session, to, true)); + try (var scope = convert(session)) { + restSessionCatalog.renameTable(scope.context(), toRemoteTable(session, from, true), toRemoteTable(session, to, true)); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, format("Failed to rename table %s to %s", from, to), e); @@ -557,14 +561,14 @@ public void renameTable(ConnectorSession session, SchemaTableName from, SchemaTa public BaseTable loadTable(ConnectorSession session, SchemaTableName schemaTableName) { Namespace namespace = toNamespace(schemaTableName.getSchemaName()); - try { + try (var scope = convert(session)) { return uncheckedCacheGet( tableCache, schemaTableName, () -> { BaseTable baseTable; try { - baseTable = (BaseTable) restSessionCatalog.loadTable(convert(session), toRemoteObject(session, schemaTableName)); + baseTable = (BaseTable) restSessionCatalog.loadTable(scope.context(), toRemoteObject(session, schemaTableName)); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to load table '%s'".formatted(schemaTableName.getTableName()), e); @@ -608,8 +612,8 @@ public Map> tryGetColumnMetadata(Connector public void updateTableComment(ConnectorSession session, SchemaTableName schemaTableName, Optional comment) { Table icebergTable; - try { - icebergTable = restSessionCatalog.loadTable(convert(session), toRemoteTable(session, schemaTableName, true)); + try (var scope = convert(session)) { + icebergTable = restSessionCatalog.loadTable(scope.context(), toRemoteTable(session, schemaTableName, true)); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to load table '%s'".formatted(schemaTableName.getTableName()), e); @@ -661,14 +665,14 @@ public void createView(ConnectorSession session, SchemaTableName schemaViewName, definition.getOwner().ifPresent(owner -> properties.put(ICEBERG_VIEW_RUN_AS_OWNER, owner)); definition.getComment().ifPresent(comment -> properties.put(COMMENT, comment)); Schema schema = IcebergUtil.schemaFromViewColumns(typeManager, definition.getColumns()); - ViewBuilder viewBuilder = restSessionCatalog.buildView(convert(session), toRemoteView(session, schemaViewName, true)); - viewBuilder = viewBuilder.withSchema(schema) - .withQuery("trino", definition.getOriginalSql()) - .withDefaultNamespace(toRemoteNamespace(session, toNamespace(schemaViewName.getSchemaName()))) - .withDefaultCatalog(definition.getCatalog().orElse(null)) - .withProperties(properties.buildOrThrow()) - .withLocation(defaultTableLocation(session, schemaViewName)); - try { + try (var scope = convert(session)) { + ViewBuilder viewBuilder = restSessionCatalog.buildView(scope.context(), toRemoteView(session, schemaViewName, true)); + viewBuilder = viewBuilder.withSchema(schema) + .withQuery("trino", definition.getOriginalSql()) + .withDefaultNamespace(toRemoteNamespace(session, toNamespace(schemaViewName.getSchemaName()))) + .withDefaultCatalog(definition.getCatalog().orElse(null)) + .withProperties(properties.buildOrThrow()) + .withLocation(defaultTableLocation(session, schemaViewName)); if (replace) { viewBuilder.createOrReplace(); } @@ -684,8 +688,8 @@ public void createView(ConnectorSession session, SchemaTableName schemaViewName, @Override public void renameView(ConnectorSession session, SchemaTableName source, SchemaTableName target) { - try { - restSessionCatalog.renameView(convert(session), toRemoteView(session, source, true), toRemoteView(session, target, true)); + try (var scope = convert(session)) { + restSessionCatalog.renameView(scope.context(), toRemoteView(session, source, true), toRemoteView(session, target, true)); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to rename view '%s' to '%s'".formatted(source, target), e); @@ -702,8 +706,8 @@ public void setViewPrincipal(ConnectorSession session, SchemaTableName schemaVie @Override public void dropView(ConnectorSession session, SchemaTableName schemaViewName) { - try { - restSessionCatalog.dropView(convert(session), toRemoteView(session, schemaViewName, true)); + try (var scope = convert(session)) { + restSessionCatalog.dropView(scope.context(), toRemoteView(session, schemaViewName, true)); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to drop view '%s'".formatted(schemaViewName.getTableName()), e); @@ -714,32 +718,33 @@ public void dropView(ConnectorSession session, SchemaTableName schemaViewName) @Override public Map getViews(ConnectorSession session, Optional namespace) { - SessionContext sessionContext = convert(session); - ImmutableMap.Builder views = ImmutableMap.builder(); - for (Namespace restNamespace : listNamespaces(session, namespace)) { - List restViews; - try { - restViews = restSessionCatalog.listViews(sessionContext, toRemoteNamespace(session, restNamespace)); - } - catch (RESTException e) { - throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list views", e); - } - for (TableIdentifier restView : restViews) { - SchemaTableName schemaTableName = SchemaTableName.schemaTableName(restView.namespace().toString(), restView.name()); + try (var scope = convert(session)) { + ImmutableMap.Builder views = ImmutableMap.builder(); + for (Namespace restNamespace : listNamespaces(session, namespace)) { + List restViews; try { - getView(session, schemaTableName).ifPresent(view -> views.put(schemaTableName, view)); + restViews = restSessionCatalog.listViews(scope.context(), toRemoteNamespace(session, restNamespace)); } - catch (TrinoException e) { - if (e.getErrorCode().equals(ICEBERG_UNSUPPORTED_VIEW_DIALECT.toErrorCode())) { - log.debug(e, "Skip unsupported view dialect: %s", schemaTableName); - continue; + catch (RESTException e) { + throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list views", e); + } + for (TableIdentifier restView : restViews) { + SchemaTableName schemaTableName = SchemaTableName.schemaTableName(restView.namespace().toString(), restView.name()); + try { + getView(session, schemaTableName).ifPresent(view -> views.put(schemaTableName, view)); + } + catch (TrinoException e) { + if (e.getErrorCode().equals(ICEBERG_UNSUPPORTED_VIEW_DIALECT.toErrorCode())) { + log.debug(e, "Skip unsupported view dialect: %s", schemaTableName); + continue; + } + throw e; } - throw e; } } - } - return views.buildOrThrow(); + return views.buildOrThrow(); + } } @Override @@ -771,8 +776,8 @@ private Optional getIcebergView(ConnectorSession session, SchemaTableName return Optional.empty(); } - try { - return Optional.of(restSessionCatalog.loadView(convert(session), toRemoteView(session, viewName, getCached))); + try (var scope = convert(session)) { + return Optional.of(restSessionCatalog.loadView(scope.context(), toRemoteView(session, viewName, getCached))); } catch (NoSuchViewException e) { return Optional.empty(); @@ -876,9 +881,10 @@ public void updateViewColumnComment(ConnectorSession session, SchemaTableName sc replaceViewVersion.commit(); } - private SessionCatalog.SessionContext convert(ConnectorSession session) + private SessionScope convert(ConnectorSession session) { - return switch (sessionType) { + ForwardingFileIo.IdentityScope identityScope = ForwardingFileIo.withIdentity(session.getIdentity()); + SessionCatalog.SessionContext context = switch (sessionType) { case NONE -> new SessionContext(randomUUID().toString(), null, credentials, ImmutableMap.of(), session.getIdentity()); case USER -> { String sessionId = format("%s-%s", session.getUser(), session.getSource().orElse("default")); @@ -909,6 +915,17 @@ private SessionCatalog.SessionContext convert(ConnectorSession session) yield new SessionCatalog.SessionContext(sessionId, session.getUser(), credentials, properties, session.getIdentity()); } }; + return new SessionScope(context, identityScope); + } + + private record SessionScope(SessionCatalog.SessionContext context, ForwardingFileIo.IdentityScope identityScope) + implements AutoCloseable + { + @Override + public void close() + { + identityScope.close(); + } } private void invalidateTableCache(SchemaTableName schemaTableName) @@ -965,8 +982,8 @@ private TableIdentifier findRemoteTable(ConnectorSession session, TableIdentifie { Namespace remoteNamespace = toRemoteNamespace(session, tableIdentifier.namespace()); List tableIdentifiers; - try { - tableIdentifiers = restSessionCatalog.listTables(convert(session), remoteNamespace); + try (var scope = convert(session)) { + tableIdentifiers = restSessionCatalog.listTables(scope.context(), remoteNamespace); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list tables", e); @@ -998,8 +1015,8 @@ private TableIdentifier findRemoteView(ConnectorSession session, TableIdentifier Namespace remoteNamespace = toRemoteNamespace(session, tableIdentifier.namespace()); List tableIdentifiers; - try { - tableIdentifiers = restSessionCatalog.listViews(convert(session), remoteNamespace); + try (var scope = convert(session)) { + tableIdentifiers = restSessionCatalog.listViews(scope.context(), remoteNamespace); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list views", e); @@ -1050,8 +1067,8 @@ private Namespace findRemoteNamespace(ConnectorSession session, Namespace trinoN private List listNamespaces(ConnectorSession session, Namespace parentNamespace) { List childNamespaces; - try { - childNamespaces = restSessionCatalog.listNamespaces(convert(session), parentNamespace); + try (var scope = convert(session)) { + childNamespaces = restSessionCatalog.listNamespaces(scope.context(), parentNamespace); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list namespaces", e); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java index dd92a6cc532a..0677149090e4 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java @@ -14,12 +14,15 @@ package io.trino.plugin.iceberg.fileio; import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; 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; @@ -27,17 +30,21 @@ import org.apache.iceberg.io.BulkDeletionFailureException; import org.apache.iceberg.io.InputFile; import org.apache.iceberg.io.OutputFile; +import org.apache.iceberg.io.StorageCredential; import org.apache.iceberg.io.SupportsBulkOperations; +import org.apache.iceberg.io.SupportsStorageCredentials; import java.io.IOException; import java.io.UncheckedIOException; import java.util.List; import java.util.Map; import java.util.concurrent.Callable; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.stream.Stream; +import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.util.concurrent.MoreExecutors.newDirectExecutorService; import static io.trino.plugin.base.util.ExecutorUtil.processWithAdditionalThreads; @@ -46,15 +53,23 @@ import static java.util.stream.Collectors.joining; public class ForwardingFileIo - implements SupportsBulkOperations + implements SupportsBulkOperations, SupportsStorageCredentials { private static final int DELETE_BATCH_SIZE = 1000; private static final int BATCH_DELETE_PATHS_MESSAGE_LIMIT = 5; - private final TrinoFileSystem fileSystem; - private final Map properties; - private final boolean useFileSizeFromMetadata; - private final ExecutorService deleteExecutor; + public static final String TRINO_CATALOG_NAME = "trino.catalog.name"; + + private static final ConcurrentHashMap contextRegistry = new ConcurrentHashMap<>(); + private static final ThreadLocal currentIdentity = new ThreadLocal<>(); + + private TrinoFileSystem fileSystem; + private Map properties = ImmutableMap.of(); + private boolean useFileSizeFromMetadata; + private ExecutorService deleteExecutor = newDirectExecutorService(); + private volatile List storageCredentials = ImmutableList.of(); + + public ForwardingFileIo() {} @VisibleForTesting public ForwardingFileIo(TrinoFileSystem fileSystem, boolean useFileSizeFromMetadata) @@ -176,15 +191,104 @@ private void deleteBatch(List filesToDelete) @Override public Map properties() { - return properties; + List credentials = storageCredentials; + if (credentials.isEmpty()) { + return properties; + } + ImmutableMap.Builder merged = ImmutableMap.builder(); + merged.putAll(properties); + for (StorageCredential credential : credentials) { + merged.putAll(credential.config()); + } + return merged.buildKeepingLast(); + } + + @Override + public void setCredentials(List credentials) + { + this.storageCredentials = ImmutableList.copyOf(requireNonNull(credentials, "credentials is null")); + } + + @Override + public List credentials() + { + return storageCredentials; } @Override public void initialize(Map properties) { - throw new UnsupportedOperationException("ForwardingFileIO does not support initialization by properties"); + String catalogName = properties.get(TRINO_CATALOG_NAME); + checkState(catalogName != null, "ForwardingFileIo requires '%s' property for initialization", TRINO_CATALOG_NAME); + CreationContext context = contextRegistry.get(catalogName); + checkState(context != null, "No creation context registered for catalog: %s", catalogName); + + ConnectorIdentity identity = currentIdentity.get(); + if (identity == null) { + // During RESTSessionCatalog.initialize(), no user session exists yet. + // Use a default identity for catalog-level operations. + identity = ConnectorIdentity.ofUser("trino"); + } + + this.properties = ImmutableMap.copyOf(requireNonNull(properties, "properties is null")); + // Use merged properties (base + storage credentials) so that the file system factory + // can see vended credentials like s3.access-key-id from storage-credentials + this.fileSystem = context.fileSystemFactory().create(identity, properties()); + this.useFileSizeFromMetadata = true; + this.deleteExecutor = context.deleteExecutor(); + } + + public static void registerContext(String catalogName, IcebergFileSystemFactory fileSystemFactory, ExecutorService deleteExecutor) + { + requireNonNull(catalogName, "catalogName is null"); + contextRegistry.put(catalogName, new CreationContext( + requireNonNull(fileSystemFactory, "fileSystemFactory is null"), + requireNonNull(deleteExecutor, "deleteExecutor is null"))); + } + + public static void deregisterContext(String catalogName) + { + contextRegistry.remove(requireNonNull(catalogName, "catalogName is null")); + } + + public static IdentityScope withIdentity(ConnectorIdentity identity) + { + ConnectorIdentity previous = currentIdentity.get(); + currentIdentity.set(requireNonNull(identity, "identity is null")); + return new IdentityScope(previous); + } + + public static final class IdentityScope + implements AutoCloseable + { + private final ConnectorIdentity previous; + + private IdentityScope(ConnectorIdentity previous) + { + this.previous = previous; + } + + @Override + public void close() + { + if (previous == null) { + currentIdentity.remove(); + } + else { + currentIdentity.set(previous); + } + } } @Override public void close() {} + + private record CreationContext(IcebergFileSystemFactory fileSystemFactory, ExecutorService deleteExecutor) + { + CreationContext + { + requireNonNull(fileSystemFactory, "fileSystemFactory is null"); + requireNonNull(deleteExecutor, "deleteExecutor is null"); + } + } } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3VendingRestCatalog.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3VendingRestCatalog.java deleted file mode 100644 index 3d84c68073aa..000000000000 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3VendingRestCatalog.java +++ /dev/null @@ -1,345 +0,0 @@ -/* - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.trino.plugin.iceberg.catalog.rest; - -import com.google.common.collect.ImmutableMap; -import io.airlift.http.server.HttpServerConfig; -import io.airlift.http.server.HttpServerInfo; -import io.airlift.http.server.ServerFeature; -import io.airlift.http.server.testing.TestingHttpServer; -import io.airlift.node.NodeInfo; -import io.trino.plugin.iceberg.IcebergQueryRunner; -import io.trino.testing.AbstractTestQueryFramework; -import io.trino.testing.QueryRunner; -import io.trino.testing.containers.Minio; -import org.apache.iceberg.CatalogProperties; -import org.apache.iceberg.jdbc.JdbcCatalog; -import org.apache.iceberg.rest.RestCatalogServlet; -import org.apache.iceberg.rest.S3CredentialVendingCatalogAdapter; -import org.junit.jupiter.api.AfterAll; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.TestInstance; - -import java.io.IOException; -import java.nio.file.Files; -import java.nio.file.Path; -import java.util.Map; - -import static io.trino.testing.TestingNames.randomNameSuffix; -import static io.trino.testing.containers.Minio.MINIO_REGION; -import static io.trino.testing.containers.Minio.MINIO_ROOT_PASSWORD; -import static io.trino.testing.containers.Minio.MINIO_ROOT_USER; -import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS; - -@TestInstance(PER_CLASS) -class TestIcebergS3VendingRestCatalog - extends AbstractTestQueryFramework -{ - private static final String BUCKET_NAME = "test-iceberg-s3-vending-" + randomNameSuffix(); - - private Minio minio; - private JdbcCatalog backendCatalog; - private TestingHttpServer restServer; - - @SuppressWarnings("resource") - @Override - protected QueryRunner createQueryRunner() - throws Exception - { - minio = Minio.builder().build(); - minio.start(); - minio.createBucket(BUCKET_NAME); - - String minioAddress = minio.getMinioAddress(); - - backendCatalog = createBackendCatalog(minioAddress); - - // S3 properties that the REST catalog will vend to Trino - Map vendedS3Properties = ImmutableMap.builder() - .put("s3.access-key-id", MINIO_ROOT_USER) - .put("s3.secret-access-key", MINIO_ROOT_PASSWORD) - .put("s3.session-token", "test-session-token") - .put("client.region", MINIO_REGION) - .put("s3.endpoint", minioAddress) - .put("s3.cross-region-access-enabled", "true") - .buildOrThrow(); - - S3CredentialVendingCatalogAdapter adapter = new S3CredentialVendingCatalogAdapter(backendCatalog, vendedS3Properties); - RestCatalogServlet servlet = new RestCatalogServlet(adapter); - - NodeInfo nodeInfo = new NodeInfo("test"); - HttpServerConfig httpConfig = new HttpServerConfig() - .setHttpPort(0) - .setHttpEnabled(true); - HttpServerInfo httpServerInfo = new HttpServerInfo(httpConfig, nodeInfo); - restServer = new TestingHttpServer("s3-vending-rest-catalog", httpServerInfo, nodeInfo, httpConfig, servlet, ServerFeature.builder() - .withLegacyUriCompliance(true) - .build()); - restServer.start(); - - return IcebergQueryRunner.builder() - .setIcebergProperties(ImmutableMap.builder() - .put("iceberg.catalog.type", "rest") - .put("iceberg.rest-catalog.uri", restServer.getBaseUrl().toString()) - .put("iceberg.rest-catalog.vended-credentials-enabled", "true") - .put("fs.native-s3.enabled", "true") - .put("s3.region", MINIO_REGION) - .put("s3.endpoint", minioAddress) - .put("s3.path-style-access", "true") - .buildOrThrow()) - .build(); - } - - @AfterAll - public void tearDown() - { - if (restServer != null) { - try { - restServer.stop(); - } - catch (Exception e) { - // ignore - } - } - if (backendCatalog != null) { - try { - backendCatalog.close(); - } - catch (Exception e) { - // ignore - } - } - if (minio != null) { - minio.close(); - } - } - - @Test - void testCreateTableInsertAndSelect() - { - String tableName = "test_s3_vending_" + randomNameSuffix(); - assertUpdate("CREATE TABLE " + tableName + " (id INTEGER, name VARCHAR)"); - assertUpdate("INSERT INTO " + tableName + " VALUES (1, 'alice'), (2, 'bob')", 2); - assertQuery("SELECT * FROM " + tableName, "VALUES (1, 'alice'), (2, 'bob')"); - assertUpdate("DROP TABLE " + tableName); - } - - @Test - void testCreateTableAsSelect() - { - String tableName = "test_s3_ctas_" + randomNameSuffix(); - assertUpdate("CREATE TABLE " + tableName + " AS SELECT 1 AS id, VARCHAR 'hello' AS greeting", 1); - assertQuery("SELECT * FROM " + tableName, "VALUES (1, 'hello')"); - assertUpdate("DROP TABLE " + tableName); - } - - @Test - void testTableLocationIsOnS3() - { - String tableName = "test_s3_location_" + randomNameSuffix(); - assertUpdate("CREATE TABLE " + tableName + " AS SELECT 1 AS id", 1); - - String filePath = (String) computeScalar("SELECT file_path FROM \"" + tableName + "$files\" LIMIT 1"); - assertThat(filePath).startsWith("s3://" + BUCKET_NAME + "/"); - assertUpdate("DROP TABLE " + tableName); - } - - @Test - void testSchemaOperations() - { - String schemaName = "test_s3_schema_" + randomNameSuffix(); - assertUpdate("CREATE SCHEMA " + schemaName); - assertThat(computeActual("SHOW SCHEMAS").getOnlyColumnAsSet()).contains(schemaName); - - String tableName = schemaName + ".test_table_" + randomNameSuffix(); - assertUpdate("CREATE TABLE " + tableName + " (id INTEGER)"); - assertUpdate("INSERT INTO " + tableName + " VALUES (1)", 1); - assertQuery("SELECT * FROM " + tableName, "VALUES 1"); - - assertUpdate("DROP TABLE " + tableName); - assertUpdate("DROP SCHEMA " + schemaName); - } - - @Test - void testUpdateAndDelete() - { - String tableName = "test_s3_update_delete_" + randomNameSuffix(); - assertUpdate("CREATE TABLE " + tableName + " (id INTEGER, value VARCHAR)"); - assertUpdate("INSERT INTO " + tableName + " VALUES (1, 'original'), (2, 'keep'), (3, 'remove')", 3); - - assertUpdate("UPDATE " + tableName + " SET value = 'updated' WHERE id = 1", 1); - assertQuery("SELECT value FROM " + tableName + " WHERE id = 1", "VALUES 'updated'"); - - assertUpdate("DELETE FROM " + tableName + " WHERE id = 3", 1); - assertQuery("SELECT * FROM " + tableName + " ORDER BY id", "VALUES (1, 'updated'), (2, 'keep')"); - - assertUpdate("DROP TABLE " + tableName); - } - - @Test - void testPartitionedTable() - { - String tableName = "test_s3_partitioned_" + randomNameSuffix(); - assertUpdate("CREATE TABLE " + tableName + " (id INTEGER, region VARCHAR) WITH (partitioning = ARRAY['region'])"); - assertUpdate("INSERT INTO " + tableName + " VALUES (1, 'us'), (2, 'eu'), (3, 'us')", 3); - - assertQuery("SELECT id FROM " + tableName + " WHERE region = 'us' ORDER BY id", "VALUES 1, 3"); - assertQuery("SELECT id FROM " + tableName + " WHERE region = 'eu'", "VALUES 2"); - - long partitionCount = (long) computeScalar("SELECT count(*) FROM \"" + tableName + "$partitions\""); - assertThat(partitionCount).isEqualTo(2L); - - assertUpdate("DROP TABLE " + tableName); - } - - @Test - void testSnapshotsAndTimeTravel() - { - String tableName = "test_s3_snapshots_" + randomNameSuffix(); - assertUpdate("CREATE TABLE " + tableName + " (id INTEGER)"); - - assertUpdate("INSERT INTO " + tableName + " VALUES (1)", 1); - long snapshotAfterFirstInsert = (long) computeScalar("SELECT snapshot_id FROM \"" + tableName + "$snapshots\" ORDER BY committed_at DESC LIMIT 1"); - - assertUpdate("INSERT INTO " + tableName + " VALUES (2)", 1); - assertQuery("SELECT * FROM " + tableName + " ORDER BY id", "VALUES 1, 2"); - - // Time travel to first snapshot - assertQuery("SELECT * FROM " + tableName + " FOR VERSION AS OF " + snapshotAfterFirstInsert, "VALUES 1"); - - assertUpdate("DROP TABLE " + tableName); - } - - @Test - void testMetadataTables() - { - String tableName = "test_s3_metadata_" + randomNameSuffix(); - assertUpdate("CREATE TABLE " + tableName + " (id INTEGER, part VARCHAR) WITH (partitioning = ARRAY['part'])"); - assertUpdate("INSERT INTO " + tableName + " VALUES (1, 'a')", 1); - assertUpdate("INSERT INTO " + tableName + " VALUES (2, 'b')", 1); - - long snapshotCount = (long) computeScalar("SELECT count(*) FROM \"" + tableName + "$snapshots\""); - assertThat(snapshotCount).isGreaterThanOrEqualTo(2L); - - long fileCount = (long) computeScalar("SELECT count(*) FROM \"" + tableName + "$files\""); - assertThat(fileCount).isEqualTo(2L); - - long historyCount = (long) computeScalar("SELECT count(*) FROM \"" + tableName + "$history\""); - assertThat(historyCount).isGreaterThanOrEqualTo(2L); - - long partitionCount = (long) computeScalar("SELECT count(*) FROM \"" + tableName + "$partitions\""); - assertThat(partitionCount).isEqualTo(2L); - - // Verify file paths are on S3 - String filePath = (String) computeScalar("SELECT file_path FROM \"" + tableName + "$files\" LIMIT 1"); - assertThat(filePath).startsWith("s3://" + BUCKET_NAME + "/"); - - assertUpdate("DROP TABLE " + tableName); - } - - @Test - void testMultipleDataTypes() - { - String tableName = "test_s3_types_" + randomNameSuffix(); - assertUpdate("CREATE TABLE " + tableName + " (" + - "c_integer INTEGER, " + - "c_bigint BIGINT, " + - "c_real REAL, " + - "c_double DOUBLE, " + - "c_decimal DECIMAL(10,2), " + - "c_varchar VARCHAR, " + - "c_boolean BOOLEAN, " + - "c_date DATE, " + - "c_timestamp TIMESTAMP(6))"); - - assertUpdate("INSERT INTO " + tableName + " VALUES (" + - "42, " + - "BIGINT '9999999999', " + - "REAL '3.14', " + - "DOUBLE '2.718281828', " + - "DECIMAL '12345.67', " + - "VARCHAR 'hello world', " + - "true, " + - "DATE '2024-01-15', " + - "TIMESTAMP '2024-01-15 10:30:00.123456')", 1); - - assertQuery("SELECT c_integer, c_bigint, c_varchar, c_boolean FROM " + tableName, - "VALUES (42, 9999999999, 'hello world', true)"); - - assertUpdate("DROP TABLE " + tableName); - } - - @Test - void testCreateOrReplaceTable() - { - String tableName = "test_s3_create_or_replace_" + randomNameSuffix(); - assertUpdate("CREATE TABLE " + tableName + " AS SELECT 1 AS id, VARCHAR 'v1' AS version", 1); - assertQuery("SELECT * FROM " + tableName, "VALUES (1, 'v1')"); - - long v1SnapshotId = (long) computeScalar("SELECT snapshot_id FROM \"" + tableName + "$snapshots\" ORDER BY committed_at DESC LIMIT 1"); - - assertUpdate("CREATE OR REPLACE TABLE " + tableName + " AS SELECT 2 AS id, VARCHAR 'v2' AS version", 1); - assertQuery("SELECT * FROM " + tableName, "VALUES (2, 'v2')"); - - // Time travel to original version - assertQuery("SELECT * FROM " + tableName + " FOR VERSION AS OF " + v1SnapshotId, "VALUES (1, 'v1')"); - - assertUpdate("DROP TABLE " + tableName); - } - - @Test - void testAlterTable() - { - String tableName = "test_s3_alter_" + randomNameSuffix(); - assertUpdate("CREATE TABLE " + tableName + " (id INTEGER)"); - assertUpdate("INSERT INTO " + tableName + " VALUES (1)", 1); - - assertUpdate("ALTER TABLE " + tableName + " ADD COLUMN name VARCHAR"); - assertUpdate("INSERT INTO " + tableName + " VALUES (2, 'alice')", 1); - - assertQuery("SELECT * FROM " + tableName + " ORDER BY id", "VALUES (1, NULL), (2, 'alice')"); - - assertUpdate("ALTER TABLE " + tableName + " RENAME COLUMN name TO full_name"); - assertQuery("SELECT id, full_name FROM " + tableName + " WHERE id = 2", "VALUES (2, 'alice')"); - - assertUpdate("DROP TABLE " + tableName); - } - - private static JdbcCatalog createBackendCatalog(String minioAddress) - throws IOException - { - Path tempFile = Files.createTempFile("iceberg-s3-test-jdbc", null); - tempFile.toFile().deleteOnExit(); - - ImmutableMap.Builder properties = ImmutableMap.builder(); - properties.put(CatalogProperties.URI, "jdbc:h2:file:" + tempFile.toAbsolutePath()); - properties.put(JdbcCatalog.PROPERTY_PREFIX + "username", "user"); - properties.put(JdbcCatalog.PROPERTY_PREFIX + "password", "password"); - properties.put(JdbcCatalog.PROPERTY_PREFIX + "schema-version", "V1"); - properties.put(CatalogProperties.WAREHOUSE_LOCATION, "s3://" + BUCKET_NAME + "/warehouse"); - properties.put(CatalogProperties.FILE_IO_IMPL, "org.apache.iceberg.aws.s3.S3FileIO"); - properties.put("s3.access-key-id", MINIO_ROOT_USER); - properties.put("s3.secret-access-key", MINIO_ROOT_PASSWORD); - properties.put("s3.endpoint", minioAddress); - properties.put("s3.path-style-access", "true"); - properties.put("client.region", MINIO_REGION); - - JdbcCatalog catalog = new JdbcCatalog(); - catalog.setConf(new org.apache.hadoop.conf.Configuration()); - catalog.initialize("s3_backend", properties.buildOrThrow()); - - return catalog; - } -} diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergStorageCredentialsRestCatalog.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergStorageCredentialsRestCatalog.java new file mode 100644 index 000000000000..fb10f0d42ea4 --- /dev/null +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergStorageCredentialsRestCatalog.java @@ -0,0 +1,110 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.iceberg.catalog.rest; + +import com.google.common.collect.ImmutableMap; +import io.airlift.http.server.testing.TestingHttpServer; +import io.trino.plugin.iceberg.IcebergQueryRunner; +import io.trino.testing.AbstractTestQueryFramework; +import io.trino.testing.QueryRunner; +import org.apache.iceberg.rest.DelegatingRestSessionCatalog; +import org.apache.iceberg.rest.credentials.ImmutableCredential; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Optional; + +import static com.google.common.io.MoreFiles.deleteRecursively; +import static com.google.common.io.RecursiveDeleteOption.ALLOW_INSECURE; +import static io.trino.plugin.iceberg.catalog.rest.RestCatalogTestUtils.backendCatalog; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS; + +@TestInstance(PER_CLASS) +public class TestIcebergStorageCredentialsRestCatalog + extends AbstractTestQueryFramework +{ + private Path warehouseLocation; + + @Override + protected QueryRunner createQueryRunner() + throws Exception + { + warehouseLocation = Files.createTempDirectory(null); + closeAfterClass(() -> deleteRecursively(warehouseLocation, ALLOW_INSECURE)); + + DelegatingRestSessionCatalog delegatingCatalog = DelegatingRestSessionCatalog.builder() + .delegate(backendCatalog(warehouseLocation)) + .addAllCredentials(List.of( + ImmutableCredential.builder() + .prefix("file://") + .config(ImmutableMap.of( + "s3.access-key-id", "test-vended-access-key", + "s3.secret-access-key", "test-vended-secret-key", + "s3.session-token", "test-vended-session-token")) + .build())) + .build(); + + TestingHttpServer testServer = delegatingCatalog.testServer(); + testServer.start(); + closeAfterClass(testServer::stop); + + return IcebergQueryRunner.builder() + .setBaseDataDir(Optional.of(warehouseLocation)) + .setIcebergProperties( + ImmutableMap.builder() + .put("iceberg.catalog.type", "rest") + .put("iceberg.rest-catalog.uri", testServer.getBaseUrl().toString()) + .put("iceberg.rest-catalog.vended-credentials-enabled", "true") + .buildOrThrow()) + .build(); + } + + @Test + public void testCreateAndReadTableWithStorageCredentials() + { + assertUpdate("CREATE SCHEMA test_storage_creds"); + assertUpdate("CREATE TABLE test_storage_creds.test_table (id INTEGER, name VARCHAR)"); + assertUpdate("INSERT INTO test_storage_creds.test_table VALUES (1, 'alice'), (2, 'bob')", 2); + + assertThat(query("SELECT * FROM test_storage_creds.test_table")) + .matches("VALUES (1, VARCHAR 'alice'), (2, VARCHAR 'bob')"); + + assertUpdate("DROP TABLE test_storage_creds.test_table"); + assertUpdate("DROP SCHEMA test_storage_creds"); + } + + @Test + public void testStorageCredentialsWithMultipleOperations() + { + assertUpdate("CREATE SCHEMA test_multi_ops"); + assertUpdate("CREATE TABLE test_multi_ops.t1 (x INTEGER)"); + assertUpdate("INSERT INTO test_multi_ops.t1 VALUES 1, 2, 3", 3); + + // Read back + assertThat(query("SELECT count(*) FROM test_multi_ops.t1")) + .matches("VALUES BIGINT '3'"); + + // Additional insert (triggers new table load with credentials) + assertUpdate("INSERT INTO test_multi_ops.t1 VALUES 4, 5", 2); + assertThat(query("SELECT count(*) FROM test_multi_ops.t1")) + .matches("VALUES BIGINT '5'"); + + assertUpdate("DROP TABLE test_multi_ops.t1"); + assertUpdate("DROP SCHEMA test_multi_ops"); + } +} diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergVendedCredentialsMapper.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergVendedCredentialsMapper.java deleted file mode 100644 index c43957b465e2..000000000000 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergVendedCredentialsMapper.java +++ /dev/null @@ -1,287 +0,0 @@ -/* - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.trino.plugin.iceberg.catalog.rest; - -import com.google.common.collect.ImmutableMap; -import io.trino.filesystem.Location; -import io.trino.filesystem.s3.S3FileSystemConfig; -import io.trino.filesystem.s3.S3SecurityMappingResult; -import io.trino.spi.security.ConnectorIdentity; -import org.junit.jupiter.api.Test; -import software.amazon.awssdk.auth.credentials.AwsCredentials; -import software.amazon.awssdk.identity.spi.AwsCredentialsIdentity; - -import java.util.Map; -import java.util.Optional; - -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_ENDPOINT_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_REGION_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY; -import static io.trino.filesystem.s3.S3FileSystemConstants.EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY; -import static org.assertj.core.api.Assertions.assertThat; - -final class TestIcebergVendedCredentialsMapper -{ - private static final Location DEFAULT_LOCATION = Location.of("s3://test-bucket/path"); - - @Test - void testNoVendedCredentials() - { - IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(new S3FileSystemConfig()); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test").build(); - - Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); - - assertThat(result).isEmpty(); - } - - @Test - void testVendedCredentialsOnly() - { - IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(new S3FileSystemConfig()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); - - assertThat(result).isPresent(); - assertThat(result.get().credentials()).isPresent(); - assertThat(result.get().credentials().get()).isInstanceOf(AwsCredentials.class); - AwsCredentialsIdentity credentials = (AwsCredentialsIdentity) result.get().credentials().get(); - assertThat(credentials.accessKeyId()).isEqualTo("vended-access"); - assertThat(credentials.secretAccessKey()).isEqualTo("vended-secret"); - } - - @Test - void testVendedRegionTakesPrecedenceOverStatic() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setRegion("us-east-1"); - - IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(config); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "us-west-2") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); - - assertThat(result).isPresent(); - assertThat(result.get().region()).hasValue("us-west-2"); - } - - @Test - void testVendedEndpointTakesPrecedenceOverStatic() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setEndpoint("https://s3.amazonaws.com"); - - IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(config); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://minio.example.com") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); - - assertThat(result).isPresent(); - assertThat(result.get().endpoint()).hasValue("https://minio.example.com"); - } - - @Test - void testStaticCrossRegionAccessTrueAlwaysWins() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setCrossRegionAccessEnabled(true); - - IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(config); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "false") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); - - assertThat(result).isPresent(); - assertThat(result.get().crossRegionAccessEnabled()).hasValue(true); - } - - @Test - void testVendedCrossRegionAccessWhenStaticIsFalse() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setCrossRegionAccessEnabled(false); - - IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(config); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_CROSS_REGION_ACCESS_ENABLED_PROPERTY, "true") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); - - assertThat(result).isPresent(); - assertThat(result.get().crossRegionAccessEnabled()).hasValue(true); - } - - @Test - void testIncompleteCredentialsReturnsEmpty() - { - IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(new S3FileSystemConfig()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); - - assertThat(result).isEmpty(); - } - - @Test - void testVendedRegionUsedWhenNoStatic() - { - IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(new S3FileSystemConfig()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_REGION_PROPERTY, "ap-south-1") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); - - assertThat(result).isPresent(); - assertThat(result.get().region()).hasValue("ap-south-1"); - } - - @Test - void testVendedEndpointUsedWhenNoStatic() - { - IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(new S3FileSystemConfig()); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .put(EXTRA_CREDENTIALS_ENDPOINT_PROPERTY, "https://custom-s3.example.com") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); - - assertThat(result).isPresent(); - assertThat(result.get().endpoint()).hasValue("https://custom-s3.example.com"); - } - - @Test - void testStaticRegionUsedWhenVendedNotProvided() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setRegion("us-east-1"); - - IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(config); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); - - assertThat(result).isPresent(); - assertThat(result.get().region()).hasValue("us-east-1"); - } - - @Test - void testStaticEndpointUsedWhenVendedNotProvided() - { - S3FileSystemConfig config = new S3FileSystemConfig() - .setEndpoint("https://s3.amazonaws.com"); - - IcebergVendedCredentialsMapper mapper = new IcebergVendedCredentialsMapper(config); - - Map extraCredentials = ImmutableMap.builder() - .put(EXTRA_CREDENTIALS_ACCESS_KEY_PROPERTY, "vended-access") - .put(EXTRA_CREDENTIALS_SECRET_KEY_PROPERTY, "vended-secret") - .put(EXTRA_CREDENTIALS_SESSION_TOKEN_PROPERTY, "vended-token") - .buildOrThrow(); - - ConnectorIdentity identity = ConnectorIdentity.forUser("test") - .withExtraCredentials(extraCredentials) - .build(); - - Optional result = mapper.getMapping(identity, DEFAULT_LOCATION); - - assertThat(result).isPresent(); - assertThat(result.get().endpoint()).hasValue("https://s3.amazonaws.com"); - } -} diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/fileio/TestForwardingFileIo.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/fileio/TestForwardingFileIo.java index 47c33066c469..6ed9230aa646 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/fileio/TestForwardingFileIo.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/fileio/TestForwardingFileIo.java @@ -13,17 +13,23 @@ */ package io.trino.plugin.iceberg.fileio; +import com.google.common.collect.ImmutableMap; import io.trino.filesystem.TrinoFileSystem; import io.trino.filesystem.local.LocalFileSystemFactory; +import io.trino.spi.security.ConnectorIdentity; import org.apache.iceberg.io.FileIO; +import org.apache.iceberg.io.StorageCredential; import org.apache.iceberg.io.SupportsBulkOperations; +import org.apache.iceberg.io.SupportsStorageCredentials; import org.junit.jupiter.api.Test; import java.nio.file.Files; import java.nio.file.Path; +import java.util.List; import static com.google.common.io.MoreFiles.deleteRecursively; import static com.google.common.io.RecursiveDeleteOption.ALLOW_INSECURE; +import static com.google.common.util.concurrent.MoreExecutors.newDirectExecutorService; import static io.trino.testing.InterfaceTestUtils.assertAllMethodsOverridden; import static io.trino.testing.TestingConnectorSession.SESSION; import static org.assertj.core.api.Assertions.assertThat; @@ -35,6 +41,7 @@ public void testEverythingImplemented() { assertAllMethodsOverridden(FileIO.class, ForwardingFileIo.class); assertAllMethodsOverridden(SupportsBulkOperations.class, ForwardingFileIo.class); + assertAllMethodsOverridden(SupportsStorageCredentials.class, ForwardingFileIo.class); } @Test @@ -61,4 +68,131 @@ public void testUseFileSizeFromMetadata() } deleteRecursively(tempDir, ALLOW_INSECURE); } + + @Test + public void testStorageCredentialsMergedIntoProperties() + { + try (ForwardingFileIo fileIo = new ForwardingFileIo( + new LocalFileSystemFactory(Path.of(System.getProperty("java.io.tmpdir"))).create(SESSION), + ImmutableMap.of("base.key", "base.value"), + true, + newDirectExecutorService())) { + // initially no credentials + assertThat(fileIo.credentials()).isEmpty(); + assertThat(fileIo.properties()).containsExactlyEntriesOf(ImmutableMap.of("base.key", "base.value")); + + // set storage credentials — config entries should appear in properties + List credentials = List.of( + StorageCredential.create("s3://bucket/", ImmutableMap.of( + "s3.access-key-id", "test-access-key", + "s3.secret-access-key", "test-secret-key", + "s3.session-token", "test-session-token")), + StorageCredential.create("gs://bucket/", ImmutableMap.of( + "gcs.oauth2.token", "test-gcs-token"))); + fileIo.setCredentials(credentials); + + assertThat(fileIo.credentials()).isEqualTo(credentials); + assertThat(fileIo.properties()) + .containsEntry("base.key", "base.value") + .containsEntry("s3.access-key-id", "test-access-key") + .containsEntry("s3.secret-access-key", "test-secret-key") + .containsEntry("s3.session-token", "test-session-token") + .containsEntry("gcs.oauth2.token", "test-gcs-token"); + } + } + + @Test + public void testStorageCredentialsTakePrecedenceOverBaseProperties() + { + try (ForwardingFileIo fileIo = new ForwardingFileIo( + new LocalFileSystemFactory(Path.of(System.getProperty("java.io.tmpdir"))).create(SESSION), + ImmutableMap.of("s3.access-key-id", "static-key", "unrelated.key", "keep-me"), + true, + newDirectExecutorService())) { + fileIo.setCredentials(List.of( + StorageCredential.create("s3://bucket/", ImmutableMap.of( + "s3.access-key-id", "vended-key")))); + + assertThat(fileIo.properties()) + .containsEntry("s3.access-key-id", "vended-key") + .containsEntry("unrelated.key", "keep-me"); + } + } + + @Test + public void testEmptyStorageCredentialsRevertToBaseProperties() + { + try (ForwardingFileIo fileIo = new ForwardingFileIo( + new LocalFileSystemFactory(Path.of(System.getProperty("java.io.tmpdir"))).create(SESSION), + ImmutableMap.of("base.key", "base.value"), + true, + newDirectExecutorService())) { + fileIo.setCredentials(List.of( + StorageCredential.create("s3://bucket/", ImmutableMap.of( + "s3.access-key-id", "vended-key")))); + + assertThat(fileIo.properties()).containsKey("s3.access-key-id"); + + // reset to empty + fileIo.setCredentials(List.of()); + + assertThat(fileIo.credentials()).isEmpty(); + assertThat(fileIo.properties()).containsExactlyEntriesOf(ImmutableMap.of("base.key", "base.value")); + } + } + + @Test + public void testInitializeWithRegisteredContext() + { + String catalogName = "test-init-" + Thread.currentThread().threadId(); + Path tempDir = Path.of(System.getProperty("java.io.tmpdir")); + LocalFileSystemFactory localFactory = new LocalFileSystemFactory(tempDir); + ForwardingFileIo.registerContext(catalogName, + (identity, props) -> localFactory.create(SESSION), + newDirectExecutorService()); + try (ForwardingFileIo.IdentityScope ignored = ForwardingFileIo.withIdentity(ConnectorIdentity.ofUser("test-user")); + ForwardingFileIo fileIo = new ForwardingFileIo()) { + fileIo.initialize(ImmutableMap.of( + ForwardingFileIo.TRINO_CATALOG_NAME, catalogName, + "base.key", "base.value")); + + assertThat(fileIo.properties()) + .containsEntry("base.key", "base.value"); + } + finally { + ForwardingFileIo.deregisterContext(catalogName); + } + } + + @Test + public void testSetCredentialsThenInitialize() + { + String catalogName = "test-creds-" + Thread.currentThread().threadId(); + Path tempDir = Path.of(System.getProperty("java.io.tmpdir")); + LocalFileSystemFactory localFactory = new LocalFileSystemFactory(tempDir); + ForwardingFileIo.registerContext(catalogName, + (identity, props) -> localFactory.create(SESSION), + newDirectExecutorService()); + try (ForwardingFileIo.IdentityScope ignored = ForwardingFileIo.withIdentity(ConnectorIdentity.ofUser("test-user")); + ForwardingFileIo fileIo = new ForwardingFileIo()) { + // CatalogUtil.loadFileIO calls setCredentials BEFORE initialize + List credentials = List.of( + StorageCredential.create("s3://bucket/", ImmutableMap.of( + "s3.access-key-id", "vended-key", + "s3.secret-access-key", "vended-secret"))); + fileIo.setCredentials(credentials); + fileIo.initialize(ImmutableMap.of( + ForwardingFileIo.TRINO_CATALOG_NAME, catalogName, + "s3.access-key-id", "static-key")); + + // storage credentials take precedence over base properties + assertThat(fileIo.properties()) + .containsEntry("s3.access-key-id", "vended-key") + .containsEntry("s3.secret-access-key", "vended-secret"); + assertThat(fileIo.credentials()).isEqualTo(credentials); + } + finally { + ForwardingFileIo.deregisterContext(catalogName); + } + } } diff --git a/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/DelegatingRestSessionCatalog.java b/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/DelegatingRestSessionCatalog.java index c4feecc0b5c0..2cb0ac0955fd 100644 --- a/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/DelegatingRestSessionCatalog.java +++ b/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/DelegatingRestSessionCatalog.java @@ -19,9 +19,15 @@ import io.airlift.http.server.testing.TestingHttpServer; import io.airlift.node.NodeInfo; import org.apache.iceberg.catalog.Catalog; +import org.apache.iceberg.rest.credentials.Credential; +import org.apache.iceberg.rest.responses.LoadTableResponse; import java.io.Closeable; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.function.Consumer; import static java.util.Objects.requireNonNull; @@ -81,6 +87,7 @@ public static Builder builder() public static class Builder { private Catalog delegate; + private final List credentials = new ArrayList<>(); public Builder delegate(Catalog delegate) { @@ -88,11 +95,53 @@ public Builder delegate(Catalog delegate) return this; } + public Builder addAllCredentials(List credentials) + { + this.credentials.addAll(credentials); + return this; + } + public DelegatingRestSessionCatalog build() { requireNonNull(delegate, "Delegate must be set"); - return new DelegatingRestSessionCatalog(new RESTCatalogAdapter(delegate), delegate); + RESTCatalogAdapter adapter = credentials.isEmpty() + ? new RESTCatalogAdapter(delegate) + : new CredentialInjectingAdapter(delegate, credentials); + return new DelegatingRestSessionCatalog(adapter, delegate); + } + } + + private static class CredentialInjectingAdapter + extends RESTCatalogAdapter + { + private final List credentials; + + CredentialInjectingAdapter(Catalog catalog, List credentials) + { + super(catalog); + this.credentials = List.copyOf(credentials); + } + + @Override + public T handleRequest( + Route route, + Map vars, + HTTPRequest request, + Class responseType, + Consumer> responseHeaders) + { + T response = super.handleRequest(route, vars, request, responseType, responseHeaders); + if (response instanceof LoadTableResponse loadTableResponse) { + LoadTableResponse.Builder builder = LoadTableResponse.builder() + .withTableMetadata(loadTableResponse.tableMetadata()) + .addAllConfig(loadTableResponse.config()) + .addAllCredentials(credentials); + @SuppressWarnings("unchecked") + T result = (T) builder.build(); + return result; + } + return response; } } } diff --git a/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/S3CredentialVendingCatalogAdapter.java b/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/S3CredentialVendingCatalogAdapter.java deleted file mode 100644 index 25968b5f3952..000000000000 --- a/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/S3CredentialVendingCatalogAdapter.java +++ /dev/null @@ -1,54 +0,0 @@ -/* - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.apache.iceberg.rest; - -import org.apache.iceberg.catalog.Catalog; -import org.apache.iceberg.rest.responses.ErrorResponse; -import org.apache.iceberg.rest.responses.LoadTableResponse; - -import java.util.Map; -import java.util.function.Consumer; - -import static java.util.Objects.requireNonNull; - -/** - * A REST catalog adapter that injects S3 vended credential properties - * into the LoadTableResponse config, simulating a REST catalog server - * that vends S3 credentials with region, endpoint, and cross-region access. - */ -public class S3CredentialVendingCatalogAdapter - extends RESTCatalogAdapter -{ - private final Map vendedS3Properties; - - public S3CredentialVendingCatalogAdapter(Catalog backendCatalog, Map vendedS3Properties) - { - super(backendCatalog); - this.vendedS3Properties = requireNonNull(vendedS3Properties, "vendedS3Properties is null"); - } - - @Override - protected T execute( - HTTPRequest request, - Class responseType, - Consumer errorHandler, - Consumer> responseHeaders) - { - T response = super.execute(request, responseType, errorHandler, responseHeaders); - if (response instanceof LoadTableResponse loadTableResponse) { - loadTableResponse.config().putAll(vendedS3Properties); - } - return response; - } -} From 515a7c8283ce0a44669ec7eb0d078265d3014b90 Mon Sep 17 00:00:00 2001 From: kaveti Date: Thu, 9 Apr 2026 15:35:01 +0530 Subject: [PATCH 7/9] Reshape REST storage credentials flow for Iceberg 1.11. Switch REST catalog file IO creation to ioBuilder with identity-aware prefix-scoped file systems, drop context-registration hacks, and align tests/adapters with the Iceberg 1.11 API so vended credentials are applied without introducing duplicate S3 client lifecycle paths. Made-with: Cursor --- .../rest/TrinoIcebergRestCatalogFactory.java | 40 ++- .../catalog/rest/TrinoRestCatalog.java | 255 ++++++++---------- .../iceberg/fileio/ForwardingFileIo.java | 155 +++++------ .../fileio/ForwardingFileIoFactory.java | 18 ++ ...tIcebergStorageCredentialsRestCatalog.java | 27 +- .../iceberg/fileio/TestForwardingFileIo.java | 178 +++++------- 6 files changed, 295 insertions(+), 378 deletions(-) diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java index 89deaa4d143e..eaedc9eae6f9 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java @@ -20,23 +20,22 @@ import com.google.inject.Inject; import io.airlift.units.Duration; import io.trino.cache.EvictableCacheBuilder; -import io.trino.plugin.iceberg.ForIcebergFileDelete; import io.trino.plugin.iceberg.IcebergConfig; import io.trino.plugin.iceberg.IcebergFileSystemFactory; import io.trino.plugin.iceberg.catalog.TrinoCatalog; import io.trino.plugin.iceberg.catalog.TrinoCatalogFactory; import io.trino.plugin.iceberg.catalog.rest.IcebergRestCatalogConfig.Security; import io.trino.plugin.iceberg.catalog.rest.IcebergRestCatalogConfig.SessionType; -import io.trino.plugin.iceberg.fileio.ForwardingFileIo; +import io.trino.plugin.iceberg.fileio.ForwardingFileIoFactory; import io.trino.spi.NodeVersion; import io.trino.spi.catalog.CatalogName; import io.trino.spi.security.ConnectorIdentity; import io.trino.spi.type.TypeManager; -import jakarta.annotation.PreDestroy; import org.apache.iceberg.CatalogProperties; 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; @@ -44,12 +43,10 @@ import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.concurrent.ExecutorService; 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.CatalogProperties.FILE_IO_IMPL; import static org.apache.iceberg.rest.auth.OAuth2Properties.CREDENTIAL; import static org.apache.iceberg.rest.auth.OAuth2Properties.TOKEN; @@ -57,7 +54,7 @@ public class TrinoIcebergRestCatalogFactory implements TrinoCatalogFactory { private final IcebergFileSystemFactory fileSystemFactory; - private final ExecutorService deleteExecutor; + private final ForwardingFileIoFactory fileIoFactory; private final CatalogName catalogName; private final String trinoVersion; private final URI serverUri; @@ -84,7 +81,7 @@ public class TrinoIcebergRestCatalogFactory @Inject public TrinoIcebergRestCatalogFactory( IcebergFileSystemFactory fileSystemFactory, - @ForIcebergFileDelete ExecutorService deleteExecutor, + ForwardingFileIoFactory fileIoFactory, CatalogName catalogName, IcebergRestCatalogConfig restConfig, SecurityProperties securityProperties, @@ -93,7 +90,7 @@ public TrinoIcebergRestCatalogFactory( NodeVersion nodeVersion) { this.fileSystemFactory = requireNonNull(fileSystemFactory, "fileSystemFactory is null"); - this.deleteExecutor = requireNonNull(deleteExecutor, "deleteExecutor is null"); + this.fileIoFactory = requireNonNull(fileIoFactory, "fileIoFactory is null"); this.catalogName = requireNonNull(catalogName, "catalogName is null"); this.trinoVersion = requireNonNull(nodeVersion, "nodeVersion is null").toString(); requireNonNull(restConfig, "restConfig is null"); @@ -123,12 +120,6 @@ public TrinoIcebergRestCatalogFactory( .build(); } - @PreDestroy - public void shutdown() - { - ForwardingFileIo.deregisterContext(catalogName.toString()); - } - @Override public synchronized TrinoCatalog create(ConnectorIdentity identity) { @@ -139,7 +130,7 @@ public synchronized TrinoCatalog create(ConnectorIdentity identity) properties.put(CatalogProperties.URI, serverUri.toString()); warehouse.ifPresent(location -> properties.put(CatalogProperties.WAREHOUSE_LOCATION, location)); prefix.ifPresent(prefix -> properties.put("prefix", prefix)); - properties.put("view-endpoints-supported", Boolean.toString(viewEndpointsEnabled)); + 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()))); @@ -150,19 +141,22 @@ public synchronized TrinoCatalog create(ConnectorIdentity identity) properties.put("header.X-Iceberg-Access-Delegation", "vended-credentials"); } - properties.put(FILE_IO_IMPL, ForwardingFileIo.class.getName()); - properties.put(ForwardingFileIo.TRINO_CATALOG_NAME, catalogName.toString()); - ForwardingFileIo.registerContext(catalogName.toString(), fileSystemFactory, deleteExecutor); - - // ioBuilder is null so RESTSessionCatalog creates FileIO via reflection, - // which allows SupportsStorageCredentials.setCredentials() to be called - // with vended credentials before FileIO.initialize() RESTSessionCatalog icebergCatalogInstance = new RESTSessionCatalog( config -> HTTPClient.builder(config) .uri(config.get(CatalogProperties.URI)) .withHeaders(RESTUtil.configHeaders(config)) .build(), - null); + (context, config) -> { + ConnectorIdentity currentIdentity = (context.wrappedIdentity() != null) + ? ((ConnectorIdentity) context.wrappedIdentity()) + : ConnectorIdentity.ofUser("trino"); + return fileIoFactory.create( + fileSystemFactory.create(currentIdentity, config), + true, + config, + fileSystemFactory, + currentIdentity); + }); icebergCatalogInstance.initialize(catalogName.toString(), properties.buildOrThrow()); icebergCatalog = icebergCatalogInstance; diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java index 1009049f9d23..600b4a59aa39 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java @@ -33,7 +33,6 @@ import io.trino.plugin.iceberg.catalog.TrinoCatalog; import io.trino.plugin.iceberg.catalog.rest.IcebergRestCatalogConfig.Security; import io.trino.plugin.iceberg.catalog.rest.IcebergRestCatalogConfig.SessionType; -import io.trino.plugin.iceberg.fileio.ForwardingFileIo; import io.trino.spi.TrinoException; import io.trino.spi.catalog.CatalogName; import io.trino.spi.connector.CatalogSchemaTableName; @@ -177,8 +176,8 @@ public Optional getNamespaceSeparator() @Override public boolean namespaceExists(ConnectorSession session, String namespace) { - try (var scope = convert(session)) { - return restSessionCatalog.namespaceExists(scope.context(), toRemoteNamespace(session, toNamespace(namespace))); + try { + return restSessionCatalog.namespaceExists(convert(session), toRemoteNamespace(session, toNamespace(namespace))); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to check namespace '%s'".formatted(namespace), e); @@ -191,8 +190,8 @@ public List listNamespaces(ConnectorSession session) if (nestedNamespaceEnabled) { return collectNamespaces(session, Namespace.empty()); } - try (var scope = convert(session)) { - return restSessionCatalog.listNamespaces(scope.context()).stream() + try { + return restSessionCatalog.listNamespaces(convert(session)).stream() .map(this::toSchemaName) .collect(toImmutableList()); } @@ -203,8 +202,8 @@ public List listNamespaces(ConnectorSession session) private List collectNamespaces(ConnectorSession session, Namespace parentNamespace) { - try (var scope = convert(session)) { - return restSessionCatalog.listNamespaces(scope.context(), parentNamespace).stream() + try { + return restSessionCatalog.listNamespaces(convert(session), parentNamespace).stream() .flatMap(childNamespace -> Stream.concat( Stream.of(childNamespace.toString()), collectNamespaces(session, childNamespace).stream())) @@ -218,8 +217,8 @@ private List collectNamespaces(ConnectorSession session, Namespace paren @Override public void dropNamespace(ConnectorSession session, String namespace) { - try (var scope = convert(session)) { - restSessionCatalog.dropNamespace(scope.context(), toRemoteNamespace(session, toNamespace(namespace))); + try { + restSessionCatalog.dropNamespace(convert(session), toRemoteNamespace(session, toNamespace(namespace))); } catch (NoSuchNamespaceException e) { throw new SchemaNotFoundException(namespace); @@ -235,9 +234,9 @@ public void dropNamespace(ConnectorSession session, String namespace) @Override public Map loadNamespaceMetadata(ConnectorSession session, String namespace) { - try (var scope = convert(session)) { + try { // Return immutable metadata as direct modifications will not be reflected on the namespace - return restSessionCatalog.loadNamespaceMetadata(scope.context(), toRemoteNamespace(session, toNamespace(namespace))).entrySet().stream() + return restSessionCatalog.loadNamespaceMetadata(convert(session), toRemoteNamespace(session, toNamespace(namespace))).entrySet().stream() .filter(property -> SUPPORTED_SCHEMA_PROPERTIES.contains(property.getKey())) .collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)); } @@ -259,9 +258,9 @@ public Optional getNamespacePrincipal(ConnectorSession session, @Override public void createNamespace(ConnectorSession session, String namespace, Map properties, TrinoPrincipal owner) { - try (var scope = convert(session)) { + try { restSessionCatalog.createNamespace( - scope.context(), + convert(session), toNamespace(namespace), Maps.transformValues(properties, property -> { if (property instanceof String stringProperty) { @@ -290,59 +289,57 @@ public void renameNamespace(ConnectorSession session, String source, String targ @Override public List listTables(ConnectorSession session, Optional namespace) { - try (var scope = convert(session)) { - List namespaces = listNamespaces(session, namespace); + SessionContext sessionContext = convert(session); + List namespaces = listNamespaces(session, namespace); - ImmutableList.Builder tables = ImmutableList.builder(); - for (Namespace restNamespace : namespaces) { + ImmutableList.Builder tables = ImmutableList.builder(); + for (Namespace restNamespace : namespaces) { + listTableIdentifiers(restNamespace, () -> { + try { + return restSessionCatalog.listTables(sessionContext, toRemoteNamespace(session, restNamespace)); + } + catch (RESTException e) { + throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list tables", e); + } + }).stream() + .map(id -> new TableInfo(SchemaTableName.schemaTableName(toSchemaName(id.namespace()), id.name()), TableInfo.ExtendedRelationType.TABLE)) + .forEach(tables::add); + if (viewEndpointsEnabled) { listTableIdentifiers(restNamespace, () -> { try { - return restSessionCatalog.listTables(scope.context(), toRemoteNamespace(session, restNamespace)); + return restSessionCatalog.listViews(sessionContext, toRemoteNamespace(session, restNamespace)); } catch (RESTException e) { - throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list tables", e); + throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list views", e); } }).stream() - .map(id -> new TableInfo(SchemaTableName.schemaTableName(toSchemaName(id.namespace()), id.name()), TableInfo.ExtendedRelationType.TABLE)) + .map(id -> new TableInfo(SchemaTableName.schemaTableName(toSchemaName(id.namespace()), id.name()), TableInfo.ExtendedRelationType.OTHER_VIEW)) .forEach(tables::add); - if (viewEndpointsEnabled) { - listTableIdentifiers(restNamespace, () -> { - try { - return restSessionCatalog.listViews(scope.context(), toRemoteNamespace(session, restNamespace)); - } - catch (RESTException e) { - throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list views", e); - } - }).stream() - .map(id -> new TableInfo(SchemaTableName.schemaTableName(toSchemaName(id.namespace()), id.name()), TableInfo.ExtendedRelationType.OTHER_VIEW)) - .forEach(tables::add); - } } - return tables.build(); } + return tables.build(); } @Override public List listIcebergTables(ConnectorSession session, Optional namespace) { - try (var scope = convert(session)) { - List namespaces = listNamespaces(session, namespace); + SessionContext sessionContext = convert(session); + List namespaces = listNamespaces(session, namespace); - ImmutableList.Builder tables = ImmutableList.builder(); - for (Namespace restNamespace : namespaces) { - listTableIdentifiers(restNamespace, () -> { - try { - return restSessionCatalog.listTables(scope.context(), toRemoteNamespace(session, restNamespace)); - } - catch (RESTException e) { - throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list tables", e); - } - }).stream() - .map(id -> SchemaTableName.schemaTableName(toSchemaName(id.namespace()), id.name())) - .forEach(tables::add); - } - return tables.build(); + ImmutableList.Builder tables = ImmutableList.builder(); + for (Namespace restNamespace : namespaces) { + listTableIdentifiers(restNamespace, () -> { + try { + return restSessionCatalog.listTables(sessionContext, toRemoteNamespace(session, restNamespace)); + } + catch (RESTException e) { + throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list tables", e); + } + }).stream() + .map(id -> SchemaTableName.schemaTableName(toSchemaName(id.namespace()), id.name())) + .forEach(tables::add); } + return tables.build(); } @Override @@ -352,24 +349,23 @@ public List listViews(ConnectorSession session, Optional namespaces = listNamespaces(session, namespace); + SessionContext sessionContext = convert(session); + List namespaces = listNamespaces(session, namespace); - ImmutableList.Builder viewNames = ImmutableList.builder(); - for (Namespace restNamespace : namespaces) { - listTableIdentifiers(restNamespace, () -> { - try { - return restSessionCatalog.listViews(scope.context(), toRemoteNamespace(session, restNamespace)); - } - catch (RESTException e) { - throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list views", e); - } - }).stream() - .map(id -> SchemaTableName.schemaTableName(id.namespace().toString(), id.name())) - .forEach(viewNames::add); - } - return viewNames.build(); + ImmutableList.Builder viewNames = ImmutableList.builder(); + for (Namespace restNamespace : namespaces) { + listTableIdentifiers(restNamespace, () -> { + try { + return restSessionCatalog.listViews(sessionContext, toRemoteNamespace(session, restNamespace)); + } + catch (RESTException e) { + throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list views", e); + } + }).stream() + .map(id -> SchemaTableName.schemaTableName(id.namespace().toString(), id.name())) + .forEach(viewNames::add); } + return viewNames.build(); } private static List listTableIdentifiers(Namespace restNamespace, Supplier> tableIdentifiersProvider) @@ -419,8 +415,8 @@ public Transaction newCreateTableTransaction( Optional location, Map properties) { - try (var scope = convert(session)) { - Catalog.TableBuilder tableBuilder = restSessionCatalog.buildTable(scope.context(), toRemoteTable(session, schemaTableName, true), schema) + try { + Catalog.TableBuilder tableBuilder = restSessionCatalog.buildTable(convert(session), toRemoteTable(session, schemaTableName, true), schema) .withPartitionSpec(partitionSpec) .withSortOrder(sortOrder) .withProperties(properties); @@ -445,8 +441,8 @@ public Transaction newCreateOrReplaceTableTransaction( String location, Map properties) { - try (var scope = convert(session)) { - return restSessionCatalog.buildTable(scope.context(), toRemoteTable(session, schemaTableName, true), schema) + try { + return restSessionCatalog.buildTable(convert(session), toRemoteTable(session, schemaTableName, true), schema) .withPartitionSpec(partitionSpec) .withSortOrder(sortOrder) .withLocation(location) @@ -462,8 +458,8 @@ public Transaction newCreateOrReplaceTableTransaction( public void registerTable(ConnectorSession session, SchemaTableName tableName, TableMetadata tableMetadata) { TableIdentifier tableIdentifier = TableIdentifier.of(toRemoteNamespace(session, toNamespace(tableName.getSchemaName())), tableName.getTableName()); - try (var scope = convert(session)) { - restSessionCatalog.registerTable(scope.context(), tableIdentifier, tableMetadata.metadataFileLocation()); + try { + restSessionCatalog.registerTable(convert(session), tableIdentifier, tableMetadata.metadataFileLocation()); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to register table '%s'".formatted(tableName.getTableName()), e); @@ -473,8 +469,8 @@ public void registerTable(ConnectorSession session, SchemaTableName tableName, T @Override public void unregisterTable(ConnectorSession session, SchemaTableName tableName) { - try (var scope = convert(session)) { - if (!restSessionCatalog.dropTable(scope.context(), toRemoteTable(session, tableName, true))) { + try { + if (!restSessionCatalog.dropTable(convert(session), toRemoteTable(session, tableName, true))) { throw new TableNotFoundException(tableName); } } @@ -526,8 +522,8 @@ private static void deleteTableDirectory(TrinoFileSystem fileSystem, SchemaTable private void purgeTable(ConnectorSession session, SchemaTableName schemaTableName) { - try (var scope = convert(session)) { - if (!restSessionCatalog.purgeTable(scope.context(), toRemoteTable(session, schemaTableName, true))) { + try { + if (!restSessionCatalog.purgeTable(convert(session), toRemoteTable(session, schemaTableName, true))) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to drop table '%s'".formatted(schemaTableName)); } } @@ -547,8 +543,8 @@ public void dropCorruptedTable(ConnectorSession session, SchemaTableName schemaT @Override public void renameTable(ConnectorSession session, SchemaTableName from, SchemaTableName to) { - try (var scope = convert(session)) { - restSessionCatalog.renameTable(scope.context(), toRemoteTable(session, from, true), toRemoteTable(session, to, true)); + try { + restSessionCatalog.renameTable(convert(session), toRemoteTable(session, from, true), toRemoteTable(session, to, true)); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, format("Failed to rename table %s to %s", from, to), e); @@ -561,14 +557,14 @@ public void renameTable(ConnectorSession session, SchemaTableName from, SchemaTa public BaseTable loadTable(ConnectorSession session, SchemaTableName schemaTableName) { Namespace namespace = toNamespace(schemaTableName.getSchemaName()); - try (var scope = convert(session)) { + try { return uncheckedCacheGet( tableCache, schemaTableName, () -> { BaseTable baseTable; try { - baseTable = (BaseTable) restSessionCatalog.loadTable(scope.context(), toRemoteObject(session, schemaTableName)); + baseTable = (BaseTable) restSessionCatalog.loadTable(convert(session), toRemoteObject(session, schemaTableName)); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to load table '%s'".formatted(schemaTableName.getTableName()), e); @@ -612,8 +608,8 @@ public Map> tryGetColumnMetadata(Connector public void updateTableComment(ConnectorSession session, SchemaTableName schemaTableName, Optional comment) { Table icebergTable; - try (var scope = convert(session)) { - icebergTable = restSessionCatalog.loadTable(scope.context(), toRemoteTable(session, schemaTableName, true)); + try { + icebergTable = restSessionCatalog.loadTable(convert(session), toRemoteTable(session, schemaTableName, true)); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to load table '%s'".formatted(schemaTableName.getTableName()), e); @@ -665,14 +661,14 @@ public void createView(ConnectorSession session, SchemaTableName schemaViewName, definition.getOwner().ifPresent(owner -> properties.put(ICEBERG_VIEW_RUN_AS_OWNER, owner)); definition.getComment().ifPresent(comment -> properties.put(COMMENT, comment)); Schema schema = IcebergUtil.schemaFromViewColumns(typeManager, definition.getColumns()); - try (var scope = convert(session)) { - ViewBuilder viewBuilder = restSessionCatalog.buildView(scope.context(), toRemoteView(session, schemaViewName, true)); - viewBuilder = viewBuilder.withSchema(schema) - .withQuery("trino", definition.getOriginalSql()) - .withDefaultNamespace(toRemoteNamespace(session, toNamespace(schemaViewName.getSchemaName()))) - .withDefaultCatalog(definition.getCatalog().orElse(null)) - .withProperties(properties.buildOrThrow()) - .withLocation(defaultTableLocation(session, schemaViewName)); + ViewBuilder viewBuilder = restSessionCatalog.buildView(convert(session), toRemoteView(session, schemaViewName, true)); + viewBuilder = viewBuilder.withSchema(schema) + .withQuery("trino", definition.getOriginalSql()) + .withDefaultNamespace(toRemoteNamespace(session, toNamespace(schemaViewName.getSchemaName()))) + .withDefaultCatalog(definition.getCatalog().orElse(null)) + .withProperties(properties.buildOrThrow()) + .withLocation(defaultTableLocation(session, schemaViewName)); + try { if (replace) { viewBuilder.createOrReplace(); } @@ -688,8 +684,8 @@ public void createView(ConnectorSession session, SchemaTableName schemaViewName, @Override public void renameView(ConnectorSession session, SchemaTableName source, SchemaTableName target) { - try (var scope = convert(session)) { - restSessionCatalog.renameView(scope.context(), toRemoteView(session, source, true), toRemoteView(session, target, true)); + try { + restSessionCatalog.renameView(convert(session), toRemoteView(session, source, true), toRemoteView(session, target, true)); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to rename view '%s' to '%s'".formatted(source, target), e); @@ -706,8 +702,8 @@ public void setViewPrincipal(ConnectorSession session, SchemaTableName schemaVie @Override public void dropView(ConnectorSession session, SchemaTableName schemaViewName) { - try (var scope = convert(session)) { - restSessionCatalog.dropView(scope.context(), toRemoteView(session, schemaViewName, true)); + try { + restSessionCatalog.dropView(convert(session), toRemoteView(session, schemaViewName, true)); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to drop view '%s'".formatted(schemaViewName.getTableName()), e); @@ -718,33 +714,32 @@ public void dropView(ConnectorSession session, SchemaTableName schemaViewName) @Override public Map getViews(ConnectorSession session, Optional namespace) { - try (var scope = convert(session)) { - ImmutableMap.Builder views = ImmutableMap.builder(); - for (Namespace restNamespace : listNamespaces(session, namespace)) { - List restViews; + SessionContext sessionContext = convert(session); + ImmutableMap.Builder views = ImmutableMap.builder(); + for (Namespace restNamespace : listNamespaces(session, namespace)) { + List restViews; + try { + restViews = restSessionCatalog.listViews(sessionContext, toRemoteNamespace(session, restNamespace)); + } + catch (RESTException e) { + throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list views", e); + } + for (TableIdentifier restView : restViews) { + SchemaTableName schemaTableName = SchemaTableName.schemaTableName(restView.namespace().toString(), restView.name()); try { - restViews = restSessionCatalog.listViews(scope.context(), toRemoteNamespace(session, restNamespace)); + getView(session, schemaTableName).ifPresent(view -> views.put(schemaTableName, view)); } - catch (RESTException e) { - throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list views", e); - } - for (TableIdentifier restView : restViews) { - SchemaTableName schemaTableName = SchemaTableName.schemaTableName(restView.namespace().toString(), restView.name()); - try { - getView(session, schemaTableName).ifPresent(view -> views.put(schemaTableName, view)); - } - catch (TrinoException e) { - if (e.getErrorCode().equals(ICEBERG_UNSUPPORTED_VIEW_DIALECT.toErrorCode())) { - log.debug(e, "Skip unsupported view dialect: %s", schemaTableName); - continue; - } - throw e; + catch (TrinoException e) { + if (e.getErrorCode().equals(ICEBERG_UNSUPPORTED_VIEW_DIALECT.toErrorCode())) { + log.debug(e, "Skip unsupported view dialect: %s", schemaTableName); + continue; } + throw e; } } - - return views.buildOrThrow(); } + + return views.buildOrThrow(); } @Override @@ -776,8 +771,8 @@ private Optional getIcebergView(ConnectorSession session, SchemaTableName return Optional.empty(); } - try (var scope = convert(session)) { - return Optional.of(restSessionCatalog.loadView(scope.context(), toRemoteView(session, viewName, getCached))); + try { + return Optional.of(restSessionCatalog.loadView(convert(session), toRemoteView(session, viewName, getCached))); } catch (NoSuchViewException e) { return Optional.empty(); @@ -881,10 +876,9 @@ public void updateViewColumnComment(ConnectorSession session, SchemaTableName sc replaceViewVersion.commit(); } - private SessionScope convert(ConnectorSession session) + private SessionCatalog.SessionContext convert(ConnectorSession session) { - ForwardingFileIo.IdentityScope identityScope = ForwardingFileIo.withIdentity(session.getIdentity()); - SessionCatalog.SessionContext context = switch (sessionType) { + return switch (sessionType) { case NONE -> new SessionContext(randomUUID().toString(), null, credentials, ImmutableMap.of(), session.getIdentity()); case USER -> { String sessionId = format("%s-%s", session.getUser(), session.getSource().orElse("default")); @@ -915,17 +909,6 @@ private SessionScope convert(ConnectorSession session) yield new SessionCatalog.SessionContext(sessionId, session.getUser(), credentials, properties, session.getIdentity()); } }; - return new SessionScope(context, identityScope); - } - - private record SessionScope(SessionCatalog.SessionContext context, ForwardingFileIo.IdentityScope identityScope) - implements AutoCloseable - { - @Override - public void close() - { - identityScope.close(); - } } private void invalidateTableCache(SchemaTableName schemaTableName) @@ -982,8 +965,8 @@ private TableIdentifier findRemoteTable(ConnectorSession session, TableIdentifie { Namespace remoteNamespace = toRemoteNamespace(session, tableIdentifier.namespace()); List tableIdentifiers; - try (var scope = convert(session)) { - tableIdentifiers = restSessionCatalog.listTables(scope.context(), remoteNamespace); + try { + tableIdentifiers = restSessionCatalog.listTables(convert(session), remoteNamespace); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list tables", e); @@ -1015,8 +998,8 @@ private TableIdentifier findRemoteView(ConnectorSession session, TableIdentifier Namespace remoteNamespace = toRemoteNamespace(session, tableIdentifier.namespace()); List tableIdentifiers; - try (var scope = convert(session)) { - tableIdentifiers = restSessionCatalog.listViews(scope.context(), remoteNamespace); + try { + tableIdentifiers = restSessionCatalog.listViews(convert(session), remoteNamespace); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list views", e); @@ -1067,8 +1050,8 @@ private Namespace findRemoteNamespace(ConnectorSession session, Namespace trinoN private List listNamespaces(ConnectorSession session, Namespace parentNamespace) { List childNamespaces; - try (var scope = convert(session)) { - childNamespaces = restSessionCatalog.listNamespaces(scope.context(), parentNamespace); + try { + childNamespaces = restSessionCatalog.listNamespaces(convert(session), parentNamespace); } catch (RESTException e) { throw new TrinoException(ICEBERG_CATALOG_ERROR, "Failed to list namespaces", e); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java index 0677149090e4..3789d5ef894f 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java @@ -36,15 +36,15 @@ 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; -import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.stream.Stream; -import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.util.concurrent.MoreExecutors.newDirectExecutorService; import static io.trino.plugin.base.util.ExecutorUtil.processWithAdditionalThreads; @@ -58,18 +58,15 @@ public class ForwardingFileIo private static final int DELETE_BATCH_SIZE = 1000; private static final int BATCH_DELETE_PATHS_MESSAGE_LIMIT = 5; - public static final String TRINO_CATALOG_NAME = "trino.catalog.name"; + private final TrinoFileSystem fileSystem; + private final Map properties; + private final boolean useFileSizeFromMetadata; + private final ExecutorService deleteExecutor; + private final IcebergFileSystemFactory storageCredentialFileSystemFactory; + private final ConnectorIdentity storageCredentialIdentity; - private static final ConcurrentHashMap contextRegistry = new ConcurrentHashMap<>(); - private static final ThreadLocal currentIdentity = new ThreadLocal<>(); - - private TrinoFileSystem fileSystem; - private Map properties = ImmutableMap.of(); - private boolean useFileSizeFromMetadata; - private ExecutorService deleteExecutor = newDirectExecutorService(); private volatile List storageCredentials = ImmutableList.of(); - - public ForwardingFileIo() {} + private volatile Map prefixedFileSystems = ImmutableMap.of(); @VisibleForTesting public ForwardingFileIo(TrinoFileSystem fileSystem, boolean useFileSizeFromMetadata) @@ -78,22 +75,36 @@ public ForwardingFileIo(TrinoFileSystem fileSystem, boolean useFileSizeFromMetad } public ForwardingFileIo(TrinoFileSystem fileSystem, Map properties, boolean useFileSizeFromMetadata, ExecutorService deleteExecutor) + { + this(fileSystem, properties, useFileSizeFromMetadata, deleteExecutor, null, null); + } + + public ForwardingFileIo( + TrinoFileSystem fileSystem, + Map 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))); } @@ -104,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); @@ -174,7 +185,15 @@ public InputFile newInputFile(ManifestListFile manifestList) private void deleteBatch(List filesToDelete) { try { - fileSystem.deleteFiles(filesToDelete.stream().map(Location::of).toList()); + Map> locationsByFileSystem = new IdentityHashMap<>(); + for (String path : filesToDelete) { + TrinoFileSystem fileSystem = fileSystemForPath(path); + locationsByFileSystem.computeIfAbsent(fileSystem, ignored -> new ArrayList<>()) + .add(Location.of(path)); + } + for (Map.Entry> entry : locationsByFileSystem.entrySet()) { + entry.getKey().deleteFiles(entry.getValue()); + } } catch (IOException e) { throw new UncheckedIOException( @@ -191,22 +210,14 @@ private void deleteBatch(List filesToDelete) @Override public Map properties() { - List credentials = storageCredentials; - if (credentials.isEmpty()) { - return properties; - } - ImmutableMap.Builder merged = ImmutableMap.builder(); - merged.putAll(properties); - for (StorageCredential credential : credentials) { - merged.putAll(credential.config()); - } - return merged.buildKeepingLast(); + return properties; } @Override public void setCredentials(List credentials) { - this.storageCredentials = ImmutableList.copyOf(requireNonNull(credentials, "credentials is null")); + storageCredentials = ImmutableList.copyOf(requireNonNull(credentials, "credentials is null")); + rebuildPrefixedFileSystems(); } @Override @@ -215,80 +226,44 @@ public List credentials() return storageCredentials; } - @Override - public void initialize(Map properties) + private TrinoFileSystem fileSystemForPath(String path) { - String catalogName = properties.get(TRINO_CATALOG_NAME); - checkState(catalogName != null, "ForwardingFileIo requires '%s' property for initialization", TRINO_CATALOG_NAME); - CreationContext context = contextRegistry.get(catalogName); - checkState(context != null, "No creation context registered for catalog: %s", catalogName); - - ConnectorIdentity identity = currentIdentity.get(); - if (identity == null) { - // During RESTSessionCatalog.initialize(), no user session exists yet. - // Use a default identity for catalog-level operations. - identity = ConnectorIdentity.ofUser("trino"); + TrinoFileSystem matchingFileSystem = fileSystem; + int matchingPrefixLength = -1; + for (Map.Entry prefixedFileSystem : prefixedFileSystems.entrySet()) { + String prefix = prefixedFileSystem.getKey(); + if (path.startsWith(prefix) && prefix.length() > matchingPrefixLength) { + matchingPrefixLength = prefix.length(); + matchingFileSystem = prefixedFileSystem.getValue(); + } } - - this.properties = ImmutableMap.copyOf(requireNonNull(properties, "properties is null")); - // Use merged properties (base + storage credentials) so that the file system factory - // can see vended credentials like s3.access-key-id from storage-credentials - this.fileSystem = context.fileSystemFactory().create(identity, properties()); - this.useFileSizeFromMetadata = true; - this.deleteExecutor = context.deleteExecutor(); - } - - public static void registerContext(String catalogName, IcebergFileSystemFactory fileSystemFactory, ExecutorService deleteExecutor) - { - requireNonNull(catalogName, "catalogName is null"); - contextRegistry.put(catalogName, new CreationContext( - requireNonNull(fileSystemFactory, "fileSystemFactory is null"), - requireNonNull(deleteExecutor, "deleteExecutor is null"))); + return matchingFileSystem; } - public static void deregisterContext(String catalogName) + private void rebuildPrefixedFileSystems() { - contextRegistry.remove(requireNonNull(catalogName, "catalogName is null")); - } - - public static IdentityScope withIdentity(ConnectorIdentity identity) - { - ConnectorIdentity previous = currentIdentity.get(); - currentIdentity.set(requireNonNull(identity, "identity is null")); - return new IdentityScope(previous); - } - - public static final class IdentityScope - implements AutoCloseable - { - private final ConnectorIdentity previous; - - private IdentityScope(ConnectorIdentity previous) - { - this.previous = previous; + if (storageCredentials.isEmpty() || storageCredentialFileSystemFactory == null || storageCredentialIdentity == null) { + prefixedFileSystems = ImmutableMap.of(); + return; } - @Override - public void close() - { - if (previous == null) { - currentIdentity.remove(); - } - else { - currentIdentity.set(previous); - } + ImmutableMap.Builder rebuiltFileSystems = ImmutableMap.builder(); + for (StorageCredential storageCredential : storageCredentials) { + Map mergedProperties = ImmutableMap.builder() + .putAll(properties) + .putAll(storageCredential.config()) + .buildKeepingLast(); + rebuiltFileSystems.put(storageCredential.prefix(), storageCredentialFileSystemFactory.create(storageCredentialIdentity, mergedProperties)); } + prefixedFileSystems = rebuiltFileSystems.buildKeepingLast(); } @Override - public void close() {} - - private record CreationContext(IcebergFileSystemFactory fileSystemFactory, ExecutorService deleteExecutor) + public void initialize(Map properties) { - CreationContext - { - requireNonNull(fileSystemFactory, "fileSystemFactory is null"); - requireNonNull(deleteExecutor, "deleteExecutor is null"); - } + throw new UnsupportedOperationException("ForwardingFileIO does not support initialization by properties"); } + + @Override + public void close() {} } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIoFactory.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIoFactory.java index 5d19b479c84a..7ba2fbd51dfd 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIoFactory.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIoFactory.java @@ -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; @@ -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 properties, + IcebergFileSystemFactory storageCredentialFileSystemFactory, + ConnectorIdentity storageCredentialIdentity) + { + return new ForwardingFileIo( + fileSystem, + properties, + useFileSizeFromMetadata, + deleteExecutor, + storageCredentialFileSystemFactory, + storageCredentialIdentity); + } } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergStorageCredentialsRestCatalog.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergStorageCredentialsRestCatalog.java index fb10f0d42ea4..57eebce4f630 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergStorageCredentialsRestCatalog.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergStorageCredentialsRestCatalog.java @@ -18,6 +18,7 @@ import io.trino.plugin.iceberg.IcebergQueryRunner; import io.trino.testing.AbstractTestQueryFramework; import io.trino.testing.QueryRunner; +import org.apache.iceberg.aws.s3.S3FileIOProperties; import org.apache.iceberg.rest.DelegatingRestSessionCatalog; import org.apache.iceberg.rest.credentials.ImmutableCredential; import org.junit.jupiter.api.Test; @@ -53,9 +54,9 @@ protected QueryRunner createQueryRunner() ImmutableCredential.builder() .prefix("file://") .config(ImmutableMap.of( - "s3.access-key-id", "test-vended-access-key", - "s3.secret-access-key", "test-vended-secret-key", - "s3.session-token", "test-vended-session-token")) + S3FileIOProperties.ACCESS_KEY_ID, "test-vended-access-key", + S3FileIOProperties.SECRET_ACCESS_KEY, "test-vended-secret-key", + S3FileIOProperties.SESSION_TOKEN, "test-vended-session-token")) .build())) .build(); @@ -87,24 +88,4 @@ public void testCreateAndReadTableWithStorageCredentials() assertUpdate("DROP TABLE test_storage_creds.test_table"); assertUpdate("DROP SCHEMA test_storage_creds"); } - - @Test - public void testStorageCredentialsWithMultipleOperations() - { - assertUpdate("CREATE SCHEMA test_multi_ops"); - assertUpdate("CREATE TABLE test_multi_ops.t1 (x INTEGER)"); - assertUpdate("INSERT INTO test_multi_ops.t1 VALUES 1, 2, 3", 3); - - // Read back - assertThat(query("SELECT count(*) FROM test_multi_ops.t1")) - .matches("VALUES BIGINT '3'"); - - // Additional insert (triggers new table load with credentials) - assertUpdate("INSERT INTO test_multi_ops.t1 VALUES 4, 5", 2); - assertThat(query("SELECT count(*) FROM test_multi_ops.t1")) - .matches("VALUES BIGINT '5'"); - - assertUpdate("DROP TABLE test_multi_ops.t1"); - assertUpdate("DROP SCHEMA test_multi_ops"); - } } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/fileio/TestForwardingFileIo.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/fileio/TestForwardingFileIo.java index 6ed9230aa646..c5b20f55a197 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/fileio/TestForwardingFileIo.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/fileio/TestForwardingFileIo.java @@ -16,7 +16,9 @@ import com.google.common.collect.ImmutableMap; import io.trino.filesystem.TrinoFileSystem; import io.trino.filesystem.local.LocalFileSystemFactory; +import io.trino.plugin.iceberg.IcebergFileSystemFactory; import io.trino.spi.security.ConnectorIdentity; +import org.apache.iceberg.aws.s3.S3FileIOProperties; import org.apache.iceberg.io.FileIO; import org.apache.iceberg.io.StorageCredential; import org.apache.iceberg.io.SupportsBulkOperations; @@ -25,7 +27,9 @@ import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; import java.util.List; +import java.util.Map; import static com.google.common.io.MoreFiles.deleteRecursively; import static com.google.common.io.RecursiveDeleteOption.ALLOW_INSECURE; @@ -70,129 +74,91 @@ public void testUseFileSizeFromMetadata() } @Test - public void testStorageCredentialsMergedIntoProperties() + public void testStorageCredentialsRemainSeparateFromProperties() + throws Exception { - try (ForwardingFileIo fileIo = new ForwardingFileIo( - new LocalFileSystemFactory(Path.of(System.getProperty("java.io.tmpdir"))).create(SESSION), - ImmutableMap.of("base.key", "base.value"), - true, - newDirectExecutorService())) { - // initially no credentials - assertThat(fileIo.credentials()).isEmpty(); - assertThat(fileIo.properties()).containsExactlyEntriesOf(ImmutableMap.of("base.key", "base.value")); - - // set storage credentials — config entries should appear in properties - List credentials = List.of( - StorageCredential.create("s3://bucket/", ImmutableMap.of( - "s3.access-key-id", "test-access-key", - "s3.secret-access-key", "test-secret-key", - "s3.session-token", "test-session-token")), - StorageCredential.create("gs://bucket/", ImmutableMap.of( - "gcs.oauth2.token", "test-gcs-token"))); - fileIo.setCredentials(credentials); - - assertThat(fileIo.credentials()).isEqualTo(credentials); - assertThat(fileIo.properties()) - .containsEntry("base.key", "base.value") - .containsEntry("s3.access-key-id", "test-access-key") - .containsEntry("s3.secret-access-key", "test-secret-key") - .containsEntry("s3.session-token", "test-session-token") - .containsEntry("gcs.oauth2.token", "test-gcs-token"); - } - } + Path tempDir = Files.createTempDirectory("test_forwarding_fileio_storage_creds"); + List> seenProperties = new ArrayList<>(); + LocalFileSystemFactory localFactory = new LocalFileSystemFactory(tempDir); + IcebergFileSystemFactory capturingFactory = (identity, fileIoProperties) -> { + seenProperties.add(ImmutableMap.copyOf(fileIoProperties)); + return localFactory.create(SESSION); + }; - @Test - public void testStorageCredentialsTakePrecedenceOverBaseProperties() - { try (ForwardingFileIo fileIo = new ForwardingFileIo( - new LocalFileSystemFactory(Path.of(System.getProperty("java.io.tmpdir"))).create(SESSION), - ImmutableMap.of("s3.access-key-id", "static-key", "unrelated.key", "keep-me"), + localFactory.create(SESSION), + ImmutableMap.of("base.key", "base-value"), true, - newDirectExecutorService())) { + newDirectExecutorService(), + capturingFactory, + ConnectorIdentity.ofUser("test-user"))) { fileIo.setCredentials(List.of( - StorageCredential.create("s3://bucket/", ImmutableMap.of( - "s3.access-key-id", "vended-key")))); - - assertThat(fileIo.properties()) - .containsEntry("s3.access-key-id", "vended-key") - .containsEntry("unrelated.key", "keep-me"); + StorageCredential.create("s3://bucket-a/", ImmutableMap.of( + S3FileIOProperties.ACCESS_KEY_ID, "access-a", + S3FileIOProperties.SECRET_ACCESS_KEY, "secret-a", + S3FileIOProperties.SESSION_TOKEN, "token-a")), + StorageCredential.create("s3://bucket-a/warehouse/", ImmutableMap.of( + S3FileIOProperties.ACCESS_KEY_ID, "access-b", + S3FileIOProperties.SECRET_ACCESS_KEY, "secret-b", + S3FileIOProperties.SESSION_TOKEN, "token-b")))); + + assertThat(fileIo.properties()).containsExactlyEntriesOf(ImmutableMap.of("base.key", "base-value")); + assertThat(fileIo.credentials()).hasSize(2); + assertThat(seenProperties).hasSize(2); + assertThat(seenProperties).contains(ImmutableMap.of( + "base.key", "base-value", + S3FileIOProperties.ACCESS_KEY_ID, "access-a", + S3FileIOProperties.SECRET_ACCESS_KEY, "secret-a", + S3FileIOProperties.SESSION_TOKEN, "token-a")); + assertThat(seenProperties).contains(ImmutableMap.of( + "base.key", "base-value", + S3FileIOProperties.ACCESS_KEY_ID, "access-b", + S3FileIOProperties.SECRET_ACCESS_KEY, "secret-b", + S3FileIOProperties.SESSION_TOKEN, "token-b")); + } + finally { + deleteRecursively(tempDir, ALLOW_INSECURE); } } @Test - public void testEmptyStorageCredentialsRevertToBaseProperties() + public void testSetCredentialsAfterInitializeRebuildsPrefixFileSystems() + throws Exception { + Path tempDir = Files.createTempDirectory("test_forwarding_fileio_refresh_creds"); + List> seenProperties = new ArrayList<>(); + LocalFileSystemFactory localFactory = new LocalFileSystemFactory(tempDir); + IcebergFileSystemFactory capturingFactory = (identity, fileIoProperties) -> { + seenProperties.add(ImmutableMap.copyOf(fileIoProperties)); + return localFactory.create(SESSION); + }; + try (ForwardingFileIo fileIo = new ForwardingFileIo( - new LocalFileSystemFactory(Path.of(System.getProperty("java.io.tmpdir"))).create(SESSION), - ImmutableMap.of("base.key", "base.value"), + localFactory.create(SESSION), + ImmutableMap.of("base.key", "base-value"), true, - newDirectExecutorService())) { + newDirectExecutorService(), + capturingFactory, + ConnectorIdentity.ofUser("test-user"))) { fileIo.setCredentials(List.of( - StorageCredential.create("s3://bucket/", ImmutableMap.of( - "s3.access-key-id", "vended-key")))); - - assertThat(fileIo.properties()).containsKey("s3.access-key-id"); + StorageCredential.create("s3://bucket-a/", ImmutableMap.of( + S3FileIOProperties.ACCESS_KEY_ID, "access-a", + S3FileIOProperties.SECRET_ACCESS_KEY, "secret-a", + S3FileIOProperties.SESSION_TOKEN, "token-a")))); + assertThat(seenProperties).hasSize(1); + assertThat(seenProperties.get(0)).containsEntry(S3FileIOProperties.ACCESS_KEY_ID, "access-a"); - // reset to empty - fileIo.setCredentials(List.of()); - - assertThat(fileIo.credentials()).isEmpty(); - assertThat(fileIo.properties()).containsExactlyEntriesOf(ImmutableMap.of("base.key", "base.value")); - } - } - - @Test - public void testInitializeWithRegisteredContext() - { - String catalogName = "test-init-" + Thread.currentThread().threadId(); - Path tempDir = Path.of(System.getProperty("java.io.tmpdir")); - LocalFileSystemFactory localFactory = new LocalFileSystemFactory(tempDir); - ForwardingFileIo.registerContext(catalogName, - (identity, props) -> localFactory.create(SESSION), - newDirectExecutorService()); - try (ForwardingFileIo.IdentityScope ignored = ForwardingFileIo.withIdentity(ConnectorIdentity.ofUser("test-user")); - ForwardingFileIo fileIo = new ForwardingFileIo()) { - fileIo.initialize(ImmutableMap.of( - ForwardingFileIo.TRINO_CATALOG_NAME, catalogName, - "base.key", "base.value")); - - assertThat(fileIo.properties()) - .containsEntry("base.key", "base.value"); - } - finally { - ForwardingFileIo.deregisterContext(catalogName); - } - } + fileIo.setCredentials(List.of( + StorageCredential.create("s3://bucket-b/", ImmutableMap.of( + S3FileIOProperties.ACCESS_KEY_ID, "access-b", + S3FileIOProperties.SECRET_ACCESS_KEY, "secret-b", + S3FileIOProperties.SESSION_TOKEN, "token-b")))); - @Test - public void testSetCredentialsThenInitialize() - { - String catalogName = "test-creds-" + Thread.currentThread().threadId(); - Path tempDir = Path.of(System.getProperty("java.io.tmpdir")); - LocalFileSystemFactory localFactory = new LocalFileSystemFactory(tempDir); - ForwardingFileIo.registerContext(catalogName, - (identity, props) -> localFactory.create(SESSION), - newDirectExecutorService()); - try (ForwardingFileIo.IdentityScope ignored = ForwardingFileIo.withIdentity(ConnectorIdentity.ofUser("test-user")); - ForwardingFileIo fileIo = new ForwardingFileIo()) { - // CatalogUtil.loadFileIO calls setCredentials BEFORE initialize - List credentials = List.of( - StorageCredential.create("s3://bucket/", ImmutableMap.of( - "s3.access-key-id", "vended-key", - "s3.secret-access-key", "vended-secret"))); - fileIo.setCredentials(credentials); - fileIo.initialize(ImmutableMap.of( - ForwardingFileIo.TRINO_CATALOG_NAME, catalogName, - "s3.access-key-id", "static-key")); - - // storage credentials take precedence over base properties - assertThat(fileIo.properties()) - .containsEntry("s3.access-key-id", "vended-key") - .containsEntry("s3.secret-access-key", "vended-secret"); - assertThat(fileIo.credentials()).isEqualTo(credentials); + assertThat(seenProperties).hasSize(2); + assertThat(seenProperties.get(1)).containsEntry(S3FileIOProperties.ACCESS_KEY_ID, "access-b"); } finally { - ForwardingFileIo.deregisterContext(catalogName); + deleteRecursively(tempDir, ALLOW_INSECURE); } } } From 3af270d41e88d68d2d77f89c976b6370fcfbefdc Mon Sep 17 00:00:00 2001 From: kaveti Date: Thu, 9 Apr 2026 15:45:17 +0530 Subject: [PATCH 8/9] Harden storage-credential routing test coverage. Add a longest-prefix routing/fallback test for ForwardingFileIo and preserve any existing LoadTableResponse credentials in the test REST adapter to better model mixed server responses. Made-with: Cursor --- .../iceberg/fileio/TestForwardingFileIo.java | 60 +++++++++++++++++++ .../rest/DelegatingRestSessionCatalog.java | 1 + 2 files changed, 61 insertions(+) diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/fileio/TestForwardingFileIo.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/fileio/TestForwardingFileIo.java index c5b20f55a197..907046e9478c 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/fileio/TestForwardingFileIo.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/fileio/TestForwardingFileIo.java @@ -161,4 +161,64 @@ public void testSetCredentialsAfterInitializeRebuildsPrefixFileSystems() deleteRecursively(tempDir, ALLOW_INSECURE); } } + + @Test + public void testLongestPrefixRoutingAndFallback() + throws Exception + { + Path tempDir = Files.createTempDirectory("test_forwarding_fileio_prefix_routing"); + Path baseRoot = tempDir.resolve("base"); + Path prefixARoot = tempDir.resolve("prefix-a"); + Path prefixBRoot = tempDir.resolve("prefix-b"); + Files.createDirectories(baseRoot); + Files.createDirectories(prefixARoot); + Files.createDirectories(prefixBRoot); + + Files.createDirectories(baseRoot.resolve("other")); + Files.writeString(baseRoot.resolve("other/fallback.txt"), "ccc"); + Files.createDirectories(prefixARoot.resolve("bucket")); + Files.writeString(prefixARoot.resolve("bucket/data.txt"), "a"); + Files.createDirectories(prefixBRoot.resolve("bucket/warehouse")); + Files.writeString(prefixBRoot.resolve("bucket/warehouse/data.txt"), "bb"); + + LocalFileSystemFactory baseFactory = new LocalFileSystemFactory(baseRoot); + LocalFileSystemFactory prefixAFactory = new LocalFileSystemFactory(prefixARoot); + LocalFileSystemFactory prefixBFactory = new LocalFileSystemFactory(prefixBRoot); + IcebergFileSystemFactory routingFactory = (identity, fileIoProperties) -> { + String accessKey = fileIoProperties.get(S3FileIOProperties.ACCESS_KEY_ID); + if ("access-a".equals(accessKey)) { + return prefixAFactory.create(SESSION); + } + if ("access-b".equals(accessKey)) { + return prefixBFactory.create(SESSION); + } + throw new IllegalArgumentException("Unexpected access key: " + accessKey); + }; + + try (ForwardingFileIo fileIo = new ForwardingFileIo( + baseFactory.create(SESSION), + ImmutableMap.of("base.key", "base-value"), + true, + newDirectExecutorService(), + routingFactory, + ConnectorIdentity.ofUser("test-user"))) { + fileIo.setCredentials(List.of( + StorageCredential.create("file:///bucket/", ImmutableMap.of( + S3FileIOProperties.ACCESS_KEY_ID, "access-a", + S3FileIOProperties.SECRET_ACCESS_KEY, "secret-a", + S3FileIOProperties.SESSION_TOKEN, "token-a")), + StorageCredential.create("file:///bucket/warehouse/", ImmutableMap.of( + S3FileIOProperties.ACCESS_KEY_ID, "access-b", + S3FileIOProperties.SECRET_ACCESS_KEY, "secret-b", + S3FileIOProperties.SESSION_TOKEN, "token-b")))); + + // Longest matching prefix should select the more specific credential filesystem. + assertThat(fileIo.newInputFile("file:///bucket/warehouse/data.txt").getLength()).isEqualTo(2); + assertThat(fileIo.newInputFile("file:///bucket/data.txt").getLength()).isEqualTo(1); + assertThat(fileIo.newInputFile("file:///other/fallback.txt").getLength()).isEqualTo(3); + } + finally { + deleteRecursively(tempDir, ALLOW_INSECURE); + } + } } diff --git a/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/DelegatingRestSessionCatalog.java b/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/DelegatingRestSessionCatalog.java index 2cb0ac0955fd..28e82e5bfcfe 100644 --- a/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/DelegatingRestSessionCatalog.java +++ b/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/DelegatingRestSessionCatalog.java @@ -136,6 +136,7 @@ public T handleRequest( LoadTableResponse.Builder builder = LoadTableResponse.builder() .withTableMetadata(loadTableResponse.tableMetadata()) .addAllConfig(loadTableResponse.config()) + .addAllCredentials(loadTableResponse.credentials()) .addAllCredentials(credentials); @SuppressWarnings("unchecked") T result = (T) builder.build(); From c64e9e237def614764bb63ba874d4786c810d236 Mon Sep 17 00:00:00 2001 From: kaveti Date: Thu, 9 Apr 2026 16:36:23 +0530 Subject: [PATCH 9/9] Document REST fixture credentials behavior with a real backend test. Add a smoke test that asserts iceberg-rest-fixture vends S3 credentials via the LoadTable config map and not via storage-credentials, and keep imports checkstyle-compliant in that test. Made-with: Cursor --- ...3VendingRestCatalogConnectorSmokeTest.java | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3VendingRestCatalogConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3VendingRestCatalogConnectorSmokeTest.java index bb8b94e45e47..d3e4443e5c1b 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3VendingRestCatalogConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3VendingRestCatalogConnectorSmokeTest.java @@ -30,6 +30,7 @@ import io.trino.testing.minio.MinioClient; import org.apache.iceberg.BaseTable; import org.apache.iceberg.CatalogProperties; +import org.apache.iceberg.aws.s3.S3FileIOProperties; import org.apache.iceberg.catalog.SessionCatalog; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.rest.RESTSessionCatalog; @@ -46,6 +47,9 @@ import java.io.IOException; import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; import java.nio.file.Files; import java.nio.file.Path; import java.util.Optional; @@ -311,6 +315,33 @@ public void testDropTableWithNonExistentTableLocation() .hasMessageMatching("Failed to load table: (.*)"); } + @Test + public void testRestFixtureVendsCredentialsViaConfigMap() + throws Exception + { + String tableName = "config_credentials_" + randomNameSuffix(); + assertUpdate("CREATE TABLE " + tableName + " AS SELECT 1 value", 1); + try { + String schemaName = getSession().getSchema().orElseThrow(); + HttpRequest request = HttpRequest.newBuilder() + .uri(URI.create("http://" + restCatalogBackendContainer.getRestCatalogEndpoint() + "/v1/namespaces/" + schemaName + "/tables/" + tableName)) + .GET() + .build(); + + HttpResponse response = HttpClient.newHttpClient().send(request, HttpResponse.BodyHandlers.ofString()); + assertThat(response.statusCode()).isEqualTo(200); + assertThat(response.body()) + .contains("\"config\"") + .contains("\"" + S3FileIOProperties.ACCESS_KEY_ID + "\"") + .contains("\"" + S3FileIOProperties.SECRET_ACCESS_KEY + "\"") + .contains("\"" + S3FileIOProperties.SESSION_TOKEN + "\"") + .doesNotContain("\"storage-credentials\""); + } + finally { + assertUpdate("DROP TABLE " + tableName); + } + } + @Override protected boolean isFileSorted(Location path, String sortColumnName) {