Re: [ovs-dev] [PATCH ovn v2] Static Routes: Add ability to specify "discard" nexthop
Hi Ben,
Thank you for the detailed explanation. That was helpful. I have made the
changes on v3.
Thank You,
Karthik C
From: Ben Pfaff
Date: Thursday, March 25, 2021 at 8:05 PM
To: svc.eng.git-mail
Cc: ovs-dev@openvswitch.org , Karthik Chandrashekar
Subject: Re: [ovs-dev] [PATCH ovn v2] Static Routes: Add ability to specify
"discard" nexthop
On Thu, Mar 25, 2021 at 11:40:59PM +, svc.eng.git-m...@nutanix.com wrote:
> diff --git a/northd/lrouter.dl b/northd/lrouter.dl
> index 1a7cb2d23..1d8722282 100644
> --- a/northd/lrouter.dl
> +++ b/northd/lrouter.dl
> @@ -770,6 +770,36 @@ RouterStaticRoute(router, key, dsts) :-
> },
> var dsts = set_singleton(RouteDst{nexthop, src_ip, port,
> ecmp_symmetric_reply}).
>
> +/* compute route-route pairs for nexthop = "discard" routes */
> +function is_discard_route(nexthop: string): bool {
> +match (nexthop) {
> +"discard" -> true,
> +_ -> false
> +}
> +}
> +relation (lrsr: nb::Logical_Router_Static_Route,
> + key: route_key,
> + nexthop: string)
> +(.lrsr= lrsr,
> + .key = RouteKey{policy, ip_prefix, plen},
> + .nexthop = lrsr.nexthop) :-
> +lrsr in nb::Logical_Router_Static_Route(),
> +var policy = route_policy_from_string(lrsr.policy),
> +Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix),
> +is_discard_route(lrsr.nexthop).
> +
> +relation RouterDiscardRoute_(
> +router : Ref,
> +key : route_key,
> +nexthop : string)
> +
> +RouterDiscardRoute_(.router = router,
> +.key = route.key,
> +.nexthop = route.nexthop) :-
> +router in (.lr = nb::Logical_Router{.static_routes = routes}),
> +var route_id = FlatMap(routes),
> +route in (.lrsr = nb::Logical_Router_Static_Route{._uuid =
> route_id}).
Thanks so much for writing the ddlog support for this! I have a few
comments.
I first noticed that this is more complicated than necessary:
function is_discard_route(nexthop: string): bool {
match (nexthop) {
"discard" -> true,
_ -> false
}
}
It can be written as simply:
function is_discard_route(nexthop: string): bool {
nexthop == "discard"
}
Then, I noticed that is_discard_route() is only used in one place, so
that the last line in this:
(.lrsr= lrsr,
.key = RouteKey{policy, ip_prefix, plen},
.nexthop = lrsr.nexthop) :-
lrsr in nb::Logical_Router_Static_Route(),
var policy = route_policy_from_string(lrsr.policy),
Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix),
is_discard_route(lrsr.nexthop).
can be written as:
lrsr.nexthop == "discard".
and be a little clearer.
However, there's an optimization possibility here. ddlog is pretty
literal-minded and does things in order. Most routes won't be
"discard", so by putting that test first we can usually bypass the other
work. Now we have:
(.lrsr= lrsr,
.key = RouteKey{policy, ip_prefix, plen},
.nexthop = lrsr.nexthop) :-
lrsr in nb::Logical_Router_Static_Route(),
lrsr.nexthop == "discard".
var policy = route_policy_from_string(lrsr.policy),
Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix).
We can make things even better by noticing that we can put the "discard"
test right into the specification of Logical_Router_Static_Route. That
makes things even faster since ddlog can index it:
(.lrsr= lrsr,
.key = RouteKey{policy, ip_prefix, plen},
.nexthop = lrsr.nexthop) :-
lrsr in nb::Logical_Router_Static_Route(.nexthop == "discard"),
var policy = route_policy_from_string(lrsr.policy),
Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix).
At this point I noticed that the nexthop column wasn't good for anything
in DiscardRoute or RouterDiscardRoute_, because it's always the constant
string "discard". So we can get rid of it:
/* compute route-route pairs for nexthop = "discard" routes */
relation (lrsr: nb::Logical_Router_Static_Route,
key: route_key)
(.lrsr= lrsr,
.key = RouteKey{policy, ip_prefix, plen}) :-
lrsr in nb::Logical_Router_Static_Route(.nexthop = "discard"),
var policy = route_policy_from_string(lrsr.policy),
Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix).
relation RouterDiscardRoute_(
router : Ref,
Re: [ovs-dev] [PATCH ovn v2] Static Routes: Add ability to specify "discard" nexthop
On Thu, Mar 25, 2021 at 11:40:59PM +, svc.eng.git-m...@nutanix.com wrote:
> diff --git a/northd/lrouter.dl b/northd/lrouter.dl
> index 1a7cb2d23..1d8722282 100644
> --- a/northd/lrouter.dl
> +++ b/northd/lrouter.dl
> @@ -770,6 +770,36 @@ RouterStaticRoute(router, key, dsts) :-
> },
> var dsts = set_singleton(RouteDst{nexthop, src_ip, port,
> ecmp_symmetric_reply}).
>
> +/* compute route-route pairs for nexthop = "discard" routes */
> +function is_discard_route(nexthop: string): bool {
> +match (nexthop) {
> +"discard" -> true,
> +_ -> false
> +}
> +}
> +relation (lrsr: nb::Logical_Router_Static_Route,
> + key: route_key,
> + nexthop: string)
> +(.lrsr= lrsr,
> + .key = RouteKey{policy, ip_prefix, plen},
> + .nexthop = lrsr.nexthop) :-
> +lrsr in nb::Logical_Router_Static_Route(),
> +var policy = route_policy_from_string(lrsr.policy),
> +Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix),
> +is_discard_route(lrsr.nexthop).
> +
> +relation RouterDiscardRoute_(
> +router : Ref,
> +key : route_key,
> +nexthop : string)
> +
> +RouterDiscardRoute_(.router = router,
> +.key = route.key,
> +.nexthop = route.nexthop) :-
> +router in (.lr = nb::Logical_Router{.static_routes = routes}),
> +var route_id = FlatMap(routes),
> +route in (.lrsr = nb::Logical_Router_Static_Route{._uuid =
> route_id}).
Thanks so much for writing the ddlog support for this! I have a few
comments.
I first noticed that this is more complicated than necessary:
function is_discard_route(nexthop: string): bool {
match (nexthop) {
"discard" -> true,
_ -> false
}
}
It can be written as simply:
function is_discard_route(nexthop: string): bool {
nexthop == "discard"
}
Then, I noticed that is_discard_route() is only used in one place, so
that the last line in this:
(.lrsr= lrsr,
.key = RouteKey{policy, ip_prefix, plen},
.nexthop = lrsr.nexthop) :-
lrsr in nb::Logical_Router_Static_Route(),
var policy = route_policy_from_string(lrsr.policy),
Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix),
is_discard_route(lrsr.nexthop).
can be written as:
lrsr.nexthop == "discard".
and be a little clearer.
However, there's an optimization possibility here. ddlog is pretty
literal-minded and does things in order. Most routes won't be
"discard", so by putting that test first we can usually bypass the other
work. Now we have:
(.lrsr= lrsr,
.key = RouteKey{policy, ip_prefix, plen},
.nexthop = lrsr.nexthop) :-
lrsr in nb::Logical_Router_Static_Route(),
lrsr.nexthop == "discard".
var policy = route_policy_from_string(lrsr.policy),
Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix).
We can make things even better by noticing that we can put the "discard"
test right into the specification of Logical_Router_Static_Route. That
makes things even faster since ddlog can index it:
(.lrsr= lrsr,
.key = RouteKey{policy, ip_prefix, plen},
.nexthop = lrsr.nexthop) :-
lrsr in nb::Logical_Router_Static_Route(.nexthop == "discard"),
var policy = route_policy_from_string(lrsr.policy),
Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix).
At this point I noticed that the nexthop column wasn't good for anything
in DiscardRoute or RouterDiscardRoute_, because it's always the constant
string "discard". So we can get rid of it:
/* compute route-route pairs for nexthop = "discard" routes */
relation (lrsr: nb::Logical_Router_Static_Route,
key: route_key)
(.lrsr= lrsr,
.key = RouteKey{policy, ip_prefix, plen}) :-
lrsr in nb::Logical_Router_Static_Route(.nexthop = "discard"),
var policy = route_policy_from_string(lrsr.policy),
Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix).
relation RouterDiscardRoute_(
router : Ref,
key : route_key)
RouterDiscardRoute_(.router = router,
.key = route.key) :-
router in (.lr = nb::Logical_Router{.static_routes = routes}),
var route_id = FlatMap(routes),
route in (.lrsr = nb::Logical_Router_Static_Route{._uuid =
route_id}).
Warning[message] :-
RouterStaticRoute_(.router = router, .key = key, .nexthop = nexthop),
not RouterStaticRoute(.router = router, .key = key),
var message = "No path for ${key.policy} static route
${key.ip_prefix}/${key.plen} with next hop ${nexthop}".
The overall
[ovs-dev] [PATCH ovn v2] Static Routes: Add ability to specify "discard" nexthop
From: Karthik Chandrashekar
Physical switches have the ability to specify "discard" or sometimes
"NULL interface" as a nexthop for routes. This can be used to prevent
routing loops in the network. Add a similar configuration for ovn
where nexthop accepts the string "discard". When the nexthop is discard
the action in the routing table will be to drop the packets.
Signed-off-by: Karthik Chandrashekar
v1 -> v2
* Add ddlog support
* Don't allow nexthop = "discard" when ECMP and BFD is set
---
northd/lrouter.dl | 30 +
northd/ovn-northd.c | 126 +-
northd/ovn_northd.dl | 10 +++
ovn-nb.xml| 4 +-
tests/ovn-nbctl.at| 25
tests/ovn.at | 94
utilities/ovn-nbctl.8.xml | 3 +-
utilities/ovn-nbctl.c | 67 +++-
8 files changed, 287 insertions(+), 72 deletions(-)
diff --git a/northd/lrouter.dl b/northd/lrouter.dl
index 1a7cb2d23..1d8722282 100644
--- a/northd/lrouter.dl
+++ b/northd/lrouter.dl
@@ -770,6 +770,36 @@ RouterStaticRoute(router, key, dsts) :-
},
var dsts = set_singleton(RouteDst{nexthop, src_ip, port,
ecmp_symmetric_reply}).
+/* compute route-route pairs for nexthop = "discard" routes */
+function is_discard_route(nexthop: string): bool {
+match (nexthop) {
+"discard" -> true,
+_ -> false
+}
+}
+relation (lrsr: nb::Logical_Router_Static_Route,
+ key: route_key,
+ nexthop: string)
+(.lrsr= lrsr,
+ .key = RouteKey{policy, ip_prefix, plen},
+ .nexthop = lrsr.nexthop) :-
+lrsr in nb::Logical_Router_Static_Route(),
+var policy = route_policy_from_string(lrsr.policy),
+Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix),
+is_discard_route(lrsr.nexthop).
+
+relation RouterDiscardRoute_(
+router : Ref,
+key : route_key,
+nexthop : string)
+
+RouterDiscardRoute_(.router = router,
+.key = route.key,
+.nexthop = route.nexthop) :-
+router in (.lr = nb::Logical_Router{.static_routes = routes}),
+var route_id = FlatMap(routes),
+route in (.lrsr = nb::Logical_Router_Static_Route{._uuid =
route_id}).
+
Warning[message] :-
RouterStaticRoute_(.router = router, .key = key, .nexthop = nexthop),
not RouterStaticRoute(.router = router, .key = key),
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 57df62b92..a6357f02c 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -7938,6 +7938,7 @@ struct parsed_route {
uint32_t hash;
const struct nbrec_logical_router_static_route *route;
bool ecmp_symmetric_reply;
+bool is_discard_route;
};
static uint32_t
@@ -7957,20 +7958,23 @@ parsed_routes_add(struct ovs_list *routes,
/* Verify that the next hop is an IP address with an all-ones mask. */
struct in6_addr nexthop;
unsigned int plen;
-if (!ip46_parse_cidr(route->nexthop, , )) {
-static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-VLOG_WARN_RL(, "bad 'nexthop' %s in static route"
- UUID_FMT, route->nexthop,
- UUID_ARGS(>header_.uuid));
-return NULL;
-}
-if ((IN6_IS_ADDR_V4MAPPED() && plen != 32) ||
-(!IN6_IS_ADDR_V4MAPPED() && plen != 128)) {
-static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-VLOG_WARN_RL(, "bad next hop mask %s in static route"
- UUID_FMT, route->nexthop,
- UUID_ARGS(>header_.uuid));
-return NULL;
+bool is_discard_route = !strcmp(route->nexthop, "discard");
+if (!is_discard_route) {
+if (!ip46_parse_cidr(route->nexthop, , )) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+VLOG_WARN_RL(, "bad 'nexthop' %s in static route"
+ UUID_FMT, route->nexthop,
+ UUID_ARGS(>header_.uuid));
+return NULL;
+}
+if ((IN6_IS_ADDR_V4MAPPED() && plen != 32) ||
+(!IN6_IS_ADDR_V4MAPPED() && plen != 128)) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+VLOG_WARN_RL(, "bad next hop mask %s in static route"
+ UUID_FMT, route->nexthop,
+ UUID_ARGS(>header_.uuid));
+return NULL;
+}
}
/* Parse ip_prefix */
@@ -7984,13 +7988,15 @@ parsed_routes_add(struct ovs_list *routes,
}
/* Verify that ip_prefix and nexthop have same address familiy. */
-if (IN6_IS_ADDR_V4MAPPED() != IN6_IS_ADDR_V4MAPPED()) {
-static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-VLOG_WARN_RL(, "Address family doesn't match between 'ip_prefix' %s"
- " and 'nexthop' %s in static
