Replae redundant resolvePublishPort() with helper.

Signed-off-by: Finn Carroll <[email protected]>

plugins/transport-grpc/src/main/java/org/opensearch/transport/grpc/Netty4GrpcServerTransport.java

@@ -44,10 +44,10 @@
 import io.grpc.protobuf.services.ProtoReflectionService;
 
 import static java.util.Collections.emptyList;
-import static org.opensearch.common.network.NetworkService.resolvePublishPort;
 import static org.opensearch.common.settings.Setting.intSetting;
 import static org.opensearch.common.settings.Setting.listSetting;
 import static org.opensearch.common.util.concurrent.OpenSearchExecutors.daemonThreadFactory;
+import static org.opensearch.transport.TcpTransport.resolveTransportPublishPort;
 
 public class Netty4GrpcServerTransport extends NetworkPlugin.AuxTransport {
     private static final Logger logger = LogManager.getLogger(Netty4GrpcServerTransport.class);
@@ -185,7 +185,7 @@ private void bindServer() {
             throw new BindTransportException("Failed to resolve publish address", e);
         }
 
-        final int publishPort = resolvePublishPort(SETTING_GRPC_PUBLISH_PORT.get(settings), boundAddresses, publishInetAddress);
+        final int publishPort = resolveTransportPublishPort(SETTING_GRPC_PUBLISH_PORT.get(settings), boundAddresses, publishInetAddress);
         if (publishPort < 0) {
             throw new BindTransportException(
                 "Failed to auto-resolve grpc publish port, multiple bound addresses "

server/src/main/java/org/opensearch/common/network/NetworkService.java

@@ -35,7 +35,6 @@
 import org.opensearch.common.settings.Setting;
 import org.opensearch.common.settings.Setting.Property;
 import org.opensearch.common.unit.TimeValue;
-import org.opensearch.core.common.transport.TransportAddress;
 import org.opensearch.core.common.unit.ByteSizeValue;
 
 import java.io.IOException;
@@ -46,7 +45,6 @@
 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;
 
@@ -239,43 +237,6 @@ 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) {

server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java

@@ -78,12 +78,12 @@
 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;
 import static org.opensearch.http.HttpTransportSettings.SETTING_HTTP_PUBLISH_HOST;
 import static org.opensearch.http.HttpTransportSettings.SETTING_HTTP_PUBLISH_PORT;
+import static org.opensearch.transport.TcpTransport.resolveTransportPublishPort;
 
 /**
  * Base HttpServer class
@@ -192,7 +192,7 @@ protected void bindServer() {
             throw new BindTransportException("Failed to resolve publish address", e);
         }
 
-        final int publishPort = resolvePublishPort(SETTING_HTTP_PUBLISH_PORT.get(settings), boundAddresses, publishInetAddress);
+        final int publishPort = resolveTransportPublishPort(SETTING_HTTP_PUBLISH_PORT.get(settings), boundAddresses, publishInetAddress);
         if (publishPort < 0) {
             throw new BindHttpException(
                 "Failed to auto-resolve http publish port, multiple bound addresses "

server/src/main/java/org/opensearch/transport/TcpTransport.java

@@ -521,16 +521,42 @@ private BoundTransportAddress createBoundTransportAddress(ProfileSettings profil
             throw new BindTransportException("Failed to resolve publish address", e);
         }
 
-        final int publishPort = resolvePublishPort(profileSettings, boundAddresses, publishInetAddress);
+        final int publishPort = resolvePublishPort(profileSettings.publishPort, boundAddresses, publishInetAddress);
+        if (publishPort == -1) {
+            String profileExplanation = profileSettings.isDefaultProfile ? "" : " for profile " + profileSettings.profileName;
+            throw new BindTransportException(
+                "Failed to auto-resolve publish port"
+                    + profileExplanation
+                    + ", multiple bound addresses "
+                    + boundAddresses
+                    + " with distinct ports and none of them matched the publish address ("
+                    + publishInetAddress
+                    + "). "
+                    + "Please specify a unique port by setting "
+                    + TransportSettings.PORT.getKey()
+                    + " or "
+                    + TransportSettings.PUBLISH_PORT.getKey()
+            );
+        }
+
         final TransportAddress publishAddress = new TransportAddress(new InetSocketAddress(publishInetAddress, publishPort));
         return new BoundTransportAddress(transportBoundAddresses, publishAddress);
     }
 
-    // package private for tests
-    static int resolvePublishPort(ProfileSettings profileSettings, List<InetSocketAddress> boundAddresses, InetAddress publishInetAddress) {
-        int publishPort = profileSettings.publishPort;
-
-        // if port not explicitly provided, search for port of address in boundAddresses that matches publishInetAddress
+    /**
+     * 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<InetSocketAddress> boundAddresses, InetAddress publishInetAddress) {
         if (publishPort < 0) {
             for (InetSocketAddress boundAddress : boundAddresses) {
                 InetAddress boundInetAddress = boundAddress.getAddress();
@@ -541,7 +567,6 @@ static int resolvePublishPort(ProfileSettings profileSettings, List<InetSocketAd
             }
         }
 
-        // 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 (InetSocketAddress boundAddress : boundAddresses) {
@@ -552,25 +577,17 @@ static int resolvePublishPort(ProfileSettings profileSettings, List<InetSocketAd
             }
         }
 
-        if (publishPort < 0) {
-            String profileExplanation = profileSettings.isDefaultProfile ? "" : " for profile " + profileSettings.profileName;
-            throw new BindTransportException(
-                "Failed to auto-resolve publish port"
-                    + profileExplanation
-                    + ", multiple bound addresses "
-                    + boundAddresses
-                    + " with distinct ports and none of them matched the publish address ("
-                    + publishInetAddress
-                    + "). "
-                    + "Please specify a unique port by setting "
-                    + TransportSettings.PORT.getKey()
-                    + " or "
-                    + TransportSettings.PUBLISH_PORT.getKey()
-            );
-        }
         return publishPort;
     }
 
+    public static int resolveTransportPublishPort(int publishPort, List<TransportAddress> boundAddresses, InetAddress publishInetAddress) {
+        return resolvePublishPort(
+            publishPort,
+            boundAddresses.stream().map(TransportAddress::address).collect(Collectors.toList()),
+            publishInetAddress
+        );
+    }
+
     @Override
     public TransportAddress[] addressesFromString(String address) throws UnknownHostException {
         return parse(address, defaultPortRange()[0]);

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.common.network.NetworkService.resolvePublishPort;
+import static org.opensearch.transport.TcpTransport.resolveTransportPublishPort;
 import static org.hamcrest.Matchers.equalTo;
 
 public class AbstractHttpServerTransportTests extends OpenSearchTestCase {
@@ -100,39 +100,39 @@ public void testHttpPublishPort() throws Exception {
         int boundPort = randomIntBetween(9000, 9100);
         int otherBoundPort = randomIntBetween(9200, 9300);
 
-        int publishPort = resolvePublishPort(9080, randomAddresses(), getByName("127.0.0.2"));
+        int publishPort = resolveTransportPublishPort(9080, randomAddresses(), getByName("127.0.0.2"));
         assertThat("Publish port should be explicitly set to 9080", publishPort, equalTo(9080));
 
-        publishPort = resolvePublishPort(
+        publishPort = resolveTransportPublishPort(
             -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(
+        publishPort = resolveTransportPublishPort(
             -1,
             asList(address("127.0.0.1", boundPort), address("127.0.0.2", boundPort)),
             getByName("127.0.0.3")
         );
         assertThat("Publish port should be derived from unique port of bound addresses", publishPort, equalTo(boundPort));
 
-        publishPort = resolvePublishPort(
+        publishPort = resolveTransportPublishPort(
             -1,
             asList(address("127.0.0.1", boundPort), address("127.0.0.2", otherBoundPort)),
             getByName("127.0.0.3")
         );
         assertThat(publishPort, equalTo(-1));
 
-        publishPort = resolvePublishPort(
+        publishPort = resolveTransportPublishPort(
             -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(
+            publishPort = resolveTransportPublishPort(
                 -1,
                 asList(address("0.0.0.0", boundPort), address("127.0.0.2", otherBoundPort)),
                 getByName("::1")

server/src/test/java/org/opensearch/transport/PublishPortTests.java

@@ -74,47 +74,43 @@ public void testPublishPort() throws Exception {
         }
 
         int publishPort = resolvePublishPort(
-            new TcpTransport.ProfileSettings(settings, profile),
+            new TcpTransport.ProfileSettings(settings, profile).publishPort,
             randomAddresses(),
             getByName("127.0.0.2")
         );
         assertThat("Publish port should be explicitly set", publishPort, equalTo(useProfile ? 9080 : 9081));
 
         publishPort = resolvePublishPort(
-            new TcpTransport.ProfileSettings(baseSettings, profile),
+            new TcpTransport.ProfileSettings(baseSettings, profile).publishPort,
             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(
-            new TcpTransport.ProfileSettings(baseSettings, profile),
+            new TcpTransport.ProfileSettings(baseSettings, profile).publishPort,
             asList(address("127.0.0.1", boundPort), address("127.0.0.2", boundPort)),
             getByName("127.0.0.3")
         );
         assertThat("Publish port should be derived from unique port of bound addresses", publishPort, equalTo(boundPort));
 
-        try {
-            resolvePublishPort(
-                new TcpTransport.ProfileSettings(baseSettings, profile),
-                asList(address("127.0.0.1", boundPort), address("127.0.0.2", otherBoundPort)),
-                getByName("127.0.0.3")
-            );
-            fail("Expected BindTransportException as publish_port not specified and non-unique port of bound addresses");
-        } catch (BindTransportException e) {
-            assertThat(e.getMessage(), containsString("Failed to auto-resolve publish port"));
-        }
+        int resPort = resolvePublishPort(
+            new TcpTransport.ProfileSettings(baseSettings, profile).publishPort,
+            asList(address("127.0.0.1", boundPort), address("127.0.0.2", otherBoundPort)),
+            getByName("127.0.0.3")
+        );
+        assertThat("as publish_port not specified and non-unique port of bound addresses", resPort, equalTo(-1));
 
         publishPort = resolvePublishPort(
-            new TcpTransport.ProfileSettings(baseSettings, profile),
+            new TcpTransport.ProfileSettings(baseSettings, profile).publishPort,
             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(
-                new TcpTransport.ProfileSettings(baseSettings, profile),
+                new TcpTransport.ProfileSettings(baseSettings, profile).publishPort,
                 asList(address("0.0.0.0", boundPort), address("127.0.0.2", otherBoundPort)),
                 getByName("::1")
             );