From 5b505ec04c2684867901d1cabfed8431d7d34fe3 Mon Sep 17 00:00:00 2001 From: Jeongmin Yu Date: Thu, 16 Nov 2023 07:11:14 +0900 Subject: [PATCH] Treat Setting value with empty array string as empty array (#10625) * Treat Setting value with empty array string as empty array Signed-off-by: Jeongmin Yu * Allow to pass the list settings through environment variables (like [], ["a", "b", "c"], ...) Signed-off-by: Andriy Redko --------- Signed-off-by: Jeongmin Yu Signed-off-by: Andriy Redko Co-authored-by: Andriy Redko --- CHANGELOG.md | 1 + server/build.gradle | 1 + .../opensearch/common/settings/Settings.java | 39 ++++++++++++++++++- .../common/settings/SettingsTests.java | 24 ++++++++++++ .../node/NodeRoleSettingsTests.java | 10 +++++ .../common/settings/loader/test-settings.yml | 1 + 6 files changed, 74 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8604f0e2914ee..a75463ff16a08 100644 --- 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 index fa8a44ef6fc94..17b8ff0469ad8 100644 --- 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 index 0557884f0f8ad..9da47ff3aa700 100644 --- 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> 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> 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 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 index af4efabb341ee..669d40f40bb2c 100644 --- 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 index 0a3af34bc12f4..b2bb6897fe164 100644 --- 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 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 index b533ae036e758..1bffbc18e83e6 100644 --- 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: []