-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
Clean up localhost resolv logic and add IPv6 support to regexp #10005
Conversation
will this fix #5811 |
@jfrazelle interesting.. it doesn't in the current form, but given what I've added, it would be relatively simple to add a check for "if IPv6" enabled, and clean out all IPv6 nameservers rather than just localhost if IPv6 is currently not enabled for the container. If that makes sense I can update the PR with that capability. |
ya that would be awesome! this is the problem we alsways have w the drone On Mon, Jan 12, 2015 at 10:10 AM, Phil Estes notifications@github.com
|
0335048
to
2708f8e
Compare
@jfrazelle The updated PR will now fix #5811: if |
defaultIPv6Dns = []string{"2001:4860:4860::8888", "2001:4860:4860::8844"} | ||
ipv4NumBlock = `(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)` | ||
ipv4Address = `(` + ipv4NumBlock + `\.){3}` + ipv4NumBlock | ||
ipv6Address = `([0-9A-Fa-f]{0,4}:){2,7}([0-9A-Fa-f]{1,4})` |
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 think it is possible to skip the last digit if it's a 0. e.g. 2001:db8::
I would use
ipv6Address = `([0-9A-Fa-f]{0,4}:){2,7}([0-9A-Fa-f]{0,4})`
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.
Wouldn't this really need to be something insane like in
http://stackoverflow.com/a/17871737/433558 ? (so that we match all
permutations of possible addresses, like IPv4-Embedded IPv6 Addresses)
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 think it will match a superset of IPv6 addresses.
e.g. invalid addresses like fe80:::1
as well as transition addresses like ::ffff:0:a:b:c:d
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.
Besides, link-local IPv6 addresses like fe80::a:b:c:d%eth0
that work on the host will not work from within the container.
But I think the person who's using link-local addresses for DNS should know what he's doing (wrong).
EDIT
Oh, I didn't know those IPv4-Embedded IPv6 Addresses.
Well. Yeah. Maybe there will be a Pull-Request for this - probably not 😄
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.
@MalteJ @tianon given the complexity of an exact regex, I chose something that was a bit less aggressive given what we are coming from (at least from IPv4 nameserver perspective) was not even refusing invalid IPv4.. and given this particular code is not trying to explicitly validate, but rather match, it seemed close enough from all my investigation. I did shorten it to not handle the IPv4-embedded, but that can be added back in to the regex if we feel that will be a valid use case. Since we have dual-stack, not sure why embedded IPv4 would be that useful.
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 agree, @estesp
But I would still use {0,4} at the end.
Thanks for this PR! 😃
cool thanks! |
2708f8e
to
db4b472
Compare
I appreciate all the code comments LGTM |
// if the resulting resolvConf has no more nameservers defined, use defaultDns | ||
if len(GetNameservers(cleanedResolvConf)) == 0 { | ||
log.Infof("No non-localhost DNS nameservers are left in resolv.conf. Using default external servers : %v", defaultIPv4Dns) | ||
cleanedResolvConf = append(cleanedResolvConf, []byte("\nnameserver "+strings.Join(defaultIPv4Dns, "\nnameserver "))...) |
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.
this Join looks superweird :) Let's make default nameservers as just strings:
nameserver 8.8.8.8
nameserver 8.8.8.4
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.
Good point--that was weird; fixed in latest amend to commit!
db4b472
to
c74479c
Compare
ping @LK4D4 |
// default DNS servers for IPv4 and (optionally) IPv6 | ||
if len(GetNameservers(cleanedResolvConf)) == 0 { | ||
log.Infof("No non-localhost DNS nameservers are left in resolv.conf. Using default external servers : %v", defaultIPv4Dns) | ||
cleanedResolvConf = append(cleanedResolvConf, []byte("\n"+strings.Join(defaultIPv4Dns, "\n"))...) |
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 think it's cleaner to combine array of needed dns, like dns := append(defaultIPv4Dns, defaultIPv6Dns)
and then use all your Join
magic.
Addresses moby#5811 This cleans up an error in the logic which removes localhost resolvers from the host resolv.conf at container creation start time. Specifically when the determination is made if any nameservers are left after removing localhost resolvers, it was using a string match on the word "nameserver", which could have been anywhere (including commented out) leading to incorrect situations where no nameservers were left but the default ones were not added. This also adds some complexity to the regular expressions for finding nameservers in general, as well as matching on localhost resolvers due to the recent addition of IPv6 support. Because of IPv6 support now available in the Docker daemon, the resolvconf code is now aware of IPv6 enable/disable state and uses that for both filter/cleaning of nameservers as well as adding default Google DNS (IPv4 only vs. IPv4 and IPv6 if IPv6 enabled). For all these changes, tests have been added/strengthened to test these additional capabilities. Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com> (github: estesp)
c74479c
to
93d51e5
Compare
@LK4D4 updated--PTAL! |
LGTM |
Clean up localhost resolv logic and add IPv6 support to regexp
Addresses #5811
This cleans up an error in the logic which removes localhost resolvers
from the host resolv.conf at container creation start time. Specifically
when the determination is made if any nameservers are left after
removing localhost resolvers, it was using a string match on the word
"nameserver", which could have been anywhere (including commented out)
leading to incorrect situations where no nameservers were left but the
default ones were not added.
This also adds some complexity to the regular expressions for finding
nameservers in general, as well as matching on localhost resolvers due
to the recent addition of IPv6 support. Because of IPv6 support now
available in the Docker daemon, the resolvconf code is now aware of
IPv6 enable/disable state and uses that for both filter/cleaning of
nameservers as well as adding default Google DNS (IPv4 only vs. IPv4
and IPv6 if IPv6 enabled). For all these changes, tests have been
added/strengthened to test these additional capabilities.
Docker-DCO-1.1-Signed-off-by: Phil Estes estesp@linux.vnet.ibm.com (github: estesp)