Re: [libvirt PATCH 18/28] util: new functions to support adding individual rollback rules

On 5/4/23 6:44 AM, Daniel P. Berrangé wrote: > On Sun, Apr 30, 2023 at 11:19:33PM -0400, Laine Stump wrote: > > In the past virFirewall required all rollback rules for a group (those > > commands necessary to "undo" any rules that had been added in that > > group in case of a later failure) to be manually added by switching > > into "rollback mode" and then re-calling the inverse of the exact > > virFirewallAddRule*() APIs that had been called to add the original > > rules (ie. for each --insert command, for rollback we would need to > > add a rule with all arguments identical except that "--insert" would > > be replaced by "--delete"). > > > > Because nftables can't search for rules to remove by comparing all the > > arguments (it instead expects *only* a handle that was issued when the > > rule was originally added), we want for the backends' vir*ApplyRule() > > functions to be able to automatically add a single rollback rule to > > the virFirewall object while applying its existing rules (this > > automatically added rule would then be able to include the handle > > returned by "nft add rule"). > > I think the mistake here is that we're trying to replicate the > iptables approach for rules 1:1. Well, my idea was to *initially* replicate it 1:1 so that we could more easily verify we haven't changed behavior in some way that we might miss during any testing, but in a way that we could also easily change it later. > > This was required in iptables world because there was only a single > global set of tables. libvirt's rules were mixed in with rules from > non-libvirt apps. Libvirt's rules for different virtual networks also > had to be mixed together, as we needed special ordering for the > forward rules. > > With nft we can drastically simplify this all with two changes to > our approach > > * Each virtual network should have a top level chain > ie instead of > > table ip libvirt > > we should have > > table ip libvirt_net_default My understanding has always been that each packet must get an ACCEPT result from *all* of the tables, and if this was the case, then what you're suggesting wouldn't work.