From 945e4f4a67e64e444b792c0283811285d334fc4e Mon Sep 17 00:00:00 2001 From: Tamas Utasi <3823780+utamas@users.noreply.github.com> Date: Mon, 6 Nov 2023 10:00:23 +0100 Subject: [PATCH] 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. https://github.com/SeleniumHQ/selenium/pull/12508#pullrequestreview-1627203552 Fixes #12491 * fix linting issues --------- Co-authored-by: titusfortner Co-authored-by: Diego Molina Co-authored-by: Titus Fortner --- .../selenium/grid/server/BaseServerFlags.java | 9 +++ .../grid/server/BaseServerOptions.java | 64 ++++++++++++------- .../grid/server/BaseServerOptionsTest.java | 53 +++++++++++++++ 3 files changed, 104 insertions(+), 22 deletions(-) diff --git a/java/src/org/openqa/selenium/grid/server/BaseServerFlags.java b/java/src/org/openqa/selenium/grid/server/BaseServerFlags.java index 1ee454d9ab798..c022026580252 100644 --- a/java/src/org/openqa/selenium/grid/server/BaseServerFlags.java +++ b/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.") diff --git a/java/src/org/openqa/selenium/grid/server/BaseServerOptions.java b/java/src/org/openqa/selenium/grid/server/BaseServerOptions.java index 7c5fb99a2909d..73347a593c5d0 100644 --- a/java/src/org/openqa/selenium/grid/server/BaseServerOptions.java +++ b/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() { diff --git a/java/test/org/openqa/selenium/grid/server/BaseServerOptionsTest.java b/java/test/org/openqa/selenium/grid/server/BaseServerOptionsTest.java index 3a94f8ab6aa53..7aea4479e9300 100644 --- a/java/test/org/openqa/selenium/grid/server/BaseServerOptionsTest.java +++ b/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); + } }