Allow external uri to be configurable for components that support server functionality - #12491 (#12508)

* Allow external uri to be configurable for components that support server functionality.

SE nodes might be behind some sort of proxy exposed to hub on a
different hostname(/ip) and/or port than component would by default
report themselves (e.g.: hub and nodes are in different k8s clusters
 and services are exposed via node ports).

Fixes #12491

* Rename external-uri to external-url.

#12508 (review)

Fixes #12491

* fix linting issues

---------

Co-authored-by: titusfortner <[email protected]>
Co-authored-by: Diego Molina <[email protected]>
Co-authored-by: Titus Fortner <[email protected]>

Author: 3 people

java/src/org/openqa/selenium/grid/server/BaseServerFlags.java

@@ -33,6 +33,15 @@ public class BaseServerFlags implements HasRoles {
 
   private static final String SERVER_SECTION = "server";
 
+  @Parameter(
+      names = {"--external-url"},
+      description =
+          "External URL where component is generally available. "
+              + "Useful on complex network topologies when components are on different networks "
+              + "and proxy servers are involved.")
+  @ConfigValue(section = SERVER_SECTION, name = "external-url", example = "http://10.0.1.1:33333")
+  private String externalUrl;
+
   @Parameter(
       names = {"--host"},
       description = "Server IP or hostname: usually determined automatically.")

java/src/org/openqa/selenium/grid/server/BaseServerOptions.java

@@ -84,28 +84,48 @@ public int getMaxServerThreads() {
 
   @ManagedAttribute(name = "Uri")
   public URI getExternalUri() {
-    // Assume the host given is addressable if it's been set
-    String host =
-        getHostname()
-            .orElseGet(
-                () -> {
-                  try {
-                    return new NetworkUtils().getNonLoopbackAddressOfThisMachine();
-                  } catch (WebDriverException e) {
-                    String name = HostIdentifier.getHostName();
-                    LOG.info("No network connection, guessing name: " + name);
-                    return name;
-                  }
-                });
-
-    int port = getPort();
-
-    try {
-      return new URI(
-          (isSecure() || isSelfSigned()) ? "https" : "http", null, host, port, null, null, null);
-    } catch (URISyntaxException e) {
-      throw new ConfigException("Cannot determine external URI: " + e.getMessage());
-    }
+    return config
+        .get(SERVER_SECTION, "external-url")
+        .map(
+            url -> {
+              try {
+                return new URI(url);
+              } catch (URISyntaxException e) {
+                throw new RuntimeException(
+                    "Supplied external URI is invalid: " + e.getMessage(), e);
+              }
+            })
+        .orElseGet(
+            () -> {
+              // Assume the host given is addressable if it's been set
+              String host =
+                  getHostname()
+                      .orElseGet(
+                          () -> {
+                            try {
+                              return new NetworkUtils().getNonLoopbackAddressOfThisMachine();
+                            } catch (WebDriverException e) {
+                              String name = HostIdentifier.getHostName();
+                              LOG.info("No network connection, guessing name: " + name);
+                              return name;
+                            }
+                          });
+
+              int port = getPort();
+
+              try {
+                return new URI(
+                    (isSecure() || isSelfSigned()) ? "https" : "http",
+                    null,
+                    host,
+                    port,
+                    null,
+                    null,
+                    null);
+              } catch (URISyntaxException e) {
+                throw new ConfigException("Cannot determine external URI: " + e.getMessage());
+              }
+            });
   }
 
   public boolean getAllowCORS() {

java/test/org/openqa/selenium/grid/server/BaseServerOptionsTest.java

@@ -18,7 +18,10 @@
 package org.openqa.selenium.grid.server;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
+import java.net.URI;
+import java.net.URISyntaxException;
 import java.util.Map;
 import org.junit.jupiter.api.Test;
 import org.openqa.selenium.grid.config.MapConfig;
@@ -43,4 +46,54 @@ void serverConfigBindsToHostByDefault() {
 
     assertThat(options.getBindHost()).isEqualTo(true);
   }
+
+  @Test
+  void externalUriFailsForNonUriStrings() {
+    BaseServerOptions options =
+        new BaseServerOptions(new MapConfig(Map.of("server", Map.of("external-url", "not a URL"))));
+
+    Exception exception =
+        assertThrows(
+            RuntimeException.class,
+            () -> {
+              options.getExternalUri();
+            });
+
+    assertThat(exception.getMessage())
+        .as("External URI must be parseable as URI.")
+        .isEqualTo(
+            "Supplied external URI is invalid: Illegal character in path at index 3: not a URL");
+  }
+
+  @Test
+  void externalUriTakesPriorityOverDefaults() throws URISyntaxException {
+    URI expected = new URI("http://10.0.1.1:33333");
+
+    BaseServerOptions options =
+        new BaseServerOptions(
+            new MapConfig(
+                Map.of(
+                    "server",
+                    Map.of(
+                        "external-url", expected.toString(), "host", "localhost", "port", 5555))));
+
+    assertThat(options.getExternalUri()).isEqualTo(expected);
+  }
+
+  @Test
+  void externalUriDefaultsToValueDerivedFromHostnameAndPortWhenNotDefined()
+      throws URISyntaxException {
+    URI expected = new URI("http://localhost:5555");
+
+    BaseServerOptions options =
+        new BaseServerOptions(
+            new MapConfig(
+                Map.of(
+                    "server",
+                    Map.of(
+                        "host", expected.getHost(),
+                        "port", expected.getPort()))));
+
+    assertThat(options.getExternalUri()).isEqualTo(expected);
+  }
 }