7:47 p.m.
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(a)garver.life>
> ---
> 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).