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

[Daniel P. Berrangé]

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.

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.

> 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.

Why shouldn't we just consider that missing kernel build option to be
a user bug ? 

>  
>      for (i = 0; i < ARRAY_CARDINALITY(data); i++)
>          virFirewallAddRuleFull(fw, data[i].layer,

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 :|