Merge branch 'main' of github.com:apache/polaris into generic-federation

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/BehaviorChangeConfiguration.java

@@ -33,8 +33,12 @@
 public class BehaviorChangeConfiguration<T> extends PolarisConfiguration<T> {
 
   protected BehaviorChangeConfiguration(
-      String key, String description, T defaultValue, Optional<String> catalogConfig) {
-    super(key, description, defaultValue, catalogConfig);
+      String key,
+      String description,
+      T defaultValue,
+      Optional<String> catalogConfig,
+      Optional<String> catalogConfigUnsafe) {
+    super(key, description, defaultValue, catalogConfig, catalogConfigUnsafe);
   }
 
   public static final BehaviorChangeConfiguration<Boolean> VALIDATE_VIEW_LOCATION_OVERLAP =

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

@@ -34,8 +34,12 @@
  */
 public class FeatureConfiguration<T> extends PolarisConfiguration<T> {
   protected FeatureConfiguration(
-      String key, String description, T defaultValue, Optional<String> catalogConfig) {
-    super(key, description, defaultValue, catalogConfig);
+      String key,
+      String description,
+      T defaultValue,
+      Optional<String> catalogConfig,
+      Optional<String> catalogConfigUnsafe) {
+    super(key, description, defaultValue, catalogConfig, catalogConfigUnsafe);
   }
 
   /**
@@ -82,7 +86,8 @@ public static void enforceFeatureEnabledOrThrow(
   public static final FeatureConfiguration<Boolean> ALLOW_TABLE_LOCATION_OVERLAP =
       PolarisConfiguration.<Boolean>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.")
@@ -116,15 +121,17 @@ public static void enforceFeatureEnabledOrThrow(
   public static final FeatureConfiguration<Boolean> ALLOW_UNSTRUCTURED_TABLE_LOCATION =
       PolarisConfiguration.<Boolean>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();
 
   public static final FeatureConfiguration<Boolean> ALLOW_EXTERNAL_TABLE_LOCATION =
       PolarisConfiguration.<Boolean>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)
@@ -133,15 +140,17 @@ public static void enforceFeatureEnabledOrThrow(
   public static final FeatureConfiguration<Boolean> ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING =
       PolarisConfiguration.<Boolean>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();
 
   public static final FeatureConfiguration<List<String>> SUPPORTED_CATALOG_STORAGE_TYPES =
       PolarisConfiguration.<List<String>>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(
@@ -154,23 +163,26 @@ public static void enforceFeatureEnabledOrThrow(
   public static final FeatureConfiguration<Boolean> CLEANUP_ON_NAMESPACE_DROP =
       PolarisConfiguration.<Boolean>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();
 
   public static final FeatureConfiguration<Boolean> CLEANUP_ON_CATALOG_DROP =
       PolarisConfiguration.<Boolean>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();
 
   public static final FeatureConfiguration<Boolean> DROP_WITH_PURGE_ENABLED =
       PolarisConfiguration.<Boolean>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)

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

@@ -21,32 +21,81 @@
 import java.util.ArrayList;
 import java.util.List;
 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
+ * An ABC for Polaris configurations that alter the service's behavior TODO: deprecate unsafe
+ * catalog configs and remove related code
  *
  * @param <T> The type of the configuration
  */
 public abstract class PolarisConfiguration<T> {
 
   private static final Logger LOGGER = LoggerFactory.getLogger(PolarisConfiguration.class);
 
+  private static final List<PolarisConfiguration<?>> allConfigurations = new ArrayList<>();
+
   public final String key;
   public final String description;
   public final T defaultValue;
   private final Optional<String> catalogConfigImpl;
+  private final Optional<String> catalogConfigUnsafeImpl;
   private final Class<T> 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);
+  }
+
+  /** Returns a list of all PolarisConfigurations that have been registered */
+  public static List<PolarisConfiguration<?>> getAllConfigurations() {
+    return List.copyOf(allConfigurations);
+  }
+
   @SuppressWarnings("unchecked")
   protected PolarisConfiguration(
-      String key, String description, T defaultValue, Optional<String> catalogConfig) {
+      String key,
+      String description,
+      T defaultValue,
+      Optional<String> catalogConfig,
+      Optional<String> catalogConfigUnsafe) {
     this.key = key;
     this.description = description;
     this.defaultValue = defaultValue;
     this.catalogConfigImpl = catalogConfig;
+    this.catalogConfigUnsafeImpl = catalogConfigUnsafe;
     this.typ = (Class<T>) defaultValue.getClass();
   }
 
@@ -61,6 +110,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 +130,7 @@ public static class Builder<T> {
     private String description;
     private T defaultValue;
     private Optional<String> catalogConfig = Optional.empty();
+    private Optional<String> catalogConfigUnsafe = Optional.empty();
 
     public Builder<T> key(String key) {
       this.key = key;
@@ -93,26 +154,58 @@ public Builder<T> defaultValue(T defaultValue) {
     }
 
     public Builder<T> 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;
     }
 
-    public FeatureConfiguration<T> buildFeatureConfiguration() {
-      if (key == null || description == null || defaultValue == null) {
-        throw new IllegalArgumentException("key, description, and defaultValue are required");
+    /**
+     * Used to support backwards compatability before there were reserved properties. Usage of this
+     * method should be removed over time.
+     */
+    @Deprecated
+    public Builder<T> 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);
       }
-      return new FeatureConfiguration<>(key, description, defaultValue, catalogConfig);
+      this.catalogConfigUnsafe = Optional.of(catalogConfig);
+      return this;
     }
 
-    public BehaviorChangeConfiguration<T> buildBehaviorChangeConfiguration() {
+    private void validateOrThrow() {
       if (key == null || description == null || defaultValue == null) {
         throw new IllegalArgumentException("key, description, and defaultValue are required");
       }
-      if (catalogConfig.isPresent()) {
+      if (key.contains(".")) {
+        throw new IllegalArgumentException("key cannot contain `.`");
+      }
+    }
+
+    public FeatureConfiguration<T> buildFeatureConfiguration() {
+      validateOrThrow();
+      FeatureConfiguration<T> config =
+          new FeatureConfiguration<>(
+              key, description, defaultValue, catalogConfig, catalogConfigUnsafe);
+      PolarisConfiguration.registerConfiguration(config);
+      return config;
+    }
+
+    public BehaviorChangeConfiguration<T> buildBehaviorChangeConfiguration() {
+      validateOrThrow();
+      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<T> config =
+          new BehaviorChangeConfiguration<>(
+              key, description, defaultValue, catalogConfig, catalogConfigUnsafe);
+      PolarisConfiguration.registerConfiguration(config);
+      return config;
     }
   }