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 <[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 >
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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
