This is an automated email from the ASF dual-hosted git repository. heneveld pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/brooklyn-server.git
commit f046b2711541f68575c4896d8ae396957f4232c3 Author: Alex Heneveld <[email protected]> AuthorDate: Tue Jan 21 12:54:31 2025 +0000 Tidy up of suppressing traces for some users in REST requests, and including a reference and better info --- .../org/apache/brooklyn/rest/domain/ApiError.java | 19 +++-- .../brooklyn/rest/util/DefaultExceptionMapper.java | 91 +++++++++++++++++----- 2 files changed, 86 insertions(+), 24 deletions(-) diff --git a/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/ApiError.java b/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/ApiError.java index cc9b70ed5e..51d7725af7 100644 --- a/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/ApiError.java +++ b/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/ApiError.java @@ -23,6 +23,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import java.io.Serializable; import java.util.Objects; +import javax.annotation.Nonnull; import javax.annotation.Nullable; import javax.ws.rs.WebApplicationException; import javax.ws.rs.core.MediaType; @@ -219,10 +220,18 @@ public class ApiError implements Serializable { } public WebApplicationException throwWebApplicationException(@Nullable Throwable exception) { - throw new WebApplicationException( - exception==null ? new Throwable(this.toString()) : - message==null ? exception : - new PropagatedRuntimeException(message, exception), - this.asJsonResponse()); + // drops any details, for now; unlikely the user will want them + throw new WebApplicationExceptionWithApiError(this, exception); + } + + public static class WebApplicationExceptionWithApiError extends WebApplicationException { + private final ApiError apiError; + public WebApplicationExceptionWithApiError(@Nonnull ApiError apiError, Throwable cause) { + super(apiError.getMessage(), cause, apiError.asJsonResponse()); + this.apiError = apiError; + } + public ApiError getApiError() { + return apiError; + } } } diff --git a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/DefaultExceptionMapper.java b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/DefaultExceptionMapper.java index 8a9c65b039..1e5e0d1aee 100644 --- a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/DefaultExceptionMapper.java +++ b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/DefaultExceptionMapper.java @@ -23,6 +23,8 @@ import java.util.function.BiFunction; import java.util.function.BiPredicate; import javax.ws.rs.WebApplicationException; +import javax.ws.rs.container.ContainerRequestContext; +import javax.ws.rs.container.ResourceContext; import javax.ws.rs.core.Context; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; @@ -65,6 +67,9 @@ public class DefaultExceptionMapper implements ExceptionMapper<Throwable> { @Context private ContextResolver<ManagementContext> mgmt; + @Context + private ResourceContext resourceContext; + /** * Maps a throwable to a response. * <p/> @@ -74,12 +79,35 @@ public class DefaultExceptionMapper implements ExceptionMapper<Throwable> { */ @Override public Response toResponse(Throwable throwable1) { + String user = Strings.toString(Entitlements.getEntitlementContext()); + if (user==null) user = "<not_logged_in>"; + + String path = null; + try { + // get path if possible for all requests + ContainerRequestContext requestContext = resourceContext.getResource(ContainerRequestContext.class); + path = requestContext.getUriInfo().getPath(); + if (LOG.isTraceEnabled()) { + LOG.trace("ERROR CONTEXT DETAILS for "+throwable1); + LOG.trace("url: "+requestContext.getUriInfo()); + LOG.trace("headers: "+requestContext.getHeaders()); + } + } catch (Exception e) { + Exceptions.propagateIfFatal(e); + if (firstEncounter(e)) { + // include debug trace for everything the first time we get one + LOG.debug("Unable to resolve path on error "+throwable1+" (logging once): "+e); + } + } + if (path==null) path = "<path_unavailable>"; + // EofException is thrown when the connection is reset, // for example when refreshing the browser window. // Don't depend on jetty, could be running in other environments as well. if (throwable1.getClass().getName().equals("org.eclipse.jetty.io.EofException")) { if (LOG.isTraceEnabled()) { - LOG.trace("REST request running as {} was disconnected, threw: {}", Entitlements.getEntitlementContext(), + LOG.trace("REST request {} running as user {} was disconnected, threw: {}", + path, user, Exceptions.collapseText(throwable1)); } return null; @@ -88,18 +116,35 @@ public class DefaultExceptionMapper implements ExceptionMapper<Throwable> { String errorReference = Identifiers.makeRandomId(13); Throwable throwable2 = Exceptions.getFirstInteresting(throwable1); if (isSevere(throwable2)) { - LOG.warn("REST request running as {} threw: {} (ref {})", Entitlements.getEntitlementContext(), - Exceptions.collapseText(throwable1), errorReference); + LOG.warn("REST request {} running as {} threw severe: {} (ref {})", + path, user, + Exceptions.collapseText(throwable1), errorReference); } else { - LOG.debug("REST request running as {} threw: {} (ref {})", Entitlements.getEntitlementContext(), - Exceptions.collapseText(throwable1), errorReference); + LOG.debug("REST request {} running as {} threw: {} (ref {})", + path, user, + Exceptions.collapseText(throwable1), errorReference); } - logExceptionDetailsForDebugging(throwable1); + logExceptionDetailsForDebugging(throwable1, errorReference); // Some methods will throw this, which gets converted automatically if (throwable2 instanceof WebApplicationException) { - WebApplicationException wae = (WebApplicationException) throwable2; - return wae.getResponse(); + if (throwable2 instanceof ApiError.WebApplicationExceptionWithApiError) { + ApiError apiError = ((ApiError.WebApplicationExceptionWithApiError)throwable2).getApiError(); + if (!isTraceVisibleToUser()) { + if (apiError.getDetails() != null) { + LOG.debug("Details of suppressed API error ref " + errorReference + ": " + apiError.getDetails()); + } + return ApiError.builder().message(apiError.getMessage()).details("Reference: " + errorReference).errorCode(apiError.getError()).build().asJsonResponse(); + } else { + // include error reference + return ApiError.builder().copy(apiError).message(""+apiError.getMessage()+" (Reference: "+errorReference+")").build().asJsonResponse(); + } + } else { + WebApplicationException wae = (WebApplicationException) throwable2; + // could suppress all messages if anything includes unwanted details (but believed not to be the case) + // note error reference not included, but we'll live with that for this error which isn't used (will still be logged above, just not reported to user) + return wae.getResponse(); + } } // The nicest way for methods to provide errors, wrap as this, and the stack trace will be suppressed @@ -107,49 +152,52 @@ public class DefaultExceptionMapper implements ExceptionMapper<Throwable> { return ApiError.of(throwable2.getMessage()).asBadRequestResponseJson(); } - // For everything else, a trace is supplied + // For everything else, a trace is supplied unless blocked // Assume ClassCoercionExceptions are caused by TypeCoercions from input parameters gone wrong // And IllegalArgumentException for malformed input parameters. if (throwable2 instanceof ClassCoercionException || throwable2 instanceof IllegalArgumentException) { + if (!isTraceVisibleToUser()) { + return ApiError.of(throwable2.getMessage()).asBadRequestResponseJson(); + } return ApiError.of(throwable2).asBadRequestResponseJson(); } // YAML exception if (throwable2 instanceof YAMLException) { - return ApiError.builder().message(throwable2.getMessage()).prefixMessage("Invalid YAML").build().asBadRequestResponseJson(); + return ApiError.builder().message(throwable2.getMessage()).details("Reference: "+errorReference).prefixMessage("Invalid YAML").build().asBadRequestResponseJson(); } if (!isTooUninterestingToLogWarn(throwable2)) { - if ( encounteredUnknownExceptions.add( throwable2.getClass() )) { - LOG.warn("REST call generated exception type "+throwable2.getClass()+" unrecognized in "+getClass()+" (subsequent occurrences will be logged debug only): " + throwable2, throwable2); + if (encounteredUnknownExceptions.add( throwable2.getClass() )) { + LOG.warn("REST call reference "+errorReference+" generated exception type "+throwable2.getClass()+" unrecognized in "+getClass()+" (subsequent occurrences will be logged debug only): " + throwable2, throwable2); } } // Before saying server error, look for a user-facing exception anywhere in the hierarchy UserFacingException userFacing = Exceptions.getFirstThrowableOfType(throwable1, UserFacingException.class); if (userFacing instanceof UserFacingException) { - return ApiError.of(userFacing.getMessage()).asBadRequestResponseJson(); + return ApiError.builder().message(userFacing.getMessage()).details("Reference: "+errorReference).build().asBadRequestResponseJson(); } if (!isTraceVisibleToUser()) { return ApiError.builder() .message("Internal error. Contact server administrator citing reference " + errorReference +" to consult logs for more details.") + .details("Reference: "+errorReference) .build().asResponse(Status.INTERNAL_SERVER_ERROR, MediaType.APPLICATION_JSON_TYPE); } else { Builder rb = ApiError.builderFromThrowable(Exceptions.collapse(throwable2)); if (Strings.isBlank(rb.getMessage())) { rb.message("Internal error. Contact server administrator citing reference " + errorReference + " to consult logs for more details."); - } else { - rb.message(rb.getMessage()+" (Reference: "+errorReference+")"); } + rb.details("Reference: "+errorReference); return rb.build().asResponse(Status.INTERNAL_SERVER_ERROR, MediaType.APPLICATION_JSON_TYPE); } } public static final ConfigKey<ReturnStackTraceMode> REST_RETURN_STACK_TRACES = ConfigKeys.newConfigKey(ReturnStackTraceMode.class, - MultiSessionAttributeAdapter.OAB_SERVER_CONFIG_PREFIX+"returnStackTraces", + MultiSessionAttributeAdapter.OAB_SERVER_CONFIG_PREFIX+".returnStackTraces", "Whether REST requests that have errors can include stack traces; " + "'true' or 'all' to mean always, 'false' or 'none' to mean never, " + "and otherwise 'root' or 'power' to allow users with root or java entitlement", @@ -167,7 +215,7 @@ public class DefaultExceptionMapper implements ExceptionMapper<Throwable> { return false; } } - enum ReturnStackTraceMode { + public enum ReturnStackTraceMode { ALL( (_m,_ec) -> true), TRUE( (_m,_ec) -> true), NONE( (_m,_ec) -> false), @@ -200,12 +248,17 @@ public class DefaultExceptionMapper implements ExceptionMapper<Throwable> { /** Logs full details at trace, unless it is the first time we've seen this in which case it is debug */ @Beta public static void logExceptionDetailsForDebugging(Throwable t) { + logExceptionDetailsForDebugging(t, null); + } + @Beta + public static void logExceptionDetailsForDebugging(Throwable t, String errorReference) { if (LOG.isDebugEnabled()) { if (firstEncounter(t)) { // include debug trace for everything the first time we get one - LOG.debug("Full details of "+Entitlements.getEntitlementContext()+" "+t+" (logging debug on first encounter; subsequent instances will be logged trace)", t); + LOG.debug("Full details of error "+(errorReference!=null ? "ref "+errorReference : "(user "+Entitlements.getEntitlementContext()+")")+": "+t+ + " (logging debug on first encounter; subsequent instances will be logged trace)", t); } else if (LOG.isTraceEnabled()) { - LOG.trace("Full details of "+Entitlements.getEntitlementContext()+" "+t, t); + LOG.trace("Full details of error "+(errorReference!=null ? "ref "+errorReference : "(user "+Entitlements.getEntitlementContext()+")")+": "+t, t); } } }
