You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "aritragster (via GitHub)" <gi...@apache.org> on 2026/03/24 21:18:21 UTC
[PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
aritragster opened a new pull request, #4052:
URL: https://github.com/apache/polaris/pull/4052
## Summary
When Polaris federates to an S3 Tables Iceberg REST endpoint, the credential vending flow must generate S3 Tables IAM policies (using `s3tables:` actions and table-level ARNs) instead of standard S3 policies.
This is a draft PR to socialize the approach with the community. Feedback on the design is welcome.
Related to #577
## Problem
S3 Tables uses a different IAM action namespace (`s3tables:`) and ARN format (`arn:aws:s3tables:REGION:ACCOUNT:bucket/BUCKET/table/TABLE_ID`) compared to standard S3. When Polaris federates to an S3 Tables Iceberg REST endpoint, the existing credential vending flow generates S3 policies that don't work for S3 Tables data access.
## Approach
- Detect S3 Tables catalogs via the `signingName` property (`"s3tables"`) from the remote endpoint's connection configuration, persisted on the catalog entity
- Capture the `tableId` returned in `loadTable` responses from the remote endpoint using a `ConfigCapturingHTTPClient` wrapper and request-scoped `CapturedConfigHolder`
- Construct proper S3 Tables ARNs from the catalog's base location and captured `tableId`
- Generate per-table scoped session policies with granular read/write actions:
- Read: `s3tables:GetTableData`, `s3tables:GetTableMetadataLocation`
- Write (additional): `s3tables:UpdateTableMetadataLocation`, `s3tables:PutTableData`
- Validate at catalog creation time that `signingName: "s3tables"` catalogs use ARN-based `default-base-location` (not `s3://` paths)
## Files Changed (15 files, ~570 lines)
- `AwsCredentialsStorageIntegration` — S3 Tables policy generation, branching on `signingName`
- `CredentialVendingContext` — added `resourceArns` and `signingName` fields
- `AwsStorageConfigurationInfo` — added `signingName` getter
- `CatalogEntity` — `setSigningNameFromConnectionConfig()` + validation
- `CapturedConfigHolder` / `ConfigCapturingHTTPClient` — new classes to capture `tableId` from remote responses
- `IcebergCatalogHandler` — ARN construction from captured config
- `StorageAccessConfigProvider` — pass `resourceArns` through to credential vending
- `IcebergRESTExternalCatalogFactory` — wire in config capture
- `PolarisAdminService` — persist `signingName` at catalog creation
- Test: 3 new unit tests for S3 Tables policy generation (read-only, read-write, fallback)
## AI Disclosure
This implementation was developed with AI assistance (Kiro).
## Checklist
- [ ] 🛡️ Don't disclose security issues! (contact security@apache.org)
- [x] 🔗 Clearly explained why the changes are needed, or linked related issues: Related to #577
- [x] 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
- [x] 💡 Added comments for complex logic
- [ ] 🧾 Updated `CHANGELOG.md` (if needed)
- [ ] 📚 Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "aritragster (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
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<String> 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<String, String> 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:
how about specifying this till : `arn:aws:s3tables`
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/ConfigCapturingHTTPClient.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * 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.service.catalog.iceberg;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.function.Consumer;
+import org.apache.iceberg.rest.RESTClient;
+import org.apache.iceberg.rest.RESTRequest;
+import org.apache.iceberg.rest.RESTResponse;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+import org.apache.iceberg.rest.responses.LoadTableResponse;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A delegating wrapper around the Iceberg {@link RESTClient} that intercepts responses to extract
+ * the {@code config} section from loadTable responses. When a {@link LoadTableResponse} is
+ * received, the config map (containing {@code tableId} for S3 Tables) is captured and stored in the
+ * request-scoped {@link CapturedConfigHolder}.
+ */
+public class ConfigCapturingHTTPClient implements RESTClient {
Review Comment:
rather than overriding the rest client i wonder if we can inject the `@RequestScopedBean` we introduced i.e `CapturedConfigHolder` where we are doing actual loadTable ? i am assuming as part of federaton we would be calling irc's loadTable anyways ?
##########
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;
Review Comment:
lets move to a private function below, also lets writes an assumption, i would say that call to federated catalog has succeeded and now we want to .....
I don't fully understand the contextConfigholder requirement though let me think more about that as well meanwhile.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
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<AwsS3TablesStorageConfigurationInfo> {
+
+ // 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<AwsCredentialsProvider> credentialsProvider;
+
+ public AwsS3TablesCredentialsStorageIntegration(
+ AwsS3TablesStorageConfigurationInfo config,
+ StsClientProvider stsClientProvider,
+ Optional<AwsCredentialsProvider> credentialsProvider) {
+ super(config, AwsS3TablesCredentialsStorageIntegration.class.getName());
+ this.stsClientProvider = stsClientProvider;
+ this.credentialsProvider = credentialsProvider;
+ }
+
+ @Override
+ public StorageAccessConfig getSubscopedCreds(
+ @Nonnull RealmConfig realmConfig,
+ boolean allowListOperation,
+ @Nonnull Set<String> allowedReadLocations,
+ @Nonnull Set<String> allowedWriteLocations,
+ @Nonnull PolarisPrincipal polarisPrincipal,
+ Optional<String> 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_SUBSCOPED_CREDENTIAL);
+ String roleSessionName =
+ includePrincipalName
+ ? AwsRoleSessionNameSanitizer.sanitize("polaris-" + polarisPrincipal.getName())
+ : "PolarisAwsS3TablesCredentialsStorageIntegration";
+
+ AssumeRoleRequest.Builder request =
+ AssumeRoleRequest.builder()
+ .externalId(storageConfig.getExternalId())
+ .roleArn(storageConfig.getRoleARN())
+ .roleSessionName(roleSessionName)
+ .policy(policy.toJson())
+ .durationSeconds(durationSeconds);
+
+ // Session tags support
+ List<String> sessionTagFieldNames =
+ realmConfig.getConfig(FeatureConfiguration.SESSION_TAGS_IN_SUBSCOPED_CREDENTIAL);
+ Set<SessionTagField> enabledSessionTagFields =
+ sessionTagFieldNames.stream()
+ .map(SessionTagField::fromConfigName)
+ .flatMap(Optional::stream)
+ .collect(Collectors.toCollection(() -> EnumSet.noneOf(SessionTagField.class)));
+
+ if (!enabledSessionTagFields.isEmpty()) {
+ List<Tag> sessionTags =
+ buildSessionTags(
+ polarisPrincipal.getName(), credentialVendingContext, enabledSessionTagFields);
+ if (!sessionTags.isEmpty()) {
+ request.tags(sessionTags);
+ request.transitiveTagKeys(sessionTags.stream().map(Tag::key).collect(Collectors.toList()));
+ }
+ }
+
+ credentialsProvider.ifPresent(
+ cp -> request.overrideConfiguration(b -> b.credentialsProvider(cp)));
+
+ @SuppressWarnings("resource")
+ StsClient stsClient =
+ stsClientProvider.stsClient(StsDestination.of(storageConfig.getStsEndpointUri(), region));
+
+ AssumeRoleResponse response = stsClient.assumeRole(request.build());
+ accessConfig.put(StorageAccessProperty.AWS_KEY_ID, response.credentials().accessKeyId());
+ accessConfig.put(
+ StorageAccessProperty.AWS_SECRET_KEY, response.credentials().secretAccessKey());
+ accessConfig.put(StorageAccessProperty.AWS_TOKEN, response.credentials().sessionToken());
+ Optional.ofNullable(response.credentials().expiration())
+ .ifPresent(
+ i -> {
+ accessConfig.put(
+ StorageAccessProperty.EXPIRATION_TIME, String.valueOf(i.toEpochMilli()));
+ accessConfig.put(
+ StorageAccessProperty.AWS_SESSION_TOKEN_EXPIRES_AT_MS,
+ String.valueOf(i.toEpochMilli()));
+ });
+
+ if (region != null) {
+ accessConfig.put(StorageAccessProperty.CLIENT_REGION, region);
+ }
+
+ refreshCredentialsEndpoint.ifPresent(
+ endpoint ->
+ accessConfig.put(StorageAccessProperty.AWS_REFRESH_CREDENTIALS_ENDPOINT, endpoint));
+
+ return accessConfig.build();
Review Comment:
This is mostly same as S3, i would say can we move this to AWSUtil or something ? we can pass the policy string & prinicpal name as args for this since these are the one which differ from s3
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/CapturedConfigHolder.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.service.catalog.iceberg;
+
+import jakarta.enterprise.context.RequestScoped;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * A request-scoped CDI bean that holds the captured config map from the remote catalog's loadTable
+ * response. Used to pass the {@code tableId} from the HTTP interception layer to the catalog
+ * handler for S3 Tables ARN construction.
+ *
+ * <p>Thread safety: CDI {@code @RequestScoped} beans are inherently isolated per request. The
+ * {@code volatile} keyword ensures visibility across threads within the same request if async
+ * processing is used.
+ */
+@RequestScoped
+public class CapturedConfigHolder {
+
+ /** Config key for the S3 Tables table identifier in the loadTable response config section. */
+ public static final String TABLE_ID_CONFIG_KEY = "tableId";
Review Comment:
This is s3Table key, can you prefix it with that or you can have
Map<String, Map<String, String>> as well
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/ConfigCapturingHTTPClient.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * 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.service.catalog.iceberg;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.function.Consumer;
+import org.apache.iceberg.rest.RESTClient;
+import org.apache.iceberg.rest.RESTRequest;
+import org.apache.iceberg.rest.RESTResponse;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+import org.apache.iceberg.rest.responses.LoadTableResponse;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A delegating wrapper around the Iceberg {@link RESTClient} that intercepts responses to extract
+ * the {@code config} section from loadTable responses. When a {@link LoadTableResponse} is
+ * received, the config map (containing {@code tableId} for S3 Tables) is captured and stored in the
Review Comment:
Lets make this generic ! just say intercepting the raw rest responses !
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "aritragster (via GitHub)" <gi...@apache.org>.
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<LoadTableResponse> 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<String> capturedTableId = capturedConfigHolder().getTableId();
+ List<String> 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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "aritragster (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "aritragster (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4052:
URL: https://github.com/apache/polaris/pull/4052#issuecomment-4760648796
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "ganeshsa986 (via GitHub)" <gi...@apache.org>.
ganeshsa986 commented on PR #4052:
URL: https://github.com/apache/polaris/pull/4052#issuecomment-6263272069
We have been evaluating Polaris as a governance layer in front of Amazon S3 Tables, hit exactly the
failure this PR describes on 1.7.0, and have since deployed a build of this branch and exercised it
against a real S3 Tables table bucket. Five things from that which may be useful, and an offer at
the end.
### 1. Independent reproduction of the failure mode on 1.7.0
Federating an `EXTERNAL` catalog to the S3 Tables Iceberg REST endpoint works for metadata.
`listNamespaces`, `listTables` and `loadTable` all succeed once `remoteCatalogName` is set to the
table bucket ARN.
Credential vending then appears to succeed and does not work. Polaris returns a well-formed `ASIA`
key with session token and expiry, and the credential authorises nothing — `GetObject` against the
table's warehouse location returns `AccessDenied`.
The cause matches this PR's problem statement. The storage role holds `s3tables:GetTableData` on
`arn:aws:s3tables:REGION:ACCOUNT:bucket/BUCKET/table/*`, while the generated session policy
grants `s3:GetObject` on the `s3://` warehouse path. Effective permission is role ∩ session policy,
which is empty. Worth noting that the role on its own can read the table; it is the scoping step
that removes the access. The failure surfaces as a bare `AccessDenied` at first read, a long way
from the cause.
### 2. The remote endpoint does return `tableId`
`S3TablesUtil.resolveTableLocations` fails closed with *"Ensure the remote S3 Tables endpoint
returns tableId in the loadTable config."* I could not tell from the thread whether that had been
confirmed against real AWS, so we issued a SigV4-signed `loadTable` directly against
`https://s3tables.REGION.amazonaws.com/iceberg`:
```
HTTP 200
top-level keys: ['config', 'metadata', 'metadata-location']
config keys: ['namespaceId', 'tableBucketId', 'tableId']
```
So the key this PR depends on is present. Two further details:
- The `tableId` is the same UUID that prefixes the AWS-generated warehouse bucket name
(`s3://TABLEID-PREFIX...--table-s3/metadata/...`), so a constructed ARN can be cross-checked
against `metadata-location`.
- `namespaceId` and `tableBucketId` are returned as well. Since `tableBucketId` is available, the
table ARN could in principle be assembled without relying on the catalog's
`default-base-location`, if that is ever preferable to the current validation requirement.
### 3. The ARN base-location validation removes a real workaround
On 1.7.0, catalog creation rejects an ARN location:
```
Location prefix not allowed: 'arn:aws:s3tables:...' expected prefixes: 's3://,s3a://'
```
The workaround is to register a placeholder `s3://` location and then add each table's
AWS-generated `--table-s3` bucket to `allowedLocations` individually. Those bucket names are
per-table and not predictable, so it does not scale past a demo. Expressed as an S3 Tables ARN,
`arn:aws:s3tables:REGION:ACCOUNT:bucket/BUCKET/table/*` is a stable prefix covering every
table in the bucket, which is what the validation in this PR enforces.
### 4. End-to-end validation of this branch against a production table bucket
Running this branch's build, with `SUPPORTED_CATALOG_STORAGE_TYPES=["S3","S3_TABLES"]`,
`ENABLE_CATALOG_FEDERATION` and `ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS`, against a table
bucket containing four live tables, with a read-only storage role scoped to those four table ARNs:
- Vended credential assumes the expected role — session name
`PolarisAwsS3TablesCredentialsStorageIntegration`.
- Read through the vended credential succeeds: `format-version 3`, 31 fields, correct `table-uuid`.
- `PutObject` through the same credential is refused by AWS, as the role is read-only.
- `loadTable` on a table in a namespace the principal was not granted is refused by Polaris at
`LOAD_TABLE_WITH_READ_DELEGATION`.
- A credential vended for one namespace gets `AccessDenied` against another namespace's storage,
i.e. the scoping is doing real work rather than vending a bucket-wide credential.
Three principals with deliberately different table grants behaved as expected, including the case
where a principal can list a table and is refused on `loadTable` — so a denial reads as a denial
rather than as absence.
**What this does not cover, and it is the gap flyingImer raised on 2026-03-28 and 2026-04-01:** our
storage role is read-only, so the write and create paths are untested here. We cannot speak to
whether create-time scoping is correct. If it would help, we can stand up a writable role and
exercise `createTable` / `UpdateTableMetadataLocation` against the same bucket.
### 5. `validateAllowedStorageType=false` does not have the intended effect
This substantiates flyingImer's 2026-04-30 point #1 concretely. The branch adds:
```java
S3("org.apache.iceberg.aws.s3.S3FileIO", true),
S3_TABLES("org.apache.iceberg.aws.s3.S3FileIO", true, false),
```
Both entries carry the same FileIO implementation string, and the static initializer in
`StorageTypeFileIO` populates the reverse map unconditionally:
```java
for (var value : StorageTypeFileIO.values()) {
if (value.validateAllowedStorageType()) { ... }
map.put(value.fileIoImplementation, value); // runs for every value
}
```
`values()` iterates in declaration order, so `S3_TABLES` is written last and wins. Confirmed by
running against this branch's compiled classes:
```
enum declaration order:
S3 validateAllowedStorageType=true
S3_TABLES validateAllowedStorageType=false
...
fromFileIoImplementation("org.apache.iceberg.aws.s3.S3FileIO")
resolved to : S3_TABLES
validateAllowedStorageType: false
```
So `fromFileIoImplementation` returns `S3_TABLES` for **every** S3 catalog, not only S3 Tables ones.
Because `S3_TABLES` opts out of `validateAllowedStorageType`, this check in
`IcebergPropertiesValidation` is then skipped for plain S3 catalogs as well:
```java
if (storageType.validateAllowedStorageType()
&& !realmConfig.getConfig(SUPPORTED_CATALOG_STORAGE_TYPES).contains(storageType.name())) {
throw new ValidationException(...);
}
```
So a realm that removes `S3` from `SUPPORTED_CATALOG_STORAGE_TYPES` in order to disable S3 catalogs
would find the restriction silently ineffective. Nothing visible breaks today because both entries
are `safe=true`, which is presumably why it passes tests. On main there is no `S3_TABLES` entry, so
nothing collides and `S3` resolves correctly. Happy to open this as a standalone issue with a
failing test if that is preferred to folding it into the redesign below.
### 6. A shape that fits current main, with one open question
main has moved on from #3699. There is now an `S3CredentialVendingMechanism` SPI — CDI beans
annotated `@Identifier("<mechanism>")`, selected per-catalog by `credentialVendingMechanism`, with
a realm allowlist in `SUPPORTED_S3_CREDENTIAL_VENDING_MECHANISMS` and `STS` as the default.
`StsCredentialVendingMechanism` is ~80 lines and does nothing but return an integration from
`integrationFor(...)`.
S3 Tables looks like a natural second mechanism:
- `S3TablesCredentialVendingMechanism` with `@Identifier("S3_TABLES")`, returning an integration
that extends `CachingStorageIntegration` and implements `getStorageAccessConfig(List<LocationGrant>,
...)`, `buildCacheKey` and `addExtraProperties`.
- The `s3tables:` policy builder in `AwsS3TablesCredentialsStorageIntegration` carries over
essentially unchanged; read and write ARN sets come from partitioning the grants by action, the
same way `AwsCredentialsStorageIntegration` already derives `readLocations` / `writeLocations`.
- `S3TablesUtil`, `ConfigCapturingHTTPClient` and `CapturedConfigHolder` carry over as-is, with the
existing TODO pointing at apache/iceberg#16399.
- `AwsStsUtil`, the `StorageType.S3_TABLES` entry, the `StorageTypeFileIO` opt-out and
`validateAccessToLocations` all drop out — STS handling now lives in the mechanism plus the
caching base class, and `validateAccessToLocations` no longer exists on main.
On the contract question from 2026-08-25 — whether `LocationGrant.locations` can represent a
backend-native resource such as an S3 Tables table ARN — it appears to need no change.
`LocationGrant` is `(Set<String> locations, Set<PolarisStorageActions> actions)`, the core does not
parse those strings, and every consumer of `locations()` is inside a provider's own integration. A
dedicated S3 Tables integration can read them as ARNs. So the abstraction does not need
generalizing for this case.
**The open question is the location prefix, not the grant.** `AwsStorageConfigurationInfo`
hardcodes `StorageType.S3`, whose prefixes are `s3://` and `s3a://`, and
`validatePrefixForStorageType` runs when the storage config is constructed — before
`validateS3CredentialVendingMechanism` in `PolarisAdminService` — so a mechanism's `validate()`
hook cannot accept an ARN location that the prefix check has already refused.
Two ways to resolve it, and we would rather be told which is acceptable than guess:
- **(a)** Add `arn:aws:s3tables:` to the prefixes accepted for `StorageType.S3`, and enforce the
correspondence in the mechanism hooks — `STS.validate()` refuses ARN locations, and
`S3_TABLES.validate()` requires them. No new storage type, so no FileIO-string collision and no
opt-out. The cost is that location-shape enforcement moves from the config class into the
mechanisms.
- **(b)** Introduce `StorageType.S3_TABLES` properly, which then requires fixing the
`FileIO impl -> storage type` reverse lookup rather than opting out of it — i.e. the structural
fix tracked in #4486, since two storage types legitimately share `S3FileIO`.
(a) is smaller and avoids #4486 entirely; (b) is the more honest modelling if a storage type is
considered the right unit. Either is fine by us.
### Status, and an offer
@aritragster — are you still carrying this forward? The 2026-08-25 question has been open for a
while, and we did not want to duplicate work.
If you are, we are glad to help with validation: we have a live S3 Tables table bucket, a working
federated catalog, and the harness described in section 4.
If you are not, we are willing to do the port described in section 6 and open it as a follow-up PR,
keeping you as co-author on the commits that carry your code. The S3 Tables knowledge in this PR —
the `s3tables:` action set, the ARN construction, and the `tableId` capture — is the hard part and
it is all still correct; what has gone stale is the surrounding vending shape, which main has since
replaced.
Before starting we would want a steer on the (a)/(b) question above, so the port lands in a shape
reviewers are willing to take.
cc @flyingImer @singhpk234 @dimas-b
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "aritragster (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #4052: feat: Add S3 Tables credential vending support for federated catalogs
URL: https://github.com/apache/polaris/pull/4052
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
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<AccessDelegationMode> delegationModes,
Set<PolarisStorageActions> actions,
- Optional<String> refreshCredentialsEndpoint) {
+ Optional<String> refreshCredentialsEndpoint,
+ Optional<String> 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<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.
+ // 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 that a constructed S3 Tables ARN falls under one of the catalog's allowed locations.
+ * This prevents a malicious remote catalog from returning a tableId that would construct an ARN
+ * outside the catalog's authorized scope.
+ */
+ private void validateS3TablesArn(
+ TableIdentifier tableIdentifier, String tableArn, CatalogEntity catalogEntity) {
+ PolarisStorageConfigurationInfo storageConfig = catalogEntity.getStorageConfigurationInfo();
+ if (storageConfig == null) {
+ return;
+ }
+ List<String> allowedLocations = storageConfig.getAllowedLocations();
+ boolean isAllowed =
+ allowedLocations.stream().anyMatch(allowed -> tableArn.startsWith(allowed + "/"));
+ if (!isAllowed) {
+ throw new ForbiddenException(
+ "Table '%s' has ARN '%s' which is outside the catalog's allowed locations: %s",
+ tableIdentifier, tableArn, allowedLocations);
+ }
+ }
+
Review Comment:
can we capture all of these in a dedicate util class
##########
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:
Might have to think more about this one : if there is a better way to capture the config part of loadTable response
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsS3TablesCredentialsStorageIntegration.java:
##########
@@ -0,0 +1,220 @@
+/*
+ * 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<AwsS3TablesStorageConfigurationInfo> {
+
+ private final StsClientProvider stsClientProvider;
+ private final Optional<AwsCredentialsProvider> credentialsProvider;
+
+ public AwsS3TablesCredentialsStorageIntegration(
+ AwsS3TablesStorageConfigurationInfo config,
+ StsClientProvider stsClientProvider,
+ Optional<AwsCredentialsProvider> credentialsProvider) {
+ super(config, AwsS3TablesCredentialsStorageIntegration.class.getName());
+ this.stsClientProvider = stsClientProvider;
+ this.credentialsProvider = credentialsProvider;
+ }
+
+ @Override
+ public StorageAccessConfig getSubscopedCreds(
+ @Nonnull RealmConfig realmConfig,
+ boolean allowListOperation,
+ @Nonnull Set<String> allowedReadLocations,
+ @Nonnull Set<String> allowedWriteLocations,
+ @Nonnull PolarisPrincipal polarisPrincipal,
+ Optional<String> 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_SUBSCOPED_CREDENTIAL);
+ String roleSessionName =
+ includePrincipalName
+ ? AwsRoleSessionNameSanitizer.sanitize("polaris-" + polarisPrincipal.getName())
+ : "PolarisAwsS3TablesCredentialsStorageIntegration";
+
+ AssumeRoleRequest.Builder request =
+ AssumeRoleRequest.builder()
+ .externalId(storageConfig.getExternalId())
+ .roleArn(storageConfig.getRoleARN())
+ .roleSessionName(roleSessionName)
+ .policy(policy.toJson())
+ .durationSeconds(durationSeconds);
+
+ // Session tags support
+ List<String> sessionTagFieldNames =
+ realmConfig.getConfig(FeatureConfiguration.SESSION_TAGS_IN_SUBSCOPED_CREDENTIAL);
+ Set<SessionTagField> enabledSessionTagFields =
+ sessionTagFieldNames.stream()
+ .map(SessionTagField::fromConfigName)
+ .flatMap(Optional::stream)
+ .collect(Collectors.toCollection(() -> EnumSet.noneOf(SessionTagField.class)));
+
+ if (!enabledSessionTagFields.isEmpty()) {
+ List<Tag> sessionTags =
+ buildSessionTags(
+ polarisPrincipal.getName(), credentialVendingContext, enabledSessionTagFields);
+ if (!sessionTags.isEmpty()) {
+ request.tags(sessionTags);
+ request.transitiveTagKeys(sessionTags.stream().map(Tag::key).collect(Collectors.toList()));
+ }
+ }
+
+ credentialsProvider.ifPresent(
+ cp -> request.overrideConfiguration(b -> b.credentialsProvider(cp)));
+
+ @SuppressWarnings("resource")
+ StsClient stsClient =
+ stsClientProvider.stsClient(StsDestination.of(storageConfig.getStsEndpointUri(), region));
+
+ AssumeRoleResponse response = stsClient.assumeRole(request.build());
+ accessConfig.put(StorageAccessProperty.AWS_KEY_ID, response.credentials().accessKeyId());
+ accessConfig.put(
+ StorageAccessProperty.AWS_SECRET_KEY, response.credentials().secretAccessKey());
+ accessConfig.put(StorageAccessProperty.AWS_TOKEN, response.credentials().sessionToken());
+ Optional.ofNullable(response.credentials().expiration())
+ .ifPresent(
+ i -> {
+ accessConfig.put(
+ StorageAccessProperty.EXPIRATION_TIME, String.valueOf(i.toEpochMilli()));
+ accessConfig.put(
+ StorageAccessProperty.AWS_SESSION_TOKEN_EXPIRES_AT_MS,
+ String.valueOf(i.toEpochMilli()));
+ });
+
+ if (region != null) {
+ accessConfig.put(StorageAccessProperty.CLIENT_REGION, region);
+ }
+
+ refreshCredentialsEndpoint.ifPresent(
+ endpoint ->
+ accessConfig.put(StorageAccessProperty.AWS_REFRESH_CREDENTIALS_ENDPOINT, endpoint));
+
+ return accessConfig.build();
+ }
+
+ /**
+ * Builds an IAM policy with s3tables:* actions scoped to the provided ARN resources.
+ *
+ * <p>Read actions ({@code s3tables:GetTableData}, {@code s3tables:GetTableMetadataLocation}) are
+ * scoped to all read and write ARNs. Write actions ({@code s3tables:UpdateTableMetadataLocation},
+ * {@code s3tables:PutTableData}) are scoped to write ARNs only.
+ */
+ private IamPolicy buildS3TablesPolicy(
+ AwsS3TablesStorageConfigurationInfo storageConfig,
+ Set<String> readLocations,
+ Set<String> writeLocations) {
+ IamPolicy.Builder policyBuilder = IamPolicy.builder();
+
+ // Read statement scoped to all ARNs (read + write)
+ IamStatement.Builder readStatement =
+ IamStatement.builder()
+ .effect(IamEffect.ALLOW)
+ .addAction("s3tables:GetTableData")
+ .addAction("s3tables:GetTableMetadataLocation");
+
+ Stream.concat(readLocations.stream(), writeLocations.stream())
+ .distinct()
+ .forEach(arn -> readStatement.addResource(IamResource.create(arn)));
+
+ policyBuilder.addStatement(readStatement.build());
+
+ // Write statement scoped to write ARNs only
+ boolean canWrite = !writeLocations.isEmpty();
+ if (canWrite) {
+ IamStatement.Builder writeStatement =
+ IamStatement.builder()
+ .effect(IamEffect.ALLOW)
+ .addAction("s3tables:UpdateTableMetadataLocation")
+ .addAction("s3tables:PutTableData");
+
+ writeLocations.forEach(arn -> writeStatement.addResource(IamResource.create(arn)));
+ policyBuilder.addStatement(writeStatement.build());
+ }
+
+ // KMS policy if configured
+ AwsCredentialsStorageIntegration.addKmsKeyPolicy(
+ storageConfig.getCurrentKmsKey(),
+ storageConfig.getAllowedKmsKeys(),
+ policyBuilder,
+ canWrite,
+ storageConfig.getRegion(),
+ null);
+
+ return policyBuilder.build();
+ }
+
+ @Override
+ public @Nonnull Map<String, Map<PolarisStorageActions, ValidationResult>>
+ validateAccessToLocations(
+ @Nonnull RealmConfig realmConfig,
+ @Nonnull AwsS3TablesStorageConfigurationInfo storageConfig,
+ @Nonnull Set<PolarisStorageActions> actions,
+ @Nonnull Set<String> locations) {
+ // No-op stub for S3 Tables — real validation deferred to a follow-up PR.
+ return Map.of();
Review Comment:
can you please add an issue and reference the issue link here ?
follow-up PR is not a meaningful name
happy to assign it to you since you have already something working
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "aritragster (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "aritragster (via GitHub)" <gi...@apache.org>.
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<LoadTableResponse> 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<String> capturedTableId = capturedConfigHolder().getTableId();
+ List<String> 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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "aritragster (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
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<LoadTableResponse> 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<String> capturedTableId = capturedConfigHolder().getTableId();
+ List<String> 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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
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<LoadTableResponse> 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<String> capturedTableId = capturedConfigHolder().getTableId();
+ List<String> 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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "aritragster (via GitHub)" <gi...@apache.org>.
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<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;
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "aritragster (via GitHub)" <gi...@apache.org>.
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<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:
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "aritragster (via GitHub)" <gi...@apache.org>.
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<String, String> 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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "ganeshsa986 (via GitHub)" <gi...@apache.org>.
ganeshsa986 commented on PR #4052:
URL: https://github.com/apache/polaris/pull/4052#issuecomment-6263224583
We have been evaluating Polaris as a governance layer in front of Amazon S3 Tables, hit exactly the
failure this PR describes on 1.7.0, and have since deployed a build of this branch and exercised it
against a real S3 Tables table bucket. Five things from that which may be useful, and an offer at
the end.
### 1. Independent reproduction of the failure mode on 1.7.0
Federating an `EXTERNAL` catalog to the S3 Tables Iceberg REST endpoint works for metadata.
`listNamespaces`, `listTables` and `loadTable` all succeed once `remoteCatalogName` is set to the
table bucket ARN.
Credential vending then appears to succeed and does not work. Polaris returns a well-formed `ASIA`
key with session token and expiry, and the credential authorises nothing — `GetObject` against the
table's warehouse location returns `AccessDenied`.
The cause matches this PR's problem statement. The storage role holds `s3tables:GetTableData` on
`arn:aws:s3tables:REGION:ACCOUNT:bucket/BUCKET/table/*`, while the generated session policy
grants `s3:GetObject` on the `s3://` warehouse path. Effective permission is role ∩ session policy,
which is empty. Worth noting that the role on its own can read the table; it is the scoping step
that removes the access. The failure surfaces as a bare `AccessDenied` at first read, a long way
from the cause.
### 2. The remote endpoint does return `tableId`
`S3TablesUtil.resolveTableLocations` fails closed with *"Ensure the remote S3 Tables endpoint
returns tableId in the loadTable config."* I could not tell from the thread whether that had been
confirmed against real AWS, so we issued a SigV4-signed `loadTable` directly against
`https://s3tables.REGION.amazonaws.com/iceberg`:
```
HTTP 200
top-level keys: ['config', 'metadata', 'metadata-location']
config keys: ['namespaceId', 'tableBucketId', 'tableId']
```
So the key this PR depends on is present. Two further details:
- The `tableId` is the same UUID that prefixes the AWS-generated warehouse bucket name
(`s3://TABLEID-PREFIX...--table-s3/metadata/...`), so a constructed ARN can be cross-checked
against `metadata-location`.
- `namespaceId` and `tableBucketId` are returned as well. Since `tableBucketId` is available, the
table ARN could in principle be assembled without relying on the catalog's
`default-base-location`, if that is ever preferable to the current validation requirement.
### 3. The ARN base-location validation removes a real workaround
On 1.7.0, catalog creation rejects an ARN location:
```
Location prefix not allowed: 'arn:aws:s3tables:...' expected prefixes: 's3://,s3a://'
```
The workaround is to register a placeholder `s3://` location and then add each table's
AWS-generated `--table-s3` bucket to `allowedLocations` individually. Those bucket names are
per-table and not predictable, so it does not scale past a demo. Expressed as an S3 Tables ARN,
`arn:aws:s3tables:REGION:ACCOUNT:bucket/BUCKET/table/*` is a stable prefix covering every
table in the bucket, which is what the validation in this PR enforces.
### 4. End-to-end validation of this branch against a production table bucket
Running this branch's build, with `SUPPORTED_CATALOG_STORAGE_TYPES=["S3","S3_TABLES"]`,
`ENABLE_CATALOG_FEDERATION` and `ENABLE_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS`, against a table
bucket containing four live tables, with a read-only storage role scoped to those four table ARNs:
- Vended credential assumes the expected role — session name
`PolarisAwsS3TablesCredentialsStorageIntegration`.
- Read through the vended credential succeeds: `format-version 3`, 31 fields, correct `table-uuid`.
- `PutObject` through the same credential is refused by AWS, as the role is read-only.
- `loadTable` on a table in a namespace the principal was not granted is refused by Polaris at
`LOAD_TABLE_WITH_READ_DELEGATION`.
- A credential vended for one namespace gets `AccessDenied` against another namespace's storage,
i.e. the scoping is doing real work rather than vending a bucket-wide credential.
Three principals with deliberately different table grants behaved as expected, including the case
where a principal can list a table and is refused on `loadTable` — so a denial reads as a denial
rather than as absence.
**What this does not cover, and it is the gap flyingImer raised on 2026-03-28 and 2026-04-01:** our
storage role is read-only, so the write and create paths are untested here. We cannot speak to
whether create-time scoping is correct. If it would help, we can stand up a writable role and
exercise `createTable` / `UpdateTableMetadataLocation` against the same bucket.
### 5. `validateAllowedStorageType=false` does not have the intended effect
This substantiates flyingImer's 2026-04-30 point #1 concretely. The branch adds:
```java
S3("org.apache.iceberg.aws.s3.S3FileIO", true),
S3_TABLES("org.apache.iceberg.aws.s3.S3FileIO", true, false),
```
Both entries carry the same FileIO implementation string, and the static initializer in
`StorageTypeFileIO` populates the reverse map unconditionally:
```java
for (var value : StorageTypeFileIO.values()) {
if (value.validateAllowedStorageType()) { ... }
map.put(value.fileIoImplementation, value); // runs for every value
}
```
`values()` iterates in declaration order, so `S3_TABLES` is written last and wins. Confirmed by
running against this branch's compiled classes:
```
enum declaration order:
S3 validateAllowedStorageType=true
S3_TABLES validateAllowedStorageType=false
...
fromFileIoImplementation("org.apache.iceberg.aws.s3.S3FileIO")
resolved to : S3_TABLES
validateAllowedStorageType: false
```
So `fromFileIoImplementation` returns `S3_TABLES` for **every** S3 catalog, not only S3 Tables ones.
Because `S3_TABLES` opts out of `validateAllowedStorageType`, this check in
`IcebergPropertiesValidation` is then skipped for plain S3 catalogs as well:
```java
if (storageType.validateAllowedStorageType()
&& !realmConfig.getConfig(SUPPORTED_CATALOG_STORAGE_TYPES).contains(storageType.name())) {
throw new ValidationException(...);
}
```
So a realm that removes `S3` from `SUPPORTED_CATALOG_STORAGE_TYPES` in order to disable S3 catalogs
would find the restriction silently ineffective. Nothing visible breaks today because both entries
are `safe=true`, which is presumably why it passes tests. On main there is no `S3_TABLES` entry, so
nothing collides and `S3` resolves correctly. Happy to open this as a standalone issue with a
failing test if that is preferred to folding it into the redesign below.
### 6. A shape that fits current main, with one open question
main has moved on from #3699. There is now an `S3CredentialVendingMechanism` SPI — CDI beans
annotated `@Identifier("<mechanism>")`, selected per-catalog by `credentialVendingMechanism`, with
a realm allowlist in `SUPPORTED_S3_CREDENTIAL_VENDING_MECHANISMS` and `STS` as the default.
`StsCredentialVendingMechanism` is ~80 lines and does nothing but return an integration from
`integrationFor(...)`.
S3 Tables looks like a natural second mechanism:
- `S3TablesCredentialVendingMechanism` with `@Identifier("S3_TABLES")`, returning an integration
that extends `CachingStorageIntegration` and implements `getStorageAccessConfig(List<LocationGrant>,
...)`, `buildCacheKey` and `addExtraProperties`.
- The `s3tables:` policy builder in `AwsS3TablesCredentialsStorageIntegration` carries over
essentially unchanged; read and write ARN sets come from partitioning the grants by action, the
same way `AwsCredentialsStorageIntegration` already derives `readLocations` / `writeLocations`.
- `S3TablesUtil`, `ConfigCapturingHTTPClient` and `CapturedConfigHolder` carry over as-is, with the
existing TODO pointing at apache/iceberg#16399.
- `AwsStsUtil`, the `StorageType.S3_TABLES` entry, the `StorageTypeFileIO` opt-out and
`validateAccessToLocations` all drop out — STS handling now lives in the mechanism plus the
caching base class, and `validateAccessToLocations` no longer exists on main.
On the contract question from 2026-08-25 — whether `LocationGrant.locations` can represent a
backend-native resource such as an S3 Tables table ARN — it appears to need no change.
`LocationGrant` is `(Set<String> locations, Set<PolarisStorageActions> actions)`, the core does not
parse those strings, and every consumer of `locations()` is inside a provider's own integration. A
dedicated S3 Tables integration can read them as ARNs. So the abstraction does not need
generalizing for this case.
**The open question is the location prefix, not the grant.** `AwsStorageConfigurationInfo`
hardcodes `StorageType.S3`, whose prefixes are `s3://` and `s3a://`, and
`validatePrefixForStorageType` runs when the storage config is constructed — before
`validateS3CredentialVendingMechanism` in `PolarisAdminService` — so a mechanism's `validate()`
hook cannot accept an ARN location that the prefix check has already refused.
Two ways to resolve it, and we would rather be told which is acceptable than guess:
- **(a)** Add `arn:aws:s3tables:` to the prefixes accepted for `StorageType.S3`, and enforce the
correspondence in the mechanism hooks — `STS.validate()` refuses ARN locations, and
`S3_TABLES.validate()` requires them. No new storage type, so no FileIO-string collision and no
opt-out. The cost is that location-shape enforcement moves from the config class into the
mechanisms.
- **(b)** Introduce `StorageType.S3_TABLES` properly, which then requires fixing the
`FileIO impl -> storage type` reverse lookup rather than opting out of it — i.e. the structural
fix tracked in #4486, since two storage types legitimately share `S3FileIO`.
(a) is smaller and avoids #4486 entirely; (b) is the more honest modelling if a storage type is
considered the right unit. Either is fine by us.
### Status, and an offer
@aritragster — are you still carrying this forward? The 2026-08-25 question has been open for a
while, and we did not want to duplicate work.
If you are, we are glad to help with validation: we have a live S3 Tables table bucket, a working
federated catalog, and the harness described in section 4.
If you are not, we are willing to do the port described in section 6 and open it as a follow-up PR,
keeping you as co-author on the commits that carry your code. The S3 Tables knowledge in this PR —
the `s3tables:` action set, the ARN construction, and the `tableId` capture — is the hard part and
it is all still correct; what has gone stale is the surrounding vending shape, which main has since
replaced.
Before starting we would want a steer on the (a)/(b) question above, so the port lands in a shape
reviewers are willing to take.
cc @flyingImer @singhpk234 @dimas-b
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: Add S3 Tables credential vending support for federated catalogs [polaris]
Posted by "aritragster (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org