Re: [libvirt] [PATCH] [DOCS] nwfilter: documentation

On 05/22/2010 09:31 AM, Stefan Berger wrote: > This patch adds documentation of the nwfilter subsystem of libvirt to > the existing (web) docs. I am attaching a PDF in case you don't want to > read the plain html sources. Sometimes, that really is a much easier way to review :) If nothing else, it proves your patch was well-formed enough to make it through the conversion to a pdf file without hassle. > + > + <h2><a name="goals">Goals and background</a></h2> > + > + <p> > + The goal of the network filtering XML is to enable administrators > + of virtualized system to configure and enforce network traffic s/of virtualized/of a virtualized/

> + > + <h2><a name="nwfconcpts">Concepts</a></h2> s/nwfconcpts/nwfconcepts/; does anything else link here yet?

> + <p> > + The network traffic filtering subsystem enables configuration > + of network traffic filtering rules on individual network > + interfaces that are configured for certain types of > + network configurations. Supported network types are > + </p> > + <ul> > + <li><code>network</code></li> > + <li><code>ethernet</code> -- must be used in bridging mode</li> > + <li><code>bridge</code></li> > + <li><code>direct</code> -- only protocols mac, arp, ip and ipv6 > + can be filtered</li> Unrelated to this patch - does it make sense to add support for a filter that rejects all traffic that does not contain one of these supported protocols?

... > + <p> > + Network filters are written in XML and may either contain references > + to other filters, contain rules for traffic filtering or can s/filtering or can/filtering, or/

> + hold a combination of both. The above referenced filter > + <code>clean-traffic </code> is a filter that for example only s/for example //

> + contains references to > + other filters and no actual filtering rules. Since references to > + other filters can be used, a <i>tree</i> of filters can be built. > + ... > + <h3><a name="nwfconcptsvars">Usage of variables in filters</a></h3> s/nwfconcptsvars/nwfconceptvars/; does anything else link here yet?

> + > + <h3><a name="nwfelemsRefs">References to other filers</a></h3> s/filers/filters/

... > + <h2><a name="nwfcli">Command line tools</a></h2> > + <p> > + The libvirt command line tool <code>virsh</code> has been extended > + with life-cycle support for network filters. All commands related > + to the network filtering subsystem start with the prefix > + <code>nwfilter</code>. The following commands are available: > + <p> > + <ul> > + <li>nwfilter-list : list UUIDs and names of all network filters</li> > + <li>nwfilter-define : define a new network filter or update an existing one</li> > + <li>nwfilter-undefine : delete a network filter given its name; it must not be currently in use</li> > + <li>nwfilter-dumpxml : display a network filter given its name</li> > + <li>nwfilter-edit : edit a network filter given its name</li> > + </ul> > + > + <h2><a name="nwfexamples">Example network filters</a></h2> > + <p> > + The following is a list of example network filters that are > + automatically installed with libvirt. </p> Unusual whitespace.

> + <table class="top_table"> > + <tr> > + <th> Name </th> > + <th> Description </th> > + </tr> > + <tr> > + <td> no-arp-spoofing </td> > + <td> Prevent a VM from spoofing ARP traffic; this filter > + only allows ARP request and reply messages and enforces > + that those packets contain the MAC and IP addresses > + of the VM.</td> The section heading of "example network filters" is a bit misleading - I was expecting to see the actual filter bodies here to form the example. Either we should document the filter bodies, or we should retitle the section to "pre-existing network filters". I could go either way, but obviously re-titling the section is the only one of the two options that is small enough to not affect my ACK (if you want to go with including the filter bodies, submit it as a second patch).

... > + The role of the <code>chain</code> attribute in the network filter > + XML is that internally a new user-defined ebtables table is created > + that then for example receives all <code>arp</code> traffic coming > + from or going to a virtual machine, if the chain <code>arp</code> > + has been specified. Further, a rule is generated in an interface's > + <code>root</code> chain that directs all ipv4 traffic into the > + user-defined chain. Therefore, all ARP traffic rules should then be > + placed into filters specifying this chain. This type of branching > + into user-define tables is only supported with filtering on the ebtables s/define/defined/

> + The result of these considerations is the following network filter XML: > + </p> > +<pre> > +&lt;filter name='test-eth0'&gt; > + &lt;!-- reference the clean traffic filter preventing s/preventing/to prevent/

> + MAC, IP and ARP spoofing. By not providing > + and IP address parameter libvirt will detect the s/parameter/parameter,/

> + IP address the VM is using. --&gt; > + &lt;filterref filter='clean-traffic'/&gt; > + > + &lt;!-- enable TCP ports 22 (ssh) and 80 (http) to be reachable --&gt; > + &lt;rule action='accept' direction='in'&gt; > + &lt;tcp dstportstart='22'/&gt; > + &lt;/rule&gt; Didn't see mention any earlier that an omitted dstportend defaults to dstportstart, but it wasn't too hard to figure out from this example. > + > + <h3><a name="nwflimitsmigr">VM Migration</a></h3> > + <p> > + VM migration is only supported if the whole filter tree > + that is referenced by a virtual machine's top level filter > + is also available on the target host. The network filter > + <i>clean-traffic</i> > + for example should be available on all libvirt installations > + of version 0.8.1 or later and thus enable migration of VMs that > + for example reference this filter. All other > + custom filters must be migrated using higher layer software. It is > + outside the scope of libvirt to ensure that referenced filters > + on the source system are equivalent to those on the target system > + and vice versa. > + <br><br> > + Migration must occurr between libvirt insallations of version s/occurr/occur/; s/insallations/installations/

> + 0.8.1 or later in order not to loose the network traffic filters s/loose/lose/

> + associated with an interface. > + </p> > + > + </body> > +</html> > Big patch, but much appreciated. You have my ACK, once the nits are addressed; if we come up with follow-up patches, it will be easier to see them as incremental diffs than to wait for a v2.