[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 && 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> && ip4.ttl == {0, 1} &&
> > + !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 <
> > + 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 < 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