Retry other external DNS servers on ServFail#2121
Conversation
|
Misspell error :) |
Codecov Report
@@ Coverage Diff @@
## master #2121 +/- ##
=========================================
Coverage ? 40.49%
=========================================
Files ? 139
Lines ? 22436
Branches ? 0
=========================================
Hits ? 9086
Misses ? 12010
Partials ? 1340
Continue to review full report at Codecov.
|
ctelfer
left a comment
ctelfer
left a comment
There was a problem hiding this comment.
LGTM
Looks correct overall. Just a minor suggestion w.r.t. the test code. Feel free to ignore.
resolver_test.go
Outdated
| m.SetRcode(r, dns.RcodeServerFailure) | ||
| } | ||
| requests = requests + 1 | ||
| w.WriteMsg(m) |
There was a problem hiding this comment.
The global state here and the comparison against it seems a little odd. The way TestDNSProxyServFail below compared "requests" w/ the previous value threw me at first. Take it for what it's worth, but suggest:
func newHandleTestDNSRequestServFailOnce(requests *int) func(dns.ResponseWriter, dns.*Msg) {
return func(w dns.ResponseWriter, r *dns.Msg) {
...
*requests = *requests + 1
...
}
}
...
func TestDNSProxyServFail(t *testing.T) {
...
var nRequests int = 0
dns.HandleFunc(".", newHandleTestDNSRequestServFailOnce(&nRequests))
...
if nRequests != 2 {
...
}
...
}Just thought it might help readability. (One could also embed the whole newHandleTestDNSRequestServFailOnce() body in the main test function and not have to pass the request counter pointer.... but that seems uglier, really.)
There was a problem hiding this comment.
Thanks for the suggestion! I did not like that global either and your suggestion is a decent way to keep it bound to the scope of the function. Will incorporate it.
| if tconn != nil { | ||
| defer tconn.Close() | ||
| } | ||
| if err != nil { |
There was a problem hiding this comment.
Can we club both if together ? it will help readability. Else block seems to be more. Just a suggestion .
LIke
Signed-off-by: Deep Debroy <ddebroy@docker.com>
|
Incorporated code review comments. |
5 participants
This PR covers the following:
respoutside null check that may lead to a null exception.