On Fri, Apr 21, 2023 at 9:51 PM Mark Michelson <[email protected]> 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 <[email protected]> > --- > tests/ovn-nbctl.at | 77 ++++++++++++++++++++++ > utilities/ovn-nbctl.8.xml | 29 ++++++--- > utilities/ovn-nbctl.c | 131 ++++++++++++++++++++++++++------------ > 3 files changed, 189 insertions(+), 48 deletions(-) > > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > index aa5baade4..52c98fd5e 100644 > --- a/tests/ovn-nbctl.at > +++ b/tests/ovn-nbctl.at > @@ -2616,6 +2616,83 @@ 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 --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.8.xml b/utilities/ovn-nbctl.8.xml > index 54dbdb791..1fc00a927 100644 > --- a/utilities/ovn-nbctl.8.xml > +++ b/utilities/ovn-nbctl.8.xml > @@ -399,7 +399,7 @@ > must be either <code>switch</code> or <code>port-group</code>. > </p> > <dl> > - <dt>[<code>--type=</code>{<code>switch</code> | > <code>port-group</code>}] [<code>--log</code>] > [<code>--meter=</code><var>meter</var>] > [<code>--severity=</code><var>severity</var>] > [<code>--name=</code><var>name</var>] > [<code>--label=</code><var>label</var>] [<code>--may-exist</code>] > [<code>--apply-after-lb</code>] <code>acl-add</code> <var>entity</var> > <var>direction</var> <var>priority</var> <var>match</var> > <var>verdict</var></dt> > + <dt>[<code>--type=</code>{<code>switch</code> | > <code>port-group</code>}] [<code>--log</code>] > [<code>--meter=</code><var>meter</var>] > [<code>--severity=</code><var>severity</var>] > [<code>--name=</code><var>name</var>] > [<code>--label=</code><var>label</var>] [<code>--may-exist</code>] > [<code>--apply-after-lb</code>] [<code>--tier</code>] <code>acl-add</code> > <var>entity</var> <var>direction</var> <var>priority</var> <var>match</var> > <var>verdict</var></dt> > <dd> > <p> > Adds the specified ACL to <var>entity</var>. > <var>direction</var> > @@ -430,16 +430,29 @@ > of the <code>ACL</code> table. As the option name suggests, > the ACL > will be applied after the logical switch load balancer stage. > </p> > + <p> > + The <code>--tier</code> option sets the ACL's tier to the > specified > + value. For more information about ACL tiers, see the > documentation > + for the <code>ovn-nb</code>(5) database. > + </p> > </dd> > > - <dt>[<code>--type=</code>{<code>switch</code> | > <code>port-group</code>}] <code>acl-del</code> <var>entity</var> > [<var>direction</var> [<var>priority</var> <var>match</var>]]</dt> > + <dt>[<code>--type=</code>{<code>switch</code> | > <code>port-group</code>}] [<code>--tier</code>] <code>acl-del</code> > <var>entity</var> [<var>direction</var> [<var>priority</var> > <var>match</var>]]</dt> > <dd> > - Deletes ACLs from <var>entity</var>. If only <var>entity</var> is > - supplied, all the ACLs from the <var>entity</var> are deleted. If > - <var>direction</var> is also specified, then all the flows in that > - direction will be deleted from the <var>entity</var>. If all the > - fields are given, then a single flow that matches all the fields > will > - be deleted. > + <p> > + Deletes ACLs from <var>entity</var>. If only <var>entity</var> > is > + supplied, all the ACLs from the <var>entity</var> are deleted. > If > + <var>direction</var> is also specified, then all the flows in > that > + direction will be deleted from the <var>entity</var>. If all > the > + fields are given, then a single flow that matches all the > fields will > + be deleted. > + </p> > + > + <p> > + If the <code>--tier</code> option is provided, then only ACLs > of the > + given tier value will be deleted, in addition to whatever other > + criteria have been provided. > + </p> > </dd> > > <dt>[<code>--type=</code>{<code>switch</code> | > <code>port-group</code>}] <code>acl-list</code> <var>entity</var> </dt> > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index 22313ecd5..104f492a5 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) { > + long 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"); > + long 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.39.2 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Reviewed-by: Ales Musil <[email protected]> -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
