[ovs-dev] tnl-ports: Add destination IP and MAC address to the match.
diff mbox

Message ID 20150904230951.GA22502@nicira.com
State Not Applicable
Headers show

Commit Message

Ben Pfaff Sept. 4, 2015, 11:09 p.m. UTC
On Fri, Sep 04, 2015 at 02:02:56PM -0700, Pravin Shelar wrote:
> On Fri, Sep 4, 2015 at 1:50 PM, Ben Pfaff <blp@nicira.com> wrote:
> > On Wed, Sep 02, 2015 at 09:03:15PM -0700, Pravin B Shelar wrote:
> >> Currently tnl-port table wildcard destination ip and mac addresses
> >> for given tunnel packet.  That could result accepting tunnel
> >> packets destined for other hosts.  Following patch adds
> >> support for matching for ip and mac address.
> >> IP address upates to tnl-port table are piggybacked on
> >> ovs-router updates.
> >>
> >> Reported-by: Ben Pfaff <blp@nicira.com>
> >> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
> >
> > When I substitute this for my patch "ovn-controller: Attach local_ip to
> > each tunnel." in my series at
> > https://github.com/blp/ovs-reviews/tree/ovn-sandbox4, or if I use the
> > two together, I get tons of dropped packets in the test that that series
> > adds.  With just my patch, it passes consistently.
> >
> > I haven't read the code in this patch yet.  I'll try to figure what's
> > happening.
> >
> 
> I guess it is missing /32 route for the interface ip-address.
> On Linux every net-device has this route set. for example if you
> configure 1.1.1.1 on br0, there would be a route 1.1.1.1/32. This
> route is used to configure tnl-port table ip-address. If that does not
> exist then OVS will drop these packet.
> So for ovs-dummy netdevices you need to explicitly set these entries
> in ovs-router, so that tnl-port table can use it. You can have look at
> diff for tests/tunnel-push-pop.at.

OK, that's the problem then.  Now it works.

Tested-by: Ben Pfaff <blp@nicira.com>

It seems weird that I need the same route with two different prefix
lengths though.

I think you should fold this into your patch to fix up ovs-sim:

Comments

Pravin B Shelar Sept. 4, 2015, 11:19 p.m. UTC | #1
On Fri, Sep 4, 2015 at 4:09 PM, Ben Pfaff <blp@nicira.com> wrote:
> On Fri, Sep 04, 2015 at 02:02:56PM -0700, Pravin Shelar wrote:
>> On Fri, Sep 4, 2015 at 1:50 PM, Ben Pfaff <blp@nicira.com> wrote:
>> > On Wed, Sep 02, 2015 at 09:03:15PM -0700, Pravin B Shelar wrote:
>> >> Currently tnl-port table wildcard destination ip and mac addresses
>> >> for given tunnel packet.  That could result accepting tunnel
>> >> packets destined for other hosts.  Following patch adds
>> >> support for matching for ip and mac address.
>> >> IP address upates to tnl-port table are piggybacked on
>> >> ovs-router updates.
>> >>
>> >> Reported-by: Ben Pfaff <blp@nicira.com>
>> >> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
>> >
>> > When I substitute this for my patch "ovn-controller: Attach local_ip to
>> > each tunnel." in my series at
>> > https://github.com/blp/ovs-reviews/tree/ovn-sandbox4, or if I use the
>> > two together, I get tons of dropped packets in the test that that series
>> > adds.  With just my patch, it passes consistently.
>> >
>> > I haven't read the code in this patch yet.  I'll try to figure what's
>> > happening.
>> >
>>
>> I guess it is missing /32 route for the interface ip-address.
>> On Linux every net-device has this route set. for example if you
>> configure 1.1.1.1 on br0, there would be a route 1.1.1.1/32. This
>> route is used to configure tnl-port table ip-address. If that does not
>> exist then OVS will drop these packet.
>> So for ovs-dummy netdevices you need to explicitly set these entries
>> in ovs-router, so that tnl-port table can use it. You can have look at
>> diff for tests/tunnel-push-pop.at.
>
> OK, that's the problem then.  Now it works.
>
> Tested-by: Ben Pfaff <blp@nicira.com>
>
> It seems weird that I need the same route with two different prefix
> lengths though.
>
It work automatically for linux system devices. But It might not be
the case on other platforms. So
I am working on v2 which will not need such route.
Ben Pfaff Sept. 4, 2015, 11:21 p.m. UTC | #2
On Fri, Sep 04, 2015 at 04:19:52PM -0700, Pravin Shelar wrote:
> On Fri, Sep 4, 2015 at 4:09 PM, Ben Pfaff <blp@nicira.com> wrote:
> > On Fri, Sep 04, 2015 at 02:02:56PM -0700, Pravin Shelar wrote:
> >> On Fri, Sep 4, 2015 at 1:50 PM, Ben Pfaff <blp@nicira.com> wrote:
> >> > On Wed, Sep 02, 2015 at 09:03:15PM -0700, Pravin B Shelar wrote:
> >> >> Currently tnl-port table wildcard destination ip and mac addresses
> >> >> for given tunnel packet.  That could result accepting tunnel
> >> >> packets destined for other hosts.  Following patch adds
> >> >> support for matching for ip and mac address.
> >> >> IP address upates to tnl-port table are piggybacked on
> >> >> ovs-router updates.
> >> >>
> >> >> Reported-by: Ben Pfaff <blp@nicira.com>
> >> >> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
> >> >
> >> > When I substitute this for my patch "ovn-controller: Attach local_ip to
> >> > each tunnel." in my series at
> >> > https://github.com/blp/ovs-reviews/tree/ovn-sandbox4, or if I use the
> >> > two together, I get tons of dropped packets in the test that that series
> >> > adds.  With just my patch, it passes consistently.
> >> >
> >> > I haven't read the code in this patch yet.  I'll try to figure what's
> >> > happening.
> >> >
> >>
> >> I guess it is missing /32 route for the interface ip-address.
> >> On Linux every net-device has this route set. for example if you
> >> configure 1.1.1.1 on br0, there would be a route 1.1.1.1/32. This
> >> route is used to configure tnl-port table ip-address. If that does not
> >> exist then OVS will drop these packet.
> >> So for ovs-dummy netdevices you need to explicitly set these entries
> >> in ovs-router, so that tnl-port table can use it. You can have look at
> >> diff for tests/tunnel-push-pop.at.
> >
> > OK, that's the problem then.  Now it works.
> >
> > Tested-by: Ben Pfaff <blp@nicira.com>
> >
> > It seems weird that I need the same route with two different prefix
> > lengths though.
> >
> It work automatically for linux system devices. But It might not be
> the case on other platforms. So
> I am working on v2 which will not need such route.

Even better, thanks!

Patch
diff mbox

diff --git a/utilities/ovs-sim.in b/utilities/ovs-sim.in
index 2d9d66d..7aa9c7f 100755
--- a/utilities/ovs-sim.in
+++ b/utilities/ovs-sim.in
@@ -293,6 +293,7 @@  EOF
 
     ovs-appctl netdev-dummy/ip4addr $bridge $ip/$masklen >/dev/null
     ovs-appctl ovs/route/add $ip/$masklen $bridge > /dev/null
+    ovs-appctl ovs/route/add $ip/32 $bridge > /dev/null
     ovs-vsctl \
 	-- set Open_vSwitch . external-ids:system-id=$sandbox \
         -- set Open_vSwitch . external-ids:ovn-remote=unix:$sim_base/ovn-sb/ovn-sb.sock \