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. 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
