Re: [libvirt] [PATCH] iptablesSetupPrivateChains: Be forgiving if a table does not exist

On 3/11/19 11:05 AM, Daniel P. Berrangé wrote: > On Mon, Mar 11, 2019 at 09:37:52AM +0100, Michal Privoznik wrote: > > The way this function works is that for both iptables and > > ip6tables (or their firewalld friends) and for every table > > ("filter", "nat", "mangle") it lists chains defined for the table > > and then calls iptablesPrivateChainCreate() over the list. The > > callback is then supposed to find libvirt private chains and if > > not found create rules to add them. So far so good. Problem is if > > one of the tables doesn't exist (e.g. due to a module missing). > > For instance, on my system I don't have CONFIG_IP6_NF_MANGLE > > enabled therefore I'm lacking "mangle" table for ip6tables. This > > means that the whole operation of setting up private chains fails > > because the whole transaction is run as "do not ignore errors". > > > > The solution is to have two transactions, the first one which > > just lists chains can run ignoring errors, and the second one > > which then installs the private chains will run normally. > > This is just going to push the failure to a later point. Is it? I mean sure, if you have some nwfilters that will apply some rules to the missing table. But if you don't have such filters?

> This causes iptablesPrivateChainCreate() to skip setup of the > libvirt global chain, so when we try to add rules to the libvirt > global chain later we'll find it doesn't exist. I my limited setup, where I use the default network (which has just IPv4 enabled) and one domain using that network, no nwfilters this patch fixes the problem for me and I don't see any subsequent errors. I don't see any appealing argument to require IPv6 support in that case.

> > diff --git a/src/util/viriptables.c b/src/util/viriptables.c > > index d67b640a3b..ea24b03ec8 100644 > > --- a/src/util/viriptables.c > > +++ b/src/util/viriptables.c > > @@ -101,6 +101,16 @@ iptablesPrivateChainCreate(virFirewallPtr fw, > > tmp++; > > } > > + /* This function is running in the context of the very first transaction, > > + * which does nothing more than just lists current tables and chains. But > > + * since some tables might not be there (e.g. because of a module missing), > > + * the transaction is run with IGNORE_ERRORS flag. But obviously, we don't > > + * want to ignore errors here, where we are constructing our own chains and > > + * rules. The only way to resolve this is to start a new transaction so > > + * that all those AddRule() calls below add rules to new transaction/group. > > + */ > > + virFirewallStartTransaction(fw, 0); > > + > > for (i = 0; i < data->nchains; i++) { > > const char *from; > > if (!virHashLookup(chains, data->chains[i].child)) { > > @@ -160,7 +170,7 @@ iptablesSetupPrivateChains(void) > > fw = virFirewallNew(); > > - virFirewallStartTransaction(fw, 0); > > + virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); > > This applies to all the chains, so if someone is missing more builtin > top level chains we'll potentially fail to create any of our global > chains which feels pretty undesirable to me. Hm.. maybe I'm missing something, but when I was debugging this I saw IGNORE_ERRORS being applied only for those "rules" [1] from the first group/transaction. Any other rules that the callback added were run normally, without ignoring errors.

> Why shouldn't we just consider that missing kernel build option to be > a user bug ? Well, as I say above, if you only have IPv4 network then the current code is going to fail the moment it gets to the first IPv6 "rule" [1]. But if we decide to require IPv6, then we need to document that. Michal 1: I say rules, but they aren't rules really, they merely just list chains.