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