[PATCH 2/8] network: firewalld: add networkAddHybridFirewallDRules()

[Eric Garver]

On Tue, Nov 15, 2022 at 11:21:43AM +0100, Michal Prívozník wrote:
> On 11/10/22 17:31, Eric Garver wrote:
> > This factors out the firewalld pieces of the iptables + firewalld
> > backend.
> > 
> > Signed-off-by: Eric Garver <eric at garver.life>
> > ---
> >  src/network/bridge_driver_linux.c | 117 ++++++++++++++++--------------
> >  1 file changed, 61 insertions(+), 56 deletions(-)
> > 
> > diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
> > index d9597d91beed..88a8e9c5fa27 100644
> > --- a/src/network/bridge_driver_linux.c
> > +++ b/src/network/bridge_driver_linux.c
> > @@ -801,6 +801,58 @@ networkRemoveIPSpecificFirewallRules(virFirewall *fw,
> >  }
> >  
> >  
> > +static int
> > +networkAddHybridFirewallDRules(virNetworkDef *def)
> > +{
> > +    /* 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 the "libvirt" zone exists, then set it. If not, and
> > +     * if firewalld is using the nftables backend, then we
> > +     * need to log an error because the combination of
> > +     * nftables + default zone means that traffic cannot be
> > +     * forwarded (and even DHCP and DNS from guest to host
> > +     * will probably no be permitted by the default zone
> > +     */
> > +    if (virFirewallDZoneExists("libvirt")) {
> > +        if (virFirewallDInterfaceSetZone(def->bridge, "libvirt") < 0)
> > +            return -1;
> > +    } else {
> > +        unsigned long version;
> > +        int vresult = virFirewallDGetVersion(&version);
> > +
> > +        if (vresult < 0)
> > +            return -1;
> > +
> > +        /* Support for nftables backend was added in firewalld
> > +         * 0.6.0. Support for rule priorities (required by the
> > +         * 'libvirt' zone, which should be installed by a
> > +         * libvirt package, *not* by firewalld) was not added
> > +         * until firewalld 0.7.0 (unless it was backported).
> > +         */
> > +        if (version >= 6000 &&
> > +            virFirewallDGetBackend() == VIR_FIREWALLD_BACKEND_NFTABLES) {
> > +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                           _("firewalld is set to use the nftables "
> > +                             "backend, but the required firewalld "
> > +                             "'libvirt' zone is missing. Either set "
> > +                             "the firewalld backend to 'iptables', or "
> > +                             "ensure that firewalld has a 'libvirt' "
> > +                             "zone by upgrading firewalld to a "
> > +                             "version supporting rule priorities "
> > +                             "(0.7.0+) and/or rebuilding "
> > +                             "libvirt with --with-firewalld-zone"));
> 
> I know you wanted this to be plain code movement, but since you're
> touching this error message you can apply our coding style rule [1] and
> unbreak the message onto one, long line. All error messages are exempt
> from the 80 chars rule, because it's easier to git grep them.
> 
> And on a similar note, per out deprecation policy [2], firewalld-0.6.0
> or older is ancient history, thus this version check can be dropped (in
> a separate commit of course).

ACK. I'll drop the whole else block in the next version.

> > +            return -1;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +
> 
> 1: https://libvirt.org/coding-style.html#error-message-format
> 2: https://libvirt.org/platforms.html#linux-freebsd-and-macos
> 
> Michal
>