diff mbox

[ovs-dev,5/8] ovn-nbctl: Add static route commands.

Message ID C93F5091-EC5D-4F08-8CA1-8D4F4565D40B@ovn.org
State Accepted
Headers show

Commit Message

Justin Pettit June 10, 2016, 11:09 p.m. UTC
> On Jun 9, 2016, at 3:17 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Thu, Jun 09, 2016 at 12:12:39AM -0700, Justin Pettit wrote:
>> Signed-off-by: Justin Pettit <jpettit@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


-=-=-=-=-=-=-=-

Comments

Ben Pfaff June 11, 2016, 2:03 a.m. UTC | #1
On Fri, Jun 10, 2016 at 04:09:31PM -0700, Justin Pettit wrote:
> 
> > On Jun 9, 2016, at 3:17 PM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > On Thu, Jun 09, 2016 at 12:12:39AM -0700, Justin Pettit wrote:
> > 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.

Thanks.

It's a little hard to tell just from looking at the incremental, but I
think that lr-route-add does not call "verify" on the static_routes
column.  It should do that now, because its behavior depends on this
column's content.  To be really and truly consistent and isolated, it
should also verify all the columns it looks at in the
Logical_Router_Static_Routes table (or we could make them immutable;
that is one reason that many "name" columns in Open_vSwitch database
tables are immutable).

Acked-by: Ben Pfaff <blp@ovn.org>
Justin Pettit June 11, 2016, 6:22 p.m. UTC | #2
> On Jun 10, 2016, at 7:03 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Fri, Jun 10, 2016 at 04:09:31PM -0700, Justin Pettit wrote:
>> 
>>> On Jun 9, 2016, at 3:17 PM, Ben Pfaff <blp@ovn.org> wrote:
>>> 
>>> On Thu, Jun 09, 2016 at 12:12:39AM -0700, Justin Pettit wrote:
>>> 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.
> 
> Thanks.
> 
> It's a little hard to tell just from looking at the incremental, but I
> think that lr-route-add does not call "verify" on the static_routes
> column.  It should do that now, because its behavior depends on this
> column's content.  To be really and truly consistent and isolated, it
> should also verify all the columns it looks at in the
> Logical_Router_Static_Routes table (or we could make them immutable;
> that is one reason that many "name" columns in Open_vSwitch database
> tables are immutable).

You're right.  It handled it for the addition case, but not the modify case.   I've updated it to also verify the columns with the static router table.

> Acked-by: Ben Pfaff <blp@ovn.org>

Thanks!

--Justin
diff mbox

Patch

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