[ovs-dev] [PATCH ovn v2] ovn-northd: Drop IP packets destined to router owned IPs (after NAT).

Tue Sep 8 13:17:35 UTC 2020

On 9/8/20 2:06 PM, Numan Siddique wrote:
> On Tue, Sep 8, 2020 at 4:54 PM Dumitru Ceara <dceara at redhat.com> wrote:
>>
>> On 9/8/20 12:58 PM, Numan Siddique wrote:
>>> On Tue, Sep 8, 2020 at 2:13 PM Dumitru Ceara <dceara at redhat.com> wrote:
>>>>
>>>> OVN was dropping IP packets destined to IPs owned by logical routers but
>>>> only if those IPs are not used for SNAT rules. However, if a packet
>>>> doesn't match an existing NAT session and its destination is still a
>>>> router owned IP, it can be safely dropped. Otherwise it will trigger an
>>>> unnecessary packet-in in stage lr_in_arp_request.
>>>>
>>>> To achieve that we add flows that drop traffic to router owned IPs in
>>>> table lr_in_arp_resolve.
>>>>
>>>> Reported-by: Tim Rozet <trozet at redhat.com>
>>>> Reported-at: https://bugzilla.redhat.com/1876174
>>>> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
>>>
>>>
>>>
>>> Hi Dumitru,
>>>
>>> Thanks for the fix.
>>>
>>> I have few comments.
>>>
>>> Suppose we have the below lr and NAT entries
>>>
>>> router 392b19fe-adf9-4b3d-bf43-040d12240e54 (lr0)
>>>     port lr0-sw0
>>>         mac: "00:00:00:00:ff:01"
>>>         networks: ["10.0.0.1/24"]
>>>     port lr0-sw1
>>>         mac: "00:00:00:00:ff:02"
>>>         networks: ["20.0.0.1/24"]
>>>     port lr0-public
>>>         mac: "00:00:20:20:12:13"
>>>         networks: ["172.168.0.100/24"]
>>>         gateway chassis: [chassis-1]
>>>     nat 4c26f3f0-0ae0-4c18-9a1e-9d2751389270
>>>         external ip: "172.168.0.110"
>>>         logical ip: "10.0.0.3"
>>>         type: "dnat_and_snat"
>>>     nat b9296dc8-ac3a-427a-a97a-a94bf16214b2
>>>         external ip: "172.168.0.120"
>>>         logical ip: "20.0.0.3"
>>>         type: "dnat_and_snat"
>>>     nat da28129c-4e81-4e20-865a-768bd88e5a26
>>>         external ip: "172.168.0.100"
>>>         logical ip: "10.0.0.0/24"
>>>         type: "snat"
>>>
>>> I see the below lflows added in lr_in_ip_input to drop the pkts for
>>> router port IPs
>>>
>>> table=3 (lr_in_ip_input     ), priority=60   , match=(ip4.dst ==
>>> {10.0.0.1} || ip6.dst == {fe80::200:ff:fe00:ff01}), action=(drop;)
>>> table=3 (lr_in_ip_input     ), priority=60   , match=(ip4.dst ==
>>> {20.0.0.1} || ip6.dst == {fe80::200:ff:fe00:ff02}), action=(drop;)
>>>
>>> So I think there is no need to add the lflows in lr_in_arp_resolve to
>>> drop for 10.0.0.1 and 20.0.0.1 again.
>>>
>>> I think it's sufficient to add lflows in lr_in_arp_resolve to drop the
>>> packet only for packets destined to the router ips which have NAT
>>> entries.
>>>
>>
>> Hi Numan,
>>
>> Thanks for the review.
>>
>> You're right we could try to optimize a bit the number of flows.
>>
>> However, I didn't want to duplicate the code that builds snat_ips [0]
>> and I thought it complicates the already long build_lrouter_flows()
>> function if I add flows to stage LR_IN_ARP_RESOLVE in the same place
>> where we add flows for LR_IN_IP_INPUT.
>>
>> Also, the number of IP addresses per router port is usually low so the
>> number of redundant logical flows in lr_in_arp_resolve would be low too.
>>
>> If you really have a strong preference about this, I can try to change
>> it though.
>>
> 
> Hi Dumitru,
> 
> I was thinking more from the scale perspective. I think this issue is
> to address ovn-k8s use case.
> Although the gateway router on each node will be configured with NAT
> entries and it will be
> definitely having very few router ports, but I think on a large scale
> setup, the cluster router will have
> many router ports and we will see these flows on this router even
> though there are no NAT entries
> for the cluster router.
> 
> For the openstack deployment too we will see these flows and a logical
> router with gateway router port
> may have many other router ports connecting to the tenant logical switches.
> 
> It would be good if we address my comment. Or at the least add these
> lflows only for routers configured
> with NAT entries.
> 

We could also just add the flows to lr_in_arp_resolve and remove the
ones that drop traffic destined to router-owned IPs in lr_in_ip_input.

It's a bit ugly because we're forcing this in a stage that should
resolve ARP but lr_in_ip_input is too early for traffic that might get
NAT-ed.

What do you think?

> @Han Zhou  Do you have comments or concerns with this patch ?
> 
> I am raising these points, because you and Han addressed lflow
> explosion issues a while back.
> Although in this case, the flows added in this patch do not result in
> exponential increase.
> 

Right, here there should be max 1 flow per IP address so it shouldn't
create a scale issue.

Thanks,
Dumitru

> Thanks
> Numan
> 
> 
>> Regards,
>> Dumitru
>>
>> [0] https://github.com/ovn-org/ovn/blob/master/northd/ovn-northd.c#L9015
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
> 