On Thu, Sep 17, 2020 at 6:25 PM Dumitru Ceara <[email protected]> wrote:
> 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 > > Thanks. I'll take a look. Numan > Regards, > Dumitru > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
