Remove defaults / overrides from feature configurations (#1572)

* double WithParentName

* autolint

* Revert some

* autolint

* add to BCconfigs

* autolint

* yank

* copy yuns test

* autolint

* remove defaults

* repair test

* autolint

* stablize test

* stable

* autolint

* configmap change

* copypaste

* regen helm docs

* autolint

* no dots in props

* remove accidental file

* small changes per review

* clean out defaults

* BCC fix

* autolint

* typefix

* autolint

Author: eric-maynard

helm/polaris/README.md

@@ -48,7 +48,7 @@ A Helm chart for Apache Polaris (incubating).
 
 ### Optional
 
-When using EclipseLink backed metastore a custom `persistence.xml` is required, a Kubernetes Secret must be created for it. Below is a sample command:
+When using a custom `persistence.xml`, a Kubernetes Secret must be created for it. Below is a sample command:
 ```bash
 kubectl create secret generic polaris-secret -n polaris --from-file=persistence.xml
 ```
@@ -67,7 +67,7 @@ helm unittest helm/polaris
 The below instructions assume Kind and Helm are installed.
 
 Simply run the `run.sh` script from the Polaris repo root, making sure to specify the
-`--eclipse-link-deps` if using EclipseLink backed metastore, option:
+`--eclipse-link-deps` option:
 
 ```bash
 ./run.sh
@@ -186,8 +186,8 @@ kubectl delete namespace polaris
 
 ## Values
 
- Key | Type | Default | Description |
-|-----|------|-----|-------------|
+| Key | Type | Default | Description |
+|-----|------|---------|-------------|
 | advancedConfig | object | `{}` | Advanced configuration. You can pass here any valid Polaris or Quarkus configuration property. Any property that is defined here takes precedence over all the other configuration values generated by this chart. Properties can be passed "flattened" or as nested YAML objects (see examples below). Note: values should be strings; avoid using numbers, booleans, or other types. |
 | affinity | object | `{}` | Affinity and anti-affinity for polaris pods. See https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/#affinity-and-anti-affinity. |
 | authentication | object | `{"authenticator":{"type":"default"},"tokenBroker":{"maxTokenGeneration":"PT1H","secret":{"name":null,"privateKey":"private.pem","publicKey":"public.pem","secretKey":"secret"},"type":"rsa-key-pair"},"tokenService":{"type":"default"}}` | Polaris authentication configuration. |
@@ -343,4 +343,4 @@ kubectl delete namespace polaris
 | tracing.attributes | object | `{}` | Resource attributes to identify the polaris service among other tracing sources. See https://opentelemetry.io/docs/reference/specification/resource/semantic_conventions/#service. If left empty, traces will be attached to a service named "Apache Polaris"; to change this, provide a service.name attribute here. |
 | tracing.enabled | bool | `false` | Specifies whether tracing for the polaris server should be enabled. |
 | tracing.endpoint | string | `"http://otlp-collector:4317"` | The collector endpoint URL to connect to (required). The endpoint URL must have either the http:// or the https:// scheme. The collector must talk the OpenTelemetry protocol (OTLP) and the port must be its gRPC port (by default 4317). See https://quarkus.io/guides/opentelemetry for more information. |
-| tracing.sample | string | `"1.0d"` | Which requests should be sampled. Valid values are: "all", "none", or a ratio between 0.0 and "1.0d" (inclusive). E.g. "0.5d" means that 50% of the requests will be sampled. Note: avoid entering numbers here, always prefer a string representation of the ratio. |
+| tracing.sample | string | `"1.0d"` | Which requests should be sampled. Valid values are: "all", "none", or a ratio between 0.0 and "1.0d" (inclusive). E.g. "0.5d" means that 50% of the requests will be sampled. Note: avoid entering numbers here, always prefer a string representation of the ratio. |

helm/polaris/templates/configmap.yaml

@@ -36,12 +36,12 @@ data:
     {{- $_ =  set $map "polaris.realm-context.realms" (join "," .Values.realmContext.realms) -}}
 
     {{- /* Features */ -}}
-    {{- range $k, $v := .Values.features.defaults -}}
-    {{- $_ = set $map (printf "polaris.features.defaults.\"%s\"" $k) (toJson $v) -}}
+    {{- range $k, $v := .Values.features -}}
+    {{- $_ = set $map (printf "polaris.features.\"%s\"" $k) (toJson $v) -}}
     {{- end -}}
     {{- range $realm, $overrides := .Values.features.realmOverrides -}}
     {{- range $k, $v := $overrides -}}
-    {{- $_ = set $map (printf "polaris.config.realm-overrides.\"%s\".\"%s\"" $realm $k) (toJson $v) -}}
+    {{- $_ = set $map (printf "polaris.features.realm-overrides.\"%s\".\"%s\"" $realm $k) (toJson $v) -}}
     {{- end -}}
     {{- end -}}
 

helm/polaris/tests/configmap_test.yaml

@@ -89,19 +89,18 @@ tests:
   - it: should configure features
     set:
       features:
-        defaults:
-          feature1: true
-          feature2: 42
+        feature1: true
+        feature2: 42
         realmOverrides:
           realm1:
             feature1: false
           realm2:
             feature2: 43
     asserts:
-      - matchRegex: { path: 'data["application.properties"]', pattern: "polaris.features.defaults.\"feature1\"=true" }
-      - matchRegex: { path: 'data["application.properties"]', pattern: "polaris.features.defaults.\"feature2\"=42" }
-      - matchRegex: { path: 'data["application.properties"]', pattern: "polaris.config.realm-overrides.\"realm1\".\"feature1\"=false" }
-      - matchRegex: { path: 'data["application.properties"]', pattern: "polaris.config.realm-overrides.\"realm2\".\"feature2\"=43" }
+      - matchRegex: { path: 'data["application.properties"]', pattern: "polaris.features.\"feature1\"=true" }
+      - matchRegex: { path: 'data["application.properties"]', pattern: "polaris.features.\"feature2\"=42" }
+      - matchRegex: { path: 'data["application.properties"]', pattern: "polaris.features.realm-overrides.\"realm1\".\"feature1\"=false" }
+      - matchRegex: { path: 'data["application.properties"]', pattern: "polaris.features.realm-overrides.\"realm2\".\"feature2\"=43" }
 
   - it: should configure persistence
     set:

polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java

@@ -177,10 +177,17 @@ public Builder<T> catalogConfigUnsafe(String catalogConfig) {
       return this;
     }
 
-    public FeatureConfiguration<T> buildFeatureConfiguration() {
+    private void validateOrThrow() {
       if (key == null || description == null || defaultValue == null) {
         throw new IllegalArgumentException("key, description, and defaultValue are required");
       }
+      if (key.contains(".")) {
+        throw new IllegalArgumentException("key cannot contain `.`");
+      }
+    }
+
+    public FeatureConfiguration<T> buildFeatureConfiguration() {
+      validateOrThrow();
       FeatureConfiguration<T> config =
           new FeatureConfiguration<>(
               key, description, defaultValue, catalogConfig, catalogConfigUnsafe);
@@ -189,9 +196,7 @@ public FeatureConfiguration<T> buildFeatureConfiguration() {
     }
 
     public BehaviorChangeConfiguration<T> buildBehaviorChangeConfiguration() {
-      if (key == null || description == null || defaultValue == null) {
-        throw new IllegalArgumentException("key, description, and defaultValue are required");
-      }
+      validateOrThrow();
       if (catalogConfig.isPresent() || catalogConfigUnsafe.isPresent()) {
         throw new IllegalArgumentException(
             "catalog configs are not valid for behavior change configs");

quarkus/defaults/src/main/resources/application-it.properties

@@ -31,16 +31,16 @@ polaris.persistence.type=in-memory
 
 polaris.secrets-manager.type=in-memory
 
-polaris.features.defaults."ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING"=false
-polaris.features.defaults."ALLOW_EXTERNAL_METADATA_FILE_LOCATION"=false
-polaris.features.defaults."ALLOW_OVERLAPPING_CATALOG_URLS"=true
-polaris.features.defaults."ALLOW_SPECIFYING_FILE_IO_IMPL"=true
-polaris.features.defaults."ALLOW_WILDCARD_LOCATION"=true
-polaris.features.defaults."ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING"=true
-polaris.features.defaults."INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_it"=true
-polaris.features.defaults."SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION"=true
-polaris.features.defaults."SUPPORTED_CATALOG_STORAGE_TYPES"=["FILE","S3","GCS","AZURE"]
-polaris.features.defaults."ENABLE_CATALOG_FEDERATION"=true
+polaris.features."ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING"=false
+polaris.features."ALLOW_EXTERNAL_METADATA_FILE_LOCATION"=false
+polaris.features."ALLOW_OVERLAPPING_CATALOG_URLS"=true
+polaris.features."ALLOW_SPECIFYING_FILE_IO_IMPL"=true
+polaris.features."ALLOW_WILDCARD_LOCATION"=true
+polaris.features."ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING"=true
+polaris.features."INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_it"=true
+polaris.features."SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION"=true
+polaris.features."SUPPORTED_CATALOG_STORAGE_TYPES"=["FILE","S3","GCS","AZURE"]
+polaris.features."ENABLE_CATALOG_FEDERATION"=true
 
 polaris.realm-context.realms=POLARIS,OTHER
 

quarkus/defaults/src/main/resources/application.properties

@@ -108,10 +108,10 @@ polaris.realm-context.realms=POLARIS
 polaris.realm-context.header-name=Polaris-Realm
 polaris.realm-context.require-header=false
 
-polaris.features.defaults."ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING"=false
-polaris.features.defaults."SUPPORTED_CATALOG_STORAGE_TYPES"=["S3","GCS","AZURE","FILE"]
-# polaris.features.defaults."ENABLE_CATALOG_FEDERATION"=true
-polaris.features.defaults."SUPPORTED_CATALOG_CONNECTION_TYPES"=["ICEBERG_REST"]
+polaris.features."ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING"=false
+polaris.features."SUPPORTED_CATALOG_STORAGE_TYPES"=["S3","GCS","AZURE","FILE"]
+# polaris.features."ENABLE_CATALOG_FEDERATION"=true
+polaris.features."SUPPORTED_CATALOG_CONNECTION_TYPES"=["ICEBERG_REST"]
 
 # realm overrides
 # polaris.features.realm-overrides."my-realm"."INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST"=true

quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusBehaviorChangesConfiguration.java

@@ -22,6 +22,7 @@
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import io.smallrye.config.ConfigMapping;
+import io.smallrye.config.WithParentName;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
@@ -33,9 +34,16 @@
 // QuarkusFeaturesConfiguration
 public interface QuarkusBehaviorChangesConfiguration {
 
+  @WithParentName
   Map<String, String> defaults();
 
-  Map<String, ? extends FeaturesConfiguration.RealmOverrides> realmOverrides();
+  Map<String, ? extends QuarkusRealmOverrides> realmOverrides();
+
+  interface QuarkusRealmOverrides extends FeaturesConfiguration.RealmOverrides {
+    @WithParentName
+    @Override
+    Map<String, String> overrides();
+  }
 
   default Map<String, Object> parseDefaults(ObjectMapper objectMapper) {
     return convertMap(objectMapper, defaults());

quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusFeaturesConfiguration.java

@@ -19,15 +19,23 @@
 package org.apache.polaris.service.quarkus.config;
 
 import io.smallrye.config.ConfigMapping;
+import io.smallrye.config.WithParentName;
 import java.util.Map;
 import org.apache.polaris.service.config.FeaturesConfiguration;
 
 @ConfigMapping(prefix = "polaris.features")
 public interface QuarkusFeaturesConfiguration extends FeaturesConfiguration {
 
+  @WithParentName
   @Override
   Map<String, String> defaults();
 
   @Override
-  Map<String, FeaturesConfiguration.RealmOverrides> realmOverrides();
+  Map<String, QuarkusRealmOverrides> realmOverrides();
+
+  interface QuarkusRealmOverrides extends RealmOverrides {
+    @WithParentName
+    @Override
+    Map<String, String> overrides();
+  }
 }

quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusResolvedBehaviorChangesConfiguration.java

@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.quarkus.config;
+
+import jakarta.annotation.Priority;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Alternative;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+/**
+ * Wraps around {@link QuarkusBehaviorChangesConfiguration} but removes properties from `defaults`
+ * that shouldn't be there
+ */
+@ApplicationScoped
+@Alternative
+@Priority(1)
+public class QuarkusResolvedBehaviorChangesConfiguration
+    implements QuarkusBehaviorChangesConfiguration {
+
+  private final Map<String, String> cleanedDefaults;
+  private final Map<String, ? extends QuarkusBehaviorChangesConfiguration.QuarkusRealmOverrides>
+      realmOverrides;
+
+  public QuarkusResolvedBehaviorChangesConfiguration(QuarkusBehaviorChangesConfiguration raw) {
+    this.realmOverrides = raw.realmOverrides();
+
+    // Filter out any keys that look like realm overrides
+    this.cleanedDefaults =
+        raw.defaults().entrySet().stream()
+            .filter(e -> e.getKey().split("\\.").length == 1)
+            .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+  }
+
+  @Override
+  public Map<String, String> defaults() {
+    return cleanedDefaults;
+  }
+
+  @Override
+  public Map<String, ? extends QuarkusBehaviorChangesConfiguration.QuarkusRealmOverrides>
+      realmOverrides() {
+    return realmOverrides;
+  }
+}

quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusResolvedFeaturesConfiguration.java

@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.quarkus.config;
+
+import jakarta.annotation.Priority;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Alternative;
+import java.util.Map;
+import java.util.stream.Collectors;
+import org.apache.polaris.service.config.FeaturesConfiguration;
+
+/**
+ * Wraps around {@link QuarkusFeaturesConfiguration} but removes properties from `defaults` that
+ * shouldn't be there
+ */
+@ApplicationScoped
+@Alternative
+@Priority(1)
+public class QuarkusResolvedFeaturesConfiguration implements FeaturesConfiguration {
+
+  private final Map<String, String> cleanedDefaults;
+  private final Map<String, ? extends RealmOverrides> realmOverrides;
+
+  public QuarkusResolvedFeaturesConfiguration(QuarkusFeaturesConfiguration raw) {
+    this.realmOverrides = raw.realmOverrides();
+
+    // Filter out any keys that look like realm overrides
+    this.cleanedDefaults =
+        raw.defaults().entrySet().stream()
+            .filter(e -> e.getKey().split("\\.").length == 1)
+            .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+  }
+
+  @Override
+  public Map<String, String> defaults() {
+    return cleanedDefaults;
+  }
+
+  @Override
+  public Map<String, ? extends RealmOverrides> realmOverrides() {
+    return realmOverrides;
+  }
+}

quarkus/service/src/test/java/org/apache/polaris/service/quarkus/admin/PolarisAuthzTestBase.java

@@ -113,9 +113,9 @@ public Set<Class<?>> getEnabledAlternatives() {
     @Override
     public Map<String, String> getConfigOverrides() {
       return Map.of(
-          "polaris.features.defaults.\"ALLOW_SPECIFYING_FILE_IO_IMPL\"",
+          "polaris.features.\"ALLOW_SPECIFYING_FILE_IO_IMPL\"",
           "true",
-          "polaris.features.defaults.\"ALLOW_EXTERNAL_METADATA_FILE_LOCATION\"",
+          "polaris.features.\"ALLOW_EXTERNAL_METADATA_FILE_LOCATION\"",
           "true");
     }
   }

quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/GenericTableCatalogTest.java

@@ -106,11 +106,11 @@ public static class Profile implements QuarkusTestProfile {
     @Override
     public Map<String, String> getConfigOverrides() {
       return Map.of(
-          "polaris.features.defaults.\"ALLOW_SPECIFYING_FILE_IO_IMPL\"",
+          "polaris.features.\"ALLOW_SPECIFYING_FILE_IO_IMPL\"",
           "true",
-          "polaris.features.defaults.\"INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST\"",
+          "polaris.features.\"INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST\"",
           "true",
-          "polaris.features.defaults.\"SUPPORTED_CATALOG_STORAGE_TYPES\"",
+          "polaris.features.\"SUPPORTED_CATALOG_STORAGE_TYPES\"",
           "[\"FILE\"]");
     }
   }

quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogHandlerAuthzTest.java

@@ -88,9 +88,9 @@ public static class Profile extends PolarisAuthzTestBase.Profile {
     @Override
     public Map<String, String> getConfigOverrides() {
       return Map.of(
-          "polaris.features.defaults.\"ALLOW_SPECIFYING_FILE_IO_IMPL\"",
+          "polaris.features.\"ALLOW_SPECIFYING_FILE_IO_IMPL\"",
           "true",
-          "polaris.features.defaults.\"ALLOW_EXTERNAL_METADATA_FILE_LOCATION\"",
+          "polaris.features.\"ALLOW_EXTERNAL_METADATA_FILE_LOCATION\"",
           "true");
     }
   }

quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java

@@ -169,13 +169,13 @@ public static class Profile implements QuarkusTestProfile {
     @Override
     public Map<String, String> getConfigOverrides() {
       return Map.of(
-          "polaris.features.defaults.\"ALLOW_SPECIFYING_FILE_IO_IMPL\"",
+          "polaris.features.\"ALLOW_SPECIFYING_FILE_IO_IMPL\"",
           "true",
-          "polaris.features.defaults.\"INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST\"",
+          "polaris.features.\"INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST\"",
           "true",
-          "polaris.features.defaults.\"SUPPORTED_CATALOG_STORAGE_TYPES\"",
+          "polaris.features.\"SUPPORTED_CATALOG_STORAGE_TYPES\"",
           "[\"FILE\"]",
-          "polaris.features.defaults.\"LIST_PAGINATION_ENABLED\"",
+          "polaris.features.\"LIST_PAGINATION_ENABLED\"",
           "true",
           "polaris.event-listener.type",
           "test");