From b423f7ad0a4c72e3ab06fc101ca4ab84b8fd47b3 Mon Sep 17 00:00:00 2001 From: Dmitri Bourlatchkov Date: Tue, 8 Jul 2025 19:46:14 -0400 Subject: [PATCH 1/6] Add `pathStyleAccess` to AwsStorageConfigInfo This change allows configuring the "path-style" access mode in S3 clients (both in Polaris Servers and Iceberg REST Catalog API clients). This change is applicable both to AWS storage and to non-AWS S3-compatible storage (#1530). The Management API change is backward-compatible. --- .../polaris/service/it/env/CatalogApi.java | 17 +++++++- .../polaris/service/it/env/RestApi.java | 10 +++-- .../polaris/core/entity/CatalogEntity.java | 3 +- .../aws/AwsCredentialsStorageIntegration.java | 4 ++ .../aws/AwsStorageConfigurationInfo.java | 19 +++++++-- .../aws/AwsStorageConfigurationInfoTest.java | 14 ++++++- .../quarkus/it/QuarkusRestCatalogMinIoIT.java | 39 ++++++++++++++----- spec/polaris-management-service.yml | 6 +++ 8 files changed, 94 insertions(+), 18 deletions(-) diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java b/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java index 0274d0ea81..eb07d55025 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java @@ -154,12 +154,27 @@ public void dropTable(String catalog, TableIdentifier id) { } public LoadTableResponse loadTable(String catalog, TableIdentifier id, String snapshots) { + return loadTable(catalog, id, snapshots, Map.of()); + } + + public LoadTableResponse loadTableWithAccessDelegation( + String catalog, TableIdentifier id, String snapshots) { + return loadTable( + catalog, id, snapshots, Map.of("X-Iceberg-Access-Delegation", "vended-credentials")); + } + + public LoadTableResponse loadTable( + String catalog, TableIdentifier id, String snapshots, Map headers) { + HashMap allHeaders = new HashMap<>(defaultHeaders()); + allHeaders.putAll(headers); + String ns = RESTUtil.encodeNamespace(id.namespace()); try (Response res = request( "v1/{cat}/namespaces/" + ns + "/tables/{table}", Map.of("cat", catalog, "table", id.name()), - snapshots == null ? Map.of() : Map.of("snapshots", snapshots)) + snapshots == null ? Map.of() : Map.of("snapshots", snapshots), + allHeaders) .get()) { if (res.getStatus() == Response.Status.OK.getStatusCode()) { return res.readEntity(LoadTableResponse.class); diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/env/RestApi.java b/integration-tests/src/main/java/org/apache/polaris/service/it/env/RestApi.java index c0475f088b..419458fe1a 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/env/RestApi.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/env/RestApi.java @@ -47,14 +47,18 @@ public Invocation.Builder request(String path, Map templateValue return request(path, templateValues, Map.of()); } - public Invocation.Builder request( - String path, Map templateValues, Map queryParams) { + protected Map defaultHeaders() { Map headers = new HashMap<>(); headers.put(endpoints.realmHeaderName(), endpoints.realmId()); if (authToken != null) { headers.put("Authorization", "Bearer " + authToken); } - return request(path, templateValues, queryParams, headers); + return headers; + } + + public Invocation.Builder request( + String path, Map templateValues, Map queryParams) { + return request(path, templateValues, queryParams, defaultHeaders()); } public Invocation.Builder request( diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java index e0d2089a7b..d516f11d3b 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java @@ -275,7 +275,8 @@ public Builder setStorageConfigurationInfo( awsConfigModel.getExternalId(), awsConfigModel.getRegion(), awsConfigModel.getEndpoint(), - awsConfigModel.getStsEndpoint()); + awsConfigModel.getStsEndpoint(), + awsConfigModel.getPathStyleAccess()); awsConfig.validateArn(awsConfigModel.getRoleArn()); config = awsConfig; break; diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java index b473c077ad..7f236061a5 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java @@ -122,6 +122,10 @@ public EnumMap getSubscopedCreds( credentialMap.put(StorageAccessProperty.AWS_ENDPOINT, endpointUri.toString()); } + if (storageConfig.forcePathStyleAccess()) { + credentialMap.put(StorageAccessProperty.AWS_PATH_STYLE_ACCESS, Boolean.TRUE.toString()); + } + if (storageConfig.getAwsPartition().equals("aws-us-gov") && credentialMap.get(StorageAccessProperty.CLIENT_REGION) == null) { throw new IllegalArgumentException( diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java index 666d4b0ea2..813fba2c1f 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java @@ -26,6 +26,7 @@ import jakarta.annotation.Nullable; import java.net.URI; import java.util.List; +import java.util.Optional; import java.util.regex.Matcher; import java.util.regex.Pattern; import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; @@ -62,6 +63,10 @@ public class AwsStorageConfigurationInfo extends PolarisStorageConfigurationInfo @JsonProperty(value = "stsEndpoint") private @Nullable String stsEndpoint; + /** A flag indicating whether path-style bucket access should be forced in S3 clients. */ + @JsonProperty(value = "pathStyleAccess") + private boolean pathStyleAccess; + @JsonCreator public AwsStorageConfigurationInfo( @JsonProperty(value = "storageType", required = true) @Nonnull StorageType storageType, @@ -71,13 +76,15 @@ public AwsStorageConfigurationInfo( @JsonProperty(value = "externalId") @Nullable String externalId, @JsonProperty(value = "region", required = false) @Nullable String region, @JsonProperty(value = "endpoint") @Nullable String endpoint, - @JsonProperty(value = "stsEndpoint") @Nullable String stsEndpoint) { + @JsonProperty(value = "stsEndpoint") @Nullable String stsEndpoint, + @JsonProperty(value = "pathStyleAccess") @Nullable Boolean pathStyleAccess) { super(storageType, allowedLocations); this.roleARN = roleARN; this.externalId = externalId; this.region = region; this.endpoint = endpoint; this.stsEndpoint = stsEndpoint; + this.pathStyleAccess = Optional.ofNullable(pathStyleAccess).orElse(false); } public AwsStorageConfigurationInfo( @@ -85,7 +92,7 @@ public AwsStorageConfigurationInfo( @Nonnull List allowedLocations, @Nonnull String roleARN, @Nullable String region) { - this(storageType, allowedLocations, roleARN, null, region, null, null); + this(storageType, allowedLocations, roleARN, null, region, null, null, false); } public AwsStorageConfigurationInfo( @@ -94,7 +101,7 @@ public AwsStorageConfigurationInfo( @Nonnull String roleARN, @Nullable String externalId, @Nullable String region) { - this(storageType, allowedLocations, roleARN, externalId, region, null, null); + this(storageType, allowedLocations, roleARN, externalId, region, null, null, false); } @Override @@ -149,6 +156,12 @@ public URI getEndpointUri() { return endpoint == null ? null : URI.create(endpoint); } + /** Returns a flag indicating whether path-style bucket access should be forced in S3 clients. */ + @JsonIgnore + public boolean forcePathStyleAccess() { + return pathStyleAccess; + } + /** Returns the STS endpoint if set, defaulting to {@link #getEndpointUri()} otherwise. */ @JsonIgnore @Nullable diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java index 61ab64ad49..831983e62d 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java @@ -29,8 +29,13 @@ public class AwsStorageConfigurationInfoTest { private static AwsStorageConfigurationInfo config(String endpoint, String stsEndpoint) { + return config(endpoint, stsEndpoint, false); + } + + private static AwsStorageConfigurationInfo config( + String endpoint, String stsEndpoint, Boolean pathStyle) { return new AwsStorageConfigurationInfo( - S3, List.of(), "role", null, null, endpoint, stsEndpoint); + S3, List.of(), "role", null, null, endpoint, stsEndpoint, pathStyle); } @Test @@ -56,4 +61,11 @@ public void testStsEndpoint() { AwsStorageConfigurationInfo::getStsEndpointUri) .containsExactly(URI.create("http://s3.example.com"), URI.create("http://sts.example.com")); } + + @Test + public void testPathStyleAccess() { + assertThat(config(null, null, null).forcePathStyleAccess()).isFalse(); + assertThat(config(null, null, false).forcePathStyleAccess()).isFalse(); + assertThat(config(null, null, true).forcePathStyleAccess()).isTrue(); + } } diff --git a/runtime/service/src/intTest/java/org/apache/polaris/service/quarkus/it/QuarkusRestCatalogMinIoIT.java b/runtime/service/src/intTest/java/org/apache/polaris/service/quarkus/it/QuarkusRestCatalogMinIoIT.java index af918f68fd..79c3751bf3 100644 --- a/runtime/service/src/intTest/java/org/apache/polaris/service/quarkus/it/QuarkusRestCatalogMinIoIT.java +++ b/runtime/service/src/intTest/java/org/apache/polaris/service/quarkus/it/QuarkusRestCatalogMinIoIT.java @@ -48,6 +48,7 @@ import org.apache.iceberg.io.PositionOutputStream; import org.apache.iceberg.rest.RESTCatalog; import org.apache.iceberg.rest.auth.OAuth2Properties; +import org.apache.iceberg.rest.responses.LoadTableResponse; import org.apache.iceberg.types.Types; import org.apache.polaris.core.admin.model.AwsStorageConfigInfo; import org.apache.polaris.core.admin.model.Catalog; @@ -68,9 +69,10 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInfo; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import software.amazon.awssdk.services.s3.S3Client; import software.amazon.awssdk.services.s3.model.GetObjectRequest; import software.amazon.awssdk.services.s3.model.GetObjectResponse; @@ -145,13 +147,15 @@ public void before(TestInfo testInfo) { catalogName = client.newEntityName(testInfo.getTestMethod().orElseThrow().getName()); } - private RESTCatalog createCatalog(Optional endpoint, Optional stsEndpoint) { + private RESTCatalog createCatalog( + Optional endpoint, Optional stsEndpoint, boolean pathStyleAccess) { AwsStorageConfigInfo.Builder storageConfig = AwsStorageConfigInfo.builder() .setRoleArn("arn:aws:iam::123456789012:role/polaris-test") .setExternalId("externalId123") .setUserArn("arn:aws:iam::123456789012:user/polaris-test") .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) + .setPathStyleAccess(pathStyleAccess) .setAllowedLocations(List.of(storageBase.toString())); endpoint.ifPresent(storageConfig::setEndpoint); @@ -190,9 +194,11 @@ public void cleanUp() { client.cleanUp(adminCredentials); } - @Test - public void testCreateTable() throws IOException { - try (RESTCatalog restCatalog = createCatalog(Optional.of(endpoint), Optional.empty())) { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testCreateTable(boolean pathStyle) throws IOException { + try (RESTCatalog restCatalog = + createCatalog(Optional.of(endpoint), Optional.empty(), pathStyle)) { catalogApi.createNamespace(catalogName, "test-ns"); TableIdentifier id = TableIdentifier.of("test-ns", "t1"); Table table = restCatalog.createTable(id, SCHEMA); @@ -212,14 +218,25 @@ public void testCreateTable() throws IOException { .response(); assertThat(response.contentLength()).isGreaterThan(0); + LoadTableResponse loadTableResponse = + catalogApi.loadTableWithAccessDelegation(catalogName, id, "ALL"); + assertThat(loadTableResponse.config()).containsKey("s3.endpoint"); + + if (pathStyle) { + assertThat(loadTableResponse.config()) + .containsEntry("s3.path-style-access", Boolean.TRUE.toString()); + } + restCatalog.dropTable(id); assertThat(restCatalog.tableExists(id)).isFalse(); } } - @Test - public void testAppendFiles() throws IOException { - try (RESTCatalog restCatalog = createCatalog(Optional.of(endpoint), Optional.of(endpoint))) { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testAppendFiles(boolean pathStyle) throws IOException { + try (RESTCatalog restCatalog = + createCatalog(Optional.of(endpoint), Optional.of(endpoint), pathStyle)) { catalogApi.createNamespace(catalogName, "test-ns"); TableIdentifier id = TableIdentifier.of("test-ns", "t1"); Table table = restCatalog.createTable(id, SCHEMA); @@ -228,7 +245,11 @@ public void testAppendFiles() throws IOException { @SuppressWarnings("resource") FileIO io = table.io(); - URI loc = URI.create(table.locationProvider().newDataLocation("test-file1.txt")); + URI loc = + URI.create( + table + .locationProvider() + .newDataLocation(String.format("test-file-%s.txt", pathStyle))); OutputFile f1 = io.newOutputFile(loc.toString()); try (PositionOutputStream os = f1.create()) { os.write("Hello World".getBytes(UTF_8)); diff --git a/spec/polaris-management-service.yml b/spec/polaris-management-service.yml index acf87f8dcc..2601500c8c 100644 --- a/spec/polaris-management-service.yml +++ b/spec/polaris-management-service.yml @@ -1056,6 +1056,12 @@ components: type: string description: endpoint for STS requests (optional). If not set, defaults to 'endpoint'. example: "https://sts.example.com:1234" + pathStyleAccess: + type: boolean + description: >- + Whether S3 requests to files in this catalog should use 'path-style addressing for buckets'. + Default: false. + example: false required: - roleArn From 2db692c28c45d820358a48d1d807a0d92165f4c0 Mon Sep 17 00:00:00 2001 From: Dmitri Bourlatchkov Date: Wed, 9 Jul 2025 09:17:49 -0400 Subject: [PATCH 2/6] review: rename to getPathStyleAccess --- .../core/storage/aws/AwsCredentialsStorageIntegration.java | 2 +- .../core/storage/aws/AwsStorageConfigurationInfo.java | 2 +- .../core/storage/aws/AwsStorageConfigurationInfoTest.java | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java index 7f236061a5..a8c94bf300 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java @@ -122,7 +122,7 @@ public EnumMap getSubscopedCreds( credentialMap.put(StorageAccessProperty.AWS_ENDPOINT, endpointUri.toString()); } - if (storageConfig.forcePathStyleAccess()) { + if (storageConfig.getPathStyleAccess()) { credentialMap.put(StorageAccessProperty.AWS_PATH_STYLE_ACCESS, Boolean.TRUE.toString()); } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java index 813fba2c1f..54d1602c3e 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java @@ -158,7 +158,7 @@ public URI getEndpointUri() { /** Returns a flag indicating whether path-style bucket access should be forced in S3 clients. */ @JsonIgnore - public boolean forcePathStyleAccess() { + public boolean getPathStyleAccess() { return pathStyleAccess; } diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java index 831983e62d..b0fc989a2b 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java @@ -64,8 +64,8 @@ public void testStsEndpoint() { @Test public void testPathStyleAccess() { - assertThat(config(null, null, null).forcePathStyleAccess()).isFalse(); - assertThat(config(null, null, false).forcePathStyleAccess()).isFalse(); - assertThat(config(null, null, true).forcePathStyleAccess()).isTrue(); + assertThat(config(null, null, null).getPathStyleAccess()).isFalse(); + assertThat(config(null, null, false).getPathStyleAccess()).isFalse(); + assertThat(config(null, null, true).getPathStyleAccess()).isTrue(); } } From f21df9523afcd15f3e68ebb7f6d8282babc4d4bd Mon Sep 17 00:00:00 2001 From: Dmitri Bourlatchkov Date: Wed, 9 Jul 2025 10:28:21 -0400 Subject: [PATCH 3/6] review: PATH_STYLE_ACCESS_DEFAULT, ternary --- .../aws/AwsStorageConfigurationInfo.java | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java index 54d1602c3e..445bbc54fb 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java @@ -26,7 +26,6 @@ import jakarta.annotation.Nullable; import java.net.URI; import java.util.List; -import java.util.Optional; import java.util.regex.Matcher; import java.util.regex.Pattern; import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; @@ -34,6 +33,8 @@ /** Aws Polaris Storage Configuration information */ public class AwsStorageConfigurationInfo extends PolarisStorageConfigurationInfo { + private static final boolean PATH_STYLE_ACCESS_DEFAULT = false; + // Technically, it should be ^arn:(aws|aws-cn|aws-us-gov):iam::(\d{12}):role/.+$, @JsonIgnore public static final String ROLE_ARN_PATTERN = "^arn:(aws|aws-us-gov):iam::(\\d{12}):role/.+$"; @@ -84,7 +85,7 @@ public AwsStorageConfigurationInfo( this.region = region; this.endpoint = endpoint; this.stsEndpoint = stsEndpoint; - this.pathStyleAccess = Optional.ofNullable(pathStyleAccess).orElse(false); + this.pathStyleAccess = pathStyleAccess == null ? PATH_STYLE_ACCESS_DEFAULT : pathStyleAccess; } public AwsStorageConfigurationInfo( @@ -92,7 +93,15 @@ public AwsStorageConfigurationInfo( @Nonnull List allowedLocations, @Nonnull String roleARN, @Nullable String region) { - this(storageType, allowedLocations, roleARN, null, region, null, null, false); + this( + storageType, + allowedLocations, + roleARN, + null, + region, + null, + null, + PATH_STYLE_ACCESS_DEFAULT); } public AwsStorageConfigurationInfo( @@ -101,7 +110,15 @@ public AwsStorageConfigurationInfo( @Nonnull String roleARN, @Nullable String externalId, @Nullable String region) { - this(storageType, allowedLocations, roleARN, externalId, region, null, null, false); + this( + storageType, + allowedLocations, + roleARN, + externalId, + region, + null, + null, + PATH_STYLE_ACCESS_DEFAULT); } @Override From 0f14287654fc541f7ae9ab10efdc971c9b5d3db0 Mon Sep 17 00:00:00 2001 From: Dmitri Bourlatchkov Date: Thu, 10 Jul 2025 18:49:53 -0400 Subject: [PATCH 4/6] fix config round trip, add tests --- .../polaris/core/entity/CatalogEntity.java | 3 + .../aws/AwsCredentialsStorageIntegration.java | 2 +- .../aws/AwsStorageConfigurationInfo.java | 39 ++++------- .../aws/AwsStorageConfigurationInfoTest.java | 2 +- .../quarkus/entity/CatalogEntityTest.java | 69 ++++++++++++++++++- 5 files changed, 86 insertions(+), 29 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java index d516f11d3b..1469f26c98 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java @@ -140,6 +140,9 @@ private StorageConfigInfo getStorageInfo(Map internalProperties) .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) .setAllowedLocations(awsConfig.getAllowedLocations()) .setRegion(awsConfig.getRegion()) + .setEndpoint(awsConfig.getEndpoint()) + .setStsEndpoint(awsConfig.getStsEndpoint()) + .setPathStyleAccess(awsConfig.getPathStyleAccess()) .build(); } if (configInfo instanceof AzureStorageConfigurationInfo) { diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java index a8c94bf300..338f60c4ed 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java @@ -122,7 +122,7 @@ public EnumMap getSubscopedCreds( credentialMap.put(StorageAccessProperty.AWS_ENDPOINT, endpointUri.toString()); } - if (storageConfig.getPathStyleAccess()) { + if (Boolean.TRUE.equals(storageConfig.getPathStyleAccess())) { credentialMap.put(StorageAccessProperty.AWS_PATH_STYLE_ACCESS, Boolean.TRUE.toString()); } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java index 445bbc54fb..a007c6a719 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java @@ -33,8 +33,6 @@ /** Aws Polaris Storage Configuration information */ public class AwsStorageConfigurationInfo extends PolarisStorageConfigurationInfo { - private static final boolean PATH_STYLE_ACCESS_DEFAULT = false; - // Technically, it should be ^arn:(aws|aws-cn|aws-us-gov):iam::(\d{12}):role/.+$, @JsonIgnore public static final String ROLE_ARN_PATTERN = "^arn:(aws|aws-us-gov):iam::(\\d{12}):role/.+$"; @@ -66,7 +64,7 @@ public class AwsStorageConfigurationInfo extends PolarisStorageConfigurationInfo /** A flag indicating whether path-style bucket access should be forced in S3 clients. */ @JsonProperty(value = "pathStyleAccess") - private boolean pathStyleAccess; + private Boolean pathStyleAccess; @JsonCreator public AwsStorageConfigurationInfo( @@ -85,7 +83,7 @@ public AwsStorageConfigurationInfo( this.region = region; this.endpoint = endpoint; this.stsEndpoint = stsEndpoint; - this.pathStyleAccess = pathStyleAccess == null ? PATH_STYLE_ACCESS_DEFAULT : pathStyleAccess; + this.pathStyleAccess = pathStyleAccess; } public AwsStorageConfigurationInfo( @@ -93,15 +91,7 @@ public AwsStorageConfigurationInfo( @Nonnull List allowedLocations, @Nonnull String roleARN, @Nullable String region) { - this( - storageType, - allowedLocations, - roleARN, - null, - region, - null, - null, - PATH_STYLE_ACCESS_DEFAULT); + this(storageType, allowedLocations, roleARN, null, region, null, null, null); } public AwsStorageConfigurationInfo( @@ -110,15 +100,7 @@ public AwsStorageConfigurationInfo( @Nonnull String roleARN, @Nullable String externalId, @Nullable String region) { - this( - storageType, - allowedLocations, - roleARN, - externalId, - region, - null, - null, - PATH_STYLE_ACCESS_DEFAULT); + this(storageType, allowedLocations, roleARN, externalId, region, null, null, null); } @Override @@ -167,6 +149,11 @@ public void setRegion(@Nullable String region) { this.region = region; } + @Nullable + public String getEndpoint() { + return endpoint; + } + @JsonIgnore @Nullable public URI getEndpointUri() { @@ -174,11 +161,15 @@ public URI getEndpointUri() { } /** Returns a flag indicating whether path-style bucket access should be forced in S3 clients. */ - @JsonIgnore - public boolean getPathStyleAccess() { + public @Nullable Boolean getPathStyleAccess() { return pathStyleAccess; } + @Nullable + public String getStsEndpoint() { + return stsEndpoint; + } + /** Returns the STS endpoint if set, defaulting to {@link #getEndpointUri()} otherwise. */ @JsonIgnore @Nullable diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java index b0fc989a2b..f33935b4ff 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java @@ -64,7 +64,7 @@ public void testStsEndpoint() { @Test public void testPathStyleAccess() { - assertThat(config(null, null, null).getPathStyleAccess()).isFalse(); + assertThat(config(null, null, null).getPathStyleAccess()).isNull(); assertThat(config(null, null, false).getPathStyleAccess()).isFalse(); assertThat(config(null, null, true).getPathStyleAccess()).isTrue(); } diff --git a/runtime/service/src/test/java/org/apache/polaris/service/quarkus/entity/CatalogEntityTest.java b/runtime/service/src/test/java/org/apache/polaris/service/quarkus/entity/CatalogEntityTest.java index 8fb591369e..4c769c1946 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/quarkus/entity/CatalogEntityTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/quarkus/entity/CatalogEntityTest.java @@ -18,7 +18,12 @@ */ package org.apache.polaris.service.quarkus.entity; +import static org.assertj.core.api.Assertions.assertThat; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; import java.util.List; +import java.util.stream.Stream; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.PolarisDefaultDiagServiceImpl; import org.apache.polaris.core.admin.model.AwsStorageConfigInfo; @@ -37,9 +42,12 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; public class CatalogEntityTest { + private static final ObjectMapper MAPPER = new ObjectMapper(); private CallContext callContext; @@ -286,7 +294,7 @@ public void testCatalogTypeDefaultsToInternal() { .build(); Catalog catalog = catalogEntity.asCatalog(); - Assertions.assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.INTERNAL); + assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.INTERNAL); } @Test @@ -309,7 +317,7 @@ public void testCatalogTypeExternalPreserved() { .build(); Catalog catalog = catalogEntity.asCatalog(); - Assertions.assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.EXTERNAL); + assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.EXTERNAL); } @Test @@ -332,6 +340,61 @@ public void testCatalogTypeInternalExplicitlySet() { .build(); Catalog catalog = catalogEntity.asCatalog(); - Assertions.assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.INTERNAL); + assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.INTERNAL); + } + + @Test + public void testAwsConfigJsonPropertiesPresence() throws JsonProcessingException { + AwsStorageConfigInfo.Builder b = + AwsStorageConfigInfo.builder() + .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) + .setRoleArn("arn:aws:iam::012345678901:role/test-role"); + assertThat(MAPPER.writeValueAsString(b.build())).contains("roleArn"); + assertThat(MAPPER.writeValueAsString(b.build())).doesNotContain("endpoint"); + assertThat(MAPPER.writeValueAsString(b.build())).doesNotContain("stsEndpoint"); + assertThat(MAPPER.writeValueAsString(b.build())).doesNotContain("pathStyleAccess"); + + b.setEndpoint("http://s3.example.com"); + b.setStsEndpoint("http://sts.example.com"); + b.setPathStyleAccess(false); + assertThat(MAPPER.writeValueAsString(b.build())).contains("roleArn"); + assertThat(MAPPER.writeValueAsString(b.build())).contains("endpoint"); + assertThat(MAPPER.writeValueAsString(b.build())).contains("stsEndpoint"); + assertThat(MAPPER.writeValueAsString(b.build())).contains("pathStyleAccess"); + } + + @ParameterizedTest + @MethodSource + public void testAwsConfigRoundTrip(AwsStorageConfigInfo config) throws JsonProcessingException { + String configStr = MAPPER.writeValueAsString(config); + CatalogEntity catalogEntity = + new CatalogEntity.Builder() + .setName("testAwsConfigRoundTrip") + .setDefaultBaseLocation(config.getAllowedLocations().getFirst()) + .setCatalogType(Catalog.TypeEnum.INTERNAL.name()) + .setStorageConfigurationInfo( + callContext, + MAPPER.readValue(configStr, StorageConfigInfo.class), + config.getAllowedLocations().getFirst()) + .build(); + + Catalog catalog = catalogEntity.asCatalog(); + assertThat(catalog.getStorageConfigInfo()).isEqualTo(config); + assertThat(MAPPER.writeValueAsString(catalog.getStorageConfigInfo())).isEqualTo(configStr); + } + + public static Stream testAwsConfigRoundTrip() { + AwsStorageConfigInfo.Builder b = + AwsStorageConfigInfo.builder() + .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) + .setAllowedLocations(List.of("s3://example.com")) + .setRoleArn("arn:aws:iam::012345678901:role/test-role"); + return Stream.of( + Arguments.of(b.build()), + Arguments.of(b.setExternalId("ex1").build()), + Arguments.of(b.setRegion("us-west-2").build()), + Arguments.of(b.setEndpoint("http://s3.example.com:1234").build()), + Arguments.of(b.setStsEndpoint("http://sts.example.com:1234").build()), + Arguments.of(b.setPathStyleAccess(true).build())); } } From 47fc30ed4a0f85333d0349d3cf764e6eab872c6d Mon Sep 17 00:00:00 2001 From: Dmitri Bourlatchkov Date: Tue, 15 Jul 2025 19:02:54 -0400 Subject: [PATCH 5/6] review: add Open API default for `pathStyleAccess` --- CHANGELOG.md | 2 ++ .../polaris/core/admin/model/CatalogSerializationTest.java | 1 + .../polaris/service/quarkus/entity/CatalogEntityTest.java | 1 - spec/polaris-management-service.yml | 3 ++- 4 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd1a5340dd..36d1f03cc5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,8 @@ at locations that better optimize for object storage. ### Changes +- Polaris Management API clients must be prepared to deal with new attributes in `AwsStorageConfigInfo` objects. + ### Deprecations * The property `polaris.active-roles-provider.type` is deprecated in favor of diff --git a/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java b/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java index 3620da4178..38076b1ec2 100644 --- a/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java +++ b/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java @@ -68,6 +68,7 @@ public void testJsonFormat() throws JsonProcessingException { + "\"properties\":{\"default-base-location\":\"s3://test/\"}," + "\"storageConfigInfo\":{" + "\"roleArn\":\"arn:aws:iam::123456789012:role/test-role\"," + + "\"pathStyleAccess\":false," + "\"storageType\":\"S3\"," + "\"allowedLocations\":[]" + "}}"); diff --git a/runtime/service/src/test/java/org/apache/polaris/service/quarkus/entity/CatalogEntityTest.java b/runtime/service/src/test/java/org/apache/polaris/service/quarkus/entity/CatalogEntityTest.java index 4c769c1946..1b5c8ed23d 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/quarkus/entity/CatalogEntityTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/quarkus/entity/CatalogEntityTest.java @@ -352,7 +352,6 @@ public void testAwsConfigJsonPropertiesPresence() throws JsonProcessingException assertThat(MAPPER.writeValueAsString(b.build())).contains("roleArn"); assertThat(MAPPER.writeValueAsString(b.build())).doesNotContain("endpoint"); assertThat(MAPPER.writeValueAsString(b.build())).doesNotContain("stsEndpoint"); - assertThat(MAPPER.writeValueAsString(b.build())).doesNotContain("pathStyleAccess"); b.setEndpoint("http://s3.example.com"); b.setStsEndpoint("http://sts.example.com"); diff --git a/spec/polaris-management-service.yml b/spec/polaris-management-service.yml index 2601500c8c..24cba360f5 100644 --- a/spec/polaris-management-service.yml +++ b/spec/polaris-management-service.yml @@ -1061,7 +1061,8 @@ components: description: >- Whether S3 requests to files in this catalog should use 'path-style addressing for buckets'. Default: false. - example: false + example: true + default: false required: - roleArn From 21fe61cf547ac72528a593fc1df0f1c298328566 Mon Sep 17 00:00:00 2001 From: Dmitri Bourlatchkov Date: Wed, 16 Jul 2025 10:01:34 -0400 Subject: [PATCH 6/6] review: remove default from doc comment for `pathStyleAccess` --- spec/polaris-management-service.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/polaris-management-service.yml b/spec/polaris-management-service.yml index 24cba360f5..5a2461ebda 100644 --- a/spec/polaris-management-service.yml +++ b/spec/polaris-management-service.yml @@ -1060,7 +1060,6 @@ components: type: boolean description: >- Whether S3 requests to files in this catalog should use 'path-style addressing for buckets'. - Default: false. example: true default: false required: