From edfedca137202a0a436802b9dbe448ca026049ec Mon Sep 17 00:00:00 2001 From: Mikita Hradovich Date: Thu, 6 Aug 2026 18:13:38 +0200 Subject: [PATCH 1/9] refactor: remove dead local-DC contact-point compatibility check (DRIVER-201) OptionalLocalDcHelper.checkLocalDatacenterCompatibility() warned when a contact point reported a datacenter different from the configured local DC. This has been dead code on scylla-4.x since 12e6acb90b: refresh matches nodes by hostId only, so contact-point nodes never get a datacenter assigned and the warning could never fire. Remove it. The separate "configured local DC matches no node" warning is retained. Nothing covered the removal, and CUSTOMER-588 is the bug the check caused: it compared the configured local DC against ephemeral placeholder Nodes (built by MetadataManager#addContactPoints via DefaultNode#newContactPoint, datacenter always null), so it warned unconditionally whenever a local DC was configured, no matter where the contact points actually were. The new test builds a placeholder Node the same way production does, plus a resolved node that genuinely is in the configured local DC, and asserts no warning is logged. It asserts on the absence of any WARN rather than of one particular message, so a regression under different wording is still caught; should_warn_if_configured_dc_matches_no_node is the positive control for the same appender, so a silent capture failure cannot make it pass by accident. Co-Authored-By: Claude Opus 5 (1M context) --- .../helper/OptionalLocalDcHelper.java | 74 +++++-------------- .../DefaultLoadBalancingPolicyInitTest.java | 36 +++++++++ 2 files changed, 53 insertions(+), 57 deletions(-) diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/helper/OptionalLocalDcHelper.java b/core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/helper/OptionalLocalDcHelper.java index b93a16a6525..97aab92fff7 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/helper/OptionalLocalDcHelper.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/helper/OptionalLocalDcHelper.java @@ -26,7 +26,6 @@ import edu.umd.cs.findbugs.annotations.NonNull; import java.util.ArrayList; import java.util.HashSet; -import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Objects; @@ -68,73 +67,34 @@ public OptionalLocalDcHelper( @Override @NonNull public Optional discoverLocalDc(@NonNull Map nodes) { - String localDcStr = context.getLocalDatacenter(profile.getName()); - Optional localDc; - if (localDcStr != null) { - LOG.debug("[{}] Local DC set programmatically: {}", logPrefix, localDcStr); - localDc = Optional.of(localDcStr); + String localDc = context.getLocalDatacenter(profile.getName()); + if (localDc != null) { + LOG.debug("[{}] Local DC set programmatically: {}", logPrefix, localDc); } else if (profile.isDefined(DefaultDriverOption.LOAD_BALANCING_LOCAL_DATACENTER)) { - localDcStr = profile.getString(DefaultDriverOption.LOAD_BALANCING_LOCAL_DATACENTER); - LOG.debug("[{}] Local DC set from configuration: {}", logPrefix, localDcStr); - localDc = Optional.of(localDcStr); - } else { - localDc = Optional.empty(); - } - if (localDc.isPresent()) { - checkLocalDatacenterCompatibility( - localDc.get(), context.getMetadataManager().getContactPoints()); - // Also warn if the configured DC doesn't match any node in the cluster - if (!nodes.isEmpty()) { - boolean found = false; - for (Node node : nodes.values()) { - if (localDc.get().equals(node.getDatacenter())) { - found = true; - break; - } - } - if (!found) { - LOG.warn( - "[{}] Configured local DC '{}' does not match any node's datacenter" - + " (available DCs: {}); please verify your configuration", - logPrefix, - localDc.get(), - formatDcs(nodes.values())); - } - } + localDc = profile.getString(DefaultDriverOption.LOAD_BALANCING_LOCAL_DATACENTER); + LOG.debug("[{}] Local DC set from configuration: {}", logPrefix, localDc); } else { LOG.debug("[{}] Local DC not set, DC awareness will be disabled", logPrefix); + return Optional.empty(); } - return localDc; - } - - /** - * Checks if the contact points are compatible with the local datacenter specified either through - * configuration, or programmatically. - * - *

The default implementation logs a warning when a contact point reports a datacenter - * different from the local one, and only for the default profile. - * - * @param localDc The local datacenter, as specified in the config, or programmatically. - * @param contactPoints The contact points provided when creating the session. - */ - protected void checkLocalDatacenterCompatibility( - @NonNull String localDc, Set contactPoints) { - if (profile.getName().equals(DriverExecutionProfile.DEFAULT_NAME)) { - Set badContactPoints = new LinkedHashSet<>(); - for (Node node : contactPoints) { - if (!Objects.equals(localDc, node.getDatacenter())) { - badContactPoints.add(node); + if (!nodes.isEmpty()) { + boolean found = false; + for (Node node : nodes.values()) { + if (localDc.equals(node.getDatacenter())) { + found = true; + break; } } - if (!badContactPoints.isEmpty()) { + if (!found) { LOG.warn( - "[{}] You specified {} as the local DC, but some contact points are from a different DC: {}; " - + "please provide the correct local DC, or check your contact points", + "[{}] Configured local DC '{}' does not match any node's datacenter" + + " (available DCs: {}); please verify your configuration", logPrefix, localDc, - formatNodesAndDcs(badContactPoints)); + formatDcs(nodes.values())); } } + return Optional.of(localDc); } /** diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicyInitTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicyInitTest.java index b5e843d77d2..9c36cbebfee 100644 --- a/core/src/test/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicyInitTest.java +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicyInitTest.java @@ -19,6 +19,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -29,9 +30,12 @@ import com.datastax.oss.driver.api.core.config.DriverExecutionProfile; import com.datastax.oss.driver.api.core.loadbalancing.NodeDistance; import com.datastax.oss.driver.api.core.metadata.NodeState; +import com.datastax.oss.driver.internal.core.metadata.DefaultEndPoint; +import com.datastax.oss.driver.internal.core.metadata.DefaultNode; import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableMap; import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableSet; import edu.umd.cs.findbugs.annotations.NonNull; +import java.net.InetSocketAddress; import java.util.UUID; import org.junit.Test; import org.junit.runner.RunWith; @@ -210,6 +214,38 @@ public void should_warn_if_configured_dc_matches_no_node() { .isTrue(); } + @Test + public void should_not_warn_about_dc_mismatch_when_the_only_real_node_matches_configured_dc() { + // Given — CUSTOMER-588. A contact point given as a hostname is represented, before the control + // connection resolves it, by an ephemeral placeholder Node (built by + // MetadataManager#addContactPoints via DefaultNode#newContactPoint) whose datacenter is always + // null: it is never populated, because real topology is attached to a *different* Node object + // matched by hostId (see MetadataManager#registerNode). + // + // The removed OptionalLocalDcHelper#checkLocalDatacenterCompatibility compared the configured + // local DC against *those* placeholders, so it warned unconditionally whenever a local DC was + // configured, no matter where the contact points actually were. Here the only node carrying + // real, resolved metadata (node1) genuinely is in the configured local DC ("dc1", per base + // setup). + DefaultNode ephemeralContactPointNode = + DefaultNode.newContactPoint( + new DefaultEndPoint(new InetSocketAddress("127.0.0.9", 9042)), context); + when(metadataManager.getContactPoints()).thenReturn(ImmutableSet.of(ephemeralContactPointNode)); + DefaultLoadBalancingPolicy policy = createPolicy(); + + // When + policy.init(ImmutableMap.of(UUID.randomUUID(), node1), distanceReporter); + + // Then — no WARN at all. The retained check inspects the resolved node map, where node1 + // matches. + // Asserting that nothing is warned, rather than that one particular message is absent, also + // catches a regression that brings the false positive back under different wording. + // should_warn_if_configured_dc_matches_no_node is the positive control for this same appender, + // so a silent capture failure cannot make this pass by accident. + verify(appender, never()).doAppend(argThat(event -> event.getLevel() == Level.WARN)); + assertThat(policy.getLocalDatacenter()).isEqualTo("dc1"); + } + @NonNull protected DefaultLoadBalancingPolicy createPolicy() { return new DefaultLoadBalancingPolicy(context, DriverExecutionProfile.DEFAULT_NAME); From 0951bce1cbd73a665f964e6850b5031e70d6af6e Mon Sep 17 00:00:00 2001 From: Mikita Hradovich Date: Thu, 6 Aug 2026 18:15:06 +0200 Subject: [PATCH 2/9] feat: keep contact points unresolved; deprecate RESOLVE_CONTACT_POINTS (DRIVER-201) Contact points backed by a hostname are now always kept unresolved, so the connection layer can expand them to all their DNS-mapped IPs at connection time. SessionBuilder no longer reads RESOLVE_CONTACT_POINTS when merging contact points; the option is deprecated and has no effect. An already-resolved InetSocketAddress passed programmatically is still used as provided, with no further expansion. OptionsMap.fillWithDriverDefaults() still carries the option's reference.conf value so the defaults map stays complete, and is annotated accordingly -- the build treats deprecation warnings as errors. Co-Authored-By: Claude Opus 5 (1M context) --- .../api/core/config/DefaultDriverOption.java | 4 +++ .../driver/api/core/config/OptionsMap.java | 3 ++ .../api/core/config/TypedDriverOption.java | 8 ++++- .../api/core/session/SessionBuilder.java | 27 ++++++++++------ core/src/main/resources/reference.conf | 32 +++++++++---------- 5 files changed, 47 insertions(+), 27 deletions(-) diff --git a/core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java b/core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java index 3a6e4ed69bb..c2d723a00e7 100644 --- a/core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java +++ b/core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java @@ -837,7 +837,11 @@ public enum DefaultDriverOption implements DriverOption { * Whether to resolve the addresses passed to `basic.contact-points`. * *

Value-type: boolean + * + * @deprecated Contact points are now always kept as unresolved hostnames and expanded to all + * their DNS-mapped IPs lazily at connection time. Setting this option has no effect. */ + @Deprecated RESOLVE_CONTACT_POINTS("advanced.resolve-contact-points"), /** diff --git a/core/src/main/java/com/datastax/oss/driver/api/core/config/OptionsMap.java b/core/src/main/java/com/datastax/oss/driver/api/core/config/OptionsMap.java index 1d906caa985..6e608e8b858 100644 --- a/core/src/main/java/com/datastax/oss/driver/api/core/config/OptionsMap.java +++ b/core/src/main/java/com/datastax/oss/driver/api/core/config/OptionsMap.java @@ -245,6 +245,9 @@ private void readObject(ObjectInputStream stream) throws InvalidObjectException throw new InvalidObjectException("Proxy required"); } + // RESOLVE_CONTACT_POINTS is deprecated and has no effect, but it is still a driver option, so the + // defaults map stays complete by carrying its reference.conf value. + @SuppressWarnings("deprecation") protected static void fillWithDriverDefaults(OptionsMap map) { Duration initQueryTimeout = Duration.ofSeconds(5); Duration requestTimeout = Duration.ofSeconds(2); diff --git a/core/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.java b/core/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.java index e412b99b404..cfe22540b4d 100644 --- a/core/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.java +++ b/core/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.java @@ -664,7 +664,13 @@ public String toString() { /** The coalescer reschedule interval. */ public static final TypedDriverOption COALESCER_INTERVAL = new TypedDriverOption<>(DefaultDriverOption.COALESCER_INTERVAL, GenericType.DURATION); - /** Whether to resolve the addresses passed to `basic.contact-points`. */ + /** + * Whether to resolve the addresses passed to `basic.contact-points`. + * + * @deprecated Contact points are now always kept as unresolved hostnames and expanded to all + * their DNS-mapped IPs lazily at connection time. Setting this option has no effect. + */ + @Deprecated public static final TypedDriverOption RESOLVE_CONTACT_POINTS = new TypedDriverOption<>(DefaultDriverOption.RESOLVE_CONTACT_POINTS, GenericType.BOOLEAN); /** diff --git a/core/src/main/java/com/datastax/oss/driver/api/core/session/SessionBuilder.java b/core/src/main/java/com/datastax/oss/driver/api/core/session/SessionBuilder.java index 8375f0ef30b..720e3233fd1 100644 --- a/core/src/main/java/com/datastax/oss/driver/api/core/session/SessionBuilder.java +++ b/core/src/main/java/com/datastax/oss/driver/api/core/session/SessionBuilder.java @@ -166,11 +166,15 @@ protected DriverConfigLoader defaultConfigLoader(@Nullable ClassLoader classLoad *

Contact points can also be provided statically in the configuration. If both are specified, * they will be merged. If both are absent, the driver will default to 127.0.0.1:9042. * - *

Contrary to the configuration, DNS names with multiple A-records will not be handled here. - * If you need that, extract them manually with {@link java.net.InetAddress#getAllByName(String)} - * before calling this method. Similarly, if you need connect addresses to stay unresolved, make - * sure you pass unresolved instances here (see {@code advanced.resolve-contact-points} in the - * configuration for more explanations). + *

The driver automatically expands any contact point backed by an unresolved hostname to all + * its DNS-mapped IPs at connection time (through Netty's configured resolver, so a custom {@code + * AddressResolverGroup} still applies), so passing a single hostname is sufficient to try all its + * IPs on initial connect. This applies equally to hostnames provided here programmatically (build + * an unresolved {@link InetSocketAddress} with {@link InetSocketAddress#createUnresolved(String, + * int)} to opt in) and to hostnames specified in the configuration. An already-resolved address + * passed here (the common case when constructing an {@code InetSocketAddress} directly from a + * hostname, which resolves eagerly) is used as provided, with no further expansion. The {@code + * advanced.resolve-contact-points} option is deprecated and has no effect. */ @NonNull public SelfT addContactPoints(@NonNull Collection contactPoints) { @@ -741,6 +745,12 @@ public SelfT withCloudSecureConnectBundle(@NonNull InputStream cloudConfigInputS * *

For more information, please refer to the DataStax Astra documentation. * + *

A proxy given as a hostname is resolved at connection time, to all of its addresses, + * and each is tried in turn. That holds however the {@link InetSocketAddress} was built: the + * driver keeps a proxy hostname unresolved internally, so passing one that the ordinary {@code + * InetSocketAddress(String, int)} constructor already resolved does not bind the session to that + * single address. + * * @param cloudProxyAddress The address of the Cloud proxy to use. * @see Server Name Indication */ @@ -957,11 +967,10 @@ protected final CompletionStage buildDefaultSessionAsync() { programmaticArguments = programmaticArgumentsBuilder.build(); } - boolean resolveAddresses = - defaultConfig.getBoolean(DefaultDriverOption.RESOLVE_CONTACT_POINTS, false); - + // RESOLVE_CONTACT_POINTS is deprecated: contact points are always kept as unresolved + // hostnames, and expanded to all their DNS IPs at connection time by ChannelFactory. Set contactPoints = - ContactPoints.merge(programmaticContactPoints, configContactPoints, resolveAddresses); + ContactPoints.merge(programmaticContactPoints, configContactPoints, false); if (keyspace == null && defaultConfig.isDefined(DefaultDriverOption.SESSION_KEYSPACE)) { keyspace = diff --git a/core/src/main/resources/reference.conf b/core/src/main/resources/reference.conf index 8a3a444319e..04fbf40e5a1 100644 --- a/core/src/main/resources/reference.conf +++ b/core/src/main/resources/reference.conf @@ -1224,27 +1224,25 @@ datastax-java-driver { } - # Whether to resolve the addresses passed to `basic.contact-points`. + # DEPRECATED: this option no longer has any effect and will be removed in a future release. # - # If this is true, addresses are created with `InetSocketAddress(String, int)`: the host name will - # be resolved the first time, and the driver will use the resolved IP address for all subsequent - # connection attempts. + # Contact points are now always kept as unresolved hostnames and expanded to all of their + # DNS-mapped IPs lazily at connection time. This means the driver tries every IP a hostname + # resolves to, and re-resolves the hostname on each new connection so DNS changes are picked up + # automatically. Previously this option selected between resolving a contact-point hostname once + # (true) and re-resolving it on every connection (false); that distinction no longer applies. # - # If this is false, addresses are created with `InetSocketAddress.createUnresolved()`: the host - # name will be resolved again every time the driver opens a new connection. This is useful for - # containerized environments where DNS records are more likely to change over time (note that the - # JVM and OS have their own DNS caching mechanisms, so you might need additional configuration - # beyond the driver). + # The lookup goes through Netty's configured AddressResolverGroup -- the same resolver an + # unresolved address would have reached had it been passed straight to Bootstrap.connect() -- so a + # custom resolver installed via NettyOptions.afterBootstrapInitialized() still applies. With + # Netty's default (JDK) resolver the lookup blocks the I/O event loop it runs on; install + # DnsAddressResolverGroup if you need it to be non-blocking. # - # This option only applies to the contact points specified in the configuration. It has no effect - # on: - # - programmatic contact points passed to SessionBuilder.addContactPoints: these addresses are - # built outside of the driver, so it is your responsibility to provide unresolved instances. - # - dynamically discovered peers: the driver relies on Cassandra system tables, which expose raw - # IP addresses. Use a custom address translator to convert them to unresolved addresses (if - # you're in a containerized environment, you probably already need address translation anyway). + # This option only ever applied to the contact points specified in the configuration -- never to + # programmatic contact points passed to SessionBuilder.addContactPoints, nor to dynamically + # discovered peers. # - # Required: no (defaults to false) + # Required: no # Modifiable at runtime: no # Overridable in a profile: no advanced.resolve-contact-points = false From 4b49338708738b2ea3476481054d4a69de01b923 Mon Sep 17 00:00:00 2001 From: Mikita Hradovich Date: Thu, 6 Aug 2026 18:19:54 +0200 Subject: [PATCH 3/9] feat: let the endpoint layer hand out unresolved addresses (DRIVER-201) Groundwork for expanding a hostname to all of its addresses: resolution becomes the connection layer's job, so everything that produces an EndPoint stops doing DNS of its own, and an endpoint gains a way to record which address a connection actually reached. PinnableEndPoint is the new internal contract: pinTo(SocketAddress) returns a copy bound to one address, and the pin is excluded from equals(), hashCode(), asMetricPrefix() and toString(). Endpoints are set and map keys, and node metrics are named after them, so a pinned copy has to be indistinguishable from its original everywhere except when the connection layer asks which address answered. A generic delegating wrapper was rejected: its equals() would be asymmetric, because DefaultEndPoint#equals tests instanceof and would reject the wrapper while the wrapper accepted the original, and it would break SniSslEngineFactory's instanceof SniEndPoint guard. Each implementation therefore carries a nullable pinnedAddress of its own. SniEndPoint additionally normalizes a resolved proxy *hostname* back to unresolved. withCloudProxyAddress(new InetSocketAddress("proxy", 9042)) resolves eagerly, which froze Cloud on whichever proxy IP the JVM happened to return; an IP-literal proxy is left alone. Contact points keep the opposite policy on purpose, since ContactPoints.merge() only ever applied its resolve flag to config-file entries. ClientRoutesTopologyMonitor.resolve() likewise returns the client route as an unresolved address and no longer looks it up, which keeps it a pure in-memory cache lookup that is safe to call from an event loop, and lets a custom resolver apply to client routes just as it does to contact points. Its protected resolveAddress() extension point, which existed only so tests could stub out InetAddress.getByName, goes with it. This has to move together with ClientRoutesEndPoint: dropping "throws UnknownHostException" from one and the matching catch from the other is a single compilable change. TopologyMonitor gains reresolvesNodeAddresses(), which tells the control connection's reconnection query plan whether this monitor already keeps addresses fresh. It defaults to false, correct for DefaultTopologyMonitor, whose peers hold an already-resolved IP from system.peers. ClientRoutesTopologyMonitor reports true only when every currently-known node actually has a live route: where the route set is incomplete, ClientRoutesEndPoint falls back to a static resolved endpoint, and those nodes still need the contact-point fallback. The "is this a name" test several of these need is shared as AddressUtils.carriesName(): a resolved address compares its host string against the literal its own bytes produce, an unresolved one parses its host string. Neither isUnresolved() nor the presence of an InetAddress can tell a name from a literal on its own. DseGssApiAuthProviderBase.serverName() falls back to getHostString() when getAddress() returns null, which is now the ordinary case for a Cloud or client-route endpoint rather than an impossible one. EndPoint.resolve() keeps its signature and is not deprecated, so third-party implementations still compile. Its javadoc gains two expectations: return the address as-is rather than looking names up, since this is now called from an event loop; and callers are warned that the returned address is no longer always resolved, so getHostString() is the safe way to read the host. Co-Authored-By: Claude Opus 5 (1M context) --- .../core/auth/DseGssApiAuthProviderBase.java | 20 ++- .../driver/api/core/metadata/EndPoint.java | 35 ++++- .../core/metadata/ClientRoutesEndPoint.java | 70 +++++++-- .../metadata/ClientRoutesTopologyMonitor.java | 43 ++++-- .../core/metadata/CloudTopologyMonitor.java | 10 ++ .../core/metadata/DefaultEndPoint.java | 52 ++++++- .../core/metadata/PinnableEndPoint.java | 80 ++++++++++ .../internal/core/metadata/SniEndPoint.java | 128 ++++++++++------ .../core/metadata/TopologyMonitor.java | 27 ++++ .../internal/core/util/AddressUtils.java | 33 +++++ .../metadata/ClientRoutesEndPointTest.java | 62 +++++++- .../ClientRoutesTopologyMonitorTest.java | 68 ++++++++- .../core/metadata/DefaultEndPointTest.java | 105 +++++++++++++ .../core/metadata/SniEndPointTest.java | 139 ++++++++++++++++++ .../internal/core/util/AddressUtilsTest.java | 60 ++++++++ 15 files changed, 850 insertions(+), 82 deletions(-) create mode 100644 core/src/main/java/com/datastax/oss/driver/internal/core/metadata/PinnableEndPoint.java create mode 100644 core/src/test/java/com/datastax/oss/driver/internal/core/metadata/SniEndPointTest.java create mode 100644 core/src/test/java/com/datastax/oss/driver/internal/core/util/AddressUtilsTest.java diff --git a/core/src/main/java/com/datastax/dse/driver/api/core/auth/DseGssApiAuthProviderBase.java b/core/src/main/java/com/datastax/dse/driver/api/core/auth/DseGssApiAuthProviderBase.java index 48a0e5b0ef3..beab2f488a8 100644 --- a/core/src/main/java/com/datastax/dse/driver/api/core/auth/DseGssApiAuthProviderBase.java +++ b/core/src/main/java/com/datastax/dse/driver/api/core/auth/DseGssApiAuthProviderBase.java @@ -27,6 +27,7 @@ import com.datastax.oss.protocol.internal.util.Bytes; import edu.umd.cs.findbugs.annotations.NonNull; import edu.umd.cs.findbugs.annotations.Nullable; +import java.net.InetAddress; import java.net.InetSocketAddress; import java.nio.ByteBuffer; import java.security.PrivilegedActionException; @@ -319,7 +320,7 @@ protected GssApiAuthenticator( SUPPORTED_MECHANISMS, options.getAuthorizationId(), protocol, - ((InetSocketAddress) endPoint.resolve()).getAddress().getCanonicalHostName(), + serverName(endPoint), options.getSaslProperties(), null); } catch (LoginException | SaslException e) { @@ -328,6 +329,23 @@ protected GssApiAuthenticator( this.endPoint = endPoint; } + /** + * The host name to build the Kerberos service principal from. + * + *

Prefers the canonical name of the resolved address, which is what Kerberos expects. The + * driver's own endpoints always hand this a resolved address — the channel carries an endpoint + * bound to the address it connected to (see {@code PinnableEndPoint}) — but a custom {@link + * EndPoint} implementation may still yield an unresolved one, in which case {@code + * getAddress()} is null. Fall back to the host string rather than throwing a {@link + * NullPointerException}: the hostname is usually the right service name anyway, and a failed + * reverse lookup should not take authentication down. + */ + private static String serverName(EndPoint endPoint) { + InetSocketAddress address = (InetSocketAddress) endPoint.resolve(); + InetAddress inetAddress = address.getAddress(); + return inetAddress != null ? inetAddress.getCanonicalHostName() : address.getHostString(); + } + @NonNull @Override protected ByteBuffer getMechanism() { diff --git a/core/src/main/java/com/datastax/oss/driver/api/core/metadata/EndPoint.java b/core/src/main/java/com/datastax/oss/driver/api/core/metadata/EndPoint.java index 530f2ad38ac..691f65fcd44 100644 --- a/core/src/main/java/com/datastax/oss/driver/api/core/metadata/EndPoint.java +++ b/core/src/main/java/com/datastax/oss/driver/api/core/metadata/EndPoint.java @@ -18,24 +18,53 @@ package com.datastax.oss.driver.api.core.metadata; import edu.umd.cs.findbugs.annotations.NonNull; -import java.net.InetSocketAddress; import java.net.SocketAddress; /** * Encapsulates the information needed to open connections to a node. * *

By default, the driver assumes plain TCP connections, and this is just a wrapper around an - * {@link InetSocketAddress}. However, more complex deployment scenarios might use a custom + * {@link java.net.InetSocketAddress}. However, more complex deployment scenarios might use a custom * implementation that contains additional information; for example, if the nodes are accessed * through a proxy with SNI routing, an SNI server name is needed in addition to the proxy address. */ public interface EndPoint { /** - * Resolves this instance to a socket address. + * Resolves this instance to the socket address connections should be opened to. * *

This will be called each time the driver opens a new connection to the node. The returned * address cannot be null. + * + *

Returning a hostname is fine, and is how multi-address support works. The returned + * address need not be resolved: an {@linkplain java.net.InetSocketAddress#isUnresolved() + * unresolved} {@link java.net.InetSocketAddress} is expanded by the driver to every + * address the name maps to, and each one is tried in turn until a connection succeeds. That is + * what {@link com.datastax.oss.driver.internal.core.metadata.DefaultEndPoint} does for contact + * points backed by a hostname, so a single unreachable IP behind a multi-record name no longer + * fails the connection. + * + *

Implementations must not resolve names themselves, and must not block. The driver + * calls this from its admin event loop, and it performs the expansion through Netty's configured + * {@code AddressResolverGroup} — the same resolver an unresolved address reaches when it is + * handed to {@code Bootstrap.connect()}. Looking the name up here instead (for example with + * {@link java.net.InetAddress#getAllByName(String)}) would both block that loop and bypass a + * custom resolver installed via {@code NettyOptions#afterBootstrapInitialized(Bootstrap)}. + * + *

Callers must not assume the returned address is resolved. It is for a node discovered + * from {@code system.peers} (built from that node's physical broadcast RPC address) and for the + * node the control connection is on (bound to the address that connection reached). It is + * not for a node reached through the Cloud SNI proxy, or through a cloud private-endpoint + * client route: there the address is the configured hostname, and {@link + * java.net.InetSocketAddress#getAddress()} returns {@code null}. Read the host with {@link + * java.net.InetSocketAddress#getHostString()}, which yields whichever of the two the address + * carries and never triggers a reverse lookup. + * + * @apiNote Timeout note: when a name expands to several addresses they are tried in + * sequence, so if every attempt times out the worst-case time before the node is declared + * unreachable is {@code N × advanced.connection.connect-timeout}. In practice DNS round-robin + * entries have only a small number of records, so this is rarely a concern, but it is worth + * bearing in mind when configuring connect timeouts. */ @NonNull SocketAddress resolve(); diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPoint.java b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPoint.java index 15d825b2efc..4d495ec5303 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPoint.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPoint.java @@ -20,19 +20,26 @@ import com.datastax.oss.driver.api.core.metadata.EndPoint; import edu.umd.cs.findbugs.annotations.NonNull; import edu.umd.cs.findbugs.annotations.Nullable; -import java.io.IOException; -import java.io.UncheckedIOException; import java.net.InetAddress; import java.net.InetSocketAddress; import java.net.SocketAddress; import java.util.Objects; import java.util.UUID; -public class ClientRoutesEndPoint implements EndPoint { +public class ClientRoutesEndPoint implements PinnableEndPoint { private final UUID hostId; private final ClientRoutesTopologyMonitor topologyMonitor; private final String metricPrefix; @NonNull private final EndPoint fallbackEndPoint; + /** Kept only so that {@link #pinTo(SocketAddress)} can rebuild an identical copy. */ + @Nullable private final InetAddress broadcastInetAddress; + + /** + * The address this endpoint has been {@linkplain #pinTo(SocketAddress) pinned} to, or {@code + * null} if it is not pinned. Deliberately excluded from {@link #equals} and {@link #hashCode}, + * which key off the host id alone. + */ + @Nullable private final InetSocketAddress pinnedAddress; /** * @param topologyMonitor the topology monitor used to resolve the endpoint address on demand. @@ -49,12 +56,23 @@ public ClientRoutesEndPoint( @NonNull UUID hostId, @Nullable InetAddress broadcastInetAddress, @NonNull EndPoint fallbackEndPoint) { + this(topologyMonitor, hostId, broadcastInetAddress, fallbackEndPoint, null); + } + + private ClientRoutesEndPoint( + @NonNull ClientRoutesTopologyMonitor topologyMonitor, + @NonNull UUID hostId, + @Nullable InetAddress broadcastInetAddress, + @NonNull EndPoint fallbackEndPoint, + @Nullable InetSocketAddress pinnedAddress) { this.topologyMonitor = Objects.requireNonNull(topologyMonitor, "Topology monitor cannot be null"); this.hostId = Objects.requireNonNull(hostId, "HOST uuid cannot be null"); this.fallbackEndPoint = Objects.requireNonNull(fallbackEndPoint, "Fallback endpoint cannot be null"); this.metricPrefix = buildMetricPrefix(broadcastInetAddress, hostId); + this.broadcastInetAddress = broadcastInetAddress; + this.pinnedAddress = pinnedAddress; } @NonNull @@ -62,18 +80,48 @@ public UUID getHostId() { return hostId; } + /** + * Returns the address connections should be opened to. + * + *

The client route for this host id is an in-memory lookup over the cached {@code + * system.client_routes} contents, and it yields exactly one address by design, so this neither + * blocks nor expands to several candidates. The route's hostname is returned {@linkplain + * InetSocketAddress#isUnresolved() unresolved}: {@link + * com.datastax.oss.driver.internal.core.channel.ChannelFactory} resolves it through Netty's + * configured {@code AddressResolverGroup}, so a custom resolver is honoured and no DNS lookup + * runs on the caller (the admin event loop, for control-connection reconnects). + * + *

When the topology monitor has no route for this host id — i.e. the node is not reached + * through a cloud private endpoint — this delegates to the fallback endpoint. + * + *

Once {@linkplain #pinTo(SocketAddress) pinned} the pinned address is returned directly. + */ @NonNull @Override public SocketAddress resolve() { - try { - InetSocketAddress address = topologyMonitor.resolve(hostId); - if (address != null) { - return address; - } - } catch (IOException e) { - throw new UncheckedIOException("DNS resolution failed for host_id=" + hostId, e); + if (pinnedAddress != null) { + return pinnedAddress; + } + InetSocketAddress address = topologyMonitor.resolve(hostId); + return address != null ? address : fallbackEndPoint.resolve(); + } + + @NonNull + @Override + public EndPoint pinTo(@NonNull SocketAddress resolvedAddress) { + Objects.requireNonNull(resolvedAddress, "resolvedAddress cannot be null"); + // Mirror DefaultEndPoint: an address we cannot hold in an InetSocketAddress field skips + // pinning rather than failing the connection. + if (!(resolvedAddress instanceof InetSocketAddress) + || resolvedAddress.equals(this.pinnedAddress)) { + return this; } - return fallbackEndPoint.resolve(); + return new ClientRoutesEndPoint( + topologyMonitor, + hostId, + broadcastInetAddress, + fallbackEndPoint, + (InetSocketAddress) resolvedAddress); } @Override diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitor.java b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitor.java index 1ffc35fd9f4..31d27aa7c7d 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitor.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitor.java @@ -21,6 +21,7 @@ import com.datastax.oss.driver.api.core.config.ClientRoutesConfig; import com.datastax.oss.driver.api.core.config.DefaultDriverOption; import com.datastax.oss.driver.api.core.metadata.EndPoint; +import com.datastax.oss.driver.api.core.metadata.Node; import com.datastax.oss.driver.internal.core.adminrequest.AdminRequestHandler; import com.datastax.oss.driver.internal.core.adminrequest.AdminResult; import com.datastax.oss.driver.internal.core.adminrequest.AdminRow; @@ -32,7 +33,6 @@ import edu.umd.cs.findbugs.annotations.Nullable; import java.net.InetAddress; import java.net.InetSocketAddress; -import java.net.UnknownHostException; import java.time.Duration; import java.util.ArrayList; import java.util.Collections; @@ -195,9 +195,18 @@ void setResolvedRoutes(Map routes) { resolvedRoutesCache.set(Collections.unmodifiableMap(new HashMap<>(routes))); } + /** + * Returns the client route for {@code hostId} as an {@linkplain InetSocketAddress#isUnresolved() + * unresolved} address, or {@code null} if this node has no route. + * + *

The route's hostname is deliberately left unresolved: {@link + * com.datastax.oss.driver.internal.core.channel.ChannelFactory} resolves it through Netty's + * configured {@code AddressResolverGroup} at connection time. That keeps this method a pure + * in-memory cache lookup, so it is safe to call from an event loop, and it means a custom + * resolver applies to client routes just like it does to contact points. + */ @Nullable - public InetSocketAddress resolve(@NonNull UUID hostId) - throws IllegalStateException, UnknownHostException { + public InetSocketAddress resolve(@NonNull UUID hostId) throws IllegalStateException { if (closed) { throw new IllegalStateException("Topology monitor is closed"); } @@ -206,7 +215,7 @@ public InetSocketAddress resolve(@NonNull UUID hostId) return null; // no client route for this node — caller falls back to default } - return new InetSocketAddress(resolveAddress(route.getHostname()), route.getPort()); + return InetSocketAddress.createUnresolved(route.getHostname(), route.getPort()); } /** @@ -480,6 +489,23 @@ protected EndPoint buildNodeEndPoint( return new ClientRoutesEndPoint(this, hostId, broadcastInetAddress, fallback); } + @Override + public boolean reresolvesNodeAddresses() { + // ClientRoutesEndPoint hands the route hostname over unresolved, so the connection layer + // re-expands it on every connection attempt -- but only when a route exists for that host_id + // (see ClientRoutesEndPoint#resolve()); for mixed/incomplete route sets it delegates to a + // static, already-resolved fallback endpoint instead. Only report true when every + // currently-known node actually has a live route; otherwise the contact-point reconnection + // fallback must stay available for the nodes stuck on that fallback. + Map routes = resolvedRoutesCache.get(); + for (Node node : context.getMetadataManager().getMetadata().getNodes().values()) { + if (!routes.containsKey(node.getHostId())) { + return false; + } + } + return true; + } + /** * Builds the CQL query to fetch client routes. * @@ -645,13 +671,4 @@ public CompletionStage closeAsync() { LOG.debug("[{}] ClientRoutesTopologyMonitor closed", logPrefix); return super.closeAsync(); } - - /** - * Resolves a hostname to an {@link InetAddress}. Extracted as a protected method so that unit - * tests can override it to return stubbed addresses without hitting the network. - */ - @NonNull - protected InetAddress resolveAddress(@NonNull String hostname) throws UnknownHostException { - return InetAddress.getByName(hostname); - } } diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/CloudTopologyMonitor.java b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/CloudTopologyMonitor.java index 021824a9b16..7bdf1c4e1ec 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/CloudTopologyMonitor.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/CloudTopologyMonitor.java @@ -44,4 +44,14 @@ protected EndPoint buildNodeEndPoint( UUID hostId = Objects.requireNonNull(row.getUuid("host_id")); return new SniEndPoint(cloudProxyAddress, hostId.toString()); } + + @Override + public boolean reresolvesNodeAddresses() { + // Every node is reached through the cloud SNI proxy, and SniEndPoint hands the proxy hostname + // over unresolved, so the connection layer re-expands it on every connection attempt (see + // ChannelFactory#resolveCandidates). Addresses therefore stay current on their own: appending + // the original contact points as a DNS re-resolution fallback would add nothing, and could + // resurrect nodes this monitor has authoritatively removed. + return true; + } } diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.java b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.java index 7ffbee8e4bb..bff230917b6 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.java @@ -19,26 +19,71 @@ import com.datastax.oss.driver.api.core.metadata.EndPoint; import edu.umd.cs.findbugs.annotations.NonNull; +import edu.umd.cs.findbugs.annotations.Nullable; import java.io.Serializable; import java.net.InetSocketAddress; +import java.net.SocketAddress; import java.util.Objects; -public class DefaultEndPoint implements EndPoint, Serializable { +public class DefaultEndPoint implements PinnableEndPoint, Serializable { private static final long serialVersionUID = 1; private final InetSocketAddress address; private final String metricPrefix; + /** + * The address this endpoint has been {@linkplain #pinTo(SocketAddress) pinned} to, or {@code + * null} if it is not pinned. Deliberately excluded from {@link #equals}, {@link #hashCode} and + * {@link #asMetricPrefix()}: a pinned copy denotes the same node as the original. + */ + @Nullable private final InetSocketAddress pinnedAddress; + public DefaultEndPoint(InetSocketAddress address) { + this(address, null); + } + + private DefaultEndPoint(InetSocketAddress address, @Nullable InetSocketAddress pinnedAddress) { this.address = Objects.requireNonNull(address, "address can't be null"); this.metricPrefix = buildMetricPrefix(address); + this.pinnedAddress = pinnedAddress; } + /** + * Returns the address connections should be opened to: the {@linkplain #pinTo(SocketAddress) + * pinned} one if this is a pinned copy, otherwise the stored address as-is. + * + *

This performs no name resolution. If the stored address is a hostname (i.e. {@linkplain + * InetSocketAddress#isUnresolved() unresolved} — contact points are always kept unresolved, see + * {@link com.datastax.oss.driver.api.core.session.SessionBuilder#addContactPoint}) it is returned + * unresolved, and {@link com.datastax.oss.driver.internal.core.channel.ChannelFactory} expands it + * to every IP it maps to through Netty's configured {@code AddressResolverGroup}. Resolving there + * rather than here is deliberate: it keeps any custom resolver installed via {@link + * com.datastax.oss.driver.internal.core.context.NettyOptions#afterBootstrapInitialized} in the + * loop, which a direct {@code InetAddress.getAllByName()} call from here would bypass, and it + * keeps this method non-blocking so it is safe to call from an event loop. + */ @NonNull @Override public InetSocketAddress resolve() { - return address; + return pinnedAddress != null ? pinnedAddress : address; + } + + @NonNull + @Override + public EndPoint pinTo(@NonNull SocketAddress resolvedAddress) { + Objects.requireNonNull(resolvedAddress, "resolvedAddress can't be null"); + if (!(resolvedAddress instanceof InetSocketAddress) + || resolvedAddress.equals(this.pinnedAddress) + // The address we already hold: pinning to it changes nothing, since resolve() and + // toString() would keep yielding what they already do. Skipping the copy spares toString() + // a + // redundant "addr(addr)" suffix on every already-resolved endpoint -- which is all of them, + // once a node is discovered from the peers rows. + || resolvedAddress.equals(this.address)) { + return this; + } + return new DefaultEndPoint(address, (InetSocketAddress) resolvedAddress); } @Override @@ -68,6 +113,9 @@ public int hashCode() { @Override public String toString() { + // Deliberately identical for a pinned copy: see PinnableEndPoint. Which IP a given connection + // landed on is in the channel's own toString(), which Netty builds from the actual remote + // address. return address.toString(); } diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/PinnableEndPoint.java b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/PinnableEndPoint.java new file mode 100644 index 00000000000..aee8630e09f --- /dev/null +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/PinnableEndPoint.java @@ -0,0 +1,80 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.datastax.oss.driver.internal.core.metadata; + +import com.datastax.oss.driver.api.core.metadata.EndPoint; +import edu.umd.cs.findbugs.annotations.NonNull; +import java.net.SocketAddress; + +/** + * An {@link EndPoint} that can produce a copy of itself bound ("pinned") to one specific address. + * + *

An endpoint whose hostname maps to several IPs describes a set of candidate addresses, + * but a channel is always connected to exactly one of them. {@link + * com.datastax.oss.driver.internal.core.channel.ChannelFactory} pins the endpoint to the address it + * actually used, and hands the pinned copy to the channel. That matters for two reasons: + * + *

    + *
  • Node identity. Once the driver has learnt, over a given connection, that {@code + * host_id} X answers at a given IP, that node must keep reconnecting to that IP. If + * the node kept the multi-address endpoint, a later reconnect could land on a different node + * while still being treated as X (see {@code DefaultTopologyMonitor#buildNodeEndPoint} and + * {@code ControlConnection}, which skip identity re-resolution for nodes that already have a + * host id). + *
  • No re-resolution on the channel path. Components handed the channel's endpoint call + * {@link EndPoint#resolve()} — SSL engine creation, GSSAPI service-name lookup, {@code + * DefaultTopologyMonitor#savePort}. On a pinned endpoint that is a field read, so it neither + * blocks on DNS (SSL setup runs on a Netty event loop) nor risks picking a different address + * than the one the channel is connected to. + *
+ * + *

This is an internal extension point: {@code ChannelFactory} pins endpoints that implement it + * and leaves any other implementation untouched, so third-party {@link EndPoint}s keep working + * exactly as before. + * + *

Implementations must keep {@link Object#equals}, {@link Object#hashCode}, {@link + * EndPoint#asMetricPrefix()} and {@link Object#toString()} identical to the unpinned + * original: a pinned copy denotes the same node, and every one of those is part of how the node is + * identified from the outside. Metric names in particular must not change depending on which IP a + * connection happened to land on — and that includes {@code toString()}, which is what {@code + * TaggingMetricIdGenerator} tags node metrics with, and what any third-party {@code + * MetricIdGenerator} is equally free to use. Nodes do adopt pinned copies (see {@code + * DefaultNode#setEndPoint}), so an identity that varied with the pin would silently re-tag a node's + * metrics mid-session. Equality must also stay symmetric: {@code original.equals(pinned)} and + * {@code pinned.equals(original)} must agree, since endpoints are used as set and map keys. + * + *

The pinned address is therefore observable only through {@link EndPoint#resolve()}. That is no + * loss for diagnostics: the address a channel is actually connected to appears in the channel's own + * {@code toString()}, which Netty builds from its remote address, and {@code ChannelFactory} logs + * each candidate as it tries it. + */ +public interface PinnableEndPoint extends EndPoint { + + /** + * Returns a copy of this endpoint that resolves to exactly {@code resolvedAddress}. + * + *

Implementations may return {@code this} when pinning does not apply (for example when the + * address is not of a type they can hold on to), or when it would be a no-op because the endpoint + * already resolves to exactly that address. + * + * @param resolvedAddress the address a connection was successfully established to; must not be + * null and must already be resolved. + */ + @NonNull + EndPoint pinTo(@NonNull SocketAddress resolvedAddress); +} diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/SniEndPoint.java b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/SniEndPoint.java index d1ab8eec98d..69c0c1151e7 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/SniEndPoint.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/SniEndPoint.java @@ -18,61 +18,113 @@ package com.datastax.oss.driver.internal.core.metadata; import com.datastax.oss.driver.api.core.metadata.EndPoint; -import com.datastax.oss.driver.shaded.guava.common.primitives.UnsignedBytes; +import com.datastax.oss.driver.internal.core.util.AddressUtils; import edu.umd.cs.findbugs.annotations.NonNull; -import java.net.InetAddress; +import edu.umd.cs.findbugs.annotations.Nullable; import java.net.InetSocketAddress; -import java.net.UnknownHostException; -import java.util.Arrays; -import java.util.Comparator; +import java.net.SocketAddress; import java.util.Objects; -import java.util.concurrent.atomic.AtomicInteger; -public class SniEndPoint implements EndPoint { - private static final AtomicInteger OFFSET = new AtomicInteger(); +public class SniEndPoint implements PinnableEndPoint { private final InetSocketAddress proxyAddress; private final String serverName; /** - * @param proxyAddress the address of the proxy. If it is {@linkplain - * InetSocketAddress#isUnresolved() unresolved}, each call to {@link #resolve()} will - * re-resolve it, fetch all of its A-records, and if there are more than 1 pick one in a - * round-robin fashion. + * The proxy IP this endpoint has been {@linkplain #pinTo(SocketAddress) pinned} to, or {@code + * null} if it is not pinned. Deliberately excluded from {@link #equals} and {@link #hashCode}: a + * pinned copy denotes the same node as the original. + */ + @Nullable private final InetSocketAddress pinnedAddress; + + /** + * @param proxyAddress the address of the proxy. A proxy hostname is stored {@linkplain + * InetSocketAddress#isUnresolved() unresolved}, whether or not it was supplied that way, so + * that the driver expands it to all of the proxy's A-records at connection time and tries + * each of them — see {@link #keepHostnameUnresolved}. A proxy given as an IP address is + * stored as-is. * @param serverName the SNI server name. In the context of Cloud, this is the string * representation of the host id. */ public SniEndPoint(InetSocketAddress proxyAddress, String serverName) { - this.proxyAddress = Objects.requireNonNull(proxyAddress, "SNI address cannot be null"); + this(proxyAddress, serverName, null); + } + + private SniEndPoint( + InetSocketAddress proxyAddress, + String serverName, + @Nullable InetSocketAddress pinnedAddress) { + this.proxyAddress = + keepHostnameUnresolved(Objects.requireNonNull(proxyAddress, "SNI address cannot be null")); this.serverName = Objects.requireNonNull(serverName, "SNI Server name cannot be null"); + this.pinnedAddress = pinnedAddress; + } + + /** + * Turns a proxy address that names a host into an unresolved one, leaving anything else + * untouched. + * + *

{@link #resolve()} hands the stored address to the connection layer as-is, and only an + * unresolved one gets expanded and re-expanded there. A proxy hostname supplied already resolved + * would therefore stay bound to whichever single IP its lookup happened to return, for the life + * of the session: no spreading across the proxy's A-records, no fallback when that one IP stops + * answering, and no pick-up of a DNS change. That is a real possibility for a hostname handed to + * {@link + * com.datastax.oss.driver.api.core.session.SessionBuilder#withCloudProxyAddress(InetSocketAddress)}, + * because the ordinary {@code InetSocketAddress(String, int)} constructor resolves eagerly. + * ({@code CloudConfigFactory}, the usual path, already builds an unresolved address.) + * + *

Normalizing here rather than at the call site keeps every {@code SniEndPoint} built from the + * same proxy comparable — {@link #equals} keys on this field — and matches what this endpoint did + * before resolution moved to the connection layer, when it re-resolved the proxy hostname on + * every {@code resolve()} call. + */ + private static InetSocketAddress keepHostnameUnresolved(InetSocketAddress proxyAddress) { + return proxyAddress.isUnresolved() || !AddressUtils.carriesName(proxyAddress) + ? proxyAddress + : InetSocketAddress.createUnresolved(proxyAddress.getHostString(), proxyAddress.getPort()); } public String getServerName() { return serverName; } + /** + * Returns the proxy address connections should be opened to. + * + *

Unpinned, this is the stored proxy address as-is. For Cloud that is a hostname, kept + * unresolved (see {@link #keepHostnameUnresolved}), which {@link + * com.datastax.oss.driver.internal.core.channel.ChannelFactory} expands to every proxy A-record, + * trying each in turn — so a single unreachable proxy IP no longer fails the connection. + * Re-resolving here instead would block whichever event loop called us, and would bypass a custom + * Netty resolver. + * + *

Once {@linkplain #pinTo(SocketAddress) pinned} this returns that one proxy IP. That is what + * {@link com.datastax.oss.driver.internal.core.ssl.SniSslEngineFactory#newSslEngine} sees: it + * runs inside Netty's channel initializer, so it gets the exact IP the channel is connected to + * without a lookup on the event loop. + */ @NonNull @Override public InetSocketAddress resolve() { - try { - InetAddress[] aRecords = InetAddress.getAllByName(proxyAddress.getHostName()); - if (aRecords.length == 0) { - // Probably never happens, but the JDK docs don't explicitly say so - throw new IllegalArgumentException( - "Could not resolve proxy address " + proxyAddress.getHostName()); - } - // The order of the returned address is unspecified. Sort by IP to make sure we get a true - // round-robin - Arrays.sort(aRecords, IP_COMPARATOR); - int index = - (aRecords.length == 1) - ? 0 - : OFFSET.getAndUpdate(x -> x == Integer.MAX_VALUE ? 0 : x + 1) % aRecords.length; - return new InetSocketAddress(aRecords[index], proxyAddress.getPort()); - } catch (UnknownHostException e) { - throw new IllegalArgumentException( - "Could not resolve proxy address " + proxyAddress.getHostName(), e); + return pinnedAddress != null ? pinnedAddress : proxyAddress; + } + + @NonNull + @Override + public EndPoint pinTo(@NonNull SocketAddress resolvedAddress) { + Objects.requireNonNull(resolvedAddress, "resolvedAddress cannot be null"); + if (!(resolvedAddress instanceof InetSocketAddress) + || resolvedAddress.equals(this.pinnedAddress) + // The address we already hold: pinning to it changes nothing -- resolve() and toString() + // keep yielding what they already do -- so spare the copy (and its redundant + // "proxy(proxy)" toString suffix). Only reachable when the proxy was given as an IP + // address: a proxy hostname is stored unresolved, and a resolved pin never compares equal + // to that. + || resolvedAddress.equals(this.proxyAddress)) { + return this; } + return new SniEndPoint(proxyAddress, serverName, (InetSocketAddress) resolvedAddress); } @Override @@ -94,10 +146,10 @@ public int hashCode() { @Override public String toString() { - // Note that this uses the original proxy address, so if there are multiple A-records it won't - // show which one was selected. If that turns out to be a problem for debugging, we might need - // to store the result of resolve() in Connection and log that instead of the endpoint. - return proxyAddress.toString() + ":" + serverName; + // Deliberately identical for a pinned copy: see PinnableEndPoint. Which proxy IP a given + // connection landed on is in the channel's own toString(), which Netty builds from the actual + // remote address. + return proxyAddress + ":" + serverName; } @NonNull @@ -110,10 +162,4 @@ public String asMetricPrefix() { } return hostString.replace('.', '_') + ':' + proxyAddress.getPort() + '_' + serverName; } - - @SuppressWarnings("UnnecessaryLambda") - private static final Comparator IP_COMPARATOR = - (InetAddress address1, InetAddress address2) -> - UnsignedBytes.lexicographicalComparator() - .compare(address1.getAddress(), address2.getAddress()); } diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/TopologyMonitor.java b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/TopologyMonitor.java index 1bb8e343d96..9be8c4c94bd 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/TopologyMonitor.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/TopologyMonitor.java @@ -141,4 +141,31 @@ public interface TopologyMonitor extends AsyncAutoCloseable { * {@link DefaultTopologyMonitor}) should override this method. */ default void resetColumnCaches() {} + + /** + * Whether this monitor re-resolves node addresses dynamically on every connection attempt (for + * example by re-resolving a proxy hostname each time), rather than relying on an endpoint address + * captured once at node-registration time. + * + *

When this returns {@code true}, the control connection's reconnection query plan must not + * append the original contact points as a DNS re-resolution fallback (see {@code + * advanced.control-connection.reconnection.fallback-to-original-contact-points}): the monitor + * already keeps addresses fresh, and appending raw contact points could resurrect nodes that the + * monitor has authoritatively removed. + * + *

The default implementation returns {@code false}, which is correct for {@link + * DefaultTopologyMonitor}: the peer nodes it registers hold a {@code DefaultEndPoint} built from + * an already-resolved physical IP (from {@code system.peers}), which never needs re-resolving. + * + *

The connected node's own {@code EndPoint} is a different case again. It originates from the + * contact point the control connection used, and {@code ChannelFactory} binds it to the single + * address that connection reached (see {@code PinnableEndPoint}), so it does not re-expand + * on later connection attempts. Recovering from an address change for that node therefore depends + * on this flag being {@code false}, i.e. on the contact-point fallback described above. + * + *

Proxy-based monitors that re-resolve per call should override this to return {@code true}. + */ + default boolean reresolvesNodeAddresses() { + return false; + } } diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/util/AddressUtils.java b/core/src/main/java/com/datastax/oss/driver/internal/core/util/AddressUtils.java index 8905edb9192..7d408fb999b 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/util/AddressUtils.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/util/AddressUtils.java @@ -18,6 +18,7 @@ package com.datastax.oss.driver.internal.core.util; import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableSet; +import com.datastax.oss.driver.shaded.guava.common.net.InetAddresses; import java.net.InetAddress; import java.net.InetSocketAddress; import java.net.UnknownHostException; @@ -56,4 +57,36 @@ public static Set extract(String address, boolean resolve) { return result; } } + + /** + * Whether {@code address} denotes a host name, as opposed to an IP address written out in + * literal form. + * + *

The distinction matters wherever a name is treated as something that can be resolved — and + * re-resolved — while a literal is taken as the final answer. Both forms can appear resolved or + * unresolved, so neither {@link InetSocketAddress#isUnresolved()} nor the presence of an {@link + * InetAddress} tells them apart. + * + *

Performs no lookup of any kind. + * + *

One known imprecision, on the unresolved branch: {@code InetAddresses.isInetAddress} rejects + * a zone suffix, so an unresolved address built over a scoped IPv6 literal (say {@code + * fe80::1%eth0}) is reported as a name. Harmless where this is used — the string still resolves + * to exactly that address, and re-attaching it as a "host name" is a no-op — and it does not + * affect the resolved branch, where {@code getHostString()} and {@code getHostAddress()} both + * carry the zone and so compare equal. + */ + public static boolean carriesName(InetSocketAddress address) { + String hostString = address.getHostString(); + if (hostString == null) { + return false; + } + // A resolved address is compared against the literal its own bytes produce, which is cheaper + // and + // stricter than parsing; an unresolved one has no bytes, so its string has to be parsed. + InetAddress ip = address.getAddress(); + return ip != null + ? !hostString.equals(ip.getHostAddress()) + : !InetAddresses.isInetAddress(hostString); + } } diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPointTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPointTest.java index f31dd2861ed..296390c53e7 100644 --- a/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPointTest.java +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPointTest.java @@ -18,11 +18,12 @@ package com.datastax.oss.driver.internal.core.metadata; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.datastax.oss.driver.api.core.metadata.EndPoint; -import java.io.UncheckedIOException; +import io.netty.channel.local.LocalAddress; import java.net.InetAddress; import java.net.InetSocketAddress; import java.net.UnknownHostException; @@ -66,20 +67,23 @@ public void should_fallback_when_resolve_returns_null() throws UnknownHostExcept } @Test - public void should_wrap_io_exceptions_in_unchecked_io_exception() throws UnknownHostException { + public void should_return_the_route_address_unresolved() { + // The route hostname is handed over unresolved on purpose: ChannelFactory resolves it through + // Netty's AddressResolverGroup, so a custom resolver applies to client routes too and no DNS + // lookup runs on the admin event loop that connect() is called from. UUID hostId = UUID.randomUUID(); - when(topologyMonitor.resolve(hostId)).thenThrow(new UnknownHostException("no-such-host")); + InetSocketAddress route = InetSocketAddress.createUnresolved("route.example.com", 9042); + when(topologyMonitor.resolve(hostId)).thenReturn(route); ClientRoutesEndPoint ep = new ClientRoutesEndPoint(topologyMonitor, hostId, null, fallbackEndPoint); - assertThatThrownBy(ep::resolve) - .isInstanceOf(UncheckedIOException.class) - .hasCauseInstanceOf(UnknownHostException.class); + assertThat(ep.resolve()).isSameAs(route); + assertThat(((InetSocketAddress) ep.resolve()).isUnresolved()).isTrue(); } @Test - public void should_reflect_route_changes_on_subsequent_resolve() throws UnknownHostException { + public void should_reflect_route_changes_on_subsequent_resolve() { UUID hostId = UUID.randomUUID(); InetSocketAddress addr1 = new InetSocketAddress("127.0.0.1", 9042); InetSocketAddress addr2 = new InetSocketAddress("10.0.0.1", 9043); @@ -96,6 +100,48 @@ public void should_reflect_route_changes_on_subsequent_resolve() throws UnknownH assertThat(ep.resolve()).isEqualTo(addr2); } + // ---- pinTo() ------------------------------------------------------------ + + @Test + public void pin_to_should_stop_consulting_the_topology_monitor() { + UUID hostId = UUID.randomUUID(); + InetSocketAddress pinnedTo = new InetSocketAddress("127.0.0.1", 9042); + + ClientRoutesEndPoint original = + new ClientRoutesEndPoint(topologyMonitor, hostId, null, fallbackEndPoint); + EndPoint pinned = original.pinTo(pinnedTo); + + assertThat(pinned.resolve()).isEqualTo(pinnedTo); + // No lookup at all -- that is the point: DefaultTopologyMonitor#savePort and the SSL factories + // read the channel's endpoint, and must not trigger a blocking re-resolution there. + verify(topologyMonitor, never()).resolve(hostId); + // Identity is keyed off the host id, so the pinned copy is still the same node. + assertThat(pinned).isEqualTo(original); + assertThat(original).isEqualTo(pinned); + assertThat(pinned.asMetricPrefix()).isEqualTo(original.asMetricPrefix()); + } + + @Test + public void pin_to_should_return_same_instance_when_already_pinned_to_that_address() { + ClientRoutesEndPoint original = + new ClientRoutesEndPoint(topologyMonitor, UUID.randomUUID(), null, fallbackEndPoint); + InetSocketAddress pinnedTo = new InetSocketAddress("127.0.0.1", 9042); + + EndPoint pinned = original.pinTo(pinnedTo); + + assertThat(((ClientRoutesEndPoint) pinned).pinTo(pinnedTo)).isSameAs(pinned); + } + + @Test + public void pin_to_should_be_a_no_op_for_a_non_inet_address() { + // Mirror DefaultEndPoint: an address that cannot be held in an InetSocketAddress field (e.g. + // the local transport used by unit tests) skips pinning rather than failing the connection. + ClientRoutesEndPoint endPoint = + new ClientRoutesEndPoint(topologyMonitor, UUID.randomUUID(), null, fallbackEndPoint); + + assertThat(endPoint.pinTo(new LocalAddress("some-id"))).isSameAs(endPoint); + } + // ---- equals / hashCode -------------------------------------------------- @Test diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitorTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitorTest.java index a1ba4617ef5..e6ab0895034 100644 --- a/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitorTest.java +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitorTest.java @@ -28,6 +28,8 @@ import com.datastax.oss.driver.api.core.config.DriverConfig; import com.datastax.oss.driver.api.core.config.DriverExecutionProfile; import com.datastax.oss.driver.api.core.metadata.EndPoint; +import com.datastax.oss.driver.api.core.metadata.Metadata; +import com.datastax.oss.driver.api.core.metadata.Node; import com.datastax.oss.driver.internal.core.adminrequest.AdminResult; import com.datastax.oss.driver.internal.core.adminrequest.AdminRow; import com.datastax.oss.driver.internal.core.channel.DriverChannel; @@ -66,6 +68,8 @@ public class ClientRoutesTopologyMonitorTest { @Mock private ControlConnection controlConnection; @Mock private DriverConfig driverConfig; @Mock private DriverExecutionProfile defaultProfile; + @Mock private MetadataManager metadataManager; + @Mock private Metadata metadata; private TestableClientRoutesTopologyMonitor handler; @@ -194,14 +198,21 @@ public void should_throw_after_close() { } @Test - public void should_throw_for_unresolvable_hostname() { + public void should_not_look_up_the_route_hostname() { UUID hostId = UUID.randomUUID(); - // Use a hostname guaranteed not to resolve + // A hostname guaranteed not to resolve: this must still succeed, because resolve() is a pure + // in-memory cache lookup that hands the name over unresolved. ChannelFactory resolves it later + // through Netty's AddressResolverGroup, so a custom resolver applies to client routes too and + // nothing blocks the admin event loop here. handler.setRoutes( ImmutableMap.of( hostId, new ClientRouteRecord(hostId, "this.host.does.not.exist.invalid", 9042))); - assertThatThrownBy(() -> handler.resolve(hostId)).isInstanceOf(UnknownHostException.class); + InetSocketAddress result = handler.resolve(hostId); + + assertThat(result.isUnresolved()).isTrue(); + assertThat(result.getHostString()).isEqualTo("this.host.does.not.exist.invalid"); + assertThat(result.getPort()).isEqualTo(9042); } @Test @@ -220,6 +231,57 @@ public void should_refresh_updates_routes() throws UnknownHostException { assertThat(handler.resolve(hostId2)).isNotNull(); } + // ---- reresolvesNodeAddresses() ------------------------------------------- + + @Test + public void should_reresolve_when_all_known_nodes_have_client_routes() { + UUID hostId1 = UUID.randomUUID(); + UUID hostId2 = UUID.randomUUID(); + Node node1 = Mockito.mock(Node.class); + when(node1.getHostId()).thenReturn(hostId1); + Node node2 = Mockito.mock(Node.class); + when(node2.getHostId()).thenReturn(hostId2); + + when(context.getMetadataManager()).thenReturn(metadataManager); + when(metadataManager.getMetadata()).thenReturn(metadata); + when(metadata.getNodes()).thenReturn(ImmutableMap.of(hostId1, node1, hostId2, node2)); + + handler.setRoutes( + ImmutableMap.of( + hostId1, new ClientRouteRecord(hostId1, "127.0.0.1", 9042), + hostId2, new ClientRouteRecord(hostId2, "127.0.0.2", 9042))); + + assertThat(handler.reresolvesNodeAddresses()).isTrue(); + } + + @Test + public void should_not_reresolve_when_a_known_node_has_no_client_route() { + UUID hostId1 = UUID.randomUUID(); + UUID hostId2 = UUID.randomUUID(); + Node node1 = Mockito.mock(Node.class); + when(node1.getHostId()).thenReturn(hostId1); + Node node2 = Mockito.mock(Node.class); + when(node2.getHostId()).thenReturn(hostId2); + + when(context.getMetadataManager()).thenReturn(metadataManager); + when(metadataManager.getMetadata()).thenReturn(metadata); + when(metadata.getNodes()).thenReturn(ImmutableMap.of(hostId1, node1, hostId2, node2)); + + // Only node1 has a live client route; node2 would fall back to a static endpoint. + handler.setRoutes(ImmutableMap.of(hostId1, new ClientRouteRecord(hostId1, "127.0.0.1", 9042))); + + assertThat(handler.reresolvesNodeAddresses()).isFalse(); + } + + @Test + public void should_reresolve_when_no_nodes_known_yet() { + when(context.getMetadataManager()).thenReturn(metadataManager); + when(metadataManager.getMetadata()).thenReturn(metadata); + when(metadata.getNodes()).thenReturn(Collections.emptyMap()); + + assertThat(handler.reresolvesNodeAddresses()).isTrue(); + } + // ---- Merge behavior tests ----------------------------------------------- @Test diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPointTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPointTest.java index 7da8fb39415..c92c7b8e20c 100644 --- a/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPointTest.java +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPointTest.java @@ -20,6 +20,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import com.datastax.oss.driver.api.core.metadata.EndPoint; +import io.netty.channel.local.LocalAddress; import java.net.InetSocketAddress; import org.junit.Test; @@ -57,4 +59,107 @@ public void should_reject_null_address() { .isInstanceOf(NullPointerException.class) .hasMessage("address can't be null"); } + + @Test + public void resolve_returns_already_resolved_address_as_is() { + DefaultEndPoint endPoint = new DefaultEndPoint(new InetSocketAddress("127.0.0.1", 9042)); + InetSocketAddress resolved = endPoint.resolve(); + assertThat(resolved.isUnresolved()).isFalse(); + assertThat(resolved.getHostString()).isEqualTo("127.0.0.1"); + } + + @Test + public void resolve_passes_unresolved_hostname_through_without_looking_it_up() { + // This endpoint does NOT resolve hostnames itself. It hands the unresolved address to + // ChannelFactory, which expands it through Netty's AddressResolverGroup so that a custom + // resolver installed via NettyOptions#afterBootstrapInitialized still applies -- a direct + // InetAddress.getAllByName() call here would bypass it, and would block the admin event loop + // that connect() runs on. "localhost" would resolve fine, so this assertion is only meaningful + // because we check the address comes back *unresolved*. + DefaultEndPoint endPoint = + new DefaultEndPoint(InetSocketAddress.createUnresolved("localhost", 9042)); + + InetSocketAddress resolved = endPoint.resolve(); + + assertThat(resolved.isUnresolved()).isTrue(); + assertThat(resolved.getHostString()).isEqualTo("localhost"); + assertThat(resolved.getPort()).isEqualTo(9042); + } + + @Test + public void resolve_does_not_throw_for_unresolvable_hostname() { + // No lookup happens, so an unresolvable name is not an error at this level: the connect attempt + // fails later with a descriptive error instead. + DefaultEndPoint endPoint = + new DefaultEndPoint( + InetSocketAddress.createUnresolved("this-host-does-not-exist.invalid", 9042)); + + assertThat(endPoint.resolve().getHostString()).isEqualTo("this-host-does-not-exist.invalid"); + } + + @Test + public void pin_to_should_override_resolution_but_preserve_identity() { + InetSocketAddress hostname = InetSocketAddress.createUnresolved("test.com", 9042); + DefaultEndPoint original = new DefaultEndPoint(hostname); + InetSocketAddress pinnedTo = new InetSocketAddress("127.0.0.1", 9042); + + EndPoint pinned = original.pinTo(pinnedTo); + + // Resolution now yields exactly the pinned address... + assertThat(pinned.resolve()).isEqualTo(pinnedTo); + // ...but the copy still denotes the same node, and metric names must not change depending on + // which IP a connection happened to land on -- including through toString(), which is what + // TaggingMetricIdGenerator tags node metrics with, and nodes do adopt pinned copies. + assertThat(pinned.asMetricPrefix()).isEqualTo(original.asMetricPrefix()); + assertThat(pinned.toString()).isEqualTo(original.toString()); + assertThat(pinned).isEqualTo(original); + assertThat(pinned.hashCode()).isEqualTo(original.hashCode()); + // Equality has to hold in both directions: endpoints are used as set and map keys. + assertThat(original).isEqualTo(pinned); + // The original is untouched. + assertThat(original.resolve()).isEqualTo(hostname); + } + + @Test + public void pin_to_should_return_same_instance_when_already_pinned_to_that_address() { + DefaultEndPoint original = + new DefaultEndPoint(InetSocketAddress.createUnresolved("test.com", 9042)); + InetSocketAddress pinnedTo = new InetSocketAddress("127.0.0.1", 9042); + + EndPoint pinned = original.pinTo(pinnedTo); + + assertThat(((DefaultEndPoint) pinned).pinTo(pinnedTo)).isSameAs(pinned); + } + + @Test + public void pin_to_should_return_same_instance_when_address_is_already_the_endpoints_own() { + // An already-resolved endpoint expands to exactly one candidate -- itself -- so ChannelFactory + // pins it to the address it already holds. That copy would be indistinguishable from the + // original in every respect, so there is no point allocating it. Every node discovered from the + // peers rows takes this path. + InetSocketAddress resolved = new InetSocketAddress("127.0.0.1", 9042); + DefaultEndPoint endPoint = new DefaultEndPoint(resolved); + + assertThat(endPoint.pinTo(new InetSocketAddress("127.0.0.1", 9042))).isSameAs(endPoint); + assertThat(endPoint.toString()).isEqualTo(resolved.toString()); + } + + @Test + public void pin_to_should_reject_null_address() { + DefaultEndPoint endPoint = new DefaultEndPoint(new InetSocketAddress("127.0.0.1", 9042)); + assertThatThrownBy(() -> endPoint.pinTo(null)) + .isInstanceOf(NullPointerException.class) + .hasMessage("resolvedAddress can't be null"); + } + + @Test + public void pin_to_should_be_a_no_op_for_a_non_inet_address() { + // ChannelFactory pins whatever address it connected to; a non-Inet one (e.g. the local + // transport + // used by unit tests) cannot be held in an InetSocketAddress field, so pinning is skipped + // rather + // than failing the connection. + DefaultEndPoint endPoint = new DefaultEndPoint(new InetSocketAddress("127.0.0.1", 9042)); + assertThat(endPoint.pinTo(new LocalAddress("some-id"))).isSameAs(endPoint); + } } diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/SniEndPointTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/SniEndPointTest.java new file mode 100644 index 00000000000..63484ec21d9 --- /dev/null +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/SniEndPointTest.java @@ -0,0 +1,139 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.datastax.oss.driver.internal.core.metadata; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.net.InetSocketAddress; +import org.junit.Test; + +public class SniEndPointTest { + + @Test + public void resolve_returns_the_proxy_address_as_is_without_looking_it_up() { + // The proxy address is a hostname (that is how CloudConfigFactory builds it) and this endpoint + // must not resolve it: ChannelFactory expands it through Netty's AddressResolverGroup, which is + // what makes a custom resolver apply to the SNI proxy too, and what keeps resolve() safe to + // call + // from an event loop -- SniSslEngineFactory#newSslEngine does exactly that. + InetSocketAddress proxy = InetSocketAddress.createUnresolved("proxy.example.com", 9042); + SniEndPoint endPoint = new SniEndPoint(proxy, "test-server-name"); + + assertThat(endPoint.resolve()).isSameAs(proxy); + assertThat(endPoint.resolve().isUnresolved()).isTrue(); + } + + @Test + public void should_keep_a_resolved_proxy_hostname_unresolved() { + // InetSocketAddress(String, int) resolves eagerly, so a hostname passed to + // withCloudProxyAddress() arrives here already bound to one of its IPs. Storing it that way + // would freeze every Cloud connection on that IP for the life of the session: resolve() hands + // the stored address straight to the connection layer, which only expands unresolved ones. + InetSocketAddress resolvedProxy = new InetSocketAddress("localhost", 9042); + assertThat(resolvedProxy.isUnresolved()).isFalse(); + + SniEndPoint endPoint = new SniEndPoint(resolvedProxy, "test-server-name"); + + assertThat(endPoint.resolve().isUnresolved()).isTrue(); + assertThat(endPoint.resolve().getHostString()).isEqualTo("localhost"); + assertThat(endPoint.resolve().getPort()).isEqualTo(9042); + // Normalization is unconditional, so endpoints built from either form of the same proxy still + // denote the same node -- equals() keys on the stored address. + assertThat(endPoint) + .isEqualTo( + new SniEndPoint( + InetSocketAddress.createUnresolved("localhost", 9042), "test-server-name")); + // The metric prefix is unaffected either way: it was already built from the host string. + assertThat(endPoint.asMetricPrefix()).isEqualTo("localhost:9042_test-server-name"); + } + + @Test + public void should_keep_a_proxy_given_as_an_ip_address_as_is() { + // Nothing to re-resolve: an IP address is the final answer, and turning it into an unresolved + // "hostname" would only send a literal through the resolver on every connect. + InetSocketAddress ipProxy = new InetSocketAddress("127.0.0.1", 9042); + + SniEndPoint endPoint = new SniEndPoint(ipProxy, "test-server-name"); + + assertThat(endPoint.resolve()).isSameAs(ipProxy); + assertThat(endPoint.resolve().isUnresolved()).isFalse(); + } + + @Test + public void resolve_does_not_throw_for_unresolvable_proxy_hostname() { + // No lookup happens here, so an unresolvable name only fails later, at connect time. + SniEndPoint endPoint = + new SniEndPoint( + InetSocketAddress.createUnresolved("this-host-does-not-exist.invalid", 9042), + "test-server-name"); + + assertThat(endPoint.resolve().getHostString()).isEqualTo("this-host-does-not-exist.invalid"); + } + + @Test + public void pin_to_should_make_resolve_return_the_connected_proxy_ip_and_preserve_identity() { + // Pinning is what lets SniSslEngineFactory#newSslEngine -- which runs inside Netty's channel + // initializer -- see the very proxy IP the channel is connected to, rather than the hostname or + // another A-record. + SniEndPoint original = + new SniEndPoint( + InetSocketAddress.createUnresolved("proxy.example.com", 9042), "test-server-name"); + InetSocketAddress pinnedTo = new InetSocketAddress("127.0.0.1", 9042); + + SniEndPoint pinned = (SniEndPoint) original.pinTo(pinnedTo); + + assertThat(pinned.resolve()).isEqualTo(pinnedTo); + assertThat(pinned.resolve().isUnresolved()).isFalse(); + // The original is untouched. + assertThat(original.resolve().isUnresolved()).isTrue(); + + // The pinned copy still denotes the same node, down to every string it is identified by: the + // tagging MetricIdGenerator tags node metrics with the endpoint's toString(), and nodes do + // adopt + // pinned copies. + assertThat(pinned.getServerName()).isEqualTo(original.getServerName()); + assertThat(pinned.asMetricPrefix()).isEqualTo(original.asMetricPrefix()); + assertThat(pinned.toString()).isEqualTo(original.toString()); + assertThat(pinned).isEqualTo(original); + assertThat(original).isEqualTo(pinned); + assertThat(pinned.hashCode()).isEqualTo(original.hashCode()); + } + + @Test + public void pin_to_should_return_same_instance_when_already_pinned_to_that_address() { + SniEndPoint original = + new SniEndPoint( + InetSocketAddress.createUnresolved("proxy.example.com", 9042), "test-server-name"); + InetSocketAddress pinnedTo = new InetSocketAddress("127.0.0.1", 9042); + + SniEndPoint pinned = (SniEndPoint) original.pinTo(pinnedTo); + + assertThat(pinned.pinTo(pinnedTo)).isSameAs(pinned); + } + + @Test + public void pin_to_should_return_same_instance_when_address_is_the_proxy_address_itself() { + // Only reachable when the proxy was given as an IP address (Cloud supplies a hostname, which is + // stored unresolved): pinning to the very address the endpoint holds is a no-op, so there is no + // point allocating a copy that would be indistinguishable from it. + InetSocketAddress resolvedProxy = new InetSocketAddress("127.0.0.1", 9042); + SniEndPoint endPoint = new SniEndPoint(resolvedProxy, "test-server-name"); + + assertThat(endPoint.pinTo(new InetSocketAddress("127.0.0.1", 9042))).isSameAs(endPoint); + } +} diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/util/AddressUtilsTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/util/AddressUtilsTest.java new file mode 100644 index 00000000000..f48d70ef838 --- /dev/null +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/util/AddressUtilsTest.java @@ -0,0 +1,60 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.datastax.oss.driver.internal.core.util; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.net.InetAddress; +import java.net.InetSocketAddress; +import org.junit.Test; + +public class AddressUtilsTest { + + @Test + public void should_recognize_a_hostname_whether_resolved_or_not() { + assertThat( + AddressUtils.carriesName(InetSocketAddress.createUnresolved("host.example.com", 9042))) + .isTrue(); + // Eagerly resolved by the constructor, but still a name. + assertThat(AddressUtils.carriesName(new InetSocketAddress("localhost", 9042))).isTrue(); + } + + @Test + public void should_not_mistake_an_ip_literal_for_a_hostname() throws Exception { + assertThat(AddressUtils.carriesName(new InetSocketAddress("127.0.0.1", 9042))).isFalse(); + assertThat(AddressUtils.carriesName(InetSocketAddress.createUnresolved("127.0.0.1", 9042))) + .isFalse(); + assertThat(AddressUtils.carriesName(InetSocketAddress.createUnresolved("::1", 9042))).isFalse(); + // Built from raw bytes, so it carries no name at all and getHostString() falls back to the + // literal -- without triggering the reverse lookup that getHostName() would. + assertThat( + AddressUtils.carriesName( + new InetSocketAddress(InetAddress.getByAddress(new byte[] {10, 0, 0, 1}), 9042))) + .isFalse(); + } + + @Test + public void should_report_an_explicitly_named_address_as_a_name() throws Exception { + // A resolver may label its results with a name of its own; that is still a name. + assertThat( + AddressUtils.carriesName( + new InetSocketAddress( + InetAddress.getByAddress("cname.example.com", new byte[] {10, 0, 0, 1}), 9042))) + .isTrue(); + } +} From 90944080bc1cedc15e2c625903b49006d59c2cc4 Mon Sep 17 00:00:00 2001 From: Mikita Hradovich Date: Thu, 6 Aug 2026 18:15:52 +0200 Subject: [PATCH 4/9] feat: try every address a hostname resolves to when connecting (DRIVER-201) This is the fix for DRIVER-201. When a contact point or a node address is a hostname that maps to several IPs, the driver used to try only the first one and raise AllNodesFailedException if it was unreachable, even though the hostname also resolved to healthy addresses. Resolution is a connection-layer concern. ChannelFactory.connect() is now the single place that turns "the address this node is known by" into "the addresses to actually try": EndPoint.resolve() yields one address and does no lookup, so it stays safe to call from an event loop; ChannelFactory expands it through the bootstrap's Netty AddressResolverGroup; the candidates are tried in sequence until one connects; and the endpoint is pinned to the address that won, so the channel carries the address it is really on. Expansion always goes through the configured resolver, mirroring Netty's own doResolveAndConnect0 short-circuit (no group, !isSupported, isResolved) rather than pre-filtering on isUnresolved(). Both isSupported() and isResolved() are overridable, so a redirecting custom resolver keeps its say over addresses that merely look resolved. The bootstrap is built once per connect() and cloned per attempt, with the clone's resolver disabled: Bootstrap.clone() carries the resolver over, so an enabled clone would resolve each candidate a second time -- through resolve(), singular -- and a redirecting resolver would collapse every candidate onto its first answer, silently killing the fallback. Details that took a round each to get right: - The queried hostname is re-attached to resolver-returned addresses, centrally rather than per endpoint, so TLS sees the name the user configured instead of an IP or a CNAME label. Scoped IPv6 keeps its zone via the numeric Inet6Address.getByAddress overload; the NetworkInterface one re-derives the scope and throws when the interface has no address of the same local type. - One EventLoop is chosen per connect() and shared by resolution and every clone(eventLoop), instead of letting Bootstrap.connect() advance the chooser a second time and land channels on half the loops. - Candidate rotation uses a per-name counter held by the factory (per session) behind a bounded LoadingCache, since client routes can churn hostnames within one session. - Protocol-version rejection is terminal only for a node whose host id is known. The addresses of an unidentified endpoint may belong to different nodes, and collapsing a contact-point hostname into one Node must not lose the query-plan advance that resolve-contact-points=true used to provide. - Failures from earlier candidates are attached to the final error as suppressed exceptions, and negotiation history is scoped per candidate address. - Every resolver and Netty callback completes the connect future on failure. connect() has no timeout at the resolution stage, so an unguarded throw would hang the caller for good. afterBootstrapInitialized() now runs once per logical connection rather than once per attempt, and sees the bootstrap before the driver's handler is installed; a handler set by the hook is overwritten, with a one-time warning. Co-Authored-By: Claude Opus 5 (1M context) --- .../internal/core/channel/ChannelFactory.java | 785 ++++++++++++++++-- .../internal/core/context/NettyOptions.java | 16 +- .../ChannelFactoryBootstrapHookTest.java | 71 ++ .../ChannelFactoryMultiAddressTest.java | 452 ++++++++++ .../ChannelFactoryNettyResolverTest.java | 379 +++++++++ .../ChannelFactoryPinnedEndPointTest.java | 189 +++++ ...ChannelFactoryProtocolNegotiationTest.java | 166 ++++ .../core/channel/ChannelFactoryTestBase.java | 37 + .../channel/TestAddressResolverGroup.java | 130 +++ 9 files changed, 2136 insertions(+), 89 deletions(-) create mode 100644 core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryBootstrapHookTest.java create mode 100644 core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryMultiAddressTest.java create mode 100644 core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryNettyResolverTest.java create mode 100644 core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryPinnedEndPointTest.java create mode 100644 core/src/test/java/com/datastax/oss/driver/internal/core/channel/TestAddressResolverGroup.java diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java b/core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java index 6bfc355f910..de6179d6d27 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java @@ -39,13 +39,18 @@ import com.datastax.oss.driver.internal.core.context.InternalDriverContext; import com.datastax.oss.driver.internal.core.context.NettyOptions; import com.datastax.oss.driver.internal.core.metadata.DefaultNode; +import com.datastax.oss.driver.internal.core.metadata.PinnableEndPoint; import com.datastax.oss.driver.internal.core.metrics.NodeMetricUpdater; import com.datastax.oss.driver.internal.core.metrics.NoopNodeMetricUpdater; import com.datastax.oss.driver.internal.core.metrics.SessionMetricUpdater; import com.datastax.oss.driver.internal.core.protocol.FrameDecoder; import com.datastax.oss.driver.internal.core.protocol.FrameEncoder; +import com.datastax.oss.driver.internal.core.util.AddressUtils; import com.datastax.oss.driver.shaded.guava.common.annotations.VisibleForTesting; import com.datastax.oss.driver.shaded.guava.common.base.Preconditions; +import com.datastax.oss.driver.shaded.guava.common.cache.CacheBuilder; +import com.datastax.oss.driver.shaded.guava.common.cache.CacheLoader; +import com.datastax.oss.driver.shaded.guava.common.cache.LoadingCache; import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableMap; import com.datastax.oss.protocol.internal.ProtocolFeatures; import io.netty.bootstrap.Bootstrap; @@ -54,14 +59,27 @@ import io.netty.channel.ChannelInitializer; import io.netty.channel.ChannelOption; import io.netty.channel.ChannelPipeline; +import io.netty.channel.EventLoop; +import io.netty.resolver.AddressResolver; +import io.netty.resolver.AddressResolverGroup; +import io.netty.util.concurrent.Future; import java.io.IOException; +import java.net.Inet6Address; +import java.net.InetAddress; import java.net.InetSocketAddress; +import java.net.NetworkInterface; import java.net.ServerSocket; import java.net.SocketAddress; +import java.net.UnknownHostException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Optional; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; import java.util.concurrent.CompletionStage; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicBoolean; @@ -83,6 +101,7 @@ public class ChannelFactory { private static final String DATASTAX_CLOUD_PRODUCT_TYPE = "DATASTAX_APOLLO"; private static final AtomicBoolean LOGGED_ORPHAN_WARNING = new AtomicBoolean(); + private static final AtomicBoolean LOGGED_HANDLER_WARNING = new AtomicBoolean(); /** * A value for {@link #productType} that indicates that the server does not report any product @@ -90,6 +109,13 @@ public class ChannelFactory { */ private static final String UNKNOWN_PRODUCT_TYPE = "UNKNOWN"; + /** + * How many names {@link #rotationOffsets} tracks before it starts evicting. Generous next to the + * handful of names a session actually expands, and an evicted counter only costs that name a + * rotation restart. + */ + @VisibleForTesting static final int MAX_ROTATION_OFFSETS = 256; + // The names of the handlers on the pipeline: public static final String SSL_HANDLER_NAME = "ssl"; public static final String INBOUND_TRAFFIC_METER_NAME = "inboundTrafficMeter"; @@ -107,6 +133,32 @@ public class ChannelFactory { private final String logPrefix; protected final InternalDriverContext context; + /** + * Round-robin counters used by {@link #rotate} to vary which of a name's addresses a connection + * tries first, one counter per name. {@code SniEndPoint} used to hold an equivalent (single) + * counter of its own, before resolution moved here. + * + *

Per name rather than one global counter, because names whose expansions interleave in + * lockstep -- say two hostname contact points tried in sequence on every reconnection round -- + * would each only ever see one offset parity, pinning every name with an even record count to a + * fixed starting address. (The same failure mode once collapsed {@code SniEndPoint}'s rotation, + * when SSL engine setup shared its counter.) + * + *

Per factory, i.e. per session, and bounded on top of that: the names that reach here -- + * contact points, the SNI proxy name, client-route hostnames -- are not fixed for the lifetime of + * a JVM, or even of a session, since client routes can hand out different hostnames on every + * refresh. Spreading connections is only ever needed among the names a session is currently + * using, so nothing is lost by letting the rest go. + */ + @VisibleForTesting + final LoadingCache rotationOffsets = + CacheBuilder.newBuilder() + .maximumSize(MAX_ROTATION_OFFSETS) + .build(CacheLoader.from(name -> new AtomicInteger())); + + /** Fallback rotation counter for the odd original address that is not name-based. */ + private final AtomicInteger fallbackRotationOffset = new AtomicInteger(); + /** either set from the configuration, or null and will be negotiated */ @VisibleForTesting volatile ProtocolVersion protocolVersion; @@ -125,6 +177,7 @@ public ChannelFactory(InternalDriverContext context) { this.context = context; DriverExecutionProfile defaultConfig = context.getConfig().getDefaultProfile(); + if (defaultConfig.isDefined(DefaultDriverOption.PROTOCOL_VERSION)) { String versionName = defaultConfig.getString(DefaultDriverOption.PROTOCOL_VERSION); this.protocolVersion = context.getProtocolVersionRegistry().fromName(versionName); @@ -161,7 +214,7 @@ public CompletionStage connect(Node node, DriverChannelOptions op } else { nodeMetricUpdater = NoopNodeMetricUpdater.INSTANCE; } - return connect(node.getEndPoint(), null, null, options, nodeMetricUpdater); + return connect(node.getEndPoint(), null, null, options, nodeMetricUpdater, isIdentified(node)); } public CompletionStage connect( @@ -172,7 +225,24 @@ public CompletionStage connect( } else { nodeMetricUpdater = NoopNodeMetricUpdater.INSTANCE; } - return connect(node.getEndPoint(), node.getShardingInfo(), shardId, options, nodeMetricUpdater); + return connect( + node.getEndPoint(), + node.getShardingInfo(), + shardId, + options, + nodeMetricUpdater, + isIdentified(node)); + } + + /** + * Whether we know which node we are connecting to, as opposed to merely which address to + * try. {@link Node#getHostId()} is null only for an initial contact point, until the driver has + * read host ids from {@code system.local} and {@code system.peers} for the first time; every node + * discovered from those rows has one. {@link #tryNextCandidate} needs the distinction because an + * unidentified contact-point name may expand to addresses of different nodes. + */ + private static boolean isIdentified(Node node) { + return node.getHostId() != null; } @VisibleForTesting @@ -182,11 +252,23 @@ CompletionStage connect( Integer shardId, DriverChannelOptions options, NodeMetricUpdater nodeMetricUpdater) { + // A bare endpoint carries no host id, so this matches the contact-point case (see + // isIdentified()). + return connect(endPoint, shardingInfo, shardId, options, nodeMetricUpdater, false); + } + + @VisibleForTesting + CompletionStage connect( + EndPoint endPoint, + NodeShardingInfo shardingInfo, + Integer shardId, + DriverChannelOptions options, + NodeMetricUpdater nodeMetricUpdater, + boolean nodeIsIdentified) { CompletableFuture resultFuture = new CompletableFuture<>(); ProtocolVersion currentVersion; boolean isNegotiating; - List attemptedVersions = new CopyOnWriteArrayList<>(); if (this.protocolVersion != null) { currentVersion = protocolVersion; isNegotiating = false; @@ -203,7 +285,7 @@ CompletionStage connect( nodeMetricUpdater, currentVersion, isNegotiating, - attemptedVersions, + nodeIsIdentified, resultFuture); return resultFuture; } @@ -216,119 +298,646 @@ private void connect( NodeMetricUpdater nodeMetricUpdater, ProtocolVersion currentVersion, boolean isNegotiating, - List attemptedVersions, + boolean nodeIsIdentified, CompletableFuture resultFuture) { - SocketAddress resolvedAddress; + // Built once per connect() rather than once per candidate: it is the only handle on the Netty + // AddressResolverGroup (see resolveCandidates()), and it means the user's + // afterBootstrapInitialized() hook runs once per logical connection instead of once per address + // attempt. Each attempt gets its own clone() with its own handler. + // + // The event loop is likewise picked once per connect() and shared by name resolution and the + // channel itself (the per-attempt clones are bound to it, see connectToAddress()). Advancing + // the group's round-robin chooser exactly once per connect keeps channels evenly distributed: + // taking one loop for resolution and letting Bootstrap.connect() take another would advance + // the chooser twice per connect, parking all channels on half the loops with the default + // power-of-two chooser. It also mirrors what Netty itself does with an unresolved address: + // Bootstrap resolves on the connecting channel's own event loop. + Bootstrap baseBootstrap; + EventLoop eventLoop; try { - resolvedAddress = endPoint.resolve(); + baseBootstrap = newBootstrap(); + eventLoop = context.getNettyOptions().ioEventLoopGroup().next(); } catch (Exception e) { resultFuture.completeExceptionally(e); return; } - NettyOptions nettyOptions = context.getNettyOptions(); + // EndPoint.resolve() is contractually non-blocking and performs no name resolution, so it is + // safe to call here even though connect() runs on the admin event loop for control-connection + // reconnects. Everything a name needs to become connectable happens in resolveCandidates(). + SocketAddress address; + try { + address = endPoint.resolve(); + } catch (Exception e) { + resultFuture.completeExceptionally(e); + return; + } + if (address == null) { + // EndPoint.resolve() is contractually non-null; fail fast instead of NPE-ing inside an + // event-loop task later, which would leave resultFuture hanging (see resolveCandidates()). + resultFuture.completeExceptionally( + new IllegalArgumentException("EndPoint.resolve() returned null: " + endPoint)); + return; + } + + resolveCandidates(baseBootstrap, address, eventLoop) + .whenComplete( + (candidates, error) -> { + if (error != null) { + Throwable cause = + (error instanceof CompletionException && error.getCause() != null) + ? error.getCause() + : error; + resultFuture.completeExceptionally(cause); + return; + } + tryNextCandidate( + baseBootstrap, + eventLoop, + endPoint, + shardingInfo, + shardId, + options, + nodeMetricUpdater, + currentVersion, + isNegotiating, + nodeIsIdentified, + resultFuture, + candidates, + 0, + new ArrayList<>()); + }); + } + /** + * Builds the {@link Bootstrap} shared by every connection attempt of a single {@code connect()} + * call, including the user's {@link NettyOptions#afterBootstrapInitialized(Bootstrap)} hook. Per + * attempt, {@link #connectToAddress} takes a {@link + * Bootstrap#clone(io.netty.channel.EventLoopGroup)} of it bound to the event loop the connect + * picked, and installs its own handler; the copy carries the resolver configuration over. The + * base bootstrap itself keeps the full I/O group, so the hook observes the same group as always. + */ + private Bootstrap newBootstrap() { + NettyOptions nettyOptions = context.getNettyOptions(); Bootstrap bootstrap = new Bootstrap() .group(nettyOptions.ioEventLoopGroup()) .channel(nettyOptions.channelClass()) - .option(ChannelOption.ALLOCATOR, nettyOptions.allocator()) - .handler( - initializer(endPoint, currentVersion, options, nodeMetricUpdater, resultFuture)); - + .option(ChannelOption.ALLOCATOR, nettyOptions.allocator()); nettyOptions.afterBootstrapInitialized(bootstrap); + if (bootstrap.config().handler() != null && LOGGED_HANDLER_WARNING.compareAndSet(false, true)) { + LOG.warn( + "[{}] NettyOptions.afterBootstrapInitialized() installed a channel handler on the" + + " bootstrap; it will be replaced by the driver's own handler. Use" + + " NettyOptions.afterChannelInitialized() to customize the pipeline instead.", + logPrefix); + } + return bootstrap; + } - ChannelFuture connectFuture; - if (shardId == null || shardingInfo == null) { - if (shardId != null) { - LOG.debug( - "Requested connection to shard {} but shardingInfo is currently missing for Node at endpoint {}. Falling back to arbitrary local port.", - shardId, - endPoint); - } - connectFuture = bootstrap.connect(resolvedAddress); - } else { - int localPort = - PortAllocator.getNextAvailablePort(shardingInfo.getShardsCount(), shardId, context); - if (localPort == -1) { - LOG.warn( - "Could not find free port for shard {} at {}. Falling back to arbitrary local port.", - shardId, - endPoint); - connectFuture = bootstrap.connect(resolvedAddress); - } else { - connectFuture = bootstrap.connect(resolvedAddress, new InetSocketAddress(localPort)); - } + /** + * Turns the address an {@link EndPoint} denotes into the concrete, connectable addresses to try, + * expanding it to all the addresses it maps to when it is a name. + * + *

Expansion goes through the bootstrap's Netty {@link AddressResolverGroup} rather than a + * direct {@code InetAddress.getAllByName()} call, so a custom resolver installed via {@link + * NettyOptions#afterBootstrapInitialized(Bootstrap)} is honoured — that is the resolver an + * unresolved address would have reached had it been handed straight to {@code + * Bootstrap.connect()}, as it was before multi-address support. This is also why endpoints are + * forbidden from resolving names themselves (see {@link EndPoint#resolve()}): doing it here is + * the only way to keep that configuration point working, and the only way to keep {@code + * resolve()} non-blocking. + * + *

Whether an address needs resolving at all is the resolver's decision, not ours: exactly as + * in {@code Bootstrap#doResolveAndConnect0}, the address is passed through untouched only when + * the resolver says it does not {@linkplain AddressResolver#isSupported support} it (e.g. {@link + * io.netty.channel.local.LocalAddress}) or that it {@linkplain AddressResolver#isResolved is + * already resolved}. Both are overridable, and a custom resolver may well report an + * already-resolved address as unresolved in order to redirect it — Netty consulted it either way, + * so a pre-check here on {@code InetSocketAddress#isUnresolved()} would silently take that + * configuration point away for every connect to an already-resolved node, which is to say for + * almost every connect. A null group means the user called {@link Bootstrap#disableResolver()}, + * which is likewise respected. + * + *

Note that with Netty's default resolver the lookup blocks the event loop it runs on, + * because {@code DefaultNameResolver} performs {@code InetAddress.getAllByName()} inline. That is + * the pre-existing behaviour of handing an unresolved address to {@code Bootstrap.connect()}, and + * it is an I/O loop, never the admin loop that {@code connect()} is called from. Deployments that + * need non-blocking resolution can now install {@code DnsAddressResolverGroup} and have it take + * effect. + */ + private CompletionStage> resolveCandidates( + Bootstrap bootstrap, SocketAddress address, EventLoop eventLoop) { + + AddressResolverGroup resolverGroup = bootstrap.config().resolver(); + if (resolverGroup == null) { + // Bootstrap.disableResolver(): the user wants the address passed through as-is. + return CompletableFuture.completedFuture(Collections.singletonList(address)); } - connectFuture.addListener( - cf -> { - if (connectFuture.isSuccess()) { - Channel channel = connectFuture.channel(); - DriverChannel driverChannel = - new DriverChannel(endPoint, channel, context.getWriteCoalescer(), currentVersion); - // If this is the first successful connection, remember the protocol version and - // cluster name for future connections. - if (isNegotiating) { - ChannelFactory.this.protocolVersion = currentVersion; - } - if (ChannelFactory.this.clusterName == null) { - ChannelFactory.this.clusterName = driverChannel.getClusterName(); - } - Map> supportedOptions = driverChannel.getOptions(); - if (ChannelFactory.this.productType == null && supportedOptions != null) { - List productTypes = supportedOptions.get("PRODUCT_TYPE"); - String productType = - productTypes != null && !productTypes.isEmpty() - ? productTypes.get(0) - : UNKNOWN_PRODUCT_TYPE; - ChannelFactory.this.productType = productType; - DriverConfig driverConfig = context.getConfig(); - if (driverConfig instanceof TypesafeDriverConfig - && productType.equals(DATASTAX_CLOUD_PRODUCT_TYPE)) { - ((TypesafeDriverConfig) driverConfig) - .overrideDefaults( - ImmutableMap.of( - DefaultDriverOption.REQUEST_CONSISTENCY, - ConsistencyLevel.LOCAL_QUORUM.name())); + // The supplied event loop is the same one the channel will be registered on (see connect()), + // which is what Netty itself does with an unresolved address: Bootstrap resolves on the + // connecting channel's own event loop. Its transport also matches the channel class, which + // matters because DnsAddressResolverGroup registers a datagram channel on the executor it + // resolves for. + CompletableFuture> result = new CompletableFuture<>(); + // Every path below must complete `result`: nothing at this stage has a timeout, so a task or + // listener that dies with the future still pending (Netty swallows their throwables, it only + // logs them) would hang the connect attempt -- and with it control-connection init or a pool + // reconnect -- forever. Hence the blanket catches around the task body, the listener body, and + // the execute() call itself (which throws RejectedExecutionException while shutting down). + try { + eventLoop.execute( + () -> { + try { + AddressResolver resolver = + resolverGroup.getResolver(eventLoop); + if (!resolver.isSupported(address) || resolver.isResolved(address)) { + // Nothing for the resolver to do; same short-circuit as + // Bootstrap#doResolveAndConnect0. + result.complete(Collections.singletonList(address)); + return; } + resolver + .resolveAll(address) + .addListener( + (Future> future) -> { + try { + if (!future.isSuccess()) { + result.completeExceptionally(future.cause()); + return; + } + @SuppressWarnings("unchecked") + List addresses = + (List) future.getNow(); + if (addresses == null || addresses.isEmpty()) { + result.completeExceptionally( + new IllegalStateException( + "Resolver returned no address for " + address)); + return; + } + result.complete(rotate(address, reattachHostnames(address, addresses))); + } catch (Throwable t) { + result.completeExceptionally(t); + } + }); + } catch (Throwable t) { + result.completeExceptionally(t); } - resultFuture.complete(driverChannel); - } else { - Throwable error = connectFuture.cause(); - if (error instanceof UnsupportedProtocolVersionException && isNegotiating) { - attemptedVersions.add(currentVersion); - Optional downgraded = - context.getProtocolVersionRegistry().downgrade(currentVersion); - if (downgraded.isPresent()) { + }); + } catch (Throwable t) { + result.completeExceptionally(t); + } + return result; + } + + /** Applies {@link #reattachHostname} to every expanded candidate. */ + private static List reattachHostnames( + SocketAddress original, List candidates) { + List result = new ArrayList<>(candidates.size()); + for (SocketAddress candidate : candidates) { + result.add(reattachHostname(original, candidate)); + } + return result; + } + + /** + * Re-attaches the {@code original} address's host name to one of the resolved candidates it + * expanded to, whatever name that candidate carries. + * + *

The JDK and Netty-DNS resolvers already attach the queried name to the {@link InetAddress}es + * they return, so this is a no-op for them. A custom resolver, however, may build its results + * from raw address bytes, or label them with a canonical/CNAME name of its own. The channel's + * pinned endpoint is built from the candidate (see {@link PinnableEndPoint}), and it is what + * {@code DefaultSslEngineFactory} and {@code SniSslEngineFactory} derive the SSL peer host from, + * inside the channel initializer. So whatever name the candidate carries is the name TLS hostname + * verification checks the server certificate against, and the only name that may be is the one + * the user configured: with a nameless address, {@code InetSocketAddress#getHostName()} + * additionally triggers a blocking reverse-DNS lookup on the event loop and validation falls back + * to the IP or the PTR record, and with a resolver-supplied label it validates a name the + * operator never chose. Hence the queried name always wins here; before multi-address support the + * initializer kept the original endpoint and Netty resolved only the TCP destination, which had + * the same effect. + * + *

Re-attaching changes nothing else: {@code InetAddress.getByAddress(host, bytes)} performs no + * lookup, the TCP connect target is the same IP, and a resolved {@link InetSocketAddress}'s + * equality ignores host names, so pinning and the pin-equality shortcuts are unaffected. It also + * makes {@link #rotate} more deterministic, not less: with one uniform name across an expansion, + * its {@code toString()} sort depends only on the IP and port. A scoped IPv6 candidate keeps its + * scope, since {@link Inet6Address} has {@code getByAddress} overloads that carry one. + * + *

An original that carries no name of its own is left alone (see {@link + * AddressUtils#carriesName}): a resolver is free to redirect it to a different IP, and labelling + * that IP with the literal form of the one we asked for would invent a name that resolves to + * something else. + */ + @VisibleForTesting + static SocketAddress reattachHostname(SocketAddress original, SocketAddress candidate) { + if (!(original instanceof InetSocketAddress) || !(candidate instanceof InetSocketAddress)) { + return candidate; + } + InetSocketAddress originalInet = (InetSocketAddress) original; + InetSocketAddress candidateInet = (InetSocketAddress) candidate; + InetAddress candidateIp = candidateInet.getAddress(); + if (!AddressUtils.carriesName(originalInet) + || candidateIp == null + // Nothing to change: the candidate already carries the queried name, which is the common + // case (the JDK and Netty-DNS resolvers attach it themselves). getHostString() never looks + // anything up -- for a nameless address it falls back to the IP literal. + || candidateInet.getHostString().equals(originalInet.getHostString())) { + return candidate; + } + try { + return new InetSocketAddress( + withHostName(originalInet.getHostString(), candidateIp), candidateInet.getPort()); + } catch (UnknownHostException impossible) { + // getByAddress only rejects illegal byte lengths, and these bytes come from a real + // InetAddress; keep the raw candidate rather than failing the connect over a cosmetic step. + return candidate; + } + } + + /** + * Returns a copy of {@code ip} labelled with {@code hostName}, preserving an IPv6 scope if there + * is one. + * + *

{@link InetAddress#getByAddress(String, byte[])} cannot carry a scope, and dropping one + * would change where the address actually points — a link-local address is only meaningful + * together with its zone. {@link Inet6Address#getByAddress(String, byte[], int)} carries the zone + * as its numeric id, which is what the connect itself goes on; a scope id of 0 means "unscoped" + * and is accepted, so this needs no special case for a plain IPv6 address. + * + *

The sibling overload taking a {@link NetworkInterface} is deliberately not used: it + * re-derives the numeric scope by searching that interface for an address of the same local type, + * and throws {@code UnknownHostException("no scope_id found")} when it finds none — so it can + * fail for an address that was legitimately built from an interface in the first place. All that + * is lost by going numeric is the interface name, which surfaces in {@code toString()} and + * nowhere else. + */ + private static InetAddress withHostName(String hostName, InetAddress ip) + throws UnknownHostException { + return ip instanceof Inet6Address + ? Inet6Address.getByAddress(hostName, ip.getAddress(), ((Inet6Address) ip).getScopeId()) + : InetAddress.getByAddress(hostName, ip.getAddress()); + } + + /** + * Rotates the expanded address list so that successive connections to the same name do not all + * start at the same address. + * + *

Without this, every connection would try the resolver's first address first and healthy + * connections would pile onto one IP; the whole point of a multi-record name is usually to spread + * them. All addresses are still returned, and in the same cyclic order, so a single attempt can + * still fall back across every one of them. + * + *

The list is sorted first so that a given rotation offset maps to the same address on every + * call, regardless of the order the resolver happened to return. Only that determinism matters + * here, not the ordering itself, so a plain string comparison is enough. + * + *

The offset is tracked per name -- {@code original} is the address the expansion was queried + * for -- so that different names rotate independently (see {@link #rotationOffsets}). + */ + @VisibleForTesting + List rotate(SocketAddress original, List addresses) { + int size = addresses.size(); + if (size == 1) { + // Nothing to rotate, and don't burn a rotation offset (or create a counter) for it. + return new ArrayList<>(addresses); + } + List sorted = new ArrayList<>(addresses); + sorted.sort(Comparator.comparing(SocketAddress::toString)); + int start = Math.floorMod(rotationOffsetFor(original).getAndIncrement(), size); + List result = new ArrayList<>(size); + for (int i = 0; i < size; i++) { + result.add(sorted.get((start + i) % size)); + } + return result; + } + + private AtomicInteger rotationOffsetFor(SocketAddress original) { + if (original instanceof InetSocketAddress) { + // DNS names are case-insensitive; normalize so the same name shares one counter. + String name = ((InetSocketAddress) original).getHostString().toLowerCase(Locale.ROOT); + return rotationOffsets.getUnchecked(name); + } + return fallbackRotationOffset; + } + + /** + * Iterates through the candidate addresses produced by {@link #resolveCandidates}. Tries each one + * in sequence; when an address fails, the next candidate is tried, and only when all candidates + * are exhausted is the overall {@code resultFuture} failed. + * + *

The one exception is a {@link UnsupportedProtocolVersionException} against an + * identified node ({@code nodeIsIdentified}, see {@link #isIdentified(Node)}): a + * protocol-version rejection -- whether negotiation exhausted every downgrade or the server + * refused a forced version -- is a property of the node, and every address of a node we have + * already identified is that same node, so the attempt fails immediately instead of replaying the + * whole negotiation ladder against every remaining IP (worst case {@code N × versions × + * connect-timeout} for nothing). This matches the pre-multi-address behaviour of a single-address + * connect. The corner it deliberately does not rescue: a heterogeneous rolling upgrade where + * different IPs of one identified node genuinely support different protocol versions. + * + *

For an unidentified endpoint -- a contact point, before the driver has read host ids + * -- the addresses a name expands to may well belong to different nodes, so a rejection by the + * first of them says nothing about the rest and the loop keeps going. That also preserves the + * behaviour this PR would otherwise have removed: with {@code advanced.resolve-contact-points = + * true} each resolved address used to be a separate {@code Node}, and {@code ControlConnection} + * advances to the next node in its query plan on any error, including this one. + * + *

Other failures -- TCP, init, authentication -- always advance to the next candidate, since + * with a multi-record name they may well be address-specific. + * + *

Timeout note: addresses are tried serially, so the worst-case time before failure is + * {@code N × connect-timeout} where N is the number of candidates. This is an intentional + * tradeoff: failing immediately on the first unreachable IP would prevent fallback to healthy + * ones. In practice DNS entries have only a small number of records. + * + *

When every candidate fails, the last candidate's error is propagated with each earlier + * candidate's failure attached as a {@linkplain Throwable#addSuppressed(Throwable) suppressed} + * exception, so the full set of per-address causes is visible for diagnosis (they are otherwise + * only logged at DEBUG). + */ + private void tryNextCandidate( + Bootstrap baseBootstrap, + EventLoop eventLoop, + EndPoint endPoint, + NodeShardingInfo shardingInfo, + Integer shardId, + DriverChannelOptions options, + NodeMetricUpdater nodeMetricUpdater, + ProtocolVersion currentVersion, + boolean isNegotiating, + boolean nodeIsIdentified, + CompletableFuture resultFuture, + List candidates, + int index, + List priorErrors) { + + // Invariant: this method always (eventually) completes resultFuture. It is invoked from + // CompletionStage and Netty callbacks that swallow throwables, so a synchronous throw -- a + // custom PinnableEndPoint.pinTo() for instance -- would otherwise leave the connect attempt + // hanging forever. Double completion is harmless: completeExceptionally() on an already + // completed future is a no-op. + try { + SocketAddress candidate = candidates.get(index); + // Everything downstream of here -- the channel, its pipeline (SSL engine, authenticator) and + // the DriverChannel handed to the caller -- sees an endpoint bound to this one address + // instead of the multi-address original. See PinnableEndPoint for why that matters. + EndPoint pinnedEndPoint = pin(endPoint, candidate); + CompletableFuture perAddressFuture = new CompletableFuture<>(); + // Fresh per candidate address: connectToAddress()'s downgrade retries stay on this one + // address, so the final UnsupportedProtocolVersionException (if negotiation is what dooms + // this candidate) only reports versions actually tried against it, not earlier candidates'. + List attemptedVersions = new CopyOnWriteArrayList<>(); + connectToAddress( + baseBootstrap, + eventLoop, + pinnedEndPoint, + shardingInfo, + shardId, + options, + nodeMetricUpdater, + currentVersion, + isNegotiating, + attemptedVersions, + perAddressFuture, + candidate); + + perAddressFuture.whenComplete( + (channel, error) -> { + try { + if (error == null) { + resultFuture.complete(channel); + } else if (!isNodeWideFailure(error, nodeIsIdentified) + && index + 1 < candidates.size()) { LOG.debug( - "[{}] Failed to connect with protocol {}, retrying with {}", + "[{}] Failed to connect to {} ({}), trying next address", logPrefix, - currentVersion, - downgraded.get()); - connect( + candidate, + error.getMessage()); + priorErrors.add(error); + tryNextCandidate( + baseBootstrap, + eventLoop, + // Deliberately the original, not the pinned copy: the next candidate must be + // pinned from the unpinned endpoint. endPoint, shardingInfo, shardId, options, nodeMetricUpdater, - downgraded.get(), - true, - attemptedVersions, - resultFuture); + currentVersion, + isNegotiating, + nodeIsIdentified, + resultFuture, + candidates, + index + 1, + priorErrors); } else { - resultFuture.completeExceptionally( - UnsupportedProtocolVersionException.forNegotiation( - endPoint, attemptedVersions)); + if (index + 1 < candidates.size()) { + // Only reachable for a node-wide failure (see the javadoc). + LOG.debug( + "[{}] Not trying the remaining addresses of {}: a protocol-version rejection" + + " is a property of the node, not of the address ({})", + logPrefix, + endPoint, + error.getMessage()); + } + // Surface the last error, carrying the earlier failures as suppressed exceptions + // so they are not lost (they were only logged at DEBUG above). + for (Throwable priorError : priorErrors) { + if (priorError != error) { + error.addSuppressed(priorError); + } + } + // Note: might be completed already if the failure happened in initializer() + resultFuture.completeExceptionally(error); } - } else { - // Note: might be completed already if the failure happened in initializer(), this is - // fine - resultFuture.completeExceptionally(error); + } catch (Throwable t) { + resultFuture.completeExceptionally(t); } - } - }); + }); + } catch (Throwable t) { + resultFuture.completeExceptionally(t); + } + } + + /** + * Whether {@code error} dooms every remaining address of the endpoint, making it pointless for + * {@link #tryNextCandidate} to try them. See its javadoc for why this is limited to a + * protocol-version rejection against an identified node. + */ + private static boolean isNodeWideFailure(Throwable error, boolean nodeIsIdentified) { + return nodeIsIdentified && error instanceof UnsupportedProtocolVersionException; + } + + /** + * Performs a Netty bootstrap connect to a single, already-resolved address. Handles + * protocol-version negotiation (downgrade retries) internally, staying on the same address. Uses + * {@code perAddressFuture} so {@link #tryNextCandidate} can distinguish a per-address TCP failure + * (try the next IP) from a successful protocol handshake. + */ + private void connectToAddress( + Bootstrap baseBootstrap, + EventLoop eventLoop, + EndPoint endPoint, + NodeShardingInfo shardingInfo, + Integer shardId, + DriverChannelOptions options, + NodeMetricUpdater nodeMetricUpdater, + ProtocolVersion currentVersion, + boolean isNegotiating, + List attemptedVersions, + CompletableFuture perAddressFuture, + SocketAddress resolvedAddress) { + + // Invariant, as in tryNextCandidate(): every path completes perAddressFuture. The synchronous + // section can throw from Bootstrap validation; the connect listener runs inside a Netty + // callback that swallows throwables and contains the downgrade recursion, the version-registry + // lookup and the config overrides, any of which throwing would otherwise hang the attempt. + try { + // clone(eventLoop) so each attempt gets its own handler while sharing the options (including + // anything afterBootstrapInitialized() set), and is registered on the event loop the + // connect() picked -- the same one resolution ran on, so the group's chooser advances exactly + // once per logical connect (see connect()). + // + // disableResolver() because resolveCandidates() has already done the one resolution pass this + // connect gets, and `resolvedAddress` is one of its results. Bootstrap.clone() otherwise + // carries the resolver over and Netty resolves again -- through resolve(), *singular*. That + // is inert for the default resolver, which short-circuits on isResolved(), but a resolver + // that reports resolved addresses as unresolved in order to redirect them -- which + // resolveCandidates() deliberately supports -- would remap every candidate onto its first + // answer: the remaining candidates would never actually be tried, and the endpoint pinned + // onto the channel would name an address the channel is not connected to (which is what the + // SSL engine's peer host and DefaultTopologyMonitor#savePort are derived from). Every other + // exit from resolveCandidates() yields an address Netty would itself have passed through + // untouched -- no group, !isSupported, or isResolved -- so nothing else changes. + Bootstrap bootstrap = + baseBootstrap + .clone(eventLoop) + .disableResolver() + .handler( + initializer( + endPoint, currentVersion, options, nodeMetricUpdater, perAddressFuture)); + + ChannelFuture connectFuture; + if (shardId == null || shardingInfo == null) { + if (shardId != null) { + LOG.debug( + "Requested connection to shard {} but shardingInfo is currently missing for Node at endpoint {}. Falling back to arbitrary local port.", + shardId, + endPoint); + } + connectFuture = bootstrap.connect(resolvedAddress); + } else { + int localPort = + PortAllocator.getNextAvailablePort(shardingInfo.getShardsCount(), shardId, context); + if (localPort == -1) { + LOG.warn( + "Could not find free port for shard {} at {}. Falling back to arbitrary local port.", + shardId, + endPoint); + connectFuture = bootstrap.connect(resolvedAddress); + } else { + connectFuture = bootstrap.connect(resolvedAddress, new InetSocketAddress(localPort)); + } + } + + connectFuture.addListener( + cf -> { + try { + if (connectFuture.isSuccess()) { + Channel channel = connectFuture.channel(); + DriverChannel driverChannel = + new DriverChannel( + endPoint, channel, context.getWriteCoalescer(), currentVersion); + // If this is the first successful connection, remember the protocol version and + // cluster name for future connections. + if (isNegotiating) { + ChannelFactory.this.protocolVersion = currentVersion; + } + if (ChannelFactory.this.clusterName == null) { + ChannelFactory.this.clusterName = driverChannel.getClusterName(); + } + Map> supportedOptions = driverChannel.getOptions(); + if (ChannelFactory.this.productType == null && supportedOptions != null) { + List productTypes = supportedOptions.get("PRODUCT_TYPE"); + String productType = + productTypes != null && !productTypes.isEmpty() + ? productTypes.get(0) + : UNKNOWN_PRODUCT_TYPE; + ChannelFactory.this.productType = productType; + DriverConfig driverConfig = context.getConfig(); + if (driverConfig instanceof TypesafeDriverConfig + && productType.equals(DATASTAX_CLOUD_PRODUCT_TYPE)) { + ((TypesafeDriverConfig) driverConfig) + .overrideDefaults( + ImmutableMap.of( + DefaultDriverOption.REQUEST_CONSISTENCY, + ConsistencyLevel.LOCAL_QUORUM.name())); + } + } + perAddressFuture.complete(driverChannel); + } else { + Throwable error = connectFuture.cause(); + if (error instanceof UnsupportedProtocolVersionException && isNegotiating) { + attemptedVersions.add(currentVersion); + Optional downgraded = + context.getProtocolVersionRegistry().downgrade(currentVersion); + if (downgraded.isPresent()) { + LOG.debug( + "[{}] Failed to connect with protocol {}, retrying with {}", + logPrefix, + currentVersion, + downgraded.get()); + // Stay on the same address for protocol-version downgrade retries. + connectToAddress( + baseBootstrap, + eventLoop, + endPoint, + shardingInfo, + shardId, + options, + nodeMetricUpdater, + downgraded.get(), + true, + attemptedVersions, + perAddressFuture, + resolvedAddress); + } else { + perAddressFuture.completeExceptionally( + UnsupportedProtocolVersionException.forNegotiation( + endPoint, attemptedVersions)); + } + } else { + // Note: might be completed already if the failure happened in initializer(), this + // is fine + perAddressFuture.completeExceptionally(error); + } + } + } catch (Throwable t) { + perAddressFuture.completeExceptionally(t); + } + }); + } catch (Throwable t) { + perAddressFuture.completeExceptionally(t); + } + } + + /** + * Binds {@code endPoint} to the address a connection is being opened to, when the implementation + * supports it. + * + *

Third-party {@link EndPoint}s that do not implement {@link PinnableEndPoint} are returned + * unchanged, so they keep behaving exactly as they did before multi-address support: the channel + * carries the endpoint it was given. + */ + private static EndPoint pin(EndPoint endPoint, SocketAddress resolvedAddress) { + return endPoint instanceof PinnableEndPoint + ? ((PinnableEndPoint) endPoint).pinTo(resolvedAddress) + : endPoint; } @VisibleForTesting diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/context/NettyOptions.java b/core/src/main/java/com/datastax/oss/driver/internal/core/context/NettyOptions.java index 5b4ff4dcec8..b319186910d 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/context/NettyOptions.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/context/NettyOptions.java @@ -66,7 +66,21 @@ public interface NettyOptions { /** * A hook invoked each time the driver creates a client bootstrap in order to open a channel. This - * is a good place to configure any custom option on the bootstrap. + * is a good place to configure any custom option, attribute, or {@link + * Bootstrap#resolver(io.netty.resolver.AddressResolverGroup)} on the bootstrap. + * + *

The hook runs once per logical connection to a node. When a hostname expands to several IP + * addresses, the same bootstrap is shared by every per-address attempt (each attempt uses a + * {@link Bootstrap#clone(io.netty.channel.EventLoopGroup)} of it); likewise, protocol-version + * downgrade retries reuse it. Before multi-address support the hook ran once per attempt, + * including once per downgrade retry. + * + *

The bootstrap does not carry the driver's channel handler yet, and a handler + * installed by this hook is not honoured: the driver sets its own handler on each + * per-attempt copy afterwards (and logs a one-time warning if it overwrites one). To customize + * the pipeline, use {@link #afterChannelInitialized(Channel)} instead. (Before multi-address + * support the hook ran after the driver's handler was installed, so replacing it was technically + * possible; that was never a supported extension point.) */ void afterBootstrapInitialized(Bootstrap bootstrap); diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryBootstrapHookTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryBootstrapHookTest.java new file mode 100644 index 00000000000..dbe60ae4432 --- /dev/null +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryBootstrapHookTest.java @@ -0,0 +1,71 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.datastax.oss.driver.internal.core.channel; + +import static com.datastax.oss.driver.Assertions.assertThatStage; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.when; + +import com.datastax.oss.driver.api.core.DefaultProtocolVersion; +import com.datastax.oss.driver.api.core.config.DefaultDriverOption; +import com.datastax.oss.driver.internal.core.context.NettyOptions; +import com.datastax.oss.driver.internal.core.metrics.NoopNodeMetricUpdater; +import io.netty.bootstrap.Bootstrap; +import io.netty.channel.ChannelInboundHandlerAdapter; +import java.util.concurrent.CompletionStage; +import org.junit.Test; + +/** + * Verifies the {@link NettyOptions#afterBootstrapInitialized(Bootstrap)} contract: the hook runs on + * a handler-less bootstrap, and a handler it installs is replaced by the driver's own. + */ +public class ChannelFactoryBootstrapHookTest extends ChannelFactoryTestBase { + + @Test + public void should_replace_handler_installed_by_bootstrap_hook() { + // Given – a hook that (incorrectly) installs its own channel handler. The driver sets its own + // handler on each per-attempt copy afterwards, logging a one-time warning; if the dummy + // handler below survived instead, the protocol handshake would never happen and this connect + // would fail on the init timeout. + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + doAnswer( + invocation -> { + Bootstrap bootstrap = invocation.getArgument(0); + bootstrap.handler(new ChannelInboundHandlerAdapter()); + return null; + }) + .when(nettyOptions) + .afterBootstrapInitialized(any(Bootstrap.class)); + ChannelFactory factory = newChannelFactory(); + + // When + CompletionStage channelFuture = + factory.connect( + SERVER_ADDRESS, + null, + null, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE); + completeSimpleChannelInit(); + + // Then + assertThatStage(channelFuture).isSuccess(); + } +} diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryMultiAddressTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryMultiAddressTest.java new file mode 100644 index 00000000000..fc6be0af645 --- /dev/null +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryMultiAddressTest.java @@ -0,0 +1,452 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.datastax.oss.driver.internal.core.channel; + +import static com.datastax.oss.driver.Assertions.assertThatStage; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assumptions.assumeThat; +import static org.mockito.Mockito.when; + +import com.datastax.oss.driver.api.core.DefaultProtocolVersion; +import com.datastax.oss.driver.api.core.config.DefaultDriverOption; +import com.datastax.oss.driver.api.core.metadata.EndPoint; +import com.datastax.oss.driver.internal.core.metadata.DefaultEndPoint; +import com.datastax.oss.driver.internal.core.metrics.NoopNodeMetricUpdater; +import com.datastax.oss.driver.internal.core.util.AddressUtils; +import edu.umd.cs.findbugs.annotations.NonNull; +import io.netty.channel.local.LocalAddress; +import java.net.Inet6Address; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.NetworkInterface; +import java.net.SocketAddress; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.concurrent.CompletionStage; +import java.util.concurrent.RejectedExecutionException; +import java.util.concurrent.TimeUnit; +import org.junit.Test; + +/** + * Verifies how {@link ChannelFactory#connect} treats the several addresses a name expands to: every + * one of them is tried in sequence, failures are aggregated rather than dropped, and the starting + * address is rotated so healthy connections do not all pile onto the same one. + * + *

The expansion itself is exercised in {@link ChannelFactoryNettyResolverTest}; here the + * resolver is only the mechanism for producing more than one address from a single endpoint. + */ +public class ChannelFactoryMultiAddressTest extends ChannelFactoryTestBase { + + // Local addresses that no server is bound to: connecting to them fails immediately. + private static final SocketAddress UNREACHABLE_1 = + new LocalAddress(ChannelFactoryMultiAddressTest.class.getSimpleName() + "-unreachable-1"); + private static final SocketAddress UNREACHABLE_2 = + new LocalAddress(ChannelFactoryMultiAddressTest.class.getSimpleName() + "-unreachable-2"); + + /** The name the endpoint reports, and that only the resolver knows how to expand. */ + private static final InetSocketAddress HOSTNAME = + InetSocketAddress.createUnresolved("test.cluster.fake", 9042); + + @Test + public void should_fail_with_suppressed_causes_when_all_addresses_are_unreachable() { + // Given – a name that expands to two dead addresses. + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + installResolver(new TestAddressResolverGroup(Arrays.asList(UNREACHABLE_1, UNREACHABLE_2))); + ChannelFactory factory = newChannelFactory(); + + // When + CompletionStage channelFuture = + factory.connect( + new DefaultEndPoint(HOSTNAME), + null, + null, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE); + + // Then – the future fails, and the earlier address's failure is preserved as a suppressed + // exception on the last one's error rather than being silently dropped. + assertThatStage(channelFuture) + .isFailed( + e -> + assertThat(e.getSuppressed()) + .as("earlier address failures should be attached as suppressed exceptions") + .isNotEmpty()); + } + + @Test + public void should_rotate_the_starting_address_across_successive_expansions() { + // Every address must still be offered -- a single attempt has to be able to fall back across + // all of them -- but the one tried *first* has to move, otherwise every connection piles onto + // the resolver's first record and a multi-record name buys no spreading at all. + InetSocketAddress name = InetSocketAddress.createUnresolved("successive.rotate.fake", 9042); + List addresses = Arrays.asList(UNREACHABLE_1, UNREACHABLE_2); + ChannelFactory factory = newChannelFactory(); + + List first = factory.rotate(name, addresses); + List second = factory.rotate(name, addresses); + + assertThat(first).containsExactlyInAnyOrder(UNREACHABLE_1, UNREACHABLE_2); + assertThat(second).containsExactlyInAnyOrder(UNREACHABLE_1, UNREACHABLE_2); + assertThat(second.get(0)) + .as("successive expansions must not start at the same address") + .isNotEqualTo(first.get(0)); + } + + @Test + public void should_leave_a_single_address_alone() { + InetSocketAddress name = InetSocketAddress.createUnresolved("single.rotate.fake", 9042); + ChannelFactory factory = newChannelFactory(); + + // Nothing to spread... + assertThat(factory.rotate(name, Collections.singletonList(UNREACHABLE_1))) + .containsExactly(UNREACHABLE_1); + + // ...and no rotation offset may be burned for it either: the name's first multi-address + // expansion still starts at the toString-sorted first element ("...-1" sorts before "...-2"). + List next = factory.rotate(name, Arrays.asList(UNREACHABLE_2, UNREACHABLE_1)); + assertThat(next.get(0)).isEqualTo(UNREACHABLE_1); + } + + @Test + public void should_rotate_names_independently() { + InetSocketAddress nameA = InetSocketAddress.createUnresolved("a.independent.fake", 9042); + InetSocketAddress nameB = InetSocketAddress.createUnresolved("b.independent.fake", 9042); + List addresses = Arrays.asList(UNREACHABLE_1, UNREACHABLE_2); + ChannelFactory factory = newChannelFactory(); + + // Interleave the two names the way two hostname contact points are expanded in sequence on + // every reconnection round. With one global counter each name would only ever see one offset + // parity, pinning both to a fixed starting address forever. + List a1 = factory.rotate(nameA, addresses); + List b1 = factory.rotate(nameB, addresses); + List a2 = factory.rotate(nameA, addresses); + List b2 = factory.rotate(nameB, addresses); + + // Each name still rotates on its own... + assertThat(a2.get(0)) + .as("name A must rotate despite interleaved expansions of name B") + .isNotEqualTo(a1.get(0)); + assertThat(b2.get(0)) + .as("name B must rotate despite interleaved expansions of name A") + .isNotEqualTo(b1.get(0)); + // ...and is not perturbed by the other: both fresh names start at the same (sorted-first) + // element instead of B starting wherever A's expansions left a shared counter. + assertThat(b1.get(0)).isEqualTo(a1.get(0)); + } + + @Test + public void should_not_share_rotation_offsets_across_factories() { + // The counters belong to the factory, i.e. to the session: a name an earlier session expanded + // must not leave an offset behind for the next one, and nothing outlives the session. + InetSocketAddress name = InetSocketAddress.createUnresolved("per.session.rotate.fake", 9042); + List addresses = Arrays.asList(UNREACHABLE_1, UNREACHABLE_2); + + List first = newChannelFactory().rotate(name, addresses); + List other = newChannelFactory().rotate(name, addresses); + + assertThat(other.get(0)) + .as("a fresh factory rotates from its own start, not from where another one left off") + .isEqualTo(first.get(0)); + } + + @Test + public void should_bound_the_number_of_tracked_names() { + // Client routes can hand out different hostnames on every refresh, so even within one session + // the set of names is not bounded by the configuration or the topology. + ChannelFactory factory = newChannelFactory(); + List addresses = Arrays.asList(UNREACHABLE_1, UNREACHABLE_2); + + for (int i = 0; i < ChannelFactory.MAX_ROTATION_OFFSETS * 4; i++) { + factory.rotate(InetSocketAddress.createUnresolved("churn-" + i + ".fake", 9042), addresses); + } + + assertThat(factory.rotationOffsets.size()) + .as("stale names must be evicted rather than retained for the session's lifetime") + .isLessThanOrEqualTo(ChannelFactory.MAX_ROTATION_OFFSETS); + } + + // ---- reattachHostname() --------------------------------------------------- + + @Test + public void should_reattach_queried_hostname_to_nameless_resolved_address() throws Exception { + // A custom resolver may build its results from raw address bytes; the queried name must be + // re-attached so TLS hostname validation checks the configured name (not the IP or a PTR + // record) and reading the host name never triggers a reverse lookup on the event loop. + InetSocketAddress candidate = + new InetSocketAddress(InetAddress.getByAddress(new byte[] {10, 0, 0, 1}), 9999); + + InetSocketAddress result = + (InetSocketAddress) ChannelFactory.reattachHostname(HOSTNAME, candidate); + + assertThat(result.isUnresolved()).isFalse(); + // getHostString() never looks anything up; getHostName() reverse-resolves a *nameless* + // address, so it returning the queried name proves the name is embedded, not looked up. + assertThat(result.getHostString()).isEqualTo("test.cluster.fake"); + assertThat(result.getHostName()).isEqualTo("test.cluster.fake"); + assertThat(result.getAddress().getHostAddress()).isEqualTo("10.0.0.1"); + // The candidate's port wins over the original's: a resolver may remap ports too. + assertThat(result.getPort()).isEqualTo(9999); + // Equality is unchanged (a resolved InetSocketAddress compares IP bytes + port only), so + // pinning and the pin-equality shortcuts behave exactly as with the raw candidate. + assertThat(result).isEqualTo(candidate); + } + + @Test + public void should_override_resolver_provided_hostname_with_queried_name() throws Exception { + // A resolver may label its results with a canonical/CNAME name of its own. That name would end + // up on the pinned endpoint and hence be the one TLS hostname verification checks the server + // certificate against, so the name the user configured has to win over it. + InetSocketAddress candidate = + new InetSocketAddress( + InetAddress.getByAddress("cname.example.fake", new byte[] {10, 0, 0, 1}), 9042); + + InetSocketAddress result = + (InetSocketAddress) ChannelFactory.reattachHostname(HOSTNAME, candidate); + + assertThat(result.getHostString()).isEqualTo("test.cluster.fake"); + assertThat(result.getAddress().getHostAddress()).isEqualTo("10.0.0.1"); + assertThat(result.getPort()).isEqualTo(9042); + } + + @Test + public void should_pass_candidate_through_when_it_already_carries_the_queried_name() + throws Exception { + // The common case: the JDK and Netty-DNS resolvers attach the queried name themselves, so + // there is nothing to rebuild. + InetSocketAddress candidate = + new InetSocketAddress( + InetAddress.getByAddress("test.cluster.fake", new byte[] {10, 0, 0, 1}), 9042); + + assertThat(ChannelFactory.reattachHostname(HOSTNAME, candidate)).isSameAs(candidate); + } + + @Test + public void should_pass_non_inet_candidate_through() { + // The local-transport addresses these unit tests connect over must never be touched. + assertThat(ChannelFactory.reattachHostname(HOSTNAME, UNREACHABLE_1)).isSameAs(UNREACHABLE_1); + } + + @Test + public void should_pass_candidate_through_when_original_carries_no_name() throws Exception { + // An original written as an IP literal has no name to carry over, and inventing one from the + // literal would be worse than leaving the candidate alone: a resolver is free to redirect it to + // a different IP, which would then be labelled with the literal form of a *different* address. + InetSocketAddress original = new InetSocketAddress("127.0.0.1", 9042); + InetSocketAddress candidate = + new InetSocketAddress(InetAddress.getByAddress(new byte[] {10, 0, 0, 1}), 9042); + + assertThat(ChannelFactory.reattachHostname(original, candidate)).isSameAs(candidate); + assertThat(AddressUtils.carriesName(original)).isFalse(); + assertThat(AddressUtils.carriesName(InetSocketAddress.createUnresolved("10.0.0.1", 9042))) + .isFalse(); + } + + @Test + public void should_reattach_name_of_a_resolved_original() throws Exception { + // A resolved original still reaches the resolver -- whether an address needs resolving is the + // resolver's call, and a custom one may redirect it. Its name is the one the operator + // configured, so it must survive onto whatever the resolver substitutes, exactly as it did when + // Netty resolved the TCP destination and the channel kept the original endpoint for TLS. + InetSocketAddress original = new InetSocketAddress("localhost", 9042); + InetSocketAddress candidate = + new InetSocketAddress(InetAddress.getByAddress(new byte[] {10, 0, 0, 1}), 9042); + + InetSocketAddress result = + (InetSocketAddress) ChannelFactory.reattachHostname(original, candidate); + + assertThat(AddressUtils.carriesName(original)).isTrue(); + assertThat(result.getHostString()).isEqualTo("localhost"); + assertThat(result.getAddress().getHostAddress()).isEqualTo("10.0.0.1"); + } + + @Test + public void should_reattach_hostname_to_nameless_ipv6_address() throws Exception { + byte[] loopback = new byte[16]; + loopback[15] = 1; // ::1 + InetSocketAddress candidate = new InetSocketAddress(InetAddress.getByAddress(loopback), 9042); + + InetSocketAddress result = + (InetSocketAddress) ChannelFactory.reattachHostname(HOSTNAME, candidate); + + assertThat(result.getHostString()).isEqualTo("test.cluster.fake"); + assertThat(result.getAddress()).isEqualTo(candidate.getAddress()); + assertThat(result.getPort()).isEqualTo(9042); + } + + @Test + public void should_keep_the_scope_when_reattaching_to_a_scoped_ipv6_address() throws Exception { + // A link-local address only points anywhere together with its zone, so the queried name has to + // be re-attached without dropping the scope. InetAddress.getByAddress(host, bytes) cannot carry + // one, but Inet6Address.getByAddress(host, bytes, scopeId) can. + byte[] linkLocal = new byte[16]; + linkLocal[0] = (byte) 0xfe; + linkLocal[1] = (byte) 0x80; + linkLocal[15] = 1; + InetSocketAddress candidate = + new InetSocketAddress(Inet6Address.getByAddress(null, linkLocal, 3), 9042); + + InetSocketAddress result = + (InetSocketAddress) ChannelFactory.reattachHostname(HOSTNAME, candidate); + + assertThat(result.getHostString()).isEqualTo("test.cluster.fake"); + assertThat(result.getAddress()).isInstanceOf(Inet6Address.class); + assertThat(((Inet6Address) result.getAddress()).getScopeId()).isEqualTo(3); + assertThat(result.getAddress().getAddress()).isEqualTo(linkLocal); + assertThat(result.getPort()).isEqualTo(9042); + } + + @Test + public void should_keep_the_zone_of_an_interface_scoped_ipv6_address() throws Exception { + // An address built from a NetworkInterface rather than from an index must keep pointing into + // the + // same zone. The numeric scope the JDK derived at construction is what the connect goes on, so + // carrying that over is enough; only the interface name, a toString() detail, is not. + Inet6Address linkLocal = firstInterfaceScopedIpv6Address(); + assumeThat(linkLocal).as("no interface-scoped IPv6 address on this host").isNotNull(); + InetSocketAddress candidate = new InetSocketAddress(linkLocal, 9042); + + InetSocketAddress result = + (InetSocketAddress) ChannelFactory.reattachHostname(HOSTNAME, candidate); + + assertThat(result.getHostString()).isEqualTo("test.cluster.fake"); + assertThat(((Inet6Address) result.getAddress()).getScopeId()).isEqualTo(linkLocal.getScopeId()); + assertThat(result.getAddress().getAddress()).isEqualTo(linkLocal.getAddress()); + } + + /** An interface-scoped IPv6 address of this host, or null if it has none. */ + private static Inet6Address firstInterfaceScopedIpv6Address() throws Exception { + for (NetworkInterface nif : Collections.list(NetworkInterface.getNetworkInterfaces())) { + for (InetAddress address : Collections.list(nif.getInetAddresses())) { + if (address instanceof Inet6Address + && ((Inet6Address) address).getScopedInterface() != null) { + return (Inet6Address) address; + } + } + } + return null; + } + + @Test + public void should_fail_future_when_endpoint_resolve_throws() { + // ChannelFactory calls EndPoint.resolve() directly on the caller thread, so a third-party + // implementation that throws must surface as a failed future rather than an escaping exception. + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + ChannelFactory factory = newChannelFactory(); + IllegalStateException failure = new IllegalStateException("resolve() blew up"); + + CompletionStage channelFuture = + factory.connect( + new ThrowingEndPoint(failure), + null, + null, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE); + + assertThatStage(channelFuture).isFailed(e -> assertThat(e).isSameAs(failure)); + } + + @Test + public void should_fail_future_when_endpoint_resolve_returns_null() { + // EndPoint.resolve() is contractually non-null, but a broken third-party implementation must + // fail fast rather than NPE later inside an event-loop task, which would leave the future + // hanging. + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + ChannelFactory factory = newChannelFactory(); + + CompletionStage channelFuture = + factory.connect( + new NullResolvingEndPoint(), + null, + null, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE); + + assertThatStage(channelFuture) + .isFailed( + e -> + assertThat(e) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("returned null")); + } + + @Test + public void should_fail_future_when_event_loop_group_is_rejecting_tasks() + throws InterruptedException { + // Resolution is dispatched to an I/O event loop; if the group is already shutting down, that + // dispatch is rejected synchronously. The rejection must fail the future rather than escape to + // the caller (connect() never used to throw) or leave the future hanging. + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + ChannelFactory factory = newChannelFactory(); + clientGroup.shutdownGracefully(0, 0, TimeUnit.MILLISECONDS).sync(); + + CompletionStage channelFuture = + factory.connect( + new DefaultEndPoint(HOSTNAME), + null, + null, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE); + + assertThatStage(channelFuture) + .isFailed(e -> assertThat(e).isInstanceOf(RejectedExecutionException.class)); + } + + /** An endpoint whose {@link EndPoint#resolve()} throws, standing in for a broken third party. */ + private static class ThrowingEndPoint implements EndPoint { + + private final RuntimeException failure; + + ThrowingEndPoint(RuntimeException failure) { + this.failure = failure; + } + + @NonNull + @Override + public SocketAddress resolve() { + throw failure; + } + + @NonNull + @Override + public String asMetricPrefix() { + return "test"; + } + } + + /** A broken third-party endpoint that violates {@code resolve()}'s non-null contract. */ + private static class NullResolvingEndPoint implements EndPoint { + + @NonNull + @Override + @SuppressWarnings("NullAway") // deliberately broken, that is the point of the test + public SocketAddress resolve() { + return null; + } + + @NonNull + @Override + public String asMetricPrefix() { + return "test"; + } + } +} diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryNettyResolverTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryNettyResolverTest.java new file mode 100644 index 00000000000..1688ed4c404 --- /dev/null +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryNettyResolverTest.java @@ -0,0 +1,379 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.datastax.oss.driver.internal.core.channel; + +import static com.datastax.oss.driver.Assertions.assertThatStage; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.when; + +import com.datastax.oss.driver.api.core.DefaultProtocolVersion; +import com.datastax.oss.driver.api.core.config.DefaultDriverOption; +import com.datastax.oss.driver.internal.core.metadata.DefaultEndPoint; +import com.datastax.oss.driver.internal.core.metrics.NoopNodeMetricUpdater; +import io.netty.bootstrap.Bootstrap; +import io.netty.channel.DefaultEventLoopGroup; +import io.netty.channel.local.LocalAddress; +import io.netty.resolver.AddressResolver; +import io.netty.resolver.AddressResolverGroup; +import io.netty.util.concurrent.EventExecutor; +import io.netty.util.concurrent.Future; +import io.netty.util.concurrent.Promise; +import java.net.InetSocketAddress; +import java.net.SocketAddress; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.concurrent.CompletionStage; +import java.util.concurrent.TimeUnit; +import org.junit.Test; + +/** + * Verifies that {@link ChannelFactory} expands unresolved candidate addresses through Netty's + * configured {@link AddressResolverGroup}, rather than doing its own JVM DNS lookup. + * + *

This is what keeps a custom resolver installed via {@link + * com.datastax.oss.driver.internal.core.context.NettyOptions#afterBootstrapInitialized(Bootstrap)} + * effective: before multi-address support, an unresolved address was handed straight to {@code + * Bootstrap.connect()} and Netty's resolver expanded it, so resolving anywhere else would silently + * bypass the user's configuration. + */ +public class ChannelFactoryNettyResolverTest extends ChannelFactoryTestBase { + + // A local address that no server is bound to: connecting to it fails immediately. + private static final SocketAddress UNREACHABLE = + new LocalAddress(ChannelFactoryNettyResolverTest.class.getSimpleName() + "-unreachable"); + + /** The hostname the endpoint reports, and that only the custom resolver knows how to expand. */ + private static final InetSocketAddress HOSTNAME = + InetSocketAddress.createUnresolved("test.cluster.fake", 9042); + + @Test + public void should_expand_unresolved_address_through_the_custom_netty_resolver() { + // Given – a resolver that maps the hostname to an unreachable address followed by the running + // local server, mimicking a DNS round-robin entry whose first record is dead. + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + TestAddressResolverGroup resolverGroup = + new TestAddressResolverGroup(Arrays.asList(UNREACHABLE, SERVER_ADDRESS.resolve())); + installResolver(resolverGroup); + ChannelFactory factory = newChannelFactory(); + + // When – the endpoint itself performs no resolution at all; it just yields the hostname. + CompletionStage channelFuture = + factory.connect( + new DefaultEndPoint(HOSTNAME), + null, + null, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE); + // The handshake only happens once we fall back to the reachable second address. + completeSimpleChannelInit(); + + // Then – the custom resolver was consulted for the hostname, and *all* the addresses it + // returned + // were tried, so the connection survived the dead first record. + assertThatStage(channelFuture).isSuccess(); + assertThat(resolverGroup.queried) + .as("the custom Netty resolver must be the one expanding the hostname") + .containsExactly(HOSTNAME); + } + + @Test + public void should_fail_when_the_custom_resolver_cannot_resolve_the_only_candidate() { + // Given – a resolver that fails every lookup. + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + TestAddressResolverGroup resolverGroup = new TestAddressResolverGroup(null); + installResolver(resolverGroup); + ChannelFactory factory = newChannelFactory(); + + // When + CompletionStage channelFuture = + factory.connect( + new DefaultEndPoint(HOSTNAME), + null, + null, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE); + + // Then – no candidate survived resolution, so the connect fails with the resolver's own cause + // rather than, say, an empty-candidate-list error. + assertThatStage(channelFuture) + .isFailed(e -> assertThat(e).hasMessageContaining("mock resolver failure")); + } + + @Test + public void should_not_resolve_at_all_when_the_user_disabled_the_resolver() { + // Given – Bootstrap.disableResolver() means config().resolver() is null. ChannelFactory must + // treat that as "pass the candidates through" instead of dereferencing the missing group. + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + TestAddressResolverGroup resolverGroup = + new TestAddressResolverGroup(Collections.singletonList(UNREACHABLE)); + doAnswer( + invocation -> { + Bootstrap bootstrap = invocation.getArgument(0); + bootstrap.resolver(resolverGroup).disableResolver(); + return null; + }) + .when(nettyOptions) + .afterBootstrapInitialized(any(Bootstrap.class)); + ChannelFactory factory = newChannelFactory(); + + // When – the endpoint yields an already-usable address. + CompletionStage channelFuture = + factory.connect( + SERVER_ADDRESS, + null, + null, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE); + completeSimpleChannelInit(); + + // Then – connection succeeds and the resolver was never even instantiated, let alone consulted. + assertThatStage(channelFuture).isSuccess(); + assertThat(resolverGroup.resolverRequested).isFalse(); + assertThat(resolverGroup.queried).isEmpty(); + } + + @Test + public void should_pass_already_resolved_address_through_untouched() { + // Given – an endpoint whose address is already resolved, which is the common case: metadata + // nodes hold resolved addresses from the peers rows, so this is every pool refill and every + // reconnect. A resolver with the usual semantics reports it as resolved and there is nothing + // to expand. + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + TestAddressResolverGroup resolverGroup = + new TestAddressResolverGroup(Collections.singletonList(UNREACHABLE)); + installResolver(resolverGroup); + ChannelFactory factory = newChannelFactory(); + + // When + CompletionStage channelFuture = + factory.connect( + SERVER_ADDRESS, + null, + null, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE); + completeSimpleChannelInit(); + + // Then – no lookup was performed: had one been, it would have redirected us to UNREACHABLE and + // the connection would have failed. The decision was the resolver's own, though -- see + // should_let_the_resolver_redirect_an_already_resolved_address. + assertThatStage(channelFuture).isSuccess(); + assertThat(resolverGroup.queried).isEmpty(); + assertThat(resolverGroup.resolverRequested) + .as("whether an address needs resolving must be the resolver's decision") + .isTrue(); + } + + @Test + public void should_let_the_resolver_redirect_an_already_resolved_address() { + // Given – a resolver that reports even an address carrying an IP as still needing resolution, + // and redirects it. Netty consulted the resolver for every connect, resolved address or not + // (Bootstrap#doResolveAndConnect0 calls isSupported()/isResolved() on it rather than testing + // the address itself), so short-circuiting on InetSocketAddress#isUnresolved() here would take + // that away for every connect to an already-resolved node -- which is nearly all of them. + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + TestAddressResolverGroup resolverGroup = + new TestAddressResolverGroup( + Collections.singletonList(SERVER_ADDRESS.resolve()), + /* claimNothingIsResolved = */ true); + installResolver(resolverGroup); + ChannelFactory factory = newChannelFactory(); + + // When – the endpoint holds a resolved address that nothing is listening on. + InetSocketAddress resolved = new InetSocketAddress("127.0.0.1", 9042); + CompletionStage channelFuture = + factory.connect( + new DefaultEndPoint(resolved), + null, + null, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE); + completeSimpleChannelInit(); + + // Then – the connect landed on the address the resolver substituted, which it could only do by + // having been asked about an address that already carried an IP. Exactly one lookup: the + // per-attempt bootstrap has the resolver disabled, so the substitute is connected to as-is + // rather than being handed back to the resolver (see the next test for why that matters). + assertThatStage(channelFuture).isSuccess(); + assertThat(resolverGroup.queried) + .as("the resolver must get a say on an address that already carries an IP") + .containsExactly(resolved); + } + + @Test + public void should_try_every_candidate_when_the_resolver_redirects() { + // Given – the same redirecting resolver as above, but answering with more than one address: + // a dead one first, then the running local server. + // + // The per-attempt bootstrap must not re-resolve. Bootstrap.clone() carries the resolver + // configuration over, and Netty's own pass calls resolve() -- *singular* -- so with a resolver + // that reports resolved addresses as unresolved, every candidate would be redirected again onto + // the resolver's first answer: the dead address, N times over. Multi-address fallback would + // silently do nothing, and the endpoint pinned onto the channel would name an address the + // channel is not connected to -- which is what the SSL engine's peer host and + // DefaultTopologyMonitor#savePort are then derived from. + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + TestAddressResolverGroup resolverGroup = + new TestAddressResolverGroup( + Arrays.asList(UNREACHABLE, SERVER_ADDRESS.resolve()), + /* claimNothingIsResolved = */ true); + installResolver(resolverGroup); + ChannelFactory factory = newChannelFactory(); + + CompletionStage channelFuture = + factory.connect( + new DefaultEndPoint(HOSTNAME), + null, + null, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE); + completeSimpleChannelInit(); + + // Then – the reachable address was actually reached. This holds whichever candidate rotate() + // starts from, and it is precisely what fails when the clone re-resolves: the dead address is + // the resolver's first answer, so both attempts would land there and the connect would fail. + assertThatStage(channelFuture).isSuccess(); + assertThat(resolverGroup.queried) + .as("the hostname is expanded once, by ChannelFactory; the candidates are not re-resolved") + .containsExactly(HOSTNAME); + } + + @Test + public void should_resolve_and_connect_on_the_same_event_loop() throws InterruptedException { + // Resolution and channel registration must share the loop picked once per connect. Taking one + // loop for resolution and letting the registration pick another would advance the group's + // round-robin chooser twice per connect, parking every channel on half the loops with the + // default power-of-two chooser. The base's single-thread group would make this assertion + // vacuous, so use two loops -- on which the split behavior was deterministic. + DefaultEventLoopGroup twoLoops = new DefaultEventLoopGroup(2); + try { + when(nettyOptions.ioEventLoopGroup()).thenReturn(twoLoops); + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + TestAddressResolverGroup resolverGroup = + new TestAddressResolverGroup(Collections.singletonList(SERVER_ADDRESS.resolve())); + installResolver(resolverGroup); + ChannelFactory factory = newChannelFactory(); + + CompletionStage channelFuture = + factory.connect( + new DefaultEndPoint(HOSTNAME), + null, + null, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE); + completeSimpleChannelInit(); + + assertThatStage(channelFuture) + .isSuccess( + channel -> + assertThat((Object) channel.eventLoop()) + .as("the channel must be registered on the loop resolution ran on") + .isSameAs(resolverGroup.resolverExecutor)); + } finally { + twoLoops.shutdownGracefully(0, 100, TimeUnit.MILLISECONDS).sync(); + } + } + + @Test + public void should_fail_future_when_resolver_throws_synchronously() { + // Given – a broken custom resolver that throws instead of returning a failed future. The throw + // happens inside an event-loop task, where nothing else would ever complete the connect future: + // nothing at this stage has a timeout, so before the blanket catch in resolveCandidates() this + // hung the connect attempt (and with it control-connection init) forever. + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + RuntimeException failure = new IllegalStateException("broken resolver"); + installResolver(new ThrowingAddressResolverGroup(failure)); + ChannelFactory factory = newChannelFactory(); + + // When + CompletionStage channelFuture = + factory.connect( + new DefaultEndPoint(HOSTNAME), + null, + null, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE); + + // Then + assertThatStage(channelFuture).isFailed(e -> assertThat(e).isSameAs(failure)); + } + + /** A resolver whose every method throws, standing in for a broken third-party implementation. */ + private static class ThrowingAddressResolverGroup extends AddressResolverGroup { + + private final RuntimeException failure; + + ThrowingAddressResolverGroup(RuntimeException failure) { + this.failure = failure; + } + + @Override + protected AddressResolver newResolver(EventExecutor executor) { + return new AddressResolver() { + + @Override + public boolean isSupported(SocketAddress address) { + throw failure; + } + + @Override + public boolean isResolved(SocketAddress address) { + throw failure; + } + + @Override + public Future resolve(SocketAddress address) { + throw failure; + } + + @Override + public Future resolve( + SocketAddress address, Promise promise) { + throw failure; + } + + @Override + public Future> resolveAll(SocketAddress address) { + throw failure; + } + + @Override + public Future> resolveAll( + SocketAddress address, Promise> promise) { + throw failure; + } + + @Override + public void close() { + // nothing to do + } + }; + } + } +} diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryPinnedEndPointTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryPinnedEndPointTest.java new file mode 100644 index 00000000000..4bf21472a79 --- /dev/null +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryPinnedEndPointTest.java @@ -0,0 +1,189 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.datastax.oss.driver.internal.core.channel; + +import static com.datastax.oss.driver.Assertions.assertThatStage; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.when; + +import com.datastax.oss.driver.api.core.DefaultProtocolVersion; +import com.datastax.oss.driver.api.core.config.DefaultDriverOption; +import com.datastax.oss.driver.api.core.metadata.EndPoint; +import com.datastax.oss.driver.internal.core.metadata.PinnableEndPoint; +import com.datastax.oss.driver.internal.core.metrics.NoopNodeMetricUpdater; +import edu.umd.cs.findbugs.annotations.NonNull; +import io.netty.channel.local.LocalAddress; +import java.net.InetSocketAddress; +import java.net.SocketAddress; +import java.util.Arrays; +import java.util.Collections; +import java.util.Objects; +import java.util.concurrent.CompletionStage; +import org.junit.Test; + +/** + * Verifies that a successfully connected {@link DriverChannel} carries an endpoint bound to the + * address the connection actually used, not the multi-address original. + * + *

Without this, a hostname shared by several nodes would let a later reconnect land on a + * different node while still being treated as the original {@code host_id}: {@code + * DefaultTopologyMonitor#buildNodeEndPoint} stores the channel's endpoint for the control node, and + * {@code ControlConnection} skips identity re-resolution for nodes that already have a host id. See + * {@link PinnableEndPoint}. + */ +public class ChannelFactoryPinnedEndPointTest extends ChannelFactoryTestBase { + + // A local address that no server is bound to: connecting to it fails immediately. + private static final SocketAddress UNREACHABLE = + new LocalAddress(ChannelFactoryPinnedEndPointTest.class.getSimpleName() + "-unreachable"); + + /** The name the endpoint reports, and that only the resolver knows how to expand. */ + private static final InetSocketAddress HOSTNAME = + InetSocketAddress.createUnresolved("test.cluster.fake", 9042); + + @Test + public void should_pin_channel_endpoint_to_the_address_that_connected() { + // Given – an endpoint reporting a name, which the resolver expands to a dead address and the + // running local server. Whichever of the two the connection ends up on, the channel must carry + // that one. + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + SocketAddress reachable = SERVER_ADDRESS.resolve(); + installResolver(new TestAddressResolverGroup(Arrays.asList(UNREACHABLE, reachable))); + ChannelFactory factory = newChannelFactory(); + TestPinnableEndPoint endPoint = new TestPinnableEndPoint(HOSTNAME); + + // When + CompletionStage channelFuture = + factory.connect( + endPoint, null, null, DriverChannelOptions.DEFAULT, NoopNodeMetricUpdater.INSTANCE); + completeSimpleChannelInit(); + + // Then + assertThatStage(channelFuture) + .isSuccess( + channel -> { + EndPoint channelEndPoint = channel.getEndPoint(); + // The channel resolves to the address it is actually connected to -- the name it was + // built from is gone from resolve(), which is what SSL engines and authenticators + // need. + assertThat(channelEndPoint.resolve()).isEqualTo(reachable); + // ...while still denoting the same node, so node lookups and metric names are stable. + assertThat(channelEndPoint).isEqualTo(endPoint); + assertThat(channelEndPoint.asMetricPrefix()).isEqualTo(endPoint.asMetricPrefix()); + }); + } + + @Test + public void should_leave_non_pinnable_endpoints_untouched() { + // A third-party EndPoint that does not implement PinnableEndPoint must reach the channel + // exactly + // as it was given, so existing implementations keep working. + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + ChannelFactory factory = newChannelFactory(); + + CompletionStage channelFuture = + factory.connect( + SERVER_ADDRESS, + null, + null, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE); + completeSimpleChannelInit(); + + assertThatStage(channelFuture) + .isSuccess(channel -> assertThat(channel.getEndPoint()).isSameAs(SERVER_ADDRESS)); + } + + @Test + public void should_fail_future_when_pin_to_throws() { + // pinTo() runs in the continuation after resolution, whose exceptions CompletionStage + // swallows; a throwing implementation must fail the connect future rather than hang it. + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + SocketAddress reachable = SERVER_ADDRESS.resolve(); + installResolver(new TestAddressResolverGroup(Collections.singletonList(reachable))); + ChannelFactory factory = newChannelFactory(); + RuntimeException failure = new IllegalStateException("pinTo blew up"); + TestPinnableEndPoint endPoint = + new TestPinnableEndPoint(HOSTNAME) { + @NonNull + @Override + public EndPoint pinTo(@NonNull SocketAddress resolvedAddress) { + throw failure; + } + }; + + CompletionStage channelFuture = + factory.connect( + endPoint, null, null, DriverChannelOptions.DEFAULT, NoopNodeMetricUpdater.INSTANCE); + + assertThatStage(channelFuture).isFailed(e -> assertThat(e).isSameAs(failure)); + } + + /** + * A {@link PinnableEndPoint} that can hold a pin to any {@link SocketAddress}, including the + * local-transport addresses these tests connect over (which {@code DefaultEndPoint} cannot). + * Identity is the unpinned address, so a pinned copy stays equal to the original — the contract + * {@link PinnableEndPoint} requires. + */ + private static class TestPinnableEndPoint implements PinnableEndPoint { + + private final SocketAddress address; + private final SocketAddress pinnedAddress; + + TestPinnableEndPoint(SocketAddress address) { + this(address, null); + } + + private TestPinnableEndPoint(SocketAddress address, SocketAddress pinnedAddress) { + this.address = address; + this.pinnedAddress = pinnedAddress; + } + + @NonNull + @Override + public SocketAddress resolve() { + return pinnedAddress != null ? pinnedAddress : address; + } + + @NonNull + @Override + public EndPoint pinTo(@NonNull SocketAddress resolvedAddress) { + return new TestPinnableEndPoint(address, resolvedAddress); + } + + @NonNull + @Override + public String asMetricPrefix() { + return "test"; + } + + @Override + public boolean equals(Object other) { + return (other instanceof TestPinnableEndPoint) + && address.equals(((TestPinnableEndPoint) other).address); + } + + @Override + public int hashCode() { + return Objects.hash(address); + } + } +} diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryProtocolNegotiationTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryProtocolNegotiationTest.java index fceb8777904..6e868e5cc0b 100644 --- a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryProtocolNegotiationTest.java +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryProtocolNegotiationTest.java @@ -25,6 +25,7 @@ import com.datastax.oss.driver.api.core.UnsupportedProtocolVersionException; import com.datastax.oss.driver.api.core.config.DefaultDriverOption; import com.datastax.oss.driver.internal.core.TestResponses; +import com.datastax.oss.driver.internal.core.metadata.DefaultEndPoint; import com.datastax.oss.driver.internal.core.metrics.NoopNodeMetricUpdater; import com.datastax.oss.protocol.internal.Frame; import com.datastax.oss.protocol.internal.ProtocolConstants; @@ -33,6 +34,9 @@ import com.datastax.oss.protocol.internal.response.Ready; import com.tngtech.java.junit.dataprovider.DataProvider; import com.tngtech.java.junit.dataprovider.UseDataProvider; +import java.net.InetSocketAddress; +import java.net.SocketAddress; +import java.util.Arrays; import java.util.Optional; import java.util.concurrent.CompletionStage; import org.junit.Test; @@ -280,6 +284,168 @@ public void should_fail_if_negotiation_finds_no_matching_version(int errorCode) }); } + @Test + public void should_not_try_next_address_of_identified_node_when_negotiation_exhausts_versions() { + // Given – an *identified* node (its host id is known, so every address its name expands to is + // that same node) whose name expands to two candidates: the same live server twice, so + // whichever the rotation picks first is irrelevant. The server rejects every protocol version. + mockNegotiationLadderDownToV3(); + SocketAddress serverAddress = SERVER_ADDRESS.resolve(); + installResolver(new TestAddressResolverGroup(Arrays.asList(serverAddress, serverAddress))); + ChannelFactory factory = newChannelFactory(); + + // When + CompletionStage channelFuture = + factory.connect( + new DefaultEndPoint(InetSocketAddress.createUnresolved("test.cluster.fake", 9042)), + null, + null, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE, + true); + + exhaustNegotiationLadder(); + + // Then – the second candidate must not be attempted: for a node we have already identified, a + // protocol-version rejection is a property of the node, not of the address, so replaying the + // negotiation ladder against the remaining IPs would buy nothing. Checked before the future + // assertion so that on regression the stray frame is drained; leaving it unread would block the + // server's exchanger and hang the whole suite in tearDown() instead of failing this test. + assertThat(tryReadOutboundFrame(200)) + .as("second candidate must not be attempted after negotiation exhaustion") + .isNull(); + assertThatStage(channelFuture) + .isFailed( + e -> { + assertThat(e).isInstanceOf(UnsupportedProtocolVersionException.class); + assertThat(((UnsupportedProtocolVersionException) e).getAttemptedVersions()) + .containsExactly(DefaultProtocolVersion.V4, DefaultProtocolVersion.V3); + assertThat(e.getSuppressed()) + .as("no other candidate should have been tried, so nothing to suppress") + .isEmpty(); + }); + } + + @Test + public void + should_try_next_address_of_unidentified_endpoint_when_negotiation_exhausts_versions() { + // Given – the same setup, but for an endpoint the driver has not identified yet: a contact + // point, before host ids have been read. Its name may well expand to addresses of *different* + // nodes, so a version rejection by the first says nothing about the second. + mockNegotiationLadderDownToV3(); + SocketAddress serverAddress = SERVER_ADDRESS.resolve(); + installResolver(new TestAddressResolverGroup(Arrays.asList(serverAddress, serverAddress))); + ChannelFactory factory = newChannelFactory(); + + // When + CompletionStage channelFuture = + factory.connect( + new DefaultEndPoint(InetSocketAddress.createUnresolved("test.cluster.fake", 9042)), + null, + null, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE, + false); + + // The first candidate exhausts the ladder... + exhaustNegotiationLadder(); + // ...and the second is tried all the same, replaying the ladder from the top. Before this, + // resolve-contact-points=true made each address a separate node and ControlConnection advanced + // to the next one on exactly this error; collapsing a name into one node must not lose that. + exhaustNegotiationLadder(); + + // Then + assertThat(tryReadOutboundFrame(200)) + .as("the name expands to two addresses, so there is no third attempt") + .isNull(); + assertThatStage(channelFuture) + .isFailed( + e -> { + assertThat(e).isInstanceOf(UnsupportedProtocolVersionException.class); + assertThat(((UnsupportedProtocolVersionException) e).getAttemptedVersions()) + .as("each candidate negotiates on its own, so this is the last one's ladder") + .containsExactly(DefaultProtocolVersion.V4, DefaultProtocolVersion.V3); + assertThat(e.getSuppressed()) + .as("the first candidate's failure must still be reported") + .hasSize(1); + assertThat(e.getSuppressed()[0]) + .isInstanceOf(UnsupportedProtocolVersionException.class); + }); + } + + /** Negotiation starts at V4 and has exactly one downgrade available, to V3. */ + private void mockNegotiationLadderDownToV3() { + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + when(protocolVersionRegistry.downgrade(DefaultProtocolVersion.V4)) + .thenReturn(Optional.of(DefaultProtocolVersion.V3)); + when(protocolVersionRegistry.downgrade(DefaultProtocolVersion.V3)).thenReturn(Optional.empty()); + } + + /** + * Plays the server side of a full negotiation ladder against one candidate address: V4 rejected, + * downgrade retry with V3 rejected, i.e. no version left to try on that address. + */ + private void exhaustNegotiationLadder() { + Frame requestFrame = readOutboundFrame(); + assertThat(requestFrame.message).isInstanceOf(Options.class); + writeInboundFrame(requestFrame, TestResponses.supportedResponse("mock_key", "mock_value")); + + requestFrame = readOutboundFrame(); + assertThat(requestFrame.protocolVersion).isEqualTo(DefaultProtocolVersion.V4.getCode()); + writeInboundFrame( + requestFrame, + new Error( + ProtocolConstants.ErrorCode.PROTOCOL_ERROR, "Invalid or unsupported protocol version")); + + requestFrame = readOutboundFrame(); + assertThat(requestFrame.message).isInstanceOf(Options.class); + writeInboundFrame(requestFrame, TestResponses.supportedResponse("mock_key", "mock_value")); + + requestFrame = readOutboundFrame(); + assertThat(requestFrame.protocolVersion).isEqualTo(DefaultProtocolVersion.V3.getCode()); + writeInboundFrame( + requestFrame, + new Error( + ProtocolConstants.ErrorCode.PROTOCOL_ERROR, "Invalid or unsupported protocol version")); + } + + @Test + public void should_fail_future_when_downgrade_lookup_throws_in_connect_listener() { + // Given – a version registry that throws when the factory looks up the downgrade. The lookup + // runs inside the Netty connect listener, which swallows throwables: without the blanket catch + // in connectToAddress() the connect future would never complete and the attempt would hang. + when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); + when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); + RuntimeException failure = new IllegalStateException("registry broken"); + when(protocolVersionRegistry.downgrade(DefaultProtocolVersion.V4)).thenThrow(failure); + ChannelFactory factory = newChannelFactory(); + + // When + CompletionStage channelFuture = + factory.connect( + SERVER_ADDRESS, + null, + null, + DriverChannelOptions.DEFAULT, + NoopNodeMetricUpdater.INSTANCE); + + Frame requestFrame = readOutboundFrame(); + assertThat(requestFrame.message).isInstanceOf(Options.class); + writeInboundFrame(requestFrame, TestResponses.supportedResponse("mock_key", "mock_value")); + + requestFrame = readOutboundFrame(); + assertThat(requestFrame.protocolVersion).isEqualTo(DefaultProtocolVersion.V4.getCode()); + // Server does not support v4, which is what sends the factory to the downgrade lookup + writeInboundFrame( + requestFrame, + new Error( + ProtocolConstants.ErrorCode.PROTOCOL_ERROR, "Invalid or unsupported protocol version")); + + // Then + assertThatStage(channelFuture).isFailed(e -> assertThat(e).isSameAs(failure)); + } + /** * Depending on the Cassandra version, an "unsupported protocol" response can use different error * codes, so we test all of them. diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.java b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.java index 2780d5bdec9..12ab96c59b9 100644 --- a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.java +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryTestBase.java @@ -20,6 +20,8 @@ import static java.util.concurrent.TimeUnit.MILLISECONDS; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.fail; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.when; import com.datastax.oss.driver.api.core.ProtocolVersion; @@ -42,6 +44,7 @@ import com.datastax.oss.protocol.internal.request.Startup; import com.datastax.oss.protocol.internal.response.Ready; import com.tngtech.java.junit.dataprovider.DataProviderRunner; +import io.netty.bootstrap.Bootstrap; import io.netty.bootstrap.ServerBootstrap; import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufAllocator; @@ -53,6 +56,8 @@ import io.netty.channel.DefaultEventLoopGroup; import io.netty.channel.local.LocalChannel; import io.netty.channel.local.LocalServerChannel; +import io.netty.resolver.AddressResolverGroup; +import java.net.SocketAddress; import java.time.Duration; import java.util.Collections; import java.util.Optional; @@ -188,6 +193,38 @@ protected Frame readOutboundFrame() { return null; // never reached } + /** + * Like {@link #readOutboundFrame()}, but returns {@code null} instead of failing the test when no + * frame arrives within {@code timeoutMillis}. + * + *

Use this to assert that the client did not send another request. Unlike asserting via + * a failing read, it also drains a frame that does arrive: the server-side exchange in {@link + * ServerInitializer} has no timeout, so a stray unread frame would block the server event loop + * and hang the whole suite in {@link #tearDown()} instead of failing just the test. + */ + protected Frame tryReadOutboundFrame(long timeoutMillis) { + try { + return requestFrameExchanger.exchange(null, timeoutMillis, MILLISECONDS); + } catch (InterruptedException e) { + fail("unexpected interruption while waiting for outbound frame", e); + return null; // never reached + } catch (TimeoutException e) { + return null; + } + } + + /** Installs {@code group} the way a user would, through the {@code NettyOptions} hook. */ + protected void installResolver(AddressResolverGroup group) { + doAnswer( + invocation -> { + Bootstrap bootstrap = invocation.getArgument(0); + bootstrap.resolver(group); + return null; + }) + .when(nettyOptions) + .afterBootstrapInitialized(any(Bootstrap.class)); + } + protected void writeInboundFrame(Frame requestFrame, Message response) { writeInboundFrame(requestFrame, response, requestFrame.protocolVersion); } diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/channel/TestAddressResolverGroup.java b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/TestAddressResolverGroup.java new file mode 100644 index 00000000000..06f5f636cba --- /dev/null +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/channel/TestAddressResolverGroup.java @@ -0,0 +1,130 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.datastax.oss.driver.internal.core.channel; + +import edu.umd.cs.findbugs.annotations.Nullable; +import io.netty.bootstrap.Bootstrap; +import io.netty.channel.local.LocalAddress; +import io.netty.resolver.AddressResolver; +import io.netty.resolver.AddressResolverGroup; +import io.netty.util.concurrent.EventExecutor; +import io.netty.util.concurrent.Future; +import io.netty.util.concurrent.Promise; +import java.net.InetSocketAddress; +import java.net.SocketAddress; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; + +/** + * A stand-in for a user-supplied {@code AddressResolverGroup} (e.g. Netty's {@code + * DnsAddressResolverGroup}), installed the way a user would install one: through {@link + * com.datastax.oss.driver.internal.core.context.NettyOptions#afterBootstrapInitialized(Bootstrap)}. + * + *

Records what it was asked to resolve, and answers with a fixed list of addresses so tests can + * assert that every one of them is tried, and in what order. + * + *

Implements {@link AddressResolver} directly rather than extending {@code + * AbstractAddressResolver} so it can hand back {@link LocalAddress}es — the unit tests connect over + * Netty's local transport, which is not reachable through an {@link InetSocketAddress}. + */ +class TestAddressResolverGroup extends AddressResolverGroup { + + /** Every address this group was asked to resolve, in order. */ + final List queried = new CopyOnWriteArrayList<>(); + + /** Whether a resolver was ever obtained from this group at all. */ + volatile boolean resolverRequested; + + /** The executor the last resolver was created for, i.e. the loop resolution runs on. */ + @Nullable volatile EventExecutor resolverExecutor; + + /** The addresses to answer with, or {@code null} to fail every lookup. */ + @Nullable private final List answer; + + /** + * Whether to claim that every address still needs resolving, even one that already carries an IP. + * A real resolver may do this to redirect traffic, and Netty honours it: {@code + * Bootstrap#doResolveAndConnect0} asks the resolver rather than testing the address itself. + */ + private final boolean claimNothingIsResolved; + + TestAddressResolverGroup(@Nullable List answer) { + this(answer, false); + } + + TestAddressResolverGroup(@Nullable List answer, boolean claimNothingIsResolved) { + this.answer = answer; + this.claimNothingIsResolved = claimNothingIsResolved; + } + + @Override + protected AddressResolver newResolver(EventExecutor executor) { + resolverRequested = true; + resolverExecutor = executor; + return new AddressResolver() { + + @Override + public boolean isSupported(SocketAddress address) { + return true; + } + + @Override + public boolean isResolved(SocketAddress address) { + if (claimNothingIsResolved) { + return false; + } + // Only hostnames need resolving; anything else (including the local-transport addresses we + // hand back) is already usable. + return !(address instanceof InetSocketAddress) + || !((InetSocketAddress) address).isUnresolved(); + } + + @Override + public Future resolve(SocketAddress address) { + return resolve(address, executor.newPromise()); + } + + @Override + public Future resolve(SocketAddress address, Promise promise) { + queried.add(address); + return answer == null + ? promise.setFailure(new IllegalStateException("mock resolver failure")) + : promise.setSuccess(answer.get(0)); + } + + @Override + public Future> resolveAll(SocketAddress address) { + return resolveAll(address, executor.newPromise()); + } + + @Override + public Future> resolveAll( + SocketAddress address, Promise> promise) { + queried.add(address); + return answer == null + ? promise.setFailure(new IllegalStateException("mock resolver failure")) + : promise.setSuccess(answer); + } + + @Override + public void close() { + // nothing to do + } + }; + } +} From 0969f67f038511fd063a32d60d80bda0b928886d Mon Sep 17 00:00:00 2001 From: Mikita Hradovich Date: Thu, 6 Aug 2026 18:16:10 +0200 Subject: [PATCH 5/9] fix: keep a node's metric identity stable across endpoint changes (DRIVER-201) Node metrics are named after the endpoint, so DefaultNode.setEndPoint() has to re-register them whenever those names change -- which is not the same question as whether this is a different node, and the old !equals() test got it wrong in both directions. It was too narrow: an unresolved hostname and the resolved address it maps to compare *equal* while their metric prefixes differ, which is exactly what happens when a contact-point node adopts the endpoint built from its system.local row. And too wide in the other direction is now possible too, since a pinned copy differs from its original only by an address that both equals() and the metric identity ignore by contract. The test is therefore asMetricPrefix() plus toString(), because both are in use: the default MetricIdGenerator names node metrics after the prefix, the tagging one tags them with toString(). The pin is excluded from toString() as well, or DefaultTopologyMonitor#buildNodeEndPoint returning the control channel's pinned endpoint for the system.local row would silently retag node metrics on every refresh and orphan the old series. The node also adopts the newest endpoint instance even when it compares equal, since a pinned copy carries the address every subsequent connection will use. Finally, the rebuild order is clear, then swap, then build. Dropwizard and MicroProfile do not remember the ids they registered under; their clearMetrics() recomputes each one from the node's endpoint as it stands at that moment. The previous order -- swap, build, clear -- therefore deleted exactly the series the new updater had just registered and left the old ones behind with nothing writing to them. That ordering is upstream's, but it used to be reached only when the endpoints compared unequal; keying the rebuild on metric identity brings the ordinary contact-point transition onto the same path. The pre-existing pin test was vacuous: a mocked context yields NoopNodeMetricUpdater, for which the rebuild is skipped entirely. Both tests now stub MetricsFactory, and the ordering test drives a real MetricRegistry through a hostname-to-IP rename; it was proven to fail under the old order. Co-Authored-By: Claude Opus 5 (1M context) --- .../internal/core/metadata/DefaultNode.java | 66 +++++++- .../core/metadata/DefaultNodeTest.java | 158 ++++++++++++++++++ 2 files changed, 215 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultNode.java b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultNode.java index 1b09c26ce16..4dc499f6035 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultNode.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultNode.java @@ -102,15 +102,63 @@ public EndPoint getEndPoint() { } public void setEndPoint(@NonNull EndPoint newEndPoint, @NonNull InternalDriverContext context) { - if (!newEndPoint.equals(endPoint)) { - endPoint = newEndPoint; - // metricUpdater is transient, so it can be null on deserialized nodes. - NodeMetricUpdater previousMetricUpdater = metricUpdater; - if (previousMetricUpdater != null - && !(previousMetricUpdater instanceof NoopNodeMetricUpdater)) { - metricUpdater = context.getMetricsFactory().newNodeUpdater(this); - previousMetricUpdater.clearMetrics(); - } + // Metrics are registered under names derived from the endpoint, so they have to be + // re-registered + // whenever those names change -- which is not the same question as whether this is a different + // node. It is narrower in one direction: a PinnableEndPoint copy differs from the original only + // by the address it is pinned to, and both equals() and the metric identity ignore that by + // contract (see PinnableEndPoint). And it is wider in the other: an unresolved hostname and the + // resolved address it maps to compare *equal* (see DefaultEndPoint#equals) while their metric + // prefixes differ, which is exactly what happens when a contact-point node adopts the endpoint + // built from its system.local row. + // + // Both halves of that identity are compared, because both are in use: the default + // MetricIdGenerator names node metrics after asMetricPrefix(), the tagging one tags them with + // the endpoint's toString(). Comparing toString() does mean that an endpoint whose string form + // is not stable across equal instances re-registers this node's metrics on every topology + // refresh; that is the intended reading, since the alternative is reporting under a name the + // endpoint no longer answers to. + boolean differentMetricIdentity = + !newEndPoint.asMetricPrefix().equals(endPoint.asMetricPrefix()) + || !newEndPoint.toString().equals(endPoint.toString()); + // metricUpdater is transient, so it can be null on deserialized nodes. + NodeMetricUpdater previousMetricUpdater = metricUpdater; + boolean rebuildMetricUpdater = + differentMetricIdentity + && previousMetricUpdater != null + && !(previousMetricUpdater instanceof NoopNodeMetricUpdater); + + // Clearing comes *before* the swap. Dropwizard and MicroProfile do not remember the ids they + // registered under; clearMetrics() recomputes each one from this node's current endpoint (see + // DropwizardMetricUpdater#clearMetrics and MetricIdGenerator#nodeMetricId). Clearing after the + // swap would therefore delete the series the new updater had just registered and leave the old + // ones behind, under a name nothing writes to any more. Micrometer removes the Meter instances + // it holds and does not care either way. + // + // The three steps are not atomic with respect to concurrent metric writes: metricUpdater is + // volatile and read from I/O threads, so a write landing between the clear and the rebuild + // goes through the updater that was just cleared, and Dropwizard re-registers on demand + // (getOrCreateCounterFor -> registry.counter(getMetricId(m))). That resurrects one series, + // named from whichever endpoint this node holds at that instant. The window is a few + // statements wide and nothing throws -- registry.counter() is get-or-create -- so it is left + // as is; closing it properly means having clearMetrics() take the ids to clear rather than + // recomputing them, which is a change to every metrics implementation. + if (rebuildMetricUpdater) { + previousMetricUpdater.clearMetrics(); + } + + // Adopt the newest instance even when it compares equal: a pinned copy carries the address + // every + // subsequent connection to this node will use, so refusing it would freeze the node on the + // first + // address it ever connected to, even after the control connection moved to another one and told + // us about it. + endPoint = newEndPoint; + + // And building comes *after* it: the updaters register every enabled metric from their + // constructor, deriving the names from the endpoint this node holds at that moment. + if (rebuildMetricUpdater) { + metricUpdater = context.getMetricsFactory().newNodeUpdater(this); } } diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultNodeTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultNodeTest.java index 6a53fe3e433..cb9e0a75cf2 100644 --- a/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultNodeTest.java +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultNodeTest.java @@ -18,10 +18,29 @@ package com.datastax.oss.driver.internal.core.metadata; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import com.codahale.metrics.MetricRegistry; +import com.datastax.oss.driver.api.core.config.DefaultDriverOption; +import com.datastax.oss.driver.api.core.config.DriverConfig; +import com.datastax.oss.driver.api.core.config.DriverExecutionProfile; import com.datastax.oss.driver.api.core.metadata.EndPoint; +import com.datastax.oss.driver.api.core.metrics.DefaultNodeMetric; +import com.datastax.oss.driver.api.core.metrics.NodeMetric; +import com.datastax.oss.driver.internal.core.context.InternalDriverContext; import com.datastax.oss.driver.internal.core.context.MockedDriverContextFactory; +import com.datastax.oss.driver.internal.core.metrics.AbstractMetricUpdater; +import com.datastax.oss.driver.internal.core.metrics.DefaultMetricIdGenerator; +import com.datastax.oss.driver.internal.core.metrics.DropwizardNodeMetricUpdater; +import com.datastax.oss.driver.internal.core.metrics.MetricsFactory; +import com.datastax.oss.driver.internal.core.metrics.NodeMetricUpdater; import java.net.InetSocketAddress; +import java.util.Collections; +import java.util.Set; import java.util.UUID; import org.junit.Test; @@ -55,4 +74,143 @@ public void should_have_expected_string_representation_if_hostid_is_null() { "Node(endPoint=localhost/127.0.0.1:9042, hostId=null, hashCode=%x)", node.hashCode()); assertThat(node.toString()).isEqualTo(expected); } + + @Test + public void should_adopt_a_newer_endpoint_that_only_differs_by_its_pinned_address() { + // A PinnableEndPoint copy compares equal to the original -- pinnedAddress is excluded from + // equals() by contract, so that a pinned copy still denotes the same node. setEndPoint() must + // therefore not use equals() to decide whether to adopt it: the pinned address is the one every + // subsequent connection to this node will use, so refusing the newer instance would freeze the + // node on the first address it ever connected to, even after the control connection has moved + // and told us about it. + InternalDriverContext context = MockedDriverContextFactory.defaultDriverContext(); + DefaultNode node = new DefaultNode(endPoint, context); + + EndPoint pinnedToFirst = + ((PinnableEndPoint) endPoint).pinTo(new InetSocketAddress("127.0.0.2", 9042)); + node.setEndPoint(pinnedToFirst, context); + assertThat(node.getEndPoint()).isSameAs(pinnedToFirst); + + EndPoint pinnedToSecond = + ((PinnableEndPoint) endPoint).pinTo(new InetSocketAddress("127.0.0.3", 9042)); + // Same node by equals(), different pinned address. + assertThat(pinnedToSecond).isEqualTo(pinnedToFirst); + node.setEndPoint(pinnedToSecond, context); + + assertThat(node.getEndPoint()).isSameAs(pinnedToSecond); + assertThat(node.getEndPoint().resolve()).isEqualTo(new InetSocketAddress("127.0.0.3", 9042)); + } + + @Test + public void should_not_rebuild_the_metric_updater_for_a_pin_only_change() { + // A pinned copy is identified exactly like the original -- same asMetricPrefix(), same + // toString() -- so rebuilding would clear and re-register metrics under identical names, and + // reset their values along the way. + MetricsFactory metricsFactory = mock(MetricsFactory.class); + InternalDriverContext context = contextWith(metricsFactory); + NodeMetricUpdater first = mock(NodeMetricUpdater.class); + NodeMetricUpdater second = mock(NodeMetricUpdater.class); + when(metricsFactory.newNodeUpdater(any())).thenReturn(first, second); + DefaultNode node = new DefaultNode(endPoint, context); + assertThat(node.getMetricUpdater()).isSameAs(first); + + node.setEndPoint( + ((PinnableEndPoint) endPoint).pinTo(new InetSocketAddress("127.0.0.2", 9042)), context); + + assertThat(node.getMetricUpdater()).isSameAs(first); + verify(first, never()).clearMetrics(); + } + + @Test + public void should_rebuild_the_metric_updater_when_an_equal_endpoint_renames_the_metrics() { + // An unresolved hostname and the address it resolves to compare *equal* (see + // DefaultEndPoint#equals) but do not produce the same metric prefix. That is exactly what + // happens when a contact-point node adopts the endpoint built from its system.local row, so + // deciding on equals() alone would leave the node's metrics registered under the hostname while + // asMetricPrefix() had moved on to the IP. + MetricsFactory metricsFactory = mock(MetricsFactory.class); + InternalDriverContext context = contextWith(metricsFactory); + NodeMetricUpdater first = mock(NodeMetricUpdater.class); + NodeMetricUpdater second = mock(NodeMetricUpdater.class); + when(metricsFactory.newNodeUpdater(any())).thenReturn(first, second); + + EndPoint asHostname = + new DefaultEndPoint(InetSocketAddress.createUnresolved("localhost", 9042)); + EndPoint asAddress = new DefaultEndPoint(new InetSocketAddress("127.0.0.1", 9042)); + assertThat(asHostname).isEqualTo(asAddress); + assertThat(asHostname.asMetricPrefix()).isNotEqualTo(asAddress.asMetricPrefix()); + + DefaultNode node = new DefaultNode(asHostname, context); + assertThat(node.getMetricUpdater()).isSameAs(first); + + node.setEndPoint(asAddress, context); + + assertThat(node.getMetricUpdater()).isSameAs(second); + verify(first).clearMetrics(); + } + + @Test + public void should_rebuild_the_metric_updater_without_wiping_the_metrics_it_registers() { + // The two tests above use mocks, which cannot see *when* clearMetrics() runs relative to the + // endpoint swap -- and that order is what decides whether the rebuild works. Dropwizard and + // MicroProfile do not remember the ids they registered under: clearMetrics() recomputes each + // one + // from the node's endpoint as it stands at that moment. Clearing after the swap therefore + // removes exactly the series the new updater has just registered, and leaves the old ones in + // the + // registry with nothing writing to them. So this one drives a real registry. + MetricRegistry registry = new MetricRegistry(); + NodeMetric metric = DefaultNodeMetric.UNSENT_REQUESTS; + InternalDriverContext context = dropwizardContext(registry, Collections.singleton(metric)); + + EndPoint asHostname = + new DefaultEndPoint(InetSocketAddress.createUnresolved("localhost", 9042)); + EndPoint asAddress = new DefaultEndPoint(new InetSocketAddress("127.0.0.1", 9042)); + String underHostname = "s.nodes." + asHostname.asMetricPrefix() + '.' + metric.getPath(); + String underAddress = "s.nodes." + asAddress.asMetricPrefix() + '.' + metric.getPath(); + assertThat(underHostname).isNotEqualTo(underAddress); + + DefaultNode node = new DefaultNode(asHostname, context); + assertThat(registry.getNames()).containsExactly(underHostname); + + node.setEndPoint(asAddress, context); + + assertThat(registry.getNames()).containsExactly(underAddress); + } + + /** A context wired to a real Dropwizard registry, enough for {@link DefaultNode} to use it. */ + private static InternalDriverContext dropwizardContext( + MetricRegistry registry, Set enabledMetrics) { + InternalDriverContext context = mock(InternalDriverContext.class); + DriverConfig config = mock(DriverConfig.class); + DriverExecutionProfile profile = mock(DriverExecutionProfile.class); + when(context.getSessionName()).thenReturn("s"); + when(context.getConfig()).thenReturn(config); + when(config.getDefaultProfile()).thenReturn(profile); + when(profile.getDuration(DefaultDriverOption.METRICS_NODE_EXPIRE_AFTER)) + .thenReturn(AbstractMetricUpdater.MIN_EXPIRE_AFTER); + when(profile.getString(DefaultDriverOption.METRICS_ID_GENERATOR_PREFIX, "")).thenReturn(""); + // Built outside the when(): the generator's constructor reads back from the context, and + // Mockito rejects a nested call on a stubbing that is still open. + DefaultMetricIdGenerator idGenerator = new DefaultMetricIdGenerator(context); + when(context.getMetricIdGenerator()).thenReturn(idGenerator); + + MetricsFactory metricsFactory = mock(MetricsFactory.class); + when(context.getMetricsFactory()).thenReturn(metricsFactory); + // Built on demand rather than up front: an updater registers its metrics from its constructor, + // under the names the node's endpoint yields at that point. + when(metricsFactory.newNodeUpdater(any())) + .thenAnswer( + invocation -> + new DropwizardNodeMetricUpdater( + invocation.getArgument(0), context, enabledMetrics, registry)); + return context; + } + + /** A context whose only stubbed behaviour is the metrics factory {@code DefaultNode} asks for. */ + private static InternalDriverContext contextWith(MetricsFactory metricsFactory) { + InternalDriverContext context = mock(InternalDriverContext.class); + when(context.getMetricsFactory()).thenReturn(metricsFactory); + return context; + } } From ee1797d561388153561e7b214e17df3d49753cbc Mon Sep 17 00:00:00 2001 From: Mikita Hradovich Date: Thu, 6 Aug 2026 18:16:25 +0200 Subject: [PATCH 6/9] fix: retry a contact point on another address when its identity read fails (DRIVER-201) ChannelFactory walks all of a hostname's addresses while it opens a channel, but the control node's identity is only read afterwards, over the channel that won: by then the remaining candidates are gone. ControlConnection advanced its query plan on that failure -- and since a contact-point hostname is now a single Node, that wrote off the whole hostname on the strength of one of its addresses. With a single contact point and the default reconnect-on-init=false, session initialization failed outright, and a rebuilt session got a fresh ChannelFactory whose rotation counters start at zero, so it failed the same way every time while a healthy address sat unused. The addresses of an unidentified endpoint may well belong to different nodes, which is the same reason ChannelFactory#isNodeWideFailure only treats a protocol-version rejection as terminal for a node whose host id is known. So the query plan entry is attempted again instead, and the next attempt lands on another address because ChannelFactory rotates its candidates once per connect. The walk terminates on the address set alone: it only grows, a retry requires it to grow, and coming back round to an address already in it ends the walk. MAX_ADDRESSES_PER_QUERY_PLAN_ENTRY is a backstop against a resolver that never repeats itself, and bounds how long one hostname can hold up initialization. It is tested before the address is recorded, so an entry is attempted at most one more time than the cap itself. It arms only for a node with no host id whose endpoint denotes a name and whose channel reports a resolved pinned address. Identified nodes stay pinned to one address on purpose, literals expand to exactly themselves, and a third-party EndPoint that ChannelFactory passed through without pinning cannot say which address answered -- all three keep behaving exactly as before. The landed address is captured as soon as the channel opens, because resolveChannelNodeIfNeeded() overwrites the channel's endpoint with the one built from the system.local row before the registration that can still fail. Co-Authored-By: Claude Opus 5 (1M context) --- .../core/control/ControlConnection.java | 405 ++++++++++++------ .../core/control/ControlConnectionTest.java | 300 ++++++++++++- .../control/ControlConnectionTestBase.java | 9 +- .../core/metadata/TestNodeFactory.java | 19 + 4 files changed, 605 insertions(+), 128 deletions(-) diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/control/ControlConnection.java b/core/src/main/java/com/datastax/oss/driver/internal/core/control/ControlConnection.java index 9da3a2a8aa2..97a2137ca71 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/control/ControlConnection.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/control/ControlConnection.java @@ -40,11 +40,13 @@ import com.datastax.oss.driver.internal.core.metadata.MetadataManager; import com.datastax.oss.driver.internal.core.metadata.NodeStateEvent; import com.datastax.oss.driver.internal.core.metadata.TopologyEvent; +import com.datastax.oss.driver.internal.core.util.AddressUtils; import com.datastax.oss.driver.internal.core.util.Loggers; import com.datastax.oss.driver.internal.core.util.concurrent.CompletableFutures; import com.datastax.oss.driver.internal.core.util.concurrent.Reconnection; import com.datastax.oss.driver.internal.core.util.concurrent.RunOrSchedule; import com.datastax.oss.driver.internal.core.util.concurrent.UncaughtExceptions; +import com.datastax.oss.driver.shaded.guava.common.annotations.VisibleForTesting; import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableList; import com.datastax.oss.protocol.internal.Message; import com.datastax.oss.protocol.internal.ProtocolConstants; @@ -54,15 +56,20 @@ import com.datastax.oss.protocol.internal.response.event.StatusChangeEvent; import com.datastax.oss.protocol.internal.response.event.TopologyChangeEvent; import edu.umd.cs.findbugs.annotations.NonNull; +import edu.umd.cs.findbugs.annotations.Nullable; import io.netty.util.concurrent.EventExecutor; +import java.net.InetSocketAddress; +import java.net.SocketAddress; import java.util.AbstractMap.SimpleEntry; import java.util.ArrayList; import java.util.Collection; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Objects; import java.util.Queue; +import java.util.Set; import java.util.WeakHashMap; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; @@ -90,6 +97,18 @@ public class ControlConnection implements EventCallback, AsyncAutoCloseable { private static final Logger LOG = LoggerFactory.getLogger(ControlConnection.class); + /** + * How many addresses of one query plan entry are recorded as tried when reading the control + * node's identity keeps failing. The cap is tested before the address is recorded, so the entry + * is attempted at most {@code MAX_ADDRESSES_PER_QUERY_PLAN_ENTRY + 1} times before the query plan + * advances. + * + *

A backstop only -- see {@code SingleThreaded#retryOrAdvance}, which stops on its own as soon + * as an address comes back round. It also bounds how long one hostname can hold up + * initialization, since every extra attempt costs a connect plus a {@code system.local} read. + */ + @VisibleForTesting static final int MAX_ADDRESSES_PER_QUERY_PLAN_ENTRY = 8; + private final InternalDriverContext context; private final String logPrefix; private final EventExecutor adminExecutor; @@ -392,141 +411,277 @@ private void connect( if (node == null) { onFailure.accept(AllNodesFailedException.fromErrors(errors)); } else { - LOG.debug("[{}] Trying to establish a connection to {}", logPrefix, node); - context - .getChannelFactory() - .connect(node, channelOptions) - .whenCompleteAsync( - (channel, error) -> { - try { - NodeDistance lastDistance = lastNodeDistance.get(node); - NodeState lastState = lastNodeState.get(node); - if (error != null) { - if (closeWasCalled || initFuture.isCancelled()) { - onSuccess.run(); // abort, we don't really care about the result + // A fresh set per query plan entry: it tracks the addresses of *this* node's endpoint, and + // it is only ever read and written on adminExecutor, so a plain HashSet is enough. + attempt(node, new HashSet<>(), nodes, errors, onSuccess, onFailure); + } + } + + /** + * One attempt at one entry of the query plan. Called again for the same entry -- with {@code + * triedAddresses} carried over -- when the control node's identity could not be read over the + * channel that was just opened; see {@link #retryOrAdvance}. + */ + private void attempt( + Node node, + Set triedAddresses, + Queue nodes, + List> errors, + Runnable onSuccess, + Consumer onFailure) { + assert adminExecutor.inEventLoop(); + LOG.debug("[{}] Trying to establish a connection to {}", logPrefix, node); + context + .getChannelFactory() + .connect(node, channelOptions) + .whenCompleteAsync( + (channel, error) -> { + try { + NodeDistance lastDistance = lastNodeDistance.get(node); + NodeState lastState = lastNodeState.get(node); + if (error != null) { + if (closeWasCalled || initFuture.isCancelled()) { + onSuccess.run(); // abort, we don't really care about the result + } else { + if (error instanceof AuthenticationException) { + Loggers.warnWithException( + LOG, "[{}] Authentication error", logPrefix, error); } else { - if (error instanceof AuthenticationException) { + if (config + .getDefaultProfile() + .getBoolean(DefaultDriverOption.CONNECTION_WARN_INIT_ERROR)) { Loggers.warnWithException( - LOG, "[{}] Authentication error", logPrefix, error); + LOG, + "[{}] Error connecting to {}, trying next node", + logPrefix, + node, + error); } else { - if (config - .getDefaultProfile() - .getBoolean(DefaultDriverOption.CONNECTION_WARN_INIT_ERROR)) { - Loggers.warnWithException( - LOG, - "[{}] Error connecting to {}, trying next node", - logPrefix, - node, - error); - } else { - LOG.debug( - "[{}] Error connecting to {}, trying next node", - logPrefix, - node, - error); - } + LOG.debug( + "[{}] Error connecting to {}, trying next node", + logPrefix, + node, + error); } - List> newErrors = - (errors == null) ? new ArrayList<>() : errors; - newErrors.add(new SimpleEntry<>(node, error)); - context.getEventBus().fire(ChannelEvent.controlConnectionFailed(node)); - connect(nodes, newErrors, onSuccess, onFailure); } - } else if (closeWasCalled || initFuture.isCancelled()) { - LOG.debug( - "[{}] New channel opened ({}) but the control connection was closed, closing it", - logPrefix, - channel); - channel.forceClose(); - onSuccess.run(); - } else if (lastDistance == NodeDistance.IGNORED) { - LOG.debug( - "[{}] New channel opened ({}) but node became ignored, " - + "closing and trying next node", - logPrefix, - channel); - channel.forceClose(); - connect(nodes, errors, onSuccess, onFailure); - } else if (lastNodeState.containsKey(node) - && (lastState == null /*(removed)*/ - || lastState == NodeState.FORCED_DOWN)) { + List> newErrors = + (errors == null) ? new ArrayList<>() : errors; + newErrors.add(new SimpleEntry<>(node, error)); + context.getEventBus().fire(ChannelEvent.controlConnectionFailed(node)); + connect(nodes, newErrors, onSuccess, onFailure); + } + } else if (closeWasCalled || initFuture.isCancelled()) { + LOG.debug( + "[{}] New channel opened ({}) but the control connection was closed, closing it", + logPrefix, + channel); + channel.forceClose(); + onSuccess.run(); + } else if (lastDistance == NodeDistance.IGNORED) { + LOG.debug( + "[{}] New channel opened ({}) but node became ignored, " + + "closing and trying next node", + logPrefix, + channel); + channel.forceClose(); + connect(nodes, errors, onSuccess, onFailure); + } else if (lastNodeState.containsKey(node) + && (lastState == null /*(removed)*/ || lastState == NodeState.FORCED_DOWN)) { + LOG.debug( + "[{}] New channel opened ({}) but node was removed or forced down, " + + "closing and trying next node", + logPrefix, + channel); + channel.forceClose(); + connect(nodes, errors, onSuccess, onFailure); + } else { + LOG.debug("[{}] New channel opened {}", logPrefix, channel); + // Captured now, not in the failure handlers below: on its way to registering + // the node, resolveChannelNodeIfNeeded() replaces the channel's endpoint with + // the one built from the system.local row, and it does so before the step + // that can still fail. + SocketAddress triedAddress = pinnedAddressOf(channel); + DriverChannel previousChannel = ControlConnection.this.channel; + ControlConnection.this.channel = channel; + controlNodeState = new ControlNodeState(null, node); + if (previousChannel != null && previousChannel != channel) { LOG.debug( - "[{}] New channel opened ({}) but node was removed or forced down, " - + "closing and trying next node", + "[{}] Forcefully closing previous channel {}", logPrefix, - channel); - channel.forceClose(); - connect(nodes, errors, onSuccess, onFailure); - } else { - LOG.debug("[{}] New channel opened {}", logPrefix, channel); - DriverChannel previousChannel = ControlConnection.this.channel; - ControlConnection.this.channel = channel; - controlNodeState = new ControlNodeState(null, node); - if (previousChannel != null && previousChannel != channel) { - LOG.debug( - "[{}] Forcefully closing previous channel {}", - logPrefix, - previousChannel); - previousChannel.forceClose(); - } - resolveChannelNodeIfNeeded(channel, (DefaultNode) node) - .whenCompleteAsync( - (resolvedNode, fetchError) -> { - if (fetchError != null) { - controlNodeState = ControlNodeState.NONE; - LOG.debug( - "[{}] Failed to resolve control node endpoint from {}, " - + "trying next node", - logPrefix, - node, - fetchError); - // Null out before forceClose() so that onChannelClosed() does not - // start a redundant reconnection on top of the connect() retry - // below. - ControlConnection.this.channel = null; - channel.forceClose(); - List> newErrors = - (errors == null) ? new ArrayList<>() : errors; - newErrors.add(new SimpleEntry<>(node, fetchError)); - connect(nodes, newErrors, onSuccess, onFailure); - } else if (channel.closeFuture().isDone()) { - controlNodeState = ControlNodeState.NONE; - ControlConnection.this.channel = null; - List> newErrors = - (errors == null) ? new ArrayList<>() : errors; - newErrors.add( - new SimpleEntry<>( - node, - new Exception("Channel closed during endpoint resolve"))); - connect(nodes, newErrors, onSuccess, onFailure); - } else { - controlNodeState = new ControlNodeState(resolvedNode, null); - context - .getEventBus() - .fire(ChannelEvent.channelOpened(resolvedNode)); - channel - .closeFuture() - .addListener( - f -> - adminExecutor - .submit( - () -> onChannelClosed(channel, resolvedNode)) - .addListener(UncaughtExceptions::log)); - onSuccess.run(); - } - }, - adminExecutor); + previousChannel); + previousChannel.forceClose(); } - } catch (Exception e) { - Loggers.warnWithException( - LOG, - "[{}] Unexpected exception while processing channel init result", - logPrefix, - e); + resolveChannelNodeIfNeeded(channel, (DefaultNode) node) + .whenCompleteAsync( + (resolvedNode, fetchError) -> { + if (fetchError != null) { + controlNodeState = ControlNodeState.NONE; + // Null out before forceClose() so that onChannelClosed() does not + // start a redundant reconnection on top of the retry below. + ControlConnection.this.channel = null; + channel.forceClose(); + List> newErrors = + (errors == null) ? new ArrayList<>() : errors; + newErrors.add(new SimpleEntry<>(node, fetchError)); + retryOrAdvance( + node, + triedAddress, + triedAddresses, + nodes, + newErrors, + onSuccess, + onFailure, + fetchError); + } else if (channel.closeFuture().isDone()) { + controlNodeState = ControlNodeState.NONE; + ControlConnection.this.channel = null; + Throwable closedError = + new Exception("Channel closed during endpoint resolve"); + List> newErrors = + (errors == null) ? new ArrayList<>() : errors; + newErrors.add(new SimpleEntry<>(node, closedError)); + retryOrAdvance( + node, + triedAddress, + triedAddresses, + nodes, + newErrors, + onSuccess, + onFailure, + closedError); + } else { + controlNodeState = new ControlNodeState(resolvedNode, null); + context + .getEventBus() + .fire(ChannelEvent.channelOpened(resolvedNode)); + channel + .closeFuture() + .addListener( + f -> + adminExecutor + .submit( + () -> onChannelClosed(channel, resolvedNode)) + .addListener(UncaughtExceptions::log)); + onSuccess.run(); + } + }, + adminExecutor); } - }, - adminExecutor); + } catch (Exception e) { + Loggers.warnWithException( + LOG, + "[{}] Unexpected exception while processing channel init result", + logPrefix, + e); + } + }, + adminExecutor); + } + + /** + * Decides what to do when a channel opened fine but the control node's identity could not be + * read over it. + * + *

A contact point is a single {@link Node} even when its hostname expands to several + * addresses, and {@code ChannelFactory} only walks those addresses while it is opening the + * channel: by the time the identity query runs, the remaining ones are gone. Advancing the + * query plan straight away would therefore write off the whole hostname on the strength of one + * of its addresses -- and the addresses of an unidentified endpoint may well belong to + * different nodes, which is the same reason {@code ChannelFactory#isNodeWideFailure} only + * treats a protocol-version rejection as terminal for a node whose host id is already known. So + * the entry is attempted again instead, and the next attempt lands on another address because + * {@code ChannelFactory} rotates its candidates once per connect. + * + *

This terminates: {@code triedAddresses} only grows, and a retry requires it to grow, so + * coming back round to an address already in it ends the walk -- after at most one pass over + * the records for any resolver that answers a name consistently. A rotation counter nudged + * along by concurrent pool connects can only make the walk end sooner, by skipping an address + * (a later reconnection round picks it up). {@link #MAX_ADDRESSES_PER_QUERY_PLAN_ENTRY} is a + * backstop against a resolver that never repeats itself. + * + *

Not covered, all deliberately: nodes whose host id is already known (they are pinned to + * one address on purpose); endpoints that are not {@link + * com.datastax.oss.driver.internal.core.metadata.PinnableEndPoint}s, since there is no way to + * tell which address answered; endpoints that denote a literal address rather than a name, and + * so expand to exactly themselves; and exhaustiveness -- the walk is best-effort, not a + * guarantee that every address is tried exactly once. + */ + private void retryOrAdvance( + Node node, + @Nullable SocketAddress triedAddress, + Set triedAddresses, + Queue nodes, + List> errors, + Runnable onSuccess, + Consumer onFailure, + Throwable cause) { + assert adminExecutor.inEventLoop(); + if (node.getHostId() == null + && triedAddress != null + && triedAddresses.size() < MAX_ADDRESSES_PER_QUERY_PLAN_ENTRY + && expandsToSeveralAddresses(node.getEndPoint()) + && triedAddresses.add(triedAddress)) { + LOG.debug( + "[{}] Failed to resolve control node endpoint from {} at {}, " + + "trying another of its addresses", + logPrefix, + node, + triedAddress, + cause); + attempt(node, triedAddresses, nodes, errors, onSuccess, onFailure); + } else { + LOG.debug( + "[{}] Failed to resolve control node endpoint from {} at {}, trying next node", + logPrefix, + node, + triedAddress, + cause); + connect(nodes, errors, onSuccess, onFailure); + } + } + + /** + * The address the channel actually landed on, or {@code null} if that cannot be told -- which + * is the case for a third-party {@link com.datastax.oss.driver.api.core.metadata.EndPoint} that + * {@code ChannelFactory} passed through without pinning. + */ + @Nullable + private SocketAddress pinnedAddressOf(DriverChannel channel) { + InetSocketAddress address = resolveQuietly(channel.getEndPoint()); + return (address != null && !address.isUnresolved()) ? address : null; + } + + /** + * Whether the endpoint denotes a host name, i.e. something {@code ChannelFactory} may + * expand to more than one address. A literal expands to exactly itself, so there would be + * nothing to retry on. + */ + private boolean expandsToSeveralAddresses(EndPoint endPoint) { + InetSocketAddress address = resolveQuietly(endPoint); + return address != null && AddressUtils.carriesName(address); + } + + /** + * What {@code endPoint} resolves to, or {@code null} if there is no endpoint, if it resolves to + * something other than an {@link InetSocketAddress}, or if resolving threw. + * + *

For the endpoints this class asks about, {@code resolve()} is contractually a field read + * on a pinned copy. But it is an extension point and this runs on the admin executor, so a + * misbehaving implementation must not take that executor down. + */ + @Nullable + private InetSocketAddress resolveQuietly(@Nullable EndPoint endPoint) { + if (endPoint == null) { + return null; + } + SocketAddress address; + try { + address = endPoint.resolve(); + } catch (RuntimeException e) { + LOG.debug("[{}] Error resolving {}", logPrefix, endPoint, e); + return null; } + return (address instanceof InetSocketAddress) ? (InetSocketAddress) address : null; } /** diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/control/ControlConnectionTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/control/ControlConnectionTest.java index d51ee445da5..56cd1b340b2 100644 --- a/core/src/test/java/com/datastax/oss/driver/internal/core/control/ControlConnectionTest.java +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/control/ControlConnectionTest.java @@ -21,30 +21,43 @@ import static com.datastax.oss.driver.Assertions.assertThatStage; import static org.awaitility.Awaitility.await; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.never; import static org.mockito.Mockito.timeout; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import com.datastax.oss.driver.api.core.AllNodesFailedException; import com.datastax.oss.driver.api.core.loadbalancing.NodeDistance; +import com.datastax.oss.driver.api.core.metadata.EndPoint; import com.datastax.oss.driver.api.core.metadata.Node; import com.datastax.oss.driver.api.core.metadata.NodeState; import com.datastax.oss.driver.internal.core.channel.ChannelEvent; import com.datastax.oss.driver.internal.core.channel.DriverChannel; +import com.datastax.oss.driver.internal.core.channel.DriverChannelOptions; import com.datastax.oss.driver.internal.core.channel.MockChannelFactoryHelper; import com.datastax.oss.driver.internal.core.metadata.DefaultNode; import com.datastax.oss.driver.internal.core.metadata.DefaultNodeInfo; import com.datastax.oss.driver.internal.core.metadata.DistanceEvent; import com.datastax.oss.driver.internal.core.metadata.NodeInfo; import com.datastax.oss.driver.internal.core.metadata.NodeStateEvent; +import com.datastax.oss.driver.internal.core.metadata.PinnableEndPoint; import com.datastax.oss.driver.internal.core.metadata.TestNodeFactory; import com.datastax.oss.driver.internal.core.metadata.TopologyMonitor; +import com.datastax.oss.driver.internal.core.util.concurrent.CompletableFutures; import com.tngtech.java.junit.dataprovider.DataProviderRunner; +import java.net.InetSocketAddress; import java.time.Duration; +import java.util.List; +import java.util.Map; import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; @@ -52,6 +65,12 @@ @RunWith(DataProviderRunner.class) public class ControlConnectionTest extends ControlConnectionTestBase { + /** + * A contact point hostname standing in for one with several A records. What matters is only that + * it is a name and not a literal: that is what tells the driver the endpoint may expand. + */ + private static final String MULTI_ADDRESS_NAME = "multi.example.com"; + @Test public void should_close_successfully_if_it_was_never_init() { // When @@ -540,7 +559,9 @@ public void should_close_channel_if_closed_during_reconnection() { @Test public void should_try_next_node_if_resolve_endpoint_fails() { // Given — use a contact point (no hostId) so resolveChannelNodeIfNeeded - // actually calls getChannelNodeInfo instead of short-circuiting + // actually calls getChannelNodeInfo instead of short-circuiting. Its endpoint is an IP + // literal, which expands to exactly itself, so the same-node address walk exercised below + // deliberately does not arm here and the query plan advances on the first failure. node1 = TestNodeFactory.newContactPoint(1, context); mockQueryPlan(node1, node2); @@ -572,10 +593,287 @@ public void should_try_next_node_if_resolve_endpoint_fails() { // channel1 should be force-closed by the resolve failure handler (previousChannel is null // at that point, so channel2's success does not close channel1 a second time) verify(channel1, timeout(500)).forceClose(); + // Exactly one attempt at node1: an IP literal has no second address to fall back on. + verify(channelFactory, times(1)).connect(eq(node1), any(DriverChannelOptions.class)); factoryHelper.verifyNoMoreCalls(); } + @Test + public void should_retry_contact_point_on_another_address_when_resolve_endpoint_fails() { + // Given — one contact point whose hostname expands to several addresses. ChannelFactory walks + // those addresses while it opens the channel, but the control node's identity is only read + // afterwards, over the channel that won; by then the other addresses are gone. Advancing the + // query plan there would write off the whole hostname on the strength of one of its addresses, + // and with a single contact point that means failing initialization outright. + DefaultNode contactPoint = TestNodeFactory.newContactPoint(MULTI_ADDRESS_NAME, context); + mockQueryPlan(contactPoint); + + DriverChannel onFirst = newMockDriverChannel(1, pinnedTo(1)); + DriverChannel onSecond = newMockDriverChannel(2, pinnedTo(2)); + MockChannelFactoryHelper factoryHelper = + MockChannelFactoryHelper.builder(channelFactory) + .success(contactPoint, onFirst) + .success(contactPoint, onSecond) + .build(); + + // The first address answers, but is still bootstrapping -- the real cause raised by + // DefaultTopologyMonitor when system.local has no host_id yet. + failResolve( + onFirst, new NullPointerException("host_id is null in system.local, node may still be...")); + + // When + CompletionStage initFuture = controlConnection.init(false, false, false); + factoryHelper.waitForCalls(contactPoint, 2); + + // Then — the second address of the same contact point carries the control connection. + assertThatStage(initFuture) + .isSuccess(v -> assertThat(controlConnection.channel()).isEqualTo(onSecond)); + verify(onFirst, VERIFY_TIMEOUT).forceClose(); + verify(eventBus, never()).fire(ChannelEvent.channelOpened(contactPoint)); + + factoryHelper.verifyNoMoreCalls(); + } + + @Test + public void should_retry_contact_point_on_another_address_when_channel_closes_during_resolve() { + // Given — same as above, but the channel dies while the identity query is in flight. + DefaultNode contactPoint = TestNodeFactory.newContactPoint(MULTI_ADDRESS_NAME, context); + mockQueryPlan(contactPoint); + + DriverChannel onFirst = newMockDriverChannel(1, pinnedTo(1)); + DriverChannel onSecond = newMockDriverChannel(2, pinnedTo(2)); + MockChannelFactoryHelper factoryHelper = + MockChannelFactoryHelper.builder(channelFactory) + .success(contactPoint, onFirst) + .success(contactPoint, onSecond) + .build(); + + CompletableFuture pendingResolve = new CompletableFuture<>(); + when(context.getTopologyMonitor().getChannelNodeInfo(onFirst)).thenReturn(pendingResolve); + + // When + CompletionStage initFuture = controlConnection.init(false, false, false); + factoryHelper.waitForCall(contactPoint); + onFirst.close(); + pendingResolve.complete( + DefaultNodeInfo.builder() + .withEndPoint(onFirst.getEndPoint()) + .withHostId(UUID.randomUUID()) + .build()); + + // Then + factoryHelper.waitForCall(contactPoint); + assertThatStage(initFuture) + .isSuccess(v -> assertThat(controlConnection.channel()).isEqualTo(onSecond)); + + factoryHelper.verifyNoMoreCalls(); + } + + @Test + public void should_stop_retrying_contact_point_when_the_same_address_comes_back() { + // Given — both attempts land on the same address, which is how the walk detects that it has + // been all the way round the records. It must then advance rather than loop. + DefaultNode contactPoint = TestNodeFactory.newContactPoint(MULTI_ADDRESS_NAME, context); + mockQueryPlan(contactPoint, node2); + + DriverChannel onFirst = newMockDriverChannel(1, pinnedTo(1)); + DriverChannel onFirstAgain = newMockDriverChannel(3, pinnedTo(1)); + DriverChannel onNode2 = newMockDriverChannel(2); + MockChannelFactoryHelper factoryHelper = + MockChannelFactoryHelper.builder(channelFactory) + .success(contactPoint, onFirst) + .success(contactPoint, onFirstAgain) + .success(node2, onNode2) + .build(); + + failResolve(onFirst, new RuntimeException("mock resolve failure")); + failResolve(onFirstAgain, new RuntimeException("mock resolve failure")); + + // When + CompletionStage initFuture = controlConnection.init(false, false, false); + factoryHelper.waitForCalls(contactPoint, 2); + factoryHelper.waitForCall(node2); + + // Then + assertThatStage(initFuture) + .isSuccess(v -> assertThat(controlConnection.channel()).isEqualTo(onNode2)); + verify(channelFactory, times(2)).connect(eq(contactPoint), any(DriverChannelOptions.class)); + + factoryHelper.verifyNoMoreCalls(); + } + + @Test + public void should_stop_walking_a_contact_point_at_the_address_cap() { + // Given — a resolver that never repeats an address would keep the walk going for as long as it + // kept inventing them, since the walk's own stop condition is an address coming back round. + // MAX_ADDRESSES_PER_QUERY_PLAN_ENTRY is the backstop for that. It is tested *before* the + // address is recorded, so the entry is attempted one more time than the cap itself. + DefaultNode contactPoint = TestNodeFactory.newContactPoint(MULTI_ADDRESS_NAME, context); + mockQueryPlan(contactPoint, node2); + + int expectedAttempts = ControlConnection.MAX_ADDRESSES_PER_QUERY_PLAN_ENTRY + 1; + MockChannelFactoryHelper.Builder builder = MockChannelFactoryHelper.builder(channelFactory); + for (int i = 0; i < expectedAttempts; i++) { + // A distinct address every time, so triedAddresses only grows and nothing but the cap can + // end the walk. + DriverChannel channel = newMockDriverChannel(10 + i, pinnedTo(10 + i)); + failResolve(channel, new RuntimeException("mock resolve failure " + i)); + builder.success(contactPoint, channel); + } + DriverChannel onNode2 = newMockDriverChannel(2); + MockChannelFactoryHelper factoryHelper = builder.success(node2, onNode2).build(); + + // When + CompletionStage initFuture = controlConnection.init(false, false, false); + factoryHelper.waitForCalls(contactPoint, expectedAttempts); + factoryHelper.waitForCall(node2); + + // Then — the walk gave up at the cap and the query plan advanced. + assertThatStage(initFuture) + .isSuccess(v -> assertThat(controlConnection.channel()).isEqualTo(onNode2)); + verify(channelFactory, times(expectedAttempts)) + .connect(eq(contactPoint), any(DriverChannelOptions.class)); + + factoryHelper.verifyNoMoreCalls(); + } + + @Test + public void should_report_every_failed_address_of_a_contact_point() { + // Given — the only entry in the query plan, with every address failing identity resolution. + DefaultNode contactPoint = TestNodeFactory.newContactPoint(MULTI_ADDRESS_NAME, context); + mockQueryPlan(contactPoint); + + DriverChannel onFirst = newMockDriverChannel(1, pinnedTo(1)); + DriverChannel onSecond = newMockDriverChannel(2, pinnedTo(2)); + MockChannelFactoryHelper factoryHelper = + MockChannelFactoryHelper.builder(channelFactory) + .success(contactPoint, onFirst) + .success(contactPoint, onSecond) + .build(); + + failResolve(onFirst, new RuntimeException("first address failed")); + failResolve(onSecond, new RuntimeException("second address failed")); + + // When — the third attempt gets onSecond again (Mockito repeats the last stub), so the address + // repeats, the walk ends, the queue is empty and init fails. + CompletionStage initFuture = controlConnection.init(false, false, false); + factoryHelper.waitForCalls(contactPoint, 3); + + // Then — every address that was tried is reported, grouped under the one node. + assertThatStage(initFuture) + .isFailed( + error -> { + assertThat(error).isInstanceOf(AllNodesFailedException.class); + Map> allErrors = + ((AllNodesFailedException) error).getAllErrors(); + assertThat(allErrors).hasSize(1).containsKey(contactPoint); + assertThat(allErrors.get(contactPoint)).hasSize(3); + }); + + factoryHelper.verifyNoMoreCalls(); + } + + @Test + public void should_not_retry_another_address_when_the_node_is_already_identified() { + // Given — a node with a known host id is deliberately pinned to one address: every address of + // an identified node is that same node, and reconnecting elsewhere would break its identity. + // resolveChannelNodeIfNeeded short-circuits for it, so this really only asserts that the walk + // stays off the path that identified nodes take. + DefaultNode identified = TestNodeFactory.newNode(MULTI_ADDRESS_NAME, context); + mockQueryPlan(identified, node2); + + DriverChannel onFirst = newMockDriverChannel(1, pinnedTo(1)); + DriverChannel onNode2 = newMockDriverChannel(2); + MockChannelFactoryHelper factoryHelper = + MockChannelFactoryHelper.builder(channelFactory) + .success(identified, onFirst) + .success(node2, onNode2) + .build(); + + // When + CompletionStage initFuture = controlConnection.init(false, false, false); + factoryHelper.waitForCall(identified); + + // Then + assertThatStage(initFuture) + .isSuccess(v -> assertThat(controlConnection.channel()).isEqualTo(onFirst)); + verify(channelFactory, times(1)).connect(eq(identified), any(DriverChannelOptions.class)); + + factoryHelper.verifyNoMoreCalls(); + } + + @Test + public void should_capture_the_tried_address_before_the_channel_endpoint_is_upgraded() { + // Given — resolveChannelNodeIfNeeded() overwrites the channel's endpoint with the one built + // from the system.local row on its way to registering the node, and it does so *before* the + // registration that can still fail. Read the landed address after that and it is the + // unresolved hostname, which tells us nothing about which candidate answered -- so the walk + // would give up. This drives exactly that sequence. + DefaultNode contactPoint = TestNodeFactory.newContactPoint(MULTI_ADDRESS_NAME, context); + mockQueryPlan(contactPoint); + + DriverChannel onFirst = newMockDriverChannel(1, pinnedTo(1)); + DriverChannel onSecond = newMockDriverChannel(2, pinnedTo(2)); + MockChannelFactoryHelper factoryHelper = + MockChannelFactoryHelper.builder(channelFactory) + .success(contactPoint, onFirst) + .success(contactPoint, onSecond) + .build(); + + AtomicReference firstEndPoint = new AtomicReference<>(onFirst.getEndPoint()); + when(onFirst.getEndPoint()).thenAnswer(i -> firstEndPoint.get()); + doAnswer( + i -> { + firstEndPoint.set(i.getArgument(0)); + return null; + }) + .when(onFirst) + .setEndPoint(any(EndPoint.class)); + // The identity query succeeds, carrying the *unpinned* name back... + when(context.getTopologyMonitor().getChannelNodeInfo(onFirst)) + .thenReturn( + CompletableFuture.completedFuture( + DefaultNodeInfo.builder() + .withEndPoint(contactPoint.getEndPoint()) + .withHostId(UUID.randomUUID()) + .build())); + // ... and registration is what fails, after the endpoint has already been swapped. Only the + // first one: the point is that the walk gets a second attempt at all. + AtomicBoolean firstRegistration = new AtomicBoolean(true); + when(metadataManager.registerNode(any(NodeInfo.class))) + .thenAnswer( + i -> { + if (firstRegistration.getAndSet(false)) { + return CompletableFutures.failedFuture( + new RuntimeException("mock register failure")); + } + return CompletableFuture.completedFuture( + TestNodeFactory.newNode((NodeInfo) i.getArgument(0), context)); + }); + + // When + CompletionStage initFuture = controlConnection.init(false, false, false); + factoryHelper.waitForCalls(contactPoint, 2); + + // Then + assertThatStage(initFuture) + .isSuccess(v -> assertThat(controlConnection.channel()).isEqualTo(onSecond)); + + factoryHelper.verifyNoMoreCalls(); + } + + /** An endpoint pinned to one of {@link #MULTI_ADDRESS_NAME}'s addresses, as ChannelFactory. */ + private EndPoint pinnedTo(int lastIpByte) { + return ((PinnableEndPoint) TestNodeFactory.newEndPoint(MULTI_ADDRESS_NAME)) + .pinTo(new InetSocketAddress("127.0.0." + lastIpByte, 9042)); + } + + private void failResolve(DriverChannel channel, Throwable error) { + when(context.getTopologyMonitor().getChannelNodeInfo(channel)) + .thenReturn(CompletableFutures.failedFuture(error)); + } + @Test public void should_try_next_node_if_channel_closes_during_init_resolve() { // Given — use a contact point (no hostId) so resolveChannelNodeIfNeeded is async diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/control/ControlConnectionTestBase.java b/core/src/test/java/com/datastax/oss/driver/internal/core/control/ControlConnectionTestBase.java index ccf7d18e086..df4768163e2 100644 --- a/core/src/test/java/com/datastax/oss/driver/internal/core/control/ControlConnectionTestBase.java +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/control/ControlConnectionTestBase.java @@ -28,6 +28,7 @@ import com.datastax.oss.driver.api.core.config.DriverConfig; import com.datastax.oss.driver.api.core.config.DriverExecutionProfile; import com.datastax.oss.driver.api.core.connection.ReconnectionPolicy; +import com.datastax.oss.driver.api.core.metadata.EndPoint; import com.datastax.oss.driver.api.core.metadata.Metadata; import com.datastax.oss.driver.api.core.metadata.Node; import com.datastax.oss.driver.internal.core.channel.ChannelFactory; @@ -203,6 +204,11 @@ public void teardown() { } protected DriverChannel newMockDriverChannel(int id) { + return newMockDriverChannel( + id, new DefaultEndPoint(new InetSocketAddress("127.0.0." + id, 9042))); + } + + protected DriverChannel newMockDriverChannel(int id, EndPoint endPoint) { DriverChannel driverChannel = mock(DriverChannel.class); Channel channel = mock(Channel.class); EventLoop adminExecutor = adminEventLoopGroup.next(); @@ -221,8 +227,7 @@ protected DriverChannel newMockDriverChannel(int id) { }); when(driverChannel.closeFuture()).thenReturn(closeFuture); when(driverChannel.toString()).thenReturn("channel" + id); - when(driverChannel.getEndPoint()) - .thenReturn(new DefaultEndPoint(new InetSocketAddress("127.0.0." + id, 9042))); + when(driverChannel.getEndPoint()).thenReturn(endPoint); return driverChannel; } } diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/TestNodeFactory.java b/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/TestNodeFactory.java index bc35d10699b..cbbf4df0195 100644 --- a/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/TestNodeFactory.java +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/TestNodeFactory.java @@ -46,12 +46,31 @@ public static DefaultNode newNode(NodeInfo nodeInfo, InternalDriverContext conte return node; } + public static DefaultNode newNode(String hostName, InternalDriverContext context) { + DefaultNode node = new DefaultNode(newEndPoint(hostName), context); + node.hostId = UUID.randomUUID(); + return node; + } + public static DefaultNode newContactPoint(int lastIpByte, InternalDriverContext context) { DefaultEndPoint endPoint = newEndPoint(lastIpByte); return DefaultNode.newContactPoint(endPoint, context); } + public static DefaultNode newContactPoint(String hostName, InternalDriverContext context) { + return DefaultNode.newContactPoint(newEndPoint(hostName), context); + } + public static DefaultEndPoint newEndPoint(int lastByteOfIp) { return new DefaultEndPoint(new InetSocketAddress("127.0.0." + lastByteOfIp, 9042)); } + + /** + * A endpoint over a host name, left unresolved the way {@code SessionBuilder} stores a + * config contact point. Unlike the IP-literal variants above, this is something the connection + * layer may expand to several addresses. + */ + public static DefaultEndPoint newEndPoint(String hostName) { + return new DefaultEndPoint(InetSocketAddress.createUnresolved(hostName, 9042)); + } } From 4bced3d35dd624c48447818c97b47009d901a348 Mon Sep 17 00:00:00 2001 From: Mikita Hradovich Date: Thu, 6 Aug 2026 18:16:56 +0200 Subject: [PATCH 7/9] feat: fall back to the original contact points on control reconnection (DRIVER-201) advanced.control-connection.reconnection.fallback-to-original-contact-points now defaults to true, and is the driver's DNS re-resolution path. Nothing else re-resolves. Metadata nodes hold an endpoint built from an already-resolved system.peers IP, and the control node's own endpoint is pinned by ChannelFactory to the single address its connection reached, deliberately, so that a node with a known identity cannot wander to a different host. Once the records behind a hostname change, appending the original contact points is therefore the only way back: they are still unresolved hostnames, so ChannelFactory expands each one to its current IPs at connection time. The append is gated on the topology monitor not re-resolving addresses itself, since a proxy-based monitor keeps them fresh and raw contact points could resurrect nodes it has authoritatively removed. The exception is an empty regular plan: with no live node to try, reconnection cannot recover on its own. The plans are concatenated rather than mutated. A RUNNING-state query plan is a built-in QueryPlan whose add()/addAll() throw UnsupportedOperationException, poll() being its only mutator, so with the fallback defaulting on every post-init control reconnect would otherwise have crashed. The append is also skipped before the LBP reaches RUNNING, where newQueryPlan() has already built the plan from the contact points and appending would duplicate every entry. Documented cost: the contact points are appended without being compared against the live-node plan, because at plan time they are hostnames while the live nodes are resolved IPs. When DNS has not changed they expand to addresses the plan just failed on, so an exhausted reconnection round retries roughly twice as many addresses -- which is why HeartbeatIT has to disable it. Co-Authored-By: Claude Opus 5 (1M context) --- .../api/core/config/DefaultDriverOption.java | 10 +- .../driver/api/core/config/OptionsMap.java | 2 +- .../api/core/config/TypedDriverOption.java | 10 +- .../metadata/LoadBalancingPolicyWrapper.java | 52 +++++++-- .../core/metadata/MetadataManager.java | 6 + core/src/main/resources/reference.conf | 21 +++- .../LoadBalancingPolicyWrapperTest.java | 108 ++++++++++++++++-- .../driver/core/heartbeat/HeartbeatIT.java | 4 + 8 files changed, 184 insertions(+), 29 deletions(-) diff --git a/core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java b/core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java index c2d723a00e7..887d346f09a 100644 --- a/core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java +++ b/core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java @@ -701,8 +701,14 @@ public enum DefaultDriverOption implements DriverOption { CONTROL_CONNECTION_AGREEMENT_WARN("advanced.control-connection.schema-agreement.warn-on-failure"), /** - * Whether to forcibly add original contact points held by MetadataManager to the reconnection - * plan, in case there is no live nodes available according to LBP. Experimental. + * Whether to append the original contact points held by MetadataManager to the reconnection plan, + * after the live nodes reported by the load balancing policy. Defaults to {@code true}. + * + *

This is also the driver's DNS re-resolution path. Contact points are appended as-is, still + * unresolved hostnames, and each is expanded to its current DNS IPs at connection time through + * Netty's configured resolver. Metadata nodes, in contrast, hold an already-resolved endpoint + * that is never re-resolved. Keeping this enabled lets control-connection reconnects re-resolve + * the original hostnames and pick up new IPs once the live-node plan is exhausted. * *

Value-type: boolean */ diff --git a/core/src/main/java/com/datastax/oss/driver/api/core/config/OptionsMap.java b/core/src/main/java/com/datastax/oss/driver/api/core/config/OptionsMap.java index 6e608e8b858..18d93f97625 100644 --- a/core/src/main/java/com/datastax/oss/driver/api/core/config/OptionsMap.java +++ b/core/src/main/java/com/datastax/oss/driver/api/core/config/OptionsMap.java @@ -372,7 +372,7 @@ protected static void fillWithDriverDefaults(OptionsMap map) { map.put(TypedDriverOption.CONTROL_CONNECTION_AGREEMENT_INTERVAL, Duration.ofMillis(200)); map.put(TypedDriverOption.CONTROL_CONNECTION_AGREEMENT_TIMEOUT, Duration.ofSeconds(10)); map.put(TypedDriverOption.CONTROL_CONNECTION_AGREEMENT_WARN, true); - map.put(TypedDriverOption.CONTROL_CONNECTION_RECONNECT_CONTACT_POINTS, false); + map.put(TypedDriverOption.CONTROL_CONNECTION_RECONNECT_CONTACT_POINTS, true); map.put(TypedDriverOption.PREPARE_ON_ALL_NODES, true); map.put(TypedDriverOption.REPREPARE_ENABLED, true); map.put(TypedDriverOption.REPREPARE_CHECK_SYSTEM_TABLE, false); diff --git a/core/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.java b/core/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.java index cfe22540b4d..a46757fb967 100644 --- a/core/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.java +++ b/core/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.java @@ -600,7 +600,15 @@ public String toString() { public static final TypedDriverOption CONTROL_CONNECTION_AGREEMENT_WARN = new TypedDriverOption<>( DefaultDriverOption.CONTROL_CONNECTION_AGREEMENT_WARN, GenericType.BOOLEAN); - /** Whether to forcibly try original contacts if no live nodes are available */ + /** + * Whether to append the original contact points to the control-connection reconnection plan, + * after the live nodes reported by the load balancing policy (defaults to {@code true}). + * + *

Contact points are appended as-is (unresolved hostnames); each is expanded to all of its + * current DNS IPs at connection time, which is also the driver's DNS re-resolution mechanism. The + * append is skipped for topology monitors that re-resolve node addresses themselves (such as the + * cloud/proxy monitors). + */ public static final TypedDriverOption CONTROL_CONNECTION_RECONNECT_CONTACT_POINTS = new TypedDriverOption<>( DefaultDriverOption.CONTROL_CONNECTION_RECONNECT_CONTACT_POINTS, GenericType.BOOLEAN); diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapper.java b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapper.java index f3f3e4fe346..42382519601 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapper.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapper.java @@ -27,6 +27,8 @@ import com.datastax.oss.driver.api.core.session.Request; import com.datastax.oss.driver.api.core.session.Session; import com.datastax.oss.driver.internal.core.context.InternalDriverContext; +import com.datastax.oss.driver.internal.core.util.collection.CompositeQueryPlan; +import com.datastax.oss.driver.internal.core.util.collection.SimpleQueryPlan; import com.datastax.oss.driver.internal.core.util.concurrent.ReplayingEventFilter; import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableMap; import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableSet; @@ -147,7 +149,9 @@ public Queue newQueryPlan( switch (stateRef.get()) { case BEFORE_INIT: case DURING_INIT: - // The contact points are not stored in the metadata yet: + // The contact points are not stored in the metadata yet. Each unresolved hostname is + // expanded to all its DNS IPs at connection time by ChannelFactory, so one entry per + // contact point is enough here. List nodes = new ArrayList<>(context.getMetadataManager().getContactPoints()); Collections.shuffle(nodes); return new ConcurrentLinkedQueue<>(nodes); @@ -164,20 +168,50 @@ public Queue newQueryPlan( @NonNull public Queue newControlReconnectionQueryPlan() { + // Read the state once, before building the regular plan. State transitions are monotonic + // (BEFORE_INIT -> DURING_INIT -> RUNNING -> ...), so this captured value is <= the value + // newQueryPlan() reads internally; that guarantees we never both build the plan from the + // contact points (pre-RUNNING branch of newQueryPlan) and append them again below. + // + // Note: this is still two separate reads of stateRef (this one, and newQueryPlan()'s own + // internal read a moment later), so a transition landing exactly between them is possible: if + // state flips BEFORE_INIT/DURING_INIT -> RUNNING in that window, newQueryPlan() takes the + // RUNNING branch (a real LBP-built plan) while the state captured here is still pre-RUNNING, + // so the contact-point fallback below is skipped for this one call even though + // regularQueryPlan didn't come from the contact-point branch. This is benign: no crash, no + // duplicate entries, and it self-corrects on the very next reconnection attempt. + State state = stateRef.get(); Queue regularQueryPlan = newQueryPlan(null, DriverExecutionProfile.DEFAULT_NAME, null); - if (context - .getConfig() - .getDefaultProfile() - .getBoolean(DefaultDriverOption.CONTROL_CONNECTION_RECONNECT_CONTACT_POINTS)) { - Set originalNodes = context.getMetadataManager().getContactPoints(); + // Only append the contact points as an explicit fallback once the LBP is RUNNING: before that + // (BEFORE_INIT/DURING_INIT), newQueryPlan() above already built regularQueryPlan directly from + // the contact points, so appending them again here would just duplicate every entry. + // + // Skipped when the topology monitor re-resolves node addresses on its own (e.g. proxy-based + // monitors such as client routes or the cloud SNI proxy): those keep addresses fresh without + // this fallback, and appending raw contact points could resurrect nodes the monitor has + // authoritatively removed. The exception is an empty regular plan: with no live node to try, + // reconnection cannot recover on its own, so the contact-point fallback is kept even for those + // monitors. + if (state == State.RUNNING + && context + .getConfig() + .getDefaultProfile() + .getBoolean(DefaultDriverOption.CONTROL_CONNECTION_RECONNECT_CONTACT_POINTS) + && (!context.getTopologyMonitor().reresolvesNodeAddresses() + || regularQueryPlan.isEmpty())) { + // Append the original (unresolved) contact points so every IP their hostname resolves to is + // tried as a fallback: ChannelFactory expands each one at connection time, instead of the + // driver being stuck with whatever single IP a metadata node happens to hold. List contactNodes = new ArrayList<>(); - for (DefaultNode node : originalNodes) { + for (DefaultNode node : context.getMetadataManager().getContactPoints()) { contactNodes.add(DefaultNode.newContactPoint(node.getEndPoint(), context)); } Collections.shuffle(contactNodes); - // Append contact points to the end of the regular query plan so they serve as a fallback - regularQueryPlan.addAll(contactNodes); + // Concatenate rather than mutate: the RUNNING-state regularQueryPlan is a built-in QueryPlan + // whose add()/addAll() throw UnsupportedOperationException (poll() is its only mutator). + // CompositeQueryPlan drains the regular plan first, then the contact-point fallback. + return new CompositeQueryPlan(regularQueryPlan, new SimpleQueryPlan(contactNodes.toArray())); } return regularQueryPlan; diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/MetadataManager.java b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/MetadataManager.java index cd765c818e6..d8671678306 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/MetadataManager.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/metadata/MetadataManager.java @@ -188,6 +188,12 @@ public boolean wasImplicitContactPoint() { * they are never added to metadata and never exposed to user-facing APIs (events, {@link * com.datastax.oss.driver.api.core.metadata.Metadata#getNodes()}, or {@link * com.datastax.oss.driver.api.core.metadata.NodeStateListener} callbacks). + * + *

The metadata node stores {@code nodeInfo.getEndPoint()} as-is and never re-resolves it on + * its own. Re-resolving the original contact-point hostname to pick up current DNS only happens + * through the original-contact-point reconnection fallback (see {@code + * advanced.control-connection.reconnection.fallback-to-original-contact-points}), which re-enters + * the contact points and lets {@code ChannelFactory} expand each hostname at connection time. */ public CompletionStage registerNode(NodeInfo nodeInfo) { Preconditions.checkNotNull(nodeInfo.getHostId(), "Cannot register node without hostId"); diff --git a/core/src/main/resources/reference.conf b/core/src/main/resources/reference.conf index 04fbf40e5a1..3a51e131e9a 100644 --- a/core/src/main/resources/reference.conf +++ b/core/src/main/resources/reference.conf @@ -2336,14 +2336,27 @@ datastax-java-driver { } reconnection { - # Whether to forcibly add original contact points held by MetadataManager to the reconnection plan, - # in case there is no live nodes available according to LBP. - # Experimental. + # Whether to append the original contact points held by MetadataManager to the reconnection + # plan, after the live nodes reported by the load balancing policy. + # + # This is also the driver's DNS re-resolution path. Contact points are kept as unresolved + # hostnames and expanded to their current DNS IPs at connection time, through Netty's + # configured resolver. Metadata nodes, in contrast, store an already-resolved endpoint that + # is never re-resolved, so once DNS records change they would otherwise become stale. Keeping + # this enabled lets control-connection reconnects re-resolve the original hostnames and pick up + # the new IPs once the live-node plan is exhausted. + # + # Note the cost: the contact points are appended without being compared against the live-node + # plan, because at plan time they are still hostnames and the live nodes are already-resolved + # IPs. When DNS has not changed they therefore expand to addresses the plan just failed on, so + # a reconnection round that exhausts the live nodes retries roughly twice as many addresses, + # each up to `advanced.connection.connect-timeout`. Set this to false if you do not need DNS + # re-resolution of contact points and would rather keep reconnection rounds short. # # Required: yes # Modifiable at runtime: yes, the new value will be used for checks issued after the change. # Overridable in a profile: no - fallback-to-original-contact-points = false + fallback-to-original-contact-points = true } } diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapperTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapperTest.java index 89b36b9ee09..8c635982eea 100644 --- a/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapperTest.java +++ b/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapperTest.java @@ -40,10 +40,11 @@ import com.datastax.oss.driver.internal.core.context.EventBus; import com.datastax.oss.driver.internal.core.context.InternalDriverContext; import com.datastax.oss.driver.internal.core.metrics.MetricsFactory; +import com.datastax.oss.driver.internal.core.util.collection.QueryPlan; +import com.datastax.oss.driver.internal.core.util.collection.SimpleQueryPlan; import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableList; import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableMap; import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableSet; -import com.datastax.oss.driver.shaded.guava.common.collect.Lists; import java.util.Map; import java.util.Objects; import java.util.Queue; @@ -79,6 +80,7 @@ public class LoadBalancingPolicyWrapperTest { private EventBus eventBus; @Mock private MetadataManager metadataManager; @Mock private Metadata metadata; + @Mock private TopologyMonitor topologyMonitor; @Mock protected MetricsFactory metricsFactory; @Captor private ArgumentCaptor> initNodesCaptor; @@ -102,13 +104,16 @@ public void setup() { when(metadata.getNodes()).thenReturn(allNodes); when(metadataManager.getContactPoints()).thenReturn(contactPoints); when(context.getMetadataManager()).thenReturn(metadataManager); + when(context.getTopologyMonitor()).thenReturn(topologyMonitor); when(context.getConfig()).thenReturn(config); when(config.getDefaultProfile()).thenReturn(defaultProfile); when(defaultProfile.getBoolean(DefaultDriverOption.CONTROL_CONNECTION_RECONNECT_CONTACT_POINTS)) .thenReturn(false); - defaultPolicyQueryPlan = Lists.newLinkedList(ImmutableList.of(node3, node2, node1)); + // Use a real built-in QueryPlan (not a mutable LinkedList): its add()/addAll() throw, so the + // control-reconnection plan must compose rather than mutate it (see CompositeQueryPlan usage). + defaultPolicyQueryPlan = new SimpleQueryPlan(node3, node2, node1); when(policy1.newQueryPlan(null, null)).thenReturn(defaultPolicyQueryPlan); eventBus = spy(new EventBus("test")); @@ -130,26 +135,28 @@ public void setup() { @Test public void should_build_control_connection_query_plan_from_contact_points_before_init() { - // When + // When — before init, the control-reconnection plan is built straight from the contact points + // (bypassing the load balancing policies), so each hostname can be tried on the first connect. Queue queryPlan = wrapper.newControlReconnectionQueryPlan(); - // Then + // Then — query plan contains the contact points, and no policy was consulted for (LoadBalancingPolicy policy : ImmutableList.of(policy1, policy2, policy3)) { verify(policy, never()).newQueryPlan(null, null); } - assertThat(queryPlan).hasSameElementsAs(contactPoints); + assertThat(queryPlan).containsExactlyInAnyOrder(node1, node2); } @Test public void should_build_query_plan_from_contact_points_before_init() { - // When + // When — before init, the query plan is built straight from the contact points (bypassing the + // load balancing policies) Queue queryPlan = wrapper.newQueryPlan(null, DriverExecutionProfile.DEFAULT_NAME, null); - // Then + // Then — query plan contains the contact points, and no policy was consulted for (LoadBalancingPolicy policy : ImmutableList.of(policy1, policy2, policy3)) { verify(policy, never()).newQueryPlan(null, null); } - assertThat(queryPlan).hasSameElementsAs(contactPoints); + assertThat(queryPlan).containsExactlyInAnyOrder(node1, node2); } @Test @@ -204,8 +211,7 @@ public void should_fetch_control_connection_query_plan_from_policy_after_init() assertThat(queryPlan.poll()).isEqualTo(node3); assertThat(queryPlan.poll()).isEqualTo(node2); assertThat(queryPlan.poll()).isEqualTo(node1); - // Remaining nodes are contact points appended at the end. - // They are new DefaultNode instances created via newContactPoint, so compare by endpoint. + // Remaining nodes are the original contact points appended at the end. Set remainingEndpoints = new java.util.HashSet<>(); for (Node n : queryPlan) { remainingEndpoints.add(n.getEndPoint()); @@ -217,14 +223,92 @@ public void should_fetch_control_connection_query_plan_from_policy_after_init() assertThat(remainingEndpoints).isEqualTo(contactEndpoints); } + @Test + public void should_not_duplicate_contact_points_before_init() { + // Given — the wrapper hasn't been init()-ed yet (state=BEFORE_INIT), so newQueryPlan() already + // builds the regular plan directly from the contact points. The reconnect-contact-points flag + // doesn't matter here: newControlReconnectionQueryPlan() short-circuits on state before even + // reading it, since appending contact points again pre-init would just duplicate every entry in + // the plan. + + // When + Queue queryPlan = wrapper.newControlReconnectionQueryPlan(); + + // Then — the contact points are read only once (not once for the regular plan and again for a + // redundant "fallback" append), and the plan has no duplicate entries. + verify(metadataManager, times(1)).getContactPoints(); + assertThat(queryPlan).containsExactlyInAnyOrder(node1, node2); + } + + @Test + public void + should_not_append_contact_points_to_query_plan_when_reconnect_contact_points_is_disabled() { + // Given — the flag defaults to false in the test setup (see @Before) + wrapper.init(); + + // When + Queue queryPlan = wrapper.newControlReconnectionQueryPlan(); + + // Then + // Only the policy query plan is returned; no contact points are appended. + assertThat(queryPlan).isEqualTo(defaultPolicyQueryPlan); + } + + @Test + public void + should_not_append_contact_points_to_query_plan_when_topology_monitor_reresolves_addresses() { + // Given — the flag is enabled, but the topology monitor re-resolves node addresses on its own + // (e.g. a proxy-based monitor such as client routes or the cloud SNI proxy). + when(defaultProfile.getBoolean(DefaultDriverOption.CONTROL_CONNECTION_RECONNECT_CONTACT_POINTS)) + .thenReturn(true); + when(topologyMonitor.reresolvesNodeAddresses()).thenReturn(true); + wrapper.init(); + + // When + Queue queryPlan = wrapper.newControlReconnectionQueryPlan(); + + // Then + // Contact points must not be appended: the monitor keeps addresses fresh, and appending raw + // contact points could resurrect nodes it has authoritatively removed. + assertThat(queryPlan).isEqualTo(defaultPolicyQueryPlan); + } + + @Test + public void + should_append_contact_points_when_query_plan_empty_even_if_topology_monitor_reresolves() { + // Given — the flag is enabled and the topology monitor re-resolves node addresses on its own, + // but the live-node query plan is empty. With no node to try, reconnection can only recover + // through the contact-point fallback, so it must be appended despite the re-resolving monitor. + when(defaultProfile.getBoolean(DefaultDriverOption.CONTROL_CONNECTION_RECONNECT_CONTACT_POINTS)) + .thenReturn(true); + when(topologyMonitor.reresolvesNodeAddresses()).thenReturn(true); + wrapper.init(); + when(policy1.newQueryPlan(null, null)).thenReturn(QueryPlan.EMPTY); + + // When + Queue queryPlan = wrapper.newControlReconnectionQueryPlan(); + + // Then — the contact points are appended (compare by endpoint since they are new instances) + assertThat(queryPlan.size()).isEqualTo(contactPoints.size()); + Set resultEndpoints = new java.util.HashSet<>(); + for (Node n : queryPlan) { + resultEndpoints.add(n.getEndPoint()); + } + Set contactEndpoints = new java.util.HashSet<>(); + for (DefaultNode n : contactPoints) { + contactEndpoints.add(n.getEndPoint()); + } + assertThat(resultEndpoints).isEqualTo(contactEndpoints); + } + @Test public void should_return_contact_points_when_query_plan_empty_and_flag_enabled() { // Given when(defaultProfile.getBoolean(DefaultDriverOption.CONTROL_CONNECTION_RECONNECT_CONTACT_POINTS)) .thenReturn(true); wrapper.init(); - // Make the policy return an empty query plan - when(policy1.newQueryPlan(null, null)).thenReturn(Lists.newLinkedList(ImmutableList.of())); + // Make the policy return an empty query plan (QueryPlan.EMPTY, as the real policies do) + when(policy1.newQueryPlan(null, null)).thenReturn(QueryPlan.EMPTY); // When Queue queryPlan = wrapper.newControlReconnectionQueryPlan(); diff --git a/integration-tests/src/test/java/com/datastax/oss/driver/core/heartbeat/HeartbeatIT.java b/integration-tests/src/test/java/com/datastax/oss/driver/core/heartbeat/HeartbeatIT.java index 26658bd76d1..b8a32c68450 100644 --- a/integration-tests/src/test/java/com/datastax/oss/driver/core/heartbeat/HeartbeatIT.java +++ b/integration-tests/src/test/java/com/datastax/oss/driver/core/heartbeat/HeartbeatIT.java @@ -235,6 +235,10 @@ private CqlSession newSession(ProgrammaticDriverConfigLoaderBuilder loaderBuilde .withDuration(DefaultDriverOption.HEARTBEAT_TIMEOUT, Duration.ofMillis(500)) .withDuration(DefaultDriverOption.CONNECTION_INIT_QUERY_TIMEOUT, Duration.ofSeconds(2)) .withDuration(DefaultDriverOption.RECONNECTION_MAX_DELAY, Duration.ofSeconds(1)) + // These tests exercise heartbeat behavior only. Disable the contact-point + // reconnection fallback, which would otherwise send an extra OPTIONS message on + // init/reconnect and skew the heartbeat counts. + .withBoolean(DefaultDriverOption.CONTROL_CONNECTION_RECONNECT_CONTACT_POINTS, false) .build(); return SessionUtils.newSession(SIMULACRON_RULE, loader); } From 1fddead0e58aa09fe6306e6aef412b4598c6b2ea Mon Sep 17 00:00:00 2001 From: Mikita Hradovich Date: Thu, 6 Aug 2026 18:17:17 +0200 Subject: [PATCH 8/9] docs: document multi-address DNS resolution (DRIVER-201) Rewrites the address-resolution manual page around the connection layer doing the expansion, and adds an upgrade-guide section covering what changes for users: - there is no public API change, but EndPoint.resolve() may now return an unresolved address for Cloud/SNI and client-route nodes, so a caller doing ((InetSocketAddress) resolve()).getAddress().getHostAddress() gets a NPE where it previously worked; getHostString() is the safe read; - advanced.resolve-contact-points is deprecated and inert; - fallback-to-original-contact-points defaults to true, with its cost stated; - a contact point whose hostname is unhealthy can take longer to give up on, and that cost compounds with the fallback above, since the nodes it appends are exactly the unidentified hostnames that arm the address walk; - the one-time TaggingMetricIdGenerator node-tag rename for hand-built Cloud proxy addresses; - the afterBootstrapInitialized() contract change; - two protected methods removed from internal classes that a subclass could have overridden. Co-Authored-By: Claude Opus 5 (1M context) --- manual/core/address_resolution/README.md | 20 ++++-- upgrade_guide/README.md | 91 ++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 7 deletions(-) diff --git a/manual/core/address_resolution/README.md b/manual/core/address_resolution/README.md index ae44feea3ea..c692aa63336 100644 --- a/manual/core/address_resolution/README.md +++ b/manual/core/address_resolution/README.md @@ -183,13 +183,19 @@ datastax-java-driver { #### DNS resolution -DNS is resolved at connection time (not at route discovery time). The driver delegates to -`InetAddress.getByName()`, which is a blocking call that uses the JVM's built-in DNS cache -(30 s default TTL in the JDK). Because this runs on Netty I/O threads, slow or unresponsive -DNS can block connection establishment and impact driver throughput. To mitigate this, configure -the JVM DNS cache TTL via the `networkaddress.cache.ttl` security property (e.g. in -`$JAVA_HOME/conf/security/java.security` or programmatically with -`java.security.Security.setProperty("networkaddress.cache.ttl", "60")`). +DNS is resolved at connection time (not at route discovery time), and through the same mechanism as +every other address the driver connects to: the route's hostname is handed to the connection layer +unresolved, and Netty's configured `AddressResolverGroup` expands it. A custom resolver installed via +`NettyOptions.afterBootstrapInitialized()` therefore applies to client routes as well, and a hostname +that maps to several addresses has all of them tried in turn. + +With Netty's default (JDK) resolver the lookup is a blocking `InetAddress` call that uses the JVM's +built-in DNS cache (30 s default TTL in the JDK). It runs on a Netty I/O event loop — never on the +admin event loop that drives control-connection reconnects — so it delays the connection attempt +itself. It is therefore worth configuring the JVM DNS cache TTL via the `networkaddress.cache.ttl` +security property (e.g. in `$JAVA_HOME/conf/security/java.security` or programmatically with +`java.security.Security.setProperty("networkaddress.cache.ttl", "60")`), or installing +`DnsAddressResolverGroup` for non-blocking resolution. - **Route-map refresh** — the driver re-queries `system.client_routes` and atomically swaps the in-memory route map in two situations: diff --git a/upgrade_guide/README.md b/upgrade_guide/README.md index 214399dacc7..ee4dfbf61a8 100644 --- a/upgrade_guide/README.md +++ b/upgrade_guide/README.md @@ -19,6 +19,97 @@ under the License. ## Upgrade guide +### 4.19.2.1 + +#### Contact points are expanded to all their DNS addresses at connection time + +Contact points backed by a hostname are now kept unresolved and expanded to **all** the IP +addresses the hostname maps to, at connection time. Previously only the first address returned by +DNS was tried, so a single non-responsive IP behind a multi-record hostname could fail the initial +connection (or a control-connection reconnect) even when the other addresses were healthy. No +configuration change is required to benefit from this. + +The expansion goes through Netty's configured `AddressResolverGroup`, which is the resolver an +unresolved address already reached when it was handed to `Bootstrap.connect()`. A custom resolver +installed through `NettyOptions.afterBootstrapInitialized()` therefore keeps working, and +`Bootstrap.disableResolver()` is still honored. As before, with Netty's default (JDK) resolver the +lookup blocks the I/O event loop it runs on — install `DnsAddressResolverGroup` for non-blocking +resolution. + +The same now applies to the two other address sources that used to perform their own JVM DNS lookups: +the Cloud/SNI proxy address and client-route hostnames are expanded by the connection layer too. A +custom Netty resolver applies to them for the first time, and a proxy hostname with several A-records +has all of them tried within a single connection attempt. + +There is **no public API change**: `EndPoint.resolve()` keeps its signature and is not deprecated. +Third-party `EndPoint` implementations keep working unchanged, with one new expectation — an +implementation should return its address as-is rather than looking names up itself, since resolution +now happens in the connection layer and `resolve()` is called from an event loop. + +**If you call `node.getEndPoint().resolve()` yourself, check how you read the host.** Because +resolution moved to the connection layer, that address is no longer always resolved: + +| Deployment | `resolve()` returns | +| --- | --- | +| Nodes discovered from `system.peers`, and the node the control connection is on | resolved, as before | +| Cloud / SNI proxy | the configured proxy hostname, **unresolved** | +| Cloud private-endpoint client routes (nodes that have a route) | the route hostname, **unresolved** | + +For the last two, `InetSocketAddress.getAddress()` now returns `null`, so a call such as +`((InetSocketAddress) node.getEndPoint().resolve()).getAddress().getHostAddress()` throws a +`NullPointerException` where it previously worked. Use `getHostString()` instead: it returns +whichever of a hostname or an IP literal the address carries, for both the resolved and the +unresolved case, and never triggers a reverse lookup. + +As part of this change: + +- `advanced.resolve-contact-points` is deprecated and now has **no effect**. Contact points are + always kept as unresolved hostnames and expanded at connection time. An already-resolved + `InetSocketAddress` passed programmatically is still used as provided, with no further expansion. +- `advanced.control-connection.reconnection.fallback-to-original-contact-points` now defaults to + `true` (previously `false`). This is also the driver's DNS re-resolution path: metadata nodes + hold an already-resolved endpoint that is never re-resolved, so on control-connection reconnect + the driver falls back to the original contact points to pick up current DNS records once the + live-node plan is exhausted. Set it to `false` to restore the previous behavior. Note the cost: + the contact points are appended without being compared against the live-node plan (at plan time + they are still hostnames, while the live nodes are already-resolved IPs), so when DNS has not + changed a reconnection round that exhausts the live nodes retries roughly twice as many addresses. +- **A contact point whose hostname is unhealthy can take longer to give up on.** The control node's + identity is read from `system.local` over the channel that won, after `ChannelFactory` has already + settled on one address, so the remaining addresses are gone by then. Rather than write off the + whole hostname on the strength of one of its addresses, the driver now retries the same contact + point on another one. The walk is bounded — it stops as soon as an address comes back round, and + never exceeds nine attempts per contact point — but every attempt costs a connect plus a + `system.local` read, and there is no timeout at this layer, so under the default + `advanced.reconnect-on-init = false` that time is paid by `SessionBuilder.build()` itself. The two + costs compound: the contact points appended by the fallback above are unidentified hostnames, + which is exactly what arms this walk, so a reconnection round that exhausts the live nodes can pay + it once per appended contact point. +- If you use `TaggingMetricIdGenerator` **and** build the Cloud proxy address yourself with + `new InetSocketAddress("proxy-host", port)` rather than through a secure connect bundle: the + `node` tag for those nodes changes once, from `proxy-host/1.2.3.4:9042` to `proxy-host:9042`. The + driver now keeps a proxy hostname unresolved so it can be re-expanded to every proxy A-record, and + the tag no longer depends on which proxy IP a connection happened to land on — that stability is + the point, but expect a one-time series rename in dashboards. `asMetricPrefix()`, and therefore + the default `MetricIdGenerator`, is unaffected. +- For advanced deployments that provide a custom `NettyOptions`: the + `afterBootstrapInitialized()` hook now runs once per logical connection to a node (previously + once per attempt, including protocol-version downgrade retries), and it receives the bootstrap + *before* the driver installs its channel handler — a handler set by the hook is replaced, and + the driver logs a one-time warning if it detects one. Use the hook for channel options, + attributes and `Bootstrap.resolver(...)`; use `afterChannelInitialized()` for pipeline + customization. +- Two `protected` methods that no longer have anything to do are removed. Both are in internal + packages, but they were reachable from a subclass, so a custom subclass overriding either of them + will no longer compile: + - `OptionalLocalDcHelper.checkLocalDatacenterCompatibility(String, Set)` — it warned when a + contact point's datacenter differed from the configured local DC, but contact-point nodes never + get a datacenter assigned, so it compared against `null` and warned unconditionally instead of + on a real mismatch. The separate "configured local DC matches no node" warning is retained and + now covers the intended case. + - `ClientRoutesTopologyMonitor.resolveAddress(String)` — a test seam for the JVM DNS lookup that + the monitor no longer performs, now that route hostnames are resolved by the connection layer. + ### 4.19.0.7 #### Cloud private-endpoint support via client routes From 2b64ec26e8ef7a5c9e8dd44bb022b625f971fe7d Mon Sep 17 00:00:00 2001 From: Mikita Hradovich Date: Thu, 6 Aug 2026 18:17:17 +0200 Subject: [PATCH 9/9] test: cover multi-address resolution against a real cluster (DRIVER-201) MockResolverIT drives the end-to-end fix through a JVM-level InetAddress hook: a hostname that maps to one dead and one live address must still produce a working session. Its multi-address test was one change away from being vacuous. The comment claimed the dead record was tried first because of resolver insertion order, but rotate() sorts candidates by toString() and discards that order; the dead address went first only because the sort is lexicographic. The test now captures ChannelFactory at DEBUG and requires the "trying next address" event, which was proven load-bearing: moving the dead IP to one that sorts last makes it fail in 7s instead of passing in 89s. ClientRoutesIT asserts on host strings rather than resolved IPs, since a client route now stays unresolved until the connection layer expands it. Co-Authored-By: Claude Opus 5 (1M context) --- .../core/clientroutes/ClientRoutesIT.java | 16 ++- .../driver/core/resolver/MockResolverIT.java | 110 ++++++++++++++++-- 2 files changed, 114 insertions(+), 12 deletions(-) diff --git a/integration-tests/src/test/java/com/datastax/oss/driver/core/clientroutes/ClientRoutesIT.java b/integration-tests/src/test/java/com/datastax/oss/driver/core/clientroutes/ClientRoutesIT.java index 9a23f368546..e658f843360 100644 --- a/integration-tests/src/test/java/com/datastax/oss/driver/core/clientroutes/ClientRoutesIT.java +++ b/integration-tests/src/test/java/com/datastax/oss/driver/core/clientroutes/ClientRoutesIT.java @@ -186,12 +186,12 @@ private void requireSystemClientRoutesTable(CqlSession admin) { } private InetSocketAddress tryResolve(ClientRoutesTopologyMonitor handler, UUID hostId) { + // resolve() is an in-memory cache lookup that hands the route hostname over unresolved -- the + // connection layer resolves it -- so the only failure left is the monitor being closed. try { return handler.resolve(hostId); } catch (IllegalStateException e) { return null; - } catch (UnknownHostException e) { - throw new RuntimeException("DNS resolution failed for host_id=" + hostId, e); } } @@ -206,7 +206,9 @@ private NodeClassification classifyNodes(CqlSession session) { NodeClassification result = new NodeClassification(); for (Node node : session.getMetadata().getNodes().values()) { InetSocketAddress addr = (InetSocketAddress) node.getEndPoint().resolve(); - String ip = addr.getAddress().getHostAddress(); + // getHostString() rather than getAddress().getHostAddress(): a client route is handed over + // unresolved (the connection layer resolves it), so getAddress() is null for proxied nodes. + String ip = addr.getHostString(); UUID hostId = node.getHostId(); boolean connected = node.getOpenConnections() > 0; LOG.info( @@ -319,7 +321,8 @@ private Map collectHostIds(CcmBridge ccm, int nodeCount, String t .build()) { for (Node node : adminSession.getMetadata().getNodes().values()) { InetSocketAddress addr = (InetSocketAddress) node.getEndPoint().resolve(); - String ip = addr.getAddress().getHostAddress(); + // See classifyNodes(): a client route is unresolved, so getAddress() would be null. + String ip = addr.getHostString(); Integer nodeId = ipToNodeId.get(ip); if (nodeId != null && node.getHostId() != null) { hostIds.put(nodeId, node.getHostId()); @@ -540,7 +543,10 @@ public void should_refresh_routes_after_table_update() throws Exception { () -> { InetSocketAddress resolved = handler.resolve(hostId); assertThat(resolved).isNotNull(); - assertThat(resolved.getAddress().getHostAddress()).isEqualTo(nodeAddr); + // The route is returned unresolved on purpose -- ChannelFactory resolves it through + // Netty's AddressResolverGroup -- so assert on the host string, not getAddress(). + assertThat(resolved.isUnresolved()).isTrue(); + assertThat(resolved.getHostString()).isEqualTo(nodeAddr); assertThat(resolved.getPort()).isEqualTo(9042); }); } diff --git a/integration-tests/src/test/java/com/datastax/oss/driver/core/resolver/MockResolverIT.java b/integration-tests/src/test/java/com/datastax/oss/driver/core/resolver/MockResolverIT.java index 4e9eefebf63..5807dcf4866 100644 --- a/integration-tests/src/test/java/com/datastax/oss/driver/core/resolver/MockResolverIT.java +++ b/integration-tests/src/test/java/com/datastax/oss/driver/core/resolver/MockResolverIT.java @@ -25,9 +25,11 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; import com.datastax.oss.driver.api.core.CqlSession; import com.datastax.oss.driver.api.core.CqlSessionBuilder; import com.datastax.oss.driver.api.core.config.DriverConfigLoader; @@ -37,6 +39,7 @@ import com.datastax.oss.driver.api.core.metadata.Node; import com.datastax.oss.driver.api.testinfra.ccm.CcmBridge; import com.datastax.oss.driver.categories.IsolatedTests; +import com.datastax.oss.driver.internal.core.channel.ChannelFactory; import com.datastax.oss.driver.internal.core.config.typesafe.DefaultProgrammaticDriverConfigLoaderBuilder; import java.net.InetSocketAddress; import java.util.Collection; @@ -47,6 +50,8 @@ import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import org.awaitility.Awaitility; +import org.junit.After; +import org.junit.Before; import org.junit.Test; import org.junit.experimental.categories.Category; import org.slf4j.Logger; @@ -60,6 +65,40 @@ public class MockResolverIT { private static final int CLUSTER_WAIT_SECONDS = 20; // Maximal wait time for cluster nodes to get up + /** An address in the test subnet that no node is ever started on. */ + private static final String DEAD_ADDRESS = "127.0.1.11"; + + private static final ch.qos.logback.classic.Logger CHANNEL_FACTORY_LOGGER = + (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(ChannelFactory.class); + + private ListAppender channelFactoryAppender; + private Level originalChannelFactoryLevel; + + @Before + public void startCapturingChannelFactoryLogs() { + // ChannelFactory reports a candidate it gave up on at DEBUG, which is the only externally + // visible evidence that the multi-address fallback did any work. + originalChannelFactoryLevel = CHANNEL_FACTORY_LOGGER.getLevel(); + CHANNEL_FACTORY_LOGGER.setLevel(Level.DEBUG); + channelFactoryAppender = new ListAppender<>(); + channelFactoryAppender.start(); + CHANNEL_FACTORY_LOGGER.addAppender(channelFactoryAppender); + } + + @After + public void stopCapturingChannelFactoryLogs() { + CHANNEL_FACTORY_LOGGER.detachAppender(channelFactoryAppender); + channelFactoryAppender.stop(); + CHANNEL_FACTORY_LOGGER.setLevel(originalChannelFactoryLevel); + } + + /** The formatted messages {@code ChannelFactory} logged during the current test. */ + private List channelFactoryLogMessages() { + return channelFactoryAppender.list.stream() + .map(ILoggingEvent::getFormattedMessage) + .collect(Collectors.toList()); + } + private static void waitForAllNodesUp(CqlSession session, int expectedNodes) { Awaitility.await() .atMost(CLUSTER_WAIT_SECONDS, TimeUnit.SECONDS) @@ -84,7 +123,6 @@ public void should_connect_with_mocked_hostname() { DriverConfigLoader loader = new DefaultProgrammaticDriverConfigLoaderBuilder() - .withBoolean(TypedDriverOption.RESOLVE_CONTACT_POINTS.getRawOption(), false) .withBoolean(TypedDriverOption.RECONNECT_ON_INIT.getRawOption(), true) .withStringList( TypedDriverOption.CONTACT_POINTS.getRawOption(), @@ -107,19 +145,78 @@ public void should_connect_with_mocked_hostname() { .filter(x -> x.toString().contains("test.cluster.fake")) .collect(Collectors.toSet()); assertThat(filteredNodes).hasSize(1); - InetSocketAddress address = - (InetSocketAddress) filteredNodes.iterator().next().getEndPoint().resolve(); - assertTrue(address.isUnresolved()); + Node node = filteredNodes.iterator().next(); + InetSocketAddress address = (InetSocketAddress) node.getEndPoint().resolve(); + // ChannelFactory pins the control connection's endpoint to the address it actually reached, + // and DefaultTopologyMonitor#buildNodeEndPoint stores that copy for the control node, so + // resolution yields that concrete IP rather than the hostname. + assertFalse(address.isUnresolved()); + assertThat(address.getAddress().getHostAddress()).isEqualTo(ccmBridge.getNodeIpAddress(1)); + // The pinned copy still denotes the same node by hostname though -- it is what the filter + // above matched on -- so metric names do not depend on which IP a connection landed on. + assertThat(node.getEndPoint().asMetricPrefix()).isEqualTo("test_cluster_fake:9042"); } } } + @Test + public void should_connect_when_first_dns_entry_is_non_responsive() { + final int numberOfNodes = 2; + DriverConfigLoader loader = + new DefaultProgrammaticDriverConfigLoaderBuilder() + .withBoolean(TypedDriverOption.RECONNECT_ON_INIT.getRawOption(), true) + .withStringList( + TypedDriverOption.CONTACT_POINTS.getRawOption(), + Collections.singletonList("test.cluster.fake:9042")) + .build(); + + CqlSessionBuilder builder = new CqlSessionBuilder().withConfigLoader(loader); + try (CcmBridge ccmBridge = + CcmBridge.builder().withNodes(numberOfNodes).withIpPrefix("127.0.1.").build()) { + MultimapHostResolverProvider.removeResolverEntries("test.cluster.fake"); + // Nothing is ever started on 127.0.1.11 in this subnet, so it is the dead record. + // + // It is also the one tried *first*, which is what makes this test meaningful -- but not + // because it is registered first. ChannelFactory#rotate sorts the expanded candidates by + // toString() precisely so that resolver order does not matter, and starts a name's first + // expansion at index 0. The sort is lexicographic, so among + // test.cluster.fake/127.0.1.11:9042 + // test.cluster.fake/127.0.1.1:9042 + // test.cluster.fake/127.0.1.2:9042 + // the dead ".11" comes first ('1' = 0x31 sorts before ':' = 0x3A). The assertion below pins + // that down rather than trusting it: if the ordering ever changes, the dead address is never + // reached and this test would otherwise keep passing while testing nothing at all. + MultimapHostResolverProvider.addResolverEntry("test.cluster.fake", DEAD_ADDRESS); + MultimapHostResolverProvider.addResolverEntry( + "test.cluster.fake", ccmBridge.getNodeIpAddress(1)); + MultimapHostResolverProvider.addResolverEntry( + "test.cluster.fake", ccmBridge.getNodeIpAddress(2)); + ccmBridge.create(); + ccmBridge.start(); + + try (CqlSession session = builder.build()) { + waitForAllNodesUp(session, numberOfNodes); + ResultSet rs = session.execute("select * from system.local where key='local'"); + assertThat(rs).isNotNull(); + List rows = rs.all(); + assertThat(rows).hasSize(1); + Collection nodes = session.getMetadata().getNodes().values(); + assertThat(nodes).hasSize(numberOfNodes); + } + + // The connection only survived because the candidate loop moved past the dead record. + assertThat(channelFactoryLogMessages()) + .as("expected a connection attempt to %s to fail and fall through", DEAD_ADDRESS) + .anyMatch( + message -> message.contains(DEAD_ADDRESS) && message.contains("trying next address")); + } + } + @Test public void replace_cluster_test() { final int numberOfNodes = 3; DriverConfigLoader loader = new DefaultProgrammaticDriverConfigLoaderBuilder() - .withBoolean(TypedDriverOption.RESOLVE_CONTACT_POINTS.getRawOption(), false) .withBoolean(TypedDriverOption.RECONNECT_ON_INIT.getRawOption(), true) .withStringList( TypedDriverOption.CONTACT_POINTS.getRawOption(), @@ -206,7 +303,6 @@ public void run_replace_test_20_times() { public void cannot_reconnect_with_resolved_socket() { DriverConfigLoader loader = new DefaultProgrammaticDriverConfigLoaderBuilder() - .withBoolean(TypedDriverOption.RESOLVE_CONTACT_POINTS.getRawOption(), false) .withBoolean(TypedDriverOption.RECONNECT_ON_INIT.getRawOption(), true) .withStringList( TypedDriverOption.CONTACT_POINTS.getRawOption(),