[BUG] generic "No OpenSearchException found" error if http.detailed_errors is disabled

[rursprung]

Describe the bug
if http.detailed_errors.enabled: false is set then the error message in a REST API call will only report back an error message if the exception contains an OpenSearchException:

OpenSearch/server/src/main/java/org/opensearch/OpenSearchException.java

Lines 595 to 608 in 4657716

	// Render the exception with a simple message
	if (detailed == false) {
	String message = "No OpenSearchException found";
	Throwable t = e;
	for (int counter = 0; counter < 10 && t != null; counter++) {
	if (t instanceof OpenSearchException) {
	message = t.getClass().getSimpleName() + "[" + t.getMessage() + "]";
	break;
	}
	t = t.getCause();
	}
	builder.field(ERROR, message);
	return;
	}

(note: confusingly enough this method is a static member of OpenSearchException but handles any Exception, IMHO it might be better located e.g. in ExceptionsHelper?)

there are however (many?) cases where this isn't the case, e.g. when trying to update a setting through the settings API which doesn't support it (fun-fact: i noticed it when trying to update the http.detailed_errors.enabled setting). this returns an IllegalArgumentException instead.

To Reproduce

1. start a cluster with http.detailed_errors.enabled: false
2. call PUT /_cluster/settings with a setting which doesn't support it, e.g.:

   {
       "persistent": {
           "http.detailed_errors.enabled": true
       }
   }

with http.detailed_errors.enabled: false (as in the reproduction case) it gives the following - useless - response:

{
    "error": "No OpenSearchException found",
    "status": 400
}

with the default settings (or explicitly http.detailed_errors.enabled: true) it gives the following response:

{
    "error": {
        "root_cause": [
            {
                "type": "illegal_argument_exception",
                "reason": "persistent setting [http.detailed_errors.enabled], not dynamically updateable"
            }
        ],
        "type": "illegal_argument_exception",
        "reason": "persistent setting [http.detailed_errors.enabled], not dynamically updateable"
    },
    "status": 400
}

Expected behavior
the error message gives a hint at the cause as here the cause is clearly on the side of the caller. without the message the caller has no way of knowing what to do to fix the issue.

otherwise the message should at least be in the server log, but this isn't the case either (with default log settings). that way no information leakage would happen but it'd still be possible to understand the cause of a failure (for people with access to the system which should be the only ones which can do config updates via APIs).

Plugins
(irrelevant, this is about core functionality)

Screenshots
n/a

Host/Environment (please complete the following information):
OpenSearch 2.2.0 running in a linux container

Additional context
see the following related tickets:

• prevent inclusion of stack trace in response (leak of detailed error information) #4255
• [DOC] Update error_trace URL parameter description documentation-website#1235

[xuezhou25]

Update my findings: When http.detailed_errors.enabled is set to false and the exception type is from JAVA exception, it will return general message "No OpenSearchException found".(Example from previous reproduction)

Providing another example with http.detailed_errors.enabled: false and exception is from OpenSearchException, it will response with type and reason of root cause:

$ curl -H 'Content-Type: application/json' -XPUT localhost:9200/_cluster/settings -d '{ "persistent" : { "cluster.blocks.read_only": true } }'

$ curl -XPUT "localhost:9200/new-index?pretty"
{
  "error" : "ClusterBlockException[index [new-index] blocked by: [FORBIDDEN/6/cluster read-only (api)];]",
  "status" : 403
}

If http.detailed_errors.enabled was set to true or with default setting

$ curl -XPUT "localhost:9200/new-index?pretty"

{
  "error" : {
    "root_cause" : [
      {
        "type" : "cluster_block_exception",
        "reason" : "index [new-index] blocked by: [FORBIDDEN/6/cluster read-only (api)];"
      }
    ],
    "type" : "cluster_block_exception",
    "reason" : "index [new-index] blocked by: [FORBIDDEN/6/cluster read-only (api)];"
  },
  "status" : 403
}

[andrross]

@reta @nknize Sorry for spamming folks here but looking for some guidance on a small but potentially impactful API change.

The current behavior when http.detailed_errors.enabled: false is to only send exception type name and message in the API response if the exception type is OpenSearchException; otherwise the text "No OpenSearchException found" is sent. I can only guess that this is intentional. However, as @rursprung notes this can be quite painful as a user if the underlying cause is something like an IllegalArgumentException because you get no useful details beyond the HTTP error code. Changing the behavior to send the type name and message regardless of exception type feels fairly innocuous to me and would provide more useful details to the user, but I'm not sure about information leakage. I see a few options here (as suggested by @rursprung above):

1. Change the behavior to send the type name and message regardless of exception type
2. Add logging in the case where no details are sent (I don't love this option...feels a bit like the log-and-throw anti-pattern while also potentially spamming the log with 400-type user errors)
3. Play whack-a-mole at these cases throughout the code base where non-OpenSearchExceptions are thrown and change them to throw OpenSearchExceptions with helpful error messages
4. Do nothing; user gets an HTTP error code and the text "No OpenSearchException found"

Does anyone here have more historical context around this behavior? Any and all opinions are welcome.

[reta]

@andrross you are right, the number of changes went it to prevent information leaks, may be we could include exception type BUT not the details, for example:

{
    "error": "No OpenSearchException found",
    "status": 400
}

becomes

{
    "error": {
        "type": "illegal_argument_exception",
        "reason": "N/A (use 'http.detailed_errors.enabled' setting to enable detailed errors reporting)"
    },
    "status": 400
}

[andrross]

Thanks @reta !

Looking at this a bit more, what do you think about mirroring the logic in ExceptionHelpers.status() with something like the following?

public final class ExceptionsHelper {

    ...

    public static String summaryMessage(Throwable t) {
        if (t != null) {
            if (t instanceof OpenSearchException) {
                return t.getClass().getSimpleName() + "[" + t.getMessage() + "]";
            } else if (t instanceof IllegalArgumentException) {
                return "Invalid argument";
            } else if (t instanceof JsonParseException) {
                return "Failed to parse JSON";
            } else if (t instanceof OpenSearchRejectedExecutionException) {
                return "Too many requests";
            }
        }
        return "Internal failure";
    }
}

The logic in OpenSearchExceptions would become:

// Render the exception with a simple message
if (detailed == false) {
    Throwable t = e;
    for (int counter = 0; counter < 10 && t != null; counter++) {
        if (t instanceof OpenSearchException) {
            break;
        }
        t = t.getCause();
    }
    builder.field(ERROR, ExceptionHelpers.simpleMessage(t));
    return;
}

We'd mirror the logic around the known 400 errors with blessed messages. We'd also avoid exposing embarrassing details like a stray NullPointerException :)

[reta]

LGTM @andrross, I think it is reasonable alternative

[rursprung]

thanks for the discussion! the suggestion from @andrross would already be an improvement, but e.g. the settings API would then still not report anything useful (i.e. for a caller getting an error it'd still be unclear what he did wrong / what he'd have to change in order to make his call work). the problem is that IllegalArgumentException is a generic java exception - the code cannot know whether this was thrown in response to a user error (= should be reported to the user) or due to an internal system error / coding error leading to a wrong call to an API which then returns this error (= should maybe be kept back)

[andrross]

@rursprung I agree with your points. I think we should go ahead with the proposal above that will result in no longer responding with the confusing/unhelpful "No OpenSearchException found" text.

Separately, I think we should update the AbstractScopedSettings class to throw OpenSearchExceptions instead of IllegalArgumentException so that the helpful error messages can be reported in all cases.

This keeps a fairly conservative approach of not reporting arbitrary exception details when http.detailed_errors.enabled: false while fixing the specific case of sending back helpful details when making invalid setting changes. Thoughts?

[rursprung]

i think that's a sensible trade-off, but it's then probably something which should be kept in mind when adding any new exceptions anywhere in the system: you always have to consider whether it should be an OpenSearchException or if it can be something else. and it might mean patching quite a few APIs over time if people encounter errors there and need a better error message.

[rursprung]

@xuezhou25 i saw that you implemented part of what was discussed here in #4669 (thanks for that!) and closed this issue here from there.
what about the second part mentioned by @andrross in #4545 (comment):

Separately, I think we should update the AbstractScopedSettings class to throw OpenSearchExceptions instead of IllegalArgumentException so that the helpful error messages can be reported in all cases.

will you address this in a separate PR? should i raise a new ticket for this? note that it doesn't just affect the line linked in the comment, i count 17 throw new IllegalArgumentException(...) occurrences in AbstractScopedSettings.
what is the recommended way to go forward here? is it to now do throw new OpenSearchException(new IllegalArgumentException(...))? add a try/catch block around the method implementation, catch IllegalArgumentException and re-throw it wrapped in an OpenSearchException? or just throw an OpenSearchException instead of an IllegalArgumentException?

[xuezhou25]

@rursprung I created a new issue to throw new OpenSearchExceptions instead of IllegalArgumentException by updating in the AbstractScopedSettings class.
Thanks for providing several solutions. I don't think do throw new OpenSearchException(new IllegalArgumentException(...)) can meet the requirement because in the logic here

OpenSearch/server/src/main/java/org/opensearch/OpenSearchException.java

Lines 593 to 598 in 77cff55

	for (int counter = 0; counter < 10 && t != null; counter++) {
	if (t instanceof OpenSearchException) {
	break;
	}
	t = t.getCause();
	}

will find the root cause of exception and return "Invalid argument". So I am thinking of the way wrapping IllegalArgumentException to an OpenSearchException.
Keeping this issue closed.

[dblock]

#4745