From 912c6bdbfff805c5c93f22c4be41708cae76a031 Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Sat, 11 May 2019 01:58:03 +0300 Subject: [PATCH] Prevent order being lost for _nodes API filters (#42045) (#42089) * Switch to using a list instead of a Set for the filters, so that the order of these filters is kept. (cherry picked from commit 74a743829799b64971e0ac5ae265f43f6c14e074) --- .../admin/cluster/RestNodesInfoAction.java | 21 ++- .../cluster/RestNodesInfoActionTests.java | 143 ++++++++++++++++++ 2 files changed, 156 insertions(+), 8 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/rest/action/admin/cluster/RestNodesInfoActionTests.java diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesInfoAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesInfoAction.java index bacc698b2a4..20370b27d43 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesInfoAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesInfoAction.java @@ -36,7 +36,7 @@ import java.util.Set; import static org.elasticsearch.rest.RestRequest.Method.GET; public class RestNodesInfoAction extends BaseRestHandler { - private static final Set ALLOWED_METRICS = Sets.newHashSet( + static final Set ALLOWED_METRICS = Sets.newHashSet( "http", "ingest", "indices", @@ -69,6 +69,13 @@ public class RestNodesInfoAction extends BaseRestHandler { @Override public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { + final NodesInfoRequest nodesInfoRequest = prepareRequest(request); + settingsFilter.addFilterSettingParams(request); + + return channel -> client.admin().cluster().nodesInfo(nodesInfoRequest, new NodesResponseRestListener<>(channel)); + } + + static NodesInfoRequest prepareRequest(final RestRequest request) { String[] nodeIds; Set metrics; @@ -76,17 +83,18 @@ public class RestNodesInfoAction extends BaseRestHandler { // still, /_nodes/_local (or any other node id) should work and be treated as usual // this means one must differentiate between allowed metrics and arbitrary node ids in the same place if (request.hasParam("nodeId") && !request.hasParam("metrics")) { - Set metricsOrNodeIds = Strings.tokenizeByCommaToSet(request.param("nodeId", "_all")); + String nodeId = request.param("nodeId", "_all"); + Set metricsOrNodeIds = Strings.tokenizeByCommaToSet(nodeId); boolean isMetricsOnly = ALLOWED_METRICS.containsAll(metricsOrNodeIds); if (isMetricsOnly) { nodeIds = new String[]{"_all"}; metrics = metricsOrNodeIds; } else { - nodeIds = metricsOrNodeIds.toArray(new String[]{}); + nodeIds = Strings.tokenizeToStringArray(nodeId, ","); metrics = Sets.newHashSet("_all"); } } else { - nodeIds = Strings.splitStringByCommaToArray(request.param("nodeId", "_all")); + nodeIds = Strings.tokenizeToStringArray(request.param("nodeId", "_all"), ","); metrics = Strings.tokenizeByCommaToSet(request.param("metrics", "_all")); } @@ -108,10 +116,7 @@ public class RestNodesInfoAction extends BaseRestHandler { nodesInfoRequest.ingest(metrics.contains("ingest")); nodesInfoRequest.indices(metrics.contains("indices")); } - - settingsFilter.addFilterSettingParams(request); - - return channel -> client.admin().cluster().nodesInfo(nodesInfoRequest, new NodesResponseRestListener<>(channel)); + return nodesInfoRequest; } @Override diff --git a/server/src/test/java/org/elasticsearch/rest/action/admin/cluster/RestNodesInfoActionTests.java b/server/src/test/java/org/elasticsearch/rest/action/admin/cluster/RestNodesInfoActionTests.java new file mode 100644 index 00000000000..d757ee095cd --- /dev/null +++ b/server/src/test/java/org/elasticsearch/rest/action/admin/cluster/RestNodesInfoActionTests.java @@ -0,0 +1,143 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.rest.action.admin.cluster; + +import org.elasticsearch.action.admin.cluster.node.info.NodesInfoRequest; +import org.elasticsearch.rest.RestRequest; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.rest.FakeRestRequest; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.elasticsearch.rest.action.admin.cluster.RestNodesInfoAction.ALLOWED_METRICS; + +public class RestNodesInfoActionTests extends ESTestCase { + + public void testDuplicatedFiltersAreNotRemoved() { + Map params = new HashMap<>(); + params.put("nodeId", "_all,master:false,_all"); + + RestRequest restRequest = buildRestRequest(params); + NodesInfoRequest actual = RestNodesInfoAction.prepareRequest(restRequest); + assertArrayEquals(new String[] { "_all", "master:false", "_all" }, actual.nodesIds()); + } + + public void testOnlyMetrics() { + Map params = new HashMap<>(); + int metricsCount = randomIntBetween(1, ALLOWED_METRICS.size()); + List metrics = new ArrayList<>(); + + for(int i = 0; i < metricsCount; i++) { + metrics.add(randomFrom(ALLOWED_METRICS)); + } + params.put("nodeId", String.join(",", metrics)); + + RestRequest restRequest = buildRestRequest(params); + NodesInfoRequest actual = RestNodesInfoAction.prepareRequest(restRequest); + assertArrayEquals(new String[] { "_all" }, actual.nodesIds()); + assertMetrics(metrics, actual); + } + + public void testAllMetricsSelectedWhenNodeAndMetricSpecified() { + Map params = new HashMap<>(); + String nodeId = randomValueOtherThanMany(ALLOWED_METRICS::contains, () -> randomAlphaOfLength(23)); + String metric = randomFrom(ALLOWED_METRICS); + + params.put("nodeId", nodeId + "," + metric); + RestRequest restRequest = buildRestRequest(params); + + NodesInfoRequest actual = RestNodesInfoAction.prepareRequest(restRequest); + assertArrayEquals(new String[] { nodeId, metric }, actual.nodesIds()); + assertAllMetricsTrue(actual); + } + + public void testSeparateNodeIdsAndMetrics() { + Map params = new HashMap<>(); + List nodeIds = new ArrayList<>(5); + List metrics = new ArrayList<>(5); + + for(int i = 0; i < 5; i++) { + nodeIds.add(randomValueOtherThanMany(ALLOWED_METRICS::contains, () -> randomAlphaOfLength(23))); + metrics.add(randomFrom(ALLOWED_METRICS)); + } + + params.put("nodeId", String.join(",", nodeIds)); + params.put("metrics", String.join(",", metrics)); + RestRequest restRequest = buildRestRequest(params); + + NodesInfoRequest actual = RestNodesInfoAction.prepareRequest(restRequest); + assertArrayEquals(nodeIds.toArray(), actual.nodesIds()); + assertMetrics(metrics, actual); + } + + public void testExplicitAllMetrics() { + Map params = new HashMap<>(); + List nodeIds = new ArrayList<>(5); + + for(int i = 0; i < 5; i++) { + nodeIds.add(randomValueOtherThanMany(ALLOWED_METRICS::contains, () -> randomAlphaOfLength(23))); + } + + params.put("nodeId", String.join(",", nodeIds)); + params.put("metrics", "_all"); + RestRequest restRequest = buildRestRequest(params); + + NodesInfoRequest actual = RestNodesInfoAction.prepareRequest(restRequest); + assertArrayEquals(nodeIds.toArray(), actual.nodesIds()); + assertAllMetricsTrue(actual); + } + + private FakeRestRequest buildRestRequest(Map params) { + return new FakeRestRequest.Builder(xContentRegistry()) + .withMethod(RestRequest.Method.GET) + .withPath("/_nodes") + .withParams(params) + .build(); + } + + private void assertMetrics(List metrics, NodesInfoRequest nodesInfoRequest) { + assertTrue((metrics.contains("http") && nodesInfoRequest.http()) || metrics.contains("http") == false); + assertTrue((metrics.contains("ingest") && nodesInfoRequest.ingest()) || metrics.contains("ingest") == false); + assertTrue((metrics.contains("indices") && nodesInfoRequest.indices()) || metrics.contains("indices") == false); + assertTrue((metrics.contains("jvm") && nodesInfoRequest.jvm()) || metrics.contains("jvm") == false); + assertTrue((metrics.contains("os") && nodesInfoRequest.os()) || metrics.contains("os") == false); + assertTrue((metrics.contains("plugins") && nodesInfoRequest.plugins()) || metrics.contains("plugins") == false); + assertTrue((metrics.contains("process") && nodesInfoRequest.process()) || metrics.contains("process") == false); + assertTrue((metrics.contains("settings") && nodesInfoRequest.settings()) || metrics.contains("settings") == false); + assertTrue((metrics.contains("thread_pool") && nodesInfoRequest.threadPool()) || metrics.contains("thread_pool") == false); + assertTrue((metrics.contains("transport") && nodesInfoRequest.transport()) || metrics.contains("transport") == false); + } + + private void assertAllMetricsTrue(NodesInfoRequest nodesInfoRequest) { + assertTrue(nodesInfoRequest.http()); + assertTrue(nodesInfoRequest.ingest()); + assertTrue(nodesInfoRequest.indices()); + assertTrue(nodesInfoRequest.jvm()); + assertTrue(nodesInfoRequest.os()); + assertTrue(nodesInfoRequest.plugins()); + assertTrue(nodesInfoRequest.process()); + assertTrue(nodesInfoRequest.settings()); + assertTrue(nodesInfoRequest.threadPool()); + assertTrue(nodesInfoRequest.transport()); + } +}