On Mon, Apr 10, 2023 at 7:26 PM Mark Michelson <mmich...@redhat.com> 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 <mmich...@redhat.com>
>
> 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;
> } else {
> return strcmp(acl1->match, acl2->match);
> }
> @@ -2283,6 +2286,7 @@ nbctl_pre_acl(struct ctl_context *ctx)
> ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_priority);
> ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_match);
> ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_options);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_tier);
> }
>
> static void
> @@ -2390,6 +2394,16 @@ nbctl_acl_add(struct ctl_context *ctx)
> nbrec_acl_set_options(acl, &options);
> }
>
> + const char *tier_s = shash_find_data(&ctx->options, "--tier");
> + if (tier_s) {
> + int64_t tier;
> + if (!str_to_long(tier_s, 10, &tier)) {
> + ctl_error(ctx, "Invalid tier %s", tier_s);
> + return;
> + }
> + 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(&ctx->options, "--tier");
> + int64_t tier;
> + unsigned long *bitmaps[3];
> + size_t n_bitmaps = 0;
>
> char *error = acl_cmd_get_pg_or_ls(ctx, &ls, &pg);
> 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, &tier)) {
> + 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);
> @@ -2438,55 +2461,83 @@ nbctl_acl_del(struct ctl_context *ctx)
> return;
> }
>
> - const char *direction;
> - error = parse_direction(ctx->argv[2], &direction);
> - if (error) {
> - ctx->error = error;
> - return;
> - }
> -
> size_t n_acls = pg ? pg->n_acls : ls->n_acls;
> struct nbrec_acl **acls = pg ? pg->acls : ls->acls;
> - /* If priority and match are not specified, delete all ACLs with the
> - * specified direction. */
> - if (ctx->argc == 3) {
> +
> + if (tier_s) {
> + bitmaps[n_bitmaps] = bitmap_allocate(n_acls);
> for (size_t i = 0; i < n_acls; i++) {
> - if (!strcmp(direction, acls[i]->direction)) {
> - if (pg) {
> - nbrec_port_group_update_acls_delvalue(pg, acls[i]);
> - } else {
> - nbrec_logical_switch_update_acls_delvalue(ls,
> acls[i]);
> - }
> + if (acls[i]->tier == tier) {
> + bitmap_set1(bitmaps[n_bitmaps], i);
> }
> }
> - return;
> + n_bitmaps++;
> }
>
> - int64_t priority;
> - error = parse_priority(ctx->argv[3], &priority);
> - if (error) {
> - ctx->error = error;
> - return;
> - }
> + if (ctx->argc >= 3) {
> + const char *direction;
> + error = parse_direction(ctx->argv[2], &direction);
> + if (error) {
> + ctx->error = error;
> + goto cleanup;
> + }
>
> - if (ctx->argc == 4) {
> - ctl_error(ctx, "cannot specify priority without match");
> - return;
> + /* If priority and match are not specified, delete all ACLs with
> the
> + * specified direction. */
> + bitmaps[n_bitmaps] = bitmap_allocate(n_acls);
> + for (size_t i = 0; i < n_acls; i++) {
> + if (!strcmp(direction, acls[i]->direction)) {
> + bitmap_set1(bitmaps[n_bitmaps], i);
> + }
> + }
> + n_bitmaps++;
> }
>
> - /* Remove the matching rule. */
> - for (size_t i = 0; i < n_acls; i++) {
> - struct nbrec_acl *acl = acls[i];
> + if (ctx->argc >= 4) {
> + int64_t priority;
> + error = parse_priority(ctx->argv[3], &priority);
> + if (error) {
> + ctx->error = error;
> + goto cleanup;
> + }
>
> - if (priority == acl->priority && !strcmp(ctx->argv[4],
> acl->match) &&
> - !strcmp(direction, acl->direction)) {
> - if (pg) {
> - nbrec_port_group_update_acls_delvalue(pg, acl);
> - } else {
> - nbrec_logical_switch_update_acls_delvalue(ls, acl);
> + if (ctx->argc == 4) {
> + ctl_error(ctx, "cannot specify priority without match");
> + goto cleanup;
> + }
> +
> + /* Remove the matching rule. */
> + bitmaps[n_bitmaps] = bitmap_allocate(n_acls);
> + for (size_t i = 0; i < n_acls; i++) {
> + struct nbrec_acl *acl = acls[i];
> +
> + if (priority == acl->priority &&
> + !strcmp(ctx->argv[4], acl->match)) {
> + bitmap_set1(bitmaps[n_bitmaps], i);
> }
> - return;
> }
> + n_bitmaps++;
> + }
> +
> + unsigned long *bitmap_result = bitmap_allocate1(n_acls);
> + for (size_t i = 0; i < n_bitmaps; i++) {
> + bitmap_result = bitmap_and(bitmap_result, bitmaps[i], n_acls);
> + }
> +
> + size_t index;
> + BITMAP_FOR_EACH_1 (index, n_acls, bitmap_result) {
> + if (pg) {
> + nbrec_port_group_update_acls_delvalue(pg, acls[index]);
> + } else {
> + nbrec_logical_switch_update_acls_delvalue(ls, acls[index]);
> + }
> + }
> +
> + free(bitmap_result);
> +
> +cleanup:
> + for (size_t i = 0; i < n_bitmaps; i++) {
> + free(bitmaps[i]);
> }
> }
>
> @@ -7658,9 +7709,9 @@ static const struct ctl_command_syntax
> nbctl_commands[] = {
> { "acl-add", 5, 6, "{SWITCH | PORTGROUP} DIRECTION PRIORITY MATCH
> ACTION",
> nbctl_pre_acl, nbctl_acl_add, NULL,
> "--log,--may-exist,--type=,--name=,--severity=,--meter=,--label=,"
> - "--apply-after-lb", RW },
> + "--apply-after-lb,--tier=", RW },
> { "acl-del", 1, 4, "{SWITCH | PORTGROUP} [DIRECTION [PRIORITY
> MATCH]]",
> - nbctl_pre_acl, nbctl_acl_del, NULL, "--type=", RW },
> + nbctl_pre_acl, nbctl_acl_del, NULL, "--type=,--tier=", RW },
> { "acl-list", 1, 1, "{SWITCH | PORTGROUP}",
> nbctl_pre_acl_list, nbctl_acl_list, NULL, "--type=", RO },
>
> --
> 2.38.1
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Please also add the --tier option into ovn-nbctl.8.xml.
Thanks,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
