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

[Laine Stump]

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 at redhat.com>
>>
>> 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 at laine.org>
>> ---
>>
>> 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...)


>
> With that fixed
>
> Reviewed-by: Daniel P. Berrangé <berrange at redhat.com>
>
> Regards,
> Daniel