Re: [ovs-dev] [PATCH ovn v2 3/4] ovn-nbctl: Add tier ACL options.
On Mon, Apr 10, 2023 at 7:26 PM Mark Michelson wrote:
> This modifies the acl-add and acl-del commands so that an ACL
> tier can be specified when adding or deleting ACLs.
>
> For acl-add, if the tier is specified, then the ACL created by the
> command will have that tier set.
>
> For acl-del, if the tier is specified, then the tier will be one of the
> criteria used when deciding which ACLs to delete. Because the tier is
> not any more or less specific than the other criteria used for deleting
> ACLs, a bitmap approach is used to determine the final set of ACLs that
> should be deleted.
>
> Signed-off-by: Mark Michelson
>
> Hi Mark,
I have two comments, see below.
> ---
> tests/ovn-nbctl.at| 81 ++
> utilities/ovn-nbctl.c | 131 +-
> 2 files changed, 172 insertions(+), 40 deletions(-)
>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index aa5baade4..3352064d1 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -2616,6 +2616,87 @@ ovn-nbctl: no row "foo1" in table Logical_Switch
>
> dnl -
>
> +OVN_NBCTL_TEST([acl_tiers], [ACL tier operations], [
> +check ovn-nbctl ls-add ls
> +#check ovn-nbctl acl-add ls from-lport 1000 "ip" drop
> +#check_column 0 nb:ACL tier priority=1000
> +#
> +#check ovn-nbctl acl-del ls
Leftover comment?
> +check ovn-nbctl --tier=3 acl-add ls from-lport 1000 "ip" drop
> +check_column 3 nb:ACL tier priority=1000
> +
> +check ovn-nbctl --tier=3 acl-add ls from-lport 1001 "ip" drop
> +check_column 3 nb:ACL tier priority=1001
> +
> +check ovn-nbctl --tier=2 acl-add ls from-lport 1002 "ip" drop
> +check_column 2 nb:ACL tier priority=1002
> +
> +# Removing the tier 3 acls from ls should result in 1 ACL
> +# remaining.
> +check ovn-nbctl --tier=3 acl-del ls
> +check_row_count nb:ACL 1
> +check_column 2 nb:ACL tier priority=1002
> +
> +# Add two egress ACLs at tier 2.
> +check ovn-nbctl --tier=2 acl-add ls to-lport 1000 "ip" drop
> +check ovn-nbctl --tier=2 acl-add ls to-lport 1001 "ip" drop
> +
> +check_row_count nb:ACL 3 tier=2
> +
> +# This should remove the egress tier 2 ACLs and leave the
> +# ingress tier 2 ACL
> +check ovn-nbctl --tier=2 acl-del ls to-lport
> +check_row_count nb:ACL 1
> +check_column 2 nb:ACL tier priority=1002
> +check_column from-lport nb:ACL direction priority=1002
> +
> +# Re-add two ingress ACLs at tier 2.
> +check ovn-nbctl --tier=2 acl-add ls from-lport 1000 "ip" drop
> +check ovn-nbctl --tier=2 acl-add ls from-lport 1001 "ip" drop
> +
> +check_row_count nb:ACL 3
> +
> +# Attempt to remove all tier 3 ACLs. All three ACLs are tier 2
> +# so this shouldn't have any effect.
> +check ovn-nbctl --tier=3 acl-del ls
> +check_row_count nb:ACL 3
> +
> +# Attempt to remove all ingress tier 3 ACLs. All three ACLs are tier
> +# 2, so this shouldn't have any effect.
> +check ovn-nbctl --tier=3 acl-del ls from-lport
> +check_row_count nb:ACL 3
> +
> +# Attempt to remove the 1000 priority ACL but specify tier 3. Since
> +# all ACLs are tier 2, this should have no effect.
> +check ovn-nbctl --tier=3 acl-del ls from-lport 1000 "ip"
> +check_row_count nb:ACL 3
> +
> +# Specifying the proper tier should result in all ACLs being deleted.
> +check ovn-nbctl --tier=2 acl-del ls
> +check_row_count nb:ACL 0
> +
> +# Now let's experiment with identical ACLs at different tiers.
> +check ovn-nbctl --tier=1 acl-add ls from-lport 1000 "ip" drop
> +check ovn-nbctl --tier=2 acl-add ls from-lport 1000 "ip" drop
> +check ovn-nbctl --tier=3 acl-add ls from-lport 1000 "ip" drop
> +check_row_count nb:ACL 3
> +check_row_count nb:ACL 1 tier=1
> +check_row_count nb:ACL 1 tier=2
> +check_row_count nb:ACL 1 tier=3
> +
> +# Specifying tier 1 should result in only one ACL being deleted.
> +check ovn-nbctl --tier=1 acl-del ls from-lport 1000 "ip"
> +check_row_count nb:ACL 2
> +check_row_count nb:ACL 1 tier=2
> +check_row_count nb:ACL 1 tier=3
> +
> +# Not specifying a tier should result in all ACLs being deleted.
> +check ovn-nbctl acl-del ls from-lport 1000 "ip"
> +check_row_count nb:ACL 0
> +])
> +
> +dnl -
> +
> AT_SETUP([ovn-nbctl - daemon retry connection])
> OVN_NBCTL_TEST_START daemon
> AT_CHECK([kill `cat ovsdb-server.pid`])
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 22313ecd5..7910b9d3f 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -48,6 +48,7 @@
> #include "unixctl.h"
> #include "util.h"
> #include "openvswitch/vlog.h"
> +#include "bitmap.h"
>
> VLOG_DEFINE_THIS_MODULE(nbctl);
>
> @@ -2100,6 +2101,8 @@ acl_cmp(const void *acl1_, const void *acl2_)
> return after_lb2 ? -1 : 1;
> } else if (acl1->priority != acl2->priority) {
> return acl1->priority > acl2->priority ? -1 : 1;
> +} else if (acl1->tier != acl2->tier) {
> +return acl1->tier > acl2->tier ? -1 : 1;
>
Re: [ovs-dev] [PATCH ovn v2 3/4] ovn-nbctl: Add tier ACL options.
On Mon, Apr 10, 2023 at 01:26:12PM -0400, Mark Michelson wrote:
...
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
...
> @@ -2390,6 +2394,16 @@ nbctl_acl_add(struct ctl_context *ctx)
> nbrec_acl_set_options(acl, );
> }
>
> +const char *tier_s = shash_find_data(>options, "--tier");
> +if (tier_s) {
> +int64_t tier;
> +if (!str_to_long(tier_s, 10, )) {
> +ctl_error(ctx, "Invalid tier %s", tier_s);
> +return;
> +}
Hi Mark,
gcc has flagged that tier is int64_t but str_to_long expects a long int.
utilities/ovn-nbctl.c: In function ‘nbctl_acl_add’:
utilities/ovn-nbctl.c:2400:38: error: passing argument 3 of ‘str_to_long’ from
incompatible pointer type [-Werror=incompatible-pointer-types]
2400 | if (!str_to_long(tier_s, 10, )) {
| ^
| |
| int64_t * {aka long long int *}
In file included from /home/runner/work/ovn/ovn/ovs/lib/hash.h:23,
from /home/runner/work/ovn/ovn/ovs/lib/smap.h:20,
from ./lib/lb.h:22,
from utilities/ovn-nbctl.c:31:
/home/runner/work/ovn/ovn/ovs/lib/util.h:243:42: note: expected ‘long int *’
but argument is of type ‘int64_t *’ {aka ‘long long int *’}
243 | bool str_to_long(const char *, int base, long *);
|
Link:
https://github.com/ovsrobot/ovn/actions/runs/4659735446/jobs/8246953406#step:14:3266
> +nbrec_acl_set_tier(acl, tier);
> +}
> +
> /* Check if same acl already exists for the ls/portgroup */
> size_t n_acls = pg ? pg->n_acls : ls->n_acls;
> struct nbrec_acl **acls = pg ? pg->acls : ls->acls;
> @@ -2418,6 +2432,10 @@ nbctl_acl_del(struct ctl_context *ctx)
> {
> const struct nbrec_logical_switch *ls = NULL;
> const struct nbrec_port_group *pg = NULL;
> +const char *tier_s = shash_find_data(>options, "--tier");
> +int64_t tier;
> +unsigned long *bitmaps[3];
> +size_t n_bitmaps = 0;
>
> char *error = acl_cmd_get_pg_or_ls(ctx, , );
> if (error) {
> @@ -2425,8 +2443,13 @@ nbctl_acl_del(struct ctl_context *ctx)
> return;
> }
>
> -if (ctx->argc == 2) {
> -/* If direction, priority, and match are not specified, delete
> +if (tier_s && !str_to_long(tier_s, 10, )) {
Likewise, here too.
> +ctl_error(ctx, "Invalid tier %s", tier_s);
> +return;
> +}
> +
> +if (ctx->argc == 2 && !tier_s) {
> +/* If direction, priority, tier, and match are not specified, delete
> * all ACLs. */
> if (pg) {
> nbrec_port_group_verify_acls(pg);
...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2 3/4] ovn-nbctl: Add tier ACL options.
This modifies the acl-add and acl-del commands so that an ACL
tier can be specified when adding or deleting ACLs.
For acl-add, if the tier is specified, then the ACL created by the
command will have that tier set.
For acl-del, if the tier is specified, then the tier will be one of the
criteria used when deciding which ACLs to delete. Because the tier is
not any more or less specific than the other criteria used for deleting
ACLs, a bitmap approach is used to determine the final set of ACLs that
should be deleted.
Signed-off-by: Mark Michelson
---
tests/ovn-nbctl.at| 81 ++
utilities/ovn-nbctl.c | 131 +-
2 files changed, 172 insertions(+), 40 deletions(-)
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index aa5baade4..3352064d1 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -2616,6 +2616,87 @@ ovn-nbctl: no row "foo1" in table Logical_Switch
dnl -
+OVN_NBCTL_TEST([acl_tiers], [ACL tier operations], [
+check ovn-nbctl ls-add ls
+#check ovn-nbctl acl-add ls from-lport 1000 "ip" drop
+#check_column 0 nb:ACL tier priority=1000
+#
+#check ovn-nbctl acl-del ls
+check ovn-nbctl --tier=3 acl-add ls from-lport 1000 "ip" drop
+check_column 3 nb:ACL tier priority=1000
+
+check ovn-nbctl --tier=3 acl-add ls from-lport 1001 "ip" drop
+check_column 3 nb:ACL tier priority=1001
+
+check ovn-nbctl --tier=2 acl-add ls from-lport 1002 "ip" drop
+check_column 2 nb:ACL tier priority=1002
+
+# Removing the tier 3 acls from ls should result in 1 ACL
+# remaining.
+check ovn-nbctl --tier=3 acl-del ls
+check_row_count nb:ACL 1
+check_column 2 nb:ACL tier priority=1002
+
+# Add two egress ACLs at tier 2.
+check ovn-nbctl --tier=2 acl-add ls to-lport 1000 "ip" drop
+check ovn-nbctl --tier=2 acl-add ls to-lport 1001 "ip" drop
+
+check_row_count nb:ACL 3 tier=2
+
+# This should remove the egress tier 2 ACLs and leave the
+# ingress tier 2 ACL
+check ovn-nbctl --tier=2 acl-del ls to-lport
+check_row_count nb:ACL 1
+check_column 2 nb:ACL tier priority=1002
+check_column from-lport nb:ACL direction priority=1002
+
+# Re-add two ingress ACLs at tier 2.
+check ovn-nbctl --tier=2 acl-add ls from-lport 1000 "ip" drop
+check ovn-nbctl --tier=2 acl-add ls from-lport 1001 "ip" drop
+
+check_row_count nb:ACL 3
+
+# Attempt to remove all tier 3 ACLs. All three ACLs are tier 2
+# so this shouldn't have any effect.
+check ovn-nbctl --tier=3 acl-del ls
+check_row_count nb:ACL 3
+
+# Attempt to remove all ingress tier 3 ACLs. All three ACLs are tier
+# 2, so this shouldn't have any effect.
+check ovn-nbctl --tier=3 acl-del ls from-lport
+check_row_count nb:ACL 3
+
+# Attempt to remove the 1000 priority ACL but specify tier 3. Since
+# all ACLs are tier 2, this should have no effect.
+check ovn-nbctl --tier=3 acl-del ls from-lport 1000 "ip"
+check_row_count nb:ACL 3
+
+# Specifying the proper tier should result in all ACLs being deleted.
+check ovn-nbctl --tier=2 acl-del ls
+check_row_count nb:ACL 0
+
+# Now let's experiment with identical ACLs at different tiers.
+check ovn-nbctl --tier=1 acl-add ls from-lport 1000 "ip" drop
+check ovn-nbctl --tier=2 acl-add ls from-lport 1000 "ip" drop
+check ovn-nbctl --tier=3 acl-add ls from-lport 1000 "ip" drop
+check_row_count nb:ACL 3
+check_row_count nb:ACL 1 tier=1
+check_row_count nb:ACL 1 tier=2
+check_row_count nb:ACL 1 tier=3
+
+# Specifying tier 1 should result in only one ACL being deleted.
+check ovn-nbctl --tier=1 acl-del ls from-lport 1000 "ip"
+check_row_count nb:ACL 2
+check_row_count nb:ACL 1 tier=2
+check_row_count nb:ACL 1 tier=3
+
+# Not specifying a tier should result in all ACLs being deleted.
+check ovn-nbctl acl-del ls from-lport 1000 "ip"
+check_row_count nb:ACL 0
+])
+
+dnl -
+
AT_SETUP([ovn-nbctl - daemon retry connection])
OVN_NBCTL_TEST_START daemon
AT_CHECK([kill `cat ovsdb-server.pid`])
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index 22313ecd5..7910b9d3f 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -48,6 +48,7 @@
#include "unixctl.h"
#include "util.h"
#include "openvswitch/vlog.h"
+#include "bitmap.h"
VLOG_DEFINE_THIS_MODULE(nbctl);
@@ -2100,6 +2101,8 @@ acl_cmp(const void *acl1_, const void *acl2_)
return after_lb2 ? -1 : 1;
} else if (acl1->priority != acl2->priority) {
return acl1->priority > acl2->priority ? -1 : 1;
+} else if (acl1->tier != acl2->tier) {
+return acl1->tier > acl2->tier ? -1 : 1;
} else {
return strcmp(acl1->match, acl2->match);
}
@@ -2283,6 +2286,7 @@ nbctl_pre_acl(struct ctl_context *ctx)
ovsdb_idl_add_column(ctx->idl, _acl_col_priority);
ovsdb_idl_add_column(ctx->idl, _acl_col_match);
ovsdb_idl_add_column(ctx->idl, _acl_col_options);
+ovsdb_idl_add_column(ctx->idl, _acl_col_tier);
}
static void
@@ -2390,6
