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 ? > > ----------------------------------------------- > 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. > > @Ales - Can you please check and confirm if I've addressed your comments and your Ack is valid ? 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
