From 7b9a97a81f10ea574a8e6f2596075615d2eeb185 Mon Sep 17 00:00:00 2001
From: Ben Evans <bene.nz@gmail.com>
Date: Tue, 4 May 2021 17:38:07 +1200
Subject: [PATCH] Only fall back to CNAME on A/AAAA queries (#11216)

Motivation:

DNS resolver falls back to trying CNAME if no records found, but should
only try this for A/AAAA queries. Does not make sense for other query
types, results in a redundant CNAME query that is just going to fail.

Modification:

Check query type before deciding to try CNAME. Only proceed if type is A
or AAAA.

Added unit test to verify CNAME is only tried after A/AAAA queries.

Result:

Fixes #11214.
---
 .../netty/resolver/dns/DnsResolveContext.java |  3 +-
 .../resolver/dns/DnsNameResolverTest.java     | 60 +++++++++++++++++++
 2 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
index 5b6d0614a7..6d55536293 100644
--- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
+++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
@@ -951,7 +951,8 @@ abstract class DnsResolveContext<T> {
             // If cause != null we know this was caused by a timeout / cancel / transport exception. In this case we
             // won't try to resolve the CNAME as we only should do this if we could not get the expected records
             // because they do not exist and the DNS server did probably signal it.
-            if (cause == null && !triedCNAME) {
+            if (cause == null && !triedCNAME &&
+                    (question.type() == DnsRecordType.A || question.type() == DnsRecordType.AAAA)) {
                 // As the last resort, try to query CNAME, just in case the name server has it.
                 triedCNAME = true;
 
diff --git a/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java b/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java
index bd16d47ba1..00fc9c6357 100644
--- a/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java
+++ b/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java
@@ -109,6 +109,7 @@ import java.util.concurrent.atomic.AtomicReference;
 import static io.netty.handler.codec.dns.DnsRecordType.A;
 import static io.netty.handler.codec.dns.DnsRecordType.AAAA;
 import static io.netty.handler.codec.dns.DnsRecordType.CNAME;
+import static io.netty.handler.codec.dns.DnsRecordType.NAPTR;
 import static io.netty.handler.codec.dns.DnsRecordType.SRV;
 import static io.netty.resolver.dns.DnsNameResolver.DEFAULT_RESOLVE_ADDRESS_TYPES;
 import static io.netty.resolver.dns.DnsServerAddresses.sequential;
@@ -3329,6 +3330,65 @@ public class DnsNameResolverTest {
         }
     }
 
+    @Test
+    public void testCNAMEOnlyTriedOnAddressLookups() throws Exception {
+
+        final AtomicInteger cnameQueries = new AtomicInteger();
+
+        TestDnsServer dnsServer2 = new TestDnsServer(new RecordStore() {
+            @Override
+            public Set<ResourceRecord> getRecords(QuestionRecord questionRecord) {
+                if (questionRecord.getRecordType() == RecordType.CNAME) {
+                    cnameQueries.incrementAndGet();
+                }
+
+                return Collections.emptySet();
+            }
+        });
+
+        dnsServer2.start();
+
+        DnsNameResolver resolver = null;
+        try {
+            resolver = newNonCachedResolver(ResolvedAddressTypes.IPV4_PREFERRED)
+                    .maxQueriesPerResolve(4)
+                    .searchDomains(Collections.<String>emptyList())
+                    .nameServerProvider(new SingletonDnsServerAddressStreamProvider(dnsServer2.localAddress()))
+                    .build();
+
+            // We expect these resolves to fail with UnknownHostException,
+            // and then check that no unexpected CNAME queries were performed.
+            assertThat(resolver.resolveAll(new DefaultDnsQuestion("lookup-srv.netty.io", SRV)).await().cause(),
+                    instanceOf(UnknownHostException.class));
+            assertEquals(0, cnameQueries.get());
+
+            assertThat(resolver.resolveAll(new DefaultDnsQuestion("lookup-naptr.netty.io", NAPTR)).await().cause(),
+                    instanceOf(UnknownHostException.class));
+            assertEquals(0, cnameQueries.get());
+
+            assertThat(resolver.resolveAll(new DefaultDnsQuestion("lookup-cname.netty.io", CNAME)).await().cause(),
+                    instanceOf(UnknownHostException.class));
+            assertEquals(1, cnameQueries.getAndSet(0));
+
+            assertThat(resolver.resolveAll(new DefaultDnsQuestion("lookup-a.netty.io", A)).await().cause(),
+                    instanceOf(UnknownHostException.class));
+            assertEquals(1, cnameQueries.getAndSet(0));
+
+            assertThat(resolver.resolveAll(new DefaultDnsQuestion("lookup-aaaa.netty.io", AAAA)).await().cause(),
+                    instanceOf(UnknownHostException.class));
+            assertEquals(1, cnameQueries.getAndSet(0));
+
+            assertThat(resolver.resolveAll("lookup-address.netty.io").await().cause(),
+                    instanceOf(UnknownHostException.class));
+            assertEquals(1, cnameQueries.getAndSet(0));
+        } finally {
+            dnsServer2.stop();
+            if (resolver != null) {
+                resolver.close();
+            }
+        }
+    }
+
     private static void assertNotEmptyAndRelease(Future<List<DnsRecord>> recordsFuture) throws Exception {
         List<DnsRecord> records = recordsFuture.get();
         assertFalse(records.isEmpty());