From f22781f176ae5bee3f2886783586f6abfdda5faf Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 10 Aug 2018 08:53:59 +0200 Subject: [PATCH] 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 --- .../netty/resolver/dns/DefaultDnsCache.java | 30 +++++----- .../dns/DnsAddressResolveContext.java | 9 +++ .../netty/resolver/dns/DnsNameResolver.java | 2 +- .../netty/resolver/dns/DnsResolveContext.java | 14 +++-- .../resolver/dns/DefaultDnsCacheTest.java | 26 ++++++++ .../resolver/dns/DnsNameResolverTest.java | 60 ++++++++++++++++--- 6 files changed, 111 insertions(+), 30 deletions(-) diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DefaultDnsCache.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DefaultDnsCache.java index d5431362fe..6116f5c1eb 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DefaultDnsCache.java +++ b/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.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) { - Entries entries = resolveCache.get(e.hostname()); + private void cache0(String hostname, DefaultDnsCacheEntry e, int ttl, EventLoop loop) { + 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 + '.'; + } } diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolveContext.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolveContext.java index 63490efa4a..98c474bbb1 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolveContext.java +++ b/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 { @@ -84,4 +85,12 @@ final class DnsAddressResolveContext extends DnsResolveContext { void cache(String hostname, DnsRecord[] additionals, UnknownHostException cause) { resolveCache.cache(hostname, additionals, cause, parent.ch.eventLoop()); } + + @Override + void doSearchDomainQuery(String hostname, Promise> 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); + } + } } diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java index eaab795d5d..06962f3703 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java +++ b/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> promise, DnsCache resolveCache) { 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 dbb1ab6bbf..b3220c39e8 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 @@ -97,7 +97,7 @@ abstract class DnsResolveContext { private final int dnsClass; private final DnsRecordType[] expectedTypes; private final int maxAllowedQueries; - private final DnsRecord[] additionals; + final DnsRecord[] additionals; private final Set>> queriesInProgress = Collections.newSetFromMap( @@ -164,7 +164,8 @@ abstract class DnsResolveContext { final String initialHostname = startWithoutSearchDomain ? hostname : hostname + '.' + searchDomains[0]; final int initialSearchDomainIdx = startWithoutSearchDomain ? 0 : 1; - doSearchDomainQuery(initialHostname, new FutureListener>() { + final Promise> searchDomainPromise = parent.executor().newPromise(); + searchDomainPromise.addListener(new FutureListener>() { private int searchDomainIdx = initialSearchDomainIdx; @Override public void operationComplete(Future> future) { @@ -175,7 +176,9 @@ abstract class DnsResolveContext { if (DnsNameResolver.isTransportOrTimeoutError(cause)) { promise.tryFailure(new SearchDomainUnknownHostException(cause, hostname)); } else if (searchDomainIdx < searchDomains.length) { - doSearchDomainQuery(hostname + '.' + searchDomains[searchDomainIdx++], this); + Promise> 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 { } } }); + doSearchDomainQuery(initialHostname, searchDomainPromise); } } @@ -213,12 +217,10 @@ abstract class DnsResolveContext { } } - private void doSearchDomainQuery(String hostname, FutureListener> listener) { + void doSearchDomainQuery(String hostname, Promise> nextPromise) { DnsResolveContext nextContext = newResolverContext(parent, hostname, dnsClass, expectedTypes, additionals, nameServerAddrs); - Promise> nextPromise = parent.executor().newPromise(); nextContext.internalResolve(nextPromise); - nextPromise.addListener(listener); } private void internalResolve(Promise> promise) { diff --git a/resolver-dns/src/test/java/io/netty/resolver/dns/DefaultDnsCacheTest.java b/resolver-dns/src/test/java/io/netty/resolver/dns/DefaultDnsCacheTest.java index 17aba98e3c..d3b2384c71 100644 --- a/resolver-dns/src/test/java/io/netty/resolver/dns/DefaultDnsCacheTest.java +++ b/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 entries = cache.get("netty.io", null); + assertEquals(2, entries.size()); + assertEntry(entries.get(0), addr1); + assertEntry(entries.get(1), addr2); + + List entries2 = cache.get("netty.io.", null); + assertEquals(2, entries2.size()); + assertEntry(entries2.get(0), addr1); + assertEntry(entries2.get(1), addr2); + } finally { + group.shutdownGracefully(); + } + } } 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 e9883e6ed1..e1b3c051b0 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 @@ -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.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; +import static org.junit.Assert.*; 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 cached = cache.get(domain, null); + List 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 cached = cache.cache.get("test.netty.io", null); + List cached2 = cache.cache.get("test.netty.io.", null); + assertEquals(1, cached.size()); + assertSame(cached, cached2); + + resolver.resolve("test").syncUninterruptibly(); + List entries = cache.cacheHits.get("test.netty.io"); + assertFalse(entries.isEmpty()); + } finally { + resolver.close(); + server.stop(); + } + } }