diff --git a/CHANGELOG.md b/CHANGELOG.md index a0c93053a2..e4842b816b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti ### Changes ### Deprecations +- Deprecates the `SUPPORTED_CATALOG_CONNECTION_TYPES` configuration property and replaces it with `SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES`. ### Fixes 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 db5176e7c3..d8392eee16 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 @@ -19,6 +19,7 @@ package org.apache.polaris.core.config; import java.util.Optional; +import java.util.Set; /** * Internal configuration flags for non-feature behavior changes in Polaris. These flags control @@ -34,11 +35,12 @@ public class BehaviorChangeConfiguration extends PolarisConfiguration { protected BehaviorChangeConfiguration( String key, + Set legacyKeys, String description, T defaultValue, Optional catalogConfig, Optional catalogConfigUnsafe) { - super(key, description, defaultValue, catalogConfig, catalogConfigUnsafe); + super(key, legacyKeys, 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 5ee36d030f..06fd98164b 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 @@ -20,6 +20,7 @@ import java.util.List; import java.util.Optional; +import java.util.Set; import org.apache.polaris.core.admin.model.AuthenticationParameters; import org.apache.polaris.core.admin.model.StorageConfigInfo; import org.apache.polaris.core.connection.ConnectionType; @@ -36,11 +37,12 @@ public class FeatureConfiguration extends PolarisConfiguration { protected FeatureConfiguration( String key, + Set legacyKeys, String description, T defaultValue, Optional catalogConfig, Optional catalogConfigUnsafe) { - super(key, description, defaultValue, catalogConfig, catalogConfigUnsafe); + super(key, legacyKeys, description, defaultValue, catalogConfig, catalogConfigUnsafe); } /** @@ -263,12 +265,14 @@ public static void enforceFeatureEnabledOrThrow( .defaultValue(true) .buildFeatureConfiguration(); - public static final FeatureConfiguration> SUPPORTED_CATALOG_CONNECTION_TYPES = - PolarisConfiguration.>builder() - .key("SUPPORTED_CATALOG_CONNECTION_TYPES") - .description("The list of supported catalog connection types for federation") - .defaultValue(List.of(ConnectionType.ICEBERG_REST.name())) - .buildFeatureConfiguration(); + public static final FeatureConfiguration> + SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES = + PolarisConfiguration.>builder() + .key("SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES") + .legacyKey("SUPPORTED_CATALOG_CONNECTION_TYPES") + .description("The list of supported external catalog connection types for federation") + .defaultValue(List.of(ConnectionType.ICEBERG_REST.name())) + .buildFeatureConfiguration(); public static final FeatureConfiguration> SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES = 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 31ae187955..f7ba93f6c7 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 @@ -19,8 +19,10 @@ package org.apache.polaris.core.config; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -41,6 +43,7 @@ public abstract class PolarisConfiguration { public final String key; public final String description; + public final Set legacyKeys; public final T defaultValue; private final Optional catalogConfigImpl; private final Optional catalogConfigUnsafeImpl; @@ -55,9 +58,22 @@ public abstract class PolarisConfiguration { */ private static void registerConfiguration(PolarisConfiguration configuration) { for (PolarisConfiguration existingConfiguration : allConfigurations) { - if (existingConfiguration.key.equals(configuration.key)) { + if (existingConfiguration.key.equals(configuration.key) + || configuration.legacyKeys.contains(existingConfiguration.key)) { + throw new IllegalArgumentException( + String.format("Config '%s' is already in use", existingConfiguration.key)); + } else if (existingConfiguration.legacyKeys.contains(configuration.key)) { throw new IllegalArgumentException( String.format("Config '%s' is already in use", configuration.key)); + } else if (!configuration.legacyKeys.isEmpty()) { + Set legacyKeys = new HashSet<>(existingConfiguration.legacyKeys); + legacyKeys.retainAll(configuration.legacyKeys); + if (!legacyKeys.isEmpty()) { + throw new IllegalArgumentException( + String.format( + "Config '%s' is already in use", + legacyKeys.stream().collect(Collectors.joining(",")))); + } } else { var configs = Stream.of( @@ -86,11 +102,13 @@ public static List> getAllConfigurations() { @SuppressWarnings("unchecked") protected PolarisConfiguration( String key, + Set legacyKeys, String description, T defaultValue, Optional catalogConfig, Optional catalogConfigUnsafe) { this.key = key; + this.legacyKeys = legacyKeys; this.description = description; this.defaultValue = defaultValue; this.catalogConfigImpl = catalogConfig; @@ -98,6 +116,14 @@ protected PolarisConfiguration( this.typ = (Class) defaultValue.getClass(); } + public boolean hasLegacyKeys() { + return !legacyKeys.isEmpty(); + } + + public Set legacyKeys() { + return legacyKeys; + } + public boolean hasCatalogConfig() { return catalogConfigImpl.isPresent(); } @@ -126,6 +152,7 @@ T cast(Object value) { public static class Builder { private String key; + private Set legacyKeys = new HashSet<>(); private String description; private T defaultValue; private Optional catalogConfig = Optional.empty(); @@ -136,6 +163,16 @@ public Builder key(String key) { return this; } + public Builder legacyKey(String legacyKey) { + legacyKeys.add(legacyKey); + return this; + } + + public Builder legacyKeys(Set legacyKeys) { + this.legacyKeys.addAll(legacyKeys); + return this; + } + public Builder description(String description) { this.description = description; return this; @@ -190,7 +227,7 @@ public FeatureConfiguration buildFeatureConfiguration() { validateOrThrow(); FeatureConfiguration config = new FeatureConfiguration<>( - key, description, defaultValue, catalogConfig, catalogConfigUnsafe); + key, legacyKeys, description, defaultValue, catalogConfig, catalogConfigUnsafe); PolarisConfiguration.registerConfiguration(config); return config; } @@ -203,7 +240,7 @@ public BehaviorChangeConfiguration buildBehaviorChangeConfiguration() { } BehaviorChangeConfiguration config = new BehaviorChangeConfiguration<>( - key, description, defaultValue, catalogConfig, catalogConfigUnsafe); + key, legacyKeys, 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 4923d97eea..0bbde0548f 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 @@ -99,7 +99,27 @@ public interface PolarisConfigurationStore { */ default @Nonnull T getConfiguration( @Nonnull RealmContext realmContext, PolarisConfiguration config) { - T result = getConfiguration(realmContext, config.key, config.defaultValue); + T result = getConfiguration(realmContext, config.key); + + if (result == null && config.hasLegacyKeys()) { + for (String legacyKey : config.legacyKeys()) { + T legacyResult = getConfiguration(realmContext, legacyKey); + if (legacyResult != null) { + LOGGER.warn( + "Legacy configuration key '{}' is in use. Please use '{}' instead.", + legacyKey, + config.key); + if (result == null) { + result = legacyResult; + } + } + } + } + + if (result == null) { + result = config.defaultValue; + } + return tryCast(config, result); } diff --git a/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java b/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java new file mode 100644 index 0000000000..56bfd028f0 --- /dev/null +++ b/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java @@ -0,0 +1,144 @@ +/* + * 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.config; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import org.junit.jupiter.api.Test; + +public class PolarisConfigurationTest { + + @Test + void testFeatureConfigurationWithLegacyKey() { + FeatureConfiguration config = + PolarisConfiguration.builder() + .key("FEATURE_CONFIG_WITH_LEGACY_KEY") + .legacyKey("OLD_FEATURE_CONFIG_WITH_LEGACY_KEY") + .description("Test configuration with deprecated key") + .defaultValue(true) + .buildFeatureConfiguration(); + + assertThat(config.hasLegacyKeys()).isTrue(); + assertThat(config.legacyKeys().contains("OLD_FEATURE_CONFIG_WITH_LEGACY_KEY")).isTrue(); + assertThat(config.key).isEqualTo("FEATURE_CONFIG_WITH_LEGACY_KEY"); + assertThat(config.description).isEqualTo("Test configuration with deprecated key"); + assertThat(config.defaultValue).isTrue(); + } + + @Test + void testBehaviorChangeConfigurationWithLegacyKey() { + BehaviorChangeConfiguration config = + PolarisConfiguration.builder() + .key("BEHAVIOR_CONFIG_WITH_LEGACY_KEY") + .legacyKey("OLD_BEHAVIOR_CONFIG_WITH_LEGACY_KEY") + .description("Test behavior configuration with deprecated key") + .defaultValue("test-value") + .buildBehaviorChangeConfiguration(); + + assertThat(config.hasLegacyKeys()).isTrue(); + assertThat(config.legacyKeys().contains("OLD_BEHAVIOR_CONFIG_WITH_LEGACY_KEY")).isTrue(); + assertThat(config.key).isEqualTo("BEHAVIOR_CONFIG_WITH_LEGACY_KEY"); + assertThat(config.defaultValue).isEqualTo("test-value"); + } + + @Test + void testDuplicateKeyValidation() { + PolarisConfiguration.builder() + .key("DUPLICATE_KEY_TEST_1") + .description("First configuration") + .defaultValue(true) + .buildFeatureConfiguration(); + + assertThatThrownBy( + () -> + PolarisConfiguration.builder() + .key("DUPLICATE_KEY_TEST_1") + .description("Second configuration") + .defaultValue(false) + .buildFeatureConfiguration()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Config 'DUPLICATE_KEY_TEST_1' is already in use"); + } + + @Test + void testDuplicateLegacyKeysValidation() { + PolarisConfiguration.builder() + .key("NEW_KEY_DEPRECATED_TEST_2") + .legacyKey("LEGACY_KEY_TEST_2_1") + .legacyKey("LEGACY_KEY_TEST_2_2") + .legacyKey("LEGACY_KEY_TEST_2_3") + .description("First configuration with deprecated key") + .defaultValue(true) + .buildFeatureConfiguration(); + + assertThatThrownBy( + () -> + PolarisConfiguration.builder() + .key("ANOTHER_NEW_KEY_2") + .legacyKey("LEGACY_KEY_TEST_2_1") + .legacyKey("LEGACY_KEY_TEST_2_3") + .description("Second configuration with same deprecated key") + .defaultValue(false) + .buildFeatureConfiguration()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Config 'LEGACY_KEY_TEST_2_3,LEGACY_KEY_TEST_2_1' is already in use"); + } + + @Test + void testNewKeyMatchingExistingLegacyKeyValidation() { + PolarisConfiguration.builder() + .key("ORIGINAL_KEY_3") + .legacyKey("OLD_LEGACY_KEY_3") + .description("First configuration with deprecated key") + .defaultValue(true) + .buildFeatureConfiguration(); + + assertThatThrownBy( + () -> + PolarisConfiguration.builder() + .key("OLD_LEGACY_KEY_3") + .description("Configuration with key matching existing deprecated key") + .defaultValue(false) + .buildFeatureConfiguration()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Config 'OLD_LEGACY_KEY_3' is already in use"); + } + + @Test + void testLegacyKeyMatchingExistingKeyValidation() { + PolarisConfiguration.builder() + .key("EXISTING_KEY_4") + .description("First configuration") + .defaultValue(true) + .buildFeatureConfiguration(); + + assertThatThrownBy( + () -> + PolarisConfiguration.builder() + .key("NEW_KEY_FOR_VALIDATION_4") + .legacyKey("EXISTING_KEY_4") + .legacyKey("EXISTING_KEY_4_1") + .description("Configuration with deprecated key matching existing key") + .defaultValue(false) + .buildFeatureConfiguration()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Config 'EXISTING_KEY_4' is already in use"); + } +} 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 4b190d3d15..dc1ad6235b 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 @@ -19,7 +19,6 @@ package org.apache.polaris.service.storage; import jakarta.annotation.Nonnull; -import jakarta.annotation.Nullable; import java.util.List; import java.util.Map; import java.util.function.Function; @@ -54,7 +53,7 @@ public void testConfigsCanBeCastedFromString() { */ @SuppressWarnings("unchecked") @Override - public @Nullable T getConfiguration(@Nonnull RealmContext ctx, String configName) { + public T getConfiguration(@Nonnull RealmContext ctx, String configName) { for (PolarisConfiguration c : configs) { if (c.key.equals(configName)) { return (T) String.valueOf(c.defaultValue); @@ -163,9 +162,9 @@ public void testEntityOverrides() { PolarisConfigurationStore store = new PolarisConfigurationStore() { + @SuppressWarnings("unchecked") @Override - public @Nullable T getConfiguration( - @Nonnull RealmContext realmContext, String configName) { + public T getConfiguration(@Nonnull RealmContext realmContext, String configName) { //noinspection unchecked return (T) Map.of("key2", "config-value2").get(configName); } @@ -186,4 +185,78 @@ public void testEntityOverrides() { Assertions.assertEquals( "entity-legacy4", store.getConfiguration(testRealmContext, entity, cfg.apply(4))); } + + @Test + public void testLegacyKeyLookup() { + FeatureConfiguration config = + PolarisConfiguration.builder() + .key("LEGACY_KEY_TEST_NEW_KEY") + .legacyKey("LEGACY_TEST_OLD_KEY_1") + .legacyKey("LEGACY_TEST_OLD_KEY_2") + .description("Test config with legacy keys") + .defaultValue("default-value") + .buildFeatureConfiguration(); + + PolarisConfigurationStore store = + new PolarisConfigurationStore() { + @SuppressWarnings("unchecked") + @Override + public T getConfiguration(@Nonnull RealmContext ctx, String configName) { + Map values = Map.of("LEGACY_TEST_OLD_KEY_1", "legacy-value-1"); + return (T) values.get(configName); + } + }; + + String result = store.getConfiguration(testRealmContext, config); + Assertions.assertEquals("legacy-value-1", result); + } + + @Test + public void testMainKeyTakesPrecedenceOverLegacyKeys() { + FeatureConfiguration config = + PolarisConfiguration.builder() + .key("PRECEDENCE_TEST_NEW_KEY") + .legacyKey("PRECEDENCE_TEST_OLD_KEY") + .description("Test config with legacy keys") + .defaultValue("default-value") + .buildFeatureConfiguration(); + + PolarisConfigurationStore store = + new PolarisConfigurationStore() { + @SuppressWarnings("unchecked") + @Override + public T getConfiguration(@Nonnull RealmContext ctx, String configName) { + Map values = + Map.of( + "PRECEDENCE_TEST_NEW_KEY", "new-value", + "PRECEDENCE_TEST_OLD_KEY", "legacy-value"); + return (T) values.get(configName); + } + }; + + String result = store.getConfiguration(testRealmContext, config); + Assertions.assertEquals("new-value", result); + } + + @Test + public void testFallbackToDefaultWhenNoKeysFound() { + FeatureConfiguration config = + PolarisConfiguration.builder() + .key("FALLBACK_TEST_NEW_KEY") + .legacyKey("FALLBACK_TEST_OLD_KEY") + .description("Test config with legacy keys") + .defaultValue("default-value") + .buildFeatureConfiguration(); + + PolarisConfigurationStore store = + new PolarisConfigurationStore() { + @Override + public T getConfiguration(@Nonnull RealmContext ctx, String configName) { + return null; + } + }; + + String result = store.getConfiguration(testRealmContext, config); + Assertions.assertEquals("default-value", result); + } } diff --git a/runtime/defaults/src/main/resources/application.properties b/runtime/defaults/src/main/resources/application.properties index d88df31d4d..133b782505 100644 --- a/runtime/defaults/src/main/resources/application.properties +++ b/runtime/defaults/src/main/resources/application.properties @@ -111,7 +111,7 @@ polaris.realm-context.require-header=false polaris.features."ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING"=false polaris.features."SUPPORTED_CATALOG_STORAGE_TYPES"=["S3","GCS","AZURE"] # polaris.features."ENABLE_CATALOG_FEDERATION"=true -polaris.features."SUPPORTED_CATALOG_CONNECTION_TYPES"=["ICEBERG_REST"] +polaris.features."SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES"=["ICEBERG_REST"] polaris.features."SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES"=["OAUTH", "BEARER"] # realm overrides diff --git a/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java b/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java index 071e6b61aa..a3e7c19f6c 100644 --- a/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java +++ b/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java @@ -192,7 +192,7 @@ private void validateConnectionConfigInfo(ConnectionConfigInfo connectionConfigI .getConfigurationStore() .getConfiguration( callContext.getRealmContext(), - FeatureConfiguration.SUPPORTED_CATALOG_CONNECTION_TYPES) + FeatureConfiguration.SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES) .stream() .map(s -> s.toUpperCase(Locale.ROOT)) .toList(); diff --git a/service/common/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplTest.java b/service/common/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplTest.java index fab1d7da41..347096fba5 100644 --- a/service/common/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplTest.java +++ b/service/common/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplTest.java @@ -76,7 +76,7 @@ void setUp() { when(callContext.getRealmContext()).thenReturn(realmContext); when(polarisCallContext.getConfigurationStore()).thenReturn(configurationStore); when(configurationStore.getConfiguration( - realmContext, FeatureConfiguration.SUPPORTED_CATALOG_CONNECTION_TYPES)) + realmContext, FeatureConfiguration.SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES)) .thenReturn(List.of("ICEBERG_REST")); when(configurationStore.getConfiguration( realmContext, FeatureConfiguration.SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES))