Adding support for configuration through environment variables (apach…

…e#12307)

* init

* Added support for env vars

* Revert removal of SystemEnvironment and Environment classes

* Fix tests

* Fix formatting

* Revert unnecessary changes

* Revert unnecessary changes

* Formatting

* Comments

* Tests fix

* Added back legacy prefix

* Fix test

* Implementing dynamic templating for env config

* Fix typo

pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java

@@ -48,18 +48,25 @@
  * <li>Apache Commons {@link Configuration} (see {@link #PinotConfiguration(Configuration)})</li>
  * <li>Generic key-value properties provided with a {@link Map} (see
  * {@link PinotConfiguration#PinotConfiguration(Map)}</li>
- * <li>Environment variables (see {@link PinotConfiguration#PinotConfiguration(Map, Map)}</li>
+ * <li>Environment variables through env.dynamic.config (see {@link PinotConfiguration#PinotConfiguration(Map, Map)}
+ * </li>
  * <li>{@link PinotFSSpec} (see {@link PinotConfiguration#PinotConfiguration(PinotFSSpec)}</li>
  * </ul>
  * </p>
  * <p>
  * These different sources will all merged in an underlying {@link CompositeConfiguration} for which all
  * configuration keys will have
  * been sanitized.
- * Through this mechanism, properties can be configured and retrieved with kebab case, camel case, snake case and
- * environment variable
+ * Through this mechanism, properties can be configured and retrieved with kebab case, camel case and snake case
  * conventions.
  * </p>
+ * <strong>Dynamic configuration</strong>
+ * <p>
+ * In order to enable loading configurations through environment variables you can specify
+ * {@value ENV_DYNAMIC_CONFIG_KEY} as a list of property keys to dynamically template.
+ * {@link PinotConfiguration#applyDynamicEnvConfig(List, Map)}}. This enables loading secrets safely
+ * into the configuration.
+ * <p/>
  * <table>
  * <tr>
  * <th>Property</th>
@@ -77,15 +84,12 @@
  * <td>controller.sub_module.alerts.email_address</td>
  * <td>Snake case notation, which is an alternative format for use in .properties and .yml files.</td>
  * </tr>
- * <tr>
- * <td>PINOT_MODULE_SUBMODULE_ALERTS_EMAILADDRESS</td>
- * <td>Upper case format, which is recommended when using system environment variables.</td>
- * </tr>
  * </table>
  *
  */
 public class PinotConfiguration {
   public static final String CONFIG_PATHS_KEY = "config.paths";
+  public static final String ENV_DYNAMIC_CONFIG_KEY = "dynamic.env.config";
 
   private final CompositeConfiguration _configuration;
 
@@ -116,7 +120,7 @@ public PinotConfiguration(Configuration baseConfiguration) {
    * @param baseProperties to provide programmatically through a {@link Map}.
    */
   public PinotConfiguration(Map<String, Object> baseProperties) {
-    this(baseProperties, new HashMap<>());
+    this(baseProperties, new SystemEnvironment().getEnvironmentVariables());
   }
 
   /**
@@ -125,10 +129,12 @@ public PinotConfiguration(Map<String, Object> baseProperties) {
    * sanitized for relaxed binding. See {@link PinotConfiguration} for further details.
    *
    * @param baseProperties with highest precedences (e.g. CLI arguments)
-   * @param environmentVariables as a {@link Map}. Can be provided with {@link SystemEnvironment}
+   * @param environmentVariables as a {@link Map}.
    */
   public PinotConfiguration(Map<String, Object> baseProperties, Map<String, String> environmentVariables) {
-    _configuration = new CompositeConfiguration(computeConfigurationsFromSources(baseProperties, environmentVariables));
+    _configuration = new CompositeConfiguration(
+        applyDynamicEnvConfig(computeConfigurationsFromSources(baseProperties, environmentVariables),
+            environmentVariables));
   }
 
   /**
@@ -139,31 +145,51 @@ public PinotConfiguration(Map<String, Object> baseProperties, Map<String, String
    */
   public PinotConfiguration(PinotFSSpec pinotFSSpec) {
     this(Optional.ofNullable(pinotFSSpec.getConfigs()).map(configs -> configs.entrySet().stream()
-        .collect(Collectors.<Entry<String, String>, String, Object>toMap(Entry::getKey, entry -> entry.getValue())))
-        .orElseGet(() -> new HashMap<>()));
+            .collect(Collectors.<Entry<String, String>, String, Object>toMap(Entry::getKey, Entry::getValue)))
+        .orElseGet(HashMap::new));
   }
 
   private static List<Configuration> computeConfigurationsFromSources(Configuration baseConfiguration,
       Map<String, String> environmentVariables) {
     return computeConfigurationsFromSources(relaxConfigurationKeys(baseConfiguration), environmentVariables);
   }
 
+  /**
+   * This function templates the configuration from the env variables using env.dynamic.config to
+   * specify the mapping.
+   * E.g.
+   * env.dynamic.mapping=test.property
+   * test.property=ENV_VAR_NAME
+   * This function will look up `ENV_VAR_NAME` and insert its content in test.property.
+   *
+   * @param configurations List of configurations to template.
+   * @param environmentVariables Env used to fetch content to insert in the configuration.
+   * @return returns configuration
+   */
+  public static List<Configuration> applyDynamicEnvConfig(List<Configuration> configurations,
+      Map<String, String> environmentVariables) {
+    return configurations.stream().peek(configuration -> {
+      for (String dynamicEnvConfigVarName : configuration.getStringArray(ENV_DYNAMIC_CONFIG_KEY)) {
+        configuration.setProperty(dynamicEnvConfigVarName,
+            environmentVariables.get(configuration.getString(dynamicEnvConfigVarName)));
+      }
+    }).collect(Collectors.toList());
+  }
+
   private static List<Configuration> computeConfigurationsFromSources(Map<String, Object> baseProperties,
       Map<String, String> environmentVariables) {
     Map<String, Object> relaxedBaseProperties = relaxProperties(baseProperties);
+    // Env is only used to check for config paths to load.
     Map<String, String> relaxedEnvVariables = relaxEnvironmentVariables(environmentVariables);
 
-    Stream<Configuration> propertiesFromConfigPaths = Stream
-        .of(Optional.ofNullable(relaxedBaseProperties.get(CONFIG_PATHS_KEY)).map(Object::toString),
-            Optional.ofNullable(relaxedEnvVariables.get(CONFIG_PATHS_KEY)))
-
-        .filter(Optional::isPresent).map(Optional::get)
-
-        .flatMap(configPaths -> Arrays.stream(configPaths.split(",")))
-
-        .map(PinotConfiguration::loadProperties);
+    Stream<Configuration> propertiesFromConfigPaths =
+        Stream.of(Optional.ofNullable(relaxedBaseProperties.get(CONFIG_PATHS_KEY)).map(Object::toString),
+                Optional.ofNullable(relaxedEnvVariables.get(CONFIG_PATHS_KEY))).filter(Optional::isPresent)
+            .map(Optional::get).flatMap(configPaths -> Arrays.stream(configPaths.split(",")))
+            .map(PinotConfiguration::loadProperties);
 
-    return Stream.concat(Stream.of(relaxedBaseProperties, relaxedEnvVariables).map(e -> {
+    // Priority in CompositeConfiguration is CLI, ConfigFile(s)
+    return Stream.concat(Stream.of(relaxedBaseProperties).map(e -> {
       MapConfiguration mapConfiguration = new MapConfiguration(e);
       mapConfiguration.setListDelimiterHandler(new LegacyListDelimiterHandler(','));
       return mapConfiguration;
@@ -187,8 +213,7 @@ private static Configuration loadProperties(String configPath) {
       PropertiesConfiguration propertiesConfiguration;
       if (configPath.startsWith("classpath:")) {
         propertiesConfiguration = CommonsConfigurationUtils.fromInputStream(
-            PinotConfiguration.class.getResourceAsStream(configPath.substring("classpath:".length())),
-            true, true);
+            PinotConfiguration.class.getResourceAsStream(configPath.substring("classpath:".length())), true, true);
       } else {
         propertiesConfiguration = CommonsConfigurationUtils.fromPath(configPath, true, true);
       }
@@ -201,16 +226,16 @@ private static Configuration loadProperties(String configPath) {
   private static Map<String, Object> relaxConfigurationKeys(Configuration configuration) {
     return CommonsConfigurationUtils.getKeysStream(configuration).distinct()
 
-        .collect(Collectors.toMap(PinotConfiguration::relaxPropertyName, key -> configuration.getProperty(key)));
+        .collect(Collectors.toMap(PinotConfiguration::relaxPropertyName, configuration::getProperty));
   }
 
   private static Map<String, String> relaxEnvironmentVariables(Map<String, String> environmentVariables) {
-    return environmentVariables.entrySet().stream().filter(entry -> entry.getKey().startsWith("PINOT_"))
+    return environmentVariables.entrySet().stream()
         .collect(Collectors.toMap(PinotConfiguration::relaxEnvVarName, Entry::getValue));
   }
 
   private static String relaxEnvVarName(Entry<String, String> envVarEntry) {
-    return envVarEntry.getKey().substring(6).replace("_", ".").toLowerCase();
+    return envVarEntry.getKey().replace("_", ".").toLowerCase();
   }
 
   private static Map<String, Object> relaxProperties(Map<String, Object> properties) {
@@ -332,8 +357,8 @@ public int getProperty(String name, int defaultValue) {
    * @return the property String value. Fallback to the provided default values if no property is found.
    */
   public List<String> getProperty(String name, List<String> defaultValues) {
-    return Optional
-        .of(Arrays.stream(_configuration.getStringArray(relaxPropertyName(name))).collect(Collectors.toList()))
+    return Optional.of(
+            Arrays.stream(_configuration.getStringArray(relaxPropertyName(name))).collect(Collectors.toList()))
         .filter(list -> !list.isEmpty()).orElse(defaultValues);
   }
 

pinot-spi/src/test/java/org/apache/pinot/spi/env/PinotConfigurationTest.java

@@ -83,7 +83,7 @@ public void assertGetKeys() {
     properties.put("property.3.key", "val1");
     properties.put("property.4.key", "val1");
 
-    PinotConfiguration pinotConfiguration = new PinotConfiguration(properties);
+    PinotConfiguration pinotConfiguration = new PinotConfiguration(properties, new HashMap<>());
 
     List<String> keys = pinotConfiguration.getKeys();
     Assert.assertTrue(keys.contains("property.1.key"));
@@ -133,10 +133,7 @@ public void assertPropertyPriorities()
 
     baseProperties.put("controller.host", "cli-argument-controller-host");
     baseProperties.put("config.paths", "classpath:/pinot-configuration-1.properties");
-    mockedEnvironmentVariables.put("PINOT_CONTROLLER_HOST", "env-var-controller-host");
-    mockedEnvironmentVariables.put("PINOT_CONTROLLER_PORT", "env-var-controller-port");
-    mockedEnvironmentVariables.put("PINOT_RELAXEDPROPERTY_TEST", "true");
-    mockedEnvironmentVariables.put("PINOT_CONFIG_PATHS", configFile2 + "," + configFile3);
+    mockedEnvironmentVariables.put("CONFIG_PATHS", configFile2 + "," + configFile3);
 
     copyClasspathResource("/pinot-configuration-2.properties", configFile2);
     copyClasspathResource("/pinot-configuration-3.properties", configFile3);
@@ -146,9 +143,6 @@ public void assertPropertyPriorities()
     // Tests that cli arguments have the highest priority.
     Assert.assertEquals(configuration.getProperty("controller.host"), "cli-argument-controller-host");
 
-    // Tests that environment variable have priority overs config.paths properties.
-    Assert.assertEquals(configuration.getProperty("controller.port"), "env-var-controller-port");
-
     // Tests that config.paths properties provided through cli arguments are prioritized.
     Assert.assertEquals(configuration.getProperty("controller.cluster-name"), "config-path-1-cluster-name");
 
@@ -162,9 +156,35 @@ public void assertPropertyPriorities()
     // Tests properties provided through the last config file of a config.paths array.
     Assert.assertEquals(configuration.getProperty("controller.config-paths-multi-value-test-2"),
         "config-path-3-config-paths-multi-value-test-2");
+  }
+
+  @Test
+  public void assertDynamicEnvConfig()
+      throws IOException {
+    Map<String, Object> baseProperties = new HashMap<>();
+    Map<String, String> mockedEnvironmentVariables = new HashMap<>();
 
-    // Tests relaxed binding on environment variables
-    Assert.assertEquals(configuration.getProperty("relaxed-property.test"), "true");
+    String configFile = File.createTempFile("pinot-configuration-test-4", ".properties").getAbsolutePath();
+
+    baseProperties.put("server.host", "ENV_SERVER_HOST");
+    baseProperties.put("not.templated.cli", "static-value");
+    baseProperties.put("dynamic.env.config", "server.host");
+
+    mockedEnvironmentVariables.put("ENV_VAR_HOST", "test-host");
+    mockedEnvironmentVariables.put("TEST_PROPERTY", "test-property");
+    mockedEnvironmentVariables.put("ENV_SERVER_HOST", "test-server-host");
+
+    baseProperties.put("config.paths", "classpath:/pinot-configuration-4.properties");
+    copyClasspathResource("/pinot-configuration-4.properties", configFile);
+
+    PinotConfiguration configuration = new PinotConfiguration(baseProperties, mockedEnvironmentVariables);
+
+    // Tests that cli arguments have the highest priority.
+    Assert.assertEquals(configuration.getProperty("server.host"), "test-server-host");
+    // Checking for non templated values
+    Assert.assertEquals(configuration.getProperty("not.templated.cli"), "static-value");
+    // Templated values in configFile
+    Assert.assertEquals(configuration.getProperty("pinot.controller.host"), "test-host");
   }
 
   @Test(expectedExceptions = IllegalArgumentException.class)
@@ -180,8 +200,8 @@ public void assertInvalidConfigPathBehavior() {
   public void assertPropertiesFromBaseConfiguration()
       throws ConfigurationException {
     PropertiesConfiguration propertiesConfiguration = CommonsConfigurationUtils.fromPath(
-        PropertiesConfiguration.class.getClassLoader().getResource("pinot-configuration-1.properties").getFile(),
-        true, true);
+        PropertiesConfiguration.class.getClassLoader().getResource("pinot-configuration-1.properties").getFile(), true,
+        true);
 
     PinotConfiguration config = new PinotConfiguration(propertiesConfiguration);
 

pinot-spi/src/test/resources/pinot-configuration-4.properties

@@ -0,0 +1,23 @@
+#
+# 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.
+#
+
+dynamic.env.config=pinot.controller.host,pinot.test.property,server.host
+
+pinot.controller.host=ENV_VAR_HOST
+server.host=incorrect-value