On Fri, Feb 01, 2019 at 10:10:21AM -0500, Laine Stump wrote: > 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@xxxxxxxxxx> > > > > > > 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@xxxxxxxxx> > > > --- > > > > > > 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...) I'd suggest fixing this before pushing. If someone is looking back at history they can tell git to show the next patch without whitespace changes. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
