On Fri, Jul 10, 2026 at 05:29:09PM +0200, Dumitru Ceara wrote: > 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") && >
Hi Dumitru, Thanks for the review. I agree that a full validation run should happen for this many changes. Ill update the commit and submit a v2. Regards, Mairtin _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
