Skip to content

Commit e5e8ab3

Browse files
authored
DNS discovery fixes (#855)
* Imported testkit directory * Migrating tests to testkit (#832) * Migrating tests to testkit Short summary of this update: - removed migrated tests - verifyConnectivity support - resolver support - consume support Test mapping (dest: stub/routing.py): - shouldHandleAcquireReadSession -> test_should_read_successfully_from_reader_using_session_run - shouldHandleAcquireReadTransaction -> test_should_read_successfully_from_reader_using_tx_function - shouldHandleAcquireReadSessionAndTransaction -> test_should_read_successfully_from_reader_using_tx_run - shouldRoundRobinReadServers -> test_should_round_robin_readers_when_reading_using_session_run - shouldRoundRobinReadServersWhenUsingTransaction -> test_should_round_robin_readers_when_reading_using_tx_run - shouldThrowSessionExpiredIfReadServerDisappears -> test_should_fail_when_reading_from_unexpectedly_interrupting_reader_using_session_run - shouldThrowSessionExpiredIfReadServerDisappearsWhenUsingTransaction -> test_should_fail_when_reading_from_unexpectedly_interrupting_reader_using_tx_run - shouldThrowSessionExpiredIfWriteServerDisappears -> test_should_fail_when_writing_on_unexpectedly_interrupting_writer_using_session_run - shouldThrowSessionExpiredIfWriteServerDisappearsWhenUsingTransaction -> test_should_fail_when_writing_on_unexpectedly_interrupting_writer_using_tx_run - shouldHandleAcquireWriteSession -> test_should_write_successfully_on_writer_using_session_run - shouldHandleAcquireWriteTransaction -> test_should_write_successfully_on_writer_using_tx_function - shouldHandleAcquireWriteSessionAndTransaction -> test_should_write_successfully_on_writer_using_tx_run - shouldRoundRobinWriteSessions -> test_should_round_robin_writers_when_writing_using_session_run - shouldRoundRobinWriteSessionsInTransaction -> test_should_round_robin_writers_when_writing_using_tx_run - shouldFailOnNonDiscoverableServer -> test_should_fail_discovery_when_router_fails_with_procedure_not_found_code - shouldFailRandomFailureInGetServers -> test_should_fail_discovery_when_router_fails_with_unknown_code - shouldHandleLeaderSwitchWhenWriting -> test_should_fail_when_writing_on_writer_that_returns_not_a_leader_code - shouldHandleLeaderSwitchWhenWritingWithoutConsuming -> test_should_fail_when_writing_without_explicit_consumption_on_writer_that_returns_not_a_leader_code - shouldHandleLeaderSwitchWhenWritingInTransaction -> test_should_fail_when_writing_on_writer_that_returns_not_a_leader_code_using_tx_run - shouldUseWriteSessionModeAndInitialBookmark -> test_should_use_write_session_mode_and_initial_bookmark_when_writing_using_tx_run - shouldUseReadSessionModeAndInitialBookmark -> test_should_use_read_session_mode_and_initial_bookmark_when_reading_using_tx_run - shouldPassBookmarkFromTransactionToTransaction -> test_should_pass_bookmark_from_tx_to_tx_using_tx_run - shouldRetryReadTransactionUntilSuccess -> test_should_retry_read_tx_until_success - shouldRetryWriteTransactionUntilSuccess -> test_should_retry_write_tx_until_success - shouldRetryReadTransactionAndPerformRediscoveryUntilSuccess -> test_should_retry_read_tx_and_rediscovery_until_success - shouldRetryWriteTransactionAndPerformRediscoveryUntilSuccess -> test_should_retry_write_tx_and_rediscovery_until_success - shouldUseInitialRouterForRediscoveryWhenAllOtherRoutersAreDead -> test_should_use_initial_router_for_discovery_when_others_unavailable - shouldInvokeProcedureGetRoutingTableWhenServerVersionPermits -> test_should_successfully_read_from_readable_router_using_tx_function - shouldSendEmptyRoutingContextInHelloMessage -> test_should_send_empty_hello - shouldServeReadsButFailWritesWhenNoWritersAvailable -> test_should_serve_reads_and_fail_writes_when_no_writers_available - shouldAcceptRoutingTableWithoutWritersAndThenRediscover -> test_should_accept_routing_table_without_writers_and_then_rediscover - shouldTreatRoutingTableWithSingleRouterAsValid -> test_should_accept_routing_table_with_single_router - shouldSendMultipleBookmarks -> test_should_successfully_send_multiple_bookmarks - shouldForgetAddressOnDatabaseUnavailableError -> test_should_forget_address_on_database_unavailable_error - shouldUseResolverDuringRediscoveryWhenExistingRoutersFail -> test_should_use_resolver_during_rediscovery_when_existing_routers_fail - shouldRevertToInitialRouterIfKnownRouterThrowsProtocolErrors -> test_should_revert_to_initial_router_if_known_router_throws_protocol_errors * Removing redundant stub server scripts * Migrating tests to testkit part 2 (#839) - shouldSendRoutingContextToServer -> test_should_successfully_get_routing_table_with_context - shouldSendRoutingContextInHelloMessage -> test_should_successfully_get_routing_table_with_context - shouldHandleLeaderSwitchAndRetryWhenWritingInTxFunction -> test_should_write_successfully_on_leader_switch_using_tx_function - shouldSendInitialBookmark -> test_should_use_write_session_mode_and_initial_bookmark_when_writing_using_tx_run - shouldRetryWriteTransactionUntilSuccessWithWhenLeaderIsRemoved -> test_should_retry_write_until_success_with_leader_change_using_tx_function - shouldRetryWriteTransactionUntilSuccessWithWhenLeaderIsRemovedV3 -> test_should_retry_write_until_success_with_leader_shutdown_during_tx_using_tx_function * Migrating tests to testkit part 3 (#840) Adding support for supportsMultiDB call. And exporting the following tests to testkit: - shouldServerWithBoltV4SupportMultiDb -> test_should_successfully_check_if_support_for_multi_db_is_available - shouldServerWithBoltV3NotSupportMultiDb -> test_should_successfully_check_if_support_for_multi_db_is_available Removing redundant scripts * Stub tests migration part 4 (#847) Removed RoutingDriverMultidatabaseBoltKitIT Migrated tests: - shouldDiscoverForDatabase -> test_should_read_successfully_from_reader_using_session_run (this tests seems to cover the same use-case and uses a non-default DB for 4+ versions) - shouldRetryOnEmptyDiscoveryResult -> test_should_read_successfully_on_empty_discovery_result_using_session_run - shouldThrowRoutingErrorIfDatabaseNotFound -> test_should_fail_with_routing_failure_on_db_not_found_discovery_failure - shouldBeAbleToServeReachableDatabase -> test_should_read_successfully_from_reachable_db_after_trying_unreachable_db (message check has been removed) - shouldPassSystemBookmarkWhenGettingRoutingTableForMultiDB -> test_should_pass_system_bookmark_when_getting_rt_for_multi_db (seems to be applicable to V4 only, also the stub server doesn't seem to check bookmarks) - shouldIgnoreSystemBookmarkWhenGettingRoutingTable -> test_should_ignore_system_bookmark_when_getting_rt_for_multi_db - shouldDriverVerifyConnectivity -> test_should_successfully_get_routing_table_with_context (pre-existing test that already tests the connectivity) Also removed redundant scripts and added code support to DriverError * Fix for initial routers DNS resolution (#849) * Fix for initial routers DNS resolution This update fixes the following issue: #833 The desired behaviour for getting a routing table from the initial router (either on bootstrap or when all known routers have failed) is: - resolve the domain name to all IPs - attempt getting a routing table from all of them until first one succeeds by: - getting a connection - trying to get a successful routing table response Prior to this change, the connection pools were created for host and port pairs. When domain name of the host resolves to multiple IP addresses, such pools provide connections to those IPs as a group. While this works for readers and writers, it negatively impacts the routing table fetching process as there is no guarantee which IP address the provided connection is setup for. This update delivers the following changes: - connection pools for routers are IP address based, which allows for deterministic connection retrieval - the resolved IP address set is kept up-to-date (in case known router IPs change) to make sure that the unused connection pools are flushed - the domain name resolution logic has been made configurable (it is private at the moment and is used to facilitate testing) - the testkit backend has been updated to support the domain name resolution configuration (a new test has been added to testkit to cover the issue described above) - the testkit backend has been updated to support connection timeout driver configuration - several tests have been updated to adopt the new changes * Updating test name * Fixed SSL handling (#851) This update fixes a number of SSL-related tests in testkit and CausalClusteringIT.shouldDropBrokenOldConnections test. The connection pooling strategy has been updated to use the same connection pool when the connection host is unambiguous. Removed hardcoded domain name resolution from the BoltServerAddress and moved the logic to ChannelConnectorImpl that uses the DomainNameResolver. * Updated .gitignore
1 parent 10da2e2 commit e5e8ab3

File tree

93 files changed

+1569
-2261
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

93 files changed

+1569
-2261
lines changed

.gitignore

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,5 @@ enterprise/server-enterprise/neo4j-home
2727
integrationtests/data
2828
dependency-reduced-pom.xml
2929
venv
30+
testkit-backend/bin/
31+
testkit/CAs/

driver/src/main/java/org/neo4j/driver/internal/BoltServerAddress.java

Lines changed: 26 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -18,19 +18,13 @@
1818
*/
1919
package org.neo4j.driver.internal;
2020

21-
import java.net.InetAddress;
22-
import java.net.InetSocketAddress;
23-
import java.net.SocketAddress;
2421
import java.net.URI;
25-
import java.net.UnknownHostException;
26-
import java.util.List;
2722
import java.util.Objects;
2823
import java.util.stream.Stream;
2924

3025
import org.neo4j.driver.net.ServerAddress;
3126

3227
import static java.util.Objects.requireNonNull;
33-
import static java.util.stream.Collectors.toList;
3428

3529
/**
3630
* Holds a host and port pair that denotes a Bolt server address.
@@ -40,12 +34,11 @@ public class BoltServerAddress implements ServerAddress
4034
public static final int DEFAULT_PORT = 7687;
4135
public static final BoltServerAddress LOCAL_DEFAULT = new BoltServerAddress( "localhost", DEFAULT_PORT );
4236

43-
private final String host; // This could either be the same as originalHost or it is an IP address resolved from the original host.
44-
private final int port;
37+
protected final String host; // Host or IP address.
38+
private final String connectionHost; // Either is equal to the host or is explicitly provided on creation and is expected to be a resolved IP address.
39+
protected final int port;
4540
private final String stringValue;
4641

47-
private InetAddress resolved;
48-
4942
public BoltServerAddress( String address )
5043
{
5144
this( uriFrom( address ) );
@@ -58,15 +51,17 @@ public BoltServerAddress( URI uri )
5851

5952
public BoltServerAddress( String host, int port )
6053
{
61-
this( host, null, port );
54+
this( host, host, port );
6255
}
6356

64-
private BoltServerAddress( String host, InetAddress resolved, int port )
57+
public BoltServerAddress( String host, String connectionHost, int port )
6558
{
6659
this.host = requireNonNull( host, "host" );
67-
this.resolved = resolved;
60+
this.connectionHost = requireNonNull( connectionHost, "connectionHost" );
6861
this.port = requireValidPort( port );
69-
this.stringValue = resolved != null ? String.format( "%s(%s):%d", host, resolved.getHostAddress(), port ) : String.format( "%s:%d", host, port );
62+
this.stringValue = host.equals( connectionHost )
63+
? String.format( "%s:%d", host, port )
64+
: String.format( "%s(%s):%d", host, connectionHost, port );
7065
}
7166

7267
public static BoltServerAddress from( ServerAddress address )
@@ -87,14 +82,14 @@ public boolean equals( Object o )
8782
{
8883
return false;
8984
}
90-
BoltServerAddress that = (BoltServerAddress) o;
91-
return port == that.port && host.equals( that.host );
85+
BoltServerAddress address = (BoltServerAddress) o;
86+
return port == address.port && host.equals( address.host ) && connectionHost.equals( address.connectionHost );
9287
}
9388

9489
@Override
9590
public int hashCode()
9691
{
97-
return Objects.hash( host, port );
92+
return Objects.hash( host, connectionHost, port );
9893
}
9994

10095
@Override
@@ -103,44 +98,6 @@ public String toString()
10398
return stringValue;
10499
}
105100

106-
/**
107-
* Create a {@link SocketAddress} from this bolt address. This method always attempts to resolve the hostname into
108-
* an {@link InetAddress}.
109-
*
110-
* @return new socket address.
111-
* @see InetSocketAddress
112-
*/
113-
public SocketAddress toSocketAddress()
114-
{
115-
return resolved == null ? new InetSocketAddress( host, port ) : new InetSocketAddress( resolved, port );
116-
}
117-
118-
/**
119-
* Resolve the host name down to an IP address
120-
*
121-
* @return a new address instance
122-
* @throws UnknownHostException if no IP address for the host could be found
123-
* @see InetAddress#getByName(String)
124-
*/
125-
public BoltServerAddress resolve() throws UnknownHostException
126-
{
127-
return new BoltServerAddress( host, InetAddress.getByName( host ), port );
128-
}
129-
130-
/**
131-
* Resolve the host name down to all IP addresses that can be resolved to
132-
*
133-
* @return an array of new address instances that holds resolved addresses
134-
* @throws UnknownHostException if no IP address for the host could be found
135-
* @see InetAddress#getAllByName(String)
136-
*/
137-
public List<BoltServerAddress> resolveAll() throws UnknownHostException
138-
{
139-
return Stream.of( InetAddress.getAllByName( host ) )
140-
.map( address -> new BoltServerAddress( host, address, port ) )
141-
.collect( toList() );
142-
}
143-
144101
@Override
145102
public String host()
146103
{
@@ -153,9 +110,21 @@ public int port()
153110
return port;
154111
}
155112

156-
public boolean isResolved()
113+
public String connectionHost()
114+
{
115+
return connectionHost;
116+
}
117+
118+
/**
119+
* Create a stream of unicast addresses.
120+
* <p>
121+
* While this implementation just returns a stream of itself, the subclasses may provide multiple addresses.
122+
*
123+
* @return stream of unicast addresses.
124+
*/
125+
public Stream<BoltServerAddress> unicastStream()
157126
{
158-
return resolved != null;
127+
return Stream.of( this );
159128
}
160129

161130
private static String hostFrom( URI uri )
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
/*
2+
* Copyright (c) "Neo4j"
3+
* Neo4j Sweden AB [http://neo4j.com]
4+
*
5+
* This file is part of Neo4j.
6+
*
7+
* Licensed under the Apache License, Version 2.0 (the "License");
8+
* you may not use this file except in compliance with the License.
9+
* You may obtain a copy of the License at
10+
*
11+
* http://www.apache.org/licenses/LICENSE-2.0
12+
*
13+
* Unless required by applicable law or agreed to in writing, software
14+
* distributed under the License is distributed on an "AS IS" BASIS,
15+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
16+
* See the License for the specific language governing permissions and
17+
* limitations under the License.
18+
*/
19+
package org.neo4j.driver.internal;
20+
21+
import java.net.InetAddress;
22+
import java.net.UnknownHostException;
23+
24+
public class DefaultDomainNameResolver implements DomainNameResolver
25+
{
26+
private static final DefaultDomainNameResolver INSTANCE = new DefaultDomainNameResolver();
27+
28+
public static DefaultDomainNameResolver getInstance()
29+
{
30+
return INSTANCE;
31+
}
32+
33+
private DefaultDomainNameResolver()
34+
{
35+
}
36+
37+
@Override
38+
public InetAddress[] resolve( String name ) throws UnknownHostException
39+
{
40+
return InetAddress.getAllByName( name );
41+
}
42+
}
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
/*
2+
* Copyright (c) "Neo4j"
3+
* Neo4j Sweden AB [http://neo4j.com]
4+
*
5+
* This file is part of Neo4j.
6+
*
7+
* Licensed under the Apache License, Version 2.0 (the "License");
8+
* you may not use this file except in compliance with the License.
9+
* You may obtain a copy of the License at
10+
*
11+
* http://www.apache.org/licenses/LICENSE-2.0
12+
*
13+
* Unless required by applicable law or agreed to in writing, software
14+
* distributed under the License is distributed on an "AS IS" BASIS,
15+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
16+
* See the License for the specific language governing permissions and
17+
* limitations under the License.
18+
*/
19+
package org.neo4j.driver.internal;
20+
21+
import java.net.InetAddress;
22+
import java.net.UnknownHostException;
23+
24+
/**
25+
* A resolver function used by the driver to resolve domain names.
26+
*/
27+
@FunctionalInterface
28+
public interface DomainNameResolver
29+
{
30+
/**
31+
* Resolve the given domain name to a set of addresses.
32+
*
33+
* @param name the name to resolve.
34+
* @return the resolved addresses.
35+
* @throws UnknownHostException must be thrown if the given name can not be resolved to at least one address.
36+
*/
37+
InetAddress[] resolve( String name ) throws UnknownHostException;
38+
}

driver/src/main/java/org/neo4j/driver/internal/DriverFactory.java

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ protected static MetricsProvider createDriverMetrics( Config config, Clock clock
127127
protected ChannelConnector createConnector( ConnectionSettings settings, SecurityPlan securityPlan,
128128
Config config, Clock clock, RoutingContext routingContext )
129129
{
130-
return new ChannelConnectorImpl( settings, securityPlan, config.logging(), clock, routingContext );
130+
return new ChannelConnectorImpl( settings, securityPlan, config.logging(), clock, routingContext, getDomainNameResolver() );
131131
}
132132

133133
private InternalDriver createDriver( URI uri, SecurityPlan securityPlan, BoltServerAddress address, ConnectionPool connectionPool,
@@ -210,7 +210,7 @@ protected LoadBalancer createLoadBalancer( BoltServerAddress address, Connection
210210
LoadBalancingStrategy loadBalancingStrategy = new LeastConnectedLoadBalancingStrategy( connectionPool, config.logging() );
211211
ServerAddressResolver resolver = createResolver( config );
212212
return new LoadBalancer( address, routingSettings, connectionPool, eventExecutorGroup, createClock(),
213-
config.logging(), loadBalancingStrategy, resolver );
213+
config.logging(), loadBalancingStrategy, resolver, getDomainNameResolver() );
214214
}
215215

216216
private static ServerAddressResolver createResolver( Config config )
@@ -271,6 +271,17 @@ protected Bootstrap createBootstrap( EventLoopGroup eventLoopGroup )
271271
return BootstrapFactory.newBootstrap( eventLoopGroup );
272272
}
273273

274+
/**
275+
* Provides an instance of {@link DomainNameResolver} that is used for domain name resolution.
276+
* <p>
277+
* <b>This method is protected only for testing</b>
278+
*
279+
* @return the instance of {@link DomainNameResolver}.
280+
*/
281+
protected DomainNameResolver getDomainNameResolver()
282+
{
283+
return DefaultDomainNameResolver.getInstance();
284+
}
274285

275286
private static void assertNoRoutingContext( URI uri, RoutingSettings routingSettings )
276287
{
Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
/*
2+
* Copyright (c) "Neo4j"
3+
* Neo4j Sweden AB [http://neo4j.com]
4+
*
5+
* This file is part of Neo4j.
6+
*
7+
* Licensed under the Apache License, Version 2.0 (the "License");
8+
* you may not use this file except in compliance with the License.
9+
* You may obtain a copy of the License at
10+
*
11+
* http://www.apache.org/licenses/LICENSE-2.0
12+
*
13+
* Unless required by applicable law or agreed to in writing, software
14+
* distributed under the License is distributed on an "AS IS" BASIS,
15+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
16+
* See the License for the specific language governing permissions and
17+
* limitations under the License.
18+
*/
19+
package org.neo4j.driver.internal;
20+
21+
import java.net.InetAddress;
22+
import java.util.Arrays;
23+
import java.util.Collections;
24+
import java.util.LinkedHashSet;
25+
import java.util.Objects;
26+
import java.util.Set;
27+
import java.util.stream.Stream;
28+
29+
import static java.util.Objects.requireNonNull;
30+
import static java.util.stream.Collectors.joining;
31+
32+
/**
33+
* An explicitly resolved version of {@link BoltServerAddress} that always contains one or more resolved IP addresses.
34+
*/
35+
public class ResolvedBoltServerAddress extends BoltServerAddress
36+
{
37+
private static final String HOST_ADDRESSES_FORMAT = "%s%s:%d";
38+
private static final int MAX_HOST_ADDRESSES_IN_STRING_VALUE = 5;
39+
private static final String HOST_ADDRESS_DELIMITER = ",";
40+
private static final String HOST_ADDRESSES_PREFIX = "(";
41+
private static final String HOST_ADDRESSES_SUFFIX = ")";
42+
private static final String TRIMMED_HOST_ADDRESSES_SUFFIX = ",..." + HOST_ADDRESSES_SUFFIX;
43+
44+
private final Set<InetAddress> resolvedAddresses;
45+
private final String stringValue;
46+
47+
public ResolvedBoltServerAddress( String host, int port, InetAddress[] resolvedAddressesArr )
48+
{
49+
super( host, port );
50+
requireNonNull( resolvedAddressesArr, "resolvedAddressesArr" );
51+
if ( resolvedAddressesArr.length == 0 )
52+
{
53+
throw new IllegalArgumentException(
54+
"The resolvedAddressesArr must not be empty, check your DomainNameResolver is compliant with the interface contract" );
55+
}
56+
resolvedAddresses = Collections.unmodifiableSet( new LinkedHashSet<>( Arrays.asList( resolvedAddressesArr ) ) );
57+
stringValue = createStringRepresentation();
58+
}
59+
60+
/**
61+
* Create a stream of unicast addresses.
62+
* <p>
63+
* The stream is created from the list of resolved IP addresses. Each unicast address is given a unique IP address as the connectionHost value.
64+
*
65+
* @return stream of unicast addresses.
66+
*/
67+
@Override
68+
public Stream<BoltServerAddress> unicastStream()
69+
{
70+
return resolvedAddresses.stream().map( address -> new BoltServerAddress( host, address.getHostAddress(), port ) );
71+
}
72+
73+
@Override
74+
public boolean equals( Object o )
75+
{
76+
if ( this == o )
77+
{
78+
return true;
79+
}
80+
if ( o == null || getClass() != o.getClass() )
81+
{
82+
return false;
83+
}
84+
if ( !super.equals( o ) )
85+
{
86+
return false;
87+
}
88+
ResolvedBoltServerAddress that = (ResolvedBoltServerAddress) o;
89+
return resolvedAddresses.equals( that.resolvedAddresses );
90+
}
91+
92+
@Override
93+
public int hashCode()
94+
{
95+
return Objects.hash( super.hashCode(), resolvedAddresses );
96+
}
97+
98+
@Override
99+
public String toString()
100+
{
101+
return stringValue;
102+
}
103+
104+
private String createStringRepresentation()
105+
{
106+
String hostAddresses = resolvedAddresses.stream()
107+
.limit( MAX_HOST_ADDRESSES_IN_STRING_VALUE )
108+
.map( InetAddress::getHostAddress )
109+
.collect( joining( HOST_ADDRESS_DELIMITER, HOST_ADDRESSES_PREFIX,
110+
resolvedAddresses.size() > MAX_HOST_ADDRESSES_IN_STRING_VALUE
111+
? TRIMMED_HOST_ADDRESSES_SUFFIX
112+
: HOST_ADDRESSES_SUFFIX ) );
113+
return String.format( HOST_ADDRESSES_FORMAT, host, hostAddresses, port );
114+
}
115+
}

0 commit comments

Comments
 (0)