From 0925b7eee640edfad58632b794ca606b0fe06fad Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 27 Sep 2023 18:08:45 -0400 Subject: [PATCH 01/26] Make the decompressor extensible and allow for a plugin to define a step in the pipeline to analyze request headers Signed-off-by: Craig Perkins --- .../netty4/AbstractNetty4HttpRequest.java | 142 ++++++++++++++++++ .../http/netty4/Netty4DefaultHttpRequest.java | 114 ++++++++++++++ .../http/netty4/Netty4HttpRequest.java | 119 +-------------- .../http/netty4/Netty4HttpRequestHandler.java | 8 +- .../http/netty4/Netty4HttpResponse.java | 2 +- .../netty4/Netty4HttpServerTransport.java | 45 ++++-- .../http/nio/HttpReadWriteHandler.java | 3 +- .../http/nio/HttpReadWriteHandlerTests.java | 13 +- .../http/AbstractHttpServerTransport.java | 109 ++++++++++++-- .../rest/DelegatingRestHandler.java | 74 +++++++++ .../org/opensearch/rest/RestController.java | 2 +- .../java/org/opensearch/rest/RestHandler.java | 66 +------- .../opensearch/rest/RestHandlerContext.java | 44 ++++++ .../java/org/opensearch/rest/RestRequest.java | 61 +++++++- .../AbstractHttpServerTransportTests.java | 13 +- .../rest/DelegatingRestHandlerTests.java | 58 +++++++ 16 files changed, 649 insertions(+), 224 deletions(-) create mode 100644 modules/transport-netty4/src/main/java/org/opensearch/http/netty4/AbstractNetty4HttpRequest.java create mode 100644 modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4DefaultHttpRequest.java create mode 100644 server/src/main/java/org/opensearch/rest/DelegatingRestHandler.java create mode 100644 server/src/main/java/org/opensearch/rest/RestHandlerContext.java create mode 100644 server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/AbstractNetty4HttpRequest.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/AbstractNetty4HttpRequest.java new file mode 100644 index 0000000000000..27489af161117 --- /dev/null +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/AbstractNetty4HttpRequest.java @@ -0,0 +1,142 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.http.netty4; + +import org.opensearch.rest.RestRequest; + +import java.util.AbstractMap; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; + +import io.netty.handler.codec.http.HttpHeaders; +import io.netty.handler.codec.http.HttpMethod; +import io.netty.handler.codec.http.HttpRequest; + +public class AbstractNetty4HttpRequest { + + protected HttpHeadersMap headers; + protected Exception inboundException; + + protected RestRequest.Method getHttpMethod(HttpRequest request) { + HttpMethod httpMethod = request.method(); + if (httpMethod == HttpMethod.GET) return RestRequest.Method.GET; + + if (httpMethod == HttpMethod.POST) return RestRequest.Method.POST; + + if (httpMethod == HttpMethod.PUT) return RestRequest.Method.PUT; + + if (httpMethod == HttpMethod.DELETE) return RestRequest.Method.DELETE; + + if (httpMethod == HttpMethod.HEAD) { + return RestRequest.Method.HEAD; + } + + if (httpMethod == HttpMethod.OPTIONS) { + return RestRequest.Method.OPTIONS; + } + + if (httpMethod == HttpMethod.PATCH) { + return RestRequest.Method.PATCH; + } + + if (httpMethod == HttpMethod.TRACE) { + return RestRequest.Method.TRACE; + } + + if (httpMethod == HttpMethod.CONNECT) { + return RestRequest.Method.CONNECT; + } + + throw new IllegalArgumentException("Unexpected http method: " + httpMethod); + } + + /** + * A wrapper of {@link HttpHeaders} that implements a map to prevent copying unnecessarily. This class does not support modifications + * and due to the underlying implementation, it performs case insensitive lookups of key to values. + * + * It is important to note that this implementation does have some downsides in that each invocation of the + * {@link #values()} and {@link #entrySet()} methods will perform a copy of the values in the HttpHeaders rather than returning a + * view of the underlying values. + */ + protected static class HttpHeadersMap implements Map> { + + private final HttpHeaders httpHeaders; + + HttpHeadersMap(HttpHeaders httpHeaders) { + this.httpHeaders = httpHeaders; + } + + @Override + public int size() { + return httpHeaders.size(); + } + + @Override + public boolean isEmpty() { + return httpHeaders.isEmpty(); + } + + @Override + public boolean containsKey(Object key) { + return key instanceof String && httpHeaders.contains((String) key); + } + + @Override + public boolean containsValue(Object value) { + return value instanceof List && httpHeaders.names().stream().map(httpHeaders::getAll).anyMatch(value::equals); + } + + @Override + public List get(Object key) { + return key instanceof String ? httpHeaders.getAll((String) key) : null; + } + + @Override + public List put(String key, List value) { + throw new UnsupportedOperationException("modifications are not supported"); + } + + @Override + public List remove(Object key) { + throw new UnsupportedOperationException("modifications are not supported"); + } + + @Override + public void putAll(Map> m) { + throw new UnsupportedOperationException("modifications are not supported"); + } + + @Override + public void clear() { + throw new UnsupportedOperationException("modifications are not supported"); + } + + @Override + public Set keySet() { + return httpHeaders.names(); + } + + @Override + public Collection> values() { + return httpHeaders.names().stream().map(k -> Collections.unmodifiableList(httpHeaders.getAll(k))).collect(Collectors.toList()); + } + + @Override + public Set>> entrySet() { + return httpHeaders.names() + .stream() + .map(k -> new AbstractMap.SimpleImmutableEntry<>(k, httpHeaders.getAll(k))) + .collect(Collectors.toSet()); + } + } +} diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4DefaultHttpRequest.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4DefaultHttpRequest.java new file mode 100644 index 0000000000000..59f5e9f308aa4 --- /dev/null +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4DefaultHttpRequest.java @@ -0,0 +1,114 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.http.netty4; + +import org.opensearch.core.common.bytes.BytesArray; +import org.opensearch.core.common.bytes.BytesReference; +import org.opensearch.core.rest.RestStatus; +import org.opensearch.http.HttpRequest; +import org.opensearch.rest.RestRequest; + +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import io.netty.handler.codec.http.DefaultHttpRequest; +import io.netty.handler.codec.http.HttpHeaderNames; +import io.netty.handler.codec.http.cookie.Cookie; +import io.netty.handler.codec.http.cookie.ServerCookieDecoder; +import io.netty.handler.codec.http.cookie.ServerCookieEncoder; + +public class Netty4DefaultHttpRequest extends AbstractNetty4HttpRequest implements HttpRequest { + + private final DefaultHttpRequest request; + + public Netty4DefaultHttpRequest(DefaultHttpRequest request) { + this(request, new HttpHeadersMap(request.headers()), null); + } + + private Netty4DefaultHttpRequest(DefaultHttpRequest request, HttpHeadersMap headers, Exception inboundException) { + this.request = request; + this.headers = headers; + this.inboundException = inboundException; + } + + @Override + public RestRequest.Method method() { + return getHttpMethod(request); + } + + @Override + public String uri() { + return request.uri(); + } + + @Override + public BytesReference content() { + // throw new RuntimeException("Not implemented"); + return BytesArray.EMPTY; + } + + @Override + public final Map> getHeaders() { + return headers; + } + + @Override + public List strictCookies() { + String cookieString = request.headers().get(HttpHeaderNames.COOKIE); + if (cookieString != null) { + Set cookies = ServerCookieDecoder.STRICT.decode(cookieString); + if (!cookies.isEmpty()) { + return ServerCookieEncoder.STRICT.encode(cookies); + } + } + return Collections.emptyList(); + } + + @Override + public HttpVersion protocolVersion() { + if (request.protocolVersion().equals(io.netty.handler.codec.http.HttpVersion.HTTP_1_0)) { + return HttpRequest.HttpVersion.HTTP_1_0; + } else if (request.protocolVersion().equals(io.netty.handler.codec.http.HttpVersion.HTTP_1_1)) { + return HttpRequest.HttpVersion.HTTP_1_1; + } else { + throw new IllegalArgumentException("Unexpected http protocol version: " + request.protocolVersion()); + } + } + + @Override + public HttpRequest removeHeader(String header) { + return null; + } + + @Override + public Netty4HttpResponse createResponse(RestStatus status, BytesReference content) { + return new Netty4HttpResponse(request.headers(), request.protocolVersion(), status, content); + } + + @Override + public Exception getInboundException() { + return inboundException; + } + + @Override + public void release() { + // do nothing + } + + @Override + public HttpRequest releaseAndCopy() { + return this; + } + + public DefaultHttpRequest nettyRequest() { + return request; + } +} diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java index 7d937157c1034..7e71e65c08c57 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java @@ -38,14 +38,11 @@ import org.opensearch.rest.RestRequest; import org.opensearch.transport.netty4.Netty4Utils; -import java.util.AbstractMap; -import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.stream.Collectors; import io.netty.buffer.ByteBuf; import io.netty.buffer.Unpooled; @@ -54,18 +51,15 @@ import io.netty.handler.codec.http.FullHttpRequest; import io.netty.handler.codec.http.HttpHeaderNames; import io.netty.handler.codec.http.HttpHeaders; -import io.netty.handler.codec.http.HttpMethod; import io.netty.handler.codec.http.cookie.Cookie; import io.netty.handler.codec.http.cookie.ServerCookieDecoder; import io.netty.handler.codec.http.cookie.ServerCookieEncoder; -public class Netty4HttpRequest implements HttpRequest { +public class Netty4HttpRequest extends AbstractNetty4HttpRequest implements HttpRequest { private final FullHttpRequest request; private final BytesReference content; - private final HttpHeadersMap headers; private final AtomicBoolean released; - private final Exception inboundException; private final boolean pooled; Netty4HttpRequest(FullHttpRequest request) { @@ -117,36 +111,7 @@ private Netty4HttpRequest( @Override public RestRequest.Method method() { - HttpMethod httpMethod = request.method(); - if (httpMethod == HttpMethod.GET) return RestRequest.Method.GET; - - if (httpMethod == HttpMethod.POST) return RestRequest.Method.POST; - - if (httpMethod == HttpMethod.PUT) return RestRequest.Method.PUT; - - if (httpMethod == HttpMethod.DELETE) return RestRequest.Method.DELETE; - - if (httpMethod == HttpMethod.HEAD) { - return RestRequest.Method.HEAD; - } - - if (httpMethod == HttpMethod.OPTIONS) { - return RestRequest.Method.OPTIONS; - } - - if (httpMethod == HttpMethod.PATCH) { - return RestRequest.Method.PATCH; - } - - if (httpMethod == HttpMethod.TRACE) { - return RestRequest.Method.TRACE; - } - - if (httpMethod == HttpMethod.CONNECT) { - return RestRequest.Method.CONNECT; - } - - throw new IllegalArgumentException("Unexpected http method: " + httpMethod); + return getHttpMethod(request); } @Override @@ -254,84 +219,4 @@ public Exception getInboundException() { public FullHttpRequest nettyRequest() { return request; } - - /** - * A wrapper of {@link HttpHeaders} that implements a map to prevent copying unnecessarily. This class does not support modifications - * and due to the underlying implementation, it performs case insensitive lookups of key to values. - * - * It is important to note that this implementation does have some downsides in that each invocation of the - * {@link #values()} and {@link #entrySet()} methods will perform a copy of the values in the HttpHeaders rather than returning a - * view of the underlying values. - */ - private static class HttpHeadersMap implements Map> { - - private final HttpHeaders httpHeaders; - - private HttpHeadersMap(HttpHeaders httpHeaders) { - this.httpHeaders = httpHeaders; - } - - @Override - public int size() { - return httpHeaders.size(); - } - - @Override - public boolean isEmpty() { - return httpHeaders.isEmpty(); - } - - @Override - public boolean containsKey(Object key) { - return key instanceof String && httpHeaders.contains((String) key); - } - - @Override - public boolean containsValue(Object value) { - return value instanceof List && httpHeaders.names().stream().map(httpHeaders::getAll).anyMatch(value::equals); - } - - @Override - public List get(Object key) { - return key instanceof String ? httpHeaders.getAll((String) key) : null; - } - - @Override - public List put(String key, List value) { - throw new UnsupportedOperationException("modifications are not supported"); - } - - @Override - public List remove(Object key) { - throw new UnsupportedOperationException("modifications are not supported"); - } - - @Override - public void putAll(Map> m) { - throw new UnsupportedOperationException("modifications are not supported"); - } - - @Override - public void clear() { - throw new UnsupportedOperationException("modifications are not supported"); - } - - @Override - public Set keySet() { - return httpHeaders.names(); - } - - @Override - public Collection> values() { - return httpHeaders.names().stream().map(k -> Collections.unmodifiableList(httpHeaders.getAll(k))).collect(Collectors.toList()); - } - - @Override - public Set>> entrySet() { - return httpHeaders.names() - .stream() - .map(k -> new AbstractMap.SimpleImmutableEntry<>(k, httpHeaders.getAll(k))) - .collect(Collectors.toSet()); - } - } } diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java index 1f7aaf17d2191..cc331f1b4fd2d 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java @@ -33,7 +33,10 @@ package org.opensearch.http.netty4; import org.opensearch.ExceptionsHelper; +import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.http.HttpPipelinedRequest; +import org.opensearch.rest.RestHandlerContext; +import org.opensearch.rest.RestResponse; import io.netty.channel.ChannelHandler; import io.netty.channel.ChannelHandlerContext; @@ -51,9 +54,12 @@ class Netty4HttpRequestHandler extends SimpleChannelInboundHandler HTTP_CHANNEL_KEY = AttributeKey.newInstance("opensearch-http-channel"); + public static final AttributeKey HTTP_CHANNEL_KEY = AttributeKey.newInstance("opensearch-http-channel"); protected static final AttributeKey HTTP_SERVER_CHANNEL_KEY = AttributeKey.newInstance( "opensearch-http-server-channel" ); + public static final AttributeKey EARLY_RESPONSE = AttributeKey.newInstance("opensearch-http-early-response"); + public static final AttributeKey CONTEXT_TO_RESTORE = AttributeKey.newInstance( + "opensearch-http-request-thread-context" + ); + protected static class HttpChannelHandler extends ChannelInitializer { private final Netty4HttpServerTransport transport; @@ -419,15 +429,11 @@ protected void channelRead0(ChannelHandlerContext ctx, HttpMessage msg) throws E // If this handler is hit then no upgrade has been attempted and the client is just talking HTTP final ChannelPipeline pipeline = ctx.pipeline(); pipeline.addAfter(ctx.name(), "handler", getRequestHandler()); - pipeline.replace(this, "decoder_compress", new HttpContentDecompressor()); - - pipeline.addAfter("decoder_compress", "aggregator", aggregator); + pipeline.replace(this, "header_verifier", transport.getHeaderVerifier()); + pipeline.addAfter("header_verifier", "decompress", transport.getDecompressor()); + pipeline.addAfter("decompress", "aggregator", aggregator); if (handlingSettings.isCompression()) { - pipeline.addAfter( - "aggregator", - "encoder_compress", - new HttpContentCompressor(handlingSettings.getCompressionLevel()) - ); + pipeline.addAfter("aggregator", "compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); } pipeline.addBefore("handler", "request_creator", requestCreator); pipeline.addBefore("handler", "response_creator", responseCreator); @@ -446,13 +452,14 @@ protected void configureDefaultHttpPipeline(ChannelPipeline pipeline) { ); decoder.setCumulator(ByteToMessageDecoder.COMPOSITE_CUMULATOR); pipeline.addLast("decoder", decoder); - pipeline.addLast("decoder_compress", new HttpContentDecompressor()); + pipeline.addLast("header_verifier", transport.getHeaderVerifier()); + pipeline.addLast("decompress", transport.getDecompressor()); pipeline.addLast("encoder", new HttpResponseEncoder()); final HttpObjectAggregator aggregator = new HttpObjectAggregator(handlingSettings.getMaxContentLength()); aggregator.setMaxCumulationBufferComponents(transport.maxCompositeBufferComponents); pipeline.addLast("aggregator", aggregator); if (handlingSettings.isCompression()) { - pipeline.addLast("encoder_compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); + pipeline.addLast("compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); } pipeline.addLast("request_creator", requestCreator); pipeline.addLast("response_creator", responseCreator); @@ -487,17 +494,16 @@ protected void initChannel(Channel childChannel) throws Exception { final HttpObjectAggregator aggregator = new HttpObjectAggregator(handlingSettings.getMaxContentLength()); aggregator.setMaxCumulationBufferComponents(transport.maxCompositeBufferComponents); - childChannel.pipeline() .addLast(new LoggingHandler(LogLevel.DEBUG)) .addLast(new Http2StreamFrameToHttpObjectCodec(true)) .addLast("byte_buf_sizer", byteBufSizer) .addLast("read_timeout", new ReadTimeoutHandler(transport.readTimeoutMillis, TimeUnit.MILLISECONDS)) - .addLast("decoder_decompress", new HttpContentDecompressor()); + .addLast("header_verifier", transport.getHeaderVerifier()) + .addLast("decompress", transport.getDecompressor()); if (handlingSettings.isCompression()) { - childChannel.pipeline() - .addLast("encoder_compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); + childChannel.pipeline().addLast("compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); } childChannel.pipeline() @@ -531,4 +537,13 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { } } } + + protected HttpContentDecompressor getDecompressor() { + return DEFAULT_DECOMPRESSOR; + } + + protected ChannelInboundHandlerAdapter getHeaderVerifier() { + // pass-through + return DEFAULT_HEADER_VERIFIER; + } } diff --git a/plugins/transport-nio/src/main/java/org/opensearch/http/nio/HttpReadWriteHandler.java b/plugins/transport-nio/src/main/java/org/opensearch/http/nio/HttpReadWriteHandler.java index d44515f3dc727..7b04f93d5c3be 100644 --- a/plugins/transport-nio/src/main/java/org/opensearch/http/nio/HttpReadWriteHandler.java +++ b/plugins/transport-nio/src/main/java/org/opensearch/http/nio/HttpReadWriteHandler.java @@ -43,6 +43,7 @@ import org.opensearch.nio.SocketChannelContext; import org.opensearch.nio.TaskScheduler; import org.opensearch.nio.WriteOperation; +import org.opensearch.rest.RestHandlerContext; import java.io.IOException; import java.util.ArrayList; @@ -172,7 +173,7 @@ private void handleRequest(Object msg) { final HttpPipelinedRequest pipelinedRequest = (HttpPipelinedRequest) msg; boolean success = false; try { - transport.incomingRequest(pipelinedRequest, nioHttpChannel); + transport.incomingRequest(pipelinedRequest, nioHttpChannel, RestHandlerContext.EMPTY); success = true; } finally { if (success == false) { diff --git a/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java b/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java index a3f7a7822cd40..10c1242ef1a94 100644 --- a/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java +++ b/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java @@ -48,6 +48,7 @@ import org.opensearch.nio.InboundChannelBuffer; import org.opensearch.nio.SocketChannelContext; import org.opensearch.nio.TaskScheduler; +import org.opensearch.rest.RestHandlerContext; import org.opensearch.rest.RestRequest; import org.opensearch.test.OpenSearchTestCase; import org.junit.Before; @@ -101,7 +102,7 @@ public void setMocks() { doAnswer(invocation -> { ((HttpRequest) invocation.getArguments()[0]).releaseAndCopy(); return null; - }).when(transport).incomingRequest(any(HttpRequest.class), any(HttpChannel.class)); + }).when(transport).incomingRequest(any(HttpRequest.class), any(HttpChannel.class), RestHandlerContext.EMPTY); Settings settings = Settings.builder().put(SETTING_HTTP_MAX_CONTENT_LENGTH.getKey(), new ByteSizeValue(1024)).build(); HttpHandlingSettings httpHandlingSettings = HttpHandlingSettings.fromSettings(settings); channel = mock(NioHttpChannel.class); @@ -122,12 +123,12 @@ public void testSuccessfulDecodeHttpRequest() throws IOException { try { handler.consumeReads(toChannelBuffer(slicedBuf)); - verify(transport, times(0)).incomingRequest(any(HttpRequest.class), any(NioHttpChannel.class)); + verify(transport, times(0)).incomingRequest(any(HttpRequest.class), any(NioHttpChannel.class), RestHandlerContext.EMPTY); handler.consumeReads(toChannelBuffer(slicedBuf2)); ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(HttpRequest.class); - verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class)); + verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class), RestHandlerContext.EMPTY); HttpRequest nioHttpRequest = requestCaptor.getValue(); assertEquals(HttpRequest.HttpVersion.HTTP_1_1, nioHttpRequest.protocolVersion()); @@ -153,7 +154,7 @@ public void testDecodeHttpRequestError() throws IOException { handler.consumeReads(toChannelBuffer(buf)); ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(HttpRequest.class); - verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class)); + verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class), RestHandlerContext.EMPTY); assertNotNull(requestCaptor.getValue().getInboundException()); assertTrue(requestCaptor.getValue().getInboundException() instanceof IllegalArgumentException); @@ -174,7 +175,7 @@ public void testDecodeHttpRequestContentLengthToLongGeneratesOutboundMessage() t } finally { buf.release(); } - verify(transport, times(0)).incomingRequest(any(), any()); + verify(transport, times(0)).incomingRequest(any(), any(), RestHandlerContext.EMPTY); List flushOperations = handler.pollFlushOperations(); assertFalse(flushOperations.isEmpty()); @@ -280,7 +281,7 @@ private void prepareHandlerForResponse(HttpReadWriteHandler handler) throws IOEx } ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(HttpPipelinedRequest.class); - verify(transport, atLeastOnce()).incomingRequest(requestCaptor.capture(), any(HttpChannel.class)); + verify(transport, atLeastOnce()).incomingRequest(requestCaptor.capture(), any(HttpChannel.class), RestHandlerContext.EMPTY); HttpRequest httpRequest = requestCaptor.getValue(); assertNotNull(httpRequest); diff --git a/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java b/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java index ed44102d0abe4..61c2fa01c9c06 100644 --- a/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java +++ b/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java @@ -53,6 +53,7 @@ import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.rest.RestChannel; +import org.opensearch.rest.RestHandlerContext; import org.opensearch.rest.RestRequest; import org.opensearch.telemetry.tracing.Span; import org.opensearch.telemetry.tracing.SpanBuilder; @@ -99,7 +100,7 @@ public abstract class AbstractHttpServerTransport extends AbstractLifecycleCompo protected final ThreadPool threadPool; protected final Dispatcher dispatcher; protected final CorsHandler corsHandler; - private final NamedXContentRegistry xContentRegistry; + protected final NamedXContentRegistry xContentRegistry; protected final PortsRange port; protected final ByteSizeValue maxContentLength; @@ -112,7 +113,7 @@ public abstract class AbstractHttpServerTransport extends AbstractLifecycleCompo private final Set httpServerChannels = Collections.newSetFromMap(new ConcurrentHashMap<>()); private final HttpTracer httpTracer; - private final Tracer tracer; + protected final Tracer tracer; protected AbstractHttpServerTransport( Settings settings, @@ -359,20 +360,29 @@ protected void serverAcceptedChannel(HttpChannel httpChannel) { * * @param httpRequest that is incoming * @param httpChannel that received the http request + * @param requestContext context carried over to the request handler from earlier stages in the request pipeline */ - public void incomingRequest(final HttpRequest httpRequest, final HttpChannel httpChannel) { + public void incomingRequest(final HttpRequest httpRequest, final HttpChannel httpChannel, final RestHandlerContext requestContext) { final Span span = tracer.startSpan(SpanBuilder.from(httpRequest), httpRequest.getHeaders()); try (final SpanScope httpRequestSpanScope = tracer.withSpanInScope(span)) { HttpChannel traceableHttpChannel = TraceableHttpChannel.create(httpChannel, span, tracer); - handleIncomingRequest(httpRequest, traceableHttpChannel, httpRequest.getInboundException()); + handleIncomingRequest(httpRequest, traceableHttpChannel, requestContext, httpRequest.getInboundException()); } } // Visible for testing - void dispatchRequest(final RestRequest restRequest, final RestChannel channel, final Throwable badRequestCause) { + protected void dispatchRequest( + final RestRequest restRequest, + final RestChannel channel, + final Throwable badRequestCause, + final ThreadContext.StoredContext storedContext + ) { RestChannel traceableRestChannel = channel; final ThreadContext threadContext = threadPool.getThreadContext(); try (ThreadContext.StoredContext ignore = threadContext.stashContext()) { + if (storedContext != null) { + storedContext.restore(); + } final Span span = tracer.startSpan(SpanBuilder.from(restRequest)); try (final SpanScope spanScope = tracer.withSpanInScope(span)) { if (channel != null) { @@ -388,7 +398,12 @@ void dispatchRequest(final RestRequest restRequest, final RestChannel channel, f } - private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChannel httpChannel, final Exception exception) { + private void handleIncomingRequest( + final HttpRequest httpRequest, + final HttpChannel httpChannel, + final RestHandlerContext requestContext, + final Exception exception + ) { if (exception == null) { HttpResponse earlyResponse = corsHandler.handleInbound(httpRequest); if (earlyResponse != null) { @@ -411,13 +426,13 @@ private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChan { RestRequest innerRestRequest; try { - innerRestRequest = RestRequest.request(xContentRegistry, httpRequest, httpChannel); + innerRestRequest = RestRequest.request(xContentRegistry, httpRequest, httpChannel, true); } catch (final RestRequest.ContentTypeHeaderException e) { badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); - innerRestRequest = requestWithoutContentTypeHeader(httpRequest, httpChannel, badRequestCause); + innerRestRequest = requestWithoutContentTypeHeader(httpRequest, httpChannel, badRequestCause, true); } catch (final RestRequest.BadParameterException e) { badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); - innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel); + innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel, true); } restRequest = innerRestRequest; } @@ -462,16 +477,84 @@ private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChan channel = innerChannel; } - dispatchRequest(restRequest, channel, badRequestCause); + if (requestContext.hasEarlyResponse()) { + channel.sendResponse(requestContext.getEarlyResponse()); + return; + } + + dispatchRequest(restRequest, channel, badRequestCause, requestContext.getContextToRestore()); + } + + public static RestRequest createRestRequest( + final NamedXContentRegistry xContentRegistry, + final HttpRequest httpRequest, + final HttpChannel httpChannel + ) { + // TODO Figure out how to only generate one request ID for each request in the pipeline. + Exception badRequestCause = httpRequest.getInboundException(); + + /* + * We want to create a REST request from the incoming request from Netty. However, creating this request could fail if there + * are incorrectly encoded parameters, or the Content-Type header is invalid. If one of these specific failures occurs, we + * attempt to create a REST request again without the input that caused the exception (e.g., we remove the Content-Type header, + * or skip decoding the parameters). Once we have a request in hand, we then dispatch the request as a bad request with the + * underlying exception that caused us to treat the request as bad. + */ + final RestRequest restRequest; + { + RestRequest innerRestRequest; + try { + innerRestRequest = RestRequest.request(xContentRegistry, httpRequest, httpChannel, false); + } catch (final RestRequest.ContentTypeHeaderException e) { + badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); + innerRestRequest = requestWithoutContentTypeHeader(xContentRegistry, httpRequest, httpChannel, badRequestCause, false); + } catch (final RestRequest.BadParameterException e) { + badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); + innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel, false); + } + restRequest = innerRestRequest; + } + return restRequest; + } + + private static RestRequest requestWithoutContentTypeHeader( + NamedXContentRegistry xContentRegistry, + HttpRequest httpRequest, + HttpChannel httpChannel, + Exception badRequestCause, + boolean shouldGenerateRequestId + ) { + HttpRequest httpRequestWithoutContentType = httpRequest.removeHeader("Content-Type"); + try { + return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel, shouldGenerateRequestId); + } catch (final RestRequest.BadParameterException e) { + badRequestCause.addSuppressed(e); + return RestRequest.requestWithoutParameters( + xContentRegistry, + httpRequestWithoutContentType, + httpChannel, + shouldGenerateRequestId + ); + } } - private RestRequest requestWithoutContentTypeHeader(HttpRequest httpRequest, HttpChannel httpChannel, Exception badRequestCause) { + private RestRequest requestWithoutContentTypeHeader( + HttpRequest httpRequest, + HttpChannel httpChannel, + Exception badRequestCause, + boolean shouldGenerateRequestId + ) { HttpRequest httpRequestWithoutContentType = httpRequest.removeHeader("Content-Type"); try { - return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel); + return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel, shouldGenerateRequestId); } catch (final RestRequest.BadParameterException e) { badRequestCause.addSuppressed(e); - return RestRequest.requestWithoutParameters(xContentRegistry, httpRequestWithoutContentType, httpChannel); + return RestRequest.requestWithoutParameters( + xContentRegistry, + httpRequestWithoutContentType, + httpChannel, + shouldGenerateRequestId + ); } } diff --git a/server/src/main/java/org/opensearch/rest/DelegatingRestHandler.java b/server/src/main/java/org/opensearch/rest/DelegatingRestHandler.java new file mode 100644 index 0000000000000..928ff94d8c1d3 --- /dev/null +++ b/server/src/main/java/org/opensearch/rest/DelegatingRestHandler.java @@ -0,0 +1,74 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.rest; + +import org.opensearch.client.node.NodeClient; + +import java.util.List; +import java.util.Objects; + +/** + * Delegating RestHandler that delegates all implementations to original handler + * + * @opensearch.api + */ +public class DelegatingRestHandler implements RestHandler { + + protected final RestHandler delegate; + + public DelegatingRestHandler(RestHandler delegate) { + Objects.requireNonNull(delegate, "RestHandler delegate can not be null"); + this.delegate = delegate; + } + + @Override + public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + delegate.handleRequest(request, channel, client); + } + + @Override + public boolean canTripCircuitBreaker() { + return delegate.canTripCircuitBreaker(); + } + + @Override + public boolean supportsContentStream() { + return delegate.supportsContentStream(); + } + + @Override + public boolean allowsUnsafeBuffers() { + return delegate.allowsUnsafeBuffers(); + } + + @Override + public List routes() { + return delegate.routes(); + } + + @Override + public List deprecatedRoutes() { + return delegate.deprecatedRoutes(); + } + + @Override + public List replacedRoutes() { + return delegate.replacedRoutes(); + } + + @Override + public boolean allowSystemIndexAccessByDefault() { + return delegate.allowSystemIndexAccessByDefault(); + } + + @Override + public String toString() { + return delegate.toString(); + } +} diff --git a/server/src/main/java/org/opensearch/rest/RestController.java b/server/src/main/java/org/opensearch/rest/RestController.java index ac30f999d0da7..4929f2a147dae 100644 --- a/server/src/main/java/org/opensearch/rest/RestController.java +++ b/server/src/main/java/org/opensearch/rest/RestController.java @@ -131,7 +131,7 @@ public RestController( this.headersToCopy = headersToCopy; this.usageService = usageService; if (handlerWrapper == null) { - handlerWrapper = h -> h; // passthrough if no wrapper set + handlerWrapper = (delegate) -> new DelegatingRestHandler(delegate); } this.handlerWrapper = handlerWrapper; this.client = client; diff --git a/server/src/main/java/org/opensearch/rest/RestHandler.java b/server/src/main/java/org/opensearch/rest/RestHandler.java index 7832649e8ad32..edb1cb341d2d8 100644 --- a/server/src/main/java/org/opensearch/rest/RestHandler.java +++ b/server/src/main/java/org/opensearch/rest/RestHandler.java @@ -44,6 +44,8 @@ /** * Handler for REST requests * + * If new methods are added to this interface they must also be added to {@link DelegatingRestHandler} + * * @opensearch.api */ @FunctionalInterface @@ -108,75 +110,13 @@ default List replacedRoutes() { } /** - * Controls whether requests handled by this class are allowed to to access system indices by default. + * Controls whether requests handled by this class are allowed to access system indices by default. * @return {@code true} if requests handled by this class should be allowed to access system indices. */ default boolean allowSystemIndexAccessByDefault() { return false; } - static RestHandler wrapper(RestHandler delegate) { - return new Wrapper(delegate); - } - - /** - * Wrapper for a handler. - * - * @opensearch.internal - */ - class Wrapper implements RestHandler { - private final RestHandler delegate; - - public Wrapper(RestHandler delegate) { - this.delegate = Objects.requireNonNull(delegate, "RestHandler delegate can not be null"); - } - - @Override - public String toString() { - return delegate.toString(); - } - - @Override - public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { - delegate.handleRequest(request, channel, client); - } - - @Override - public boolean canTripCircuitBreaker() { - return delegate.canTripCircuitBreaker(); - } - - @Override - public boolean supportsContentStream() { - return delegate.supportsContentStream(); - } - - @Override - public boolean allowsUnsafeBuffers() { - return delegate.allowsUnsafeBuffers(); - } - - @Override - public List routes() { - return delegate.routes(); - } - - @Override - public List deprecatedRoutes() { - return delegate.deprecatedRoutes(); - } - - @Override - public List replacedRoutes() { - return delegate.replacedRoutes(); - } - - @Override - public boolean allowSystemIndexAccessByDefault() { - return delegate.allowSystemIndexAccessByDefault(); - } - } - /** * Route for the request. * diff --git a/server/src/main/java/org/opensearch/rest/RestHandlerContext.java b/server/src/main/java/org/opensearch/rest/RestHandlerContext.java new file mode 100644 index 0000000000000..297a44c705e2e --- /dev/null +++ b/server/src/main/java/org/opensearch/rest/RestHandlerContext.java @@ -0,0 +1,44 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.rest; + +import org.opensearch.common.util.concurrent.ThreadContext; + +/** + * Holder for information that is shared between stages of the request pipeline + */ +public class RestHandlerContext { + private RestResponse earlyResponse; + private ThreadContext.StoredContext contextToRestore; + + public static RestHandlerContext EMPTY = new RestHandlerContext(); + + private RestHandlerContext() {} + + public RestHandlerContext(final RestResponse earlyResponse, ThreadContext.StoredContext contextToRestore) { + this.earlyResponse = earlyResponse; + this.contextToRestore = contextToRestore; + } + + public boolean hasEarlyResponse() { + return this.earlyResponse != null; + } + + public boolean hasContextToRestore() { + return this.contextToRestore != null; + } + + public RestResponse getEarlyResponse() { + return this.earlyResponse; + } + + public ThreadContext.StoredContext getContextToRestore() { + return contextToRestore; + } +} diff --git a/server/src/main/java/org/opensearch/rest/RestRequest.java b/server/src/main/java/org/opensearch/rest/RestRequest.java index f64774686c89d..275341cd960b8 100644 --- a/server/src/main/java/org/opensearch/rest/RestRequest.java +++ b/server/src/main/java/org/opensearch/rest/RestRequest.java @@ -76,7 +76,6 @@ public class RestRequest implements ToXContent.Params { // tchar pattern as defined by RFC7230 section 3.2.6 private static final Pattern TCHAR_PATTERN = Pattern.compile("[a-zA-z0-9!#$%&'*+\\-.\\^_`|~]+"); - private static final AtomicLong requestIdGenerator = new AtomicLong(); private final NamedXContentRegistry xContentRegistry; @@ -152,7 +151,7 @@ protected RestRequest(RestRequest restRequest) { * with an unpooled copy. This is supposed to be used before passing requests to {@link RestHandler} instances that can not safely * handle http requests that use pooled buffers as determined by {@link RestHandler#allowsUnsafeBuffers()}. */ - void ensureSafeBuffers() { + protected void ensureSafeBuffers() { httpRequest = httpRequest.releaseAndCopy(); } @@ -180,6 +179,36 @@ public static RestRequest request(NamedXContentRegistry xContentRegistry, HttpRe ); } + /** + * Creates a new REST request. This method will throw {@link BadParameterException} if the path cannot be + * decoded + * + * @param xContentRegistry the content registry + * @param httpRequest the http request + * @param httpChannel the http channel + * @param shouldGenerateRequestId should generate a new request id + * @throws BadParameterException if the parameters can not be decoded + * @throws ContentTypeHeaderException if the Content-Type header can not be parsed + */ + public static RestRequest request( + NamedXContentRegistry xContentRegistry, + HttpRequest httpRequest, + HttpChannel httpChannel, + boolean shouldGenerateRequestId + ) { + Map params = params(httpRequest.uri()); + String path = path(httpRequest.uri()); + return new RestRequest( + xContentRegistry, + params, + path, + httpRequest.getHeaders(), + httpRequest, + httpChannel, + shouldGenerateRequestId ? requestIdGenerator.incrementAndGet() : -1 + ); + } + private static Map params(final String uri) { final Map params = new HashMap<>(); int index = uri.indexOf('?'); @@ -228,6 +257,34 @@ public static RestRequest requestWithoutParameters( ); } + /** + * Creates a new REST request. The path is not decoded so this constructor will not throw a + * {@link BadParameterException}. + * + * @param xContentRegistry the content registry + * @param httpRequest the http request + * @param httpChannel the http channel + * @param shouldGenerateRequestId should generate new request id + * @throws ContentTypeHeaderException if the Content-Type header can not be parsed + */ + public static RestRequest requestWithoutParameters( + NamedXContentRegistry xContentRegistry, + HttpRequest httpRequest, + HttpChannel httpChannel, + boolean shouldGenerateRequestId + ) { + Map params = Collections.emptyMap(); + return new RestRequest( + xContentRegistry, + params, + httpRequest.uri(), + httpRequest.getHeaders(), + httpRequest, + httpChannel, + shouldGenerateRequestId ? requestIdGenerator.incrementAndGet() : -1 + ); + } + /** * The method used. * diff --git a/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java b/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java index c34f13041cb11..654eb6c6a374a 100644 --- a/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java +++ b/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java @@ -49,6 +49,7 @@ import org.opensearch.core.rest.RestStatus; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.rest.RestChannel; +import org.opensearch.rest.RestHandlerContext; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestResponse; import org.opensearch.tasks.Task; @@ -200,11 +201,11 @@ public HttpStats stats() { } ) { - transport.dispatchRequest(null, null, null); + transport.dispatchRequest(null, null, null, null); assertNull(threadPool.getThreadContext().getHeader("foo")); assertNull(threadPool.getThreadContext().getTransient("bar")); - transport.dispatchRequest(null, null, new Exception()); + transport.dispatchRequest(null, null, new Exception(), null); assertNull(threadPool.getThreadContext().getHeader("foo_bad")); assertNull(threadPool.getThreadContext().getTransient("bar_bad")); } @@ -321,7 +322,7 @@ public HttpStats stats() { .withInboundException(inboundException) .build(); - transport.incomingRequest(fakeRestRequest.getHttpRequest(), fakeRestRequest.getHttpChannel()); + transport.incomingRequest(fakeRestRequest.getHttpRequest(), fakeRestRequest.getHttpChannel(), RestHandlerContext.EMPTY); final Exception inboundExceptionExcludedPath; if (randomBoolean()) { @@ -338,7 +339,11 @@ public HttpStats stats() { .withInboundException(inboundExceptionExcludedPath) .build(); - transport.incomingRequest(fakeRestRequestExcludedPath.getHttpRequest(), fakeRestRequestExcludedPath.getHttpChannel()); + transport.incomingRequest( + fakeRestRequestExcludedPath.getHttpRequest(), + fakeRestRequestExcludedPath.getHttpChannel(), + RestHandlerContext.EMPTY + ); appender.assertAllExpectationsMatched(); } } diff --git a/server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java b/server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java new file mode 100644 index 0000000000000..2db6cdac0dd4c --- /dev/null +++ b/server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java @@ -0,0 +1,58 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.rest; + +import org.opensearch.client.node.NodeClient; +import org.opensearch.core.common.bytes.BytesArray; +import org.opensearch.core.rest.RestStatus; +import org.opensearch.test.OpenSearchTestCase; + +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.Arrays; +import java.util.List; +import java.util.stream.Collectors; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +public class DelegatingRestHandlerTests extends OpenSearchTestCase { + public void testDelegatingRestHandlerShouldActAsOriginal() throws Exception { + RestHandler rh = new RestHandler() { + @Override + public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + new BytesRestResponse(RestStatus.OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY); + } + }; + RestHandler handlerSpy = spy(rh); + DelegatingRestHandler drh = new DelegatingRestHandler(handlerSpy); + + List overridableMethods = Arrays.stream(RestHandler.class.getDeclaredMethods()) + .filter( + m -> !(Modifier.isPrivate(m.getModifiers()) || Modifier.isStatic(m.getModifiers()) || Modifier.isFinal(m.getModifiers())) + ) + .collect(Collectors.toList()); + + for (Method method : overridableMethods) { + int argCount = method.getParameterCount(); + Object[] args = new Object[argCount]; + for (int i = 0; i < argCount; i++) { + args[i] = any(); + } + if (args.length > 0) { + method.invoke(drh, args); + } else { + method.invoke(drh); + } + method.invoke(verify(handlerSpy, times(1)), args); + } + } +} From a8d9b7356e54554666a3d16b3662f77dca59e6f6 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 27 Sep 2023 18:27:33 -0400 Subject: [PATCH 02/26] Add to CHANGELOG Signed-off-by: Craig Perkins --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 780db204aa1aa..8274876d75f2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -87,6 +87,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [Telemetry-Otel] Added support for OtlpGrpcSpanExporter exporter ([#9666](https://github.com/opensearch-project/OpenSearch/pull/9666)) - Async blob read support for encrypted containers ([#10131](https://github.com/opensearch-project/OpenSearch/pull/10131)) - Add capability to restrict async durability mode for remote indexes ([#10189](https://github.com/opensearch-project/OpenSearch/pull/10189)) +- Improve compressed request handling ([#10261](https://github.com/opensearch-project/OpenSearch/pull/10261)) ### Dependencies - Bump `peter-evans/create-or-update-comment` from 2 to 3 ([#9575](https://github.com/opensearch-project/OpenSearch/pull/9575)) From 51886971683227177ab3ce307cd28c6c98f1303e Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 27 Sep 2023 21:14:27 -0400 Subject: [PATCH 03/26] Use getMethods Signed-off-by: Craig Perkins --- .../java/org/opensearch/rest/DelegatingRestHandlerTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java b/server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java index 2db6cdac0dd4c..ca802a7784ca0 100644 --- a/server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java +++ b/server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java @@ -35,7 +35,7 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c RestHandler handlerSpy = spy(rh); DelegatingRestHandler drh = new DelegatingRestHandler(handlerSpy); - List overridableMethods = Arrays.stream(RestHandler.class.getDeclaredMethods()) + List overridableMethods = Arrays.stream(RestHandler.class.getMethods()) .filter( m -> !(Modifier.isPrivate(m.getModifiers()) || Modifier.isStatic(m.getModifiers()) || Modifier.isFinal(m.getModifiers())) ) From cd9e72f8635eb7aa1937c39a57cf782f39754bc0 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 27 Sep 2023 22:01:05 -0400 Subject: [PATCH 04/26] Create new instance of each inbound handler Signed-off-by: Craig Perkins --- .../netty4/Netty4HttpServerTransport.java | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java index 6a3db18086a49..88ecaafad2597 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java @@ -186,9 +186,6 @@ public class Netty4HttpServerTransport extends AbstractHttpServerTransport { private volatile ServerBootstrap serverBootstrap; private volatile SharedGroupFactory.SharedGroup sharedGroup; - private static final HttpContentDecompressor DEFAULT_DECOMPRESSOR = new HttpContentDecompressor(); - private static final ChannelInboundHandlerAdapter DEFAULT_HEADER_VERIFIER = new ChannelInboundHandlerAdapter(); - public Netty4HttpServerTransport( Settings settings, NetworkService networkService, @@ -429,8 +426,8 @@ protected void channelRead0(ChannelHandlerContext ctx, HttpMessage msg) throws E // If this handler is hit then no upgrade has been attempted and the client is just talking HTTP final ChannelPipeline pipeline = ctx.pipeline(); pipeline.addAfter(ctx.name(), "handler", getRequestHandler()); - pipeline.replace(this, "header_verifier", transport.getHeaderVerifier()); - pipeline.addAfter("header_verifier", "decompress", transport.getDecompressor()); + pipeline.replace(this, "header_verifier", transport.creategetHeaderVerifier()); + pipeline.addAfter("header_verifier", "decompress", transport.createDecompressor()); pipeline.addAfter("decompress", "aggregator", aggregator); if (handlingSettings.isCompression()) { pipeline.addAfter("aggregator", "compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); @@ -452,8 +449,8 @@ protected void configureDefaultHttpPipeline(ChannelPipeline pipeline) { ); decoder.setCumulator(ByteToMessageDecoder.COMPOSITE_CUMULATOR); pipeline.addLast("decoder", decoder); - pipeline.addLast("header_verifier", transport.getHeaderVerifier()); - pipeline.addLast("decompress", transport.getDecompressor()); + pipeline.addLast("header_verifier", transport.creategetHeaderVerifier()); + pipeline.addLast("decompress", transport.createDecompressor()); pipeline.addLast("encoder", new HttpResponseEncoder()); final HttpObjectAggregator aggregator = new HttpObjectAggregator(handlingSettings.getMaxContentLength()); aggregator.setMaxCumulationBufferComponents(transport.maxCompositeBufferComponents); @@ -499,8 +496,8 @@ protected void initChannel(Channel childChannel) throws Exception { .addLast(new Http2StreamFrameToHttpObjectCodec(true)) .addLast("byte_buf_sizer", byteBufSizer) .addLast("read_timeout", new ReadTimeoutHandler(transport.readTimeoutMillis, TimeUnit.MILLISECONDS)) - .addLast("header_verifier", transport.getHeaderVerifier()) - .addLast("decompress", transport.getDecompressor()); + .addLast("header_verifier", transport.creategetHeaderVerifier()) + .addLast("decompress", transport.createDecompressor()); if (handlingSettings.isCompression()) { childChannel.pipeline().addLast("compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); @@ -538,12 +535,12 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { } } - protected HttpContentDecompressor getDecompressor() { - return DEFAULT_DECOMPRESSOR; + protected HttpContentDecompressor createDecompressor() { + return new HttpContentDecompressor(); } - protected ChannelInboundHandlerAdapter getHeaderVerifier() { + protected ChannelInboundHandlerAdapter creategetHeaderVerifier() { // pass-through - return DEFAULT_HEADER_VERIFIER; + return new ChannelInboundHandlerAdapter(); } } From bf2d70709dc69aeeaad748f0545fb547cc7cda8b Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 27 Sep 2023 22:11:18 -0400 Subject: [PATCH 05/26] Update name Signed-off-by: Craig Perkins --- .../org/opensearch/http/netty4/Netty4HttpServerTransport.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java index 88ecaafad2597..bbaf38e21e2fe 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java @@ -539,7 +539,7 @@ protected HttpContentDecompressor createDecompressor() { return new HttpContentDecompressor(); } - protected ChannelInboundHandlerAdapter creategetHeaderVerifier() { + protected ChannelInboundHandlerAdapter createHeaderVerifier() { // pass-through return new ChannelInboundHandlerAdapter(); } From bfc3ceb974c117a189563620054e4947f210ce0f Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 27 Sep 2023 22:15:06 -0400 Subject: [PATCH 06/26] Update name Signed-off-by: Craig Perkins --- .../opensearch/http/netty4/Netty4HttpServerTransport.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java index bbaf38e21e2fe..206f98017e8ec 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java @@ -426,7 +426,7 @@ protected void channelRead0(ChannelHandlerContext ctx, HttpMessage msg) throws E // If this handler is hit then no upgrade has been attempted and the client is just talking HTTP final ChannelPipeline pipeline = ctx.pipeline(); pipeline.addAfter(ctx.name(), "handler", getRequestHandler()); - pipeline.replace(this, "header_verifier", transport.creategetHeaderVerifier()); + pipeline.replace(this, "header_verifier", transport.createHeaderVerifier()); pipeline.addAfter("header_verifier", "decompress", transport.createDecompressor()); pipeline.addAfter("decompress", "aggregator", aggregator); if (handlingSettings.isCompression()) { @@ -449,7 +449,7 @@ protected void configureDefaultHttpPipeline(ChannelPipeline pipeline) { ); decoder.setCumulator(ByteToMessageDecoder.COMPOSITE_CUMULATOR); pipeline.addLast("decoder", decoder); - pipeline.addLast("header_verifier", transport.creategetHeaderVerifier()); + pipeline.addLast("header_verifier", transport.createHeaderVerifier()); pipeline.addLast("decompress", transport.createDecompressor()); pipeline.addLast("encoder", new HttpResponseEncoder()); final HttpObjectAggregator aggregator = new HttpObjectAggregator(handlingSettings.getMaxContentLength()); @@ -496,7 +496,7 @@ protected void initChannel(Channel childChannel) throws Exception { .addLast(new Http2StreamFrameToHttpObjectCodec(true)) .addLast("byte_buf_sizer", byteBufSizer) .addLast("read_timeout", new ReadTimeoutHandler(transport.readTimeoutMillis, TimeUnit.MILLISECONDS)) - .addLast("header_verifier", transport.creategetHeaderVerifier()) + .addLast("header_verifier", transport.createHeaderVerifier()) .addLast("decompress", transport.createDecompressor()); if (handlingSettings.isCompression()) { From 2584e94a2049658393a480a33626ca78c9a947f0 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 27 Sep 2023 22:57:33 -0400 Subject: [PATCH 07/26] Update test Signed-off-by: Craig Perkins --- .../http/nio/HttpReadWriteHandlerTests.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java b/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java index 10c1242ef1a94..1172472a3c6b1 100644 --- a/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java +++ b/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java @@ -102,7 +102,7 @@ public void setMocks() { doAnswer(invocation -> { ((HttpRequest) invocation.getArguments()[0]).releaseAndCopy(); return null; - }).when(transport).incomingRequest(any(HttpRequest.class), any(HttpChannel.class), RestHandlerContext.EMPTY); + }).when(transport).incomingRequest(any(HttpRequest.class), any(HttpChannel.class), any(RestHandlerContext.class)); Settings settings = Settings.builder().put(SETTING_HTTP_MAX_CONTENT_LENGTH.getKey(), new ByteSizeValue(1024)).build(); HttpHandlingSettings httpHandlingSettings = HttpHandlingSettings.fromSettings(settings); channel = mock(NioHttpChannel.class); @@ -123,12 +123,12 @@ public void testSuccessfulDecodeHttpRequest() throws IOException { try { handler.consumeReads(toChannelBuffer(slicedBuf)); - verify(transport, times(0)).incomingRequest(any(HttpRequest.class), any(NioHttpChannel.class), RestHandlerContext.EMPTY); + verify(transport, times(0)).incomingRequest(any(HttpRequest.class), any(NioHttpChannel.class), any(RestHandlerContext.class)); handler.consumeReads(toChannelBuffer(slicedBuf2)); ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(HttpRequest.class); - verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class), RestHandlerContext.EMPTY); + verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class), any(RestHandlerContext.class)); HttpRequest nioHttpRequest = requestCaptor.getValue(); assertEquals(HttpRequest.HttpVersion.HTTP_1_1, nioHttpRequest.protocolVersion()); @@ -154,7 +154,7 @@ public void testDecodeHttpRequestError() throws IOException { handler.consumeReads(toChannelBuffer(buf)); ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(HttpRequest.class); - verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class), RestHandlerContext.EMPTY); + verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class), any(RestHandlerContext.class)); assertNotNull(requestCaptor.getValue().getInboundException()); assertTrue(requestCaptor.getValue().getInboundException() instanceof IllegalArgumentException); @@ -175,7 +175,7 @@ public void testDecodeHttpRequestContentLengthToLongGeneratesOutboundMessage() t } finally { buf.release(); } - verify(transport, times(0)).incomingRequest(any(), any(), RestHandlerContext.EMPTY); + verify(transport, times(0)).incomingRequest(any(), any(), any(RestHandlerContext.class)); List flushOperations = handler.pollFlushOperations(); assertFalse(flushOperations.isEmpty()); @@ -281,7 +281,7 @@ private void prepareHandlerForResponse(HttpReadWriteHandler handler) throws IOEx } ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(HttpPipelinedRequest.class); - verify(transport, atLeastOnce()).incomingRequest(requestCaptor.capture(), any(HttpChannel.class), RestHandlerContext.EMPTY); + verify(transport, atLeastOnce()).incomingRequest(requestCaptor.capture(), any(HttpChannel.class), any(RestHandlerContext.class)); HttpRequest httpRequest = requestCaptor.getValue(); assertNotNull(httpRequest); From e02a8a35866ddedd60b19f6c4037fd4c72d89848 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 28 Sep 2023 13:28:13 -0400 Subject: [PATCH 08/26] Add netty request tests Signed-off-by: Craig Perkins --- .../netty4/AbstractNetty4HttpRequest.java | 2 +- .../netty4/Netty4DefaultHttpRequestTests.java | 74 +++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4DefaultHttpRequestTests.java diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/AbstractNetty4HttpRequest.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/AbstractNetty4HttpRequest.java index 27489af161117..69f6b9f200e75 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/AbstractNetty4HttpRequest.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/AbstractNetty4HttpRequest.java @@ -22,7 +22,7 @@ import io.netty.handler.codec.http.HttpMethod; import io.netty.handler.codec.http.HttpRequest; -public class AbstractNetty4HttpRequest { +public abstract class AbstractNetty4HttpRequest { protected HttpHeadersMap headers; protected Exception inboundException; diff --git a/modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4DefaultHttpRequestTests.java b/modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4DefaultHttpRequestTests.java new file mode 100644 index 0000000000000..e9df6ab376c79 --- /dev/null +++ b/modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4DefaultHttpRequestTests.java @@ -0,0 +1,74 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.http.netty4; + +import org.opensearch.core.common.bytes.BytesArray; +import org.opensearch.core.common.bytes.BytesReference; +import org.opensearch.test.OpenSearchTestCase; + +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.stream.Collectors; + +import io.netty.buffer.ByteBuf; +import io.netty.buffer.Unpooled; +import io.netty.handler.codec.http.DefaultFullHttpRequest; +import io.netty.handler.codec.http.DefaultHttpHeaders; +import io.netty.handler.codec.http.DefaultHttpRequest; +import io.netty.handler.codec.http.FullHttpRequest; +import io.netty.handler.codec.http.HttpHeaders; +import io.netty.handler.codec.http.HttpMethod; +import io.netty.handler.codec.http.HttpVersion; + +public class Netty4DefaultHttpRequestTests extends OpenSearchTestCase { + public void testDefaultHttpRequestHasEmptyContent() throws Exception { + HttpHeaders testHeaders = new DefaultHttpHeaders(); + testHeaders.add("test-header", "test-value"); + DefaultHttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "https://localhost:9200"); + Netty4DefaultHttpRequest netty4Request = new Netty4DefaultHttpRequest(request); + + assertEquals("Expected content to be empty", BytesArray.EMPTY, netty4Request.content()); + } + + public void testDefaultHttpRequestAndFullHttpRequestHaveSameStructure() throws Exception { + HttpHeaders testHeaders = new DefaultHttpHeaders(); + testHeaders.add("test-header", "test-value"); + DefaultHttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "https://localhost:9200", testHeaders); + Netty4DefaultHttpRequest netty4Request = new Netty4DefaultHttpRequest(request); + + ByteBuf expectedByteBuf = Unpooled.copiedBuffer("Hello, World!".getBytes(StandardCharsets.UTF_8)); + BytesReference expectedBytesArray = new BytesArray("Hello, World!".getBytes(StandardCharsets.UTF_8)); + + HttpHeaders trailingHeaders = new DefaultHttpHeaders(); + FullHttpRequest fullRequest = new DefaultFullHttpRequest( + HttpVersion.HTTP_1_1, + HttpMethod.GET, + "https://localhost:9200", + expectedByteBuf, + testHeaders, + trailingHeaders + ); + Netty4HttpRequest fullNetty4Request = new Netty4HttpRequest(fullRequest); + + assertEquals("Expected methods to be equal", netty4Request.method(), fullNetty4Request.method()); + assertEquals("Expected URIs to be equal", netty4Request.uri(), fullNetty4Request.uri()); + assertEquals("Expected Protocol Versions to be equal", netty4Request.protocolVersion(), fullNetty4Request.protocolVersion()); + + assertEquals("Expected headers to be of equal length", netty4Request.headers.size(), fullNetty4Request.headers.size()); + for (String header : netty4Request.headers.keySet()) { + assertTrue(fullNetty4Request.headers.containsKey(header)); + List headerValue = netty4Request.headers.get(header).stream().sorted().collect(Collectors.toList()); + List otherValue = fullNetty4Request.headers.get(header).stream().sorted().collect(Collectors.toList()); + assertEquals(headerValue, otherValue); + } + + assertEquals("Expected content to be empty", BytesArray.EMPTY, netty4Request.content()); + assertEquals("Expected content to not be empty", expectedBytesArray, fullNetty4Request.content()); + } +} From 104c512123da372fc33641bbae21f388d6df226d Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 28 Sep 2023 13:45:32 -0400 Subject: [PATCH 09/26] Add test for createRestRequest Signed-off-by: Craig Perkins --- .../http/AbstractHttpServerTransportTests.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java b/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java index 654eb6c6a374a..eaa199664fceb 100644 --- a/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java +++ b/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java @@ -65,6 +65,7 @@ import java.net.InetSocketAddress; import java.net.UnknownHostException; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -150,6 +151,21 @@ public void testHttpPublishPort() throws Exception { } } + public void testCreateRestRequestDoesNotGenerateRequestID() { + FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent( + new BytesArray("bar".getBytes(StandardCharsets.UTF_8)), + null + ).withPath("/foo").withHeaders(Collections.singletonMap("Content-Type", Collections.singletonList("text/plain"))).build(); + + RestRequest request = AbstractHttpServerTransport.createRestRequest( + xContentRegistry(), + fakeRestRequest.getHttpRequest(), + fakeRestRequest.getHttpChannel() + ); + + assertEquals("request should not generate id", -1, request.getRequestId()); + } + public void testDispatchDoesNotModifyThreadContext() { final HttpServerTransport.Dispatcher dispatcher = new HttpServerTransport.Dispatcher() { From aec43e98f568088562b32c40f4f742f05c5ecff4 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Thu, 28 Sep 2023 23:25:31 +0000 Subject: [PATCH 10/26] Very basic header validator Signed-off-by: Peter Nied --- .../opensearch/http/netty4/Netty4Http2IT.java | 32 +++++ .../http/netty4/Netty4HeaderVerifier.java | 57 +++++++++ .../http/netty4/Netty4HttpRequestHandler.java | 6 +- .../netty4/Netty4HttpServerTransport.java | 36 +++--- .../http/nio/HttpReadWriteHandler.java | 3 +- .../http/nio/HttpReadWriteHandlerTests.java | 13 +-- .../http/AbstractHttpServerTransport.java | 109 +++--------------- .../rest/DelegatingRestHandler.java | 74 ------------ .../org/opensearch/rest/RestController.java | 2 +- .../java/org/opensearch/rest/RestHandler.java | 66 ++++++++++- .../opensearch/rest/RestHandlerContext.java | 44 ------- .../java/org/opensearch/rest/RestRequest.java | 61 +--------- .../AbstractHttpServerTransportTests.java | 29 +---- .../rest/DelegatingRestHandlerTests.java | 58 ---------- 14 files changed, 196 insertions(+), 394 deletions(-) create mode 100644 modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HeaderVerifier.java delete mode 100644 server/src/main/java/org/opensearch/rest/DelegatingRestHandler.java delete mode 100644 server/src/main/java/org/opensearch/rest/RestHandlerContext.java delete mode 100644 server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java diff --git a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4Http2IT.java b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4Http2IT.java index eba2c5ce1e094..4206c3728c620 100644 --- a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4Http2IT.java +++ b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4Http2IT.java @@ -16,18 +16,26 @@ import org.opensearch.test.OpenSearchIntegTestCase.Scope; import java.util.Collection; +import java.util.ArrayList; import java.util.List; import java.util.Locale; import java.util.stream.IntStream; +import io.netty.handler.codec.http.DefaultFullHttpRequest; +import io.netty.handler.codec.http.FullHttpRequest; import io.netty.handler.codec.http.FullHttpResponse; +import io.netty.handler.codec.http.HttpMethod; import io.netty.handler.codec.http.HttpResponseStatus; +import io.netty.handler.codec.http.HttpVersion; import io.netty.util.ReferenceCounted; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.hasSize; +import io.netty.handler.codec.http2.HttpConversionUtil; +import static io.netty.handler.codec.http.HttpHeaderNames.HOST; + @ClusterScope(scope = Scope.TEST, supportsDedicatedMasters = false, numDataNodes = 1) public class Netty4Http2IT extends OpenSearchNetty4IntegTestCase { @@ -56,6 +64,30 @@ public void testThatNettyHttpServerSupportsHttp2GetUpgrades() throws Exception { } } + + public void testThatNettyHttpServerRequestBlockedWithHeaderVerifier() throws Exception { + HttpServerTransport httpServerTransport = internalCluster().getInstance(HttpServerTransport.class); + TransportAddress[] boundAddresses = httpServerTransport.boundAddress().boundAddresses(); + TransportAddress transportAddress = randomFrom(boundAddresses); + + final FullHttpRequest blockedRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/"); + blockedRequest.headers().add("blockme", "Not Allowed"); + blockedRequest.headers().add(HOST, "localhost"); + blockedRequest.headers().add(HttpConversionUtil.ExtensionHeaderNames.SCHEME.text(), "http"); + + + final List responses = new ArrayList<>(); + try (Netty4HttpClient nettyHttpClient = Netty4HttpClient.http2() ) { + try { + FullHttpResponse blockedResponse = nettyHttpClient.send(transportAddress.address(), blockedRequest); + responses.add(blockedResponse); + assertThat(blockedResponse.status().code(), equalTo(401)); + } finally { + responses.forEach(ReferenceCounted::release); + } + } + } + public void testThatNettyHttpServerSupportsHttp2PostUpgrades() throws Exception { final List> requests = List.of(Tuple.tuple("/_search", "{\"query\":{ \"match_all\":{}}}")); diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HeaderVerifier.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HeaderVerifier.java new file mode 100644 index 0000000000000..296e928ccb0d8 --- /dev/null +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HeaderVerifier.java @@ -0,0 +1,57 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.http.netty4; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import io.netty.channel.ChannelHandler; +import io.netty.channel.ChannelHandlerContext; +import io.netty.channel.ChannelInboundHandlerAdapter; +import io.netty.channel.ChannelFutureListener; +import io.netty.handler.codec.http.HttpRequest; +import io.netty.handler.codec.http.FullHttpResponse; +import io.netty.handler.codec.http.DefaultFullHttpResponse; +import io.netty.handler.codec.http.HttpVersion; +import io.netty.handler.codec.http.HttpResponseStatus; +import io.netty.util.ReferenceCountUtil; + +/** POC for how an external header verifier would be implemented */ +@ChannelHandler.Sharable +public class Netty4HeaderVerifier extends ChannelInboundHandlerAdapter { + + final static Logger log = LogManager.getLogger(Netty4HeaderVerifier.class); + + @Override + public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception { + if (!(msg instanceof HttpRequest)) { + ctx.fireChannelRead(msg); + } + + HttpRequest request = (HttpRequest) msg; + if (!isAuthenticated(request)) { + final FullHttpResponse response = new DefaultFullHttpResponse( + HttpVersion.HTTP_1_1, + HttpResponseStatus.UNAUTHORIZED); + ctx.writeAndFlush(response).addListener(ChannelFutureListener.CLOSE); + ReferenceCountUtil.release(msg); + } else { + // Lets the request pass to the next channel handler + ctx.fireChannelRead(msg); + } + } + + private boolean isAuthenticated(HttpRequest request) { + log.info("Checking if request is authenticated:\n" + request); + + final boolean shouldBlock = request.headers().contains("blockme"); + + return !shouldBlock; + } +} diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java index cc331f1b4fd2d..37a6f131b4468 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java @@ -35,7 +35,6 @@ import org.opensearch.ExceptionsHelper; import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.http.HttpPipelinedRequest; -import org.opensearch.rest.RestHandlerContext; import org.opensearch.rest.RestResponse; import io.netty.channel.ChannelHandler; @@ -54,12 +53,9 @@ class Netty4HttpRequestHandler extends SimpleChannelInboundHandler HTTP_CHANNEL_KEY = AttributeKey.newInstance("opensearch-http-channel"); + protected static final AttributeKey HTTP_CHANNEL_KEY = AttributeKey.newInstance("opensearch-http-channel"); protected static final AttributeKey HTTP_SERVER_CHANNEL_KEY = AttributeKey.newInstance( "opensearch-http-server-channel" ); - public static final AttributeKey EARLY_RESPONSE = AttributeKey.newInstance("opensearch-http-early-response"); - public static final AttributeKey CONTEXT_TO_RESTORE = AttributeKey.newInstance( - "opensearch-http-request-thread-context" - ); - protected static class HttpChannelHandler extends ChannelInitializer { private final Netty4HttpServerTransport transport; @@ -427,10 +420,14 @@ protected void channelRead0(ChannelHandlerContext ctx, HttpMessage msg) throws E final ChannelPipeline pipeline = ctx.pipeline(); pipeline.addAfter(ctx.name(), "handler", getRequestHandler()); pipeline.replace(this, "header_verifier", transport.createHeaderVerifier()); - pipeline.addAfter("header_verifier", "decompress", transport.createDecompressor()); - pipeline.addAfter("decompress", "aggregator", aggregator); + pipeline.addAfter("header_verifier", "decoder_compress", new HttpContentDecompressor()); + pipeline.addAfter("decoder_compress", "aggregator", aggregator); if (handlingSettings.isCompression()) { - pipeline.addAfter("aggregator", "compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); + pipeline.addAfter( + "aggregator", + "encoder_compress", + new HttpContentCompressor(handlingSettings.getCompressionLevel()) + ); } pipeline.addBefore("handler", "request_creator", requestCreator); pipeline.addBefore("handler", "response_creator", responseCreator); @@ -450,13 +447,13 @@ protected void configureDefaultHttpPipeline(ChannelPipeline pipeline) { decoder.setCumulator(ByteToMessageDecoder.COMPOSITE_CUMULATOR); pipeline.addLast("decoder", decoder); pipeline.addLast("header_verifier", transport.createHeaderVerifier()); - pipeline.addLast("decompress", transport.createDecompressor()); + pipeline.addLast("decoder_compress", new HttpContentDecompressor()); pipeline.addLast("encoder", new HttpResponseEncoder()); final HttpObjectAggregator aggregator = new HttpObjectAggregator(handlingSettings.getMaxContentLength()); aggregator.setMaxCumulationBufferComponents(transport.maxCompositeBufferComponents); pipeline.addLast("aggregator", aggregator); if (handlingSettings.isCompression()) { - pipeline.addLast("compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); + pipeline.addLast("encoder_compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); } pipeline.addLast("request_creator", requestCreator); pipeline.addLast("response_creator", responseCreator); @@ -491,16 +488,18 @@ protected void initChannel(Channel childChannel) throws Exception { final HttpObjectAggregator aggregator = new HttpObjectAggregator(handlingSettings.getMaxContentLength()); aggregator.setMaxCumulationBufferComponents(transport.maxCompositeBufferComponents); + childChannel.pipeline() .addLast(new LoggingHandler(LogLevel.DEBUG)) .addLast(new Http2StreamFrameToHttpObjectCodec(true)) .addLast("byte_buf_sizer", byteBufSizer) .addLast("read_timeout", new ReadTimeoutHandler(transport.readTimeoutMillis, TimeUnit.MILLISECONDS)) .addLast("header_verifier", transport.createHeaderVerifier()) - .addLast("decompress", transport.createDecompressor()); + .addLast("decoder_decompress", new HttpContentDecompressor()); if (handlingSettings.isCompression()) { - childChannel.pipeline().addLast("compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); + childChannel.pipeline() + .addLast("encoder_compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); } childChannel.pipeline() @@ -535,12 +534,9 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { } } - protected HttpContentDecompressor createDecompressor() { - return new HttpContentDecompressor(); - } - protected ChannelInboundHandlerAdapter createHeaderVerifier() { + return new Netty4HeaderVerifier(); // pass-through - return new ChannelInboundHandlerAdapter(); +// return new ChannelInboundHandlerAdapter(); } } diff --git a/plugins/transport-nio/src/main/java/org/opensearch/http/nio/HttpReadWriteHandler.java b/plugins/transport-nio/src/main/java/org/opensearch/http/nio/HttpReadWriteHandler.java index 7b04f93d5c3be..d44515f3dc727 100644 --- a/plugins/transport-nio/src/main/java/org/opensearch/http/nio/HttpReadWriteHandler.java +++ b/plugins/transport-nio/src/main/java/org/opensearch/http/nio/HttpReadWriteHandler.java @@ -43,7 +43,6 @@ import org.opensearch.nio.SocketChannelContext; import org.opensearch.nio.TaskScheduler; import org.opensearch.nio.WriteOperation; -import org.opensearch.rest.RestHandlerContext; import java.io.IOException; import java.util.ArrayList; @@ -173,7 +172,7 @@ private void handleRequest(Object msg) { final HttpPipelinedRequest pipelinedRequest = (HttpPipelinedRequest) msg; boolean success = false; try { - transport.incomingRequest(pipelinedRequest, nioHttpChannel, RestHandlerContext.EMPTY); + transport.incomingRequest(pipelinedRequest, nioHttpChannel); success = true; } finally { if (success == false) { diff --git a/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java b/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java index 1172472a3c6b1..a3f7a7822cd40 100644 --- a/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java +++ b/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java @@ -48,7 +48,6 @@ import org.opensearch.nio.InboundChannelBuffer; import org.opensearch.nio.SocketChannelContext; import org.opensearch.nio.TaskScheduler; -import org.opensearch.rest.RestHandlerContext; import org.opensearch.rest.RestRequest; import org.opensearch.test.OpenSearchTestCase; import org.junit.Before; @@ -102,7 +101,7 @@ public void setMocks() { doAnswer(invocation -> { ((HttpRequest) invocation.getArguments()[0]).releaseAndCopy(); return null; - }).when(transport).incomingRequest(any(HttpRequest.class), any(HttpChannel.class), any(RestHandlerContext.class)); + }).when(transport).incomingRequest(any(HttpRequest.class), any(HttpChannel.class)); Settings settings = Settings.builder().put(SETTING_HTTP_MAX_CONTENT_LENGTH.getKey(), new ByteSizeValue(1024)).build(); HttpHandlingSettings httpHandlingSettings = HttpHandlingSettings.fromSettings(settings); channel = mock(NioHttpChannel.class); @@ -123,12 +122,12 @@ public void testSuccessfulDecodeHttpRequest() throws IOException { try { handler.consumeReads(toChannelBuffer(slicedBuf)); - verify(transport, times(0)).incomingRequest(any(HttpRequest.class), any(NioHttpChannel.class), any(RestHandlerContext.class)); + verify(transport, times(0)).incomingRequest(any(HttpRequest.class), any(NioHttpChannel.class)); handler.consumeReads(toChannelBuffer(slicedBuf2)); ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(HttpRequest.class); - verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class), any(RestHandlerContext.class)); + verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class)); HttpRequest nioHttpRequest = requestCaptor.getValue(); assertEquals(HttpRequest.HttpVersion.HTTP_1_1, nioHttpRequest.protocolVersion()); @@ -154,7 +153,7 @@ public void testDecodeHttpRequestError() throws IOException { handler.consumeReads(toChannelBuffer(buf)); ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(HttpRequest.class); - verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class), any(RestHandlerContext.class)); + verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class)); assertNotNull(requestCaptor.getValue().getInboundException()); assertTrue(requestCaptor.getValue().getInboundException() instanceof IllegalArgumentException); @@ -175,7 +174,7 @@ public void testDecodeHttpRequestContentLengthToLongGeneratesOutboundMessage() t } finally { buf.release(); } - verify(transport, times(0)).incomingRequest(any(), any(), any(RestHandlerContext.class)); + verify(transport, times(0)).incomingRequest(any(), any()); List flushOperations = handler.pollFlushOperations(); assertFalse(flushOperations.isEmpty()); @@ -281,7 +280,7 @@ private void prepareHandlerForResponse(HttpReadWriteHandler handler) throws IOEx } ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(HttpPipelinedRequest.class); - verify(transport, atLeastOnce()).incomingRequest(requestCaptor.capture(), any(HttpChannel.class), any(RestHandlerContext.class)); + verify(transport, atLeastOnce()).incomingRequest(requestCaptor.capture(), any(HttpChannel.class)); HttpRequest httpRequest = requestCaptor.getValue(); assertNotNull(httpRequest); diff --git a/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java b/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java index 61c2fa01c9c06..ed44102d0abe4 100644 --- a/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java +++ b/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java @@ -53,7 +53,6 @@ import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.rest.RestChannel; -import org.opensearch.rest.RestHandlerContext; import org.opensearch.rest.RestRequest; import org.opensearch.telemetry.tracing.Span; import org.opensearch.telemetry.tracing.SpanBuilder; @@ -100,7 +99,7 @@ public abstract class AbstractHttpServerTransport extends AbstractLifecycleCompo protected final ThreadPool threadPool; protected final Dispatcher dispatcher; protected final CorsHandler corsHandler; - protected final NamedXContentRegistry xContentRegistry; + private final NamedXContentRegistry xContentRegistry; protected final PortsRange port; protected final ByteSizeValue maxContentLength; @@ -113,7 +112,7 @@ public abstract class AbstractHttpServerTransport extends AbstractLifecycleCompo private final Set httpServerChannels = Collections.newSetFromMap(new ConcurrentHashMap<>()); private final HttpTracer httpTracer; - protected final Tracer tracer; + private final Tracer tracer; protected AbstractHttpServerTransport( Settings settings, @@ -360,29 +359,20 @@ protected void serverAcceptedChannel(HttpChannel httpChannel) { * * @param httpRequest that is incoming * @param httpChannel that received the http request - * @param requestContext context carried over to the request handler from earlier stages in the request pipeline */ - public void incomingRequest(final HttpRequest httpRequest, final HttpChannel httpChannel, final RestHandlerContext requestContext) { + public void incomingRequest(final HttpRequest httpRequest, final HttpChannel httpChannel) { final Span span = tracer.startSpan(SpanBuilder.from(httpRequest), httpRequest.getHeaders()); try (final SpanScope httpRequestSpanScope = tracer.withSpanInScope(span)) { HttpChannel traceableHttpChannel = TraceableHttpChannel.create(httpChannel, span, tracer); - handleIncomingRequest(httpRequest, traceableHttpChannel, requestContext, httpRequest.getInboundException()); + handleIncomingRequest(httpRequest, traceableHttpChannel, httpRequest.getInboundException()); } } // Visible for testing - protected void dispatchRequest( - final RestRequest restRequest, - final RestChannel channel, - final Throwable badRequestCause, - final ThreadContext.StoredContext storedContext - ) { + void dispatchRequest(final RestRequest restRequest, final RestChannel channel, final Throwable badRequestCause) { RestChannel traceableRestChannel = channel; final ThreadContext threadContext = threadPool.getThreadContext(); try (ThreadContext.StoredContext ignore = threadContext.stashContext()) { - if (storedContext != null) { - storedContext.restore(); - } final Span span = tracer.startSpan(SpanBuilder.from(restRequest)); try (final SpanScope spanScope = tracer.withSpanInScope(span)) { if (channel != null) { @@ -398,12 +388,7 @@ protected void dispatchRequest( } - private void handleIncomingRequest( - final HttpRequest httpRequest, - final HttpChannel httpChannel, - final RestHandlerContext requestContext, - final Exception exception - ) { + private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChannel httpChannel, final Exception exception) { if (exception == null) { HttpResponse earlyResponse = corsHandler.handleInbound(httpRequest); if (earlyResponse != null) { @@ -426,13 +411,13 @@ private void handleIncomingRequest( { RestRequest innerRestRequest; try { - innerRestRequest = RestRequest.request(xContentRegistry, httpRequest, httpChannel, true); + innerRestRequest = RestRequest.request(xContentRegistry, httpRequest, httpChannel); } catch (final RestRequest.ContentTypeHeaderException e) { badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); - innerRestRequest = requestWithoutContentTypeHeader(httpRequest, httpChannel, badRequestCause, true); + innerRestRequest = requestWithoutContentTypeHeader(httpRequest, httpChannel, badRequestCause); } catch (final RestRequest.BadParameterException e) { badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); - innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel, true); + innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel); } restRequest = innerRestRequest; } @@ -477,84 +462,16 @@ private void handleIncomingRequest( channel = innerChannel; } - if (requestContext.hasEarlyResponse()) { - channel.sendResponse(requestContext.getEarlyResponse()); - return; - } - - dispatchRequest(restRequest, channel, badRequestCause, requestContext.getContextToRestore()); - } - - public static RestRequest createRestRequest( - final NamedXContentRegistry xContentRegistry, - final HttpRequest httpRequest, - final HttpChannel httpChannel - ) { - // TODO Figure out how to only generate one request ID for each request in the pipeline. - Exception badRequestCause = httpRequest.getInboundException(); - - /* - * We want to create a REST request from the incoming request from Netty. However, creating this request could fail if there - * are incorrectly encoded parameters, or the Content-Type header is invalid. If one of these specific failures occurs, we - * attempt to create a REST request again without the input that caused the exception (e.g., we remove the Content-Type header, - * or skip decoding the parameters). Once we have a request in hand, we then dispatch the request as a bad request with the - * underlying exception that caused us to treat the request as bad. - */ - final RestRequest restRequest; - { - RestRequest innerRestRequest; - try { - innerRestRequest = RestRequest.request(xContentRegistry, httpRequest, httpChannel, false); - } catch (final RestRequest.ContentTypeHeaderException e) { - badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); - innerRestRequest = requestWithoutContentTypeHeader(xContentRegistry, httpRequest, httpChannel, badRequestCause, false); - } catch (final RestRequest.BadParameterException e) { - badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); - innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel, false); - } - restRequest = innerRestRequest; - } - return restRequest; - } - - private static RestRequest requestWithoutContentTypeHeader( - NamedXContentRegistry xContentRegistry, - HttpRequest httpRequest, - HttpChannel httpChannel, - Exception badRequestCause, - boolean shouldGenerateRequestId - ) { - HttpRequest httpRequestWithoutContentType = httpRequest.removeHeader("Content-Type"); - try { - return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel, shouldGenerateRequestId); - } catch (final RestRequest.BadParameterException e) { - badRequestCause.addSuppressed(e); - return RestRequest.requestWithoutParameters( - xContentRegistry, - httpRequestWithoutContentType, - httpChannel, - shouldGenerateRequestId - ); - } + dispatchRequest(restRequest, channel, badRequestCause); } - private RestRequest requestWithoutContentTypeHeader( - HttpRequest httpRequest, - HttpChannel httpChannel, - Exception badRequestCause, - boolean shouldGenerateRequestId - ) { + private RestRequest requestWithoutContentTypeHeader(HttpRequest httpRequest, HttpChannel httpChannel, Exception badRequestCause) { HttpRequest httpRequestWithoutContentType = httpRequest.removeHeader("Content-Type"); try { - return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel, shouldGenerateRequestId); + return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel); } catch (final RestRequest.BadParameterException e) { badRequestCause.addSuppressed(e); - return RestRequest.requestWithoutParameters( - xContentRegistry, - httpRequestWithoutContentType, - httpChannel, - shouldGenerateRequestId - ); + return RestRequest.requestWithoutParameters(xContentRegistry, httpRequestWithoutContentType, httpChannel); } } diff --git a/server/src/main/java/org/opensearch/rest/DelegatingRestHandler.java b/server/src/main/java/org/opensearch/rest/DelegatingRestHandler.java deleted file mode 100644 index 928ff94d8c1d3..0000000000000 --- a/server/src/main/java/org/opensearch/rest/DelegatingRestHandler.java +++ /dev/null @@ -1,74 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.rest; - -import org.opensearch.client.node.NodeClient; - -import java.util.List; -import java.util.Objects; - -/** - * Delegating RestHandler that delegates all implementations to original handler - * - * @opensearch.api - */ -public class DelegatingRestHandler implements RestHandler { - - protected final RestHandler delegate; - - public DelegatingRestHandler(RestHandler delegate) { - Objects.requireNonNull(delegate, "RestHandler delegate can not be null"); - this.delegate = delegate; - } - - @Override - public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { - delegate.handleRequest(request, channel, client); - } - - @Override - public boolean canTripCircuitBreaker() { - return delegate.canTripCircuitBreaker(); - } - - @Override - public boolean supportsContentStream() { - return delegate.supportsContentStream(); - } - - @Override - public boolean allowsUnsafeBuffers() { - return delegate.allowsUnsafeBuffers(); - } - - @Override - public List routes() { - return delegate.routes(); - } - - @Override - public List deprecatedRoutes() { - return delegate.deprecatedRoutes(); - } - - @Override - public List replacedRoutes() { - return delegate.replacedRoutes(); - } - - @Override - public boolean allowSystemIndexAccessByDefault() { - return delegate.allowSystemIndexAccessByDefault(); - } - - @Override - public String toString() { - return delegate.toString(); - } -} diff --git a/server/src/main/java/org/opensearch/rest/RestController.java b/server/src/main/java/org/opensearch/rest/RestController.java index 4929f2a147dae..ac30f999d0da7 100644 --- a/server/src/main/java/org/opensearch/rest/RestController.java +++ b/server/src/main/java/org/opensearch/rest/RestController.java @@ -131,7 +131,7 @@ public RestController( this.headersToCopy = headersToCopy; this.usageService = usageService; if (handlerWrapper == null) { - handlerWrapper = (delegate) -> new DelegatingRestHandler(delegate); + handlerWrapper = h -> h; // passthrough if no wrapper set } this.handlerWrapper = handlerWrapper; this.client = client; diff --git a/server/src/main/java/org/opensearch/rest/RestHandler.java b/server/src/main/java/org/opensearch/rest/RestHandler.java index edb1cb341d2d8..7832649e8ad32 100644 --- a/server/src/main/java/org/opensearch/rest/RestHandler.java +++ b/server/src/main/java/org/opensearch/rest/RestHandler.java @@ -44,8 +44,6 @@ /** * Handler for REST requests * - * If new methods are added to this interface they must also be added to {@link DelegatingRestHandler} - * * @opensearch.api */ @FunctionalInterface @@ -110,13 +108,75 @@ default List replacedRoutes() { } /** - * Controls whether requests handled by this class are allowed to access system indices by default. + * Controls whether requests handled by this class are allowed to to access system indices by default. * @return {@code true} if requests handled by this class should be allowed to access system indices. */ default boolean allowSystemIndexAccessByDefault() { return false; } + static RestHandler wrapper(RestHandler delegate) { + return new Wrapper(delegate); + } + + /** + * Wrapper for a handler. + * + * @opensearch.internal + */ + class Wrapper implements RestHandler { + private final RestHandler delegate; + + public Wrapper(RestHandler delegate) { + this.delegate = Objects.requireNonNull(delegate, "RestHandler delegate can not be null"); + } + + @Override + public String toString() { + return delegate.toString(); + } + + @Override + public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + delegate.handleRequest(request, channel, client); + } + + @Override + public boolean canTripCircuitBreaker() { + return delegate.canTripCircuitBreaker(); + } + + @Override + public boolean supportsContentStream() { + return delegate.supportsContentStream(); + } + + @Override + public boolean allowsUnsafeBuffers() { + return delegate.allowsUnsafeBuffers(); + } + + @Override + public List routes() { + return delegate.routes(); + } + + @Override + public List deprecatedRoutes() { + return delegate.deprecatedRoutes(); + } + + @Override + public List replacedRoutes() { + return delegate.replacedRoutes(); + } + + @Override + public boolean allowSystemIndexAccessByDefault() { + return delegate.allowSystemIndexAccessByDefault(); + } + } + /** * Route for the request. * diff --git a/server/src/main/java/org/opensearch/rest/RestHandlerContext.java b/server/src/main/java/org/opensearch/rest/RestHandlerContext.java deleted file mode 100644 index 297a44c705e2e..0000000000000 --- a/server/src/main/java/org/opensearch/rest/RestHandlerContext.java +++ /dev/null @@ -1,44 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.rest; - -import org.opensearch.common.util.concurrent.ThreadContext; - -/** - * Holder for information that is shared between stages of the request pipeline - */ -public class RestHandlerContext { - private RestResponse earlyResponse; - private ThreadContext.StoredContext contextToRestore; - - public static RestHandlerContext EMPTY = new RestHandlerContext(); - - private RestHandlerContext() {} - - public RestHandlerContext(final RestResponse earlyResponse, ThreadContext.StoredContext contextToRestore) { - this.earlyResponse = earlyResponse; - this.contextToRestore = contextToRestore; - } - - public boolean hasEarlyResponse() { - return this.earlyResponse != null; - } - - public boolean hasContextToRestore() { - return this.contextToRestore != null; - } - - public RestResponse getEarlyResponse() { - return this.earlyResponse; - } - - public ThreadContext.StoredContext getContextToRestore() { - return contextToRestore; - } -} diff --git a/server/src/main/java/org/opensearch/rest/RestRequest.java b/server/src/main/java/org/opensearch/rest/RestRequest.java index 275341cd960b8..f64774686c89d 100644 --- a/server/src/main/java/org/opensearch/rest/RestRequest.java +++ b/server/src/main/java/org/opensearch/rest/RestRequest.java @@ -76,6 +76,7 @@ public class RestRequest implements ToXContent.Params { // tchar pattern as defined by RFC7230 section 3.2.6 private static final Pattern TCHAR_PATTERN = Pattern.compile("[a-zA-z0-9!#$%&'*+\\-.\\^_`|~]+"); + private static final AtomicLong requestIdGenerator = new AtomicLong(); private final NamedXContentRegistry xContentRegistry; @@ -151,7 +152,7 @@ protected RestRequest(RestRequest restRequest) { * with an unpooled copy. This is supposed to be used before passing requests to {@link RestHandler} instances that can not safely * handle http requests that use pooled buffers as determined by {@link RestHandler#allowsUnsafeBuffers()}. */ - protected void ensureSafeBuffers() { + void ensureSafeBuffers() { httpRequest = httpRequest.releaseAndCopy(); } @@ -179,36 +180,6 @@ public static RestRequest request(NamedXContentRegistry xContentRegistry, HttpRe ); } - /** - * Creates a new REST request. This method will throw {@link BadParameterException} if the path cannot be - * decoded - * - * @param xContentRegistry the content registry - * @param httpRequest the http request - * @param httpChannel the http channel - * @param shouldGenerateRequestId should generate a new request id - * @throws BadParameterException if the parameters can not be decoded - * @throws ContentTypeHeaderException if the Content-Type header can not be parsed - */ - public static RestRequest request( - NamedXContentRegistry xContentRegistry, - HttpRequest httpRequest, - HttpChannel httpChannel, - boolean shouldGenerateRequestId - ) { - Map params = params(httpRequest.uri()); - String path = path(httpRequest.uri()); - return new RestRequest( - xContentRegistry, - params, - path, - httpRequest.getHeaders(), - httpRequest, - httpChannel, - shouldGenerateRequestId ? requestIdGenerator.incrementAndGet() : -1 - ); - } - private static Map params(final String uri) { final Map params = new HashMap<>(); int index = uri.indexOf('?'); @@ -257,34 +228,6 @@ public static RestRequest requestWithoutParameters( ); } - /** - * Creates a new REST request. The path is not decoded so this constructor will not throw a - * {@link BadParameterException}. - * - * @param xContentRegistry the content registry - * @param httpRequest the http request - * @param httpChannel the http channel - * @param shouldGenerateRequestId should generate new request id - * @throws ContentTypeHeaderException if the Content-Type header can not be parsed - */ - public static RestRequest requestWithoutParameters( - NamedXContentRegistry xContentRegistry, - HttpRequest httpRequest, - HttpChannel httpChannel, - boolean shouldGenerateRequestId - ) { - Map params = Collections.emptyMap(); - return new RestRequest( - xContentRegistry, - params, - httpRequest.uri(), - httpRequest.getHeaders(), - httpRequest, - httpChannel, - shouldGenerateRequestId ? requestIdGenerator.incrementAndGet() : -1 - ); - } - /** * The method used. * diff --git a/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java b/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java index eaa199664fceb..c34f13041cb11 100644 --- a/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java +++ b/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java @@ -49,7 +49,6 @@ import org.opensearch.core.rest.RestStatus; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.rest.RestChannel; -import org.opensearch.rest.RestHandlerContext; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestResponse; import org.opensearch.tasks.Task; @@ -65,7 +64,6 @@ import java.net.InetSocketAddress; import java.net.UnknownHostException; -import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -151,21 +149,6 @@ public void testHttpPublishPort() throws Exception { } } - public void testCreateRestRequestDoesNotGenerateRequestID() { - FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent( - new BytesArray("bar".getBytes(StandardCharsets.UTF_8)), - null - ).withPath("/foo").withHeaders(Collections.singletonMap("Content-Type", Collections.singletonList("text/plain"))).build(); - - RestRequest request = AbstractHttpServerTransport.createRestRequest( - xContentRegistry(), - fakeRestRequest.getHttpRequest(), - fakeRestRequest.getHttpChannel() - ); - - assertEquals("request should not generate id", -1, request.getRequestId()); - } - public void testDispatchDoesNotModifyThreadContext() { final HttpServerTransport.Dispatcher dispatcher = new HttpServerTransport.Dispatcher() { @@ -217,11 +200,11 @@ public HttpStats stats() { } ) { - transport.dispatchRequest(null, null, null, null); + transport.dispatchRequest(null, null, null); assertNull(threadPool.getThreadContext().getHeader("foo")); assertNull(threadPool.getThreadContext().getTransient("bar")); - transport.dispatchRequest(null, null, new Exception(), null); + transport.dispatchRequest(null, null, new Exception()); assertNull(threadPool.getThreadContext().getHeader("foo_bad")); assertNull(threadPool.getThreadContext().getTransient("bar_bad")); } @@ -338,7 +321,7 @@ public HttpStats stats() { .withInboundException(inboundException) .build(); - transport.incomingRequest(fakeRestRequest.getHttpRequest(), fakeRestRequest.getHttpChannel(), RestHandlerContext.EMPTY); + transport.incomingRequest(fakeRestRequest.getHttpRequest(), fakeRestRequest.getHttpChannel()); final Exception inboundExceptionExcludedPath; if (randomBoolean()) { @@ -355,11 +338,7 @@ public HttpStats stats() { .withInboundException(inboundExceptionExcludedPath) .build(); - transport.incomingRequest( - fakeRestRequestExcludedPath.getHttpRequest(), - fakeRestRequestExcludedPath.getHttpChannel(), - RestHandlerContext.EMPTY - ); + transport.incomingRequest(fakeRestRequestExcludedPath.getHttpRequest(), fakeRestRequestExcludedPath.getHttpChannel()); appender.assertAllExpectationsMatched(); } } diff --git a/server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java b/server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java deleted file mode 100644 index ca802a7784ca0..0000000000000 --- a/server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java +++ /dev/null @@ -1,58 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.rest; - -import org.opensearch.client.node.NodeClient; -import org.opensearch.core.common.bytes.BytesArray; -import org.opensearch.core.rest.RestStatus; -import org.opensearch.test.OpenSearchTestCase; - -import java.lang.reflect.Method; -import java.lang.reflect.Modifier; -import java.util.Arrays; -import java.util.List; -import java.util.stream.Collectors; - -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; - -public class DelegatingRestHandlerTests extends OpenSearchTestCase { - public void testDelegatingRestHandlerShouldActAsOriginal() throws Exception { - RestHandler rh = new RestHandler() { - @Override - public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { - new BytesRestResponse(RestStatus.OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY); - } - }; - RestHandler handlerSpy = spy(rh); - DelegatingRestHandler drh = new DelegatingRestHandler(handlerSpy); - - List overridableMethods = Arrays.stream(RestHandler.class.getMethods()) - .filter( - m -> !(Modifier.isPrivate(m.getModifiers()) || Modifier.isStatic(m.getModifiers()) || Modifier.isFinal(m.getModifiers())) - ) - .collect(Collectors.toList()); - - for (Method method : overridableMethods) { - int argCount = method.getParameterCount(); - Object[] args = new Object[argCount]; - for (int i = 0; i < argCount; i++) { - args[i] = any(); - } - if (args.length > 0) { - method.invoke(drh, args); - } else { - method.invoke(drh); - } - method.invoke(verify(handlerSpy, times(1)), args); - } - } -} From 46f3e4a8ff9262d0b3f61e7c0e1170402ddfd7b4 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 29 Sep 2023 04:20:09 -0400 Subject: [PATCH 11/26] Revert "Very basic header validator" This reverts commit aec43e98f568088562b32c40f4f742f05c5ecff4. Signed-off-by: Craig Perkins --- .../opensearch/http/netty4/Netty4Http2IT.java | 32 ----- .../http/netty4/Netty4HeaderVerifier.java | 57 --------- .../http/netty4/Netty4HttpRequestHandler.java | 6 +- .../netty4/Netty4HttpServerTransport.java | 36 +++--- .../http/nio/HttpReadWriteHandler.java | 3 +- .../http/nio/HttpReadWriteHandlerTests.java | 13 ++- .../http/AbstractHttpServerTransport.java | 109 +++++++++++++++--- .../rest/DelegatingRestHandler.java | 74 ++++++++++++ .../org/opensearch/rest/RestController.java | 2 +- .../java/org/opensearch/rest/RestHandler.java | 66 +---------- .../opensearch/rest/RestHandlerContext.java | 44 +++++++ .../java/org/opensearch/rest/RestRequest.java | 61 +++++++++- .../AbstractHttpServerTransportTests.java | 29 ++++- .../rest/DelegatingRestHandlerTests.java | 58 ++++++++++ 14 files changed, 394 insertions(+), 196 deletions(-) delete mode 100644 modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HeaderVerifier.java create mode 100644 server/src/main/java/org/opensearch/rest/DelegatingRestHandler.java create mode 100644 server/src/main/java/org/opensearch/rest/RestHandlerContext.java create mode 100644 server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java diff --git a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4Http2IT.java b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4Http2IT.java index 4206c3728c620..eba2c5ce1e094 100644 --- a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4Http2IT.java +++ b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4Http2IT.java @@ -16,26 +16,18 @@ import org.opensearch.test.OpenSearchIntegTestCase.Scope; import java.util.Collection; -import java.util.ArrayList; import java.util.List; import java.util.Locale; import java.util.stream.IntStream; -import io.netty.handler.codec.http.DefaultFullHttpRequest; -import io.netty.handler.codec.http.FullHttpRequest; import io.netty.handler.codec.http.FullHttpResponse; -import io.netty.handler.codec.http.HttpMethod; import io.netty.handler.codec.http.HttpResponseStatus; -import io.netty.handler.codec.http.HttpVersion; import io.netty.util.ReferenceCounted; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.hasSize; -import io.netty.handler.codec.http2.HttpConversionUtil; -import static io.netty.handler.codec.http.HttpHeaderNames.HOST; - @ClusterScope(scope = Scope.TEST, supportsDedicatedMasters = false, numDataNodes = 1) public class Netty4Http2IT extends OpenSearchNetty4IntegTestCase { @@ -64,30 +56,6 @@ public void testThatNettyHttpServerSupportsHttp2GetUpgrades() throws Exception { } } - - public void testThatNettyHttpServerRequestBlockedWithHeaderVerifier() throws Exception { - HttpServerTransport httpServerTransport = internalCluster().getInstance(HttpServerTransport.class); - TransportAddress[] boundAddresses = httpServerTransport.boundAddress().boundAddresses(); - TransportAddress transportAddress = randomFrom(boundAddresses); - - final FullHttpRequest blockedRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/"); - blockedRequest.headers().add("blockme", "Not Allowed"); - blockedRequest.headers().add(HOST, "localhost"); - blockedRequest.headers().add(HttpConversionUtil.ExtensionHeaderNames.SCHEME.text(), "http"); - - - final List responses = new ArrayList<>(); - try (Netty4HttpClient nettyHttpClient = Netty4HttpClient.http2() ) { - try { - FullHttpResponse blockedResponse = nettyHttpClient.send(transportAddress.address(), blockedRequest); - responses.add(blockedResponse); - assertThat(blockedResponse.status().code(), equalTo(401)); - } finally { - responses.forEach(ReferenceCounted::release); - } - } - } - public void testThatNettyHttpServerSupportsHttp2PostUpgrades() throws Exception { final List> requests = List.of(Tuple.tuple("/_search", "{\"query\":{ \"match_all\":{}}}")); diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HeaderVerifier.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HeaderVerifier.java deleted file mode 100644 index 296e928ccb0d8..0000000000000 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HeaderVerifier.java +++ /dev/null @@ -1,57 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.http.netty4; - -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; - -import io.netty.channel.ChannelHandler; -import io.netty.channel.ChannelHandlerContext; -import io.netty.channel.ChannelInboundHandlerAdapter; -import io.netty.channel.ChannelFutureListener; -import io.netty.handler.codec.http.HttpRequest; -import io.netty.handler.codec.http.FullHttpResponse; -import io.netty.handler.codec.http.DefaultFullHttpResponse; -import io.netty.handler.codec.http.HttpVersion; -import io.netty.handler.codec.http.HttpResponseStatus; -import io.netty.util.ReferenceCountUtil; - -/** POC for how an external header verifier would be implemented */ -@ChannelHandler.Sharable -public class Netty4HeaderVerifier extends ChannelInboundHandlerAdapter { - - final static Logger log = LogManager.getLogger(Netty4HeaderVerifier.class); - - @Override - public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception { - if (!(msg instanceof HttpRequest)) { - ctx.fireChannelRead(msg); - } - - HttpRequest request = (HttpRequest) msg; - if (!isAuthenticated(request)) { - final FullHttpResponse response = new DefaultFullHttpResponse( - HttpVersion.HTTP_1_1, - HttpResponseStatus.UNAUTHORIZED); - ctx.writeAndFlush(response).addListener(ChannelFutureListener.CLOSE); - ReferenceCountUtil.release(msg); - } else { - // Lets the request pass to the next channel handler - ctx.fireChannelRead(msg); - } - } - - private boolean isAuthenticated(HttpRequest request) { - log.info("Checking if request is authenticated:\n" + request); - - final boolean shouldBlock = request.headers().contains("blockme"); - - return !shouldBlock; - } -} diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java index 37a6f131b4468..cc331f1b4fd2d 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java @@ -35,6 +35,7 @@ import org.opensearch.ExceptionsHelper; import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.http.HttpPipelinedRequest; +import org.opensearch.rest.RestHandlerContext; import org.opensearch.rest.RestResponse; import io.netty.channel.ChannelHandler; @@ -53,9 +54,12 @@ class Netty4HttpRequestHandler extends SimpleChannelInboundHandler HTTP_CHANNEL_KEY = AttributeKey.newInstance("opensearch-http-channel"); + public static final AttributeKey HTTP_CHANNEL_KEY = AttributeKey.newInstance("opensearch-http-channel"); protected static final AttributeKey HTTP_SERVER_CHANNEL_KEY = AttributeKey.newInstance( "opensearch-http-server-channel" ); + public static final AttributeKey EARLY_RESPONSE = AttributeKey.newInstance("opensearch-http-early-response"); + public static final AttributeKey CONTEXT_TO_RESTORE = AttributeKey.newInstance( + "opensearch-http-request-thread-context" + ); + protected static class HttpChannelHandler extends ChannelInitializer { private final Netty4HttpServerTransport transport; @@ -420,14 +427,10 @@ protected void channelRead0(ChannelHandlerContext ctx, HttpMessage msg) throws E final ChannelPipeline pipeline = ctx.pipeline(); pipeline.addAfter(ctx.name(), "handler", getRequestHandler()); pipeline.replace(this, "header_verifier", transport.createHeaderVerifier()); - pipeline.addAfter("header_verifier", "decoder_compress", new HttpContentDecompressor()); - pipeline.addAfter("decoder_compress", "aggregator", aggregator); + pipeline.addAfter("header_verifier", "decompress", transport.createDecompressor()); + pipeline.addAfter("decompress", "aggregator", aggregator); if (handlingSettings.isCompression()) { - pipeline.addAfter( - "aggregator", - "encoder_compress", - new HttpContentCompressor(handlingSettings.getCompressionLevel()) - ); + pipeline.addAfter("aggregator", "compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); } pipeline.addBefore("handler", "request_creator", requestCreator); pipeline.addBefore("handler", "response_creator", responseCreator); @@ -447,13 +450,13 @@ protected void configureDefaultHttpPipeline(ChannelPipeline pipeline) { decoder.setCumulator(ByteToMessageDecoder.COMPOSITE_CUMULATOR); pipeline.addLast("decoder", decoder); pipeline.addLast("header_verifier", transport.createHeaderVerifier()); - pipeline.addLast("decoder_compress", new HttpContentDecompressor()); + pipeline.addLast("decompress", transport.createDecompressor()); pipeline.addLast("encoder", new HttpResponseEncoder()); final HttpObjectAggregator aggregator = new HttpObjectAggregator(handlingSettings.getMaxContentLength()); aggregator.setMaxCumulationBufferComponents(transport.maxCompositeBufferComponents); pipeline.addLast("aggregator", aggregator); if (handlingSettings.isCompression()) { - pipeline.addLast("encoder_compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); + pipeline.addLast("compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); } pipeline.addLast("request_creator", requestCreator); pipeline.addLast("response_creator", responseCreator); @@ -488,18 +491,16 @@ protected void initChannel(Channel childChannel) throws Exception { final HttpObjectAggregator aggregator = new HttpObjectAggregator(handlingSettings.getMaxContentLength()); aggregator.setMaxCumulationBufferComponents(transport.maxCompositeBufferComponents); - childChannel.pipeline() .addLast(new LoggingHandler(LogLevel.DEBUG)) .addLast(new Http2StreamFrameToHttpObjectCodec(true)) .addLast("byte_buf_sizer", byteBufSizer) .addLast("read_timeout", new ReadTimeoutHandler(transport.readTimeoutMillis, TimeUnit.MILLISECONDS)) .addLast("header_verifier", transport.createHeaderVerifier()) - .addLast("decoder_decompress", new HttpContentDecompressor()); + .addLast("decompress", transport.createDecompressor()); if (handlingSettings.isCompression()) { - childChannel.pipeline() - .addLast("encoder_compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); + childChannel.pipeline().addLast("compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); } childChannel.pipeline() @@ -534,9 +535,12 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { } } + protected HttpContentDecompressor createDecompressor() { + return new HttpContentDecompressor(); + } + protected ChannelInboundHandlerAdapter createHeaderVerifier() { - return new Netty4HeaderVerifier(); // pass-through -// return new ChannelInboundHandlerAdapter(); + return new ChannelInboundHandlerAdapter(); } } diff --git a/plugins/transport-nio/src/main/java/org/opensearch/http/nio/HttpReadWriteHandler.java b/plugins/transport-nio/src/main/java/org/opensearch/http/nio/HttpReadWriteHandler.java index d44515f3dc727..7b04f93d5c3be 100644 --- a/plugins/transport-nio/src/main/java/org/opensearch/http/nio/HttpReadWriteHandler.java +++ b/plugins/transport-nio/src/main/java/org/opensearch/http/nio/HttpReadWriteHandler.java @@ -43,6 +43,7 @@ import org.opensearch.nio.SocketChannelContext; import org.opensearch.nio.TaskScheduler; import org.opensearch.nio.WriteOperation; +import org.opensearch.rest.RestHandlerContext; import java.io.IOException; import java.util.ArrayList; @@ -172,7 +173,7 @@ private void handleRequest(Object msg) { final HttpPipelinedRequest pipelinedRequest = (HttpPipelinedRequest) msg; boolean success = false; try { - transport.incomingRequest(pipelinedRequest, nioHttpChannel); + transport.incomingRequest(pipelinedRequest, nioHttpChannel, RestHandlerContext.EMPTY); success = true; } finally { if (success == false) { diff --git a/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java b/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java index a3f7a7822cd40..1172472a3c6b1 100644 --- a/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java +++ b/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java @@ -48,6 +48,7 @@ import org.opensearch.nio.InboundChannelBuffer; import org.opensearch.nio.SocketChannelContext; import org.opensearch.nio.TaskScheduler; +import org.opensearch.rest.RestHandlerContext; import org.opensearch.rest.RestRequest; import org.opensearch.test.OpenSearchTestCase; import org.junit.Before; @@ -101,7 +102,7 @@ public void setMocks() { doAnswer(invocation -> { ((HttpRequest) invocation.getArguments()[0]).releaseAndCopy(); return null; - }).when(transport).incomingRequest(any(HttpRequest.class), any(HttpChannel.class)); + }).when(transport).incomingRequest(any(HttpRequest.class), any(HttpChannel.class), any(RestHandlerContext.class)); Settings settings = Settings.builder().put(SETTING_HTTP_MAX_CONTENT_LENGTH.getKey(), new ByteSizeValue(1024)).build(); HttpHandlingSettings httpHandlingSettings = HttpHandlingSettings.fromSettings(settings); channel = mock(NioHttpChannel.class); @@ -122,12 +123,12 @@ public void testSuccessfulDecodeHttpRequest() throws IOException { try { handler.consumeReads(toChannelBuffer(slicedBuf)); - verify(transport, times(0)).incomingRequest(any(HttpRequest.class), any(NioHttpChannel.class)); + verify(transport, times(0)).incomingRequest(any(HttpRequest.class), any(NioHttpChannel.class), any(RestHandlerContext.class)); handler.consumeReads(toChannelBuffer(slicedBuf2)); ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(HttpRequest.class); - verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class)); + verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class), any(RestHandlerContext.class)); HttpRequest nioHttpRequest = requestCaptor.getValue(); assertEquals(HttpRequest.HttpVersion.HTTP_1_1, nioHttpRequest.protocolVersion()); @@ -153,7 +154,7 @@ public void testDecodeHttpRequestError() throws IOException { handler.consumeReads(toChannelBuffer(buf)); ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(HttpRequest.class); - verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class)); + verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class), any(RestHandlerContext.class)); assertNotNull(requestCaptor.getValue().getInboundException()); assertTrue(requestCaptor.getValue().getInboundException() instanceof IllegalArgumentException); @@ -174,7 +175,7 @@ public void testDecodeHttpRequestContentLengthToLongGeneratesOutboundMessage() t } finally { buf.release(); } - verify(transport, times(0)).incomingRequest(any(), any()); + verify(transport, times(0)).incomingRequest(any(), any(), any(RestHandlerContext.class)); List flushOperations = handler.pollFlushOperations(); assertFalse(flushOperations.isEmpty()); @@ -280,7 +281,7 @@ private void prepareHandlerForResponse(HttpReadWriteHandler handler) throws IOEx } ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(HttpPipelinedRequest.class); - verify(transport, atLeastOnce()).incomingRequest(requestCaptor.capture(), any(HttpChannel.class)); + verify(transport, atLeastOnce()).incomingRequest(requestCaptor.capture(), any(HttpChannel.class), any(RestHandlerContext.class)); HttpRequest httpRequest = requestCaptor.getValue(); assertNotNull(httpRequest); diff --git a/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java b/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java index ed44102d0abe4..61c2fa01c9c06 100644 --- a/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java +++ b/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java @@ -53,6 +53,7 @@ import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.rest.RestChannel; +import org.opensearch.rest.RestHandlerContext; import org.opensearch.rest.RestRequest; import org.opensearch.telemetry.tracing.Span; import org.opensearch.telemetry.tracing.SpanBuilder; @@ -99,7 +100,7 @@ public abstract class AbstractHttpServerTransport extends AbstractLifecycleCompo protected final ThreadPool threadPool; protected final Dispatcher dispatcher; protected final CorsHandler corsHandler; - private final NamedXContentRegistry xContentRegistry; + protected final NamedXContentRegistry xContentRegistry; protected final PortsRange port; protected final ByteSizeValue maxContentLength; @@ -112,7 +113,7 @@ public abstract class AbstractHttpServerTransport extends AbstractLifecycleCompo private final Set httpServerChannels = Collections.newSetFromMap(new ConcurrentHashMap<>()); private final HttpTracer httpTracer; - private final Tracer tracer; + protected final Tracer tracer; protected AbstractHttpServerTransport( Settings settings, @@ -359,20 +360,29 @@ protected void serverAcceptedChannel(HttpChannel httpChannel) { * * @param httpRequest that is incoming * @param httpChannel that received the http request + * @param requestContext context carried over to the request handler from earlier stages in the request pipeline */ - public void incomingRequest(final HttpRequest httpRequest, final HttpChannel httpChannel) { + public void incomingRequest(final HttpRequest httpRequest, final HttpChannel httpChannel, final RestHandlerContext requestContext) { final Span span = tracer.startSpan(SpanBuilder.from(httpRequest), httpRequest.getHeaders()); try (final SpanScope httpRequestSpanScope = tracer.withSpanInScope(span)) { HttpChannel traceableHttpChannel = TraceableHttpChannel.create(httpChannel, span, tracer); - handleIncomingRequest(httpRequest, traceableHttpChannel, httpRequest.getInboundException()); + handleIncomingRequest(httpRequest, traceableHttpChannel, requestContext, httpRequest.getInboundException()); } } // Visible for testing - void dispatchRequest(final RestRequest restRequest, final RestChannel channel, final Throwable badRequestCause) { + protected void dispatchRequest( + final RestRequest restRequest, + final RestChannel channel, + final Throwable badRequestCause, + final ThreadContext.StoredContext storedContext + ) { RestChannel traceableRestChannel = channel; final ThreadContext threadContext = threadPool.getThreadContext(); try (ThreadContext.StoredContext ignore = threadContext.stashContext()) { + if (storedContext != null) { + storedContext.restore(); + } final Span span = tracer.startSpan(SpanBuilder.from(restRequest)); try (final SpanScope spanScope = tracer.withSpanInScope(span)) { if (channel != null) { @@ -388,7 +398,12 @@ void dispatchRequest(final RestRequest restRequest, final RestChannel channel, f } - private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChannel httpChannel, final Exception exception) { + private void handleIncomingRequest( + final HttpRequest httpRequest, + final HttpChannel httpChannel, + final RestHandlerContext requestContext, + final Exception exception + ) { if (exception == null) { HttpResponse earlyResponse = corsHandler.handleInbound(httpRequest); if (earlyResponse != null) { @@ -411,13 +426,13 @@ private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChan { RestRequest innerRestRequest; try { - innerRestRequest = RestRequest.request(xContentRegistry, httpRequest, httpChannel); + innerRestRequest = RestRequest.request(xContentRegistry, httpRequest, httpChannel, true); } catch (final RestRequest.ContentTypeHeaderException e) { badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); - innerRestRequest = requestWithoutContentTypeHeader(httpRequest, httpChannel, badRequestCause); + innerRestRequest = requestWithoutContentTypeHeader(httpRequest, httpChannel, badRequestCause, true); } catch (final RestRequest.BadParameterException e) { badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); - innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel); + innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel, true); } restRequest = innerRestRequest; } @@ -462,16 +477,84 @@ private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChan channel = innerChannel; } - dispatchRequest(restRequest, channel, badRequestCause); + if (requestContext.hasEarlyResponse()) { + channel.sendResponse(requestContext.getEarlyResponse()); + return; + } + + dispatchRequest(restRequest, channel, badRequestCause, requestContext.getContextToRestore()); + } + + public static RestRequest createRestRequest( + final NamedXContentRegistry xContentRegistry, + final HttpRequest httpRequest, + final HttpChannel httpChannel + ) { + // TODO Figure out how to only generate one request ID for each request in the pipeline. + Exception badRequestCause = httpRequest.getInboundException(); + + /* + * We want to create a REST request from the incoming request from Netty. However, creating this request could fail if there + * are incorrectly encoded parameters, or the Content-Type header is invalid. If one of these specific failures occurs, we + * attempt to create a REST request again without the input that caused the exception (e.g., we remove the Content-Type header, + * or skip decoding the parameters). Once we have a request in hand, we then dispatch the request as a bad request with the + * underlying exception that caused us to treat the request as bad. + */ + final RestRequest restRequest; + { + RestRequest innerRestRequest; + try { + innerRestRequest = RestRequest.request(xContentRegistry, httpRequest, httpChannel, false); + } catch (final RestRequest.ContentTypeHeaderException e) { + badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); + innerRestRequest = requestWithoutContentTypeHeader(xContentRegistry, httpRequest, httpChannel, badRequestCause, false); + } catch (final RestRequest.BadParameterException e) { + badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); + innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel, false); + } + restRequest = innerRestRequest; + } + return restRequest; + } + + private static RestRequest requestWithoutContentTypeHeader( + NamedXContentRegistry xContentRegistry, + HttpRequest httpRequest, + HttpChannel httpChannel, + Exception badRequestCause, + boolean shouldGenerateRequestId + ) { + HttpRequest httpRequestWithoutContentType = httpRequest.removeHeader("Content-Type"); + try { + return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel, shouldGenerateRequestId); + } catch (final RestRequest.BadParameterException e) { + badRequestCause.addSuppressed(e); + return RestRequest.requestWithoutParameters( + xContentRegistry, + httpRequestWithoutContentType, + httpChannel, + shouldGenerateRequestId + ); + } } - private RestRequest requestWithoutContentTypeHeader(HttpRequest httpRequest, HttpChannel httpChannel, Exception badRequestCause) { + private RestRequest requestWithoutContentTypeHeader( + HttpRequest httpRequest, + HttpChannel httpChannel, + Exception badRequestCause, + boolean shouldGenerateRequestId + ) { HttpRequest httpRequestWithoutContentType = httpRequest.removeHeader("Content-Type"); try { - return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel); + return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel, shouldGenerateRequestId); } catch (final RestRequest.BadParameterException e) { badRequestCause.addSuppressed(e); - return RestRequest.requestWithoutParameters(xContentRegistry, httpRequestWithoutContentType, httpChannel); + return RestRequest.requestWithoutParameters( + xContentRegistry, + httpRequestWithoutContentType, + httpChannel, + shouldGenerateRequestId + ); } } diff --git a/server/src/main/java/org/opensearch/rest/DelegatingRestHandler.java b/server/src/main/java/org/opensearch/rest/DelegatingRestHandler.java new file mode 100644 index 0000000000000..928ff94d8c1d3 --- /dev/null +++ b/server/src/main/java/org/opensearch/rest/DelegatingRestHandler.java @@ -0,0 +1,74 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.rest; + +import org.opensearch.client.node.NodeClient; + +import java.util.List; +import java.util.Objects; + +/** + * Delegating RestHandler that delegates all implementations to original handler + * + * @opensearch.api + */ +public class DelegatingRestHandler implements RestHandler { + + protected final RestHandler delegate; + + public DelegatingRestHandler(RestHandler delegate) { + Objects.requireNonNull(delegate, "RestHandler delegate can not be null"); + this.delegate = delegate; + } + + @Override + public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + delegate.handleRequest(request, channel, client); + } + + @Override + public boolean canTripCircuitBreaker() { + return delegate.canTripCircuitBreaker(); + } + + @Override + public boolean supportsContentStream() { + return delegate.supportsContentStream(); + } + + @Override + public boolean allowsUnsafeBuffers() { + return delegate.allowsUnsafeBuffers(); + } + + @Override + public List routes() { + return delegate.routes(); + } + + @Override + public List deprecatedRoutes() { + return delegate.deprecatedRoutes(); + } + + @Override + public List replacedRoutes() { + return delegate.replacedRoutes(); + } + + @Override + public boolean allowSystemIndexAccessByDefault() { + return delegate.allowSystemIndexAccessByDefault(); + } + + @Override + public String toString() { + return delegate.toString(); + } +} diff --git a/server/src/main/java/org/opensearch/rest/RestController.java b/server/src/main/java/org/opensearch/rest/RestController.java index ac30f999d0da7..4929f2a147dae 100644 --- a/server/src/main/java/org/opensearch/rest/RestController.java +++ b/server/src/main/java/org/opensearch/rest/RestController.java @@ -131,7 +131,7 @@ public RestController( this.headersToCopy = headersToCopy; this.usageService = usageService; if (handlerWrapper == null) { - handlerWrapper = h -> h; // passthrough if no wrapper set + handlerWrapper = (delegate) -> new DelegatingRestHandler(delegate); } this.handlerWrapper = handlerWrapper; this.client = client; diff --git a/server/src/main/java/org/opensearch/rest/RestHandler.java b/server/src/main/java/org/opensearch/rest/RestHandler.java index 7832649e8ad32..edb1cb341d2d8 100644 --- a/server/src/main/java/org/opensearch/rest/RestHandler.java +++ b/server/src/main/java/org/opensearch/rest/RestHandler.java @@ -44,6 +44,8 @@ /** * Handler for REST requests * + * If new methods are added to this interface they must also be added to {@link DelegatingRestHandler} + * * @opensearch.api */ @FunctionalInterface @@ -108,75 +110,13 @@ default List replacedRoutes() { } /** - * Controls whether requests handled by this class are allowed to to access system indices by default. + * Controls whether requests handled by this class are allowed to access system indices by default. * @return {@code true} if requests handled by this class should be allowed to access system indices. */ default boolean allowSystemIndexAccessByDefault() { return false; } - static RestHandler wrapper(RestHandler delegate) { - return new Wrapper(delegate); - } - - /** - * Wrapper for a handler. - * - * @opensearch.internal - */ - class Wrapper implements RestHandler { - private final RestHandler delegate; - - public Wrapper(RestHandler delegate) { - this.delegate = Objects.requireNonNull(delegate, "RestHandler delegate can not be null"); - } - - @Override - public String toString() { - return delegate.toString(); - } - - @Override - public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { - delegate.handleRequest(request, channel, client); - } - - @Override - public boolean canTripCircuitBreaker() { - return delegate.canTripCircuitBreaker(); - } - - @Override - public boolean supportsContentStream() { - return delegate.supportsContentStream(); - } - - @Override - public boolean allowsUnsafeBuffers() { - return delegate.allowsUnsafeBuffers(); - } - - @Override - public List routes() { - return delegate.routes(); - } - - @Override - public List deprecatedRoutes() { - return delegate.deprecatedRoutes(); - } - - @Override - public List replacedRoutes() { - return delegate.replacedRoutes(); - } - - @Override - public boolean allowSystemIndexAccessByDefault() { - return delegate.allowSystemIndexAccessByDefault(); - } - } - /** * Route for the request. * diff --git a/server/src/main/java/org/opensearch/rest/RestHandlerContext.java b/server/src/main/java/org/opensearch/rest/RestHandlerContext.java new file mode 100644 index 0000000000000..297a44c705e2e --- /dev/null +++ b/server/src/main/java/org/opensearch/rest/RestHandlerContext.java @@ -0,0 +1,44 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.rest; + +import org.opensearch.common.util.concurrent.ThreadContext; + +/** + * Holder for information that is shared between stages of the request pipeline + */ +public class RestHandlerContext { + private RestResponse earlyResponse; + private ThreadContext.StoredContext contextToRestore; + + public static RestHandlerContext EMPTY = new RestHandlerContext(); + + private RestHandlerContext() {} + + public RestHandlerContext(final RestResponse earlyResponse, ThreadContext.StoredContext contextToRestore) { + this.earlyResponse = earlyResponse; + this.contextToRestore = contextToRestore; + } + + public boolean hasEarlyResponse() { + return this.earlyResponse != null; + } + + public boolean hasContextToRestore() { + return this.contextToRestore != null; + } + + public RestResponse getEarlyResponse() { + return this.earlyResponse; + } + + public ThreadContext.StoredContext getContextToRestore() { + return contextToRestore; + } +} diff --git a/server/src/main/java/org/opensearch/rest/RestRequest.java b/server/src/main/java/org/opensearch/rest/RestRequest.java index f64774686c89d..275341cd960b8 100644 --- a/server/src/main/java/org/opensearch/rest/RestRequest.java +++ b/server/src/main/java/org/opensearch/rest/RestRequest.java @@ -76,7 +76,6 @@ public class RestRequest implements ToXContent.Params { // tchar pattern as defined by RFC7230 section 3.2.6 private static final Pattern TCHAR_PATTERN = Pattern.compile("[a-zA-z0-9!#$%&'*+\\-.\\^_`|~]+"); - private static final AtomicLong requestIdGenerator = new AtomicLong(); private final NamedXContentRegistry xContentRegistry; @@ -152,7 +151,7 @@ protected RestRequest(RestRequest restRequest) { * with an unpooled copy. This is supposed to be used before passing requests to {@link RestHandler} instances that can not safely * handle http requests that use pooled buffers as determined by {@link RestHandler#allowsUnsafeBuffers()}. */ - void ensureSafeBuffers() { + protected void ensureSafeBuffers() { httpRequest = httpRequest.releaseAndCopy(); } @@ -180,6 +179,36 @@ public static RestRequest request(NamedXContentRegistry xContentRegistry, HttpRe ); } + /** + * Creates a new REST request. This method will throw {@link BadParameterException} if the path cannot be + * decoded + * + * @param xContentRegistry the content registry + * @param httpRequest the http request + * @param httpChannel the http channel + * @param shouldGenerateRequestId should generate a new request id + * @throws BadParameterException if the parameters can not be decoded + * @throws ContentTypeHeaderException if the Content-Type header can not be parsed + */ + public static RestRequest request( + NamedXContentRegistry xContentRegistry, + HttpRequest httpRequest, + HttpChannel httpChannel, + boolean shouldGenerateRequestId + ) { + Map params = params(httpRequest.uri()); + String path = path(httpRequest.uri()); + return new RestRequest( + xContentRegistry, + params, + path, + httpRequest.getHeaders(), + httpRequest, + httpChannel, + shouldGenerateRequestId ? requestIdGenerator.incrementAndGet() : -1 + ); + } + private static Map params(final String uri) { final Map params = new HashMap<>(); int index = uri.indexOf('?'); @@ -228,6 +257,34 @@ public static RestRequest requestWithoutParameters( ); } + /** + * Creates a new REST request. The path is not decoded so this constructor will not throw a + * {@link BadParameterException}. + * + * @param xContentRegistry the content registry + * @param httpRequest the http request + * @param httpChannel the http channel + * @param shouldGenerateRequestId should generate new request id + * @throws ContentTypeHeaderException if the Content-Type header can not be parsed + */ + public static RestRequest requestWithoutParameters( + NamedXContentRegistry xContentRegistry, + HttpRequest httpRequest, + HttpChannel httpChannel, + boolean shouldGenerateRequestId + ) { + Map params = Collections.emptyMap(); + return new RestRequest( + xContentRegistry, + params, + httpRequest.uri(), + httpRequest.getHeaders(), + httpRequest, + httpChannel, + shouldGenerateRequestId ? requestIdGenerator.incrementAndGet() : -1 + ); + } + /** * The method used. * diff --git a/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java b/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java index c34f13041cb11..eaa199664fceb 100644 --- a/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java +++ b/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java @@ -49,6 +49,7 @@ import org.opensearch.core.rest.RestStatus; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.rest.RestChannel; +import org.opensearch.rest.RestHandlerContext; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestResponse; import org.opensearch.tasks.Task; @@ -64,6 +65,7 @@ import java.net.InetSocketAddress; import java.net.UnknownHostException; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -149,6 +151,21 @@ public void testHttpPublishPort() throws Exception { } } + public void testCreateRestRequestDoesNotGenerateRequestID() { + FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent( + new BytesArray("bar".getBytes(StandardCharsets.UTF_8)), + null + ).withPath("/foo").withHeaders(Collections.singletonMap("Content-Type", Collections.singletonList("text/plain"))).build(); + + RestRequest request = AbstractHttpServerTransport.createRestRequest( + xContentRegistry(), + fakeRestRequest.getHttpRequest(), + fakeRestRequest.getHttpChannel() + ); + + assertEquals("request should not generate id", -1, request.getRequestId()); + } + public void testDispatchDoesNotModifyThreadContext() { final HttpServerTransport.Dispatcher dispatcher = new HttpServerTransport.Dispatcher() { @@ -200,11 +217,11 @@ public HttpStats stats() { } ) { - transport.dispatchRequest(null, null, null); + transport.dispatchRequest(null, null, null, null); assertNull(threadPool.getThreadContext().getHeader("foo")); assertNull(threadPool.getThreadContext().getTransient("bar")); - transport.dispatchRequest(null, null, new Exception()); + transport.dispatchRequest(null, null, new Exception(), null); assertNull(threadPool.getThreadContext().getHeader("foo_bad")); assertNull(threadPool.getThreadContext().getTransient("bar_bad")); } @@ -321,7 +338,7 @@ public HttpStats stats() { .withInboundException(inboundException) .build(); - transport.incomingRequest(fakeRestRequest.getHttpRequest(), fakeRestRequest.getHttpChannel()); + transport.incomingRequest(fakeRestRequest.getHttpRequest(), fakeRestRequest.getHttpChannel(), RestHandlerContext.EMPTY); final Exception inboundExceptionExcludedPath; if (randomBoolean()) { @@ -338,7 +355,11 @@ public HttpStats stats() { .withInboundException(inboundExceptionExcludedPath) .build(); - transport.incomingRequest(fakeRestRequestExcludedPath.getHttpRequest(), fakeRestRequestExcludedPath.getHttpChannel()); + transport.incomingRequest( + fakeRestRequestExcludedPath.getHttpRequest(), + fakeRestRequestExcludedPath.getHttpChannel(), + RestHandlerContext.EMPTY + ); appender.assertAllExpectationsMatched(); } } diff --git a/server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java b/server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java new file mode 100644 index 0000000000000..ca802a7784ca0 --- /dev/null +++ b/server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java @@ -0,0 +1,58 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.rest; + +import org.opensearch.client.node.NodeClient; +import org.opensearch.core.common.bytes.BytesArray; +import org.opensearch.core.rest.RestStatus; +import org.opensearch.test.OpenSearchTestCase; + +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.Arrays; +import java.util.List; +import java.util.stream.Collectors; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +public class DelegatingRestHandlerTests extends OpenSearchTestCase { + public void testDelegatingRestHandlerShouldActAsOriginal() throws Exception { + RestHandler rh = new RestHandler() { + @Override + public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + new BytesRestResponse(RestStatus.OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY); + } + }; + RestHandler handlerSpy = spy(rh); + DelegatingRestHandler drh = new DelegatingRestHandler(handlerSpy); + + List overridableMethods = Arrays.stream(RestHandler.class.getMethods()) + .filter( + m -> !(Modifier.isPrivate(m.getModifiers()) || Modifier.isStatic(m.getModifiers()) || Modifier.isFinal(m.getModifiers())) + ) + .collect(Collectors.toList()); + + for (Method method : overridableMethods) { + int argCount = method.getParameterCount(); + Object[] args = new Object[argCount]; + for (int i = 0; i < argCount; i++) { + args[i] = any(); + } + if (args.length > 0) { + method.invoke(drh, args); + } else { + method.invoke(drh); + } + method.invoke(verify(handlerSpy, times(1)), args); + } + } +} From 16ecd7ff0980f4dd64db5ad0c7e30bbf629a3877 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 29 Sep 2023 04:45:28 -0400 Subject: [PATCH 12/26] Remove createDecompressor extension point in favor of attributeKey that can be populated by an extending transport Signed-off-by: Craig Perkins --- .../netty4/Netty4ConditionalDecompressor.java | 16 ++++++++++++++ .../netty4/Netty4HttpServerTransport.java | 22 +++++++++---------- .../http/AbstractHttpServerTransport.java | 1 - 3 files changed, 27 insertions(+), 12 deletions(-) create mode 100644 modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4ConditionalDecompressor.java diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4ConditionalDecompressor.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4ConditionalDecompressor.java new file mode 100644 index 0000000000000..57e08134ab16c --- /dev/null +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4ConditionalDecompressor.java @@ -0,0 +1,16 @@ +package org.opensearch.http.netty4; + +import io.netty.channel.embedded.EmbeddedChannel; +import io.netty.handler.codec.http.HttpContentDecompressor; + +import static org.opensearch.http.netty4.Netty4HttpServerTransport.SHOULD_DECOMPRESS; + +public class Netty4ConditionalDecompressor extends HttpContentDecompressor { + @Override + protected EmbeddedChannel newContentDecoder(String contentEncoding) throws Exception { + if (Boolean.FALSE.equals(ctx.channel().attr(SHOULD_DECOMPRESS).get())) { + return super.newContentDecoder("identity"); + } + return super.newContentDecoder(contentEncoding); + } +} diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java index 206f98017e8ec..d38cd1609efcf 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java @@ -80,7 +80,6 @@ import io.netty.channel.socket.nio.NioChannelOption; import io.netty.handler.codec.ByteToMessageDecoder; import io.netty.handler.codec.http.HttpContentCompressor; -import io.netty.handler.codec.http.HttpContentDecompressor; import io.netty.handler.codec.http.HttpMessage; import io.netty.handler.codec.http.HttpObjectAggregator; import io.netty.handler.codec.http.HttpRequestDecoder; @@ -345,6 +344,7 @@ public ChannelHandler configureServerChannelHandler() { public static final AttributeKey CONTEXT_TO_RESTORE = AttributeKey.newInstance( "opensearch-http-request-thread-context" ); + public static final AttributeKey SHOULD_DECOMPRESS = AttributeKey.newInstance("opensearch-http-should-decompress"); protected static class HttpChannelHandler extends ChannelInitializer { @@ -427,10 +427,14 @@ protected void channelRead0(ChannelHandlerContext ctx, HttpMessage msg) throws E final ChannelPipeline pipeline = ctx.pipeline(); pipeline.addAfter(ctx.name(), "handler", getRequestHandler()); pipeline.replace(this, "header_verifier", transport.createHeaderVerifier()); - pipeline.addAfter("header_verifier", "decompress", transport.createDecompressor()); - pipeline.addAfter("decompress", "aggregator", aggregator); + pipeline.addAfter("header_verifier", "decoder_compress", new Netty4ConditionalDecompressor()); + pipeline.addAfter("decoder_compress", "aggregator", aggregator); if (handlingSettings.isCompression()) { - pipeline.addAfter("aggregator", "compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); + pipeline.addAfter( + "aggregator", + "encoder_compress", + new HttpContentCompressor(handlingSettings.getCompressionLevel()) + ); } pipeline.addBefore("handler", "request_creator", requestCreator); pipeline.addBefore("handler", "response_creator", responseCreator); @@ -450,13 +454,13 @@ protected void configureDefaultHttpPipeline(ChannelPipeline pipeline) { decoder.setCumulator(ByteToMessageDecoder.COMPOSITE_CUMULATOR); pipeline.addLast("decoder", decoder); pipeline.addLast("header_verifier", transport.createHeaderVerifier()); - pipeline.addLast("decompress", transport.createDecompressor()); + pipeline.addLast("decoder_compress", new Netty4ConditionalDecompressor()); pipeline.addLast("encoder", new HttpResponseEncoder()); final HttpObjectAggregator aggregator = new HttpObjectAggregator(handlingSettings.getMaxContentLength()); aggregator.setMaxCumulationBufferComponents(transport.maxCompositeBufferComponents); pipeline.addLast("aggregator", aggregator); if (handlingSettings.isCompression()) { - pipeline.addLast("compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); + pipeline.addLast("encoder_compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); } pipeline.addLast("request_creator", requestCreator); pipeline.addLast("response_creator", responseCreator); @@ -497,7 +501,7 @@ protected void initChannel(Channel childChannel) throws Exception { .addLast("byte_buf_sizer", byteBufSizer) .addLast("read_timeout", new ReadTimeoutHandler(transport.readTimeoutMillis, TimeUnit.MILLISECONDS)) .addLast("header_verifier", transport.createHeaderVerifier()) - .addLast("decompress", transport.createDecompressor()); + .addLast("decoder_compress", new Netty4ConditionalDecompressor()); if (handlingSettings.isCompression()) { childChannel.pipeline().addLast("compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); @@ -535,10 +539,6 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { } } - protected HttpContentDecompressor createDecompressor() { - return new HttpContentDecompressor(); - } - protected ChannelInboundHandlerAdapter createHeaderVerifier() { // pass-through return new ChannelInboundHandlerAdapter(); diff --git a/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java b/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java index 61c2fa01c9c06..d4c3a8c79c5db 100644 --- a/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java +++ b/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java @@ -490,7 +490,6 @@ public static RestRequest createRestRequest( final HttpRequest httpRequest, final HttpChannel httpChannel ) { - // TODO Figure out how to only generate one request ID for each request in the pipeline. Exception badRequestCause = httpRequest.getInboundException(); /* From e6209c7c363420d27f8dc7ed1d286448a61cac56 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 29 Sep 2023 11:12:29 -0400 Subject: [PATCH 13/26] Minor update Signed-off-by: Craig Perkins --- .../org/opensearch/http/netty4/Netty4DefaultHttpRequest.java | 1 - .../org/opensearch/http/netty4/Netty4HttpServerTransport.java | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4DefaultHttpRequest.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4DefaultHttpRequest.java index 59f5e9f308aa4..2f8ab02d68585 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4DefaultHttpRequest.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4DefaultHttpRequest.java @@ -51,7 +51,6 @@ public String uri() { @Override public BytesReference content() { - // throw new RuntimeException("Not implemented"); return BytesArray.EMPTY; } diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java index d38cd1609efcf..c82869de4bc8b 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java @@ -504,7 +504,8 @@ protected void initChannel(Channel childChannel) throws Exception { .addLast("decoder_compress", new Netty4ConditionalDecompressor()); if (handlingSettings.isCompression()) { - childChannel.pipeline().addLast("compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); + childChannel.pipeline() + .addLast("encoder_compress", new HttpContentCompressor(handlingSettings.getCompressionLevel())); } childChannel.pipeline() From 54a0a96821e403a1015628e776992485a57fb123 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 29 Sep 2023 11:19:52 -0400 Subject: [PATCH 14/26] Match previous name Signed-off-by: Craig Perkins --- .../org/opensearch/http/netty4/Netty4HttpServerTransport.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java index c82869de4bc8b..c5e8aff0960c3 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java @@ -501,7 +501,7 @@ protected void initChannel(Channel childChannel) throws Exception { .addLast("byte_buf_sizer", byteBufSizer) .addLast("read_timeout", new ReadTimeoutHandler(transport.readTimeoutMillis, TimeUnit.MILLISECONDS)) .addLast("header_verifier", transport.createHeaderVerifier()) - .addLast("decoder_compress", new Netty4ConditionalDecompressor()); + .addLast("decoder_decompress", new Netty4ConditionalDecompressor()); if (handlingSettings.isCompression()) { childChannel.pipeline() From f4eb416a30f3a431afd338ecdda34802d8c7dd9d Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 29 Sep 2023 11:32:32 -0400 Subject: [PATCH 15/26] Add license header Signed-off-by: Craig Perkins --- .../http/netty4/Netty4ConditionalDecompressor.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4ConditionalDecompressor.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4ConditionalDecompressor.java index 57e08134ab16c..72815bae24ae6 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4ConditionalDecompressor.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4ConditionalDecompressor.java @@ -1,3 +1,11 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + package org.opensearch.http.netty4; import io.netty.channel.embedded.EmbeddedChannel; From 226299a338abfa6d06183fd986a3be00c519b515 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 29 Sep 2023 13:21:45 -0400 Subject: [PATCH 16/26] Back out DelegatingRestHandler changes to simplify this PR and follow with a PR to introduce DelegatingRestHandler Signed-off-by: Craig Perkins --- .../http/netty4/Netty4BlockingPlugin.java | 131 ++++++++++++++++++ .../http/netty4/Netty4HeaderVerifierIT.java | 69 +++++++++ .../transport/Netty4ModulePlugin.java | 2 +- .../example/ExampleNetty4HeaderVerifier.java | 9 ++ .../rest/DelegatingRestHandler.java | 74 ---------- .../org/opensearch/rest/RestController.java | 2 +- .../java/org/opensearch/rest/RestHandler.java | 64 ++++++++- .../rest/DelegatingRestHandlerTests.java | 58 -------- 8 files changed, 273 insertions(+), 136 deletions(-) create mode 100644 modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java create mode 100644 modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java create mode 100644 modules/transport-netty4/src/test/java/org/opensearch/http/netty4/example/ExampleNetty4HeaderVerifier.java delete mode 100644 server/src/main/java/org/opensearch/rest/DelegatingRestHandler.java delete mode 100644 server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java diff --git a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java new file mode 100644 index 0000000000000..853ea34eb7fbc --- /dev/null +++ b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java @@ -0,0 +1,131 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.http.netty4; + +import org.opensearch.common.network.NetworkService; +import org.opensearch.common.settings.ClusterSettings; +import org.opensearch.common.settings.Settings; +import org.opensearch.common.util.BigArrays; +import org.opensearch.common.util.PageCacheRecycler; +import org.opensearch.core.indices.breaker.CircuitBreakerService; +import org.opensearch.core.xcontent.NamedXContentRegistry; +import org.opensearch.http.HttpServerTransport; +import org.opensearch.telemetry.tracing.Tracer; +import org.opensearch.threadpool.ThreadPool; +import org.opensearch.transport.Netty4ModulePlugin; +import org.opensearch.transport.SharedGroupFactory; + +import java.util.Collections; +import java.util.Map; +import java.util.function.Supplier; + +import io.netty.channel.ChannelFutureListener; +import io.netty.channel.ChannelHandlerContext; +import io.netty.channel.ChannelInboundHandlerAdapter; +import io.netty.handler.codec.http.DefaultFullHttpResponse; +import io.netty.handler.codec.http.FullHttpResponse; +import io.netty.handler.codec.http.HttpRequest; +import io.netty.handler.codec.http.HttpResponseStatus; +import io.netty.handler.codec.http.HttpVersion; +import io.netty.util.ReferenceCountUtil; + +public class Netty4BlockingPlugin extends Netty4ModulePlugin { + + public Netty4BlockingPlugin() { + super(); + } + + public class Netty4BlockingHttpServerTransport extends Netty4HttpServerTransport { + + public Netty4BlockingHttpServerTransport( + Settings settings, + NetworkService networkService, + BigArrays bigArrays, + ThreadPool threadPool, + NamedXContentRegistry xContentRegistry, + Dispatcher dispatcher, + ClusterSettings clusterSettings, + SharedGroupFactory sharedGroupFactory, + Tracer tracer + ) { + super( + settings, + networkService, + bigArrays, + threadPool, + xContentRegistry, + dispatcher, + clusterSettings, + sharedGroupFactory, + tracer + ); + } + + @Override + protected ChannelInboundHandlerAdapter createHeaderVerifier() { + return new ExampleBlockingNetty4HeaderVerifier(); + } + } + + @Override + public Map> getHttpTransports( + Settings settings, + ThreadPool threadPool, + BigArrays bigArrays, + PageCacheRecycler pageCacheRecycler, + CircuitBreakerService circuitBreakerService, + NamedXContentRegistry xContentRegistry, + NetworkService networkService, + HttpServerTransport.Dispatcher dispatcher, + ClusterSettings clusterSettings, + Tracer tracer + ) { + return Collections.singletonMap( + NETTY_HTTP_TRANSPORT_NAME, + () -> new Netty4BlockingHttpServerTransport( + settings, + networkService, + bigArrays, + threadPool, + xContentRegistry, + dispatcher, + clusterSettings, + getSharedGroupFactory(settings), + tracer + ) + ); + } + + /** POC for how an external header verifier would be implemented */ + public class ExampleBlockingNetty4HeaderVerifier extends ChannelInboundHandlerAdapter { + + @Override + public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception { + if (!(msg instanceof HttpRequest)) { + ctx.fireChannelRead(msg); + } + + HttpRequest request = (HttpRequest) msg; + if (!isAuthenticated(request)) { + final FullHttpResponse response = new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.UNAUTHORIZED); + ctx.writeAndFlush(response).addListener(ChannelFutureListener.CLOSE); + ReferenceCountUtil.release(msg); + } else { + // Lets the request pass to the next channel handler + ctx.fireChannelRead(msg); + } + } + + private boolean isAuthenticated(HttpRequest request) { + final boolean shouldBlock = request.headers().contains("blockme"); + + return !shouldBlock; + } + } +} diff --git a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java new file mode 100644 index 0000000000000..e4437761851af --- /dev/null +++ b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java @@ -0,0 +1,69 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.http.netty4; + +import org.opensearch.OpenSearchNetty4IntegTestCase; +import org.opensearch.core.common.transport.TransportAddress; +import org.opensearch.http.HttpServerTransport; +import org.opensearch.plugins.Plugin; +import org.opensearch.test.OpenSearchIntegTestCase.ClusterScope; +import org.opensearch.test.OpenSearchIntegTestCase.Scope; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; + +import io.netty.handler.codec.http.DefaultFullHttpRequest; +import io.netty.handler.codec.http.FullHttpRequest; +import io.netty.handler.codec.http.FullHttpResponse; +import io.netty.handler.codec.http.HttpMethod; +import io.netty.handler.codec.http.HttpVersion; +import io.netty.util.ReferenceCounted; + +import static org.hamcrest.CoreMatchers.equalTo; +import static io.netty.handler.codec.http.HttpHeaderNames.HOST; + +@ClusterScope(scope = Scope.TEST, supportsDedicatedMasters = false, numDataNodes = 1) +public class Netty4HeaderVerifierIT extends OpenSearchNetty4IntegTestCase { + + @Override + protected boolean addMockHttpTransport() { + return false; // enable http + } + + @Override + protected Collection> nodePlugins() { + return Collections.singletonList(Netty4BlockingPlugin.class); + } + + @AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/10260") + public void testThatNettyHttpServerRequestBlockedWithHeaderVerifier() throws Exception { + HttpServerTransport httpServerTransport = internalCluster().getInstance(HttpServerTransport.class); + TransportAddress[] boundAddresses = httpServerTransport.boundAddress().boundAddresses(); + TransportAddress transportAddress = randomFrom(boundAddresses); + + final FullHttpRequest blockedRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/"); + blockedRequest.headers().add("blockme", "Not Allowed"); + blockedRequest.headers().add(HOST, "localhost"); + blockedRequest.headers().add("scheme", "http"); + + final List responses = new ArrayList<>(); + try (Netty4HttpClient nettyHttpClient = Netty4HttpClient.http2()) { + try { + FullHttpResponse blockedResponse = nettyHttpClient.send(transportAddress.address(), blockedRequest); + responses.add(blockedResponse); + assertThat(blockedResponse.status().code(), equalTo(401)); + } finally { + responses.forEach(ReferenceCounted::release); + } + } + } + +} diff --git a/modules/transport-netty4/src/main/java/org/opensearch/transport/Netty4ModulePlugin.java b/modules/transport-netty4/src/main/java/org/opensearch/transport/Netty4ModulePlugin.java index ca51d70702a82..6cfa09988ae91 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/transport/Netty4ModulePlugin.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/transport/Netty4ModulePlugin.java @@ -142,7 +142,7 @@ public Map> getHttpTransports( ); } - private SharedGroupFactory getSharedGroupFactory(Settings settings) { + protected SharedGroupFactory getSharedGroupFactory(Settings settings) { SharedGroupFactory groupFactory = this.groupFactory.get(); if (groupFactory != null) { assert groupFactory.getSettings().equals(settings) : "Different settings than originally provided"; diff --git a/modules/transport-netty4/src/test/java/org/opensearch/http/netty4/example/ExampleNetty4HeaderVerifier.java b/modules/transport-netty4/src/test/java/org/opensearch/http/netty4/example/ExampleNetty4HeaderVerifier.java new file mode 100644 index 0000000000000..fb2d228cdd431 --- /dev/null +++ b/modules/transport-netty4/src/test/java/org/opensearch/http/netty4/example/ExampleNetty4HeaderVerifier.java @@ -0,0 +1,9 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.http.netty4.example; diff --git a/server/src/main/java/org/opensearch/rest/DelegatingRestHandler.java b/server/src/main/java/org/opensearch/rest/DelegatingRestHandler.java deleted file mode 100644 index 928ff94d8c1d3..0000000000000 --- a/server/src/main/java/org/opensearch/rest/DelegatingRestHandler.java +++ /dev/null @@ -1,74 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.rest; - -import org.opensearch.client.node.NodeClient; - -import java.util.List; -import java.util.Objects; - -/** - * Delegating RestHandler that delegates all implementations to original handler - * - * @opensearch.api - */ -public class DelegatingRestHandler implements RestHandler { - - protected final RestHandler delegate; - - public DelegatingRestHandler(RestHandler delegate) { - Objects.requireNonNull(delegate, "RestHandler delegate can not be null"); - this.delegate = delegate; - } - - @Override - public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { - delegate.handleRequest(request, channel, client); - } - - @Override - public boolean canTripCircuitBreaker() { - return delegate.canTripCircuitBreaker(); - } - - @Override - public boolean supportsContentStream() { - return delegate.supportsContentStream(); - } - - @Override - public boolean allowsUnsafeBuffers() { - return delegate.allowsUnsafeBuffers(); - } - - @Override - public List routes() { - return delegate.routes(); - } - - @Override - public List deprecatedRoutes() { - return delegate.deprecatedRoutes(); - } - - @Override - public List replacedRoutes() { - return delegate.replacedRoutes(); - } - - @Override - public boolean allowSystemIndexAccessByDefault() { - return delegate.allowSystemIndexAccessByDefault(); - } - - @Override - public String toString() { - return delegate.toString(); - } -} diff --git a/server/src/main/java/org/opensearch/rest/RestController.java b/server/src/main/java/org/opensearch/rest/RestController.java index 4929f2a147dae..92f31076a1489 100644 --- a/server/src/main/java/org/opensearch/rest/RestController.java +++ b/server/src/main/java/org/opensearch/rest/RestController.java @@ -131,7 +131,7 @@ public RestController( this.headersToCopy = headersToCopy; this.usageService = usageService; if (handlerWrapper == null) { - handlerWrapper = (delegate) -> new DelegatingRestHandler(delegate); + handlerWrapper = (h) -> h; } this.handlerWrapper = handlerWrapper; this.client = client; diff --git a/server/src/main/java/org/opensearch/rest/RestHandler.java b/server/src/main/java/org/opensearch/rest/RestHandler.java index edb1cb341d2d8..294dc3ffbe329 100644 --- a/server/src/main/java/org/opensearch/rest/RestHandler.java +++ b/server/src/main/java/org/opensearch/rest/RestHandler.java @@ -44,8 +44,6 @@ /** * Handler for REST requests * - * If new methods are added to this interface they must also be added to {@link DelegatingRestHandler} - * * @opensearch.api */ @FunctionalInterface @@ -117,6 +115,68 @@ default boolean allowSystemIndexAccessByDefault() { return false; } + static RestHandler wrapper(RestHandler delegate) { + return new Wrapper(delegate); + } + + /** + * Wrapper for a handler. + * + * @opensearch.internal + */ + class Wrapper implements RestHandler { + private final RestHandler delegate; + + public Wrapper(RestHandler delegate) { + this.delegate = Objects.requireNonNull(delegate, "RestHandler delegate can not be null"); + } + + @Override + public String toString() { + return delegate.toString(); + } + + @Override + public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + delegate.handleRequest(request, channel, client); + } + + @Override + public boolean canTripCircuitBreaker() { + return delegate.canTripCircuitBreaker(); + } + + @Override + public boolean supportsContentStream() { + return delegate.supportsContentStream(); + } + + @Override + public boolean allowsUnsafeBuffers() { + return delegate.allowsUnsafeBuffers(); + } + + @Override + public List routes() { + return delegate.routes(); + } + + @Override + public List deprecatedRoutes() { + return delegate.deprecatedRoutes(); + } + + @Override + public List replacedRoutes() { + return delegate.replacedRoutes(); + } + + @Override + public boolean allowSystemIndexAccessByDefault() { + return delegate.allowSystemIndexAccessByDefault(); + } + } + /** * Route for the request. * diff --git a/server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java b/server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java deleted file mode 100644 index ca802a7784ca0..0000000000000 --- a/server/src/test/java/org/opensearch/rest/DelegatingRestHandlerTests.java +++ /dev/null @@ -1,58 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.rest; - -import org.opensearch.client.node.NodeClient; -import org.opensearch.core.common.bytes.BytesArray; -import org.opensearch.core.rest.RestStatus; -import org.opensearch.test.OpenSearchTestCase; - -import java.lang.reflect.Method; -import java.lang.reflect.Modifier; -import java.util.Arrays; -import java.util.List; -import java.util.stream.Collectors; - -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; - -public class DelegatingRestHandlerTests extends OpenSearchTestCase { - public void testDelegatingRestHandlerShouldActAsOriginal() throws Exception { - RestHandler rh = new RestHandler() { - @Override - public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { - new BytesRestResponse(RestStatus.OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY); - } - }; - RestHandler handlerSpy = spy(rh); - DelegatingRestHandler drh = new DelegatingRestHandler(handlerSpy); - - List overridableMethods = Arrays.stream(RestHandler.class.getMethods()) - .filter( - m -> !(Modifier.isPrivate(m.getModifiers()) || Modifier.isStatic(m.getModifiers()) || Modifier.isFinal(m.getModifiers())) - ) - .collect(Collectors.toList()); - - for (Method method : overridableMethods) { - int argCount = method.getParameterCount(); - Object[] args = new Object[argCount]; - for (int i = 0; i < argCount; i++) { - args[i] = any(); - } - if (args.length > 0) { - method.invoke(drh, args); - } else { - method.invoke(drh); - } - method.invoke(verify(handlerSpy, times(1)), args); - } - } -} From 4689e307bf20ac03ffedb9e44ac2208fc092c1ad Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 29 Sep 2023 14:28:24 -0400 Subject: [PATCH 17/26] Small update to test Signed-off-by: Craig Perkins --- .../org/opensearch/http/netty4/Netty4BlockingPlugin.java | 8 ++------ .../opensearch/http/netty4/Netty4HeaderVerifierIT.java | 5 +++-- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java index 853ea34eb7fbc..0acf3e50325c7 100644 --- a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java +++ b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java @@ -32,15 +32,10 @@ import io.netty.handler.codec.http.FullHttpResponse; import io.netty.handler.codec.http.HttpRequest; import io.netty.handler.codec.http.HttpResponseStatus; -import io.netty.handler.codec.http.HttpVersion; import io.netty.util.ReferenceCountUtil; public class Netty4BlockingPlugin extends Netty4ModulePlugin { - public Netty4BlockingPlugin() { - super(); - } - public class Netty4BlockingHttpServerTransport extends Netty4HttpServerTransport { public Netty4BlockingHttpServerTransport( @@ -109,11 +104,12 @@ public class ExampleBlockingNetty4HeaderVerifier extends ChannelInboundHandlerAd public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception { if (!(msg instanceof HttpRequest)) { ctx.fireChannelRead(msg); + return; } HttpRequest request = (HttpRequest) msg; if (!isAuthenticated(request)) { - final FullHttpResponse response = new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.UNAUTHORIZED); + final FullHttpResponse response = new DefaultFullHttpResponse(request.protocolVersion(), HttpResponseStatus.UNAUTHORIZED); ctx.writeAndFlush(response).addListener(ChannelFutureListener.CLOSE); ReferenceCountUtil.release(msg); } else { diff --git a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java index e4437761851af..33af5ed185b12 100644 --- a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java +++ b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java @@ -52,14 +52,15 @@ public void testThatNettyHttpServerRequestBlockedWithHeaderVerifier() throws Exc final FullHttpRequest blockedRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/"); blockedRequest.headers().add("blockme", "Not Allowed"); blockedRequest.headers().add(HOST, "localhost"); - blockedRequest.headers().add("scheme", "http"); final List responses = new ArrayList<>(); - try (Netty4HttpClient nettyHttpClient = Netty4HttpClient.http2()) { + try (Netty4HttpClient nettyHttpClient = Netty4HttpClient.http()) { try { FullHttpResponse blockedResponse = nettyHttpClient.send(transportAddress.address(), blockedRequest); responses.add(blockedResponse); assertThat(blockedResponse.status().code(), equalTo(401)); + } catch (Exception e) { + e.printStackTrace(); } finally { responses.forEach(ReferenceCounted::release); } From c227e6e3598be5f2b5d364e6e72294a38663591d Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 29 Sep 2023 15:49:24 -0400 Subject: [PATCH 18/26] remove printStackTrace Signed-off-by: Craig Perkins --- .../java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java index 33af5ed185b12..9ba93b8f91bf1 100644 --- a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java +++ b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java @@ -59,8 +59,6 @@ public void testThatNettyHttpServerRequestBlockedWithHeaderVerifier() throws Exc FullHttpResponse blockedResponse = nettyHttpClient.send(transportAddress.address(), blockedRequest); responses.add(blockedResponse); assertThat(blockedResponse.status().code(), equalTo(401)); - } catch (Exception e) { - e.printStackTrace(); } finally { responses.forEach(ReferenceCounted::release); } From aec3ad3dbf5e45c6ee0e79e6752b8b53bd2a72c0 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 3 Oct 2023 22:20:19 -0400 Subject: [PATCH 19/26] Remove channel attributes that are request specific Signed-off-by: Craig Perkins --- .../org/opensearch/http/netty4/Netty4HttpRequestHandler.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java index cc331f1b4fd2d..b26d849e62dce 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java @@ -56,6 +56,8 @@ protected void channelRead0(ChannelHandlerContext ctx, HttpPipelinedRequest http final Netty4HttpChannel channel = ctx.channel().attr(Netty4HttpServerTransport.HTTP_CHANNEL_KEY).get(); final RestResponse earlyResponse = ctx.channel().attr(Netty4HttpServerTransport.EARLY_RESPONSE).get(); final ThreadContext.StoredContext contextToRestore = ctx.channel().attr(Netty4HttpServerTransport.CONTEXT_TO_RESTORE).get(); + ctx.channel().attr(Netty4HttpServerTransport.CONTEXT_TO_RESTORE).set(null); + ctx.channel().attr(Netty4HttpServerTransport.EARLY_RESPONSE).set(null); final RestHandlerContext requestContext = new RestHandlerContext(earlyResponse, contextToRestore); boolean success = false; try { From 01dfa8993458f82f9c54c5c5c4b5a29495c7d627 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 4 Oct 2023 16:29:28 -0400 Subject: [PATCH 20/26] Move new AttributeKeys to security plugin Signed-off-by: Craig Perkins --- .../http/netty4/Netty4BlockingPlugin.java | 24 +++++----- .../http/netty4/Netty4HeaderVerifierIT.java | 10 ++++- .../netty4/Netty4ConditionalDecompressor.java | 24 ---------- .../http/netty4/Netty4HttpRequestHandler.java | 10 +---- .../netty4/Netty4HttpServerTransport.java | 19 ++++---- .../example/ExampleNetty4HeaderVerifier.java | 9 ---- .../http/nio/HttpReadWriteHandler.java | 3 +- .../http/nio/HttpReadWriteHandlerTests.java | 13 +++--- .../http/AbstractHttpServerTransport.java | 30 +++---------- .../opensearch/rest/RestHandlerContext.java | 44 ------------------- .../AbstractHttpServerTransportTests.java | 13 ++---- 11 files changed, 45 insertions(+), 154 deletions(-) delete mode 100644 modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4ConditionalDecompressor.java delete mode 100644 modules/transport-netty4/src/test/java/org/opensearch/http/netty4/example/ExampleNetty4HeaderVerifier.java delete mode 100644 server/src/main/java/org/opensearch/rest/RestHandlerContext.java diff --git a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java index 0acf3e50325c7..ae17209543e5e 100644 --- a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java +++ b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java @@ -25,10 +25,14 @@ import java.util.Map; import java.util.function.Supplier; +import io.netty.buffer.ByteBuf; +import io.netty.buffer.Unpooled; import io.netty.channel.ChannelFutureListener; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelInboundHandlerAdapter; +import io.netty.channel.SimpleChannelInboundHandler; import io.netty.handler.codec.http.DefaultFullHttpResponse; +import io.netty.handler.codec.http.DefaultHttpRequest; import io.netty.handler.codec.http.FullHttpResponse; import io.netty.handler.codec.http.HttpRequest; import io.netty.handler.codec.http.HttpResponseStatus; @@ -98,18 +102,14 @@ public Map> getHttpTransports( } /** POC for how an external header verifier would be implemented */ - public class ExampleBlockingNetty4HeaderVerifier extends ChannelInboundHandlerAdapter { + public class ExampleBlockingNetty4HeaderVerifier extends SimpleChannelInboundHandler { @Override - public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception { - if (!(msg instanceof HttpRequest)) { - ctx.fireChannelRead(msg); - return; - } - - HttpRequest request = (HttpRequest) msg; - if (!isAuthenticated(request)) { - final FullHttpResponse response = new DefaultFullHttpResponse(request.protocolVersion(), HttpResponseStatus.UNAUTHORIZED); + public void channelRead0(ChannelHandlerContext ctx, DefaultHttpRequest msg) throws Exception { + ReferenceCountUtil.retain(msg); + if (isBlocked(msg)) { + ByteBuf buf = Unpooled.copiedBuffer("Hit header_verifier".getBytes()); + final FullHttpResponse response = new DefaultFullHttpResponse(msg.protocolVersion(), HttpResponseStatus.UNAUTHORIZED, buf); ctx.writeAndFlush(response).addListener(ChannelFutureListener.CLOSE); ReferenceCountUtil.release(msg); } else { @@ -118,10 +118,10 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception } } - private boolean isAuthenticated(HttpRequest request) { + private boolean isBlocked(HttpRequest request) { final boolean shouldBlock = request.headers().contains("blockme"); - return !shouldBlock; + return shouldBlock; } } } diff --git a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java index 9ba93b8f91bf1..94e7a3240454b 100644 --- a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java +++ b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java @@ -15,18 +15,22 @@ import org.opensearch.test.OpenSearchIntegTestCase.ClusterScope; import org.opensearch.test.OpenSearchIntegTestCase.Scope; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.List; +import io.netty.buffer.ByteBufUtil; import io.netty.handler.codec.http.DefaultFullHttpRequest; import io.netty.handler.codec.http.FullHttpRequest; import io.netty.handler.codec.http.FullHttpResponse; import io.netty.handler.codec.http.HttpMethod; import io.netty.handler.codec.http.HttpVersion; +import io.netty.handler.codec.http2.HttpConversionUtil; import io.netty.util.ReferenceCounted; +import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.equalTo; import static io.netty.handler.codec.http.HttpHeaderNames.HOST; @@ -43,7 +47,6 @@ protected Collection> nodePlugins() { return Collections.singletonList(Netty4BlockingPlugin.class); } - @AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/10260") public void testThatNettyHttpServerRequestBlockedWithHeaderVerifier() throws Exception { HttpServerTransport httpServerTransport = internalCluster().getInstance(HttpServerTransport.class); TransportAddress[] boundAddresses = httpServerTransport.boundAddress().boundAddresses(); @@ -52,12 +55,15 @@ public void testThatNettyHttpServerRequestBlockedWithHeaderVerifier() throws Exc final FullHttpRequest blockedRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/"); blockedRequest.headers().add("blockme", "Not Allowed"); blockedRequest.headers().add(HOST, "localhost"); + blockedRequest.headers().add(HttpConversionUtil.ExtensionHeaderNames.SCHEME.text(), "http"); final List responses = new ArrayList<>(); - try (Netty4HttpClient nettyHttpClient = Netty4HttpClient.http()) { + try (Netty4HttpClient nettyHttpClient = Netty4HttpClient.http2()) { try { FullHttpResponse blockedResponse = nettyHttpClient.send(transportAddress.address(), blockedRequest); responses.add(blockedResponse); + String blockedResponseContent = new String(ByteBufUtil.getBytes(blockedResponse.content()), StandardCharsets.UTF_8); + assertThat(blockedResponseContent, containsString("Hit header_verifier")); assertThat(blockedResponse.status().code(), equalTo(401)); } finally { responses.forEach(ReferenceCounted::release); diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4ConditionalDecompressor.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4ConditionalDecompressor.java deleted file mode 100644 index 72815bae24ae6..0000000000000 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4ConditionalDecompressor.java +++ /dev/null @@ -1,24 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.http.netty4; - -import io.netty.channel.embedded.EmbeddedChannel; -import io.netty.handler.codec.http.HttpContentDecompressor; - -import static org.opensearch.http.netty4.Netty4HttpServerTransport.SHOULD_DECOMPRESS; - -public class Netty4ConditionalDecompressor extends HttpContentDecompressor { - @Override - protected EmbeddedChannel newContentDecoder(String contentEncoding) throws Exception { - if (Boolean.FALSE.equals(ctx.channel().attr(SHOULD_DECOMPRESS).get())) { - return super.newContentDecoder("identity"); - } - return super.newContentDecoder(contentEncoding); - } -} diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java index b26d849e62dce..1f7aaf17d2191 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequestHandler.java @@ -33,10 +33,7 @@ package org.opensearch.http.netty4; import org.opensearch.ExceptionsHelper; -import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.http.HttpPipelinedRequest; -import org.opensearch.rest.RestHandlerContext; -import org.opensearch.rest.RestResponse; import io.netty.channel.ChannelHandler; import io.netty.channel.ChannelHandlerContext; @@ -54,14 +51,9 @@ class Netty4HttpRequestHandler extends SimpleChannelInboundHandler EARLY_RESPONSE = AttributeKey.newInstance("opensearch-http-early-response"); - public static final AttributeKey CONTEXT_TO_RESTORE = AttributeKey.newInstance( - "opensearch-http-request-thread-context" - ); - public static final AttributeKey SHOULD_DECOMPRESS = AttributeKey.newInstance("opensearch-http-should-decompress"); - protected static class HttpChannelHandler extends ChannelInitializer { private final Netty4HttpServerTransport transport; @@ -427,7 +420,7 @@ protected void channelRead0(ChannelHandlerContext ctx, HttpMessage msg) throws E final ChannelPipeline pipeline = ctx.pipeline(); pipeline.addAfter(ctx.name(), "handler", getRequestHandler()); pipeline.replace(this, "header_verifier", transport.createHeaderVerifier()); - pipeline.addAfter("header_verifier", "decoder_compress", new Netty4ConditionalDecompressor()); + pipeline.addAfter("header_verifier", "decoder_compress", transport.createDecompressor()); pipeline.addAfter("decoder_compress", "aggregator", aggregator); if (handlingSettings.isCompression()) { pipeline.addAfter( @@ -454,7 +447,7 @@ protected void configureDefaultHttpPipeline(ChannelPipeline pipeline) { decoder.setCumulator(ByteToMessageDecoder.COMPOSITE_CUMULATOR); pipeline.addLast("decoder", decoder); pipeline.addLast("header_verifier", transport.createHeaderVerifier()); - pipeline.addLast("decoder_compress", new Netty4ConditionalDecompressor()); + pipeline.addLast("decoder_compress", transport.createDecompressor()); pipeline.addLast("encoder", new HttpResponseEncoder()); final HttpObjectAggregator aggregator = new HttpObjectAggregator(handlingSettings.getMaxContentLength()); aggregator.setMaxCumulationBufferComponents(transport.maxCompositeBufferComponents); @@ -501,7 +494,7 @@ protected void initChannel(Channel childChannel) throws Exception { .addLast("byte_buf_sizer", byteBufSizer) .addLast("read_timeout", new ReadTimeoutHandler(transport.readTimeoutMillis, TimeUnit.MILLISECONDS)) .addLast("header_verifier", transport.createHeaderVerifier()) - .addLast("decoder_decompress", new Netty4ConditionalDecompressor()); + .addLast("decoder_decompress", transport.createDecompressor()); if (handlingSettings.isCompression()) { childChannel.pipeline() @@ -544,4 +537,8 @@ protected ChannelInboundHandlerAdapter createHeaderVerifier() { // pass-through return new ChannelInboundHandlerAdapter(); } + + protected ChannelInboundHandlerAdapter createDecompressor() { + return new HttpContentDecompressor(); + } } diff --git a/modules/transport-netty4/src/test/java/org/opensearch/http/netty4/example/ExampleNetty4HeaderVerifier.java b/modules/transport-netty4/src/test/java/org/opensearch/http/netty4/example/ExampleNetty4HeaderVerifier.java deleted file mode 100644 index fb2d228cdd431..0000000000000 --- a/modules/transport-netty4/src/test/java/org/opensearch/http/netty4/example/ExampleNetty4HeaderVerifier.java +++ /dev/null @@ -1,9 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.http.netty4.example; diff --git a/plugins/transport-nio/src/main/java/org/opensearch/http/nio/HttpReadWriteHandler.java b/plugins/transport-nio/src/main/java/org/opensearch/http/nio/HttpReadWriteHandler.java index 7b04f93d5c3be..d44515f3dc727 100644 --- a/plugins/transport-nio/src/main/java/org/opensearch/http/nio/HttpReadWriteHandler.java +++ b/plugins/transport-nio/src/main/java/org/opensearch/http/nio/HttpReadWriteHandler.java @@ -43,7 +43,6 @@ import org.opensearch.nio.SocketChannelContext; import org.opensearch.nio.TaskScheduler; import org.opensearch.nio.WriteOperation; -import org.opensearch.rest.RestHandlerContext; import java.io.IOException; import java.util.ArrayList; @@ -173,7 +172,7 @@ private void handleRequest(Object msg) { final HttpPipelinedRequest pipelinedRequest = (HttpPipelinedRequest) msg; boolean success = false; try { - transport.incomingRequest(pipelinedRequest, nioHttpChannel, RestHandlerContext.EMPTY); + transport.incomingRequest(pipelinedRequest, nioHttpChannel); success = true; } finally { if (success == false) { diff --git a/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java b/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java index 1172472a3c6b1..a3f7a7822cd40 100644 --- a/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java +++ b/plugins/transport-nio/src/test/java/org/opensearch/http/nio/HttpReadWriteHandlerTests.java @@ -48,7 +48,6 @@ import org.opensearch.nio.InboundChannelBuffer; import org.opensearch.nio.SocketChannelContext; import org.opensearch.nio.TaskScheduler; -import org.opensearch.rest.RestHandlerContext; import org.opensearch.rest.RestRequest; import org.opensearch.test.OpenSearchTestCase; import org.junit.Before; @@ -102,7 +101,7 @@ public void setMocks() { doAnswer(invocation -> { ((HttpRequest) invocation.getArguments()[0]).releaseAndCopy(); return null; - }).when(transport).incomingRequest(any(HttpRequest.class), any(HttpChannel.class), any(RestHandlerContext.class)); + }).when(transport).incomingRequest(any(HttpRequest.class), any(HttpChannel.class)); Settings settings = Settings.builder().put(SETTING_HTTP_MAX_CONTENT_LENGTH.getKey(), new ByteSizeValue(1024)).build(); HttpHandlingSettings httpHandlingSettings = HttpHandlingSettings.fromSettings(settings); channel = mock(NioHttpChannel.class); @@ -123,12 +122,12 @@ public void testSuccessfulDecodeHttpRequest() throws IOException { try { handler.consumeReads(toChannelBuffer(slicedBuf)); - verify(transport, times(0)).incomingRequest(any(HttpRequest.class), any(NioHttpChannel.class), any(RestHandlerContext.class)); + verify(transport, times(0)).incomingRequest(any(HttpRequest.class), any(NioHttpChannel.class)); handler.consumeReads(toChannelBuffer(slicedBuf2)); ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(HttpRequest.class); - verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class), any(RestHandlerContext.class)); + verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class)); HttpRequest nioHttpRequest = requestCaptor.getValue(); assertEquals(HttpRequest.HttpVersion.HTTP_1_1, nioHttpRequest.protocolVersion()); @@ -154,7 +153,7 @@ public void testDecodeHttpRequestError() throws IOException { handler.consumeReads(toChannelBuffer(buf)); ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(HttpRequest.class); - verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class), any(RestHandlerContext.class)); + verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class)); assertNotNull(requestCaptor.getValue().getInboundException()); assertTrue(requestCaptor.getValue().getInboundException() instanceof IllegalArgumentException); @@ -175,7 +174,7 @@ public void testDecodeHttpRequestContentLengthToLongGeneratesOutboundMessage() t } finally { buf.release(); } - verify(transport, times(0)).incomingRequest(any(), any(), any(RestHandlerContext.class)); + verify(transport, times(0)).incomingRequest(any(), any()); List flushOperations = handler.pollFlushOperations(); assertFalse(flushOperations.isEmpty()); @@ -281,7 +280,7 @@ private void prepareHandlerForResponse(HttpReadWriteHandler handler) throws IOEx } ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(HttpPipelinedRequest.class); - verify(transport, atLeastOnce()).incomingRequest(requestCaptor.capture(), any(HttpChannel.class), any(RestHandlerContext.class)); + verify(transport, atLeastOnce()).incomingRequest(requestCaptor.capture(), any(HttpChannel.class)); HttpRequest httpRequest = requestCaptor.getValue(); assertNotNull(httpRequest); diff --git a/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java b/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java index d4c3a8c79c5db..bc64bd85cac04 100644 --- a/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java +++ b/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java @@ -53,7 +53,6 @@ import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.rest.RestChannel; -import org.opensearch.rest.RestHandlerContext; import org.opensearch.rest.RestRequest; import org.opensearch.telemetry.tracing.Span; import org.opensearch.telemetry.tracing.SpanBuilder; @@ -360,29 +359,20 @@ protected void serverAcceptedChannel(HttpChannel httpChannel) { * * @param httpRequest that is incoming * @param httpChannel that received the http request - * @param requestContext context carried over to the request handler from earlier stages in the request pipeline */ - public void incomingRequest(final HttpRequest httpRequest, final HttpChannel httpChannel, final RestHandlerContext requestContext) { + public void incomingRequest(final HttpRequest httpRequest, final HttpChannel httpChannel) { final Span span = tracer.startSpan(SpanBuilder.from(httpRequest), httpRequest.getHeaders()); try (final SpanScope httpRequestSpanScope = tracer.withSpanInScope(span)) { HttpChannel traceableHttpChannel = TraceableHttpChannel.create(httpChannel, span, tracer); - handleIncomingRequest(httpRequest, traceableHttpChannel, requestContext, httpRequest.getInboundException()); + handleIncomingRequest(httpRequest, traceableHttpChannel, httpRequest.getInboundException()); } } // Visible for testing - protected void dispatchRequest( - final RestRequest restRequest, - final RestChannel channel, - final Throwable badRequestCause, - final ThreadContext.StoredContext storedContext - ) { + void dispatchRequest(final RestRequest restRequest, final RestChannel channel, final Throwable badRequestCause) { RestChannel traceableRestChannel = channel; final ThreadContext threadContext = threadPool.getThreadContext(); try (ThreadContext.StoredContext ignore = threadContext.stashContext()) { - if (storedContext != null) { - storedContext.restore(); - } final Span span = tracer.startSpan(SpanBuilder.from(restRequest)); try (final SpanScope spanScope = tracer.withSpanInScope(span)) { if (channel != null) { @@ -398,12 +388,7 @@ protected void dispatchRequest( } - private void handleIncomingRequest( - final HttpRequest httpRequest, - final HttpChannel httpChannel, - final RestHandlerContext requestContext, - final Exception exception - ) { + private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChannel httpChannel, final Exception exception) { if (exception == null) { HttpResponse earlyResponse = corsHandler.handleInbound(httpRequest); if (earlyResponse != null) { @@ -477,12 +462,7 @@ private void handleIncomingRequest( channel = innerChannel; } - if (requestContext.hasEarlyResponse()) { - channel.sendResponse(requestContext.getEarlyResponse()); - return; - } - - dispatchRequest(restRequest, channel, badRequestCause, requestContext.getContextToRestore()); + dispatchRequest(restRequest, channel, badRequestCause); } public static RestRequest createRestRequest( diff --git a/server/src/main/java/org/opensearch/rest/RestHandlerContext.java b/server/src/main/java/org/opensearch/rest/RestHandlerContext.java deleted file mode 100644 index 297a44c705e2e..0000000000000 --- a/server/src/main/java/org/opensearch/rest/RestHandlerContext.java +++ /dev/null @@ -1,44 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.rest; - -import org.opensearch.common.util.concurrent.ThreadContext; - -/** - * Holder for information that is shared between stages of the request pipeline - */ -public class RestHandlerContext { - private RestResponse earlyResponse; - private ThreadContext.StoredContext contextToRestore; - - public static RestHandlerContext EMPTY = new RestHandlerContext(); - - private RestHandlerContext() {} - - public RestHandlerContext(final RestResponse earlyResponse, ThreadContext.StoredContext contextToRestore) { - this.earlyResponse = earlyResponse; - this.contextToRestore = contextToRestore; - } - - public boolean hasEarlyResponse() { - return this.earlyResponse != null; - } - - public boolean hasContextToRestore() { - return this.contextToRestore != null; - } - - public RestResponse getEarlyResponse() { - return this.earlyResponse; - } - - public ThreadContext.StoredContext getContextToRestore() { - return contextToRestore; - } -} diff --git a/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java b/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java index eaa199664fceb..7dcea1c206ac3 100644 --- a/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java +++ b/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java @@ -49,7 +49,6 @@ import org.opensearch.core.rest.RestStatus; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.rest.RestChannel; -import org.opensearch.rest.RestHandlerContext; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestResponse; import org.opensearch.tasks.Task; @@ -217,11 +216,11 @@ public HttpStats stats() { } ) { - transport.dispatchRequest(null, null, null, null); + transport.dispatchRequest(null, null, null); assertNull(threadPool.getThreadContext().getHeader("foo")); assertNull(threadPool.getThreadContext().getTransient("bar")); - transport.dispatchRequest(null, null, new Exception(), null); + transport.dispatchRequest(null, null, new Exception()); assertNull(threadPool.getThreadContext().getHeader("foo_bad")); assertNull(threadPool.getThreadContext().getTransient("bar_bad")); } @@ -338,7 +337,7 @@ public HttpStats stats() { .withInboundException(inboundException) .build(); - transport.incomingRequest(fakeRestRequest.getHttpRequest(), fakeRestRequest.getHttpChannel(), RestHandlerContext.EMPTY); + transport.incomingRequest(fakeRestRequest.getHttpRequest(), fakeRestRequest.getHttpChannel()); final Exception inboundExceptionExcludedPath; if (randomBoolean()) { @@ -355,11 +354,7 @@ public HttpStats stats() { .withInboundException(inboundExceptionExcludedPath) .build(); - transport.incomingRequest( - fakeRestRequestExcludedPath.getHttpRequest(), - fakeRestRequestExcludedPath.getHttpChannel(), - RestHandlerContext.EMPTY - ); + transport.incomingRequest(fakeRestRequestExcludedPath.getHttpRequest(), fakeRestRequestExcludedPath.getHttpChannel()); appender.assertAllExpectationsMatched(); } } From 3085f645a1a49fc141392a82c5cbba5c676b33b1 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 4 Oct 2023 17:06:30 -0400 Subject: [PATCH 21/26] Add charset Signed-off-by: Craig Perkins --- .../java/org/opensearch/http/netty4/Netty4BlockingPlugin.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java index ae17209543e5e..d1bc1d5fd0fa8 100644 --- a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java +++ b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java @@ -21,6 +21,7 @@ import org.opensearch.transport.Netty4ModulePlugin; import org.opensearch.transport.SharedGroupFactory; +import java.nio.charset.StandardCharsets; import java.util.Collections; import java.util.Map; import java.util.function.Supplier; @@ -108,7 +109,7 @@ public class ExampleBlockingNetty4HeaderVerifier extends SimpleChannelInboundHan public void channelRead0(ChannelHandlerContext ctx, DefaultHttpRequest msg) throws Exception { ReferenceCountUtil.retain(msg); if (isBlocked(msg)) { - ByteBuf buf = Unpooled.copiedBuffer("Hit header_verifier".getBytes()); + ByteBuf buf = Unpooled.copiedBuffer("Hit header_verifier".getBytes(StandardCharsets.UTF_8)); final FullHttpResponse response = new DefaultFullHttpResponse(msg.protocolVersion(), HttpResponseStatus.UNAUTHORIZED, buf); ctx.writeAndFlush(response).addListener(ChannelFutureListener.CLOSE); ReferenceCountUtil.release(msg); From 7ca4c7e94864700f4989df7fbc832119d1d4c62c Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 4 Oct 2023 23:12:08 -0400 Subject: [PATCH 22/26] Add javadoc on new extension points Signed-off-by: Craig Perkins --- .../opensearch/http/netty4/Netty4HttpServerTransport.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java index a5fdb0a444f8d..1677f333a4b1c 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java @@ -533,11 +533,19 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { } } + /** + * Extension point that allows a NetworkPlugin to extend the netty pipeline and inspect headers after request decoding + */ protected ChannelInboundHandlerAdapter createHeaderVerifier() { // pass-through return new ChannelInboundHandlerAdapter(); } + /** + * Extension point that allows a NetworkPlugin to override the default netty HttpContentDecompressor and supply a custom decompressor. + * + * Used in instances to conditionally decompress depending on the outcome from header verification + */ protected ChannelInboundHandlerAdapter createDecompressor() { return new HttpContentDecompressor(); } From 02b92ab86771ea5530c66c48d4127a3a135aa8e2 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 5 Oct 2023 12:53:49 -0400 Subject: [PATCH 23/26] Single request class Signed-off-by: Craig Perkins --- .../netty4/AbstractNetty4HttpRequest.java | 142 -------------- .../http/netty4/Netty4DefaultHttpRequest.java | 113 ----------- .../http/netty4/Netty4HttpRequest.java | 179 +++++++++++++++--- .../http/netty4/Netty4HttpResponse.java | 2 +- .../netty4/Netty4DefaultHttpRequestTests.java | 74 -------- 5 files changed, 149 insertions(+), 361 deletions(-) delete mode 100644 modules/transport-netty4/src/main/java/org/opensearch/http/netty4/AbstractNetty4HttpRequest.java delete mode 100644 modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4DefaultHttpRequest.java delete mode 100644 modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4DefaultHttpRequestTests.java diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/AbstractNetty4HttpRequest.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/AbstractNetty4HttpRequest.java deleted file mode 100644 index 69f6b9f200e75..0000000000000 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/AbstractNetty4HttpRequest.java +++ /dev/null @@ -1,142 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.http.netty4; - -import org.opensearch.rest.RestRequest; - -import java.util.AbstractMap; -import java.util.Collection; -import java.util.Collections; -import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.stream.Collectors; - -import io.netty.handler.codec.http.HttpHeaders; -import io.netty.handler.codec.http.HttpMethod; -import io.netty.handler.codec.http.HttpRequest; - -public abstract class AbstractNetty4HttpRequest { - - protected HttpHeadersMap headers; - protected Exception inboundException; - - protected RestRequest.Method getHttpMethod(HttpRequest request) { - HttpMethod httpMethod = request.method(); - if (httpMethod == HttpMethod.GET) return RestRequest.Method.GET; - - if (httpMethod == HttpMethod.POST) return RestRequest.Method.POST; - - if (httpMethod == HttpMethod.PUT) return RestRequest.Method.PUT; - - if (httpMethod == HttpMethod.DELETE) return RestRequest.Method.DELETE; - - if (httpMethod == HttpMethod.HEAD) { - return RestRequest.Method.HEAD; - } - - if (httpMethod == HttpMethod.OPTIONS) { - return RestRequest.Method.OPTIONS; - } - - if (httpMethod == HttpMethod.PATCH) { - return RestRequest.Method.PATCH; - } - - if (httpMethod == HttpMethod.TRACE) { - return RestRequest.Method.TRACE; - } - - if (httpMethod == HttpMethod.CONNECT) { - return RestRequest.Method.CONNECT; - } - - throw new IllegalArgumentException("Unexpected http method: " + httpMethod); - } - - /** - * A wrapper of {@link HttpHeaders} that implements a map to prevent copying unnecessarily. This class does not support modifications - * and due to the underlying implementation, it performs case insensitive lookups of key to values. - * - * It is important to note that this implementation does have some downsides in that each invocation of the - * {@link #values()} and {@link #entrySet()} methods will perform a copy of the values in the HttpHeaders rather than returning a - * view of the underlying values. - */ - protected static class HttpHeadersMap implements Map> { - - private final HttpHeaders httpHeaders; - - HttpHeadersMap(HttpHeaders httpHeaders) { - this.httpHeaders = httpHeaders; - } - - @Override - public int size() { - return httpHeaders.size(); - } - - @Override - public boolean isEmpty() { - return httpHeaders.isEmpty(); - } - - @Override - public boolean containsKey(Object key) { - return key instanceof String && httpHeaders.contains((String) key); - } - - @Override - public boolean containsValue(Object value) { - return value instanceof List && httpHeaders.names().stream().map(httpHeaders::getAll).anyMatch(value::equals); - } - - @Override - public List get(Object key) { - return key instanceof String ? httpHeaders.getAll((String) key) : null; - } - - @Override - public List put(String key, List value) { - throw new UnsupportedOperationException("modifications are not supported"); - } - - @Override - public List remove(Object key) { - throw new UnsupportedOperationException("modifications are not supported"); - } - - @Override - public void putAll(Map> m) { - throw new UnsupportedOperationException("modifications are not supported"); - } - - @Override - public void clear() { - throw new UnsupportedOperationException("modifications are not supported"); - } - - @Override - public Set keySet() { - return httpHeaders.names(); - } - - @Override - public Collection> values() { - return httpHeaders.names().stream().map(k -> Collections.unmodifiableList(httpHeaders.getAll(k))).collect(Collectors.toList()); - } - - @Override - public Set>> entrySet() { - return httpHeaders.names() - .stream() - .map(k -> new AbstractMap.SimpleImmutableEntry<>(k, httpHeaders.getAll(k))) - .collect(Collectors.toSet()); - } - } -} diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4DefaultHttpRequest.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4DefaultHttpRequest.java deleted file mode 100644 index 2f8ab02d68585..0000000000000 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4DefaultHttpRequest.java +++ /dev/null @@ -1,113 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.http.netty4; - -import org.opensearch.core.common.bytes.BytesArray; -import org.opensearch.core.common.bytes.BytesReference; -import org.opensearch.core.rest.RestStatus; -import org.opensearch.http.HttpRequest; -import org.opensearch.rest.RestRequest; - -import java.util.Collections; -import java.util.List; -import java.util.Map; -import java.util.Set; - -import io.netty.handler.codec.http.DefaultHttpRequest; -import io.netty.handler.codec.http.HttpHeaderNames; -import io.netty.handler.codec.http.cookie.Cookie; -import io.netty.handler.codec.http.cookie.ServerCookieDecoder; -import io.netty.handler.codec.http.cookie.ServerCookieEncoder; - -public class Netty4DefaultHttpRequest extends AbstractNetty4HttpRequest implements HttpRequest { - - private final DefaultHttpRequest request; - - public Netty4DefaultHttpRequest(DefaultHttpRequest request) { - this(request, new HttpHeadersMap(request.headers()), null); - } - - private Netty4DefaultHttpRequest(DefaultHttpRequest request, HttpHeadersMap headers, Exception inboundException) { - this.request = request; - this.headers = headers; - this.inboundException = inboundException; - } - - @Override - public RestRequest.Method method() { - return getHttpMethod(request); - } - - @Override - public String uri() { - return request.uri(); - } - - @Override - public BytesReference content() { - return BytesArray.EMPTY; - } - - @Override - public final Map> getHeaders() { - return headers; - } - - @Override - public List strictCookies() { - String cookieString = request.headers().get(HttpHeaderNames.COOKIE); - if (cookieString != null) { - Set cookies = ServerCookieDecoder.STRICT.decode(cookieString); - if (!cookies.isEmpty()) { - return ServerCookieEncoder.STRICT.encode(cookies); - } - } - return Collections.emptyList(); - } - - @Override - public HttpVersion protocolVersion() { - if (request.protocolVersion().equals(io.netty.handler.codec.http.HttpVersion.HTTP_1_0)) { - return HttpRequest.HttpVersion.HTTP_1_0; - } else if (request.protocolVersion().equals(io.netty.handler.codec.http.HttpVersion.HTTP_1_1)) { - return HttpRequest.HttpVersion.HTTP_1_1; - } else { - throw new IllegalArgumentException("Unexpected http protocol version: " + request.protocolVersion()); - } - } - - @Override - public HttpRequest removeHeader(String header) { - return null; - } - - @Override - public Netty4HttpResponse createResponse(RestStatus status, BytesReference content) { - return new Netty4HttpResponse(request.headers(), request.protocolVersion(), status, content); - } - - @Override - public Exception getInboundException() { - return inboundException; - } - - @Override - public void release() { - // do nothing - } - - @Override - public HttpRequest releaseAndCopy() { - return this; - } - - public DefaultHttpRequest nettyRequest() { - return request; - } -} diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java index 7e71e65c08c57..51a96b180c2c9 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java @@ -32,17 +32,20 @@ package org.opensearch.http.netty4; +import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.rest.RestStatus; -import org.opensearch.http.HttpRequest; import org.opensearch.rest.RestRequest; import org.opensearch.transport.netty4.Netty4Utils; +import java.util.AbstractMap; +import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.stream.Collectors; import io.netty.buffer.ByteBuf; import io.netty.buffer.Unpooled; @@ -51,50 +54,48 @@ import io.netty.handler.codec.http.FullHttpRequest; import io.netty.handler.codec.http.HttpHeaderNames; import io.netty.handler.codec.http.HttpHeaders; +import io.netty.handler.codec.http.HttpMethod; +import io.netty.handler.codec.http.HttpRequest; import io.netty.handler.codec.http.cookie.Cookie; import io.netty.handler.codec.http.cookie.ServerCookieDecoder; import io.netty.handler.codec.http.cookie.ServerCookieEncoder; -public class Netty4HttpRequest extends AbstractNetty4HttpRequest implements HttpRequest { +public class Netty4HttpRequest implements org.opensearch.http.HttpRequest { - private final FullHttpRequest request; + private final HttpRequest request; private final BytesReference content; + private final HttpHeadersMap headers; private final AtomicBoolean released; + private final Exception inboundException; private final boolean pooled; - Netty4HttpRequest(FullHttpRequest request) { + public Netty4HttpRequest(HttpRequest request) { this( request, new HttpHeadersMap(request.headers()), new AtomicBoolean(false), true, - Netty4Utils.toBytesReference(request.content()) + (request instanceof FullHttpRequest) ? Netty4Utils.toBytesReference(((FullHttpRequest) request).content()) : BytesArray.EMPTY ); } - Netty4HttpRequest(FullHttpRequest request, Exception inboundException) { + Netty4HttpRequest(HttpRequest request, Exception inboundException) { this( request, new HttpHeadersMap(request.headers()), new AtomicBoolean(false), true, - Netty4Utils.toBytesReference(request.content()), + (request instanceof FullHttpRequest) ? Netty4Utils.toBytesReference(((FullHttpRequest) request).content()) : BytesArray.EMPTY, inboundException ); } - private Netty4HttpRequest( - FullHttpRequest request, - HttpHeadersMap headers, - AtomicBoolean released, - boolean pooled, - BytesReference content - ) { + private Netty4HttpRequest(HttpRequest request, HttpHeadersMap headers, AtomicBoolean released, boolean pooled, BytesReference content) { this(request, headers, released, pooled, content, null); } private Netty4HttpRequest( - FullHttpRequest request, + HttpRequest request, HttpHeadersMap headers, AtomicBoolean released, boolean pooled, @@ -111,7 +112,36 @@ private Netty4HttpRequest( @Override public RestRequest.Method method() { - return getHttpMethod(request); + HttpMethod httpMethod = request.method(); + if (httpMethod == HttpMethod.GET) return RestRequest.Method.GET; + + if (httpMethod == HttpMethod.POST) return RestRequest.Method.POST; + + if (httpMethod == HttpMethod.PUT) return RestRequest.Method.PUT; + + if (httpMethod == HttpMethod.DELETE) return RestRequest.Method.DELETE; + + if (httpMethod == HttpMethod.HEAD) { + return RestRequest.Method.HEAD; + } + + if (httpMethod == HttpMethod.OPTIONS) { + return RestRequest.Method.OPTIONS; + } + + if (httpMethod == HttpMethod.PATCH) { + return RestRequest.Method.PATCH; + } + + if (httpMethod == HttpMethod.TRACE) { + return RestRequest.Method.TRACE; + } + + if (httpMethod == HttpMethod.CONNECT) { + return RestRequest.Method.CONNECT; + } + + throw new IllegalArgumentException("Unexpected http method: " + httpMethod); } @Override @@ -127,27 +157,32 @@ public BytesReference content() { @Override public void release() { + assert request instanceof FullHttpRequest : "release can only be called when underlying request object is of type FullHttpRequest"; if (pooled && released.compareAndSet(false, true)) { - request.release(); + FullHttpRequest req = (FullHttpRequest) request; + req.release(); } } @Override - public HttpRequest releaseAndCopy() { + public org.opensearch.http.HttpRequest releaseAndCopy() { + assert request instanceof FullHttpRequest + : "releaseAndCopy can only be called when underlying request object is of type FullHttpRequest"; assert released.get() == false; if (pooled == false) { return this; } + FullHttpRequest req = (FullHttpRequest) request; try { - final ByteBuf copiedContent = Unpooled.copiedBuffer(request.content()); + final ByteBuf copiedContent = Unpooled.copiedBuffer(req.content()); return new Netty4HttpRequest( new DefaultFullHttpRequest( - request.protocolVersion(), - request.method(), - request.uri(), + req.protocolVersion(), + req.method(), + req.uri(), copiedContent, - request.headers(), - request.trailingHeaders() + req.headers(), + req.trailingHeaders() ), headers, new AtomicBoolean(false), @@ -179,27 +214,29 @@ public List strictCookies() { @Override public HttpVersion protocolVersion() { if (request.protocolVersion().equals(io.netty.handler.codec.http.HttpVersion.HTTP_1_0)) { - return HttpRequest.HttpVersion.HTTP_1_0; + return org.opensearch.http.HttpRequest.HttpVersion.HTTP_1_0; } else if (request.protocolVersion().equals(io.netty.handler.codec.http.HttpVersion.HTTP_1_1)) { - return HttpRequest.HttpVersion.HTTP_1_1; + return org.opensearch.http.HttpRequest.HttpVersion.HTTP_1_1; } else { throw new IllegalArgumentException("Unexpected http protocol version: " + request.protocolVersion()); } } @Override - public HttpRequest removeHeader(String header) { + public org.opensearch.http.HttpRequest removeHeader(String header) { HttpHeaders headersWithoutContentTypeHeader = new DefaultHttpHeaders(); headersWithoutContentTypeHeader.add(request.headers()); headersWithoutContentTypeHeader.remove(header); HttpHeaders trailingHeaders = new DefaultHttpHeaders(); - trailingHeaders.add(request.trailingHeaders()); - trailingHeaders.remove(header); + if (request instanceof FullHttpRequest) { + trailingHeaders.add(((FullHttpRequest) request).trailingHeaders()); + trailingHeaders.remove(header); + } FullHttpRequest requestWithoutHeader = new DefaultFullHttpRequest( request.protocolVersion(), request.method(), request.uri(), - request.content(), + (request instanceof FullHttpRequest) ? ((FullHttpRequest) request).content() : Unpooled.EMPTY_BUFFER, headersWithoutContentTypeHeader, trailingHeaders ); @@ -216,7 +253,87 @@ public Exception getInboundException() { return inboundException; } - public FullHttpRequest nettyRequest() { + public HttpRequest nettyRequest() { return request; } + + /** + * A wrapper of {@link HttpHeaders} that implements a map to prevent copying unnecessarily. This class does not support modifications + * and due to the underlying implementation, it performs case insensitive lookups of key to values. + * + * It is important to note that this implementation does have some downsides in that each invocation of the + * {@link #values()} and {@link #entrySet()} methods will perform a copy of the values in the HttpHeaders rather than returning a + * view of the underlying values. + */ + private static class HttpHeadersMap implements Map> { + + private final HttpHeaders httpHeaders; + + private HttpHeadersMap(HttpHeaders httpHeaders) { + this.httpHeaders = httpHeaders; + } + + @Override + public int size() { + return httpHeaders.size(); + } + + @Override + public boolean isEmpty() { + return httpHeaders.isEmpty(); + } + + @Override + public boolean containsKey(Object key) { + return key instanceof String && httpHeaders.contains((String) key); + } + + @Override + public boolean containsValue(Object value) { + return value instanceof List && httpHeaders.names().stream().map(httpHeaders::getAll).anyMatch(value::equals); + } + + @Override + public List get(Object key) { + return key instanceof String ? httpHeaders.getAll((String) key) : null; + } + + @Override + public List put(String key, List value) { + throw new UnsupportedOperationException("modifications are not supported"); + } + + @Override + public List remove(Object key) { + throw new UnsupportedOperationException("modifications are not supported"); + } + + @Override + public void putAll(Map> m) { + throw new UnsupportedOperationException("modifications are not supported"); + } + + @Override + public void clear() { + throw new UnsupportedOperationException("modifications are not supported"); + } + + @Override + public Set keySet() { + return httpHeaders.names(); + } + + @Override + public Collection> values() { + return httpHeaders.names().stream().map(k -> Collections.unmodifiableList(httpHeaders.getAll(k))).collect(Collectors.toList()); + } + + @Override + public Set>> entrySet() { + return httpHeaders.names() + .stream() + .map(k -> new AbstractMap.SimpleImmutableEntry<>(k, httpHeaders.getAll(k))) + .collect(Collectors.toSet()); + } + } } diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpResponse.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpResponse.java index 31a8ccef0aa76..83284230be049 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpResponse.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpResponse.java @@ -46,7 +46,7 @@ public class Netty4HttpResponse extends DefaultFullHttpResponse implements HttpR private final HttpHeaders requestHeaders; - public Netty4HttpResponse(HttpHeaders requestHeaders, HttpVersion version, RestStatus status, BytesReference content) { + Netty4HttpResponse(HttpHeaders requestHeaders, HttpVersion version, RestStatus status, BytesReference content) { super(version, HttpResponseStatus.valueOf(status.getStatus()), Netty4Utils.toByteBuf(content)); this.requestHeaders = requestHeaders; } diff --git a/modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4DefaultHttpRequestTests.java b/modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4DefaultHttpRequestTests.java deleted file mode 100644 index e9df6ab376c79..0000000000000 --- a/modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4DefaultHttpRequestTests.java +++ /dev/null @@ -1,74 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.http.netty4; - -import org.opensearch.core.common.bytes.BytesArray; -import org.opensearch.core.common.bytes.BytesReference; -import org.opensearch.test.OpenSearchTestCase; - -import java.nio.charset.StandardCharsets; -import java.util.List; -import java.util.stream.Collectors; - -import io.netty.buffer.ByteBuf; -import io.netty.buffer.Unpooled; -import io.netty.handler.codec.http.DefaultFullHttpRequest; -import io.netty.handler.codec.http.DefaultHttpHeaders; -import io.netty.handler.codec.http.DefaultHttpRequest; -import io.netty.handler.codec.http.FullHttpRequest; -import io.netty.handler.codec.http.HttpHeaders; -import io.netty.handler.codec.http.HttpMethod; -import io.netty.handler.codec.http.HttpVersion; - -public class Netty4DefaultHttpRequestTests extends OpenSearchTestCase { - public void testDefaultHttpRequestHasEmptyContent() throws Exception { - HttpHeaders testHeaders = new DefaultHttpHeaders(); - testHeaders.add("test-header", "test-value"); - DefaultHttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "https://localhost:9200"); - Netty4DefaultHttpRequest netty4Request = new Netty4DefaultHttpRequest(request); - - assertEquals("Expected content to be empty", BytesArray.EMPTY, netty4Request.content()); - } - - public void testDefaultHttpRequestAndFullHttpRequestHaveSameStructure() throws Exception { - HttpHeaders testHeaders = new DefaultHttpHeaders(); - testHeaders.add("test-header", "test-value"); - DefaultHttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "https://localhost:9200", testHeaders); - Netty4DefaultHttpRequest netty4Request = new Netty4DefaultHttpRequest(request); - - ByteBuf expectedByteBuf = Unpooled.copiedBuffer("Hello, World!".getBytes(StandardCharsets.UTF_8)); - BytesReference expectedBytesArray = new BytesArray("Hello, World!".getBytes(StandardCharsets.UTF_8)); - - HttpHeaders trailingHeaders = new DefaultHttpHeaders(); - FullHttpRequest fullRequest = new DefaultFullHttpRequest( - HttpVersion.HTTP_1_1, - HttpMethod.GET, - "https://localhost:9200", - expectedByteBuf, - testHeaders, - trailingHeaders - ); - Netty4HttpRequest fullNetty4Request = new Netty4HttpRequest(fullRequest); - - assertEquals("Expected methods to be equal", netty4Request.method(), fullNetty4Request.method()); - assertEquals("Expected URIs to be equal", netty4Request.uri(), fullNetty4Request.uri()); - assertEquals("Expected Protocol Versions to be equal", netty4Request.protocolVersion(), fullNetty4Request.protocolVersion()); - - assertEquals("Expected headers to be of equal length", netty4Request.headers.size(), fullNetty4Request.headers.size()); - for (String header : netty4Request.headers.keySet()) { - assertTrue(fullNetty4Request.headers.containsKey(header)); - List headerValue = netty4Request.headers.get(header).stream().sorted().collect(Collectors.toList()); - List otherValue = fullNetty4Request.headers.get(header).stream().sorted().collect(Collectors.toList()); - assertEquals(headerValue, otherValue); - } - - assertEquals("Expected content to be empty", BytesArray.EMPTY, netty4Request.content()); - assertEquals("Expected content to not be empty", expectedBytesArray, fullNetty4Request.content()); - } -} From 5af481b1061ea01d3b03f7b3bc09d41e502204e2 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 5 Oct 2023 13:11:58 -0400 Subject: [PATCH 24/26] Revert access modifier changes Signed-off-by: Craig Perkins --- .../org/opensearch/http/netty4/Netty4HeaderVerifierIT.java | 1 + .../{http/netty4 => transport}/Netty4BlockingPlugin.java | 3 ++- .../java/org/opensearch/transport/Netty4ModulePlugin.java | 2 +- .../java/org/opensearch/http/AbstractHttpServerTransport.java | 4 ++-- 4 files changed, 6 insertions(+), 4 deletions(-) rename modules/transport-netty4/src/internalClusterTest/java/org/opensearch/{http/netty4 => transport}/Netty4BlockingPlugin.java (97%) diff --git a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java index 94e7a3240454b..ed715fd918077 100644 --- a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java +++ b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java @@ -29,6 +29,7 @@ import io.netty.handler.codec.http.HttpVersion; import io.netty.handler.codec.http2.HttpConversionUtil; import io.netty.util.ReferenceCounted; +import org.opensearch.transport.Netty4BlockingPlugin; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.equalTo; diff --git a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/transport/Netty4BlockingPlugin.java similarity index 97% rename from modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java rename to modules/transport-netty4/src/internalClusterTest/java/org/opensearch/transport/Netty4BlockingPlugin.java index d1bc1d5fd0fa8..a8ed7414c9bc3 100644 --- a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4BlockingPlugin.java +++ b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/transport/Netty4BlockingPlugin.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.http.netty4; +package org.opensearch.transport; import org.opensearch.common.network.NetworkService; import org.opensearch.common.settings.ClusterSettings; @@ -16,6 +16,7 @@ import org.opensearch.core.indices.breaker.CircuitBreakerService; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.http.HttpServerTransport; +import org.opensearch.http.netty4.Netty4HttpServerTransport; import org.opensearch.telemetry.tracing.Tracer; import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.Netty4ModulePlugin; diff --git a/modules/transport-netty4/src/main/java/org/opensearch/transport/Netty4ModulePlugin.java b/modules/transport-netty4/src/main/java/org/opensearch/transport/Netty4ModulePlugin.java index fed4f7575b1e8..2bc795d11ed5d 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/transport/Netty4ModulePlugin.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/transport/Netty4ModulePlugin.java @@ -144,7 +144,7 @@ public Map> getHttpTransports( ); } - protected SharedGroupFactory getSharedGroupFactory(Settings settings) { + SharedGroupFactory getSharedGroupFactory(Settings settings) { SharedGroupFactory groupFactory = this.groupFactory.get(); if (groupFactory != null) { assert groupFactory.getSettings().equals(settings) : "Different settings than originally provided"; diff --git a/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java b/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java index 7e847d04d30e3..398810d1ecccb 100644 --- a/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java +++ b/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java @@ -99,7 +99,7 @@ public abstract class AbstractHttpServerTransport extends AbstractLifecycleCompo protected final ThreadPool threadPool; protected final Dispatcher dispatcher; protected final CorsHandler corsHandler; - protected final NamedXContentRegistry xContentRegistry; + private final NamedXContentRegistry xContentRegistry; protected final PortsRange port; protected final ByteSizeValue maxContentLength; @@ -112,7 +112,7 @@ public abstract class AbstractHttpServerTransport extends AbstractLifecycleCompo private final Set httpServerChannels = Collections.newSetFromMap(new ConcurrentHashMap<>()); private final HttpTracer httpTracer; - protected final Tracer tracer; + private final Tracer tracer; protected AbstractHttpServerTransport( Settings settings, From 91fc5bc067857a8d521237b3c5e173e835c78d93 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 5 Oct 2023 13:25:35 -0400 Subject: [PATCH 25/26] Spotless Signed-off-by: Craig Perkins --- .../java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java | 2 +- .../java/org/opensearch/transport/Netty4BlockingPlugin.java | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java index ed715fd918077..c39567a005fd1 100644 --- a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java +++ b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4HeaderVerifierIT.java @@ -14,6 +14,7 @@ import org.opensearch.plugins.Plugin; import org.opensearch.test.OpenSearchIntegTestCase.ClusterScope; import org.opensearch.test.OpenSearchIntegTestCase.Scope; +import org.opensearch.transport.Netty4BlockingPlugin; import java.nio.charset.StandardCharsets; import java.util.ArrayList; @@ -29,7 +30,6 @@ import io.netty.handler.codec.http.HttpVersion; import io.netty.handler.codec.http2.HttpConversionUtil; import io.netty.util.ReferenceCounted; -import org.opensearch.transport.Netty4BlockingPlugin; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.equalTo; diff --git a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/transport/Netty4BlockingPlugin.java b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/transport/Netty4BlockingPlugin.java index a8ed7414c9bc3..d5fe49952add3 100644 --- a/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/transport/Netty4BlockingPlugin.java +++ b/modules/transport-netty4/src/internalClusterTest/java/org/opensearch/transport/Netty4BlockingPlugin.java @@ -19,8 +19,6 @@ import org.opensearch.http.netty4.Netty4HttpServerTransport; import org.opensearch.telemetry.tracing.Tracer; import org.opensearch.threadpool.ThreadPool; -import org.opensearch.transport.Netty4ModulePlugin; -import org.opensearch.transport.SharedGroupFactory; import java.nio.charset.StandardCharsets; import java.util.Collections; From ddaca29d5926ca63609dc2f7dfeba94ed700d25c Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 5 Oct 2023 16:27:47 -0400 Subject: [PATCH 26/26] Remove createRestRequest changes in favor of new security rest channel in security plugin Signed-off-by: Craig Perkins --- .../http/netty4/Netty4HttpRequest.java | 62 ++++++++-------- .../http/AbstractHttpServerTransport.java | 74 ++----------------- .../org/opensearch/rest/RestController.java | 2 +- .../java/org/opensearch/rest/RestRequest.java | 61 +-------------- .../AbstractHttpServerTransportTests.java | 16 ---- 5 files changed, 39 insertions(+), 176 deletions(-) diff --git a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java index 51a96b180c2c9..7d937157c1034 100644 --- a/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java +++ b/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java @@ -32,9 +32,9 @@ package org.opensearch.http.netty4; -import org.opensearch.core.common.bytes.BytesArray; import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.rest.RestStatus; +import org.opensearch.http.HttpRequest; import org.opensearch.rest.RestRequest; import org.opensearch.transport.netty4.Netty4Utils; @@ -55,47 +55,52 @@ import io.netty.handler.codec.http.HttpHeaderNames; import io.netty.handler.codec.http.HttpHeaders; import io.netty.handler.codec.http.HttpMethod; -import io.netty.handler.codec.http.HttpRequest; import io.netty.handler.codec.http.cookie.Cookie; import io.netty.handler.codec.http.cookie.ServerCookieDecoder; import io.netty.handler.codec.http.cookie.ServerCookieEncoder; -public class Netty4HttpRequest implements org.opensearch.http.HttpRequest { +public class Netty4HttpRequest implements HttpRequest { - private final HttpRequest request; + private final FullHttpRequest request; private final BytesReference content; private final HttpHeadersMap headers; private final AtomicBoolean released; private final Exception inboundException; private final boolean pooled; - public Netty4HttpRequest(HttpRequest request) { + Netty4HttpRequest(FullHttpRequest request) { this( request, new HttpHeadersMap(request.headers()), new AtomicBoolean(false), true, - (request instanceof FullHttpRequest) ? Netty4Utils.toBytesReference(((FullHttpRequest) request).content()) : BytesArray.EMPTY + Netty4Utils.toBytesReference(request.content()) ); } - Netty4HttpRequest(HttpRequest request, Exception inboundException) { + Netty4HttpRequest(FullHttpRequest request, Exception inboundException) { this( request, new HttpHeadersMap(request.headers()), new AtomicBoolean(false), true, - (request instanceof FullHttpRequest) ? Netty4Utils.toBytesReference(((FullHttpRequest) request).content()) : BytesArray.EMPTY, + Netty4Utils.toBytesReference(request.content()), inboundException ); } - private Netty4HttpRequest(HttpRequest request, HttpHeadersMap headers, AtomicBoolean released, boolean pooled, BytesReference content) { + private Netty4HttpRequest( + FullHttpRequest request, + HttpHeadersMap headers, + AtomicBoolean released, + boolean pooled, + BytesReference content + ) { this(request, headers, released, pooled, content, null); } private Netty4HttpRequest( - HttpRequest request, + FullHttpRequest request, HttpHeadersMap headers, AtomicBoolean released, boolean pooled, @@ -157,32 +162,27 @@ public BytesReference content() { @Override public void release() { - assert request instanceof FullHttpRequest : "release can only be called when underlying request object is of type FullHttpRequest"; if (pooled && released.compareAndSet(false, true)) { - FullHttpRequest req = (FullHttpRequest) request; - req.release(); + request.release(); } } @Override - public org.opensearch.http.HttpRequest releaseAndCopy() { - assert request instanceof FullHttpRequest - : "releaseAndCopy can only be called when underlying request object is of type FullHttpRequest"; + public HttpRequest releaseAndCopy() { assert released.get() == false; if (pooled == false) { return this; } - FullHttpRequest req = (FullHttpRequest) request; try { - final ByteBuf copiedContent = Unpooled.copiedBuffer(req.content()); + final ByteBuf copiedContent = Unpooled.copiedBuffer(request.content()); return new Netty4HttpRequest( new DefaultFullHttpRequest( - req.protocolVersion(), - req.method(), - req.uri(), + request.protocolVersion(), + request.method(), + request.uri(), copiedContent, - req.headers(), - req.trailingHeaders() + request.headers(), + request.trailingHeaders() ), headers, new AtomicBoolean(false), @@ -214,29 +214,27 @@ public List strictCookies() { @Override public HttpVersion protocolVersion() { if (request.protocolVersion().equals(io.netty.handler.codec.http.HttpVersion.HTTP_1_0)) { - return org.opensearch.http.HttpRequest.HttpVersion.HTTP_1_0; + return HttpRequest.HttpVersion.HTTP_1_0; } else if (request.protocolVersion().equals(io.netty.handler.codec.http.HttpVersion.HTTP_1_1)) { - return org.opensearch.http.HttpRequest.HttpVersion.HTTP_1_1; + return HttpRequest.HttpVersion.HTTP_1_1; } else { throw new IllegalArgumentException("Unexpected http protocol version: " + request.protocolVersion()); } } @Override - public org.opensearch.http.HttpRequest removeHeader(String header) { + public HttpRequest removeHeader(String header) { HttpHeaders headersWithoutContentTypeHeader = new DefaultHttpHeaders(); headersWithoutContentTypeHeader.add(request.headers()); headersWithoutContentTypeHeader.remove(header); HttpHeaders trailingHeaders = new DefaultHttpHeaders(); - if (request instanceof FullHttpRequest) { - trailingHeaders.add(((FullHttpRequest) request).trailingHeaders()); - trailingHeaders.remove(header); - } + trailingHeaders.add(request.trailingHeaders()); + trailingHeaders.remove(header); FullHttpRequest requestWithoutHeader = new DefaultFullHttpRequest( request.protocolVersion(), request.method(), request.uri(), - (request instanceof FullHttpRequest) ? ((FullHttpRequest) request).content() : Unpooled.EMPTY_BUFFER, + request.content(), headersWithoutContentTypeHeader, trailingHeaders ); @@ -253,7 +251,7 @@ public Exception getInboundException() { return inboundException; } - public HttpRequest nettyRequest() { + public FullHttpRequest nettyRequest() { return request; } diff --git a/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java b/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java index 398810d1ecccb..b8f8abb6c2c23 100644 --- a/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java +++ b/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java @@ -412,13 +412,13 @@ private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChan { RestRequest innerRestRequest; try { - innerRestRequest = RestRequest.request(xContentRegistry, httpRequest, httpChannel, true); + innerRestRequest = RestRequest.request(xContentRegistry, httpRequest, httpChannel); } catch (final RestRequest.ContentTypeHeaderException e) { badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); - innerRestRequest = requestWithoutContentTypeHeader(httpRequest, httpChannel, badRequestCause, true); + innerRestRequest = requestWithoutContentTypeHeader(httpRequest, httpChannel, badRequestCause); } catch (final RestRequest.BadParameterException e) { badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); - innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel, true); + innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel); } restRequest = innerRestRequest; } @@ -466,75 +466,13 @@ private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChan dispatchRequest(restRequest, channel, badRequestCause); } - public static RestRequest createRestRequest( - final NamedXContentRegistry xContentRegistry, - final HttpRequest httpRequest, - final HttpChannel httpChannel - ) { - Exception badRequestCause = httpRequest.getInboundException(); - - /* - * We want to create a REST request from the incoming request from Netty. However, creating this request could fail if there - * are incorrectly encoded parameters, or the Content-Type header is invalid. If one of these specific failures occurs, we - * attempt to create a REST request again without the input that caused the exception (e.g., we remove the Content-Type header, - * or skip decoding the parameters). Once we have a request in hand, we then dispatch the request as a bad request with the - * underlying exception that caused us to treat the request as bad. - */ - final RestRequest restRequest; - { - RestRequest innerRestRequest; - try { - innerRestRequest = RestRequest.request(xContentRegistry, httpRequest, httpChannel, false); - } catch (final RestRequest.ContentTypeHeaderException e) { - badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); - innerRestRequest = requestWithoutContentTypeHeader(xContentRegistry, httpRequest, httpChannel, badRequestCause, false); - } catch (final RestRequest.BadParameterException e) { - badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e); - innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel, false); - } - restRequest = innerRestRequest; - } - return restRequest; - } - - private static RestRequest requestWithoutContentTypeHeader( - NamedXContentRegistry xContentRegistry, - HttpRequest httpRequest, - HttpChannel httpChannel, - Exception badRequestCause, - boolean shouldGenerateRequestId - ) { + private RestRequest requestWithoutContentTypeHeader(HttpRequest httpRequest, HttpChannel httpChannel, Exception badRequestCause) { HttpRequest httpRequestWithoutContentType = httpRequest.removeHeader("Content-Type"); try { - return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel, shouldGenerateRequestId); + return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel); } catch (final RestRequest.BadParameterException e) { badRequestCause.addSuppressed(e); - return RestRequest.requestWithoutParameters( - xContentRegistry, - httpRequestWithoutContentType, - httpChannel, - shouldGenerateRequestId - ); - } - } - - private RestRequest requestWithoutContentTypeHeader( - HttpRequest httpRequest, - HttpChannel httpChannel, - Exception badRequestCause, - boolean shouldGenerateRequestId - ) { - HttpRequest httpRequestWithoutContentType = httpRequest.removeHeader("Content-Type"); - try { - return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel, shouldGenerateRequestId); - } catch (final RestRequest.BadParameterException e) { - badRequestCause.addSuppressed(e); - return RestRequest.requestWithoutParameters( - xContentRegistry, - httpRequestWithoutContentType, - httpChannel, - shouldGenerateRequestId - ); + return RestRequest.requestWithoutParameters(xContentRegistry, httpRequestWithoutContentType, httpChannel); } } diff --git a/server/src/main/java/org/opensearch/rest/RestController.java b/server/src/main/java/org/opensearch/rest/RestController.java index 92f31076a1489..ac30f999d0da7 100644 --- a/server/src/main/java/org/opensearch/rest/RestController.java +++ b/server/src/main/java/org/opensearch/rest/RestController.java @@ -131,7 +131,7 @@ public RestController( this.headersToCopy = headersToCopy; this.usageService = usageService; if (handlerWrapper == null) { - handlerWrapper = (h) -> h; + handlerWrapper = h -> h; // passthrough if no wrapper set } this.handlerWrapper = handlerWrapper; this.client = client; diff --git a/server/src/main/java/org/opensearch/rest/RestRequest.java b/server/src/main/java/org/opensearch/rest/RestRequest.java index 275341cd960b8..f64774686c89d 100644 --- a/server/src/main/java/org/opensearch/rest/RestRequest.java +++ b/server/src/main/java/org/opensearch/rest/RestRequest.java @@ -76,6 +76,7 @@ public class RestRequest implements ToXContent.Params { // tchar pattern as defined by RFC7230 section 3.2.6 private static final Pattern TCHAR_PATTERN = Pattern.compile("[a-zA-z0-9!#$%&'*+\\-.\\^_`|~]+"); + private static final AtomicLong requestIdGenerator = new AtomicLong(); private final NamedXContentRegistry xContentRegistry; @@ -151,7 +152,7 @@ protected RestRequest(RestRequest restRequest) { * with an unpooled copy. This is supposed to be used before passing requests to {@link RestHandler} instances that can not safely * handle http requests that use pooled buffers as determined by {@link RestHandler#allowsUnsafeBuffers()}. */ - protected void ensureSafeBuffers() { + void ensureSafeBuffers() { httpRequest = httpRequest.releaseAndCopy(); } @@ -179,36 +180,6 @@ public static RestRequest request(NamedXContentRegistry xContentRegistry, HttpRe ); } - /** - * Creates a new REST request. This method will throw {@link BadParameterException} if the path cannot be - * decoded - * - * @param xContentRegistry the content registry - * @param httpRequest the http request - * @param httpChannel the http channel - * @param shouldGenerateRequestId should generate a new request id - * @throws BadParameterException if the parameters can not be decoded - * @throws ContentTypeHeaderException if the Content-Type header can not be parsed - */ - public static RestRequest request( - NamedXContentRegistry xContentRegistry, - HttpRequest httpRequest, - HttpChannel httpChannel, - boolean shouldGenerateRequestId - ) { - Map params = params(httpRequest.uri()); - String path = path(httpRequest.uri()); - return new RestRequest( - xContentRegistry, - params, - path, - httpRequest.getHeaders(), - httpRequest, - httpChannel, - shouldGenerateRequestId ? requestIdGenerator.incrementAndGet() : -1 - ); - } - private static Map params(final String uri) { final Map params = new HashMap<>(); int index = uri.indexOf('?'); @@ -257,34 +228,6 @@ public static RestRequest requestWithoutParameters( ); } - /** - * Creates a new REST request. The path is not decoded so this constructor will not throw a - * {@link BadParameterException}. - * - * @param xContentRegistry the content registry - * @param httpRequest the http request - * @param httpChannel the http channel - * @param shouldGenerateRequestId should generate new request id - * @throws ContentTypeHeaderException if the Content-Type header can not be parsed - */ - public static RestRequest requestWithoutParameters( - NamedXContentRegistry xContentRegistry, - HttpRequest httpRequest, - HttpChannel httpChannel, - boolean shouldGenerateRequestId - ) { - Map params = Collections.emptyMap(); - return new RestRequest( - xContentRegistry, - params, - httpRequest.uri(), - httpRequest.getHeaders(), - httpRequest, - httpChannel, - shouldGenerateRequestId ? requestIdGenerator.incrementAndGet() : -1 - ); - } - /** * The method used. * diff --git a/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java b/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java index 7dcea1c206ac3..c34f13041cb11 100644 --- a/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java +++ b/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java @@ -64,7 +64,6 @@ import java.net.InetSocketAddress; import java.net.UnknownHostException; -import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -150,21 +149,6 @@ public void testHttpPublishPort() throws Exception { } } - public void testCreateRestRequestDoesNotGenerateRequestID() { - FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent( - new BytesArray("bar".getBytes(StandardCharsets.UTF_8)), - null - ).withPath("/foo").withHeaders(Collections.singletonMap("Content-Type", Collections.singletonList("text/plain"))).build(); - - RestRequest request = AbstractHttpServerTransport.createRestRequest( - xContentRegistry(), - fakeRestRequest.getHttpRequest(), - fakeRestRequest.getHttpChannel() - ); - - assertEquals("request should not generate id", -1, request.getRequestId()); - } - public void testDispatchDoesNotModifyThreadContext() { final HttpServerTransport.Dispatcher dispatcher = new HttpServerTransport.Dispatcher() {