[grid] Adding a "bind-host" flag

diemol · diemol · commit 40a2977b90ae · 2021-12-15T14:17:39.000+01:00
#9981 fixed a regression where the Grid server was binding to all interfaces, even if a host was being configured. However, this was not ideal for environments like Docker, where the host value can be the reachable IP of the machine where the containers are running, but we cannot bind to that IP inside the container. This adds a flag where the user can decide if the server should bind to the host or not, true by default to keep the current behaviour, but it will be configured to false by default in the docker-selenium project. This is a partial fix for SeleniumHQ/docker-selenium#1453
diff --git a/java/src/org/openqa/selenium/grid/server/BaseServerFlags.java b/java/src/org/openqa/selenium/grid/server/BaseServerFlags.java
@@ -42,6 +42,16 @@ public class BaseServerFlags implements HasRoles {
   @ConfigValue(section = SERVER_SECTION, name = "host", example = "\"localhost\"")
   private String host;
 
+  @Parameter(
+    names = "--bind-host",
+    description = "Whether the server should bind to the host address/name, or only use it to" +
+                  " report its reachable url. Helpful in complex network topologies where the" +
+                  " server cannot report itself with the current IP/hostname but rather an" +
+                  " external IP or hostname (e.g. inside a Docker container).",
+    arity = 1)
+  @ConfigValue(section = SERVER_SECTION, name = "bind-host", example = "true")
+  private Boolean bindHost = true;
+
   @Parameter(
     description = "Port to listen on. There is no default as this parameter is used by "
                   + "different components, for example Router/Hub/Standalone will use 4444 and "
diff --git a/java/src/org/openqa/selenium/grid/server/BaseServerOptions.java b/java/src/org/openqa/selenium/grid/server/BaseServerOptions.java
@@ -115,6 +115,10 @@ public boolean getAllowCORS() {
     return config.getBool(SERVER_SECTION, "allow-cors").orElse(false);
   }
 
+  public boolean getBindHost() {
+    return config.getBool(SERVER_SECTION, "bind-host").orElse(true);
+  }
+
   public boolean isSecure() {
     return config.get(SERVER_SECTION, "https-private-key").isPresent()
            && config.get(SERVER_SECTION, "https-certificate").isPresent();
diff --git a/java/src/org/openqa/selenium/netty/server/NettyServer.java b/java/src/org/openqa/selenium/netty/server/NettyServer.java
@@ -17,7 +17,14 @@
 
 package org.openqa.selenium.netty.server;
 
-import java.net.InetSocketAddress;
+import org.openqa.selenium.grid.server.BaseServerOptions;
+import org.openqa.selenium.grid.server.Server;
+import org.openqa.selenium.internal.Require;
+import org.openqa.selenium.remote.AddWebDriverSpecHeaders;
+import org.openqa.selenium.remote.ErrorFilter;
+import org.openqa.selenium.remote.http.HttpHandler;
+import org.openqa.selenium.remote.http.Message;
+
 import io.netty.bootstrap.ServerBootstrap;
 import io.netty.channel.Channel;
 import io.netty.channel.EventLoopGroup;
@@ -30,31 +37,27 @@
 import io.netty.handler.ssl.util.SelfSignedCertificate;
 import io.netty.util.internal.logging.InternalLoggerFactory;
 import io.netty.util.internal.logging.JdkLoggerFactory;
-import org.openqa.selenium.remote.AddWebDriverSpecHeaders;
-import org.openqa.selenium.grid.server.BaseServerOptions;
-import org.openqa.selenium.grid.server.Server;
-import org.openqa.selenium.remote.ErrorFilter;
-import org.openqa.selenium.internal.Require;
-import org.openqa.selenium.remote.http.HttpHandler;
-import org.openqa.selenium.remote.http.Message;
 
-import javax.net.ssl.SSLException;
 import java.io.IOException;
 import java.io.UncheckedIOException;
 import java.net.BindException;
+import java.net.InetSocketAddress;
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.security.cert.CertificateException;
 import java.util.Optional;
 import java.util.function.BiFunction;
 import java.util.function.Consumer;
 
+import javax.net.ssl.SSLException;
+
 public class NettyServer implements Server<NettyServer> {
 
   private final EventLoopGroup bossGroup;
   private final EventLoopGroup workerGroup;
   private final int port;
   private final String host;
+  private final boolean bindHost;
   private final URL externalUrl;
   private final HttpHandler handler;
   private final BiFunction<String, Consumer<Message>, Optional<Consumer<Message>>> websocketHandler;
@@ -106,6 +109,7 @@ public NettyServer(
 
     port = options.getPort();
     host = options.getHostname().orElse("0.0.0.0");
+    bindHost = options.getBindHost();
     allowCors = options.getAllowCORS();
 
     try {
@@ -150,7 +154,11 @@ public NettyServer start() {
       .childHandler(new SeleniumHttpInitializer(sslCtx, handler, websocketHandler, allowCors));
 
     try {
-      channel = b.bind(new InetSocketAddress(host, port)).sync().channel();
+      // Using a flag to avoid binding to the host, useful in environments like Docker,
+      // where the "host" value can be the IP of the Docker host machine, which cannot
+      // be bind inside the container.
+      channel = bindHost ? b.bind(new InetSocketAddress(host, port)).sync().channel() :
+                b.bind(port).sync().channel();
     } catch (InterruptedException e) {
       Thread.currentThread().interrupt();
       throw new UncheckedIOException(new IOException("Start up interrupted", e));
diff --git a/java/test/org/openqa/selenium/grid/server/BaseServerOptionsTest.java b/java/test/org/openqa/selenium/grid/server/BaseServerOptionsTest.java
@@ -17,23 +17,33 @@
 
 package org.openqa.selenium.grid.server;
 
+import static org.assertj.core.api.Assertions.assertThat;
+
 import org.junit.Test;
 import org.openqa.selenium.grid.config.MapConfig;
 
 import java.util.Map;
 
-import static org.assertj.core.api.Assertions.assertThat;
-
 public class BaseServerOptionsTest {
 
   @Test
   public void readingThePortTwiceShouldGiveTheSameResult() {
-    BaseServerOptions options = new BaseServerOptions(new MapConfig(Map.of("server", Map.of("port", -1))));
+    BaseServerOptions options = new BaseServerOptions(
+      new MapConfig(Map.of("server", Map.of("port", -1))));
 
     int first = options.getPort();
     int second = options.getPort();
 
     assertThat(first).isEqualTo(second);
   }
 
+  @Test
+  public void serverConfigBindsToHostByDefault() {
+    BaseServerOptions options = new BaseServerOptions(
+      new MapConfig(Map.of("server", Map.of("port", 4444))));
+
+    assertThat(options.getBindHost()).isEqualTo(true);
+  }
+
+
 }
diff --git a/java/test/org/openqa/selenium/netty/server/NettyServerTest.java b/java/test/org/openqa/selenium/netty/server/NettyServerTest.java
@@ -19,6 +19,7 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.openqa.selenium.remote.http.Contents.utf8String;
@@ -107,7 +108,6 @@ public void shouldAllowCORS() {
     BaseServerOptions options = new BaseServerOptions(cfg);
     assertTrue("Allow CORS should be enabled", options.getAllowCORS());
 
-    // TODO: Server setup
     Server<?> server = new NettyServer(
       options,
       req -> new HttpResponse()
@@ -126,6 +126,22 @@ public void shouldAllowCORS() {
       response.getHeader("Access-Control-Allow-Origin"));
   }
 
+  @Test
+  public void shouldNotBindToHost() {
+    Config cfg = new CompoundConfig(
+      new MapConfig(ImmutableMap.of("server", ImmutableMap.of(
+        "bind-host", "false", "host", "anyRandomHost"))));
+    BaseServerOptions options = new BaseServerOptions(cfg);
+    assertFalse("Bind to host should be disabled", options.getBindHost());
+
+    Server<?> server = new NettyServer(
+      options,
+      req -> new HttpResponse()
+    ).start();
+
+    assertEquals("anyRandomHost", server.getUrl().getHost());
+  }
+
   private void outputHeaders(HttpResponse res) {
     res.getHeaderNames()
       .forEach(name ->
