On 9/8/20 3:42 PM, Numan Siddique wrote:
> On Tue, Sep 8, 2020 at 6:48 PM Dumitru Ceara <[email protected]> wrote:
>>
>> On 9/8/20 2:06 PM, Numan Siddique wrote:
>>> On Tue, Sep 8, 2020 at 4:54 PM Dumitru Ceara <[email protected]> wrote:
>>>>
>>>> On 9/8/20 12:58 PM, Numan Siddique wrote:
>>>>> On Tue, Sep 8, 2020 at 2:13 PM Dumitru Ceara <[email protected]> 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 <[email protected]>
>>>>>> Reported-at: https://bugzilla.redhat.com/1876174
>>>>>> Signed-off-by: Dumitru Ceara <[email protected]>
>>>>>
>>>>>
>>>>>
>>>>> 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
