[ovs-dev] [PATCH 06/23] ovn: Update TODO, ovn-northd flow table design, ovn-architecture for L3.

Ben Pfaff blp at nicira.com
Sat Oct 17 03:15:06 UTC 2015


On Fri, Oct 16, 2015 at 02:46:55PM -0700, Justin Pettit wrote:
> 
> > On Oct 9, 2015, at 9:15 PM, Ben Pfaff <blp at nicira.com> wrote:
> > 
> > +*** Allow output to ingress port
> > +
> > +Sometimes when a packet ingresses into a router, it has to egress the
> > +same port.  One example is a "one-armed" router that has multiple
> > +routes on a single port (or in which a host is (mis)configured to send
> > +every IP packet to the router, e.g. due to a bad netmask).  Another is
> > +when a router needs to send an ICMP reply to a ingressing packet.
> 
> s/a/an/

Fixed, thanks.

> > +        <p>
> > +          ICMP echo reply.  These flows reply to ICMP echo requests received
> > +          for the router's IP address.  Let <var>A</var> be an IP address or
> > +          broadcast address owned by a router port.  Then, for each
> > +          <var>A</var>, a priority-210 flow matches on <code>ip4.dst ==
> > +          <var>A</var></code> and <code>icmp4.type == 8 &amp;&amp; icmp4.code
> > +          == 0</code> (ICMP echo request).  These flows use the following
> > +          actions where, if <var>A</var> is unicast, then <var>S</var> is
> > +          <var>A</var>, and if <var>A</var> is broadcast, <var>S</var> is the
> > +          router's IP address in <var>A</var>'s network:
> > +        </p>
> 
> I don't believe this is actually implemented in patch 23.  It might be
> nice to put a bolded "future" or something in the descriptions of
> things that aren't yet implemented.

OK, I added "Not yet implemented." to a bunch of items.

> > +        <p>
> > +          UDP port unreachable.  These flows generate ICMP port unreachable
> > +          messages in reply to UDP datagrams directed to the router's IP
> > +          address.  The logical router doesn't accept any UDP traffic so it
> > +          always generates such a reply.
> > +        </p>
> > ...
> > +          TCP reset.  These flows generate TCP reset messages in reply to TCP
> > +          datagrams directed to the router's IP address.  The logical router
> > +          doesn't accept any TCP traffic so it always generates such a reply.
> > +        </p>
> > ...
> > +        <p>
> > +          Protocol unreachable.  These flows generate ICMP protocol unreachable
> > +          messages in reply to packets directed to the router's IP address on
> > +          IP protocols other than UDP, TCP, and ICMP.
> > +        </p>
> 
> Did you want to specify a priority for these flows?  The ping and ARP
> processing have priority-210 and the drop everything else shows
> priority-200, so it may be worth throwing in there.

I added a priority.

> > +        Drop IP multicast.  A priority-190 flow with match
> > +        <code>ip4.dst[28..31] == 0xe</code> drops IP multicast traffic.
> 
> Do you want to use "ip.mcast"?

Yes, thanks.

> > +          ICMP time exceeded.  For each router port <var>P</var>, whose IP
> > +          address is <var>A</var>, a priority-180 flow with match <code>inport
> > +          == <var>P</var> &amp;&amp; ip4.ttl == {0, 1} &amp;&amp;
> > +          !ip.later_frag</code> matches packets whose TTL has expired, with the
> > +          following actions to send an ICMP time exceeded reply:
> 
> The "ICMP time exceeded" and "TTL discard" flows match on an exceeded
> TTL differently.  It might be nice to use the same match in both.

I changed them both to say "ip4.ttl == {0, 1}".

> > +        TTL discard.  A priority-170 flow with match <code>ip4.ttl &lt;
> > +        2</code> and actions <code>drop;</code> drops other packets whose TTL
> > +        has expired, that should not receive a ICMP error reply.
> 
> It might be nice to explain that this handles fragments.

Done.

> > +    <h3>Ingress Table 2: IP Routing</h3>
> 
> Is there anything in the Table 1 description that indicates the common
> case of moving to this table?

No, this was missing.  I added a description of a priority-0 flow.

> > +          Unknown MAC bindings.  For each non-gateway route to IPv4 network
> > +          <var>N</var> with netmask <var>M</var> on router port <var>P</var>
> > +          that owns IP address <var>A</var> and Ethernet address <var>E</var>,
> > +          a logical flow with match <code>ip4.dst ==
> > +          <var>N</var>/<var>M</var></code>, whose priority is the number of
> > +          1-bits in <var>M</var>, has the following actions:
> > ...
> > +arp {
> > +    eth.dst = ff:ff:ff:ff:ff:ff;
> > +    eth.src = <var>E</var>;
> 
> I think you may want to set "eth.type".

No, eth.type isn't modifiable.  arp { ... } always uses ARP Ethertype.

> > diff --git a/ovn/ovn-architecture.7.xml b/ovn/ovn-architecture.7.xml
> > index 47dfc2a..a7ff674 100644
> > --- a/ovn/ovn-architecture.7.xml
> > +++ b/ovn/ovn-architecture.7.xml
> > @@ -596,7 +596,7 @@
> >     </li>
> >   </ol>
> > 
> > -  <h2>Life Cycle of a Packet</h2>
> > +  <h2>Architectural Life Cycle of a Packet</h2>
> 
> There's the very similar sounding "Architectural Logical Life Cycle of
> a Packet" in ovn-sb, which I think could be confusing.  What do you
> think about throwing a "Physical" in there?

Changed, thanks.

> >       This following description focuses on the life cycle of a packet through
> >       a logical datapath, ignoring physical details of the implementation.
> > -      Please refer to <em>Life Cycle of a Packet</em> in
> > +      Please refer to <em>Architectural Life Cycle of a Packet</em> in
> 
> If you change it, this would need to be updated, too, of course.

Done.

> > +        <dt><code>ip.ttl--;</code></dt>
> > +        <dd>
> > +          <p>
> > +            Decrements the IPv4 or IPv6 TTL.  If this would make the TTL zero
> > +            or negative, then processing of the packet halts; no further
> > +            actions are processed.  (To properly handle such cases, a
> > +            higher-priority flow should match on <code>ip.ttl &lt; 2</code>.)
> > +          </p>
> 
> This is put in the "to be implemented later"category of actions, but I
> believe "ip.ttl--" got implemented a few patches ago.

You're right.  I mistakenly omitted the documentation.  I broke the
documentation of this action into a separate commit and pushed it.

> > +        <dt><code>icmp4 { <var>action</var>; </code>...<code> };</code></dt>
> > ...
> > +          <p>
> > +            XXX need to explain exactly how the ICMP packet is constructed
> > +          </p>
> 
> I don't understand what this means.

I changed it to say "Details TBD."

> Acked-by: Justin Pettit <jpettit at nicira.com>

Thanks for the detailed review.  I applied this to master.



More information about the dev mailing list