Re: [libvirt] [PATCH v2 5/7] network: set firewalld zone of bridges to "libvirt" zone when appropriate

On 2/1/19 8:24 AM, Daniel P. Berrangé wrote: > On Thu, Jan 31, 2019 at 08:24:56PM -0500, Laine Stump wrote: > > From: Laine Stump <laine(a)redhat.com&gt; > > > > This patch restores broken guest network connectivity after a host > > firewalld is switched to using an nftables backend. It does this by > > adding libvirt networks' bridge interfaces to the new "libvirt" zone > > in firewalld. > > > > After this patch, the bridge interface of any network created by > > libvirt (when firewalld is active) will be added to the firewalld > > zone called "libvirt" if it exists (regardless of the firewalld > > backend setting). This behavior does *not* depend on whether or not > > libvirt has installed the libvirt zone file (set with > > "--with[out]-firewalld-zone" during the configure phase of the package > > build). > > > > If the libvirt zone doesn't exist (either because the package was > > configured to not install it, or possibly it was installed, but > > firewalld doesn't support rule priorities, resulting in a parse > > error), the bridge will remain in firewalld's default zone, which > > could be innocuous (in the case that the firewalld backend is > > iptables, guest networking will still function properly with the > > bridge in the default zone), or it could be disastrous (if the > > firewalld backend is nftables, we can be assured that guest networking > > will fail). In order to be unobtrusive in the former case, and > > informative in the latter, when the libvirt zone doesn't exist we > > then check the firewalld version to see if it's new enough to support > > the nftables backend, and then if the backend is actually set to > > nftables, before logging an error (and failing the net-start > > operation, since the network couldn't possibly work anyway). > > > > When the libvirt zone is used, network behavior is *slightly* > > different from behavior of previous libvirt. In the past, libvirt > > network behavior would be affected by the configuration of firewalld's > > default zone (usually "public"), but now it is affected only by the > > "libvirt" zone), and thus almost surely warrants a release note for > > any distro upgrading to libvirt 5.1 or above. Although it's > > unfortunate that we have to deal with a mandatory behavior change, the > > architecture of multiple hooks makes it impossible to *not* change > > behavior in some way, and the new behavior is arguably better (since > > it will now be possible to manage access to the host from virtual > > machines vs from public interfaces separately). > > > > Resolves: https://bugzilla.redhat.com/1638342 > > Creates-and-Resolves: https://bugzilla.redhat.com/1650320 > > Signed-off-by: Laine Stump <laine(a)laine.org&gt; > > --- > > > > NB: I had considered that it might be useful to cache the results of > > checking the list of active zones, the firewalld version, or the > > firewalld backend, but in my tests of restarting libvirtd with 100 > > active networks, the full startup time (from the beginning of > > "systemctl restart libvirtd.service" until successful return from a > > subsequent "virsh list") showed 42 seconds and a bit regardless of > > whether or not I made those checks. This tells me that the amount of > > time to be saved by caching the results of a single call vs calling > > once for each network are insignificant relative to everything else > > that is being done. Because any cached values would need to be stored > > in the network driver state object, and thus require acquiring the > > driver-wide lock to update at potentially very different times > > (e.g. in the response to a dbus message informing us that firewalld > > was restarted, as well as while starting a new network from an API > > call) I consider the chance of a bug in my code causing an obscure > > deadlock sometime in the future to be a much greater concern than > > maybe saving 1/10th of a second out of 42 (and lock contention might > > eliminate the gain anyway(), so I have left the code to retrieve the > > list of zones once for each network start. > > > > Changes in V2: > > > > * split off from Patch 5. This patch only sets the libvirt zone if it > > exists (and attempts to somewhat document the behavior in > > firewall.html), it doesn't install the libvirt zone. > > > > * check for existence of libvirt zone before attempting to set it. > > > > * if libvirt zone doesn't exist, only log an error in the case that > > the firewalld version is new enough to have an nftables backend, and > > the backend is actually set to nftables. > > > > > > docs/firewall.html.in | 33 +++++++++++++++++++++ > > src/network/bridge_driver_linux.c | 48 +++++++++++++++++++++++++++++++ > > 2 files changed, 81 insertions(+) > > > > diff --git a/docs/firewall.html.in b/docs/firewall.html.in > > index 0a50687c26..5d584e582e 100644 > > --- a/docs/firewall.html.in > > +++ b/docs/firewall.html.in > > @@ -129,6 +129,39 @@ MASQUERADE all -- * * 192.168.122.0/24 !192.168.122.0/24</pre> > > </li> > > </ul> > > + <h3><a id="fw-firewalld-and-virtual-network-driver">firewalld and the virtual network driver</a> > > + </h3> > > + <p> > > + If <a href="https://firewalld.org">firewalld</a> is active on > > + the host, libvirt will attempt to place the bridge interface of > > + a libvirt virtual network into the firewalld zone named > > + "libvirt" (thus making all guest->host traffic on that network > > + subject to the rules of the "libvirt" zone). This is done > > + because, if firewalld is using its nftables backend (available > > + since firewalld 0.6.0) the default firewalld zone (which would > > + be used if libvirt didn't explicitly set the zone) prevents > > + forwarding traffic from guests through the bridge, as well as > > + preventing DHCP, DNS, and most other traffic from guests to > > + host. The zone named "libvirt" is installed into the firewalld > > + configuration by libvirt (not by firewalld), and allows > > + forwarded traffic through the bridge as well as DHCP, DNS, TFTP, > > + and SSH traffic to the host - depending on firewalld's backend > > + this will be implemented via either iptables or nftables > > + rules. libvirt's own rules outlined above will *always* be > > + iptables rules regardless of which backend is in use by > > + firewalld. > > + </p> > > + <p> > > + NB: Prior to libvirt 5.1.0, the firewalld "libvirt" zone did not > > + exist, and prior to firewalld 0.7.0 a feature crucial to making > > + the "libvirt" zone operate properly (rich rule priority > > + settings) was not implemented in firewalld. In cases where one > > + or the other of the two packages is missing the necessary > > + functionality, it's still possible to have functional guest > > + networking by setting the firewalld backend to "iptables" (in > > + firewalld prior to 0.6.0, this was the only backend available). > > + </p> > > + > > <h3><a id="fw-network-filter-driver">The network filter driver</a> > > </h3> > > <p>This driver provides a fully configurable network filtering capability > > diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c > > index e5e48c90f1..9d2e6877ae 100644 > > --- a/src/network/bridge_driver_linux.c > > +++ b/src/network/bridge_driver_linux.c > > @@ -27,6 +27,7 @@ > > #include "virstring.h" > > #include "virlog.h" > > #include "virfirewall.h" > > +#include "virfirewalld.h" > > #define VIR_FROM_THIS VIR_FROM_NONE > > @@ -670,6 +671,53 @@ int networkAddFirewallRules(virNetworkDefPtr def) > > virFirewallPtr fw = NULL; > > int ret = -1; > > + /* if firewalld is active, try to set the "libvirt" zone. This is > > + * desirable (for consistency) if firewalld is using the iptables > > + * backend, but is necessary (for basic network connectivity) if > > + * firewalld is using the nftables backend > > + */ > > + if (virFirewallDIsRegistered() == 0) { > > The indentation here is odd - its 4 spaces too much for the > surrounding context. I did that to eliminate the pointless churn in the next patch, which would have just indented all this code by 4 spaces, thus making it more difficult to see what exactly was changed (you would have to either look through the diff line by line, or apply it and use diff -w). So should I still reindent? (I guess now that you've seen the trimmed down patches, this version has already done its job, so there's not much downside unless someone comes along 5 years from now and is interested in exactly the next commit, which seems unlikely...)