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 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.
+ }
+
/* 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
