Log errors in RestResponse regardless of error_trace parameter (elast…

…ic#101066)

Currently we only log exceptions in RestResponse when the request parameter
"error_trace" isn't set or set to "false". If the client requests traces in the
rest response, we skip the server side logging. In order to improve visibility
of those errors in our logs this changes the logging behaviour to only depend on
the "rest.exception.stacktrace.skip" which is only used internally.

Closes elastic#100884

docs/changelog/101066.yaml

@@ -0,0 +1,6 @@
+pr: 101066
+summary: Log errors in `RestResponse` regardless of `error_trace` parameter
+area: "Infra/Core"
+type: enhancement
+issues:
+ - 100884

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

@@ -33,7 +33,6 @@
 
 import static java.util.Collections.singletonMap;
 import static org.elasticsearch.ElasticsearchException.REST_EXCEPTION_SKIP_STACK_TRACE;
-import static org.elasticsearch.ElasticsearchException.REST_EXCEPTION_SKIP_STACK_TRACE_DEFAULT;
 import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
 import static org.elasticsearch.rest.RestController.ELASTIC_PRODUCT_HTTP_HEADER;
 
@@ -115,9 +114,8 @@ public RestResponse(RestChannel channel, Exception e) throws IOException {
     @SuppressWarnings("this-escape")
     public RestResponse(RestChannel channel, RestStatus status, Exception e) throws IOException {
         this.status = status;
-        ToXContent.Params params = paramsFromRequest(channel.request());
-        if (params.paramAsBoolean(REST_EXCEPTION_SKIP_STACK_TRACE, REST_EXCEPTION_SKIP_STACK_TRACE_DEFAULT) && e != null) {
-            // log exception only if it is not returned in the response
+        ToXContent.Params params = channel.request();
+        if (e != null) {
             Supplier<?> messageSupplier = () -> String.format(
                 Locale.ROOT,
                 "path: %s, params: %s, status: %d",
@@ -131,6 +129,14 @@ public RestResponse(RestChannel channel, RestStatus status, Exception e) throws
                 SUPPRESSED_ERROR_LOGGER.warn(messageSupplier, e);
             }
         }
+        // if "error_trace" is turned on in the request, we want to render it in the rest response
+        // for that the REST_EXCEPTION_SKIP_STACK_TRACE flag that if "true" omits the stack traces is
+        // switched in the xcontent rendering parameters.
+        // For authorization problems (RestStatus.UNAUTHORIZED) we don't want to do this since this could
+        // leak information to the caller who is unauthorized to make this call
+        if (params.paramAsBoolean("error_trace", false) && status != RestStatus.UNAUTHORIZED) {
+            params = new ToXContent.DelegatingMapParams(singletonMap(REST_EXCEPTION_SKIP_STACK_TRACE, "false"), params);
+        }
         try (XContentBuilder builder = channel.newErrorBuilder()) {
             build(builder, params, status, channel.detailedErrorsEnabled(), e);
             this.content = BytesReference.bytes(builder);
@@ -164,18 +170,6 @@ public RestStatus status() {
         return this.status;
     }
 
-    private ToXContent.Params paramsFromRequest(RestRequest restRequest) {
-        ToXContent.Params params = restRequest;
-        if (restRequest.paramAsBoolean("error_trace", REST_EXCEPTION_SKIP_STACK_TRACE_DEFAULT == false) && skipStackTrace() == false) {
-            params = new ToXContent.DelegatingMapParams(singletonMap(REST_EXCEPTION_SKIP_STACK_TRACE, "false"), params);
-        }
-        return params;
-    }
-
-    protected boolean skipStackTrace() {
-        return status() == RestStatus.UNAUTHORIZED;
-    }
-
     private static void build(
         XContentBuilder builder,
         ToXContent.Params params,

server/src/test/java/org/elasticsearch/rest/RestResponseTests.java

@@ -476,14 +476,10 @@ public void testSupressedLogging() throws IOException {
             "unauthorized"
         );
 
-        // setting "rest.exception.stacktrace.skip" to false should prevent logging to happen
-        request.params().put(REST_EXCEPTION_SKIP_STACK_TRACE, "false");
-        assertLogging(channel, new ElasticsearchException("simulated"), null, null, null);
-        // setting "error_trace" to true currently also prevents logging
+        // setting "error_trace" to true should not affect logging
         request.params().clear();
         request.params().put("error_trace", "true");
-        assertLogging(channel, new ElasticsearchException("simulated"), null, null, null);
-        // we still seem to log 401s though, regardless of "error_trace" setting
+        assertLogging(channel, new ElasticsearchException("simulated"), Level.WARN, "500", "simulated");
         assertLogging(
             channel,
             new ElasticsearchSecurityException("unauthorized", RestStatus.UNAUTHORIZED),