Skip to content
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

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

Merged
merged 2 commits into from Jun 4, 2019

Conversation

tiborvass
Copy link
Contributor

@tiborvass tiborvass commented May 31, 2019

Signed-off-by: Tibor Vass tibor@docker.com

procfs was copied from https://github.com/moby/moby/blob/e353e7e/daemon/daemon_linux.go#L142-L167. It was a package that was only used for this purpose. It is being removed in moby/moby#39295. It was added in moby in moby/moby#37485

This PR is updated according to the comment below #2385 (comment)

@tiborvass
Copy link
Contributor Author

Fixed ineffassign linter issue. And added testutils import in the procfs test, because apparently that's needed...

"regexp"
"strings"
"sync"

"github.com/docker/docker/pkg/ioutils"
"github.com/docker/libnetwork/procfs"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a copy of that small package, which I'd rather have than depend on k8s.io/kubernetes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was copied at the time to prevent having to pull the whole kubernetes repo on each vendor update.

@@ -11,8 +11,7 @@ RUN go get golang.org/x/lint/golint \
golang.org/x/tools/cmd/cover \
github.com/mattn/goveralls \
github.com/gordonklaus/ineffassign \
github.com/client9/misspell/cmd/misspell \
honnef.co/go/tools/cmd/gosimple
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this cmd failing consistently ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, try to go get honnef.co/go/tools/cmd/gosimple

resolvconf/resolvconf.go Outdated Show resolved Hide resolved
resolvconf/resolvconf.go Outdated Show resolved Hide resolved
if len(pids) > 0 && pids[0] > 0 {
_, err := os.Stat(alternatePath)
if err == nil {
logrus.Infof("systemd-resolved is running, so using resolvconf: %s", alternatePath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we should use systemd-resolved config only if /etc/resolv.conf points to a loopback address?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. when running with RootlessKit+slirp4netns/VPNKit, systemd-resolved would be visible (unless PIDNS is unshared) but /etc/resolv.conf would point to slirp4netns/VPNKit-builtin DNS.
Even in that case, using systemd DNS should be fine for almost all usecases, but not 100% sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the correct behavior is. After reading a bit about systemd-resolved i was surprised of the current behavior in moby, but this PR is about bringing whatever we have in moby to libnetwork in order to fix a difference between buildkit and non-buildkit builds.
If you think this won't work with rootlesskit, I suggest we fix it in a follow up.

@tiborvass
Copy link
Contributor Author

tiborvass commented Jun 1, 2019

@arkodg @selansen updated, PTAL

@tiborvass tiborvass force-pushed the resolvconf-systemd branch 2 times, most recently from 14d5651 to 4a75b1b Compare June 1, 2019 19:24
Copy link
Collaborator

@selansen selansen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@justincormack
Copy link
Contributor

This seems totally the wrong way to do this.

You can identify whether systemd-resolved is being used by whether /etc/resolv.conf is a symlink to /run/systemd/resolve/stub-resolv.conf or if it contains 127.0.0.53 as the DNS server, full details in https://www.freedesktop.org/software/systemd/man/systemd-resolved.service.html - this should be sanely detectable.

It is never going to work properly to just see if it is running as it could be running in a container or not activated or anything.

@thaJeztah
Copy link
Member

I seem to recall that the symlink is created only in upgrade scenarios, but not in fresh installs (or the other way round).

@tiborvass
Copy link
Contributor Author

@justincormack I agree with you that the logic is super weird and I think we should fix it. This PR is not about fixing that, it's about moving code here so it can be used by both buildkit and moby to fix a discrepancy between legacy and buildkit builder.

So I suggest we merge this and open up a new one to fix the behavior for all.

@tiborvass
Copy link
Contributor Author

For the record this was the original PR: moby/moby#37485 I'm adding a reference in the description of this PR as well.

@tiborvass
Copy link
Contributor Author

@mavenugo @justincormack ok I read https://www.freedesktop.org/software/systemd/man/systemd-resolved.service.html#/etc/resolv.conf

I'm trying to understand the proper logic here. We can obviously not use 127.0.0.53 inside the container.

So question Q1: do you agree that IF it is detected that systemd-resolved handles DNS, THEN, /run/systemd/resolve/resolv.conf should be read to find out what are the actual nameservers, despite the note in the docs:

Note that /run/systemd/resolve/resolv.conf should not be used directly by applications, but only through a symlink from /etc/resolv.conf.

Question Q2: do you agree that in order to detect whether systemd-resolved handles DNS, I would need to check if EITHER /etc/resolv.conf is a symlink to any of /run/systemd/resolve/stub-resolv.conf, /usr/lib/systemd/resolv.conf, /run/systemd/resolve/resolv.conf, OR /etc/resolv.conf ONLY contains 127.0.0.53 as a nameserver.

If you answered YES for Q1 and Q2, then Q3: do you agree that we don't need to check symlinks at all, we could simplify the logic to: if /etc/resolv.conf ONLY contains 127.0.0.53 as a nameserver, then read /run/systemd/resolve/resolv.conf, otherwise read /etc/resolv.conf.

In particular Q3 implies that if /etc/resolv.conf does not contain 127.0.0.53 as the sole nameserver then it could still be a symlink to /run/systemd/resolve/resolv.conf, in which case reading /etc/resolv.conf is the intended logic.

@tiborvass tiborvass changed the title resolvconf: use /run/systemd/resolve/resolv.conf if systemd-resolved is running resolvconf: use /run/systemd/resolve/resolv.conf if systemd-resolved manages DNS Jun 4, 2019
@tiborvass
Copy link
Contributor Author

tiborvass commented Jun 4, 2019

I updated the PR assuming you agree with my comment above @justincormack @mavenugo @arkodg

@tiborvass tiborvass force-pushed the resolvconf-systemd branch 2 times, most recently from 330a3fc to bea9524 Compare June 4, 2019 04:50
Tibor Vass added 2 commits June 4, 2019 04:50
…manages DNS

Signed-off-by: Tibor Vass <tibor@docker.com>
Also fixes issue reported by ineffassign

Signed-off-by: Tibor Vass <tibor@docker.com>
@justincormack
Copy link
Contributor

Yes this looks way better as it is much simpler! There are still configs in which DNS will not work, eg /etc/resolv.conf points to a caching nameserver on 127.0.0.1 but they didn't work before anyway so this is no worse than the current situation.

Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for rewriting the path logic !

@thaJeztah
Copy link
Member

Should we backport this to 18.09 (or would that not be useful without the buildkit changes?)

@tiborvass
Copy link
Contributor Author

@thaJeztah yeah it's not worth the complications I think.

@superseed
Copy link

superseed commented Jun 25, 2019

@tiborvass I opened #moby/moby#38243 when 18.09 was released because this update included "support for systemd-resolved" and ended up being a regression on my system as I suddenly lost the ability to resolve names available only on one of my interfaces' DNS. See the issue for more details, but the gist of it is that using /run/systemd/resolve/resolv.conf as a "resolvconf style source for DNS servers" hides the complexity that can be expressed by systemd-resolved: in my case, per-interface DNS servers. Queries should be sent to 127.0.0.53 when resolved manages the system, not to the servers listed in /run/systemd/resolve/resolv.conf.

I don't know if this PR is exactly related to the issue but given that it talks of using /run/systemd/resolve/resolv.conf, it seems like it is. In my case, it's for non-default networks (where the containers contain a 127.0.0.11 nameserver in their resolv.conf which from what I get, is an internal resolver in Docker)

I've had to pin Docker to version 18.06 ever since, which is a situation I'd like to get out of if possible.

@thaJeztah
Copy link
Member

Resolving using 127.0.0.53 from within the container would only be possible in the "custom network" situation, in which case the docker embedded DNS (127.0.0.11) can forward the requests to 127.0.0.53

In the default bridge network, the embedded DNS is not used (for historic reasons / backward compatibility); in that case using 127.0.0.53 won't be possible (so I guess "best alternative" would be to try the DNS servers listed in /run/systemd/resolve/resolv.conf - even if that means you won't be able to make use of the advanced functionality of resolved.

@thaJeztah
Copy link
Member

That said; perhaps we should revisit the "default bridge network doesn't use the embedded DNS"; also see moby/moby#22295

@superseed
Copy link

@thaJeztah I am indeed in a situation where I use custom networks (type bridge, but not the default one: it's created by docker-compose) and resolution fails since the host' 127.0.0.53 ends up not getting the queries from the Docker embedded DNS as it bypasses it since 18.09 when its behavior seems to have been changed to "use the servers in /run/systemd/resolve/resolv.conf when systemd-resolved exists" from "use the resolver in /etc/resolv.conf". Only systemd-resolved knows what DNS server to use for what domains and that can't be found in /run/systemd/resolve/resolv.conf as it's not translatable to resolvconf logic.

Also huge +1 for the idea of revisiting the default bridge network behavior, or at least letting it be opt-in through configuration :)

@thaJeztah
Copy link
Member

@superseed could you perhaps open a ticket in moby/moby that captures the conversation above? (just so that it doesn't get lost)?

@tiborvass
Copy link
Contributor Author

@superseed thanks for bringing this up to our attention. This PR was to bring the behavior that was in master to libnetwork. But indeed it is worth discussing any improvements or regression fixes. My understanding is that moby/moby#38243 is the issue for a regression in 18.09 from 18.06.

@superseed
Copy link

@thaJeztah Oh, I created the issue I mentioned above (#moby/moby#38243) for it. Do you want me to make changes to it/open another one on another project, or is it enough?

@tiborvass Yep, exactly. Sorry for discussing the issue in the wrong place, my search was pretty fuzzy and I don't know exactly how moby/docker components projects are related so I thought this PR might be it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants