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

[Michal Privoznik]

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.

> 
>> In the code, this approach is pushed to another level - every
>> table for which private chains are created is run as a separate
>> transaction. The reason is that it saves us one more variable
>> where we would track if the second transaction was started
>> already or not; and also, it doesn't matter.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   src/util/viriptables.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> 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.