Repository: aurora Updated Branches: refs/heads/master a071af345 -> 0105a151b
Support TBinaryProtocol over HTTP This replaces the `TServlet` servlet from thrift with our own servlet which dispatches thrift responses based on the content type of the request. This enables a client to use either the thrift json protocol or the binary protocol when communicating with the scheduler. Without this patch the current behaviour is: - Regardless of content type of the request, assume the request is json and return json with a `application/x-thrift` content type. With this patch the behaviour becomes: - A request with no content type header, or a type of `application/x-thrift` or `application/json` or `application/vnd.apache.thrift.json` is assumed to be JSON. - A request with a content type header of `application/vnd.apache.thrift.binary` is assumed to be binary. - A request with an `Accept` header of `application/vnd.apache.thrift.binary` will have a binary response. - A request with any other `Accept` header will have a JSON response. Bugs closed: AURORA-1743 Reviewed at https://reviews.apache.org/r/50685/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/0105a151 Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/0105a151 Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/0105a151 Branch: refs/heads/master Commit: 0105a151b0431aec413b0f46021b21b993101424 Parents: a071af3 Author: Zameer Manji <zma...@apache.org> Authored: Wed Aug 3 15:31:18 2016 -0700 Committer: Zameer Manji <zma...@apache.org> Committed: Wed Aug 3 15:31:18 2016 -0700 ---------------------------------------------------------------------- RELEASE-NOTES.md | 6 + .../aurora/scheduler/http/api/ApiModule.java | 62 ++++++- .../http/api/TContentAwareServlet.java | 166 +++++++++++++++++++ .../scheduler/http/AbstractJettyTest.java | 6 + .../apache/aurora/scheduler/http/api/ApiIT.java | 70 +++++++- 5 files changed, 304 insertions(+), 6 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/0105a151/RELEASE-NOTES.md ---------------------------------------------------------------------- diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md index 19d7f7e..d14c3d7 100644 --- a/RELEASE-NOTES.md +++ b/RELEASE-NOTES.md @@ -17,6 +17,12 @@ from Mesos. This has affected rendering of some of the existing attributes. Furthermore, it now dumps additional offer attributes including [reservations](http://mesos.apache.org/documentation/latest/reservation/) and [persistent volumes](http://mesos.apache.org/documentation/latest/persistent-volume/). +* The scheduler API now accepts both thrift JSON and binary thrift. If a request is sent with a + `Content-Type` header, or a `Content-Type` header of `application/x-thrift` or `application/json` + or `application/vnd.apache.thrift.json` the request is treated as thrift JSON. If a request is + sent with a `Content-Type` header of `application/vnd.apache.thrift.binary` the request is treated + as binary thrift. If the `Accept` header of the request is `application/vnd.apache.thrift.binary` + then the response will be binary thrift. Any other value for `Accept` will result in thrift JSON. ### Deprecations and removals: http://git-wip-us.apache.org/repos/asf/aurora/blob/0105a151/src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java b/src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java index cd5adf9..e468209 100644 --- a/src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java +++ b/src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java @@ -14,6 +14,7 @@ package org.apache.aurora.scheduler.http.api; import javax.inject.Singleton; +import javax.ws.rs.core.MediaType; import com.google.common.collect.ImmutableMap; import com.google.inject.Provides; @@ -26,14 +27,24 @@ import org.apache.aurora.gen.AuroraAdmin; import org.apache.aurora.scheduler.http.CorsFilter; import org.apache.aurora.scheduler.http.JettyServerModule; import org.apache.aurora.scheduler.http.LeaderRedirectFilter; +import org.apache.aurora.scheduler.http.api.TContentAwareServlet.ContentFactoryPair; +import org.apache.aurora.scheduler.http.api.TContentAwareServlet.InputConfig; +import org.apache.aurora.scheduler.http.api.TContentAwareServlet.OutputConfig; import org.apache.aurora.scheduler.thrift.aop.AnnotatedAuroraAdmin; +import org.apache.thrift.protocol.TBinaryProtocol; import org.apache.thrift.protocol.TJSONProtocol; -import org.apache.thrift.server.TServlet; import org.eclipse.jetty.servlet.DefaultServlet; import org.eclipse.jetty.util.resource.Resource; +import static javax.ws.rs.core.MediaType.APPLICATION_JSON_TYPE; + public class ApiModule extends ServletModule { public static final String API_PATH = "/api"; + private static final MediaType GENERIC_THRIFT = new MediaType("application", "x-thrift"); + private static final MediaType THRIFT_JSON = + new MediaType("application", "vnd.apache.thrift.json"); + private static final MediaType THRIFT_BINARY = + new MediaType("application", "vnd.apache.thrift.binary"); /** * Set the {@code Access-Control-Allow-Origin} header for API requests. See @@ -52,7 +63,7 @@ public class ApiModule extends ServletModule { if (ENABLE_CORS_FOR.get() != null) { filter(API_PATH).through(new CorsFilter(ENABLE_CORS_FOR.get())); } - serve(API_PATH).with(TServlet.class); + serve(API_PATH).with(TContentAwareServlet.class); filter(ApiBeta.PATH, ApiBeta.PATH + "/*").through(LeaderRedirectFilter.class); filter(ApiBeta.PATH, ApiBeta.PATH + "/*") @@ -69,8 +80,49 @@ public class ApiModule extends ServletModule { @Provides @Singleton - TServlet provideApiThriftServlet(AnnotatedAuroraAdmin schedulerThriftInterface) { - return new TServlet( - new AuroraAdmin.Processor<>(schedulerThriftInterface), new TJSONProtocol.Factory()); + TContentAwareServlet provideApiThriftServlet(AnnotatedAuroraAdmin schedulerThriftInterface) { + /* + * For backwards compatibility the servlet is configured to assume `application/x-thrift` and + * `application/json` have TJSON bodies. + * + * Requests that have the registered MIME type for apache thrift are mapped to their respective + * protocols. See + * http://www.iana.org/assignments/media-types/application/vnd.apache.thrift.binary and + * http://www.iana.org/assignments/media-types/application/vnd.apache.thrift.json for details. + * + * Responses have the registered MIME type so the client can decode appropriately. + * + * The Accept header is used to determine the response type. By default JSON is sent for any + * value except for the binary thrift header. + */ + + ContentFactoryPair jsonFactory = new ContentFactoryPair( + new TJSONProtocol.Factory(), + THRIFT_JSON); + ContentFactoryPair binFactory = new ContentFactoryPair( + new TBinaryProtocol.Factory(), + THRIFT_BINARY); + + // Which factory to use based on the Content-Type header of the request for reading the request. + InputConfig inputConfig = new InputConfig(GENERIC_THRIFT, ImmutableMap.of( + GENERIC_THRIFT, jsonFactory, + THRIFT_JSON, jsonFactory, + APPLICATION_JSON_TYPE, jsonFactory, + THRIFT_BINARY, binFactory + )); + + // Which factory to use based on the Accept header of the request for the response. + OutputConfig outputConfig = new OutputConfig(APPLICATION_JSON_TYPE, ImmutableMap.of( + APPLICATION_JSON_TYPE, jsonFactory, + GENERIC_THRIFT, jsonFactory, + THRIFT_JSON, jsonFactory, + THRIFT_BINARY, binFactory + )); + + // A request without a Content-Type (like from curl) should be treated as GENERIC_THRIFT + return new TContentAwareServlet( + new AuroraAdmin.Processor<>(schedulerThriftInterface), + inputConfig, + outputConfig); } } http://git-wip-us.apache.org/repos/asf/aurora/blob/0105a151/src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java b/src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java new file mode 100644 index 0000000..1634cb8 --- /dev/null +++ b/src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java @@ -0,0 +1,166 @@ +/** + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.aurora.scheduler.http.api; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.util.Map; +import java.util.Optional; + +import javax.servlet.ServletException; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.ws.rs.core.MediaType; + +import org.apache.thrift.TException; +import org.apache.thrift.TProcessor; +import org.apache.thrift.protocol.TProtocol; +import org.apache.thrift.protocol.TProtocolFactory; +import org.apache.thrift.transport.TIOStreamTransport; +import org.apache.thrift.transport.TTransport; + +import static java.util.Objects.requireNonNull; + +import static javax.ws.rs.core.HttpHeaders.ACCEPT; + +/** + * An implementation of {@link org.apache.thrift.server.TServlet} that can handle multiple thrift + * protocols. The protocols are dispatched on HTTP headers. + */ +public class TContentAwareServlet extends HttpServlet { + private final TProcessor processor; + private final InputConfig inputConfig; + private final OutputConfig outputConfig; + + /** + * Class which contains the mapping of the factory and the content type of the output. + */ + static class ContentFactoryPair implements TProtocolFactory { + private final TProtocolFactory factory; + + private final MediaType outputType; + + ContentFactoryPair(TProtocolFactory factory, MediaType outputType) { + this.factory = requireNonNull(factory); + this.outputType = requireNonNull(outputType); + } + + MediaType getOutputType() { + return outputType; + } + + @Override + public TProtocol getProtocol(TTransport tTransport) { + return factory.getProtocol(tTransport); + } + } + + /** + * Configures how to interpret the Content-Type of the request. + */ + static class InputConfig { + // Type to use when there is no Content-Type + private final MediaType defaultType; + // Mapping of values in Content-Type to protocol to use to deserialize + private final Map<MediaType, ContentFactoryPair> inputMapping; + + InputConfig(MediaType defaultType, Map<MediaType, ContentFactoryPair> inputMapping) { + this.defaultType = requireNonNull(defaultType); + this.inputMapping = requireNonNull(inputMapping); + } + + Optional<ContentFactoryPair> getFactory(Optional<MediaType> mediaType) { + return Optional.ofNullable(inputMapping.get(mediaType.orElse(defaultType))); + } + } + + /** + * Configures how to interpret the Accept header of the request. The defaultType's factory is + * returned for almost all values to maintain backwards compatibility. + */ + static class OutputConfig { + // Type to use when there is no Accept header + private final MediaType defaultType; + // Mapping of MediaTypes in the Accept header to protocol used to serialize the response + private final Map<MediaType, ContentFactoryPair> outputMapping; + private final ContentFactoryPair defaultFactory; + + OutputConfig(MediaType defaultType, Map<MediaType, ContentFactoryPair> outputMapping) { + this.defaultType = requireNonNull(defaultType); + this.outputMapping = requireNonNull(outputMapping); + this.defaultFactory = requireNonNull(outputMapping.get(defaultType)); + } + + ContentFactoryPair getFactory(Optional<MediaType> type) { + return Optional.ofNullable(outputMapping.get(type.orElse(defaultType))) + .orElse(defaultFactory); + } + } + + TContentAwareServlet(TProcessor processor, InputConfig inputConfig, OutputConfig outputConfig) { + this.processor = requireNonNull(processor); + this.inputConfig = requireNonNull(inputConfig); + this.outputConfig = requireNonNull(outputConfig); + } + + @Override + protected void doPost(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + + Optional<ContentFactoryPair> factoryOptional = + inputConfig.getFactory(Optional.of(request.getContentType()).map(MediaType::valueOf)); + + if (!factoryOptional.isPresent()) { + response.setStatus(HttpServletResponse.SC_UNSUPPORTED_MEDIA_TYPE); + String msg = "Unsupported Content-Type: " + request.getContentType(); + response.getOutputStream().write(msg.getBytes(StandardCharsets.UTF_8)); + return; + } + + TTransport transport = + new TIOStreamTransport(request.getInputStream(), response.getOutputStream()); + + TProtocol inputProtocol = factoryOptional.get().getProtocol(transport); + + Optional<String> acceptHeader = Optional.ofNullable(request.getHeader(ACCEPT)); + Optional<MediaType> acceptType = Optional.empty(); + if (acceptHeader.isPresent()) { + try { + acceptType = acceptHeader.map(MediaType::valueOf); + } catch (IllegalArgumentException e) { + // Thrown if the Accept header contains more than one type or something else we can't + // parse, we just treat is as no header (which will pick up the default value). + acceptType = Optional.empty(); + } + } + + ContentFactoryPair outputProtocolFactory = outputConfig.getFactory(acceptType); + + response.setContentType(outputProtocolFactory.getOutputType().toString()); + TProtocol outputProtocol = outputProtocolFactory.getProtocol(transport); + try { + processor.process(inputProtocol, outputProtocol); + response.getOutputStream().flush(); + } catch (TException e) { + throw new ServletException(e); + } + } + + @Override + protected void doGet(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + doPost(request, response); + } +} http://git-wip-us.apache.org/repos/asf/aurora/blob/0105a151/src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java b/src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java index 7dbe48b..c2ceb4e 100644 --- a/src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java +++ b/src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java @@ -175,6 +175,12 @@ public abstract class AbstractJettyTest extends EasyMockTest { return String.format("http://%s:%s%s", httpServer.getHostText(), httpServer.getPort(), path); } + protected WebResource.Builder getPlainRequestBuilder(String path) { + assertNotNull("HTTP server must be started first", httpServer); + Client client = Client.create(new DefaultClientConfig()); + return client.resource(makeUrl(path)).getRequestBuilder(); + } + protected WebResource.Builder getRequestBuilder(String path) { assertNotNull("HTTP server must be started first", httpServer); ClientConfig config = new DefaultClientConfig(); http://git-wip-us.apache.org/repos/asf/aurora/blob/0105a151/src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java b/src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java index 31f5cb3..0a3ff05 100644 --- a/src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java +++ b/src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java @@ -13,8 +13,14 @@ */ package org.apache.aurora.scheduler.http.api; +import java.util.Arrays; +import java.util.List; + import javax.ws.rs.core.HttpHeaders; +import javax.ws.rs.core.MediaType; +import com.google.common.collect.ImmutableList; +import com.google.common.primitives.Bytes; import com.google.inject.AbstractModule; import com.google.inject.Module; import com.google.inject.util.Modules; @@ -26,10 +32,15 @@ import org.apache.aurora.scheduler.thrift.aop.AnnotatedAuroraAdmin; import org.junit.Before; import org.junit.Test; +import static javax.servlet.http.HttpServletResponse.SC_OK; +import static javax.servlet.http.HttpServletResponse.SC_UNSUPPORTED_MEDIA_TYPE; +import static javax.ws.rs.core.HttpHeaders.CONTENT_TYPE; + import static org.easymock.EasyMock.expect; import static org.junit.Assert.assertEquals; public class ApiIT extends AbstractJettyTest { + private static final String JSON_FIXTURE = "[1,\"getRoleSummary\",1,0,{}]"; private AnnotatedAuroraAdmin thrift; @Before @@ -58,8 +69,65 @@ public class ApiIT extends AbstractJettyTest { ClientResponse response = getRequestBuilder(ApiModule.API_PATH) .header(HttpHeaders.ACCEPT_ENCODING, "gzip") .type("application/x-thrift") - .post(ClientResponse.class, "[1,\"getRoleSummary\",1,0,{}]"); + .post(ClientResponse.class, JSON_FIXTURE); + assertEquals(SC_OK, response.getStatus()); assertEquals("gzip", response.getHeaders().getFirst(HttpHeaders.CONTENT_ENCODING)); } + + @Test + public void testThriftJsonAccepted() throws Exception { + expect(thrift.getRoleSummary()).andReturn(new Response()); + + replayAndStart(); + + ClientResponse response = getPlainRequestBuilder(ApiModule.API_PATH) + .type("application/vnd.apache.thrift.json") + .accept("application/vnd.apache.thrift.json") + .post(ClientResponse.class, JSON_FIXTURE); + + assertEquals(SC_OK, response.getStatus()); + assertEquals( + "application/vnd.apache.thrift.json", + response.getHeaders().getFirst(CONTENT_TYPE)); + } + + @Test + public void testUnknownContentTypeRejected() throws Exception { + replayAndStart(); + + ClientResponse response = getRequestBuilder(ApiModule.API_PATH) + .type(MediaType.TEXT_HTML_TYPE) + .post(ClientResponse.class, JSON_FIXTURE); + + assertEquals(SC_UNSUPPORTED_MEDIA_TYPE, response.getStatus()); + } + + @Test + public void testBinaryContentTypeAccepted() throws Exception { + expect(thrift.getRoleSummary()).andReturn(new Response()); + + replayAndStart(); + + // This fixture represents a 'getRoleSummary' call encoded as binary thrift. + List<Integer> fixture = ImmutableList.<Integer>builder() + .addAll(ImmutableList.of(-128, 1, 0, 1, 0, 0, 0, 14, 103)) + .addAll(ImmutableList.of(101, 116, 82, 111, 108, 101, 83, 117, 109)) + .addAll(ImmutableList.of(109, 97, 114, 121, 0, 0, 0, 1, 0)) + .addAll(ImmutableList.of(0, 0, 0, 0, 0)) + .build(); + + // Note the array has to be exactly 27 bytes long. + byte[] rawBytes = Arrays.copyOf(Bytes.toArray(fixture), 27); + + ClientResponse response = getPlainRequestBuilder(ApiModule.API_PATH) + .type("application/vnd.apache.thrift.binary") + .accept("application/vnd.apache.thrift.binary") + .post(ClientResponse.class, rawBytes); + + assertEquals(SC_OK, response.getStatus()); + assertEquals( + "application/vnd.apache.thrift.binary", + response.getHeaders().getFirst(CONTENT_TYPE)); + } }