> On Jun 9, 2016, at 3:17 PM, Ben Pfaff <b...@ovn.org> wrote:
>
> On Thu, Jun 09, 2016 at 12:12:39AM -0700, Justin Pettit wrote:
>> Signed-off-by: Justin Pettit <jpet...@ovn.org>
>
> I think the documentation can be improved. Suggested incremental
> appended.
>
> It's not actually necessary to call
> nbrec_logical_router_verify_static_routes() when deleting all routes,
> because the result of deleting all of them does not depend on what was
> there before. (This comment is petty.)
Always happy to delete some code. Thanks.
> Most of our *-add and *-del commands fail, by default, if there is
> already a duplicate or if there is nothing matching to delete,
> respectively, unless given --may-exist or --if-exists, respectively, but
> these new commands do not behave that way. (In particular adding a
> duplicate static route seems like annoying behavior.)
That's fair. While adding this functionality, I decided to normalize the IP
addresses, too, which makes the output more consistent.
> It would be nice for lr-route-list to warn about an invalid route
> prefix. Just ignoring them is likely to confuse users.
Good suggestion.
This was a pretty invasive change, so I've appended an incremental for a sanity
check. Let me know if you'd prefer me to just send it as a v2.
--Justin
-=-=-=-=-=-=-=-
diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index ba6a1ed..ab166b7 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -331,25 +331,45 @@
</dd>
</dl>
- <h1>Logical Route Commands</h1>
+ <h1>Logical Router Static Route Commands</h1>
<dl>
- <dt><code>lr-route-add</code> <var>router</var> <var>prefix</var> <var>ne
+ <dt>[<code>--may-exist</code>] <code>lr-route-add</code> <var>router</var
<dd>
- Adds the specified route to <var>router</var>. <var>prefix</var>
- describes the IP prefix for this route. <var>nexthop</var>
- specifies the gateway to use for this route. If <var>port</var>
- is specified, packets that match this route will be sent out
- that port.
+ <p>
+ Adds the specified route to <var>router</var>.
+ <var>prefix</var> describes an IPv4 or IPv6 prefix for this
+ route, such as <code>192.168.100.0/24</code>.
+ <var>nexthop</var> specifies the gateway to use for this
+ route, which should be the IP address of one of
+ <var>router</var> logical router ports or the IP address of a
+ logical port. If <var>port</var> is specified, packets that
+ match this route will be sent out that port. When
+ <var>port</var> is omitted, OVN infers the output port based
+ on <var>nexthop</var>.
+ </p>
+
+ <p>
+ It is an error if a route with <var>prefix</var> already exists,
+ unless <code>--may-exist</code> is specified.
+ </p>
</dd>
- <dt><code>lr-route-del</code> <var>router</var> [<var>prefix</var>]</dt>
+ <dt>[<code>--if-exists</code>] <code>lr-route-del</code> <var>router</var
<dd>
- 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.
+ <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.
+ </p>
+
+ <p>
+ It is an error if <code>prefix</code> is specified and there
+ is no matching route entry, unless <code>--if-exists</code> is
+ specified.
+ </p>
</dd>
<dt><code>lr-route-list</code> <var>router</var></dt>
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index eeed70f..8656e10 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -1275,39 +1275,123 @@ nbctl_lr_list(struct ctl_context *ctx)
smap_destroy(&lrs);
free(nodes);
}
+
+/* The caller must free the returned string. */
+static char *
+normalize_ipv4_prefix(ovs_be32 ipv4, unsigned int plen)
+{
+ ovs_be32 network = ipv4 & be32_prefix_mask(plen);
+ if (plen == 32) {
+ return xasprintf(IP_FMT, IP_ARGS(network));
+ } else {
+ return xasprintf(IP_FMT"/%d", IP_ARGS(network), plen);
+ }
+}
+
+/* The caller must free the returned string. */
+static char *
+normalize_ipv6_prefix(struct in6_addr ipv6, unsigned int plen)
+{
+ char network_s[INET6_ADDRSTRLEN];
+
+ struct in6_addr mask = ipv6_create_mask(plen);
+ struct in6_addr network = ipv6_addr_bitand(&ipv6, &mask);
+
+ inet_ntop(AF_INET6, &network, network_s, INET6_ADDRSTRLEN);
+ if (plen == 128) {
+ return xasprintf("%s", network_s);
+ } else {
+ return xasprintf("%s/%d", network_s, plen);
+ }
+}
+
+/* The caller must free the returned string. */
+static char *
+normalize_prefix_str(const char *orig_prefix)
+{
+ unsigned int plen;
+ ovs_be32 ipv4;
+ char *error;
+
+ error = ip_parse_cidr(orig_prefix, &ipv4, &plen);
+ if (!error) {
+ return normalize_ipv4_prefix(ipv4, plen);
+ } else {
+ struct in6_addr ipv6;
+ free(error);
+
+ error = ipv6_parse_cidr(orig_prefix, &ipv6, &plen);
+ if (error) {
+ free(error);
+ return NULL;
+ }
+ return normalize_ipv6_prefix(ipv6, plen);
+ }
+}
^L
static void
nbctl_lr_route_add(struct ctl_context *ctx)
{
const struct nbrec_logical_router *lr;
lr = lr_by_name_or_uuid(ctx, ctx->argv[1], true);
- unsigned int plen;
- ovs_be32 ipv4;
- char *error;
+ char *prefix, *next_hop;
- error = ip_parse_cidr(ctx->argv[2], &ipv4, &plen);
- if (!error) {
- if (!ip_parse(ctx->argv[3], &ipv4)) {
+ prefix = normalize_prefix_str(ctx->argv[2]);
+ if (!prefix) {
+ ctl_fatal("bad prefix argument: %s", ctx->argv[2]);
+ }
+
+ next_hop = normalize_prefix_str(ctx->argv[3]);
+ if (!next_hop) {
+ ctl_fatal("bad next hop argument: %s", ctx->argv[3]);
+ }
+
+ if (strchr(prefix, '.')) {
+ ovs_be32 hop_ipv4;
+ if (!ip_parse(ctx->argv[3], &hop_ipv4)) {
ctl_fatal("bad IPv4 nexthop argument: %s", ctx->argv[3]);
}
} else {
- free(error);
+ struct in6_addr hop_ipv6;
+ if (!ipv6_parse(ctx->argv[3], &hop_ipv6)) {
+ ctl_fatal("bad IPv6 nexthop argument: %s", ctx->argv[3]);
+ }
+ }
- struct in6_addr ipv6;
- error = ipv6_parse_cidr(ctx->argv[2], &ipv6, &plen);
- if (!error) {
- if (!ipv6_parse(ctx->argv[3], &ipv6)) {
- ctl_fatal("bad IPv6 nexthop argument: %s", ctx->argv[3]);
- }
- } else {
- ctl_fatal("bad prefix argument: %s", error);
+ bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
+ for (int i = 0; i < lr->n_static_routes; i++) {
+ char *rt_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 (!may_exist) {
+ ctl_fatal("duplicate prefix: %s", prefix);
+ }
+
+ /* Update the next hop for an existing route. */
+ nbrec_logical_router_static_route_set_ip_prefix(lr->static_routes[i],
+ prefix);
+ nbrec_logical_router_static_route_set_nexthop(lr->static_routes[i],
+ next_hop);
+ free(rt_prefix);
+ free(next_hop);
+ free(prefix);
+ return;
}
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, ctx->argv[2]);
- nbrec_logical_router_static_route_set_nexthop(route, ctx->argv[3]);
+ nbrec_logical_router_static_route_set_ip_prefix(route, prefix);
+ nbrec_logical_router_static_route_set_nexthop(route, next_hop);
if (ctx->argc == 5) {
nbrec_logical_router_static_route_set_output_port(route, ctx->argv[4]);
}
@@ -1321,6 +1405,8 @@ nbctl_lr_route_add(struct ctl_context *ctx)
nbrec_logical_router_set_static_routes(lr, new_routes,
lr->n_static_routes + 1);
free(new_routes);
+ free(next_hop);
+ free(prefix);
}
static void
@@ -1331,13 +1417,23 @@ nbctl_lr_route_del(struct ctl_context *ctx)
if (ctx->argc == 2) {
/* If a prefix is not specified, delete all routes. */
- nbrec_logical_router_verify_static_routes(lr);
nbrec_logical_router_set_static_routes(lr, NULL, 0);
return;
}
+ char *prefix = normalize_prefix_str(ctx->argv[2]);
+ if (!prefix) {
+ ctl_fatal("bad prefix argument: %s", ctx->argv[2]);
+ }
+
for (int i = 0; i < lr->n_static_routes; i++) {
- if (!strcmp(lr->static_routes[i]->ip_prefix, ctx->argv[2])) {
+ char *rt_prefix = normalize_prefix_str(lr->static_routes[i]->ip_prefix)
+ if (!rt_prefix) {
+ /* Ignore existing prefix we couldn't parse. */
+ 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);
@@ -1347,9 +1443,17 @@ nbctl_lr_route_del(struct ctl_context *ctx)
nbrec_logical_router_set_static_routes(lr, new_routes,
lr->n_static_routes - 1);
free(new_routes);
+ free(rt_prefix);
+ free(prefix);
return;
}
+ free(rt_prefix);
+ }
+
+ if (!shash_find(&ctx->options, "--if-exists")) {
+ ctl_fatal("no matching prefix: %s", prefix);
}
+ free(prefix);
}
^L
static const struct nbrec_logical_router_port *
@@ -1624,6 +1728,22 @@ ipv6_route_cmp(const void *route1_, const void *route2_)
}
static void
+print_route(const struct nbrec_logical_router_static_route *route, struct ds *s
+{
+
+ char *prefix = normalize_prefix_str(route->ip_prefix);
+ char *next_hop = normalize_prefix_str(route->nexthop);
+ ds_put_format(s, "%25s %25s", prefix, next_hop);
+ free(prefix);
+ free(next_hop);
+
+ if (route->output_port) {
+ ds_put_format(s, " %s", route->output_port);
+ }
+ ds_put_char(s, '\n');
+}
+
+static void
nbctl_lr_route_list(struct ctl_context *ctx)
{
const struct nbrec_logical_router *lr;
@@ -1661,6 +1781,9 @@ nbctl_lr_route_list(struct ctl_context *ctx)
n_ipv6_routes++;
} else {
/* Invalid prefix. */
+ VLOG_WARN("router "UUID_FMT" (%s) has invalid prefix: %s",
+ UUID_ARGS(&lr->header_.uuid), lr->name,
+ route->ip_prefix);
free(error);
continue;
}
@@ -1674,14 +1797,7 @@ nbctl_lr_route_list(struct ctl_context *ctx)
ds_put_cstr(&ctx->output, "IPv4 Routes\n");
}
for (int i = 0; i < n_ipv4_routes; i++) {
- const struct nbrec_logical_router_static_route *route
- = ipv4_routes[i].route;
- ds_put_format(&ctx->output, "%25s %25s", route->ip_prefix,
- route->nexthop);
- if (route->output_port) {
- ds_put_format(&ctx->output, " %s", route->output_port);
- }
- ds_put_char(&ctx->output, '\n');
+ print_route(ipv4_routes[i].route, &ctx->output);
}
if (n_ipv6_routes) {
@@ -1689,14 +1805,7 @@ nbctl_lr_route_list(struct ctl_context *ctx)
n_ipv4_routes ? "\n" : "");
}
for (int i = 0; i < n_ipv6_routes; i++) {
- const struct nbrec_logical_router_static_route *route
- = ipv6_routes[i].route;
- ds_put_format(&ctx->output, "%25s %25s", route->ip_prefix,
- route->nexthop);
- if (route->output_port) {
- ds_put_format(&ctx->output, " %s", route->output_port);
- }
- ds_put_char(&ctx->output, '\n');
+ print_route(ipv6_routes[i].route, &ctx->output);
}
free(ipv4_routes);
@@ -2000,9 +2109,9 @@ static const struct ctl_command_syntax nbctl_commands[] =
/* logical router route commands. */
{ "lr-route-add", 3, 4, "ROUTER PREFIX NEXTHOP [PORT]", NULL,
- nbctl_lr_route_add, NULL, "", RW },
+ nbctl_lr_route_add, NULL, "--may-exist", RW },
{ "lr-route-del", 1, 2, "ROUTER [PREFIX]", NULL, nbctl_lr_route_del,
- NULL, "", RW },
+ NULL, "--if-exists", RW },
{ "lr-route-list", 1, 1, "ROUTER", NULL, nbctl_lr_route_list, NULL,
"", RO },
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 73792b3..74855b0 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -361,21 +361,33 @@ AT_CHECK([ovn-nbctl lr-add lr0])
dnl Check IPv4 routes
AT_CHECK([ovn-nbctl lr-route-add lr0 0.0.0.0/0 192.168.0.1])
-AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.1.1/24 11.0.1.1 lp0])
-AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.1/24 11.0.0.1])
+AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.1.0/24 11.0.1.1 lp0])
+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
+])
+AT_CHECK([ovn-nbctl --may-exist lr-route-add lr0 10.0.0.111/24 11.0.0.1])
AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
IPv4 Routes
- 10.0.0.1/24 11.0.0.1
- 10.0.1.1/24 11.0.1.1 lp0
+ 10.0.0.0/24 11.0.0.1
+ 10.0.1.0/24 11.0.1.1 lp0
0.0.0.0/0 192.168.0.1
])
+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
+])
+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-list lr0], [0], [dnl
IPv4 Routes
- 10.0.0.1/24 11.0.0.1
+ 10.0.0.0/24 11.0.0.1
0.0.0.0/0 192.168.0.1
])
@@ -390,17 +402,17 @@ AT_CHECK([ovn-nbctl lr-route-add lr0 2001:0db8:1::/64 2001
AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
IPv6 Routes
- 2001:0db8:0::/64 2001:0db8:0:f102::1 lp0
- 2001:0db8:1::/64 2001:0db8:0:f103::1
- ::/0 2001:0db8:0:f101::1
+ 2001:db8::/64 2001:db8:0:f102::1 lp0
+ 2001:db8:1::/64 2001:db8:0:f103::1
+ ::/0 2001:db8:0:f101::1
])
AT_CHECK([ovn-nbctl lr-route-del lr0 2001:0db8:0::/64])
AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
IPv6 Routes
- 2001:0db8:1::/64 2001:0db8:0:f103::1
- ::/0 2001:0db8:0:f101::1
+ 2001:db8:1::/64 2001:db8:0:f103::1
+ ::/0 2001:db8:0:f101::1
])
AT_CHECK([ovn-nbctl lr-route-del lr0])
@@ -417,14 +429,14 @@ AT_CHECK([ovn-nbctl lr-route-add lr0 2001:0db8:1::/64 2001
AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
IPv4 Routes
- 10.0.0.1/24 11.0.0.1
- 10.0.1.1/24 11.0.1.1 lp0
+ 10.0.0.0/24 11.0.0.1
+ 10.0.1.0/24 11.0.1.1 lp0
0.0.0.0/0 192.168.0.1
IPv6 Routes
- 2001:0db8:0::/64 2001:0db8:0:f102::1 lp0
- 2001:0db8:1::/64 2001:0db8:0:f103::1
- ::/0 2001:0db8:0:f101::1
+ 2001:db8::/64 2001:db8:0:f102::1 lp0
+ 2001:db8:1::/64 2001:db8:0:f103::1
+ ::/0 2001:db8:0:f101::1
])
OVN_NBCTL_TEST_STOP
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
