On 10/8/24 10:59, Lorenzo Bianconi wrote:
> Considering the following configuration:
>
> $ovn-nbctl acl-list sw01
> from-lport 100 (inport == "sw01-port1" && udp.dst == 5201) allow-related
> [after-lb]
> from-lport 10 (inport == "sw01-port1" && udp) drop [after-lb]
>
> $ovn-nbctl list acl
> _uuid : e440336a-84d3-4a6d-95a9-edd1db1c3631
> action : drop
> direction : from-lport
> external_ids : {}
> label : 0
> log : false
> match : "inport == \"sw01-port1\" && udp"
> meter : []
> name : []
> options : {apply-after-lb="true"}
> priority : 10
> sample_est : ac6a6efc-a2e0-4d68-b5f8-8cd91113e554
> sample_new : 5cdad2ab-4390-4772-ac40-74aa2980c06e
> severity : []
> tier : 0
>
> _uuid : 85ef08d7-aacc-41d7-b808-6ab011edd753
> action : allow-related
> direction : from-lport
> external_ids : {}
> label : 0
> log : false
> match : "inport == \"sw01-port1\" && udp.dst == 5201"
> meter : []
> name : []
> options : {apply-after-lb="true"}
> priority : 100
> sample_est : 143ce7e2-fd13-4d5e-930c-133d5cf87d0d
> sample_new : 1d1a0a05-2a8a-4c72-ad35-77d7e2908183
> severity : []
> tier : 0
>
> If the priority-100 acl is removed, the udp traffic with destination port
> 5201 will be dropped however ovn-controller will continue sampling the
> existing connection with the observationPointID associated to the removed
> acl. Fix the issue updating the ct_label.obs_point_id for the connection
> marked with ct_mark.blocked.
>
> Fixes: d15b12da6fe6 ("northd: Add ACL Sampling.")
> Repoerted-at: https://issues.redhat.com/browse/FDP-819
> Signed-off-by: Lorenzo Bianconi <[email protected]>
>
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
Hi Lorenzo,
Could you please also add tests for these scenarios?
> northd/northd.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 2c4703301..d5b9a54b2 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -7205,7 +7205,14 @@ consider_acl(struct lflow_table *lflows, const struct
> ovn_datapath *od,
> /* For drop ACLs just sample all packets as "new" packets. */
> build_acl_sample_label_action(actions, acl, acl->sample_new, NULL,
> obs_stage);
> - ds_put_cstr(actions, "ct_commit { ct_mark.blocked = 1; }; next;");
> + if (acl->sample_est) {
> + ds_put_format(actions,
> + "ct_commit { ct_mark.blocked = 1; "
> + "ct_label.obs_point_id = %"PRIu32"; }; next;",
> + (uint32_t) acl->sample_est->metadata);
> + } else {
> + ds_put_cstr(actions, "ct_commit { ct_mark.blocked = 1; };
> next;");
> + }
Will this ct_commit change the value of ct_label.obs_point_id to 0? I
guess not. Shouldn't we explicitly do that here? Otherwise, if the
drop ACL doesn't have sampling enabled, I'm afraid we'll still generate
samples with the stale point ID.
> ovn_lflow_add_with_hint(lflows, od, stage, priority,
> ds_cstr(match), ds_cstr(actions),
> &acl->header_, lflow_ref);
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
