Remove direct snakeyaml dependency from launcher tools (elastic#86248)

The launcher tools currently depend on snakeyaml for doing a rough parsing of elasticsearch.yml to determine node roles for heap settings. However, the launchers (and in the future, the server-cli) already have the rest of server available on the classpath. This change removes snakeyaml and replaces it with x-content. relates elastic#85758
CohenIdo · Apr 28, 2022 · e54d0ae · e54d0ae
1 parent 97f9d29
commit e54d0ae
Show file tree

Hide file tree

Showing 8 changed files with 22 additions and 260 deletions.
diff --git a/...c/main/java/org/elasticsearch/gradle/internal/precommit/ForbiddenApisPrecommitPlugin.java b/...c/main/java/org/elasticsearch/gradle/internal/precommit/ForbiddenApisPrecommitPlugin.java
@@ -60,7 +60,6 @@ public TaskProvider<? extends Task> createTask(Project project) {
             t.copy("forbidden/hppc-signatures.txt");
             t.copy("forbidden/http-signatures.txt");
             t.copy("forbidden/es-server-signatures.txt");
-            t.copy("forbidden/snakeyaml-signatures.txt");
         });
 
         project.getExtensions().getByType(SourceSetContainer.class).configureEach(sourceSet -> {

diff --git a/build-tools-internal/src/main/resources/forbidden/snakeyaml-signatures.txt b/build-tools-internal/src/main/resources/forbidden/snakeyaml-signatures.txt
diff --git a/distribution/tools/server-cli/build.gradle b/distribution/tools/server-cli/build.gradle
@@ -10,14 +10,14 @@ import de.thetaphi.forbiddenapis.gradle.CheckForbiddenApis
 apply plugin: 'elasticsearch.build'
 
 dependencies {
-  implementation "org.yaml:snakeyaml:${versions.snakeyaml}"
-  testImplementation "com.carrotsearch.randomizedtesting:randomizedtesting-runner:${versions.randomizedrunner}"
-  testImplementation "junit:junit:${versions.junit}"
-  testImplementation "org.hamcrest:hamcrest:${versions.hamcrest}"
+  compileOnly project(":server")
+  compileOnly project(":libs:elasticsearch-cli")
+
+  testImplementation project(":test:framework")
 }
 
 tasks.withType(CheckForbiddenApis).configureEach {
-  replaceSignatureFiles 'jdk-signatures', 'snakeyaml-signatures'
+  replaceSignatureFiles 'jdk-signatures'
 }
 
 tasks.named("testingConventions").configure {

diff --git a/distribution/tools/server-cli/licenses/snakeyaml-1.26.jar.sha1 b/distribution/tools/server-cli/licenses/snakeyaml-1.26.jar.sha1
diff --git a/distribution/tools/server-cli/licenses/snakeyaml-LICENSE.txt b/distribution/tools/server-cli/licenses/snakeyaml-LICENSE.txt
diff --git a/distribution/tools/server-cli/licenses/snakeyaml-NOTICE.txt b/distribution/tools/server-cli/licenses/snakeyaml-NOTICE.txt
diff --git a/...ion/tools/server-cli/src/main/java/org/elasticsearch/server/cli/MachineDependentHeap.java b/...ion/tools/server-cli/src/main/java/org/elasticsearch/server/cli/MachineDependentHeap.java
@@ -8,9 +8,10 @@
 
 package org.elasticsearch.server.cli;
 
-import org.yaml.snakeyaml.Yaml;
-import org.yaml.snakeyaml.constructor.SafeConstructor;
-import org.yaml.snakeyaml.error.YAMLException;
+import org.elasticsearch.common.ParsingException;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.xcontent.XContentParserConfiguration;
+import org.elasticsearch.xcontent.yaml.YamlXContent;
 
 import java.io.IOException;
 import java.io.InputStream;
@@ -19,7 +20,6 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.function.Function;
@@ -86,27 +86,23 @@ static class NodeRoleParser {
 
         @SuppressWarnings("unchecked")
         public static MachineNodeRole parse(InputStream config) {
-            Yaml yaml = new Yaml(new SafeConstructor());
-            Map<String, Object> root;
+            final Settings settings;
             try {
-                root = yaml.load(config);
-            } catch (YAMLException | ClassCastException ex) {
+                var parser = YamlXContent.yamlXContent.createParser(XContentParserConfiguration.EMPTY, config);
+                if (parser.currentToken() == null && parser.nextToken() == null) {
+                    settings = null;
+                } else {
+                    settings = Settings.fromXContent(parser);
+                }
+            } catch (IOException | ParsingException ex) {
                 // Strangely formatted config, so just return defaults and let startup settings validation catch the problem
                 return MachineNodeRole.UNKNOWN;
             }
 
-            if (root != null) {
-                Map<String, Object> map = flatten(root, null);
-                List<String> roles = null;
-                try {
-                    if (map.containsKey("node.roles")) {
-                        roles = (List<String>) map.get("node.roles");
-                    }
-                } catch (ClassCastException ex) {
-                    return MachineNodeRole.UNKNOWN;
-                }
+            if (settings != null && settings.isEmpty() == false) {
+                List<String> roles = settings.getAsList("node.roles");
 
-                if (roles == null || roles.isEmpty()) {
+                if (roles.isEmpty()) {
                     // If roles are missing or empty (coordinating node) assume defaults and consider this a data node
                     return MachineNodeRole.DATA;
                 } else if (containsOnly(roles, "master")) {
@@ -121,33 +117,6 @@ public static MachineNodeRole parse(InputStream config) {
             }
         }
 
-        /**
-         * Flattens a nested configuration structure. This creates a consistent way of referencing settings from a config file that uses
-         * a mix of object and flat setting notation. The returned map is a single-level deep structure of dot-notation property names
-         * to values.
-         *
-         * <p>No attempt is made to deterministically deal with duplicate settings, nor are they explicitly disallowed.
-         *
-         * @param config nested configuration map
-         * @param parentPath parent node path or {@code null} if parsing the root node
-         * @return flattened configuration map
-         */
-        @SuppressWarnings("unchecked")
-        private static Map<String, Object> flatten(Map<String, Object> config, String parentPath) {
-            Map<String, Object> flatMap = new HashMap<>();
-            String prefix = parentPath != null ? parentPath + "." : "";
-
-            for (Map.Entry<String, Object> entry : config.entrySet()) {
-                if (entry.getValue() instanceof Map) {
-                    flatMap.putAll(flatten((Map<String, Object>) entry.getValue(), prefix + entry.getKey()));
-                } else {
-                    flatMap.put(prefix + entry.getKey(), entry.getValue());
-                }
-            }
-
-            return flatMap;
-        }
-
         @SuppressWarnings("unchecked")
         private static <T> boolean containsOnly(Collection<T> collection, T... items) {
             return Arrays.asList(items).containsAll(collection);

diff --git a/...tion/tools/server-cli/src/test/java/org/elasticsearch/server/cli/NodeRoleParserTests.java b/...tion/tools/server-cli/src/test/java/org/elasticsearch/server/cli/NodeRoleParserTests.java
@@ -86,7 +86,8 @@ public void testInvalidYaml() throws IOException {
 
     public void testInvalidRoleSyntax() throws IOException {
         MachineDependentHeap.MachineNodeRole nodeRole = parseConfig(sb -> sb.append("node.roles: foo"));
-        assertThat(nodeRole, equalTo(UNKNOWN));
+        // roles we don't know about are considered data, but will fail validation when ES starts up
+        assertThat(nodeRole, equalTo(DATA));
     }
 
     private static MachineDependentHeap.MachineNodeRole parseConfig(Consumer<StringBuilder> action) throws IOException {