On Tue, Sep 8, 2020 at 6:48 PM Dumitru Ceara <dce...@redhat.com> wrote: > > On 9/8/20 2:06 PM, Numan Siddique wrote: > > On Tue, Sep 8, 2020 at 4:54 PM Dumitru Ceara <dce...@redhat.com> wrote: > >> > >> On 9/8/20 12:58 PM, Numan Siddique wrote: > >>> On Tue, Sep 8, 2020 at 2:13 PM Dumitru Ceara <dce...@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 <tro...@redhat.com> > >>>> Reported-at: https://bugzilla.redhat.com/1876174 > >>>> Signed-off-by: Dumitru Ceara <dce...@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 ? 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); } } } Thanks Numan > > > @Han Zhou Do you have comments or concerns with this patch ? > > > > I am raising these points, because you and Han addressed lflow > > explosion issues a while back. > > Although in this case, the flows added in this patch do not result in > > exponential increase. > > > > Right, here there should be max 1 flow per IP address so it shouldn't > create a scale issue. > > Thanks, > Dumitru > > > Thanks > > Numan > > > > > >> Regards, > >> Dumitru > >> > >> [0] https://github.com/ovn-org/ovn/blob/master/northd/ovn-northd.c#L9015 > >> > >> _______________________________________________ > >> dev mailing list > >> d...@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > > > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev