From eecb91353c709f2c15c773e72c969f68b23b7cdd Mon Sep 17 00:00:00 2001 From: David Lu Date: Thu, 20 Mar 2025 13:02:14 -0400 Subject: [PATCH 1/7] populate credentials field for loadTableResponse --- .../iceberg/IcebergCatalogHandlerWrapper.java | 70 ++++++++----------- 1 file changed, 28 insertions(+), 42 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerWrapper.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerWrapper.java index c774525c84..1689d943aa 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerWrapper.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerWrapper.java @@ -56,6 +56,7 @@ import org.apache.iceberg.exceptions.NoSuchTableException; import org.apache.iceberg.exceptions.NoSuchViewException; import org.apache.iceberg.rest.CatalogHandlers; +import org.apache.iceberg.rest.credentials.ImmutableCredential; import org.apache.iceberg.rest.requests.CommitTransactionRequest; import org.apache.iceberg.rest.requests.CreateNamespaceRequest; import org.apache.iceberg.rest.requests.CreateTableRequest; @@ -637,24 +638,8 @@ public LoadTableResponse createTableDirectWithWriteDelegation( if (table instanceof BaseTable baseTable) { TableMetadata tableMetadata = baseTable.operations().current(); - LoadTableResponse.Builder responseBuilder = - LoadTableResponse.builder().withTableMetadata(tableMetadata); - if (baseCatalog instanceof SupportsCredentialDelegation credentialDelegation) { - LOGGER - .atDebug() - .addKeyValue("tableIdentifier", tableIdentifier) - .addKeyValue("tableLocation", tableMetadata.location()) - .log("Fetching client credentials for table"); - responseBuilder.addAllConfig( - credentialDelegation.getCredentialConfig( - tableIdentifier, - tableMetadata, - Set.of( - PolarisStorageActions.READ, - PolarisStorageActions.WRITE, - PolarisStorageActions.LIST))); - } - return responseBuilder.build(); + return buildLoadTableResponseWithDelegationCredentials( + tableIdentifier, tableMetadata, Set.of(PolarisStorageActions.READ, PolarisStorageActions.WRITE, PolarisStorageActions.LIST)).build(); } else if (table instanceof BaseMetadataTable) { // metadata tables are loaded on the client side, return NoSuchTableException for now throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); @@ -752,17 +737,7 @@ public LoadTableResponse createTableStagedWithWriteDelegation( LoadTableResponse.Builder responseBuilder = LoadTableResponse.builder().withTableMetadata(metadata); - if (baseCatalog instanceof SupportsCredentialDelegation credentialDelegation) { - LOGGER - .atDebug() - .addKeyValue("tableIdentifier", ident) - .addKeyValue("tableLocation", metadata.location()) - .log("Fetching client credentials for table"); - responseBuilder.addAllConfig( - credentialDelegation.getCredentialConfig( - ident, metadata, Set.of(PolarisStorageActions.ALL))); - } - return responseBuilder.build(); + return buildLoadTableResponseWithDelegationCredentials(ident, metadata, Set.of(PolarisStorageActions.ALL)).build(); }); } @@ -871,19 +846,7 @@ public LoadTableResponse loadTableWithAccessDelegation( if (table instanceof BaseTable baseTable) { TableMetadata tableMetadata = baseTable.operations().current(); - LoadTableResponse.Builder responseBuilder = - LoadTableResponse.builder().withTableMetadata(tableMetadata); - if (baseCatalog instanceof SupportsCredentialDelegation credentialDelegation) { - LOGGER - .atDebug() - .addKeyValue("tableIdentifier", tableIdentifier) - .addKeyValue("tableLocation", tableMetadata.location()) - .log("Fetching client credentials for table"); - responseBuilder.addAllConfig( - credentialDelegation.getCredentialConfig( - tableIdentifier, tableMetadata, actionsRequested)); - } - return responseBuilder.build(); + return buildLoadTableResponseWithDelegationCredentials(tableIdentifier, tableMetadata, Set.of(PolarisStorageActions.ALL)).build(); } else if (table instanceof BaseMetadataTable) { // metadata tables are loaded on the client side, return NoSuchTableException for now throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); @@ -893,6 +856,29 @@ public LoadTableResponse loadTableWithAccessDelegation( }); } + private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredentials( + TableIdentifier tableIdentifier, TableMetadata tableMetadata, Set actions) { + LoadTableResponse.Builder responseBuilder = LoadTableResponse.builder() + .withTableMetadata(tableMetadata); + if (baseCatalog instanceof SupportsCredentialDelegation credentialDelegation) { + LOGGER + .atDebug() + .addKeyValue("tableIdentifier", tableIdentifier) + .addKeyValue("tableLocation", tableMetadata.location()) + .log("Fetching client credentials for table"); + Map credentialConfig = + credentialDelegation.getCredentialConfig( + tableIdentifier, tableMetadata, actions); + responseBuilder.addAllConfig(credentialConfig); + responseBuilder.addCredential( + ImmutableCredential.builder() + .prefix(tableMetadata.location()) + .config(credentialConfig) + .build()); + } + return responseBuilder; + } + private UpdateTableRequest applyUpdateFilters(UpdateTableRequest request) { // Certain MetadataUpdates need to be explicitly transformed to achieve the same behavior // as using a local Catalog client via TableBuilder. From 92540bd72d3779c5e897e1c168920e86db3a2c0c Mon Sep 17 00:00:00 2001 From: David Lu Date: Thu, 20 Mar 2025 13:03:17 -0400 Subject: [PATCH 2/7] spotless --- .../iceberg/IcebergCatalogHandlerWrapper.java | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerWrapper.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerWrapper.java index 1689d943aa..db45df30f1 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerWrapper.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerWrapper.java @@ -639,7 +639,13 @@ public LoadTableResponse createTableDirectWithWriteDelegation( if (table instanceof BaseTable baseTable) { TableMetadata tableMetadata = baseTable.operations().current(); return buildLoadTableResponseWithDelegationCredentials( - tableIdentifier, tableMetadata, Set.of(PolarisStorageActions.READ, PolarisStorageActions.WRITE, PolarisStorageActions.LIST)).build(); + tableIdentifier, + tableMetadata, + Set.of( + PolarisStorageActions.READ, + PolarisStorageActions.WRITE, + PolarisStorageActions.LIST)) + .build(); } else if (table instanceof BaseMetadataTable) { // metadata tables are loaded on the client side, return NoSuchTableException for now throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); @@ -737,7 +743,9 @@ public LoadTableResponse createTableStagedWithWriteDelegation( LoadTableResponse.Builder responseBuilder = LoadTableResponse.builder().withTableMetadata(metadata); - return buildLoadTableResponseWithDelegationCredentials(ident, metadata, Set.of(PolarisStorageActions.ALL)).build(); + return buildLoadTableResponseWithDelegationCredentials( + ident, metadata, Set.of(PolarisStorageActions.ALL)) + .build(); }); } @@ -846,7 +854,9 @@ public LoadTableResponse loadTableWithAccessDelegation( if (table instanceof BaseTable baseTable) { TableMetadata tableMetadata = baseTable.operations().current(); - return buildLoadTableResponseWithDelegationCredentials(tableIdentifier, tableMetadata, Set.of(PolarisStorageActions.ALL)).build(); + return buildLoadTableResponseWithDelegationCredentials( + tableIdentifier, tableMetadata, Set.of(PolarisStorageActions.ALL)) + .build(); } else if (table instanceof BaseMetadataTable) { // metadata tables are loaded on the client side, return NoSuchTableException for now throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); @@ -857,9 +867,11 @@ public LoadTableResponse loadTableWithAccessDelegation( } private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredentials( - TableIdentifier tableIdentifier, TableMetadata tableMetadata, Set actions) { - LoadTableResponse.Builder responseBuilder = LoadTableResponse.builder() - .withTableMetadata(tableMetadata); + TableIdentifier tableIdentifier, + TableMetadata tableMetadata, + Set actions) { + LoadTableResponse.Builder responseBuilder = + LoadTableResponse.builder().withTableMetadata(tableMetadata); if (baseCatalog instanceof SupportsCredentialDelegation credentialDelegation) { LOGGER .atDebug() @@ -867,8 +879,7 @@ private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredential .addKeyValue("tableLocation", tableMetadata.location()) .log("Fetching client credentials for table"); Map credentialConfig = - credentialDelegation.getCredentialConfig( - tableIdentifier, tableMetadata, actions); + credentialDelegation.getCredentialConfig(tableIdentifier, tableMetadata, actions); responseBuilder.addAllConfig(credentialConfig); responseBuilder.addCredential( ImmutableCredential.builder() From 72462665ba6b661d78fe49a042f2ee4a90389de2 Mon Sep 17 00:00:00 2001 From: David Lu Date: Sat, 22 Mar 2025 14:42:59 -0400 Subject: [PATCH 3/7] spotless --- .../iceberg/IcebergCatalogHandlerWrapper.java | 56 +++++++++---------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerWrapper.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerWrapper.java index c705311114..b4decc7865 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerWrapper.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerWrapper.java @@ -602,20 +602,20 @@ public LoadTableResponse createTableDirectWithWriteDelegation( .withProperties(properties) .create(); - if (table instanceof BaseTable baseTable) { - TableMetadata tableMetadata = baseTable.operations().current(); - return buildLoadTableResponseWithDelegationCredentials( - tableIdentifier, - tableMetadata, - Set.of( - PolarisStorageActions.READ, - PolarisStorageActions.WRITE, - PolarisStorageActions.LIST)) - .build(); - } else if (table instanceof BaseMetadataTable) { - // metadata tables are loaded on the client side, return NoSuchTableException for now - throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); - } + if (table instanceof BaseTable baseTable) { + TableMetadata tableMetadata = baseTable.operations().current(); + return buildLoadTableResponseWithDelegationCredentials( + tableIdentifier, + tableMetadata, + Set.of( + PolarisStorageActions.READ, + PolarisStorageActions.WRITE, + PolarisStorageActions.LIST)) + .build(); + } else if (table instanceof BaseMetadataTable) { + // metadata tables are loaded on the client side, return NoSuchTableException for now + throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); + } throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable"); } @@ -700,9 +700,9 @@ public LoadTableResponse createTableStagedWithWriteDelegation( TableIdentifier ident = TableIdentifier.of(namespace, request.name()); TableMetadata metadata = stageTableCreateHelper(namespace, request); - return buildLoadTableResponseWithDelegationCredentials( - ident, metadata, Set.of(PolarisStorageActions.ALL)) - .build(); + return buildLoadTableResponseWithDelegationCredentials( + ident, metadata, Set.of(PolarisStorageActions.ALL)) + .build(); } public LoadTableResponse registerTable(Namespace namespace, RegisterTableRequest request) { @@ -806,17 +806,17 @@ public LoadTableResponse loadTableWithAccessDelegation( // when data-access is specified but access delegation grants are not found. Table table = baseCatalog.loadTable(tableIdentifier); - if (table instanceof BaseTable baseTable) { - TableMetadata tableMetadata = baseTable.operations().current(); - return buildLoadTableResponseWithDelegationCredentials( - tableIdentifier, tableMetadata, Set.of(PolarisStorageActions.ALL)) - .build(); - } else if (table instanceof BaseMetadataTable) { - // metadata tables are loaded on the client side, return NoSuchTableException for now - throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); - } - - throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable"); + if (table instanceof BaseTable baseTable) { + TableMetadata tableMetadata = baseTable.operations().current(); + return buildLoadTableResponseWithDelegationCredentials( + tableIdentifier, tableMetadata, Set.of(PolarisStorageActions.ALL)) + .build(); + } else if (table instanceof BaseMetadataTable) { + // metadata tables are loaded on the client side, return NoSuchTableException for now + throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); + } + + throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable"); } private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredentials( From 200341879536bbaf6d98f419220f5ea062d0222a Mon Sep 17 00:00:00 2001 From: David Lu Date: Mon, 24 Mar 2025 22:30:03 -0400 Subject: [PATCH 4/7] remove unused hashset --- .../service/catalog/iceberg/IcebergCatalogHandlerWrapper.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerWrapper.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerWrapper.java index b4decc7865..4d3e5ee0cf 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerWrapper.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerWrapper.java @@ -26,7 +26,6 @@ import java.time.ZoneOffset; import java.util.ArrayList; import java.util.Arrays; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; @@ -771,7 +770,7 @@ public LoadTableResponse loadTableWithAccessDelegation( PolarisAuthorizableOperation.LOAD_TABLE_WITH_WRITE_DELEGATION; Set actionsRequested = - new HashSet<>(Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST)); + Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST); try { // TODO: Refactor to have a boolean-return version of the helpers so we can fallthrough // easily. From c82b69ef1f01bdfc742dae3d05581e9d3a877813 Mon Sep 17 00:00:00 2001 From: David Lu Date: Thu, 10 Apr 2025 11:42:50 -0400 Subject: [PATCH 5/7] fix merge --- .../iceberg/IcebergCatalogHandler.java | 24 ++++--------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java index ec5950da68..b13d2efcdb 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java @@ -617,26 +617,10 @@ public Optional loadTableWithAccessDelegationIfStale( if (table instanceof BaseTable baseTable) { TableMetadata tableMetadata = baseTable.operations().current(); - LoadTableResponse.Builder responseBuilder = - LoadTableResponse.builder().withTableMetadata(tableMetadata); - if (baseCatalog instanceof SupportsCredentialDelegation credentialDelegation) { - LOGGER - .atDebug() - .addKeyValue("tableIdentifier", tableIdentifier) - .addKeyValue("tableLocation", tableMetadata.location()) - .log("Fetching client credentials for table"); - responseBuilder.addAllConfig( - credentialDelegation.getCredentialConfig( - tableIdentifier, tableMetadata, actionsRequested)); - } - - return Optional.of(responseBuilder.build()); - } else if (table instanceof BaseMetadataTable) { - // metadata tables are loaded on the client side, return NoSuchTableException for now - throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); - return buildLoadTableResponseWithDelegationCredentials( - tableIdentifier, tableMetadata, Set.of(PolarisStorageActions.ALL)) - .build(); + return Optional.of( + buildLoadTableResponseWithDelegationCredentials( + tableIdentifier, tableMetadata, actionsRequested) + .build()); } else if (table instanceof BaseMetadataTable) { // metadata tables are loaded on the client side, return NoSuchTableException for now throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); From 7ade54add16795979365eff956b7850fdcd4d8ce Mon Sep 17 00:00:00 2001 From: David Lu Date: Thu, 10 Apr 2025 15:58:39 -0400 Subject: [PATCH 6/7] fix empty credential case --- .../catalog/iceberg/IcebergCatalogHandler.java | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java index b13d2efcdb..d75d6f0e4e 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java @@ -26,6 +26,7 @@ import java.time.ZoneOffset; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; @@ -558,7 +559,7 @@ public Optional loadTableWithAccessDelegationIfStale( PolarisAuthorizableOperation.LOAD_TABLE_WITH_WRITE_DELEGATION; Set actionsRequested = - Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST); + new HashSet<>(Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST)); try { // TODO: Refactor to have a boolean-return version of the helpers so we can fallthrough // easily. @@ -578,6 +579,11 @@ public Optional loadTableWithAccessDelegationIfStale( CatalogEntity catalogEntity = CatalogEntity.of(catalogPath.getRawLeafEntity()); PolarisConfigurationStore configurationStore = callContext.getPolarisCallContext().getConfigurationStore(); + LOGGER.info("Catalog type: {}", catalogEntity.getCatalogType()); + LOGGER.info("allow external catalog credential vending: {}", configurationStore.getConfiguration( + callContext.getPolarisCallContext(), + catalogEntity, + FeatureConfiguration.ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING)); if (catalogEntity .getCatalogType() .equals(org.apache.polaris.core.admin.model.Catalog.TypeEnum.EXTERNAL) @@ -644,11 +650,13 @@ private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredential Map credentialConfig = credentialDelegation.getCredentialConfig(tableIdentifier, tableMetadata, actions); responseBuilder.addAllConfig(credentialConfig); - responseBuilder.addCredential( + if (!credentialConfig.isEmpty()){ + responseBuilder.addCredential( ImmutableCredential.builder() .prefix(tableMetadata.location()) .config(credentialConfig) .build()); + } } return responseBuilder; } From 4a0bdc7ad1f594633e412f932cc29e82f526a66c Mon Sep 17 00:00:00 2001 From: David Lu Date: Thu, 10 Apr 2025 16:04:35 -0400 Subject: [PATCH 7/7] spotlessApply --- .../iceberg/IcebergCatalogHandler.java | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java index d75d6f0e4e..ace46a3a30 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java @@ -580,10 +580,12 @@ public Optional loadTableWithAccessDelegationIfStale( PolarisConfigurationStore configurationStore = callContext.getPolarisCallContext().getConfigurationStore(); LOGGER.info("Catalog type: {}", catalogEntity.getCatalogType()); - LOGGER.info("allow external catalog credential vending: {}", configurationStore.getConfiguration( - callContext.getPolarisCallContext(), - catalogEntity, - FeatureConfiguration.ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING)); + LOGGER.info( + "allow external catalog credential vending: {}", + configurationStore.getConfiguration( + callContext.getPolarisCallContext(), + catalogEntity, + FeatureConfiguration.ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING)); if (catalogEntity .getCatalogType() .equals(org.apache.polaris.core.admin.model.Catalog.TypeEnum.EXTERNAL) @@ -650,12 +652,12 @@ private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredential Map credentialConfig = credentialDelegation.getCredentialConfig(tableIdentifier, tableMetadata, actions); responseBuilder.addAllConfig(credentialConfig); - if (!credentialConfig.isEmpty()){ + if (!credentialConfig.isEmpty()) { responseBuilder.addCredential( - ImmutableCredential.builder() - .prefix(tableMetadata.location()) - .config(credentialConfig) - .build()); + ImmutableCredential.builder() + .prefix(tableMetadata.location()) + .config(credentialConfig) + .build()); } } return responseBuilder;