Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
dimas-b commented on PR #4052: URL: https://github.com/apache/polaris/pull/4052#issuecomment-4513797268 I'm fine merging this before #3699 ... please resolve conflicts for CI to run and enable the next review round :+1: -- 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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
aritragster commented on PR #4052: URL: https://github.com/apache/polaris/pull/4052#issuecomment-4483109667 @flyingImer @singhpk234 I've addressed all outstanding feedback, rebased on latest main, and filed the upstream issues. Here's the summary: ## Issues Filed - **Iceberg upstream:** https://github.com/apache/iceberg/issues/16399 — Expose `tableId` natively in `LoadTableResponse` (removes need for `ConfigCapturingHTTPClient` long-term) - **Polaris follow-up:** https://github.com/apache/polaris/issues/4486 — Refactor `StorageTypeFileIO` discriminator to support S3 Tables without opt-out ## Review Feedback Addressed **@singhpk234's feedback:** - `S3_TABLES` case moved before `GCS` in `PolarisStorageIntegrationProviderImpl` switch - `TABLE_ID_CONFIG_KEY` renamed to `S3TABLES_TABLE_ID_CONFIG_KEY` - Shared STS assume-role logic extracted to `AwsStsUtil` — both `AwsCredentialsStorageIntegration` (S3) and `AwsS3TablesCredentialsStorageIntegration` now use it, eliminating duplication - `ConfigCapturingHTTPClient` Javadoc made generic (removed S3 Tables-specific reference) - Rebased on latest main, all conflicts resolved **@flyingImer's feedback:** - TODO added on `ConfigCapturingHTTPClient` linking https://github.com/apache/iceberg/issues/16399 - TODO added on `StorageTypeFileIO` S3_TABLES entry linking https://github.com/apache/polaris/issues/4486 **Additional cleanup:** - Fixed `IcebergRESTFederatedCatalogFactory` constructor name mismatch (pre-existing bug exposed by rebase) - Removed dead inline ARN logic from `IcebergCatalogHandler` (already encapsulated in `S3TablesUtil.resolveTableLocations`) - Added `@Nullable` annotations on `AwsStsUtil` parameters - Fixed duplicate `CLIENT_REGION`/`refreshCredentialsEndpoint` setting in S3 integration (util handles STS path, else-block handles non-STS path) ## New Tests - `AwsStsUtilTest` — 12 tests covering assume-role behavior (parameters, credentials, expiration, region, refresh endpoint, session names, session tags) - `S3TablesUtilTest` — 9 tests covering ARN construction, validation, fail-closed behavior, and type detection ## On Sequencing with #3699 I've been tracking that PR. It currently has no formal approvals, an unresolved cache design discussion (`LoadingCache` vs `Supplier`-based Caffeine), and a pending dev ML vote on SPI removals. There's no clear timeline for it to land. This PR is self-contained and additive (~1,200 lines including tests, primarily new files). When #3699 lands with its `LocationGrant` model and `StorageIntegration` SPI, my S3 Tables integration becomes a clean new implementation of that interface — the `AwsS3TablesCredentialsStorageIntegration` maps directly to the new abstraction, and `AwsStsUtil` remains shared. I commit to doing that adaptation when #3699 merges. Merging this first doesn't add debt to the refactor path. It just means S3 Tables credential vending ships sooner. -- 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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
aritragster commented on PR #4052: URL: https://github.com/apache/polaris/pull/4052#issuecomment-4483106696 @flyingImer @singhpk234 I've addressed all outstanding feedback, rebased on latest main, and filed the upstream issues. Here's the summary: ## Issues Filed - **Iceberg upstream:** https://github.com/apache/iceberg/issues/16399 — Expose `tableId` natively in `LoadTableResponse` (removes need for `ConfigCapturingHTTPClient` long-term) - **Polaris follow-up:** https://github.com/apache/polaris/issues/4486 — Refactor `StorageTypeFileIO` discriminator to support S3 Tables without opt-out ## Review Feedback Addressed **@singhpk234's feedback:** - `S3_TABLES` case moved before `GCS` in `PolarisStorageIntegrationProviderImpl` switch - `TABLE_ID_CONFIG_KEY` renamed to `S3TABLES_TABLE_ID_CONFIG_KEY` - Shared STS assume-role logic extracted to `AwsStsUtil` — both `AwsCredentialsStorageIntegration` (S3) and `AwsS3TablesCredentialsStorageIntegration` now use it, eliminating duplication - `ConfigCapturingHTTPClient` Javadoc made generic (removed S3 Tables-specific reference) - Rebased on latest main, all conflicts resolved **@flyingImer's feedback:** - TODO added on `ConfigCapturingHTTPClient` linking https://github.com/apache/iceberg/issues/16399 - TODO added on `StorageTypeFileIO` S3_TABLES entry linking https://github.com/apache/polaris/issues/4486 **Additional cleanup:** - Fixed `IcebergRESTFederatedCatalogFactory` constructor name mismatch (pre-existing bug exposed by rebase) - Removed dead inline ARN logic from `IcebergCatalogHandler` (already encapsulated in `S3TablesUtil.resolveTableLocations`) - Added `@Nullable` annotations on `AwsStsUtil` parameters - Fixed duplicate `CLIENT_REGION`/`refreshCredentialsEndpoint` setting in S3 integration (util handles STS path, else-block handles non-STS path) ## New Tests - `AwsStsUtilTest` — 12 tests covering assume-role behavior (parameters, credentials, expiration, region, refresh endpoint, session names, session tags) - `S3TablesUtilTest` — 9 tests covering ARN construction, validation, fail-closed behavior, and type detection ## On Sequencing with #3699 I've been tracking that PR. It currently has no formal approvals, an unresolved cache design discussion (`LoadingCache` vs `Supplier`-based Caffeine), and a pending dev ML vote on SPI removals. There's no clear timeline for it to land. This PR is self-contained and additive (~1,200 lines including tests, primarily new files). When #3699 lands with its `LocationGrant` model and `StorageIntegration` SPI, my S3 Tables integration becomes a clean new implementation of that interface — the `AwsS3TablesCredentialsStorageIntegration` maps directly to the new abstraction, and `AwsStsUtil` remains shared. I commit to doing that adaptation when #3699 merges. Merging this first doesn't add debt to the refactor path. It just means S3 Tables credential vending ships sooner. -- 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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
aritragster commented on PR #4052: URL: https://github.com/apache/polaris/pull/4052#issuecomment-4483105022 @flyingImer @singhpk234 I've addressed all outstanding feedback, rebased on latest main, and filed the upstream issues. Here's the summary: ## Issues Filed - **Iceberg upstream:** https://github.com/apache/iceberg/issues/16399 — Expose `tableId` natively in `LoadTableResponse` (removes need for `ConfigCapturingHTTPClient` long-term) - **Polaris follow-up:** https://github.com/apache/polaris/issues/4486 — Refactor `StorageTypeFileIO` discriminator to support S3 Tables without opt-out ## Review Feedback Addressed **@singhpk234's feedback:** - `S3_TABLES` case moved before `GCS` in `PolarisStorageIntegrationProviderImpl` switch - `TABLE_ID_CONFIG_KEY` renamed to `S3TABLES_TABLE_ID_CONFIG_KEY` - Shared STS assume-role logic extracted to `AwsStsUtil` — both `AwsCredentialsStorageIntegration` (S3) and `AwsS3TablesCredentialsStorageIntegration` now use it, eliminating duplication - `ConfigCapturingHTTPClient` Javadoc made generic (removed S3 Tables-specific reference) - Rebased on latest main, all conflicts resolved **@flyingImer's feedback:** - TODO added on `ConfigCapturingHTTPClient` linking https://github.com/apache/iceberg/issues/16399 - TODO added on `StorageTypeFileIO` S3_TABLES entry linking https://github.com/apache/polaris/issues/4486 **Additional cleanup:** - Fixed `IcebergRESTFederatedCatalogFactory` constructor name mismatch (pre-existing bug exposed by rebase) - Removed dead inline ARN logic from `IcebergCatalogHandler` (already encapsulated in `S3TablesUtil.resolveTableLocations`) - Added `@Nullable` annotations on `AwsStsUtil` parameters - Fixed duplicate `CLIENT_REGION`/`refreshCredentialsEndpoint` setting in S3 integration (util handles STS path, else-block handles non-STS path) ## New Tests - `AwsStsUtilTest` — 12 tests covering assume-role behavior (parameters, credentials, expiration, region, refresh endpoint, session names, session tags) - `S3TablesUtilTest` — 9 tests covering ARN construction, validation, fail-closed behavior, and type detection ## On Sequencing with #3699 I've been tracking that PR. It currently has no formal approvals, an unresolved cache design discussion (`LoadingCache` vs `Supplier`-based Caffeine), and a pending dev ML vote on SPI removals. There's no clear timeline for it to land. This PR is self-contained and additive (~1,200 lines including tests, primarily new files). When #3699 lands with its `LocationGrant` model and `StorageIntegration` SPI, my S3 Tables integration becomes a clean new implementation of that interface — the `AwsS3TablesCredentialsStorageIntegration` maps directly to the new abstraction, and `AwsStsUtil` remains shared. Merging this first doesn't add debt to the refactor path. It just means S3 Tables credential vending ships sooner. -- 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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
flyingImer commented on PR #4052: URL: https://github.com/apache/polaris/pull/4052#issuecomment-4354776252 Coming back after the latest round. aritragster addressed my 4-01 blockers and singhpk234 is on the finish line for local issues. Before this lands, three structural things. **1. `StorageTypeFileIO`: the current resolution opts out of the contract, it does not fix it.** My 4-01 point was that `S3` and `S3_TABLES` both mapping to `S3FileIO` breaks the reverse-lookup assumption (`FileIO impl -> storage type`). The resolution here sets `validateAllowedStorageType=false` on `S3_TABLES` so it is excluded from the reverse map. The contract is still "FileIO impl uniquely determines storage type", with `S3_TABLES` as an exception. Before this PR, the `validate=false` escape was only used by `IN_MEMORY` (test FileIO). Extending it to a production storage type is a different character of decision. The next S3-family storage type hits the same shape. The structural fix is a dedicated discriminator (the `StorageType` enum is already there) and dropping `FileIO class -> storage type` reverse lookup in the validation path. Two ways to land this, my preference is A: - **A (preferred)**: do the discriminator refactor in this PR. `StorageType` enum already exists, the scope is the validation path's reverse lookup. - **B (acceptable)**: file a follow-up issue and add a TODO next to `S3_TABLES(..., false)` citing the issue, with a commit to do the refactor before another S3-family type lands. This matches singhpk234's 4-25 pattern for `AwsS3TablesCredentialsStorageIntegration:218`. The one outcome I would push back on is skipping A without filing the follow-up. **2. `ConfigCapturingHTTPClient` patches an Iceberg-library API gap from inside Polaris.** aritragster laid this out on 4-27: `tableId` sits in the remote `loadTable` response's `config`, but `RESTCatalog` consumes `config` internally and never exposes it on `BaseTable` or `TableMetadata`. So we wrap the HTTP client to intercept the raw response. Three issues compound here: - brittle against Iceberg library version bumps, since internal config consumption is not a stable boundary - narrow: it hooks on `LoadTableResponse` specifically. Any other response type from another endpoint needs its own wrapper. - the problem is upstream in Iceberg, the maintenance cost lands in Polaris aritragster already considered the upstream Iceberg fix on 4-27 and ruled it out as long-lead and not guaranteed to land. I would flip that call. Concrete ask: - file the Iceberg issue now (happy to help draft if useful) - add a TODO or `@see` on `ConfigCapturingHTTPClient` class Javadoc linking the upstream issue, so future readers see this is a temporary shim - keep the interception in this PR on that footing, not as a permanent mechanism Shipping it without the upstream link signs Polaris up for fork-shaped debt inside OSS. singhpk234's 4-25 comment on `IcebergRESTExternalCatalogFactory:69` is the same instinct at the local level. Worth lifting it to the boundary. **3. Sequencing vs #3699.** aritragster himself offered on 3-25 to rebase on #3699 once it lands, or to collaborate with tokoko so the refactoring accounts for S3 Tables. +1 to that and to dimas-b 4-13. Let's hold to that sequencing rather than flip to merge-first. If this merges first, #3699 either carries S3 Tables semantics through the refactor or leaves the S3 Tables path as a legacy branch. Either way the "one coherent vending abstraction" outcome gets worse. If #3699's abstraction can express ARN-based resources alongside path-based ones, which aritragster flagged as a fit on 3-25, S3 Tables drops in cleanly. Better landing than branching-in-S3 now and retrofitting later. --- Not trying to block. Iteration since the 3-24 draft has moved from `if/else` branching inside S3 logic to a dedicated S3 Tables integration path. But the discriminator contract and the interception layer are shapes we live with long after this merges, and I would rather get the shape right once. -- 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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
singhpk234 commented on code in PR #4052:
URL: https://github.com/apache/polaris/pull/4052#discussion_r3169337852
##
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsS3TablesCredentialsStorageIntegration.java:
##
@@ -0,0 +1,232 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.polaris.core.storage.aws;
+
+import static
org.apache.polaris.core.config.FeatureConfiguration.STORAGE_CREDENTIAL_DURATION_SECONDS;
+import static
org.apache.polaris.core.storage.aws.AwsSessionTagsBuilder.buildSessionTags;
+
+import jakarta.annotation.Nonnull;
+import java.util.EnumSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.polaris.core.auth.PolarisPrincipal;
+import org.apache.polaris.core.config.FeatureConfiguration;
+import org.apache.polaris.core.config.RealmConfig;
+import org.apache.polaris.core.storage.CredentialVendingContext;
+import org.apache.polaris.core.storage.InMemoryStorageIntegration;
+import org.apache.polaris.core.storage.PolarisStorageActions;
+import org.apache.polaris.core.storage.StorageAccessConfig;
+import org.apache.polaris.core.storage.StorageAccessProperty;
+import org.apache.polaris.core.storage.aws.StsClientProvider.StsDestination;
+import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
+import software.amazon.awssdk.policybuilder.iam.IamEffect;
+import software.amazon.awssdk.policybuilder.iam.IamPolicy;
+import software.amazon.awssdk.policybuilder.iam.IamResource;
+import software.amazon.awssdk.policybuilder.iam.IamStatement;
+import software.amazon.awssdk.services.sts.StsClient;
+import software.amazon.awssdk.services.sts.model.AssumeRoleRequest;
+import software.amazon.awssdk.services.sts.model.AssumeRoleResponse;
+import software.amazon.awssdk.services.sts.model.Tag;
+
+/**
+ * Credential vendor for Amazon S3 Tables. Generates IAM session policies
using {@code s3tables:*}
+ * actions scoped to table-level ARNs, rather than the path-based {@code s3:*}
policies used by
+ * {@link AwsCredentialsStorageIntegration}.
+ */
+public class AwsS3TablesCredentialsStorageIntegration
+extends InMemoryStorageIntegration {
+
+ // S3 Tables IAM actions for read operations
+ public static final String ACTION_GET_TABLE_DATA = "s3tables:GetTableData";
+ public static final String ACTION_GET_TABLE_METADATA_LOCATION =
+ "s3tables:GetTableMetadataLocation";
+
+ // S3 Tables IAM actions for write operations
+ public static final String ACTION_UPDATE_TABLE_METADATA_LOCATION =
+ "s3tables:UpdateTableMetadataLocation";
+ public static final String ACTION_PUT_TABLE_DATA = "s3tables:PutTableData";
+
+ private final StsClientProvider stsClientProvider;
+ private final Optional credentialsProvider;
+
+ public AwsS3TablesCredentialsStorageIntegration(
+ AwsS3TablesStorageConfigurationInfo config,
+ StsClientProvider stsClientProvider,
+ Optional credentialsProvider) {
+super(config, AwsS3TablesCredentialsStorageIntegration.class.getName());
+this.stsClientProvider = stsClientProvider;
+this.credentialsProvider = credentialsProvider;
+ }
+
+ @Override
+ public StorageAccessConfig getSubscopedCreds(
+ @Nonnull RealmConfig realmConfig,
+ boolean allowListOperation,
+ @Nonnull Set allowedReadLocations,
+ @Nonnull Set allowedWriteLocations,
+ @Nonnull PolarisPrincipal polarisPrincipal,
+ Optional refreshCredentialsEndpoint,
+ @Nonnull CredentialVendingContext credentialVendingContext) {
+
+AwsS3TablesStorageConfigurationInfo storageConfig = config();
+String region = storageConfig.getRegion();
+int durationSeconds =
realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS);
+
+StorageAccessConfig.Builder accessConfig = StorageAccessConfig.builder();
+
+// Generate s3tables:* session policy
+IamPolicy policy =
+buildS3TablesPolicy(storageConfig, allowedReadLocations,
allowedWriteLocations);
+
+// Role session name
+boolean includePrincipalName =
+
realmConfig.getConfig(FeatureConfiguration.INCLUDE_PRINCIPAL_NAME_IN
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
singhpk234 commented on PR #4052: URL: https://github.com/apache/polaris/pull/4052#issuecomment-4354156691 can you resolve the conflicts @aritragster -- 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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
aritragster commented on code in PR #4052: URL: https://github.com/apache/polaris/pull/4052#discussion_r3150119523 ## runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRESTExternalCatalogFactory.java: ## @@ -54,9 +62,11 @@ public Catalog createCatalog( new RESTCatalog( context, (config) -> -HTTPClient.builder(config) -.uri(config.get(org.apache.iceberg.CatalogProperties.URI)) -.build()); +new ConfigCapturingHTTPClient( +HTTPClient.builder(config) + .uri(config.get(org.apache.iceberg.CatalogProperties.URI)) +.build(), +capturedConfigHolder)); Review Comment: I thought about this one a bit. The HTTP interception is really the only way to get the tableId without either changing the Iceberg library or making an extra API call. The problem is that the S3 Tables Iceberg REST endpoint returns tableId in the config section of the loadTable response, but Iceberg's RESTCatalog consumes that config internally (for token refresh, endpoint routing, etc.) and never exposes it on the BaseTable or TableMetadata. So by the time baseCatalog.loadTable() returns, the tableId is just gone. The alternatives I considered: A supplementary s3tables:GetTable API call, but that adds latency and requires extra IAM permissions on the Polaris server. An upstream Iceberg change to expose response config, but that's a long lead time and not guaranteed to be accepted. The wrapper itself is pretty thin (~40 lines of delegation) and only captures data, never modifies the response. Happy to revisit if you see a cleaner path though. -- 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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
singhpk234 commented on code in PR #4052:
URL: https://github.com/apache/polaris/pull/4052#discussion_r3141012166
##
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java:
##
@@ -200,6 +202,7 @@ public enum StorageType {
AZURE(List.of("abfs://", "wasb://", "abfss://", "wasbs://")),
GCS("gs://"),
FILE("file://"),
+S3_TABLES("arn:aws:s3tables"),
Review Comment:
you can move it before ?
##
polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java:
##
@@ -356,6 +370,28 @@ public Builder setStorageConfigurationInfo(
.storageName(storageConfigModel.getStorageName())
.build();
break;
+ case S3_TABLES:
+if (defaultBaseLocation == null
+|| !defaultBaseLocation.startsWith("arn:aws:s3tables")) {
Review Comment:
`arn:aws:s3tables` may be move this to the constants ?
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -837,7 +846,8 @@ private LoadTableResponse.Builder
buildLoadTableResponseWithDelegationCredential
TableMetadata tableMetadata,
EnumSet delegationModes,
Set actions,
- Optional refreshCredentialsEndpoint) {
+ Optional refreshCredentialsEndpoint,
+ Optional capturedTableId) {
Review Comment:
do we need to pass this as Args ? since this a request scoped bean its
applicable to this Request only right ?
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -855,8 +865,36 @@ private LoadTableResponse.Builder
buildLoadTableResponseWithDelegationCredential
Set 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.
+ // Assumption: the federated catalog loadTable call has already
succeeded at this point,
+ // and the CapturedConfigHolder contains the tableId from the remote
response.
+ CatalogEntity catalogEntity =
CatalogEntity.of(getResolvedCatalogEntity());
+ boolean isS3Tables = isS3TablesCatalog(catalogEntity);
+
+ if (isS3Tables && capturedTableId.isPresent()) {
+String tableArn = constructS3TablesArn(catalogEntity,
capturedTableId.get());
+validateS3TablesArn(tableIdentifier, tableArn, catalogEntity);
+tableLocations = Set.of(tableArn);
+LOGGER
+.atDebug()
+.addKeyValue("tableIdentifier", tableIdentifier)
+.addKeyValue("tableArn", tableArn)
+.log("Replaced table locations with S3 Tables ARN for credential
vending");
+ } else if (isS3Tables) {
+// Fail closed: S3 Tables catalogs require a tableId to construct the
table ARN
+// for scoped credential vending. Without it, we cannot generate a
properly scoped
+// IAM session policy.
+throw new BadRequestException(
+"Cannot vend credentials for S3 Tables table '%s': "
++ "no tableId was captured from the remote catalog response. "
++ "Ensure the remote S3 Tables endpoint returns tableId in the
loadTable config.",
+tableIdentifier);
+ }
Review Comment:
I am a bit confused, wouldn't this be completely inside, since this is what
marks this catalog as a federated catalog ?
> !(baseCatalog instanceof IcebergCatalog)
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -892,6 +930,45 @@ private LoadTableResponse.Builder
buildLoadTableResponseWithDelegationCredential
return responseBuilder;
}
+ /** Checks whether the resolved catalog entity is configured with S3_TABLES
storage type. */
+ private boolean isS3TablesCatalog(CatalogEntity catalogEntity) {
+PolarisStorageConfigurationInfo storageConfig =
catalogEntity.getStorageConfigurationInfo();
+return storageConfig != null
+&& storageConfig.getStorageType() ==
PolarisStorageConfigurationInfo.StorageType.S3_TABLES;
+ }
+
+ /**
+ * Constructs an S3 Tables ARN from the catalog's default-base-location and
a tableId. The
+ * default-base-location for S3 Tables catalogs is the table bucket ARN
(e.g.,
+ * arn:aws:s3tables:us-east-1:123456789012:bucket/my-bucket). The resulting
table ARN is
+ * bucket-arn/table/tableId.
+ */
+ private String constructS3TablesArn(CatalogEntity catalogEntity, String
tableId) {
+String baseLocation = catalogEntity.getBaseLocation();
+return baseLocation + "/table/" + tableId;
+ }
+
+ /**
+ * Validates
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
dimas-b commented on code in PR #4052:
URL: https://github.com/apache/polaris/pull/4052#discussion_r3075000837
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -812,6 +816,13 @@ public Optional loadTable(
// when data-access is specified but access delegation grants are not
found.
Table table = baseCatalog.loadTable(tableIdentifier);
+// Capture tableId from remote catalog response for S3 Tables ARN
construction
+Optional capturedTableId = capturedConfigHolder().getTableId();
+List resourceArns = List.of();
+if (capturedTableId.isPresent()) {
+ resourceArns = List.of(constructS3TablesArn(capturedTableId.get()));
Review Comment:
@aritragster : just to sync up: I tend to prefer completing #3699 first... I
hope it does not cause too much delay on this PR. Please let us know if this is
timeline is problematic.
#3699 may not provide all the features needed by this PR, but at least I
hope we can perform the main refactoring in #3699 and later add remaining
features for S3 tables in this PR.
--
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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
aritragster commented on code in PR #4052:
URL: https://github.com/apache/polaris/pull/4052#discussion_r3047380956
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -855,8 +865,35 @@ private LoadTableResponse.Builder
buildLoadTableResponseWithDelegationCredential
Set 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;
Review Comment:
Done on the private function - extracted to isS3TablesCatalog() with a
comment documenting the assumption that the federated catalog loadTable call
has already succeeded.
On the CapturedConfigHolder: the problem it solves is that the S3 Tables
tableId only exists in the config section of the remote loadTable REST
response. When Polaris calls baseCatalog.loadTable(), the Iceberg RESTCatalog
internally deserializes the HTTP response, consumes the config map (for token
refresh, endpoint config, etc.), and returns a BaseTable object. The
BaseTable/TableMetadata doesn't expose the config section - by the time our
handler sees the table, the tableId is gone.
The CapturedConfigHolder is a request-scoped bean that bridges this gap. The
ConfigCapturingHTTPClient wraps the RESTClient at the HTTP layer (the only
point where the config is visible), extracts the tableId, and stashes it in the
holder. After baseCatalog.loadTable() returns, the handler reads the tableId
from the holder to construct the table ARN.
Let me know what you think of this.
--
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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
aritragster commented on code in PR #4052:
URL: https://github.com/apache/polaris/pull/4052#discussion_r3047372379
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -855,8 +865,35 @@ private LoadTableResponse.Builder
buildLoadTableResponseWithDelegationCredential
Set 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:
Agreed. I changed it from warn-and-continue to BadRequestException. If the
catalog is S3_TABLES and no tableId was captured, we now refuse to vend
credentials rather than proceeding with unscoped permissions.
--
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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
aritragster commented on code in PR #4052:
URL: https://github.com/apache/polaris/pull/4052#discussion_r3047370080
##
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:
Fixed. Set validateAllowedStorageType=false on S3_TABLES so it's excluded
from the FileIO reverse lookup map. S3FileIO still resolves to S3. S3_TABLES
validation happens via storage type, not FileIO impl.
--
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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
aritragster commented on code in PR #4052:
URL: https://github.com/apache/polaris/pull/4052#discussion_r3047366439
##
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsS3TablesStorageConfigurationInfo.java:
##
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.polaris.core.storage.aws;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonTypeName;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import java.net.URI;
+import java.util.List;
+import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo;
+import org.apache.polaris.immutables.PolarisImmutable;
+import org.immutables.value.Value;
+
+/** AWS S3 Tables storage configuration information. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutableAwsS3TablesStorageConfigurationInfo.class)
+@JsonDeserialize(as = ImmutableAwsS3TablesStorageConfigurationInfo.class)
+@JsonTypeName("AwsS3TablesStorageConfigurationInfo")
+public abstract class AwsS3TablesStorageConfigurationInfo extends
PolarisStorageConfigurationInfo {
Review Comment:
This was Intentional choice. S3 Tables doesn't use most of the S3-specific
fields (endpoint, pathStyleAccess, stsUnavailable, userARN, endpointInternal).
Extending AwsStorageConfigurationInfo would inherit all of those as nullable
fields that are never set, which made me think about this. The only shared
fields are roleARN, region, externalId, and KMS — and those are explicitly
declared on the S3 Tables config class. Happy to reconsider if you feel
strongly about it.
--
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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
aritragster commented on code in PR #4052:
URL: https://github.com/apache/polaris/pull/4052#discussion_r3047360725
##
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java:
##
@@ -200,6 +202,7 @@ public enum StorageType {
AZURE(List.of("abfs://", "wasb://", "abfss://", "wasbs://")),
GCS("gs://"),
FILE("file://"),
+S3_TABLES("arn:"),
Review Comment:
Good catch — changed to arn:aws:s3tables to avoid matching non-S3-Tables
ARNs. Updated the CatalogEntity base location validation to match
--
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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
aritragster commented on code in PR #4052:
URL: https://github.com/apache/polaris/pull/4052#discussion_r3047359000
##
polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java:
##
@@ -200,6 +202,18 @@ private StorageConfigInfo getStorageInfo(Map internalProperties)
.setStorageName(fileConfigModel.getStorageName())
.build();
}
+ if (configInfo instanceof AwsS3TablesStorageConfigurationInfo
s3TablesConfig) {
+return AwsS3TablesStorageConfigInfo.builder()
+.setRoleArn(s3TablesConfig.getRoleARN())
+.setExternalId(s3TablesConfig.getExternalId())
+.setRegion(s3TablesConfig.getRegion())
+.setCurrentKmsKey(s3TablesConfig.getCurrentKmsKey())
+.setAllowedKmsKeys(s3TablesConfig.getAllowedKmsKeys())
+.setStorageType(StorageConfigInfo.StorageTypeEnum.S3_TABLES)
+.setAllowedLocations(s3TablesConfig.getAllowedLocations())
+.setStorageName(s3TablesConfig.getStorageName())
+.build();
+ }
Review Comment:
Done — moved the S3_TABLES block right after S3 to keep AWS storage types
grouped.
--
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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
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 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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
flyingImer commented on PR #4052: URL: https://github.com/apache/polaris/pull/4052#issuecomment-4172776720 I do think this is directionally better now. Pulling S3 Tables into a more dedicated path makes sense to me, and I think this is in a better place than the earlier version where the logic was drifting further into the generic S3 vending path. That said, I still think there are two issues that block this for me: 1. `StorageTypeFileIO` is no longer a stable discriminator once both `S3` and `S3_TABLES` point at the same `S3FileIO`, but the validation path still reverse-maps from FileIO impl back to storage type. 2. Once we know the catalog is `S3_TABLES`, the no-`tableId` path still warns and continues instead of failing closed. So IIUC, the create-time/unresolved-scope gap is still not really closed yet. So overall, I'm aligned with the structural direction here, but I still don't think the contract is explicit enough yet for merge. -- 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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
singhpk234 commented on code in PR #4052:
URL: https://github.com/apache/polaris/pull/4052#discussion_r3022120213
##
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsS3TablesStorageConfigurationInfo.java:
##
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.polaris.core.storage.aws;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonTypeName;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import jakarta.annotation.Nullable;
+import java.net.URI;
+import java.util.List;
+import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo;
+import org.apache.polaris.immutables.PolarisImmutable;
+import org.immutables.value.Value;
+
+/** AWS S3 Tables storage configuration information. */
+@PolarisImmutable
+@JsonSerialize(as = ImmutableAwsS3TablesStorageConfigurationInfo.class)
+@JsonDeserialize(as = ImmutableAwsS3TablesStorageConfigurationInfo.class)
+@JsonTypeName("AwsS3TablesStorageConfigurationInfo")
+public abstract class AwsS3TablesStorageConfigurationInfo extends
PolarisStorageConfigurationInfo {
Review Comment:
why not extend AWSS3StorageIntegrations ?
##
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##
@@ -393,6 +399,36 @@ private static String arnKeyAll(String region, String
accountId) {
return String.format("arn:aws:kms:%s:%s:key/*", region, accountId);
}
+ private boolean isS3TablesRequest(CredentialVendingContext context) {
+return context.signingName().map("s3tables"::equals).orElse(false);
+ }
+
+ /**
+ * Generates an IAM session policy for S3 Tables access. Read access always
includes GetTableData
+ * and GetTableMetadataLocation. Write access additionally includes
UpdateTableMetadataLocation
+ * and PutTableData. Resources are scoped to the specific table ARN(s) from
the context.
+ */
+ private IamPolicy s3TablesPolicyString(CredentialVendingContext context,
boolean canWrite) {
+IamStatement.Builder statement =
+IamStatement.builder()
+.effect(IamEffect.ALLOW)
+.addAction("s3tables:GetTableData")
+.addAction("s3tables:GetTableMetadataLocation");
+
+if (canWrite) {
+ statement
+ .addAction("s3tables:UpdateTableMetadataLocation")
+ .addAction("s3tables:PutTableData");
+}
+
+List arns = context.resourceArns().orElse(List.of());
+for (String arn : arns) {
+ statement.addResource(IamResource.create(arn));
+}
+
+return IamPolicy.builder().addStatement(statement.build()).build();
Review Comment:
I think having a dedidcated storage integration would be really helpful than
writing if else for s3 tables
##
polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java:
##
@@ -200,6 +202,18 @@ private StorageConfigInfo getStorageInfo(Map internalProperties)
.setStorageName(fileConfigModel.getStorageName())
.build();
}
+ if (configInfo instanceof AwsS3TablesStorageConfigurationInfo
s3TablesConfig) {
+return AwsS3TablesStorageConfigInfo.builder()
+.setRoleArn(s3TablesConfig.getRoleARN())
+.setExternalId(s3TablesConfig.getExternalId())
+.setRegion(s3TablesConfig.getRegion())
+.setCurrentKmsKey(s3TablesConfig.getCurrentKmsKey())
+.setAllowedKmsKeys(s3TablesConfig.getAllowedKmsKeys())
+.setStorageType(StorageConfigInfo.StorageTypeEnum.S3_TABLES)
+.setAllowedLocations(s3TablesConfig.getAllowedLocations())
+.setStorageName(s3TablesConfig.getStorageName())
+.build();
+ }
Review Comment:
lets keep this just next to S3 ?
##
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java:
##
@@ -200,6 +202,7 @@ public enum StorageType {
AZURE(List.of("abfs://", "wasb://", "abfss://", "wasbs://")),
GCS("gs://"),
FILE("file://"),
+S3_TABLES("arn:"),
Review Comment:
h
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
flyingImer commented on PR #4052: URL: https://github.com/apache/polaris/pull/4052#issuecomment-4146110446 Thanks for the draft here. I’m aligned with the direction, and I think this is already useful in showing that S3 Tables can fit the federated catalog path at the protocol level. My bias would be to treat this as a good proof point, and then use the next iteration to tighten both the shape and the vending semantics a bit more. In particular, I wonder if the cleaner direction here would be: - keep the S3 Tables-specific policy generation in a dedicated storage-integration path, rather than growing more branching in the existing S3 logic - align that with the broader push toward a more unified call path for storage locations, rather than introducing another parallel path here On the credential vending side, there is also a concrete next-step item that feel worth making explicit: - IIUC, only `loadTable` currently derives S3 Tables `resourceArns`, while the create paths still flow through with empty resource scope. I think the next iteration should make those create-time semantics explicit: either derive the usable scope there as well, or fail/defer vending rather than returning partially-scoped credentials. This seems especially relevant given the S3 Tables REST docs note that stage-create is not supported. Also worth aligning with Tornike's PR #3699 on the unified call path for storage locations, seems like natural synergy. Looking forward to seeing this evolve. -- 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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
flyrain commented on PR #4052: URL: https://github.com/apache/polaris/pull/4052#issuecomment-4145491915 Hi @aritragster , thanks for working on it. The PR is in draft now. Is it ready for review? -- 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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
aritragster commented on code in PR #4052:
URL: https://github.com/apache/polaris/pull/4052#discussion_r2985603851
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -812,6 +816,13 @@ public Optional loadTable(
// when data-access is specified but access delegation grants are not
found.
Table table = baseCatalog.loadTable(tableIdentifier);
+// Capture tableId from remote catalog response for S3 Tables ARN
construction
+Optional capturedTableId = capturedConfigHolder().getTableId();
+List resourceArns = List.of();
+if (capturedTableId.isPresent()) {
+ resourceArns = List.of(constructS3TablesArn(capturedTableId.get()));
Review Comment:
I took a look at #3699 and the direction there looks like a natural fit for
what S3 Tables needs. The key difference with S3 Tables is that the resource
identifier is an ARN (not an s3:// path) and the IAM actions use the s3tables:
namespace, but the overall credential vending flow is the same. If #3699's
refactored abstractions can accommodate ARN-based resources alongside
path-based ones, S3 Tables support should fit in cleanly.
Happy to rebase this PR on top of #3699 once it lands, or collaborate with
@tokoko to make sure the refactoring accounts for the S3 Tables case. In the
meantime I'll keep this draft up as a reference for the S3 Tables-specific
logic (policy generation, signingName detection, tableId capture).
Let me know what you'd prefer — happy to help either way.
--
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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
aritragster commented on code in PR #4052:
URL: https://github.com/apache/polaris/pull/4052#discussion_r2985584991
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -812,6 +816,13 @@ public Optional loadTable(
// when data-access is specified but access delegation grants are not
found.
Table table = baseCatalog.loadTable(tableIdentifier);
+// Capture tableId from remote catalog response for S3 Tables ARN
construction
+Optional capturedTableId = capturedConfigHolder().getTableId();
+List resourceArns = List.of();
+if (capturedTableId.isPresent()) {
+ resourceArns = List.of(constructS3TablesArn(capturedTableId.get()));
Review Comment:
hey @dimas-b thanks for the initial review on this.
The idea was to get some early feedback if this makes sense, or we should
think of a different approach. I also started the discussion on the ML:
https://lists.apache.org/thread/sk84l9wdgpkyvk50v0d3wclh7f3575xn
--
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]
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
dimas-b commented on code in PR #4052:
URL: https://github.com/apache/polaris/pull/4052#discussion_r2984736272
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -812,6 +816,13 @@ public Optional loadTable(
// when data-access is specified but access delegation grants are not
found.
Table table = baseCatalog.loadTable(tableIdentifier);
+// Capture tableId from remote catalog response for S3 Tables ARN
construction
+Optional capturedTableId = capturedConfigHolder().getTableId();
+List resourceArns = List.of();
+if (capturedTableId.isPresent()) {
+ resourceArns = List.of(constructS3TablesArn(capturedTableId.get()));
Review Comment:
This is a smart idea 😉 However, I'm not sure it is viable from the internal
APIs POV. This approach appears to circumvent the usual way of passing
locations to the storage integration code and instead introduces a new optional
way of passing similar information via a different channel
(`CredentialVendingContext`).
I believe it would be beneficial to holistically refactor all storage
integrations call paths so that location data is handled uniformly.
Note: such a refactoring will probably overlap with #3699.
--
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]
