fix: check network mode when choosing resolv.conf#5207
fix: check network mode when choosing resolv.conf#5207crazy-max merged 1 commit intomoby:masterfrom Ka0o0:fix-resolvd-host-network
Conversation
github-actions
Bot
added
area/testing
area/dependencies
| // interface. Once legacy networking is removed, this can always return | ||
| // defaultPath. | ||
| func Path() string { | ||
| func Path(netMode pb.NetMode) string { |
There was a problem hiding this comment.
Changes in vendor/github.com/docker/docker should be first done in https://github.com/moby/moby repo and vendored back here.
There was a problem hiding this comment.
We need to figure out a way for build containers to use something similar to the internal resolver used by custom bridge networks (127.0.0.11) - it proxies DNS requests from the container, making upstream requests from the host's network namespace when necessary.
(At the moment, we use addresses from the host's resolv.conf, but they may not work in the container's network namespace - including nameservers with localhost addresses like systemd's 127.0.0.53, or IPv6 nameservers in an IPv4-only build container. If there are no nameservers the container can use, we fall back to Google's DNS.)
Once that's done, we'll be able to completely get rid of the code that looks at /run/systemd/resolve/resolv.conf (and the Google DNS fallbacks, and the default-bridge network would be able to use the internal resolver, making it more like other networks).
However, once that's done, the build container will still need a special-case for host networking. (It just needs a copy of the host's resolv.conf in that case. There's no need for a DNS proxy, because there's no container namespace to escape from).
So, for this fix, it might be best to deal with that special case in executor/oci/resolvconf.go now - for host networking, just don't call resolvconf.Path and use /etc/resolv.conf? (Then, no need to update vendored code.)
There was a problem hiding this comment.
That's a good idea. I've refactored the code and tested it which seems to work the same.
github-actions
Bot
removed
the
area/dependencies
There was a problem hiding this comment.
That's simplified things a bit - thank you!
The commits will need squashing, and could you update the commit message to explain the change? (Just something like "In host networking mode, unconditionally use "/etc/resolv.conf" rather than systemd's config".)
I was a bit unsure what happens here and if it might get problematic in consecutive builds with different networking modes. Maybe someone can explain this to me.
Looks like it's ok because it already had to deal with removing/not-removing localhost nameservers, so the output path depends on netMode ...
buildkit/executor/oci/resolvconf.go
Lines 29 to 32 in 5c66a49
... and it's covered by
TestRegenerateResolvconfToRemoveLocalDNS / TestRegenerateResolvconfToAddLocalDNS. But, that needs checking by someone who knows this code better than me.
(Not directly related to this change - but lastNotEmpty is never set to true, so it doesn't do anything. Probably not causing any issues, but I'm not sure what the intention was. I think if the input file is removed, unlikely as that is, an old output file will be left as-is rather than getting cleared - maybe it was something to do with that?)
Update: "I think if the input file is removed, unlikely as that is, an old output file will be left as-is rather than getting cleared" ... oh, not that - because an empty path is returned in that case. Maybe lastNotEmpty could be removed. (Not needed as part of this change though.)
| resolvconfPath = oldResolvconfPath | ||
| }) | ||
| resolvconfPath = func() string { | ||
| resolvconfPath = func(netMode pb.NetMode) string { |
There was a problem hiding this comment.
It's perhaps worth checking this is called with the expected netMode?
There was a problem hiding this comment.
Done. I've had to move the whole block into the iterator over the network modes. Can you please check if that's okay like this?
Signed-off-by: Kai Takac <[email protected]>
|
Cool, thanks for the review! |
3 participants
Hi,
this fixes #2404 . Instead of always checking for systemd-resolved it's sufficient to do so only if the run does not happen within the host networking mode.
I was a bit unsure what happens here and if it might get problematic in consecutive builds with different networking modes. Maybe someone can explain this to me.
BR Kai