ECMP routing policy selection flows use the policy match without the
chain ID condition applied to non-ECMP policies. A main-chain ECMP
policy can therefore override a lower-priority rule after a jump, and
an ECMP policy in a named chain can match outside that chain.
Use a shared match builder for both policy paths, preserving the
unqualified match when the router has no policy chains. Add IPv4 and
IPv6 trace coverage for main-chain and named-chain ECMP policies,
including jumps to lower-priority allow rules.
Fixes: 33ecea5b9b13 ("Logical Router Policy chains.")
Assisted-by: OpenAI Codex
Signed-off-by: Tim Rozet <[email protected]>
---
This was found while using policy-chain jumps for egress routing in
OVN-Kubernetes. A main-chain priority-100 ECMP reroute could override
an allow at priority 20 in the chain selected by a priority-102 jump.
Testing:
- The new ECMP-chain regression fails without the fix and passes
with it.
- Focused router-policy and IPv4/IPv6 forwarding tests pass.
- GitHub Actions Build and Test passed:
https://github.com/trozet/ovn/actions/runs/40391436937
- GitHub Actions ovn-kubernetes passed:
https://github.com/trozet/ovn/actions/runs/40391436986
northd/northd.c | 25 +++++++++++------
tests/ovn-northd.at | 65 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 82 insertions(+), 8 deletions(-)
diff --git a/northd/northd.c b/northd/northd.c
index 3f59894bd..be961fc8d 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -12463,6 +12463,18 @@ static bool check_bfd_state(const struct
nbrec_logical_router_policy *rule,
return true;
}
+static void
+build_routing_policy_match(const struct route_policy *rp, struct ds *match)
+{
+ ds_clear(match);
+ if (rp->chain_id == -1) {
+ ds_put_cstr(match, rp->rule->match);
+ } else {
+ ds_put_format(match, REG_POLICY_CHAIN_ID" == %d && (%s)",
+ rp->chain_id, rp->rule->match);
+ }
+}
+
static void
build_routing_policy_flow(struct lflow_table *lflows, struct ovn_datapath *od,
const struct hmap *lr_ports, struct route_policy *rp,
@@ -12525,12 +12537,7 @@ build_routing_policy_flow(struct lflow_table *lflows,
struct ovn_datapath *od,
ds_put_cstr(&actions, REG_ECMP_GROUP_ID" = 0; next;");
}
- if (rp->chain_id == -1) {
- ds_put_format(&match, "%s", rule->match);
- } else {
- ds_put_format(&match, REG_POLICY_CHAIN_ID" == %d && (%s)",
- rp->chain_id, rule->match);
- }
+ build_routing_policy_match(rp, &match);
ovn_lflow_add(lflows, od, S_ROUTER_IN_POLICY, rule->priority,
ds_cstr(&match), ds_cstr(&actions), lflow_ref,
@@ -12633,8 +12640,10 @@ build_ecmp_routing_policy_flows(struct lflow_table
*lflows,
"; %s = 1; next;", REG_ECMP_GROUP_ID,
ecmp_group_id, REG_ECMP_MEMBER_ID);
}
- ovn_lflow_add(lflows, od, S_ROUTER_IN_POLICY, rule->priority, rule->match,
- ds_cstr(&actions), lflow_ref, WITH_HINT(&rule->header_));
+ build_routing_policy_match(rp, &match);
+ ovn_lflow_add(lflows, od, S_ROUTER_IN_POLICY, rule->priority,
+ ds_cstr(&match), ds_cstr(&actions), lflow_ref,
+ WITH_HINT(&rule->header_));
cleanup:
ds_destroy(&match);
ds_destroy(&actions);
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index d9682f78e..ff0d87d77 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -4220,6 +4220,71 @@ OVN_CLEANUP_NORTHD
AT_CLEANUP
])
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([Router policies - ECMP chains])
+AT_KEYWORDS([policy-based-routing ecmp chains])
+ovn_start
+
+check ovn-nbctl lr-add lr0
+check ovn-nbctl lrp-add lr0 lr0-p0 00:00:00:00:00:01 \
+ 10.0.0.1/24 fd00::1/64
+check ovn-nbctl lr-route-add lr0 0.0.0.0/0 10.0.0.4
+check ovn-nbctl lr-route-add lr0 ::/0 fd00::4
+
+AT_CAPTURE_FILE([trace])
+check_policy() {
+ AT_CHECK([ovn-trace --detailed lr0 \
+ "inport == \"lr0-p0\" && eth.src == 00:00:00:00:00:10 && \
+ eth.dst == 00:00:00:00:00:01 && $family.src == $src && \
+ $family.dst == $dst && ip.ttl == 64" > trace], [0])
+ echo "pkt.mark = $mark;" > expout
+ AT_CHECK([grep "pkt.mark = " trace | sed 's/^ *//'], [0], [expout])
+}
+
+for family in ip4 ip6; do
+ if test "$family" = ip4; then
+ src=10.0.0.10
+ prefix=203.0.113.
+ nexthops=10.0.0.2,10.0.0.3
+ else
+ src=fd00::10
+ prefix=2001:db8::
+ nexthops=fd00::2,fd00::3
+ fi
+
+ dnl Without chains, ECMP should still select a next hop.
+ check ovn-nbctl --wait=sb lr-policy-add lr0 100 "$family" \
+ reroute "$nexthops" pkt_mark=100
+ dst=${prefix}10 mark=100 check_policy
+
+ dnl A main-chain ECMP rule must not capture traffic after a jump to a
+ dnl lower-priority allow in another chain.
+ check ovn-nbctl --chain=bypass lr-policy-add lr0 20 "1" \
+ allow pkt_mark=20
+ check ovn-nbctl --wait=sb lr-policy-add lr0 102 \
+ "$family.dst == ${prefix}10" jump bypass
+ dst=${prefix}10 mark=20 check_policy
+ dst=${prefix}11 mark=100 check_policy
+
+ dnl ECMP in a named chain must run only after a jump to that chain.
+ dnl It must not override lower-priority rules in the main or bypass chain.
+ check ovn-nbctl lr-policy-del lr0 100 "$family"
+ check ovn-nbctl --chain=ecmp lr-policy-add lr0 100 "$family" \
+ reroute "$nexthops" pkt_mark=100
+ check ovn-nbctl lr-policy-add lr0 90 "1" allow pkt_mark=90
+ check ovn-nbctl --wait=sb lr-policy-add lr0 102 \
+ "$family.dst == ${prefix}12" jump ecmp
+ dst=${prefix}10 mark=20 check_policy
+ dst=${prefix}11 mark=90 check_policy
+ dst=${prefix}12 mark=100 check_policy
+
+ check ovn-nbctl --wait=sb lr-policy-del lr0
+done
+
+OVN_CLEANUP_NORTHD
+AT_CLEANUP
+])
+
OVN_FOR_EACH_NORTHD_NO_HV([
AT_SETUP([Router policies - output_port])
ovn_start
--
2.56.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
