ZOOKEEPER-3100: ZooKeeper client times out due to random choice of resolved addresses

zookeeper-docs/src/main/resources/markdown/zookeeperProgrammers.md

@@ -1381,6 +1381,22 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
 * *zookeeper.kinit* :
     Specifies path to kinit binary. Default is "/usr/bin/kinit".
 
+* *zookeeper.shuffleDnsResponse* :
+  **New in 3.10.0:**
+  Specifies whether ZooKeeper client should randomly pick an IP address from the DNS lookup query result when resolving 
+  server addresses or not. This is a feature flag in order to keep the old behavior of the default DNS resolver in 
+  `StaticHostProvider`, because we disabled it by default. The reason behind that is shuffling the response of DNS queries 
+  shadows JVM network property `java.net.preferIPv6Addresses` (default: false). This property controls whether JVM's built-in 
+  resolver should prioritize v4 (false value) or v6 (true value) addresses when putting together the list of IP addresses 
+  in the result. In other words the above Java system property was completely ineffective in the case of ZooKeeper server host 
+  resolution protocol and this must have been fixed. In a dual stack environment one might want to prefer one stack over 
+  the other which, in case of Java processes, should be controlled by JVM network properties and ZooKeeper must honor 
+  these settings. Since the old behavior has been with us since day zero, we introduced this feature flag in case you 
+  need it. Such case could be when you have a buggy DNS server which responds IP addresses always in the same order and 
+  you want to randomize that.
+  Default: false
+
+
 <a name="C+Binding"></a>
 
 ### C Binding

zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java

@@ -1140,7 +1140,7 @@ public ZooKeeper(ZooKeeperOptions options) throws IOException {
         if (options.getHostProvider() != null) {
             hostProvider = options.getHostProvider().apply(connectStringParser.getServerAddresses());
         } else {
-            hostProvider = new StaticHostProvider(connectStringParser.getServerAddresses());
+            hostProvider = new StaticHostProvider(connectStringParser.getServerAddresses(), clientConfig);
         }
         this.hostProvider = hostProvider;
 

zookeeper-server/src/main/java/org/apache/zookeeper/client/StaticHostProvider.java

@@ -50,6 +50,8 @@ public interface Resolver {
 
     private static final Logger LOG = LoggerFactory.getLogger(StaticHostProvider.class);
 
+    private ZKClientConfig clientConfig = null;
+
     private List<InetSocketAddress> serverAddresses = new ArrayList<>(5);
 
     private Random sourceOfRandomness;
@@ -90,6 +92,26 @@ public InetAddress[] getAllByName(String name) throws UnknownHostException {
         });
     }
 
+    /**
+     * Constructs a SimpleHostSet with ZKClientConfig.
+     *
+     * Introduced this new overload in 3.10.0 in order to take advantage of some newly introduced feature flags. Like
+     * the shuffle (old) / not to shuffle (new) behavior of DNS resolution.
+     *
+     * @param serverAddresses
+     *            possibly unresolved ZooKeeper server addresses
+     * @param clientConfig
+     *            ZooKeeper client configuration
+     */
+    public StaticHostProvider(Collection<InetSocketAddress> serverAddresses, ZKClientConfig clientConfig) {
+        init(serverAddresses, System.currentTimeMillis() ^ this.hashCode(), new Resolver() {
+            @Override
+            public InetAddress[] getAllByName(String name) throws UnknownHostException {
+                return InetAddress.getAllByName(name);
+            }
+        }, clientConfig);
+    }
+
     /**
      * Constructs a SimpleHostSet.
      *
@@ -125,6 +147,12 @@ public InetAddress[] getAllByName(String name) throws UnknownHostException {
     }
 
     private void init(Collection<InetSocketAddress> serverAddresses, long randomnessSeed, Resolver resolver) {
+        init(serverAddresses, randomnessSeed, resolver, null);
+    }
+
+    private void init(Collection<InetSocketAddress> serverAddresses, long randomnessSeed, Resolver resolver,
+                      ZKClientConfig clientConfig) {
+        this.clientConfig = clientConfig == null ? new ZKClientConfig() : clientConfig;
         this.sourceOfRandomness = new Random(randomnessSeed);
         this.resolver = resolver;
         if (serverAddresses.isEmpty()) {
@@ -142,7 +170,9 @@ private InetSocketAddress resolve(InetSocketAddress address) {
             if (resolvedAddresses.isEmpty()) {
                 return address;
             }
-            Collections.shuffle(resolvedAddresses);
+            if (clientConfig.isShuffleDnsResponseEnabled()) {
+                Collections.shuffle(resolvedAddresses);
+            }
             return new InetSocketAddress(resolvedAddresses.get(0), address.getPort());
         } catch (UnknownHostException e) {
             LOG.error("Unable to resolve address: {}", address.toString(), e);

zookeeper-server/src/main/java/org/apache/zookeeper/client/ZKClientConfig.java

@@ -62,6 +62,13 @@ public class ZKClientConfig extends ZKConfig {
     public static final long ZOOKEEPER_REQUEST_TIMEOUT_DEFAULT = 0;
     public static final String ZK_SASL_CLIENT_ALLOW_REVERSE_DNS = "zookeeper.sasl.client.allowReverseDnsLookup";
     public static final boolean ZK_SASL_CLIENT_ALLOW_REVERSE_DNS_DEFAULT = false;
+    /**
+     * True value preserves the old behavior is to shuffle the addresses in DNS response regardless of address type.
+     * This could help with buggy DNS servers which always return addresses in the same order, but at the same time
+     * ignores the JVM option java.net.preferIPv6Addresses.
+     */
+    public static final String ZOOKEEPER_SHUFFLE_DNS_RESPONSE = "zookeeper.shuffleDnsResponse";
+    public static final boolean ZOOKEEPER_SHUFFLE_DNS_RESPONSE_DEFAULT = false;
 
     public ZKClientConfig() {
         super();
@@ -137,6 +144,14 @@ public boolean isSaslClientEnabled() {
         return Boolean.valueOf(getProperty(ENABLE_CLIENT_SASL_KEY, ENABLE_CLIENT_SASL_DEFAULT));
     }
 
+    /**
+     * Return true if we need to use the old behavior of default DNS resolver and always shuffle the IP addresses
+     * in the DNS lookup response in order to pick a completely random IP address.
+     */
+    public boolean isShuffleDnsResponseEnabled() {
+        return getBoolean(ZOOKEEPER_SHUFFLE_DNS_RESPONSE, ZOOKEEPER_SHUFFLE_DNS_RESPONSE_DEFAULT);
+    }
+
     /**
      * Get the value of the <code>key</code> property as an <code>long</code>.
      * If property is not set, the provided <code>defaultValue</code> is

zookeeper-server/src/test/java/org/apache/zookeeper/test/StaticHostProviderTest.java

@@ -30,9 +30,12 @@
 import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
+import java.net.Inet4Address;
+import java.net.Inet6Address;
 import java.net.InetAddress;
 import java.net.InetSocketAddress;
 import java.net.UnknownHostException;
@@ -115,6 +118,7 @@ public void testNextDoesNotSleepForZero() {
         assertTrue(5 > stop - start);
     }
 
+
     @Test
     public void testEmptyServerAddressesList() {
         assertThrows(IllegalArgumentException.class, () -> {
@@ -888,6 +892,42 @@ public void testReResolvingLocalhost() {
                         + hostProvider.size() + " (after), " + sizeBefore + " (before)");
     }
 
+    @Test
+    public void testResolverKeepsResolutionOrderPreferV6() {
+        // Arrange
+        Collection<InetSocketAddress> servers = new ArrayList<>();
+        servers.add(InetSocketAddress.createUnresolved("test-server-name", 1111));
+        InetAddress[] resolvedAddresses = new InetAddress[] {
+            mock(Inet6Address.class),
+            mock(Inet4Address.class)
+        };
+        StaticHostProvider staticHostProvider = new StaticHostProvider(servers, name -> resolvedAddresses);
+
+        // Act & Assert
+        for (int i = 0; i < 100; i++) {
+           InetSocketAddress next = staticHostProvider.next(0);
+           assertSame(next.getAddress(), resolvedAddresses[0], "Default resolver should always return the first address");
+        }
+    }
+
+    @Test
+    public void testResolverKeepsResolutionOrderPreferV4() {
+        // Arrange
+        Collection<InetSocketAddress> servers = new ArrayList<>();
+        servers.add(InetSocketAddress.createUnresolved("test-server-name", 1111));
+        InetAddress[] resolvedAddresses = new InetAddress[] {
+            mock(Inet4Address.class),
+            mock(Inet6Address.class)
+        };
+        StaticHostProvider staticHostProvider = new StaticHostProvider(servers, name -> resolvedAddresses);
+
+        // Act & Assert
+        for (int i = 0; i < 100; i++) {
+            InetSocketAddress next = staticHostProvider.next(0);
+            assertSame(next.getAddress(), resolvedAddresses[0], "Default resolver should always return the first address");
+        }
+    }
+
     private StaticHostProvider getHostProviderUnresolved(byte size) {
         return new StaticHostProvider(getUnresolvedServerAddresses(size), r.nextLong());
     }