A "pass" ACL with sampling enabled stores its observation point and
collector IDs in the ACL observation registers (REGBIT_ACL_LABEL,
REG_OBS_POINT_ID_*, REG_OBS_COLLECTOR_ID_*) and gets sampled in the
sample stage of its own tier. The tier advance flow in the ACL action
stage left those registers untouched, so they were still set when the
ACLs of the next tier were evaluated:
- the generic new-connection sampler matched again on the collector ID
recorded by the "pass" ACL, so a new connection decided by an
unsampled ACL in the next tier was sampled twice, both times with the
"pass" ACL's observation point;
- the conntrack commit of a connection allowed by an unsampled
allow-related ACL in the next tier stored the "pass" ACL's observation
data in the ct_label. With sample_est configured on the "pass" ACL,
the established traffic of that connection was then attributed to the
"pass" ACL.
Add a tier advance flow at priority 550 that clears the observation
registers before moving to the next tier when they were populated by
the current ACL stage (REGBIT_ACL_LABEL == 1 and REGBIT_ACL_OBS_STAGE
equal to the stage). The only ACL action that populates those registers
without a verdict is "pass", so this precisely removes the "pass" ACL's
observation data. Data recorded by another stage, e.g. by a from-lport
ACL while the from-lport-after-lb tier advances, is left in place
because the conntrack commit in the stateful stage still needs it.
The "pass" ACL itself is still sampled once in its own tier. The
expectations of the existing tiered ACL sampling tests do not change.
Fixes: d15b12da6fe6 ("northd: Add ACL Sampling.")
Assisted-by: OpenAI Codex
Assisted-by: Claude Code
Signed-off-by: Chanyeol Yoon <[email protected]>
---
NEWS | 3 ++
northd/northd.c | 39 ++++++++++++++++++++
ovn-nb.xml | 9 +++++
tests/ovn-northd.at | 87 +++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 138 insertions(+)
diff --git a/NEWS b/NEWS
index 4176f8d7f..d41626a90 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,8 @@
Post v26.09.0
--------------
+ - ACL sampling: observation data recorded by a sampled "pass" ACL is no
+ longer inherited by the ACL that determines the verdict in the next
+ tier.
- Learn ARP sender hardware address (arp.sha) and IPv6 Neighbor
Discovery link-layer addresses (nd.tll, nd.sll) into the FDB
table in addition to eth.src. This allows VRRP virtual MACs to
diff --git a/northd/northd.c b/northd/northd.c
index 3f59894bd..ccbf1e759 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -7640,6 +7640,19 @@ build_acl_sample_label_action(struct ds *actions, const
struct nbrec_acl *acl,
(uint8_t) obs_stage);
}
+/* This builds the ACL action that clears the observation point and collector
+ * registers populated by build_acl_sample_label_action().
+ */
+static void
+build_acl_sample_label_clear_action(struct ds *actions)
+{
+ ds_put_cstr(actions, REGBIT_ACL_LABEL" = 0; "
+ REG_OBS_POINT_ID_NEW " = 0; "
+ REG_OBS_POINT_ID_EST " = 0; "
+ REG_OBS_COLLECTOR_ID_NEW " = 0; "
+ REG_OBS_COLLECTOR_ID_EST " = 0; ");
+}
+
/* This builds an ACL logical flow specific match that selects traffic
* with an associated observation point ID register equal to that of the
* ACL label (if configured) or sample->metadata.
@@ -8372,6 +8385,12 @@ build_acl_action_lflows(const struct ls_stateful_record
*ls_stateful_rec,
ls_stateful_rec->max_acl_tier.egress,
};
+ enum acl_observation_stage obs_stages[] = {
+ ACL_OBS_FROM_LPORT,
+ ACL_OBS_FROM_LPORT_AFTER_LB,
+ ACL_OBS_TO_LPORT,
+ };
+
ds_clear(actions);
ds_put_cstr(actions, REGBIT_ACL_VERDICT_ALLOW " = 0; "
REGBIT_ACL_VERDICT_DROP " = 0; "
@@ -8425,6 +8444,26 @@ build_acl_action_lflows(const struct ls_stateful_record
*ls_stateful_rec,
ovn_stage_get_table(eval_stages[i]));
ovn_lflow_add(lflows, od, stage, 500, ds_cstr(match),
ds_cstr(&tier_actions), lflow_ref);
+
+ /* A "pass" ACL with sampling enabled records its observation
+ * point in registers and gets sampled in the sample stage of
+ * its own tier. Clear that observation data before moving to
+ * the next tier so that it is not attributed to the verdict of
+ * an ACL in a later tier. Observation data recorded by another
+ * ACL stage (e.g., by a from-lport ACL while the
+ * from-lport-after-lb tier advances) is left untouched: the
+ * conntrack commit in the stateful stage still needs it. */
+ ds_put_format(match, " && "REGBIT_ACL_LABEL" == 1 && "
+ REGBIT_ACL_OBS_STAGE" == %"PRIu8,
+ (uint8_t) obs_stages[i]);
+ ds_clear(&tier_actions);
+ build_acl_sample_label_clear_action(&tier_actions);
+ ds_put_format(&tier_actions, REG_ACL_TIER " = %"PRIuSIZE"; "
+ "next(pipeline=%s,table=%d);",
+ j + 1, ingress ? "ingress" : "egress",
+ ovn_stage_get_table(eval_stages[i]));
+ ovn_lflow_add(lflows, od, stage, 550, ds_cstr(match),
+ ds_cstr(&tier_actions), lflow_ref);
}
ds_destroy(&tier_actions);
ds_truncate(actions, verdict_len);
diff --git a/ovn-nb.xml b/ovn-nb.xml
index d3f3cc197..f72fb688b 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -3199,6 +3199,15 @@ or
this is used for sampling all traffic matched by the ACL.
</p>
+ <p>
+ Sampling of a <code>pass</code> ACL follows the connection state
+ rules of the ACL sample stage, in the tier of the <code>pass</code>
+ ACL. Its observation data is not inherited by the ACLs evaluated in
+ a subsequent tier of the same stage: the ACL that eventually
+ determines the verdict is sampled according to its own
+ configuration.
+ </p>
+
<p>
Note: if an ACL has both sampling enabled and a label associated to it
then the label value overrides the observation point ID defined in the
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 93af177ab..ab3e62529 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -15566,6 +15566,93 @@ OVN_CLEANUP_NORTHD
AT_CLEANUP
])
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([ACL Sampling - pass ACL observation data does not leak to the next
tier])
+AT_KEYWORDS([acl sampling])
+
+ovn_start
+
+collector1=$(ovn-nbctl create Sample_Collector id=1 name=c1 probability=65535
set_id=100)
+check_row_count nb:Sample_Collector 1
+
+check_uuid ovn-nbctl create Sampling_App type="acl-new" id="42"
+check_uuid ovn-nbctl create Sampling_App type="acl-est" id="43"
+check_row_count nb:Sampling_App 2
+
+check ovn-nbctl \
+ -- lr-add lr \
+ -- lrp-add lr lrp1 00:00:00:00:01:00 42.42.42.1/24 \
+ -- ls-add ls \
+ -- lsp-add ls vm1 \
+ -- lsp-set-addresses vm1 00:00:00:00:00:01 \
+ -- lsp-add ls vm2 \
+ -- lsp-set-addresses vm2 00:00:00:00:00:02 \
+ -- lsp-add-router-port ls ls-lr lrp1
+check ovn-nbctl --wait=sb sync
+
+rp_flow="inport == \"ls-lr\" && eth.src == 00:00:00:00:01:00 && eth.dst ==
00:00:00:00:00:01 && ip4.src == 10.0.0.1 && ip4.dst == 42.42.42.2 && tcp"
+vm_flow="inport == \"vm1\" && eth.src == 00:00:00:00:00:01 && eth.dst ==
00:00:00:00:00:02 && ip4.src == 42.42.42.2 && ip4.dst == 42.42.42.3 && tcp"
+m4_define([TRACE_FILTER], [grep -e sample -e commit | grep -v _sample | sort])
+
+dnl Tier 0: a "pass" ACL with sampling enabled.
+dnl Tier 1: a drop ACL sharing the Sample of the "pass" ACL, a drop ACL
+dnl without sampling and an allow-related ACL without sampling.
+check ovn-nbctl acl-add ls from-lport 1 'inport == "vm1"' allow-related
+check_uuid ovn-nbctl
\
+ -- --id=@s create Sample collector="$collector1" metadata=4501
\
+ -- --tier=1 --sample-new=@s acl-add ls from-lport 10 'tcp.dst == 22' drop
+shared_sample=$(ovn-nbctl --bare --columns=_uuid find Sample metadata=4501)
+check ovn-nbctl --wait=sb
\
+ -- --tier=0 --sample-new=$shared_sample acl-add ls from-lport 10 'tcp' pass
\
+ -- --tier=1 acl-add ls from-lport 10 'tcp.dst == 23' drop
\
+ -- --tier=1 acl-add ls from-lport 10 'tcp.dst == 24' allow-related
+check_row_count nb:ACL 5
+check_row_count nb:Sample 1
+
+dnl Moving to the next tier clears observation data recorded by a "pass"
+dnl ACL in the current stage.
+AT_CHECK([ovn-sbctl lflow-list ls | grep ls_in_acl_action | grep -e
'priority=5[[05]]0' | ovn_strip_lflows], [0], [dnl
+ table=??(ls_in_acl_action ), priority=500 , match=(reg8[[30..31]] == 0),
action=(reg8[[30..31]] = 1; next(pipeline=ingress,table=??);)
+ table=??(ls_in_acl_action ), priority=550 , match=(reg8[[30..31]] == 0 &&
reg0[[13]] == 1 && reg8[[19..20]] == 0), action=(reg0[[13]] = 0; reg3 = 0; reg9
= 0; reg8[[0..7]] = 0; reg8[[8..15]] = 0; reg8[[30..31]] = 1;
next(pipeline=ingress,table=??);)
+])
+
+check ovn-sbctl chassis-add gw1 geneve 127.0.0.1
+m4_foreach([REGS], [true, false], [
+AS_BOX([ovn-sample-with-registers=REGS])
+check ovn-sbctl set chassis gw1 other_config:ovn-sample-with-registers="REGS"
+check ovn-nbctl --wait=sb sync
+
+dnl The "pass" ACL is sampled in its own tier, according to the connection
+dnl state rules of the sample stage (new connections on this stateful
+dnl switch).
+AS_BOX([sampled drop after pass, new connection: pass and drop are both
sampled])
+AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new ls "$vm_flow && tcp.dst == 22"
| TRACE_FILTER], [0], [dnl
+
sample(probability=65535,collector_set=100,obs_domain=42,obs_point=m4_if(REGS,
[true], [reg3], [4501]));
+
sample(probability=65535,collector_set=100,obs_domain=42,obs_point=m4_if(REGS,
[true], [reg3], [4501]));
+])
+
+dnl The unsampled drop ACL must not be attributed to the "pass" ACL Sample.
+AS_BOX([unsampled drop after pass, new connection: only the pass is sampled])
+AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new ls "$vm_flow && tcp.dst == 23"
| TRACE_FILTER], [0], [dnl
+
sample(probability=65535,collector_set=100,obs_domain=42,obs_point=m4_if(REGS,
[true], [reg3], [4501]));
+])
+AS_BOX([unsampled drop after pass, untracked: nothing is sampled])
+AT_CHECK_UNQUOTED([ovn_trace ls "$rp_flow && tcp.dst == 23" | TRACE_FILTER],
[0], [])
+
+dnl The unsampled allow-related ACL must not inherit the "pass" ACL Sample
+dnl either: the connection is committed without observation data.
+AS_BOX([unsampled allow-related after pass, new connection])
+AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new ls "$vm_flow && tcp.dst == 24"
| TRACE_FILTER], [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; };
+ 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; };
+
sample(probability=65535,collector_set=100,obs_domain=42,obs_point=m4_if(REGS,
[true], [reg3], [4501]));
+])
+])
+
+OVN_CLEANUP_NORTHD
+AT_CLEANUP
+])
+
OVN_FOR_EACH_NORTHD_NO_HV([
AT_SETUP([NAT with match])
ovn_start
--
2.55.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
