cleaned up how non-master filter errors are reported
Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/d04d560a Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/d04d560a Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/d04d560a Branch: refs/heads/master Commit: d04d560aed764c7637c8cba24ff2fd2300558bd8 Parents: e400d77 Author: Alex Heneveld <[email protected]> Authored: Fri Mar 20 14:06:45 2015 +0000 Committer: Alex Heneveld <[email protected]> Committed: Fri Mar 20 14:06:45 2015 +0000 ---------------------------------------------------------------------- .../java/brooklyn/rest/domain/ApiError.java | 48 ++++++++++++++++---- .../rest/filter/HaHotCheckResourceFilter.java | 17 ++++--- .../rest/filter/HaMasterCheckFilter.java | 18 +++++--- .../brooklyn/rest/resources/ServerResource.java | 3 +- .../brooklyn/rest/util/WebResourceUtils.java | 26 +++++++++-- 5 files changed, 85 insertions(+), 27 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d04d560a/usage/rest-api/src/main/java/brooklyn/rest/domain/ApiError.java ---------------------------------------------------------------------- diff --git a/usage/rest-api/src/main/java/brooklyn/rest/domain/ApiError.java b/usage/rest-api/src/main/java/brooklyn/rest/domain/ApiError.java index e2689f4..37f2eee 100644 --- a/usage/rest-api/src/main/java/brooklyn/rest/domain/ApiError.java +++ b/usage/rest-api/src/main/java/brooklyn/rest/domain/ApiError.java @@ -71,6 +71,7 @@ public class ApiError { public static class Builder { private String message; private String details; + private Integer errorCode; public Builder message(String message) { this.message = checkNotNull(message, "message"); @@ -82,6 +83,15 @@ public class ApiError { return this; } + public Builder errorCode(Status errorCode) { + return errorCode(errorCode.getStatusCode()); + } + + public Builder errorCode(Integer errorCode) { + this.errorCode = errorCode; + return this; + } + /** as {@link #prefixMessage(String, String)} with default separator of `: ` */ public Builder prefixMessage(String prefix) { return prefixMessage(prefix, ": "); @@ -97,7 +107,7 @@ public class ApiError { } public ApiError build() { - return new ApiError(message, details); + return new ApiError(message, details, errorCode); } /** @deprecated since 0.7.0; use {@link #copy(ApiError)} */ @@ -109,7 +119,8 @@ public class ApiError { public Builder copy(ApiError error) { return this .message(error.message) - .details(error.details); + .details(error.details) + .errorCode(error.error); } public String getMessage() { @@ -122,15 +133,18 @@ public class ApiError { @JsonSerialize(include=Inclusion.NON_EMPTY) private final String details; - public ApiError(String message) { - this(message, null); - } + @JsonSerialize(include=Inclusion.NON_NULL) + private final Integer error; + public ApiError(String message) { this(message, null); } + public ApiError(String message, String details) { this(message, details, null); } public ApiError( @JsonProperty("message") String message, - @JsonProperty("details") String details) { + @JsonProperty("details") String details, + @JsonProperty("error") Integer error) { this.message = checkNotNull(message, "message"); this.details = details != null ? details : ""; + this.error = error; } public String getMessage() { @@ -141,18 +155,23 @@ public class ApiError { return details; } + public Integer getErrorCode() { + return error; + } + @Override public boolean equals(Object other) { if (this == other) return true; if (other == null || getClass() != other.getClass()) return false; ApiError that = ApiError.class.cast(other); return Objects.equal(this.message, that.message) && - Objects.equal(this.details, that.details); + Objects.equal(this.details, that.details) && + Objects.equal(this.error, that.error); } @Override public int hashCode() { - return Objects.hashCode(message, details); + return Objects.hashCode(message, details, error); } @Override @@ -160,6 +179,7 @@ public class ApiError { return Objects.toStringHelper(this) .add("message", message) .add("details", details) + .add("error", error) .toString(); } @@ -167,10 +187,18 @@ public class ApiError { return asResponse(Status.BAD_REQUEST, MediaType.APPLICATION_JSON_TYPE); } - public Response asResponse(Status status, MediaType type) { - return Response.status(status) + public Response asResponse(Status defaultStatus, MediaType type) { + return Response.status(error!=null ? error : defaultStatus!=null ? defaultStatus.getStatusCode() : Status.INTERNAL_SERVER_ERROR.getStatusCode()) .type(type) .entity(this) .build(); } + + public Response asResponse(MediaType type) { + return asResponse(null, type); + } + + public Response asJsonResponse() { + return asResponse(MediaType.APPLICATION_JSON_TYPE); + } } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d04d560a/usage/rest-server/src/main/java/brooklyn/rest/filter/HaHotCheckResourceFilter.java ---------------------------------------------------------------------- diff --git a/usage/rest-server/src/main/java/brooklyn/rest/filter/HaHotCheckResourceFilter.java b/usage/rest-server/src/main/java/brooklyn/rest/filter/HaHotCheckResourceFilter.java index 5367439..c219de2 100644 --- a/usage/rest-server/src/main/java/brooklyn/rest/filter/HaHotCheckResourceFilter.java +++ b/usage/rest-server/src/main/java/brooklyn/rest/filter/HaHotCheckResourceFilter.java @@ -24,12 +24,15 @@ import java.util.Set; import javax.ws.rs.WebApplicationException; import javax.ws.rs.core.Context; -import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import brooklyn.entity.rebind.RebindManagerImpl.RebindTracker; import brooklyn.management.ManagementContext; import brooklyn.management.ha.ManagementNodeState; +import brooklyn.rest.domain.ApiError; import brooklyn.util.time.Duration; import com.google.common.collect.ImmutableSet; @@ -41,6 +44,9 @@ import com.sun.jersey.spi.container.ResourceFilter; import com.sun.jersey.spi.container.ResourceFilterFactory; public class HaHotCheckResourceFilter implements ResourceFilterFactory { + + private static final Logger log = LoggerFactory.getLogger(HaHotCheckResourceFilter.class); + private static final Set<ManagementNodeState> HOT_STATES = ImmutableSet.of( ManagementNodeState.MASTER, ManagementNodeState.HOT_STANDBY, ManagementNodeState.HOT_BACKUP); private static final long STATE_CHANGE_SETTLE_OFFSET = Duration.seconds(10).toMilliseconds(); @@ -71,11 +77,10 @@ public class HaHotCheckResourceFilter implements ResourceFilterFactory { @Override public ContainerRequest filter(ContainerRequest request) { if (!isStateLoaded() && isUnsafe(request)) { - Response response = Response.status(Response.Status.FORBIDDEN) - .type(MediaType.APPLICATION_JSON) - .entity("{\"error\":403,\"message\":\"Requests should be made to the master Brooklyn server\"}") - .build(); - throw new WebApplicationException(response); + log.warn("Disallowed request to standby brooklyn: "+request+"/"+am+" (caller should set '"+HaMasterCheckFilter.SKIP_CHECK_HEADER+"' to force)"); + throw new WebApplicationException(ApiError.builder() + .message("This request is not permitted against a standby Brooklyn server") + .errorCode(Response.Status.FORBIDDEN).build().asJsonResponse()); } return request; } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d04d560a/usage/rest-server/src/main/java/brooklyn/rest/filter/HaMasterCheckFilter.java ---------------------------------------------------------------------- diff --git a/usage/rest-server/src/main/java/brooklyn/rest/filter/HaMasterCheckFilter.java b/usage/rest-server/src/main/java/brooklyn/rest/filter/HaMasterCheckFilter.java index 03d75e0..bfb2cf4 100644 --- a/usage/rest-server/src/main/java/brooklyn/rest/filter/HaMasterCheckFilter.java +++ b/usage/rest-server/src/main/java/brooklyn/rest/filter/HaMasterCheckFilter.java @@ -24,11 +24,13 @@ import java.util.Set; import javax.servlet.Filter; import javax.servlet.FilterChain; import javax.servlet.FilterConfig; +import javax.servlet.ServletContext; import javax.servlet.ServletException; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import javax.ws.rs.core.Response; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -36,6 +38,8 @@ import org.slf4j.LoggerFactory; import brooklyn.config.BrooklynServiceAttributes; import brooklyn.management.ManagementContext; import brooklyn.management.ha.ManagementNodeState; +import brooklyn.rest.domain.ApiError; +import brooklyn.rest.util.WebResourceUtils; import com.google.common.collect.Sets; @@ -51,22 +55,22 @@ public class HaMasterCheckFilter implements Filter { public static final String SKIP_CHECK_HEADER = "Brooklyn-Allow-Non-Master-Access"; private static final Set<String> SAFE_STANDBY_METHODS = Sets.newHashSet("GET", "HEAD"); + protected ServletContext servletContext; protected ManagementContext mgmt; @Override public void init(FilterConfig config) throws ServletException { - mgmt = (ManagementContext) config.getServletContext().getAttribute(BrooklynServiceAttributes.BROOKLYN_MANAGEMENT_CONTEXT); + servletContext = config.getServletContext(); + mgmt = (ManagementContext) servletContext.getAttribute(BrooklynServiceAttributes.BROOKLYN_MANAGEMENT_CONTEXT); } @Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { if (!isMaster() && !isRequestAllowedForNonMaster(request)) { - log.warn("Disallowed request to non-HA master: "+request+"/"+request.getParameterMap()+" (caller should set '"+SKIP_CHECK_HEADER+"' to force)"); - HttpServletResponse httpResponse = (HttpServletResponse) response; - httpResponse.setStatus(HttpServletResponse.SC_FORBIDDEN); - httpResponse.setContentType("application/json"); - httpResponse.setCharacterEncoding("UTF-8"); - httpResponse.getWriter().write("{\"error\":403,\"message\":\"Requests should be made to the master Brooklyn server\"}"); + log.warn("Disallowed request to non-HA-master brooklyn: "+request+"/"+request.getParameterMap()+" (caller should set '"+SKIP_CHECK_HEADER+"' to force)"); + WebResourceUtils.applyJsonResponse(servletContext, ApiError.builder() + .message("This request is only permitted against a master Brooklyn server") + .errorCode(Response.Status.FORBIDDEN).build().asJsonResponse(), (HttpServletResponse)response); } else { chain.doFilter(request, response); } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d04d560a/usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java ---------------------------------------------------------------------- diff --git a/usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java b/usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java index 0714c4b..e44437f 100644 --- a/usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java +++ b/usage/rest-server/src/main/java/brooklyn/rest/resources/ServerResource.java @@ -102,7 +102,8 @@ public class ServerResource extends AbstractBrooklynRestResource implements Serv log.info("REST call to shutdown server, stopAppsFirst="+stopAppsFirst+", delayForHttpReturn="+shutdownTimeoutRaw); if (stopAppsFirst && !isMaster()) { - throw WebResourceUtils.serverError("Not allowed to stop all apps when server is not master"); + log.warn("REST call to shutdown non-master server while stopping apps is disallowed"); + throw WebResourceUtils.forbidden("Not allowed to stop all apps when server is not master"); } final Duration shutdownTimeout = parseDuration(shutdownTimeoutRaw, Duration.of(20, TimeUnit.SECONDS)); Duration requestTimeout = parseDuration(requestTimeoutRaw, Duration.of(20, TimeUnit.SECONDS)); http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d04d560a/usage/rest-server/src/main/java/brooklyn/rest/util/WebResourceUtils.java ---------------------------------------------------------------------- diff --git a/usage/rest-server/src/main/java/brooklyn/rest/util/WebResourceUtils.java b/usage/rest-server/src/main/java/brooklyn/rest/util/WebResourceUtils.java index 2a4f5e2..fcc2c5c 100644 --- a/usage/rest-server/src/main/java/brooklyn/rest/util/WebResourceUtils.java +++ b/usage/rest-server/src/main/java/brooklyn/rest/util/WebResourceUtils.java @@ -18,8 +18,11 @@ */ package brooklyn.rest.util; +import java.io.IOException; import java.util.Map; +import javax.servlet.ServletContext; +import javax.servlet.http.HttpServletResponse; import javax.ws.rs.WebApplicationException; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; @@ -30,11 +33,13 @@ import org.slf4j.LoggerFactory; import brooklyn.catalog.internal.CatalogUtils; import brooklyn.rest.domain.ApiError; +import brooklyn.rest.util.json.BrooklynJacksonJsonProvider; import brooklyn.util.exceptions.Exceptions; import brooklyn.util.net.Urls; import brooklyn.util.text.StringEscapes.JavaStringEscapes; import com.google.common.collect.ImmutableMap; +import com.sun.jersey.spi.container.ContainerResponse; public class WebResourceUtils { @@ -47,9 +52,9 @@ public class WebResourceUtils { log.debug("responding {} {} ({})", new Object[]{status.getStatusCode(), status.getReasonPhrase(), msg}); } - throw new WebApplicationException(Response.status(status) - .type(MediaType.APPLICATION_JSON_TYPE) - .entity(ApiError.builder().message(msg).build()).build()); + ApiError apiError = ApiError.builder().message(msg).errorCode(status).build(); + // including a Throwable is the only way to include a message with the WebApplicationException - ugly! + throw new WebApplicationException(new Throwable(apiError.toString()), apiError.asJsonResponse()); } /** @throws WebApplicationException With code 500 internal server error */ @@ -67,6 +72,11 @@ public class WebResourceUtils { return throwWebApplicationException(Response.Status.UNAUTHORIZED, format, args); } + /** @throws WebApplicationException With code 403 forbidden */ + public static WebApplicationException forbidden(String format, Object... args) { + return throwWebApplicationException(Response.Status.FORBIDDEN, format, args); + } + /** @throws WebApplicationException With code 404 not found */ public static WebApplicationException notFound(String format, Object... args) { return throwWebApplicationException(Response.Status.NOT_FOUND, format, args); @@ -139,4 +149,14 @@ public class WebResourceUtils { return Urls.encode(versionedId); } } + + /** Sets the {@link HttpServletResponse} target (last argument) from the given source {@link Response}; + * useful in filters where we might have a {@link Response} and need to set up an {@link HttpServletResponse}. + * Similar to {@link ContainerResponse#setResponse(Response)}; nothing like that seems to be available for {@link HttpServletResponse}. */ + public static void applyJsonResponse(ServletContext servletContext, Response source, HttpServletResponse target) throws IOException { + target.setStatus(source.getStatus()); + target.setContentType(MediaType.APPLICATION_JSON); + target.setCharacterEncoding("UTF-8"); + target.getWriter().write(BrooklynJacksonJsonProvider.findAnyObjectMapper(servletContext, null).writeValueAsString(source.getEntity())); + } }
