[ovs-dev] Tunnel matching and remote_ip=flow (was Re: [ovs-discuss] vxlan remote_ip flow help)

[Jesse Gross]

On Fri, Jan 31, 2014 at 4:59 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Fri, Jan 31, 2014 at 10:38:20AM -0800, Jesse Gross wrote:
>> On Thu, Jan 30, 2014 at 1:31 PM, Thomas Morin <thomas.morin at orange.com> wrote:
>> > Hi,
>> >
>> > I ran into the same issue as the one described below (post on ovs-discuss a
>> > few months back).
>> >
>> > The goal is to use a vport tunnel configured to use a flow-based remote IP,
>> > e.g:
>> >     ovs-vscl add-port br-vnet tun-port -- set Interface tun-port type=gre
>> > options:remote_ip=flow
>> >
>> > This works fine on the sending OVS, but on the receiving OVS the same vport
>> > won't receive any traffic (packets received trigger a "receive tunnel port
>> > not found" warning, and are not further processed).
>> >
>> > The original poster had found a workaround relying on also using
>> > options:key=flow, but as far as I understand, this work around should not be
>> > necessary: it just seems that the tunnel matching code is missing a match.
>> >
>> > --- a/ofproto/tunnel.c
>> > +++ b/ofproto/tunnel.c
>> > @@ -427,8 +427,9 @@ tnl_find(const struct flow *flow) OVS_REQ_RDLOCK(rwlock)
>> >          { false, false, IP_SRC_ANY },  /* remote_ip, in_key. */
>> >          { true,  false, IP_SRC_CFG },  /* remote_ip, local_ip. */
>> >          { true,  false, IP_SRC_ANY },  /* remote_ip. */
>> > -        { true,  true,  IP_SRC_ANY },  /* Flow-based remote. */
>> > +        { true,  true,  IP_SRC_ANY },  /* Flow-based remote and flow-based
>> > key. */
>> >          { true,  true,  IP_SRC_FLOW }, /* Flow-based everything. */
>> > +        { false, true,  IP_SRC_ANY },  /* Flow-based remote */
>> >      };
>> >
>> >      const struct tnl_match_pattern *p;
>> >
>> > With this patch, a tunnel configured with just remote_ip=flow will work fine
>> > on the receiving end.
>> > One thing that I'm not sure is at which position this pattern should appear
>> > in the list.
>> > The patch also changes the comment for true/true/IP_SRC_ANY.
>>
>> This looks fine to me although I think that it the new entry belongs
>> first in the list of flow-based types. Can you submit a formal patch
>> with this change?
>
> By my count there are 12 possible entries in the list of
> possibilities.  I don't know if there's any point in adding them one
> by one.  I guess we might as well add all of them at one
> systematically.
>
> How about this, then?  It's not correct--I haven't debugged it--but it
> gives the flavor of the idea.

I think it is a reasonable thing to do although I think it might
change the priority order that we have now - should we switch the two
outer loops when we do the lookup?