Treat Setting value with empty array string as empty array (opensearc…

…h-project#10625) * Treat Setting value with empty array string as empty array Signed-off-by: Jeongmin Yu <machenity@gmail.com> * Allow to pass the list settings through environment variables (like [], ["a", "b", "c"], ...) Signed-off-by: Andriy Redko <andriy.redko@aiven.io> --------- Signed-off-by: Jeongmin Yu <machenity@gmail.com> Signed-off-by: Andriy Redko <andriy.redko@aiven.io> Co-authored-by: Andriy Redko <andriy.redko@aiven.io> Signed-off-by: Shivansh Arora <hishiv@amazon.com>
Gaurav614 · Apr 25, 2024 · b9f43b9 · b9f43b9
1 parent 512917d
commit b9f43b9
Show file tree

Hide file tree

Showing 6 changed files with 74 additions and 2 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -20,6 +20,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - [Remote cluster state] Restore global metadata from remote store when local state is lost after quorum loss ([#10404](https://github.com/opensearch-project/OpenSearch/pull/10404))
 - [AdmissionControl] Added changes for AdmissionControl Interceptor and AdmissionControlService for RateLimiting ([#9286](https://github.com/opensearch-project/OpenSearch/pull/9286))
 - GHA to verify checklist items completion in PR descriptions ([#10800](https://github.com/opensearch-project/OpenSearch/pull/10800))
+- Allow to pass the list settings through environment variables (like [], ["a", "b", "c"], ...) ([#10625](https://github.com/opensearch-project/OpenSearch/pull/10625))
 - [Remote cluster state] Restore cluster state version during remote state auto restore ([#10853](https://github.com/opensearch-project/OpenSearch/pull/10853))
 - Add back half_float BKD based sort query optimization ([#11024](https://github.com/opensearch-project/OpenSearch/pull/11024))
 

diff --git a/server/build.gradle b/server/build.gradle
@@ -403,6 +403,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"]
     }

diff --git a/server/src/main/java/org/opensearch/common/settings/Settings.java b/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

diff --git a/server/src/test/java/org/opensearch/common/settings/SettingsTests.java b/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 {

diff --git a/server/src/test/java/org/opensearch/node/NodeRoleSettingsTests.java b/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 {
 
@@ -72,4 +73,13 @@ public void testUnknownNodeRoleOnly() {
         assertEquals(testRole, nodeRoles.get(0).roleName());
         assertEquals(testRole, nodeRoles.get(0).roleNameAbbreviation());
     }
+
+    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());
+    }
 }
diff --git a/server/src/test/resources/org/opensearch/common/settings/loader/test-settings.yml b/server/src/test/resources/org/opensearch/common/settings/loader/test-settings.yml
@@ -6,3 +6,4 @@ test1:
   test3:
     - test3-1
     - test3-2
+  test4: []
-Original file line number
+Diff line change
@@ Expand Up / @@ -6,3 +6,4 @@ test1: @@
       test3:
         - test3-1
         - test3-2
+      test4: []