Correctly handle hostnames with and without trailing dot in the DefaultDnsCache and use it for searchdomains. (#8181)

Motivation:

We should ensure we return the same cached entries for the hostname and hostname ending with dot. Beside this we also should use it for the searchdomains as well.

Modifications:

- Internally always use hostname with a dot as a key and so ensure we correctly handle it in the cache.
- Also query the cache for each searchdomain
- Add unit tests

Result:

Use the same cached entries for hostname with and without trailing dot. Query the cache for each searchdomain query as well

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

@@ -19,6 +19,7 @@ import io.netty.channel.EventLoop;
 import io.netty.handler.codec.dns.DnsRecord;
 import io.netty.util.concurrent.ScheduledFuture;
 import io.netty.util.internal.PlatformDependent;
+import io.netty.util.internal.StringUtil;
 import io.netty.util.internal.UnstableApi;
 
 import java.net.InetAddress;
@@ -115,7 +116,7 @@ public class DefaultDnsCache implements DnsCache {
     @Override
     public boolean clear(String hostname) {
         checkNotNull(hostname, "hostname");
-        Entries entries = resolveCache.remove(hostname);
+        Entries entries = resolveCache.remove(appendDot(hostname));
         return entries != null && entries.clearAndCancel();
     }
 
@@ -130,7 +131,7 @@ public class DefaultDnsCache implements DnsCache {
             return Collections.<DnsCacheEntry>emptyList();
         }
 
-        Entries entries = resolveCache.get(hostname);
+        Entries entries = resolveCache.get(appendDot(hostname));
         return entries == null ? null : entries.get();
     }
 
@@ -144,7 +145,8 @@ public class DefaultDnsCache implements DnsCache {
         if (maxTtl == 0 || !emptyAdditionals(additionals)) {
             return e;
         }
-        cache0(e, Math.max(minTtl, Math.min(MAX_SUPPORTED_TTL_SECS, (int) Math.min(maxTtl, originalTtl))), loop);
+        cache0(appendDot(hostname), e,
+                Math.max(minTtl, Math.min(MAX_SUPPORTED_TTL_SECS, (int) Math.min(maxTtl, originalTtl))), loop);
         return e;
     }
 
@@ -159,25 +161,25 @@ public class DefaultDnsCache implements DnsCache {
             return e;
         }
 
-        cache0(e, Math.min(MAX_SUPPORTED_TTL_SECS, negativeTtl), loop);
+        cache0(appendDot(hostname), e, Math.min(MAX_SUPPORTED_TTL_SECS, negativeTtl), loop);
         return e;
     }
 
-    private void cache0(DefaultDnsCacheEntry e, int ttl, EventLoop loop) {
+    private void cache0(String hostname, DefaultDnsCacheEntry e, int ttl, EventLoop loop) {
-        Entries entries = resolveCache.get(e.hostname());
+        Entries entries = resolveCache.get(hostname);
         if (entries == null) {
             entries = new Entries(e);
-            Entries oldEntries = resolveCache.putIfAbsent(e.hostname(), entries);
+            Entries oldEntries = resolveCache.putIfAbsent(hostname, entries);
             if (oldEntries != null) {
                 entries = oldEntries;
             }
         }
         entries.add(e);
 
-        scheduleCacheExpiration(e, ttl, loop);
+        scheduleCacheExpiration(hostname, e, ttl, loop);
     }
 
-    private void scheduleCacheExpiration(final DefaultDnsCacheEntry e,
+    private void scheduleCacheExpiration(final String hostname, final DefaultDnsCacheEntry e,
                                          int ttl,
                                          EventLoop loop) {
         e.scheduleExpiration(loop, new Runnable() {
@@ -192,7 +194,7 @@ public class DefaultDnsCache implements DnsCache {
                         // completely fine to remove the entry even if the TTL is not reached yet.
                         //
                         // See https://github.com/netty/netty/issues/7329
-                        Entries entries = resolveCache.remove(e.hostname);
+                        Entries entries = resolveCache.remove(hostname);
                         if (entries != null) {
                             entries.clearAndCancel();
                         }
@@ -239,10 +241,6 @@ public class DefaultDnsCache implements DnsCache {
             return cause;
         }
 
-        String hostname() {
-            return hostname;
-        }
-
         void scheduleExpiration(EventLoop loop, Runnable task, long delay, TimeUnit unit) {
             assert expirationFuture == null : "expiration task scheduled already";
             expirationFuture = loop.schedule(task, delay, unit);
@@ -338,4 +336,8 @@ public class DefaultDnsCache implements DnsCache {
             }
         }
     }
+
+    private static String appendDot(String hostname) {
+        return StringUtil.endsWith(hostname, '.') ? hostname : hostname + '.';
+    }
 }

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

@@ -25,6 +25,7 @@ import java.util.List;
 import io.netty.channel.EventLoop;
 import io.netty.handler.codec.dns.DnsRecord;
 import io.netty.handler.codec.dns.DnsRecordType;
+import io.netty.util.concurrent.Promise;
 
 final class DnsAddressResolveContext extends DnsResolveContext<InetAddress> {
 
@@ -84,4 +85,12 @@ final class DnsAddressResolveContext extends DnsResolveContext<InetAddress> {
     void cache(String hostname, DnsRecord[] additionals, UnknownHostException cause) {
         resolveCache.cache(hostname, additionals, cause, parent.ch.eventLoop());
     }
+
+    @Override
+    void doSearchDomainQuery(String hostname, Promise<List<InetAddress>> nextPromise) {
+        // Query the cache for the hostname first and only do a query if we could not find it in the cache.
+        if (!parent.doResolveAllCached(hostname, additionals, nextPromise, resolveCache)) {
+            super.doSearchDomainQuery(hostname, nextPromise);
+        }
+    }
 }

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

@@ -790,7 +790,7 @@ public class DnsNameResolver extends InetNameResolver {
         }
     }
 
-    private boolean doResolveAllCached(String hostname,
+    boolean doResolveAllCached(String hostname,
                                        DnsRecord[] additionals,
                                        Promise<List<InetAddress>> promise,
                                        DnsCache resolveCache) {

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

@@ -97,7 +97,7 @@ abstract class DnsResolveContext<T> {
     private final int dnsClass;
     private final DnsRecordType[] expectedTypes;
     private final int maxAllowedQueries;
-    private final DnsRecord[] additionals;
+    final DnsRecord[] additionals;
 
     private final Set<Future<AddressedEnvelope<DnsResponse, InetSocketAddress>>> queriesInProgress =
             Collections.newSetFromMap(
@@ -164,7 +164,8 @@ abstract class DnsResolveContext<T> {
             final String initialHostname = startWithoutSearchDomain ? hostname : hostname + '.' + searchDomains[0];
             final int initialSearchDomainIdx = startWithoutSearchDomain ? 0 : 1;
 
-            doSearchDomainQuery(initialHostname, new FutureListener<List<T>>() {
+            final Promise<List<T>> searchDomainPromise = parent.executor().newPromise();
+            searchDomainPromise.addListener(new FutureListener<List<T>>() {
                 private int searchDomainIdx = initialSearchDomainIdx;
                 @Override
                 public void operationComplete(Future<List<T>> future) {
@@ -175,7 +176,9 @@ abstract class DnsResolveContext<T> {
                         if (DnsNameResolver.isTransportOrTimeoutError(cause)) {
                             promise.tryFailure(new SearchDomainUnknownHostException(cause, hostname));
                         } else if (searchDomainIdx < searchDomains.length) {
-                            doSearchDomainQuery(hostname + '.' + searchDomains[searchDomainIdx++], this);
+                            Promise<List<T>> newPromise = parent.executor().newPromise();
+                            newPromise.addListener(this);
+                            doSearchDomainQuery(hostname + '.' + searchDomains[searchDomainIdx++], newPromise);
                         } else if (!startWithoutSearchDomain) {
                             internalResolve(promise);
                         } else {
@@ -184,6 +187,7 @@ abstract class DnsResolveContext<T> {
                     }
                 }
             });
+            doSearchDomainQuery(initialHostname, searchDomainPromise);
         }
     }
 
@@ -213,12 +217,10 @@ abstract class DnsResolveContext<T> {
         }
     }
 
-    private void doSearchDomainQuery(String hostname, FutureListener<List<T>> listener) {
+    void doSearchDomainQuery(String hostname, Promise<List<T>> nextPromise) {
         DnsResolveContext<T> nextContext = newResolverContext(parent, hostname, dnsClass, expectedTypes,
                                                               additionals, nameServerAddrs);
-        Promise<List<T>> nextPromise = parent.executor().newPromise();
         nextContext.internalResolve(nextPromise);
-        nextPromise.addListener(listener);
     }
 
     private void internalResolve(Promise<List<T>> promise) {

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

@@ -172,4 +172,30 @@ public class DefaultDnsCacheTest {
             group.shutdownGracefully();
         }
     }
+
+    @Test
+    public void testDotHandling() throws Exception {
+        InetAddress addr1 = InetAddress.getByAddress(new byte[] { 10, 0, 0, 1 });
+        InetAddress addr2 = InetAddress.getByAddress(new byte[] { 10, 0, 0, 2 });
+        EventLoopGroup group = new DefaultEventLoopGroup(1);
+
+        try {
+            EventLoop loop = group.next();
+            final DefaultDnsCache cache = new DefaultDnsCache(1, 100, 100);
+            cache.cache("netty.io", null, addr1, 10000, loop);
+            cache.cache("netty.io.", null, addr2, 10000, loop);
+
+            List<? extends DnsCacheEntry> entries = cache.get("netty.io", null);
+            assertEquals(2, entries.size());
+            assertEntry(entries.get(0), addr1);
+            assertEntry(entries.get(1), addr2);
+
+            List<? extends DnsCacheEntry> entries2 = cache.get("netty.io.", null);
+            assertEquals(2, entries2.size());
+            assertEntry(entries2.get(0), addr1);
+            assertEntry(entries2.get(1), addr2);
+        } finally {
+            group.shutdownGracefully();
+        }
+    }
 }

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

@@ -96,13 +96,7 @@ import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
-import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.*;
-import static org.junit.Assert.assertNotEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertThat;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
 
 public class DnsNameResolverTest {
 
@@ -1746,8 +1740,7 @@ public class DnsNameResolverTest {
                     String hostname, DnsRecord[] additionals, Throwable cause, EventLoop loop) {
                 return null;
             }
-        })
+        }).authoritativeDnsServerCache(new DnsCache() {
-        .authoritativeDnsServerCache(new DnsCache() {
             @Override
             public void clear() {
                 authoritativeLatch.countDown();
@@ -1779,4 +1772,53 @@ public class DnsNameResolverTest {
         resolveLatch.await();
         authoritativeLatch.await();
     }
+
+    @Test
+    public void testResolveACachedWithDot() {
+        final DnsCache cache = new DefaultDnsCache();
+        DnsNameResolver resolver = newResolver(ResolvedAddressTypes.IPV4_ONLY)
+                .resolveCache(cache).build();
+
+        try {
+            String domain = DOMAINS.iterator().next();
+            String domainWithDot = domain + '.';
+
+            resolver.resolve(domain).syncUninterruptibly();
+            List<? extends DnsCacheEntry> cached = cache.get(domain, null);
+            List<? extends DnsCacheEntry> cached2 = cache.get(domainWithDot, null);
+
+            assertEquals(1, cached.size());
+            assertSame(cached, cached2);
+        } finally {
+            resolver.close();
+        }
+    }
+
+    @Test
+    public void testResolveACachedWithDotSearchDomain() throws Exception {
+        final TestDnsCache cache = new TestDnsCache(new DefaultDnsCache());
+        TestDnsServer server = new TestDnsServer(Collections.singleton("test.netty.io"));
+        server.start();
+        DnsNameResolver resolver = newResolver(ResolvedAddressTypes.IPV4_ONLY)
+                .searchDomains(Collections.singletonList("netty.io"))
+                .nameServerProvider(new SingletonDnsServerAddressStreamProvider(server.localAddress()))
+                .resolveCache(cache).build();
+        try {
+            resolver.resolve("test").syncUninterruptibly();
+
+            assertNull(cache.cacheHits.get("test.netty.io"));
+
+            List<? extends DnsCacheEntry> cached = cache.cache.get("test.netty.io", null);
+            List<? extends DnsCacheEntry> cached2 = cache.cache.get("test.netty.io.", null);
+            assertEquals(1, cached.size());
+            assertSame(cached, cached2);
+
+            resolver.resolve("test").syncUninterruptibly();
+            List<? extends DnsCacheEntry> entries = cache.cacheHits.get("test.netty.io");
+            assertFalse(entries.isEmpty());
+        } finally {
+            resolver.close();
+            server.stop();
+        }
+    }
 }