New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly handle DNS redirects for NS servers that have no ADDITIONAL… #8177

Merged
merged 1 commit into from Aug 22, 2018

Conversation

@normanmaurer
Member

normanmaurer commented Aug 7, 2018

… record

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 uncachedRedirectDnsServerList(...)
  • Add unit tests

Result:

Our DnsResolver now correctly handle redirects in all cases.

@normanmaurer normanmaurer requested a review from trustin Aug 7, 2018

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Aug 7, 2018

Member

@slandelle FYI as well...

Member

normanmaurer commented Aug 7, 2018

@slandelle FYI as well...

@normanmaurer normanmaurer self-assigned this Aug 7, 2018

@normanmaurer normanmaurer added the defect label Aug 7, 2018

@normanmaurer normanmaurer added this to the 4.1.29.Final milestone Aug 7, 2018

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Aug 8, 2018

Member

@trustin can you please check again ?

Member

normanmaurer commented Aug 8, 2018

@trustin can you please check again ?

Show outdated Hide outdated ...dns/src/main/java/io/netty/resolver/dns/AuthoritativeDnsServerCache.java
Show outdated Hide outdated .../main/java/io/netty/resolver/dns/DefaultAuthoritativeDnsServerCache.java
Show outdated Hide outdated .../main/java/io/netty/resolver/dns/DefaultAuthoritativeDnsServerCache.java
Show outdated Hide outdated .../main/java/io/netty/resolver/dns/DefaultAuthoritativeDnsServerCache.java
* @param originalTtl the TTL as returned by the DNS server
* @param loop the {@link EventLoop} used to register the TTL timeout
*/
void cache(String hostname, InetSocketAddress address, long originalTtl, EventLoop loop);

This comment has been minimized.

@carl-mastrangelo

carl-mastrangelo Aug 8, 2018

Member

This API is odd, because DNS always gives you back lots of addresses at the same TTL, not one. If you had to call this in a loop, the TTLs might slide. This would be surprising as a user.

Consider taking a list of records for a given TTL.

@carl-mastrangelo

carl-mastrangelo Aug 8, 2018

Member

This API is odd, because DNS always gives you back lots of addresses at the same TTL, not one. If you had to call this in a loop, the TTLs might slide. This would be surprising as a user.

Consider taking a list of records for a given TTL.

This comment has been minimized.

@normanmaurer

normanmaurer Aug 8, 2018

Member

@carl-mastrangelo while the TTL may be the same its not required...

$  dig @192.12.94.30 ns1.google.com

; <<>> DiG 9.10.3-P4-Ubuntu <<>> @192.12.94.30 ns1.google.com
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 57245
;; flags: qr rd; QUERY: 1, ANSWER: 0, AUTHORITY: 4, ADDITIONAL: 9
;; WARNING: recursion requested but not available

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;ns1.google.com.			IN	A

;; AUTHORITY SECTION:
google.com.		172800	IN	NS	ns2.google.com.
google.com.		172800	IN	NS	ns1.google.com.
google.com.		172800	IN	NS	ns3.google.com.
google.com.		172800	IN	NS	ns4.google.com.

;; ADDITIONAL SECTION:
ns2.google.com.		172800	IN	AAAA	2001:4860:4802:34::a
ns2.google.com.		172800	IN	A	216.239.34.10
ns1.google.com.		172800	IN	AAAA	2001:4860:4802:32::a
ns1.google.com.		172800	IN	A	216.239.32.10
ns3.google.com.		172800	IN	AAAA	2001:4860:4802:36::a
ns3.google.com.		172800	IN	A	216.239.36.10
ns4.google.com.		172800	IN	AAAA	2001:4860:4802:38::a
ns4.google.com.		172800	IN	A	216.239.38.10

;; Query time: 16 msec
;; SERVER: 192.12.94.30#53(192.12.94.30)
;; WHEN: Wed Aug 08 19:36:52 CEST 2018
;; MSG SIZE  rcvd: 287

As you can see each of these define their own TTL. Sure its the same here but this is not required by the protocol so we should not make assumptions here.

@normanmaurer

normanmaurer Aug 8, 2018

Member

@carl-mastrangelo while the TTL may be the same its not required...

$  dig @192.12.94.30 ns1.google.com

; <<>> DiG 9.10.3-P4-Ubuntu <<>> @192.12.94.30 ns1.google.com
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 57245
;; flags: qr rd; QUERY: 1, ANSWER: 0, AUTHORITY: 4, ADDITIONAL: 9
;; WARNING: recursion requested but not available

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;ns1.google.com.			IN	A

;; AUTHORITY SECTION:
google.com.		172800	IN	NS	ns2.google.com.
google.com.		172800	IN	NS	ns1.google.com.
google.com.		172800	IN	NS	ns3.google.com.
google.com.		172800	IN	NS	ns4.google.com.

;; ADDITIONAL SECTION:
ns2.google.com.		172800	IN	AAAA	2001:4860:4802:34::a
ns2.google.com.		172800	IN	A	216.239.34.10
ns1.google.com.		172800	IN	AAAA	2001:4860:4802:32::a
ns1.google.com.		172800	IN	A	216.239.32.10
ns3.google.com.		172800	IN	AAAA	2001:4860:4802:36::a
ns3.google.com.		172800	IN	A	216.239.36.10
ns4.google.com.		172800	IN	AAAA	2001:4860:4802:38::a
ns4.google.com.		172800	IN	A	216.239.38.10

;; Query time: 16 msec
;; SERVER: 192.12.94.30#53(192.12.94.30)
;; WHEN: Wed Aug 08 19:36:52 CEST 2018
;; MSG SIZE  rcvd: 287

As you can see each of these define their own TTL. Sure its the same here but this is not required by the protocol so we should not make assumptions here.

This comment has been minimized.

@carl-mastrangelo

carl-mastrangelo Aug 9, 2018

Member

In the case the TTL is the same, I am suggesting that they be added atomically. If the cache is queried right as the scheduled removal is happening, the cache may return partial results. This is what I meant by sliding.

Also, not clear on the expected performance needs here, but I would also assume a single scheduled future is lighter weight than four.

@carl-mastrangelo

carl-mastrangelo Aug 9, 2018

Member

In the case the TTL is the same, I am suggesting that they be added atomically. If the cache is queried right as the scheduled removal is happening, the cache may return partial results. This is what I meant by sliding.

Also, not clear on the expected performance needs here, but I would also assume a single scheduled future is lighter weight than four.

This comment has been minimized.

@trustin

trustin Aug 9, 2018

Member

+1 for consolidating cache records but perhaps we could take care of it in a follow-up PR to fix both DnsCache and AuthoritativeDnsServerCache?

@trustin

trustin Aug 9, 2018

Member

+1 for consolidating cache records but perhaps we could take care of it in a follow-up PR to fix both DnsCache and AuthoritativeDnsServerCache?

This comment has been minimized.

@normanmaurer

normanmaurer Aug 9, 2018

Member

At least in our current implementation it’s not a problem as we just remove all entries once the smallest TTL matches. This may result in more queries then needed but is the safer bet to not end up with partial results imho:

https://github.com/netty/netty/pull/8177/files#diff-7ab1cd3db010ccc37c01987e522675caR178

That said I guess we could also just schedule for the smallest TTL here.

WDYT?

@normanmaurer

normanmaurer Aug 9, 2018

Member

At least in our current implementation it’s not a problem as we just remove all entries once the smallest TTL matches. This may result in more queries then needed but is the safer bet to not end up with partial results imho:

https://github.com/netty/netty/pull/8177/files#diff-7ab1cd3db010ccc37c01987e522675caR178

That said I guess we could also just schedule for the smallest TTL here.

WDYT?

This comment has been minimized.

@normanmaurer

normanmaurer Aug 9, 2018

Member

Here is a PR for the "smallest TTL change" for the DefaultDnsCache:

#8187

Something like this could also be used for the DefaultAuthoritativeDnsServerCache here. Let me know what you think @carl-mastrangelo @trustin

@normanmaurer

normanmaurer Aug 9, 2018

Member

Here is a PR for the "smallest TTL change" for the DefaultDnsCache:

#8187

Something like this could also be used for the DefaultAuthoritativeDnsServerCache here. Let me know what you think @carl-mastrangelo @trustin

This comment has been minimized.

@trustin

trustin Aug 11, 2018

Member

Sounds good to me.

@trustin

trustin Aug 11, 2018

Member

Sounds good to me.

Show outdated Hide outdated .../main/java/io/netty/resolver/dns/DefaultAuthoritativeDnsServerCache.java
@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer
Member

normanmaurer commented Aug 9, 2018

Also @vietj

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Aug 10, 2018

Member

@Scottmitch I think once this and #8187 is merged we can reduce a bit of code duplication in the caches (related to
f9115d1) by creating something that can be used by both internally.

Member

normanmaurer commented Aug 10, 2018

@Scottmitch I think once this and #8187 is merged we can reduce a bit of code duplication in the caches (related to
f9115d1) by creating something that can be used by both internally.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Aug 10, 2018

Member

@carl-mastrangelo can you re-check as well ?

Member

normanmaurer commented Aug 10, 2018

@carl-mastrangelo can you re-check as well ?

@trustin

👍

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Aug 11, 2018

Member

@Scottmitch I replaced List usage by DnsServerAddressStream and addressed also your other comments. Please check again. Also maybe @trustin want to check as well again.

Please note I did not expose our DnsSterverAddressStream implementation to the user that allows unresolved InetSocketAddresses for now as I suspect the user want to write their own implementation anyway if a custom cache is implemented or uncachedRedirectDnsServerStream is overridden.

Member

normanmaurer commented Aug 11, 2018

@Scottmitch I replaced List usage by DnsServerAddressStream and addressed also your other comments. Please check again. Also maybe @trustin want to check as well again.

Please note I did not expose our DnsSterverAddressStream implementation to the user that allows unresolved InetSocketAddresses for now as I suspect the user want to write their own implementation anyway if a custom cache is implemented or uncachedRedirectDnsServerStream is overridden.

do {
InetAddress addr = entries.get(i).address();
addresses.add(new InetSocketAddress(addr, DefaultDnsServerAddressStreamProvider.DNS_PORT));
} while (++i < entries.size());

This comment has been minimized.

@Scottmitch

Scottmitch Aug 14, 2018

Member

do we have to iterate through the list and copy up front? IIUC the list is copy on write in DefaultDnsCache so we shouldn't have to worry about concurrent modifications. Perhaps this isn't strictly guaranteed by the DnsCache interface ... but it would make practical usage of the returned List buggy/volatile if it was being modified under the hood.

@Scottmitch

Scottmitch Aug 14, 2018

Member

do we have to iterate through the list and copy up front? IIUC the list is copy on write in DefaultDnsCache so we shouldn't have to worry about concurrent modifications. Perhaps this isn't strictly guaranteed by the DnsCache interface ... but it would make practical usage of the returned List buggy/volatile if it was being modified under the hood.

This comment has been minimized.

@normanmaurer

normanmaurer Aug 14, 2018

Member

@Scottmitch yeah I didn't want to depend on the semantics of DefaultDnsCache as its an imp detail imho. Also this is just a "translation" layer to keep API breakage to a minimum for now which will be removed later so I would not worry about improving this a lot.

@normanmaurer

normanmaurer Aug 14, 2018

Member

@Scottmitch yeah I didn't want to depend on the semantics of DefaultDnsCache as its an imp detail imho. Also this is just a "translation" layer to keep API breakage to a minimum for now which will be removed later so I would not worry about improving this a lot.

This comment has been minimized.

@Scottmitch

Scottmitch Aug 22, 2018

Member

What would the usage semantics be if the list was modifiable? Seems like it would lead to undefined behavior, no?

@Scottmitch

Scottmitch Aug 22, 2018

Member

What would the usage semantics be if the list was modifiable? Seems like it would lead to undefined behavior, no?

This comment has been minimized.

@normanmaurer

normanmaurer Aug 22, 2018

Member

maybe... not sure. That said I thought better safe then sorry. And again its really just something to give people time to upgrade to the new API, so I am not really worried by some extra copies. If you are super strong on this I can change it tho.

@normanmaurer

normanmaurer Aug 22, 2018

Member

maybe... not sure. That said I thought better safe then sorry. And again its really just something to give people time to upgrade to the new API, so I am not really worried by some extra copies. If you are super strong on this I can change it tho.

Show outdated Hide outdated .../main/java/io/netty/resolver/dns/DefaultAuthoritativeDnsServerCache.java
Show outdated Hide outdated .../main/java/io/netty/resolver/dns/DefaultAuthoritativeDnsServerCache.java
Show outdated Hide outdated .../main/java/io/netty/resolver/dns/DefaultAuthoritativeDnsServerCache.java
Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/DnsServerAddresses.java
Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/NameServerComparator.java
Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/NameServerComparator.java
Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/NameServerComparator.java
Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/NameServerComparator.java
@carl-mastrangelo

This PR is pretty big, and it seems github is making it hard for me to see changes from my last review.

I don't know if I can commit to reviewing it this week, but I can do so next week. (i.e. don't block for me)

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Aug 16, 2018

Member

@carl-mastrangelo sounds good ... :) I mean at worse I can do a follow up. I know its quite big but there was so much stuff broken :(

Member

normanmaurer commented Aug 16, 2018

@carl-mastrangelo sounds good ... :) I mean at worse I can do a follow up. I know its quite big but there was so much stuff broken :(

Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
}
AuthoritativeNameServer server = new AuthoritativeNameServer(serverName);
server.next = serverName.next;
serverName.next = server;

This comment has been minimized.

@Scottmitch

Scottmitch Aug 20, 2018

Member

Say we get DNS results:

1
2
3

IIUC the list order here will be:

1 -> 3 -> 2

should we be preserving insertion order (for more than 2 entries)? If so, this can be accomplished by either:

  1. Instead of having boolean isCopy, you can save a AuthoritativeNameServer lastNsEntry reference which tells you were to insert duplicate NS entries.
  2. Don't save any additional state, and just iterate until serverName.nsName.equalsIgnoreCase(nsName) no longer returns true.

(1) requires some additional state, but saves intermediate comparisons and iteration.

@Scottmitch

Scottmitch Aug 20, 2018

Member

Say we get DNS results:

1
2
3

IIUC the list order here will be:

1 -> 3 -> 2

should we be preserving insertion order (for more than 2 entries)? If so, this can be accomplished by either:

  1. Instead of having boolean isCopy, you can save a AuthoritativeNameServer lastNsEntry reference which tells you were to insert duplicate NS entries.
  2. Don't save any additional state, and just iterate until serverName.nsName.equalsIgnoreCase(nsName) no longer returns true.

(1) requires some additional state, but saves intermediate comparisons and iteration.

This comment has been minimized.

@normanmaurer

normanmaurer Aug 21, 2018

Member

I added a test-case that show prove the current implementation already does what you describe and so works as expected.

@normanmaurer

normanmaurer Aug 21, 2018

Member

I added a test-case that show prove the current implementation already does what you describe and so works as expected.

This comment has been minimized.

@Scottmitch

Scottmitch Aug 22, 2018

Member

yes my mistake. the while loop checking for isCopy should cover it. The reference would save some intermediate iteration but I would expect the number of "copies" to be low shouldn't be significant.

@Scottmitch

Scottmitch Aug 22, 2018

Member

yes my mistake. the while loop checking for isCopy should cover it. The reference would save some intermediate iteration but I would expect the number of "copies" to be low shouldn't be significant.

This comment has been minimized.

@normanmaurer

normanmaurer Aug 22, 2018

Member

yep.. I think it is fine as it is is.

@normanmaurer

normanmaurer Aug 22, 2018

Member

yep.. I think it is fine as it is is.

Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
/**
* Creates a new {@link List} which holds the {@link InetSocketAddress}es.
*/
List<InetSocketAddress> addressList() {

This comment has been minimized.

@Scottmitch

Scottmitch Aug 20, 2018

Member

the current algorithm requires handleAll to make a pass through the entire list. Can we just make that method return the "address list" and avoid another iteration over the list? We can size the ArrayList more conservatively if we want to avoid re-allocations/copies (e.g. leave some space for more entries bcz we don't yet know the total number of entries).

@Scottmitch

Scottmitch Aug 20, 2018

Member

the current algorithm requires handleAll to make a pass through the entire list. Can we just make that method return the "address list" and avoid another iteration over the list? We can size the ArrayList more conservatively if we want to avoid re-allocations/copies (e.g. leave some space for more entries bcz we don't yet know the total number of entries).

This comment has been minimized.

@normanmaurer

normanmaurer Aug 21, 2018

Member

I think I made this better with my latest changes (I removed one iteration).

@normanmaurer

normanmaurer Aug 21, 2018

Member

I think I made this better with my latest changes (I removed one iteration).

This comment has been minimized.

@Scottmitch

Scottmitch Aug 22, 2018

Member

Here is what I had in mind to avoid this additional iteration at the cost of potentially copy/resize of the resulting list. It depends upon the use case but maybe resizing vs always doing another iteration maybe a good tradeoff to start. We didn't even handle the "no-additionals" case before this, so not sure how common this is? We could also add some additional space to make resize less likely if that is a concern.

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 b4c83e4c67..069c97dccf 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
@@ -487,6 +487,12 @@ abstract class DnsResolveContext<T> {
         if (res.count(DnsSection.ANSWER) == 0) {
             AuthoritativeNameServerList serverNames = extractAuthoritativeNameServers(question.name(), res);
             if (serverNames != null) {
+                // Make a best guess for how many elements there will be in the redirect list. This amount is variable
+                // because authority entries with no corresponding additional entries may result in adding a variable
+                // amount of addresses to this list from the cache.
+                List<InetSocketAddress> redirectedNameServerAddresses =
+                        new ArrayList<InetSocketAddress>(serverNames.getNameServerCount());
+                AuthoritativeDnsServerCache authoritativeDnsServerCache = authoritativeDnsServerCache();
                 int additionalCount = res.count(DnsSection.ADDITIONAL);
 
                 for (int i = 0; i < additionalCount; i++) {
@@ -499,17 +505,17 @@ abstract class DnsResolveContext<T> {
 
                     // We may have multiple ADDITIONAL entries for the same nameserver name. For example one AAAA and
                     // one A record.
-                    serverNames.handleWithAdditional(parent, r, authoritativeDnsServerCache());
+                    serverNames.handleWithAdditional(parent, r, authoritativeDnsServerCache,
+                            redirectedNameServerAddresses);
                 }
 
                 // Process all unresolved nameservers as well.
-                serverNames.handleWithoutAdditionals(parent, resolveCache(), authoritativeDnsServerCache());
-
-                List<InetSocketAddress> addresses = serverNames.addressList();
+                serverNames.handleWithoutAdditionals(parent, resolveCache(), authoritativeDnsServerCache,
+                        redirectedNameServerAddresses);
 
                 // Give the user the chance to sort or filter the used servers for the query.
                 DnsServerAddressStream serverStream = parent.newRedirectDnsServerStream(
-                        question.name(), addresses);
+                        question.name(), redirectedNameServerAddresses);
 
                 if (serverStream != null) {
                     query(serverStream, 0, question,
@@ -920,10 +926,10 @@ abstract class DnsResolveContext<T> {
 
         private final String questionName;
 
-        // We not expect the linked-list to be very long so a double-linked-list is overkill.
+        // We do not expect the linked-list to be very long so a double-linked-list is overkill.
         private AuthoritativeNameServer head;
 
-        private int servers;
+        private int nameServerCount;
 
         AuthoritativeNameServerList(String questionName) {
             this.questionName = questionName.toLowerCase(Locale.US);
@@ -967,20 +973,29 @@ abstract class DnsResolveContext<T> {
             // We are only interested in preserving the nameservers which are the closest to our qName, so ensure
             // we drop servers that have a smaller dots count.
             if (head == null || head.dots < dots) {
-                servers = 1;
                 head = new AuthoritativeNameServer(dots, r.timeToLive(), recordName, domainName);
+                nameServerCount = 1;
             } else if (head.dots == dots) {
                 AuthoritativeNameServer serverName = head;
                 while (serverName.next != null) {
                     serverName = serverName.next;
                 }
                 serverName.next = new AuthoritativeNameServer(dots, r.timeToLive(), recordName, domainName);
-                servers++;
+                ++nameServerCount;
             }
         }
 
+        boolean isEmpty() {
+            return head == null;
+        }
+
+        int getNameServerCount() {
+            return nameServerCount;
+        }
+
         void handleWithAdditional(
-                DnsNameResolver parent, DnsRecord r, AuthoritativeDnsServerCache authoritativeCache) {
+                DnsNameResolver parent, DnsRecord r, AuthoritativeDnsServerCache authoritativeCache,
+                List<InetSocketAddress> redirectedNameServerAddresses) {
             // Just walk the linked-list and mark the entry as handled when matched.
             AuthoritativeNameServer serverName = head;
 
@@ -1003,8 +1018,7 @@ abstract class DnsResolveContext<T> {
                         server.next = serverName.next;
                         serverName.next = server;
                         serverName = server;
-
-                        servers++;
+                        ++nameServerCount; // optional, this isn't used after this point
                     }
                     // We should replace the TTL if needed with the one of the ADDITIONAL record so we use
                     // the smallest for caching.
@@ -1012,6 +1026,7 @@ abstract class DnsResolveContext<T> {
 
                     // Cache the server now.
                     cache(serverName, authoritativeCache, parent.executor());
+                    redirectedNameServerAddresses.add(serverName.address);
                     return;
                 }
                 serverName = serverName.next;
@@ -1020,17 +1035,16 @@ abstract class DnsResolveContext<T> {
 
         // Now handle all AuthoritativeNameServer for which we had no ADDITIONAL record
         void handleWithoutAdditionals(
-                DnsNameResolver parent, DnsCache cache, AuthoritativeDnsServerCache authoritativeCache) {
+                DnsNameResolver parent, DnsCache cache, AuthoritativeDnsServerCache authoritativeCache,
+                List<InetSocketAddress> redirectedNameServerAddresses) {
             AuthoritativeNameServer serverName = head;
 
             while (serverName != null) {
                 if (serverName.address == null) {
                     // These will be resolved on the fly if needed.
                     cacheUnresolved(serverName, authoritativeCache, parent.executor());
-                    servers++;
 
                     // Try to resolve via cache as we had no ADDITIONAL entry for the server.
-
                     List<? extends DnsCacheEntry> entries = cache.get(serverName.nsName, null);
                     if (entries != null && !entries.isEmpty()) {
                         InetAddress address = entries.get(0).address();
@@ -1038,6 +1052,7 @@ abstract class DnsResolveContext<T> {
                         // If address is null we have a resolution failure cached so just use an unresolved address.
                         if (address != null) {
                             serverName.update(parent.newRedirectServerAddress(address));
+                            redirectedNameServerAddresses.add(serverName.address);
 
                             for (int i = 1; i < entries.size(); i++) {
                                 address = entries.get(i).address();
@@ -1049,10 +1064,14 @@ abstract class DnsResolveContext<T> {
                                 serverName.next = server;
                                 serverName = server;
                                 serverName.update(parent.newRedirectServerAddress(address));
-
-                                servers++;
+                                ++nameServerCount; // optional, this isn't used after this point
+                                redirectedNameServerAddresses.add(serverName.address);
                             }
+                        } else {
+                            redirectedNameServerAddresses.add(serverName.address);
                         }
+                    } else {
+                        redirectedNameServerAddresses.add(serverName.address);
                     }
                 }
                 serverName = serverName.next;
@@ -1061,10 +1080,6 @@ abstract class DnsResolveContext<T> {
 
         private static void cacheUnresolved(
                 AuthoritativeNameServer server, AuthoritativeDnsServerCache authoritativeCache, EventLoop loop) {
-            // As we have no idea about how long the TTL should be we will just cache the unresolved
-            // address
-
-            // We still want to cached the unresolved address
             server.address = InetSocketAddress.createUnresolved(
                     server.nsName, DefaultDnsServerAddressStreamProvider.DNS_PORT);
 
@@ -1074,40 +1089,9 @@ abstract class DnsResolveContext<T> {
 
         private static void cache(AuthoritativeNameServer server, AuthoritativeDnsServerCache cache, EventLoop loop) {
             // Cache NS record if not for a root server as we should never cache for root servers.
-            if (!server.isRootServer() && server.address != null) {
-                // If we resolved the AuthoritativeNameServer via the DnsCache before we should not cache the
-                // resolved address as we don't have a good idea about the TTL to apply. We will just cache
-                // the unresolved address for now. We will then query the cache again the next time we try to
-                // use it for a query which will do the correct thing in terms of respecting the TTL of the
-                // A / AAAA record.
-                InetSocketAddress addressToCache = server.ttl == Long.MIN_VALUE ?
-                        InetSocketAddress.createUnresolved(server.nsName, server.address.getPort()) :
-                        server.address;
-                cache.cache(server.domainName, addressToCache, server.ttl, loop);
-            }
-        }
-
-        /**
-         * Returns {@code true} if empty, {@code false} otherwise.
-         */
-        boolean isEmpty() {
-            return servers == 0;
-        }
-
-        /**
-         * Creates a new {@link List} which holds the {@link InetSocketAddress}es.
-         */
-        List<InetSocketAddress> addressList() {
-            List<InetSocketAddress> addressList = new ArrayList<InetSocketAddress>(servers);
-
-            AuthoritativeNameServer server = head;
-            while (server != null) {
-                if (server.address != null) {
-                    addressList.add(server.address);
-                }
-                server = server.next;
+            if (!server.isRootServer()) {
+                cache.cache(server.domainName, server.address, server.ttl, loop);
             }
-            return addressList;
         }
     }
@Scottmitch

Scottmitch Aug 22, 2018

Member

Here is what I had in mind to avoid this additional iteration at the cost of potentially copy/resize of the resulting list. It depends upon the use case but maybe resizing vs always doing another iteration maybe a good tradeoff to start. We didn't even handle the "no-additionals" case before this, so not sure how common this is? We could also add some additional space to make resize less likely if that is a concern.

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 b4c83e4c67..069c97dccf 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
@@ -487,6 +487,12 @@ abstract class DnsResolveContext<T> {
         if (res.count(DnsSection.ANSWER) == 0) {
             AuthoritativeNameServerList serverNames = extractAuthoritativeNameServers(question.name(), res);
             if (serverNames != null) {
+                // Make a best guess for how many elements there will be in the redirect list. This amount is variable
+                // because authority entries with no corresponding additional entries may result in adding a variable
+                // amount of addresses to this list from the cache.
+                List<InetSocketAddress> redirectedNameServerAddresses =
+                        new ArrayList<InetSocketAddress>(serverNames.getNameServerCount());
+                AuthoritativeDnsServerCache authoritativeDnsServerCache = authoritativeDnsServerCache();
                 int additionalCount = res.count(DnsSection.ADDITIONAL);
 
                 for (int i = 0; i < additionalCount; i++) {
@@ -499,17 +505,17 @@ abstract class DnsResolveContext<T> {
 
                     // We may have multiple ADDITIONAL entries for the same nameserver name. For example one AAAA and
                     // one A record.
-                    serverNames.handleWithAdditional(parent, r, authoritativeDnsServerCache());
+                    serverNames.handleWithAdditional(parent, r, authoritativeDnsServerCache,
+                            redirectedNameServerAddresses);
                 }
 
                 // Process all unresolved nameservers as well.
-                serverNames.handleWithoutAdditionals(parent, resolveCache(), authoritativeDnsServerCache());
-
-                List<InetSocketAddress> addresses = serverNames.addressList();
+                serverNames.handleWithoutAdditionals(parent, resolveCache(), authoritativeDnsServerCache,
+                        redirectedNameServerAddresses);
 
                 // Give the user the chance to sort or filter the used servers for the query.
                 DnsServerAddressStream serverStream = parent.newRedirectDnsServerStream(
-                        question.name(), addresses);
+                        question.name(), redirectedNameServerAddresses);
 
                 if (serverStream != null) {
                     query(serverStream, 0, question,
@@ -920,10 +926,10 @@ abstract class DnsResolveContext<T> {
 
         private final String questionName;
 
-        // We not expect the linked-list to be very long so a double-linked-list is overkill.
+        // We do not expect the linked-list to be very long so a double-linked-list is overkill.
         private AuthoritativeNameServer head;
 
-        private int servers;
+        private int nameServerCount;
 
         AuthoritativeNameServerList(String questionName) {
             this.questionName = questionName.toLowerCase(Locale.US);
@@ -967,20 +973,29 @@ abstract class DnsResolveContext<T> {
             // We are only interested in preserving the nameservers which are the closest to our qName, so ensure
             // we drop servers that have a smaller dots count.
             if (head == null || head.dots < dots) {
-                servers = 1;
                 head = new AuthoritativeNameServer(dots, r.timeToLive(), recordName, domainName);
+                nameServerCount = 1;
             } else if (head.dots == dots) {
                 AuthoritativeNameServer serverName = head;
                 while (serverName.next != null) {
                     serverName = serverName.next;
                 }
                 serverName.next = new AuthoritativeNameServer(dots, r.timeToLive(), recordName, domainName);
-                servers++;
+                ++nameServerCount;
             }
         }
 
+        boolean isEmpty() {
+            return head == null;
+        }
+
+        int getNameServerCount() {
+            return nameServerCount;
+        }
+
         void handleWithAdditional(
-                DnsNameResolver parent, DnsRecord r, AuthoritativeDnsServerCache authoritativeCache) {
+                DnsNameResolver parent, DnsRecord r, AuthoritativeDnsServerCache authoritativeCache,
+                List<InetSocketAddress> redirectedNameServerAddresses) {
             // Just walk the linked-list and mark the entry as handled when matched.
             AuthoritativeNameServer serverName = head;
 
@@ -1003,8 +1018,7 @@ abstract class DnsResolveContext<T> {
                         server.next = serverName.next;
                         serverName.next = server;
                         serverName = server;
-
-                        servers++;
+                        ++nameServerCount; // optional, this isn't used after this point
                     }
                     // We should replace the TTL if needed with the one of the ADDITIONAL record so we use
                     // the smallest for caching.
@@ -1012,6 +1026,7 @@ abstract class DnsResolveContext<T> {
 
                     // Cache the server now.
                     cache(serverName, authoritativeCache, parent.executor());
+                    redirectedNameServerAddresses.add(serverName.address);
                     return;
                 }
                 serverName = serverName.next;
@@ -1020,17 +1035,16 @@ abstract class DnsResolveContext<T> {
 
         // Now handle all AuthoritativeNameServer for which we had no ADDITIONAL record
         void handleWithoutAdditionals(
-                DnsNameResolver parent, DnsCache cache, AuthoritativeDnsServerCache authoritativeCache) {
+                DnsNameResolver parent, DnsCache cache, AuthoritativeDnsServerCache authoritativeCache,
+                List<InetSocketAddress> redirectedNameServerAddresses) {
             AuthoritativeNameServer serverName = head;
 
             while (serverName != null) {
                 if (serverName.address == null) {
                     // These will be resolved on the fly if needed.
                     cacheUnresolved(serverName, authoritativeCache, parent.executor());
-                    servers++;
 
                     // Try to resolve via cache as we had no ADDITIONAL entry for the server.
-
                     List<? extends DnsCacheEntry> entries = cache.get(serverName.nsName, null);
                     if (entries != null && !entries.isEmpty()) {
                         InetAddress address = entries.get(0).address();
@@ -1038,6 +1052,7 @@ abstract class DnsResolveContext<T> {
                         // If address is null we have a resolution failure cached so just use an unresolved address.
                         if (address != null) {
                             serverName.update(parent.newRedirectServerAddress(address));
+                            redirectedNameServerAddresses.add(serverName.address);
 
                             for (int i = 1; i < entries.size(); i++) {
                                 address = entries.get(i).address();
@@ -1049,10 +1064,14 @@ abstract class DnsResolveContext<T> {
                                 serverName.next = server;
                                 serverName = server;
                                 serverName.update(parent.newRedirectServerAddress(address));
-
-                                servers++;
+                                ++nameServerCount; // optional, this isn't used after this point
+                                redirectedNameServerAddresses.add(serverName.address);
                             }
+                        } else {
+                            redirectedNameServerAddresses.add(serverName.address);
                         }
+                    } else {
+                        redirectedNameServerAddresses.add(serverName.address);
                     }
                 }
                 serverName = serverName.next;
@@ -1061,10 +1080,6 @@ abstract class DnsResolveContext<T> {
 
         private static void cacheUnresolved(
                 AuthoritativeNameServer server, AuthoritativeDnsServerCache authoritativeCache, EventLoop loop) {
-            // As we have no idea about how long the TTL should be we will just cache the unresolved
-            // address
-
-            // We still want to cached the unresolved address
             server.address = InetSocketAddress.createUnresolved(
                     server.nsName, DefaultDnsServerAddressStreamProvider.DNS_PORT);
 
@@ -1074,40 +1089,9 @@ abstract class DnsResolveContext<T> {
 
         private static void cache(AuthoritativeNameServer server, AuthoritativeDnsServerCache cache, EventLoop loop) {
             // Cache NS record if not for a root server as we should never cache for root servers.
-            if (!server.isRootServer() && server.address != null) {
-                // If we resolved the AuthoritativeNameServer via the DnsCache before we should not cache the
-                // resolved address as we don't have a good idea about the TTL to apply. We will just cache
-                // the unresolved address for now. We will then query the cache again the next time we try to
-                // use it for a query which will do the correct thing in terms of respecting the TTL of the
-                // A / AAAA record.
-                InetSocketAddress addressToCache = server.ttl == Long.MIN_VALUE ?
-                        InetSocketAddress.createUnresolved(server.nsName, server.address.getPort()) :
-                        server.address;
-                cache.cache(server.domainName, addressToCache, server.ttl, loop);
-            }
-        }
-
-        /**
-         * Returns {@code true} if empty, {@code false} otherwise.
-         */
-        boolean isEmpty() {
-            return servers == 0;
-        }
-
-        /**
-         * Creates a new {@link List} which holds the {@link InetSocketAddress}es.
-         */
-        List<InetSocketAddress> addressList() {
-            List<InetSocketAddress> addressList = new ArrayList<InetSocketAddress>(servers);
-
-            AuthoritativeNameServer server = head;
-            while (server != null) {
-                if (server.address != null) {
-                    addressList.add(server.address);
-                }
-                server = server.next;
+            if (!server.isRootServer()) {
+                cache.cache(server.domainName, server.address, server.ttl, loop);
             }
-            return addressList;
         }
     }

This comment has been minimized.

@normanmaurer

normanmaurer Aug 22, 2018

Member

So after talking with Scott we will keep it as it is as it often happens that the NS record count is not a good way to size the list.

For example it may see:

; <<>> DiG 9.10.6 <<>> @192.12.94.30 ns1.google.com
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 13399
;; flags: qr rd; QUERY: 1, ANSWER: 0, AUTHORITY: 4, ADDITIONAL: 9
;; WARNING: recursion requested but not available

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;ns1.google.com.			IN	A

;; AUTHORITY SECTION:
google.com.		172800	IN	NS	ns2.google.com.
google.com.		172800	IN	NS	ns1.google.com.
google.com.		172800	IN	NS	ns3.google.com.
google.com.		172800	IN	NS	ns4.google.com.

;; ADDITIONAL SECTION:
ns2.google.com.		172800	IN	AAAA	2001:4860:4802:34::a
ns2.google.com.		172800	IN	A	216.239.34.10
ns1.google.com.		172800	IN	AAAA	2001:4860:4802:32::a
ns1.google.com.		172800	IN	A	216.239.32.10
ns3.google.com.		172800	IN	AAAA	2001:4860:4802:36::a
ns3.google.com.		172800	IN	A	216.239.36.10
ns4.google.com.		172800	IN	AAAA	2001:4860:4802:38::a
ns4.google.com.		172800	IN	A	216.239.38.10
@normanmaurer

normanmaurer Aug 22, 2018

Member

So after talking with Scott we will keep it as it is as it often happens that the NS record count is not a good way to size the list.

For example it may see:

; <<>> DiG 9.10.6 <<>> @192.12.94.30 ns1.google.com
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 13399
;; flags: qr rd; QUERY: 1, ANSWER: 0, AUTHORITY: 4, ADDITIONAL: 9
;; WARNING: recursion requested but not available

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;ns1.google.com.			IN	A

;; AUTHORITY SECTION:
google.com.		172800	IN	NS	ns2.google.com.
google.com.		172800	IN	NS	ns1.google.com.
google.com.		172800	IN	NS	ns3.google.com.
google.com.		172800	IN	NS	ns4.google.com.

;; ADDITIONAL SECTION:
ns2.google.com.		172800	IN	AAAA	2001:4860:4802:34::a
ns2.google.com.		172800	IN	A	216.239.34.10
ns1.google.com.		172800	IN	AAAA	2001:4860:4802:32::a
ns1.google.com.		172800	IN	A	216.239.32.10
ns3.google.com.		172800	IN	AAAA	2001:4860:4802:36::a
ns3.google.com.		172800	IN	A	216.239.36.10
ns4.google.com.		172800	IN	AAAA	2001:4860:4802:38::a
ns4.google.com.		172800	IN	A	216.239.38.10

This comment has been minimized.

@Scottmitch

Scottmitch Aug 22, 2018

Member

fine as is ... just for more context we can also account for the ADDITIONAL count up front too ... but accounting up front will only ever be an estimate and may require resize (tradeoff is maybe resize or over allocation vs always additional iteration).

@Scottmitch

Scottmitch Aug 22, 2018

Member

fine as is ... just for more context we can also account for the ADDITIONAL count up front too ... but accounting up front will only ever be an estimate and may require resize (tradeoff is maybe resize or over allocation vs always additional iteration).

Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer
Member

normanmaurer commented Aug 21, 2018

@Scottmitch PTAL again

do {
InetAddress addr = entries.get(i).address();
addresses.add(new InetSocketAddress(addr, DefaultDnsServerAddressStreamProvider.DNS_PORT));
} while (++i < entries.size());

This comment has been minimized.

@Scottmitch

Scottmitch Aug 22, 2018

Member

What would the usage semantics be if the list was modifiable? Seems like it would lead to undefined behavior, no?

@Scottmitch

Scottmitch Aug 22, 2018

Member

What would the usage semantics be if the list was modifiable? Seems like it would lead to undefined behavior, no?

Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/Cache.java
Show outdated Hide outdated .../main/java/io/netty/resolver/dns/DefaultAuthoritativeDnsServerCache.java
Show outdated Hide outdated .../main/java/io/netty/resolver/dns/DefaultAuthoritativeDnsServerCache.java
}
AuthoritativeNameServer server = new AuthoritativeNameServer(serverName);
server.next = serverName.next;
serverName.next = server;

This comment has been minimized.

@Scottmitch

Scottmitch Aug 22, 2018

Member

yes my mistake. the while loop checking for isCopy should cover it. The reference would save some intermediate iteration but I would expect the number of "copies" to be low shouldn't be significant.

@Scottmitch

Scottmitch Aug 22, 2018

Member

yes my mistake. the while loop checking for isCopy should cover it. The reference would save some intermediate iteration but I would expect the number of "copies" to be low shouldn't be significant.

Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Aug 22, 2018

Member

@Scottmitch ready for another review + added some replies to your questions. I think we are close (hopefully) :)

Member

normanmaurer commented Aug 22, 2018

@Scottmitch ready for another review + added some replies to your questions. I think we are close (hopefully) :)

Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
Show outdated Hide outdated resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java
/**
* Creates a new {@link List} which holds the {@link InetSocketAddress}es.
*/
List<InetSocketAddress> addressList() {

This comment has been minimized.

@Scottmitch

Scottmitch Aug 22, 2018

Member

Here is what I had in mind to avoid this additional iteration at the cost of potentially copy/resize of the resulting list. It depends upon the use case but maybe resizing vs always doing another iteration maybe a good tradeoff to start. We didn't even handle the "no-additionals" case before this, so not sure how common this is? We could also add some additional space to make resize less likely if that is a concern.

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 b4c83e4c67..069c97dccf 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
@@ -487,6 +487,12 @@ abstract class DnsResolveContext<T> {
         if (res.count(DnsSection.ANSWER) == 0) {
             AuthoritativeNameServerList serverNames = extractAuthoritativeNameServers(question.name(), res);
             if (serverNames != null) {
+                // Make a best guess for how many elements there will be in the redirect list. This amount is variable
+                // because authority entries with no corresponding additional entries may result in adding a variable
+                // amount of addresses to this list from the cache.
+                List<InetSocketAddress> redirectedNameServerAddresses =
+                        new ArrayList<InetSocketAddress>(serverNames.getNameServerCount());
+                AuthoritativeDnsServerCache authoritativeDnsServerCache = authoritativeDnsServerCache();
                 int additionalCount = res.count(DnsSection.ADDITIONAL);
 
                 for (int i = 0; i < additionalCount; i++) {
@@ -499,17 +505,17 @@ abstract class DnsResolveContext<T> {
 
                     // We may have multiple ADDITIONAL entries for the same nameserver name. For example one AAAA and
                     // one A record.
-                    serverNames.handleWithAdditional(parent, r, authoritativeDnsServerCache());
+                    serverNames.handleWithAdditional(parent, r, authoritativeDnsServerCache,
+                            redirectedNameServerAddresses);
                 }
 
                 // Process all unresolved nameservers as well.
-                serverNames.handleWithoutAdditionals(parent, resolveCache(), authoritativeDnsServerCache());
-
-                List<InetSocketAddress> addresses = serverNames.addressList();
+                serverNames.handleWithoutAdditionals(parent, resolveCache(), authoritativeDnsServerCache,
+                        redirectedNameServerAddresses);
 
                 // Give the user the chance to sort or filter the used servers for the query.
                 DnsServerAddressStream serverStream = parent.newRedirectDnsServerStream(
-                        question.name(), addresses);
+                        question.name(), redirectedNameServerAddresses);
 
                 if (serverStream != null) {
                     query(serverStream, 0, question,
@@ -920,10 +926,10 @@ abstract class DnsResolveContext<T> {
 
         private final String questionName;
 
-        // We not expect the linked-list to be very long so a double-linked-list is overkill.
+        // We do not expect the linked-list to be very long so a double-linked-list is overkill.
         private AuthoritativeNameServer head;
 
-        private int servers;
+        private int nameServerCount;
 
         AuthoritativeNameServerList(String questionName) {
             this.questionName = questionName.toLowerCase(Locale.US);
@@ -967,20 +973,29 @@ abstract class DnsResolveContext<T> {
             // We are only interested in preserving the nameservers which are the closest to our qName, so ensure
             // we drop servers that have a smaller dots count.
             if (head == null || head.dots < dots) {
-                servers = 1;
                 head = new AuthoritativeNameServer(dots, r.timeToLive(), recordName, domainName);
+                nameServerCount = 1;
             } else if (head.dots == dots) {
                 AuthoritativeNameServer serverName = head;
                 while (serverName.next != null) {
                     serverName = serverName.next;
                 }
                 serverName.next = new AuthoritativeNameServer(dots, r.timeToLive(), recordName, domainName);
-                servers++;
+                ++nameServerCount;
             }
         }
 
+        boolean isEmpty() {
+            return head == null;
+        }
+
+        int getNameServerCount() {
+            return nameServerCount;
+        }
+
         void handleWithAdditional(
-                DnsNameResolver parent, DnsRecord r, AuthoritativeDnsServerCache authoritativeCache) {
+                DnsNameResolver parent, DnsRecord r, AuthoritativeDnsServerCache authoritativeCache,
+                List<InetSocketAddress> redirectedNameServerAddresses) {
             // Just walk the linked-list and mark the entry as handled when matched.
             AuthoritativeNameServer serverName = head;
 
@@ -1003,8 +1018,7 @@ abstract class DnsResolveContext<T> {
                         server.next = serverName.next;
                         serverName.next = server;
                         serverName = server;
-
-                        servers++;
+                        ++nameServerCount; // optional, this isn't used after this point
                     }
                     // We should replace the TTL if needed with the one of the ADDITIONAL record so we use
                     // the smallest for caching.
@@ -1012,6 +1026,7 @@ abstract class DnsResolveContext<T> {
 
                     // Cache the server now.
                     cache(serverName, authoritativeCache, parent.executor());
+                    redirectedNameServerAddresses.add(serverName.address);
                     return;
                 }
                 serverName = serverName.next;
@@ -1020,17 +1035,16 @@ abstract class DnsResolveContext<T> {
 
         // Now handle all AuthoritativeNameServer for which we had no ADDITIONAL record
         void handleWithoutAdditionals(
-                DnsNameResolver parent, DnsCache cache, AuthoritativeDnsServerCache authoritativeCache) {
+                DnsNameResolver parent, DnsCache cache, AuthoritativeDnsServerCache authoritativeCache,
+                List<InetSocketAddress> redirectedNameServerAddresses) {
             AuthoritativeNameServer serverName = head;
 
             while (serverName != null) {
                 if (serverName.address == null) {
                     // These will be resolved on the fly if needed.
                     cacheUnresolved(serverName, authoritativeCache, parent.executor());
-                    servers++;
 
                     // Try to resolve via cache as we had no ADDITIONAL entry for the server.
-
                     List<? extends DnsCacheEntry> entries = cache.get(serverName.nsName, null);
                     if (entries != null && !entries.isEmpty()) {
                         InetAddress address = entries.get(0).address();
@@ -1038,6 +1052,7 @@ abstract class DnsResolveContext<T> {
                         // If address is null we have a resolution failure cached so just use an unresolved address.
                         if (address != null) {
                             serverName.update(parent.newRedirectServerAddress(address));
+                            redirectedNameServerAddresses.add(serverName.address);
 
                             for (int i = 1; i < entries.size(); i++) {
                                 address = entries.get(i).address();
@@ -1049,10 +1064,14 @@ abstract class DnsResolveContext<T> {
                                 serverName.next = server;
                                 serverName = server;
                                 serverName.update(parent.newRedirectServerAddress(address));
-
-                                servers++;
+                                ++nameServerCount; // optional, this isn't used after this point
+                                redirectedNameServerAddresses.add(serverName.address);
                             }
+                        } else {
+                            redirectedNameServerAddresses.add(serverName.address);
                         }
+                    } else {
+                        redirectedNameServerAddresses.add(serverName.address);
                     }
                 }
                 serverName = serverName.next;
@@ -1061,10 +1080,6 @@ abstract class DnsResolveContext<T> {
 
         private static void cacheUnresolved(
                 AuthoritativeNameServer server, AuthoritativeDnsServerCache authoritativeCache, EventLoop loop) {
-            // As we have no idea about how long the TTL should be we will just cache the unresolved
-            // address
-
-            // We still want to cached the unresolved address
             server.address = InetSocketAddress.createUnresolved(
                     server.nsName, DefaultDnsServerAddressStreamProvider.DNS_PORT);
 
@@ -1074,40 +1089,9 @@ abstract class DnsResolveContext<T> {
 
         private static void cache(AuthoritativeNameServer server, AuthoritativeDnsServerCache cache, EventLoop loop) {
             // Cache NS record if not for a root server as we should never cache for root servers.
-            if (!server.isRootServer() && server.address != null) {
-                // If we resolved the AuthoritativeNameServer via the DnsCache before we should not cache the
-                // resolved address as we don't have a good idea about the TTL to apply. We will just cache
-                // the unresolved address for now. We will then query the cache again the next time we try to
-                // use it for a query which will do the correct thing in terms of respecting the TTL of the
-                // A / AAAA record.
-                InetSocketAddress addressToCache = server.ttl == Long.MIN_VALUE ?
-                        InetSocketAddress.createUnresolved(server.nsName, server.address.getPort()) :
-                        server.address;
-                cache.cache(server.domainName, addressToCache, server.ttl, loop);
-            }
-        }
-
-        /**
-         * Returns {@code true} if empty, {@code false} otherwise.
-         */
-        boolean isEmpty() {
-            return servers == 0;
-        }
-
-        /**
-         * Creates a new {@link List} which holds the {@link InetSocketAddress}es.
-         */
-        List<InetSocketAddress> addressList() {
-            List<InetSocketAddress> addressList = new ArrayList<InetSocketAddress>(servers);
-
-            AuthoritativeNameServer server = head;
-            while (server != null) {
-                if (server.address != null) {
-                    addressList.add(server.address);
-                }
-                server = server.next;
+            if (!server.isRootServer()) {
+                cache.cache(server.domainName, server.address, server.ttl, loop);
             }
-            return addressList;
         }
     }
@Scottmitch

Scottmitch Aug 22, 2018

Member

Here is what I had in mind to avoid this additional iteration at the cost of potentially copy/resize of the resulting list. It depends upon the use case but maybe resizing vs always doing another iteration maybe a good tradeoff to start. We didn't even handle the "no-additionals" case before this, so not sure how common this is? We could also add some additional space to make resize less likely if that is a concern.

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 b4c83e4c67..069c97dccf 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
@@ -487,6 +487,12 @@ abstract class DnsResolveContext<T> {
         if (res.count(DnsSection.ANSWER) == 0) {
             AuthoritativeNameServerList serverNames = extractAuthoritativeNameServers(question.name(), res);
             if (serverNames != null) {
+                // Make a best guess for how many elements there will be in the redirect list. This amount is variable
+                // because authority entries with no corresponding additional entries may result in adding a variable
+                // amount of addresses to this list from the cache.
+                List<InetSocketAddress> redirectedNameServerAddresses =
+                        new ArrayList<InetSocketAddress>(serverNames.getNameServerCount());
+                AuthoritativeDnsServerCache authoritativeDnsServerCache = authoritativeDnsServerCache();
                 int additionalCount = res.count(DnsSection.ADDITIONAL);
 
                 for (int i = 0; i < additionalCount; i++) {
@@ -499,17 +505,17 @@ abstract class DnsResolveContext<T> {
 
                     // We may have multiple ADDITIONAL entries for the same nameserver name. For example one AAAA and
                     // one A record.
-                    serverNames.handleWithAdditional(parent, r, authoritativeDnsServerCache());
+                    serverNames.handleWithAdditional(parent, r, authoritativeDnsServerCache,
+                            redirectedNameServerAddresses);
                 }
 
                 // Process all unresolved nameservers as well.
-                serverNames.handleWithoutAdditionals(parent, resolveCache(), authoritativeDnsServerCache());
-
-                List<InetSocketAddress> addresses = serverNames.addressList();
+                serverNames.handleWithoutAdditionals(parent, resolveCache(), authoritativeDnsServerCache,
+                        redirectedNameServerAddresses);
 
                 // Give the user the chance to sort or filter the used servers for the query.
                 DnsServerAddressStream serverStream = parent.newRedirectDnsServerStream(
-                        question.name(), addresses);
+                        question.name(), redirectedNameServerAddresses);
 
                 if (serverStream != null) {
                     query(serverStream, 0, question,
@@ -920,10 +926,10 @@ abstract class DnsResolveContext<T> {
 
         private final String questionName;
 
-        // We not expect the linked-list to be very long so a double-linked-list is overkill.
+        // We do not expect the linked-list to be very long so a double-linked-list is overkill.
         private AuthoritativeNameServer head;
 
-        private int servers;
+        private int nameServerCount;
 
         AuthoritativeNameServerList(String questionName) {
             this.questionName = questionName.toLowerCase(Locale.US);
@@ -967,20 +973,29 @@ abstract class DnsResolveContext<T> {
             // We are only interested in preserving the nameservers which are the closest to our qName, so ensure
             // we drop servers that have a smaller dots count.
             if (head == null || head.dots < dots) {
-                servers = 1;
                 head = new AuthoritativeNameServer(dots, r.timeToLive(), recordName, domainName);
+                nameServerCount = 1;
             } else if (head.dots == dots) {
                 AuthoritativeNameServer serverName = head;
                 while (serverName.next != null) {
                     serverName = serverName.next;
                 }
                 serverName.next = new AuthoritativeNameServer(dots, r.timeToLive(), recordName, domainName);
-                servers++;
+                ++nameServerCount;
             }
         }
 
+        boolean isEmpty() {
+            return head == null;
+        }
+
+        int getNameServerCount() {
+            return nameServerCount;
+        }
+
         void handleWithAdditional(
-                DnsNameResolver parent, DnsRecord r, AuthoritativeDnsServerCache authoritativeCache) {
+                DnsNameResolver parent, DnsRecord r, AuthoritativeDnsServerCache authoritativeCache,
+                List<InetSocketAddress> redirectedNameServerAddresses) {
             // Just walk the linked-list and mark the entry as handled when matched.
             AuthoritativeNameServer serverName = head;
 
@@ -1003,8 +1018,7 @@ abstract class DnsResolveContext<T> {
                         server.next = serverName.next;
                         serverName.next = server;
                         serverName = server;
-
-                        servers++;
+                        ++nameServerCount; // optional, this isn't used after this point
                     }
                     // We should replace the TTL if needed with the one of the ADDITIONAL record so we use
                     // the smallest for caching.
@@ -1012,6 +1026,7 @@ abstract class DnsResolveContext<T> {
 
                     // Cache the server now.
                     cache(serverName, authoritativeCache, parent.executor());
+                    redirectedNameServerAddresses.add(serverName.address);
                     return;
                 }
                 serverName = serverName.next;
@@ -1020,17 +1035,16 @@ abstract class DnsResolveContext<T> {
 
         // Now handle all AuthoritativeNameServer for which we had no ADDITIONAL record
         void handleWithoutAdditionals(
-                DnsNameResolver parent, DnsCache cache, AuthoritativeDnsServerCache authoritativeCache) {
+                DnsNameResolver parent, DnsCache cache, AuthoritativeDnsServerCache authoritativeCache,
+                List<InetSocketAddress> redirectedNameServerAddresses) {
             AuthoritativeNameServer serverName = head;
 
             while (serverName != null) {
                 if (serverName.address == null) {
                     // These will be resolved on the fly if needed.
                     cacheUnresolved(serverName, authoritativeCache, parent.executor());
-                    servers++;
 
                     // Try to resolve via cache as we had no ADDITIONAL entry for the server.
-
                     List<? extends DnsCacheEntry> entries = cache.get(serverName.nsName, null);
                     if (entries != null && !entries.isEmpty()) {
                         InetAddress address = entries.get(0).address();
@@ -1038,6 +1052,7 @@ abstract class DnsResolveContext<T> {
                         // If address is null we have a resolution failure cached so just use an unresolved address.
                         if (address != null) {
                             serverName.update(parent.newRedirectServerAddress(address));
+                            redirectedNameServerAddresses.add(serverName.address);
 
                             for (int i = 1; i < entries.size(); i++) {
                                 address = entries.get(i).address();
@@ -1049,10 +1064,14 @@ abstract class DnsResolveContext<T> {
                                 serverName.next = server;
                                 serverName = server;
                                 serverName.update(parent.newRedirectServerAddress(address));
-
-                                servers++;
+                                ++nameServerCount; // optional, this isn't used after this point
+                                redirectedNameServerAddresses.add(serverName.address);
                             }
+                        } else {
+                            redirectedNameServerAddresses.add(serverName.address);
                         }
+                    } else {
+                        redirectedNameServerAddresses.add(serverName.address);
                     }
                 }
                 serverName = serverName.next;
@@ -1061,10 +1080,6 @@ abstract class DnsResolveContext<T> {
 
         private static void cacheUnresolved(
                 AuthoritativeNameServer server, AuthoritativeDnsServerCache authoritativeCache, EventLoop loop) {
-            // As we have no idea about how long the TTL should be we will just cache the unresolved
-            // address
-
-            // We still want to cached the unresolved address
             server.address = InetSocketAddress.createUnresolved(
                     server.nsName, DefaultDnsServerAddressStreamProvider.DNS_PORT);
 
@@ -1074,40 +1089,9 @@ abstract class DnsResolveContext<T> {
 
         private static void cache(AuthoritativeNameServer server, AuthoritativeDnsServerCache cache, EventLoop loop) {
             // Cache NS record if not for a root server as we should never cache for root servers.
-            if (!server.isRootServer() && server.address != null) {
-                // If we resolved the AuthoritativeNameServer via the DnsCache before we should not cache the
-                // resolved address as we don't have a good idea about the TTL to apply. We will just cache
-                // the unresolved address for now. We will then query the cache again the next time we try to
-                // use it for a query which will do the correct thing in terms of respecting the TTL of the
-                // A / AAAA record.
-                InetSocketAddress addressToCache = server.ttl == Long.MIN_VALUE ?
-                        InetSocketAddress.createUnresolved(server.nsName, server.address.getPort()) :
-                        server.address;
-                cache.cache(server.domainName, addressToCache, server.ttl, loop);
-            }
-        }
-
-        /**
-         * Returns {@code true} if empty, {@code false} otherwise.
-         */
-        boolean isEmpty() {
-            return servers == 0;
-        }
-
-        /**
-         * Creates a new {@link List} which holds the {@link InetSocketAddress}es.
-         */
-        List<InetSocketAddress> addressList() {
-            List<InetSocketAddress> addressList = new ArrayList<InetSocketAddress>(servers);
-
-            AuthoritativeNameServer server = head;
-            while (server != null) {
-                if (server.address != null) {
-                    addressList.add(server.address);
-                }
-                server = server.next;
+            if (!server.isRootServer()) {
+                cache.cache(server.domainName, server.address, server.ttl, loop);
             }
-            return addressList;
         }
     }
Correctly handle DNS redirects for NS servers that have no ADDITIONAL…
… record

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 uncachedRedirectDnsServerList(...)
- Add unit tests

Result:

Our DnsResolver now correctly handle redirects in all cases.

@normanmaurer normanmaurer merged commit bbb6e12 into 4.1 Aug 22, 2018

1 check passed

continuous-integration/teamcity Finished TeamCity Build pull requests :: netty : Tests passed: 13886, ignored: 129
Details

@normanmaurer normanmaurer deleted the dns_fix branch Aug 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment