[ovs-dev] [PATCH ovn v2] ovn-northd: Drop IP packets destined to router owned IPs (after NAT).

Numan Siddique numans at ovn.org
Tue Sep 8 12:06:05 UTC 2020


On Tue, Sep 8, 2020 at 4:54 PM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 9/8/20 12:58 PM, Numan Siddique wrote:
> > On Tue, Sep 8, 2020 at 2:13 PM Dumitru Ceara <dceara at 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 <trozet at redhat.com>
> >> Reported-at: https://bugzilla.redhat.com/1876174
> >> Signed-off-by: Dumitru Ceara <dceara at 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.

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

Thanks
Numan


> Regards,
> Dumitru
>
> [0] https://github.com/ovn-org/ovn/blob/master/northd/ovn-northd.c#L9015
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list