Skip to content

Commit f5ac0e5

Browse files
jasontedorjasontedor
authored
Remove lenient stats parsing
Today when parsing a stats request, Elasticsearch silently ignores incorrect metrics. This commit removes lenient parsing of stats requests for the nodes stats and indices stats APIs. Relates #21417
1 parent 568a7ea commit f5ac0e5

File tree

10 files changed

+472
-86
lines changed

10 files changed

+472
-86
lines changed

‎core/src/main/java/org/elasticsearch/rest/BaseRestHandler.java

Lines changed: 46 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -71,49 +71,58 @@ public final void handleRequest(RestRequest request, RestChannel channel, NodeCl
7171
request.unconsumedParams().stream().filter(p -> !responseParams().contains(p)).collect(Collectors.toCollection(TreeSet::new));
7272

7373
// validate the non-response params
74-
if (unconsumedParams.isEmpty() == false) {
75-
String message = String.format(
76-
Locale.ROOT,
77-
"request [%s] contains unrecognized parameter%s: ",
78-
request.path(),
79-
unconsumedParams.size() > 1 ? "s" : "");
80-
boolean first = true;
81-
for (final String unconsumedParam : unconsumedParams) {
82-
final LevensteinDistance ld = new LevensteinDistance();
83-
final List<Tuple<Float, String>> scoredParams = new ArrayList<>();
84-
final Set<String> candidateParams = new HashSet<>();
85-
candidateParams.addAll(request.consumedParams());
86-
candidateParams.addAll(responseParams());
87-
for (final String candidateParam : candidateParams) {
88-
final float distance = ld.getDistance(unconsumedParam, candidateParam);
89-
if (distance > 0.5f) {
90-
scoredParams.add(new Tuple<>(distance, candidateParam));
91-
}
92-
}
93-
CollectionUtil.timSort(scoredParams, (a, b) -> {
94-
// sort by distance in reverse order, then parameter name for equal distances
95-
int compare = a.v1().compareTo(b.v1());
96-
if (compare != 0) return -compare;
97-
else return a.v2().compareTo(b.v2());
98-
});
99-
if (first == false) {
100-
message += ", ";
101-
}
102-
message += "[" + unconsumedParam + "]";
103-
final List<String> keys = scoredParams.stream().map(Tuple::v2).collect(Collectors.toList());
104-
if (keys.isEmpty() == false) {
105-
message += " -> did you mean " + (keys.size() == 1 ? "[" + keys.get(0) + "]": "any of " + keys.toString()) + "?";
106-
}
107-
first = false;
108-
}
109-
110-
throw new IllegalArgumentException(message);
74+
if (!unconsumedParams.isEmpty()) {
75+
final Set<String> candidateParams = new HashSet<>();
76+
candidateParams.addAll(request.consumedParams());
77+
candidateParams.addAll(responseParams());
78+
throw new IllegalArgumentException(unrecognized(request, unconsumedParams, candidateParams, "parameter"));
11179
}
11280

11381
// execute the action
11482
action.accept(channel);
11583
}
11684

85+
protected final String unrecognized(
86+
final RestRequest request,
87+
final Set<String> invalids,
88+
final Set<String> candidates,
89+
final String detail) {
90+
String message = String.format(
91+
Locale.ROOT,
92+
"request [%s] contains unrecognized %s%s: ",
93+
request.path(),
94+
detail,
95+
invalids.size() > 1 ? "s" : "");
96+
boolean first = true;
97+
for (final String invalid : invalids) {
98+
final LevensteinDistance ld = new LevensteinDistance();
99+
final List<Tuple<Float, String>> scoredParams = new ArrayList<>();
100+
for (final String candidate : candidates) {
101+
final float distance = ld.getDistance(invalid, candidate);
102+
if (distance > 0.5f) {
103+
scoredParams.add(new Tuple<>(distance, candidate));
104+
}
105+
}
106+
CollectionUtil.timSort(scoredParams, (a, b) -> {
107+
// sort by distance in reverse order, then parameter name for equal distances
108+
int compare = a.v1().compareTo(b.v1());
109+
if (compare != 0) return -compare;
110+
else return a.v2().compareTo(b.v2());
111+
});
112+
if (first == false) {
113+
message += ", ";
114+
}
115+
message += "[" + invalid + "]";
116+
final List<String> keys = scoredParams.stream().map(Tuple::v2).collect(Collectors.toList());
117+
if (keys.isEmpty() == false) {
118+
message += " -> did you mean " + (keys.size() == 1 ? "[" + keys.get(0) + "]" : "any of " + keys.toString()) + "?";
119+
}
120+
first = false;
121+
}
122+
123+
return message;
124+
}
125+
117126
/**
118127
* REST requests are handled by preparing a channel consumer that represents the execution of
119128
* the request against a channel.

‎core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesInfoAction.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ public class RestNodesInfoAction extends BaseRestHandler {
5555
public RestNodesInfoAction(Settings settings, RestController controller, SettingsFilter settingsFilter) {
5656
super(settings);
5757
controller.registerHandler(GET, "/_nodes", this);
58-
// this endpoint is used for metrics, not for nodeIds, like /_nodes/fs
58+
// this endpoint is used for metrics, not for node IDs, like /_nodes/fs
5959
controller.registerHandler(GET, "/_nodes/{nodeId}", this);
6060
controller.registerHandler(GET, "/_nodes/{nodeId}/{metrics}", this);
6161
// added this endpoint to be aligned with stats

‎core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestNodesStatsAction.java

Lines changed: 89 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,13 @@
3333

3434
import java.io.IOException;
3535
import java.util.Collections;
36+
import java.util.HashMap;
37+
import java.util.HashSet;
38+
import java.util.Locale;
39+
import java.util.Map;
3640
import java.util.Set;
41+
import java.util.TreeSet;
42+
import java.util.function.Consumer;
3743

3844
import static org.elasticsearch.rest.RestRequest.Method.GET;
3945

@@ -48,9 +54,38 @@ public RestNodesStatsAction(Settings settings, RestController controller) {
4854
controller.registerHandler(GET, "/_nodes/stats/{metric}", this);
4955
controller.registerHandler(GET, "/_nodes/{nodeId}/stats/{metric}", this);
5056

51-
controller.registerHandler(GET, "/_nodes/stats/{metric}/{indexMetric}", this);
57+
controller.registerHandler(GET, "/_nodes/stats/{metric}/{index_metric}", this);
5258

53-
controller.registerHandler(GET, "/_nodes/{nodeId}/stats/{metric}/{indexMetric}", this);
59+
controller.registerHandler(GET, "/_nodes/{nodeId}/stats/{metric}/{index_metric}", this);
60+
}
61+
62+
static final Map<String, Consumer<NodesStatsRequest>> METRICS;
63+
64+
static {
65+
final Map<String, Consumer<NodesStatsRequest>> metrics = new HashMap<>();
66+
metrics.put("os", r -> r.os(true));
67+
metrics.put("jvm", r -> r.jvm(true));
68+
metrics.put("thread_pool", r -> r.threadPool(true));
69+
metrics.put("fs", r -> r.fs(true));
70+
metrics.put("transport", r -> r.transport(true));
71+
metrics.put("http", r -> r.http(true));
72+
metrics.put("indices", r -> r.indices(true));
73+
metrics.put("process", r -> r.process(true));
74+
metrics.put("breaker", r -> r.breaker(true));
75+
metrics.put("script", r -> r.script(true));
76+
metrics.put("discovery", r -> r.discovery(true));
77+
metrics.put("ingest", r -> r.ingest(true));
78+
METRICS = Collections.unmodifiableMap(metrics);
79+
}
80+
81+
static final Map<String, Consumer<CommonStatsFlags>> FLAGS;
82+
83+
static {
84+
final Map<String, Consumer<CommonStatsFlags>> flags = new HashMap<>();
85+
for (final Flag flag : CommonStatsFlags.Flag.values()) {
86+
flags.put(flag.getRestName(), f -> f.set(flag, true));
87+
}
88+
FLAGS = Collections.unmodifiableMap(flags);
5489
}
5590

5691
@Override
@@ -62,35 +97,72 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
6297
nodesStatsRequest.timeout(request.param("timeout"));
6398

6499
if (metrics.size() == 1 && metrics.contains("_all")) {
100+
if (request.hasParam("index_metric")) {
101+
throw new IllegalArgumentException(
102+
String.format(
103+
Locale.ROOT,
104+
"request [%s] contains index metrics [%s] but all stats requested",
105+
request.path(),
106+
request.param("index_metric")));
107+
}
65108
nodesStatsRequest.all();
66109
nodesStatsRequest.indices(CommonStatsFlags.ALL);
110+
} else if (metrics.contains("_all")) {
111+
throw new IllegalArgumentException(
112+
String.format(Locale.ROOT,
113+
"request [%s] contains _all and individual metrics [%s]",
114+
request.path(),
115+
request.param("metric")));
67116
} else {
68117
nodesStatsRequest.clear();
69-
nodesStatsRequest.os(metrics.contains("os"));
70-
nodesStatsRequest.jvm(metrics.contains("jvm"));
71-
nodesStatsRequest.threadPool(metrics.contains("thread_pool"));
72-
nodesStatsRequest.fs(metrics.contains("fs"));
73-
nodesStatsRequest.transport(metrics.contains("transport"));
74-
nodesStatsRequest.http(metrics.contains("http"));
75-
nodesStatsRequest.indices(metrics.contains("indices"));
76-
nodesStatsRequest.process(metrics.contains("process"));
77-
nodesStatsRequest.breaker(metrics.contains("breaker"));
78-
nodesStatsRequest.script(metrics.contains("script"));
79-
nodesStatsRequest.discovery(metrics.contains("discovery"));
80-
nodesStatsRequest.ingest(metrics.contains("ingest"));
118+
119+
// use a sorted set so the unrecognized parameters appear in a reliable sorted order
120+
final Set<String> invalidMetrics = new TreeSet<>();
121+
for (final String metric : metrics) {
122+
final Consumer<NodesStatsRequest> handler = METRICS.get(metric);
123+
if (handler != null) {
124+
handler.accept(nodesStatsRequest);
125+
} else {
126+
invalidMetrics.add(metric);
127+
}
128+
}
129+
130+
if (!invalidMetrics.isEmpty()) {
131+
throw new IllegalArgumentException(unrecognized(request, invalidMetrics, METRICS.keySet(), "metric"));
132+
}
81133

82134
// check for index specific metrics
83135
if (metrics.contains("indices")) {
84-
Set<String> indexMetrics = Strings.splitStringByCommaToSet(request.param("indexMetric", "_all"));
136+
Set<String> indexMetrics = Strings.splitStringByCommaToSet(request.param("index_metric", "_all"));
85137
if (indexMetrics.size() == 1 && indexMetrics.contains("_all")) {
86138
nodesStatsRequest.indices(CommonStatsFlags.ALL);
87139
} else {
88140
CommonStatsFlags flags = new CommonStatsFlags();
89-
for (Flag flag : CommonStatsFlags.Flag.values()) {
90-
flags.set(flag, indexMetrics.contains(flag.getRestName()));
141+
flags.clear();
142+
// use a sorted set so the unrecognized parameters appear in a reliable sorted order
143+
final Set<String> invalidIndexMetrics = new TreeSet<>();
144+
for (final String indexMetric : indexMetrics) {
145+
final Consumer<CommonStatsFlags> handler = FLAGS.get(indexMetric);
146+
if (handler != null) {
147+
handler.accept(flags);
148+
} else {
149+
invalidIndexMetrics.add(indexMetric);
150+
}
151+
}
152+
153+
if (!invalidIndexMetrics.isEmpty()) {
154+
throw new IllegalArgumentException(unrecognized(request, invalidIndexMetrics, FLAGS.keySet(), "index metric"));
91155
}
156+
92157
nodesStatsRequest.indices(flags);
93158
}
159+
} else if (request.hasParam("index_metric")) {
160+
throw new IllegalArgumentException(
161+
String.format(
162+
Locale.ROOT,
163+
"request [%s] contains index metrics [%s] but indices stats not requested",
164+
request.path(),
165+
request.param("index_metric")));
94166
}
95167
}
96168

‎core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestIndicesStatsAction.java

Lines changed: 50 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,13 @@
3636

3737
import java.io.IOException;
3838
import java.util.Collections;
39+
import java.util.HashMap;
40+
import java.util.HashSet;
41+
import java.util.Locale;
42+
import java.util.Map;
3943
import java.util.Set;
44+
import java.util.TreeSet;
45+
import java.util.function.Consumer;
4046

4147
import static org.elasticsearch.rest.RestRequest.Method.GET;
4248
import static org.elasticsearch.rest.RestStatus.OK;
@@ -49,11 +55,34 @@ public RestIndicesStatsAction(Settings settings, RestController controller) {
4955
super(settings);
5056
controller.registerHandler(GET, "/_stats", this);
5157
controller.registerHandler(GET, "/_stats/{metric}", this);
52-
controller.registerHandler(GET, "/_stats/{metric}/{indexMetric}", this);
5358
controller.registerHandler(GET, "/{index}/_stats", this);
5459
controller.registerHandler(GET, "/{index}/_stats/{metric}", this);
5560
}
5661

62+
static Map<String, Consumer<IndicesStatsRequest>> METRICS;
63+
64+
static {
65+
final Map<String, Consumer<IndicesStatsRequest>> metrics = new HashMap<>();
66+
metrics.put("docs", r -> r.docs(true));
67+
metrics.put("store", r -> r.store(true));
68+
metrics.put("indexing", r -> r.indexing(true));
69+
metrics.put("search", r -> r.search(true));
70+
metrics.put("suggest", r -> r.search(true));
71+
metrics.put("get", r -> r.get(true));
72+
metrics.put("merge", r -> r.merge(true));
73+
metrics.put("refresh", r -> r.refresh(true));
74+
metrics.put("flush", r -> r.flush(true));
75+
metrics.put("warmer", r -> r.warmer(true));
76+
metrics.put("query_cache", r -> r.queryCache(true));
77+
metrics.put("segments", r -> r.segments(true));
78+
metrics.put("fielddata", r -> r.fieldData(true));
79+
metrics.put("completion", r -> r.completion(true));
80+
metrics.put("request_cache", r -> r.requestCache(true));
81+
metrics.put("recovery", r -> r.recovery(true));
82+
metrics.put("translog", r -> r.translog(true));
83+
METRICS = Collections.unmodifiableMap(metrics);
84+
}
85+
5786
@Override
5887
public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
5988
IndicesStatsRequest indicesStatsRequest = new IndicesStatsRequest();
@@ -65,24 +94,28 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
6594
// short cut, if no metrics have been specified in URI
6695
if (metrics.size() == 1 && metrics.contains("_all")) {
6796
indicesStatsRequest.all();
97+
} else if (metrics.contains("_all")) {
98+
throw new IllegalArgumentException(
99+
String.format(Locale.ROOT,
100+
"request [%s] contains _all and individual metrics [%s]",
101+
request.path(),
102+
request.param("metric")));
68103
} else {
69104
indicesStatsRequest.clear();
70-
indicesStatsRequest.docs(metrics.contains("docs"));
71-
indicesStatsRequest.store(metrics.contains("store"));
72-
indicesStatsRequest.indexing(metrics.contains("indexing"));
73-
indicesStatsRequest.search(metrics.contains("search") || metrics.contains("suggest"));
74-
indicesStatsRequest.get(metrics.contains("get"));
75-
indicesStatsRequest.merge(metrics.contains("merge"));
76-
indicesStatsRequest.refresh(metrics.contains("refresh"));
77-
indicesStatsRequest.flush(metrics.contains("flush"));
78-
indicesStatsRequest.warmer(metrics.contains("warmer"));
79-
indicesStatsRequest.queryCache(metrics.contains("query_cache"));
80-
indicesStatsRequest.segments(metrics.contains("segments"));
81-
indicesStatsRequest.fieldData(metrics.contains("fielddata"));
82-
indicesStatsRequest.completion(metrics.contains("completion"));
83-
indicesStatsRequest.requestCache(metrics.contains("request_cache"));
84-
indicesStatsRequest.recovery(metrics.contains("recovery"));
85-
indicesStatsRequest.translog(metrics.contains("translog"));
105+
// use a sorted set so the unrecognized parameters appear in a reliable sorted order
106+
final Set<String> invalidMetrics = new TreeSet<>();
107+
for (final String metric : metrics) {
108+
final Consumer<IndicesStatsRequest> consumer = METRICS.get(metric);
109+
if (consumer != null) {
110+
consumer.accept(indicesStatsRequest);
111+
} else {
112+
invalidMetrics.add(metric);
113+
}
114+
}
115+
116+
if (!invalidMetrics.isEmpty()) {
117+
throw new IllegalArgumentException(unrecognized(request, invalidMetrics, METRICS.keySet(), "metric"));
118+
}
86119
}
87120

88121
if (request.hasParam("groups")) {

0 commit comments

Comments
 (0)