[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 13:42:28 UTC 2020
On Tue, Sep 8, 2020 at 6:48 PM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 9/8/20 2:06 PM, Numan Siddique wrote:
> > 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.
> >
>
> 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
> >> dev at openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list