Systemd-resolved proper handling by fcrisciani · Pull Request #37485 · moby/moby

fcrisciani · 2018-07-17T19:03:11Z

- What I did
Added code to handle case where the OS is using systemd-resolved
Cleaned up some of the code in the resolv.conf area
Moved option NetworkControlPlaneMTU under networking options
Added a library to fetch the pid of a process, coming from (https://github.com/kubernetes/kubernetes/tree/master/pkg/util/procfs) minus, things not useful and avoiding other dependencies

- How to verify it
Did not change functionality so current testing should cover it already (moby and libnetwork)

- Description for the changelog

Handle systemd-resolved case providing appropriate resolv.conf to networking layer

- A picture of a cute animal (not mandatory but encouraged)

fcrisciani · 2018-07-17T19:06:20Z

will update the vendoring as the libnetwork side gets merged
cc @yongtang, because some of the recent code changes were done by you
@msabansal @johnstep to verify that moving some of the code I'm not removing things that are needed form windows
@thaJeztah just as a general review and to check on the import of the procfs from kube

johnstep

Looks good from a Windows perspective.

codecov · 2018-07-17T23:23:23Z

Codecov Report

❗ No coverage uploaded for pull request base (master@c3a0207). Click here to learn what that means.
The diff coverage is 40.98%.

@@            Coverage Diff            @@
##             master   #37485   +/-   ##
=========================================
  Coverage          ?   34.96%           
=========================================
  Files             ?      611           
  Lines             ?    44939           
  Branches          ?        0           
=========================================
  Hits              ?    15714           
  Misses            ?    27097           
  Partials          ?     2128

thaJeztah

some questions/thoughts 🤗

thaJeztah · 2018-07-18T13:47:40Z

internal/procfs/procfs_linux.go

Wondering; would shelling out to pidof work? (would it be slower?), e.g.;

pids, err := exec.Command("pidof", "some-binary").Output() # check error and len(pids) > 0

more worried if the pidof is available on every platform and if it is reachable from the daemon (like is in the $PATH) etc

I'm not sure Walk is really what we want here since it traverses every dir recursively.

What do you think about some either hitting the systemd-resolved API or checking /etc/resolv.conf for 127.0.0.53 instead?

more worried if the pidof is available on every platform and if it is reachable from the daemon (like is in the $PATH) etc

Good point; looking at http://man7.org/linux/man-pages/man1/pidof.1.html, it's part of https://gitlab.com/procps-ng/procps. I think it's fairly common to be installed (at least on common distros), but we may want to check.

I had the same concerns as @cpuguy83; recursively traversing /proc just for detection looks like a lot of overhead. Not sure if string-matching for 127.0.0.53 in /etc/resolv.conf is a good solution though; sounds a bit brittle.

We should consider to what extend we want the automatic-detection to be "perfect", and possibly add a manual configuration, as discussed in moby/libnetwork#2232 (comment)

(...) is checking for a particular binary running the right way to do this? K8s is mainly doing it to issue a warning so that's "safe" in a sense. But transparently substituting in libnetwork doesn't seem robust. Maybe we should do what k8s is doing even more so: require the user to provide an option for a default resolv.conf file when starting up the engine. (...)

I opened a PR on k8s repo here to optimize the pid lookup: kubernetes/kubernetes#66367

@thaJeztah being in the config, does not automatically exposing it as an option? so if the user is not satisfied on the auto selection can specify it?

@cpuguy83 will wait for that PR to be merged and will update, thanks for opening it

thaJeztah · 2018-07-18T14:25:03Z

daemon/config/config_common_unix.go

Was thinking if we should do this "on demand", e.g.:

var systemdResolvedRunning *bool func systemdResolvedPresent() bool { if systemdResolvedRunning != nil { return systemdResolvedRunning } systemdResolvedRunning = false pids, err := procfs.PidOf("systemd-resolved") if err != nil { logrus.Errorf("unable to check systemd-resolved status: %s", err) } if len(pids) > 0 && pids[0] > 0 { isSystemdResolvedPresent = true } return systemdResolvedRunning }

But also noticed that would only need this if conf.ResolvConf == ''; which will be only once (once GetResolvConf() is called, and it's not set, it will be set to conf.ResolvConf = defaultResolvConf)

So perhaps we can just as well inline this in GetResolvConf() ? (or a combination of the above)

I think is expensive to check: procfs.PidOf("systemd-resolved") for every container creation

Can we do this during daemon initialization instead of package init?

sure don't have a, is there a specific point that you can suggest?
Just drop something like this one: https://github.com/moby/moby/blob/master/daemon/daemon.go#L569

As good a place as any, I suppose.

thaJeztah · 2018-07-18T14:26:34Z

vendor.conf

Looks like this needs vendoring; let me add that label, otherwise people may overlook that

yep, I put it in this comment: #37485 (comment)

fcrisciani · 2018-07-18T23:29:21Z

@cpuguy83 wdyt of this one?

AkihiroSuda · 2018-07-20T01:20:03Z

daemon/daemon_linux.go

no else, line 152 (https://github.com/moby/moby/pull/37485/files/e241a0d3a74fc3b37f597464951187e132bbd7ac#diff-857979ffd7f9baf3aa83894d548f110aR152) is setting the value

even if alternateResolvConf is present, "alternateResolvConf is not present" is printed here?

sorry was looking at the wrong line

Signed-off-by: Flavio Crisciani <[email protected]>

Handle the case of systemd-resolved, and if in place use a different resolv.conf source. Set appropriately the option on libnetwork. Move unix specific code to container_operation_unix Signed-off-by: Flavio Crisciani <[email protected]>

Signed-off-by: Flavio Crisciani <[email protected]>

thaJeztah

two questions/notes, but otherwise looks good

thaJeztah · 2018-07-31T19:06:44Z

vendor/github.com/docker/libnetwork/osl/namespace_linux.go

@@ -394,13 +394,10 @@ func (n *networkNamespace) InvokeFunc(f func()) error {
 // InitOSContext initializes OS context while configuring network resources
 func InitOSContext() func() {


Did this file end up in the wrong commit?

thaJeztah · 2018-07-31T19:08:20Z

daemon/container_operations.go


 	if container.HostConfig.NetworkMode.IsHost() {
 		sboxOptions = append(sboxOptions, libnetwork.OptionUseDefaultSandbox())
-		if len(container.HostConfig.ExtraHosts) == 0 {


This code is not used for LCOW?

Oh, just see @johnstep's comment; so ignore this one; only check if the vendored file ended up in the right commit

@johnstep can you confirm the LCOW behavior here?

This code path cannot be hit on Windows (including LCOW) because IsHost always returns false on Windows.

thaJeztah

let me LGTM

thaJeztah · 2018-08-01T12:52:28Z

z failure is unrelated; merging

nickbroon · 2018-10-23T11:54:50Z

Is this likely to appear in the next release of docker-ce?

nickbroon · 2018-10-23T12:11:03Z

Answering my own question: Yes, this appears to be in the 18.09 beta.

kenden · 2018-11-05T16:12:16Z

List of commits in the beta: https://github.com/docker/docker-ce/releases/tag/v18.09.0-rc1

GordonTheTurtle added the status/0-triage label Jul 17, 2018

fcrisciani changed the title ~~Resolv~~ Systemd-resolved proper handling Jul 17, 2018

fcrisciani mentioned this pull request Jul 17, 2018

Proper handling sandbox options moby/libnetwork#2232

Merged

fcrisciani force-pushed the resolv branch 2 times, most recently from 7750bed to a9cd9df Compare July 17, 2018 19:14

johnstep approved these changes Jul 17, 2018

View reviewed changes

fcrisciani force-pushed the resolv branch 3 times, most recently from d1b4b19 to 643787c Compare July 17, 2018 23:23

yongtang added status/2-code-review and removed status/0-triage labels Jul 18, 2018

thaJeztah requested changes Jul 18, 2018

View reviewed changes

thaJeztah added area/systemd area/networking Networking status/needs-vendoring labels Jul 18, 2018

thaJeztah mentioned this pull request Jul 18, 2018

Cannot resolve host on docker build #16600

Closed

fcrisciani force-pushed the resolv branch from 643787c to e241a0d Compare July 18, 2018 23:28

AkihiroSuda reviewed Jul 20, 2018

View reviewed changes

fcrisciani force-pushed the resolv branch from e241a0d to 06e0231 Compare July 25, 2018 18:11

fcrisciani requested a review from tianon as a code owner July 25, 2018 18:11

fcrisciani force-pushed the resolv branch 5 times, most recently from 22bd786 to cbf69f0 Compare July 26, 2018 18:10

Flavio Crisciani added 2 commits July 26, 2018 11:17

Add a way to get a pid of a process

0fc52ca

Signed-off-by: Flavio Crisciani <[email protected]>

Fixes for resolv.conf

e353e7e

Handle the case of systemd-resolved, and if in place use a different resolv.conf source. Set appropriately the option on libnetwork. Move unix specific code to container_operation_unix Signed-off-by: Flavio Crisciani <[email protected]>

Vendor libnetwork

fc4ebe0

Signed-off-by: Flavio Crisciani <[email protected]>

fcrisciani force-pushed the resolv branch from cbf69f0 to fc4ebe0 Compare July 26, 2018 18:18

thaJeztah requested changes Jul 31, 2018

View reviewed changes

thaJeztah removed the status/needs-vendoring label Jul 31, 2018

thaJeztah approved these changes Jul 31, 2018

View reviewed changes

GordonTheTurtle assigned MHBauer Aug 1, 2018

thaJeztah merged commit e158451 into moby:master Aug 1, 2018

thaJeztah mentioned this pull request Aug 2, 2018

getaddrinfo ENOTFOUND #14755

Closed

fcrisciani mentioned this pull request Sep 11, 2018

DNS broken out-of-the-box on Ubuntu Artful moby/libnetwork#2068

Closed

kenden mentioned this pull request Nov 5, 2018

Ubuntu 18.04 + Docker 17.12.1-ce break DNS resolution moby/libnetwork#2187

Closed

thaJeztah mentioned this pull request Jan 10, 2019

Cannot access .local domains in Docker #38531

Open

CecileRobertMichon mentioned this pull request Jan 14, 2019

feat: add support for Ubuntu 18.04-LTS Azure/aks-engine#223

Merged

4 tasks

hacst mentioned this pull request Jan 31, 2019

Default docker-daemon configuration can fail to find correct DNS canonical/microk8s#252

Closed

This was referenced Mar 28, 2019

Fix: network=host using wrong resolv.conf with systemd-resolved #38579

Merged

[18.09 backport] Fix: network=host using wrong resolv.conf with systemd-resolved docker-archive/engine#180

Merged

tiborvass mentioned this pull request Jun 3, 2019

resolvconf: use /run/systemd/resolve/resolv.conf if systemd-resolved manages DNS moby/libnetwork#2385

Merged

nielsvz mentioned this pull request Oct 15, 2019

Adding new Registry Endpoint (docker-hub) results in Failed to ping endpoint error on Photon OS 3.0 goharbor/harbor#9218

Closed

thaJeztah mentioned this pull request May 25, 2020

Better selection of DNS server #41022

Merged

thaJeztah mentioned this pull request Aug 19, 2020

[19.03 backport] Better selection of DNS server #41374

Closed

		@@ -394,13 +394,10 @@ func (n *networkNamespace) InvokeFunc(f func()) error {
		// InitOSContext initializes OS context while configuring network resources
		func InitOSContext() func() {

Conversation

fcrisciani commented Jul 17, 2018

Uh oh!

fcrisciani commented Jul 17, 2018

Uh oh!

johnstep left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fcrisciani commented Jul 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 1, 2018

Uh oh!

nickbroon commented Oct 23, 2018

Uh oh!

nickbroon commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kenden commented Nov 5, 2018

Uh oh!

Reviewers

Assignees

codecov bot commented Jul 17, 2018 •

edited

Loading

nickbroon commented Oct 23, 2018 •

edited

Loading