[ovs-dev] [PATCH ovn v2] ovn-northd: Drop IP packets destined to router owned IPs (after NAT).
Dumitru Ceara
dceara at redhat.com
Thu Sep 17 12:54:59 UTC 2020
On 9/8/20 8:39 PM, Dumitru Ceara wrote:
> 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
>
Hi Numan,
I sent v4 as series, the first patch being the bug fix and the following
refactoring work:
http://patchwork.ozlabs.org/project/ovn/list/?series=202406
Regards,
Dumitru
More information about the dev
mailing list