Fix roles parsing in client nodes sniffer (#52888)

jasontedor · jasontedor · commit a57778c2fe3b · 2020-02-27T15:28:26.000-05:00
We mades roles pluggable, but never updated the client to account for
this. This means that when speaking to a modern cluster, application
logs are spammed with warning messages around unrecognized roles. This
commit addresses this by accounting for the fact that roles can extend
beyond master/data/ingest now.
diff --git a/client/rest/src/main/java/org/elasticsearch/client/Node.java b/client/rest/src/main/java/org/elasticsearch/client/Node.java
@@ -19,12 +19,13 @@
 
 package org.elasticsearch.client;
 
+import org.apache.http.HttpHost;
+
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
-
-import org.apache.http.HttpHost;
+import java.util.TreeSet;
 
 /**
  * Metadata about an {@link HttpHost} running Elasticsearch.
@@ -175,42 +176,35 @@ public int hashCode() {
      * Role information about an Elasticsearch process.
      */
     public static final class Roles {
-        private final boolean masterEligible;
-        private final boolean data;
-        private final boolean ingest;
-
-        public Roles(boolean masterEligible, boolean data, boolean ingest) {
-            this.masterEligible = masterEligible;
-            this.data = data;
-            this.ingest = ingest;
+
+        private final Set<String> roles;
+
+        public Roles(final Set<String> roles) {
+            this.roles = new TreeSet<>(roles);
         }
 
         /**
          * Teturns whether or not the node <strong>could</strong> be elected master.
          */
         public boolean isMasterEligible() {
-            return masterEligible;
+            return roles.contains("master");
         }
         /**
          * Teturns whether or not the node stores data.
          */
         public boolean isData() {
-            return data;
+            return roles.contains("data");
         }
         /**
          * Teturns whether or not the node runs ingest pipelines.
          */
         public boolean isIngest() {
-            return ingest;
+            return roles.contains("ingest");
         }
 
         @Override
         public String toString() {
-            StringBuilder result = new StringBuilder(3);
-            if (masterEligible) result.append('m');
-            if (data) result.append('d');
-            if (ingest) result.append('i');
-            return result.toString();
+            return String.join(",", roles);
         }
 
         @Override
@@ -219,14 +213,13 @@ public boolean equals(Object obj) {
                 return false;
             }
             Roles other = (Roles) obj;
-            return masterEligible == other.masterEligible
-                && data == other.data
-                && ingest == other.ingest;
+            return roles.equals(other.roles);
         }
 
         @Override
         public int hashCode() {
-            return Objects.hash(masterEligible, data, ingest);
+            return roles.hashCode();
         }
+
     }
 }
diff --git a/client/rest/src/test/java/org/elasticsearch/client/HasAttributeNodeSelectorTests.java b/client/rest/src/test/java/org/elasticsearch/client/HasAttributeNodeSelectorTests.java
@@ -27,6 +27,8 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
+import java.util.TreeSet;
 
 import static java.util.Collections.singletonList;
 import static java.util.Collections.singletonMap;
@@ -51,9 +53,19 @@ public void testHasAttribute() {
     }
 
     private static Node dummyNode(Map<String, List<String>> attributes) {
+        final Set<String> roles = new TreeSet<>();
+        if (randomBoolean()) {
+            roles.add("master");
+        }
+        if (randomBoolean()) {
+            roles.add("data");
+        }
+        if (randomBoolean()) {
+            roles.add("ingest");
+        }
         return new Node(new HttpHost("dummy"), Collections.<HttpHost>emptySet(),
                 randomAsciiAlphanumOfLength(5), randomAsciiAlphanumOfLength(5),
-                new Roles(randomBoolean(), randomBoolean(), randomBoolean()),
+                new Roles(roles),
                 attributes);
     }
 }
diff --git a/client/rest/src/test/java/org/elasticsearch/client/NodeSelectorTests.java b/client/rest/src/test/java/org/elasticsearch/client/NodeSelectorTests.java
@@ -25,6 +25,8 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
+import java.util.Set;
+import java.util.TreeSet;
 
 import static org.junit.Assert.assertEquals;
 
@@ -64,9 +66,19 @@ public void testNotMasterOnly() {
     }
 
     private static Node dummyNode(boolean master, boolean data, boolean ingest) {
+        final Set<String> roles = new TreeSet<>();
+        if (master) {
+            roles.add("master");
+        }
+        if (data) {
+            roles.add("data");
+        }
+        if (ingest) {
+            roles.add("ingest");
+        }
         return new Node(new HttpHost("dummy"), Collections.<HttpHost>emptySet(),
                 randomAsciiAlphanumOfLength(5), randomAsciiAlphanumOfLength(5),
-                new Roles(master, data, ingest),
+                new Roles(roles),
                 Collections.<String, List<String>>emptyMap());
     }
 }
diff --git a/client/rest/src/test/java/org/elasticsearch/client/NodeTests.java b/client/rest/src/test/java/org/elasticsearch/client/NodeTests.java
@@ -23,10 +23,12 @@
 import org.elasticsearch.client.Node.Roles;
 
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.TreeSet;
 
 import static java.util.Collections.singleton;
 import static java.util.Collections.singletonList;
@@ -43,19 +45,19 @@ public void testToString() {
         assertEquals("[host=http://1]", new Node(new HttpHost("1")).toString());
         assertEquals("[host=http://1, attributes={foo=[bar], baz=[bort, zoom]}]",
                 new Node(new HttpHost("1"), null, null, null, null, attributes).toString());
-        assertEquals("[host=http://1, roles=mdi]", new Node(new HttpHost("1"),
-                null, null, null, new Roles(true, true, true), null).toString());
+        assertEquals("[host=http://1, roles=data,ingest,master]", new Node(new HttpHost("1"),
+                null, null, null, new Roles(new TreeSet<>(Arrays.asList("master", "data", "ingest"))), null).toString());
         assertEquals("[host=http://1, version=ver]", new Node(new HttpHost("1"),
                 null, null, "ver", null, null).toString());
         assertEquals("[host=http://1, name=nam]", new Node(new HttpHost("1"),
                 null, "nam", null, null, null).toString());
         assertEquals("[host=http://1, bound=[http://1, http://2]]", new Node(new HttpHost("1"),
                 new HashSet<>(Arrays.asList(new HttpHost("1"), new HttpHost("2"))), null, null, null, null).toString());
         assertEquals(
-                "[host=http://1, bound=[http://1, http://2], name=nam, version=ver, roles=m, attributes={foo=[bar], baz=[bort, zoom]}]",
+                "[host=http://1, bound=[http://1, http://2], "
+                    + "name=nam, version=ver, roles=master, attributes={foo=[bar], baz=[bort, zoom]}]",
                 new Node(new HttpHost("1"), new HashSet<>(Arrays.asList(new HttpHost("1"), new HttpHost("2"))),
-                    "nam", "ver", new Roles(true, false, false), attributes).toString());
-
+                    "nam", "ver", new Roles(Collections.singleton("master")), attributes).toString());
     }
 
     public void testEqualsAndHashCode() {
@@ -64,7 +66,7 @@ public void testEqualsAndHashCode() {
                 randomBoolean() ? null : singleton(host),
                 randomBoolean() ? null : randomAsciiAlphanumOfLength(5),
                 randomBoolean() ? null : randomAsciiAlphanumOfLength(5),
-                randomBoolean() ? null : new Roles(true, true, true),
+                randomBoolean() ? null : new Roles(new TreeSet<>(Arrays.asList("master", "data", "ingest"))),
                 randomBoolean() ? null : singletonMap("foo", singletonList("bar")));
         assertFalse(node.equals(null));
         assertTrue(node.equals(node));
@@ -82,7 +84,7 @@ public void testEqualsAndHashCode() {
         assertFalse(node.equals(new Node(host, node.getBoundHosts(), node.getName(),
                 node.getVersion() + "changed", node.getRoles(), node.getAttributes())));
         assertFalse(node.equals(new Node(host, node.getBoundHosts(), node.getName(),
-                node.getVersion(), new Roles(false, false, false), node.getAttributes())));
+                node.getVersion(), new Roles(Collections.emptySet()), node.getAttributes())));
                 assertFalse(node.equals(new Node(host, node.getBoundHosts(), node.getName(),
                 node.getVersion(), node.getRoles(), singletonMap("bort", singletonList("bing")))));
     }
diff --git a/client/rest/src/test/java/org/elasticsearch/client/PreferHasAttributeNodeSelectorTests.java b/client/rest/src/test/java/org/elasticsearch/client/PreferHasAttributeNodeSelectorTests.java
@@ -27,6 +27,8 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
+import java.util.TreeSet;
 
 import static java.util.Collections.singletonList;
 import static java.util.Collections.singletonMap;
@@ -61,9 +63,19 @@ public void testNotFoundPreferHasAttribute() {
     }
 
     private static Node dummyNode(Map<String, List<String>> attributes) {
+        final Set<String> roles = new TreeSet<>();
+        if (randomBoolean()) {
+            roles.add("master");
+        }
+        if (randomBoolean()) {
+            roles.add("data");
+        }
+        if (randomBoolean()) {
+            roles.add("ingest");
+        }
         return new Node(new HttpHost("dummy"), Collections.<HttpHost>emptySet(),
             randomAsciiAlphanumOfLength(5), randomAsciiAlphanumOfLength(5),
-            new Roles(randomBoolean(), randomBoolean(), randomBoolean()),
+            new Roles(roles),
             attributes);
     }
 }
diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsTests.java
@@ -27,11 +27,13 @@
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Set;
+import java.util.TreeSet;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 
@@ -271,7 +273,9 @@ public void testSetNodes() throws Exception {
         RestClient restClient = createRestClient(NodeSelector.SKIP_DEDICATED_MASTERS);
         List<Node> newNodes = new ArrayList<>(nodes.size());
         for (int i = 0; i < nodes.size(); i++) {
-            Node.Roles roles = i == 0 ? new Node.Roles(false, true, true) : new Node.Roles(true, false, false);
+            Node.Roles roles = i == 0 ?
+                new Node.Roles(new TreeSet<>(Arrays.asList("data", "ingest"))) :
+                new Node.Roles(new TreeSet<>(Arrays.asList("master")));
             newNodes.add(new Node(nodes.get(i).getHost(), null, null, null, roles, null));
         }
         restClient.setNodes(newNodes);
diff --git a/client/sniffer/src/main/java/org/elasticsearch/client/sniff/ElasticsearchNodesSniffer.java b/client/sniffer/src/main/java/org/elasticsearch/client/sniff/ElasticsearchNodesSniffer.java
@@ -27,10 +27,10 @@
 import org.apache.http.HttpEntity;
 import org.apache.http.HttpHost;
 import org.elasticsearch.client.Node;
+import org.elasticsearch.client.Node.Roles;
 import org.elasticsearch.client.Request;
 import org.elasticsearch.client.Response;
 import org.elasticsearch.client.RestClient;
-import org.elasticsearch.client.Node.Roles;
 
 import java.io.IOException;
 import java.io.InputStream;
@@ -42,6 +42,7 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import java.util.TreeSet;
 import java.util.concurrent.TimeUnit;
 
 import static java.util.Collections.singletonList;
@@ -152,9 +153,7 @@ private static Node readNode(String nodeId, JsonParser parser, Scheme scheme) th
         final Map<String, String> protoAttributes = new HashMap<String, String>();
 
         boolean sawRoles = false;
-        boolean master = false;
-        boolean data = false;
-        boolean ingest = false;
+        final Set<String> roles = new TreeSet<>();
 
         String fieldName = null;
         while (parser.nextToken() != JsonToken.END_OBJECT) {
@@ -207,19 +206,7 @@ private static Node readNode(String nodeId, JsonParser parser, Scheme scheme) th
                 if ("roles".equals(fieldName)) {
                     sawRoles = true;
                     while (parser.nextToken() != JsonToken.END_ARRAY) {
-                        switch (parser.getText()) {
-                        case "master":
-                            master = true;
-                            break;
-                        case "data":
-                            data = true;
-                            break;
-                        case "ingest":
-                            ingest = true;
-                            break;
-                        default:
-                            logger.warn("unknown role [" + parser.getText() + "] on node [" + nodeId + "]");
-                        }
+                        roles.add(parser.getText());
                     }
                 } else {
                     parser.skipChildren();
@@ -268,15 +255,19 @@ private static Node readNode(String nodeId, JsonParser parser, Scheme scheme) th
             boolean clientAttribute = v2RoleAttributeValue(realAttributes, "client", false);
             Boolean masterAttribute = v2RoleAttributeValue(realAttributes, "master", null);
             Boolean dataAttribute = v2RoleAttributeValue(realAttributes, "data", null);
-            master = masterAttribute == null ? false == clientAttribute : masterAttribute;
-            data = dataAttribute == null ? false == clientAttribute : dataAttribute;
+            if ((masterAttribute == null && false == clientAttribute) || masterAttribute) {
+                roles.add("master");
+            }
+            if ((dataAttribute == null && false == clientAttribute) || dataAttribute) {
+                roles.add("data");
+            }
         } else {
             assert sawRoles : "didn't see roles for [" + nodeId + "]";
         }
         assert boundHosts.contains(publishedHost) :
                 "[" + nodeId + "] doesn't make sense! publishedHost should be in boundHosts";
         logger.trace("adding node [" + nodeId + "]");
-        return new Node(publishedHost, boundHosts, name, version, new Roles(master, data, ingest),
+        return new Node(publishedHost, boundHosts, name, version, new Roles(roles),
                 unmodifiableMap(realAttributes));
     }
 
diff --git a/client/sniffer/src/test/java/org/elasticsearch/client/sniff/ElasticsearchNodesSnifferParseTests.java b/client/sniffer/src/test/java/org/elasticsearch/client/sniff/ElasticsearchNodesSnifferParseTests.java
diff --git a/client/sniffer/src/test/java/org/elasticsearch/client/sniff/ElasticsearchNodesSnifferTests.java b/client/sniffer/src/test/java/org/elasticsearch/client/sniff/ElasticsearchNodesSnifferTests.java
diff --git a/client/sniffer/src/test/resources/7.3.0_nodes_http.json b/client/sniffer/src/test/resources/7.3.0_nodes_http.json
