Skip to content

Commit 21efdf6

Browse files
eric-maynarderic-maynard
authored
Adjustable limit on the number of locations per storage config (#1068)
* initial commit * autolint * change the config store access * autolint * add support for 01 * autolint * fix test * autolint * retest * rebase * autolint * change the config store access * autolint * add support for 01 * autolint * fix test * autolint * retest * fix a test * autolint * fix another test * autolint * remove catalog config for now as it's not used * changes * autolint * update test to reflect -1 default * autolint * autolint * move the check * changes per review * ready * autolint
1 parent 9425a6e commit 21efdf6

File tree

9 files changed

+50
-35
lines changed

9 files changed

+50
-35
lines changed

‎polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,4 +43,13 @@ protected BehaviorChangeConfiguration(
4343
.description("If true, validate that view locations don't overlap when views are created")
4444
.defaultValue(true)
4545
.buildBehaviorChangeConfiguration();
46+
47+
public static final BehaviorChangeConfiguration<Integer> STORAGE_CONFIGURATION_MAX_LOCATIONS =
48+
PolarisConfiguration.<Integer>builder()
49+
.key("STORAGE_CONFIGURATION_MAX_LOCATIONS")
50+
.description(
51+
"How many locations can be associated with a storage configuration, or -1 for"
52+
+ " unlimited locations")
53+
.defaultValue(-1)
54+
.buildBehaviorChangeConfiguration();
4655
}

‎polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import jakarta.annotation.Nonnull;
2424
import jakarta.annotation.Nullable;
2525
import java.util.ArrayList;
26+
import java.util.Collection;
2627
import java.util.HashMap;
2728
import java.util.HashSet;
2829
import java.util.Map;
@@ -39,6 +40,7 @@
3940
import org.apache.polaris.core.admin.model.GcpStorageConfigInfo;
4041
import org.apache.polaris.core.admin.model.PolarisCatalog;
4142
import org.apache.polaris.core.admin.model.StorageConfigInfo;
43+
import org.apache.polaris.core.config.BehaviorChangeConfiguration;
4244
import org.apache.polaris.core.storage.FileStorageConfigurationInfo;
4345
import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo;
4446
import org.apache.polaris.core.storage.aws.AwsStorageConfigurationInfo;
@@ -237,6 +239,7 @@ public Builder setStorageConfigurationInfo(
237239
throw new BadRequestException("Must specify default base location");
238240
}
239241
allowedLocations.add(defaultBaseLocation);
242+
validateMaxAllowedLocations(allowedLocations);
240243
switch (storageConfigModel.getStorageType()) {
241244
case S3:
242245
AwsStorageConfigInfo awsConfigModel = (AwsStorageConfigInfo) storageConfigModel;
@@ -279,6 +282,19 @@ public Builder setStorageConfigurationInfo(
279282
return this;
280283
}
281284

285+
/** Validate the number of allowed locations not exceeding the max value. */
286+
private void validateMaxAllowedLocations(Collection<String> allowedLocations) {
287+
int maxAllowedLocations =
288+
BehaviorChangeConfiguration.loadConfig(
289+
BehaviorChangeConfiguration.STORAGE_CONFIGURATION_MAX_LOCATIONS);
290+
if (maxAllowedLocations != -1 && allowedLocations.size() > maxAllowedLocations) {
291+
throw new IllegalArgumentException(
292+
String.format(
293+
"Number of configured locations (%s) exceeds the limit of %s",
294+
allowedLocations.size(), maxAllowedLocations));
295+
}
296+
}
297+
282298
@Override
283299
public CatalogEntity build() {
284300
return new CatalogEntity(buildBase());

‎polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -230,14 +230,6 @@ protected void validatePrefixForStorageType(String loc) {
230230
}
231231
}
232232

233-
/** Validate the number of allowed locations not exceeding the max value. */
234-
public void validateMaxAllowedLocations(int maxAllowedLocations) {
235-
if (allowedLocations.size() > maxAllowedLocations) {
236-
throw new IllegalArgumentException(
237-
"Number of allowed locations exceeds " + maxAllowedLocations);
238-
}
239-
}
240-
241233
/** Polaris' storage type, each has a fixed prefix for its location */
242234
public enum StorageType {
243235
S3("s3://"),

‎polaris-core/src/main/java/org/apache/polaris/core/storage/StorageConfigurationOverride.java‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,4 @@ public String getFileIoImplClassName() {
4949
protected void validatePrefixForStorageType(String loc) {
5050
parentStorageConfiguration.validatePrefixForStorageType(loc);
5151
}
52-
53-
@Override
54-
public void validateMaxAllowedLocations(int maxAllowedLocations) {
55-
parentStorageConfiguration.validateMaxAllowedLocations(maxAllowedLocations);
56-
}
5752
}

‎polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,6 @@
3232
/** Aws Polaris Storage Configuration information */
3333
public class AwsStorageConfigurationInfo extends PolarisStorageConfigurationInfo {
3434

35-
// 5 is the approximate max allowed locations for the size of AccessPolicy when LIST is required
36-
// for allowed read and write locations for subscoping creds.
37-
@JsonIgnore private static final int MAX_ALLOWED_LOCATIONS = 5;
38-
3935
// Technically, it should be ^arn:(aws|aws-cn|aws-us-gov):iam::(\d{12}):role/.+$,
4036
@JsonIgnore
4137
public static final String ROLE_ARN_PATTERN = "^arn:(aws|aws-us-gov):iam::(\\d{12}):role/.+$";
@@ -75,7 +71,6 @@ public AwsStorageConfigurationInfo(
7571
this.roleARN = roleARN;
7672
this.externalId = externalId;
7773
this.region = region;
78-
validateMaxAllowedLocations(MAX_ALLOWED_LOCATIONS);
7974
}
8075

8176
@Override

‎polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureStorageConfigurationInfo.java‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
package org.apache.polaris.core.storage.azure;
2020

2121
import com.fasterxml.jackson.annotation.JsonCreator;
22-
import com.fasterxml.jackson.annotation.JsonIgnore;
2322
import com.fasterxml.jackson.annotation.JsonProperty;
2423
import com.google.common.base.MoreObjects;
2524
import jakarta.annotation.Nonnull;
@@ -30,9 +29,6 @@
3029

3130
/** Azure storage configuration information. */
3231
public class AzureStorageConfigurationInfo extends PolarisStorageConfigurationInfo {
33-
// technically there is no limitation since expectation for Azure locations are for the same
34-
// storage account and same container
35-
@JsonIgnore private static final int MAX_ALLOWED_LOCATIONS = 20;
3632

3733
// Azure tenant id
3834
private final @Nonnull String tenantId;
@@ -52,7 +48,6 @@ public AzureStorageConfigurationInfo(
5248
@JsonProperty(value = "tenantId", required = true) @Nonnull String tenantId) {
5349
super(StorageType.AZURE, allowedLocations);
5450
this.tenantId = tenantId;
55-
validateMaxAllowedLocations(MAX_ALLOWED_LOCATIONS);
5651
}
5752

5853
@Override

‎polaris-core/src/main/java/org/apache/polaris/core/storage/gcp/GcpStorageConfigurationInfo.java‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
package org.apache.polaris.core.storage.gcp;
2020

2121
import com.fasterxml.jackson.annotation.JsonCreator;
22-
import com.fasterxml.jackson.annotation.JsonIgnore;
2322
import com.fasterxml.jackson.annotation.JsonProperty;
2423
import com.google.common.base.MoreObjects;
2524
import jakarta.annotation.Nonnull;
@@ -30,11 +29,6 @@
3029
/** Gcp storage storage configuration information. */
3130
public class GcpStorageConfigurationInfo extends PolarisStorageConfigurationInfo {
3231

33-
// 8 is an experimental result from generating GCP accessBoundaryRules when subscoping creds,
34-
// when the rule is too large, GCS only returns error: 400 bad request "Invalid arguments
35-
// provided in the request"
36-
@JsonIgnore private static final int MAX_ALLOWED_LOCATIONS = 8;
37-
3832
/** The gcp service account */
3933
@JsonProperty(value = "gcpServiceAccount", required = false)
4034
private @Nullable String gcpServiceAccount = null;
@@ -44,7 +38,6 @@ public GcpStorageConfigurationInfo(
4438
@JsonProperty(value = "allowedLocations", required = true) @Nonnull
4539
List<String> allowedLocations) {
4640
super(StorageType.GCS, allowedLocations);
47-
validateMaxAllowedLocations(MAX_ALLOWED_LOCATIONS);
4841
}
4942

5043
@Override

‎polaris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import org.apache.polaris.core.config.PolarisConfigurationStore;
2929
import org.junit.jupiter.api.Assertions;
3030
import org.junit.jupiter.api.Test;
31+
import org.mockito.Mockito;
3132

3233
/** Unit test for the default behaviors of the PolarisConfigurationStore interface. */
3334
public class PolarisConfigurationStoreTest {
@@ -64,8 +65,9 @@ public void testConfigsCanBeCastedFromString() {
6465

6566
// Ensure that we can fetch all the configs and that the value is what we expect, which
6667
// is the config's default value based on how we've implemented PolarisConfigurationStore above.
68+
PolarisCallContext polarisCallContext = Mockito.mock(PolarisCallContext.class);
6769
for (PolarisConfiguration<?> c : configs) {
68-
Assertions.assertEquals(c.defaultValue, store.getConfiguration(null, c));
70+
Assertions.assertEquals(c.defaultValue, store.getConfiguration(polarisCallContext, c));
6971
}
7072
}
7173

@@ -84,8 +86,10 @@ public <T> T getConfiguration(PolarisCallContext ctx, String configName) {
8486
}
8587
};
8688

89+
PolarisCallContext polarisCallContext = Mockito.mock(PolarisCallContext.class);
8790
for (PolarisConfiguration<?> c : configs) {
88-
Assertions.assertThrows(NumberFormatException.class, () -> store.getConfiguration(null, c));
91+
Assertions.assertThrows(
92+
NumberFormatException.class, () -> store.getConfiguration(polarisCallContext, c));
8993
}
9094
}
9195

‎quarkus/service/src/test/java/org/apache/polaris/service/quarkus/entity/CatalogEntityTest.java‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,21 +19,38 @@
1919
package org.apache.polaris.service.quarkus.entity;
2020

2121
import java.util.List;
22+
import org.apache.polaris.core.PolarisCallContext;
23+
import org.apache.polaris.core.PolarisDefaultDiagServiceImpl;
2224
import org.apache.polaris.core.admin.model.AwsStorageConfigInfo;
2325
import org.apache.polaris.core.admin.model.AzureStorageConfigInfo;
2426
import org.apache.polaris.core.admin.model.Catalog;
2527
import org.apache.polaris.core.admin.model.CatalogProperties;
2628
import org.apache.polaris.core.admin.model.GcpStorageConfigInfo;
2729
import org.apache.polaris.core.admin.model.PolarisCatalog;
2830
import org.apache.polaris.core.admin.model.StorageConfigInfo;
31+
import org.apache.polaris.core.context.CallContext;
2932
import org.apache.polaris.core.entity.CatalogEntity;
33+
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
34+
import org.apache.polaris.service.persistence.InMemoryPolarisMetaStoreManagerFactory;
3035
import org.assertj.core.api.Assertions;
36+
import org.junit.jupiter.api.BeforeAll;
3137
import org.junit.jupiter.api.Test;
3238
import org.junit.jupiter.params.ParameterizedTest;
3339
import org.junit.jupiter.params.provider.ValueSource;
3440

3541
public class CatalogEntityTest {
3642

43+
@BeforeAll
44+
public static void setup() {
45+
MetaStoreManagerFactory metaStoreManagerFactory = new InMemoryPolarisMetaStoreManagerFactory();
46+
PolarisCallContext polarisCallContext =
47+
new PolarisCallContext(
48+
metaStoreManagerFactory.getOrCreateSessionSupplier(() -> "realm").get(),
49+
new PolarisDefaultDiagServiceImpl());
50+
CallContext callContext = CallContext.of(() -> "realm", polarisCallContext);
51+
CallContext.setCurrentContext(callContext);
52+
}
53+
3754
@Test
3855
public void testInvalidAllowedLocationPrefix() {
3956
String storageLocation = "unsupportPrefix://mybucket/path";
@@ -123,9 +140,8 @@ public void testExceedMaxAllowedLocations() {
123140
.setProperties(prop)
124141
.setStorageConfigInfo(awsStorageConfigModel)
125142
.build();
126-
Assertions.assertThatThrownBy(() -> CatalogEntity.fromCatalog(awsCatalog))
127-
.isInstanceOf(IllegalArgumentException.class)
128-
.hasMessageContaining("Number of allowed locations exceeds 5");
143+
Assertions.assertThatCode(() -> CatalogEntity.fromCatalog(awsCatalog))
144+
.doesNotThrowAnyException();
129145
}
130146

131147
@Test

0 commit comments

Comments
 (0)