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

DNS Recurse Truncation #2384

Closed
christianang opened this issue Oct 3, 2016 · 4 comments · Fixed by #2467
Closed

DNS Recurse Truncation #2384

christianang opened this issue Oct 3, 2016 · 4 comments · Fixed by #2467
Labels
type/bug Feature does not function as expected

Comments

@christianang
Copy link

Currently it looks like dns recurse truncation does not work correctly. When a recursor returns a dns response with the truncation flag set consul returns a SERVFAIL instead of returning the truncated dns response. This means the clients won't get the truncated responses, nor will it attempt to retry the request over TCP.

It looks like miekg/dns returns an ErrTruncate for https://github.com/hashicorp/consul/blob/master/command/agent/dns.go#L850, which turns out to be an error case with a valid message (https://github.com/miekg/dns/blob/master/client.go#L229-L234). We should actually be returning this message if ErrTruncate occurs.

We tested this with consul 0.7.

We can open a PR to fix this if this sounds like acceptable behavior for consul.

-Christian and @kkallday

@Amit-PivotalLabs
Copy link

Hey @slackpad we'll go ahead and submit a PR for this, and are open to discuss in this issue or on the forthcoming PR.

@slackpad slackpad added the type/bug Feature does not function as expected label Oct 12, 2016
@slackpad
Copy link
Contributor

This sounds like a reasonable change - happy to look at a PR - thanks!

@jameshartig
Copy link
Contributor

The original PR I made that lead to that behavior is a good read: miekg/dns#281

The reason we return an error with a valid message is because the message was truncated so it doesn't contain a full response. The request should be retried over TCP since the response was too big for UDP. If you understand that you're not receiving the full response, then you can ignore the error and the message contains as much as we could parse, but that's usually not the case.

@evanfarrar
Copy link

I agree it makes sense for miekg/dns to return an error when a response is truncated, and indeed most consumers should simply ignore the truncated response and escalate to TCP. But that should be a decision for the client library to make.

In the case of recursing though, we probably should ignore this particular error and use the UDP response. While most clients should escalate to TCP, if consul did in this instance it would then need to immediately re-truncate the message for UDP before handing the response to the original requester.

Instead it could return as much as it received from the server it recursed to with the truncation bit set so that the client can know that it should escalate to TCP. When the client escalates, consul recurses in TCP also and can return a full response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Feature does not function as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants