On Sun, Apr 21, 2024 at 10:53:28PM -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@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?
IMHO we should just stick with recording the info we need from a functional POV. Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|