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@xxxxxxxxxx> > > == > > 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@xxxxxxxxxx> > Reviewed-by: Daniel P. Berrangé <berrange@xxxxxxxxxx> > --- > 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 :| _______________________________________________ Devel mailing list -- devel@xxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
