On 7/10/26 12:57 AM, Mairtin O'Loingsigh via dev wrote:
> Add a new "pass-related" action that behaves like "pass" for
> verdict (no verdict bit is set, so evaluation continues to
> the next tier) but follows the stateful path like
> "allow-related": it matches on hint registers, commits the
> connection via conntrack, and allows established/related
> traffic flows.
>
> Reported-at: https://redhat.atlassian.net/browse/FDP-3166
> Signed-off-by: Mairtin O'Loingsigh <[email protected]>
> ---
Hi Mairtin,
Thanks for this addition!
> NEWS | 3 +
> northd/en-acl-ids.c | 3 +-
> northd/en-ls-stateful.c | 5 +-
> northd/northd.c | 17 ++++--
> ovn-nb.ovsschema | 7 ++-
> ovn-nb.xml | 24 +++++++-
> tests/ovn-nbctl.at | 6 +-
> tests/ovn-northd.at | 84 +++++++++++++++++++++++++++
> tests/ovn.at | 123 ++++++++++++++++++++++++++++++++++++++++
> tests/system-ovn.at | 87 ++++++++++++++++++++++++++++
> utilities/ovn-nbctl.c | 19 ++++---
> 11 files changed, 353 insertions(+), 25 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index d1a6ad27f..d18800599 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,5 +1,8 @@
> Post v26.03.0
> -------------
> + - Added "pass-related" ACL action, a stateful version of "pass" that
> + commits connections and allows established/related traffic through
> + the fast-path.
Nit: "through the fast-path" reads weird to me. I'd just skip it,
I think.
> - Load balancer health checks now "fail closed" for newly added backends:
> ovn-northd initializes a newly created Service_Monitor row to the
> "offline" state, so traffic is not forwarded to a backend until its
> first
> diff --git a/northd/en-acl-ids.c b/northd/en-acl-ids.c
> index 09c7955cb..79e8156d2 100644
> --- a/northd/en-acl-ids.c
> +++ b/northd/en-acl-ids.c
> @@ -36,7 +36,8 @@ en_acl_id_init(struct engine_node *node OVS_UNUSED,
> static bool
> should_sync_to_sb(const struct nbrec_acl *nb_acl)
> {
> - return !strcmp(nb_acl->action, "allow-related") &&
> + return (!strcmp(nb_acl->action, "allow-related") ||
> + !strcmp(nb_acl->action, "pass-related")) &&
> smap_get_bool(&nb_acl->options,
> "persist-established",
> false);
> diff --git a/northd/en-ls-stateful.c b/northd/en-ls-stateful.c
> index 1127b7d50..ab97e917b 100644
> --- a/northd/en-ls-stateful.c
> +++ b/northd/en-ls-stateful.c
> @@ -468,8 +468,9 @@ ls_stateful_record_set_acls_(struct ls_stateful_record
> *ls_stateful_rec,
> const struct nbrec_acl *acl = acls[i];
> update_ls_max_acl_tier(ls_stateful_rec, acl);
> uuidset_insert(&ls_stateful_rec->related_acls, &acl->header_.uuid);
> - if (!ls_stateful_rec->has_stateful_acl
> - && !strcmp(acl->action, "allow-related")) {
> + if (!ls_stateful_rec->has_stateful_acl &&
> + (!strcmp(acl->action, "allow-related") ||
> + !strcmp(acl->action, "pass-related"))) {
> ls_stateful_rec->has_stateful_acl = true;
> }
> }
> diff --git a/northd/northd.c b/northd/northd.c
> index 8ce06da78..035be09d7 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -7116,7 +7116,8 @@ build_acl_log(struct ds *actions, const struct
> nbrec_acl *acl,
> ds_put_cstr(actions, "verdict=drop, ");
> } else if (!strcmp(acl->action, "reject")) {
> ds_put_cstr(actions, "verdict=reject, ");
> - } else if (!strcmp(acl->action, "pass")) {
> + } else if (!strcmp(acl->action, "pass") ||
> + !strcmp(acl->action, "pass-related")) {
> ds_put_cstr(actions, "verdict=pass, ");
> } else if (!strcmp(acl->action, "allow")
> || !strcmp(acl->action, "allow-related")
> @@ -7522,7 +7523,8 @@ build_acl_sample_flows(const struct ls_stateful_record
> *ls_stateful_rec,
> bool should_sample_established =
> ls_stateful_rec->has_stateful_acl
> && acl->sample_est
> - && !strcmp(acl->action, "allow-related");
> + && (!strcmp(acl->action, "allow-related") ||
> + !strcmp(acl->action, "pass-related"));
>
> bool stateful_match =
> ls_stateful_rec->has_stateful_acl
> @@ -7631,7 +7633,8 @@ consider_acl(struct lflow_table *lflows, const struct
> ovn_datapath *od,
> verdict = REGBIT_ACL_VERDICT_DROP " = 1; ";
> } else if (!strcmp(acl->action, "reject")) {
> verdict = REGBIT_ACL_VERDICT_REJECT " = 1; ";
> - } else if (!strcmp(acl->action, "pass")) {
> + } else if (!strcmp(acl->action, "pass") ||
> + !strcmp(acl->action, "pass-related")) {
> verdict = "";
> } else {
> verdict = REGBIT_ACL_VERDICT_ALLOW " = 1; ";
> @@ -7681,7 +7684,8 @@ consider_acl(struct lflow_table *lflows, const struct
> ovn_datapath *od,
> }
>
Above this, we shouldn't return early if the ACL is of type "pass" (when
we have stateful configs).
> if (!strcmp(acl->action, "allow")
> - || !strcmp(acl->action, "allow-related")) {
> + || !strcmp(acl->action, "allow-related")
> + || !strcmp(acl->action, "pass-related")) {
We should include "|| !strcmp(acl->action, "pass")" here too. That's to
maintain the same "<allow> becomes <allow-related> if there are stateful
configs on the switch" semantics for "<pass> -> <pass-related>".
> /* If there are any stateful flows, we must even commit "allow"
Then: s/even commit "allow"/also commit "allow" and "pass"/
> * actions. This is because, while the initiater's
> * direction may not have any stateful rules, the server's
> @@ -8035,8 +8039,9 @@ build_acl_log_related_flows(const struct ovn_datapath
> *od,
> return;
> }
>
> - if (strcmp(acl->action, "allow") && strcmp(acl->action,
> "allow-related")) {
> - /* Not an allow ACL */
> + if (strcmp(acl->action, "allow") && strcmp(acl->action, "allow-related")
> &&
> + strcmp(acl->action, "pass-related")) {
Missing 'strcmp(acl->action, "pass") && ' here, same as above, to maintain
pass->pass-related semantics for stateful switches.
> + /* Not an allow or pass-related ACL */
> return;
> }
>
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index e5945b831..626b3def5 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Northbound",
> - "version": "7.18.0",
> - "cksum": "1537030958 45190",
> + "version": "7.19.0",
> + "cksum": "956969323 45254",
> "tables": {
> "NB_Global": {
> "columns": {
> @@ -386,7 +386,8 @@
> "enum": ["set",
> ["allow", "allow-related",
> "allow-stateless", "drop",
> - "reject", "pass"]]}}},
> + "reject", "pass",
> + "pass-related"]]}}},
> "network_function_group": {
> "type": {"key": {"type": "uuid",
> "refTable": "Network_Function_Group",
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 33a6dc676..73f84706c 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -2888,7 +2888,7 @@ or
> For example, through this "label" we can backtrack to the ACL rule
> which is causing a "leaked" connection. Connection tracker entries
> are
> created only for allowed connections so the label is valid only
> - for allow and allow-related actions.
> + for allow, allow-related, and pass-related actions.
> </p>
>
> <p>
> @@ -2906,8 +2906,9 @@ or
> </p>
>
> <p>
> - Return traffic from an <code>allow-related</code> flow is always
> - allowed and cannot be changed through an ACL.
> + Return traffic from an <code>allow-related</code> or
> + <code>allow-related</code> flow is always allowed and cannot be
Typo? I assume the second one should be "<code>pass-related</code>".
> + changed through an ACL.
> </p>
>
> <p>
> @@ -3004,6 +3005,23 @@ or
> option from the <ref table="NB_Global" /> table is used to
> determine how to proceed.
> </li>
We should also update the <code>allow</code> to mention that the existence
of a "pass-related" ACL makes "allow" behave as "allow-related" too.
> +
> + <li>
> + <code>pass-related</code>: A stateful version of
> + <code>pass</code>. Like <code>pass</code>, a match on this
> + action stops evaluating ACLs at the current tier and moves to
> + the next one. Unlike <code>pass</code>, the connection is
> + committed through conntrack, and established or related return
> + traffic (e.g. inbound replies to an outbound connection) is
> + allowed to bypass subsequent ACL evaluation, the same way it
> + does for <code>allow-related</code>. If not using ACL tiers
> + or if a <code>pass-related</code> ACL is matched at the final
> + tier, then the
> + <ref column="options" key="default_acl_drop" table="NB_Global" />
> + option from the <ref table="NB_Global" /> table is used to
> + determine how to proceed.
> + </li>
> +
> </ul>
> </column>
>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index bf3896361..d33a2a675 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -222,7 +222,8 @@ ovn_nbctl_test_acl() {
> AT_CHECK([ovn-nbctl $2 acl-add $1 from-lport 200 ip drop])
> AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 100 ip drop])
> AT_CHECK([ovn-nbctl $2 --label=1234 acl-add $1 from-lport 70 icmp
> allow-related])
> - AT_CHECK([ovn-nbctl $2 --label=1235 acl-add $1 to-lport 70 icmp
> allow-related])
> + AT_CHECK([ovn-nbctl $2 --tier=0 --label=1235 acl-add $1 to-lport 80 icmp
> pass-related])
> + AT_CHECK([ovn-nbctl $2 --tier=1 --label=1235 acl-add $1 to-lport 70 icmp
> allow-related])
> AT_CHECK([ovn-nbctl $2 --apply-after-lb acl-add $1 from-lport 500 tcp
> allow])
> AT_CHECK([ovn-nbctl $2 --apply-after-lb acl-add $1 from-lport 300 tcp
> drop])
> AT_CHECK([ovn-nbctl $2 --apply-after-lb acl-add $1 from-lport 300 udp
> allow])
> @@ -256,7 +257,8 @@ from-lport 300 (udp) allow [[tier 0]] [[after-lb]]
> to-lport 500 (udp) drop log(name=test,severity=info) [[tier 0]]
> to-lport 300 (tcp) drop [[tier 0]]
> to-lport 100 (ip) drop [[tier 0]]
> - to-lport 70 (icmp) allow-related label=1235 [[tier 0]]
> + to-lport 80 (icmp) pass-related label=1235 [[tier 0]]
> + to-lport 70 (icmp) allow-related label=1235 [[tier 1]]
> ])
>
> dnl Delete in one direction.
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 59ac23cdc..d2140ca09 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -11574,6 +11574,90 @@ OVN_CLEANUP_NORTHD
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([ACL "pass-related" logical flows])
> +AT_KEYWORDS([acl])
> +
> +ovn_start
> +check ovn-nbctl ls-add ls
> +check ovn-nbctl lsp-add ls lsp
> +check ovn-nbctl pg-add pg lsp
> +
> +m4_define([ACL_FLOWS], [grep -w $1 lflows | grep "$2" | ovn_strip_lflows |
> sed "s/\($1[[^)]]*\)/$1/" ])
> +
> +acl_test() {
> + direction=$1
> + options=$2
> + thing=$3
> + eval_stage=$4
> +
> + ovn-sbctl lflow-list ls > lflows
> + AT_CHECK([ACL_FLOWS([$eval_stage], [priority=2000])], [0], [])
> +
> + # Unlike "pass" which generates a single stateless flow, "pass-related"
> + # goes through the stateful path and generates two flows: one matching
> + # REGBIT_ACL_HINT_ALLOW_NEW and one matching REGBIT_ACL_HINT_ALLOW.
> + # Neither sets a verdict bit.
> + check ovn-nbctl --wait=sb $options acl-add $thing $direction 1000
> "ip4.addr == 80.111.111.112" pass-related
> + ovn-sbctl lflow-list ls > lflows
> + AT_CHECK_UNQUOTED([ACL_FLOWS([$eval_stage], [priority=2000])], [0], [dnl
> + table=??($eval_stage), priority=2000 , match=(reg0[[7]] == 1 && (ip4.addr
> == 80.111.111.112)), action=(next;)
> + table=??($eval_stage), priority=2000 , match=(reg0[[8]] == 1 && (ip4.addr
> == 80.111.111.112)), action=(next;)
> +])
> +
> + check ovn-nbctl --wait=sb acl-del $thing
> + ovn-sbctl lflow-list ls > lflows
> + AT_CHECK([ACL_FLOWS([$eval_stage], [priority=2000])], [0], [])
> +}
> +
> +acl_test from-lport "" ls ls_in_acl_eval
> +acl_test from-lport "--apply-after-lb" ls ls_in_acl_after_lb_eval
> +acl_test to-lport "" ls ls_out_acl_eval
> +acl_test from-lport "" pg ls_in_acl_eval
> +acl_test from-lport "--apply-after-lb" pg ls_in_acl_after_lb_eval
> +acl_test to-lport "" pg ls_out_acl_eval
> +
> +OVN_CLEANUP_NORTHD
> +AT_CLEANUP
> +])
> +
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([ACL "pass-related" with label])
> +AT_KEYWORDS([acl])
> +
> +ovn_start
> +
> +check ovn-nbctl \
> + -- ls-add ls \
> + -- lsp-add ls lsp1 \
> + -- lsp-set-addresses lsp1 "00:00:00:00:00:01" \
> + -- lsp-add ls lsp2 \
> + -- lsp-set-addresses lsp2 "00:00:00:00:00:02" \
> + -- lb-add lb-test 42.42.42.42:4242 43.43.43.43:4343 udp \
> + -- ls-lb-add ls lb-test
> +
> +check ovn-nbctl --wait=sb --label=1234 acl-add ls from-lport 1 ip
> pass-related
> +
> +ovn-sbctl dump-flows ls > lflows
> +AT_CAPTURE_FILE([lflows])
> +
> +dnl Verify label registers are set. Unlike allow-related, no verdict bit
> +dnl (reg8[[16]]) appears in the action.
> +AT_CHECK([grep -w "ls_in_acl_eval" lflows | grep 1001 | ovn_strip_lflows],
> [0], [dnl
> + table=??(ls_in_acl_eval ), priority=1001 , match=(reg0[[7]] == 1 &&
> (ip)), action=(reg0[[13]] = 1; reg3 = 1234; reg9 = 1234; reg8[[0..7]] = 0;
> reg8[[8..15]] = 0; reg8[[19..20]] = 0; next;)
> + table=??(ls_in_acl_eval ), priority=1001 , match=(reg0[[8]] == 1 &&
> (ip)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; reg9 = 1234;
> reg8[[0..7]] = 0; reg8[[8..15]] = 0; reg8[[19..20]] = 0; next;)
> +])
> +
> +dnl Verify commit happens via ovn_trace.
> +flow="inport == \"lsp1\" && eth.src == 00:00:00:00:00:01 && eth.dst ==
> 00:00:00:00:00:02 && ip4.src == 42.42.42.1 && ip4.dst == 42.42.42.42 &&
> ip.ttl==64 && udp.dst == 4242"
> +AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --ct new ls "$flow" | grep -e
> ls_in_stateful -A 2 | grep commit], [0], [dnl
> + ct_commit { ct_mark.blocked = 0; ct_mark.allow_established = reg0[[20]];
> ct_mark.obs_stage = reg8[[19..20]]; ct_mark.obs_collector_id = reg8[[8..15]];
> ct_label.obs_point_id = reg9; ct_label.acl_id = reg2[[16..31]]; ct_label.nf =
> 0; ct_label.nf_id = 0; };
> +])
> +
> +OVN_CLEANUP_NORTHD
> +AT_CLEANUP
> +])
> +
> OVN_FOR_EACH_NORTHD_NO_HV([
> AT_SETUP([ACL - ct state clear on router ports])
> AT_KEYWORDS([acl])
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 286395298..0455753b3 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -43672,6 +43672,129 @@ OVN_CLEANUP_NORTHD
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([ACL "pass-related" with drop on same tier])
> +AT_KEYWORDS([acl])
> +
> +ovn_start
> +
> +check ovn-nbctl \
> + -- ls-add ls \
> + -- lsp-add ls lsp1 \
> + -- lsp-set-addresses lsp1 "00:00:00:00:00:01 10.0.0.1" \
Nit: alignment (same thing 3 times in this file).
> + -- lsp-add ls lsp2 \
> + -- lsp-set-addresses lsp2 "00:00:00:00:00:02 10.0.0.2"
> +
> +dnl Tier 0: pass-related at priority 500, drop at priority 450.
> +dnl Tier 1: allow-related at priority 400.
> +check ovn-nbctl --wait=sb \
> + --tier=0 acl-add ls from-lport 500 "ip4.src == 10.0.0.1" pass-related \
> + -- --tier=0 acl-add ls from-lport 450 "ip4" drop \
> + -- --tier=1 acl-add ls from-lport 400 "ip4" allow-related
> +
> +flow="inport == \"lsp1\" && eth.src == 00:00:00:00:00:01 && eth.dst ==
> 00:00:00:00:00:02 && ip4.src == 10.0.0.1 && ip4.dst == 10.0.0.2 && ip.ttl ==
> 64 && udp.dst == 80"
> +
> +dnl Verify pass-related generates stateful-path flows with hint register
> +dnl matches, unlike pass which uses the stateless path.
> +ovn-sbctl lflow-list ls > lflows
> +AT_CAPTURE_FILE([lflows])
> +AT_CHECK([grep -w "ls_in_acl_eval" lflows | grep "10.0.0.1" | grep -qF
> "reg0"])
> +
> +dnl New connection: pass-related on tier 0 sets no verdict, falls through to
> +dnl tier 1 where allow-related sets VERDICT_ALLOW. The ingress stateful stage
> +dnl commits the connection.
> +AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new ls "$flow" | grep -e
> ls_in_stateful -A 2 | grep commit], [0], [dnl
> + ct_commit { ct_mark.blocked = 0; ct_mark.allow_established = reg0[[20]];
> ct_label.acl_id = reg2[[16..31]]; ct_label.nf = 0; ct_label.nf_id = 0; };
> +])
> +
> +dnl Established reply: hits the priority-65532 fast-path, bypasses the
> +dnl drop ACL entirely. Verify output is reached.
> +AT_CHECK([ovn_trace --ct est,rpl --ct est,rpl --minimal ls "$flow" | grep -q
> 'output("lsp2")'])
> +
> +OVN_CLEANUP_NORTHD
> +AT_CLEANUP
> +])
> +
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([ACL "pass-related" multi-tier])
> +AT_KEYWORDS([acl])
> +
> +ovn_start
> +
> +check ovn-nbctl \
> + -- ls-add ls \
> + -- lsp-add ls lsp1 \
> + -- lsp-set-addresses lsp1 "00:00:00:00:00:01 10.0.0.1" \
> + -- lsp-add ls lsp2 \
> + -- lsp-set-addresses lsp2 "00:00:00:00:00:02 10.0.0.2"
> +
> +dnl Tier 0: pass-related. Tier 1: allow-related.
> +check ovn-nbctl --wait=sb --tier=0 acl-add ls from-lport 500 "ip4.src ==
> 10.0.0.1" pass-related
> +check ovn-nbctl --wait=sb --tier=1 acl-add ls from-lport 400 "ip4"
> allow-related
> +
> +flow="inport == \"lsp1\" && eth.src == 00:00:00:00:00:01 && eth.dst ==
> 00:00:00:00:00:02 && ip4.src == 10.0.0.1 && ip4.dst == 10.0.0.2 && ip.ttl ==
> 64 && udp.dst == 80"
> +
> +dnl Verify tier increment flow exists in the action stage.
> +ovn-sbctl lflow-list ls > lflows
> +AT_CAPTURE_FILE([lflows])
> +AT_CHECK([grep -w "ls_in_acl_action" lflows | grep "priority=500" |
> ovn_strip_lflows | grep -q "next(pipeline=ingress,table=??)"])
> +dnl New connection passes tier 0 (pass-related) and tier 1 (allow-related),
> +dnl gets committed in the ingress stateful stage.
> +AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new ls "$flow" | grep -e
> ls_in_stateful -A 2 | grep commit], [0], [dnl
> + ct_commit { ct_mark.blocked = 0; ct_mark.allow_established = reg0[[20]];
> ct_label.acl_id = reg2[[16..31]]; ct_label.nf = 0; ct_label.nf_id = 0; };
> +])
> +
> +dnl Established reply uses the fast-path.
> +AT_CHECK([ovn_trace --ct est,rpl --ct est,rpl --minimal ls "$flow" | grep -q
> 'output("lsp2")'])
> +
> +dnl Remove the tier 1 ACL. With only tier 0 pass-related and no explicit
> +dnl tier 1 ACL, the default action (allow) applies after the pass.
> +check ovn-nbctl --wait=sb acl-del ls
> +check ovn-nbctl --wait=sb \
> + --tier=0 acl-add ls from-lport 500 "ip4.src == 10.0.0.1" pass-related
> +
> +dnl pass-related alone makes the switch stateful, so the connection
> +dnl gets committed even without an explicit tier 1 ACL.
> +AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new ls "$flow" | grep -e
> ls_in_stateful -A 2 | grep commit], [0], [dnl
> + ct_commit { ct_mark.blocked = 0; ct_mark.allow_established = reg0[[20]];
> ct_label.acl_id = reg2[[16..31]]; ct_label.nf = 0; ct_label.nf_id = 0; };
> +])
> +
> +OVN_CLEANUP_NORTHD
> +AT_CLEANUP
> +])
> +
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([ACL "pass-related" ICMP related traffic])
> +AT_KEYWORDS([acl])
> +
> +ovn_start
> +
> +check ovn-nbctl \
> + -- ls-add ls \
> + -- lsp-add ls lsp1 \
> + -- lsp-set-addresses lsp1 "00:00:00:00:00:01 10.0.0.1" \
> + -- lsp-add ls lsp2 \
> + -- lsp-set-addresses lsp2 "00:00:00:00:00:02 10.0.0.2"
> +
> +dnl Tier 0: pass-related at priority 500, drop at priority 450.
> +dnl Tier 1: allow-related at priority 400.
> +check ovn-nbctl --wait=sb \
> + --tier=0 acl-add ls from-lport 500 "ip4.src == 10.0.0.1" pass-related \
> + -- --tier=0 acl-add ls from-lport 450 "ip4" drop \
> + -- --tier=1 acl-add ls from-lport 400 "ip4" allow-related
> +
> +flow="inport == \"lsp1\" && eth.src == 00:00:00:00:00:01 && eth.dst ==
> 00:00:00:00:00:02 && ip4.src == 10.0.0.1 && ip4.dst == 10.0.0.2 && ip.ttl ==
> 64 && udp.dst == 80"
> +
> +dnl Related traffic (e.g., ICMP destination-unreachable) for a committed
> +dnl connection hits the ct.rel fast-path at priority 65532 and is allowed
> +dnl through with ct_commit_nat, bypassing the drop ACL on tier 0.
> +AT_CHECK([ovn_trace --ct rel --ct rel ls "$flow" | grep -q "ct_commit_nat"])
> +AT_CHECK([ovn_trace --ct rel --ct rel --minimal ls "$flow" | grep -q
> 'output("lsp2")'])
> +
> +OVN_CLEANUP_NORTHD
> +AT_CLEANUP
> +])
> +
> OVN_FOR_EACH_NORTHD_NO_HV([
> AT_SETUP([ovn-controller -- local datapaths check])
> ovn_start
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 17b4dcb3d..8c7685b9b 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -21859,6 +21859,93 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port
> patch-.*/d
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ACL pass-related conntrack])
> +AT_KEYWORDS([acl])
> +
> +CHECK_CONNTRACK()
> +ovn_start
> +
> +OVS_TRAFFIC_VSWITCHD_START()
> +ADD_BR([br-int])
> +
> +check ovs-vsctl \
> + -- set Open_vSwitch . external-ids:system-id=hv1 \
> + -- set Open_vSwitch .
> external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> + -- set bridge br-int fail-mode=secure
> other-config:disable-in-band=true
> +
> +start_daemon ovn-controller
> +
> +check ovn-nbctl ls-add sw0
> +
> +check ovn-nbctl lsp-add sw0 sw0-p1
> +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3"
> +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3"
> +
> +check ovn-nbctl lsp-add sw0 sw0-p2
> +check ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4"
> +check ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4"
> +
> +ADD_NAMESPACES(sw0-p1)
> +ADD_VETH(sw0-p1, sw0-p1, br-int, "10.0.0.3/24", "50:54:00:00:00:03", \
> + "10.0.0.1")
> +
> +ADD_NAMESPACES(sw0-p2)
> +ADD_VETH(sw0-p2, sw0-p2, br-int, "10.0.0.4/24", "50:54:00:00:00:04", \
> + "10.0.0.1")
> +
> +wait_for_ports_up sw0-p1 sw0-p2
> +check ovn-nbctl --wait=hv sync
> +
> +dnl Phase 1: pass-related alone creates conntrack entries.
Nit: I'd avoid numbering comments. If we ever need to add stuff in the
middle of the list we have to update everything that comes after.
> +dnl Unlike plain "pass", pass-related makes the switch stateful and commits
> +dnl connections to conntrack even without an explicit allow-related ACL.
> +check ovn-nbctl --wait=sb acl-add sw0 from-lport 1000 "ip4.src == 10.0.0.3"
> pass-related
> +check ovn-nbctl --wait=hv sync
> +
> +NS_CHECK_EXEC([sw0-p1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.4 | FORMAT_PING], \
> +[0], [dnl
> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.4) | \
> +sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | grep icmp], [0], [ignore])
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \
> +sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | grep icmp], [0], [ignore])
> +
> +dnl Phase 2: Multi-tier with drop bypass.
> +dnl pass-related at higher priority skips the drop, falls through to tier 1
> +dnl where allow-related commits the connection. Reply traffic uses
> +dnl the established fast-path, bypassing the tier 0 drop ACL.
> +check ovn-nbctl acl-del sw0
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack])
> +check ovn-nbctl --wait=hv sync
> +
> +check ovn-nbctl --tier=0 acl-add sw0 from-lport 1000 "ip4.src == 10.0.0.3"
> pass-related
> +check ovn-nbctl --tier=0 acl-add sw0 from-lport 500 "ip4" drop
> +check ovn-nbctl --tier=1 acl-add sw0 from-lport 1000 "ip4" allow-related
The actual test should be "pass-related" (tier 0), "allow-related" (tier 1).
Like that we really test the pass-related semantics. But I tried it out
and it worked like that too.
> +check ovn-nbctl --wait=hv sync
> +
> +NS_CHECK_EXEC([sw0-p1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.4 | FORMAT_PING], \
> +[0], [dnl
> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.4) | \
> +sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | grep icmp], [0], [ignore])
> +
> +OVN_CLEANUP_CONTROLLER([hv1])
> +OVN_CLEANUP_NORTHD
> +
> +as
> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> +/connection dropped.*/d"])
> +
> +AT_CLEANUP
> +])
> +
> OVN_FOR_EACH_NORTHD([
> AT_SETUP([DHCPv6 - Options heap overread])
> CHECK_SCAPY
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 4e2271d07..b3a0290c9 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -2951,10 +2951,11 @@ nbctl_acl_add(struct ctl_context *ctx)
> /* Validate action. */
> if (strcmp(action, "allow") && strcmp(action, "allow-related")
> && strcmp(action, "allow-stateless") && strcmp(action, "drop")
> - && strcmp(action, "reject") && strcmp(action, "pass")) {
> + && strcmp(action, "reject") && strcmp(action, "pass")
> + && strcmp(action, "pass-related")) {
> ctl_error(ctx, "%s: action must be one of \"allow\", "
> "\"allow-related\", \"allow-stateless\", \"drop\", "
> - "and \"reject\"", action);
> + "\"pass\", \"pass-related\", and \"reject\"", action);
> return;
> }
>
> @@ -3027,12 +3028,14 @@ nbctl_acl_add(struct ctl_context *ctx)
> /* Set the ACL label */
> const char *label = shash_find_data(&ctx->options, "--label");
> if (label) {
> - /* Ensure that the action is either allow or allow-related */
> - if (strcmp(action, "allow") && strcmp(action, "allow-related")) {
> - ctl_error(ctx, "label can only be set with actions \"allow\" or "
> - "\"allow-related\"");
> - return;
> - }
> + /* Ensure that the action is either allow, allow-related or
> + * pass-related */
> + if (strcmp(action, "allow") && strcmp(action, "allow-related") &&
> + strcmp(action, "pass-related")) {
> + ctl_error(ctx, "label can only be set with actions \"allow\", "
> + "\"allow-related\" or \"pass-related\"");
> + return;
> + }
>
> int64_t label_value = 0;
> error = parse_acl_label(label, &label_value);
We might as well fix the indent for the following chunk while at it.
All of the above are relatively minor changes that I had to implement
anyway when reviewing the patch. I have them in this diff below.
Also already folded in here:
https://github.com/dceara/ovn/commit/90c4a6e
I was thinking of just squashing that it into your patch and pushing
it to main. But then I got a bit worried about the "pass->pass-related"
implicit behavior change when stateful ACLs (or LBs) are configured
on the switch.
I even had to change the tier-mismatch system test due to this
behavior change (I think that test change is fine). While I'm quite
confident that these modifications are correct maybe it's better
to play it safe and post a v2 with all this feedback incorporated.
We could try to run more validation on it, e.g., full upstream
ovn-kubernetes CI.
What do you think?
Regards,
Dumitru
---
diff --git a/NEWS b/NEWS
index 9ab5e7ddb3..70ef6d3778 100644
--- a/NEWS
+++ b/NEWS
@@ -1,8 +1,7 @@
Post v26.03.0
-------------
- Added "pass-related" ACL action, a stateful version of "pass" that
- commits connections and allows established/related traffic through
- the fast-path.
+ commits connections and allows established/related traffic.
- Load balancer health checks now "fail closed" for newly added backends:
ovn-northd initializes a newly created Service_Monitor row to the
"offline" state, so traffic is not forwarded to a backend until its first
diff --git a/northd/northd.c b/northd/northd.c
index 5f10b1b68b..a14fd2f135 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -7660,9 +7660,7 @@ consider_acl(struct lflow_table *lflows, const struct
ovn_datapath *od,
* translation only affects L4 port fields in ovn-controller. */
bool needs_ct_trans = has_stateful && acl_ct_translation;
- if (!has_stateful
- || !strcmp(acl->action, "pass")
- || !strcmp(acl->action, "allow-stateless")) {
+ if (!has_stateful || !strcmp(acl->action, "allow-stateless")) {
if (acl->network_function_group) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
@@ -7684,10 +7682,11 @@ consider_acl(struct lflow_table *lflows, const struct
ovn_datapath *od,
}
if (!strcmp(acl->action, "allow")
+ || !strcmp(acl->action, "pass")
|| !strcmp(acl->action, "allow-related")
|| !strcmp(acl->action, "pass-related")) {
- /* If there are any stateful flows, we must even commit "allow"
- * actions. This is because, while the initiater's
+ /* If there are any stateful flows, we must also commit "allow"
+ * and "pass" actions. This is because, while the initiater's
* direction may not have any stateful rules, the server's
* may and then its return traffic would not have an
* associated conntrack entry and would return "+invalid". */
@@ -8040,7 +8039,7 @@ build_acl_log_related_flows(const struct ovn_datapath *od,
}
if (strcmp(acl->action, "allow") && strcmp(acl->action, "allow-related") &&
- strcmp(acl->action, "pass-related")) {
+ strcmp(acl->action, "pass") && strcmp(acl->action, "pass-related")) {
/* Not an allow or pass-related ACL */
return;
}
@@ -8257,8 +8256,9 @@ build_acls(const struct ls_stateful_record
*ls_stateful_rec,
ds_cstr(&match), ct_in_acl_action, lflow_ref);
ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL_EVAL, UINT16_MAX - 3,
ds_cstr(&match), ct_out_acl_action, lflow_ref);
- /* Reply and related traffic matched by an "allow-related" ACL
- * should be allowed in the ls_in_acl_after_lb stage too. */
+ /* Reply and related traffic matched by an "allow-related" or
+ * "pass-related" ACL should be allowed in the ls_in_acl_after_lb
+ * stage too. */
ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL_AFTER_LB_EVAL,
UINT16_MAX - 3,
REGBIT_ACL_HINT_ALLOW_REL" == 1",
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 73f84706c6..bd0c25b10b 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -2907,7 +2907,7 @@ or
<p>
Return traffic from an <code>allow-related</code> or
- <code>allow-related</code> flow is always allowed and cannot be
+ <code>pass-related</code> flow is always allowed and cannot be
changed through an ACL.
</p>
@@ -2977,8 +2977,9 @@ or
<li>
<code>allow</code>: Forward the packet. It will also send the
packets through connection tracking when
- <code>allow-related</code> rules exist on the logical switch.
- Otherwise, it's equivalent to <code>allow-stateless</code>.
+ <code>allow-related</code> or <code>pass-related</code> rules
+ exist on the logical switch. Otherwise, it's equivalent to
+ <code>allow-stateless</code>.
</li>
<li>
@@ -3003,7 +3004,9 @@ or
<code>pass</code> ACL is matched at the final tier, then the
<ref column="options" key="default_acl_drop" table="NB_Global" />
option from the <ref table="NB_Global" /> table is used to
- determine how to proceed.
+ determine how to proceed. Behaves as <code>pass-related</code>
+ if <code>allow-related</code> or <code>pass-related</code> rules
+ exist on the logical switch.
</li>
<li>
diff --git a/tests/ovn.at b/tests/ovn.at
index 3f4c07a59b..75ef85109a 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -43686,7 +43686,7 @@ ovn_start
check ovn-nbctl \
-- ls-add ls \
-- lsp-add ls lsp1 \
- -- lsp-set-addresses lsp1 "00:00:00:00:00:01 10.0.0.1" \
+ -- lsp-set-addresses lsp1 "00:00:00:00:00:01 10.0.0.1" \
-- lsp-add ls lsp2 \
-- lsp-set-addresses lsp2 "00:00:00:00:00:02 10.0.0.2"
@@ -43729,7 +43729,7 @@ ovn_start
check ovn-nbctl \
-- ls-add ls \
-- lsp-add ls lsp1 \
- -- lsp-set-addresses lsp1 "00:00:00:00:00:01 10.0.0.1" \
+ -- lsp-set-addresses lsp1 "00:00:00:00:00:01 10.0.0.1" \
-- lsp-add ls lsp2 \
-- lsp-set-addresses lsp2 "00:00:00:00:00:02 10.0.0.2"
@@ -43777,7 +43777,7 @@ ovn_start
check ovn-nbctl \
-- ls-add ls \
-- lsp-add ls lsp1 \
- -- lsp-set-addresses lsp1 "00:00:00:00:00:01 10.0.0.1" \
+ -- lsp-set-addresses lsp1 "00:00:00:00:00:01 10.0.0.1" \
-- lsp-add ls lsp2 \
-- lsp-set-addresses lsp2 "00:00:00:00:00:02 10.0.0.2"
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 8c7685b9b9..2761e0dde4 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -13416,11 +13416,10 @@ check_row_count nb:Sampling_App 2
dnl Create two tiers of ACLs.
dnl The first ACL is an ingress "pass" ACL at tier 0.
-check_uuid ovn-nbctl \
- -- --id=@sample_1_new create Sample collector="$collector1" metadata=1001 \
- -- --id=@sample_1_est create Sample collector="$collector1" metadata=1002 \
- -- --tier=0 --sample-new=@sample_1_new --sample-est=@sample_1_est \
- acl-add ls from-lport 1 "inport == \"vm1\" && udp.dst == 1000" \
+check_uuid ovn-nbctl \
+ -- --id=@sample_new create Sample collector="$collector1" metadata=1001 \
+ -- --tier=0 --sample-new=@sample_new \
+ acl-add ls from-lport 1 "inport == \"vm1\" && udp.dst == 1000" \
pass
dnl The second ACL is an unrelated egress ACL. However, it uses tier 1 instead
@@ -13428,7 +13427,7 @@ dnl of tier 0.
check ovn-nbctl --tier=1 acl-add ls to-lport 1 "inport == \"vm1\" && udp.dst
== 1000" allow-related
check_row_count nb:ACL 2
-check_row_count nb:Sample 2
+check_row_count nb:Sample 1
dnl Wait for ovn-controller to catch up.
wait_for_ports_up
@@ -21870,11 +21869,11 @@ OVS_TRAFFIC_VSWITCHD_START()
ADD_BR([br-int])
check ovs-vsctl \
- -- set Open_vSwitch . external-ids:system-id=hv1 \
- -- set Open_vSwitch .
external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
- -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
- -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
- -- set bridge br-int fail-mode=secure other-config:disable-in-band=true
+ -- set Open_vSwitch . external-ids:system-id=hv1 \
+ -- set Open_vSwitch .
external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
+ -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
+ -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
+ -- set bridge br-int fail-mode=secure other-config:disable-in-band=true
start_daemon ovn-controller
@@ -21899,7 +21898,7 @@ ADD_VETH(sw0-p2, sw0-p2, br-int, "10.0.0.4/24",
"50:54:00:00:00:04", \
wait_for_ports_up sw0-p1 sw0-p2
check ovn-nbctl --wait=hv sync
-dnl Phase 1: pass-related alone creates conntrack entries.
+dnl Test that pass-related alone creates conntrack entries.
dnl Unlike plain "pass", pass-related makes the switch stateful and commits
dnl connections to conntrack even without an explicit allow-related ACL.
check ovn-nbctl --wait=sb acl-add sw0 from-lport 1000 "ip4.src == 10.0.0.3"
pass-related
@@ -21915,7 +21914,7 @@ sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | grep icmp],
[0], [ignore])
AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \
sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | grep icmp], [0], [ignore])
-dnl Phase 2: Multi-tier with drop bypass.
+dnl Test multi-tier with drop bypass due to pass-related ACL.
dnl pass-related at higher priority skips the drop, falls through to tier 1
dnl where allow-related commits the connection. Reply traffic uses
dnl the established fast-path, bypassing the tier 0 drop ACL.
@@ -21925,6 +21924,28 @@ check ovn-nbctl --wait=hv sync
check ovn-nbctl --tier=0 acl-add sw0 from-lport 1000 "ip4.src == 10.0.0.3"
pass-related
check ovn-nbctl --tier=0 acl-add sw0 from-lport 500 "ip4" drop
+check ovn-nbctl --tier=1 acl-add sw0 from-lport 1000 "ip4" allow
+check ovn-nbctl --wait=hv sync
+
+NS_CHECK_EXEC([sw0-p1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.4 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.4) | \
+sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | grep icmp], [0], [ignore])
+
+dnl Test multi-tier with drop bypass due to allow-related ACL.
+dnl pass (behaving as pass-related due to allow-related) at higher
+dnl priority skips the drop, falls through to tier 1 where allow-related
+dnl commits the connection. Reply traffic uses the established fast-path,
+dnl bypassing the tier 0 drop ACL.
+check ovn-nbctl acl-del sw0
+AT_CHECK([ovs-appctl dpctl/flush-conntrack])
+check ovn-nbctl --wait=hv sync
+
+check ovn-nbctl --tier=0 acl-add sw0 from-lport 1000 "ip4.src == 10.0.0.3" pass
+check ovn-nbctl --tier=0 acl-add sw0 from-lport 500 "ip4" drop
check ovn-nbctl --tier=1 acl-add sw0 from-lport 1000 "ip4" allow-related
check ovn-nbctl --wait=hv sync
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index b3a0290c93..ad6940d79c 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -3033,17 +3033,17 @@ nbctl_acl_add(struct ctl_context *ctx)
if (strcmp(action, "allow") && strcmp(action, "allow-related") &&
strcmp(action, "pass-related")) {
ctl_error(ctx, "label can only be set with actions \"allow\", "
- "\"allow-related\" or \"pass-related\"");
+ "\"allow-related\" or \"pass-related\"");
return;
}
- int64_t label_value = 0;
- error = parse_acl_label(label, &label_value);
- if (error) {
- ctx->error = error;
- return;
- }
- nbrec_acl_set_label(acl, label_value);
+ int64_t label_value = 0;
+ error = parse_acl_label(label, &label_value);
+ if (error) {
+ ctx->error = error;
+ return;
+ }
+ nbrec_acl_set_label(acl, label_value);
}
if (!strcmp(direction, "from-lport") &&
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
