Correctly limit queries done to resolve unresolved nameservers (#10478)

Motivation:

We need limit the number of queries done to resolve unresolved nameserver as otherwise we may never fail the resolve when there is a missconfigured dns server that lead to a "resolve loop".

Modifications:

- When resolve unresolved nameservers ensure we use the allowedQueries as max upper limit for queries so it will eventually fail
- Add unit tests

Result:

No more possibility to fail in a loop while resolve. This is related to https://github.com/netty/netty/issues/10420

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

@@ -34,11 +34,11 @@ final class DnsAddressResolveContext extends DnsResolveContext<InetAddress> {
 
     DnsAddressResolveContext(DnsNameResolver parent, Promise<?> originalPromise,
                              String hostname, DnsRecord[] additionals,
-                             DnsServerAddressStream nameServerAddrs, DnsCache resolveCache,
+                             DnsServerAddressStream nameServerAddrs, int allowedQueries, DnsCache resolveCache,
                              AuthoritativeDnsServerCache authoritativeDnsServerCache,
                              boolean completeEarlyIfPossible) {
         super(parent, originalPromise, hostname, DnsRecord.CLASS_IN,
-              parent.resolveRecordTypes(), additionals, nameServerAddrs);
+              parent.resolveRecordTypes(), additionals, nameServerAddrs, allowedQueries);
         this.resolveCache = resolveCache;
         this.authoritativeDnsServerCache = authoritativeDnsServerCache;
         this.completeEarlyIfPossible = completeEarlyIfPossible;
@@ -49,9 +49,9 @@ final class DnsAddressResolveContext extends DnsResolveContext<InetAddress> {
                                                       String hostname,
                                                       int dnsClass, DnsRecordType[] expectedTypes,
                                                       DnsRecord[] additionals,
-                                                      DnsServerAddressStream nameServerAddrs) {
+                                                      DnsServerAddressStream nameServerAddrs, int allowedQueries) {
         return new DnsAddressResolveContext(parent, originalPromise, hostname, additionals, nameServerAddrs,
-                                            resolveCache, authoritativeDnsServerCache, completeEarlyIfPossible);
+                allowedQueries, resolveCache, authoritativeDnsServerCache, completeEarlyIfPossible);
     }
 
     @Override

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

@@ -816,7 +816,8 @@ public class DnsNameResolver extends InetNameResolver {
         // It was not A/AAAA question or there was no entry in /etc/hosts.
         final DnsServerAddressStream nameServerAddrs =
                 dnsServerAddressStreamProvider.nameServerAddressStream(hostname);
-        new DnsRecordResolveContext(this, promise, question, additionals, nameServerAddrs).resolve(promise);
+        new DnsRecordResolveContext(this, promise, question, additionals, nameServerAddrs, maxQueriesPerResolve)
+                .resolve(promise);
         return promise;
     }
 
@@ -1058,7 +1059,8 @@ public class DnsNameResolver extends InetNameResolver {
         final DnsServerAddressStream nameServerAddrs =
                 dnsServerAddressStreamProvider.nameServerAddressStream(hostname);
         new DnsAddressResolveContext(this, originalPromise, hostname, additionals, nameServerAddrs,
-                                     resolveCache, authoritativeDnsServerCache, completeEarlyIfPossible)
+                                     maxQueriesPerResolve, resolveCache,
+                                     authoritativeDnsServerCache, completeEarlyIfPossible)
                 .resolve(promise);
     }
 

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

@@ -28,17 +28,18 @@ import io.netty.util.concurrent.Promise;
 final class DnsRecordResolveContext extends DnsResolveContext<DnsRecord> {
 
     DnsRecordResolveContext(DnsNameResolver parent, Promise<?> originalPromise, DnsQuestion question,
-                            DnsRecord[] additionals, DnsServerAddressStream nameServerAddrs) {
+                            DnsRecord[] additionals, DnsServerAddressStream nameServerAddrs, int allowedQueries) {
         this(parent, originalPromise, question.name(), question.dnsClass(),
              new DnsRecordType[] { question.type() },
-             additionals, nameServerAddrs);
+             additionals, nameServerAddrs, allowedQueries);
     }
 
     private DnsRecordResolveContext(DnsNameResolver parent, Promise<?> originalPromise, String hostname,
                                     int dnsClass, DnsRecordType[] expectedTypes,
                                     DnsRecord[] additionals,
-                                    DnsServerAddressStream nameServerAddrs) {
-        super(parent, originalPromise, hostname, dnsClass, expectedTypes, additionals, nameServerAddrs);
+                                    DnsServerAddressStream nameServerAddrs,
+                                    int allowedQueries) {
+        super(parent, originalPromise, hostname, dnsClass, expectedTypes, additionals, nameServerAddrs, allowedQueries);
     }
 
     @Override
@@ -46,9 +47,10 @@ final class DnsRecordResolveContext extends DnsResolveContext<DnsRecord> {
                                                     String hostname,
                                                     int dnsClass, DnsRecordType[] expectedTypes,
                                                     DnsRecord[] additionals,
-                                                    DnsServerAddressStream nameServerAddrs) {
+                                                    DnsServerAddressStream nameServerAddrs,
+                                                    int allowedQueries) {
         return new DnsRecordResolveContext(parent, originalPromise, hostname, dnsClass,
-                                           expectedTypes, additionals, nameServerAddrs);
+                                           expectedTypes, additionals, nameServerAddrs, allowedQueries);
     }
 
     @Override

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

@@ -88,7 +88,6 @@ abstract class DnsResolveContext<T> {
     private final String hostname;
     private final int dnsClass;
     private final DnsRecordType[] expectedTypes;
-    private final int maxAllowedQueries;
     final DnsRecord[] additionals;
 
     private final Set<Future<AddressedEnvelope<DnsResponse, InetSocketAddress>>> queriesInProgress =
@@ -102,7 +101,7 @@ abstract class DnsResolveContext<T> {
 
     DnsResolveContext(DnsNameResolver parent, Promise<?> originalPromise,
                       String hostname, int dnsClass, DnsRecordType[] expectedTypes,
-                      DnsRecord[] additionals, DnsServerAddressStream nameServerAddrs) {
+                      DnsRecord[] additionals, DnsServerAddressStream nameServerAddrs, int allowedQueries) {
         assert expectedTypes.length > 0;
 
         this.parent = parent;
@@ -113,8 +112,7 @@ abstract class DnsResolveContext<T> {
         this.additionals = additionals;
 
         this.nameServerAddrs = requireNonNull(nameServerAddrs, "nameServerAddrs");
-        maxAllowedQueries = parent.maxQueriesPerResolve();
-        allowedQueries = maxAllowedQueries;
+        this.allowedQueries = allowedQueries;
     }
 
     static final class DnsResolveContextException extends RuntimeException {
@@ -156,7 +154,7 @@ abstract class DnsResolveContext<T> {
                                                      String hostname,
                                                      int dnsClass, DnsRecordType[] expectedTypes,
                                                      DnsRecord[] additionals,
-                                                     DnsServerAddressStream nameServerAddrs);
+                                                     DnsServerAddressStream nameServerAddrs, int allowedQueries);
 
     /**
      * Converts the given {@link DnsRecord} into {@code T}.
@@ -258,7 +256,8 @@ abstract class DnsResolveContext<T> {
 
     void doSearchDomainQuery(String hostname, Promise<List<T>> nextPromise) {
         DnsResolveContext<T> nextContext = newResolverContext(parent, originalPromise, hostname, dnsClass,
-                                                              expectedTypes, additionals, nameServerAddrs);
+                                                              expectedTypes, additionals, nameServerAddrs,
+                parent.maxQueriesPerResolve());
         nextContext.internalResolve(hostname, nextPromise);
     }
 
@@ -488,8 +487,13 @@ abstract class DnsResolveContext<T> {
         DnsCache resolveCache = resolveCache();
         if (!DnsNameResolver.doResolveAllCached(nameServerName, additionals, resolverPromise, resolveCache,
                 parent.resolvedInternetProtocolFamiliesUnsafe())) {
+
             new DnsAddressResolveContext(parent, originalPromise, nameServerName, additionals,
-                                         parent.newNameServerAddressStream(nameServerName), resolveCache,
+                                         parent.newNameServerAddressStream(nameServerName),
+                                         // Resolving the unresolved nameserver must be limited by allowedQueries
+                                         // so we eventually fail
+                                         allowedQueries,
+                                         resolveCache,
                                          redirectAuthoritativeDnsServerCache(authoritativeDnsServerCache()), false)
                     .resolve(resolverPromise);
         }
@@ -964,6 +968,7 @@ abstract class DnsResolveContext<T> {
         }
 
         // No resolved address found.
+        final int maxAllowedQueries = parent.maxQueriesPerResolve();
         final int tries = maxAllowedQueries - allowedQueries;
         final StringBuilder buf = new StringBuilder(64);
 

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

@@ -1768,6 +1768,58 @@ public class DnsNameResolverTest {
         }
     }
 
+    @Test
+    public void testNsLoopFailsResolveWithAuthoritativeDnsServerCache() throws Exception {
+        testNsLoopFailsResolve(new DefaultAuthoritativeDnsServerCache());
+    }
+
+    @Test
+    public void testNsLoopFailsResolveWithoutAuthoritativeDnsServerCache() throws Exception {
+        testNsLoopFailsResolve(NoopAuthoritativeDnsServerCache.INSTANCE);
+    }
+
+    private void testNsLoopFailsResolve(AuthoritativeDnsServerCache authoritativeDnsServerCache) throws Exception {
+        final String domain = "netty.io";
+        final String ns1Name = "ns1." + domain;
+        final String ns2Name = "ns2." + domain;
+
+        TestDnsServer testDnsServer = new TestDnsServer(new HashSet<String>(
+                Collections.singletonList(domain))) {
+
+            @Override
+            protected DnsMessage filterMessage(DnsMessage message) {
+                // Just always return NS records only without any additional records (glue records).
+                // Because of this the resolver will never be able to resolve and so fail eventually at some
+                // point.
+                for (QuestionRecord record: message.getQuestionRecords()) {
+                    if (record.getDomainName().equals(domain)) {
+                        message.getAdditionalRecords().clear();
+                        message.getAnswerRecords().clear();
+                        message.getAuthorityRecords().add(TestDnsServer.newNsRecord(domain, ns1Name));
+                        message.getAuthorityRecords().add(TestDnsServer.newNsRecord(domain, ns2Name));
+                    }
+                }
+                return message;
+            }
+        };
+        testDnsServer.start();
+        DnsNameResolverBuilder builder = newResolver();
+
+        final DnsNameResolver resolver = builder.resolveCache(NoopDnsCache.INSTANCE)
+                .authoritativeDnsServerCache(authoritativeDnsServerCache)
+                .nameServerProvider(new SingletonDnsServerAddressStreamProvider(testDnsServer.localAddress())).build();
+
+        try {
+            assertThat(resolver.resolve(domain).await().cause(),
+                    Matchers.<Throwable>instanceOf(UnknownHostException.class));
+            assertThat(resolver.resolveAll(domain).await().cause(),
+                    Matchers.<Throwable>instanceOf(UnknownHostException.class));
+        } finally {
+            resolver.close();
+            testDnsServer.stop();
+        }
+    }
+
     private static InetSocketAddress unresolved(InetSocketAddress address) {
         return InetSocketAddress.createUnresolved(address.getHostString(), address.getPort());
     }