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

Don't use request headers for remote address/port in hosted Vespa

3 files changed, 34 insertions, 26 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 7085f07585a..e8fd92f8e19 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
@@ -4,6 +4,7 @@ package com.yahoo.jdisc.http.server.jetty;
 import com.google.common.base.Objects;
 import com.yahoo.container.logging.AccessLog;
 import com.yahoo.container.logging.AccessLogEntry;
+import com.yahoo.jdisc.http.ServerConfig;
 import com.yahoo.jdisc.http.servlet.ServletRequest;
 import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.RequestLog;
@@ -15,6 +16,7 @@ import java.security.Principal;
 import java.security.cert.X509Certificate;
 import java.util.List;
 import java.util.Optional;
+import java.util.OptionalInt;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -27,25 +29,21 @@ import static com.yahoo.jdisc.http.core.HttpServletRequestUtils.getConnectorLoca
  * @author Oyvind Bakksjo
  * @author bjorncs
  */
-public class AccessLogRequestLog extends AbstractLifeCycle implements RequestLog {
+class AccessLogRequestLog extends AbstractLifeCycle implements RequestLog {
 
     private static final Logger logger = Logger.getLogger(AccessLogRequestLog.class.getName());
 
-    // 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";
-    private static final String HEADER_NAME_CLIENT_IP = "client-ip";
-
     // HTTP headers that are logged as extra key-value-pairs in access log entries
     private static final List<String> LOGGED_REQUEST_HEADERS = List.of("Vespa-Client-Version");
 
     private final AccessLog accessLog;
+    private final List<String> remoteAddressHeaders;
+    private final List<String> remotePortHeaders;
 
-    public AccessLogRequestLog(AccessLog accessLog) {
+    AccessLogRequestLog(AccessLog accessLog, ServerConfig.AccessLog config) {
         this.accessLog = accessLog;
+        this.remoteAddressHeaders = config.remoteAddressHeaders();
+        this.remotePortHeaders = config.remotePortHeaders();
     }
 
     @Override
@@ -121,26 +119,30 @@ public class AccessLogRequestLog extends AbstractLifeCycle implements RequestLog
         }
     }
 
-    private static String getRemoteAddress(HttpServletRequest request) {
-        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 String getRemoteAddress(HttpServletRequest request) {
+        for (String header : remoteAddressHeaders) {
+            String value = request.getHeader(header);
+            if (value != null) return value;
+        }
+        return request.getRemoteAddr();
     }
 
-    private static int getRemotePort(HttpServletRequest request) {
-        return Optional.ofNullable(request.getHeader(HEADER_NAME_X_FORWARDED_PORT))
-                .or(() -> Optional.ofNullable(request.getHeader(HEADER_NAME_Y_RP)))
-                .flatMap(AccessLogRequestLog::parsePort)
-                .orElseGet(request::getRemotePort);
+    private int getRemotePort(HttpServletRequest request) {
+        for (String header : remotePortHeaders) {
+            String value = request.getHeader(header);
+            if (value != null) {
+                OptionalInt maybePort = parsePort(value);
+                if (maybePort.isPresent()) return maybePort.getAsInt();
+            }
+        }
+        return request.getRemotePort();
     }
 
-    private static Optional<Integer> parsePort(String port) {
+    private static OptionalInt parsePort(String port) {
         try {
-            return Optional.of(Integer.valueOf(port));
+            return OptionalInt.of(Integer.parseInt(port));
         } catch (IllegalArgumentException e) {
-            return Optional.empty();
+            return OptionalInt.empty();
         }
     }
 
diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java
index c826f52a865..45dc9144ffe 100644
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java
@@ -147,7 +147,7 @@ public class JettyHttpServer extends AbstractServerProvider {
 
         server = new Server();
         server.setStopTimeout((long)(serverConfig.stopTimeout() * 1000.0));
-        server.setRequestLog(new AccessLogRequestLog(accessLog));
+        server.setRequestLog(new AccessLogRequestLog(accessLog, serverConfig.accessLog()));
         setupJmx(server, serverConfig);
         ((QueuedThreadPool)server.getThreadPool()).setMaxThreads(serverConfig.maxWorkerThreads());
 
diff --git a/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.jdisc.http.server.def b/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.jdisc.http.server.def
index 0cb5b89b20c..3118a7dea64 100644
--- a/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.jdisc.http.server.def
+++ b/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.jdisc.http.server.def
@@ -43,4 +43,10 @@ jmx.listenPort                        int    default = 1099
 metric.monitoringHandlerPaths[]       string
 
 # Paths that should be reported with search dimensions where applicable
-metric.searchHandlerPaths[]           string
\ No newline at end of file
+metric.searchHandlerPaths[]           string
+
+# HTTP request headers that contain remote address
+accessLog.remoteAddressHeaders[]      string
+
+# HTTP request headers that contain remote port
+accessLog.remotePortHeaders[]         string