Respect DNS TTL #1085

bookmoons · 2019-07-18T09:04:13Z

Updates DNS resolution to respect TTL.

Replaces the viki-org/dnscache DNS resolver with domainr/dnsr which supports TTL.

This adds several packages to vendor. All of them are deps of the underlying miekg/dns package.

This is a larger piece of logic than I originally described. There are some kinks to work out, I think because the new resolver returns some different errors. But I wanted to ask for feedback before getting into that, in case this is further than you really want to go.

Closes #726.

Replaces the viki-org/dnscache DNS resolver with domainr/dnsr. The new resolver respects DNS record TTLs.

Package miekg/dns requires the new version.

CLAassistant · 2019-07-18T09:04:17Z

All committers have signed the CLA.

bookmoons · 2019-07-18T09:05:04Z

The current setup does some things dnsr doesn't:

Support both IP versions. Subsequent queries pull whichever version was observed from cache.
Accept an IP address string. Resolves to the represented address.
Follow CNAME chains. dnsr returns the CNAME and seems to not to cache them.
Detect IP version support, so that eg it doesn't return IPv6 if the system doesn't support it.

I've tried to preserve all of this. IP versions are detected by init(). IP address strings are detected and parsed. Last observed IP version is noted and preferred in subsequent queries. CNAME chains are followed and cached.

Those last 2 minimize network exchanges.

If you support v4 and v6 and resolve something to v4, you don't want to keep trying for v6 after that. You'll hit network every time. Instead it sticks with v4 until it expires then prefers v4 in the cache refresh, since it will probably still be v4.
CNAME records are cached. Cache for the full chain is traversed before hitting any network. Records expire separately. So if they have different TTLs you resolve as much from cache as possible then refresh the tail bits from network.

I could write up a summary of the logic if it would be useful.

golangcibot · 2019-07-18T09:05:33Z

lib/netext/dialer.go

+// findIP6 returns the first IPv6 address found in rrs.
+// Alternately returns a CNAME record if found.
+func findIP6(rrs []dnsr.RR) (net.IP, *dnsr.RR) {
+	var cname *dnsr.RR = nil


should drop = nil from declaration of var cname; it is the zero value (from golint)

golangcibot · 2019-07-18T09:05:33Z

lib/netext/dialer.go

+// findIP4 returns the first IPv4 address found in rrs.
+// Alternately returns a CNAME record if found.
+func findIP4(rrs []dnsr.RR) (net.IP, *dnsr.RR) {
+	var cname *dnsr.RR = nil


should drop = nil from declaration of var cname; it is the zero value (from golint)

golangcibot · 2019-07-18T09:05:34Z

lib/netext/dialer.go

+func findIP6(rrs []dnsr.RR) (net.IP, *dnsr.RR) {
+	var cname *dnsr.RR = nil
+	for _, rr := range rrs {
+		ip := extractIP6(&rr)


Using a reference for the variable on range scope rr (from scopelint)

golangcibot · 2019-07-18T09:05:34Z

lib/netext/dialer.go

+			return ip, nil
+		}
+		if rr.Type == "CNAME" {
+			cname = &rr


Using a reference for the variable on range scope rr (from scopelint)

golangcibot · 2019-07-18T09:05:34Z

lib/netext/dialer.go

+func findIP4(rrs []dnsr.RR) (net.IP, *dnsr.RR) {
+	var cname *dnsr.RR = nil
+	for _, rr := range rrs {
+		ip := extractIP4(&rr)


Using a reference for the variable on range scope rr (from scopelint)

golangcibot · 2019-07-18T09:05:34Z

lib/netext/dialer.go

+			return ip, nil
+		}
+		if rr.Type == "CNAME" {
+			cname = &rr


Using a reference for the variable on range scope rr (from scopelint)

golangcibot · 2019-07-18T09:05:35Z

lib/netext/dialer.go

 )

+var ip6 bool


ip6 is a global variable (from gochecknoglobals)

golangcibot · 2019-07-18T09:05:35Z

lib/netext/dialer.go

 )

+var ip6 bool
+var ip4 bool


ip4 is a global variable (from gochecknoglobals)

mstoykov

Very sorry for the long delay, we were fixing issues in order to release v0.25.0 and it took way longer than expected ;(

Looking at the code ... it seems okay, and I don't see how else we could use dnsr/dns and if there is no alternative we will need at least 80% of this code either way. I left some comments on the current implementation, and will make a more full review looking through all the dns logic in more details (possibly with some rfcs) when you finish it ... or at least the tests pass :).

I would also prefer if this is code is very well tested ... although I don't know how hard that will be.

Also we do intend to add dns testing ( #851 ), so while definitely a lot will need to change for that, can you maybe refactor all the dns stuff to not be directly on the Dialer but possibly on a Resolver type ? This way we will possibly have less to refactor than and I think it will be better overall

mstoykov · 2019-08-08T10:11:51Z

Gopkg.lock

@@ -3,595 +3,481 @@

 [[projects]]


I have no idea why all the changes to this file has happened, but I would prefer if they are reverted ;) We will be moving to go modules #1070 as soon as we move to 1.13, so even if this is because dep changed we prefer if we don't have unnecessary amount of changes ...

mstoykov · 2019-08-08T10:20:09Z

lib/netext/dialer.go

+}
+
+// detectInterfaces detects available IP network versions.
+func detectInterfaces() {


I think this should be moved to some initialization of each Dialer , and put the ip6,ip4 globalvars as fields of the Dialer. You could use sync.Once if no good place for initialization exists, although I doubt this

mstoykov · 2019-08-08T10:28:36Z

lib/netext/dialer.go

 // NewDialer constructs a new Dialer and initializes its cache.
 func NewDialer(dialer net.Dialer) *Dialer {
 	return &Dialer{
 		Dialer:   dialer,
-		Resolver: dnscache.New(0),
+		Resolver: dnsr.NewExpiring(0),


here 0 means 1000 given the current code of dnsr. Can you either add a comment for this or actually put a 1000?

bookmoons · 2019-08-08T18:28:08Z

I know how it is. You can never tell how long these things will take. Thanks for looking at it.

Will go over all of this soon.

bookmoons · 2019-08-14T01:57:44Z

Closing this in favor of the new approach discussed offthread.

bookmoons added 14 commits July 17, 2019 05:56

Resolve names with dnsr

d6d25c4

Replaces the viki-org/dnscache DNS resolver with domainr/dnsr. The new resolver respects DNS record TTLs.

runner: Update resolver

a590076

dialer: Support IP address host

3aef2bf

Upgrade ed25519

57536e0

Package miekg/dns requires the new version.

dialer: Rename fetch lookup

b0210ce

dialer: Rename translate resolve

58f92f2

dialer: Rename Metacache IP4

3b9dc62

dialer: Update lookup name in tests

2f6b7ae

dialer: Rename TestResolution TestLookup

4671a77

dialer: Handle nonrequested RR types

a0dfb20

dialer: Improve resolve description

2d7b8a7

dialer: Follow CNAME chains

adbf07e

dialer: Detect available IP versions

9d4b56e

dialer: Reset interfaces config after tests

dfdeacd

golangcibot reviewed Jul 18, 2019

View reviewed changes

bookmoons mentioned this pull request Jul 18, 2019

Unexpected DNS caching #726

Closed

mstoykov requested changes Aug 8, 2019

View reviewed changes

bookmoons closed this Aug 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect DNS TTL #1085

Respect DNS TTL #1085

bookmoons commented Jul 18, 2019

CLAassistant commented Jul 18, 2019 •

edited

Loading

bookmoons commented Jul 18, 2019

golangcibot Jul 18, 2019

golangcibot Jul 18, 2019

golangcibot Jul 18, 2019

golangcibot Jul 18, 2019

golangcibot Jul 18, 2019

golangcibot Jul 18, 2019

golangcibot Jul 18, 2019

golangcibot Jul 18, 2019

mstoykov left a comment

mstoykov Aug 8, 2019

mstoykov Aug 8, 2019

mstoykov Aug 8, 2019

bookmoons commented Aug 8, 2019

bookmoons commented Aug 14, 2019

Respect DNS TTL #1085

Respect DNS TTL #1085

Conversation

bookmoons commented Jul 18, 2019

CLAassistant commented Jul 18, 2019 • edited Loading

bookmoons commented Jul 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bookmoons commented Aug 8, 2019

bookmoons commented Aug 14, 2019

CLAassistant commented Jul 18, 2019 •

edited

Loading