@@ -1335,7 +1335,7 @@ AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.1/24 11.0.0.2])
dnl Add overlapping route with 10.0.0.1/24
AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111/24 11.0.0.1], [1], [],
- [ovn-nbctl: duplicate prefix: 10.0.0.0/24
+ [ovn-nbctl: duplicate prefix: 10.0.0.0/24 (policy: dst-ip)
])
AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111a/24 11.0.0.1], [1], [],
[ovn-nbctl: bad prefix argument: 10.0.0.111a/24
@@ -1355,12 +1355,15 @@ AT_CHECK([ovn-nbctl lr-route-add lr0 2001:0db8:1::/64 2001:0db8:0:f103::1/64], [
AT_CHECK([ovn-nbctl --may-exist lr-route-add lr0 10.0.0.111/24 11.0.0.1])
AT_CHECK([ovn-nbctl --policy=src-ip lr-route-add lr0 9.16.1.0/24 11.0.0.1])
+dnl Add a route with existed prefix but different policy (src-ip)
+AT_CHECK([ovn-nbctl --policy=src-ip lr-route-add 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.1 dst-ip
10.0.1.0/24 11.0.1.1 dst-ip lp0
9.16.1.0/24 11.0.0.1 src-ip
+ 10.0.0.0/24 11.0.0.2 src-ip
0.0.0.0/0 192.168.0.1 dst-ip
])
@@ -1370,24 +1373,44 @@ IPv4 Routes
10.0.0.0/24 11.0.0.1 dst-ip lp1
10.0.1.0/24 11.0.1.1 dst-ip lp0
9.16.1.0/24 11.0.0.1 src-ip
+ 10.0.0.0/24 11.0.0.2 src-ip
0.0.0.0/0 192.168.0.1 dst-ip
])
dnl Delete non-existent prefix
AT_CHECK([ovn-nbctl lr-route-del lr0 10.0.2.1/24], [1], [],
- [ovn-nbctl: no matching prefix: 10.0.2.0/24
+ [ovn-nbctl: no matching prefix: 10.0.2.0/24 (policy: any)
])
AT_CHECK([ovn-nbctl --if-exists lr-route-del lr0 10.0.2.1/24])
AT_CHECK([ovn-nbctl lr-route-del lr0 10.0.1.1/24])
-AT_CHECK([ovn-nbctl lr-route-del lr0 9.16.1.0/24])
+AT_CHECK([ovn-nbctl --policy=src-ip lr-route-del lr0 9.16.1.0/24])
AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
IPv4 Routes
10.0.0.0/24 11.0.0.1 dst-ip lp1
+ 10.0.0.0/24 11.0.0.2 src-ip
0.0.0.0/0 192.168.0.1 dst-ip
])
+dnl Delete route by explicitely specifying --policy
+AT_CHECK([ovn-nbctl --policy=dst-ip lr-route-del lr0 10.0.0.0/24])
+AT_CHECK([ovn-nbctl --policy=src-ip lr-route-del lr0 10.0.0.0/24])
+AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
+IPv4 Routes
+ 0.0.0.0/0 192.168.0.1 dst-ip
+])
+
+dnl Delete route without specifying --policy
+AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.0/24 11.0.0.1])
+AT_CHECK([ovn-nbctl --policy=src-ip lr-route-add lr0 10.0.0.0/24 11.0.0.2])
+AT_CHECK([ovn-nbctl lr-route-del lr0 10.0.0.0/24])
+AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
+IPv4 Routes
+ 0.0.0.0/0 192.168.0.1 dst-ip
+])
+
+dnl Delete all routes for the router
AT_CHECK([ovn-nbctl lr-route-del lr0])
AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
])
@@ -634,25 +634,25 @@
</p>
<p>
- It is an error if a route with <var>prefix</var> already exists,
- unless <code>--may-exist</code> is specified.
+ It is an error if a route with <var>prefix</var> and
+ <var>POLICY</var> already exists, unless <code>--may-exist</code> is
+ specified.
</p>
</dd>
- <dt>[<code>--if-exists</code>] <code>lr-route-del</code> <var>router</var> [<var>prefix</var>]</dt>
+ <dt>[<code>--if-exists</code>] [<code>--policy</code>=<var>POLICY</var>] <code>lr-route-del</code> <var>router</var> [<var>prefix</var>]</dt>
<dd>
<p>
Deletes routes from <var>router</var>. If only <var>router</var>
is supplied, all the routes from the logical router are
- deleted. If <var>prefix</var> is also specified, then all the
- routes that match the prefix will be deleted from the logical
- router.
+ deleted. If <var>POLICY</var> and/or <var>prefix</var> are also
+ specified, then all the routes that match the conditions will be
+ deleted from the logical router.
</p>
<p>
- It is an error if <var>prefix</var> is specified and there
- is no matching route entry, unless <code>--if-exists</code> is
- specified.
+ It is an error if there is no matching route entry, unless
+ <code>--if-exists</code> is specified.
</p>
</dd>
@@ -682,7 +682,7 @@ Logical router port commands:\n\
Route commands:\n\
[--policy=POLICY] lr-route-add ROUTER PREFIX NEXTHOP [PORT]\n\
add a route to ROUTER\n\
- lr-route-del ROUTER [PREFIX]\n\
+ [--policy=POLICY] lr-route-del ROUTER [PREFIX]\n\
remove routes from ROUTER\n\
lr-route-list ROUTER print routes for ROUTER\n\
\n\
@@ -3697,9 +3697,14 @@ nbctl_lr_route_add(struct ctl_context *ctx)
char *prefix, *next_hop;
const char *policy = shash_find_data(&ctx->options, "--policy");
- if (policy && strcmp(policy, "src-ip") && strcmp(policy, "dst-ip")) {
- ctl_error(ctx, "bad policy: %s", policy);
- return;
+ bool is_src_route = false;
+ if (policy) {
+ if (!strcmp(policy, "src-ip")) {
+ is_src_route = true;
+ } else if (strcmp(policy, "dst-ip")) {
+ ctl_error(ctx, "bad policy: %s", policy);
+ return;
+ }
}
prefix = normalize_prefix_str(ctx->argv[2]);
@@ -3739,6 +3744,17 @@ nbctl_lr_route_add(struct ctl_context *ctx)
= 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. */
@@ -3751,7 +3767,8 @@ nbctl_lr_route_add(struct ctl_context *ctx)
}
if (!may_exist) {
- ctl_error(ctx, "duplicate prefix: %s", prefix);
+ ctl_error(ctx, "duplicate prefix: %s (policy: %s)",
+ prefix, is_src_route ? "src-ip" : "dst-ip");
free(next_hop);
free(rt_prefix);
free(prefix);
@@ -3811,45 +3828,80 @@ nbctl_lr_route_del(struct ctl_context *ctx)
return;
}
- if (ctx->argc == 2) {
- /* If a prefix is not specified, delete all routes. */
+ const char *policy = shash_find_data(&ctx->options, "--policy");
+ bool is_src_route = false;
+ if (policy) {
+ if (!strcmp(policy, "src-ip")) {
+ is_src_route = true;
+ } else if (strcmp(policy, "dst-ip")) {
+ ctl_error(ctx, "bad policy: %s", policy);
+ return;
+ }
+ }
+
+ if (ctx->argc == 2 && !policy) {
+ /* If neither prefix nor policy is specified, delete all routes. */
nbrec_logical_router_set_static_routes(lr, NULL, 0);
return;
}
- char *prefix = normalize_prefix_str(ctx->argv[2]);
- if (!prefix) {
- ctl_error(ctx, "bad prefix argument: %s", ctx->argv[2]);
- return;
+ char *prefix = NULL;
+ if (ctx->argc >= 3) {
+ prefix = normalize_prefix_str(ctx->argv[2]);
+ if (!prefix) {
+ ctl_error(ctx, "bad prefix argument: %s", ctx->argv[2]);
+ return;
+ }
}
+ struct nbrec_logical_router_static_route **new_routes
+ = xmemdup(lr->static_routes,
+ sizeof *new_routes * lr->n_static_routes);
+ size_t n_new = 0;
for (int i = 0; i < lr->n_static_routes; i++) {
- char *rt_prefix = normalize_prefix_str(lr->static_routes[i]->ip_prefix);
- if (!rt_prefix) {
- /* Ignore existing prefix we couldn't parse. */
- continue;
+ /* Compare route policy, if specified. */
+ if (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) {
+ new_routes[n_new++] = lr->static_routes[i];
+ continue;
+ }
}
- if (!strcmp(prefix, rt_prefix)) {
- struct nbrec_logical_router_static_route **new_routes
- = xmemdup(lr->static_routes,
- sizeof *new_routes * lr->n_static_routes);
+ /* Compare route prefix, if specified. */
+ if (prefix) {
+ char *rt_prefix =
+ normalize_prefix_str(lr->static_routes[i]->ip_prefix);
+ if (!rt_prefix) {
+ /* Ignore existing prefix we couldn't parse. */
+ new_routes[n_new++] = lr->static_routes[i];
+ continue;
+ }
- new_routes[i] = lr->static_routes[lr->n_static_routes - 1];
- nbrec_logical_router_verify_static_routes(lr);
- nbrec_logical_router_set_static_routes(lr, new_routes,
- lr->n_static_routes - 1);
- free(new_routes);
+ if (strcmp(prefix, rt_prefix)) {
+ new_routes[n_new++] = lr->static_routes[i];
+ }
free(rt_prefix);
- free(prefix);
- return;
}
- free(rt_prefix);
+ }
+
+ if (n_new < lr->n_static_routes) {
+ nbrec_logical_router_verify_static_routes(lr);
+ nbrec_logical_router_set_static_routes(lr, new_routes, n_new);
+ free(new_routes);
+ free(prefix);
+ return;
}
if (!shash_find(&ctx->options, "--if-exists")) {
- ctl_error(ctx, "no matching prefix: %s", prefix);
+ ctl_error(ctx, "no matching prefix: %s (policy: %s)",
+ prefix, policy ? policy : "any");
}
+ free(new_routes);
free(prefix);
}
@@ -5736,7 +5788,7 @@ static const struct ctl_command_syntax nbctl_commands[] = {
{ "lr-route-add", 3, 4, "ROUTER PREFIX NEXTHOP [PORT]", NULL,
nbctl_lr_route_add, NULL, "--may-exist,--policy=", RW },
{ "lr-route-del", 1, 2, "ROUTER [PREFIX]", NULL, nbctl_lr_route_del,
- NULL, "--if-exists", RW },
+ NULL, "--if-exists,--policy=", RW },
{ "lr-route-list", 1, 1, "ROUTER", NULL, nbctl_lr_route_list, NULL,
"", RO },
When adding a new route, if the prefix existed but the policy is different, then they are different routes and the new one should be added, instead of updating the old one that has different policy. For example, below commands should add two routes. ovn-nbctl lr-route-add lr1 10.0.0.0/24 192.168.0.1 ovn-nbctl --policy=src-ip lr-route-add lr1 10.0.0.0/24 192.168.0.2 Without this patch, the second command will fail. It would succeed with --may-exist, but it would end up with only the second one added. Similarly, this patch fixes the ambiguity for lr-route-del command by adding --policy option to specify the policy together with prefix for matching the route that is supposed to be deleted. If --policy is not specified, it deletes all routes with the specified prefix. Without this patch, the command would delete a route randomly with the given prefix, and there was no way to acurrately specify the route to be deleted when there are multiple routes with same prefix but different policy. Signed-off-by: Han Zhou <hzhou@ovn.org> --- tests/ovn-nbctl.at | 29 ++++++++++-- utilities/ovn-nbctl.8.xml | 18 ++++---- utilities/ovn-nbctl.c | 110 ++++++++++++++++++++++++++++++++++------------ 3 files changed, 116 insertions(+), 41 deletions(-)