-
Notifications
You must be signed in to change notification settings - Fork 879
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
Retain V6 DNS server in resolv.conf; use only V4 servers for fallback #886
Conversation
Signed-off-by: Santhosh Manohar <santhosh@docker.com>
013a807
to
c77459a
Compare
) | ||
|
||
type extDNSEntry struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need this ?
You will convert the configured address from string form to net.IP, then when you need to program you will convert it again back to string form just to store the IPv6 flag.
Are you using anywhere extDNEntry.IP as a net.IP ? Otherwise this seem unnecessary.
I would just check in NameServer()
with isIPv6 := net.ParseIP().to4() == nil
in the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to do the parsing once and set the v6 flag when the extDNS is populated rather than doing it on each DNS query handling which could be 100s of times.
I would just check in NameServer() with isIPv6 := net.ParseIP().to4() == nil in the loop.
Are you referring to ServeDNS() function ? That is the one handling the query where the v6 flag is checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, also I misread the code, you actually keep the IP
as string, so never mind.
Got confused by the IP parsing. I, in fact, would just look for a signature since it seems the addresses are already validated, something like:
r.extDNS[i].ip = dns[i]
r.extDNS[i].isIPv6 = strings.Contains(dns[i], ":")
but this is minor, up to you.
@mrjana Updated changes keep the v6 behavior same as 1.9 and embedded server will use only the v4 DNS servers to forward the requests. PTAL. |
LGTM |
nameservers := []string{} | ||
for _, line := range getLines(resolvConf, []byte("#")) { | ||
var ns = nsRegexp.FindSubmatch(line) | ||
var ns [][]byte | ||
if kind == netutils.IP { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using a switch
here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because if, else is just fine for small number of cases..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far we have been using switch for 3 else, but I see some places in the code where we have kept 3 else.
Anyway this is minor.
LGTM |
Retain V6 DNS server in resolv.conf; use only V4 servers for fallback
Fixes #885
Signed-off-by: Santhosh Manohar santhosh@docker.com