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

Use TCP for weave dns. #1038

Merged
merged 1 commit into from
Feb 26, 2016
Merged

Use TCP for weave dns. #1038

merged 1 commit into from
Feb 26, 2016

Conversation

tomwilkie
Copy link
Contributor

Fixes #1013

@rade
Copy link
Member

rade commented Feb 25, 2016

We really should use golang's resolver. Alas it does not allow you to specify a server.

But if we have to do our own thing, we should at least do it correctly, i.e. try udp first and fall back to tcp if the answer is truncated. It's pretty simple to do and exactly what golang does

@tomwilkie
Copy link
Contributor Author

But if we have to do our own thing, we should at least do it correctly, i.e. try udp first and fall back to tcp if the answer is truncated. It's pretty simple to do and exactly what golang does

Why? Whats the downside to just using TCP?

@rade
Copy link
Member

rade commented Feb 25, 2016

Why? Whats the downside to just using TCP?

Protocol compliance. Alas you are off the hook. From RFC 5966:

Regarding the choice of when to use UDP or TCP, Section 6.1.3.2 of RFC 1123 also says:

 ... a DNS resolver or server that is sending a non-zone-transfer query MUST send a UDP query first.

That requirement is hereby relaxed. A resolver SHOULD send a UDP query first, but MAY elect to send a TCP query instead if it has good reason to expect the response would be truncated if it were sent over UDP (with or without EDNS0) or for other operational reasons, in particular, if it already has an open TCP connection to the server.

@2opremio
Copy link
Contributor

@errordeveloper Can you test this? I have pushed an image called weaveworks/scope:1038-dns-tcp . You will also have to change the version in the scope script (I think you already know the drill ... )

@tomwilkie
Copy link
Contributor Author

@errordeveloper reports this worked on slack. Ilya can you merge when you're happy?

@errordeveloper
Copy link
Contributor

I will run a few more tests today to double-check.

On Thu, 25 Feb 2016 16:42 Tom Wilkie notifications@github.com wrote:

@errordeveloper https://github.com/errordeveloper reports this worked
on slack. Ilya can you merge when you're happy?


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

errordeveloper added a commit that referenced this pull request Feb 26, 2016
@errordeveloper errordeveloper merged commit b0d6078 into master Feb 26, 2016
@errordeveloper errordeveloper deleted the 1013-use-tcp-for-weavedns branch February 26, 2016 11:00
@errordeveloper
Copy link
Contributor

@rade I think we do have a good reason here, as answer is expected to be truncated, as we currently have several records for each host and in #1013 it ended up with 15 records for 5 hosts.

@rade
Copy link
Member

rade commented Feb 26, 2016

I think we do have a good reason here

yes, which is why I said "you are off the hook".

davkal added a commit that referenced this pull request Feb 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants