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.
>
> Since ipset management is quite complex, the idea was to leave ipset
> management outside of libvirt but still allow users to reference an ipset.
Again, I think that we should be thinking about supporting ipset
management in libvirt rather than requiring external tools. But that
can come incrementally (just like right now, disk snapshots still
require some use of external qemu-img to cover the gaps in the things I
have not yet wired up in libvirt).
> docs/formatnwfilter.html.in | 64 ++++++++++++++
> docs/schemas/nwfilter.rng | 23 +++++
> src/conf/nwfilter_conf.c | 136 +++++++++++++++++++++++++++++-
> src/conf/nwfilter_conf.h | 13 ++
> src/nwfilter/nwfilter_ebiptables_driver.c | 75 +++++++++++++++-
> tests/nwfilterxml2xmlin/ipset-test.xml | 24 +++++
> tests/nwfilterxml2xmlout/ipset-test.xml | 24 +++++
> tests/nwfilterxml2xmltest.c | 2
> 8 files changed, 356 insertions(+), 5 deletions(-)
>
> +static bool
> +ipsetFlagsFormatter(virBufferPtr buf,
> + virNWFilterRuleDefPtr nwf ATTRIBUTE_UNUSED,
> + nwItemDesc *item)
> +{
> + uint8_t ctr;
> +
> + for (ctr = 0; ctr < item->u.ipset.numFlags; ctr++) {
> + if (ctr != 0)
> + virBufferAddLit(buf, ",");
> + if ((item->u.ipset.flags & (1 << ctr)))
> + virBufferAddLit(buf, "src");
> + else
> + virBufferAddLit(buf, "dst");
Life would be a bit easier with a virBuffer command that removed a
trailing comma; then you could always append 'src,' or 'dst,' and clean
up things at the end of the loop. Then again, you're not the first
person to hit that situation where making virBuffer more powerful would
be nice, so don't bother waiting to refactor on top of such a patch
unless you really want it.
>
> +# define VALID_IPSETNAME \
> + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_.:-+ "
Brave to allow a space; are we sure it will always be properly shell-quoted?
> @@ -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.
> @@ -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.
> @@ -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.
> 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?
> </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.
> + <td>STRING</td>
> + <td>The name of an IPSet managed outside of libvirt</td>
> + </tr>
> + <tr>
> + <td>ipsetflags <span class="since">(Since 0.9.12)</span></td>
0.9.13 (several more, won't point them out)
> + <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.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list