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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

With that fixed

Reviewed-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux