Refactor resolvePublishPort to NetworkService

Signed-off-by: Finn Carroll <[email protected]>
finnegancarroll · Oct 31, 2024 · b1b4f25 · b1b4f25
1 parent 03155d5
commit b1b4f25
Show file tree

Hide file tree

Showing 4 changed files with 77 additions and 84 deletions.
diff --git a/server/src/main/java/org/opensearch/common/network/NetworkService.java b/server/src/main/java/org/opensearch/common/network/NetworkService.java
@@ -34,8 +34,11 @@
 
 import org.opensearch.common.settings.Setting;
 import org.opensearch.common.settings.Setting.Property;
+import org.opensearch.common.settings.Settings;
 import org.opensearch.common.unit.TimeValue;
+import org.opensearch.core.common.transport.TransportAddress;
 import org.opensearch.core.common.unit.ByteSizeValue;
+import org.opensearch.transport.BindTransportException;
 
 import java.io.IOException;
 import java.net.InetAddress;
@@ -45,9 +48,13 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
+import java.util.Set;
 import java.util.concurrent.TimeUnit;
 import java.util.function.Function;
 
+import static org.opensearch.grpc.GrpcTransportSettings.SETTING_GRPC_PORT;
+import static org.opensearch.grpc.GrpcTransportSettings.SETTING_GRPC_PUBLISH_PORT;
+
 /**
  * Core network service.
  *
@@ -237,6 +244,43 @@ public InetAddress resolvePublishHostAddresses(String publishHosts[]) throws IOE
         return addresses[0];
     }
 
+    /**
+     * Resolve the publishPort for a server provided a list of boundAddresses and a publishInetAddress.
+     * Resolution strategy is as follows:
+     * If a configured port exists resolve to that port.
+     * If a bound address matches the publishInetAddress resolve to that port.
+     * If a bound address is a wildcard address resolve to that port.
+     * If all bound addresses share the same port resolve to that port.
+     *
+     * @param publishPort -1 if no configured publish port exists
+     * @param boundAddresses addresses bound by the server
+     * @param publishInetAddress address published for the server
+     * @return Resolved port. If publishPort is negative and no port can be resolved return publishPort.
+     */
+    public static int resolvePublishPort(int publishPort, List<TransportAddress> boundAddresses, InetAddress publishInetAddress) {
+        if (publishPort < 0) {
+            for (TransportAddress boundAddress : boundAddresses) {
+                InetAddress boundInetAddress = boundAddress.address().getAddress();
+                if (boundInetAddress.isAnyLocalAddress() || boundInetAddress.equals(publishInetAddress)) {
+                    publishPort = boundAddress.getPort();
+                    break;
+                }
+            }
+        }
+
+        if (publishPort < 0) {
+            final Set<Integer> ports = new HashSet<>();
+            for (TransportAddress boundAddress : boundAddresses) {
+                ports.add(boundAddress.getPort());
+            }
+            if (ports.size() == 1) {
+                publishPort = ports.iterator().next();
+            }
+        }
+
+        return publishPort;
+    }
+
     /** resolves (and deduplicates) host specification */
     private InetAddress[] resolveInetAddresses(String hosts[]) throws IOException {
         if (hosts.length == 0) {

diff --git a/server/src/main/java/org/opensearch/grpc/AbstractGrpcServerTransport.java b/server/src/main/java/org/opensearch/grpc/AbstractGrpcServerTransport.java
@@ -49,9 +49,9 @@
 import java.net.InetSocketAddress;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.HashSet;
 import java.util.List;
-import java.util.Set;
+
+import static org.opensearch.common.network.NetworkService.resolvePublishPort;
 
 import static org.opensearch.grpc.GrpcTransportSettings.SETTING_GRPC_BIND_HOST;
 import static org.opensearch.grpc.GrpcTransportSettings.SETTING_GRPC_MAX_CONTENT_LENGTH;
@@ -140,37 +140,7 @@ protected void bindServer() {
             throw new BindTransportException("Failed to resolve publish address", e);
         }
 
-        final int publishPort = resolvePublishPort(settings, boundAddresses, publishInetAddress);
-        TransportAddress publishAddress = new TransportAddress(new InetSocketAddress(publishInetAddress, publishPort));
-        this.boundAddress = new BoundTransportAddress(boundAddresses.toArray(new TransportAddress[0]), publishAddress);
-        logger.info("{}", boundAddress);
-    }
-
-    // TODO: IDENTICAL TO - HTTP SERVER TRANSPORT
-    static int resolvePublishPort(Settings settings, List<TransportAddress> boundAddresses, InetAddress publishInetAddress) {
-        int publishPort = SETTING_GRPC_PUBLISH_PORT.get(settings);
-
-        if (publishPort < 0) {
-            for (TransportAddress boundAddress : boundAddresses) {
-                InetAddress boundInetAddress = boundAddress.address().getAddress();
-                if (boundInetAddress.isAnyLocalAddress() || boundInetAddress.equals(publishInetAddress)) {
-                    publishPort = boundAddress.getPort();
-                    break;
-                }
-            }
-        }
-
-        // if no matching boundAddress found, check if there is a unique port for all bound addresses
-        if (publishPort < 0) {
-            final Set<Integer> ports = new HashSet<>();
-            for (TransportAddress boundAddress : boundAddresses) {
-                ports.add(boundAddress.getPort());
-            }
-            if (ports.size() == 1) {
-                publishPort = ports.iterator().next();
-            }
-        }
-
+        final int publishPort = resolvePublishPort(SETTING_GRPC_PUBLISH_PORT.get(settings), boundAddresses, publishInetAddress);
         if (publishPort < 0) {
             throw new BindTransportException(
                 "Failed to auto-resolve http publish port, multiple bound addresses "
@@ -184,6 +154,11 @@ static int resolvePublishPort(Settings settings, List<TransportAddress> boundAdd
                     + SETTING_GRPC_PUBLISH_PORT.getKey()
             );
         }
-        return publishPort;
+
+
+
+        TransportAddress publishAddress = new TransportAddress(new InetSocketAddress(publishInetAddress, publishPort));
+        this.boundAddress = new BoundTransportAddress(boundAddresses.toArray(new TransportAddress[0]), publishAddress);
+        logger.info("{}", boundAddress);
     }
 }
diff --git a/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java b/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java
@@ -71,14 +71,15 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.atomic.AtomicReference;
 
+import static org.opensearch.common.network.NetworkService.resolvePublishPort;
+
 import static org.opensearch.http.HttpTransportSettings.SETTING_HTTP_BIND_HOST;
 import static org.opensearch.http.HttpTransportSettings.SETTING_HTTP_MAX_CONTENT_LENGTH;
 import static org.opensearch.http.HttpTransportSettings.SETTING_HTTP_PORT;
@@ -192,7 +193,21 @@ protected void bindServer() {
             throw new BindTransportException("Failed to resolve publish address", e);
         }
 
-        final int publishPort = resolvePublishPort(settings, boundAddresses, publishInetAddress);
+        final int publishPort = resolvePublishPort(SETTING_HTTP_PUBLISH_PORT.get(settings), boundAddresses, publishInetAddress);
+        if (publishPort < 0) {
+            throw new BindHttpException(
+                "Failed to auto-resolve http publish port, multiple bound addresses "
+                    + boundAddresses
+                    + " with distinct ports and none of them matched the publish address ("
+                    + publishInetAddress
+                    + "). "
+                    + "Please specify a unique port by setting "
+                    + SETTING_HTTP_PORT.getKey()
+                    + " or "
+                    + SETTING_HTTP_PUBLISH_PORT.getKey()
+            );
+        }
+
         TransportAddress publishAddress = new TransportAddress(new InetSocketAddress(publishInetAddress, publishPort));
         this.boundAddress = new BoundTransportAddress(boundAddresses.toArray(new TransportAddress[0]), publishAddress);
         logger.info("{}", boundAddress);
@@ -258,47 +273,6 @@ protected void doClose() {}
      */
     protected abstract void stopInternal();
 
-    // package private for tests
-    static int resolvePublishPort(Settings settings, List<TransportAddress> boundAddresses, InetAddress publishInetAddress) {
-        int publishPort = SETTING_HTTP_PUBLISH_PORT.get(settings);
-
-        if (publishPort < 0) {
-            for (TransportAddress boundAddress : boundAddresses) {
-                InetAddress boundInetAddress = boundAddress.address().getAddress();
-                if (boundInetAddress.isAnyLocalAddress() || boundInetAddress.equals(publishInetAddress)) {
-                    publishPort = boundAddress.getPort();
-                    break;
-                }
-            }
-        }
-
-        // if no matching boundAddress found, check if there is a unique port for all bound addresses
-        if (publishPort < 0) {
-            final Set<Integer> ports = new HashSet<>();
-            for (TransportAddress boundAddress : boundAddresses) {
-                ports.add(boundAddress.getPort());
-            }
-            if (ports.size() == 1) {
-                publishPort = ports.iterator().next();
-            }
-        }
-
-        if (publishPort < 0) {
-            throw new BindHttpException(
-                "Failed to auto-resolve http publish port, multiple bound addresses "
-                    + boundAddresses
-                    + " with distinct ports and none of them matched the publish address ("
-                    + publishInetAddress
-                    + "). "
-                    + "Please specify a unique port by setting "
-                    + SETTING_HTTP_PORT.getKey()
-                    + " or "
-                    + SETTING_HTTP_PUBLISH_PORT.getKey()
-            );
-        }
-        return publishPort;
-    }
-
     public void onException(HttpChannel channel, Exception e) {
         channel.handleException(e);
         if (lifecycle.started() == false) {

diff --git a/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java b/server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java
@@ -70,7 +70,7 @@
 
 import static java.net.InetAddress.getByName;
 import static java.util.Arrays.asList;
-import static org.opensearch.http.AbstractHttpServerTransport.resolvePublishPort;
+import static org.opensearch.common.network.NetworkService.resolvePublishPort;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 
@@ -102,21 +102,21 @@ public void testHttpPublishPort() throws Exception {
         int otherBoundPort = randomIntBetween(9200, 9300);
 
         int publishPort = resolvePublishPort(
-            Settings.builder().put(HttpTransportSettings.SETTING_HTTP_PUBLISH_PORT.getKey(), 9080).build(),
+            9080,
             randomAddresses(),
             getByName("127.0.0.2")
         );
         assertThat("Publish port should be explicitly set to 9080", publishPort, equalTo(9080));
 
         publishPort = resolvePublishPort(
-            Settings.EMPTY,
+            -1,
             asList(address("127.0.0.1", boundPort), address("127.0.0.2", otherBoundPort)),
             getByName("127.0.0.1")
         );
         assertThat("Publish port should be derived from matched address", publishPort, equalTo(boundPort));
 
         publishPort = resolvePublishPort(
-            Settings.EMPTY,
+            -1,
             asList(address("127.0.0.1", boundPort), address("127.0.0.2", boundPort)),
             getByName("127.0.0.3")
         );
@@ -125,23 +125,23 @@ public void testHttpPublishPort() throws Exception {
         final BindHttpException e = expectThrows(
             BindHttpException.class,
             () -> resolvePublishPort(
-                Settings.EMPTY,
+                -1,
                 asList(address("127.0.0.1", boundPort), address("127.0.0.2", otherBoundPort)),
                 getByName("127.0.0.3")
             )
         );
         assertThat(e.getMessage(), containsString("Failed to auto-resolve http publish port"));
 
         publishPort = resolvePublishPort(
-            Settings.EMPTY,
+            -1,
             asList(address("0.0.0.0", boundPort), address("127.0.0.2", otherBoundPort)),
             getByName("127.0.0.1")
         );
         assertThat("Publish port should be derived from matching wildcard address", publishPort, equalTo(boundPort));
 
         if (NetworkUtils.SUPPORTS_V6) {
             publishPort = resolvePublishPort(
-                Settings.EMPTY,
+                -1,
                 asList(address("0.0.0.0", boundPort), address("127.0.0.2", otherBoundPort)),
                 getByName("::1")
             );