Always follow cnames even if a matching A or AAAA record was found. (#7919)

Motivation:

At the moment if you do a resolveAll and at least one A / AAAA record is present we will not follow any CNAMEs that are also present. This is different to how the JDK behaves.

Modifications:

- Allows follow CNAMEs.
- Add unit test.

Result:

Fixes https://github.com/netty/netty/issues/7915.

resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolveContext.java

@@ -49,19 +49,6 @@ final class DnsAddressResolveContext extends DnsResolveContext<InetAddress> {
         return decodeAddress(record, hostname, parent.isDecodeIdn());
     }
 
-    @Override
-    boolean containsExpectedResult(List<InetAddress> finalResult) {
-        final int size = finalResult.size();
-        final Class<? extends InetAddress> inetAddressType = parent.preferredAddressType().addressType();
-        for (int i = 0; i < size; i++) {
-            InetAddress address = finalResult.get(i);
-            if (inetAddressType.isInstance(address)) {
-                return true;
-            }
-        }
-        return false;
-    }
-
     @Override
     List<InetAddress> filterResults(List<InetAddress> unfiltered) {
         final Class<? extends InetAddress> inetAddressType = parent.preferredAddressType().addressType();

resolver-dns/src/main/java/io/netty/resolver/dns/DnsRecordResolveContext.java

@@ -15,9 +15,6 @@
  */
 package io.netty.resolver.dns;
 
-import static io.netty.resolver.dns.DnsAddressDecoder.decodeAddress;
-
-import java.net.InetAddress;
 import java.net.UnknownHostException;
 import java.util.List;
 
@@ -56,11 +53,6 @@ final class DnsRecordResolveContext extends DnsResolveContext<DnsRecord> {
         return ReferenceCountUtil.retain(record);
     }
 
-    @Override
-    boolean containsExpectedResult(List<DnsRecord> finalResult) {
-        return true;
-    }
-
     @Override
     List<DnsRecord> filterResults(List<DnsRecord> unfiltered) {
         return unfiltered;

resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java

@@ -137,12 +137,6 @@ abstract class DnsResolveContext<T> {
      */
     abstract T convertRecord(DnsRecord record, String hostname, DnsRecord[] additionals, EventLoop eventLoop);
 
-    /**
-     * Returns {@code true} if the given list contains any expected records. {@code finalResult} always contains
-     * at least one element.
-     */
-    abstract boolean containsExpectedResult(List<T> finalResult);
-
     /**
      * Returns a filtered list of results which should be the final result of DNS resolution. This must take into
      * account JDK semantics such as {@link NetUtil#isIpV6AddressesPreferred()}.
@@ -173,7 +167,7 @@ abstract class DnsResolveContext<T> {
             doSearchDomainQuery(initialHostname, new FutureListener<List<T>>() {
                 private int searchDomainIdx = initialSearchDomainIdx;
                 @Override
-                public void operationComplete(Future<List<T>> future) throws Exception {
+                public void operationComplete(Future<List<T>> future) {
                     Throwable cause = future.cause();
                     if (cause == null) {
                         promise.trySuccess(future.getNow());
@@ -232,11 +226,11 @@ abstract class DnsResolveContext<T> {
 
         final int end = expectedTypes.length - 1;
         for (int i = 0; i < end; ++i) {
-            if (!query(hostname, expectedTypes[i], nameServerAddressStream.duplicate(), promise, null)) {
+            if (!query(hostname, expectedTypes[i], nameServerAddressStream.duplicate(), promise)) {
                 return;
             }
         }
-        query(hostname, expectedTypes[end], nameServerAddressStream, promise, null);
+        query(hostname, expectedTypes[end], nameServerAddressStream, promise);
     }
 
     /**
@@ -391,7 +385,7 @@ abstract class DnsResolveContext<T> {
         });
     }
 
-    void onResponse(final DnsServerAddressStream nameServerAddrStream, final int nameServerAddrStreamIndex,
+    private void onResponse(final DnsServerAddressStream nameServerAddrStream, final int nameServerAddrStreamIndex,
                     final DnsQuestion question, AddressedEnvelope<DnsResponse, InetSocketAddress> envelope,
                     final DnsQueryLifecycleObserver queryLifecycleObserver,
                     Promise<List<T>> promise) {
@@ -406,7 +400,7 @@ abstract class DnsResolveContext<T> {
                 final DnsRecordType type = question.type();
 
                 if (type == DnsRecordType.CNAME) {
-                    onResponseCNAME(question, envelope, queryLifecycleObserver, promise);
+                    onResponseCNAME(question, buildAliasMap(envelope.content()), queryLifecycleObserver, promise);
                     return;
                 }
 
@@ -562,24 +556,20 @@ abstract class DnsResolveContext<T> {
             // Note that we do not break from the loop here, so we decode/cache all A/AAAA records.
         }
 
+        if (cnames.isEmpty()) {
             if (found) {
                 queryLifecycleObserver.querySucceed();
                 return;
             }
-
-        if (cnames.isEmpty()) {
             queryLifecycleObserver.queryFailed(NO_MATCHING_RECORD_QUERY_FAILED_EXCEPTION);
         } else {
-            // We got only CNAME, not one of expectedTypes.
+            queryLifecycleObserver.querySucceed();
-            onResponseCNAME(question, cnames, queryLifecycleObserver, promise);
+            // We also got a CNAME so we need to ensure we also query it.
+            onResponseCNAME(question, cnames,
+                    parent.dnsQueryLifecycleObserverFactory().newDnsQueryLifecycleObserver(question), promise);
         }
     }
 
-    private void onResponseCNAME(DnsQuestion question, AddressedEnvelope<DnsResponse, InetSocketAddress> envelope,
-                                 final DnsQueryLifecycleObserver queryLifecycleObserver, Promise<List<T>> promise) {
-        onResponseCNAME(question, buildAliasMap(envelope.content()), queryLifecycleObserver, promise);
-    }
-
     private void onResponseCNAME(
             DnsQuestion question, Map<String, String> cnames,
             final DnsQueryLifecycleObserver queryLifecycleObserver,
@@ -637,23 +627,19 @@ abstract class DnsResolveContext<T> {
         return cnames != null? cnames : Collections.<String, String>emptyMap();
     }
 
-    void tryToFinishResolve(final DnsServerAddressStream nameServerAddrStream,
+    private void tryToFinishResolve(final DnsServerAddressStream nameServerAddrStream,
                             final int nameServerAddrStreamIndex,
                             final DnsQuestion question,
                             final DnsQueryLifecycleObserver queryLifecycleObserver,
                             final Promise<List<T>> promise,
                             final Throwable cause) {
+
         // There are no queries left to try.
         if (!queriesInProgress.isEmpty()) {
             queryLifecycleObserver.queryCancelled(allowedQueries);
 
-            // There are still some queries we did not receive responses for.
+            // There are still some queries in process, we will try to notify once the next one finishes until
-            if (finalResult != null && containsExpectedResult(finalResult)) {
+            // all are finished.
-                // But it's OK to finish the resolution process if we got something expected.
-                finishResolve(promise, cause);
-            }
-
-            // We did not get an expected result yet, so we can't finish the resolution process.
             return;
         }
 
@@ -682,7 +668,7 @@ abstract class DnsResolveContext<T> {
                 // As the last resort, try to query CNAME, just in case the name server has it.
                 triedCNAME = true;
 
-                query(hostname, DnsRecordType.CNAME, getNameServers(hostname), promise, null);
+                query(hostname, DnsRecordType.CNAME, getNameServers(hostname), promise);
                 return;
             }
         } else {
@@ -773,12 +759,12 @@ abstract class DnsResolveContext<T> {
     }
 
     private boolean query(String hostname, DnsRecordType type, DnsServerAddressStream dnsServerAddressStream,
-                          Promise<List<T>> promise, Throwable cause) {
+                          Promise<List<T>> promise) {
         final DnsQuestion question = newQuestion(hostname, type);
         if (question == null) {
             return false;
         }
-        query(dnsServerAddressStream, 0, question, promise, cause);
+        query(dnsServerAddressStream, 0, question, promise, null);
         return true;
     }
 

resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java

@@ -72,6 +72,7 @@ import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
@@ -567,7 +568,7 @@ public class DnsNameResolverTest {
     }
 
     @Test
-    public void testQueryMx() throws Exception {
+    public void testQueryMx() {
         DnsNameResolver resolver = newResolver().build();
         try {
             assertThat(resolver.isRecursionDesired(), is(true));
@@ -1632,4 +1633,53 @@ public class DnsNameResolverTest {
         assertEquals(channelFactory, copiedBuilder.channelFactory());
         assertEquals(newChannelFactory, builder.channelFactory());
     }
+
+    @Test
+    public void testFollowCNAMEEvenIfARecordIsPresent() throws IOException {
+        TestDnsServer dnsServer2 = new TestDnsServer(new RecordStore() {
+
+            @Override
+            public Set<ResourceRecord> getRecords(QuestionRecord question) {
+                if (question.getDomainName().equals("cname.netty.io")) {
+                    Map<String, Object> map1 = new HashMap<String, Object>();
+                    map1.put(DnsAttribute.IP_ADDRESS.toLowerCase(), "10.0.0.99");
+                    return Collections.<ResourceRecord>singleton(
+                            new TestDnsServer.TestResourceRecord(question.getDomainName(), RecordType.A, map1));
+                } else {
+                    Set<ResourceRecord> records = new LinkedHashSet<ResourceRecord>(2);
+                    Map<String, Object> map = new HashMap<String, Object>();
+                    map.put(DnsAttribute.DOMAIN_NAME.toLowerCase(), "cname.netty.io");
+                    records.add(new TestDnsServer.TestResourceRecord(
+                            question.getDomainName(), RecordType.CNAME, map));
+
+                    Map<String, Object> map1 = new HashMap<String, Object>();
+                    map1.put(DnsAttribute.IP_ADDRESS.toLowerCase(), "10.0.0.2");
+                    records.add(new TestDnsServer.TestResourceRecord(
+                            question.getDomainName(), RecordType.A, map1));
+                    return records;
+                }
+            }
+        });
+        dnsServer2.start();
+        DnsNameResolver resolver = null;
+        try {
+            DnsNameResolverBuilder builder = newResolver()
+                    .recursionDesired(true)
+                    .resolvedAddressTypes(ResolvedAddressTypes.IPV4_ONLY)
+                    .maxQueriesPerResolve(16)
+                    .nameServerProvider(new SingletonDnsServerAddressStreamProvider(dnsServer2.localAddress()));
+
+            resolver = builder.build();
+            List<InetAddress> resolvedAddresses =
+                    resolver.resolveAll("somehost.netty.io").syncUninterruptibly().getNow();
+            assertEquals(2, resolvedAddresses.size());
+            assertTrue(resolvedAddresses.contains(InetAddress.getByAddress(new byte[] { 10, 0, 0, 99 })));
+            assertTrue(resolvedAddresses.contains(InetAddress.getByAddress(new byte[] { 10, 0, 0, 2 })));
+        } finally {
+            dnsServer2.stop();
+            if (resolver != null) {
+                resolver.close();
+            }
+        }
+    }
 }