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

Mon, 03 Feb 2025 13:13:05 -0800 

Hi Aleksandr,
Sorry for the delayed review. I was out sick three days last week and am just back in the office today. 

I have some findings on this new patch.

On 1/23/25 08:51, 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,
according to their priorities. If match occurs on rule having the 'jump'
action, a traversal restarts for rules that belongs to that chain, again,
according 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. Generate router policy flow in the following manner:
       lr_in_ip_routing_ecmp  (not changed)
       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 <[email protected]>
---
  northd/en-northd.c    |   2 +-
  northd/northd.c       |  72 ++++++++++++++++-
  ovn-nb.ovsschema      |   9 ++-
  ovn-nb.xml            |  22 ++++-
  tests/ovn-nbctl.at    |  54 +++++++++++--
  tests/ovn-northd.at   |  27 +++++++
  tests/ovn.at          |  93 +++++++++++++++++++++
  utilities/ovn-nbctl.c | 183 +++++++++++++++++++++++++++---------------
  8 files changed, 383 insertions(+), 79 deletions(-)
---
  v2: Addressed Mark's comments.

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 @@
-    /*
+/*


This change is irrelevant to the patch series.


   * 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 74014de32..f759f603e 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);
+    }



policy_chain_id() can return 0 to mean either that the rule has no 
configured chain, or that the chain couldn't be found in the chain_ids 
simap. This can cause some bad matches to be formed in the latter case.



See my paragraphs down below about changes to when policy chain IDs are 
generated to see how I think this can best be avoided.



  
      ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_POLICY, rule->priority,

                              ds_cstr(&match), ds_cstr(&actions), stage_hint,
@@ -14150,6 +14198,19 @@ 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);
+    }



After looking at the existing code more thoroughly, chain_id is a 
property of a logical router policy, so chain ID generation belongs in 
the build_route_policies() function. This function is called by the 
en_route_policies incremental processing node in northd/en-northd.c ( 
specifically in en_route_policies_run() ).



To do this, you can modify the struct route_policies_data in 
northd/northd.h to contain the chain_ids simap. Initialize the chain_ids 
in route_policies_init() in northd/northd.c, and destroy the chain_ids 
in route_policies_destroy() in northd/northd.c. Pass this simap as a 
parameter to build_route_policies().



Then modify the struct route_policy in northd/northd.h to have an 
unsigned_int chain_id as part of the structure. Calculate the chain_id 
in build_route_policies(). If a chain_id could be successfully chosen, 
then assign that ID to the policy. Critically, if a chain ID could not 
be successfully chosen, then the route policy as a whole is invalid. The 
policy should NOT be added to the route_policies hmap, and the candidate 
route policy should be freed.



Doing it this way has some advantages over the way it is implemented in 
this patch:
1) It removes invalid router policies as inputs to northd logical flow 
generation (e.g. if the chain ID would end up being > UINT16_MAX).
2) If we add more incremental processing to route policies in the 
future, then we already have the data where it needs to be to ensure 
route policy IDs are processed incrementally.
3) It places the chain_id directly on the route_policy structure, so 
there is no need to perform lookups in an simap when we want to refer to 
the chain_id when creating logical flows.



+
      /* 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",
@@ -14163,7 +14224,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 +14237,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/ovn-nb.ovsschema b/ovn-nb.ovsschema
index e7aa6b2b1..0ef097850 100644
--- a/ovn-nb.ovsschema
+++ b/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
  {
      "name": "OVN_Northbound",
-    "version": "7.9.0",
-    "cksum": "2414335430 38682",
+    "version": "7.9.1",
+    "cksum": "3638697906 38821",
      "tables": {
          "NB_Global": {
              "columns": {
@@ -524,10 +524,13 @@
                  "priority": {"type": {"key": {"type": "integer",
                                                "minInteger": 0,
                                                "maxInteger": 32767}}},
+                "chain": {"type": "string"},



The "chain" and "jump_chain" columns are defined in a way such that they 
are always non-NULL. Since they are optional elements on a router 
policy, they should be defined as:


{"type": {"key": "string", "min": 0, "max": 1}}


                  "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 24ef12f3b..5fbcf1f33 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -3951,7 +3951,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>

  
@@ -3986,9 +3994,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 d4f61cb77..7d5c7d8a9 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 edfd5764b..2333c113e 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -3533,6 +3533,33 @@ sed 's/reg8\[[0..15\]] == [[0-9]]*/reg8\[[0..15\]] == 
<cleared>/' | ovn_strip_lf
  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
+])
+
  OVN_FOR_EACH_NORTHD_NO_HV([
  AT_SETUP([ACL allow-stateless omit conntrack - Logical_Switch])
  ovn_start
diff --git a/tests/ovn.at b/tests/ovn.at
index f0eca6ae4..625996302 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -8205,6 +8205,99 @@ echo $expected | ovstest test-ovn expr-to-packets > 
3.expected

  
  OVN_CHECK_PACKETS([pbr-hv/vif3-tx.pcap], [3.expected])
  
+#

+# Test routing policy chains
+#
+
+# Create 3 chains: any drop, any allow, any reroute
+check ovn-nbctl --wait=hv lr-policy-del R1
+check ovn-nbctl --wait=hv --chain anydrop lr-policy-add R1 111 "1" drop
+check ovn-nbctl --wait=hv --chain anyallow lr-policy-add R1 112 "1" allow
+check ovn-nbctl --wait=hv --chain anyreroute lr-policy-add R1 113 "1" reroute 
20.20.1.2
+
+reset_pcap_file vif2 pbr-hv/vif2
+reset_pcap_file vif3 pbr-hv/vif3
+
+# Create a policy jumping to drop chain
+check ovn-nbctl --wait=hv lr-policy-add R1 201 "ip4.src==192.168.1.0/24 && 
ip4.dst==172.16.1.0/24" jump anydrop
+
+# Check logical flow
+AT_CHECK([ovn-sbctl dump-flows | grep lr_in_policy | grep "192.168.1.0" | wc 
-l], [0], [dnl
+1
+])
+
+# Send packet.
+packet="inport==\"ls1-lp1\" && eth.src==$ls1_p1_mac && eth.dst==$ls1_ro_mac &&
+       ip4 && ip.ttl==64 && ip4.src==$ls1_p1_ip && ip4.dst==$ls2_p1_ip &&
+       udp && udp.src==53 && udp.dst==4369"
+
+OVS_WAIT_UNTIL([as pbr-hv ovs-appctl -t ovn-controller inject-pkt "$packet"])
+
+# Check if packet hit the drop policy
+OVS_WAIT_UNTIL([test "1" = "$(ovs-ofctl dump-flows br-int | \
+    grep "actions=drop" | \
+    grep "priority=111" | grep "n_packets=1" -c)"])
+
+# Expected to drop the packet.
+$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" pbr-hv/vif2-tx.pcap > vif2.packets
+rcvd_packet=`cat vif2.packets`
+
+AT_FAIL_IF([test "$rcvd_packet" != ""])
+
+# Create a priority policy jumping to allow chain
+check ovn-nbctl --wait=hv lr-policy-add R1 301 "ip4.src==192.168.1.0/24 && 
ip4.dst==172.16.1.0/24"  jump anyallow
+
+# Check logical flow
+AT_CHECK([ovn-sbctl dump-flows | grep lr_in_policy | grep "192.168.1.0" -c], 
[0], [dnl
+2
+])
+
+# Send packet.
+packet="inport==\"ls1-lp1\" && eth.src==$ls1_p1_mac && eth.dst==$ls1_ro_mac &&
+       ip4 && ip.ttl==64 && ip4.src==$ls1_p1_ip && ip4.dst==$ls2_p1_ip &&
+       udp && udp.src==53 && udp.dst==4369"
+OVS_WAIT_UNTIL([as pbr-hv ovs-appctl -t ovn-controller inject-pkt "$packet"])
+
+# Check if packet hit the allow policy
+OVS_WAIT_UNTIL([test "1" = "$(ovs-ofctl dump-flows br-int | \
+    grep "priority=112" | grep "n_packets=1" -c)"])
+
+# Expected packet has TTL decreased by 1
+expected="eth.src==$ls2_ro_mac && eth.dst==$ls2_p1_mac &&
+       ip4 && ip.ttl==63 && ip4.src==$ls1_p1_ip && ip4.dst==$ls2_p1_ip &&
+       udp && udp.src==53 && udp.dst==4369"
+echo $expected | ovstest test-ovn expr-to-packets > expected
+
+OVN_CHECK_PACKETS([pbr-hv/vif2-tx.pcap], [expected])
+
+# Create a priority policy jumping to reroute chain
+check ovn-nbctl --wait=hv lr-policy-add R1 401 "ip4.src==192.168.1.0/24 && 
ip4.dst==172.16.1.0/24"  jump anyreroute
+
+# Check logical flow
+AT_CHECK([ovn-sbctl dump-flows | grep lr_in_policy | \
+    grep "192.168.1.0" | \
+    grep "priority=30" -c], [0], [dnl
+1
+])
+
+# Send packet.
+packet="inport==\"ls1-lp1\" && eth.src==$ls1_p1_mac && eth.dst==$ls1_ro_mac &&
+       ip4 && ip.ttl==64 && ip4.src==$ls1_p1_ip && ip4.dst==$ls2_p1_ip &&
+       udp && udp.src==53 && udp.dst==4369"
+OVS_WAIT_UNTIL([as pbr-hv ovs-appctl -t ovn-controller inject-pkt "$packet"])
+
+# Check if packet hit the allow policy
+OVS_WAIT_UNTIL([test "1" = "$(ovs-ofctl dump-flows br-int | \
+    grep "priority=113" | grep "n_packets=1" -c)"])
+
+# Expected packet has TTL decreased by 1
+expected="eth.src==$ls3_ro_mac && eth.dst==$ls3_p1_mac &&
+       ip4 && ip.ttl==63 && ip4.src==$ls1_p1_ip && ip4.dst==$ls2_p1_ip &&
+       udp && udp.src==53 && udp.dst==4369"
+echo $expected | ovstest test-ovn expr-to-packets > 3.expected
+
+OVN_CHECK_PACKETS([pbr-hv/vif3-tx.pcap], [3.expected])
+
  OVN_CLEANUP([pbr-hv])
  AT_CLEANUP
  ])
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index f5277af7c..7b93d6dc2 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++) {
          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);
  }

  
@@ -8094,10 +8146,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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev























					Reply via email to