Motivation
Applications should not depend on internal packages with Java 9 and later. This cause a warning now, but will break in future versions of Java.
Modification
This change adds methods to UnixResolverDnsServerAddressStreamProvider (following after #6844) that parse /etc/resolv.conf for domain and search entries. Then DnsNameResolver does not need to rely on sun.net.dns.ResolverConfiguration to do this.
Result
Fixes#8318. Furthermore, at least in my testing with Java 11, this also makes multiple search entries work properly (previously I was only getting the first entry).
Motivation:
We should not try to cast the Channel to a DatagramChannel as this will cause a ClassCastException.
Modifications:
- Do not cast
- rethrow from constructor if we detect the registration failed.
- Add unit test.
Result:
Propagate correct exception.
Motiviation:
We incorrectly did ignore NS servers during redirect which had no ADDITIONAL record. This could at worse have the affect that we failed the query completely as none of the NS servers had a ADDITIONAL record. Beside this using a DnsCache to cache authoritative nameservers does not work in practise as we we need different features and semantics when cache these servers (for example we also want to cache unresolved nameservers and resolve these on the fly when needed).
Modifications:
- Correctly take NS records into account that have no matching ADDITIONAL record
- Correctly handle multiple ADDITIONAL records for the same NS record
- Introduce AuthoritativeDnsServerCache as a replacement of the DnsCache when caching authoritative nameservers + adding default implementation
- Add an adapter layer to reduce API breakage as much as possible
- Replace DnsNameResolver.uncachedRedirectDnsServerStream(...) with newRedirectDnsServerStream(...)
- Add unit tests
Result:
Our DnsResolver now correctly handle redirects in all cases.
Motivation:
We are currently always remove all entries from the cache for a hostname if the lowest TTL was reached but schedule one for each of the cached entries. This is wasteful.
Modifications:
- Reimplement logic to schedule TTL to only schedule a new removal task if the requested TTL was actual lower then the one for the already scheduled task.
- Ensure we only remove from the internal map if we did not replace the Entries in the meantime.
Result:
Less overhead in terms of scheduled tasks for the DefaultDnsCache
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
Motivation:
55fec94592 fixed a bug where we did not correctly clear all caches when the resolver was closed but did not add a testcase.
Modifications:
Add testcase.
Result:
More tests.
Motivation:
DnsNameResolver manages search domains and will retry the request with the different search domains provided to it. However if the query results in an invalid hostname, the Future corresponding to the resolve request will never be completed.
Modifications:
- If a resolve attempt results in an invalid hostname and the query isn't issued we should fail the associated promise
Result:
No more hang from DnsNameResolver if search domain results in invalid hostname.
Motivation:
At the moment we only clear the resolveCache when the Channel is closed. We should also do the same for the authoritativeDnsServerCache.
Modifications:
Add authoritativeDnsServerCache.clear() to the Channel closeFuture.
Result:
Correctly clear all caches.
Motivation:
We did not handle the case when the query was cancelled which could lead to an exhausted id space. Beside this we did not not cancel the timeout on failed promises.
Modifications:
- Do the removal of the id from the manager in a FutureListener so its handled in all cases.
- Cancel the timeout whenever the original promise was full-filled.
Result:
Fixes https://github.com/netty/netty/issues/8013.
Motivation:
Whenever we fail the query we should also remove the id from the DnsQueryContextManager.
Modifications:
Remove the id from the DnsQueryContextManager if we fail the query because the channel failed to become active.
Result:
More correct code.
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.
Motivation:
a598c3b69b added a upper limit for ttl but missed to also do the same for minTtl.
Modifications:
- Add upper limit for minTtl
- Add testcase.
Result:
No more IllegalArgumentException possible.
Motivation:
Due a bug we did never store more then one address per hostname in DefaultDnsCache.
Modifications:
- Correctly store multiple entries per hostname
- Add tests
Result:
DefaultDnsCache correctly stores more then one entry. Also fixes https://github.com/netty/netty/issues/7882 .
Motivation:
In b47fb81799 we limited the max supported delay to match what our internal implementat can support. Because of this it was possible that DefaultDnsCache produced an IllegalArgumentException when it tried to schedule a expiration > 3 years.
Modifications:
Limit the max supported TTL to 2 years which is safe for all our EventLoop implementations.
Result:
No more exceptions when adding records to the cache with a huge TTL.
Motivation:
Right now to customize DNS name resolver when using DnsAddressResolverGroup
one should subclass implementation and override newNameResolver method when
in fact it's possible to collect all settings in a DnsNameResolverBuilder
instance. Described in #7749.
Modifications:
- Added new constructor for DnsNameResolverBuilder in order to delay
EventLoop specification
- Added copy() method to DnsNameResolverBuilder to provide an immutable
copy of the builder
- Added new single-argument constructor for DnsAddressResolverGroup and
RoundRobinDnsAddressResolverGroup accepting DnsNameResolverBuilder
instance
- DnsAddressResolverGroup to build a new resolver using DnsNameResolverBuilder
given instead of creating a new one
- Test cases to check that changing channelFactory after the builder was passed
to create a DnsNameResolverGroup would not propagate to the name resolver
Result:
Much easier to customize DNS settings w/o subclassing DnsAddressResolverGroup
Motivation:
Currently, if a DNS server returns a non-preferred address type before the preferred one, then both will be returned as the result, and when only taking a single one, this usually ends up being the non-preferred type. However, the JDK requires lookups to only return the preferred type when possible to allow for backwards compatibility.
To allow a client to be able to resolve the appropriate address when running on a machine that does not support IPv6 but the DNS server returns IPv6 addresses before IPv4 addresses when querying.
Modification:
Filter the returned records to the expected type when both types are present.
Result:
Allows a client to run on a machine with IPv6 disabled even when a server returns both IPv4 and IPv6 results. Netty-based code can be a drop-in replacement for JDK-based code in such circumstances.
This PR filters results before returning them to respect JDK expectations.
* Add DnsNameResolver.resolveAll(DnsQuestion)
Motivation:
A user is currently expected to use DnsNameResolver.query() when he or
she wants to look up the full DNS records rather than just InetAddres.
However, query() only performs a single query. It does not handle
/etc/hosts file, redirection, CNAMEs or multiple name servers.
As a result, such a user has to duplicate all the logic in
DnsNameResolverContext.
Modifications:
- Refactor DnsNameResolverContext so that it can send queries for
arbitrary record types.
- Rename DnsNameResolverContext to DnsResolveContext
- Add DnsAddressResolveContext which extends DnsResolveContext for
A/AAAA lookup
- Add DnsRecordResolveContext which extends DnsResolveContext for
arbitrary lookup
- Add DnsNameResolverContext.resolveAll(DnsQuestion) and its variants
- Change DnsNameResolverContext.resolve() delegates the resolve request
to resolveAll() for simplicity
- Move the code that decodes A/AAAA record content to DnsAddressDecoder
Result:
- Fixes#7795
- A user does not have to duplicate DnsNameResolverContext in his or her
own code to implement the usual DNS resolver behavior.
Motivation:
When we do DNS queries we need to ensure we always release the AddressEnvelope.
Modifications:
Also release the AddressEnvelope if the original resolution was done in the meantime and we did not cancel the extra query yet.
Result:
Should fix [#7713]
Motivation:
When following a CNAME response DnsNameResovlerContext may issue a A and AAAA query. However the DnsNameResolverContext would have already issued a A and AAAA query to get the CNAME response, and this may result in 2 additional A/AAAA queries per CNAME response.
Modifications:
- DnsNameResovlerContext#followCname shouldn't issue 2 queries, but instead just a single query with the same record type as the original query
Result:
No more duplicate queries as a result of CNAME responses.
Motivation:
DnsServerAddressStream provides an iterator like interface but maybe expected to start at a specific point upon each new usage. If a DnsServerAddressStream is re-used in multiple independent iterations the order of iteration maybe incorrect. DnsNameResolverContext has a fallback DnsServerAddressStream reference if the cache doesn't contain a hit, but it is shared across multiple independent iterations. This may lead to undesirable DNS query order.
Modifications:
- DnsNameResolverContext#getNameServers should duplicate the default DnsServerAddressStream
Result:
Consistent iteration over the default DnsServerAddressStream in DnsNameResolverContext.
Motivation:
When following a CNAME it is possible there are multiple name servers to query against. However DnsNameResolverContext#followCname explicitly only uses the first name server address when attempting the query. This may lead to resolution failures because we didn't try all the available name servers.
Modifications:
DnsNameResolverContext#followCname should not just try the first name server, but it should try all name servers
Result:
More complete CNAME resolution.
Motivation:
At the moment DefaultDnsCache will expire each record dependong on its own TTL. This may result in unexpected results for the end-user especially if the user for example uses IPV4_PREFERED but the cached AAAA records has a higher TTL then the A records and so the A record was removed. In this case we would only return the AAAA record and not even try to refresh.
Modifications:
Always expire all records for a hostname when one TTL is reached.
Result:
Fixes [#7329]
Motivation:
The usage of DnsCache in DnsNameResolver was racy in general. First of the isEmpty() was not called in a synchronized block while we depended on synchronized. The other problem was that this whole synchronization only worked if the DefaultDnsCache was used and the returned List was not wrapped by the user.
Modifications:
- Rewrite DefaultDnsCache to not depend on synchronization on the returned List by using a CoW approach.
Result:
Fixes [#7583] and other races.
Motivation:
We need to ensure we only call List.* methods in the synchronized block as the returned List may not be thread-safe.
Modifications:
Do not call isEmpty() outside of the synchronized block.
Result:
Fixes [#7583]
Motivation:
DnsNameResolverTest has not been updated in a while.
Modifications:
- Update the DOMAINS definition in DnsNameResolverTest
Result:
More current domain names.
Automatic-Module-Name entry provides a stable JDK9 module name, when Netty is used in a modular JDK9 applications. More info: http://blog.joda.org/2017/05/java-se-9-jpms-automatic-modules.html
When Netty migrates to JDK9 in the future, the entry can be replaced by actual module-info descriptor.
Modification:
The POM-s are configured to put the correct module names to the manifest.
Result:
Fixes#7218.
Motivation:
At the moment there is not way for the user to know if resolving a domain was failed because the domain was unkown or because of an IO error / timeout. If it was caused by an timeout / IO error the user may want to retry the query. Also if the query was failed because of an IO error / timeout we should not cache it.
Modifications:
- Add DnsNameResolverTimeoutException and include it in the UnkownHostException if the domain could not be resolved because of an timeout. This will allow the user to retry the query when inspecting the cause.
- Do not cache IO errors / timeouts
- Add unit test
Result:
Easier for users to implement retries for DNS querys and not cache IO errors / timeouts.
Motivation:
At the moment there is not way for the user to know if resolving a domain was failed because the domain was unkown or because of an IO error / timeout. If it was caused by an timeout / IO error the user may want to retry the query. Also if the query was failed because of an IO error / timeout we should not cache it.
Modifications:
- Add DnsNameResolverTimeoutException and include it in the UnkownHostException if the domain could not be resolved because of an timeout. This will allow the user to retry the query when inspecting the cause.
- Do not cache IO errors / timeouts
- Add unit test
Result:
Easier for users to implement retries for DNS querys and not cache IO errors / timeouts.
Motivation:
Minor cleanup from 844d804 just to reduce the conditional statements and indentation level.
Modifications:
- combine the else + if into an else if statement
Result:
Code cleaned up.
Motivation:
DN resolution does not fall back to the "original name" lookup after search list is checked. This results in a failure to resolve any name (outside of search list) that has number of dots less than resolv.conf's ndots value (which, for example, is often the case in the context of Kubernetes where kubelet passes on resolv.conf containing "options ndots:5").
It also does not go through the search list in a situation described in resolv.conf man:
"The default for n[dots] is 1, meaning that if there are any dots in a name, the name will be tried first as an absolute name before any search list elements are appended to it."
Modifications:
DnsNameResolverContext::resolve was updated to match Go's https://github.com/golang/go/blob/release-branch.go1.9/src/net/dnsclient_unix.go#L338 logic.
Result:
DnsNameResolverContext::resolve will now try to resolve "original name" if search list yields no results when number of dots in the original name is less than resolv.conf's ndots value. It will also go through the search list in case "origin name" resolution fails and number of dots is equal or larger than resolv.conf's ndots value.
Motivation:
We should not try to use UnixResolverDnsServerAddressStreamProvider when on Windows as it will log some error that will produce noise and may confuse users.
Modifications:
Just use DefaultDnsServerAddressStreamProvider if windows is used.
Result:
Less noise in the logs. This was reported in vert.x: https://github.com/eclipse/vert.x/issues/2204
Motviation:
DnsNameResolverContext#followCname attempts to build a query to follow a CNAME, but puts the original hostname in the DnsQuery instead of the CNAME hostname. This will result in not following CNAME redirects correctly.
Result:
- DnsNameResolverContext#followCname should use the CNAME instead of the original hostname when building the DnsQuery
Result:
More correct handling of redirect queries.
Motivation:
Even if it's a super micro-optimization (most JVM could optimize such
cases in runtime), in theory (and according to some perf tests) it
may help a bit. It also makes a code more clear and allows you to
access such methods in the test scope directly, without instance of
the class.
Modifications:
Add 'static' modifier for all methods, where it possible. Mostly in
test scope.
Result:
Cleaner code with proper 'static' modifiers.
Motivation:
Without a 'serialVersionUID' field, any change to a class will make
previously serialized versions unreadable.
Modifications:
Add missed 'serialVersionUID' field for all Serializable
classes.
Result:
Proper deserialization of previously serialized objects.
Motivation:
DnsCache (an interface) is coupled to DnsCacheEntry (a final class). This means that DnsCache implementations can't implement their own DnsCacheEntry objects if the default behavior isn't appropriate.
Modifications:
- DnsCacheEntry should be moved to DefaultDnsCache as it is an implementation detail
- DnsCache#cache(..) should return a new DnsCacheEntry
- The methods which from DnsCacheEntry that were used outside the scope of DefaultDnsCache should be moved into an interface
Result:
DnsCache is more extensible and not tightly coupled to a default implementation of DnsCacheEntry.
Motivation:
To be able to build with latest java9 release we need to adjust commons-lang version and maven-enforcer-plugin.
Modifications:
- Use commons-lang 2.6.0
- Use maven-enforcer-plugin 3.0.0.M1 when building with java9
Result:
Netty builds again with latest java9 release
Motivation:
We should not use ipv4 google dns servers if the app is configured to run ipv6.
Modifications:
Use either ipv4 or ipv6 dns servers depending on the system config.
Result:
More correct behaviour
Motivation:
When the hostname portion can not be extracted we should just skip the server as otherwise we will produce and exception when trying to create the InetSocketAddress.
This was happing when trying to run the test-suite on a system and using java7:
java.lang.IllegalArgumentException: hostname can't be null
at java.net.InetSocketAddress.checkHost(InetSocketAddress.java:149)
at java.net.InetSocketAddress.<init>(InetSocketAddress.java:216)
at io.netty.util.internal.SocketUtils$10.run(SocketUtils.java:171)
at io.netty.util.internal.SocketUtils$10.run(SocketUtils.java:168)
at java.security.AccessController.doPrivileged(Native Method)
at io.netty.util.internal.SocketUtils.socketAddress(SocketUtils.java:168)
at io.netty.resolver.dns.DefaultDnsServerAddressStreamProvider.<clinit>(DefaultDnsServerAddressStreamProvider.java:74)
at io.netty.resolver.dns.DnsServerAddressesTest.testDefaultAddresses(DnsServerAddressesTest.java:39)
Modifications:
Skip if hostname can not be extracted.
Result:
No more java.lang.ExceptionInInitializerError.
Motivation:
JNDI allows to specify an port so we should respect it.
Modifications:
Use the specified port and if none is specifed use 53.
Result:
Correct handling of JNDI configured DNS.
Motivation:
DnsNameResolverTest has been observed to timeout on the CI servers. We should increase the timeout from 5 seconds to 30 seconds.
Modifications:
- Increase timeout from 5 to 30 seconds.
Result:
Less false failures due to slower CI machines.
Motivation:
The DNS resolver may use default configuration inherited from the environment. This means the ndots value may change and result in test failure if the tests don't explicitly set the assumed value.
Modifications:
- Explicitly set ndots in resolver-dns unit tests so we don't fail if the environment overrides the search domain and ndots
Result:
Unit tests are less dependent upon the enviroment they run in.
Fixes https://github.com/netty/netty/issues/6966.
Motivation:
UnixResolverDnsServerAddressStreamProvider currently throws an exception if /etc/resolver exists but it empty. This shouldn't be an exception and can be tolerated as if there is no contribution from /etc/resolver.
Modifications:
- Treat /etc/resolver as present and empty the same as not being present
Result:
UnixResolverDnsServerAddressStreamProvider initialization can tolerate empty /etc/resolver directory.
Motivation:
InetSocketAddress#getHostName() may attempt a reverse lookup which may lead to test failures because the expected address will not match.
Modifications:
- Use InetSocketAddress#getHostString() which will not attempt any lookups and instead return the original String
Result:
UnixResolverDnsServerAddressStreamProviderTest is more reliable.
Motivation:
If there are multiple DNS servers to query Java's DNS resolver will attempt to resolve A and AAAA records in sequential order and will terminate with a failure once all DNS servers have been exhausted. Netty's DNS server will share the same DnsServerAddressStream for the different record types which may send the A question to the first host and the AAAA question to the second host. Netty's DNS resolution also may not progress to the next DNS server in all situations and doesn't have a means to know when resolution has completed.
Modifications:
- DnsServerAddressStream should support new methods to allow the same stream to be used to issue multiple queries (e.g. A and AAAA) against the same host.
- DnsServerAddressStream should support a method to determine when the stream will start to repeat, and therefore a failure can be returned.
- Introduce SequentialDnsServerAddressStreamProvider for sequential use cases
Result:
Fixes https://github.com/netty/netty/issues/6926.
Motivation:
ba80fbbe05 modified the UnknownHostException to not include the search domain if the DNS query failed, but this masks what DNS query actually failed. Have the full hostname (including the search domain) provides more visibility and may help diagnose a configuration error if queries are unexpectedly failing.
Modifications:
- Remove DnsNameResolverContext#pristineHostname
Result:
UnknownHostException is more accurate and reflect what hostname actually resulted in failure.
Motivation:
DnsQueryLifecycleObserver is designed to capture the life cycle of every query. DnsNameResolverContext has a custom trace mechanism which consists of a StringBuilder and manual calls throughout the class. We can remove some special case code in DnsNameResolverContext and instead use a special implementation of DnsQueryLifecycleObserver when trace is enabled.
Modifications:
- Remove all references to the boolean trace variables in DnsNameResolverContext and DnsNameResolver
- Introduce TraceDnsQueryLifecycleObserver which will be used when trace is enabled and will log similar data as what trace currently provides
Result:
Less special case code in DnsNameResolverContext and instead delegate to TraceDnsQueryLifecycleObserver to capture trace information.
Motivation:
The DNS resolver supports search domains. However the ndots are not correctly enforced. The search domain should only be appended under the following scenario [1]:
> Resolver queries having fewer than ndots dots (default is 1) in them will be attempted using each component of the search path in turn until a match is found.
The DNS resolver current appends the search domains if ndots is 0 which should never happen (because no domain can have less than 0 dots).
[1] https://linux.die.net/man/5/resolv.conf
Modifications:
- Parse /etc/resolv.conf to get the default value for ndots on Unix platforms
- The search domain shouldn't be used if ndots is 0
- Avoid failing a promise to trigger the search domain queries in DnsNameResolverContext#resolve
Result:
More correct usage of search domains in the DNS resolver.
Fixes https://github.com/netty/netty/issues/6844.
Motivation:
It’s currently complicated to extend `DnsNameResolver` as the default
value for `searchDomain` is package private.
Modifications:
* let `DnsNameResolver` accept a null `searchDomains` and then default
to `DEFAULT_SEARCH_DOMAINS`, just like it’s being done with
`resolvedAddressTypes`.
* set default `DnsNameResolverBuilder#searchDomains` value to null to
avoid cloning internal `DnsNameResolver.DEFAULT_SEARCH_DOMAINS` in
`DnsNameResolver` constructor.
Result:
More versatile `DnsNameResolver` constructor.
No array copy when using default search domains.
Motivation:
UnixResolverDnsServerAddressStreamProvider allows the default name server address stream to be null, but there should always be a default stream to fall back to ([1] Search Strategy).
UnixResolverDnsServerAddressStreamProvider currently shuffles the names servers are multiple are present, but the defined behavior is to try them sequentially [2].
[1] Search Strategy Section - https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man5/resolver.5.html
[2] DESCRIPTION/nameserver Section - https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man5/resolver.5.html
Modifications:
- UnixResolverDnsServerAddressStreamProvider should always use the first file provided to derive the default domain server address stream. Currently if there are multiple domain names in the file identified by the first argument of the constructor then one will be selected at random.
- UnixResolverDnsServerAddressStreamProvider should return name servers sequentially.
- Reduce access level on some methods which don't have known use-cases externally.
Result:
Fixes https://github.com/netty/netty/issues/6736
Motivation:
IDN.toUnicode(...) removes trailing dots when used in Java7 while it not does on java8.
Modifications:
Check if we should test with the trailing dot removed or not.
Result:
Test pass on Java7 as well.
Motivation:
A single DNS query may follow many different paths through resolver-dns. The query may fail for various reasons related to the DNS protocol, general IO errors, it may be cancelled due to the query count being exceeded, or other reasons. A query may also result in other queries as we follow the DNS protocol (e.g. redirects, CNAME, etc...). It is currently impossible to collect information about the life cycle of an individual query though resolver-dns. This information may be valuable when considering which DNS servers are preferred over others.
Modifications:
- Introduce an interface which can provide visibility into all the potential outcomes of an individual DNS query
Result:
resolver-dns provides visibility into individual DNS queries which can be used to avoid poorly performing DNS servers.
Motivation:
1. The use of InternetProtocolFamily is not consistent:
the DnsNameResolverContext and DnsNameResolver contains switches
instead of appropriate methods usage.
2. The InternetProtocolFamily class contains redundant switches in the
constructor.
Modifications:
1. Replacing switches to the use of an appropriate methods.
2. Simplifying the InternetProtocolFamily constructor.
Result:
Code is cleaner and simpler.
Motivation:
Using reflection to obtain the default name servers may fail in Java9 and also in previous Java versions if a SecurityManager is present.
Modifications:
Try using jndi-dns to obtain default name servers and only try using reflection if this fails.
Result:
Be able to detect default name servers in all cases. Fixes [#6347].
Motivation:
DnsServerAddresses loads the default DNS servers used for DNS resolution in a static initialization block. This is subject to blocking and may cause unexpected delays. We can move this initialization to DefaultDnsServerAddressStreamProvider where it is more expected to load the JDK's default configuration.
Modifications:
- Move all the static initialization from DnsServerAddresses to DefaultDnsServerAddressStreamProvider
- Deprecate static methods in DnsServerAddresses which have moved to DefaultDnsServerAddressStreamProvider
- Remove usage of deprecated methods in DnsServerAddresses
Result:
Usage of JDK's blocking DNS resolver is not required to use resolver-dns.
