On Wed, Feb 19, 2025 at 8:22 PM Numan Siddique <[email protected]> wrote:
> 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 ? That would be ideal, but it's not a big deal IMO, this works. > 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
