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. > @Ales - Can you please check and confirm if I've addressed your comments > and your Ack is valid ? > Yes, my ack still holds. > 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
