[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Mon, 2010-05-24 at 14:09 -0600, Eric Blake wrote:
> 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/

done.

> 
> > +
> > +    <h2><a name="nwfconcpts">Concepts</a></h2>
> 
> s/nwfconcpts/nwfconcepts/; does anything else link here yet?
> 
done - nothing links to it, 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?
> 

That's really what a rule with priority 1000 would do

<rule action='drop' direction='inout' priority='1000'/>

Depending on how one has used the chain parameter in the filter node for
particularly these prtocols mac, arp etc , the rule would need to be
written in separate XML files.

> ...
> > +    <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/
> 
ok

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

ok

> > +    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?
> 
done -- no links, yet


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

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

Removed.

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

I renamed it. :-)


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

fixed


> > +     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/

what meant in the sense of 'that is preventing', but I fixed it

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

ok.

> 
> > +       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/
> 

fixed

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

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

I'll wait until tomorrow morning and will then push it unless I hear
other comments.

  Thanks.  Stefan



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]