JVMCBC-1542 Service creation failed to use external network host

Motivation
----------
In the wake of JVMCBC-1527 and JVMCBC-1541, Node.addService()
ended up accidentally using the host from the default network
to create services, regardless of which network was actually selected.

Modifications
-------------
Add NodeIdentifier.hostForNetworkConnections(), which returns
the host from the selected network.

Use this instead of NodeIdentifier.address() in Node.addService().

Caveats
-------
NodeIdentifiers created by the legacy GlobalConfigParser and
BucketConfigParser are not assigned a value for the new
`hostForNetworkConnections` property. Those parsers are no longer
used by production SDK code, and are retained for now just in case
other projects reference them.

Change-Id: I1fb8d0d56700b05ead501b14494407db5d15797c
Reviewed-on: https://review.couchbase.org/c/couchbase-jvm-clients/+/212914
Reviewed-by: Michael Reiche <[email protected]>
Tested-by: Build Bot <[email protected]>

core-io/src/integrationTest/java/com/couchbase/client/core/config/loader/ClusterManagerBucketLoaderIntegrationTest.java

@@ -77,7 +77,7 @@ void loadConfigViaClusterManagerHttp() throws Exception {
     ClusterManagerBucketLoader loader = new ClusterManagerBucketLoader(core);
     int port = config.ports().get(Services.MANAGER);
     ProposedBucketConfigContext ctx = loader.load(
-      new NodeIdentifier(config.hostname(), port),
+      NodeIdentifier.forBootstrap(config.hostname(), port),
       port,
       config().bucketname()
     ).block();

core-io/src/integrationTest/java/com/couchbase/client/core/config/loader/GlobalLoaderIntegrationTest.java

@@ -17,7 +17,6 @@
 package com.couchbase.client.core.config.loader;
 
 import com.couchbase.client.core.Core;
-import com.couchbase.client.core.cnc.events.core.ReconfigurationCompletedEvent;
 import com.couchbase.client.core.config.ProposedGlobalConfigContext;
 import com.couchbase.client.core.env.CoreEnvironment;
 import com.couchbase.client.core.node.NodeIdentifier;
@@ -67,7 +66,7 @@ void loadGlobalConfigViaCarrierPublication() {
     logger.info("Proposing config");
 
     ProposedGlobalConfigContext globalConfigContext = loader.load(
-      new NodeIdentifier(config.hostname(), config.ports().get(Services.MANAGER)),
+      NodeIdentifier.forBootstrap(config.hostname(), config.ports().get(Services.MANAGER)),
       config.ports().get(Services.KV)
     ).block();
 

core-io/src/integrationTest/java/com/couchbase/client/core/config/loader/KeyValueBucketLoaderIntegrationTest.java

@@ -62,7 +62,7 @@ void loadConfigViaCarrierPublication() {
     configWaitHelper.await();
     KeyValueBucketLoader loader = new KeyValueBucketLoader(core);
     ProposedBucketConfigContext loaded = loader.load(
-      new NodeIdentifier(config.hostname(), config.ports().get(Services.MANAGER)),
+      NodeIdentifier.forBootstrap(config.hostname(), config.ports().get(Services.MANAGER)),
       config.ports().get(Services.KV),
       config().bucketname()
     ).block();

core-io/src/integrationTest/java/com/couchbase/client/core/config/refresher/GlobalBucketRefresherIntegrationTest.java

@@ -108,7 +108,7 @@ private void loadGlobalConfig(final ConfigurationProvider provider) {
     GlobalLoader loader = new GlobalLoader(core);
 
     ProposedGlobalConfigContext globalConfigContext = loader.load(
-      new NodeIdentifier(config.hostname(), config.ports().get(Services.MANAGER)),
+      NodeIdentifier.forBootstrap(config.hostname(), config.ports().get(Services.MANAGER)),
       config.ports().get(Services.KV)
     ).block();
 

core-io/src/main/java/com/couchbase/client/core/config/DefaultConfigurationProvider.java

@@ -821,7 +821,10 @@ private Mono<ProposedBucketConfigContext> fetchBucketConfigs(final String name,
         .take(Math.min(index, seedNodes.size()))
         .last()
         .flatMap(seed -> {
-          NodeIdentifier identifier = new NodeIdentifier(seed.address(), seed.clusterManagerPort().orElse(DEFAULT_MANAGER_PORT));
+          NodeIdentifier identifier = NodeIdentifier.forBootstrap(
+            seed.address(),
+            seed.clusterManagerPort().orElse(DEFAULT_MANAGER_PORT)
+          );
 
           final int mappedKvPort = seed.kvPort().orElse(kvPort);
           final int mappedManagerPort = seed.clusterManagerPort().orElse(managerPort);
@@ -891,7 +894,10 @@ private Mono<ProposedGlobalConfigContext> fetchGlobalConfigs(final Set<SeedNode>
             return Mono.empty();
           }
 
-          NodeIdentifier identifier = new NodeIdentifier(seed.address(), seed.clusterManagerPort().orElse(DEFAULT_MANAGER_PORT));
+          NodeIdentifier identifier = NodeIdentifier.forBootstrap(
+            seed.address(),
+            seed.clusterManagerPort().orElse(DEFAULT_MANAGER_PORT)
+          );
           return globalLoader
             .load(identifier, seed.kvPort().orElse(kvPort))
             .doOnError(throwable -> core.context().environment().eventBus().publish(new IndividualGlobalConfigLoadFailedEvent(

core-io/src/main/java/com/couchbase/client/core/node/Node.java

@@ -235,7 +235,7 @@ public Mono<Void> addService(
       }
 
       HostAndPort newServiceAddress = new HostAndPort(
-        identifier.address(),
+        identifier.hostForNetworkConnections(),
         port
       );
 

core-io/src/main/java/com/couchbase/client/core/node/NodeIdentifier.java

@@ -15,55 +15,102 @@
  */
 package com.couchbase.client.core.node;
 
+import com.couchbase.client.core.annotation.Stability;
+import com.couchbase.client.core.util.HostAndPort;
+import reactor.util.annotation.Nullable;
+
+import java.util.NoSuchElementException;
 import java.util.Objects;
 
 import static com.couchbase.client.core.logging.RedactableArgument.redactSystem;
+import static java.util.Objects.requireNonNull;
 
 /**
- * Identifies a node uniquely in the cluster.
- *
- * <p>If you ask yourself: why is the hostname not enough? Well, let me tell you that
- * it is possible to run multiple nodes on the same host if you compile it from source and know how. So the
- * hostname alone is not enough, we also need to know the cluster manager port to properly identify each
- * node.</p>
+ * Uniquely identifies a node within the cluster, using the node's
+ * host and manager port from the default network.
  */
 public class NodeIdentifier {
 
-  private final String address;
-  private final int managerPort;
+  private final String canonicalHost;
+  private final int canonicalManagerPort;
+
+  // Nullable only when created by  a
+  @Nullable private final String hostForNetworkConnections;
+
+  @Deprecated
+  public NodeIdentifier(
+    final String canonicalHost,
+    final int canonicalManagerPort
+  ) {
+    this.canonicalHost = canonicalHost;
+    this.canonicalManagerPort = canonicalManagerPort;
+    this.hostForNetworkConnections = null;
+  }
+
+  @Stability.Internal
+  public static NodeIdentifier forBootstrap(String bootstrapHost, int bootstrapPort) {
+    // This address isn't really "canonical", since it may be an "external" address.
+    // If it's an external address, the node created from this identifier will be discarded
+    // when the config with the _real_ canonical addresses is applied.
+    return new NodeIdentifier(new HostAndPort(bootstrapHost, bootstrapPort), bootstrapHost);
+  }
+
+  public NodeIdentifier(
+    HostAndPort canonicalAddress,
+    String hostForNetworkConnections
+  ) {
+    this.canonicalHost = canonicalAddress.host();
+    this.canonicalManagerPort = canonicalAddress.port();
+    this.hostForNetworkConnections = requireNonNull(hostForNetworkConnections);
+  }
 
-  public NodeIdentifier(final String address, final int managerPort) {
-    this.address = address;
-    this.managerPort = managerPort;
+  /**
+   * Returns this node's host on the selected network.
+   * <p>
+   * If the default network was selected, this is the same as the canonical host.
+   *
+   * @throws NoSuchElementException if this info is not available
+   */
+  public String hostForNetworkConnections() throws NoSuchElementException {
+    if (hostForNetworkConnections == null) {
+      throw new NoSuchElementException(
+        "This NodeIdentifier (" + this + ") doesn't have the host to use for network connections." +
+          " It might have been created by a legacy config parser or some other component that did not specify it."
+      );
+    }
+    return hostForNetworkConnections;
   }
 
+  /**
+   * Returns the node's host on the default network.
+   */
   public String address() {
-    return address;
+    return canonicalHost;
   }
 
   public int managerPort() {
-    return managerPort;
+    return canonicalManagerPort;
   }
 
   @Override
   public String toString() {
     return "NodeIdentifier{" +
-      "address=" + redactSystem(address) +
-      ", managerPort=" + redactSystem(managerPort) +
-      '}';
+      "canonicalAddress=" + redactSystem(canonicalHost + ":" + canonicalManagerPort) +
+      ", hostForNetworkConnections=" + hostForNetworkConnections +
+      "}";
   }
 
   @Override
-  public boolean equals(final Object o) {
+  public boolean equals(Object o) {
     if (this == o) return true;
     if (o == null || getClass() != o.getClass()) return false;
     NodeIdentifier that = (NodeIdentifier) o;
-    return managerPort == that.managerPort && Objects.equals(address, that.address);
+    return canonicalManagerPort == that.canonicalManagerPort && Objects.equals(canonicalHost, that.canonicalHost);
   }
 
   @Override
   public int hashCode() {
-    return Objects.hash(address, managerPort);
+    return Objects.hash(canonicalHost, canonicalManagerPort);
   }
 
 }

core-io/src/main/java/com/couchbase/client/core/topology/HostAndServicePorts.java

@@ -49,7 +49,7 @@ public class HostAndServicePorts implements KetamaRingNode {
   public static final HostAndServicePorts INACCESSIBLE = new HostAndServicePorts(
     "<inaccessible>",
     emptyMap(),
-    new NodeIdentifier("<inaccessible>", 0),
+    new NodeIdentifier("<inaccessible>", 0, "<inaccessible>"),
     null
   );
 

core-io/src/main/java/com/couchbase/client/core/topology/HostAndServicePortsParser.java

@@ -54,13 +54,12 @@ public static Map<NetworkResolution, HostAndServicePorts> parse(
   ) {
     Map<NetworkResolution, HostAndRawServicePorts> raw = parseIntermediate(json);
     HostAndPort ketamaAuthority = getKetamaAuthority(raw);
-    NodeIdentifier id = getId(raw);
 
     return transformValues(raw, value ->
       new HostAndServicePorts(
         value.host,
         portSelector.selectPorts(value.rawServicePorts),
-        id,
+        getId(value.host, raw),
         ketamaAuthority
       )
     );
@@ -95,6 +94,7 @@ private static HostAndPort getKetamaAuthority(Map<NetworkResolution, HostAndRawS
    * @throws CouchbaseException If the default network has no manager ports for the node
    */
   private static NodeIdentifier getId(
+    String hostForNetworkConnections,
     Map<NetworkResolution, HostAndRawServicePorts> networkToNodeInfo
   ) {
     HostAndRawServicePorts defaultNodeMap = networkToNodeInfo.get(NetworkResolution.DEFAULT);
@@ -114,7 +114,7 @@ private static NodeIdentifier getId(
       );
     }
 
-    return new NodeIdentifier(defaultNodeMap.host, idPort);
+    return new NodeIdentifier(defaultNodeMap.host, idPort, hostForNetworkConnections);
   }
 
   @Nullable

core-io/src/main/java/com/couchbase/client/core/topology/NodeIdentifier.java

@@ -17,45 +17,53 @@
 package com.couchbase.client.core.topology;
 
 import com.couchbase.client.core.annotation.Stability;
+import com.couchbase.client.core.util.HostAndPort;
 
 import java.util.Objects;
 
 import static java.util.Objects.requireNonNull;
 
 @Stability.Internal
 public class NodeIdentifier {
-  private final String host;
-  private final int port;
+  private final HostAndPort canonical; // manager host:port on default network
+  private final String hostForNetworkConnections;
 
-  public NodeIdentifier(String host, int port) {
-    this.host = requireNonNull(host);
-    this.port = port;
+  public NodeIdentifier(String host, int port, String hostForNetworkConnections) {
+    this(new HostAndPort(host, port), hostForNetworkConnections);
+  }
+
+  public NodeIdentifier(HostAndPort canonical, String hostForNetworkConnections) {
+    this.canonical = requireNonNull(canonical);
+    this.hostForNetworkConnections = requireNonNull(hostForNetworkConnections);
   }
 
   @Deprecated
   public com.couchbase.client.core.node.NodeIdentifier toLegacy() {
-    return new com.couchbase.client.core.node.NodeIdentifier(host, port);
+    return new com.couchbase.client.core.node.NodeIdentifier(canonical, hostForNetworkConnections);
   }
 
-  @Override
-  public String toString() {
-    return host + ":" + port;
+  public String hostForNetworkConnections() {
+    return hostForNetworkConnections;
   }
 
   @Override
   public boolean equals(Object o) {
-    if (this == o) {
-      return true;
-    }
-    if (o == null || getClass() != o.getClass()) {
-      return false;
-    }
+    if (this == o) return true;
+    if (o == null || getClass() != o.getClass()) return false;
     NodeIdentifier that = (NodeIdentifier) o;
-    return port == that.port && host.equals(that.host);
+    return canonical.equals(that.canonical);
   }
 
   @Override
   public int hashCode() {
-    return Objects.hash(host, port);
+    return Objects.hash(canonical);
+  }
+
+  @Override
+  public String toString() {
+    return "NodeID{" +
+      "canonical=" + canonical +
+      ", hostForNetworkConnections='" + hostForNetworkConnections + '\'' +
+      '}';
   }
 }

core-io/src/test/java/com/couchbase/client/core/config/GlobalConfigTranslationTest.java

@@ -27,6 +27,7 @@
 import org.junit.jupiter.api.Test;
 
 import java.util.Collections;
+import java.util.List;
 
 import static com.couchbase.client.core.util.CbCollections.listOf;
 import static com.couchbase.client.core.util.CbCollections.setOf;
@@ -88,6 +89,7 @@ void nodeIdsComeFromInternalNetwork() {
 
     GlobalConfig config = new GlobalConfig(topology);
 
+    List<NodeIdentifier> nodeIds = transform(config.portInfos(), PortInfo::identifier);
     assertEquals(
       listOf(
         new NodeIdentifier("svc-dqisea-node-001.nyarjaj-crhge67o.sandbox.nonprod-project-avengers.com", 8091),
@@ -96,7 +98,12 @@ void nodeIdsComeFromInternalNetwork() {
         new NodeIdentifier("svc-dqisea-node-004.nyarjaj-crhge67o.sandbox.nonprod-project-avengers.com", 8091),
         new NodeIdentifier("svc-dqisea-node-005.nyarjaj-crhge67o.sandbox.nonprod-project-avengers.com", 8091)
       ),
-      transform(config.portInfos(), PortInfo::identifier)
+      nodeIds
+    );
+
+    assertEquals(
+      listOf(originHost, originHost, originHost, originHost, originHost),
+      transform(nodeIds, NodeIdentifier::hostForNetworkConnections)
     );
   }
 

core-io/src/test/java/com/couchbase/client/core/node/NodeTest.java

@@ -74,7 +74,7 @@ class NodeTest {
   private static CoreContext CTX;
 
   private static NodeIdentifier testNodeIdentifier() {
-    return new NodeIdentifier("example.com", 8091);
+    return new NodeIdentifier(new HostAndPort("example.com", 8091), "example.com");
   }
 
   @BeforeAll

core-io/src/test/java/com/couchbase/client/core/topology/ClusterTopologyParserTest.java

@@ -228,15 +228,25 @@ void nodeIdsComeFromInternalNetwork() {
 
     assertEquals(NetworkResolution.EXTERNAL, config.network());
 
+    // This config has the same external host for all nodes
+    String externalHost = originHost;
+
+    List<NodeIdentifier> nodeIds = transform(config.nodes(), HostAndServicePorts::id);
     assertEquals(
       listOf(
-        new NodeIdentifier("svc-dqisea-node-001.nyarjaj-crhge67o.sandbox.nonprod-project-avengers.com", 8091),
-        new NodeIdentifier("svc-dqisea-node-002.nyarjaj-crhge67o.sandbox.nonprod-project-avengers.com", 8091),
-        new NodeIdentifier("svc-dqisea-node-003.nyarjaj-crhge67o.sandbox.nonprod-project-avengers.com", 8091),
-        new NodeIdentifier("svc-dqisea-node-004.nyarjaj-crhge67o.sandbox.nonprod-project-avengers.com", 8091),
-        new NodeIdentifier("svc-dqisea-node-005.nyarjaj-crhge67o.sandbox.nonprod-project-avengers.com", 8091)
+        new NodeIdentifier("svc-dqisea-node-001.nyarjaj-crhge67o.sandbox.nonprod-project-avengers.com", 8091, externalHost),
+        new NodeIdentifier("svc-dqisea-node-002.nyarjaj-crhge67o.sandbox.nonprod-project-avengers.com", 8091, externalHost),
+        new NodeIdentifier("svc-dqisea-node-003.nyarjaj-crhge67o.sandbox.nonprod-project-avengers.com", 8091, externalHost),
+        new NodeIdentifier("svc-dqisea-node-004.nyarjaj-crhge67o.sandbox.nonprod-project-avengers.com", 8091, externalHost),
+        new NodeIdentifier("svc-dqisea-node-005.nyarjaj-crhge67o.sandbox.nonprod-project-avengers.com", 8091, externalHost)
       ),
-      transform(config.nodes(), HostAndServicePorts::id)
+      nodeIds
+    );
+
+    // external host is not part of equals, so:
+    assertEquals(
+      listOf(externalHost, externalHost, externalHost, externalHost, externalHost),
+      transform(nodeIds, NodeIdentifier::hostForNetworkConnections)
     );
   }