diff mbox series

[ovs-dev,v2] Static Routes: Add ability to specify "discard" nexthop

Message ID 20210325234059.120141-1-svc.eng.git-mail@nutanix.com
State Superseded
Headers show
Series [ovs-dev,v2] Static Routes: Add ability to specify "discard" nexthop | expand

Commit Message

svc.eng.git-mail March 25, 2021, 11:40 p.m. UTC 
From: Karthik Chandrashekar <karthik.c@nutanix.com>

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 <karthik.c@nutanix.com>

----
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(-)

Comments

Ben Pfaff March 26, 2021, 3:05 a.m. UTC | #1 
On Thu, Mar 25, 2021 at 11:40:59PM +0000, svc.eng.git-mail@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 &DiscardRoute(lrsr: nb::Logical_Router_Static_Route,
> +                       key: route_key,
> +                       nexthop: string)
> +&DiscardRoute(.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<Router>,
> +    key         : route_key,
> +    nexthop     : string)
> +
> +RouterDiscardRoute_(.router = router,
> +                    .key = route.key,
> +                    .nexthop = route.nexthop) :-
> +    router in &Router(.lr = nb::Logical_Router{.static_routes = routes}),
> +    var route_id = FlatMap(routes),
> +    route in &DiscardRoute(.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:
    &DiscardRoute(.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:
    &DiscardRoute(.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:
    &DiscardRoute(.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 &DiscardRoute(lrsr: nb::Logical_Router_Static_Route,
                           key: route_key)
    &DiscardRoute(.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<Router>,
        key         : route_key)

    RouterDiscardRoute_(.router = router,
                        .key = route.key) :-
        router in &Router(.lr = nb::Logical_Router{.static_routes = routes}),
        var route_id = FlatMap(routes),
        route in &DiscardRoute(.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 incremental change is the following:

diff --git a/northd/lrouter.dl b/northd/lrouter.dl
index 1d87222820eb..8c12b2303bda 100644
--- a/northd/lrouter.dl
+++ b/northd/lrouter.dl
@@ -771,31 +771,20 @@ 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 &DiscardRoute(lrsr: nb::Logical_Router_Static_Route,
-                       key: route_key,
-                       nexthop: string)
+                       key: route_key)
 &DiscardRoute(.lrsr        = lrsr,
-              .key         = RouteKey{policy, ip_prefix, plen},
-              .nexthop     = lrsr.nexthop) :-
-    lrsr in nb::Logical_Router_Static_Route(),
+              .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),
-    is_discard_route(lrsr.nexthop).
+    Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix).
 
 relation RouterDiscardRoute_(
     router      : Ref<Router>,
-    key         : route_key,
-    nexthop     : string)
+    key         : route_key)
 
 RouterDiscardRoute_(.router = router,
-                    .key = route.key,
-                    .nexthop = route.nexthop) :-
+                    .key = route.key) :-
     router in &Router(.lr = nb::Logical_Router{.static_routes = routes}),
     var route_id = FlatMap(routes),
     route in &DiscardRoute(.lrsr = nb::Logical_Router_Static_Route{._uuid = route_id}).
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 1d25c1e5c58e..76067cd4201d 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -6457,7 +6457,7 @@ Flow(.logical_datapath = router.lr._uuid,
      .__match          = ip_match,
      .actions          = "drop;",
      .external_ids     = map_empty()) :-
-    r in RouterDiscardRoute_(.router = router, .key = key, .nexthop = nexthop),
+    r in RouterDiscardRoute_(.router = router, .key = key),
     (var ip_match, var priority) = build_route_match(r.key).
 
 /* Logical router ingress table IP_ROUTING & IP_ROUTING_ECMP: IP Routing.

Since discard routes are a special case, I also spent some time fussing
around trying to figure out whether it was better to alternatively come
up with a new type for next hops, something like this:
    diff --git a/northd/lrouter.dl b/northd/lrouter.dl
    index 1d87222820eb..6963a2346bcc 100644
    --- a/northd/lrouter.dl
    +++ b/northd/lrouter.dl
    @@ -628,9 +628,11 @@ StaticRouteDown(lrsr_uuid) :-
             _ -> false
         }.

    +typedef next_hop = NextHop{ip: v46_ip} | Discard
    +
     relation &StaticRoute(lrsr: nb::Logical_Router_Static_Route,
                           key: route_key,
    -                      nexthop: v46_ip,
    +                      nexthop: next_hop,
                           output_port: Option<string>,
                           ecmp_symmetric_reply: bool)
 But this introduced other special cases, and I concluded that it wasn't
 necessarily better in the end.
svc.eng.git-mail April 2, 2021, 9:15 p.m. UTC | #2 
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 <blp@ovn.org>
Date: Thursday, March 25, 2021 at 8:05 PM
To: svc.eng.git-mail <svc.eng.git-mail@nutanix.com>
Cc: ovs-dev@openvswitch.org <ovs-dev@openvswitch.org>, Karthik Chandrashekar <karthik.c@nutanix.com>
Subject: Re: [ovs-dev] [PATCH ovn v2] Static Routes: Add ability to specify "discard" nexthop
On Thu, Mar 25, 2021 at 11:40:59PM +0000, svc.eng.git-mail@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 &DiscardRoute(lrsr: nb::Logical_Router_Static_Route,
> +                       key: route_key,
> +                       nexthop: string)
> +&DiscardRoute(.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<Router>,
> +    key         : route_key,
> +    nexthop     : string)
> +
> +RouterDiscardRoute_(.router = router,
> +                    .key = route.key,
> +                    .nexthop = route.nexthop) :-
> +    router in &Router(.lr = nb::Logical_Router{.static_routes = routes}),
> +    var route_id = FlatMap(routes),
> +    route in &DiscardRoute(.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:
    &DiscardRoute(.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:
    &DiscardRoute(.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:
    &DiscardRoute(.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 &DiscardRoute(lrsr: nb::Logical_Router_Static_Route,
                           key: route_key)
    &DiscardRoute(.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<Router>,
        key         : route_key)

    RouterDiscardRoute_(.router = router,
                        .key = route.key) :-
        router in &Router(.lr = nb::Logical_Router{.static_routes = routes}),
        var route_id = FlatMap(routes),
        route in &DiscardRoute(.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 incremental change is the following:

diff --git a/northd/lrouter.dl b/northd/lrouter.dl
index 1d87222820eb..8c12b2303bda 100644
--- a/northd/lrouter.dl
+++ b/northd/lrouter.dl
@@ -771,31 +771,20 @@ 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 &DiscardRoute(lrsr: nb::Logical_Router_Static_Route,
-                       key: route_key,
-                       nexthop: string)
+                       key: route_key)
 &DiscardRoute(.lrsr        = lrsr,
-              .key         = RouteKey{policy, ip_prefix, plen},
-              .nexthop     = lrsr.nexthop) :-
-    lrsr in nb::Logical_Router_Static_Route(),
+              .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),
-    is_discard_route(lrsr.nexthop).
+    Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix).

 relation RouterDiscardRoute_(
     router      : Ref<Router>,
-    key         : route_key,
-    nexthop     : string)
+    key         : route_key)

 RouterDiscardRoute_(.router = router,
-                    .key = route.key,
-                    .nexthop = route.nexthop) :-
+                    .key = route.key) :-
     router in &Router(.lr = nb::Logical_Router{.static_routes = routes}),
     var route_id = FlatMap(routes),
     route in &DiscardRoute(.lrsr = nb::Logical_Router_Static_Route{._uuid = route_id}).
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 1d25c1e5c58e..76067cd4201d 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -6457,7 +6457,7 @@ Flow(.logical_datapath = router.lr._uuid,
      .__match          = ip_match,
      .actions          = "drop;",
      .external_ids     = map_empty()) :-
-    r in RouterDiscardRoute_(.router = router, .key = key, .nexthop = nexthop),
+    r in RouterDiscardRoute_(.router = router, .key = key),
     (var ip_match, var priority) = build_route_match(r.key).

 /* Logical router ingress table IP_ROUTING & IP_ROUTING_ECMP: IP Routing.

Since discard routes are a special case, I also spent some time fussing
around trying to figure out whether it was better to alternatively come
up with a new type for next hops, something like this:
    diff --git a/northd/lrouter.dl b/northd/lrouter.dl
    index 1d87222820eb..6963a2346bcc 100644
    --- a/northd/lrouter.dl
    +++ b/northd/lrouter.dl
    @@ -628,9 +628,11 @@ StaticRouteDown(lrsr_uuid) :-
             _ -> false
         }.

    +typedef next_hop = NextHop{ip: v46_ip} | Discard
    +
     relation &StaticRoute(lrsr: nb::Logical_Router_Static_Route,
                           key: route_key,
    -                      nexthop: v46_ip,
    +                      nexthop: next_hop,
                           output_port: Option<string>,
                           ecmp_symmetric_reply: bool)
 But this introduced other special cases, and I concluded that it wasn't
 necessarily better in the end.
diff mbox series

Patch

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 &DiscardRoute(lrsr: nb::Logical_Router_Static_Route,
+                       key: route_key,
+                       nexthop: string)
+&DiscardRoute(.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<Router>,
+    key         : route_key,
+    nexthop     : string)
+
+RouterDiscardRoute_(.router = router,
+                    .key = route.key,
+                    .nexthop = route.nexthop) :-
+    router in &Router(.lr = nb::Logical_Router{.static_routes = routes}),
+    var route_id = FlatMap(routes),
+    route in &DiscardRoute(.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, &nexthop, &plen)) {
-        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-        VLOG_WARN_RL(&rl, "bad 'nexthop' %s in static route"
-                     UUID_FMT, route->nexthop,
-                     UUID_ARGS(&route->header_.uuid));
-        return NULL;
-    }
-    if ((IN6_IS_ADDR_V4MAPPED(&nexthop) && plen != 32) ||
-        (!IN6_IS_ADDR_V4MAPPED(&nexthop) && plen != 128)) {
-        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-        VLOG_WARN_RL(&rl, "bad next hop mask %s in static route"
-                     UUID_FMT, route->nexthop,
-                     UUID_ARGS(&route->header_.uuid));
-        return NULL;
+    bool is_discard_route = !strcmp(route->nexthop, "discard");
+    if (!is_discard_route) {
+        if (!ip46_parse_cidr(route->nexthop, &nexthop, &plen)) {
+            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+            VLOG_WARN_RL(&rl, "bad 'nexthop' %s in static route"
+                         UUID_FMT, route->nexthop,
+                         UUID_ARGS(&route->header_.uuid));
+            return NULL;
+        }
+        if ((IN6_IS_ADDR_V4MAPPED(&nexthop) && plen != 32) ||
+            (!IN6_IS_ADDR_V4MAPPED(&nexthop) && plen != 128)) {
+            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+            VLOG_WARN_RL(&rl, "bad next hop mask %s in static route"
+                         UUID_FMT, route->nexthop,
+                         UUID_ARGS(&route->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(&prefix) != IN6_IS_ADDR_V4MAPPED(&nexthop)) {
-        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-        VLOG_WARN_RL(&rl, "Address family doesn't match between 'ip_prefix' %s"
-                     " and 'nexthop' %s in static route"UUID_FMT,
-                     route->ip_prefix, route->nexthop,
-                     UUID_ARGS(&route->header_.uuid));
-        return NULL;
+    if (!is_discard_route) {
+        if (IN6_IS_ADDR_V4MAPPED(&prefix) != IN6_IS_ADDR_V4MAPPED(&nexthop)) {
+            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+            VLOG_WARN_RL(&rl, "Address family doesn't match between 'ip_prefix'"
+                         " %s and 'nexthop' %s in static route"UUID_FMT,
+                         route->ip_prefix, route->nexthop,
+                         UUID_ARGS(&route->header_.uuid));
+            return NULL;
+        }
     }
 
     const struct nbrec_bfd *nb_bt = route->bfd;
@@ -8021,6 +8027,7 @@  parsed_routes_add(struct ovs_list *routes,
     pr->route = route;
     pr->ecmp_symmetric_reply = smap_get_bool(&route->options,
                                              "ecmp_symmetric_reply", false);
+    pr->is_discard_route = is_discard_route;
     ovs_list_insert(routes, &pr->list_node);
     return pr;
 }
@@ -8433,10 +8440,11 @@  build_ecmp_route_flow(struct hmap *lflows, struct ovn_datapath *od,
 }
 
 static void
-add_route(struct hmap *lflows, const struct ovn_port *op,
-          const char *lrp_addr_s, const char *network_s, int plen,
-          const char *gateway, bool is_src_route,
-          const struct ovsdb_idl_row *stage_hint)
+add_route(struct hmap *lflows, struct ovn_datapath *od,
+          const struct ovn_port *op, const char *lrp_addr_s,
+          const char *network_s, int plen, const char *gateway,
+          bool is_src_route, const struct ovsdb_idl_row *stage_hint,
+          bool is_discard_route)
 {
     bool is_ipv4 = strchr(network_s, '.') ? true : false;
     struct ds match = DS_EMPTY_INITIALIZER;
@@ -8455,30 +8463,34 @@  add_route(struct hmap *lflows, const struct ovn_port *op,
                       &match, &priority);
 
     struct ds common_actions = DS_EMPTY_INITIALIZER;
-    ds_put_format(&common_actions, REG_ECMP_GROUP_ID" = 0; %s = ",
-                  is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6);
-    if (gateway) {
-        ds_put_cstr(&common_actions, gateway);
-    } else {
-        ds_put_format(&common_actions, "ip%s.dst", is_ipv4 ? "4" : "6");
-    }
-    ds_put_format(&common_actions, "; "
-                  "%s = %s; "
-                  "eth.src = %s; "
-                  "outport = %s; "
-                  "flags.loopback = 1; "
-                  "next;",
-                  is_ipv4 ? REG_SRC_IPV4 : REG_SRC_IPV6,
-                  lrp_addr_s,
-                  op->lrp_networks.ea_s,
-                  op->json_key);
     struct ds actions = DS_EMPTY_INITIALIZER;
-    ds_put_format(&actions, "ip.ttl--; %s", ds_cstr(&common_actions));
+    if (is_discard_route) {
+        ds_put_format(&actions, "drop;");
+    } else {
+        ds_put_format(&common_actions, REG_ECMP_GROUP_ID" = 0; %s = ",
+                      is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6);
+        if (gateway) {
+            ds_put_cstr(&common_actions, gateway);
+        } else {
+            ds_put_format(&common_actions, "ip%s.dst", is_ipv4 ? "4" : "6");
+        }
+        ds_put_format(&common_actions, "; "
+                      "%s = %s; "
+                      "eth.src = %s; "
+                      "outport = %s; "
+                      "flags.loopback = 1; "
+                      "next;",
+                      is_ipv4 ? REG_SRC_IPV4 : REG_SRC_IPV6,
+                      lrp_addr_s,
+                      op->lrp_networks.ea_s,
+                      op->json_key);
+        ds_put_format(&actions, "ip.ttl--; %s", ds_cstr(&common_actions));
+    }
 
-    ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_IP_ROUTING, priority,
+    ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_IP_ROUTING, priority,
                             ds_cstr(&match), ds_cstr(&actions),
                             stage_hint);
-    if (op->has_bfd) {
+    if (op && op->has_bfd) {
         ds_put_format(&match, " && udp.dst == 3784");
         ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_IP_ROUTING,
                                 priority + 1, ds_cstr(&match),
@@ -8500,16 +8512,18 @@  build_static_route_flow(struct hmap *lflows, struct ovn_datapath *od,
     const struct nbrec_logical_router_static_route *route = route_->route;
 
     /* Find the outgoing port. */
-    if (!find_static_route_outport(od, ports, route,
-                                   IN6_IS_ADDR_V4MAPPED(&route_->prefix),
-                                   &lrp_addr_s, &out_port)) {
-        return;
+    if (!route_->is_discard_route) {
+        if (!find_static_route_outport(od, ports, route,
+                                       IN6_IS_ADDR_V4MAPPED(&route_->prefix),
+                                       &lrp_addr_s, &out_port)) {
+            return;
+        }
     }
 
     char *prefix_s = build_route_prefix_s(&route_->prefix, route_->plen);
-    add_route(lflows, out_port, lrp_addr_s, prefix_s, route_->plen,
-              route->nexthop, route_->is_src_route,
-              &route->header_);
+    add_route(lflows, route_->is_discard_route ? od : out_port->od, out_port,
+              lrp_addr_s, prefix_s, route_->plen, route->nexthop,
+              route_->is_src_route, &route->header_, route_->is_discard_route);
 
     free(prefix_s);
 }
@@ -9735,17 +9749,17 @@  build_ip_routing_flows_for_lrouter_port(
     if (op->nbrp) {
 
         for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
-            add_route(lflows, op, op->lrp_networks.ipv4_addrs[i].addr_s,
+            add_route(lflows, op->od, op, op->lrp_networks.ipv4_addrs[i].addr_s,
                       op->lrp_networks.ipv4_addrs[i].network_s,
                       op->lrp_networks.ipv4_addrs[i].plen, NULL, false,
-                      &op->nbrp->header_);
+                      &op->nbrp->header_, false);
         }
 
         for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
-            add_route(lflows, op, op->lrp_networks.ipv6_addrs[i].addr_s,
+            add_route(lflows, op->od, op, op->lrp_networks.ipv6_addrs[i].addr_s,
                       op->lrp_networks.ipv6_addrs[i].network_s,
                       op->lrp_networks.ipv6_addrs[i].plen, NULL, false,
-                      &op->nbrp->header_);
+                      &op->nbrp->header_, false);
         }
     }
 }
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 4262b83b9..1d25c1e5c 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -6450,6 +6450,16 @@  for (Route(.port        = port,
     }
 }
 
+/* Install drop routes for all the static routes with nexthop = "discard" */
+Flow(.logical_datapath = router.lr._uuid,
+     .stage            = router_stage(IN, IP_ROUTING),
+     .priority         = priority as integer,
+     .__match          = ip_match,
+     .actions          = "drop;",
+     .external_ids     = map_empty()) :-
+    r in RouterDiscardRoute_(.router = router, .key = key, .nexthop = nexthop),
+    (var ip_match, var priority) = build_route_match(r.key).
+
 /* Logical router ingress table IP_ROUTING & IP_ROUTING_ECMP: IP Routing.
  *
  * A packet that arrives at this table is an IP packet that should be
diff --git a/ovn-nb.xml b/ovn-nb.xml
index b0a4adffe..98543404e 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -2649,7 +2649,9 @@ 
     <column name="nexthop">
       <p>
         Nexthop IP address for this route.  Nexthop IP address should be the IP
-        address of a connected router port or the IP address of a logical port.
+        address of a connected router port or the IP address of a logical port
+        or can be set to <code>discard</code> for dropping packets which match
+        the given route.
       </p>
     </column>
 
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 6d91aa4c5..0392cd968 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -1467,18 +1467,38 @@  AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111/24 11.0.0.1/24], [1], [],
 AT_CHECK([ovn-nbctl lr-route-add lr0 2001:0db8:1::/64 2001:0db8:0:f103::1/64], [1], [],
   [ovn-nbctl: bad IPv6 nexthop argument: 2001:0db8:0:f103::1/64
 ])
+AT_CHECK([ovn-nbctl --ecmp lr-route-add lr0 20.0.0.0/24 discard], [1], [],
+  [ovn-nbctl: ecmp is not valid for discard routes.
+])
+AT_CHECK([ovn-nbctl --ecmp-symmetric-reply lr-route-add lr0 20.0.0.0/24 discard], [1], [],
+  [ovn-nbctl: ecmp is not valid for discard routes.
+])
+AT_CHECK([ovn-nbctl --bfd lr-route-add lr0 20.0.0.0/24 discard lp0], [1], [],
+  [ovn-nbctl: bfd dst_ip cannot be discard.
+])
+AT_CHECK([ovn-nbctl lr-route-add lr0 20.0.0.0/24 discard lp0], [1], [],
+  [ovn-nbctl: outport is not valid for discard routes.
+])
 
 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-add lr0 20.0.0.0/24 discard])
+AT_CHECK([ovn-nbctl --policy=src-ip lr-route-add lr0 20.0.0.0/24 discard])
+AT_CHECK([ovn-nbctl --ecmp --policy=src-ip lr-route-add lr0 20.0.0.0/24 11.0.0.1], [1], [],
+  [ovn-nbctl: discard nexthop for the same ECMP route exists.
+])
+
 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
+              20.0.0.0/24                   discard dst-ip
               9.16.1.0/24                  11.0.0.1 src-ip
               10.0.0.0/24                  11.0.0.2 src-ip
+              20.0.0.0/24                   discard src-ip
                 0.0.0.0/0               192.168.0.1 dst-ip
 ])
 
@@ -1487,11 +1507,16 @@  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.1.0/24                  11.0.1.1 dst-ip lp0
+              20.0.0.0/24                   discard dst-ip
               9.16.1.0/24                  11.0.0.1 src-ip
               10.0.0.0/24                  11.0.0.2 src-ip
+              20.0.0.0/24                   discard src-ip
                 0.0.0.0/0               192.168.0.1 dst-ip
 ])
 
+AT_CHECK([ovn-nbctl --policy=src-ip lr-route-del lr0 20.0.0.0/24])
+AT_CHECK([ovn-nbctl lr-route-del lr0 20.0.0.0/24 discard])
+
 dnl Delete non-existent prefix
 AT_CHECK([ovn-nbctl lr-route-del lr0 10.0.2.1/24], [1], [],
   [ovn-nbctl: no matching route: policy 'any', prefix '10.0.2.0/24', nexthop 'any', output_port 'any'.
diff --git a/tests/ovn.at b/tests/ovn.at
index 391a8bcd9..427231533 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -25216,3 +25216,97 @@  AT_CHECK([cat hv2_offlows_table72.txt | grep -v NXST], [1], [dnl
 OVN_CLEANUP([hv1], [hv2])
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn -- Static route with discard nexthop])
+ovn_start
+
+# Logical network:
+# Logical Router lr1 has Logical Switch sw1 (10.0.0.0/24) connected to it. sw1 has one
+# switch port sw1-ls1 (10.0.0.100)
+
+ovn-nbctl lr-add lr1
+
+ovn-nbctl ls-add sw1
+
+# Connect sw1 to lr1
+ovn-nbctl lrp-add lr1 lr1-sw1 00:00:01:01:02:03 10.0.0.1/24
+ovn-nbctl lsp-add sw1 sw1-lr1
+ovn-nbctl lsp-set-type sw1-lr1 router
+ovn-nbctl lsp-set-addresses sw1-lr1 router
+ovn-nbctl lsp-set-options sw1-lr1 router-port=lr1-sw1
+
+# Create logical port sw1-lp1 in sw1
+ovn-nbctl lsp-add sw1 sw1-lp1 \
+-- lsp-set-addresses sw1-lp1 "00:00:04:01:02:03 10.0.0.100"
+
+# Create one hypervisor and create OVS ports corresponding to logical ports.
+net_add n1
+
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+
+ovs-vsctl -- add-port br-int hv1-vif1 -- \
+    set interface hv1-vif1 external-ids:iface-id=sw1-lp1 \
+    options:tx_pcap=hv1/vif1-tx.pcap \
+    options:rxq_pcap=hv1/vif1-rx.pcap \
+    ofport-request=1
+
+# Install static routes to drop traffic
+ovn-nbctl lr-route-add lr1 20.0.0.0/24 discard
+
+# Allow some time for ovn-controller to catch up.
+ovn-nbctl --wait=hv sync
+
+# Check logical flows for drop rule
+AT_CHECK([ovn-sbctl dump-flows | grep lr_in_ip_routing | grep "20.0.0.0/24" | \
+    grep drop | wc -l], [0], [dnl
+1
+])
+
+# Send packet.
+packet="inport==\"sw1-lp1\" && eth.src==00:00:04:01:02:03 &&
+       eth.dst==00:00:01:01:02:03 && ip4 && ip.ttl==64 &&
+       ip4.src==10.0.0.100 && ip4.dst==20.0.0.200 &&
+       udp && udp.src==53 && udp.dst==4369"
+
+as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
+
+# Check if packet hit the drop rule
+AT_CHECK([ovs-ofctl dump-flows br-int | grep "nw_dst=20.0.0.0/24" | \
+    grep "actions=drop" | grep "n_packets=1" | wc -l], [0], [dnl
+1
+])
+
+# Remove destination match and add discard route with source match
+ovn-nbctl lr-route-del lr1 20.0.0.0/24
+ovn-nbctl --policy="src-ip" lr-route-add lr1 10.0.0.0/24 discard
+
+# Allow some time for ovn-controller to catch up.
+ovn-nbctl --wait=hv sync
+
+# Check logical flows for drop rule
+AT_CHECK([ovn-sbctl dump-flows | grep lr_in_ip_routing | grep "10.0.0.0/24" | \
+    grep drop | wc -l], [0], [dnl
+1
+])
+
+# Send packet.
+packet="inport==\"sw1-lp1\" && eth.src==00:00:04:01:02:03 &&
+       eth.dst==00:00:01:01:02:03 && ip4 && ip.ttl==64 &&
+       ip4.src==10.0.0.100 && ip4.dst==20.0.0.200 &&
+       udp && udp.src==53 && udp.dst==4369"
+
+as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
+
+# Check if packet hit the drop rule
+AT_CHECK([ovs-ofctl dump-flows br-int "nw_src=10.0.0.0/24" | \
+    grep "actions=drop" | grep "n_packets=1" | wc -l], [0], [dnl
+1
+])
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
\ No newline at end of file
diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
index 2cab592ce..165cc4ea8 100644
--- a/utilities/ovn-nbctl.8.xml
+++ b/utilities/ovn-nbctl.8.xml
@@ -673,7 +673,8 @@ 
           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>.
+          on <var>nexthop</var>. Nexthop can be set to <var>discard</var>
+          for dropping packets which match the given route.
         </p>
 
         <p>
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index 2c77f4ba7..061acc518 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -3972,6 +3972,7 @@  nbctl_lr_route_add(struct ctl_context *ctx)
 
     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;
@@ -3992,13 +3993,18 @@  nbctl_lr_route_add(struct ctl_context *ctx)
         return;
     }
 
-    next_hop = v6_prefix
-        ? normalize_ipv6_addr_str(ctx->argv[3])
-        : normalize_ipv4_addr_str(ctx->argv[3]);
-    if (!next_hop) {
-        ctl_error(ctx, "bad %s nexthop argument: %s",
-                  v6_prefix ? "IPv6" : "IPv4", ctx->argv[3]);
-        goto cleanup;
+    bool is_discard_route = !strcmp(ctx->argv[3], "discard");
+    if (is_discard_route) {
+        next_hop = xasprintf("discard");
+    } else {
+        next_hop = v6_prefix
+            ? normalize_ipv6_addr_str(ctx->argv[3])
+            : normalize_ipv4_addr_str(ctx->argv[3]);
+        if (!next_hop) {
+            ctl_error(ctx, "bad %s nexthop argument: %s",
+                      v6_prefix ? "IPv6" : "IPv4", ctx->argv[3]);
+            goto cleanup;
+        }
     }
 
     struct shash_node *bfd = shash_find(&ctx->options, "--bfd");
@@ -4031,6 +4037,25 @@  nbctl_lr_route_add(struct ctl_context *ctx)
                 ecmp_symmetric_reply;
     struct nbrec_logical_router_static_route *route =
         nbctl_lr_get_route(lr, prefix, next_hop, is_src_route, ecmp);
+
+    /* Validations for nexthop = "discard" */
+    if (is_discard_route) {
+        if (ecmp) {
+            ctl_error(ctx, "ecmp is not valid for discard routes.");
+            goto cleanup;
+        }
+        if (bfd) {
+            ctl_error(ctx, "bfd dst_ip cannot be discard.");
+            goto cleanup;
+        }
+        if (ctx->argc == 5) {
+            if (is_discard_route) {
+                ctl_error(ctx, "outport is not valid for discard routes.");
+                goto cleanup;
+            }
+        }
+    }
+
     if (!ecmp) {
         if (route) {
             if (!may_exist) {
@@ -4072,6 +4097,13 @@  nbctl_lr_route_add(struct ctl_context *ctx)
         goto cleanup;
     }
 
+    struct nbrec_logical_router_static_route *discard_route =
+        nbctl_lr_get_route(lr, prefix, "discard", is_src_route, true);
+    if (discard_route) {
+        ctl_error(ctx, "discard nexthop for the same ECMP route exists.");
+        goto cleanup;
+    }
+
     route = nbrec_logical_router_static_route_insert(ctx->txn);
     nbrec_logical_router_static_route_set_ip_prefix(route, prefix);
     nbrec_logical_router_static_route_set_nexthop(route, next_hop);
@@ -4146,10 +4178,14 @@  nbctl_lr_route_del(struct ctl_context *ctx)
 
     char *nexthop = NULL;
     if (ctx->argc >= 4) {
-        nexthop = normalize_prefix_str(ctx->argv[3]);
-        if (!nexthop) {
-            ctl_error(ctx, "bad nexthop argument: %s", ctx->argv[3]);
-            return;
+        if (!strcmp(ctx->argv[3], "discard")) {
+            nexthop = xasprintf("discard");
+        } else {
+            nexthop = normalize_prefix_str(ctx->argv[3]);
+            if (!nexthop) {
+                ctl_error(ctx, "bad nexthop argument: %s", ctx->argv[3]);
+                return;
+            }
         }
     }
 
@@ -4190,8 +4226,9 @@  nbctl_lr_route_del(struct ctl_context *ctx)
 
         /* Compare nexthop, if specified. */
         if (nexthop) {
-            char *rt_nexthop =
-                normalize_prefix_str(lr->static_routes[i]->nexthop);
+            char *rt_nexthop = !strcmp(lr->static_routes[i]->nexthop, "discard")
+                ? xasprintf("discard")
+                : normalize_prefix_str(lr->static_routes[i]->nexthop);
             if (!rt_nexthop) {
                 /* Ignore existing nexthop we couldn't parse. */
                 continue;
@@ -5579,7 +5616,9 @@  print_route(const struct nbrec_logical_router_static_route *route,
 {
 
     char *prefix = normalize_prefix_str(route->ip_prefix);
-    char *next_hop = normalize_prefix_str(route->nexthop);
+    char *next_hop = !strcmp(route->nexthop, "discard")
+        ? xasprintf("discard")
+        : normalize_prefix_str(route->nexthop);
     ds_put_format(s, "%25s %25s", prefix, next_hop);
     free(prefix);
     free(next_hop);