Re: [ovs-dev] [PATCH ovn] Logical Router Policy chains.

[Mark Michelson]

On 1/22/25 07:31, Aleksandr Smirnov (K2 Cloud) wrote:
Hi Mark,
Please look at my answers below
++Vlad to discuss proposed approach.

On 1/20/25 7:25 PM, Mark Michelson wrote:
Hi Aleksandr, thanks for the submission.

This submission should have a system test added, to ensure packets
behave as expected when policy chains are used.

I see a test in ovn.at (policy-based routing: 1 HVs, 2 LSs, 1 lport/LS,
1 LR) that check route policy traffic.

This test is good candidate to extend to check policy chains also. Could
I modify this one to test policy chains?

If that will make it easier for you, then I'm totally fine with that.



I have some notes and suggestions below.

On 12/18/24 05:23, Aleksandr Smirnov wrote:
1. Objective

The objective is to enchance routing policies to organize them as set of
independent lists, here 'chains', and allow to select and traverse
chains
depending on match conditions.

Every routing policy rule have assigned chain name, a type of string, or
an empty string. Rule actions have new action 'jump' with string
argument
that allows to switch rule's traversal to given chain.

2. New traversal flow.

The flow starts rule's traversal for only those having an empty chain
name,
accoring to their priorities. If match occurs on rule having the 'jump'

s/accoring/according/

This mistake appears again later in the commit message and should be
fixed there as well.

action, a traversal restarts for rules that belongs to that chain,
again,
accoring to their priorities. Any other actions will stop router
policy flow.
(i.e. 'jump' have a meaning of 'goto' but not 'call/return').

3. Change of utilities.

ovn-nbctl lr-policy-add accepts chain name as optional argument:
    ovn-nbctl [--chain Name] lr-policy-add ...

      If omitted, chain name will be empty, i.e. rule added will belong
      to default flow.

ovn-nbctl lr-policy-add accepts new action 'jump':
    ovn-nbctl lr-policy-add ... jump Name

ovn-nbctl lr-policy-list changes its output format:
    to print rules grouped by chain name
    to print jump actions

ovn-nbctl lr-policy-del accepts chain name as optional argument:
    ovn-nbctl [--chain Name] lr-policy-del ...

      If --chain is used:
          Delete by LR name: Delete whole chain.
          Delete by uuid: just delete by uuid.
          Delete by priority: Delete rules of this chain and given
                              priority.
          Delete by priority & match: Delete rules of this chain
                              having given priority and match.
      Else (--chain is not used):
          Delete by LR name: Delete all rules.
          Delete by uuid: just delete by uuid.
          Delete by priority: Delete rules of default chain
                              (i.e. having chain name '') and given
                              priority.
          Delete by priority & match: Delete rules of default chain
                               having given priority and match.

4. Changes in NB DB schema

Logical_Router_Policy:
      New field:
          Name: chain
          Type: string
      New field:
          Name: jump_chain
          Type: string
      Modified field:
          Name: actions
          Value: add enum value 'jump'

5. Change in the SB DB generation

      1. Reserve (reuse) one 16 bit register reg9[16..31] to hold
chain ID
         numeric value designating current chain to traverse.
      2. Reserve new stage (lr_in_policy_pre) at Logical Router ingress
         datapath, to make initial assignment reg9[16..31] = 0
      3. Generate router policy flow in the following manner:
        lr_in_ip_routing_ecmp  (not changed)
        lr_in_policy_pre
          match:  1
          actoin: reg9[16..31] = 0; next;
        lr_in_policy
          match: (reg9[16..31] == <chain_id>) && (northd generated
expression)
          action: for 'jump': reg9[16..31] = <jump_chain_id>;
next(ingress, lr_in_policy);
                  for other actions: northd generated expression

Signed-off-by: Aleksandr Smirnov <alekssmirnov@k2.cloud>
---
   northd/en-northd.c    |   2 +-
   northd/northd.c       |  75 ++++++++++++++++-
   northd/northd.h       |  19 ++---
   ovn-nb.ovsschema      |   9 ++-
   ovn-nb.xml            |  22 ++++-
   tests/ovn-nbctl.at    |  54 +++++++++++--
   tests/ovn-northd.at   |  33 ++++++++
   utilities/ovn-nbctl.c | 183 +++++++++++++++++++++++++++---------------
   8 files changed, 309 insertions(+), 88 deletions(-)

diff --git a/northd/en-northd.c b/northd/en-northd.c
index c7d1ebcb3..9c5ba5405 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -1,4 +1,4 @@
-    /*
+/*
    * Licensed under the Apache License, Version 2.0 (the "License");
    * you may not use this file except in compliance with the License.
    * You may obtain a copy of the License at:
diff --git a/northd/northd.c b/northd/northd.c
index b01e40ecd..e4ee12cdd 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -190,6 +190,7 @@ BUILD_ASSERT_DECL(ACL_OBS_STAGE_MAX < (1 << 2));
   #define REG_SRC_IPV4 "reg5"
   #define REG_SRC_IPV6 "xxreg1"
   #define REG_DHCP_RELAY_DIP_IPV4 "reg2"
+#define REG_POLICY_CHAIN_ID "reg9[16..31]"
   #define REG_ROUTE_TABLE_ID "reg7"
     /* Register used to store backend ipv6 address
@@ -10873,11 +10874,38 @@ static bool check_bfd_state(const struct
nbrec_logical_router_policy *rule,
       return true;
   }
   +static void
+policy_chain_add(struct simap *chain_ids, const char *chain_name)
+{
+    if (chain_name && *chain_name && !simap_get(chain_ids,
chain_name)) {
+        uint32_t id = simap_count(chain_ids) + 1;
+
+        if (id == UINT16_MAX) {
+            static struct vlog_rate_limit rl =
VLOG_RATE_LIMIT_INIT(5, 1);
+            VLOG_WARN_RL(&rl, "Too many policy chains for Logical
Router.");
+            return;
+        }
+
+        if (!simap_put(chain_ids, chain_name, id)) {
+            static struct vlog_rate_limit rl =
VLOG_RATE_LIMIT_INIT(5, 1);
+            VLOG_WARN_RL(&rl, "Policy chain id unexpectedly appeared");
+        }
+    }
+}
+
+static uint32_t
+policy_chain_id(struct simap *chain_ids, const char *chain_name)
+{
+    return (chain_name && *chain_name)
+           ? simap_get(chain_ids, chain_name)
+           : 0;
+}
+
   static void
   build_routing_policy_flow(struct lflow_table *lflows, struct
ovn_datapath *od,
                             const struct hmap *lr_ports, struct
route_policy *rp,
                             const struct ovsdb_idl_row *stage_hint,
-                          struct lflow_ref *lflow_ref)
+                          struct lflow_ref *lflow_ref, struct simap
*chain_ids)
   {
       const struct nbrec_logical_router_policy *rule = rp->rule;
       struct ds match = DS_EMPTY_INITIALIZER;
@@ -10927,7 +10955,21 @@ build_routing_policy_flow(struct lflow_table
*lflows, struct ovn_datapath *od,
                         out_port->lrp_networks.ea_s,
                         out_port->json_key,
                         is_ipv4);
+    } else if (!strcmp(rule->action, "jump")) {
+        uint32_t chain_id = policy_chain_id(chain_ids,
rule->jump_chain);
+
+        if (!chain_id) {
+            static struct vlog_rate_limit rl =
VLOG_RATE_LIMIT_INIT(5, 1);
+            VLOG_WARN_RL(&rl,
+                         "Logical router: %s, policy action 'jump'"
+                         " follows to non-existent chain %s",
+                         od->nbr->name, rule->jump_chain);
+            return;
+        }
   +        ds_put_format(&actions, REG_POLICY_CHAIN_ID
+                      "=%d; next(pipeline=ingress, table=%d);",
+                      chain_id,
ovn_stage_get_table(S_ROUTER_IN_POLICY));
       } else if (!strcmp(rule->action, "drop")) {
           ds_put_cstr(&actions, debug_drop_action());
       } else if (!strcmp(rule->action, "allow")) {
@@ -10937,7 +10979,13 @@ build_routing_policy_flow(struct lflow_table
*lflows, struct ovn_datapath *od,
           }
           ds_put_cstr(&actions, REG_ECMP_GROUP_ID" = 0; next;");
       }
-    ds_put_format(&match, "%s", rule->match);
+
+    if (simap_is_empty(chain_ids)) {
+        ds_put_format(&match, "%s", rule->match);
+    } else {
+        ds_put_format(&match, REG_POLICY_CHAIN_ID" == %d && (%s)",
+                      policy_chain_id(chain_ids, rule->chain),
rule->match);
+    }
         ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_POLICY,
rule->priority,
                               ds_cstr(&match), ds_cstr(&actions),
stage_hint,
@@ -14150,11 +14198,27 @@ build_ingress_policy_flows_for_lrouter(
           struct lflow_ref *lflow_ref)
   {
       ovs_assert(od->nbr);
+
+    struct route_policy *rp;
+
+    /* Policy generation is different if chains are used. */
+    struct simap chain_ids;
+
+    simap_init(&chain_ids);
+
+    HMAP_FOR_EACH_WITH_HASH (rp, key_node, uuid_hash(&od->key),
+                             route_policies) {
+        policy_chain_add(&chain_ids, rp->rule->chain);

The way this is written, it means that changes to the ordering of the
hmap may result in chains having their ID changed. For instance, let's
say you have a single policy chain "foo". When the logical flows are
generated, "foo" will have ID 1. Next, a new chain "bar" is added to
the policy chains. Now, this time, "bar" might have ID 1 and "foo"
will get ID 2 assigned to it. This means we have to rewrite foo's
flows to the SB DB just because the ID number changed. It would be
nicer if we only had to write the new "bar" flows instead.

I think the easiest way to avoid this issue is to change chains to be
numbers instead of arbitrary strings. This has some advantages:
1) It's easier to match logical flows to the router policies in the NB
DB.
2) This eliminates the need to translate chain names into numbers. We
can just use the numbers directly.
3) It eliminates the issue I mentioned in the previous paragraph; ID
values stay consistent across runs of ovn-northd.

It means the chains won't have as user-friendly names, but a CMS could
easily associate chain numbers with names outside of OVN.

Vladislav, as the feature's requestor, wants to have chains named. He
referenced to similar problem discussed during the 'multiple routing
tables' implementation.

There was a long discussion about routing table ids generated from names
vs explicitly set from CMS
https://patchwork.ozlabs.org/project/ovn/patch/20210816211507.40325-3-odiv...@gmail.com/

As result, routing tables were implemented as referenced by string names
that automatically converted to numerical ids.

Thanks for pointing to that discussion. Since we have precedent for 
using names, and not really caring too much if a changed ID results in 
re-calculating flows, then we can do the same thing here. If there ends 
up being a performance problem later because of flow churn, we can 
address the problem then. We can possibly do something similar to what 
you suggest below.


However, if we still want to avoid unnecessary sb db update (and, I
suppose, avoid similar problems on ovn-controller side during open flow
generation) I could propose following schema, applied upon current
implementation.

1. Extend table Logical_Router_Policy with yet another field chain_n
(type of integer 0..32767)
2. Modify 'nbctl --chain <name> lr-policy-add' to allocate available
numeric id for chain <name> and write to chain_n -- for new <name>, or,
fill up chain_n from other record if same-name chain is already in the
table.
3. northd loads association chain <-> chain_n from Logical_Router_Policy
table, and additionally perform automatic associations for entries
without associations stored in Logical_Router_Policy (for example, if
records were created with other tools)


+    }
+
       /* This is a catch-all rule. It has the lowest priority (0)
        * does a match-all("1") and pass-through (next) */
       ovn_lflow_add(lflows, od, S_ROUTER_IN_POLICY, 0, "1",
                     REG_ECMP_GROUP_ID" = 0; next;",
                     lflow_ref);
+    ovn_lflow_add(lflows, od, S_ROUTER_IN_POLICY_PRE, 0, "1",
+                  REG_POLICY_CHAIN_ID" = 0; next;",
+                  lflow_ref);
       ovn_lflow_add(lflows, od, S_ROUTER_IN_POLICY_ECMP, 150,
                     REG_ECMP_GROUP_ID" == 0", "next;",
                     lflow_ref);
@@ -14163,7 +14227,7 @@ build_ingress_policy_flows_for_lrouter(
         /* Convert routing policies to flows. */
       uint16_t ecmp_group_id = 1;
-    struct route_policy *rp;
+
       HMAP_FOR_EACH_WITH_HASH (rp, key_node, uuid_hash(&od->key),
                                route_policies) {
           const struct nbrec_logical_router_policy *rule = rp->rule;
@@ -14176,9 +14240,12 @@ build_ingress_policy_flows_for_lrouter(
               ecmp_group_id++;
           } else {
               build_routing_policy_flow(lflows, od, lr_ports, rp,
-                                      &rule->header_, lflow_ref);
+                                      &rule->header_, lflow_ref,
+                                      &chain_ids);
           }
       }
+
+    simap_destroy(&chain_ids);
   }
     /* Local router ingress table ARP_RESOLVE: ARP Resolution. */
diff --git a/northd/northd.h b/northd/northd.h
index 9457a7be6..364f8f3e6 100644
--- a/northd/northd.h
+++ b/northd/northd.h
@@ -491,17 +491,18 @@ enum ovn_stage {
       PIPELINE_STAGE(ROUTER, IN,  IP_ROUTING_PRE,  13,
"lr_in_ip_routing_pre")  \
       PIPELINE_STAGE(ROUTER, IN,  IP_ROUTING,      14,
"lr_in_ip_routing")      \
       PIPELINE_STAGE(ROUTER, IN,  IP_ROUTING_ECMP, 15,
"lr_in_ip_routing_ecmp") \
-    PIPELINE_STAGE(ROUTER, IN,  POLICY,          16,
"lr_in_policy")          \
-    PIPELINE_STAGE(ROUTER, IN,  POLICY_ECMP,     17,
"lr_in_policy_ecmp")     \
-    PIPELINE_STAGE(ROUTER, IN,  DHCP_RELAY_RESP_CHK,
18,                      \
+    PIPELINE_STAGE(ROUTER, IN,  POLICY_PRE,      16,
"lr_in_policy_pre")      \

I don't think this new pipeline stage is necessary.
REG_POLICY_CHAIN_ID defaults to 0 anyway, so the first time evaluating
it in the POLICY stage will have it set to 0. The register is only
used in the POLICY stage, so there is no need to ensure the register
has some default value for later stages.

+    PIPELINE_STAGE(ROUTER, IN, POLICY,          17,
"lr_in_policy")          \
+    PIPELINE_STAGE(ROUTER, IN,  POLICY_ECMP,     18,
"lr_in_policy_ecmp")     \
+    PIPELINE_STAGE(ROUTER, IN,  DHCP_RELAY_RESP_CHK,
19,                      \
"lr_in_dhcp_relay_resp_chk")                                \
-    PIPELINE_STAGE(ROUTER, IN,  DHCP_RELAY_RESP,
19,                          \
+    PIPELINE_STAGE(ROUTER, IN,  DHCP_RELAY_RESP,
20,                          \
"lr_in_dhcp_relay_resp")                                    \
-    PIPELINE_STAGE(ROUTER, IN,  ARP_RESOLVE,     20,
"lr_in_arp_resolve")     \
-    PIPELINE_STAGE(ROUTER, IN,  CHK_PKT_LEN,     21,
"lr_in_chk_pkt_len")     \
-    PIPELINE_STAGE(ROUTER, IN,  LARGER_PKTS,     22,
"lr_in_larger_pkts")     \
-    PIPELINE_STAGE(ROUTER, IN,  GW_REDIRECT,     23,
"lr_in_gw_redirect")     \
-    PIPELINE_STAGE(ROUTER, IN,  ARP_REQUEST,     24,
"lr_in_arp_request")     \
+    PIPELINE_STAGE(ROUTER, IN,  ARP_RESOLVE,     21,
"lr_in_arp_resolve")     \
+    PIPELINE_STAGE(ROUTER, IN,  CHK_PKT_LEN,     22,
"lr_in_chk_pkt_len")     \
+    PIPELINE_STAGE(ROUTER, IN,  LARGER_PKTS,     23,
"lr_in_larger_pkts")     \
+    PIPELINE_STAGE(ROUTER, IN,  GW_REDIRECT,     24,
"lr_in_gw_redirect")     \
+    PIPELINE_STAGE(ROUTER, IN,  ARP_REQUEST,     25,
"lr_in_arp_request")     \
\
       /* Logical router egress stages.
*/                               \
       PIPELINE_STAGE(ROUTER, OUT, CHECK_DNAT_LOCAL,
0,                       \
diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
index 09361920f..709d32c93 100644
--- a/ovn-nb.ovsschema
+++ b/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
   {
       "name": "OVN_Northbound",
-    "version": "7.8.0",
-    "cksum": "3497747919 38626",
+    "version": "7.8.1",
+    "cksum": "884115012 38765",
       "tables": {
           "NB_Global": {
               "columns": {
@@ -524,10 +524,13 @@
                   "priority": {"type": {"key": {"type": "integer",
                                                 "minInteger": 0,
                                                 "maxInteger": 32767}}},
+                "chain": {"type": "string"},
                   "match": {"type": "string"},
                   "action": {"type": {
                       "key": {"type": "string",
-                            "enum": ["set", ["allow", "drop",
"reroute"]]}}},
+                            "enum": ["set",
+                                    ["allow", "drop", "reroute",
"jump"]]}}},
+                "jump_chain": {"type": "string"},
                   "nexthop": {"type": {"key": "string", "min": 0,
"max": 1}},
                   "nexthops": {"type": {
                       "key": "string", "min": 0, "max": "unlimited"}},
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 8373ddb99..6e43e9bc9 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -3942,7 +3942,15 @@ or
         <p>
           The routing policy's priority.  Rules with numerically
higher priority
           take precedence over those with lower. A rule is uniquely
identified
-        by the priority and match string.
+        by the priority, chain and match string.
+      </p>
+    </column>
+
+    <column name="chain">
+      <p>
+        The routing policy rule's chain name.  Only rules with empty
chain name
+        are traversed by default.  Other chains are traversed as
response to
+        jump action.
         </p>
       </column>
   @@ -3977,9 +3985,21 @@ or
             <code>reroute</code>: Reroute packet to <ref
column="nexthop"/> or
             <ref column="nexthops"/>.
           </li>
+
+        <li>
+          <code>jump</code>: Start examining rules that have the same
+          <ref column="chain"/> value as specified in
+          <ref column="jump_chain"/>.
+        </li>
         </ul>
       </column>
   +    <column name="jump_chain">
+      <p>
+        The routing policy rule's chain name selected to be examined
next.
+      </p>
+    </column>
+
       <column name="nexthop">
         <p>
           Note: This column is deprecated in favor of <ref
column="nexthops"/>.
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 39c02d295..583c3ec74 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -2318,7 +2318,7 @@ dnl
---------------------------------------------------------------------
   OVN_NBCTL_TEST([ovn_nbctl_policies], [policies], [
   AT_CHECK([ovn-nbctl lr-add lr0])
   -dnl Add policies with allow and drop actions
+dnl Add policies with allow, drop and jump actions
   AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.1.0/24"
drop])
   AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.2.0/24"
allow pkt_mark=100,foo=bar])
   AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.1.0/24"
allow])
@@ -2330,6 +2330,8 @@ AT_CHECK([ovn-nbctl --bfd lr-policy-add lr0 103
"ip4.src == 1.2.3.0/24" reroute
   AT_CHECK([ovn-nbctl --bfd lr-policy-add lr0 103 "ip4.src ==
1.2.3.0/24" drop], [1], [],
     [ovn-nbctl: BFD is valid only with reroute action.
   ])
+AT_CHECK([ovn-nbctl --chain=inbound lr-policy-add lr0 100 "ip4.src
== 1.1.1.0/24" drop])
+AT_CHECK([ovn-nbctl --chain=outbound lr-policy-add lr0 100 "ip4.src
== 2.1.1.0/24" jump inbound])
     dnl Incomplete option set.
   AT_CHECK([ovn-nbctl lr-policy-add lr0 200 "ip4.src == 1.1.4.0/24"
reroute 192.168.0.10 foo], [1], [],
@@ -2340,6 +2342,14 @@ AT_CHECK([ovn-nbctl lr-policy-add lr0 200
"ip4.src == 1.1.4.0/24" allow bar=], [
     [ovn-nbctl: No value specified for the option : bar
   ])
   +AT_CHECK([ovn-nbctl --chain lr-policy-add lr0 100 "ip4.src ==
1.1.1.0/24" drop], [1], [],
+  [ovn-nbctl: unknown command 'lr0'; use --help for help
+])
+
+AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 2.1.1.0/24"
jump], [1], [],
+  [ovn-nbctl: Chain name is required when action is jump.
+])
+
   dnl Add duplicated policy
   AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.1.0/24"
drop], [1], [],
     [ovn-nbctl: Same routing policy already existed on the logical
router lr0.
@@ -2349,35 +2359,69 @@ AT_CHECK([ovn-nbctl --may-exist lr-policy-add
lr0 100 "ip4.src == 1.1.1.0/24" dr
     dnl Add duplicated policy
   AT_CHECK([ovn-nbctl lr-policy-add lr0 103 "ip4.src == 1.1.1.0/24"
deny], [1], [],
-  [ovn-nbctl: deny: action must be one of "allow", "drop", and
"reroute"
+  [ovn-nbctl: deny: action must be one of "allow", "drop",
"reroute", "jump"
   ])
     dnl Delete by priority and match string
   AT_CHECK([ovn-nbctl lr-policy-del lr0 100 "ip4.src == 1.1.1.0/24"])
+
+AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl
+Routing Policies
+Chain <Default>:
+       101                              ip4.src ==
2.1.1.0/24           allow
+       101                              ip4.src ==
2.1.2.0/24            drop
+       101                               ip6.src ==
2002::/64            drop
+       100                              ip4.src ==
1.1.2.0/24           allow               pkt_mark=100,foo=bar
+
+Chain inbound:
+       100                              ip4.src ==
1.1.1.0/24            drop
+
+Chain outbound:
+       100                              ip4.src ==
2.1.1.0/24            jump -> inbound
+])
+
+AT_CHECK([ovn-nbctl --chain inbound lr-policy-del lr0 100 "ip4.src
== 1.1.1.0/24"])
+
   AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl
   Routing Policies
+Chain <Default>:
          101                              ip4.src ==
2.1.1.0/24           allow
          101                              ip4.src ==
2.1.2.0/24            drop
          101                               ip6.src ==
2002::/64            drop
          100                              ip4.src ==
1.1.2.0/24           allow               pkt_mark=100,foo=bar
+
+Chain outbound:
+       100                              ip4.src ==
2.1.1.0/24            jump -> inbound
   ])
     dnl Delete all policies for given priority
   AT_CHECK([ovn-nbctl lr-policy-del lr0 101])
   AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl
   Routing Policies
+Chain <Default>:
          100                              ip4.src ==
1.1.2.0/24           allow               pkt_mark=100,foo=bar
-])
   +Chain outbound:
+       100                              ip4.src ==
2.1.1.0/24            jump -> inbound
+])
     dnl Delete policy by specified uuid
-uuid=$(ovn-nbctl --bare --column _uuid list logical_router_policy)
+uuid=$(ovn-nbctl --bare --column _uuid find logical_router_policy
chain=\"\")
   AT_CHECK([ovn-nbctl lr-policy-del lr0 $uuid])
-AT_CHECK([ovn-nbctl list logical-router-policy], [0], [dnl
+AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl
+Routing Policies
+
+Chain outbound:
+       100                              ip4.src ==
2.1.1.0/24            jump -> inbound
   ])
     AT_CHECK([ovn-nbctl --if-exists lr-policy-del lr0 $uuid])
   +AT_CHECK([ovn-nbctl --chain outbound lr-policy-del lr0])
+
+AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl
+])
+
   dnl Add policy with reroute action
   AT_CHECK([ovn-nbctl lr-policy-add lr0 102 "ip4.src == 3.1.2.0/24"
reroute 3.3.3.3])
   diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 21d9d63ab..922bc6df3 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -3444,6 +3444,7 @@ AT_CHECK([grep "lr_in_policy" lr0flows3 |
ovn_strip_lflows], [0], [dnl
     table=??(lr_in_policy_ecmp  ), priority=100  ,
match=(reg8[[0..15]] == 1 && reg8[[16..31]] == 1), action=(reg0 =
172.168.0.101; reg5 = 172.168.0.100; eth.src = 00:00:20:20:12:13;
outport = "lr0-public"; flags.loopback = 1; reg9[[9]] = 1; next;)
     table=??(lr_in_policy_ecmp  ), priority=100  ,
match=(reg8[[0..15]] == 1 && reg8[[16..31]] == 2), action=(reg0 =
172.168.0.102; reg5 = 172.168.0.100; eth.src = 00:00:20:20:12:13;
outport = "lr0-public"; flags.loopback = 1; reg9[[9]] = 1; next;)
     table=??(lr_in_policy_ecmp  ), priority=150  ,
match=(reg8[[0..15]] == 0), action=(next;)
+  table=??(lr_in_policy_pre   ), priority=0    , match=(1),
action=(reg9[[16..31]] = 0; next;)
   ])
     check ovn-nbctl --wait=sb lr-policy-add lr0  10 "ip4.src ==
10.0.0.4" reroute 172.168.0.101,172.168.0.102,172.168.0.103
@@ -3463,6 +3464,7 @@ sed 's/reg8\[[0..15\]] ==
[[0-9]]*/reg8\[[0..15\]] == <cleared>/' | ovn_strip_lf
     table=??(lr_in_policy_ecmp  ), priority=100  ,
match=(reg8[[0..15]] == <cleared> && reg8[[16..31]] == 2),
action=(reg0 = 172.168.0.102; reg5 = 172.168.0.100; eth.src =
00:00:20:20:12:13; outport = "lr0-public"; flags.loopback = 1;
reg9[[9]] = 1; next;)
     table=??(lr_in_policy_ecmp  ), priority=100  ,
match=(reg8[[0..15]] == <cleared> && reg8[[16..31]] == 3),
action=(reg0 = 172.168.0.103; reg5 = 172.168.0.100; eth.src =
00:00:20:20:12:13; outport = "lr0-public"; flags.loopback = 1;
reg9[[9]] = 1; next;)
     table=??(lr_in_policy_ecmp  ), priority=150  ,
match=(reg8[[0..15]] == <cleared>), action=(next;)
+  table=??(lr_in_policy_pre   ), priority=0    , match=(1),
action=(reg9[[16..31]] = 0; next;)
   ])
     check ovn-nbctl --wait=sb lr-policy-add lr0  10 "ip4.src ==
10.0.0.5" reroute 172.168.0.110
@@ -3483,6 +3485,7 @@ sed 's/reg8\[[0..15\]] ==
[[0-9]]*/reg8\[[0..15\]] == <cleared>/' | ovn_strip_lf
     table=??(lr_in_policy_ecmp  ), priority=100  ,
match=(reg8[[0..15]] == <cleared> && reg8[[16..31]] == 2),
action=(reg0 = 172.168.0.102; reg5 = 172.168.0.100; eth.src =
00:00:20:20:12:13; outport = "lr0-public"; flags.loopback = 1;
reg9[[9]] = 1; next;)
     table=??(lr_in_policy_ecmp  ), priority=100  ,
match=(reg8[[0..15]] == <cleared> && reg8[[16..31]] == 3),
action=(reg0 = 172.168.0.103; reg5 = 172.168.0.100; eth.src =
00:00:20:20:12:13; outport = "lr0-public"; flags.loopback = 1;
reg9[[9]] = 1; next;)
     table=??(lr_in_policy_ecmp  ), priority=150  ,
match=(reg8[[0..15]] == <cleared>), action=(next;)
+  table=??(lr_in_policy_pre   ), priority=0    , match=(1),
action=(reg9[[16..31]] = 0; next;)
   ])
     check ovn-nbctl --wait=sb lr-policy-del lr0  10 "ip4.src ==
10.0.0.3"
@@ -3500,6 +3503,7 @@ sed 's/reg8\[[0..15\]] ==
[[0-9]]*/reg8\[[0..15\]] == <cleared>/' | ovn_strip_lf
     table=??(lr_in_policy_ecmp  ), priority=100  ,
match=(reg8[[0..15]] == <cleared> && reg8[[16..31]] == 2),
action=(reg0 = 172.168.0.102; reg5 = 172.168.0.100; eth.src =
00:00:20:20:12:13; outport = "lr0-public"; flags.loopback = 1;
reg9[[9]] = 1; next;)
     table=??(lr_in_policy_ecmp  ), priority=100  ,
match=(reg8[[0..15]] == <cleared> && reg8[[16..31]] == 3),
action=(reg0 = 172.168.0.103; reg5 = 172.168.0.100; eth.src =
00:00:20:20:12:13; outport = "lr0-public"; flags.loopback = 1;
reg9[[9]] = 1; next;)
     table=??(lr_in_policy_ecmp  ), priority=150  ,
match=(reg8[[0..15]] == <cleared>), action=(next;)
+  table=??(lr_in_policy_pre   ), priority=0    , match=(1),
action=(reg9[[16..31]] = 0; next;)
   ])
     check ovn-nbctl --wait=sb lr-policy-del lr0  10 "ip4.src ==
10.0.0.4"
@@ -3513,6 +3517,7 @@ sed 's/reg8\[[0..15\]] ==
[[0-9]]*/reg8\[[0..15\]] == <cleared>/' | ovn_strip_lf
     table=??(lr_in_policy       ), priority=10   , match=(ip4.src ==
10.0.0.5), action=(reg0 = 172.168.0.110; reg5 = 172.168.0.100;
eth.src = 00:00:20:20:12:13; outport = "lr0-public"; flags.loopback =
1; reg8[[0..15]] = <cleared>; reg9[[9]] = 1; next;)
     table=??(lr_in_policy_ecmp  ), priority=0    , match=(1),
action=(drop;)
     table=??(lr_in_policy_ecmp  ), priority=150  ,
match=(reg8[[0..15]] == <cleared>), action=(next;)
+  table=??(lr_in_policy_pre   ), priority=0    , match=(1),
action=(reg9[[16..31]] = 0; next;)
   ])
     check ovn-nbctl --wait=sb add logical_router_policy . nexthops
"2000\:\:b"
@@ -3525,6 +3530,34 @@ sed 's/reg8\[[0..15\]] ==
[[0-9]]*/reg8\[[0..15\]] == <cleared>/' | ovn_strip_lf
     table=??(lr_in_policy       ), priority=0    , match=(1),
action=(reg8[[0..15]] = <cleared>; next;)
     table=??(lr_in_policy_ecmp  ), priority=0    , match=(1),
action=(drop;)
     table=??(lr_in_policy_ecmp  ), priority=150  ,
match=(reg8[[0..15]] == <cleared>), action=(next;)
+  table=??(lr_in_policy_pre   ), priority=0    , match=(1),
action=(reg9[[16..31]] = 0; next;)
+])
+
+AT_CLEANUP
+])
+
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([Router policies - chains])
+ovn_start
+
+check ovn-nbctl --wait=sb lr-add lr0
+check ovn-nbctl --wait=sb lr-policy-add lr0 112 "ip4.src ==
10.0.0.0/24" jump nochain
+check ovn-nbctl --wait=sb lr-policy-add lr0 102 "ip4.src ==
10.0.0.0/24" jump inbound
+check ovn-nbctl --wait=sb lr-policy-add lr0 101 "ip4.src ==
10.0.16.0/24" jump inbound
+check ovn-nbctl --wait=sb lr-policy-add lr0 100 "ip4.src ==
192.168.0.1/32" allow
+check ovn-nbctl --wait=sb --chain=inbound lr-policy-add lr0 201 "1"
allow
+
+AT_CHECK([grep -qE "Logical router: lr0, policy action 'jump'
follows to non-existent chain nochain" northd/ovn-northd.log], [0])
+
+ovn-sbctl dump-flows lr0 > lr0flows2
+AT_CAPTURE_FILE([lr0flows2])
+
+AT_CHECK([grep "lr_in_policy[[^_]]" lr0flows2 | ovn_strip_lflows |
sort], [0], [dnl
+  table=??(lr_in_policy       ), priority=0    , match=(1),
action=(reg8[[0..15]] = 0; next;)
+  table=??(lr_in_policy       ), priority=100  ,
match=(reg9[[16..31]] == 0 && (ip4.src == 192.168.0.1/32)),
action=(reg8[[0..15]] = 0; next;)
+  table=??(lr_in_policy       ), priority=101  ,
match=(reg9[[16..31]] == 0 && (ip4.src == 10.0.16.0/24)),
action=(reg9[[16..31]]=1; next(pipeline=ingress, table=??);)
+  table=??(lr_in_policy       ), priority=102  ,
match=(reg9[[16..31]] == 0 && (ip4.src == 10.0.0.0/24)),
action=(reg9[[16..31]]=1; next(pipeline=ingress, table=??);)
+  table=??(lr_in_policy       ), priority=201  ,
match=(reg9[[16..31]] == 1 && (1)), action=(reg8[[0..15]] = 0; next;)
   ])
     AT_CLEANUP
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index 131d9f0bf..0c31a9698 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -4091,6 +4091,7 @@ nbctl_pre_lr_policy_add(struct ctl_context *ctx)
         ovsdb_idl_add_column(ctx->idl,
&nbrec_logical_router_policy_col_priority);
       ovsdb_idl_add_column(ctx->idl,
&nbrec_logical_router_policy_col_match);
+    ovsdb_idl_add_column(ctx->idl,
&nbrec_logical_router_policy_col_chain);
         ovsdb_idl_add_column(ctx->idl, &nbrec_bfd_col_dst_ip);
       ovsdb_idl_add_column(ctx->idl, &nbrec_bfd_col_logical_port);
@@ -4114,13 +4115,16 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
       const char *action = ctx->argv[4];
       size_t n_nexthops = 0;
       char **nexthops = NULL;
+    const char *chain_s = shash_find_data(&ctx->options, "--chain");
         bool reroute = false;
+    bool jump = false;
+
       /* Validate action. */
       if (strcmp(action, "allow") && strcmp(action, "drop")
-        && strcmp(action, "reroute")) {
+        && strcmp(action, "jump") && strcmp(action, "reroute")) {
           ctl_error(ctx, "%s: action must be one of \"allow\",
\"drop\", "
-                  "and \"reroute\"", action);
+                  "\"reroute\", \"jump\"", action);
           return;
       }
       if (!strcmp(action, "reroute")) {
@@ -4131,14 +4135,23 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
           reroute = true;
       }
   +    if (!strcmp(action, "jump")) {
+        if (ctx->argc < 6) {
+            ctl_error(ctx, "Chain name is required when action is
jump.");
+            return;
+        }
+        jump = true;
+    }
+
       /* Check if same routing policy already exists.
        * A policy is uniquely identified by priority and match */
       bool may_exist = !!shash_find(&ctx->options, "--may-exist");
       size_t i;
       for (i = 0; i < lr->n_policies; i++) {
           const struct nbrec_logical_router_policy *policy =
lr->policies[i];
-        if (policy->priority == priority &&
-            !strcmp(policy->match, ctx->argv[3])) {
+        if (policy->priority == priority
+            && !strcmp(policy->match, ctx->argv[3])
+            && !strcmp(policy->chain, (chain_s) ? chain_s : "")) {
               if (!may_exist) {
                   ctl_error(ctx, "Same routing policy already existed
on the "
                             "logical router %s.", ctx->argv[1]);
@@ -4200,6 +4213,15 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
       nbrec_logical_router_policy_set_priority(policy, priority);
       nbrec_logical_router_policy_set_match(policy, ctx->argv[3]);
       nbrec_logical_router_policy_set_action(policy, action);
+
+    if (chain_s) {
+        nbrec_logical_router_policy_set_chain(policy, chain_s);
+    }
+
+    if (jump) {
+        nbrec_logical_router_policy_set_jump_chain(policy,
ctx->argv[5]);
+    }
+
       if (reroute) {
           nbrec_logical_router_policy_set_nexthops(
               policy, (const char **)nexthops, n_nexthops);
@@ -4207,7 +4229,7 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
         /* Parse the options. */
       struct smap options = SMAP_INITIALIZER(&options);
-    for (i = reroute ? 6 : 5; i < ctx->argc; i++) {
+    for (i = (reroute | jump) ? 6 : 5; i < ctx->argc; i++) {

Nit: Use "||" instead of "|" since we are performing a logical or of
two boolean values.

           char *key, *value;
           value = xstrdup(ctx->argv[i]);
           key = strsep(&value, "=");
@@ -4319,84 +4341,81 @@ nbctl_pre_lr_policy_del(struct ctl_context *ctx)
         ovsdb_idl_add_column(ctx->idl,
&nbrec_logical_router_policy_col_priority);
       ovsdb_idl_add_column(ctx->idl,
&nbrec_logical_router_policy_col_match);
+    ovsdb_idl_add_column(ctx->idl,
&nbrec_logical_router_policy_col_chain);
   }
     static void
   nbctl_lr_policy_del(struct ctl_context *ctx)
   {
       const struct nbrec_logical_router *lr;
-    int64_t priority = 0;
       char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
+
       if (error) {
           ctx->error = error;
           return;
       }
   -    if (ctx->argc == 2) {
-        /* If a priority is not specified, delete all policies. */
+    const char *chain = shash_find_data(&ctx->options, "--chain");
+
+    if (!chain) {
+        chain = "";
+    }
+
+    /* An uuid or priority either chain are not specified.
+       Delete all policies for lr */
+    if (ctx->argc == 2 && !*chain) {
           nbrec_logical_router_set_policies(lr, NULL, 0);
           return;
       }
   -    const struct uuid *lr_policy_uuid = NULL;
+    int64_t priority = 0;
       struct uuid uuid_from_cmd;
-    if (uuid_from_string(&uuid_from_cmd, ctx->argv[2])) {
-        lr_policy_uuid = &uuid_from_cmd;
-    } else {
-        error = parse_priority(ctx->argv[2], &priority);
-        if (error) {
-            ctx->error = error;
-            return;
-        }
-    }
-    /* If uuid was specified, delete routing policy with the
-     * specified uuid. */
-    if (ctx->argc == 3) {
-        size_t i;
   -        if (lr_policy_uuid) {
-            for (i = 0; i < lr->n_policies; i++) {
-                if (uuid_equals(lr_policy_uuid,
- &(lr->policies[i]->header_.uuid))) {
- nbrec_logical_router_update_policies_delvalue(
-                        lr, lr->policies[i]);
-                    break;
-                }
-            }
-            if (i == lr->n_policies) {
-                if (!shash_find(&ctx->options, "--if-exists")) {
-                    ctl_error(ctx, "Logical router policy uuid is
not found.");
-                }
+    /* An uuid is specified, delete the policy with uuid only */
+    if (ctx->argc == 3 && uuid_from_string(&uuid_from_cmd,
ctx->argv[2])) {
+        for (int i = 0; i < lr->n_policies; i++) {
+            if (uuid_equals(&uuid_from_cmd,
+ &(lr->policies[i]->header_.uuid))) {
+                nbrec_logical_router_update_policies_delvalue(
+                lr, lr->policies[i]);
                   return;
               }
+        }
   -        /* If match is not specified, delete all routing policies
with the
-         * specified priority. */
-        } else {
-            for (i = 0; i < lr->n_policies; i++) {
-                if (priority == lr->policies[i]->priority) {
- nbrec_logical_router_update_policies_delvalue(
-                        lr, lr->policies[i]);
-                }
-            }
+        if (!shash_find(&ctx->options, "--if-exists")) {
+            ctl_error(ctx, "Logical router policy uuid is not found.");
           }
+
           return;
       }
   -    /* Delete policy that has the same priority and match string */
-    for (int i = 0; i < lr->n_policies; i++) {
-        struct nbrec_logical_router_policy *routing_policy =
lr->policies[i];
-        if (priority == routing_policy->priority &&
-            !strcmp(ctx->argv[3], routing_policy->match)) {
-            nbrec_logical_router_update_policies_delvalue(lr,
routing_policy);
+    if (ctx->argc >= 3) {
+        error = parse_priority(ctx->argv[2], &priority);
+        if (error) {
+            ctx->error = error;
               return;
           }
       }
+
+    char *match = (ctx->argc == 4) ? ctx->argv[3] : NULL;
+
+    for (int i = 0; i < lr->n_policies; i++) {
+        struct nbrec_logical_router_policy *policy = lr->policies[i];
+
+        /* Delete policies selected by chain, priority (if set),
+           match (if set). Sure, at least one option is set here */
+        if ((!priority || priority == policy->priority)
+            && (!strcmp(chain, policy->chain))
+            && (!match || !strcmp(match, policy->match))) {
+            nbrec_logical_router_update_policies_delvalue(lr, policy);
+        }
+    }
   }
   - struct routing_policy {
+struct routing_policy {
       int priority;
       char *match;
+    char *chain;
       const struct nbrec_logical_router_policy *policy;
   };
   @@ -4405,6 +4424,12 @@ routing_policy_cmp(const void *policy1_,
const void *policy2_)
   {
       const struct routing_policy *policy1p = policy1_;
       const struct routing_policy *policy2p = policy2_;
+    int chain_match = strcmp(policy1p->chain, policy2p->chain);
+
+    if (chain_match) {
+        return chain_match;
+    }
+
       if (policy1p->priority != policy2p->priority) {
           return policy1p->priority > policy2p->priority ? -1 : 1;
       } else {
@@ -4416,17 +4441,18 @@ static void
   print_routing_policy(const struct nbrec_logical_router_policy *policy,
                        struct ds *s)
   {
-    if (policy->n_nexthops) {
-        ds_put_format(s, "%10"PRId64" %50s %15s", policy->priority,
-                      policy->match, policy->action);
-        for (int i = 0; i < policy->n_nexthops; i++) {
-            char *next_hop = normalize_prefix_str(policy->nexthops[i]);
-            ds_put_format(s, i ? ", %s" : " %25s", next_hop ?
next_hop : "");
-            free(next_hop);
-        }
-    } else {
-        ds_put_format(s, "%10"PRId64" %50s %15s", policy->priority,
-                      policy->match, policy->action);
+    ds_put_format(s, "%10"PRId64" %50s %15s",
+                  policy->priority, policy->match, policy->action);
+
+    if (strcmp(policy->action, "jump") == 0
+        && policy->jump_chain && *policy->jump_chain) {
+        ds_put_format(s, " -> %s", policy->jump_chain);
+    }
+
+    for (int i = 0; i < policy->n_nexthops; i++) {
+        char *next_hop = normalize_prefix_str(policy->nexthops[i]);
+        ds_put_format(s, i ? ", %s" : " %25s", next_hop ? next_hop :
"");
+        free(next_hop);
       }
         if (!smap_is_empty(&policy->options) ||
policy->n_bfd_sessions) {
@@ -4457,6 +4483,9 @@ nbctl_pre_lr_policy_list(struct ctl_context *ctx)
       ovsdb_idl_add_column(ctx->idl,
&nbrec_logical_router_policy_col_options);
       ovsdb_idl_add_column(ctx->idl,
&nbrec_logical_router_policy_col_bfd_sessions);
+    ovsdb_idl_add_column(ctx->idl,
&nbrec_logical_router_policy_col_chain);
+    ovsdb_idl_add_column(ctx->idl,
+ &nbrec_logical_router_policy_col_jump_chain);
   }
     static void
@@ -4476,6 +4505,7 @@ nbctl_lr_policy_list(struct ctl_context *ctx)
               = lr->policies[i];
           policies[n_policies].priority = policy->priority;
           policies[n_policies].match = policy->match;
+        policies[n_policies].chain = (policy->chain) ? policy->chain
: "";
           policies[n_policies].policy = policy;
           n_policies++;
       }
@@ -4483,9 +4513,31 @@ nbctl_lr_policy_list(struct ctl_context *ctx)
       if (n_policies) {
           ds_put_cstr(&ctx->output, "Routing Policies\n");
       }
+
+    /* Print chain headings only if there are non-default chains.
+       Otherwise print format will be fully backward compatible.
+       Note that at this point policies are sorted out by chain
names too.
+    */
+    char *chain_current;
+    int  print_chain_head = (n_policies
+                             && policies[n_policies - 1].chain[0]);
+
       for (int i = 0; i < n_policies; i++) {
+        /* Print chain heading */
+        if (print_chain_head
+            && (i == 0
+                || strcmp(chain_current, policies[i].chain))) {
+            chain_current = policies[i].chain;
+            if (*chain_current) {
+                ds_put_format(&ctx->output, "\nChain %s:\n",
chain_current);
+            } else {
+                ds_put_format(&ctx->output, "Chain <Default>:\n");
+            }
+        }
+
           print_routing_policy(policies[i].policy, &ctx->output);
       }
+
       free(policies);
   }
   @@ -8091,10 +8143,11 @@ static const struct ctl_command_syntax
nbctl_commands[] = {
       /* Policy commands */
       { "lr-policy-add", 4, INT_MAX,
        "ROUTER PRIORITY MATCH ACTION [NEXTHOP] [OPTIONS - KEY=VALUE
...]",
-     nbctl_pre_lr_policy_add, nbctl_lr_policy_add, NULL,
"--may-exist,--bfd?",
-     RW },
+     nbctl_pre_lr_policy_add, nbctl_lr_policy_add, NULL,
+     "--may-exist,--bfd?,--chain=", RW },
       { "lr-policy-del", 1, 3, "ROUTER [{PRIORITY | UUID} [MATCH]]",
-      nbctl_pre_lr_policy_del, nbctl_lr_policy_del, NULL,
"--if-exists", RW },
+      nbctl_pre_lr_policy_del, nbctl_lr_policy_del, NULL,
+      "--if-exists,--chain=", RW },
       { "lr-policy-list", 1, 1, "ROUTER", nbctl_pre_lr_policy_list,
         nbctl_lr_policy_list, NULL, "", RO },



_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev