Hi Lorenzo, a few comments down below.
On 2/3/21 10:54 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 <[email protected]>
---
tests/ovn-nbctl.at | 19 +++++-----
utilities/ovn-nbctl.c | 81 +++++++++++++++++++++++++++----------------
2 files changed, 63 insertions(+), 37 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..07f2b008d 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -3917,6 +3917,48 @@ nbctl_lr_policy_list(struct ctl_context *ctx)
}
free(policies);
}
+
+static struct nbrec_logical_router_static_route *
+nbctl_lr_get_route_prefix(const struct nbrec_logical_router *lr,
+ char *prefix, char *next_hop, bool is_src_route,
+ bool ecmp)
I think this function should be renamed to "nbctl_lr_get_route". The
return type is a static route, not a prefix.
+{
+ 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,42 +4030,21 @@ 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_prefix(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;
}
+ char *rt_prefix = normalize_prefix_str(route->ip_prefix);
+ if (!rt_prefix) {
+ goto cleanup;
+ }
This can be removed:
1) Since route is non-NULL, normalize_prefix_str(route->ip_prefix) is
guaranteed to return non-NULL as well.
2) rt_prefix is not actually being used anywhere in this block.
/* Update the next hop for an existing route. */
nbrec_logical_router_verify_static_routes(lr);
nbrec_logical_router_static_route_verify_ip_prefix(route);
@@ -4052,9 +4073,11 @@ nbctl_lr_route_add(struct ctl_context *ctx)
free(rt_prefix);
goto cleanup;
}
+ } else if (route && !strcmp(route->nexthop, next_hop)) {
You can remove the strcmp() call. Since ecmp is true, route->nexthop and
next_hop must be the same in order for route to be non-NULL.
+ 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
