On 10/8/24 13:16, Adrián Moreno wrote:
> 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
0 is not a valid sample ID (obs_point_id) from OVN's perspective. From
ovn-nb.ovsschema:
"metadata": {"type": {"key": {"type": "integer",
"minInteger": 1,
"maxInteger": 4294967295},
"min": 1, "max":1}}
> sample based on the mark, how would we know if the label should be
> used or not?
>
I'm not sure I follow this part. ct_mark.blocked doesn't decide whether
a sample should be generated or not. It only is used to "tag" sessions
that used to be allowed and now are not anymore (the ACL that used to
allow them was removed).
From a sampling perspective, after the original ACL is removed we need
to either:
a. sample with the drop ACL configured sample ID, if that's non-zero
b. don't sample, if the drop ACL doesn't have sampling configured
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
