Created attachment 290941 [details] [review]
proof of concept patch to turn /etc/resolv.conf into a symlink

The patch does the following:

1) NetworkManager no longer tries to find the target path of the /etc/resolv.conf symlink (if it is a symlink at all). That's important because the symlink may link to other daemons file in its /run directory and NetworkManager would thus overwrite other deamons data.

2) NetworkManager writes data to its own private resolv.conf located in /run/NetworkManager and still writes to a .tmp file and rename it.

3) NetworkManager unlinks /etc/resolv.conf whether it is a symlink or an ordinary file and creates a /etc/resolv.conf symlink pointing to its private resolv.conf.

(In reply to comment #1)
> Created an attachment (id=290941) [details] [review]
> proof of concept patch to turn /etc/resolv.conf into a symlink

looks good

> 3) NetworkManager unlinks /etc/resolv.conf whether it is a symlink or an
> ordinary file and creates a /etc/resolv.conf symlink pointing to its private
> resolv.conf.

How about touching the symlink only if it does not yet point to NM's resolv.conf?

And if we rewrite the symlink, how about doing it atomically too (by creating it first with a temporary name "/etc/.resolv.conf.NetworkManager").

lrintel pointed out to be careful about selinux labels.

I guess, that is relevant both all files in play, such as the /etc/resolv.conf symlink, /run/NetworkManager/resolv.conf.tmp, /run/NetworkManager/resolv.conf, etc.





Also, how does it work with inotify if you want to monitor changes of resolv.conf?

Obviously, it is already common to have /etc/resolv.conf a symlink. But is there some special care we should take to best support inotify on symlinked resolv.conf? (I don't know).

(In reply to comment #3)
> lrintel pointed out to be careful about selinux labels.
> 
> I guess, that is relevant both all files in play, such as the /etc/resolv.conf
> symlink, /run/NetworkManager/resolv.conf.tmp, /run/NetworkManager/resolv.conf,
> etc.

See https://bugzilla.redhat.com/show_bug.cgi?id=1166071

> Also, how does it work with inotify if you want to monitor changes of
> resolv.conf?

See https://bugzilla.redhat.com/show_bug.cgi?id=1116999#c7

> Obviously, it is already common to have /etc/resolv.conf a symlink. But is
> there some special care we should take to best support inotify on symlinked
> resolv.conf? (I don't know).

There's actually one. You might want to replace the symlink on every change even if it already points to the right target, just in case there's an app that just watches for /etc/resolv.conf replacement and doesn't care about the target of the symlink.

But for example systemd-resolved would be unaffected as it's using stat() instead of inotify. I'll check with glibc as there it might be a bit more interesting.

Created attachment 291088 [details] [review]
patch to turn /etc/resolv.conf into a symlink

Some details from IRC discussion:

1) We want to always replace /etc/resolv.conf symlink just in case someone is watching inotify to see when it gets replaced and doesn't care that it's a symlink.

2) We want to avoid the gap between removing old /etc/resolv.conf and creating the symlink. The new patch achieves that through the use of a temporary file name for the symlink and the call to `rename()`.

3) There are other possible improvements in this area that would be discussed later. That includes writing out the private resolv.conf always (regardless of the dns=xxx setting) and using a separate private resolv.conf for dnsmasq. NetworkManater might also want to check the symlink to see whether it indicates another service is in charge of it.

Please review.

(In reply to comment #5)
> Created an attachment (id=291088) [details] [review]
> patch to turn /etc/resolv.conf into a symlink

how about explicitly setting the selinux attributes of MY_RESOLV_CONF_TMP before rename?

We could either:

(a) copy the security context from MY_RESOLV_CONF over to MY_RESOLV_CONF_TMP,

(b) or create MY_RESOLV_CONF_TMP with the default security context (similar to http://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/settings/plugins/ifcfg-rh/plugin.c?id=1706bd0308e14607dbe55192a071c21a3a512468#n671 )


(if (a) fails because the file does not exist, we would have to fallback to (b))




Otherwise LGTM

(In reply to comment #7)
> (In reply to comment #5)
> > Created an attachment (id=291088) [details] [review] [details] [review]
> > patch to turn /etc/resolv.conf into a symlink
> 
> how about explicitly setting the selinux attributes of MY_RESOLV_CONF_TMP
> before rename?
> 
> We could either:
> 
> (a) copy the security context from MY_RESOLV_CONF over to MY_RESOLV_CONF_TMP,

I wouldn't copy security context from existing files. It might be wrong for various reasons.

> (b) or create MY_RESOLV_CONF_TMP with the default security context (similar to
> http://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/settings/plugins/ifcfg-rh/plugin.c?id=1706bd0308e14607dbe55192a071c21a3a512468#n671
> )

I would be perfectly ok with changing the policy so that selinux can cope without NM intervention. But if you think it will be better to do it in the code, feel free to do so.

Created attachment 291121 [details] [review]
patch acked by thaller

Waiting for comments from dcbw.

Not related specifically to this patch, but I can imagine that DNS plugins or external processes really do want to know about the full picture of DNS.  resolv.conf's format simply can't provide that.  But these can be written in addition to the private resolv.conf.

What I mean is that other external tools likely want to know the following:

1) interface name/IP addresses that the DNS information applies to
2) DNS servers for the interface's connection
3) search domains/domain name for the connection (eg, for split DNS)
4) what NetworkManager thinks the priority of this connection should be in the DNS list

This information would allow external tools to construct a much more complete DNS picture, including for split DNS and VPN connections, etc, than resolv.conf itself can provide.

Created attachment 291133 [details] [review]
dns-error-cleanup.patch

Before the previous patch, lets do this patch.  It's icky to check *error because that requires the caller to pass an error, and they may not always do that.  That's what g_propagate_error() is for.

Although I like the general idea, I worry about changing the default behavior this close to release, both because of bugs, and because we haven't taken much time to think through all the details.

Eg, AFAICT, this patch doesn't actually do anything to support the "let other people take over resolv.conf" use case which was given as one of the primary reasons for doing it in comment 0...

(In reply to comment #11)
> Created an attachment (id=291133) [details] [review]
> dns-error-cleanup.patch
> 
> Before the previous patch, lets do this patch.  It's icky to check *error
> because that requires the caller to pass an error, and they may not always do
> that.  That's what g_propagate_error() is for.

Hmm disregard this patch, it's not as simple as I thought plus the function enforces that callers pass 'error' anyway.

(In reply to comment #10)
> Not related specifically to this patch, but I can imagine that DNS plugins or
> external processes really do want to know about the full picture of DNS. 
> resolv.conf's format simply can't provide that.  But these can be written in
> addition to the private resolv.conf.

That's true. The unbound plugin still queries the DNS information using D-Bus which also has the disadvantage that it needs to be done asynchronously *after* the unbound plugin returns control to NetworkManager. This way NetworkManager can't be informed about completed DNS configuration and can't for example delay the published state so that apps can wait for it.

> What I mean is that other external tools likely want to know the following:
> 
> 1) interface name/IP addresses that the DNS information applies to
> 2) DNS servers for the interface's connection
> 3) search domains/domain name for the connection (eg, for split DNS)
> 4) what NetworkManager thinks the priority of this connection should be in the
> DNS list

What we currently use in unbound is just the mapping of search domains and name servers for split DNS, and the global name servers which we decide autonomously from the connection name servers with a couple of simple rules. I'll be more than happy for closer collaboration to avoid having to deduce all of this from the connection data.

Apart from what's written here we're using the connection type and information whether the connection is VPN and we have configuration knobs in /etc/dnssec.conf whether to use that information at all and whether to use it when it comes form a wireless device. Either NetworkManager or the unbound plugin or dnssec-trigger-script has to read /etc/resolv.conf and act accordingly.

We also want to get more information from the administrator.

Please see:

https://bugzilla.gnome.org/show_bug.cgi?id=699810
https://bugzilla.gnome.org/show_bug.cgi?id=708829

Might be good to switch this discussion to the above bugs or a newly created one as this bug is simply about using the symlink for /etc/resolv.conf the simplest possible way that is ideally acceptable for 1.0.

I'd start new bug reports for the other building blocks.

(In reply to comment #12)
> Although I like the general idea, I worry about changing the default behavior
> this close to release, both because of bugs, and because we haven't taken much
> time to think through all the details.

That is why I'm trying to keep the patch as simple and failsafe as possible.

> Eg, AFAICT, this patch doesn't actually do anything to support the "let other
> people take over resolv.conf" use case which was given as one of the primary
> reasons for doing it in comment 0...

That's just a consequence of the above. I didn't want to change the behavior in ways that might be problematic so quickly. This patch just changes /etc/resolv.conf into a symlink which has been also done by other tools.

The patch is also very important for the user experience with dnssec-trigger. Without it (or other missing NM features like re-reading resolv.conf) we are forced to restart NetworkManager when stopping dnssec-trigger using:

    systemctl --ignore-dependencies try-restart NetworkManager

The feature described in comment #0 requires behavior changes that have to be considered carefully.

I still propose to include the patch in 1.0 in order to at least use a symlink in 1.0 so this user visible change doesn't have to be done later. I would then stage a behavior change towards the feature in comment #0 for a later release.

This approach allows us a bit more flexibility later, as we can then avoid a user visible change. Also a behavioral change to avoid switching /etc/resolv.conf from e.g. resolved would be a very simple patch adding code before the "unlink" call that would check the symlink in a defined way and exit the function if the decision is to leave it as is.

(note: bugzilla.redhat.com's use of NEEDINFO is non-standard; in other bugzillas (such as this one), NEEDINFO *always* means "the developers need info from the users", and so marking a bug NEEDINFO causes it to disappear from the default queries/bug lists (under the theory that the developers don't need to think about it again until the reporter has responded). So don't mark bugs NEEDINFO when you mean you want the developers to respond.)

(In reply to comment #14)
> The patch is also very important for the user experience with dnssec-trigger.
> Without it (or other missing NM features like re-reading resolv.conf) we are
> forced to restart NetworkManager when stopping dnssec-trigger

Sure, but it doesn't address the fact that NM will still overwrite resolv.conf any time the network configuration changes[1], and then dnssec-trigger needs to hope it can fix it again before anyone makes an unsafe DNS query. So it's like, we're pretending to be more friendly to the idea of sharing resolv.conf, but we're not actually being more friendly.

It would not be much more code to change things so that we only re-link /etc/resolv.conf if one of:

  a) it doesn't exist
  b) it's a plain file
  c) it's a symlink to a file that doesn't exist
  d) it's a symlink to MY_RESOLV_CONF

Then dnssec-trigger could just replace the link with a link to its own private file, and link it back on exit. Or something. (Although then what happens if dnssec-trigger crashes without cleaning up? Or what if systemd-resolved was running when dnssec-trigger started, but has since handed things off to NetworkManager?)

>+#define MY_RESOLV_CONF "/run/NetworkManager/resolv.conf.default"

Use NMRUNDIR, and I think it should be just "resolv.conf", not "resolv.conf.default".



[1] Since NM rewrites resolv.conf any time the set of available DNS servers changes, dnssec-trigger could force a rewrite by adding a dummy interface, activating a temporary connection on it, and then removing it...

(In reply to comment #16)
> Sure, but it doesn't address the fact that NM will still overwrite resolv.conf
> any time the network configuration changes[1], and then dnssec-trigger needs to
> hope it can fix it again before anyone makes an unsafe DNS query.

Indeed.

> So it's like, we're pretending to be more friendly to the idea of sharing resolv.conf,

We're not pretending, we're performing the first stage of changes needed to implement the feature in comment #0 that has some other advantages as well.

> It would not be much more code

Indeed, that's what I already said. After this patch the code is ready for simple insertion of code that will perform the checking. Is there anything that stops us from pushing the first stage to master to be done with it and focus on what needs to be done for the checking?

> to change things so that we only re-link
> /etc/resolv.conf if one of:
> 
>   a) it doesn't exist

Yes.

>   b) it's a plain file

Yes.

>   c) it's a symlink to a file that doesn't exist

Not sure. A missing file just says the service doesn't provide resolv.conf right now, not that it's not supposed to provide it in a couple of seconds or something like that. IMO this is against what Lennart wants to achieve.

This is exactly why I first prepared an essentially non-controversial patch that could be easily accepted and then focus on the details of the checks. Especially when I was told that there's not much time to get things to nm-1.0 and I would be glad to at least get the symlink there.

But if the developers are able to reach a consensus *now*, I'll gladly write the second patch (or basically anyone can add the checks easily) and will squash those two patches if so requested.

>   d) it's a symlink to MY_RESOLV_CONF

Yes. That's to propagate updates to inotify /etc/resolv.conf.

> Then dnssec-trigger could just replace the link with a link to its own private
> file, and link it back on exit.

It's exactly what it does if you use the current git and configure it so in /etc/dnssec.conf (patches got merged a couple of hours ago).

> Although then what happens if dnssec-trigger crashes without cleaning up?

Then systemd will run the clean up action anyway.

> Or what if systemd-resolved was running when dnssec-trigger started, but has since handed things off to
> NetworkManager?)

dnssec-trigger-script functionality mostly depends on NetworkManager. The current code thus expect NetworkManager and gives control back to it (using a hardcoded target path /etc/NetworkManager/resolv.conf.default).

This is subject to change, though. If Lennarts' proposal was implemented in full, then the installer or the administrator would set the symlink and none of the tools would change it. In that case dnssec-trigger daemon may even be run only when /etc/resolv.conf links to its private resolv.conf. But that would of course only work when other tools don't rewrite /etc/resolv.conf just because it points to a nonexistant path.

> >+#define MY_RESOLV_CONF "/run/NetworkManager/resolv.conf.default"

Definitely, I'll change it once a final decision on the name is delivered.

> Use NMRUNDIR, and I think it should be just "resolv.conf", not
> "resolv.conf.default".

The change from resolv.conf to resolv.conf.default was the result of an IRC discussion. Please people talk and come up with just one decision.

> [1] Since NM rewrites resolv.conf any time the set of available DNS servers
> changes, dnssec-trigger could force a rewrite by adding a dummy interface,
> activating a temporary connection on it, and then removing it...

That's a slightly better workaround than restarting, as it keeps other connections running. But the symlinks feel much better, especially when we need them for other stuff as well.

Created attachment 291180 [details] [review]
0001-dns-manager-make-etc-resolv.conf-a-symlink-to-run-Ne.patch

Created attachment 291181 [details] [review]
0002-dns-manager-MY_RESOLV_CONF-is-resolv.conf-not-resolv.patch

Created attachment 291182 [details] [review]
0003-dns-manager-don-t-replace-etc-resolv.conf-installed-.patch

The new patchset seems to work for me. The second patch is optional, depending on the decision on the resolv.conf name. NetworkManager no longer attempts to rewrite /etc/resolv.conf installed by dnssec-trigger, which is indeed nice. We don't replace the link even if it points to a missing file as explained in comment #17.

I haven't tested the last patch thoroughly, though. Please review.

Review of attachment 291182 [details] [review]:

::: src/dns-manager/nm-dns-manager.c
@@ +494,3 @@
+	if (lstat (_PATH_RESCONF, &st) != -1) {
+		/* Don't overwrite a symbolic link. */
+		if (S_ISLNK(st.st_mode)) {

Space before (

@@ +498,3 @@
+				char path[PATH_MAX];
+
+				if (g_strcmp0(realpath(_PATH_RESCONF, path), MY_RESOLV_CONF) != 0)

Spaces before (

Other than that I have no other issues with these patches.  The only question is whether it's appropriate or not this close to 1.0.

One question I still have is what happens on reboot when resolv.conf is a dangling symlink until NM starts?  I'm not sure anything guarantees that resolv.conf is a valid file during bootup, so if /run is gone resolv.conf won't point to anything, and stuff might be unhappy with that during bootup because they can't even map 127.0.0.1 to 'localhost', right?

(In reply to comment #24)
> Other than that I have no other issues with these patches.  The only question
> is whether it's appropriate or not this close to 1.0.
> 
> One question I still have is what happens on reboot when resolv.conf is a
> dangling symlink until NM starts?  I'm not sure anything guarantees that
> resolv.conf is a valid file during bootup, so if /run is gone resolv.conf won't
> point to anything, and stuff might be unhappy with that during bootup because
> they can't even map 127.0.0.1 to 'localhost', right?

localhost should generally not be resolved using DNS, and NetworkManager provides an implementation of the network-online.target those days. So I wouldn't personally see a problem here.

(In reply to comment #16)
> we're pretending to be more friendly to the idea of sharing resolv.conf, but
> we're not actually being more friendly.

To be honest we were pretending it by the commit message in the original version of the patch that was wrong. It is fixed now and only the second patch implementing the /etc/resolv.conf pre-rewrite checks is marked as actually resolving the bug.

Created attachment 291194 [details] [review]
0001-dns-manager-make-etc-resolv.conf-a-symlink-to-run-Ne.patch

Added missing spaces.

(In reply to comment #27)
> Created an attachment (id=291194) [details] [review]
> 0001-dns-manager-make-etc-resolv.conf-a-symlink-to-run-Ne.patch
> 
> Added missing spaces.

Wrong, just added back the bugs as "Related". Upcoming patch 0003 will fix the missing spaces.

Created attachment 291195 [details] [review]
0003-dns-manager-don-t-replace-etc-resolv.conf-installed-.patch

Created attachment 291204 [details] [review]
0001-dns-manager-make-etc-resolv.conf-a-symlink-to-run-Ne.patch

Created attachment 291205 [details] [review]
0002-dns-manager-don-t-replace-etc-resolv.conf-installed-.patch

Created attachment 291206 [details] [review]
0002-dns-manager-don-t-replace-etc-resolv.conf-installed-.patch

So we agreed on resolv.conf on IRC, squashed the two patches. The remaining questions are:

1) Whether to include the first patch in 1.0 or not.

I'm definitely for because it will help dnssec-trigger-script. Also the the patch is basically a rewrite of the function, so it would help with future patching to get it there.

2) If yes, whether to include the second as well.

I would.

Please let me know how you decided and feel free to push the patches to master and close the bug.

I am ok with the patch, and I would include it to 1.0.



Only a minor note (sorry for coming up with it so late :$) ...

I dislike realpath() accepting PATH_MAX sized buffer.
I would avoid it altogether by doing:

               if (stat (_PATH_RESCONF, &st) != -1) {
                    char *path = NULL;
 
                    path = g_file_read_link (_PATH_RESCONF, NULL);
                    if (g_strcmp0 (path, MY_RESOLV_CONF) != 0) {
                         g_free (path);
                         return TRUE;
                    }
                    g_free (path);
               } else {

Created attachment 291350 [details] [review]
patch to use g_file_read_link(), to be squashed

Agreed. This should work.

(In reply to comment #34)
> Created an attachment (id=291350) [details] [review]
> patch to use g_file_read_link(), to be squashed
> 
> Agreed. This should work.

LGTM!

Created attachment 291633 [details] [review]
0002-dns-manager-don-t-replace-etc-resolv.conf-installed-.patch

Just did the squashing.

So I'm good with the patches now, but I don't think it's 1.0 material right now. I think we can cherry-pick it to 1.0.x though after the release goes out so that it gets into the next point release.

Definitely OK for git master though, so please push to git master and close?

(In reply to comment #37)
> Definitely OK for git master though, so please push to git master and close?

Done.