[ovs-dev] [PATCH ovn v2] ovn-northd: Drop IP packets destined to router owned IPs (after NAT).
Numan Siddique
numans at ovn.org
Thu Sep 17 13:19:29 UTC 2020
On Thu, Sep 17, 2020 at 6:25 PM Dumitru Ceara <dceara at redhat.com> 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 <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 ?
> >>
> >
> > 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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
More information about the dev
mailing list