On Wed, Feb 19, 2025 at 12:31 PM Ales Musil <[email protected]> wrote: > > > > On Wed, Feb 19, 2025 at 6:19 PM Numan Siddique <[email protected]> wrote: >> >> >> >> On Wed, Feb 19, 2025 at 10:54 AM Numan Siddique <[email protected]> wrote: >>> >>> 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 ? > > > Yes the commit message looks good, thanks. > >>> >>> ----------------------------------------------- >>> 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. >>> > > > Sounds like implementation detail exposure, but I don't want to block the fix > on this.
Sorry. I didn't understand the concern. You mean we should try to reproduce the issue in a different way ? I couldn't find a more reliable way than this to reproduce it 100% of the time. > >> >> @Ales - Can you please check and confirm if I've addressed your comments and >> your Ack is valid ? > > > Yes, my ack still holds. Thanks Ales. I applied the patch to main and backported to branch-25.03 and branch-24.09 Numan > >> >> We need this fix to upgrade to 24.09. >> >> >> Numan >> >> >>> 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 > > > Thanks, > Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
