DNS : Fixes recursors answering the DNS query to properly return the correct response. #4461
Conversation
agent/dns.go
Outdated
| // Check if the response is valid and has the desired Response code | ||
| if r != nil && (r.Rcode != dns.RcodeSuccess && r.Rcode != dns.RcodeNameError) { | ||
| d.logger.Printf("[DEBUG] dns: recurse RTT for %v (%v) Recursor queried: %v Status returned: %v", q, rtt, recursor, dns.RcodeToString[r.Rcode]) | ||
| // This is so that if the last known recursor also throws an error |
There was a problem hiding this comment.
I dont think the body inside the if {} here is doing anything useful other than logging the recursor failure.
Condensed down this is the logic you have.
for ... {
if ... {
if last recursor {
break
}
continue
} else {
if no error or truncated {
write out the response
return
}
}
}
The break if its the last recursor and continuing the loop will do the exact same thing which is to finish/exit the loop. This could be condensed into
for ... {
if bad rcode {
log
continue
}
if no error or truncated {
write out response
return
}
}
There was a problem hiding this comment.
Noted. I felt that the other it was totally unnecessary. Still wanted to get an opinion. Will add the test case and push it.
|
Added in the requested changes. I left the main |
agent/dns.go
Outdated
| // we move forward onto the next one else the loop ends | ||
| continue | ||
| } else { | ||
| if err == nil || err == dns.ErrTruncated { |
There was a problem hiding this comment.
Leaving the same conditions and body is fine but this really should be
} else if err == nil || err == dns.ErrTruncated {
<body>
}
There is no reason to have the extra indentation
This PR addresses the fact that if multiple recursors are provided to the Consul agent, Consul tries to fetch the DNS query instead of prematurely returning the response received from the first known recursor. i.e If a recursor returns a response like SERVFAIL or REFUSED the previous behaviour was to return the DNS response, instead of moving onto the next recursor for getting the DNS query issued.
Fixes #4426