[ovs-dev] [PATCH ovn 2/4] ovn-nbctl.c: Fix lr-route-add/del command with policy considered.
Han Zhou
hzhou at ovn.org
Tue Jan 7 02:30:03 UTC 2020
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 at 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(-)
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 2679f1f..22bc38b 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -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
])
diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
index 88ebd13..ec7c28c 100644
--- a/utilities/ovn-nbctl.8.xml
+++ b/utilities/ovn-nbctl.8.xml
@@ -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>
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index 46ba3a9..10c458f 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -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 },
--
2.1.0
More information about the dev
mailing list