Skip to content

Commit 22a9a90

Browse files
piergmpiergm
authored
Prevent data nodes from sending stack traces to coordinator when error_trace=false (#118266) (#118969)
* first iterations * added tests * Update docs/changelog/118266.yaml * constant for error_trace and typos * centralized putHeader * moved threadContext to parent class * uses NodeClient.threadpool * updated async tests to retrieve final result * moved test to avoid starting up a node * added transport version to avoid sending useless bytes * more async tests (cherry picked from commit 97bc291) # Conflicts: # server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java
1 parent fe0f14f commit 22a9a90

File tree

18 files changed

+535
-13
lines changed

18 files changed

+535
-13
lines changed

‎docs/changelog/118266.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 118266
2+
summary: Prevent data nodes from sending stack traces to coordinator when `error_trace=false`
3+
area: Search
4+
type: enhancement
5+
issues: []

‎modules/aggregations/src/internalClusterTest/java/org/elasticsearch/aggregations/bucket/SearchCancellationIT.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,8 @@ public void testCancellationDuringTimeSeriesAggregation() throws Exception {
9696
}
9797

9898
logger.info("Executing search");
99+
// we have to explicitly set error_trace=true for the later exception check for `TimeSeriesIndexSearcher`
100+
client().threadPool().getThreadContext().putHeader("error_trace", "true");
99101
TimeSeriesAggregationBuilder timeSeriesAggregationBuilder = new TimeSeriesAggregationBuilder("test_agg");
100102
ActionFuture<SearchResponse> searchResponse = prepareSearch("test").setQuery(matchAllQuery())
101103
.addAggregation(

‎qa/smoke-test-http/src/javaRestTest/java/org/elasticsearch/http/SearchErrorTraceIT.java‎

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the "Elastic License
4+
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
5+
* Public License v 1"; you may not use this file except in compliance with, at
6+
* your election, the "Elastic License 2.0", the "GNU Affero General Public
7+
* License v3.0 only", or the "Server Side Public License, v 1".
8+
*/
9+
10+
package org.elasticsearch.http;
11+
12+
import org.apache.http.entity.ContentType;
13+
import org.apache.http.nio.entity.NByteArrayEntity;
14+
import org.elasticsearch.ExceptionsHelper;
15+
import org.elasticsearch.action.search.MultiSearchRequest;
16+
import org.elasticsearch.action.search.SearchRequest;
17+
import org.elasticsearch.client.Request;
18+
import org.elasticsearch.search.builder.SearchSourceBuilder;
19+
import org.elasticsearch.transport.TransportMessageListener;
20+
import org.elasticsearch.transport.TransportService;
21+
import org.elasticsearch.xcontent.XContentType;
22+
import org.junit.Before;
23+
24+
import java.io.IOException;
25+
import java.nio.charset.Charset;
26+
import java.util.Optional;
27+
import java.util.concurrent.atomic.AtomicBoolean;
28+
29+
import static org.elasticsearch.index.query.QueryBuilders.simpleQueryStringQuery;
30+
31+
public class SearchErrorTraceIT extends HttpSmokeTestCase {
32+
private AtomicBoolean hasStackTrace;
33+
34+
@Before
35+
private void setupMessageListener() {
36+
internalCluster().getDataNodeInstances(TransportService.class).forEach(ts -> {
37+
ts.addMessageListener(new TransportMessageListener() {
38+
@Override
39+
public void onResponseSent(long requestId, String action, Exception error) {
40+
TransportMessageListener.super.onResponseSent(requestId, action, error);
41+
if (action.startsWith("indices:data/read/search")) {
42+
Optional<Throwable> throwable = ExceptionsHelper.unwrapCausesAndSuppressed(
43+
error,
44+
t -> t.getStackTrace().length > 0
45+
);
46+
hasStackTrace.set(throwable.isPresent());
47+
}
48+
}
49+
});
50+
});
51+
}
52+
53+
private void setupIndexWithDocs() {
54+
createIndex("test1", "test2");
55+
indexRandom(
56+
true,
57+
prepareIndex("test1").setId("1").setSource("field", "foo"),
58+
prepareIndex("test2").setId("10").setSource("field", 5)
59+
);
60+
refresh();
61+
}
62+
63+
public void testSearchFailingQueryErrorTraceDefault() throws IOException {
64+
hasStackTrace = new AtomicBoolean();
65+
setupIndexWithDocs();
66+
67+
Request searchRequest = new Request("POST", "/_search");
68+
searchRequest.setJsonEntity("""
69+
{
70+
"query": {
71+
"simple_query_string" : {
72+
"query": "foo",
73+
"fields": ["field"]
74+
}
75+
}
76+
}
77+
""");
78+
getRestClient().performRequest(searchRequest);
79+
assertFalse(hasStackTrace.get());
80+
}
81+
82+
public void testSearchFailingQueryErrorTraceTrue() throws IOException {
83+
hasStackTrace = new AtomicBoolean();
84+
setupIndexWithDocs();
85+
86+
Request searchRequest = new Request("POST", "/_search");
87+
searchRequest.setJsonEntity("""
88+
{
89+
"query": {
90+
"simple_query_string" : {
91+
"query": "foo",
92+
"fields": ["field"]
93+
}
94+
}
95+
}
96+
""");
97+
searchRequest.addParameter("error_trace", "true");
98+
getRestClient().performRequest(searchRequest);
99+
assertTrue(hasStackTrace.get());
100+
}
101+
102+
public void testSearchFailingQueryErrorTraceFalse() throws IOException {
103+
hasStackTrace = new AtomicBoolean();
104+
setupIndexWithDocs();
105+
106+
Request searchRequest = new Request("POST", "/_search");
107+
searchRequest.setJsonEntity("""
108+
{
109+
"query": {
110+
"simple_query_string" : {
111+
"query": "foo",
112+
"fields": ["field"]
113+
}
114+
}
115+
}
116+
""");
117+
searchRequest.addParameter("error_trace", "false");
118+
getRestClient().performRequest(searchRequest);
119+
assertFalse(hasStackTrace.get());
120+
}
121+
122+
public void testMultiSearchFailingQueryErrorTraceDefault() throws IOException {
123+
hasStackTrace = new AtomicBoolean();
124+
setupIndexWithDocs();
125+
126+
XContentType contentType = XContentType.JSON;
127+
MultiSearchRequest multiSearchRequest = new MultiSearchRequest().add(
128+
new SearchRequest("test*").source(new SearchSourceBuilder().query(simpleQueryStringQuery("foo").field("field")))
129+
);
130+
Request searchRequest = new Request("POST", "/_msearch");
131+
byte[] requestBody = MultiSearchRequest.writeMultiLineFormat(multiSearchRequest, contentType.xContent());
132+
searchRequest.setEntity(
133+
new NByteArrayEntity(requestBody, ContentType.create(contentType.mediaTypeWithoutParameters(), (Charset) null))
134+
);
135+
getRestClient().performRequest(searchRequest);
136+
assertFalse(hasStackTrace.get());
137+
}
138+
139+
public void testMultiSearchFailingQueryErrorTraceTrue() throws IOException {
140+
hasStackTrace = new AtomicBoolean();
141+
setupIndexWithDocs();
142+
143+
XContentType contentType = XContentType.JSON;
144+
MultiSearchRequest multiSearchRequest = new MultiSearchRequest().add(
145+
new SearchRequest("test*").source(new SearchSourceBuilder().query(simpleQueryStringQuery("foo").field("field")))
146+
);
147+
Request searchRequest = new Request("POST", "/_msearch");
148+
byte[] requestBody = MultiSearchRequest.writeMultiLineFormat(multiSearchRequest, contentType.xContent());
149+
searchRequest.setEntity(
150+
new NByteArrayEntity(requestBody, ContentType.create(contentType.mediaTypeWithoutParameters(), (Charset) null))
151+
);
152+
searchRequest.addParameter("error_trace", "true");
153+
getRestClient().performRequest(searchRequest);
154+
assertTrue(hasStackTrace.get());
155+
}
156+
157+
public void testMultiSearchFailingQueryErrorTraceFalse() throws IOException {
158+
hasStackTrace = new AtomicBoolean();
159+
setupIndexWithDocs();
160+
161+
XContentType contentType = XContentType.JSON;
162+
MultiSearchRequest multiSearchRequest = new MultiSearchRequest().add(
163+
new SearchRequest("test*").source(new SearchSourceBuilder().query(simpleQueryStringQuery("foo").field("field")))
164+
);
165+
Request searchRequest = new Request("POST", "/_msearch");
166+
byte[] requestBody = MultiSearchRequest.writeMultiLineFormat(multiSearchRequest, contentType.xContent());
167+
searchRequest.setEntity(
168+
new NByteArrayEntity(requestBody, ContentType.create(contentType.mediaTypeWithoutParameters(), (Charset) null))
169+
);
170+
searchRequest.addParameter("error_trace", "false");
171+
getRestClient().performRequest(searchRequest);
172+
173+
assertFalse(hasStackTrace.get());
174+
}
175+
}

‎server/src/main/java/org/elasticsearch/TransportVersions.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,7 @@ static TransportVersion def(int id) {
148148
public static final TransportVersion ESQL_QUERY_BUILDER_IN_SEARCH_FUNCTIONS = def(8_808_00_0);
149149
public static final TransportVersion EQL_ALLOW_PARTIAL_SEARCH_RESULTS = def(8_809_00_0);
150150
public static final TransportVersion NODE_VERSION_INFORMATION_WITH_MIN_READ_ONLY_INDEX_VERSION = def(8_810_00_0);
151+
public static final TransportVersion ERROR_TRACE_IN_TRANSPORT_HEADER = def(8_811_00_0);
151152

152153
/*
153154
* STOP! READ THIS FIRST! No, really,

‎server/src/main/java/org/elasticsearch/action/search/SearchTransportService.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -456,7 +456,8 @@ public static void registerRequestHandler(TransportService transportService, Sea
456456
(request, channel, task) -> searchService.executeQueryPhase(
457457
request,
458458
(SearchShardTask) task,
459-
new ChannelActionListener<>(channel)
459+
new ChannelActionListener<>(channel),
460+
channel.getVersion()
460461
)
461462
);
462463
TransportActionProxy.registerProxyAction(transportService, QUERY_ID_ACTION_NAME, true, QuerySearchResult::new);
@@ -468,7 +469,8 @@ public static void registerRequestHandler(TransportService transportService, Sea
468469
(request, channel, task) -> searchService.executeQueryPhase(
469470
request,
470471
(SearchShardTask) task,
471-
new ChannelActionListener<>(channel)
472+
new ChannelActionListener<>(channel),
473+
channel.getVersion()
472474
)
473475
);
474476
TransportActionProxy.registerProxyAction(transportService, QUERY_SCROLL_ACTION_NAME, true, ScrollQuerySearchResult::new);

‎server/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424
import org.elasticsearch.core.Releasable;
2525
import org.elasticsearch.core.Tuple;
2626
import org.elasticsearch.http.HttpTransportSettings;
27+
import org.elasticsearch.rest.RestController;
28+
import org.elasticsearch.rest.RestRequest;
2729
import org.elasticsearch.tasks.Task;
2830
import org.elasticsearch.telemetry.tracing.TraceContext;
2931

@@ -530,6 +532,17 @@ public String getHeader(String key) {
530532
return value;
531533
}
532534

535+
/**
536+
* Returns the header for the given key or defaultValue if not present
537+
*/
538+
public String getHeaderOrDefault(String key, String defaultValue) {
539+
String value = getHeader(key);
540+
if (value == null) {
541+
return defaultValue;
542+
}
543+
return value;
544+
}
545+
533546
/**
534547
* Returns all of the request headers from the thread's context.<br>
535548
* <b>Be advised, headers might contain credentials.</b>
@@ -589,6 +602,14 @@ public void putHeader(Map<String, String> header) {
589602
threadLocal.set(threadLocal.get().putHeaders(header));
590603
}
591604

605+
public void setErrorTraceTransportHeader(RestRequest r) {
606+
// set whether data nodes should send back stack trace based on the `error_trace` query parameter
607+
if (r.paramAsBoolean("error_trace", RestController.ERROR_TRACE_DEFAULT)) {
608+
// We only set it if error_trace is true (defaults to false) to avoid sending useless bytes
609+
putHeader("error_trace", "true");
610+
}
611+
}
612+
592613
/**
593614
* Puts a transient header object into this context
594615
*/

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -276,5 +276,4 @@ protected Set<String> responseParams() {
276276
protected Set<String> responseParams(RestApiVersion restApiVersion) {
277277
return responseParams();
278278
}
279-
280279
}

‎server/src/main/java/org/elasticsearch/rest/RestController.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@ public class RestController implements HttpServerTransport.Dispatcher {
9393
public static final String STATUS_CODE_KEY = "es_rest_status_code";
9494
public static final String HANDLER_NAME_KEY = "es_rest_handler_name";
9595
public static final String REQUEST_METHOD_KEY = "es_rest_request_method";
96+
public static final boolean ERROR_TRACE_DEFAULT = false;
9697

9798
static {
9899
try (InputStream stream = RestController.class.getResourceAsStream("/config/favicon.ico")) {
@@ -673,7 +674,7 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel
673674
private static void validateErrorTrace(RestRequest request, RestChannel channel) {
674675
// error_trace cannot be used when we disable detailed errors
675676
// we consume the error_trace parameter first to ensure that it is always consumed
676-
if (request.paramAsBoolean("error_trace", false) && channel.detailedErrorsEnabled() == false) {
677+
if (request.paramAsBoolean("error_trace", ERROR_TRACE_DEFAULT) && channel.detailedErrorsEnabled() == false) {
677678
throw new IllegalArgumentException("error traces in responses are disabled.");
678679
}
679680
}

‎server/src/main/java/org/elasticsearch/rest/RestResponse.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import static java.util.Collections.singletonMap;
3737
import static org.elasticsearch.ElasticsearchException.REST_EXCEPTION_SKIP_STACK_TRACE;
3838
import static org.elasticsearch.rest.RestController.ELASTIC_PRODUCT_HTTP_HEADER;
39+
import static org.elasticsearch.rest.RestController.ERROR_TRACE_DEFAULT;
3940

4041
public final class RestResponse implements Releasable {
4142

@@ -142,7 +143,7 @@ public RestResponse(RestChannel channel, RestStatus status, Exception e) throws
142143
// switched in the xcontent rendering parameters.
143144
// For authorization problems (RestStatus.UNAUTHORIZED) we don't want to do this since this could
144145
// leak information to the caller who is unauthorized to make this call
145-
if (params.paramAsBoolean("error_trace", false) && status != RestStatus.UNAUTHORIZED) {
146+
if (params.paramAsBoolean("error_trace", ERROR_TRACE_DEFAULT) && status != RestStatus.UNAUTHORIZED) {
146147
params = new ToXContent.DelegatingMapParams(singletonMap(REST_EXCEPTION_SKIP_STACK_TRACE, "false"), params);
147148
}
148149

‎server/src/main/java/org/elasticsearch/rest/action/search/RestMultiSearchAction.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,9 @@ public String getName() {
8383

8484
@Override
8585
public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
86+
if (client.threadPool() != null && client.threadPool().getThreadContext() != null) {
87+
client.threadPool().getThreadContext().setErrorTraceTransportHeader(request);
88+
}
8689
final MultiSearchRequest multiSearchRequest = parseRequest(request, allowExplicitIndex, searchUsageHolder, clusterSupportsFeature);
8790
return channel -> {
8891
final RestCancellableNodeClient cancellableClient = new RestCancellableNodeClient(client, request.getHttpChannel());

0 commit comments

Comments
 (0)