Skip to content
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

resolver: Compress all DNS answers #173

Merged
merged 3 commits into from Jun 18, 2015
Merged

resolver: Compress all DNS answers #173

merged 3 commits into from Jun 18, 2015

Conversation

tsenart
Copy link
Contributor

@tsenart tsenart commented Jun 16, 2015

This patch makes mesos-dns compress all DNS answers. This improves
performance of many DNS questions which, when compressed, fit in 512
bytes. See https://tools.ietf.org/html/rfc1035#section-4.2.1

Fixes #170 because the native Go DNS resolver which is enabled when
statically compiling doesn't fallback to TCP when failing to decode
a UDP response larger than 512 bytes.

It isn't a perfect solution since an answer might be larger than 512
bytes even after compression but that's an issue with the client
resolver.

This patch makes mesos-dns compress all DNS answers. This improves
performance of many DNS questions which, when compressed, fit in 512
bytes. See https://tools.ietf.org/html/rfc1035#section-4.2.1

Fixes #170 because the native Go DNS resolver which is enabled when
statically compiling doesn't fallback to TCP when failing to decode
a UDP response larger than 512 bytes.

It isn't a perfect solution since an answer might be larger than 512
bytes even after compression but that's an issue with the client
resolver.
@tsenart tsenart added the PTAL label Jun 16, 2015
@tsenart tsenart assigned tsenart and jdef and unassigned tsenart Jun 16, 2015
@tsenart
Copy link
Contributor Author

tsenart commented Jun 16, 2015

Fixes #170.

@jdef
Copy link
Contributor

jdef commented Jun 16, 2015

My first inclination was to request that you file a miekg/dns issue (or perhaps a PR) to properly set the "truncated" flag on UDP responses (and only on UDP responses) that are greater than 512 bytes in length. From the spec:

Messages carried by UDP are restricted to 512 bytes (not counting the IP
or UDP headers). Longer messages are truncated and the TC bit is set in the header.

In the miekg/dns code the length is not checked and the message is returned as-is:

However it looks like he's rejected the idea of fixing this problem in the lib. So we might want to do the ugly thing suggested here for clients that don't advertise large enough payload size - they'd probably want the TC bit set if information was missing:

And we probably want to allow payloads > 512 without setting the TC bit in some circumstances:

So I think our approach to resolving the problem more broadly needs a little more work. Thoughts?

@jdef jdef added WIP and removed PTAL labels Jun 16, 2015
@jdef jdef assigned tsenart and unassigned jdef Jun 16, 2015
@tsenart tsenart closed this Jun 18, 2015
@tsenart
Copy link
Contributor Author

tsenart commented Jun 18, 2015

@jdef: The pure Go stdlib DNS implementation doesn't implement RFC6891. It also doesn't fallback correctly to TCP even if the TC bit is set in a UDP reply. What we can do on our side to fix #170 is to compress the responses in a best effort policy. For protocol compliance I'll add the code to also set the TC bit.

Now, if we want to support RFC6891 on our side, I think we should open a separate issue for that and wait for miekg/dns#223 to be implemented.

@tsenart tsenart reopened this Jun 18, 2015
@jdef
Copy link
Contributor

jdef commented Jun 18, 2015

Sounds like a plan. Let's stop the bleeding with best effort compress and
set TC bit (if the compressed size > 512). And then file a separate issue
to track the miekg/dns issue.

On Thu, Jun 18, 2015 at 10:42 AM, Tomás Senart notifications@github.com
wrote:

Reopened #173 #173.


Reply to this email directly or view it on GitHub
#173 (comment).

@tsenart tsenart added PTAL and removed WIP labels Jun 18, 2015
@tsenart tsenart assigned jdef and unassigned tsenart Jun 18, 2015
@tsenart
Copy link
Contributor Author

tsenart commented Jun 18, 2015

PTAL

@@ -349,8 +349,8 @@ func (res *Resolver) HandleNonMesos(w dns.ResponseWriter, r *dns.Msg) {
}
}

err = w.WriteMsg(m)
if err != nil {
m.Compress = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would like to see some TODO comments here describing the pain point and the potential problems that a user will see until we resolve this properly. Perhaps even a reference to tickets 173 and 174.

@jdef
Copy link
Contributor

jdef commented Jun 18, 2015

minor doc nit, otherwise lgtm

@jdef jdef assigned tsenart and unassigned jdef Jun 18, 2015
@jdef jdef added WIP and removed PTAL labels Jun 18, 2015
tsenart added a commit that referenced this pull request Jun 18, 2015
resolver: Compress all DNS answers
@jameshartig
Copy link

Just an update to this since I'm running into this as well:

It also doesn't fallback correctly to TCP even if the TC bit is set in a UDP reply
It does support falling back to TCP if the TC bit is set: https://github.com/golang/go/blob/master/src/net/dnsclient_unix.go#L153

But you have to send back a response of < 512 in order to get to that check. There still hasn't been any traction yet on miekg/dns#223 but you can try and just use the Fit method from skydns: https://github.com/skynetservices/skydns/blob/master/server/msg.go in addition to setting the TC flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker can't parse mesos-dns answers
3 participants