On Tue, Apr 30, 2024 at 01:44:10PM -0400, Laine Stump wrote:
When destroying a network, the network driver has always assumed
that
it knew what firewall rules had been added as the network was
started. This was usually correct - I only recall one time in the past
that the firewall rules added by libvirt were changed. But if the
exact rules used for a network *were* ever changed from one
build/version of libvirt to another, then we would end up attempting
to remove rules that hadn't been added, and could possibly *not*
remove rules that had been added.
The solution to this to not make such brash assumptions about the
past, but instead to save (in the network status object at network
start time) a list of all the rules needed to remove the rules that
were added for the network, and then use that saved list during
network destroy to remove exactly what was previous added.
Beyond making net-destroy more precise, there are other benefits:
1) We can change the details of the rules we add for networks from one
build/release of libvirt to another and painlessly upgrade.
2) The user can switch from one firewall backend to another by simply
changing the setting in network.conf and restarting
libvirtd/virtnetworkd.
In both cases, the restarted libvirtd/virtnetworkd will remove all the
rules that had been previously added (based on the network status),
and then add new rules (saving the new removal commands back into the
network status)
Signed-off-by: Laine Stump <laine(a)redhat.com>
==
NB: the current implementation saves only the commands necessary to
remove the network's firewall, and names this <firewall> in the status
XML. It would be simple to instead save the *entire* virFirewall
object (thus also including the commands that were used to add the
firewall, as well as the commands needed to remove it) - although very
verbose, it's possible it could be useful when debugging a firewall
issue (since it's not obvious which rules were added for which network
when just looking at the output of "nft list ruleset". Alternately, we
could continue to store only the removal commands, but maybe change
the name of the element in XML from <firewall> to <fwRemoval> (which
would leave the door open to expanding what is saved in the
future). Any opinions on this?
This can be dropped, since there weren't objections to my
answer in v3 that your impl here is fine as-is.
Signed-off-by: Laine Stump <laine(a)redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
src/network/bridge_driver.c | 35 ++++++++++++++--------
src/network/bridge_driver_linux.c | 26 +++++++++++++----
src/network/bridge_driver_nop.c | 6 ++--
src/network/bridge_driver_platform.h | 6 ++--
src/network/network_iptables.c | 43 +++++++++++++++++++++++++---
src/network/network_iptables.h | 2 +-
tests/networkxml2firewalltest.c | 2 +-
7 files changed, 91 insertions(+), 29 deletions(-)
With 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 :|