7:44 p.m.
On 05/14/2012 06:20 PM, Eric Blake wrote:
On 04/23/2012 06:00 AM, Stefan Berger wrote:
> This patch adds support for the recent ipset iptables extension
> to libvirt's nwfilter subsystem. Ipset allows to maintain 'sets'
> of IP addresses, ports and other packet parameters and allows for
> faster lookup (in the order of O(1) vs. O(n)) and rule evaluation
> to achieve higher throughput than what can be achieved with
> individual iptables rules.
>
> On the command line iptables supports ipset using
>
> iptables ... -m set --match-set<ipset name> <flags> -j ...
How will this interact with firewalld in Fedora 17? But adding things
incrementally is okay by me, so I'll still review this.
Firewalld's firewall-cmd has a passthrough mode where the command line
for iptables referencing an ipset would just be written like this:
firewall-cmd --direct --passthrough ipv4 ... -m set --match-set <ipset
name> <flagse> -j ...
So this is not the real problem. Unfortunately using firewall-cmd makes
everything much slower, i.e., the TCK testsuite runs probably at least 8
times slower and starting a VM referencing a filter also takes
considerably longer to start. So the solution may be to either find an
intermediate format for creating the commands by the ebiptables driver
so the 'scripts' can be converted to either bash or direct Dbus
execution or, as I have done, to parse today's generated scripts and
convert them to DBus commands. This works fine so far as long as one
doesn't encounter comments, which we have assign to a variable
(comment), and has its own challenges to parse. The speedup for the TCK
testsuite was so far maybe 40% with scripts containing comments or the
'other' embedded scripts containing loops left to running them using
firewall-cmd. Any comments on this? The code so far parsing the script
doesn't look 'too bad'...
>
> +# define VALID_IPSETNAME \
> + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_.:-+ "
Brave to allow a space; are we sure it will always be properly shell-quoted?
Yes, I payed attention. ;-)
> @@ -346,6 +349,44 @@ _printDataType(virNWFilterVarCombIterPtr
> }
> break;
>
> + case DATATYPE_IPSETNAME:
> + snprintf(buf, bufsize, "%s", item->u.ipset.setname);
Are we sure that it is safe to ignore the return value of snprintf here?
And a format of "%s" is generally an indication that you should really
be using virStrncpy instead.
Right, converting it.
> @@ -927,6 +975,7 @@ iptablesHandleIpHdr(virBufferPtr buf,
> char ipaddr[INET6_ADDRSTRLEN],
> number[MAX(INT_BUFSIZE_BOUND(uint32_t),
> INT_BUFSIZE_BOUND(int))];
> + char str[200];
Why 200? A comment, or even a composition of #define'd macros, instead
of a magic number, would make me feel better.
Introducing MAX_IPSET_NAME_LENGTH.
> @@ -1060,4 +1068,19 @@
> <param
name="pattern">((SYN|ACK|URG|PSH|FIN|RST)(,(SYN|ACK|URG|PSH|FIN|RST))*|ALL|NONE)/((SYN|ACK|URG|PSH|FIN|RST)(,(SYN|ACK|URG|PSH|FIN|RST))*|ALL|NONE)</param>
> </data>
> </define>
> +
> +<define name='ipset-type'>
> +<choice>
> +<ref name="variable-name-type"/>
> +<data type="string">
> +<param name="pattern">[a-zA-Z0-9_\.:\-\+]{1,31}</param>
Hmm, no space in this pattern; it doesn't match your VALID_IPSETNAME
#define above.
Ooops, I will add it.
> Index: libvirt-acl/docs/formatnwfilter.html.in
> ===================================================================
> --- libvirt-acl.orig/docs/formatnwfilter.html.in
> +++ libvirt-acl/docs/formatnwfilter.html.in
> @@ -528,6 +528,10 @@
> <li>IPV6_MASK: IPv6 mask in numbers format (FFFF:FFFF:FC00::) or CIDR
mask (0-128)</li>
> <li>STRING: A string</li>
> <li>BOOLEAN: 'true', 'yes', '1' or
'false', 'no', '0'</li>
> +<li>IPSETFLAGS: The source and destination flags of the ipset described
> + by up to 6 'src' or 'dst' elements selecting features from
either
> + the source or destination part of the packet header; example:
> + src,src,dst</li>
Do we have a mapping of _which_ elements are selected? If I say
'src,dst', is it always the first two elements of a packet header (and
which two elements are they), or does the choice of which two elements
depend on other context?
It depends on the type of ipset that is referenced by a rule, i.e., the
ipset hash:ip,port would allow to select IP address and port in the
source or destination part of the packet independently, while the ipset
hash:ip,port,ip would allow IP address to be selected in the source
and/or destination part and the port in the sort or destination part.
> </ul>
> <p>
> <br/><br/>
> @@ -1169,6 +1173,16 @@
> <td>STRING</td>
> <td>TCP-only: format of mask/flags with mask and flags each being a
comma separated list of SYN,ACK,URG,PSH,FIN,RST or NONE or ALL</td>
> </tr>
> +<tr>
> +<td>ipset<span class="since">(Since
0.9.12)</span></td>
0.9.13, now.
Yes, will replace.
> +<tr>
> +<td>ipsetflags<span class="since">(Since
0.9.12)</span></td>
> +<td>IPSETFLAGS</td>
> +<td>flags for the IPSet; requires ipset attributed</td>
I think you meant:
s/attributed/attribute/
(several instances)
Overall, the patch seems reasonable (sorry for delaying the review until
after 0.9.12). But I do think it's worth a v4 to rebase to latest and
fix the nits above.
Thanks for the review. I will post v4 shortly.
Stefan