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? > > 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. 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
