code review 12458043: net: Retry DNS with TCP if UDP response is truncated.

net: implement DNS TCP fallback query if UDP response is truncated

Fixes issue 5686.

[axaxs, 2013-08-05 06:55:57]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2013-08-05 13:49:27]

https://codereview.appspot.com/12458043/diff/7001/src/pkg/net/dnsclient_unix.go
File src/pkg/net/dnsclient_unix.go (right):

https://codereview.appspot.com/12458043/diff/7001/src/pkg/net/dnsclient_unix....
src/pkg/net/dnsclient_unix.go:42: //Must add 2 byte length header to front of
msg
space after //

https://codereview.appspot.com/12458043/diff/7001/src/pkg/net/dnsclient_unix....
src/pkg/net/dnsclient_unix.go:45: msg = msg[0 : mlen+2]
this line is doing nothing?

https://codereview.appspot.com/12458043/diff/7001/src/pkg/net/dnsclient_unix....
src/pkg/net/dnsclient_unix.go:60: //TCP will have 2 byte length header
space after //

https://codereview.appspot.com/12458043/diff/7001/src/pkg/net/dnsclient_unix....
src/pkg/net/dnsclient_unix.go:61: lenbuf := make([]byte, 2)
just re-use buf (which hasn't been used yet).  read into buf[:2].

https://codereview.appspot.com/12458043/diff/7001/src/pkg/net/dnsclient_unix....
src/pkg/net/dnsclient_unix.go:62: n, err = c.Read(lenbuf)
Read isn't guaranteed to fill the buffer.

You want io.ReadFull

https://codereview.appspot.com/12458043/diff/7001/src/pkg/net/dnsclient_unix....
src/pkg/net/dnsclient_unix.go:116: if (*msg).truncated {
why not just msg.truncated?

https://codereview.appspot.com/12458043/diff/7001/src/pkg/net/dnsclient_unix....
src/pkg/net/dnsclient_unix.go:168: 
unrelated change

[axaxs, 2013-08-05 19:01:49]

On 2013/08/05 13:49:27, bradfitz wrote:
>
https://codereview.appspot.com/12458043/diff/7001/src/pkg/net/dnsclient_unix.go
> File src/pkg/net/dnsclient_unix.go (right):
> 
>
https://codereview.appspot.com/12458043/diff/7001/src/pkg/net/dnsclient_unix....
> src/pkg/net/dnsclient_unix.go:42: //Must add 2 byte length header to front of
> msg
> space after //
> 
>
https://codereview.appspot.com/12458043/diff/7001/src/pkg/net/dnsclient_unix....
> src/pkg/net/dnsclient_unix.go:45: msg = msg[0 : mlen+2]
> this line is doing nothing?
> 
>
https://codereview.appspot.com/12458043/diff/7001/src/pkg/net/dnsclient_unix....
> src/pkg/net/dnsclient_unix.go:60: //TCP will have 2 byte length header
> space after //
> 
>
https://codereview.appspot.com/12458043/diff/7001/src/pkg/net/dnsclient_unix....
> src/pkg/net/dnsclient_unix.go:61: lenbuf := make([]byte, 2)
> just re-use buf (which hasn't been used yet).  read into buf[:2].
> 
>
https://codereview.appspot.com/12458043/diff/7001/src/pkg/net/dnsclient_unix....
> src/pkg/net/dnsclient_unix.go:62: n, err = c.Read(lenbuf)
> Read isn't guaranteed to fill the buffer.
> 
> You want io.ReadFull
> 
>
https://codereview.appspot.com/12458043/diff/7001/src/pkg/net/dnsclient_unix....
> src/pkg/net/dnsclient_unix.go:116: if (*msg).truncated {
> why not just msg.truncated?
> 
>
https://codereview.appspot.com/12458043/diff/7001/src/pkg/net/dnsclient_unix....
> src/pkg/net/dnsclient_unix.go:168: 
> unrelated change

Thanks Brad.  Please take another look.

Alex

[bradfitz, 2013-08-05 20:29:58]

https://codereview.appspot.com/12458043/diff/13001/src/pkg/net/dnsclient_unix.go
File src/pkg/net/dnsclient_unix.go (right):

https://codereview.appspot.com/12458043/diff/13001/src/pkg/net/dnsclient_unix...
src/pkg/net/dnsclient_unix.go:61: buf = make([]byte, 2)
you don't need this allocation. just use buf[:2]

https://codereview.appspot.com/12458043/diff/13001/src/pkg/net/dnsclient_unix...
src/pkg/net/dnsclient_unix.go:62: n, err = c.Read(buf)
ReadFull here too

[axaxs, 2013-08-05 21:12:11]

On 2013/08/05 20:29:58, bradfitz wrote:
>
https://codereview.appspot.com/12458043/diff/13001/src/pkg/net/dnsclient_unix.go
> File src/pkg/net/dnsclient_unix.go (right):
> 
>
https://codereview.appspot.com/12458043/diff/13001/src/pkg/net/dnsclient_unix...
> src/pkg/net/dnsclient_unix.go:61: buf = make([]byte, 2)
> you don't need this allocation. just use buf[:2]
> 
>
https://codereview.appspot.com/12458043/diff/13001/src/pkg/net/dnsclient_unix...
> src/pkg/net/dnsclient_unix.go:62: n, err = c.Read(buf)
> ReadFull here too


Thank you and fixed.  Please take another look.

Thanks,
Alex

[axaxs, 2013-08-10 04:38:29]

On 2013/08/05 21:12:11, axaxs wrote:
> On 2013/08/05 20:29:58, bradfitz wrote:
> >
>
https://codereview.appspot.com/12458043/diff/13001/src/pkg/net/dnsclient_unix.go
> > File src/pkg/net/dnsclient_unix.go (right):
> > 
> >
>
https://codereview.appspot.com/12458043/diff/13001/src/pkg/net/dnsclient_unix...
> > src/pkg/net/dnsclient_unix.go:61: buf = make([]byte, 2)
> > you don't need this allocation. just use buf[:2]
> > 
> >
>
https://codereview.appspot.com/12458043/diff/13001/src/pkg/net/dnsclient_unix...
> > src/pkg/net/dnsclient_unix.go:62: n, err = c.Read(buf)
> > ReadFull here too
> 
> 
> Thank you and fixed.  Please take another look.
> 
> Thanks,
> Alex

Hi Brad+Team,  

Apologies as I'm still not yet familiar with standard protocol.  Does the latest
revision look good to you?  Does someone from the team typically submit them at
some interval, so it can close out the bug report as well?  Please let me know
if more is needed on my part in this cl.

Thanks,
Alex

[mikio, 2013-08-10 07:09:22]

Perhaps the CL description "net: implement DNS TCP fallback query" might be
clear. Also w/ a reference to RFC 5986 too.

And please add a test case like dialgoogle or lookup; do you know any good
external sites for this purpose?

https://codereview.appspot.com/12458043/diff/20001/src/pkg/net/dnsclient_unix.go
File src/pkg/net/dnsclient_unix.go (right):

https://codereview.appspot.com/12458043/diff/20001/src/pkg/net/dnsclient_unix...
src/pkg/net/dnsclient_unix.go:43: // Must add 2 byte length header to front of
msg
It's already described in RFC 1035 clearly, so pls drop.

https://codereview.appspot.com/12458043/diff/20001/src/pkg/net/dnsclient_unix...
src/pkg/net/dnsclient_unix.go:130: 
drop blankline

[axaxs, 2013-08-10 19:35:38]

Hi,

Per your suggestion I've removed two comments covered by RFC1035 and the
blank trailing line.  Also renamed to hopefully be more descriptive.  PTAL.

I do not know of a reliably long live rrset to feel comfortable
implementing a test.  I have built one on my own nameservers(see
description), but do not want tests to fail if I don't maintain it.  Any
suggestions?

Lastly, I do not see how RFC 5986 is related to this CL - it seems to fall
strictly under jurisdiction of RFC 1035.  Please advise.

Thanks much,
Alex


On Sat, Aug 10, 2013 at 3:09 AM, <mikioh.mikioh@gmail.com> wrote:

> Perhaps the CL description "net: implement DNS TCP fallback query" might
> be clear. Also w/ a reference to RFC 5986 too.
>
> And please add a test case like dialgoogle or lookup; do you know any
> good external sites for this purpose?
>
>
> https://codereview.appspot.**com/12458043/diff/20001/src/**
>
pkg/net/dnsclient_unix.go<https://codereview.appspot.com/12458043/diff/20001/src/pkg/net/dnsclient_unix.go>
> File src/pkg/net/dnsclient_unix.go (right):
>
> https://codereview.appspot.**com/12458043/diff/20001/src/**
>
pkg/net/dnsclient_unix.go#**newcode43<https://codereview.appspot.com/12458043/diff/20001/src/pkg/net/dnsclient_unix.go#newcode43>
> src/pkg/net/dnsclient_unix.go:**43: // Must add 2 byte length header to
> front of msg
> It's already described in RFC 1035 clearly, so pls drop.
>
> https://codereview.appspot.**com/12458043/diff/20001/src/**
>
pkg/net/dnsclient_unix.go#**newcode130<https://codereview.appspot.com/12458043/diff/20001/src/pkg/net/dnsclient_unix.go#newcode130>
> src/pkg/net/dnsclient_unix.go:**130:
> drop blankline
>
>
https://codereview.appspot.**com/12458043/<https://codereview.appspot.com/124...
>

[mikio, 2013-08-10 22:16:42]

On Sun, Aug 11, 2013 at 4:35 AM, Alex Skinner <alex@lx.lc> wrote:

> I do not know of a reliably long live rrset to feel comfortable implementing
> a test.  I have built one on my own nameservers(see description), but do not
> want tests to fail if I don't maintain it.  Any suggestions?

Can we use the existing public sites that generate a bit big RRs and support
plain-old TCP fallback? For example,

dig @8.8.8.8 +dnssec +bufsize=512 gov
;; Truncated, retrying in TCP mode.

In a test case, we just run tryOneName for querying something above, no
need to parse the results because what we need to test is just DNS transport.
Also, we need to add -dns or reuse -external flag for this test case.

Does this make sense?

> Lastly, I do not see how RFC 5986 is related to this CL

Sure, s/5986/5966/, oops.

[axaxs, 2013-08-11 23:32:58]

Thanks.  I see 5966 being more relevant now :).

Still, the test described is not reliable.   First, by current design, we
can only lookup one record type.  The reason is because if I do a 255
query, it returns nosuchhost because it tries to match rrtypes with query
type when building the rr slice.  I changed this logic in testing to
indicate 255 matches all, but in real use there is no external function for
an any query, so it's not necessary to propose such a change.

Further, dnssec is not indicated in the query, so records aren't returned.

So, to accurately test, we need a domain with a single rrset > 512 bytes,
which I cannot find.  If you want, i'll include it with my own
texttest.lx.lc, to be changed later?  Any other ideas?

Thanks,
Alex


On Sat, Aug 10, 2013 at 6:16 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:

> On Sun, Aug 11, 2013 at 4:35 AM, Alex Skinner <alex@lx.lc> wrote:
>
> > I do not know of a reliably long live rrset to feel comfortable
> implementing
> > a test.  I have built one on my own nameservers(see description), but do
> not
> > want tests to fail if I don't maintain it.  Any suggestions?
>
> Can we use the existing public sites that generate a bit big RRs and
> support
> plain-old TCP fallback? For example,
>
> dig @8.8.8.8 +dnssec +bufsize=512 gov
> ;; Truncated, retrying in TCP mode.
>
> In a test case, we just run tryOneName for querying something above, no
> need to parse the results because what we need to test is just DNS
> transport.
> Also, we need to add -dns or reuse -external flag for this test case.
>
> Does this make sense?
>
> > Lastly, I do not see how RFC 5986 is related to this CL
>
> Sure, s/5986/5966/, oops.
>

[mikio, 2013-08-12 21:53:59]

On Mon, Aug 12, 2013 at 8:32 AM, Alex Skinner <alex@lx.lc> wrote:

> Still, the test described is not reliable.   First, by current design, we
> can only lookup one record type.  The reason is because if I do a 255 query,
> it returns nosuchhost because it tries to match rrtypes with query type when
> building the rr slice.  I changed this logic in testing to indicate 255
> matches all, but in real use there is no external function for an any query,
> so it's not necessary to propose such a change.

Hm, can we make a DNS transport TCP fallback test case by using exchange
w/ external public sites?

> So, to accurately test, we need a domain with a single rrset > 512 bytes,

I think that a DNS transport test case is enough, because you addressed
the root cause is the lack of TCP fallback for DNS transport. Also, non-CGO
case, we have to use this transport for all queries.

[axaxs, 2013-08-13 01:16:09]

On 2013/08/12 21:53:59, mikio wrote:
> On Mon, Aug 12, 2013 at 8:32 AM, Alex Skinner <mailto:alex@lx.lc> wrote:
> 
> > Still, the test described is not reliable.   First, by current design, we
> > can only lookup one record type.  The reason is because if I do a 255 query,
> > it returns nosuchhost because it tries to match rrtypes with query type when
> > building the rr slice.  I changed this logic in testing to indicate 255
> > matches all, but in real use there is no external function for an any query,
> > so it's not necessary to propose such a change.
> 
> Hm, can we make a DNS transport TCP fallback test case by using exchange
> w/ external public sites?
> 
> > So, to accurately test, we need a domain with a single rrset > 512 bytes,
> 
> I think that a DNS transport test case is enough, because you addressed
> the root cause is the lack of TCP fallback for DNS transport. Also, non-CGO
> case, we have to use this transport for all queries.

Thanks, that seems reasonable.

Per suggestion, added short comment reference to RFC 5966, and added manual TCP
exchange to lookup_test.go if GOOS is not windows or plan9.  Please take another
look.

Thanks,
Alex

[mikio, 2013-08-13 03:31:27]

pls dorp "See http://play.golang.org/p/9Vvr1u5_nz" line from the CL description.
also try cross-compile, thx.

https://codereview.appspot.com/12458043/diff/37001/src/pkg/net/dnsclient_unix.go
File src/pkg/net/dnsclient_unix.go (right):

https://codereview.appspot.com/12458043/diff/37001/src/pkg/net/dnsclient_unix...
src/pkg/net/dnsclient_unix.go:28: func exchange(cfg *dnsConfig, c Conn, name
string, qtype uint16, useTCP bool) (*dnsMsg, error) {
you don't need to add new arg useTCP here, c is an interface.

https://codereview.appspot.com/12458043/diff/37001/src/pkg/net/dnsclient_unix...
src/pkg/net/dnsclient_unix.go:42: if useTCP {
type assertion

https://codereview.appspot.com/12458043/diff/37001/src/pkg/net/dnsclient_unix...
src/pkg/net/dnsclient_unix.go:58: if useTCP {
ditto

https://codereview.appspot.com/12458043/diff/37001/src/pkg/net/dnsclient_unix...
src/pkg/net/dnsclient_unix.go:116: // TCP should be supported, see RFC 5966
move up one line and just // see RFC 5966

https://codereview.appspot.com/12458043/diff/37001/src/pkg/net/lookup_test.go
File src/pkg/net/lookup_test.go (right):

https://codereview.appspot.com/12458043/diff/37001/src/pkg/net/lookup_test.go...
src/pkg/net/lookup_test.go:140: func TestTCPLookup(t *testing.T) {
we need a new file, lookup_unix_test.go or dns_unix_test.go, to avoid build
breakages; exchange is not implemented on all platforms; see build tag of
dnsclient_unix.go.

https://codereview.appspot.com/12458043/diff/37001/src/pkg/net/lookup_test.go...
src/pkg/net/lookup_test.go:149: t.Fatalf("failed: %s", err)
Dial failed

https://codereview.appspot.com/12458043/diff/37001/src/pkg/net/lookup_test.go...
src/pkg/net/lookup_test.go:153: c.Close()
defer c.Close();  and put just after Dial block

https://codereview.appspot.com/12458043/diff/37001/src/pkg/net/lookup_test.go...
src/pkg/net/lookup_test.go:155: t.Fatalf("tcp lookup failed: %s", err)
t.Fatalf("exchange failed: %v", err)

[mikio, 2013-08-13 04:05:53]

On Tuesday, August 13, 2013 12:31:27 PM UTC+9, Mikio Hara wrote:

>
https://codereview.appspot.com/12458043/diff/37001/src/pkg/net/lookup_test.go...

>
>
src/pkg/net/lookup_test.go:140<https://codereview.appspot.com/12458043/diff/37001/src/pkg/net/lookup_test.go#newcode140src/pkg/net/lookup_test.go:140>:

> func TestTCPLookup(t *testing.T) { 
> we need a new file, lookup_unix_test.go or dns_unix_test.go, to avoid 
> build breakages; exchange is not implemented on all platforms; see build 
> tag of dnsclient_unix.go. 
>

dnsclient_unix_test.go might be better.

[axaxs, 2013-08-13 05:38:05]

All suggestions accounted for I think.  Kept useTCP as bool value but set
to a type assertion rather than argument, hope this is what you had in mind
as asserting twice seems wasteful.  Cross compile works without error.
 Created new test file as suggested.

PTAL

Thanks,
Alex


On Tue, Aug 13, 2013 at 12:05 AM, Mikio Hara <mikioh.mikioh@gmail.com>wrote:

> On Tuesday, August 13, 2013 12:31:27 PM UTC+9, Mikio Hara wrote:
>
>> https://codereview.appspot.**com/12458043/diff/37001/src/**
>> pkg/net/lookup_test.go#**newcode140
>>
src/pkg/net/lookup_test.go:140<https://codereview.appspot.com/12458043/diff/37001/src/pkg/net/lookup_test.go#newcode140src/pkg/net/lookup_test.go:140>
>> **: func TestTCPLookup(t *testing.T) {
>> we need a new file, lookup_unix_test.go or dns_unix_test.go, to avoid
>> build breakages; exchange is not implemented on all platforms; see build
>> tag of dnsclient_unix.go.
>>
>
> dnsclient_unix_test.go might be better.
>

[bradfitz, 2013-08-13 16:43:09]

LGTM

[bradfitz, 2013-08-13 16:44:14]

*** Submitted as https://code.google.com/p/go/source/detail?r=77ceb1db4a1e ***

net: implement DNS TCP fallback query if UDP response is truncated

Fixes issue 5686.

R=golang-dev, bradfitz, mikioh.mikioh
CC=golang-dev
https://codereview.appspot.com/12458043

Committer: Brad Fitzpatrick <bradfitz@golang.org>