Hi Mark, I have sent updated version for your review: https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ Could you please continue? I also kindly ask you to confirm that register initialization (with zero value) is not really required.
Thank you, Alexander On 1/22/25 7:13 PM, Mark Michelson wrote: > On 1/22/25 07:31, Aleksandr Smirnov (K2 Cloud) wrote: >> Hi Mark, >> >> Please look at my answers below >> ++Vlad to discuss proposed approach. >> >> On 1/20/25 7:25 PM, Mark Michelson wrote: >>> Hi Aleksandr, thanks for the submission. >>> >>> This submission should have a system test added, to ensure packets >>> behave as expected when policy chains are used. >> >> I see a test in ovn.at (policy-based routing: 1 HVs, 2 LSs, 1 lport/LS, >> 1 LR) that check route policy traffic. >> >> This test is good candidate to extend to check policy chains also. Could >> I modify this one to test policy chains? > > If that will make it easier for you, then I'm totally fine with that. > >> >>> >>> I have some notes and suggestions below. >>> >>> On 12/18/24 05:23, Aleksandr Smirnov wrote: >>>> 1. Objective >>>> >>>> The objective is to enchance routing policies to organize them as >>>> set of >>>> independent lists, here 'chains', and allow to select and traverse >>>> chains >>>> depending on match conditions. >>>> >>>> Every routing policy rule have assigned chain name, a type of >>>> string, or >>>> an empty string. Rule actions have new action 'jump' with string >>>> argument >>>> that allows to switch rule's traversal to given chain. >>>> >>>> 2. New traversal flow. >>>> >>>> The flow starts rule's traversal for only those having an empty chain >>>> name, >>>> accoring to their priorities. If match occurs on rule having the >>>> 'jump' >>> >>> s/accoring/according/ >>> >>> This mistake appears again later in the commit message and should be >>> fixed there as well. >>> >>>> action, a traversal restarts for rules that belongs to that chain, >>>> again, >>>> accoring to their priorities. Any other actions will stop router >>>> policy flow. >>>> (i.e. 'jump' have a meaning of 'goto' but not 'call/return'). >>>> >>>> 3. Change of utilities. >>>> >>>> ovn-nbctl lr-policy-add accepts chain name as optional argument: >>>> ovn-nbctl [--chain Name] lr-policy-add ... >>>> >>>> If omitted, chain name will be empty, i.e. rule added will >>>> belong >>>> to default flow. >>>> >>>> ovn-nbctl lr-policy-add accepts new action 'jump': >>>> ovn-nbctl lr-policy-add ... jump Name >>>> >>>> ovn-nbctl lr-policy-list changes its output format: >>>> to print rules grouped by chain name >>>> to print jump actions >>>> >>>> ovn-nbctl lr-policy-del accepts chain name as optional argument: >>>> ovn-nbctl [--chain Name] lr-policy-del ... >>>> >>>> If --chain is used: >>>> Delete by LR name: Delete whole chain. >>>> Delete by uuid: just delete by uuid. >>>> Delete by priority: Delete rules of this chain and given >>>> priority. >>>> Delete by priority & match: Delete rules of this chain >>>> having given priority and match. >>>> Else (--chain is not used): >>>> Delete by LR name: Delete all rules. >>>> Delete by uuid: just delete by uuid. >>>> Delete by priority: Delete rules of default chain >>>> (i.e. having chain name '') and given >>>> priority. >>>> Delete by priority & match: Delete rules of default chain >>>> having given priority and match. >>>> >>>> 4. Changes in NB DB schema >>>> >>>> Logical_Router_Policy: >>>> New field: >>>> Name: chain >>>> Type: string >>>> New field: >>>> Name: jump_chain >>>> Type: string >>>> Modified field: >>>> Name: actions >>>> Value: add enum value 'jump' >>>> >>>> 5. Change in the SB DB generation >>>> >>>> 1. Reserve (reuse) one 16 bit register reg9[16..31] to hold >>>> chain ID >>>> numeric value designating current chain to traverse. >>>> 2. Reserve new stage (lr_in_policy_pre) at Logical Router >>>> ingress >>>> datapath, to make initial assignment reg9[16..31] = 0 >>>> 3. Generate router policy flow in the following manner: >>>> lr_in_ip_routing_ecmp (not changed) >>>> lr_in_policy_pre >>>> match: 1 >>>> actoin: reg9[16..31] = 0; next; >>>> lr_in_policy >>>> match: (reg9[16..31] == <chain_id>) && (northd generated >>>> expression) >>>> action: for 'jump': reg9[16..31] = <jump_chain_id>; >>>> next(ingress, lr_in_policy); >>>> for other actions: northd generated expression >>>> >>>> Signed-off-by: Aleksandr Smirnov <[email protected]> >>>> --- >>>> northd/en-northd.c | 2 +- >>>> northd/northd.c | 75 ++++++++++++++++- >>>> northd/northd.h | 19 ++--- >>>> ovn-nb.ovsschema | 9 ++- >>>> ovn-nb.xml | 22 ++++- >>>> tests/ovn-nbctl.at | 54 +++++++++++-- >>>> tests/ovn-northd.at | 33 ++++++++ >>>> utilities/ovn-nbctl.c | 183 >>>> +++++++++++++++++++++++++++--------------- >>>> 8 files changed, 309 insertions(+), 88 deletions(-) >>>> >>>> diff --git a/northd/en-northd.c b/northd/en-northd.c >>>> index c7d1ebcb3..9c5ba5405 100644 >>>> --- a/northd/en-northd.c >>>> +++ b/northd/en-northd.c >>>> @@ -1,4 +1,4 @@ >>>> - /* >>>> +/* >>>> * Licensed under the Apache License, Version 2.0 (the "License"); >>>> * you may not use this file except in compliance with the License. >>>> * You may obtain a copy of the License at: >>>> diff --git a/northd/northd.c b/northd/northd.c >>>> index b01e40ecd..e4ee12cdd 100644 >>>> --- a/northd/northd.c >>>> +++ b/northd/northd.c >>>> @@ -190,6 +190,7 @@ BUILD_ASSERT_DECL(ACL_OBS_STAGE_MAX < (1 << 2)); >>>> #define REG_SRC_IPV4 "reg5" >>>> #define REG_SRC_IPV6 "xxreg1" >>>> #define REG_DHCP_RELAY_DIP_IPV4 "reg2" >>>> +#define REG_POLICY_CHAIN_ID "reg9[16..31]" >>>> #define REG_ROUTE_TABLE_ID "reg7" >>>> /* Register used to store backend ipv6 address >>>> @@ -10873,11 +10874,38 @@ static bool check_bfd_state(const struct >>>> nbrec_logical_router_policy *rule, >>>> return true; >>>> } >>>> +static void >>>> +policy_chain_add(struct simap *chain_ids, const char *chain_name) >>>> +{ >>>> + if (chain_name && *chain_name && !simap_get(chain_ids, >>>> chain_name)) { >>>> + uint32_t id = simap_count(chain_ids) + 1; >>>> + >>>> + if (id == UINT16_MAX) { >>>> + static struct vlog_rate_limit rl = >>>> VLOG_RATE_LIMIT_INIT(5, 1); >>>> + VLOG_WARN_RL(&rl, "Too many policy chains for Logical >>>> Router."); >>>> + return; >>>> + } >>>> + >>>> + if (!simap_put(chain_ids, chain_name, id)) { >>>> + static struct vlog_rate_limit rl = >>>> VLOG_RATE_LIMIT_INIT(5, 1); >>>> + VLOG_WARN_RL(&rl, "Policy chain id unexpectedly >>>> appeared"); >>>> + } >>>> + } >>>> +} >>>> + >>>> +static uint32_t >>>> +policy_chain_id(struct simap *chain_ids, const char *chain_name) >>>> +{ >>>> + return (chain_name && *chain_name) >>>> + ? simap_get(chain_ids, chain_name) >>>> + : 0; >>>> +} >>>> + >>>> static void >>>> build_routing_policy_flow(struct lflow_table *lflows, struct >>>> ovn_datapath *od, >>>> const struct hmap *lr_ports, struct >>>> route_policy *rp, >>>> const struct ovsdb_idl_row *stage_hint, >>>> - struct lflow_ref *lflow_ref) >>>> + struct lflow_ref *lflow_ref, struct simap >>>> *chain_ids) >>>> { >>>> const struct nbrec_logical_router_policy *rule = rp->rule; >>>> struct ds match = DS_EMPTY_INITIALIZER; >>>> @@ -10927,7 +10955,21 @@ build_routing_policy_flow(struct lflow_table >>>> *lflows, struct ovn_datapath *od, >>>> out_port->lrp_networks.ea_s, >>>> out_port->json_key, >>>> is_ipv4); >>>> + } else if (!strcmp(rule->action, "jump")) { >>>> + uint32_t chain_id = policy_chain_id(chain_ids, >>>> rule->jump_chain); >>>> + >>>> + if (!chain_id) { >>>> + static struct vlog_rate_limit rl = >>>> VLOG_RATE_LIMIT_INIT(5, 1); >>>> + VLOG_WARN_RL(&rl, >>>> + "Logical router: %s, policy action 'jump'" >>>> + " follows to non-existent chain %s", >>>> + od->nbr->name, rule->jump_chain); >>>> + return; >>>> + } >>>> + ds_put_format(&actions, REG_POLICY_CHAIN_ID >>>> + "=%d; next(pipeline=ingress, table=%d);", >>>> + chain_id, >>>> ovn_stage_get_table(S_ROUTER_IN_POLICY)); >>>> } else if (!strcmp(rule->action, "drop")) { >>>> ds_put_cstr(&actions, debug_drop_action()); >>>> } else if (!strcmp(rule->action, "allow")) { >>>> @@ -10937,7 +10979,13 @@ build_routing_policy_flow(struct lflow_table >>>> *lflows, struct ovn_datapath *od, >>>> } >>>> ds_put_cstr(&actions, REG_ECMP_GROUP_ID" = 0; next;"); >>>> } >>>> - ds_put_format(&match, "%s", rule->match); >>>> + >>>> + if (simap_is_empty(chain_ids)) { >>>> + ds_put_format(&match, "%s", rule->match); >>>> + } else { >>>> + ds_put_format(&match, REG_POLICY_CHAIN_ID" == %d && (%s)", >>>> + policy_chain_id(chain_ids, rule->chain), >>>> rule->match); >>>> + } >>>> ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_POLICY, >>>> rule->priority, >>>> ds_cstr(&match), ds_cstr(&actions), >>>> stage_hint, >>>> @@ -14150,11 +14198,27 @@ build_ingress_policy_flows_for_lrouter( >>>> struct lflow_ref *lflow_ref) >>>> { >>>> ovs_assert(od->nbr); >>>> + >>>> + struct route_policy *rp; >>>> + >>>> + /* Policy generation is different if chains are used. */ >>>> + struct simap chain_ids; >>>> + >>>> + simap_init(&chain_ids); >>>> + >>>> + HMAP_FOR_EACH_WITH_HASH (rp, key_node, uuid_hash(&od->key), >>>> + route_policies) { >>>> + policy_chain_add(&chain_ids, rp->rule->chain); >>> >>> The way this is written, it means that changes to the ordering of the >>> hmap may result in chains having their ID changed. For instance, let's >>> say you have a single policy chain "foo". When the logical flows are >>> generated, "foo" will have ID 1. Next, a new chain "bar" is added to >>> the policy chains. Now, this time, "bar" might have ID 1 and "foo" >>> will get ID 2 assigned to it. This means we have to rewrite foo's >>> flows to the SB DB just because the ID number changed. It would be >>> nicer if we only had to write the new "bar" flows instead. >>> >>> I think the easiest way to avoid this issue is to change chains to be >>> numbers instead of arbitrary strings. This has some advantages: >>> 1) It's easier to match logical flows to the router policies in the NB >>> DB. >>> 2) This eliminates the need to translate chain names into numbers. We >>> can just use the numbers directly. >>> 3) It eliminates the issue I mentioned in the previous paragraph; ID >>> values stay consistent across runs of ovn-northd. >>> >>> It means the chains won't have as user-friendly names, but a CMS could >>> easily associate chain numbers with names outside of OVN. >>> >> Vladislav, as the feature's requestor, wants to have chains named. He >> referenced to similar problem discussed during the 'multiple routing >> tables' implementation. >> >> There was a long discussion about routing table ids generated from names >> vs explicitly set from CMS >> https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ >> >> >> >> As result, routing tables were implemented as referenced by string names >> that automatically converted to numerical ids. > > Thanks for pointing to that discussion. Since we have precedent for > using names, and not really caring too much if a changed ID results in > re-calculating flows, then we can do the same thing here. If there > ends up being a performance problem later because of flow churn, we > can address the problem then. We can possibly do something similar to > what you suggest below. > >> >> However, if we still want to avoid unnecessary sb db update (and, I >> suppose, avoid similar problems on ovn-controller side during open flow >> generation) I could propose following schema, applied upon current >> implementation. >> >> 1. Extend table Logical_Router_Policy with yet another field chain_n >> (type of integer 0..32767) >> 2. Modify 'nbctl --chain <name> lr-policy-add' to allocate available >> numeric id for chain <name> and write to chain_n -- for new <name>, or, >> fill up chain_n from other record if same-name chain is already in the >> table. >> 3. northd loads association chain <-> chain_n from Logical_Router_Policy >> table, and additionally perform automatic associations for entries >> without associations stored in Logical_Router_Policy (for example, if >> records were created with other tools) >> >> >>>> + } >>>> + >>>> /* This is a catch-all rule. It has the lowest priority (0) >>>> * does a match-all("1") and pass-through (next) */ >>>> ovn_lflow_add(lflows, od, S_ROUTER_IN_POLICY, 0, "1", >>>> REG_ECMP_GROUP_ID" = 0; next;", >>>> lflow_ref); >>>> + ovn_lflow_add(lflows, od, S_ROUTER_IN_POLICY_PRE, 0, "1", >>>> + REG_POLICY_CHAIN_ID" = 0; next;", >>>> + lflow_ref); >>>> ovn_lflow_add(lflows, od, S_ROUTER_IN_POLICY_ECMP, 150, >>>> REG_ECMP_GROUP_ID" == 0", "next;", >>>> lflow_ref); >>>> @@ -14163,7 +14227,7 @@ build_ingress_policy_flows_for_lrouter( >>>> /* Convert routing policies to flows. */ >>>> uint16_t ecmp_group_id = 1; >>>> - struct route_policy *rp; >>>> + >>>> HMAP_FOR_EACH_WITH_HASH (rp, key_node, uuid_hash(&od->key), >>>> route_policies) { >>>> const struct nbrec_logical_router_policy *rule = rp->rule; >>>> @@ -14176,9 +14240,12 @@ build_ingress_policy_flows_for_lrouter( >>>> ecmp_group_id++; >>>> } else { >>>> build_routing_policy_flow(lflows, od, lr_ports, rp, >>>> - &rule->header_, lflow_ref); >>>> + &rule->header_, lflow_ref, >>>> + &chain_ids); >>>> } >>>> } >>>> + >>>> + simap_destroy(&chain_ids); >>>> } >>>> /* Local router ingress table ARP_RESOLVE: ARP Resolution. */ >>>> diff --git a/northd/northd.h b/northd/northd.h >>>> index 9457a7be6..364f8f3e6 100644 >>>> --- a/northd/northd.h >>>> +++ b/northd/northd.h >>>> @@ -491,17 +491,18 @@ enum ovn_stage { >>>> PIPELINE_STAGE(ROUTER, IN, IP_ROUTING_PRE, 13, >>>> "lr_in_ip_routing_pre") \ >>>> PIPELINE_STAGE(ROUTER, IN, IP_ROUTING, 14, >>>> "lr_in_ip_routing") \ >>>> PIPELINE_STAGE(ROUTER, IN, IP_ROUTING_ECMP, 15, >>>> "lr_in_ip_routing_ecmp") \ >>>> - PIPELINE_STAGE(ROUTER, IN, POLICY, 16, >>>> "lr_in_policy") \ >>>> - PIPELINE_STAGE(ROUTER, IN, POLICY_ECMP, 17, >>>> "lr_in_policy_ecmp") \ >>>> - PIPELINE_STAGE(ROUTER, IN, DHCP_RELAY_RESP_CHK, >>>> 18, \ >>>> + PIPELINE_STAGE(ROUTER, IN, POLICY_PRE, 16, >>>> "lr_in_policy_pre") \ >>> >>> I don't think this new pipeline stage is necessary. >>> REG_POLICY_CHAIN_ID defaults to 0 anyway, so the first time evaluating >>> it in the POLICY stage will have it set to 0. The register is only >>> used in the POLICY stage, so there is no need to ensure the register >>> has some default value for later stages. >>> >>>> + PIPELINE_STAGE(ROUTER, IN, POLICY, 17, >>>> "lr_in_policy") \ >>>> + PIPELINE_STAGE(ROUTER, IN, POLICY_ECMP, 18, >>>> "lr_in_policy_ecmp") \ >>>> + PIPELINE_STAGE(ROUTER, IN, DHCP_RELAY_RESP_CHK, >>>> 19, \ >>>> "lr_in_dhcp_relay_resp_chk") \ >>>> - PIPELINE_STAGE(ROUTER, IN, DHCP_RELAY_RESP, >>>> 19, \ >>>> + PIPELINE_STAGE(ROUTER, IN, DHCP_RELAY_RESP, >>>> 20, \ >>>> "lr_in_dhcp_relay_resp") \ >>>> - PIPELINE_STAGE(ROUTER, IN, ARP_RESOLVE, 20, >>>> "lr_in_arp_resolve") \ >>>> - PIPELINE_STAGE(ROUTER, IN, CHK_PKT_LEN, 21, >>>> "lr_in_chk_pkt_len") \ >>>> - PIPELINE_STAGE(ROUTER, IN, LARGER_PKTS, 22, >>>> "lr_in_larger_pkts") \ >>>> - PIPELINE_STAGE(ROUTER, IN, GW_REDIRECT, 23, >>>> "lr_in_gw_redirect") \ >>>> - PIPELINE_STAGE(ROUTER, IN, ARP_REQUEST, 24, >>>> "lr_in_arp_request") \ >>>> + PIPELINE_STAGE(ROUTER, IN, ARP_RESOLVE, 21, >>>> "lr_in_arp_resolve") \ >>>> + PIPELINE_STAGE(ROUTER, IN, CHK_PKT_LEN, 22, >>>> "lr_in_chk_pkt_len") \ >>>> + PIPELINE_STAGE(ROUTER, IN, LARGER_PKTS, 23, >>>> "lr_in_larger_pkts") \ >>>> + PIPELINE_STAGE(ROUTER, IN, GW_REDIRECT, 24, >>>> "lr_in_gw_redirect") \ >>>> + PIPELINE_STAGE(ROUTER, IN, ARP_REQUEST, 25, >>>> "lr_in_arp_request") \ >>>> \ >>>> /* Logical router egress stages. >>>> */ \ >>>> PIPELINE_STAGE(ROUTER, OUT, CHECK_DNAT_LOCAL, >>>> 0, \ >>>> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema >>>> index 09361920f..709d32c93 100644 >>>> --- a/ovn-nb.ovsschema >>>> +++ b/ovn-nb.ovsschema >>>> @@ -1,7 +1,7 @@ >>>> { >>>> "name": "OVN_Northbound", >>>> - "version": "7.8.0", >>>> - "cksum": "3497747919 38626", >>>> + "version": "7.8.1", >>>> + "cksum": "884115012 38765", >>>> "tables": { >>>> "NB_Global": { >>>> "columns": { >>>> @@ -524,10 +524,13 @@ >>>> "priority": {"type": {"key": {"type": "integer", >>>> "minInteger": 0, >>>> "maxInteger": 32767}}}, >>>> + "chain": {"type": "string"}, >>>> "match": {"type": "string"}, >>>> "action": {"type": { >>>> "key": {"type": "string", >>>> - "enum": ["set", ["allow", "drop", >>>> "reroute"]]}}}, >>>> + "enum": ["set", >>>> + ["allow", "drop", "reroute", >>>> "jump"]]}}}, >>>> + "jump_chain": {"type": "string"}, >>>> "nexthop": {"type": {"key": "string", "min": 0, >>>> "max": 1}}, >>>> "nexthops": {"type": { >>>> "key": "string", "min": 0, "max": >>>> "unlimited"}}, >>>> diff --git a/ovn-nb.xml b/ovn-nb.xml >>>> index 8373ddb99..6e43e9bc9 100644 >>>> --- a/ovn-nb.xml >>>> +++ b/ovn-nb.xml >>>> @@ -3942,7 +3942,15 @@ or >>>> <p> >>>> The routing policy's priority. Rules with numerically >>>> higher priority >>>> take precedence over those with lower. A rule is uniquely >>>> identified >>>> - by the priority and match string. >>>> + by the priority, chain and match string. >>>> + </p> >>>> + </column> >>>> + >>>> + <column name="chain"> >>>> + <p> >>>> + The routing policy rule's chain name. Only rules with empty >>>> chain name >>>> + are traversed by default. Other chains are traversed as >>>> response to >>>> + jump action. >>>> </p> >>>> </column> >>>> @@ -3977,9 +3985,21 @@ or >>>> <code>reroute</code>: Reroute packet to <ref >>>> column="nexthop"/> or >>>> <ref column="nexthops"/>. >>>> </li> >>>> + >>>> + <li> >>>> + <code>jump</code>: Start examining rules that have the same >>>> + <ref column="chain"/> value as specified in >>>> + <ref column="jump_chain"/>. >>>> + </li> >>>> </ul> >>>> </column> >>>> + <column name="jump_chain"> >>>> + <p> >>>> + The routing policy rule's chain name selected to be examined >>>> next. >>>> + </p> >>>> + </column> >>>> + >>>> <column name="nexthop"> >>>> <p> >>>> Note: This column is deprecated in favor of <ref >>>> column="nexthops"/>. >>>> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at >>>> index 39c02d295..583c3ec74 100644 >>>> --- a/tests/ovn-nbctl.at >>>> +++ b/tests/ovn-nbctl.at >>>> @@ -2318,7 +2318,7 @@ dnl >>>> --------------------------------------------------------------------- >>>> OVN_NBCTL_TEST([ovn_nbctl_policies], [policies], [ >>>> AT_CHECK([ovn-nbctl lr-add lr0]) >>>> -dnl Add policies with allow and drop actions >>>> +dnl Add policies with allow, drop and jump actions >>>> AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.1.0/24" >>>> drop]) >>>> AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.2.0/24" >>>> allow pkt_mark=100,foo=bar]) >>>> AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.1.0/24" >>>> allow]) >>>> @@ -2330,6 +2330,8 @@ AT_CHECK([ovn-nbctl --bfd lr-policy-add lr0 103 >>>> "ip4.src == 1.2.3.0/24" reroute >>>> AT_CHECK([ovn-nbctl --bfd lr-policy-add lr0 103 "ip4.src == >>>> 1.2.3.0/24" drop], [1], [], >>>> [ovn-nbctl: BFD is valid only with reroute action. >>>> ]) >>>> +AT_CHECK([ovn-nbctl --chain=inbound lr-policy-add lr0 100 "ip4.src >>>> == 1.1.1.0/24" drop]) >>>> +AT_CHECK([ovn-nbctl --chain=outbound lr-policy-add lr0 100 "ip4.src >>>> == 2.1.1.0/24" jump inbound]) >>>> dnl Incomplete option set. >>>> AT_CHECK([ovn-nbctl lr-policy-add lr0 200 "ip4.src == 1.1.4.0/24" >>>> reroute 192.168.0.10 foo], [1], [], >>>> @@ -2340,6 +2342,14 @@ AT_CHECK([ovn-nbctl lr-policy-add lr0 200 >>>> "ip4.src == 1.1.4.0/24" allow bar=], [ >>>> [ovn-nbctl: No value specified for the option : bar >>>> ]) >>>> +AT_CHECK([ovn-nbctl --chain lr-policy-add lr0 100 "ip4.src == >>>> 1.1.1.0/24" drop], [1], [], >>>> + [ovn-nbctl: unknown command 'lr0'; use --help for help >>>> +]) >>>> + >>>> +AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 2.1.1.0/24" >>>> jump], [1], [], >>>> + [ovn-nbctl: Chain name is required when action is jump. >>>> +]) >>>> + >>>> dnl Add duplicated policy >>>> AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.1.0/24" >>>> drop], [1], [], >>>> [ovn-nbctl: Same routing policy already existed on the logical >>>> router lr0. >>>> @@ -2349,35 +2359,69 @@ AT_CHECK([ovn-nbctl --may-exist lr-policy-add >>>> lr0 100 "ip4.src == 1.1.1.0/24" dr >>>> dnl Add duplicated policy >>>> AT_CHECK([ovn-nbctl lr-policy-add lr0 103 "ip4.src == 1.1.1.0/24" >>>> deny], [1], [], >>>> - [ovn-nbctl: deny: action must be one of "allow", "drop", and >>>> "reroute" >>>> + [ovn-nbctl: deny: action must be one of "allow", "drop", >>>> "reroute", "jump" >>>> ]) >>>> dnl Delete by priority and match string >>>> AT_CHECK([ovn-nbctl lr-policy-del lr0 100 "ip4.src == 1.1.1.0/24"]) >>>> + >>>> +AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl >>>> +Routing Policies >>>> +Chain <Default>: >>>> + 101 ip4.src == >>>> 2.1.1.0/24 allow >>>> + 101 ip4.src == >>>> 2.1.2.0/24 drop >>>> + 101 ip6.src == >>>> 2002::/64 drop >>>> + 100 ip4.src == >>>> 1.1.2.0/24 allow pkt_mark=100,foo=bar >>>> + >>>> +Chain inbound: >>>> + 100 ip4.src == >>>> 1.1.1.0/24 drop >>>> + >>>> +Chain outbound: >>>> + 100 ip4.src == >>>> 2.1.1.0/24 jump -> inbound >>>> +]) >>>> + >>>> +AT_CHECK([ovn-nbctl --chain inbound lr-policy-del lr0 100 "ip4.src >>>> == 1.1.1.0/24"]) >>>> + >>>> AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl >>>> Routing Policies >>>> +Chain <Default>: >>>> 101 ip4.src == >>>> 2.1.1.0/24 allow >>>> 101 ip4.src == >>>> 2.1.2.0/24 drop >>>> 101 ip6.src == >>>> 2002::/64 drop >>>> 100 ip4.src == >>>> 1.1.2.0/24 allow pkt_mark=100,foo=bar >>>> + >>>> +Chain outbound: >>>> + 100 ip4.src == >>>> 2.1.1.0/24 jump -> inbound >>>> ]) >>>> dnl Delete all policies for given priority >>>> AT_CHECK([ovn-nbctl lr-policy-del lr0 101]) >>>> AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl >>>> Routing Policies >>>> +Chain <Default>: >>>> 100 ip4.src == >>>> 1.1.2.0/24 allow pkt_mark=100,foo=bar >>>> -]) >>>> +Chain outbound: >>>> + 100 ip4.src == >>>> 2.1.1.0/24 jump -> inbound >>>> +]) >>>> dnl Delete policy by specified uuid >>>> -uuid=$(ovn-nbctl --bare --column _uuid list logical_router_policy) >>>> +uuid=$(ovn-nbctl --bare --column _uuid find logical_router_policy >>>> chain=\"\") >>>> AT_CHECK([ovn-nbctl lr-policy-del lr0 $uuid]) >>>> -AT_CHECK([ovn-nbctl list logical-router-policy], [0], [dnl >>>> +AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl >>>> +Routing Policies >>>> + >>>> +Chain outbound: >>>> + 100 ip4.src == >>>> 2.1.1.0/24 jump -> inbound >>>> ]) >>>> AT_CHECK([ovn-nbctl --if-exists lr-policy-del lr0 $uuid]) >>>> +AT_CHECK([ovn-nbctl --chain outbound lr-policy-del lr0]) >>>> + >>>> +AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl >>>> +]) >>>> + >>>> dnl Add policy with reroute action >>>> AT_CHECK([ovn-nbctl lr-policy-add lr0 102 "ip4.src == 3.1.2.0/24" >>>> reroute 3.3.3.3]) >>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >>>> index 21d9d63ab..922bc6df3 100644 >>>> --- a/tests/ovn-northd.at >>>> +++ b/tests/ovn-northd.at >>>> @@ -3444,6 +3444,7 @@ AT_CHECK([grep "lr_in_policy" lr0flows3 | >>>> ovn_strip_lflows], [0], [dnl >>>> table=??(lr_in_policy_ecmp ), priority=100 , >>>> match=(reg8[[0..15]] == 1 && reg8[[16..31]] == 1), action=(reg0 = >>>> 172.168.0.101; reg5 = 172.168.0.100; eth.src = 00:00:20:20:12:13; >>>> outport = "lr0-public"; flags.loopback = 1; reg9[[9]] = 1; next;) >>>> table=??(lr_in_policy_ecmp ), priority=100 , >>>> match=(reg8[[0..15]] == 1 && reg8[[16..31]] == 2), action=(reg0 = >>>> 172.168.0.102; reg5 = 172.168.0.100; eth.src = 00:00:20:20:12:13; >>>> outport = "lr0-public"; flags.loopback = 1; reg9[[9]] = 1; next;) >>>> table=??(lr_in_policy_ecmp ), priority=150 , >>>> match=(reg8[[0..15]] == 0), action=(next;) >>>> + table=??(lr_in_policy_pre ), priority=0 , match=(1), >>>> action=(reg9[[16..31]] = 0; next;) >>>> ]) >>>> check ovn-nbctl --wait=sb lr-policy-add lr0 10 "ip4.src == >>>> 10.0.0.4" reroute 172.168.0.101,172.168.0.102,172.168.0.103 >>>> @@ -3463,6 +3464,7 @@ sed 's/reg8\[[0..15\]] == >>>> [[0-9]]*/reg8\[[0..15\]] == <cleared>/' | ovn_strip_lf >>>> table=??(lr_in_policy_ecmp ), priority=100 , >>>> match=(reg8[[0..15]] == <cleared> && reg8[[16..31]] == 2), >>>> action=(reg0 = 172.168.0.102; reg5 = 172.168.0.100; eth.src = >>>> 00:00:20:20:12:13; outport = "lr0-public"; flags.loopback = 1; >>>> reg9[[9]] = 1; next;) >>>> table=??(lr_in_policy_ecmp ), priority=100 , >>>> match=(reg8[[0..15]] == <cleared> && reg8[[16..31]] == 3), >>>> action=(reg0 = 172.168.0.103; reg5 = 172.168.0.100; eth.src = >>>> 00:00:20:20:12:13; outport = "lr0-public"; flags.loopback = 1; >>>> reg9[[9]] = 1; next;) >>>> table=??(lr_in_policy_ecmp ), priority=150 , >>>> match=(reg8[[0..15]] == <cleared>), action=(next;) >>>> + table=??(lr_in_policy_pre ), priority=0 , match=(1), >>>> action=(reg9[[16..31]] = 0; next;) >>>> ]) >>>> check ovn-nbctl --wait=sb lr-policy-add lr0 10 "ip4.src == >>>> 10.0.0.5" reroute 172.168.0.110 >>>> @@ -3483,6 +3485,7 @@ sed 's/reg8\[[0..15\]] == >>>> [[0-9]]*/reg8\[[0..15\]] == <cleared>/' | ovn_strip_lf >>>> table=??(lr_in_policy_ecmp ), priority=100 , >>>> match=(reg8[[0..15]] == <cleared> && reg8[[16..31]] == 2), >>>> action=(reg0 = 172.168.0.102; reg5 = 172.168.0.100; eth.src = >>>> 00:00:20:20:12:13; outport = "lr0-public"; flags.loopback = 1; >>>> reg9[[9]] = 1; next;) >>>> table=??(lr_in_policy_ecmp ), priority=100 , >>>> match=(reg8[[0..15]] == <cleared> && reg8[[16..31]] == 3), >>>> action=(reg0 = 172.168.0.103; reg5 = 172.168.0.100; eth.src = >>>> 00:00:20:20:12:13; outport = "lr0-public"; flags.loopback = 1; >>>> reg9[[9]] = 1; next;) >>>> table=??(lr_in_policy_ecmp ), priority=150 , >>>> match=(reg8[[0..15]] == <cleared>), action=(next;) >>>> + table=??(lr_in_policy_pre ), priority=0 , match=(1), >>>> action=(reg9[[16..31]] = 0; next;) >>>> ]) >>>> check ovn-nbctl --wait=sb lr-policy-del lr0 10 "ip4.src == >>>> 10.0.0.3" >>>> @@ -3500,6 +3503,7 @@ sed 's/reg8\[[0..15\]] == >>>> [[0-9]]*/reg8\[[0..15\]] == <cleared>/' | ovn_strip_lf >>>> table=??(lr_in_policy_ecmp ), priority=100 , >>>> match=(reg8[[0..15]] == <cleared> && reg8[[16..31]] == 2), >>>> action=(reg0 = 172.168.0.102; reg5 = 172.168.0.100; eth.src = >>>> 00:00:20:20:12:13; outport = "lr0-public"; flags.loopback = 1; >>>> reg9[[9]] = 1; next;) >>>> table=??(lr_in_policy_ecmp ), priority=100 , >>>> match=(reg8[[0..15]] == <cleared> && reg8[[16..31]] == 3), >>>> action=(reg0 = 172.168.0.103; reg5 = 172.168.0.100; eth.src = >>>> 00:00:20:20:12:13; outport = "lr0-public"; flags.loopback = 1; >>>> reg9[[9]] = 1; next;) >>>> table=??(lr_in_policy_ecmp ), priority=150 , >>>> match=(reg8[[0..15]] == <cleared>), action=(next;) >>>> + table=??(lr_in_policy_pre ), priority=0 , match=(1), >>>> action=(reg9[[16..31]] = 0; next;) >>>> ]) >>>> check ovn-nbctl --wait=sb lr-policy-del lr0 10 "ip4.src == >>>> 10.0.0.4" >>>> @@ -3513,6 +3517,7 @@ sed 's/reg8\[[0..15\]] == >>>> [[0-9]]*/reg8\[[0..15\]] == <cleared>/' | ovn_strip_lf >>>> table=??(lr_in_policy ), priority=10 , match=(ip4.src == >>>> 10.0.0.5), action=(reg0 = 172.168.0.110; reg5 = 172.168.0.100; >>>> eth.src = 00:00:20:20:12:13; outport = "lr0-public"; flags.loopback = >>>> 1; reg8[[0..15]] = <cleared>; reg9[[9]] = 1; next;) >>>> table=??(lr_in_policy_ecmp ), priority=0 , match=(1), >>>> action=(drop;) >>>> table=??(lr_in_policy_ecmp ), priority=150 , >>>> match=(reg8[[0..15]] == <cleared>), action=(next;) >>>> + table=??(lr_in_policy_pre ), priority=0 , match=(1), >>>> action=(reg9[[16..31]] = 0; next;) >>>> ]) >>>> check ovn-nbctl --wait=sb add logical_router_policy . nexthops >>>> "2000\:\:b" >>>> @@ -3525,6 +3530,34 @@ sed 's/reg8\[[0..15\]] == >>>> [[0-9]]*/reg8\[[0..15\]] == <cleared>/' | ovn_strip_lf >>>> table=??(lr_in_policy ), priority=0 , match=(1), >>>> action=(reg8[[0..15]] = <cleared>; next;) >>>> table=??(lr_in_policy_ecmp ), priority=0 , match=(1), >>>> action=(drop;) >>>> table=??(lr_in_policy_ecmp ), priority=150 , >>>> match=(reg8[[0..15]] == <cleared>), action=(next;) >>>> + table=??(lr_in_policy_pre ), priority=0 , match=(1), >>>> action=(reg9[[16..31]] = 0; next;) >>>> +]) >>>> + >>>> +AT_CLEANUP >>>> +]) >>>> + >>>> +OVN_FOR_EACH_NORTHD_NO_HV([ >>>> +AT_SETUP([Router policies - chains]) >>>> +ovn_start >>>> + >>>> +check ovn-nbctl --wait=sb lr-add lr0 >>>> +check ovn-nbctl --wait=sb lr-policy-add lr0 112 "ip4.src == >>>> 10.0.0.0/24" jump nochain >>>> +check ovn-nbctl --wait=sb lr-policy-add lr0 102 "ip4.src == >>>> 10.0.0.0/24" jump inbound >>>> +check ovn-nbctl --wait=sb lr-policy-add lr0 101 "ip4.src == >>>> 10.0.16.0/24" jump inbound >>>> +check ovn-nbctl --wait=sb lr-policy-add lr0 100 "ip4.src == >>>> 192.168.0.1/32" allow >>>> +check ovn-nbctl --wait=sb --chain=inbound lr-policy-add lr0 201 "1" >>>> allow >>>> + >>>> +AT_CHECK([grep -qE "Logical router: lr0, policy action 'jump' >>>> follows to non-existent chain nochain" northd/ovn-northd.log], [0]) >>>> + >>>> +ovn-sbctl dump-flows lr0 > lr0flows2 >>>> +AT_CAPTURE_FILE([lr0flows2]) >>>> + >>>> +AT_CHECK([grep "lr_in_policy[[^_]]" lr0flows2 | ovn_strip_lflows | >>>> sort], [0], [dnl >>>> + table=??(lr_in_policy ), priority=0 , match=(1), >>>> action=(reg8[[0..15]] = 0; next;) >>>> + table=??(lr_in_policy ), priority=100 , >>>> match=(reg9[[16..31]] == 0 && (ip4.src == 192.168.0.1/32)), >>>> action=(reg8[[0..15]] = 0; next;) >>>> + table=??(lr_in_policy ), priority=101 , >>>> match=(reg9[[16..31]] == 0 && (ip4.src == 10.0.16.0/24)), >>>> action=(reg9[[16..31]]=1; next(pipeline=ingress, table=??);) >>>> + table=??(lr_in_policy ), priority=102 , >>>> match=(reg9[[16..31]] == 0 && (ip4.src == 10.0.0.0/24)), >>>> action=(reg9[[16..31]]=1; next(pipeline=ingress, table=??);) >>>> + table=??(lr_in_policy ), priority=201 , >>>> match=(reg9[[16..31]] == 1 && (1)), action=(reg8[[0..15]] = 0; next;) >>>> ]) >>>> AT_CLEANUP >>>> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c >>>> index 131d9f0bf..0c31a9698 100644 >>>> --- a/utilities/ovn-nbctl.c >>>> +++ b/utilities/ovn-nbctl.c >>>> @@ -4091,6 +4091,7 @@ nbctl_pre_lr_policy_add(struct ctl_context *ctx) >>>> ovsdb_idl_add_column(ctx->idl, >>>> &nbrec_logical_router_policy_col_priority); >>>> ovsdb_idl_add_column(ctx->idl, >>>> &nbrec_logical_router_policy_col_match); >>>> + ovsdb_idl_add_column(ctx->idl, >>>> &nbrec_logical_router_policy_col_chain); >>>> ovsdb_idl_add_column(ctx->idl, &nbrec_bfd_col_dst_ip); >>>> ovsdb_idl_add_column(ctx->idl, &nbrec_bfd_col_logical_port); >>>> @@ -4114,13 +4115,16 @@ nbctl_lr_policy_add(struct ctl_context *ctx) >>>> const char *action = ctx->argv[4]; >>>> size_t n_nexthops = 0; >>>> char **nexthops = NULL; >>>> + const char *chain_s = shash_find_data(&ctx->options, "--chain"); >>>> bool reroute = false; >>>> + bool jump = false; >>>> + >>>> /* Validate action. */ >>>> if (strcmp(action, "allow") && strcmp(action, "drop") >>>> - && strcmp(action, "reroute")) { >>>> + && strcmp(action, "jump") && strcmp(action, "reroute")) { >>>> ctl_error(ctx, "%s: action must be one of \"allow\", >>>> \"drop\", " >>>> - "and \"reroute\"", action); >>>> + "\"reroute\", \"jump\"", action); >>>> return; >>>> } >>>> if (!strcmp(action, "reroute")) { >>>> @@ -4131,14 +4135,23 @@ nbctl_lr_policy_add(struct ctl_context *ctx) >>>> reroute = true; >>>> } >>>> + if (!strcmp(action, "jump")) { >>>> + if (ctx->argc < 6) { >>>> + ctl_error(ctx, "Chain name is required when action is >>>> jump."); >>>> + return; >>>> + } >>>> + jump = true; >>>> + } >>>> + >>>> /* Check if same routing policy already exists. >>>> * A policy is uniquely identified by priority and match */ >>>> bool may_exist = !!shash_find(&ctx->options, "--may-exist"); >>>> size_t i; >>>> for (i = 0; i < lr->n_policies; i++) { >>>> const struct nbrec_logical_router_policy *policy = >>>> lr->policies[i]; >>>> - if (policy->priority == priority && >>>> - !strcmp(policy->match, ctx->argv[3])) { >>>> + if (policy->priority == priority >>>> + && !strcmp(policy->match, ctx->argv[3]) >>>> + && !strcmp(policy->chain, (chain_s) ? chain_s : "")) { >>>> if (!may_exist) { >>>> ctl_error(ctx, "Same routing policy already existed >>>> on the " >>>> "logical router %s.", ctx->argv[1]); >>>> @@ -4200,6 +4213,15 @@ nbctl_lr_policy_add(struct ctl_context *ctx) >>>> nbrec_logical_router_policy_set_priority(policy, priority); >>>> nbrec_logical_router_policy_set_match(policy, ctx->argv[3]); >>>> nbrec_logical_router_policy_set_action(policy, action); >>>> + >>>> + if (chain_s) { >>>> + nbrec_logical_router_policy_set_chain(policy, chain_s); >>>> + } >>>> + >>>> + if (jump) { >>>> + nbrec_logical_router_policy_set_jump_chain(policy, >>>> ctx->argv[5]); >>>> + } >>>> + >>>> if (reroute) { >>>> nbrec_logical_router_policy_set_nexthops( >>>> policy, (const char **)nexthops, n_nexthops); >>>> @@ -4207,7 +4229,7 @@ nbctl_lr_policy_add(struct ctl_context *ctx) >>>> /* Parse the options. */ >>>> struct smap options = SMAP_INITIALIZER(&options); >>>> - for (i = reroute ? 6 : 5; i < ctx->argc; i++) { >>>> + for (i = (reroute | jump) ? 6 : 5; i < ctx->argc; i++) { >>> >>> Nit: Use "||" instead of "|" since we are performing a logical or of >>> two boolean values. >>> >>>> char *key, *value; >>>> value = xstrdup(ctx->argv[i]); >>>> key = strsep(&value, "="); >>>> @@ -4319,84 +4341,81 @@ nbctl_pre_lr_policy_del(struct ctl_context >>>> *ctx) >>>> ovsdb_idl_add_column(ctx->idl, >>>> &nbrec_logical_router_policy_col_priority); >>>> ovsdb_idl_add_column(ctx->idl, >>>> &nbrec_logical_router_policy_col_match); >>>> + ovsdb_idl_add_column(ctx->idl, >>>> &nbrec_logical_router_policy_col_chain); >>>> } >>>> static void >>>> nbctl_lr_policy_del(struct ctl_context *ctx) >>>> { >>>> const struct nbrec_logical_router *lr; >>>> - int64_t priority = 0; >>>> char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr); >>>> + >>>> if (error) { >>>> ctx->error = error; >>>> return; >>>> } >>>> - if (ctx->argc == 2) { >>>> - /* If a priority is not specified, delete all policies. */ >>>> + const char *chain = shash_find_data(&ctx->options, "--chain"); >>>> + >>>> + if (!chain) { >>>> + chain = ""; >>>> + } >>>> + >>>> + /* An uuid or priority either chain are not specified. >>>> + Delete all policies for lr */ >>>> + if (ctx->argc == 2 && !*chain) { >>>> nbrec_logical_router_set_policies(lr, NULL, 0); >>>> return; >>>> } >>>> - const struct uuid *lr_policy_uuid = NULL; >>>> + int64_t priority = 0; >>>> struct uuid uuid_from_cmd; >>>> - if (uuid_from_string(&uuid_from_cmd, ctx->argv[2])) { >>>> - lr_policy_uuid = &uuid_from_cmd; >>>> - } else { >>>> - error = parse_priority(ctx->argv[2], &priority); >>>> - if (error) { >>>> - ctx->error = error; >>>> - return; >>>> - } >>>> - } >>>> - /* If uuid was specified, delete routing policy with the >>>> - * specified uuid. */ >>>> - if (ctx->argc == 3) { >>>> - size_t i; >>>> - if (lr_policy_uuid) { >>>> - for (i = 0; i < lr->n_policies; i++) { >>>> - if (uuid_equals(lr_policy_uuid, >>>> - &(lr->policies[i]->header_.uuid))) { >>>> - nbrec_logical_router_update_policies_delvalue( >>>> - lr, lr->policies[i]); >>>> - break; >>>> - } >>>> - } >>>> - if (i == lr->n_policies) { >>>> - if (!shash_find(&ctx->options, "--if-exists")) { >>>> - ctl_error(ctx, "Logical router policy uuid is >>>> not found."); >>>> - } >>>> + /* An uuid is specified, delete the policy with uuid only */ >>>> + if (ctx->argc == 3 && uuid_from_string(&uuid_from_cmd, >>>> ctx->argv[2])) { >>>> + for (int i = 0; i < lr->n_policies; i++) { >>>> + if (uuid_equals(&uuid_from_cmd, >>>> + &(lr->policies[i]->header_.uuid))) { >>>> + nbrec_logical_router_update_policies_delvalue( >>>> + lr, lr->policies[i]); >>>> return; >>>> } >>>> + } >>>> - /* If match is not specified, delete all routing policies >>>> with the >>>> - * specified priority. */ >>>> - } else { >>>> - for (i = 0; i < lr->n_policies; i++) { >>>> - if (priority == lr->policies[i]->priority) { >>>> - nbrec_logical_router_update_policies_delvalue( >>>> - lr, lr->policies[i]); >>>> - } >>>> - } >>>> + if (!shash_find(&ctx->options, "--if-exists")) { >>>> + ctl_error(ctx, "Logical router policy uuid is not >>>> found."); >>>> } >>>> + >>>> return; >>>> } >>>> - /* Delete policy that has the same priority and match >>>> string */ >>>> - for (int i = 0; i < lr->n_policies; i++) { >>>> - struct nbrec_logical_router_policy *routing_policy = >>>> lr->policies[i]; >>>> - if (priority == routing_policy->priority && >>>> - !strcmp(ctx->argv[3], routing_policy->match)) { >>>> - nbrec_logical_router_update_policies_delvalue(lr, >>>> routing_policy); >>>> + if (ctx->argc >= 3) { >>>> + error = parse_priority(ctx->argv[2], &priority); >>>> + if (error) { >>>> + ctx->error = error; >>>> return; >>>> } >>>> } >>>> + >>>> + char *match = (ctx->argc == 4) ? ctx->argv[3] : NULL; >>>> + >>>> + for (int i = 0; i < lr->n_policies; i++) { >>>> + struct nbrec_logical_router_policy *policy = lr->policies[i]; >>>> + >>>> + /* Delete policies selected by chain, priority (if set), >>>> + match (if set). Sure, at least one option is set here */ >>>> + if ((!priority || priority == policy->priority) >>>> + && (!strcmp(chain, policy->chain)) >>>> + && (!match || !strcmp(match, policy->match))) { >>>> + nbrec_logical_router_update_policies_delvalue(lr, policy); >>>> + } >>>> + } >>>> } >>>> - struct routing_policy { >>>> +struct routing_policy { >>>> int priority; >>>> char *match; >>>> + char *chain; >>>> const struct nbrec_logical_router_policy *policy; >>>> }; >>>> @@ -4405,6 +4424,12 @@ routing_policy_cmp(const void *policy1_, >>>> const void *policy2_) >>>> { >>>> const struct routing_policy *policy1p = policy1_; >>>> const struct routing_policy *policy2p = policy2_; >>>> + int chain_match = strcmp(policy1p->chain, policy2p->chain); >>>> + >>>> + if (chain_match) { >>>> + return chain_match; >>>> + } >>>> + >>>> if (policy1p->priority != policy2p->priority) { >>>> return policy1p->priority > policy2p->priority ? -1 : 1; >>>> } else { >>>> @@ -4416,17 +4441,18 @@ static void >>>> print_routing_policy(const struct nbrec_logical_router_policy >>>> *policy, >>>> struct ds *s) >>>> { >>>> - if (policy->n_nexthops) { >>>> - ds_put_format(s, "%10"PRId64" %50s %15s", policy->priority, >>>> - policy->match, policy->action); >>>> - for (int i = 0; i < policy->n_nexthops; i++) { >>>> - char *next_hop = >>>> normalize_prefix_str(policy->nexthops[i]); >>>> - ds_put_format(s, i ? ", %s" : " %25s", next_hop ? >>>> next_hop : ""); >>>> - free(next_hop); >>>> - } >>>> - } else { >>>> - ds_put_format(s, "%10"PRId64" %50s %15s", policy->priority, >>>> - policy->match, policy->action); >>>> + ds_put_format(s, "%10"PRId64" %50s %15s", >>>> + policy->priority, policy->match, policy->action); >>>> + >>>> + if (strcmp(policy->action, "jump") == 0 >>>> + && policy->jump_chain && *policy->jump_chain) { >>>> + ds_put_format(s, " -> %s", policy->jump_chain); >>>> + } >>>> + >>>> + for (int i = 0; i < policy->n_nexthops; i++) { >>>> + char *next_hop = normalize_prefix_str(policy->nexthops[i]); >>>> + ds_put_format(s, i ? ", %s" : " %25s", next_hop ? next_hop : >>>> ""); >>>> + free(next_hop); >>>> } >>>> if (!smap_is_empty(&policy->options) || >>>> policy->n_bfd_sessions) { >>>> @@ -4457,6 +4483,9 @@ nbctl_pre_lr_policy_list(struct ctl_context >>>> *ctx) >>>> ovsdb_idl_add_column(ctx->idl, >>>> &nbrec_logical_router_policy_col_options); >>>> ovsdb_idl_add_column(ctx->idl, >>>> &nbrec_logical_router_policy_col_bfd_sessions); >>>> + ovsdb_idl_add_column(ctx->idl, >>>> &nbrec_logical_router_policy_col_chain); >>>> + ovsdb_idl_add_column(ctx->idl, >>>> + &nbrec_logical_router_policy_col_jump_chain); >>>> } >>>> static void >>>> @@ -4476,6 +4505,7 @@ nbctl_lr_policy_list(struct ctl_context *ctx) >>>> = lr->policies[i]; >>>> policies[n_policies].priority = policy->priority; >>>> policies[n_policies].match = policy->match; >>>> + policies[n_policies].chain = (policy->chain) ? policy->chain >>>> : ""; >>>> policies[n_policies].policy = policy; >>>> n_policies++; >>>> } >>>> @@ -4483,9 +4513,31 @@ nbctl_lr_policy_list(struct ctl_context *ctx) >>>> if (n_policies) { >>>> ds_put_cstr(&ctx->output, "Routing Policies\n"); >>>> } >>>> + >>>> + /* Print chain headings only if there are non-default chains. >>>> + Otherwise print format will be fully backward compatible. >>>> + Note that at this point policies are sorted out by chain >>>> names too. >>>> + */ >>>> + char *chain_current; >>>> + int print_chain_head = (n_policies >>>> + && policies[n_policies - 1].chain[0]); >>>> + >>>> for (int i = 0; i < n_policies; i++) { >>>> + /* Print chain heading */ >>>> + if (print_chain_head >>>> + && (i == 0 >>>> + || strcmp(chain_current, policies[i].chain))) { >>>> + chain_current = policies[i].chain; >>>> + if (*chain_current) { >>>> + ds_put_format(&ctx->output, "\nChain %s:\n", >>>> chain_current); >>>> + } else { >>>> + ds_put_format(&ctx->output, "Chain <Default>:\n"); >>>> + } >>>> + } >>>> + >>>> print_routing_policy(policies[i].policy, &ctx->output); >>>> } >>>> + >>>> free(policies); >>>> } >>>> @@ -8091,10 +8143,11 @@ static const struct ctl_command_syntax >>>> nbctl_commands[] = { >>>> /* Policy commands */ >>>> { "lr-policy-add", 4, INT_MAX, >>>> "ROUTER PRIORITY MATCH ACTION [NEXTHOP] [OPTIONS - KEY=VALUE >>>> ...]", >>>> - nbctl_pre_lr_policy_add, nbctl_lr_policy_add, NULL, >>>> "--may-exist,--bfd?", >>>> - RW }, >>>> + nbctl_pre_lr_policy_add, nbctl_lr_policy_add, NULL, >>>> + "--may-exist,--bfd?,--chain=", RW }, >>>> { "lr-policy-del", 1, 3, "ROUTER [{PRIORITY | UUID} [MATCH]]", >>>> - nbctl_pre_lr_policy_del, nbctl_lr_policy_del, NULL, >>>> "--if-exists", RW }, >>>> + nbctl_pre_lr_policy_del, nbctl_lr_policy_del, NULL, >>>> + "--if-exists,--chain=", RW }, >>>> { "lr-policy-list", 1, 1, "ROUTER", nbctl_pre_lr_policy_list, >>>> nbctl_lr_policy_list, NULL, "", RO }, >>> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
