Re: [PATCH 3/8] network: firewalld: use native routed networks

[Laine Stump <laine@xxxxxxxxxx>]

On 11/15/22 11:34 AM, Daniel P. Berrangé wrote:
> On Thu, Nov 10, 2022 at 11:31:47AM -0500, Eric Garver wrote:
> > The firewalld backend for routed networks can now use a native
> > implementation. The hybrid of iptables + firewalld is no longer
> > necessary. When full native firewalld is in use there are zero iptables
> > rules add by libvirt.
> >
> > This is accomplished by returning early in networkAddFirewallRules() and
> > avoiding calls to networkSetupPrivateChains().
> >
> > Signed-off-by: Eric Garver <eric@xxxxxxxxxxx>
> > ---
> >   src/network/bridge_driver_linux.c | 51 +++++++++++++++++++++++++------
> >   1 file changed, 42 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
> > index 88a8e9c5fa27..42f098ff1f9b 100644
> > --- a/src/network/bridge_driver_linux.c
> > +++ b/src/network/bridge_driver_linux.c
> > @@ -133,6 +133,21 @@ networkHasRunningNetworksWithFW(virNetworkDriverState *driver)
> >   }
> >   
> >   
> > +static bool
> > +networkUseOnlyFirewallDRules(void)
> > +{
> > +    if (virFirewallDIsRegistered() < 0)
> > +        return false;
> > +
> > +    if (virFirewallDPolicyExists("libvirt-routed-out") &&
> > +        virFirewallDZoneExists("libvirt-routed")) {
> > +        return true;
> > +    }
> > +
> > +    return false;
> > +}
> > +
> > +
> >   void
> >   networkPreReloadFirewallRules(virNetworkDriverState *driver,
> >                                 bool startup G_GNUC_UNUSED,
> > @@ -172,6 +187,9 @@ networkPreReloadFirewallRules(virNetworkDriverState *driver,
> >               return;
> >           }
> >   
> > +        if (!chainInitDone && networkUseOnlyFirewallDRules())
> > +            return;
> > +
> >           ignore_value(virOnce(&createdOnce, networkSetupPrivateChains));
> >       }
> >   }
> > @@ -801,6 +819,18 @@ networkRemoveIPSpecificFirewallRules(virFirewall *fw,
> >   }
> >   
> >   
> > +static int
> > +networkAddOnlyFirewallDRules(virNetworkDef *def)
> > +{
> > +    if (def->forward.type == VIR_NETWORK_FORWARD_ROUTE) {
> > +        if (virFirewallDInterfaceSetZone(def->bridge, "libvirt-routed") < 0)
> > +            return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +
> >   static int
> >   networkAddHybridFirewallDRules(virNetworkDef *def)
> >   {
> > @@ -860,6 +890,11 @@ int networkAddFirewallRules(virNetworkDef *def)
> >       virNetworkIPDef *ipdef;
> >       g_autoptr(virFirewall) fw = virFirewallNew();
> >   
> > +    if (!def->bridgeZone && networkUseOnlyFirewallDRules() &&
> > +        def->forward.type == VIR_NETWORK_FORWARD_ROUTE) {
> > +        return networkAddOnlyFirewallDRules(def);
> > +    }
> > +
> >       if (virOnce(&createdOnce, networkSetupPrivateChains) < 0)
> >           return -1;
> >   
> > @@ -895,15 +930,8 @@ int networkAddFirewallRules(virNetworkDef *def)
> >               return -1;
> >   
> >       } else if (virFirewallDIsRegistered() == 0) {
> > -        if (def->forward.type == VIR_NETWORK_FORWARD_ROUTE &&
> > -            virFirewallDPolicyExists("libvirt-routed-out") &&
> > -            virFirewallDZoneExists("libvirt-routed")) {
> > -            if (virFirewallDInterfaceSetZone(def->bridge, "libvirt-routed") < 0)
> > -                return -1;
> > -        } else {
> > -            if (networkAddHybridFirewallDRules(def) < 0)
> > -                return -1;
> > -        }
> > +        if (networkAddHybridFirewallDRules(def) < 0)
> > +            return -1;
> >       }
> >   
> >       virFirewallStartTransaction(fw, 0);
> > @@ -940,6 +968,11 @@ void networkRemoveFirewallRules(virNetworkDef *def)
> >       virNetworkIPDef *ipdef;
> >       g_autoptr(virFirewall) fw = virFirewallNew();
> >   
> > +    if (!def->bridgeZone && networkUseOnlyFirewallDRules() &&
> > +        def->forward.type == VIR_NETWORK_FORWARD_ROUTE) {
> > +        return;
> > +    }
> > +
>
>
> This logic doesn't work well in the upgrade scenario.
>
> Consider that we have existing running virtual networks,  and
> we upgrade libvirt in-place on the host.
>
> During virtnetworkd startup, we tear down old firwall rules
> and create the new ones.  Except that we need to teardown the
> old iptables rules, and this skips that because it decided we
> need to use firewalld instead.  So we're left with dangling
> iptables rules on upgrade

libvirt has a longstanding problem that each time it starts, it assumes 
that all the active networks had been started with firewall rules 
exactly matching the rules that the *current* libvirt version would add 
if it had started the network. This usually is okay; in the past it was 
only a problem when someone changed the rulelist added for each network, 
which historically has only happened a few times (and in those cases we 
just told people to restart their host if they wanted everything to be 
exactly correct after the upgrade).

This is just another instance of that same problem.

I have a patchset to add a "native nftables" backend that I've been 
alternately working on and abandoning for several months, and in that 
patchset I save the entire ruleset that was added for each network into 
that network's status XML. Then when the daemon is restarted, the rules 
that need to be removed (or, more correctly, the commands that need to 
be run in order to remove the rules) is read from the status XML for the 
network rather than just being blindly assumed. This permits switching 
from iptables to nftables backend without requiring a reboot to get it 
right, and also makes sure that if a future update to libvirt changes 
the ruleset, we will properly remove the old rules during the update.

Coincidentally, this same code will fix the problem with a restart after 
switching to a pure firewalld backend (there will be a list of "firewall 
commands", but that list will be empty[*])


[*] It has to be an empty list rather than no list at all because the 
absence of a list of necessary commands in the status XML must be 
recognized as "previous libvirt didn't save the list of commands - 
assume that we currently have the rules that would have been added by a 
'pre-epoch' libvirt".

Anyway, I am soonish planning to rebase my nftables-backend patches, and 
see how Eric's patches and mine can mutually feed off each other (I need 
to rethink where I draw the abstraction/API line for the functions that 
each backend must provide).