From add2fa9de7214b85851c1769d6856cfc544b8a19 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Sat, 10 May 2025 00:35:57 -0700 Subject: [PATCH 1/4] rewrite --- .../PolarisPolicyServiceIntegrationTest.java | 8 +- .../config/BehaviorChangeConfiguration.java | 8 +- .../core/config/FeatureConfiguration.java | 32 +++++-- .../core/config/PolarisConfiguration.java | 91 ++++++++++++++++++- .../config/PolarisConfigurationStore.java | 16 +++- .../PolarisConfigurationStoreTest.java | 4 +- 6 files changed, 134 insertions(+), 25 deletions(-) diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java index 6166d5d65c..c99ab993e1 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java @@ -104,8 +104,8 @@ public class PolarisPolicyServiceIntegrationTest { s3BucketBase + "/" + System.getenv("USER") + "/path/to/data"; private static final String[] DEFAULT_CATALOG_PROPERTIES = { - "allow.unstructured.table.location", "true", - "allow.external.table.location", "true" + "polaris.config.allow.unstructured.table.location", "true", + "polaris.config.allow.external.table.location", "true" }; @Retention(RetentionPolicy.RUNTIME) @@ -113,8 +113,8 @@ public class PolarisPolicyServiceIntegrationTest { Catalog.TypeEnum value() default Catalog.TypeEnum.INTERNAL; String[] properties() default { - "allow.unstructured.table.location", "true", - "allow.external.table.location", "true" + "polaris.config.allow.unstructured.table.location", "true", + "polaris.config.allow.external.table.location", "true" }; } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java index 7ab8a5fb66..db5176e7c3 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java @@ -33,8 +33,12 @@ public class BehaviorChangeConfiguration extends PolarisConfiguration { protected BehaviorChangeConfiguration( - String key, String description, T defaultValue, Optional catalogConfig) { - super(key, description, defaultValue, catalogConfig); + String key, + String description, + T defaultValue, + Optional catalogConfig, + Optional catalogConfigUnsafe) { + super(key, description, defaultValue, catalogConfig, catalogConfigUnsafe); } public static final BehaviorChangeConfiguration VALIDATE_VIEW_LOCATION_OVERLAP = diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java index 26437a8f64..926c99fd93 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java @@ -33,8 +33,12 @@ */ public class FeatureConfiguration extends PolarisConfiguration { protected FeatureConfiguration( - String key, String description, T defaultValue, Optional catalogConfig) { - super(key, description, defaultValue, catalogConfig); + String key, + String description, + T defaultValue, + Optional catalogConfig, + Optional catalogConfigUnsafe) { + super(key, description, defaultValue, catalogConfig, catalogConfigUnsafe); } /** @@ -81,7 +85,8 @@ public static void enforceFeatureEnabledOrThrow( public static final FeatureConfiguration ALLOW_TABLE_LOCATION_OVERLAP = PolarisConfiguration.builder() .key("ALLOW_TABLE_LOCATION_OVERLAP") - .catalogConfig("allow.overlapping.table.location") + .catalogConfig("polaris.config.allow.overlapping.table.location") + .catalogConfigUnsafe("allow.overlapping.table.location") .description( "If set to true, allow one table's location to reside within another table's location. " + "This is only enforced within a given namespace.") @@ -115,7 +120,8 @@ public static void enforceFeatureEnabledOrThrow( public static final FeatureConfiguration ALLOW_UNSTRUCTURED_TABLE_LOCATION = PolarisConfiguration.builder() .key("ALLOW_UNSTRUCTURED_TABLE_LOCATION") - .catalogConfig("allow.unstructured.table.location") + .catalogConfig("polaris.config.allow.unstructured.table.location") + .catalogConfigUnsafe("allow.unstructured.table.location") .description("If set to true, allows unstructured table locations.") .defaultValue(false) .buildFeatureConfiguration(); @@ -123,7 +129,8 @@ public static void enforceFeatureEnabledOrThrow( public static final FeatureConfiguration ALLOW_EXTERNAL_TABLE_LOCATION = PolarisConfiguration.builder() .key("ALLOW_EXTERNAL_TABLE_LOCATION") - .catalogConfig("allow.external.table.location") + .catalogConfig("polaris.config.allow.external.table.location") + .catalogConfigUnsafe("allow.external.table.location") .description( "If set to true, allows tables to have external locations outside the default structure.") .defaultValue(false) @@ -132,7 +139,8 @@ public static void enforceFeatureEnabledOrThrow( public static final FeatureConfiguration ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING = PolarisConfiguration.builder() .key("ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING") - .catalogConfig("enable.credential.vending") + .catalogConfig("polaris.config.enable.credential.vending") + .catalogConfigUnsafe("enable.credential.vending") .description("If set to true, allow credential vending for external catalogs.") .defaultValue(true) .buildFeatureConfiguration(); @@ -140,7 +148,8 @@ public static void enforceFeatureEnabledOrThrow( public static final FeatureConfiguration> SUPPORTED_CATALOG_STORAGE_TYPES = PolarisConfiguration.>builder() .key("SUPPORTED_CATALOG_STORAGE_TYPES") - .catalogConfig("supported.storage.types") + .catalogConfig("polaris.config.supported.storage.types") + .catalogConfigUnsafe("supported.storage.types") .description("The list of supported storage types for a catalog") .defaultValue( List.of( @@ -153,7 +162,8 @@ public static void enforceFeatureEnabledOrThrow( public static final FeatureConfiguration CLEANUP_ON_NAMESPACE_DROP = PolarisConfiguration.builder() .key("CLEANUP_ON_NAMESPACE_DROP") - .catalogConfig("cleanup.on.namespace.drop") + .catalogConfig("polaris.config.cleanup.on.namespace.drop") + .catalogConfigUnsafe("cleanup.on.namespace.drop") .description("If set to true, clean up data when a namespace is dropped") .defaultValue(false) .buildFeatureConfiguration(); @@ -161,7 +171,8 @@ public static void enforceFeatureEnabledOrThrow( public static final FeatureConfiguration CLEANUP_ON_CATALOG_DROP = PolarisConfiguration.builder() .key("CLEANUP_ON_CATALOG_DROP") - .catalogConfig("cleanup.on.catalog.drop") + .catalogConfig("polaris.config.cleanup.on.catalog.drop") + .catalogConfigUnsafe("cleanup.on.catalog.drop") .description("If set to true, clean up data when a catalog is dropped") .defaultValue(false) .buildFeatureConfiguration(); @@ -169,7 +180,8 @@ public static void enforceFeatureEnabledOrThrow( public static final FeatureConfiguration DROP_WITH_PURGE_ENABLED = PolarisConfiguration.builder() .key("DROP_WITH_PURGE_ENABLED") - .catalogConfig("drop-with-purge.enabled") + .catalogConfig("polaris.config.drop-with-purge.enabled") + .catalogConfigUnsafe("drop-with-purge.enabled") .description( "If set to true, allows tables to be dropped with the purge parameter set to true.") .defaultValue(true) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java index bcb3809881..41169d5f47 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java @@ -20,13 +20,20 @@ import java.util.ArrayList; import java.util.List; +import java.util.Map; +import java.util.Objects; import java.util.Optional; +import java.util.function.Function; +import java.util.stream.Collectors; +import java.util.stream.Stream; + import org.apache.polaris.core.context.CallContext; import org.slf4j.Logger; import org.slf4j.LoggerFactory; /** * An ABC for Polaris configurations that alter the service's behavior + * TODO: deprecate unsafe catalog configs and remove related code * * @param The type of the configuration */ @@ -34,19 +41,60 @@ public abstract class PolarisConfiguration { private static final Logger LOGGER = LoggerFactory.getLogger(PolarisConfiguration.class); + public static final List> allConfigurations = new ArrayList<>(); + public final String key; public final String description; public final T defaultValue; private final Optional catalogConfigImpl; + private final Optional catalogConfigUnsafeImpl; private final Class typ; + /** catalog configs are expected to start with this prefix */ + private static final String SAFE_CATALOG_CONFIG_PREFIX = "polaris.config."; + + /** + * Helper method for building `allConfigurations` and checking for duplicate use of keys across + * configs. + */ + private static void registerConfiguration(PolarisConfiguration configuration) { + for (PolarisConfiguration existingConfiguration : allConfigurations) { + if (existingConfiguration.key.equals(configuration.key)) { + throw new IllegalArgumentException( + String.format("Config '%s' is already in use", configuration.key)); + } else { + var configs = Stream.of( + configuration.catalogConfigImpl, + configuration.catalogConfigUnsafeImpl, + existingConfiguration.catalogConfigImpl, + existingConfiguration.catalogConfigUnsafeImpl + ) + .flatMap(Optional::stream) + .collect(Collectors.groupingBy(Function.identity(), Collectors.counting())); + for (var entry: configs.entrySet()) { + if (entry.getValue() > 1) { + throw new IllegalArgumentException( + String.format("Catalog config %s is already in use", entry.getKey())); + } + } + + } + } + allConfigurations.add(configuration); + } + @SuppressWarnings("unchecked") protected PolarisConfiguration( - String key, String description, T defaultValue, Optional catalogConfig) { + String key, + String description, + T defaultValue, + Optional catalogConfig, + Optional catalogConfigUnsafe) { this.key = key; this.description = description; this.defaultValue = defaultValue; this.catalogConfigImpl = catalogConfig; + this.catalogConfigUnsafeImpl = catalogConfigUnsafe; this.typ = (Class) defaultValue.getClass(); } @@ -61,6 +109,17 @@ public String catalogConfig() { "Attempted to read a catalog config key from a configuration that doesn't have one.")); } + public boolean hasCatalogConfigUnsafe() { + return catalogConfigUnsafeImpl.isPresent(); + } + + public String catalogConfigUnsafe() { + return catalogConfigUnsafeImpl.orElseThrow( + () -> + new IllegalStateException( + "Attempted to read an unsafe catalog config key from a configuration that doesn't have one.")); + } + T cast(Object value) { return this.typ.cast(value); } @@ -70,6 +129,7 @@ public static class Builder { private String description; private T defaultValue; private Optional catalogConfig = Optional.empty(); + private Optional catalogConfigUnsafe = Optional.empty(); public Builder key(String key) { this.key = key; @@ -93,26 +153,47 @@ public Builder defaultValue(T defaultValue) { } public Builder catalogConfig(String catalogConfig) { + if (!catalogConfig.startsWith(SAFE_CATALOG_CONFIG_PREFIX)) { + throw new IllegalArgumentException( + "Catalog configs are expected to start with " + SAFE_CATALOG_CONFIG_PREFIX); + } this.catalogConfig = Optional.of(catalogConfig); return this; } + /** + * Used to support backwards compatability before there were reserved properties. Usage of this + * method should be removed over time. + */ + @Deprecated + public Builder catalogConfigUnsafe(String catalogConfig) { + LOGGER.info("catalogConfigUnsafe is deprecated! Use catalogConfig() instead."); + this.catalogConfigUnsafe = Optional.of(catalogConfig); + return this; + } + public FeatureConfiguration buildFeatureConfiguration() { if (key == null || description == null || defaultValue == null) { throw new IllegalArgumentException("key, description, and defaultValue are required"); } - return new FeatureConfiguration<>(key, description, defaultValue, catalogConfig); + FeatureConfiguration config = + new FeatureConfiguration<>(key, description, defaultValue, catalogConfig, catalogConfigUnsafe); + PolarisConfiguration.registerConfiguration(config); + return config; } public BehaviorChangeConfiguration buildBehaviorChangeConfiguration() { if (key == null || description == null || defaultValue == null) { throw new IllegalArgumentException("key, description, and defaultValue are required"); } - if (catalogConfig.isPresent()) { + if (catalogConfig.isPresent() || catalogConfigUnsafe.isPresent()) { throw new IllegalArgumentException( - "catalogConfig is not valid for behavior change configs"); + "catalog configs are not valid for behavior change configs"); } - return new BehaviorChangeConfiguration<>(key, description, defaultValue, catalogConfig); + BehaviorChangeConfiguration config = + new BehaviorChangeConfiguration<>(key, description, defaultValue, catalogConfig, catalogConfigUnsafe); + PolarisConfiguration.registerConfiguration(config); + return config; } } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java index 65e6133515..8c3116f9ea 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java @@ -23,14 +23,20 @@ import jakarta.annotation.Nullable; import java.util.ArrayList; import java.util.List; +import java.util.Map; + import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.entity.CatalogEntity; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Dynamic configuration store used to retrieve runtime parameters, which may vary by realm or by * request. */ public interface PolarisConfigurationStore { + Logger LOGGER = LoggerFactory.getLogger(PolarisConfigurationStore.class); + /** * Retrieve the current value for a configuration key. May be null if not set. * @@ -111,11 +117,17 @@ public interface PolarisConfigurationStore { PolarisCallContext ctx, @Nonnull CatalogEntity catalogEntity, PolarisConfiguration config) { - if (config.hasCatalogConfig() - && catalogEntity.getPropertiesAsMap().containsKey(config.catalogConfig())) { + if (config.hasCatalogConfig()) { return tryCast(config, catalogEntity.getPropertiesAsMap().get(config.catalogConfig())); + } else if (config.hasCatalogConfigUnsafe()) { + LOGGER.warn( + String.format( + "Deprecated config %s is in use and will be removed in a future version", + config.catalogConfigUnsafe())); + return tryCast(config, catalogEntity.getPropertiesAsMap().get(config.catalogConfigUnsafe())); } else { return getConfiguration(ctx, config); } } + } diff --git a/polaris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java b/polaris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java index 5a1cfed303..9994b3833f 100644 --- a/polaris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java @@ -75,7 +75,7 @@ public void testConfigsCanBeCastedFromString() { public void testInvalidCastThrowsException() { // Bool not included because Boolean.valueOf turns non-boolean strings to false List> configs = - List.of(buildConfig("int", 12), buildConfig("long", 34L), buildConfig("double", 5.6D)); + List.of(buildConfig("int2", 12), buildConfig("long2", 34L), buildConfig("double2", 5.6D)); PolarisConfigurationStore store = new PolarisConfigurationStore() { @@ -135,7 +135,7 @@ public void testBehaviorAndFeatureConfigs() { BehaviorChangeConfiguration behaviorChangeConfig = PolarisConfiguration.builder() - .key("example") + .key("example2") .description("example") .defaultValue(true) .buildBehaviorChangeConfiguration(); From ccd5a86ab83520650eb5a6d07664419cc9bb2226 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Sat, 10 May 2025 00:50:23 -0700 Subject: [PATCH 2/4] rewrite --- .../core/config/PolarisConfiguration.java | 36 ++++++++++--------- .../config/PolarisConfigurationStore.java | 3 -- .../config/QuarkusReservedProperties.java | 26 ++++++++++++++ .../service/config/ReservedProperties.java | 32 ++++++++++++----- 4 files changed, 68 insertions(+), 29 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java index 41169d5f47..e45b819c53 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java @@ -20,20 +20,17 @@ import java.util.ArrayList; import java.util.List; -import java.util.Map; -import java.util.Objects; import java.util.Optional; import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; - import org.apache.polaris.core.context.CallContext; import org.slf4j.Logger; import org.slf4j.LoggerFactory; /** - * An ABC for Polaris configurations that alter the service's behavior - * TODO: deprecate unsafe catalog configs and remove related code + * An ABC for Polaris configurations that alter the service's behavior TODO: deprecate unsafe + * catalog configs and remove related code * * @param The type of the configuration */ @@ -63,21 +60,20 @@ private static void registerConfiguration(PolarisConfiguration configuration) throw new IllegalArgumentException( String.format("Config '%s' is already in use", configuration.key)); } else { - var configs = Stream.of( - configuration.catalogConfigImpl, - configuration.catalogConfigUnsafeImpl, - existingConfiguration.catalogConfigImpl, - existingConfiguration.catalogConfigUnsafeImpl - ) - .flatMap(Optional::stream) - .collect(Collectors.groupingBy(Function.identity(), Collectors.counting())); - for (var entry: configs.entrySet()) { + var configs = + Stream.of( + configuration.catalogConfigImpl, + configuration.catalogConfigUnsafeImpl, + existingConfiguration.catalogConfigImpl, + existingConfiguration.catalogConfigUnsafeImpl) + .flatMap(Optional::stream) + .collect(Collectors.groupingBy(Function.identity(), Collectors.counting())); + for (var entry : configs.entrySet()) { if (entry.getValue() > 1) { throw new IllegalArgumentException( String.format("Catalog config %s is already in use", entry.getKey())); } } - } } allConfigurations.add(configuration); @@ -168,6 +164,10 @@ public Builder catalogConfig(String catalogConfig) { @Deprecated public Builder catalogConfigUnsafe(String catalogConfig) { LOGGER.info("catalogConfigUnsafe is deprecated! Use catalogConfig() instead."); + if (catalogConfig.startsWith(SAFE_CATALOG_CONFIG_PREFIX)) { + throw new IllegalArgumentException( + "Unsafe catalog configs are not expected to start with " + SAFE_CATALOG_CONFIG_PREFIX); + } this.catalogConfigUnsafe = Optional.of(catalogConfig); return this; } @@ -177,7 +177,8 @@ public FeatureConfiguration buildFeatureConfiguration() { throw new IllegalArgumentException("key, description, and defaultValue are required"); } FeatureConfiguration config = - new FeatureConfiguration<>(key, description, defaultValue, catalogConfig, catalogConfigUnsafe); + new FeatureConfiguration<>( + key, description, defaultValue, catalogConfig, catalogConfigUnsafe); PolarisConfiguration.registerConfiguration(config); return config; } @@ -191,7 +192,8 @@ public BehaviorChangeConfiguration buildBehaviorChangeConfiguration() { "catalog configs are not valid for behavior change configs"); } BehaviorChangeConfiguration config = - new BehaviorChangeConfiguration<>(key, description, defaultValue, catalogConfig, catalogConfigUnsafe); + new BehaviorChangeConfiguration<>( + key, description, defaultValue, catalogConfig, catalogConfigUnsafe); PolarisConfiguration.registerConfiguration(config); return config; } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java index 8c3116f9ea..0f38c067ca 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java @@ -23,8 +23,6 @@ import jakarta.annotation.Nullable; import java.util.ArrayList; import java.util.List; -import java.util.Map; - import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.entity.CatalogEntity; import org.slf4j.Logger; @@ -129,5 +127,4 @@ public interface PolarisConfigurationStore { return getConfiguration(ctx, config); } } - } diff --git a/quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusReservedProperties.java b/quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusReservedProperties.java index 4d45a456e0..41953b6ed8 100644 --- a/quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusReservedProperties.java +++ b/quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusReservedProperties.java @@ -19,7 +19,10 @@ package org.apache.polaris.service.quarkus.config; import io.smallrye.config.ConfigMapping; +import java.util.HashSet; import java.util.List; +import java.util.Set; +import org.apache.polaris.core.config.PolarisConfiguration; import org.apache.polaris.service.config.ReservedProperties; @ConfigMapping(prefix = "polaris.reserved-properties") @@ -28,4 +31,27 @@ public interface QuarkusReservedProperties extends ReservedProperties { default List prefixes() { return List.of("polaris."); } + + @Override + default Set allowlist() { + return AllowlistHolder.INSTANCE; + } + + class AllowlistHolder { + static final Set INSTANCE = computeAllowlist(); + + private static Set computeAllowlist() { + Set allowlist = new HashSet<>(); + PolarisConfiguration.allConfigurations.forEach( + c -> { + if (c.hasCatalogConfig()) { + allowlist.add(c.catalogConfig()); + } + if (c.hasCatalogConfigUnsafe()) { + allowlist.add(c.catalogConfigUnsafe()); + } + }); + return allowlist; + } + } } diff --git a/service/common/src/main/java/org/apache/polaris/service/config/ReservedProperties.java b/service/common/src/main/java/org/apache/polaris/service/config/ReservedProperties.java index 3c4b108d08..ca29845a68 100644 --- a/service/common/src/main/java/org/apache/polaris/service/config/ReservedProperties.java +++ b/service/common/src/main/java/org/apache/polaris/service/config/ReservedProperties.java @@ -22,6 +22,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.stream.Collectors; import org.apache.iceberg.MetadataUpdate; import org.slf4j.Logger; @@ -43,6 +44,11 @@ public interface ReservedProperties { public List prefixes() { return List.of(); } + + @Override + public Set allowlist() { + return Set.of(); + } }; /** @@ -51,6 +57,12 @@ public List prefixes() { */ List prefixes(); + /** + * A list of properties that are *not* considered reserved, even if they start with a reserved + * prefix + */ + Set allowlist(); + /** If true, attempts to modify a reserved property should throw an exception. */ default boolean shouldThrow() { return true; @@ -94,15 +106,17 @@ default Map removeReservedProperties(Map propert List prefixes = prefixes(); for (var entry : properties.entrySet()) { boolean isReserved = false; - for (String prefix : prefixes) { - if (entry.getKey().startsWith(prefix)) { - isReserved = true; - String message = - String.format("Property '%s' matches reserved prefix '%s'", entry.getKey(), prefix); - if (shouldThrow()) { - throw new IllegalArgumentException(message); - } else { - LOGGER.debug(message); + if (!allowlist().contains(entry.getKey())) { + for (String prefix : prefixes) { + if (entry.getKey().startsWith(prefix)) { + isReserved = true; + String message = + String.format("Property '%s' matches reserved prefix '%s'", entry.getKey(), prefix); + if (shouldThrow()) { + throw new IllegalArgumentException(message); + } else { + LOGGER.debug(message); + } } } } From ca2533bb2b6461283986aff146167141619ebf9b Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Sat, 10 May 2025 00:59:25 -0700 Subject: [PATCH 3/4] stable --- .../PolarisPolicyServiceIntegrationTest.java | 8 +++--- .../config/PolarisConfigurationStore.java | 27 ++++++++++++------- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java index c99ab993e1..6166d5d65c 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java @@ -104,8 +104,8 @@ public class PolarisPolicyServiceIntegrationTest { s3BucketBase + "/" + System.getenv("USER") + "/path/to/data"; private static final String[] DEFAULT_CATALOG_PROPERTIES = { - "polaris.config.allow.unstructured.table.location", "true", - "polaris.config.allow.external.table.location", "true" + "allow.unstructured.table.location", "true", + "allow.external.table.location", "true" }; @Retention(RetentionPolicy.RUNTIME) @@ -113,8 +113,8 @@ public class PolarisPolicyServiceIntegrationTest { Catalog.TypeEnum value() default Catalog.TypeEnum.INTERNAL; String[] properties() default { - "polaris.config.allow.unstructured.table.location", "true", - "polaris.config.allow.external.table.location", "true" + "allow.unstructured.table.location", "true", + "allow.external.table.location", "true" }; } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java index 0f38c067ca..acb171ce30 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java @@ -23,6 +23,7 @@ import jakarta.annotation.Nullable; import java.util.ArrayList; import java.util.List; +import java.util.Map; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.entity.CatalogEntity; import org.slf4j.Logger; @@ -115,16 +116,22 @@ public interface PolarisConfigurationStore { PolarisCallContext ctx, @Nonnull CatalogEntity catalogEntity, PolarisConfiguration config) { - if (config.hasCatalogConfig()) { - return tryCast(config, catalogEntity.getPropertiesAsMap().get(config.catalogConfig())); - } else if (config.hasCatalogConfigUnsafe()) { - LOGGER.warn( - String.format( - "Deprecated config %s is in use and will be removed in a future version", - config.catalogConfigUnsafe())); - return tryCast(config, catalogEntity.getPropertiesAsMap().get(config.catalogConfigUnsafe())); - } else { - return getConfiguration(ctx, config); + if (config.hasCatalogConfig() || config.hasCatalogConfigUnsafe()) { + Map propertiesMap = catalogEntity.getPropertiesAsMap(); + String propertyValue = propertiesMap.get(config.catalogConfig()); + if (propertyValue == null) { + propertyValue = propertiesMap.get(config.catalogConfigUnsafe()); + if (propertyValue != null) { + LOGGER.warn( + String.format( + "Deprecated config %s is in use and will be removed in a future version", + config.catalogConfigUnsafe())); + } + } + if (propertyValue != null) { + return tryCast(config, propertyValue); + } } + return getConfiguration(ctx, config); } } From 5c96c7f8fbd387d431f7a58c15cdccdb69aa8940 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Tue, 13 May 2025 00:00:45 -0700 Subject: [PATCH 4/4] changes per comments --- .../core/config/PolarisConfiguration.java | 7 ++++++- .../config/QuarkusReservedProperties.java | 19 ++++++++++--------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java index e45b819c53..c99ff780ee 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java @@ -38,7 +38,7 @@ public abstract class PolarisConfiguration { private static final Logger LOGGER = LoggerFactory.getLogger(PolarisConfiguration.class); - public static final List> allConfigurations = new ArrayList<>(); + private static final List> allConfigurations = new ArrayList<>(); public final String key; public final String description; @@ -79,6 +79,11 @@ private static void registerConfiguration(PolarisConfiguration configuration) allConfigurations.add(configuration); } + /** Returns a list of all PolarisConfigurations that have been registered */ + public static List> getAllConfigurations() { + return List.copyOf(allConfigurations); + } + @SuppressWarnings("unchecked") protected PolarisConfiguration( String key, diff --git a/quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusReservedProperties.java b/quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusReservedProperties.java index 41953b6ed8..31f6deaf28 100644 --- a/quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusReservedProperties.java +++ b/quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusReservedProperties.java @@ -42,15 +42,16 @@ class AllowlistHolder { private static Set computeAllowlist() { Set allowlist = new HashSet<>(); - PolarisConfiguration.allConfigurations.forEach( - c -> { - if (c.hasCatalogConfig()) { - allowlist.add(c.catalogConfig()); - } - if (c.hasCatalogConfigUnsafe()) { - allowlist.add(c.catalogConfigUnsafe()); - } - }); + PolarisConfiguration.getAllConfigurations() + .forEach( + c -> { + if (c.hasCatalogConfig()) { + allowlist.add(c.catalogConfig()); + } + if (c.hasCatalogConfigUnsafe()) { + allowlist.add(c.catalogConfigUnsafe()); + } + }); return allowlist; } }