Add new config to filter internal Druid-related messages from Query API response by maytasm · Pull Request #11711 · apache/druid

maytasm · 2021-09-15T09:17:00Z

Add new config to filter internal Druid-related messages from Query API response

Description

This PR adds the following new config: druid.server.http.sanitizeJettyErrorResponse and druid.server.http.sanitizeDruidErrorResponse.strategy

These configs will allow Druid API responses to hide internal information (class name, stack trace, internal concept, thread name / servlet name, code, line/column number, host/ip). This is useful for when we have external users using Druid query API.

For the initial functionality of this feature, we will handle the following Query API responses:

Jetty layer / Jetty filter (authentication filter, cannot route/connect to downstream services, limit request, http allow method, etc.)
In AsyncQueryForwardingServlet of the Router
SqlResource (/druid/v2/sql/ POST endpoint)

The effect of this feature for each layer is described below:

Without druid.server.http.sanitizeJettyErrorResponse enabled, any error in the Jetty layer / Jetty filter will have "servlet", "message", "url", "status" and "cause" fields in the JSON response. With druid.server.http.sanitizeJettyErrorResponse enabled, the JSON response will only contains "message", "url", and "status". Note that the value of these fields remain unchanged.
AsyncQueryForwardingServlet of the Router error Response is changed from a map containing only "error" field to the same format as SqlResource/QueryResource (using QueryException). The error response will have the field "error", "errorMessage", "errorClass", and "host" (as described in https://druid.apache.org/docs/latest/querying/querying.html#query-execution-failures). This change apply to both when the new config is not set and when set.
Without druid.server.http.sanitizeDruidErrorResponse.strategy set or set to none, any error in AsyncQueryForwardingServlet of the Router and SqlResource (/druid/v2/sql/ POST endpoint) will have the field "error", "errorMessage", "errorClass", and "host" (as described in https://druid.apache.org/docs/latest/querying/querying.html#query-execution-failures). When druid.server.http.sanitizeDruidErrorResponse.strategy is set, the field "errorClass", and "host" will always be null. Moreover, the field "errorMessage" will be transformed based on the set strategy. Currently, this PR only includes one strategy which is based on allowedRegex. Basically, the value of the field "errorMessage" will be checked against a list of regular expressions in druid.server.http.sanitizeDruidErrorResponse.allowedRegex. If the value match any of the regular expressions, then it will be included in the response, otherwise it will be null. Note that the value of the field "error" will remain unchanged. (Note that prior to this PR "errorMessage", "errorClass", and "host" can be null)

Future work:
We can reuse this config and apply it to other Query API such as Native query and JDBC.

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added documentation for new or modified features or behaviors.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
added integration tests.
been tested in a test Druid cluster.

clintropolis

concept makes sense, have some questions/suggestions about longer term stuff;

this will need docs i think too once configs settle

clintropolis · 2021-09-16T08:04:01Z

  }
+
+  @VisibleForTesting
+  String applyErrorMessageFilter(final String errorMessage, final List<Pattern> whitelistRegex)


Maybe this should be a utility method on QueryException or someplace similar so it can be better shared for when JDBC and native JSON queries are added (also a few other places i see this .stream().noneMatch(pattern -> pattern.matcher(errorMessage).matches())) pattern in this PR that could probably be shared). If you imagine this will only ever be done to queries, maybe it makes sense to just bake method into QueryException so that all implementations just know how to trim the information they contain, then everything just has to make sure they throw some sort of QueryException and everything should be good.

Thinking further though, if this functionality only applies to queries, do we really need to use regex? Could we instead just configure a set of exception message by errorCode or maybe the classes of QueryException (not errorClass, but like java class name) to allow, and anything that doesn't match would be sanitized? I worry that regex based on the error messages might be a bit tricky/steep learning curve, and if we go this way we should probably update the documents to include several examples of the types of messages that query errors can encounter, and example configs.

Moved to a common class so that we can reuse in multiple places.

clintropolis · 2021-09-16T08:07:29Z

+        && serverConfig.getResponseWhitelistRegex().stream().noneMatch(pattern -> pattern.matcher(errorMessage).matches())) {
+      objectMapper.writeValue(
+          response.getOutputStream(),
+          ImmutableMap.of("error", DEFAULT_QUERY_PARSE_EXCEPTION_MESSAGE)


Maybe it is finally time to make a basic ErrorResponse class to capture this ImmutableMap.of("error", ...) pattern, searching the code-base I see it in a lot of places...

On the other hand, it is somewhat inconsistent with QueryException though, which uses error for the errorCode and has separate message, class, and some additional information. Since this is specifically forwarding queries, I wonder if this should be a type of QueryException? (Especially if we bake a method into QueryException to make them clean themselves up if configured as such)

The response format here is inconsistent with QueryException. I am not sure why this was done this way. However, in this PR my intention is to keep the response format the same as original. Notice, that this PR does not add any new field in the response format, only remove fields that could already be null, and change string message for existing fields.

However, both Exception thrown here and the Exception from further downstream which uses QueryException already make the response inconsistent for the same API call. Since, the format of QueryException is well documented, I think we should change this to use QueryException too.

clintropolis · 2021-09-16T08:12:32Z


+  @JsonProperty
+  @NotNull
+  private List<Pattern> responseWhitelistRegex = ImmutableList.of();


this config applies to the http servers, not queries specifically, so I think if it is going to live here it might need a name that indicates it only applies to queries, unless you imagine that at some point this setting will apply to all HTTP error responses?

Alternatively, if it lived on a more query centric config object this wouldn't be necessary, though i'm not quite sure off the top of my head what is most appropriate.

I imagine the setting for filterResponse will apply to all HTTP error responses. Currently it does apply to API outside of query APIs if the error is thrown in the Jetty filter layer. For example, if you access non-query API with invalid auth or an invalid API endpoint.

The setting for responseWhitelistRegex...I am not sure yet. I think it can apply to all HTTP error responses. Another idea I had was instead of using a whitelist, we can use a blacklist regex. Then it would be simpler to blacklist with regex like org\.apache\..* and java\..* to filter out stack traces. This would then be easy to apply to all HTTP error responses.

clintropolis · 2021-09-16T08:25:24Z

marking design review because it both adds new config and said config changes API responses 😅

clintropolis

this seems pretty flexible/powerful to potentially give operators a lot more control over error responses 👍

clintropolis · 2021-09-20T22:56:10Z

+        }
+    );
+
+    String errorMessage = "This will be support in Druid 9999";


clintropolis · 2021-09-20T23:30:44Z


      if (e instanceof RelOptPlanner.CannotPlanException) {
-        exceptionToReport = new ISE("Cannot build plan for query: %s", sqlQuery.getQuery());
+        exceptionToReport = QueryInterruptedException.wrapIfNeeded((new ISE("Cannot build plan for query: %s", sqlQuery.getQuery())));


nit: this isn't new or your change, but I think it is semi confusing that there exists aSqlPlanningException that isn't being used here. The reason for this is because I think that this is a different kind of planning exception - one that occurs for some query which we couldn't translate to a native Druid query, which we seem to be treating as a server-side error, where the other one is currently used for SQL parsing and validation errors and considered client side errors since they are things the user can fix.

I wonder if we should have some sort of unsupported query exception to give this a bit more prominence, but I think that is a tangential discussion to this PRs changed.

Actually now that I look at it...I didn't have to change/refactor those and can just call serverConfig.getErrorResponseTransformStrategy().transformIfNeeded at the end

clintropolis · 2021-09-20T23:34:27Z

    catch (ForbiddenException e) {
      endLifecycleWithoutEmittingMetrics(sqlQueryId, lifecycle);
-      throw e; // let ForbiddenExceptionMapper handle this
+      throw (ForbiddenException) serverConfig.getErrorResponseTransformStrategy().transformIfNeeded(e); // let ForbiddenExceptionMapper handle this


at some point in the future, assuming we eventually make this functionality apply to all API response errors, it probably makes sense to push this transformation functionality into the error handlers so that we everything that lets exceptions fall through doesn't need to do this itself. It is probably ok to leave here for now since this is the only thing doing it

in fact, whenever in the future we do that, it might also make sense to move QueryException handling out to an ExceptionMapper<QueryException> and just rethrow all exceptions so that error response handling can be shared between sql and native json queries. it doesn't gain too much since will still need to handle internally so that metrics can be emitted, but it would cover the response writing.

clintropolis · 2021-09-20T23:37:57Z

    catch (QueryCapacityExceededException cap) {
      endLifecycle(sqlQueryId, lifecycle, cap, remoteAddr, -1);
-      return buildNonOkResponse(QueryCapacityExceededException.STATUS_CODE, cap);
+      return buildNonOkResponse(


it looks like all callers of buildNonOkResponse are calling transformIfNeeded, it makes sense to update the signature to just accept SanitizableException

clintropolis · 2021-09-20T23:54:03Z

           inflateBufferSize == that.inflateBufferSize &&
           compressionLevel == that.compressionLevel &&
           enableForwardedRequestCustomizer == that.enableForwardedRequestCustomizer &&
+           sanitizeJettyErrorResponse == that.sanitizeJettyErrorResponse &&


hm, I wonder if technically ErrorResponseTransformStrategy should implement equals and be checked here

I was having problem with the AllowedRegexErrorResponseTransformStrategy as the Pattern class doesn't implement equals method. So i just decided to exclude the ErrorResponseTransformStrategy here

yeah, I guess you would need to deserialize into strings and keep them around alongside a set of patterns to build (which might be worth doing?)

clintropolis · 2021-09-20T23:56:44Z

+
+import java.util.function.Function;
+
+public interface SanitizableException


nit: if a longer term goal is to make sure all surface exceptions are wrapped in this to allow operators to tidy up the error responses, i wonder if we should take a stronger name for this interface like ServiceApiException or .. idk naming is hard.

This shouldn't effect/change the UI/UX or configs/settings so I think if we want, we can easily rename it later

javadocs please. Explanation about how this is expected to be used and why this class exists will be helpful.

Instead of calling this an Exception - let's make this a Factory class since the interface can't extend an Exception.

public interface SanitizingExceptionFactory<T extends Exception> { T transform(T exception, Function<String, String> errorMessageTransformFunction); }

Instead of calling this an Exception - let's make this a Factory class since the interface can't extend an Exception

Please ignore this suggestion. It makes the ErrorResponseTransformStrategy clunky

clintropolis · 2021-09-20T23:57:42Z

+   * Apply the function for transforming the error message then
+   * return new Exception with sanitized fields and transformed message.
+   */
+  Exception applyErrorMessageTransformAndSanitizeFields(


nit: i think transform would be descriptive enough of a method name, between the parameter name and the javadocs.

clintropolis · 2021-09-21T00:02:36Z

+    if (exception instanceof Exception) {
+      return (Exception) exception;
+    } else {
+      return new IAE(
+          "Cannot sanitize error response as given argument[%s] is not an Exception",
+          exception.getClass().getName()
+      );
+    }


why not just return exception; and be a no-op like the javadoc says?

The method takes in SanitizableException which isn't guarantee to be an Exception. If it not, then it would result in ClassCastException and wouldn't be a no-op too.

Changed to return (Exception) exception;

suneet-s

+1 on naming of system properties. They seem reasonable.

suneet-s · 2021-09-22T04:40:53Z

+  public Function<String, String> getErrorMessageTransformFunction()
+  {
+    return (String errorMessage) -> {
+      if (allowedRegex.stream().noneMatch(pattern -> pattern.matcher(errorMessage).matches())) {


nit: Is it better to use anyMatch to return the errorMessage so the stream can short circuit and exit early?

suneet-s · 2021-09-22T05:28:31Z

+
+import java.util.function.Function;
+
+public interface SanitizableException


javadocs please. Explanation about how this is expected to be used and why this class exists will be helpful.

suneet-s · 2021-09-22T05:29:17Z

+   * return new Exception with sanitized fields and transformed message.
+   */
+  Exception transform(
+      Function<String, String> errorMessageTransformFunction


Can you add details on why only the errorMessage is expected to be transformed in this interface to the javadoc please

suneet-s · 2021-09-22T05:38:15Z

+   * Apply the function for transforming the error message then
+   * return new Exception with sanitized fields and transformed message.
+   */
+  Exception transform(


nit: rename to sanitize to be inline with SanitizableException

heh oops, was my suggestion to rename transform, but I guess I also suggested renaming SanitizableException to something else, though I feel less strongly about the name of this than the property name in the config, (which I think should be druid.server.http.enableErrorResponseTransform and druid.server.http.errorResponseTransformStrategy)

suneet-s · 2021-09-22T05:40:27Z

+    if (Strings.isNullOrEmpty(errorMessageTransformFunction.apply(getMessage()))) {
+      return new ForbiddenException();
+    } else {
+      return this;


appears to be a logic bug here

unit tests please

suneet-s · 2021-09-22T05:43:44Z

+  @Override
+  public QueryException transform(Function<String, String> errorMessageTransformFunction)
+  {
+    return new QueryException(errorCode, errorMessageTransformFunction.apply(getMessage()), null, null);


can we add unit tests for this please

clintropolis · 2021-09-22T06:34:29Z

      boolean enableForwardedRequestCustomizer,
-      @NotNull List<String> allowedHttpMethods
+      @NotNull List<String> allowedHttpMethods,
+      boolean sanitizeJettyErrorResponse,


I think these configs should be called enableErrorResponseTransform and errorResponseTransformStrategy respectively, or something similar, since it is clearer their purpose (and also ErrorResponseTransformStrategy is the type name)

sorry i got confused about sanitizeJettyErrorResponse, it shouldn't be renamed to enableErrorResponseTransform, though sanitizeDruidErrorResponse should still be renamed errorResponseTransformStrategy

We don't need enableErrorResponseTransform as not setting errorResponseTransformStrategy or setting errorResponseTransformStrategy to none would be the same as current behavior (no-op) and leaving the error response unchanged.

I renamed sanitizeJettyErrorResponse to showDetailedJettyErrors and flip it
also renamed sanitizeDruidErrorResponse to errorResponseTransformStrategy

yep sorry confused myself, thinking about too many things at once 🙃 , this sgtm 👍

Actually it should be errorResponseTransform not errorResponseTransformStrategy for the @JsonProperty
so that the configs look something like

druid.server.http.showDetailedJettyErrors=false druid.server.http.errorResponseTransform.strategy=allowedRegex druid.server.http.errorResponseTransform.allowedRegex=["asd"]

clintropolis · 2021-09-22T06:37:30Z

+   * Apply the function for transforming the error message then
+   * return new Exception with sanitized fields and transformed message.
+   */
+  Exception transform(


heh oops, was my suggestion to rename transform, but I guess I also suggested renaming SanitizableException to something else, though I feel less strongly about the name of this than the property name in the config, (which I think should be druid.server.http.enableErrorResponseTransform and druid.server.http.errorResponseTransformStrategy)

clintropolis · 2021-09-22T06:38:35Z

           inflateBufferSize == that.inflateBufferSize &&
           compressionLevel == that.compressionLevel &&
           enableForwardedRequestCustomizer == that.enableForwardedRequestCustomizer &&
+           sanitizeJettyErrorResponse == that.sanitizeJettyErrorResponse &&


yeah, I guess you would need to deserialize into strings and keep them around alongside a set of patterns to build (which might be worth doing?)

clintropolis

👍 lgtm

we should document this, but is ok with me to do as a follow-up

suneet-s

LGTM after CI + docs in a follow up PR

…PI response (apache#11711) * add impl * add impl * add tests * add unit test * fix checkstyle * address comments * fix checkstyle * fix checkstyle * fix checkstyle * fix checkstyle * fix checkstyle * address comments * address comments * address comments * fix test * fix test * fix test * fix test * fix test * change config name * change config name * change config name * address comments * address comments * address comments * address comments * address comments * address comments * fix compile * fix compile * change config * add more tests * fix IT

maytasm added 6 commits September 15, 2021 16:04

add impl

a7902e5

add impl

b44f16e

add tests

3102771

add unit test

d15cd69

fix checkstyle

32b189f

Merge branch 'master' into IMPLY-9307

d17adef

clintropolis reviewed Sep 16, 2021

View reviewed changes

clintropolis added Area - Operations Area - Querying Design Review labels Sep 16, 2021

maytasm added 18 commits September 16, 2021 17:50

address comments

d35c081

fix checkstyle

82fdee3

fix checkstyle

b4a302f

fix checkstyle

d416f18

fix checkstyle

bb95dff

fix checkstyle

ffb9497

Merge branch 'master' into IMPLY-9307

b59a5e5

address comments

7725532

address comments

eab495a

address comments

e0578de

fix test

12907b6

fix test

f276b91

fix test

94293dd

fix test

1ef87e2

fix test

559c177

change config name

6f3ed3f

change config name

bc30082

change config name

e5a53ba

clintropolis reviewed Sep 21, 2021

View reviewed changes

maytasm added 3 commits September 21, 2021 20:54

address comments

d2e04ac

address comments

8a63664

Merge branch 'master' into IMPLY-9307

068e764

suneet-s reviewed Sep 22, 2021

View reviewed changes

clintropolis reviewed Sep 22, 2021

View reviewed changes

maytasm added 4 commits September 22, 2021 16:32

address comments

926c65e

address comments

23b5292

address comments

675b350

address comments

92d09b0

clintropolis approved these changes Sep 22, 2021

View reviewed changes

suneet-s approved these changes Sep 22, 2021

View reviewed changes

maytasm added 5 commits September 22, 2021 23:04

fix compile

16eb95a

fix compile

f4dc781

change config

8a51b01

add more tests

2e1eae8

fix IT

b5f4ffd

maytasm merged commit a04b08e into apache:master Sep 29, 2021

maytasm deleted the IMPLY-9307 branch September 29, 2021 05:55

maytasm mentioned this pull request Sep 29, 2021

Add documentations for config to filter internal Druid-related messages from error response #11755

Merged

2 of 9 tasks

abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022

cryptoe mentioned this pull request Sep 17, 2024

Updated error response to hide error stack in case of JsonMappingException #16821

Merged

10 tasks

capistrant mentioned this pull request Oct 8, 2026

fix: apply errorResponseTransform to query execution failures #20529

Open

5 of 9 tasks


		import java.util.function.Function;

		public interface SanitizableException

Conversation

maytasm commented Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maytasm Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintropolis commented Sep 16, 2021

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suneet-s left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

maytasm commented Sep 15, 2021 •

edited

Loading

maytasm Sep 16, 2021 •

edited

Loading