[ovs-dev] [PATCH v2 ovn] ovn-nbctl: do not allow duplicated ECMP routes

Lorenzo Bianconi lorenzo.bianconi at redhat.com
Thu Feb 11 09:16:40 UTC 2021


> On Thu, Feb 11, 2021 at 1:32 AM Mark Michelson <mmichels at redhat.com> wrote:
> >
> > Thanks Lorenzo,
> >
> > Acked-by: Mark Michelson <mmichels at redhat.com>
> 
> Thanks. I applied this patch to master.
> 
> Does it need a backport ?

Hi Numan,

I think so, but I guess it did not apply cleanly since we need to backport bfd patch as well.
Do you want me to do the backport?

Regards,
Lorenzo

> 
> Numan
> 
> >
> > On 2/6/21 4:53 AM, Lorenzo Bianconi wrote:
> > > In the current ovn-nbctl lr-route-add implementation is possible to add
> > > multiple duplicated routes adding --ecmp or --ecmp-symmetric-reply
> > > option, e.g:
> > >
> > > $ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 10.244.0.7/32 172.18.0.4
> > > $ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 10.244.0.7/32 172.18.0.4
> > > $ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 10.244.0.7/32 172.18.0.4
> > >
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1916842
> > >
> > > Fix the issue checking nexthop for ECMP routes.
> > >
> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> > > ---
> > > Changes since v1:
> > > - rename nbctl_lr_get_route_prefix() in nbctl_lr_get_route()
> > > - drop normalize_prefix_str routine in nbctl_lr_route_add()
> > > - remove unnecessary strcmp() in nbctl_lr_route_add()
> > > ---
> > >   tests/ovn-nbctl.at    | 19 ++++++-----
> > >   utilities/ovn-nbctl.c | 77 ++++++++++++++++++++++++++-----------------
> > >   2 files changed, 58 insertions(+), 38 deletions(-)
> > >
> > > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> > > index e4a2a9695..6d91aa4c5 100644
> > > --- a/tests/ovn-nbctl.at
> > > +++ b/tests/ovn-nbctl.at
> > > @@ -1539,34 +1539,34 @@ AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
> > >   dnl Add ecmp routes
> > >   AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.0/24 11.0.0.1])
> > >   AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.2])
> > > -AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.2])
> > >   AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.3])
> > > -AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.3 lp0])
> > > +AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.4 lp0])
> > >   AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
> > >   IPv4 Routes
> > >                 10.0.0.0/24                  11.0.0.1 dst-ip ecmp
> > >                 10.0.0.0/24                  11.0.0.2 dst-ip ecmp
> > > -              10.0.0.0/24                  11.0.0.2 dst-ip ecmp
> > >                 10.0.0.0/24                  11.0.0.3 dst-ip ecmp
> > > -              10.0.0.0/24                  11.0.0.3 dst-ip lp0 ecmp
> > > +              10.0.0.0/24                  11.0.0.4 dst-ip lp0 ecmp
> > > +])
> > > +AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 10.0.0.0/24 11.0.0.2], [1], [],
> > > +  [ovn-nbctl: duplicate nexthop for the same ECMP route
> > >   ])
> > >
> > >   dnl Delete ecmp routes
> > >   AT_CHECK([ovn-nbctl lr-route-del lr0 10.0.0.0/24 11.0.0.1])
> > >   AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
> > >   IPv4 Routes
> > > -              10.0.0.0/24                  11.0.0.2 dst-ip ecmp
> > >                 10.0.0.0/24                  11.0.0.2 dst-ip ecmp
> > >                 10.0.0.0/24                  11.0.0.3 dst-ip ecmp
> > > -              10.0.0.0/24                  11.0.0.3 dst-ip lp0 ecmp
> > > +              10.0.0.0/24                  11.0.0.4 dst-ip lp0 ecmp
> > >   ])
> > >   AT_CHECK([ovn-nbctl lr-route-del lr0 10.0.0.0/24 11.0.0.2])
> > >   AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
> > >   IPv4 Routes
> > >                 10.0.0.0/24                  11.0.0.3 dst-ip ecmp
> > > -              10.0.0.0/24                  11.0.0.3 dst-ip lp0 ecmp
> > > +              10.0.0.0/24                  11.0.0.4 dst-ip lp0 ecmp
> > >   ])
> > > -AT_CHECK([ovn-nbctl lr-route-del lr0 10.0.0.0/24 11.0.0.3 lp0])
> > > +AT_CHECK([ovn-nbctl lr-route-del lr0 10.0.0.0/24 11.0.0.4 lp0])
> > >   AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
> > >   IPv4 Routes
> > >                 10.0.0.0/24                  11.0.0.3 dst-ip
> > > @@ -1611,6 +1611,9 @@ AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 2001:0db8:1::/64 2001:0db8:0:f103::3
> > >   AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 2001:0db8:1::/64 2001:0db8:0:f103::4])
> > >   AT_CHECK([ovn-nbctl lr-route-add lr0 2002:0db8:1::/64 2001:0db8:0:f103::5])
> > >   AT_CHECK([ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 2003:0db8:1::/64 2001:0db8:0:f103::6])
> > > +AT_CHECK([ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 2003:0db8:1::/64 2001:0db8:0:f103::6], [1], [],
> > > +  [ovn-nbctl: duplicate nexthop for the same ECMP route
> > > +])
> > >
> > >   AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
> > >   IPv4 Routes
> > > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> > > index 3f28ba11a..2c77f4ba7 100644
> > > --- a/utilities/ovn-nbctl.c
> > > +++ b/utilities/ovn-nbctl.c
> > > @@ -3917,6 +3917,47 @@ nbctl_lr_policy_list(struct ctl_context *ctx)
> > >       }
> > >       free(policies);
> > >   }
> > > +
> > > +static struct nbrec_logical_router_static_route *
> > > +nbctl_lr_get_route(const struct nbrec_logical_router *lr, char *prefix,
> > > +                   char *next_hop, bool is_src_route, bool ecmp)
> > > +{
> > > +    for (int i = 0; i < lr->n_static_routes; i++) {
> > > +        struct nbrec_logical_router_static_route *route = lr->static_routes[i];
> > > +
> > > +        /* Compare route policy. */
> > > +        char *nb_policy = route->policy;
> > > +        bool nb_is_src_route = false;
> > > +        if (nb_policy && !strcmp(nb_policy, "src-ip")) {
> > > +                nb_is_src_route = true;
> > > +        }
> > > +        if (is_src_route != nb_is_src_route) {
> > > +            continue;
> > > +        }
> > > +
> > > +        /* Compare route prefix. */
> > > +        char *rt_prefix = normalize_prefix_str(route->ip_prefix);
> > > +        if (!rt_prefix) {
> > > +            /* Ignore existing prefix we couldn't parse. */
> > > +            continue;
> > > +        }
> > > +
> > > +        if (strcmp(rt_prefix, prefix)) {
> > > +            free(rt_prefix);
> > > +            continue;
> > > +        }
> > > +
> > > +        if (ecmp && strcmp(next_hop, route->nexthop)) {
> > > +            free(rt_prefix);
> > > +            continue;
> > > +        }
> > > +
> > > +        free(rt_prefix);
> > > +        return route;
> > > +    }
> > > +    return NULL;
> > > +}
> > > +
> > >
> > >   static void
> > >   nbctl_lr_route_add(struct ctl_context *ctx)
> > > @@ -3988,39 +4029,14 @@ nbctl_lr_route_add(struct ctl_context *ctx)
> > >                                              "--ecmp-symmetric-reply") != NULL;
> > >       bool ecmp = shash_find(&ctx->options, "--ecmp") != NULL ||
> > >                   ecmp_symmetric_reply;
> > > +    struct nbrec_logical_router_static_route *route =
> > > +        nbctl_lr_get_route(lr, prefix, next_hop, is_src_route, ecmp);
> > >       if (!ecmp) {
> > > -        for (int i = 0; i < lr->n_static_routes; i++) {
> > > -            const struct nbrec_logical_router_static_route *route
> > > -                = lr->static_routes[i];
> > > -            char *rt_prefix;
> > > -
> > > -            /* Compare route policy. */
> > > -            char *nb_policy = lr->static_routes[i]->policy;
> > > -            bool nb_is_src_route = false;
> > > -            if (nb_policy && !strcmp(nb_policy, "src-ip")) {
> > > -                    nb_is_src_route = true;
> > > -            }
> > > -            if (is_src_route != nb_is_src_route) {
> > > -                continue;
> > > -            }
> > > -
> > > -            /* Compare route prefix. */
> > > -            rt_prefix = normalize_prefix_str(lr->static_routes[i]->ip_prefix);
> > > -            if (!rt_prefix) {
> > > -                /* Ignore existing prefix we couldn't parse. */
> > > -                continue;
> > > -            }
> > > -
> > > -            if (strcmp(rt_prefix, prefix)) {
> > > -                free(rt_prefix);
> > > -                continue;
> > > -            }
> > > -
> > > +        if (route) {
> > >               if (!may_exist) {
> > >                   ctl_error(ctx, "duplicate prefix: %s (policy: %s). Use option"
> > >                             " --ecmp to allow this for ECMP routing.",
> > >                             prefix, is_src_route ? "src-ip" : "dst-ip");
> > > -                free(rt_prefix);
> > >                   goto cleanup;
> > >               }
> > >
> > > @@ -4049,12 +4065,13 @@ nbctl_lr_route_add(struct ctl_context *ctx)
> > >                   }
> > >                   nbrec_logical_router_static_route_set_bfd(route, nb_bt);
> > >               }
> > > -            free(rt_prefix);
> > >               goto cleanup;
> > >           }
> > > +    } else if (route) {
> > > +        ctl_error(ctx, "duplicate nexthop for the same ECMP route");
> > > +        goto cleanup;
> > >       }
> > >
> > > -    struct nbrec_logical_router_static_route *route;
> > >       route = nbrec_logical_router_static_route_insert(ctx->txn);
> > >       nbrec_logical_router_static_route_set_ip_prefix(route, prefix);
> > >       nbrec_logical_router_static_route_set_nexthop(route, next_hop);
> > >
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> 


More information about the dev mailing list