|
|
Descriptionnet: separate DNS transport from DNS query-response interaction
Before fixing issue 6579 this CL separates DNS transport from
DNS message interaction to make it easier to add builtin DNS
resolver control logic.
Update issue 6579
Patch Set 1 : diff -r 30307cc8bef2 https://go.googlecode.com/hg/ #
Total comments: 4
Patch Set 2 : diff -r 824ea5943ba8 https://code.google.com/p/go #
Total comments: 1
Patch Set 3 : diff -r f70ac618569d https://go.googlecode.com/hg/ #
Total comments: 4
Patch Set 4 : diff -r 2699961d1143 https://code.google.com/p/go #
Total comments: 22
Patch Set 5 : diff -r 6b696a34e0af https://code.google.com/p/go #
Total comments: 6
Patch Set 6 : diff -r 163fae12c83b https://code.google.com/p/go #Patch Set 7 : diff -r d1cf884a594f https://code.google.com/p/go #
MessagesTotal messages: 28
Hello golang-codereviews@googlegroups.com (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
Also - have you run any mem benchmarks to see how this change affects? https://codereview.appspot.com/101220044/diff/40001/src/pkg/net/dnsclient_uni... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/101220044/diff/40001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:51: b := make([]byte, 512) // see RFC 1035 This is 'correct' behavior per RFC, but can one be sure that all resolvers respect these settings? We were using 2000B buffer before which would handle it if resolver sent message over 512 limit. Just a question for your thoughts. https://codereview.appspot.com/101220044/diff/40001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:147: for _, network := range []string{"udp", "tcp"} { I don't like this behavior. If a dns query times out, or if ids don't match it will retry same server on tcp which I don't feel is correct behavior. For now let's reserve tcp for proven truncates only. This would involve removing this loop and adding a bit more code below likely.
Sign in to reply to this message.
> Also - have you run any mem benchmarks to see how this change affects? slightly. the following is the result of tryOneName(cfg, "com.", dnsTypeALL) benchmark run. benchmark old ns/op new ns/op delta BenchmarkDNSTransport 130379229 124166302 -4.77% benchmark old allocs new allocs delta BenchmarkDNSTransport 256 255 -0.39% benchmark old bytes new bytes delta BenchmarkDNSTransport 16587 15672 -5.52% https://codereview.appspot.com/101220044/diff/40001/src/pkg/net/dnsclient_uni... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/101220044/diff/40001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:51: b := make([]byte, 512) // see RFC 1035 please let me know if you have a nice solution for preventing "1st fragment piggyback attack" even if it doesn't much matter on resolver side. also, for now--fortunately, we have no edns0 support. one of the motivation of this cl is to explicit that there're several matters to be attended for using udp as a dns transport. https://codereview.appspot.com/101220044/diff/40001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:147: for _, network := range []string{"udp", "tcp"} { nice catch! thx.
Sign in to reply to this message.
Not sure I follow udp fragmentation attack at a dns client level. If you have any literature on it would be interested to read - it seems such attacks are normally targeted at a recursive resolver. No matter to the code, 512 is fine by me unless we get complaints of noncompliant resolvers... Thanks https://codereview.appspot.com/101220044/diff/60001/src/pkg/net/dnsclient_uni... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/101220044/diff/60001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:151: if nerr, ok := err.(Error); ok && nerr.Timeout() { Why are we checking for timeouts each err case? Can we return the error and move these checks and coercion to DNSError to tryOneName? Functionally should be the same, but would save many loc(and rid break statements). The 'for attempt' loop doesn't belong in this function anyhow, this is part of what other change I submitted fixes. So there's no benefit in checking error type repeatedly when we may do once later. Thoughts?
Sign in to reply to this message.
> Not sure I follow udp fragmentation attack at a dns client level. If you have > any literature on it would be interested to read - it seems such attacks are > normally targeted at a recursive resolver. but indirectly, resolvers that prefer tcp fallback would make some effect on servers. i'd like to see what happens. > No matter to the code, 512 is fine by me unless we get complaints of noncompliant resolvers... [...] > Why are we checking for timeouts each err case? Can we return the error and > move these checks and coercion to DNSError to tryOneName? that's fine. well, i'm still not sure what's the timeout for. it's a minimum seconds btw retries, right?
Sign in to reply to this message.
On 2014/06/13 04:35:40, mikio wrote: > > Not sure I follow udp fragmentation attack at a dns client level. If you have > > any literature on it would be interested to read - it seems such attacks are > > normally targeted at a recursive resolver. > > but indirectly, resolvers that prefer tcp fallback would make some effect on > servers. i'd like to see what happens. > > > No matter to the code, 512 is fine by me unless we get complaints of > noncompliant resolvers... > > [...] > > > Why are we checking for timeouts each err case? Can we return the error and > > move these checks and coercion to DNSError to tryOneName? > > that's fine. well, i'm still not sure what's the timeout for. it's a minimum > seconds btw retries, right? I have no idea why we are collecting it really. I was going to suggest scrapping it, then realized it is used for the exported Timeout function that we can't scrap. And Temporary(). Maybe we can get rid of the IsTimeout value and have the timeout function done there? Not sure really, up to you. But constantly checking every error for it doesn't seem to be doing anything useful. Thanks, Alex
Sign in to reply to this message.
On 2014/06/13 05:22:11, axaxs wrote: > On 2014/06/13 04:35:40, mikio wrote: > > > Not sure I follow udp fragmentation attack at a dns client level. If you > have > > > any literature on it would be interested to read - it seems such attacks are > > > normally targeted at a recursive resolver. > > > > but indirectly, resolvers that prefer tcp fallback would make some effect on > > servers. i'd like to see what happens. > > > > > No matter to the code, 512 is fine by me unless we get complaints of > > noncompliant resolvers... > > > > [...] > > > > > Why are we checking for timeouts each err case? Can we return the error and > > > move these checks and coercion to DNSError to tryOneName? > > > > that's fine. well, i'm still not sure what's the timeout for. it's a minimum > > seconds btw retries, right? > > > I have no idea why we are collecting it really. I was going to suggest > scrapping it, then realized it is used for the exported Timeout function that we > can't scrap. And Temporary(). Maybe we can get rid of the IsTimeout value and > have the timeout function done there? Not sure really, up to you. But > constantly checking every error for it doesn't seem to be doing anything useful. > > Thanks, > Alex And replying to myself at this hour, also realize IsTimeout is exported obviously...so can't scrap that either... Thanks, Alex
Sign in to reply to this message.
> I have no idea why we are collecting it really. I was going to suggest > scrapping it, then realized it is used for the exported Timeout function that we > can't scrap. And Temporary(). Maybe we can get rid of the IsTimeout value and > have the timeout function done there? Not sure really, up to you. But > constantly checking every error for it doesn't seem to be doing anything useful. we cannot scrap it but have to try to reduce its cost. well, to be clear, what i'm confusing is; does the timeout value of dnsconfig mean "minimum" seconds between retries? i mean, should we wait a few seconds (timeout) for sending next query?
Sign in to reply to this message.
https://codereview.appspot.com/101220044/diff/80001/src/pkg/net/dnsclient_uni... File src/pkg/net/dnsclient_unix.go (left): https://codereview.appspot.com/101220044/diff/80001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:83: continue When this occurs, it's going to retry the entire query over TCP. This should reset the timeout value. A deadline is a hard time in the future. If it is set to 5 seconds, then deadline as passed from tryOneName is 5 seconds in future from when it was calculated in other function (which itself is a little scary since it ideally should be started RIGHT before the dial...). Anyhow, if someone has slow internet that takes 3s to do a round trip, every query will fail because deadline is not reset. So the udp query takes 3 seconds, then it is truncated so it retries over tcp. This fails after 2 seconds of tcp trying as we are now past the originally calculated deadline. So, please pass either the timeout value or else the cfg to exchange function and calculate deadline at top of function. Then, here in the truncate section, recalculate the deadline time and update the 'deadline' variable and set d.Deadline to the new deadline time. https://codereview.appspot.com/101220044/diff/80001/src/pkg/net/dnsclient_uni... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/101220044/diff/80001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:141: func exchange(server, name string, qtype uint16, deadline time.Time) (*dnsMsg, error) { Do not pass deadline here as an argument. Pass the timeout or else cfg as we did initially. See truncate check comments.
Sign in to reply to this message.
Hi Mikio, Any update? This change is shaping up nicely, would like to see it land soon and we can start on performance. Thanks Alex On Fri, Jun 13, 2014 at 11:33 AM, <alex@lx.lc> wrote: > > https://codereview.appspot.com/101220044/diff/80001/src/ > pkg/net/dnsclient_unix.go > File src/pkg/net/dnsclient_unix.go (left): > > https://codereview.appspot.com/101220044/diff/80001/src/ > pkg/net/dnsclient_unix.go#oldcode83 > src/pkg/net/dnsclient_unix.go:83: continue > When this occurs, it's going to retry the entire query over TCP. This > should reset the timeout value. A deadline is a hard time in the > future. If it is set to 5 seconds, then deadline as passed from > tryOneName is 5 seconds in future from when it was calculated in other > function (which itself is a little scary since it ideally should be > started RIGHT before the dial...). Anyhow, if someone has slow internet > that takes 3s to do a round trip, every query will fail because deadline > is not reset. So the udp query takes 3 seconds, then it is truncated so > it retries over tcp. This fails after 2 seconds of tcp trying as we are > now past the originally calculated deadline. > So, please pass either the timeout value or else the cfg to exchange > function and calculate deadline at top of function. Then, here in the > truncate section, recalculate the deadline time and update the > 'deadline' variable and set d.Deadline to the new deadline time. > > https://codereview.appspot.com/101220044/diff/80001/src/ > pkg/net/dnsclient_unix.go > File src/pkg/net/dnsclient_unix.go (right): > > https://codereview.appspot.com/101220044/diff/80001/src/ > pkg/net/dnsclient_unix.go#newcode141 > src/pkg/net/dnsclient_unix.go:141: func exchange(server, name string, > qtype uint16, deadline time.Time) (*dnsMsg, error) { > Do not pass deadline here as an argument. Pass the timeout or else cfg > as we did initially. See truncate check comments. > > https://codereview.appspot.com/101220044/ >
Sign in to reply to this message.
sorry for being late, ptal. https://codereview.appspot.com/101220044/diff/80001/src/pkg/net/dnsclient_uni... File src/pkg/net/dnsclient_unix.go (left): https://codereview.appspot.com/101220044/diff/80001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:83: continue i'm still not convinced of the timeout in /etc/resolv.conf. On some BSD, timeout: Specifies the total amount of time allowed for a name resolution. This time interval is divided by the number of nameservers and the number of retries allowed for each nameserver. And other BSDs, timeout: sets the initial amount of time the resolver will wait for a response from a remote name server before retrying the query via a different name server. The resolver may wait longer during subsequent retries of the current query since an exponential back-off is applied to the timeout value. Measured in seconds, the default is RES_TIMEOUT, the allowed maximum is RES_MAXRETRANS (see <resolv.h>). On Linux, timeout: sets the amount of time the resolver will wait for a response from a remote name server before retrying the query via a different name server. Measured in econds, the default is RES_TIMEOUT (currently 5, see <resolv.h>). The value for this option is silently capped to 30. for now i take your suggestion and i'd like to revisit this in your cl review. https://codereview.appspot.com/101220044/diff/80001/src/pkg/net/dnsclient_uni... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/101220044/diff/80001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:141: func exchange(server, name string, qtype uint16, deadline time.Time) (*dnsMsg, error) { On 2014/06/13 15:33:55, axaxs wrote: > Do not pass deadline here as an argument. Pass the timeout or else cfg as we > did initially. See truncate check comments. Done.
Sign in to reply to this message.
Thanks. LGTM. Will revise and resubmit other CL once this lands. On Wed, Jun 25, 2014 at 10:37 PM, <mikioh.mikioh@gmail.com> wrote: > sorry for being late, ptal. > > > > https://codereview.appspot.com/101220044/diff/80001/src/ > pkg/net/dnsclient_unix.go > File src/pkg/net/dnsclient_unix.go (left): > > https://codereview.appspot.com/101220044/diff/80001/src/ > pkg/net/dnsclient_unix.go#oldcode83 > src/pkg/net/dnsclient_unix.go:83: continue > i'm still not convinced of the timeout in /etc/resolv.conf. > > On some BSD, timeout: Specifies the total amount of time allowed for a > name resolution. This time interval is divided by the number of > nameservers and the number of retries allowed for each nameserver. > > And other BSDs, timeout: sets the initial amount of time the resolver > will wait for a response from a remote name server before retrying the > query via a different name server. The resolver may wait longer during > subsequent retries of the current query since an exponential back-off is > applied to the timeout value. Measured in seconds, the default is > RES_TIMEOUT, the allowed maximum is RES_MAXRETRANS (see <resolv.h>). > > On Linux, timeout: sets the amount of time the resolver will wait for a > response from a remote name server before retrying the query via a > different name server. Measured in econds, the default is RES_TIMEOUT > (currently 5, see <resolv.h>). The value for this option is silently > capped to 30. > > for now i take your suggestion and i'd like to revisit this in your cl > review. > > > https://codereview.appspot.com/101220044/diff/80001/src/ > pkg/net/dnsclient_unix.go > File src/pkg/net/dnsclient_unix.go (right): > > https://codereview.appspot.com/101220044/diff/80001/src/ > pkg/net/dnsclient_unix.go#newcode141 > src/pkg/net/dnsclient_unix.go:141: func exchange(server, name string, > qtype uint16, deadline time.Time) (*dnsMsg, error) { > On 2014/06/13 15:33:55, axaxs wrote: > >> Do not pass deadline here as an argument. Pass the timeout or else >> > cfg as we > >> did initially. See truncate check comments. >> > > Done. > > https://codereview.appspot.com/101220044/ >
Sign in to reply to this message.
Just following up since this is blocking other changes. Is it waiting for more eyes? Thanks, Alex On Thursday, June 26, 2014, Alex Skinner <alex@lx.lc> wrote: > Thanks. LGTM. Will revise and resubmit other CL once this lands. > > > On Wed, Jun 25, 2014 at 10:37 PM, <mikioh.mikioh@gmail.com > <javascript:_e(%7B%7D,'cvml','mikioh.mikioh@gmail.com');>> wrote: > >> sorry for being late, ptal. >> >> >> >> https://codereview.appspot.com/101220044/diff/80001/src/ >> pkg/net/dnsclient_unix.go >> File src/pkg/net/dnsclient_unix.go (left): >> >> https://codereview.appspot.com/101220044/diff/80001/src/ >> pkg/net/dnsclient_unix.go#oldcode83 >> src/pkg/net/dnsclient_unix.go:83: continue >> i'm still not convinced of the timeout in /etc/resolv.conf. >> >> On some BSD, timeout: Specifies the total amount of time allowed for a >> name resolution. This time interval is divided by the number of >> nameservers and the number of retries allowed for each nameserver. >> >> And other BSDs, timeout: sets the initial amount of time the resolver >> will wait for a response from a remote name server before retrying the >> query via a different name server. The resolver may wait longer during >> subsequent retries of the current query since an exponential back-off is >> applied to the timeout value. Measured in seconds, the default is >> RES_TIMEOUT, the allowed maximum is RES_MAXRETRANS (see <resolv.h>). >> >> On Linux, timeout: sets the amount of time the resolver will wait for a >> response from a remote name server before retrying the query via a >> different name server. Measured in econds, the default is RES_TIMEOUT >> (currently 5, see <resolv.h>). The value for this option is silently >> capped to 30. >> >> for now i take your suggestion and i'd like to revisit this in your cl >> review. >> >> >> https://codereview.appspot.com/101220044/diff/80001/src/ >> pkg/net/dnsclient_unix.go >> File src/pkg/net/dnsclient_unix.go (right): >> >> https://codereview.appspot.com/101220044/diff/80001/src/ >> pkg/net/dnsclient_unix.go#newcode141 >> src/pkg/net/dnsclient_unix.go:141: func exchange(server, name string, >> qtype uint16, deadline time.Time) (*dnsMsg, error) { >> On 2014/06/13 15:33:55, axaxs wrote: >> >>> Do not pass deadline here as an argument. Pass the timeout or else >>> >> cfg as we >> >>> did initially. See truncate check comments. >>> >> >> Done. >> >> https://codereview.appspot.com/101220044/ >> > >
Sign in to reply to this message.
Just following up since this is blocking other changes. Is it waiting for more eyes? Thanks, Alex On Thursday, June 26, 2014, Alex Skinner <alex@lx.lc> wrote: > Thanks. LGTM. Will revise and resubmit other CL once this lands. > > > On Wed, Jun 25, 2014 at 10:37 PM, <mikioh.mikioh@gmail.com > <javascript:_e(%7B%7D,'cvml','mikioh.mikioh@gmail.com');>> wrote: > >> sorry for being late, ptal. >> >> >> >> https://codereview.appspot.com/101220044/diff/80001/src/ >> pkg/net/dnsclient_unix.go >> File src/pkg/net/dnsclient_unix.go (left): >> >> https://codereview.appspot.com/101220044/diff/80001/src/ >> pkg/net/dnsclient_unix.go#oldcode83 >> src/pkg/net/dnsclient_unix.go:83: continue >> i'm still not convinced of the timeout in /etc/resolv.conf. >> >> On some BSD, timeout: Specifies the total amount of time allowed for a >> name resolution. This time interval is divided by the number of >> nameservers and the number of retries allowed for each nameserver. >> >> And other BSDs, timeout: sets the initial amount of time the resolver >> will wait for a response from a remote name server before retrying the >> query via a different name server. The resolver may wait longer during >> subsequent retries of the current query since an exponential back-off is >> applied to the timeout value. Measured in seconds, the default is >> RES_TIMEOUT, the allowed maximum is RES_MAXRETRANS (see <resolv.h>). >> >> On Linux, timeout: sets the amount of time the resolver will wait for a >> response from a remote name server before retrying the query via a >> different name server. Measured in econds, the default is RES_TIMEOUT >> (currently 5, see <resolv.h>). The value for this option is silently >> capped to 30. >> >> for now i take your suggestion and i'd like to revisit this in your cl >> review. >> >> >> https://codereview.appspot.com/101220044/diff/80001/src/ >> pkg/net/dnsclient_unix.go >> File src/pkg/net/dnsclient_unix.go (right): >> >> https://codereview.appspot.com/101220044/diff/80001/src/ >> pkg/net/dnsclient_unix.go#newcode141 >> src/pkg/net/dnsclient_unix.go:141: func exchange(server, name string, >> qtype uint16, deadline time.Time) (*dnsMsg, error) { >> On 2014/06/13 15:33:55, axaxs wrote: >> >>> Do not pass deadline here as an argument. Pass the timeout or else >>> >> cfg as we >> >>> did initially. See truncate check comments. >>> >> >> Done. >> >> https://codereview.appspot.com/101220044/ >> > >
Sign in to reply to this message.
On 2014/07/10 12:13:54, axaxs wrote: > Just following up since this is blocking other changes. oops, sorry for being late. the big football games consumed my spare time. > Is it waiting for more eyes? yup, we need a sign off from established people.
Sign in to reply to this message.
R=iant@golang.org (assigned by mikioh.mikioh@gmail.com)
Sign in to reply to this message.
iant?
Sign in to reply to this message.
On Mon, Jul 28, 2014 at 12:50 AM, <mikioh.mikioh@gmail.com> wrote: > iant? > FYI, iant is on vacation.
Sign in to reply to this message.
On Mon, Jul 28, 2014 at 1:52 PM, minux <minux@golang.org> wrote: > FYI, iant is on vacation. thanks. minux, do you have a spare time?
Sign in to reply to this message.
R=golang-dev Sorry, I'm not familiar with the code. Given that rsc is also on vacation, let's ask for reviewers on golang-dev.
Sign in to reply to this message.
Drive-by comments. https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:110: if n < len(b) { From io.Writer: "Write must return a non-nil error if it returns n < len(p)." https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:134: default: I think I would case "udp", "udp4", "udp6": return c.(*UDPConn), nil } panic("unreachable") just to be incrementally more explicit (and also potentially guard against future accidentally missed additions of networks or something). Could just be me though, I don't know if the stdlib has common idioms for this already in place. https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:139: // exchange sends a query on the connection and hope for a response. and hopes for https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:140: // Up to cfg.attempts attempts. cfg is not an argument anymore. https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:143: out := dnsMsg{} out := dnsMsg{ recursion_desired: true, question: []dnsQuestion{ {name, qtype, dnsClassINET}, }, } https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:178: return "", nil, &DNSError{Err: errDNSNoServers.Error(), Name: name} I'm personally not a fan of these canned error messages. There seems to be very little point in defining them consistently if clients can't make use of them. I tend to much prefer using `fmt.Errorf` to add in context to error messages, which (while not canned) at least contain enough extra information to be useful in debugging. In particular it seems odd to define an `error` to just call .Error() to get its string. I don't know what promises we've made about the text of error messages being consistent, but I don't think we have, though we should try to keep common strings like "no DNS servers" if we think people might've hard-coded them. https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:183: timeout := time.Duration(cfg.timeout) * time.Second can cfg.timeout be changed to be a time.Duration itself? https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:187: lastErr = &DNSError{Err: errDNSNoAnswer.Error(), Name: name, Server: server, IsTimeout: true} write this out on lines. https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:200: if err != nil { I think you can always set lastErr = err here. https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... File src/pkg/net/dnsclient_unix_test.go (right): https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix_test.go:19: var dnsTransportTests = []struct { While you're at it, you might add some NXDOMAIN tests too. My favorite is invalid., which RFC6761 says should always be NXDOMAIN. Possible downside... If you're on an ISP that is doing some DNS hijack, and consequently ignoring the RFCs, that may cause this test to fail... Part of me thinks that might be a good thing, though.
Sign in to reply to this message.
ptal https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:110: if n < len(b) { i don't understand what you are suggesting. https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:134: default: On 2014/07/28 20:44:04, kevlar wrote: > I think I would > > case "udp", "udp4", "udp6": > return c.(*UDPConn), nil > } > panic("unreachable") > > just to be incrementally more explicit (and also potentially guard against > future accidentally missed additions of networks or something). > > Could just be me though, I don't know if the stdlib has common idioms for this > already in place. Done. https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:139: // exchange sends a query on the connection and hope for a response. On 2014/07/28 20:44:04, kevlar wrote: > and hopes for Done. https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:140: // Up to cfg.attempts attempts. On 2014/07/28 20:44:05, kevlar wrote: > cfg is not an argument anymore. Done. https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:143: out := dnsMsg{} On 2014/07/28 20:44:05, kevlar wrote: > out := dnsMsg{ > recursion_desired: true, > question: []dnsQuestion{ > {name, qtype, dnsClassINET}, > }, > } Done. https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:178: return "", nil, &DNSError{Err: errDNSNoServers.Error(), Name: name} fine, dropped. anyway we'll revisit all error messages when we smash issue 4856. https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:183: timeout := time.Duration(cfg.timeout) * time.Second On 2014/07/28 20:44:04, kevlar wrote: > can cfg.timeout be changed to be a time.Duration itself? it would be in another cl; alex is working on 75180043 https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:187: lastErr = &DNSError{Err: errDNSNoAnswer.Error(), Name: name, Server: server, IsTimeout: true} On 2014/07/28 20:44:05, kevlar wrote: > write this out on lines. Done. https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:200: if err != nil { On 2014/07/28 20:44:04, kevlar wrote: > I think you can always set lastErr = err here. Done. https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... File src/pkg/net/dnsclient_unix_test.go (right): https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix_test.go:19: var dnsTransportTests = []struct { On 2014/07/28 20:44:05, kevlar wrote: > While you're at it, you might add some NXDOMAIN tests too. My favorite is > invalid., which RFC6761 says should always be NXDOMAIN. > > Possible downside... > If you're on an ISP that is doing some DNS hijack, and consequently ignoring the > RFCs, that may cause this test to fail... > Part of me thinks that might be a good thing, though. Done.
Sign in to reply to this message.
https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:110: if n < len(b) { > i don't understand what you are suggesting. This check is redundant. If n < len(b), Write would've returned an error. This should either be checked first or removed. Since nothing is checking the errDNSShortWrite (it's not exported), I'd opt for just removing it. https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:41: func (c *UDPConn) readDNSResponse() (*dnsMsg, error) { Is attaching unexported methods to UDPConn/TCPConn/Dialer a pattern that is used elsewhere in net? Normally I would say that it'd be better to make your own type that embeds them, but I do see the appeal. https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:192: lastErr = &DNSError{Err: nerr.Error(), Name: name, Server: server, IsTimeout: true} (optional, nitpick) I'd write all of your &DNSErrors out one-field-per-line https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un... File src/pkg/net/dnsclient_unix_test.go (right): https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix_test.go:37: {"8.8.8.8:53", "com.", dnsTypeALL, 2, []int{dnsRcodeSuccess, dnsRcodeServerFailure}}, If the only purpose of the rcodes slice is to allow RcodeServerFailure and you want to allow that for all tests, you might as well just add that to the test and make rcode singular.
Sign in to reply to this message.
https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:110: if n < len(b) { ah, got it, thanks. https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:41: func (c *UDPConn) readDNSResponse() (*dnsMsg, error) { no, this is a special case. we'll make new dns endpoint type and move builtin dns resolver stuff into some internal package for avoiding the issue of circular dependency when we need to implement t-dns and/or edns1. https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:192: lastErr = &DNSError{Err: nerr.Error(), Name: name, Server: server, IsTimeout: true} On 2014/08/04 18:08:04, kevlar wrote: > (optional, nitpick) I'd write all of your &DNSErrors out one-field-per-line Done. https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un... File src/pkg/net/dnsclient_unix_test.go (right): https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix_test.go:37: {"8.8.8.8:53", "com.", dnsTypeALL, 2, []int{dnsRcodeSuccess, dnsRcodeServerFailure}}, On 2014/08/04 18:08:04, kevlar wrote: > If the only purpose of the rcodes slice is to allow RcodeServerFailure and you > want to allow that for all tests, you might as well just add that to the test > and make rcode singular. Done.
Sign in to reply to this message.
LGTM from my perspective... not sure how much weight you want to give to me though :).
Sign in to reply to this message.
On Wed, Aug 6, 2014 at 4:45 AM, <kevlar@google.com> wrote: > from my perspective... not sure how much weight you want to give to me > though :). thanks. ah... https://codereview.appspot.com/105110044/ is also waiting for your review. ;)
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=ed83901ff17e *** net: separate DNS transport from DNS query-response interaction Before fixing issue 6579 this CL separates DNS transport from DNS message interaction to make it easier to add builtin DNS resolver control logic. Update issue 6579 LGTM=alex, kevlar R=golang-codereviews, alex, gobot, iant, minux, kevlar CC=golang-codereviews https://codereview.appspot.com/101220044
Sign in to reply to this message.
Message was sent while issue was closed.
This CL appears to have broken the netbsd-386-minux builder. See http://build.golang.org/log/c9df177ad34350f0240b81a1b0fb80bafbe7e095
Sign in to reply to this message.
|