flyingImer commented on code in PR #4052:
URL: https://github.com/apache/polaris/pull/4052#discussion_r3024435967
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -855,8 +865,35 @@ private LoadTableResponse.Builder
buildLoadTableResponseWithDelegationCredential
Set<String> tableLocations =
StorageUtil.getLocationsUsedByTable(tableMetadata);
- // For non polaris' catalog, validate that table locations are within
allowed locations
- if (!(baseCatalog instanceof IcebergCatalog)) {
+ // For S3 Tables catalogs, replace s3:// table locations with the
constructed table ARN.
+ // s3tables:* IAM actions require ARN resources, not s3:// paths.
+ CatalogEntity catalogEntity =
CatalogEntity.of(getResolvedCatalogEntity());
+ boolean isS3Tables =
+ catalogEntity.getStorageConfigurationInfo() != null
+ && catalogEntity.getStorageConfigurationInfo().getStorageType()
+ == PolarisStorageConfigurationInfo.StorageType.S3_TABLES;
+
+ if (isS3Tables && capturedTableId.isPresent()) {
+ String tableArn = constructS3TablesArn(catalogEntity,
capturedTableId.get());
+ validateS3TablesArn(tableIdentifier, tableArn, catalogEntity);
+ tableLocations = Set.of(tableArn);
+ LOGGER
+ .atDebug()
+ .addKeyValue("tableIdentifier", tableIdentifier)
Review Comment:
This is the main remaining gap for me. If we already know the catalog is
`S3_TABLES`, I wonder if this should just fail closed once `tableId`/table ARN
cannot be derived. Otherwise the dedicated path is better, but the
unresolved-scope/create-time story still feels incomplete to me, because the
downstream S3 Tables policy path is still being reached without validated
table-ARN-shaped input.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/validation/StorageTypeFileIO.java:
##########
@@ -32,6 +32,8 @@ enum StorageTypeFileIO {
FILE("org.apache.iceberg.hadoop.HadoopFileIO", false),
+ S3_TABLES("org.apache.iceberg.aws.s3.S3FileIO", true),
Review Comment:
I think the issue here is not the dedicated S3 Tables path itself. That part
feels directionally right. The issue is that `StorageTypeFileIO` no longer
gives you a stable discriminator once both `S3` and `S3_TABLES` map to the same
`S3FileIO`, but the reverse lookup still assumes FileIO impl uniquely
determines storage type. Would it make sense to switch this validation path to
a discriminator that remains explicit, instead of depending on shared FileIO
impl here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
