[ovs-dev] Logical Router Policy chains.

This is RFC proposal, the code below is a working prototype, unit test is not ready to look into.

1. Introduction

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' 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 meaning of 'goto' but not 'call/return').

3. Change of utilities

ovn-nbctl is changed to accept and report new options.

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 chain name in first column
    to print jump actions

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 one 16 bit register to hold chain ID numeric value
       designating current chain to traverse (here for example Rx[0..15])
    2. Reserve new stage at LR, ingress, to make initial assignment to Rx[0..15] := 0
    3. Generate router policy flow in the following manner:
        3.1 lr_in_ip_routing_ecmp  (not changed)
        3.2 lr_in_policy_pre
                match:  1
                actoin: Rx[0..15] = 0; next;
        3.3 lr_in_policy
                match: (Rx[0..15] == <chain_id>) && (<nb:match>)
                action: <nb:action_not_jump>
                        | 'Rx[0..15] = <jump_chain_id>; next(ingress, lr_in_policy);'

Signed-off-by: Aleksandr Smirnov <alekssmirnov@k2.cloud>
---
RFC: First post.
---
 northd/northd.c       | 68 ++++++++++++++++++++++++++++++++++++++++---
 northd/northd.h       | 19 ++++++------
 ovn-nb.ovsschema      |  8 +++--
 ovn-nb.xml            | 21 ++++++++++++-
 tests/ovn-nbctl.at    | 17 +++++++++--
 tests/ovn-northd.at   | 14 ++++++++-
 utilities/ovn-nbctl.c | 56 ++++++++++++++++++++++++++++-------
 7 files changed, 172 insertions(+), 31 deletions(-)

Bleep bloop.  Greetings Aleksandr Smirnov, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 85 characters long (recommended limit is 79)
#268 FILE: ovn-nb.ovsschema:525:
                            "enum": ["set", ["allow", "drop", "reroute", "jump"]]}}},

WARNING: Line is 82 characters long (recommended limit is 79)
#288 FILE: ovn-nb.xml:3915:
        The routing policy rule's chain name. Only rules with empty chain name are

WARNING: Line is 80 characters long (recommended limit is 79)
#495 FILE: utilities/ovn-nbctl.c:4494:
    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_policy_col_jump_chain);

WARNING: Line is 87 characters long (recommended limit is 79)
#512 FILE: utilities/ovn-nbctl.c:8139:
     nbctl_pre_lr_policy_add, nbctl_lr_policy_add, NULL, "--may-exist,--bfd?,--chain=",

Lines checked: 519, Warnings: 4, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot

On Mon, Oct 21, 2024 at 7:03 AM Aleksandr Smirnov <alekssmirnov@k2.cloud> wrote:
>
> This is RFC proposal, the code below is a working prototype, unit test is not ready to look into.
>
> 1. Introduction
>
> 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' 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 meaning of 'goto' but not 'call/return').
>
> 3. Change of utilities
>
> ovn-nbctl is changed to accept and report new options.
>
> 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 chain name in first column
>     to print jump actions
>
> 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 one 16 bit register to hold chain ID numeric value
>        designating current chain to traverse (here for example Rx[0..15])
>     2. Reserve new stage at LR, ingress, to make initial assignment to Rx[0..15] := 0
>     3. Generate router policy flow in the following manner:
>         3.1 lr_in_ip_routing_ecmp  (not changed)
>         3.2 lr_in_policy_pre
>                 match:  1
>                 actoin: Rx[0..15] = 0; next;
>         3.3 lr_in_policy
>                 match: (Rx[0..15] == <chain_id>) && (<nb:match>)
>                 action: <nb:action_not_jump>
>                         | 'Rx[0..15] = <jump_chain_id>; next(ingress, lr_in_policy);'
>
> Signed-off-by: Aleksandr Smirnov <alekssmirnov@k2.cloud>
> ---
> RFC: First post.

Thanks Aleksandr. This idea looks good to me in general. I didn't
review in much detail since it is RFC. Just some reminders:
- We need to document the register usage carefully and make sure it
does not conflict with other features for the router pipeline.
- Do we need a mechanism to avoid looping forever in the chains?
Probably by maintaining a counter of jumps and check if it reaches the
max allowed count then drop?

Regards,
Han

> ---
>  northd/northd.c       | 68 ++++++++++++++++++++++++++++++++++++++++---
>  northd/northd.h       | 19 ++++++------
>  ovn-nb.ovsschema      |  8 +++--
>  ovn-nb.xml            | 21 ++++++++++++-
>  tests/ovn-nbctl.at    | 17 +++++++++--
>  tests/ovn-northd.at   | 14 ++++++++-
>  utilities/ovn-nbctl.c | 56 ++++++++++++++++++++++++++++-------
>  7 files changed, 172 insertions(+), 31 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 0fe15ac59..0639b4881 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -189,6 +189,7 @@ BUILD_ASSERT_DECL(ACL_OBS_STAGE_MAX < (1 << 2));
>  #define REG_SRC_IPV4 "reg1"
>  #define REG_SRC_IPV6 "xxreg1"
>  #define REG_DHCP_RELAY_DIP_IPV4 "reg2"
> +#define REG_POLICY_CHAIN_ID "reg6[0..15]"
>  #define REG_ROUTE_TABLE_ID "reg7"
>
>  /* Register used to store backend ipv6 address
> @@ -10756,11 +10757,50 @@ static bool check_bfd_state(const struct nbrec_logical_router_policy *rule,
>      return true;
>  }
>
> +
> +
> +static uint32_t
> +policy_chain_add(struct simap *chain_ids,
> +                 const char *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 0;
> +    }
> +
> +    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");
> +    }
> +
> +    return id;
> +}
> +
> +static uint32_t
> +policy_chain_id(struct simap *chain_ids,
> +                const char *chain_name)
> +{
> +    if (!chain_name || !chain_name[0]) {
> +        return 0;
> +    }
> +
> +    uint32_t id = simap_get(chain_ids, chain_name);
> +    if (!id) {
> +        id = policy_chain_add(chain_ids, chain_name);
> +    }
> +
> +    return id;
> +}
> +
>  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;
> @@ -10808,7 +10848,12 @@ build_routing_policy_flow(struct lflow_table *lflows, struct ovn_datapath *od,
>                        lrp_addr_s,
>                        out_port->lrp_networks.ea_s,
>                        out_port->json_key);
> -
> +    } else if (!strcmp(rule->action, "jump")) {
> +        ds_put_format(&actions,
> +                      "%s=%d; next(pipeline=ingress,table=%d);",
> +                      REG_POLICY_CHAIN_ID,
> +                      policy_chain_id(chain_ids, rule->jump_chain),
> +                      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")) {
> @@ -10818,7 +10863,9 @@ 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);
> +
> +    ds_put_format(&match, "%s == %d && (%s)", REG_POLICY_CHAIN_ID,
> +                  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,
> @@ -13804,9 +13851,15 @@ build_ingress_policy_flows_for_lrouter(
>      ovs_assert(od->nbr);
>      /* 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);
> @@ -13816,6 +13869,10 @@ build_ingress_policy_flows_for_lrouter(
>      /* Convert routing policies to flows. */
>      uint16_t ecmp_group_id = 1;
>      struct route_policy *rp;
> +    struct simap chain_ids;
> +
> +    simap_init(&chain_ids);
> +
>      HMAP_FOR_EACH_WITH_HASH (rp, key_node, uuid_hash(&od->key),
>                               route_policies) {
>          const struct nbrec_logical_router_policy *rule = rp->rule;
> @@ -13828,9 +13885,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 8f76d642d..c711e2587 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")      \
> +    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 b4a395c56..b81647fa1 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>  {
>      "name": "OVN_Northbound",
> -    "version": "7.6.0",
> -    "cksum": "2171465655 38284",
> +    "version": "7.6.1",
> +    "cksum": "2213857026 38387",
>      "tables": {
>          "NB_Global": {
>              "columns": {
> @@ -518,10 +518,12 @@
>                  "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 2836f58f5..4476c7e7e 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -3906,7 +3906,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. Others chains are traversed as responce to
> +        jump action.
>        </p>
>      </column>
>
> @@ -3941,9 +3949,20 @@ or
>            <code>reroute</code>: Reroute packet to <ref column="nexthop"/> or
>            <ref column="nexthops"/>.
>          </li>
> +
> +        <li>
> +          <code>jump</code>: Restart rules traversal for those having chain
> +          name equal to <ref column="jump_chain"/>.
> +        </li>
>        </ul>
>      </column>
>
> +    <column name="jump_chain">
> +      <p>
> +        The routing policy rule's chain name selected to traverse.
> +      </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 2efa13b93..7795d5ed4 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -2288,11 +2288,22 @@ OVN_NBCTL_TEST([ovn_nbctl_policies], [policies], [
>  AT_CHECK([ovn-nbctl lr-add lr0])
>
>  dnl Add policies with allow and drop 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 --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" allow])
> +AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.2.0/24" jump "inbound"])
> +AT_CHECK([ovn-nbctl --chain "inbound" lr-policy-add lr0 101 "ip4.src == 1.1.2.0/24" allow pkt_mark=100,foo=bar])
> +AT_CHECK([ovn-nbctl lr-policy-add lr0 11 "ip6.src == 2002::/64" jump "outbound"])
> +
> +ovn-nbctl list Logical_Router_Policy
> +ovn-nbctl lr-policy-list lr0
> +
>  AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.1.0/24" allow])
>  AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.2.0/24" drop])
> -AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip6.src == 2002::/64" drop])
> +
> +
> +
> +
> +
>  AT_CHECK([ovn-nbctl --bfd lr-policy-add lr0 103 "ip4.src == 1.2.3.0/24" reroute 192.168.1.1], [1], [],
>    [ovn-nbctl: out lrp not found for 192.168.1.1 nexthop
>  ])
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index d6a8c4640..f423242b9 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -3419,7 +3419,19 @@ check ovn-nbctl lsp-set-type public-lr0 router
>  check ovn-nbctl lsp-set-addresses public-lr0 router
>  check ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public
>
> -check ovn-nbctl --wait=sb lr-policy-add lr0  10 "ip4.src == 10.0.0.3" reroute 172.168.0.101,172.168.0.102
> +###check ovn-nbctl --wait=sb lr-policy-add lr0  10 "ip4.src == 10.0.0.3" reroute 172.168.0.101,172.168.0.102
> +
> +AT_CHECK([ovn-nbctl --chain "inbound" lr-policy-add lr0 100 "ip4.src == 1.1.1.0/24" drop])
> +AT_CHECK([ovn-nbctl --chain "inbound" lr-policy-add lr0 100 "ip4.src == 1.1.2.0/24" allow pkt_mark=100,foo=bar])
> +AT_CHECK([ovn-nbctl --chain "outbound" lr-policy-add lr0 101 "ip4.src == 2.1.1.0/24" allow])
> +AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.2.0/24" jump "inbound"])
> +AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip6.src == 2002::/64" jump "outbound"])
> +
> +ovn-nbctl list Logical_Router_Policy
> +
> +sleep 1
> +
> +ovn-sbctl lflow-list lr0
>
>  ovn-nbctl lr-policy-list lr0 > policy-list
>  AT_CAPTURE_FILE([policy-list])
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index d45be75c7..13277db66 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -4116,11 +4116,13 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
>      char **nexthops = NULL;
>
>      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,6 +4133,14 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
>          reroute = true;
>      }
>
> +    if (!strcmp(action, "jump")) {
> +        if (ctx->argc < 6) {
> +            ctl_error(ctx, "Chain number 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");
> @@ -4195,11 +4205,22 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
>          free(nexthops_arg);
>      }
>
> +    const char *chain_s = shash_find_data(&ctx->options, "--chain");
> +
>      struct nbrec_logical_router_policy *policy;
>      policy = nbrec_logical_router_policy_insert(ctx->txn);
>      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 +4228,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, "=");
> @@ -4394,9 +4415,10 @@ nbctl_lr_policy_del(struct ctl_context *ctx)
>      }
>  }
>
> - struct routing_policy {
> +struct routing_policy {
>      int priority;
>      char *match;
> +    char *chain;
>      const struct nbrec_logical_router_policy *policy;
>  };
>
> @@ -4405,6 +4427,13 @@ 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 +4445,21 @@ static void
>  print_routing_policy(const struct nbrec_logical_router_policy *policy,
>                       struct ds *s)
>  {
> +    ds_put_format(s, "%25s %10"PRId64" %50s %15s",
> +                  (*policy->chain) ? policy->chain : "",
> +                  policy->priority, policy->match, policy->action);
> +
> +    if (strcmp(policy->action, "jump") == 0
> +        && *policy->jump_chain) {
> +        ds_put_format(s, " %s", policy->jump_chain);
> +    }
> +
>      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);
>      }
>
>      if (!smap_is_empty(&policy->options) || policy->n_bfd_sessions) {
> @@ -4457,6 +4490,8 @@ 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 +4511,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;
>          policies[n_policies].policy = policy;
>          n_policies++;
>      }
> @@ -8100,7 +8136,7 @@ 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?",
> +     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 },
> --
> 2.47.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On 10/21/24 9:41 PM, Han Zhou wrote:
> On Mon, Oct 21, 2024 at 7:03 AM Aleksandr Smirnov <alekssmirnov@k2.cloud> wrote:
>> This is RFC proposal, the code below is a working prototype, unit test is not ready to look into.
>>
>> 1. Introduction
>>
>> 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' 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 meaning of 'goto' but not 'call/return').
>>
>> 3. Change of utilities
>>
>> ovn-nbctl is changed to accept and report new options.
>>
>> 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 chain name in first column
>>      to print jump actions
>>
>> 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 one 16 bit register to hold chain ID numeric value
>>         designating current chain to traverse (here for example Rx[0..15])
>>      2. Reserve new stage at LR, ingress, to make initial assignment to Rx[0..15] := 0
>>      3. Generate router policy flow in the following manner:
>>          3.1 lr_in_ip_routing_ecmp  (not changed)
>>          3.2 lr_in_policy_pre
>>                  match:  1
>>                  actoin: Rx[0..15] = 0; next;
>>          3.3 lr_in_policy
>>                  match: (Rx[0..15] == <chain_id>) && (<nb:match>)
>>                  action: <nb:action_not_jump>
>>                          | 'Rx[0..15] = <jump_chain_id>; next(ingress, lr_in_policy);'
>>
>> Signed-off-by: Aleksandr Smirnov <alekssmirnov@k2.cloud>
>> ---
>> RFC: First post.
> Thanks Aleksandr. This idea looks good to me in general. I didn't
> review in much detail since it is RFC. Just some reminders:
> - We need to document the register usage carefully and make sure it
> does not conflict with other features for the router pipeline.
> - Do we need a mechanism to avoid looping forever in the chains?
> Probably by maintaining a counter of jumps and check if it reaches the
> max allowed count then drop?
>
> Regards,
> Han

Hello Han,

I apologise for late answer, I has been urgently assigned to find/fix 
openvswitch kmod kernel crash.

Answering your questions:

1. I looked into northd register usage and see that

    REG_ORIG_TP_DPORT_ROUTER   "reg9[16..31]"

   could be reused to policing purposes.

   I see no other options because xxreg0, xxreg1 are in-use, reg8 is 
also in-use for ECMP processing.

   However, I have a question: I see there are sixteen registers defined 
but northd is using only reg0..reg9.

   Why it is? Can we use other registers?

2. Seems we already have flow loop protection in the vSwitch. Here is my 
findings from OVS:

 >>While processing a given packet, Open vSwitch limits the flow table 
recursion depth to 64, to en-
 >>sure that packet processing uses a finite amount of time and space. 
Actions that count against the
 >>recursion limit include resubmit from a given OpenFlow table to the 
same or an earlier table,
 >>group, and output to patch ports.

Thus, I think I do not need to provde extra care against infinite flow loops


Thank you,

Alexander


>> ---
>>   northd/northd.c       | 68 ++++++++++++++++++++++++++++++++++++++++---
>>   northd/northd.h       | 19 ++++++------
>>   ovn-nb.ovsschema      |  8 +++--
>>   ovn-nb.xml            | 21 ++++++++++++-
>>   tests/ovn-nbctl.at    | 17 +++++++++--
>>   tests/ovn-northd.at   | 14 ++++++++-
>>   utilities/ovn-nbctl.c | 56 ++++++++++++++++++++++++++++-------
>>   7 files changed, 172 insertions(+), 31 deletions(-)
>>
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 0fe15ac59..0639b4881 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -189,6 +189,7 @@ BUILD_ASSERT_DECL(ACL_OBS_STAGE_MAX < (1 << 2));
>>   #define REG_SRC_IPV4 "reg1"
>>   #define REG_SRC_IPV6 "xxreg1"
>>   #define REG_DHCP_RELAY_DIP_IPV4 "reg2"
>> +#define REG_POLICY_CHAIN_ID "reg6[0..15]"
>>   #define REG_ROUTE_TABLE_ID "reg7"
>>
>>   /* Register used to store backend ipv6 address
>> @@ -10756,11 +10757,50 @@ static bool check_bfd_state(const struct nbrec_logical_router_policy *rule,
>>       return true;
>>   }
>>
>> +
>> +
>> +static uint32_t
>> +policy_chain_add(struct simap *chain_ids,
>> +                 const char *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 0;
>> +    }
>> +
>> +    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");
>> +    }
>> +
>> +    return id;
>> +}
>> +
>> +static uint32_t
>> +policy_chain_id(struct simap *chain_ids,
>> +                const char *chain_name)
>> +{
>> +    if (!chain_name || !chain_name[0]) {
>> +        return 0;
>> +    }
>> +
>> +    uint32_t id = simap_get(chain_ids, chain_name);
>> +    if (!id) {
>> +        id = policy_chain_add(chain_ids, chain_name);
>> +    }
>> +
>> +    return id;
>> +}
>> +
>>   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;
>> @@ -10808,7 +10848,12 @@ build_routing_policy_flow(struct lflow_table *lflows, struct ovn_datapath *od,
>>                         lrp_addr_s,
>>                         out_port->lrp_networks.ea_s,
>>                         out_port->json_key);
>> -
>> +    } else if (!strcmp(rule->action, "jump")) {
>> +        ds_put_format(&actions,
>> +                      "%s=%d; next(pipeline=ingress,table=%d);",
>> +                      REG_POLICY_CHAIN_ID,
>> +                      policy_chain_id(chain_ids, rule->jump_chain),
>> +                      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")) {
>> @@ -10818,7 +10863,9 @@ 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);
>> +
>> +    ds_put_format(&match, "%s == %d && (%s)", REG_POLICY_CHAIN_ID,
>> +                  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,
>> @@ -13804,9 +13851,15 @@ build_ingress_policy_flows_for_lrouter(
>>       ovs_assert(od->nbr);
>>       /* 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);
>> @@ -13816,6 +13869,10 @@ build_ingress_policy_flows_for_lrouter(
>>       /* Convert routing policies to flows. */
>>       uint16_t ecmp_group_id = 1;
>>       struct route_policy *rp;
>> +    struct simap chain_ids;
>> +
>> +    simap_init(&chain_ids);
>> +
>>       HMAP_FOR_EACH_WITH_HASH (rp, key_node, uuid_hash(&od->key),
>>                                route_policies) {
>>           const struct nbrec_logical_router_policy *rule = rp->rule;
>> @@ -13828,9 +13885,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 8f76d642d..c711e2587 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")      \
>> +    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 b4a395c56..b81647fa1 100644
>> --- a/ovn-nb.ovsschema
>> +++ b/ovn-nb.ovsschema
>> @@ -1,7 +1,7 @@
>>   {
>>       "name": "OVN_Northbound",
>> -    "version": "7.6.0",
>> -    "cksum": "2171465655 38284",
>> +    "version": "7.6.1",
>> +    "cksum": "2213857026 38387",
>>       "tables": {
>>           "NB_Global": {
>>               "columns": {
>> @@ -518,10 +518,12 @@
>>                   "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 2836f58f5..4476c7e7e 100644
>> --- a/ovn-nb.xml
>> +++ b/ovn-nb.xml
>> @@ -3906,7 +3906,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. Others chains are traversed as responce to
>> +        jump action.
>>         </p>
>>       </column>
>>
>> @@ -3941,9 +3949,20 @@ or
>>             <code>reroute</code>: Reroute packet to <ref column="nexthop"/> or
>>             <ref column="nexthops"/>.
>>           </li>
>> +
>> +        <li>
>> +          <code>jump</code>: Restart rules traversal for those having chain
>> +          name equal to <ref column="jump_chain"/>.
>> +        </li>
>>         </ul>
>>       </column>
>>
>> +    <column name="jump_chain">
>> +      <p>
>> +        The routing policy rule's chain name selected to traverse.
>> +      </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 2efa13b93..7795d5ed4 100644
>> --- a/tests/ovn-nbctl.at
>> +++ b/tests/ovn-nbctl.at
>> @@ -2288,11 +2288,22 @@ OVN_NBCTL_TEST([ovn_nbctl_policies], [policies], [
>>   AT_CHECK([ovn-nbctl lr-add lr0])
>>
>>   dnl Add policies with allow and drop 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 --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" allow])
>> +AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.2.0/24" jump "inbound"])
>> +AT_CHECK([ovn-nbctl --chain "inbound" lr-policy-add lr0 101 "ip4.src == 1.1.2.0/24" allow pkt_mark=100,foo=bar])
>> +AT_CHECK([ovn-nbctl lr-policy-add lr0 11 "ip6.src == 2002::/64" jump "outbound"])
>> +
>> +ovn-nbctl list Logical_Router_Policy
>> +ovn-nbctl lr-policy-list lr0
>> +
>>   AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.1.0/24" allow])
>>   AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.2.0/24" drop])
>> -AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip6.src == 2002::/64" drop])
>> +
>> +
>> +
>> +
>> +
>>   AT_CHECK([ovn-nbctl --bfd lr-policy-add lr0 103 "ip4.src == 1.2.3.0/24" reroute 192.168.1.1], [1], [],
>>     [ovn-nbctl: out lrp not found for 192.168.1.1 nexthop
>>   ])
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index d6a8c4640..f423242b9 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -3419,7 +3419,19 @@ check ovn-nbctl lsp-set-type public-lr0 router
>>   check ovn-nbctl lsp-set-addresses public-lr0 router
>>   check ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public
>>
>> -check ovn-nbctl --wait=sb lr-policy-add lr0  10 "ip4.src == 10.0.0.3" reroute 172.168.0.101,172.168.0.102
>> +###check ovn-nbctl --wait=sb lr-policy-add lr0  10 "ip4.src == 10.0.0.3" reroute 172.168.0.101,172.168.0.102
>> +
>> +AT_CHECK([ovn-nbctl --chain "inbound" lr-policy-add lr0 100 "ip4.src == 1.1.1.0/24" drop])
>> +AT_CHECK([ovn-nbctl --chain "inbound" lr-policy-add lr0 100 "ip4.src == 1.1.2.0/24" allow pkt_mark=100,foo=bar])
>> +AT_CHECK([ovn-nbctl --chain "outbound" lr-policy-add lr0 101 "ip4.src == 2.1.1.0/24" allow])
>> +AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.2.0/24" jump "inbound"])
>> +AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip6.src == 2002::/64" jump "outbound"])
>> +
>> +ovn-nbctl list Logical_Router_Policy
>> +
>> +sleep 1
>> +
>> +ovn-sbctl lflow-list lr0
>>
>>   ovn-nbctl lr-policy-list lr0 > policy-list
>>   AT_CAPTURE_FILE([policy-list])
>> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
>> index d45be75c7..13277db66 100644
>> --- a/utilities/ovn-nbctl.c
>> +++ b/utilities/ovn-nbctl.c
>> @@ -4116,11 +4116,13 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
>>       char **nexthops = NULL;
>>
>>       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,6 +4133,14 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
>>           reroute = true;
>>       }
>>
>> +    if (!strcmp(action, "jump")) {
>> +        if (ctx->argc < 6) {
>> +            ctl_error(ctx, "Chain number 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");
>> @@ -4195,11 +4205,22 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
>>           free(nexthops_arg);
>>       }
>>
>> +    const char *chain_s = shash_find_data(&ctx->options, "--chain");
>> +
>>       struct nbrec_logical_router_policy *policy;
>>       policy = nbrec_logical_router_policy_insert(ctx->txn);
>>       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 +4228,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, "=");
>> @@ -4394,9 +4415,10 @@ nbctl_lr_policy_del(struct ctl_context *ctx)
>>       }
>>   }
>>
>> - struct routing_policy {
>> +struct routing_policy {
>>       int priority;
>>       char *match;
>> +    char *chain;
>>       const struct nbrec_logical_router_policy *policy;
>>   };
>>
>> @@ -4405,6 +4427,13 @@ 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 +4445,21 @@ static void
>>   print_routing_policy(const struct nbrec_logical_router_policy *policy,
>>                        struct ds *s)
>>   {
>> +    ds_put_format(s, "%25s %10"PRId64" %50s %15s",
>> +                  (*policy->chain) ? policy->chain : "",
>> +                  policy->priority, policy->match, policy->action);
>> +
>> +    if (strcmp(policy->action, "jump") == 0
>> +        && *policy->jump_chain) {
>> +        ds_put_format(s, " %s", policy->jump_chain);
>> +    }
>> +
>>       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);
>>       }
>>
>>       if (!smap_is_empty(&policy->options) || policy->n_bfd_sessions) {
>> @@ -4457,6 +4490,8 @@ 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 +4511,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;
>>           policies[n_policies].policy = policy;
>>           n_policies++;
>>       }
>> @@ -8100,7 +8136,7 @@ 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?",
>> +     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 },
>> --
>> 2.47.0
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Wed, Nov 6, 2024 at 9:06 AM Aleksandr Smirnov (K2 Cloud)
<AleksSmirnov@k2.cloud> wrote:
>
> On 10/21/24 9:41 PM, Han Zhou wrote:
> > On Mon, Oct 21, 2024 at 7:03 AM Aleksandr Smirnov <alekssmirnov@k2.cloud> wrote:
> >> This is RFC proposal, the code below is a working prototype, unit test is not ready to look into.
> >>
> >> 1. Introduction
> >>
> >> 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' 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 meaning of 'goto' but not 'call/return').
> >>
> >> 3. Change of utilities
> >>
> >> ovn-nbctl is changed to accept and report new options.
> >>
> >> 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 chain name in first column
> >>      to print jump actions
> >>
> >> 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 one 16 bit register to hold chain ID numeric value
> >>         designating current chain to traverse (here for example Rx[0..15])
> >>      2. Reserve new stage at LR, ingress, to make initial assignment to Rx[0..15] := 0
> >>      3. Generate router policy flow in the following manner:
> >>          3.1 lr_in_ip_routing_ecmp  (not changed)
> >>          3.2 lr_in_policy_pre
> >>                  match:  1
> >>                  actoin: Rx[0..15] = 0; next;
> >>          3.3 lr_in_policy
> >>                  match: (Rx[0..15] == <chain_id>) && (<nb:match>)
> >>                  action: <nb:action_not_jump>
> >>                          | 'Rx[0..15] = <jump_chain_id>; next(ingress, lr_in_policy);'
> >>
> >> Signed-off-by: Aleksandr Smirnov <alekssmirnov@k2.cloud>
> >> ---
> >> RFC: First post.
> > Thanks Aleksandr. This idea looks good to me in general. I didn't
> > review in much detail since it is RFC. Just some reminders:
> > - We need to document the register usage carefully and make sure it
> > does not conflict with other features for the router pipeline.
> > - Do we need a mechanism to avoid looping forever in the chains?
> > Probably by maintaining a counter of jumps and check if it reaches the
> > max allowed count then drop?
> >
> > Regards,
> > Han
>
> Hello Han,
>
> I apologise for late answer, I has been urgently assigned to find/fix
> openvswitch kmod kernel crash.
>
> Answering your questions:
>
> 1. I looked into northd register usage and see that
>
>     REG_ORIG_TP_DPORT_ROUTER   "reg9[16..31]"
>
>    could be reused to policing purposes.
>
>    I see no other options because xxreg0, xxreg1 are in-use, reg8 is
> also in-use for ECMP processing.
>
>    However, I have a question: I see there are sixteen registers defined
> but northd is using only reg0..reg9.
>
>    Why it is? Can we use other registers?

reg10 onwards is used by ovn-controller to store inport, outport and
conntrack zones.

Numan

>
> 2. Seems we already have flow loop protection in the vSwitch. Here is my
> findings from OVS:
>
>  >>While processing a given packet, Open vSwitch limits the flow table
> recursion depth to 64, to en-
>  >>sure that packet processing uses a finite amount of time and space.
> Actions that count against the
>  >>recursion limit include resubmit from a given OpenFlow table to the
> same or an earlier table,
>  >>group, and output to patch ports.
>
> Thus, I think I do not need to provde extra care against infinite flow loops
>
>
> Thank you,
>
> Alexander
>
>
> >> ---
> >>   northd/northd.c       | 68 ++++++++++++++++++++++++++++++++++++++++---
> >>   northd/northd.h       | 19 ++++++------
> >>   ovn-nb.ovsschema      |  8 +++--
> >>   ovn-nb.xml            | 21 ++++++++++++-
> >>   tests/ovn-nbctl.at    | 17 +++++++++--
> >>   tests/ovn-northd.at   | 14 ++++++++-
> >>   utilities/ovn-nbctl.c | 56 ++++++++++++++++++++++++++++-------
> >>   7 files changed, 172 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/northd/northd.c b/northd/northd.c
> >> index 0fe15ac59..0639b4881 100644
> >> --- a/northd/northd.c
> >> +++ b/northd/northd.c
> >> @@ -189,6 +189,7 @@ BUILD_ASSERT_DECL(ACL_OBS_STAGE_MAX < (1 << 2));
> >>   #define REG_SRC_IPV4 "reg1"
> >>   #define REG_SRC_IPV6 "xxreg1"
> >>   #define REG_DHCP_RELAY_DIP_IPV4 "reg2"
> >> +#define REG_POLICY_CHAIN_ID "reg6[0..15]"
> >>   #define REG_ROUTE_TABLE_ID "reg7"
> >>
> >>   /* Register used to store backend ipv6 address
> >> @@ -10756,11 +10757,50 @@ static bool check_bfd_state(const struct nbrec_logical_router_policy *rule,
> >>       return true;
> >>   }
> >>
> >> +
> >> +
> >> +static uint32_t
> >> +policy_chain_add(struct simap *chain_ids,
> >> +                 const char *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 0;
> >> +    }
> >> +
> >> +    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");
> >> +    }
> >> +
> >> +    return id;
> >> +}
> >> +
> >> +static uint32_t
> >> +policy_chain_id(struct simap *chain_ids,
> >> +                const char *chain_name)
> >> +{
> >> +    if (!chain_name || !chain_name[0]) {
> >> +        return 0;
> >> +    }
> >> +
> >> +    uint32_t id = simap_get(chain_ids, chain_name);
> >> +    if (!id) {
> >> +        id = policy_chain_add(chain_ids, chain_name);
> >> +    }
> >> +
> >> +    return id;
> >> +}
> >> +
> >>   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;
> >> @@ -10808,7 +10848,12 @@ build_routing_policy_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> >>                         lrp_addr_s,
> >>                         out_port->lrp_networks.ea_s,
> >>                         out_port->json_key);
> >> -
> >> +    } else if (!strcmp(rule->action, "jump")) {
> >> +        ds_put_format(&actions,
> >> +                      "%s=%d; next(pipeline=ingress,table=%d);",
> >> +                      REG_POLICY_CHAIN_ID,
> >> +                      policy_chain_id(chain_ids, rule->jump_chain),
> >> +                      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")) {
> >> @@ -10818,7 +10863,9 @@ 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);
> >> +
> >> +    ds_put_format(&match, "%s == %d && (%s)", REG_POLICY_CHAIN_ID,
> >> +                  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,
> >> @@ -13804,9 +13851,15 @@ build_ingress_policy_flows_for_lrouter(
> >>       ovs_assert(od->nbr);
> >>       /* 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);
> >> @@ -13816,6 +13869,10 @@ build_ingress_policy_flows_for_lrouter(
> >>       /* Convert routing policies to flows. */
> >>       uint16_t ecmp_group_id = 1;
> >>       struct route_policy *rp;
> >> +    struct simap chain_ids;
> >> +
> >> +    simap_init(&chain_ids);
> >> +
> >>       HMAP_FOR_EACH_WITH_HASH (rp, key_node, uuid_hash(&od->key),
> >>                                route_policies) {
> >>           const struct nbrec_logical_router_policy *rule = rp->rule;
> >> @@ -13828,9 +13885,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 8f76d642d..c711e2587 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")      \
> >> +    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 b4a395c56..b81647fa1 100644
> >> --- a/ovn-nb.ovsschema
> >> +++ b/ovn-nb.ovsschema
> >> @@ -1,7 +1,7 @@
> >>   {
> >>       "name": "OVN_Northbound",
> >> -    "version": "7.6.0",
> >> -    "cksum": "2171465655 38284",
> >> +    "version": "7.6.1",
> >> +    "cksum": "2213857026 38387",
> >>       "tables": {
> >>           "NB_Global": {
> >>               "columns": {
> >> @@ -518,10 +518,12 @@
> >>                   "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 2836f58f5..4476c7e7e 100644
> >> --- a/ovn-nb.xml
> >> +++ b/ovn-nb.xml
> >> @@ -3906,7 +3906,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. Others chains are traversed as responce to
> >> +        jump action.
> >>         </p>
> >>       </column>
> >>
> >> @@ -3941,9 +3949,20 @@ or
> >>             <code>reroute</code>: Reroute packet to <ref column="nexthop"/> or
> >>             <ref column="nexthops"/>.
> >>           </li>
> >> +
> >> +        <li>
> >> +          <code>jump</code>: Restart rules traversal for those having chain
> >> +          name equal to <ref column="jump_chain"/>.
> >> +        </li>
> >>         </ul>
> >>       </column>
> >>
> >> +    <column name="jump_chain">
> >> +      <p>
> >> +        The routing policy rule's chain name selected to traverse.
> >> +      </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 2efa13b93..7795d5ed4 100644
> >> --- a/tests/ovn-nbctl.at
> >> +++ b/tests/ovn-nbctl.at
> >> @@ -2288,11 +2288,22 @@ OVN_NBCTL_TEST([ovn_nbctl_policies], [policies], [
> >>   AT_CHECK([ovn-nbctl lr-add lr0])
> >>
> >>   dnl Add policies with allow and drop 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 --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" allow])
> >> +AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.2.0/24" jump "inbound"])
> >> +AT_CHECK([ovn-nbctl --chain "inbound" lr-policy-add lr0 101 "ip4.src == 1.1.2.0/24" allow pkt_mark=100,foo=bar])
> >> +AT_CHECK([ovn-nbctl lr-policy-add lr0 11 "ip6.src == 2002::/64" jump "outbound"])
> >> +
> >> +ovn-nbctl list Logical_Router_Policy
> >> +ovn-nbctl lr-policy-list lr0
> >> +
> >>   AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.1.0/24" allow])
> >>   AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.2.0/24" drop])
> >> -AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip6.src == 2002::/64" drop])
> >> +
> >> +
> >> +
> >> +
> >> +
> >>   AT_CHECK([ovn-nbctl --bfd lr-policy-add lr0 103 "ip4.src == 1.2.3.0/24" reroute 192.168.1.1], [1], [],
> >>     [ovn-nbctl: out lrp not found for 192.168.1.1 nexthop
> >>   ])
> >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> >> index d6a8c4640..f423242b9 100644
> >> --- a/tests/ovn-northd.at
> >> +++ b/tests/ovn-northd.at
> >> @@ -3419,7 +3419,19 @@ check ovn-nbctl lsp-set-type public-lr0 router
> >>   check ovn-nbctl lsp-set-addresses public-lr0 router
> >>   check ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public
> >>
> >> -check ovn-nbctl --wait=sb lr-policy-add lr0  10 "ip4.src == 10.0.0.3" reroute 172.168.0.101,172.168.0.102
> >> +###check ovn-nbctl --wait=sb lr-policy-add lr0  10 "ip4.src == 10.0.0.3" reroute 172.168.0.101,172.168.0.102
> >> +
> >> +AT_CHECK([ovn-nbctl --chain "inbound" lr-policy-add lr0 100 "ip4.src == 1.1.1.0/24" drop])
> >> +AT_CHECK([ovn-nbctl --chain "inbound" lr-policy-add lr0 100 "ip4.src == 1.1.2.0/24" allow pkt_mark=100,foo=bar])
> >> +AT_CHECK([ovn-nbctl --chain "outbound" lr-policy-add lr0 101 "ip4.src == 2.1.1.0/24" allow])
> >> +AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.2.0/24" jump "inbound"])
> >> +AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip6.src == 2002::/64" jump "outbound"])
> >> +
> >> +ovn-nbctl list Logical_Router_Policy
> >> +
> >> +sleep 1
> >> +
> >> +ovn-sbctl lflow-list lr0
> >>
> >>   ovn-nbctl lr-policy-list lr0 > policy-list
> >>   AT_CAPTURE_FILE([policy-list])
> >> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> >> index d45be75c7..13277db66 100644
> >> --- a/utilities/ovn-nbctl.c
> >> +++ b/utilities/ovn-nbctl.c
> >> @@ -4116,11 +4116,13 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
> >>       char **nexthops = NULL;
> >>
> >>       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,6 +4133,14 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
> >>           reroute = true;
> >>       }
> >>
> >> +    if (!strcmp(action, "jump")) {
> >> +        if (ctx->argc < 6) {
> >> +            ctl_error(ctx, "Chain number 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");
> >> @@ -4195,11 +4205,22 @@ nbctl_lr_policy_add(struct ctl_context *ctx)
> >>           free(nexthops_arg);
> >>       }
> >>
> >> +    const char *chain_s = shash_find_data(&ctx->options, "--chain");
> >> +
> >>       struct nbrec_logical_router_policy *policy;
> >>       policy = nbrec_logical_router_policy_insert(ctx->txn);
> >>       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 +4228,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, "=");
> >> @@ -4394,9 +4415,10 @@ nbctl_lr_policy_del(struct ctl_context *ctx)
> >>       }
> >>   }
> >>
> >> - struct routing_policy {
> >> +struct routing_policy {
> >>       int priority;
> >>       char *match;
> >> +    char *chain;
> >>       const struct nbrec_logical_router_policy *policy;
> >>   };
> >>
> >> @@ -4405,6 +4427,13 @@ 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 +4445,21 @@ static void
> >>   print_routing_policy(const struct nbrec_logical_router_policy *policy,
> >>                        struct ds *s)
> >>   {
> >> +    ds_put_format(s, "%25s %10"PRId64" %50s %15s",
> >> +                  (*policy->chain) ? policy->chain : "",
> >> +                  policy->priority, policy->match, policy->action);
> >> +
> >> +    if (strcmp(policy->action, "jump") == 0
> >> +        && *policy->jump_chain) {
> >> +        ds_put_format(s, " %s", policy->jump_chain);
> >> +    }
> >> +
> >>       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);
> >>       }
> >>
> >>       if (!smap_is_empty(&policy->options) || policy->n_bfd_sessions) {
> >> @@ -4457,6 +4490,8 @@ 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 +4511,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;
> >>           policies[n_policies].policy = policy;
> >>           n_policies++;
> >>       }
> >> @@ -8100,7 +8136,7 @@ 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?",
> >> +     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 },
> >> --
> >> 2.47.0
> >>
> >> _______________________________________________
> >> dev mailing list
> >> dev@openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev