From 5449484bf048b025680c309f4987e2a8c2466929 Mon Sep 17 00:00:00 2001 From: kvosper Date: Mon, 17 Sep 2018 10:09:53 +0100 Subject: [PATCH] Remove client address (#280) * Remove clientAddress and duplicate HttpInterceptorContext classes --- .../com/hotels/styx/api/FullHttpRequest.java | 22 +------ .../com/hotels/styx/api/HttpInterceptor.java | 8 +++ .../java/com/hotels/styx/api/HttpRequest.java | 34 ---------- .../com/hotels/styx/api/HttpRequestTest.java | 31 ++------- .../service/spi/AbstractStyxServiceTest.java | 3 +- .../service/spi}/MockContext.java | 15 ++++- .../styx/server/HttpInterceptorContext.java | 54 ++++++++++++--- .../proxy/HttpErrorStatusCauseLogger.java | 6 +- .../styx/proxy/HttpErrorStatusMetrics.java | 4 +- .../RequestEnrichingInterceptor.java | 46 +++++++++---- .../proxy/HttpErrorStatusCauseLoggerTest.java | 10 +-- .../proxy/HttpErrorStatusMetricsTest.java | 3 +- ...urationContextResolverInterceptorTest.java | 29 ++------ .../HttpMessageLoggingInterceptorTest.java | 4 +- .../RequestEnrichingInterceptorTest.java | 66 ++++++++++++------- .../handlers/StandardHttpPipelineTest.java | 7 +- .../handlers/ConditionRouterConfigSpec.scala | 15 +++-- .../CompositeHttpErrorStatusListener.java | 5 +- .../styx/server/HttpErrorStatusListener.java | 6 +- .../codec/NettyToStyxRequestDecoder.java | 5 +- .../netty/connectors/HttpPipelineHandler.java | 41 +++++------- .../CompositeHttpErrorStatusListenerTest.java | 10 +-- .../handlers/StaticFileHandlerTest.java | 41 ++++++------ .../connectors/HttpPipelineHandlerTest.java | 27 ++++---- .../styx/server/routing/AntlrMatcherTest.java | 6 +- support/api-testsupport/pom.xml | 11 ++-- .../styx/server/HttpInterceptorContext.java | 60 ----------------- .../styx/testapi/HttpInterceptorContext.java | 58 ---------------- 28 files changed, 253 insertions(+), 374 deletions(-) rename components/api/src/test/java/com/hotels/styx/api/{ => extension/service/spi}/MockContext.java (67%) delete mode 100644 support/api-testsupport/src/main/java/com/hotels/styx/server/HttpInterceptorContext.java delete mode 100644 support/test-api/src/main/java/com/hotels/styx/testapi/HttpInterceptorContext.java diff --git a/components/api/src/main/java/com/hotels/styx/api/FullHttpRequest.java b/components/api/src/main/java/com/hotels/styx/api/FullHttpRequest.java index 73bf68c301..e99004d429 100644 --- a/components/api/src/main/java/com/hotels/styx/api/FullHttpRequest.java +++ b/components/api/src/main/java/com/hotels/styx/api/FullHttpRequest.java @@ -19,7 +19,6 @@ import io.netty.buffer.Unpooled; import rx.Observable; -import java.net.InetSocketAddress; import java.nio.charset.Charset; import java.util.Collection; import java.util.Collections; @@ -36,8 +35,6 @@ import static com.hotels.styx.api.HttpHeaderNames.COOKIE; import static com.hotels.styx.api.HttpHeaderNames.HOST; import static com.hotels.styx.api.HttpHeaderValues.KEEP_ALIVE; -import static com.hotels.styx.api.RequestCookie.decode; -import static com.hotels.styx.api.RequestCookie.encode; import static com.hotels.styx.api.HttpMethod.DELETE; import static com.hotels.styx.api.HttpMethod.GET; import static com.hotels.styx.api.HttpMethod.HEAD; @@ -46,8 +43,9 @@ import static com.hotels.styx.api.HttpMethod.POST; import static com.hotels.styx.api.HttpMethod.PUT; import static com.hotels.styx.api.HttpVersion.HTTP_1_1; +import static com.hotels.styx.api.RequestCookie.decode; +import static com.hotels.styx.api.RequestCookie.encode; import static java.lang.Integer.parseInt; -import static java.net.InetSocketAddress.createUnresolved; import static java.util.Arrays.asList; import static java.util.Objects.requireNonNull; import static java.util.UUID.randomUUID; @@ -82,8 +80,6 @@ */ public class FullHttpRequest implements FullHttpMessage { private final Object id; - // Relic of old API, kept for conversions - private final InetSocketAddress clientAddress; private final HttpVersion version; private final HttpMethod method; private final Url url; @@ -92,7 +88,6 @@ public class FullHttpRequest implements FullHttpMessage { FullHttpRequest(Builder builder) { this.id = builder.id == null ? randomUUID() : builder.id; - this.clientAddress = builder.clientAddress; this.version = builder.version; this.method = builder.method; this.url = builder.url; @@ -258,11 +253,6 @@ public boolean keepAlive() { } - // Relic of old API, kept only for conversions - InetSocketAddress clientAddress() { - return this.clientAddress; - } - /** * Get a query parameter by name if present. * @@ -322,8 +312,7 @@ public Builder newBuilder() { */ public HttpRequest toStreamingRequest() { HttpRequest.Builder streamingBuilder = new HttpRequest.Builder(this) - .disableValidation() - .clientAddress(clientAddress); + .disableValidation(); if (this.body.length == 0) { return streamingBuilder.body(new StyxCoreObservable<>(Observable.empty())).build(); @@ -370,11 +359,8 @@ public String toString() { * Builder. */ public static final class Builder { - private static final InetSocketAddress LOCAL_HOST = createUnresolved("127.0.0.1", 0); - private Object id; private HttpMethod method = HttpMethod.GET; - private InetSocketAddress clientAddress = LOCAL_HOST; private boolean validate = true; private Url url; private HttpHeaders.Builder headers; @@ -411,7 +397,6 @@ public Builder(HttpMethod method, String uri) { public Builder(HttpRequest request, byte[] body) { this.id = request.id(); this.method = request.method(); - this.clientAddress = request.clientAddress(); this.url = request.url(); this.version = request.version(); this.headers = request.headers().newBuilder(); @@ -421,7 +406,6 @@ public Builder(HttpRequest request, byte[] body) { Builder(FullHttpRequest request) { this.id = request.id(); this.method = request.method(); - this.clientAddress = request.clientAddress; this.url = request.url(); this.version = request.version(); this.headers = request.headers().newBuilder(); diff --git a/components/api/src/main/java/com/hotels/styx/api/HttpInterceptor.java b/components/api/src/main/java/com/hotels/styx/api/HttpInterceptor.java index e49d139bed..1186f894ee 100644 --- a/components/api/src/main/java/com/hotels/styx/api/HttpInterceptor.java +++ b/components/api/src/main/java/com/hotels/styx/api/HttpInterceptor.java @@ -15,6 +15,7 @@ */ package com.hotels.styx.api; +import java.net.InetSocketAddress; import java.util.Optional; /** @@ -58,6 +59,13 @@ default Optional getIfAvailable(String key, Class clazz) { * @return returns true if this request was received over a secure connection */ boolean isSecure(); + + /** + * Provides address of the client that sent the request to Styx. + * + * @return address of the client that sent the request to Styx + */ + Optional clientAddress(); } /** diff --git a/components/api/src/main/java/com/hotels/styx/api/HttpRequest.java b/components/api/src/main/java/com/hotels/styx/api/HttpRequest.java index c6aade7fda..00d3a047c5 100644 --- a/components/api/src/main/java/com/hotels/styx/api/HttpRequest.java +++ b/components/api/src/main/java/com/hotels/styx/api/HttpRequest.java @@ -20,7 +20,6 @@ import io.netty.buffer.CompositeByteBuf; import rx.Observable; -import java.net.InetSocketAddress; import java.util.Collection; import java.util.Collections; import java.util.List; @@ -55,7 +54,6 @@ import static io.netty.util.ReferenceCountUtil.release; import static java.lang.Integer.parseInt; import static java.lang.String.format; -import static java.net.InetSocketAddress.createUnresolved; import static java.util.Arrays.asList; import static java.util.Objects.requireNonNull; import static java.util.UUID.randomUUID; @@ -101,8 +99,6 @@ */ public class HttpRequest implements StreamingHttpMessage { private final Object id; - // Relic of old API, kept for conversions - private final InetSocketAddress clientAddress; private final HttpVersion version; private final HttpMethod method; private final Url url; @@ -111,7 +107,6 @@ public class HttpRequest implements StreamingHttpMessage { HttpRequest(Builder builder) { this.id = builder.id == null ? randomUUID() : builder.id; - this.clientAddress = builder.clientAddress; this.version = builder.version; this.method = builder.method; this.url = builder.url; @@ -288,14 +283,6 @@ public boolean keepAlive() { return HttpMessageSupport.keepAlive(headers, version); } - /** - * @deprecated will be removed from Styx 1.0 api release - */ - @Deprecated - public InetSocketAddress clientAddress() { - return this.clientAddress; - } - /** * Get a query parameter by name if present. * @@ -429,7 +416,6 @@ public String toString() { .add("uri", url) .add("headers", headers) .add("id", id) - .add("clientAddress", clientAddress) .toString(); } @@ -437,11 +423,8 @@ public String toString() { * An HTTP request builder. */ public static final class Builder { - private static final InetSocketAddress LOCAL_HOST = createUnresolved("127.0.0.1", 0); - private Object id; private HttpMethod method = HttpMethod.GET; - private InetSocketAddress clientAddress = LOCAL_HOST; private boolean validate = true; private Url url; private HttpHeaders.Builder headers; @@ -478,7 +461,6 @@ public Builder(HttpMethod method, String uri) { public Builder(HttpRequest request, StyxObservable contentStream) { this.id = request.id(); this.method = httpMethod(request.method().name()); - this.clientAddress = request.clientAddress(); this.url = request.url(); this.version = httpVersion(request.version().toString()); this.headers = request.headers().newBuilder(); @@ -488,7 +470,6 @@ public Builder(HttpRequest request, StyxObservable contentStream) { Builder(HttpRequest request) { this.id = request.id(); this.method = request.method(); - this.clientAddress = request.clientAddress(); this.url = request.url(); this.version = request.version(); this.headers = request.headers().newBuilder(); @@ -498,7 +479,6 @@ public Builder(HttpRequest request, StyxObservable contentStream) { Builder(FullHttpRequest request) { this.id = request.id(); this.method = request.method(); - this.clientAddress = request.clientAddress(); this.url = request.url(); this.version = request.version(); this.headers = request.headers().newBuilder(); @@ -620,20 +600,6 @@ public Builder method(HttpMethod method) { return this; } - /** - * Do not use in any new code. - * - * @deprecated Will not appear in 1.0 API. - * - * @param clientAddress - * @return {@code this} - */ - @Deprecated - public Builder clientAddress(InetSocketAddress clientAddress) { - this.clientAddress = clientAddress; - return this; - } - /** * Enable validation of uri and some headers. * diff --git a/components/api/src/test/java/com/hotels/styx/api/HttpRequestTest.java b/components/api/src/test/java/com/hotels/styx/api/HttpRequestTest.java index e931e5a58e..18204b3c95 100644 --- a/components/api/src/test/java/com/hotels/styx/api/HttpRequestTest.java +++ b/components/api/src/test/java/com/hotels/styx/api/HttpRequestTest.java @@ -21,24 +21,23 @@ import org.testng.annotations.DataProvider; import org.testng.annotations.Test; -import java.net.InetSocketAddress; import java.util.concurrent.ExecutionException; import java.util.stream.Stream; import static com.hotels.styx.api.HttpHeader.header; import static com.hotels.styx.api.HttpHeaderNames.CONTENT_LENGTH; import static com.hotels.styx.api.HttpHeaderNames.HOST; +import static com.hotels.styx.api.HttpMethod.DELETE; +import static com.hotels.styx.api.HttpMethod.GET; +import static com.hotels.styx.api.HttpMethod.POST; import static com.hotels.styx.api.HttpRequest.get; import static com.hotels.styx.api.HttpRequest.patch; import static com.hotels.styx.api.HttpRequest.post; import static com.hotels.styx.api.HttpRequest.put; -import static com.hotels.styx.api.Url.Builder.url; -import static com.hotels.styx.api.RequestCookie.requestCookie; -import static com.hotels.styx.api.HttpMethod.DELETE; -import static com.hotels.styx.api.HttpMethod.GET; -import static com.hotels.styx.api.HttpMethod.POST; import static com.hotels.styx.api.HttpVersion.HTTP_1_0; import static com.hotels.styx.api.HttpVersion.HTTP_1_1; +import static com.hotels.styx.api.RequestCookie.requestCookie; +import static com.hotels.styx.api.Url.Builder.url; import static com.hotels.styx.support.matchers.IsOptional.isAbsent; import static com.hotels.styx.support.matchers.IsOptional.isValue; import static com.hotels.styx.support.matchers.MapMatcher.isMap; @@ -119,7 +118,6 @@ public void createsARequestWithDefaultValues() throws Exception { assertThat(request.version(), is(HTTP_1_1)); assertThat(request.url().toString(), is("/index")); assertThat(request.path(), is("/index")); - assertThat(request.clientAddress().getHostName(), is("127.0.0.1")); assertThat(request.id(), is(notNullValue())); assertThat(request.cookies(), is(emptyIterable())); assertThat(request.headers(), is(emptyIterable())); @@ -143,8 +141,7 @@ public void canUseBuilderToSetRequestProperties() { .cookies(requestCookie("cfoo", "bar")) .build(); - assertThat(request.toString(), is("HttpRequest{version=HTTP/1.0, method=PATCH, uri=https://hotels.com, headers=[headerName=a, Cookie=cfoo=bar, Host=hotels.com]," + - " id=id, clientAddress=127.0.0.1:0}")); + assertThat(request.toString(), is("HttpRequest{version=HTTP/1.0, method=PATCH, uri=https://hotels.com, headers=[headerName=a, Cookie=cfoo=bar, Host=hotels.com], id=id}")); assertThat(request.headers("headerName"), is(singletonList("a"))); } @@ -375,22 +372,6 @@ public void createsANewRequestWithSameVersionAsBefore() { assertThat(newRequest.version(), is(HTTP_1_0)); } - // Make tests to ensure conversion from HttpRequest and back again preserves clientAddress - do it for Fullhttprequest too - @Test - public void conversionPreservesClientAddress() throws Exception { - InetSocketAddress address = InetSocketAddress.createUnresolved("styx.io", 8080); - HttpRequest original = HttpRequest.post("/foo").clientAddress(address).build(); - - HttpRequest streaming = new HttpRequest.Builder(original).build(); - - HttpRequest shouldMatchOriginal = streaming.toFullRequest(0x100) - .asCompletableFuture() - .get() - .toStreamingRequest(); - - assertThat(shouldMatchOriginal.clientAddress(), is(address)); - } - @Test public void addsCookies() { HttpRequest request = HttpRequest.get("/") diff --git a/components/api/src/test/java/com/hotels/styx/api/extension/service/spi/AbstractStyxServiceTest.java b/components/api/src/test/java/com/hotels/styx/api/extension/service/spi/AbstractStyxServiceTest.java index a11c8bc591..2e46047b42 100644 --- a/components/api/src/test/java/com/hotels/styx/api/extension/service/spi/AbstractStyxServiceTest.java +++ b/components/api/src/test/java/com/hotels/styx/api/extension/service/spi/AbstractStyxServiceTest.java @@ -17,13 +17,12 @@ import com.hotels.styx.api.FullHttpResponse; import com.hotels.styx.api.HttpRequest; -import com.hotels.styx.api.extension.service.spi.AbstractStyxService; import org.testng.annotations.Test; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; -import static com.hotels.styx.api.MockContext.MOCK_CONTEXT; +import static com.hotels.styx.api.extension.service.spi.MockContext.MOCK_CONTEXT; import static com.hotels.styx.api.extension.service.spi.StyxServiceStatus.FAILED; import static com.hotels.styx.api.extension.service.spi.StyxServiceStatus.RUNNING; import static com.hotels.styx.api.extension.service.spi.StyxServiceStatus.STARTING; diff --git a/components/api/src/test/java/com/hotels/styx/api/MockContext.java b/components/api/src/test/java/com/hotels/styx/api/extension/service/spi/MockContext.java similarity index 67% rename from components/api/src/test/java/com/hotels/styx/api/MockContext.java rename to components/api/src/test/java/com/hotels/styx/api/extension/service/spi/MockContext.java index f72f61928f..2569cd4248 100644 --- a/components/api/src/test/java/com/hotels/styx/api/MockContext.java +++ b/components/api/src/test/java/com/hotels/styx/api/extension/service/spi/MockContext.java @@ -13,11 +13,15 @@ See the License for the specific language governing permissions and limitations under the License. */ -package com.hotels.styx.api; +package com.hotels.styx.api.extension.service.spi; -public class MockContext implements HttpInterceptor.Context { +import com.hotels.styx.api.HttpInterceptor; - public static final HttpInterceptor.Context MOCK_CONTEXT = new MockContext(); +import java.net.InetSocketAddress; +import java.util.Optional; + +class MockContext implements HttpInterceptor.Context { + static final HttpInterceptor.Context MOCK_CONTEXT = new MockContext(); @Override public void add(String key, Object value) { @@ -33,4 +37,9 @@ public T get(String key, Class clazz) { public boolean isSecure() { return false; } + + @Override + public Optional clientAddress() { + return Optional.empty(); + } } diff --git a/components/common/src/main/java/com/hotels/styx/server/HttpInterceptorContext.java b/components/common/src/main/java/com/hotels/styx/server/HttpInterceptorContext.java index bec9bf7385..5ea5b8dff7 100644 --- a/components/common/src/main/java/com/hotels/styx/server/HttpInterceptorContext.java +++ b/components/common/src/main/java/com/hotels/styx/server/HttpInterceptorContext.java @@ -17,7 +17,9 @@ import com.hotels.styx.api.HttpInterceptor; +import java.net.InetSocketAddress; import java.util.Map; +import java.util.Optional; import java.util.concurrent.ConcurrentHashMap; /** @@ -25,10 +27,49 @@ */ public final class HttpInterceptorContext implements HttpInterceptor.Context { private final Map context = new ConcurrentHashMap<>(); - private boolean secure; - private HttpInterceptorContext(boolean secure) { + private final boolean secure; + private final InetSocketAddress clientAddress; + + /** + * Construct a new instance. + * + * @param secure true if the request was received via SSL + * @param clientAddress address that request came from, or null if not-applicable + */ + public HttpInterceptorContext(boolean secure, InetSocketAddress clientAddress) { this.secure = secure; + this.clientAddress = clientAddress; // intentionally nullable + } + + /** + * Construct a new instance. + * + * @param clientAddress address that request came from, or null if not-applicable + */ + public HttpInterceptorContext(InetSocketAddress clientAddress) { + this(false, clientAddress); + } + + /** + * Construct a new instance. + * + * @param secure true if the request was received via SSL + */ + public HttpInterceptorContext(boolean secure) { + this(secure, null); + } + + /** + * Construct a new instance. + */ + public HttpInterceptorContext() { + this(false, null); + } + + // TODO deprecate + public static HttpInterceptor.Context create() { + return new HttpInterceptorContext(); } @Override @@ -46,11 +87,8 @@ public boolean isSecure() { return secure; } - public static HttpInterceptorContext create() { - return new HttpInterceptorContext(false); - } - - public static HttpInterceptorContext create(boolean secure) { - return new HttpInterceptorContext(secure); + @Override + public Optional clientAddress() { + return Optional.ofNullable(clientAddress); } } diff --git a/components/proxy/src/main/java/com/hotels/styx/proxy/HttpErrorStatusCauseLogger.java b/components/proxy/src/main/java/com/hotels/styx/proxy/HttpErrorStatusCauseLogger.java index 0a14b31815..1535be6301 100644 --- a/components/proxy/src/main/java/com/hotels/styx/proxy/HttpErrorStatusCauseLogger.java +++ b/components/proxy/src/main/java/com/hotels/styx/proxy/HttpErrorStatusCauseLogger.java @@ -21,6 +21,8 @@ import com.hotels.styx.api.HttpResponseStatus; import org.slf4j.Logger; +import java.net.InetSocketAddress; + import static org.slf4j.LoggerFactory.getLogger; /** @@ -40,9 +42,9 @@ public void proxyErrorOccurred(HttpResponseStatus status, Throwable cause) { } @Override - public void proxyErrorOccurred(HttpRequest request, HttpResponseStatus status, Throwable cause) { + public void proxyErrorOccurred(HttpRequest request, InetSocketAddress clientAddress, HttpResponseStatus status, Throwable cause) { if (status.code() == 500) { - LOG.error("Failure status=\"{}\" during request={}", new Object[]{status, request, cause}); + LOG.error("Failure status=\"{}\" during request={}, clientAddress={}", new Object[]{status, request, clientAddress, cause}); } else { proxyErrorOccurred(status, cause); } diff --git a/components/proxy/src/main/java/com/hotels/styx/proxy/HttpErrorStatusMetrics.java b/components/proxy/src/main/java/com/hotels/styx/proxy/HttpErrorStatusMetrics.java index 61ad575622..1f7bf92550 100644 --- a/components/proxy/src/main/java/com/hotels/styx/proxy/HttpErrorStatusMetrics.java +++ b/components/proxy/src/main/java/com/hotels/styx/proxy/HttpErrorStatusMetrics.java @@ -23,6 +23,8 @@ import com.hotels.styx.api.MetricRegistry; import com.hotels.styx.api.plugins.spi.PluginException; +import java.net.InetSocketAddress; + import static com.hotels.styx.api.HttpResponseStatus.INTERNAL_SERVER_ERROR; import static java.util.Objects.requireNonNull; @@ -56,7 +58,7 @@ public void proxyErrorOccurred(Throwable cause) { } @Override - public void proxyErrorOccurred(HttpRequest request, HttpResponseStatus status, Throwable cause) { + public void proxyErrorOccurred(HttpRequest request, InetSocketAddress clientAddress, HttpResponseStatus status, Throwable cause) { proxyErrorOccurred(status, cause); } diff --git a/components/proxy/src/main/java/com/hotels/styx/proxy/interceptors/RequestEnrichingInterceptor.java b/components/proxy/src/main/java/com/hotels/styx/proxy/interceptors/RequestEnrichingInterceptor.java index 7a9497d093..a2dceef225 100644 --- a/components/proxy/src/main/java/com/hotels/styx/proxy/interceptors/RequestEnrichingInterceptor.java +++ b/components/proxy/src/main/java/com/hotels/styx/proxy/interceptors/RequestEnrichingInterceptor.java @@ -16,19 +16,26 @@ package com.hotels.styx.proxy.interceptors; import com.hotels.styx.api.HttpInterceptor; +import com.hotels.styx.api.HttpRequest; import com.hotels.styx.api.HttpResponse; import com.hotels.styx.api.StyxObservable; import com.hotels.styx.client.StyxHeaderConfig; +import org.slf4j.Logger; + +import java.net.InetSocketAddress; +import java.util.Optional; import static com.hotels.styx.api.HttpHeaderNames.X_FORWARDED_FOR; import static com.hotels.styx.api.HttpHeaderNames.X_FORWARDED_PROTO; -import com.hotels.styx.api.HttpRequest; +import static org.slf4j.LoggerFactory.getLogger; /** * Adds X-Forwarded-For, X-Forwarded-Proto and X-Hcom-Request-Id headers to requests. */ public class RequestEnrichingInterceptor implements HttpInterceptor { + private static final Logger LOGGER = getLogger(RequestEnrichingInterceptor.class); + private final CharSequence requestIdHeaderName; public RequestEnrichingInterceptor(StyxHeaderConfig styxHeaderConfig) { @@ -37,28 +44,39 @@ public RequestEnrichingInterceptor(StyxHeaderConfig styxHeaderConfig) { @Override public StyxObservable intercept(HttpRequest request, Chain chain) { - return chain.proceed(enrich(request, chain.context().isSecure())); + return chain.proceed(enrich(request, chain.context())); } - private HttpRequest enrich(HttpRequest request, boolean secure) { - return request.newBuilder() + private HttpRequest enrich(HttpRequest request, Context context) { + HttpRequest.Builder builder = request.newBuilder(); + + xForwardedFor(request, context) + .ifPresent(headerValue -> builder.header(X_FORWARDED_FOR, headerValue)); + + return builder .header(requestIdHeaderName, request.id()) - .header(X_FORWARDED_FOR, xForwardedFor(request)) - .header(X_FORWARDED_PROTO, xForwardedProto(request, secure)) + .header(X_FORWARDED_PROTO, xForwardedProto(request, context.isSecure())) .build(); } + private static Optional xForwardedFor(HttpRequest request, HttpInterceptor.Context context) { + Optional maybeClientAddress = context.clientAddress() + .map(InetSocketAddress::getHostString) + .map(hostName -> request + .header(X_FORWARDED_FOR) + .map(xForwardedFor -> xForwardedFor + ", " + hostName) + .orElse(hostName)); + + if (!maybeClientAddress.isPresent()) { + LOGGER.warn("No clientAddress in context url={}", request.url()); + } + + return maybeClientAddress; + } + private static CharSequence xForwardedProto(HttpRequest request, boolean secure) { return request .header(X_FORWARDED_PROTO) .orElse(secure ? "https" : "http"); } - - private static String xForwardedFor(HttpRequest request) { - String hostName = request.clientAddress().getHostString(); - - return request.header(X_FORWARDED_FOR) - .map(xForwardedFor -> xForwardedFor + ", " + hostName) - .orElse(hostName); - } } diff --git a/components/proxy/src/test/java/com/hotels/styx/proxy/HttpErrorStatusCauseLoggerTest.java b/components/proxy/src/test/java/com/hotels/styx/proxy/HttpErrorStatusCauseLoggerTest.java index a29ddf59cc..f26d39601a 100644 --- a/components/proxy/src/test/java/com/hotels/styx/proxy/HttpErrorStatusCauseLoggerTest.java +++ b/components/proxy/src/test/java/com/hotels/styx/proxy/HttpErrorStatusCauseLoggerTest.java @@ -21,6 +21,8 @@ import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; +import java.net.InetSocketAddress; + import static ch.qos.logback.classic.Level.ERROR; import static com.hotels.styx.api.HttpResponseStatus.BAD_GATEWAY; import static com.hotels.styx.api.HttpResponseStatus.INTERNAL_SERVER_ERROR; @@ -74,22 +76,22 @@ public void logsInternalServerErrorWithRequest() { HttpRequest request = HttpRequest.get("/foo").build(); Exception exception = new Exception("This is just a test"); - httpErrorStatusCauseLogger.proxyErrorOccurred(request, INTERNAL_SERVER_ERROR, exception); + httpErrorStatusCauseLogger.proxyErrorOccurred(request, InetSocketAddress.createUnresolved("localhost", 80), INTERNAL_SERVER_ERROR, exception); assertThat(loggingTestSupport.log(), hasItem( loggingEvent( ERROR, - "Failure status=\"500 Internal Server Error\" during request=HttpRequest\\{version=HTTP/1.1, method=GET, uri=/foo, headers=\\[\\], id=.*, clientAddress=.*:.*\\}", + "Failure status=\"500 Internal Server Error\" during request=HttpRequest\\{version=HTTP/1.1, method=GET, uri=/foo, headers=\\[\\], id=.*\\}, clientAddress=localhost:80", "java.lang.Exception", "This is just a test"))); } @Test - public void logsOtherExceptionsWithoutRequest() throws Exception { + public void logsOtherExceptionsWithoutRequest() { HttpRequest request = HttpRequest.get("/foo").build(); Exception exception = new Exception("This is just a test"); - httpErrorStatusCauseLogger.proxyErrorOccurred(request, BAD_GATEWAY, exception); + httpErrorStatusCauseLogger.proxyErrorOccurred(request, InetSocketAddress.createUnresolved("127.0.0.1", 0), BAD_GATEWAY, exception); assertThat(loggingTestSupport.log(), hasItem( loggingEvent( diff --git a/components/proxy/src/test/java/com/hotels/styx/proxy/HttpErrorStatusMetricsTest.java b/components/proxy/src/test/java/com/hotels/styx/proxy/HttpErrorStatusMetricsTest.java index 9c37e7b004..a216adf733 100644 --- a/components/proxy/src/test/java/com/hotels/styx/proxy/HttpErrorStatusMetricsTest.java +++ b/components/proxy/src/test/java/com/hotels/styx/proxy/HttpErrorStatusMetricsTest.java @@ -27,6 +27,7 @@ import org.testng.annotations.DataProvider; import org.testng.annotations.Test; +import java.net.InetSocketAddress; import java.util.Collection; import java.util.stream.Stream; @@ -125,7 +126,7 @@ public void nonErrorStatusesIsNotRecordedForProxyEvenIfExceptionIsSupplied() { @Test public void updatesCountersForProxyErrorsWithResponse() { HttpRequest request = get("/foo").build(); - errorListener.proxyErrorOccurred(request, INTERNAL_SERVER_ERROR, new CustomException()); + errorListener.proxyErrorOccurred(request, InetSocketAddress.createUnresolved("127.0.0.1", 0), INTERNAL_SERVER_ERROR, new CustomException()); assertThat(count("styx.response.status.500"), is(1)); assertThat(statusCountsExcluding("styx.response.status.500"), everyItem(is(0))); diff --git a/components/proxy/src/test/java/com/hotels/styx/proxy/interceptors/ConfigurationContextResolverInterceptorTest.java b/components/proxy/src/test/java/com/hotels/styx/proxy/interceptors/ConfigurationContextResolverInterceptorTest.java index 82cc9fec09..3a96590598 100644 --- a/components/proxy/src/test/java/com/hotels/styx/proxy/interceptors/ConfigurationContextResolverInterceptorTest.java +++ b/components/proxy/src/test/java/com/hotels/styx/proxy/interceptors/ConfigurationContextResolverInterceptorTest.java @@ -17,15 +17,14 @@ import com.google.common.collect.ImmutableMap; import com.hotels.styx.api.HttpInterceptor; +import com.hotels.styx.api.HttpRequest; import com.hotels.styx.api.HttpResponse; +import com.hotels.styx.api.StyxObservable; import com.hotels.styx.api.configuration.Configuration; import com.hotels.styx.api.configuration.ConfigurationContextResolver; -import com.hotels.styx.api.StyxObservable; +import com.hotels.styx.server.HttpInterceptorContext; import org.testng.annotations.Test; -import java.util.HashMap; -import java.util.Map; - import static com.hotels.styx.api.HttpRequest.get; import static com.hotels.styx.api.HttpResponse.response; import static com.hotels.styx.api.HttpResponseStatus.OK; @@ -35,7 +34,6 @@ import static org.hamcrest.Matchers.is; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; -import com.hotels.styx.api.HttpRequest; public class ConfigurationContextResolverInterceptorTest { @Test @@ -69,7 +67,7 @@ private Configuration.Context context(ImmutableMap map) { } private static class TestChain implements HttpInterceptor.Chain { - private TestChainContext context = new TestChainContext(); + private final HttpInterceptor.Context context = new HttpInterceptorContext(); private boolean proceedWasCalled; @Override @@ -83,24 +81,5 @@ public StyxObservable proceed(HttpRequest request) { return StyxObservable.of(response(OK).build()); } - - private class TestChainContext implements HttpInterceptor.Context { - private Map map = new HashMap<>(); - - @Override - public void add(String key, Object value) { - map.put(key, value); - } - - @Override - public T get(String key, Class type) { - return type.cast(map.get(key)); - } - - @Override - public boolean isSecure() { - return false; - } - } } } \ No newline at end of file diff --git a/components/proxy/src/test/java/com/hotels/styx/proxy/interceptors/HttpMessageLoggingInterceptorTest.java b/components/proxy/src/test/java/com/hotels/styx/proxy/interceptors/HttpMessageLoggingInterceptorTest.java index 59a4007d9a..faf64406d4 100644 --- a/components/proxy/src/test/java/com/hotels/styx/proxy/interceptors/HttpMessageLoggingInterceptorTest.java +++ b/components/proxy/src/test/java/com/hotels/styx/proxy/interceptors/HttpMessageLoggingInterceptorTest.java @@ -29,9 +29,9 @@ import static ch.qos.logback.classic.Level.INFO; import static com.hotels.styx.api.HttpRequest.get; import static com.hotels.styx.api.HttpResponse.response; +import static com.hotels.styx.api.HttpResponseStatus.OK; import static com.hotels.styx.api.RequestCookie.requestCookie; import static com.hotels.styx.api.ResponseCookie.responseCookie; -import static com.hotels.styx.api.HttpResponseStatus.OK; import static com.hotels.styx.common.StyxFutures.await; import static com.hotels.styx.support.matchers.LoggingEventMatcher.loggingEvent; import static org.hamcrest.MatcherAssert.assertThat; @@ -122,7 +122,7 @@ public StyxObservable proceed(HttpRequest request) { @Override public HttpInterceptor.Context context() { - return HttpInterceptorContext.create(true); + return new HttpInterceptorContext(true); } }; } diff --git a/components/proxy/src/test/java/com/hotels/styx/proxy/interceptors/RequestEnrichingInterceptorTest.java b/components/proxy/src/test/java/com/hotels/styx/proxy/interceptors/RequestEnrichingInterceptorTest.java index 35880855b9..fcb6fcfaad 100644 --- a/components/proxy/src/test/java/com/hotels/styx/proxy/interceptors/RequestEnrichingInterceptorTest.java +++ b/components/proxy/src/test/java/com/hotels/styx/proxy/interceptors/RequestEnrichingInterceptorTest.java @@ -15,89 +15,105 @@ */ package com.hotels.styx.proxy.interceptors; +import com.hotels.styx.api.HttpInterceptor; +import com.hotels.styx.api.HttpInterceptor.Chain; import com.hotels.styx.api.HttpRequest; +import com.hotels.styx.api.HttpResponse; +import com.hotels.styx.api.StyxObservable; import com.hotels.styx.client.StyxHeaderConfig; import com.hotels.styx.server.HttpInterceptorContext; import org.testng.annotations.Test; +import java.net.InetSocketAddress; + import static com.google.common.net.HttpHeaders.X_FORWARDED_FOR; import static com.google.common.net.HttpHeaders.X_FORWARDED_PROTO; import static com.hotels.styx.api.HttpRequest.get; import static com.hotels.styx.api.HttpResponse.response; import static com.hotels.styx.client.StyxHeaderConfig.REQUEST_ID_DEFAULT; import static com.hotels.styx.proxy.interceptors.RequestRecordingChain.requestRecordingChain; -import static com.hotels.styx.proxy.interceptors.ReturnResponseChain.returnsResponse; import static com.hotels.styx.support.matchers.IsOptional.isValue; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.notNullValue; import static org.hamcrest.MatcherAssert.assertThat; public class RequestEnrichingInterceptorTest { + private final RequestRecordingChain recording = requestRecordingChain(new TestChain(false)); + private final RequestRecordingChain recordingSecure = requestRecordingChain(new TestChain(true)); + private final RequestEnrichingInterceptor requestEnrichingInterceptor = new RequestEnrichingInterceptor(new StyxHeaderConfig()); @Test public void addsRequestIdToTheHeaders() { - RequestRecordingChain recording = intercept(get("/some-uri"), false); + requestEnrichingInterceptor.intercept(get("/some-uri").build(), recording); assertThat(recording.recordedRequest().header(REQUEST_ID_DEFAULT), is(notNullValue())); } @Test public void setsTheRemoteAddressToTheForwardedForList() { - RequestRecordingChain recording = intercept(get(""), false); + requestEnrichingInterceptor.intercept(get("").build(), recording); assertThat(recording.recordedRequest().header(X_FORWARDED_FOR), isValue("127.0.0.1")); } @Test public void appendsTheRemoteAddressToTheForwardedForList() { - RequestRecordingChain recording = intercept(get("/some") - .header(X_FORWARDED_FOR, "172.21.175.59"), false); + requestEnrichingInterceptor.intercept(get("/some") + .header(X_FORWARDED_FOR, "172.21.175.59").build(), recording); assertThat(recording.recordedRequest().header(X_FORWARDED_FOR), isValue("172.21.175.59, 127.0.0.1")); } @Test - public void addsXForwardedProtoToHttpWhenAbsent() throws Exception { - RequestRecordingChain recording = intercept(get("/some"), false); + public void addsXForwardedProtoToHttpWhenAbsent() { + requestEnrichingInterceptor.intercept(get("/some").build(), recording); assertThat(recording.recordedRequest().header(X_FORWARDED_PROTO), isValue("http")); } @Test - public void addsXForwardedProtoToHttpsWhenAbsent() throws Exception { - RequestRecordingChain recording = intercept(get("/some"), true); + public void addsXForwardedProtoToHttpsWhenAbsent() { + requestEnrichingInterceptor.intercept(get("/some").build(), recordingSecure); - assertThat(recording.recordedRequest().header(X_FORWARDED_PROTO), isValue("https")); + assertThat(recordingSecure.recordedRequest().header(X_FORWARDED_PROTO), isValue("https")); } @Test - public void retainsXForwardedProtoWhenPresentInHttpMessage() throws Exception { - RequestRecordingChain recording = intercept(get("/some").addHeader(X_FORWARDED_PROTO, "https"), false); + public void retainsXForwardedProtoWhenPresentInHttpMessage() { + requestEnrichingInterceptor.intercept(get("/some").addHeader(X_FORWARDED_PROTO, "https").build(), recording); + assertThat(recording.recordedRequest().header(X_FORWARDED_PROTO), isValue("https")); - recording = intercept(get("/some").addHeader(X_FORWARDED_PROTO, "http"), false); + requestEnrichingInterceptor.intercept(get("/some").addHeader(X_FORWARDED_PROTO, "http").build(), recording); assertThat(recording.recordedRequest().header(X_FORWARDED_PROTO), isValue("http")); } - @Test - public void retainsXForwardedProtoWhenPresentInHttpsMessage() throws Exception { - RequestRecordingChain recording = intercept(get("/some").addHeader(X_FORWARDED_PROTO, "http"), true); + public void retainsXForwardedProtoWhenPresentInHttpsMessage() { + requestEnrichingInterceptor.intercept(get("/some").addHeader(X_FORWARDED_PROTO, "http").build(), recording); + assertThat(recording.recordedRequest().header(X_FORWARDED_PROTO), isValue("http")); - recording = intercept(get("/some").addHeader(X_FORWARDED_PROTO, "https"), true); + requestEnrichingInterceptor.intercept(get("/some").addHeader(X_FORWARDED_PROTO, "https").build(), recording); assertThat(recording.recordedRequest().header(X_FORWARDED_PROTO), isValue("https")); } - private RequestRecordingChain intercept(HttpRequest.Builder builder, boolean secure) { - return intercept(builder.build(), secure); - } + private static class TestChain implements Chain { + private final boolean secure; - private RequestRecordingChain intercept(HttpRequest request, boolean secure) { - RequestRecordingChain recording = requestRecordingChain(returnsResponse(response().build(), HttpInterceptorContext.create(secure))); - requestEnrichingInterceptor.intercept(request, recording); - return recording; - } + TestChain(boolean secure) { + this.secure = secure; + } + @Override + public StyxObservable proceed(HttpRequest request) { + return StyxObservable.of(response().build()); + } + + @Override + public HttpInterceptor.Context context() { + return new HttpInterceptorContext(secure, InetSocketAddress.createUnresolved("127.0.0.1", 80)); + } + } } diff --git a/components/proxy/src/test/java/com/hotels/styx/routing/handlers/StandardHttpPipelineTest.java b/components/proxy/src/test/java/com/hotels/styx/routing/handlers/StandardHttpPipelineTest.java index e48daa0c0b..8802a74ee8 100644 --- a/components/proxy/src/test/java/com/hotels/styx/routing/handlers/StandardHttpPipelineTest.java +++ b/components/proxy/src/test/java/com/hotels/styx/routing/handlers/StandardHttpPipelineTest.java @@ -23,6 +23,7 @@ import org.testng.annotations.DataProvider; import org.testng.annotations.Test; +import java.net.InetSocketAddress; import java.util.ArrayList; import java.util.List; import java.util.concurrent.atomic.AtomicReference; @@ -30,8 +31,8 @@ import static com.hotels.styx.api.HttpRequest.get; import static com.hotels.styx.api.HttpResponse.response; -import static com.hotels.styx.api.StyxInternalObservables.toRxObservable; import static com.hotels.styx.api.HttpResponseStatus.OK; +import static com.hotels.styx.api.StyxInternalObservables.toRxObservable; import static com.hotels.styx.common.StyxFutures.await; import static java.util.Arrays.asList; import static java.util.Collections.singletonList; @@ -225,7 +226,9 @@ private HttpInterceptor recordingInterceptor(String name, Consumer onInt } private HttpResponse sendRequestTo(StandardHttpPipeline pipeline) { - return await(pipeline.handle(get("/").build(), HttpInterceptorContext.create()).asCompletableFuture()); + HttpInterceptor.Context context = new HttpInterceptorContext(InetSocketAddress.createUnresolved("127.0.0.1", 0)); + + return await(pipeline.handle(get("/").build(), context).asCompletableFuture()); } private StandardHttpPipeline pipeline(HttpInterceptor... interceptors) { diff --git a/components/proxy/src/test/scala/com/hotels/styx/routing/handlers/ConditionRouterConfigSpec.scala b/components/proxy/src/test/scala/com/hotels/styx/routing/handlers/ConditionRouterConfigSpec.scala index 65abacb544..550d7a7172 100644 --- a/components/proxy/src/test/scala/com/hotels/styx/routing/handlers/ConditionRouterConfigSpec.scala +++ b/components/proxy/src/test/scala/com/hotels/styx/routing/handlers/ConditionRouterConfigSpec.scala @@ -16,13 +16,13 @@ package com.hotels.styx.routing.handlers import com.hotels.styx.api.HttpResponse.response -import com.hotels.styx.api.{HttpHandler, HttpRequest, StyxObservable} +import com.hotels.styx.api.HttpResponseStatus.{BAD_GATEWAY, OK} +import com.hotels.styx.api.{HttpHandler, HttpInterceptor, HttpRequest, StyxObservable} import com.hotels.styx.common.StyxFutures import com.hotels.styx.infrastructure.configuration.yaml.YamlConfig import com.hotels.styx.routing.HttpHandlerAdapter import com.hotels.styx.routing.config.{HttpHandlerFactory, RouteHandlerConfig, RouteHandlerDefinition, RouteHandlerFactory} import com.hotels.styx.server.HttpInterceptorContext -import com.hotels.styx.api.HttpResponseStatus.{BAD_GATEWAY, OK} import org.mockito.Matchers.{any, eq => meq} import org.mockito.Mockito.{verify, when} import org.scalatest.mock.MockitoSugar @@ -72,7 +72,7 @@ class ConditionRouterConfigSpec extends FunSpec with ShouldMatchers with Mockito it("Builds an instance with fallback handler") { val router = new ConditionRouter.ConfigFactory().build(List(), routeHandlerFactory, config) - val response = StyxFutures.await(router.handle(request, HttpInterceptorContext.create(true)) + val response = StyxFutures.await(router.handle(request, new HttpInterceptorContext(true)) .asCompletableFuture()) response.status() should be(OK) @@ -80,7 +80,7 @@ class ConditionRouterConfigSpec extends FunSpec with ShouldMatchers with Mockito it("Builds condition router instance routes") { val router = new ConditionRouter.ConfigFactory().build(List(), routeHandlerFactory, config) - val response = StyxFutures.await(router.handle(request, HttpInterceptorContext.create(false)).asCompletableFuture()) + val response = StyxFutures.await(router.handle(request, new HttpInterceptorContext()).asCompletableFuture()) response.status().code() should be(301) } @@ -95,7 +95,7 @@ class ConditionRouterConfigSpec extends FunSpec with ShouldMatchers with Mockito val router = new ConditionRouter.ConfigFactory().build(List(), routeHandlerFactory, configWithReferences) - val resp = StyxFutures.await(router.handle(request, HttpInterceptorContext.create(false)).asCompletableFuture()) + val resp = StyxFutures.await(router.handle(request, new HttpInterceptorContext()).asCompletableFuture()) resp.header("source").get() should be("fallback") } @@ -114,7 +114,7 @@ class ConditionRouterConfigSpec extends FunSpec with ShouldMatchers with Mockito configWithReferences ) - val resp = StyxFutures.await(router.handle(request, HttpInterceptorContext.create(true)).asCompletableFuture()) + val resp = StyxFutures.await(router.handle(request, new HttpInterceptorContext(true)).asCompletableFuture()) resp.header("source").get() should be("secure") } @@ -159,7 +159,8 @@ class ConditionRouterConfigSpec extends FunSpec with ShouldMatchers with Mockito |""".stripMargin) val router = new ConditionRouter.ConfigFactory().build(List(), routeHandlerFactory, config) - val resp = StyxFutures.await(router.handle(request, HttpInterceptorContext.create(false)).asCompletableFuture()) + + val resp = StyxFutures.await(router.handle(request, new HttpInterceptorContext()).asCompletableFuture()) resp.status() should be(BAD_GATEWAY) } diff --git a/components/server/src/main/java/com/hotels/styx/server/CompositeHttpErrorStatusListener.java b/components/server/src/main/java/com/hotels/styx/server/CompositeHttpErrorStatusListener.java index ebdae5c7d8..ec3bff474a 100644 --- a/components/server/src/main/java/com/hotels/styx/server/CompositeHttpErrorStatusListener.java +++ b/components/server/src/main/java/com/hotels/styx/server/CompositeHttpErrorStatusListener.java @@ -19,6 +19,7 @@ import com.hotels.styx.api.HttpResponse; import com.hotels.styx.api.HttpResponseStatus; +import java.net.InetSocketAddress; import java.util.ArrayList; import java.util.List; @@ -50,7 +51,7 @@ public void proxyErrorOccurred(HttpResponseStatus status, Throwable cause) { } @Override - public void proxyErrorOccurred(HttpRequest request, HttpResponseStatus status, Throwable cause) { - listeners.forEach(listener -> listener.proxyErrorOccurred(request, status, cause)); + public void proxyErrorOccurred(HttpRequest request, InetSocketAddress clientAddress, HttpResponseStatus status, Throwable cause) { + listeners.forEach(listener -> listener.proxyErrorOccurred(request, clientAddress, status, cause)); } } diff --git a/components/server/src/main/java/com/hotels/styx/server/HttpErrorStatusListener.java b/components/server/src/main/java/com/hotels/styx/server/HttpErrorStatusListener.java index 2c4681e9f1..dbe23ab1fd 100644 --- a/components/server/src/main/java/com/hotels/styx/server/HttpErrorStatusListener.java +++ b/components/server/src/main/java/com/hotels/styx/server/HttpErrorStatusListener.java @@ -19,6 +19,8 @@ import com.hotels.styx.api.HttpResponse; import com.hotels.styx.api.HttpResponseStatus; +import java.net.InetSocketAddress; + import static java.util.Arrays.asList; /** @@ -39,7 +41,7 @@ public void proxyingFailure(HttpRequest request, HttpResponse response, Throwabl public void proxyErrorOccurred(HttpResponseStatus status, Throwable cause) { } @Override - public void proxyErrorOccurred(HttpRequest request, HttpResponseStatus status, Throwable cause) { } + public void proxyErrorOccurred(HttpRequest request, InetSocketAddress clientAddress, HttpResponseStatus status, Throwable cause) { } }; static HttpErrorStatusListener compose(HttpErrorStatusListener... listeners) { @@ -85,5 +87,5 @@ static HttpErrorStatusListener compose(HttpErrorStatusListener... listeners) { * @param status HTTP response status * @param cause the throwable class associated with this error */ - void proxyErrorOccurred(HttpRequest request, HttpResponseStatus status, Throwable cause); + void proxyErrorOccurred(HttpRequest request, InetSocketAddress clientAddress, HttpResponseStatus status, Throwable cause); } diff --git a/components/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java b/components/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java index 8dde18bf71..01f5f7bf7d 100644 --- a/components/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java +++ b/components/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java @@ -87,7 +87,7 @@ protected void decode(ChannelHandlerContext ctx, HttpObject httpObject, List contentObservable) { + private com.hotels.styx.api.HttpRequest toStyxRequest(HttpRequest request, Observable contentObservable) { validateHostHeader(request); return makeAStyxRequestFrom(request, contentObservable) - .clientAddress(remoteAddress(ctx)) .removeHeader(EXPECT) .build(); } diff --git a/components/server/src/main/java/com/hotels/styx/server/netty/connectors/HttpPipelineHandler.java b/components/server/src/main/java/com/hotels/styx/server/netty/connectors/HttpPipelineHandler.java index 0f0a99e249..2c6a5d6471 100644 --- a/components/server/src/main/java/com/hotels/styx/server/netty/connectors/HttpPipelineHandler.java +++ b/components/server/src/main/java/com/hotels/styx/server/netty/connectors/HttpPipelineHandler.java @@ -40,11 +40,13 @@ import com.hotels.styx.common.StateMachine; import com.hotels.styx.server.BadRequestException; import com.hotels.styx.server.HttpErrorStatusListener; +import com.hotels.styx.server.HttpInterceptorContext; import com.hotels.styx.server.NoServiceConfiguredException; import com.hotels.styx.server.RequestProgressListener; import com.hotels.styx.server.RequestTimeoutException; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.SimpleChannelInboundHandler; +import io.netty.channel.embedded.EmbeddedChannel; import io.netty.handler.codec.DecoderException; import io.netty.handler.codec.TooLongFrameException; import org.slf4j.Logger; @@ -54,9 +56,8 @@ import javax.net.ssl.SSLHandshakeException; import java.io.IOException; -import java.util.Map; +import java.net.InetSocketAddress; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.ConcurrentHashMap; import java.util.function.Supplier; import static com.hotels.styx.api.HttpHeaderNames.CONTENT_LENGTH; @@ -215,7 +216,7 @@ public void channelInactive(ChannelHandlerContext ctx) throws Exception { } @Override - public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { + public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { eventProcessor.submit(new ChannelExceptionEvent(ctx, cause)); } @@ -253,7 +254,7 @@ private State onLegitimateRequest(HttpRequest request, ChannelHandlerContext ctx // the same call stack as "onLegitimateRequest" handler. This happens when a plugin // generates a response. try { - Observable responseObservable = toRxObservable(httpPipeline.handle(v11Request, new HttpInterceptorContext(this.secure))); + Observable responseObservable = toRxObservable(httpPipeline.handle(v11Request, newInterceptorContext(ctx))); subscription = responseObservable .subscribe(new Subscriber() { @Override @@ -276,7 +277,7 @@ public void onNext(HttpResponse response) { return WAITING_FOR_RESPONSE; } catch (Throwable cause) { HttpResponse response = exceptionToResponse(cause, request); - httpErrorStatusListener.proxyErrorOccurred(request, response.status(), cause); + httpErrorStatusListener.proxyErrorOccurred(request, remoteAddress(ctx), response.status(), cause); statsSink.onTerminate(request.id()); if (ctx.channel().isActive()) { respondAndClose(ctx, response); @@ -285,6 +286,10 @@ public void onNext(HttpResponse response) { } } + private HttpInterceptor.Context newInterceptorContext(ChannelHandlerContext ctx) { + return new HttpInterceptorContext(this.secure, remoteAddress(ctx)); + } + private State onResponseReceived(HttpResponse response, ChannelHandlerContext ctx) { ongoingResponse = response; HttpResponseWriter httpResponseWriter = responseWriterFactory.create(ctx); @@ -444,7 +449,7 @@ private State onResponseObservableError(ChannelHandlerContext ctx, Throwable cau httpErrorStatusListener.proxyErrorOccurred(cause); httpErrorStatusListener.proxyErrorOccurred(exception); } else { - httpErrorStatusListener.proxyErrorOccurred(ongoingRequest, response.status(), cause); + httpErrorStatusListener.proxyErrorOccurred(ongoingRequest, remoteAddress(ctx), response.status(), cause); statsSink.onComplete(ongoingRequest.id(), response.status().code()); } @@ -706,27 +711,11 @@ protected void channelRead0(ChannelHandlerContext ctx, HttpRequest request) thro } } - private static final class HttpInterceptorContext implements HttpInterceptor.Context { - private final Map context = new ConcurrentHashMap<>(); - private final boolean secure; - - public HttpInterceptorContext(boolean secure) { - this.secure = secure; + private static InetSocketAddress remoteAddress(ChannelHandlerContext ctx) { + if (ctx.channel() instanceof EmbeddedChannel) { + return new InetSocketAddress(0); } - @Override - public void add(String key, Object value) { - context.put(key, value); - } - - @Override - public T get(String key, Class clazz) { - return (T) context.get(key); - } - - @Override - public boolean isSecure() { - return secure; - } + return (InetSocketAddress) ctx.channel().remoteAddress(); } } diff --git a/components/server/src/test/java/com/hotels/styx/server/CompositeHttpErrorStatusListenerTest.java b/components/server/src/test/java/com/hotels/styx/server/CompositeHttpErrorStatusListenerTest.java index 534bd455bc..0e3e1d1c0e 100644 --- a/components/server/src/test/java/com/hotels/styx/server/CompositeHttpErrorStatusListenerTest.java +++ b/components/server/src/test/java/com/hotels/styx/server/CompositeHttpErrorStatusListenerTest.java @@ -21,6 +21,7 @@ import org.testng.annotations.Test; import java.io.IOException; +import java.net.InetSocketAddress; import static com.hotels.styx.api.HttpResponseStatus.INTERNAL_SERVER_ERROR; import static com.hotels.styx.api.HttpResponseStatus.OK; @@ -88,10 +89,11 @@ public void propagatesProxyingFailures() { public void propagatesProxyErrorsWithRequests() { HttpRequest request = HttpRequest.get("/foo").build(); IOException cause = new IOException(); - listener.proxyErrorOccurred(request, INTERNAL_SERVER_ERROR, cause); + InetSocketAddress clientAddress = InetSocketAddress.createUnresolved("127.0.0.1", 0); + listener.proxyErrorOccurred(request, clientAddress, INTERNAL_SERVER_ERROR, cause); - verify(delegate1).proxyErrorOccurred(request, INTERNAL_SERVER_ERROR, cause); - verify(delegate2).proxyErrorOccurred(request, INTERNAL_SERVER_ERROR, cause); - verify(delegate3).proxyErrorOccurred(request, INTERNAL_SERVER_ERROR, cause); + verify(delegate1).proxyErrorOccurred(request, clientAddress, INTERNAL_SERVER_ERROR, cause); + verify(delegate2).proxyErrorOccurred(request, clientAddress, INTERNAL_SERVER_ERROR, cause); + verify(delegate3).proxyErrorOccurred(request, clientAddress, INTERNAL_SERVER_ERROR, cause); } } \ No newline at end of file diff --git a/components/server/src/test/java/com/hotels/styx/server/handlers/StaticFileHandlerTest.java b/components/server/src/test/java/com/hotels/styx/server/handlers/StaticFileHandlerTest.java index 12c116c5f6..c8f56a0f01 100644 --- a/components/server/src/test/java/com/hotels/styx/server/handlers/StaticFileHandlerTest.java +++ b/components/server/src/test/java/com/hotels/styx/server/handlers/StaticFileHandlerTest.java @@ -17,7 +17,6 @@ import com.google.common.io.Files; import com.google.common.net.MediaType; -import com.hotels.styx.api.HttpInterceptor; import com.hotels.styx.api.HttpRequest; import com.hotels.styx.api.HttpResponse; import com.hotels.styx.server.HttpInterceptorContext; @@ -43,12 +42,12 @@ import static com.google.common.net.MediaType.PLAIN_TEXT_UTF_8; import static com.google.common.net.MediaType.PNG; import static com.hotels.styx.api.HttpRequest.get; +import static com.hotels.styx.api.HttpResponseStatus.NOT_FOUND; +import static com.hotels.styx.api.HttpResponseStatus.OK; import static com.hotels.styx.server.handlers.MediaTypes.ICON; import static com.hotels.styx.server.handlers.MediaTypes.MICROSOFT_ASF_VIDEO; import static com.hotels.styx.server.handlers.MediaTypes.MICROSOFT_MS_VIDEO; import static com.hotels.styx.server.handlers.MediaTypes.WAV_AUDIO; -import static com.hotels.styx.api.HttpResponseStatus.NOT_FOUND; -import static com.hotels.styx.api.HttpResponseStatus.OK; import static com.hotels.styx.support.api.BlockingObservables.getFirst; import static com.hotels.styx.support.api.matchers.HttpStatusMatcher.hasStatus; import static com.hotels.styx.support.matchers.IsOptional.isValue; @@ -74,9 +73,9 @@ public void cleanUpWorkingDir() { @Test public void should404ForMissingFiles() { - assertThat(handle(get("/index.html").build(), HttpInterceptorContext.create()), hasStatus(NOT_FOUND)); - assertThat(handle(get("/notfound.html").build(), HttpInterceptorContext.create()), hasStatus(NOT_FOUND)); - assertThat(handle(get("/foo/bar").build(), HttpInterceptorContext.create()), hasStatus(NOT_FOUND)); + assertThat(handle(get("/index.html").build()), hasStatus(NOT_FOUND)); + assertThat(handle(get("/notfound.html").build()), hasStatus(NOT_FOUND)); + assertThat(handle(get("/foo/bar").build()), hasStatus(NOT_FOUND)); } @Test @@ -85,12 +84,12 @@ public void shouldServeExistingFiles() throws Exception { writeFile("foo.js", "Blah"); mkdir("/a/b"); writeFile("a/b/good", "hi"); - assertThat("asking for /index.html", handle(get("/index.html").build(), HttpInterceptorContext.create()), hasStatus(OK)); - assertThat(handle(get("/foo.js").build(), HttpInterceptorContext.create()), hasStatus(OK)); + assertThat("asking for /index.html", handle(get("/index.html").build()), hasStatus(OK)); + assertThat(handle(get("/foo.js").build()), hasStatus(OK)); - assertThat(handle(get("/notfound.html").build(), HttpInterceptorContext.create()), hasStatus(NOT_FOUND)); - assertThat(handle(get("/a/b/good").build(), HttpInterceptorContext.create()), hasStatus(OK)); - assertThat(handle(get("/a/b/bad").build(), HttpInterceptorContext.create()), hasStatus(NOT_FOUND)); + assertThat(handle(get("/notfound.html").build()), hasStatus(NOT_FOUND)); + assertThat(handle(get("/a/b/good").build()), hasStatus(OK)); + assertThat(handle(get("/a/b/bad").build()), hasStatus(NOT_FOUND)); } @Test @@ -100,9 +99,9 @@ public void shouldIgnoreQueryParams() throws Exception { mkdir("/a/b"); writeFile("a/b/good", "hi"); - assertThat(handle(get("/index.html?foo=x").build(), HttpInterceptorContext.create()), hasStatus(OK)); - assertThat(handle(get("/foo.js?sdfsd").build(), HttpInterceptorContext.create()), hasStatus(OK)); - assertThat(handle(get("/a/b/good?xx").build(), HttpInterceptorContext.create()), hasStatus(OK)); + assertThat(handle(get("/index.html?foo=x").build()), hasStatus(OK)); + assertThat(handle(get("/foo.js?sdfsd").build()), hasStatus(OK)); + assertThat(handle(get("/a/b/good?xx").build()), hasStatus(OK)); } @Test @@ -115,8 +114,8 @@ public void shouldDisallowDirectoryTraversals() throws Exception { handler = new StaticFileHandler(new File(dir, "/a/public")); - assertThat(handle(get("/index.html").build(), HttpInterceptorContext.create()), hasStatus(OK)); - assertThat(handle(get("/../private/index.html").build(), HttpInterceptorContext.create()), hasStatus(NOT_FOUND)); + assertThat(handle(get("/index.html").build()), hasStatus(OK)); + assertThat(handle(get("/../private/index.html").build()), hasStatus(NOT_FOUND)); } @Test @@ -129,8 +128,8 @@ public void shouldDisallowDirectoryTraversalsWithEncodedRequests() throws Except handler = new StaticFileHandler(new File(dir, "/a/public")); - assertThat(handle(get("/index.html").build(), HttpInterceptorContext.create()), hasStatus(OK)); - assertThat(handle(get("/%2e%2e%2fprivate/index.html").build(), HttpInterceptorContext.create()), hasStatus(NOT_FOUND)); + assertThat(handle(get("/index.html").build()), hasStatus(OK)); + assertThat(handle(get("/%2e%2e%2fprivate/index.html").build()), hasStatus(NOT_FOUND)); } @Test(dataProvider = "fileTypesProvider") @@ -139,7 +138,7 @@ public void setsTheContentTypeBasedOnFileExtension(String path, MediaType mediaT handler = new StaticFileHandler(dir); - HttpResponse response = handle(get("/" + path).build(), HttpInterceptorContext.create()); + HttpResponse response = handle(get("/" + path).build()); assertThat(response, hasStatus(OK)); assertThat(response.contentType(), isValue(mediaType.toString())); } @@ -181,7 +180,7 @@ private void writeFile(String path, String contents) throws IOException { } } - private HttpResponse handle(HttpRequest request, HttpInterceptor.Context context) { - return getFirst(handler.handle(request, context)); + private HttpResponse handle(HttpRequest request) { + return getFirst(handler.handle(request, new HttpInterceptorContext())); } } diff --git a/components/server/src/test/java/com/hotels/styx/server/netty/connectors/HttpPipelineHandlerTest.java b/components/server/src/test/java/com/hotels/styx/server/netty/connectors/HttpPipelineHandlerTest.java index 03e4e6c6d0..bc94594827 100644 --- a/components/server/src/test/java/com/hotels/styx/server/netty/connectors/HttpPipelineHandlerTest.java +++ b/components/server/src/test/java/com/hotels/styx/server/netty/connectors/HttpPipelineHandlerTest.java @@ -20,13 +20,14 @@ import com.hotels.styx.api.FullHttpResponse; import com.hotels.styx.api.HttpHandler; import com.hotels.styx.api.HttpInterceptor; +import com.hotels.styx.api.HttpRequest; import com.hotels.styx.api.HttpResponse; import com.hotels.styx.api.StyxObservable; -import com.hotels.styx.server.HttpErrorStatusListener; -import com.hotels.styx.server.RequestStatsCollector; import com.hotels.styx.api.metrics.codahale.CodaHaleMetricRegistry; import com.hotels.styx.client.StyxClientException; import com.hotels.styx.server.BadRequestException; +import com.hotels.styx.server.HttpErrorStatusListener; +import com.hotels.styx.server.RequestStatsCollector; import com.hotels.styx.server.RequestTimeoutException; import com.hotels.styx.server.netty.codec.NettyToStyxRequestDecoder; import com.hotels.styx.server.netty.connectors.HttpPipelineHandler.HttpResponseWriterFactory; @@ -47,11 +48,10 @@ import org.testng.annotations.Test; import rx.Observable; import rx.subjects.PublishSubject; -import com.hotels.styx.api.HttpRequest; import javax.net.ssl.SSLHandshakeException; import java.io.IOException; -import java.net.SocketAddress; +import java.net.InetSocketAddress; import java.util.concurrent.CompletableFuture; import java.util.concurrent.atomic.AtomicBoolean; @@ -64,13 +64,13 @@ import static com.hotels.styx.api.HttpHeaderValues.CLOSE; import static com.hotels.styx.api.HttpRequest.get; import static com.hotels.styx.api.HttpResponse.response; -import static com.hotels.styx.api.StyxInternalObservables.fromRxObservable; import static com.hotels.styx.api.HttpResponseStatus.BAD_GATEWAY; import static com.hotels.styx.api.HttpResponseStatus.BAD_REQUEST; import static com.hotels.styx.api.HttpResponseStatus.INTERNAL_SERVER_ERROR; import static com.hotels.styx.api.HttpResponseStatus.OK; import static com.hotels.styx.api.HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE; import static com.hotels.styx.api.HttpResponseStatus.REQUEST_TIMEOUT; +import static com.hotels.styx.api.StyxInternalObservables.fromRxObservable; import static com.hotels.styx.server.netty.connectors.HttpPipelineHandler.State.ACCEPTING_REQUESTS; import static com.hotels.styx.server.netty.connectors.HttpPipelineHandler.State.SENDING_RESPONSE; import static com.hotels.styx.server.netty.connectors.HttpPipelineHandler.State.SENDING_RESPONSE_CLIENT_CLOSED; @@ -237,7 +237,7 @@ public void mapsUnrecoverableInternalErrorsToInternalServerError500ResponseCode( assertThat(response.status(), is(io.netty.handler.codec.http.HttpResponseStatus.INTERNAL_SERVER_ERROR)); verify(responseEnhancer).enhance(any(HttpResponse.Builder.class), any(HttpRequest.class)); - verify(errorListener, only()).proxyErrorOccurred(any(HttpRequest.class), eq(INTERNAL_SERVER_ERROR), any(RuntimeException.class)); + verify(errorListener, only()).proxyErrorOccurred(any(HttpRequest.class), any(InetSocketAddress.class), eq(INTERNAL_SERVER_ERROR), any(RuntimeException.class)); } @Test @@ -597,7 +597,7 @@ public void pluginPipelineThrowsAnExceptionInAcceptingRequestsState() throws Exc .toStreamingResponse()); verify(responseEnhancer).enhance(any(HttpResponse.Builder.class), eq(request)); - verify(errorListener).proxyErrorOccurred(request, INTERNAL_SERVER_ERROR, cause); + verify(errorListener).proxyErrorOccurred(request, InetSocketAddress.createUnresolved("localhost", 2), INTERNAL_SERVER_ERROR, cause); verify(statsCollector).onTerminate(request.id()); assertThat(handler.state(), is(TERMINATED)); } @@ -677,7 +677,7 @@ public void responseObservableEmitsContentOverflowExceptionInWaitingForResponseS writerFuture.complete(null); verify(statsCollector).onComplete(request.id(), 502); - verify(errorListener).proxyErrorOccurred(any(HttpRequest.class), eq(BAD_GATEWAY), any(RuntimeException.class)); + verify(errorListener).proxyErrorOccurred(any(HttpRequest.class), any(InetSocketAddress.class), eq(BAD_GATEWAY), any(RuntimeException.class)); // NOTE: channel closure is not verified. This is because cannot mock channel future. verify(ctx).close(); @@ -702,7 +702,7 @@ public void mapsStyxClientExceptionToInternalServerErrorInWaitingForResponseStat writerFuture.complete(null); verify(statsCollector).onComplete(request.id(), 500); - verify(errorListener).proxyErrorOccurred(any(HttpRequest.class), eq(INTERNAL_SERVER_ERROR), any(RuntimeException.class)); + verify(errorListener).proxyErrorOccurred(any(HttpRequest.class), any(InetSocketAddress.class), eq(INTERNAL_SERVER_ERROR), any(RuntimeException.class)); // NOTE: channel closure is not verified. This is because cannot mock channel future. verify(ctx).close(); @@ -968,13 +968,8 @@ private static ChannelHandlerContext mockCtx() { when(ctx.writeAndFlush(any(Object.class))).thenReturn(future); when(ctx.channel()).thenReturn(channel); - SocketAddress localAddress = mock(SocketAddress.class); - when(localAddress.toString()).thenReturn("localhost:1"); - when(ctx.channel().localAddress()).thenReturn(localAddress); - - SocketAddress remoteAddress = mock(SocketAddress.class); - when(remoteAddress.toString()).thenReturn("localhost:2"); - when(ctx.channel().remoteAddress()).thenReturn(remoteAddress); + when(ctx.channel().localAddress()).thenReturn(InetSocketAddress.createUnresolved("localhost", 1)); + when(ctx.channel().remoteAddress()).thenReturn(InetSocketAddress.createUnresolved("localhost", 2)); return ctx; } diff --git a/components/server/src/test/java/com/hotels/styx/server/routing/AntlrMatcherTest.java b/components/server/src/test/java/com/hotels/styx/server/routing/AntlrMatcherTest.java index 8cbd7c4430..01f3fa47db 100644 --- a/components/server/src/test/java/com/hotels/styx/server/routing/AntlrMatcherTest.java +++ b/components/server/src/test/java/com/hotels/styx/server/routing/AntlrMatcherTest.java @@ -24,11 +24,11 @@ public class AntlrMatcherTest { - private final HttpInterceptorContext contextHttps = HttpInterceptorContext.create(true); - private final HttpInterceptorContext contextHttp = HttpInterceptorContext.create(); + private final HttpInterceptorContext contextHttps = new HttpInterceptorContext(true); + private final HttpInterceptorContext contextHttp = new HttpInterceptorContext(); @Test - public void matchesHttpProtocol() throws Exception { + public void matchesHttpProtocol() { AntlrMatcher matcher = AntlrMatcher.antlrMatcher("protocol() == 'https'"); assertThat(matcher.apply(get("/path").build(), contextHttps), is(true)); assertThat(matcher.apply(get("/path").build(), contextHttp), is(false)); diff --git a/support/api-testsupport/pom.xml b/support/api-testsupport/pom.xml index 6c977a7db5..2049e2c98a 100755 --- a/support/api-testsupport/pom.xml +++ b/support/api-testsupport/pom.xml @@ -28,6 +28,11 @@ styx-api + + com.hotels.styx + styx-common + + org.testng testng @@ -44,10 +49,6 @@ wiremock compile - - com.hotels.styx - styx-common - - + diff --git a/support/api-testsupport/src/main/java/com/hotels/styx/server/HttpInterceptorContext.java b/support/api-testsupport/src/main/java/com/hotels/styx/server/HttpInterceptorContext.java deleted file mode 100644 index 2f5d65b5cd..0000000000 --- a/support/api-testsupport/src/main/java/com/hotels/styx/server/HttpInterceptorContext.java +++ /dev/null @@ -1,60 +0,0 @@ -/* - Copyright (C) 2013-2018 Expedia Inc. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. - */ -package com.hotels.styx.server; - -import com.hotels.styx.api.HttpInterceptor; - -import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; - -/** - * This is a duplicate of `com.hotels.styx.testapi.HttpInterceptorContext` - * for the use of Styx core unit tests. - * They cannot use the `testapi` implementation because the `testapi` itself - * depends on the styx core modules, introducing a cyclic dependency. - * - */ -public final class HttpInterceptorContext implements HttpInterceptor.Context { - private final Map context = new ConcurrentHashMap<>(); - private boolean secure; - - private HttpInterceptorContext(boolean secure) { - this.secure = secure; - } - - @Override - public void add(String key, Object value) { - context.put(key, value); - } - - @Override - public T get(String key, Class clazz) { - return (T) context.get(key); - } - - @Override - public boolean isSecure() { - return secure; - } - - public static HttpInterceptorContext create() { - return new HttpInterceptorContext(false); - } - - public static HttpInterceptorContext create(boolean secure) { - return new HttpInterceptorContext(secure); - } -} diff --git a/support/test-api/src/main/java/com/hotels/styx/testapi/HttpInterceptorContext.java b/support/test-api/src/main/java/com/hotels/styx/testapi/HttpInterceptorContext.java deleted file mode 100644 index 186a64d193..0000000000 --- a/support/test-api/src/main/java/com/hotels/styx/testapi/HttpInterceptorContext.java +++ /dev/null @@ -1,58 +0,0 @@ -/* - Copyright (C) 2013-2018 Expedia Inc. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. - */ -package com.hotels.styx.testapi; - -import com.hotels.styx.api.HttpInterceptor; - -import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; - -/** - * - * An HttpInterceptor.Context implementation for 3rd party plugin tests. - * This is a minimal ConcurrentHashMap backed implementation. - */ -public final class HttpInterceptorContext implements HttpInterceptor.Context { - private final Map context = new ConcurrentHashMap<>(); - private final boolean secure; - - private HttpInterceptorContext(boolean secure) { - this.secure = secure; - } - - @Override - public void add(String key, Object value) { - context.put(key, value); - } - - @Override - public T get(String key, Class clazz) { - return (T) context.get(key); - } - - @Override - public boolean isSecure() { - return secure; - } - - public static HttpInterceptorContext create() { - return new HttpInterceptorContext(false); - } - - public static HttpInterceptorContext create(boolean secure) { - return new HttpInterceptorContext(secure); - } -}