[ovs-dev] [PATCH v2 ovn] ovn-nbctl: do not allow duplicated ECMP routes
Mark Michelson
mmichels at redhat.com
Wed Feb 10 20:01:25 UTC 2021
Thanks Lorenzo,
Acked-by: Mark Michelson <mmichels at redhat.com>
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);
>
More information about the dev
mailing list