[libvirt] [PATCH 0/7] POC: Saner firewall integration

[Laine Stump]

On 03/12/2014 07:21 AM, Daniel P. Berrange wrote:
> We currently have three areas of code which deal with firewall
> changes. The bridge driver's iptables usage, the QEMU driver's
> ebtables usage for mac filters and the nwfilter code.
>
> These all directly invoke the iptables/ebtables commands or
> in the case of nwfilter invoke horrible generated shell scripts.
> The latter in particular has always been an unpleasant design
> choice, but it has been made much worse by support for firewalld.
> We are now invoking firewall-cmd just in order to make a DBus
> method call to firewalld which then invokes the real *tables
> commands. This has a notable performance impact.
>
> This proof of concept series introduces a new virFirewallPtr
> object for encapsulating all firewall changes. It provides
> a transactional API for making firewall changes, so the caller
> can define a set of rules which must either all succeed or all
> fail, along with a set of rules to perform rollback upon fail.
> It will either execute *tables commands directly or will call
> the DBus method for firewalld directly.
>

Aside from the reported huge decrease in network startup time, I love
the fact that this eliminates all of the multi-layered error exit labels
in the bridge driver functions that add multiple rules. It looks *much*
cleaner.

I was a bit confused to see IPv4 vs. IPv6 status put into a variable
called "layer", since they are two different protocols at the same
layer, but it made more sense when I looked at the enum definition and
saw that the other choice is "ethernet"; I can't really think of a
better name (unless it was made more generic, e.g. "type", but that is
probably a bad idea, since "type" is used for so many different things.

This seems like a reasonable initial implementation, but I think it
would be very good in the longer term to replace the
freeform-and-yet-platform-specific "list of strings to add to an
iptables command" interface of virFirewallAddRule() with something more
structured/abstract, so that the calls would be less tied to the
iptables backend implementation. This might make it possible to replace
the backend with something that used ipfw (or whatever *BSD is using
these days), or maybe even the packet filtering in Open vSwitch (This
would allow us to use Open vSwitch for virtual networks without any loss
of functionality, and to support nwfilter rules for guests connected to
an Open vSwitch bridge). That's likely better done after what you have
here is all in and tested, though.

Another thing that would be nice to consider for the future is some
method of storing the list of rules that were applied (so that they
could be saved as part of the domain/network status). This would permit
us to delete exactly the rules that we had previously added. right now
we have to assume that the libvirtd that wrote the rules we want to
delete was running the same code as the current libvirtd; instead, when
we add rules we should be saving all the information needed to delete
those exact same rules, meaning that if the code is changed somehow, we
end up trying to delete rules that don't exist, or not deleting rules
that we should delete. Again, that is new functionality, so out of the
scope of this series.

Since this is just Proof of Concept code, I won't bother with a detailed
enough look to ACK (unless you're thinking of pushing this, in which
case I'll go back and look for more details), but definitely "thumbs up" :-)