On Tue, Oct 08, 2024 at 11:51:54AM GMT, Dumitru Ceara wrote:
> 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
small typo: s/Repoerted-at/Reported-at/
> > 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.
>
Maybe a stupid question:
A zero obs_point_id (zero label) is still a valid one from OVS's pov
(probably not for the reader), right? If we cannot avoid emitting the
sample based on the mark, how would we know if the label should be
used or not?
Thanks.
Adrián
> > 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
