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

Dumitru Ceara dceara at redhat.com
Tue Sep 8 18:39:36 UTC 2020


On 9/8/20 3:42 PM, Numan Siddique wrote:
> On Tue, Sep 8, 2020 at 6:48 PM Dumitru Ceara <dceara at redhat.com> wrote:
>>
>> 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?
> 
> I think we can keep the existing flows in lr_in_ip_input and add only
> the required flows in lr_in_arp_resolve
> to drop for the router ips which have NAT entry ?
> 

OK. I'll work on v3.

> I would do something like
> 
> HMAP_FOR_EACH (op, key_node, ports) {
>         if (!op->nbrp) {
>             continue;
>         }
> 
>        for (int i = 0; i < od->nbr->n_nat; i++) {
>           struct ovn_nat *nat_entry = &od->nat_entries[i];
>           const struct nbrec_nat *nat = nat_entry->nb;
> 
>          if  (nat->externa_ip belongs to one of the logical router ip) {
>                 /* Drop traffic with IP.dest == router-ip. */
>                ovn_lflow_add_with_hint(lflows, op->od,
> S_ROUTER_IN_ARP_RESOLVE, 1,
>                                     "ip4.dst == %s, "drop;",
>                                     &op->nbrp->header_, nat_entry->external_ip);
> 
>          }
>      }
> }

This is not really enough, we'd also need to check if
lb_force_snat_ip/dnat_force_snat_ip are also router IPs so the above
will basically mean rewriting almost the same code as here (lr_in_ip_input):
https://github.com/ovn-org/ovn/blob/fc79d690b9e5479ce0190a9808c21fdca76575c8/northd/ovn-northd.c#L9015

I'll find a way to refactor it a bit so we don't have so much code
duplication and at the same time not have any redundant flows.

Regards,
Dumitru



More information about the dev mailing list