On Wed, Feb 19, 2025 at 5:49 AM Ales Musil <[email protected]> wrote:
>
>
>
> On Wed, Feb 19, 2025 at 4:51 AM <[email protected]> wrote:
>>
>> From: Numan Siddique <[email protected]>
>>
>> After the commit [1], ovn-northd doesn't generate the required logical
>> flows for the tiered ACLs causing the higher tier ACLs to be skipped
>> from the evaluation. It should have used
>> 'nbrec_acl_col_tier.type.key.integer.max' instead of
>> 'nbrec_acl_col_tier.type.value.integer.max' to get the max tier
>> supported by the schema since the 'tier' column is not an smap.
>>
>> 'nbrec_acl_col_tier.type.value.integer.max' is 0 and the function
>> ls_acl_tiers_are_maxed_out() returns true for an ACL with tier 0.
>>
>> This issue will only be seen if the first ACL in the acl column of
>> logical switch or port group has tier value 0. In this case
>> ls_stateful_record_set_acl_flags_() only evaluates the first ACL
>> and never updates the ls_stateful_rec->max_acl_tier to the actual
>> values, there by skipping the required logical flows for tier
>> evaluation.
>>
>> This patch fixes it. It also enhances the existing test case
>> to cover this scenario.
>>
>> [1] - 6aa66f9e1c58 ("northd: Track max ACL tiers more accurately.")
>>
>> Fixes: 6aa66f9e1c58 ("northd: Track max ACL tiers more accurately.")
>> Reported-at: https://issues.redhat.com/browse/FDP-1154
>> Signed-off-by: Numan Siddique <[email protected]>
>> ---
>
>
> Hi Numan,
> thank you for the fix, the change looks good, but I don't agree with
> the conclusion that the 6aa66f9e1c58 broke it, it just exposed the
> issue because the wrong type was used since the beginning [0], so we
> should probably do a custom backport to 24.03 I suppose?
Thanks for pointing this out. Does the below commit message looks good to you ?
-----------------------------------------------
northd: Fix missing tier related ACL flows.
ovn-northd doesn't generate the required logical flows for the tiered
ACLs causing the higher tier ACLs to be skipped from the evaluation.
It should have used 'nbrec_acl_col_tier.type.key.integer.max' instead
of 'nbrec_acl_col_tier.type.value.integer.max' to get the max tier
supported by the schema since the 'tier' column is not an smap.
'nbrec_acl_col_tier.type.value.integer.max' is 0 and the function
ls_acl_tiers_are_maxed_out() returns true for an ACL with tier 0.
This issue will only be seen if the first ACL in the acl column of
logical switch or port group has tier value 0. In this case
ls_stateful_record_set_acl_flags_() only evaluates the first ACL
and never updates the ls_stateful_rec->max_acl_tier to the actual
values, there by skipping the required logical flows for tier
evaluation.
This patch fixes it. It also enhances the existing test case
to cover this scenario.
Even though this issue is present since the ACL support was added,
the issue can manifest more frequently after the commit [1], since
we now maintain max tiers for 3 different stages separately.
[1] - 6aa66f9e1c58 ("northd: Track max ACL tiers more accurately.")
Fixes: 119f14e05cb4 ("northd: Add tiered ACL support.")
Reported-at: https://issues.redhat.com/browse/FDP-1154
Signed-off-by: Numan Siddique <[email protected]>
-----------------------------------------------
Agree we should do a custom fix to branch-24.03 (and perhaps to 23.09
if it is desired).
>
>
>> northd/en-ls-stateful.c | 2 +-
>> tests/ovn-northd.at | 28 ++++++++++++++++++++++++++++
>> 2 files changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/northd/en-ls-stateful.c b/northd/en-ls-stateful.c
>> index 11e97aa9bb..69cda5008c 100644
>> --- a/northd/en-ls-stateful.c
>> +++ b/northd/en-ls-stateful.c
>> @@ -455,7 +455,7 @@ ls_stateful_record_set_acl_flags_(struct
>> ls_stateful_record *ls_stateful_rec,
>> if (ls_stateful_rec->has_stateful_acl &&
>> ls_acl_tiers_are_maxed_out(
>> &ls_stateful_rec->max_acl_tier,
>> - nbrec_acl_col_tier.type.value.integer.max)) {
>> + nbrec_acl_col_tier.type.key.integer.max)) {
>> return true;
>> }
>> }
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index 0ddb120279..07aac830d7 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -14636,6 +14636,34 @@ AT_CHECK([ovn-sbctl lflow-list S1 | grep
>> ls_out_acl_action | grep priority=500 |
>> table=??(ls_out_acl_action ), priority=500 , match=(reg8[[30..31]] ==
>> 1), action=(reg8[[30..31]] = 2; next(pipeline=egress,table=??);)
>> ])
>>
>> +# Create a port group and add 2 ACLs.
>> +# First add an allow-related ACL with tier 1 with a predefine UUID and
>> +# make sure that there is a pri 500 flow to check for ACLs in tier 1.
>> +# Then add another ACL with tier 0 and make sure that the pri 500
>> +# flow still exists.
>> +check ovn-nbctl ls-add S2
>> +check ovn-nbctl lsp-add S2 S2-p1
>> +check ovn-nbctl lsp-add S2 S2-p2
>> +check ovn-nbctl pg-add pg1 S2-p1 S2-p2
>> +
>> +check_uuid ovn-nbctl --id=3f507ce6-f6e6-4b18-829b-80a18a8143cd create ACL \
>
>
> Is there a specific reason for hardcoded uuid? If the only purpose
> is to add the ACL into the PG right away you can replace it with
> the "@' identifier. e.g. "@acl".
Yes. There is a specific reason. The issue is not seen if the first
ACL in the acls column of logical_switch has a tier greater than 0.
My assumption is that ovsdb-server orders the ACL list in the acls
column by sorting the uuids. Please correct me if I'm wrong here.
So I'm making sure that the uuid value of 2nd ACL with tier 0 is
smaller than the 1st one so that ovn-northd processes this ACL first.
If I use the "acl-add" command, then the issue is not reproducible
100% of the time.
Hope this makes sense. Please let me know if you have any questions.
Thanks
Numan
>
>> +action=allow-related direction=from-lport match=tcp priority=1001 tier=1 --
>> \
>> +add port_group pg1 acls 3f507ce6-f6e6-4b18-829b-80a18a8143cd
>> +check ovn-nbctl --wait=sb sync
>> +
>> +AT_CHECK([ovn-sbctl lflow-list S2 | grep ls_in_acl_action | grep
>> priority=500 | 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=??);)
>> +])
>> +
>> +check_uuid ovn-nbctl --id=3f507ce6-f6e6-4b18-829b-80a18a8143cc create ACL \
>
>
> Same here.
>
>> +action=allow-related direction=from-lport match=ip4 priority=2001 tier=0 --
>> \
>> +add port_group pg1 acls 3f507ce6-f6e6-4b18-829b-80a18a8143cc
>> +check ovn-nbctl --wait=sb sync
>> +
>> +AT_CHECK([ovn-sbctl lflow-list S2 | grep ls_in_acl_action | grep
>> priority=500 | 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=??);)
>> +])
>> +
>> AT_CLEANUP
>> ])
>>
>> --
>> 2.48.1
>>
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
> With the above addressed:
> Acked-by: Ales Musil <[email protected]>
>
> Thanks,
> Ales
>
> [0]
> https://github.com/ovn-org/ovn/commit/119f14e0#diff-97e16400e2bcbb4b65f7f3b8f2c05e9e8e56148df77719b71d60f235e3bcc0edR5682
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
