Merge pull request #10913 from vespa-engine/bjorncs/jdisc-access-logging

Bjorncs/jdisc access logging

4 files changed, 30 insertions, 218 deletions

diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLog.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLog.java
index 9a10c70ceab..0074f5cfe89 100644
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLog.java
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLog.java
@@ -31,6 +31,7 @@ public class AccessLogRequestLog extends AbstractLifeCycle implements RequestLog
 
     // TODO These hardcoded headers should be provided by config instead
     private static final String HEADER_NAME_X_FORWARDED_FOR = "x-forwarded-for";
+    private static final String HEADER_NAME_X_FORWARDED_PORT = "X-Forwarded-Port";
     private static final String HEADER_NAME_Y_RA = "y-ra";
     private static final String HEADER_NAME_Y_RP = "y-rp";
     private static final String HEADER_NAME_YAHOOREMOTEIP = "yahooremoteip";
@@ -58,23 +59,24 @@ public class AccessLogRequestLog extends AbstractLifeCycle implements RequestLog
                 accessLogEntry.setRawQuery(queryString);
             }
 
-            final String remoteAddress = getRemoteAddress(request);
-            final int remotePort = getRemotePort(request);
-            final String peerAddress = request.getRemoteAddr();
-            final int peerPort = request.getRemotePort();
-
             accessLogEntry.setUserAgent(request.getHeader("User-Agent"));
             accessLogEntry.setHttpMethod(request.getMethod());
             accessLogEntry.setHostString(request.getHeader("Host"));
             accessLogEntry.setReferer(request.getHeader("Referer"));
+
+            String peerAddress = request.getRemoteAddr();
             accessLogEntry.setIpV4Address(peerAddress);
-            accessLogEntry.setRemoteAddress(remoteAddress);
-            accessLogEntry.setRemotePort(remotePort);
+            accessLogEntry.setPeerAddress(peerAddress);
+            String remoteAddress = getRemoteAddress(request);
             if (!Objects.equal(remoteAddress, peerAddress)) {
-                accessLogEntry.setPeerAddress(peerAddress);
+                accessLogEntry.setRemoteAddress(remoteAddress);
             }
+
+            int peerPort = request.getRemotePort();
+            accessLogEntry.setPeerPort(peerPort);
+            int remotePort = getRemotePort(request);
             if (remotePort != peerPort) {
-                accessLogEntry.setPeerPort(peerPort);
+                accessLogEntry.setRemotePort(remotePort);
             }
             accessLogEntry.setHttpVersion(request.getProtocol());
             accessLogEntry.setScheme(request.getScheme());
@@ -118,15 +120,16 @@ public class AccessLogRequestLog extends AbstractLifeCycle implements RequestLog
     }
 
     private static String getRemoteAddress(final HttpServletRequest request) {
-        return Alternative.preferred(request.getHeader(HEADER_NAME_X_FORWARDED_FOR))
-                .alternatively(() -> request.getHeader(HEADER_NAME_Y_RA))
-                .alternatively(() -> request.getHeader(HEADER_NAME_YAHOOREMOTEIP))
-                .alternatively(() -> request.getHeader(HEADER_NAME_CLIENT_IP))
+        return Optional.ofNullable(request.getHeader(HEADER_NAME_X_FORWARDED_FOR))
+                .or(() -> Optional.ofNullable(request.getHeader(HEADER_NAME_Y_RA)))
+                .or(() -> Optional.ofNullable(request.getHeader(HEADER_NAME_YAHOOREMOTEIP)))
+                .or(() -> Optional.ofNullable(request.getHeader(HEADER_NAME_CLIENT_IP)))
                 .orElseGet(request::getRemoteAddr);
     }
 
     private static int getRemotePort(final HttpServletRequest request) {
-        return Optional.ofNullable(request.getHeader(HEADER_NAME_Y_RP))
+        return Optional.ofNullable(request.getHeader(HEADER_NAME_X_FORWARDED_PORT))
+                .or(() -> Optional.ofNullable(request.getHeader(HEADER_NAME_Y_RP)))
                 .map(Integer::valueOf)
                 .orElseGet(request::getRemotePort);
     }
diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/Alternative.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/Alternative.java
deleted file mode 100644
index 441082e95c1..00000000000
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/Alternative.java
+++ /dev/null
@@ -1,68 +0,0 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-package com.yahoo.jdisc.http.server.jetty;
-
-import java.util.Objects;
-import java.util.function.Supplier;
-
-/**
- * Simple monad class, like Optional but with support for chaining alternatives in preferred order.
- *
- * Holds a current value (immutably), but if the current value is null provides an easy way to obtain an instance
- * with another value, ad infinitum.
- *
- * Instances of this class are immutable and thread-safe.
- *
- * @author bakksjo
- */
-public class Alternative<T> {
-    private final T value;
-
-    private Alternative(final T value) {
-        this.value = value;
-    }
-
-    /**
-     * Creates an instance with the supplied value.
-     */
-    public static <T> Alternative<T> preferred(final T value) {
-        return new Alternative<>(value);
-    }
-
-    /**
-     * Returns itself (unchanged) iff current value != null,
-     * otherwise returns a new instance with the value supplied by the supplier.
-     */
-    public Alternative<T> alternatively(final Supplier<? extends T> supplier) {
-        if (value != null) {
-            return this;
-        }
-
-        return new Alternative<>(supplier.get());
-    }
-
-    /**
-     * Returns the held value iff != null, otherwise invokes the supplier and returns its value.
-     */
-    public T orElseGet(final Supplier<? extends T> supplier) {
-        if (value != null) {
-            return value;
-        }
-        return supplier.get();
-    }
-
-    @Override
-    public boolean equals(final Object o) {
-        if (!(o instanceof Alternative<?>)) {
-            return false;
-        }
-
-        final Alternative<?> other = (Alternative<?>) o;
-
-        return Objects.equals(value, other.value);
-    }
-
-    @Override
-    public int hashCode() {
-        return Objects.hashCode(value);
-    }
-}
diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLogTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLogTest.java
index 3a605040742..580533be4c3 100644
--- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLogTest.java
+++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLogTest.java
@@ -82,6 +82,19 @@ public class AccessLogRequestLogTest {
         assertThat(accessLogEntry.getRemoteAddress(), is("1.2.3.4"));
     }
 
+    @Test
+    public void verify_x_forwarded_port_precedence () {
+        AccessLogEntry accessLogEntry = new AccessLogEntry();
+        Request jettyRequest = createRequestMock(accessLogEntry);
+        when(jettyRequest.getRequestURI()).thenReturn("//search/");
+        when(jettyRequest.getQueryString()).thenReturn("q=%%2");
+        when(jettyRequest.getHeader("X-Forwarded-Port")).thenReturn("80");
+        when(jettyRequest.getHeader("y-rp")).thenReturn("8080");
+
+        new AccessLogRequestLog(mock(AccessLog.class)).log(jettyRequest, createResponseMock());
+        assertThat(accessLogEntry.getRemotePort(), is(80));
+    }
+
     private static Request createRequestMock(AccessLogEntry entry) {
         Request request = mock(Request.class);
         when(request.getAttribute(JDiscHttpServlet.ATTRIBUTE_NAME_ACCESS_LOG_ENTRY)).thenReturn(entry);
diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/AlternativeTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/AlternativeTest.java
deleted file mode 100644
index 966c8d418b1..00000000000
--- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/AlternativeTest.java
+++ /dev/null
@@ -1,136 +0,0 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-package com.yahoo.jdisc.http.server.jetty;
-
-import org.junit.Test;
-
-import java.util.Set;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
-
-import static org.hamcrest.CoreMatchers.equalTo;
-import static org.hamcrest.CoreMatchers.is;
-import static org.hamcrest.CoreMatchers.not;
-import static org.hamcrest.CoreMatchers.nullValue;
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.greaterThan;
-
-/**
- * @author bakksjo
- */
-public class AlternativeTest {
-    private static final String MAN = "man";
-    private static final String BEAR = "bear";
-    private static final String PIG = "pig";
-
-    @Test
-    public void singleValue() {
-        assertThat(
-                Alternative.preferred(MAN)
-                        .orElseGet(() -> BEAR),
-                is(MAN));
-    }
-
-    @Test
-    public void singleNull() {
-        assertThat(
-                Alternative.preferred(null)
-                        .orElseGet(() -> BEAR),
-                is(BEAR));
-    }
-
-    @Test
-    public void twoValues() {
-        assertThat(
-                Alternative.preferred(MAN)
-                        .alternatively(() -> BEAR)
-                        .orElseGet(() -> PIG),
-                is(MAN));
-    }
-
-    @Test
-    public void oneNullOneValue() {
-        assertThat(
-                Alternative.preferred(null)
-                        .alternatively(() -> MAN)
-                        .orElseGet(() -> BEAR),
-                is(MAN));
-    }
-
-    @Test
-    public void twoNulls() {
-        assertThat(
-                Alternative.preferred(null)
-                        .alternatively(() -> null)
-                        .orElseGet(() -> MAN),
-                is(MAN));
-    }
-
-    @Test
-    public void singleNullLastResortIsNull() {
-        assertThat(
-                Alternative.preferred(null)
-                        .orElseGet(() -> null),
-                is(nullValue()));
-    }
-
-    @Test
-    public void twoNullsLastResortIsNull() {
-        assertThat(
-                Alternative.preferred(null)
-                        .alternatively(() -> null)
-                        .orElseGet(() -> null),
-                is(nullValue()));
-    }
-
-    @Test
-    public void oneNullTwoValues() {
-        assertThat(
-                Alternative.preferred(null)
-                        .alternatively(() -> MAN)
-                        .alternatively(() -> BEAR)
-                        .orElseGet(() -> PIG),
-                is(MAN));
-    }
-
-    @Test
-    public void equalValuesMakeEqualAlternatives() {
-        assertThat(Alternative.preferred(MAN), is(equalTo(Alternative.preferred(MAN))));
-        assertThat(Alternative.preferred(BEAR), is(equalTo(Alternative.preferred(BEAR))));
-        assertThat(Alternative.preferred(PIG), is(equalTo(Alternative.preferred(PIG))));
-        assertThat(Alternative.preferred(null), is(equalTo(Alternative.preferred(null))));
-    }
-
-    @Test
-    public void equalValuesMakeEqualHashCodes() {
-        assertThat(Alternative.preferred(MAN).hashCode(), is(equalTo(Alternative.preferred(MAN).hashCode())));
-        assertThat(Alternative.preferred(BEAR).hashCode(), is(equalTo(Alternative.preferred(BEAR).hashCode())));
-        assertThat(Alternative.preferred(PIG).hashCode(), is(equalTo(Alternative.preferred(PIG).hashCode())));
-        assertThat(Alternative.preferred(null).hashCode(), is(equalTo(Alternative.preferred(null).hashCode())));
-    }
-
-    @Test
-    public void unequalValuesMakeUnequalAlternatives() {
-        assertThat(Alternative.preferred(MAN), is(not(equalTo(Alternative.preferred(BEAR)))));
-        assertThat(Alternative.preferred(MAN), is(not(equalTo(Alternative.preferred(PIG)))));
-        assertThat(Alternative.preferred(MAN), is(not(equalTo(Alternative.preferred(null)))));
-        assertThat(Alternative.preferred(BEAR), is(not(equalTo(Alternative.preferred(MAN)))));
-        assertThat(Alternative.preferred(BEAR), is(not(equalTo(Alternative.preferred(PIG)))));
-        assertThat(Alternative.preferred(BEAR), is(not(equalTo(Alternative.preferred(null)))));
-        assertThat(Alternative.preferred(PIG), is(not(equalTo(Alternative.preferred(MAN)))));
-        assertThat(Alternative.preferred(PIG), is(not(equalTo(Alternative.preferred(BEAR)))));
-        assertThat(Alternative.preferred(PIG), is(not(equalTo(Alternative.preferred(null)))));
-        assertThat(Alternative.preferred(null), is(not(equalTo(Alternative.preferred(MAN)))));
-        assertThat(Alternative.preferred(null), is(not(equalTo(Alternative.preferred(BEAR)))));
-        assertThat(Alternative.preferred(null), is(not(equalTo(Alternative.preferred(PIG)))));
-    }
-
-    @Test
-    public void hashValuesAreDecent() {
-        final String[] animals = { MAN, BEAR, PIG, "squirrel", "aardvark", "porcupine", "sasquatch", null };
-        final Set<Integer> hashCodes = Stream.of(animals)
-                .map(Alternative::preferred)
-                .map(Alternative::hashCode)
-                .collect(Collectors.toSet());
-        assertThat(hashCodes.size(), is(greaterThan(animals.length / 2)));  // A modest requirement.
-    }
-}