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?
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".
+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
