Allow to pass the list settings through environment variables (like […

…], ["a", "b", "c"], ...)

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

CHANGELOG.md

@@ -14,6 +14,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Implement Visitor Design pattern in QueryBuilder to enable the capability to traverse through the complex QueryBuilder tree. ([#10110](https://github.com/opensearch-project/OpenSearch/pull/10110))
 - Provide service accounts tokens to extensions ([#9618](https://github.com/opensearch-project/OpenSearch/pull/9618))
 - Configurable merge policy for index with an option to choose from LogByteSize and Tiered merge policy ([#9992](https://github.com/opensearch-project/OpenSearch/pull/9992))
+- Allow to pass the list settings through environment variables (like [], ["a", "b", "c"], ...) ([#10625](https://github.com/opensearch-project/OpenSearch/pull/10625))
 
 ### Dependencies
 - Bump `log4j-core` from 2.18.0 to 2.19.0

server/build.gradle

@@ -395,6 +395,7 @@ tasks.named("licenseHeaders").configure {
 }
 
 tasks.test {
+    environment "node.roles.test", "[]"
     if (BuildParams.runtimeJavaVersion > JavaVersion.VERSION_1_8) {
         jvmArgs += ["--add-opens", "java.base/java.nio.file=ALL-UNNAMED"]
     }

server/src/main/java/org/opensearch/common/settings/Setting.java

@@ -2337,17 +2337,11 @@ public static <T> Setting<List<T>> listSetting(
         if (defaultStringValue.apply(Settings.EMPTY) == null) {
             throw new IllegalArgumentException("default value function must not return null");
         }
-        Function<String, List<T>> parser = (s) -> isParsedAsEmptyArray(s)
-            ? Collections.emptyList()
-            : parseableStringToList(s).stream().map(singleValueParser).collect(Collectors.toList());
+        Function<String, List<T>> parser = (s) -> parseableStringToList(s).stream().map(singleValueParser).collect(Collectors.toList());
 
         return new ListSetting<>(key, fallbackSetting, defaultStringValue, parser, validator, properties);
     }
 
-    private static boolean isParsedAsEmptyArray(String parsableString) {
-        return parsableString.replaceAll("\\s", "").contentEquals("[\"[]\"]");
-    }
-
     private static List<String> parseableStringToList(String parsableString) {
         // fromXContent doesn't use named xcontent or deprecation.
         try (

server/src/main/java/org/opensearch/common/settings/Settings.java

@@ -80,6 +80,7 @@
 import java.util.Map;
 import java.util.NoSuchElementException;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.Set;
 import java.util.TreeMap;
 import java.util.concurrent.TimeUnit;
@@ -1211,8 +1212,14 @@ public boolean shouldRemoveMissingPlaceholder(String placeholderName) {
                 String value = propertyPlaceholder.replacePlaceholders(Settings.toString(entry.getValue()), placeholderResolver);
                 // if the values exists and has length, we should maintain it in the map
                 // otherwise, the replace process resolved into removing it
-                if (Strings.hasLength(value)) {
-                    entry.setValue(value);
+                if (Strings.hasLength(value) == true) {
+                    // try to parse the value as a list first
+                    final Optional<List<String>> optList = tryParseableStringToList(value);
+                    if (optList.isPresent()) {
+                        entry.setValue(optList.get());
+                    } else {
+                        entry.setValue(value);
+                    }
                 } else {
                     entryItr.remove();
                 }
@@ -1248,6 +1255,34 @@ public Settings build() {
             processLegacyLists(map);
             return new Settings(map, secureSettings.get());
         }
+
+        /**
+         * Tries to parse the placeholder value as a list (fe [], ["a", "b", "c"])
+         * @param parsableString placeholder value to parse
+         * @return the {@link Optional} result of the parsing attempt
+         */
+        private static Optional<List<String>> tryParseableStringToList(String parsableString) {
+            // fromXContent doesn't use named xcontent or deprecation.
+            try (
+                XContentParser xContentParser = MediaTypeRegistry.JSON.xContent()
+                    .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, parsableString)
+            ) {
+                XContentParser.Token token = xContentParser.nextToken();
+                if (token != XContentParser.Token.START_ARRAY) {
+                    return Optional.empty();
+                }
+                ArrayList<String> list = new ArrayList<>();
+                while ((token = xContentParser.nextToken()) != XContentParser.Token.END_ARRAY) {
+                    if (token != XContentParser.Token.VALUE_STRING) {
+                        return Optional.empty();
+                    }
+                    list.add(xContentParser.text());
+                }
+                return Optional.of(list);
+            } catch (IOException e) {
+                return Optional.empty();
+            }
+        }
     }
 
     // TODO We could use an FST internally to make things even faster and more compact

server/src/test/java/org/opensearch/common/settings/SettingsTests.java

@@ -50,6 +50,7 @@
 
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
+import java.io.InputStream;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -64,7 +65,9 @@
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.hasToString;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.notNullValue;
@@ -93,6 +96,15 @@ public void testReplacePropertiesPlaceholderSystemPropertyList() {
         assertThat(settings.getAsList("setting1"), contains(hostname, hostip));
     }
 
+    public void testReplacePropertiesPlaceholderSystemPropertyEmptyList() {
+        final Settings settings = Settings.builder()
+            .put("setting1", "${HOSTNAMES}")
+            .replacePropertyPlaceholders(name -> name.equals("HOSTNAMES") ? "[]" : null)
+            .build();
+        assertThat(settings.getAsList("setting1"), empty());
+        assertThat(settings.get("setting1"), equalTo("[]"));
+    }
+
     public void testReplacePropertiesPlaceholderSystemVariablesHaveNoEffect() {
         final String value = System.getProperty("java.home");
         assertNotNull(value);
@@ -603,6 +615,18 @@ public void testSimpleYamlSettings() throws Exception {
         assertThat(settings.getAsList("test1.test3").size(), equalTo(2));
         assertThat(settings.getAsList("test1.test3").get(0), equalTo("test3-1"));
         assertThat(settings.getAsList("test1.test3").get(1), equalTo("test3-2"));
+        assertThat(settings.getAsList("test1.test4"), empty());
+    }
+
+    public void testYamlPlaceholder() throws IOException {
+        try (InputStream in = new ByteArrayInputStream("hosts: ${HOSTNAMES}".getBytes(StandardCharsets.UTF_8))) {
+            Settings settings = Settings.builder()
+                .loadFromStream("foo.yml", in, false)
+                .replacePropertyPlaceholders(name -> name.equals("HOSTNAMES") ? "[\"h1\", \"h2\"]" : null)
+                .build();
+            assertThat(settings.getAsList("hosts"), hasSize(2));
+            assertThat(settings.getAsList("hosts"), containsInAnyOrder("h1", "h2"));
+        }
     }
 
     public void testYamlLegacyList() throws IOException {

server/src/test/java/org/opensearch/node/NodeRoleSettingsTests.java

@@ -18,6 +18,7 @@
 import java.util.List;
 
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.empty;
 
 public class NodeRoleSettingsTests extends OpenSearchTestCase {
 
@@ -73,9 +74,12 @@ public void testUnknownNodeRoleOnly() {
         assertEquals(testRole, nodeRoles.get(0).roleNameAbbreviation());
     }
 
-    public void testNodeRoleOfEmptyArrayNameShouldBeIgnored() {
-        String testRole = "[ ]";
-        Settings roleSettings = Settings.builder().put(NodeRoleSettings.NODE_ROLES_SETTING.getKey(), testRole).build();
-        assertEquals(Collections.emptyList(), NodeRoleSettings.NODE_ROLES_SETTING.get(roleSettings));
+    public void testNodeRolesFromEnvironmentVariables() {
+        Settings roleSettings = Settings.builder()
+            .put(NodeRoleSettings.NODE_ROLES_SETTING.getKey(), "${node.roles.test}")
+            .replacePropertyPlaceholders()
+            .build();
+        List<DiscoveryNodeRole> nodeRoles = NodeRoleSettings.NODE_ROLES_SETTING.get(roleSettings);
+        assertThat(nodeRoles, empty());
     }
 }

server/src/test/resources/org/opensearch/common/settings/loader/test-settings.yml

@@ -6,3 +6,4 @@ test1:
   test3:
     - test3-1
     - test3-2
+  test4: []