On Tue, Jun 24, 2025 at 9:36 PM Mairtin O'Loingsigh via dev < ovs-dev@openvswitch.org> wrote:
> Introduce --all option for acl-list command. > If this option is set while attempting to show ACLs of a logical > switch, port group ACLs associated with each logical switch port will > also be listed. > > $ovn-nbctl --all acl-list ls1 > > Reported-at: https://issues.redhat.com/browse/FDP-1462 > Signed-off-by: Mairtin O'Loingsigh <moloi...@redhat.com> > --- > Hi Mairtin, thank you for the v2. I have few more comment see down below. > v2: Code review changes > > NEWS | 3 ++ > tests/ovn-nbctl.at | 46 +++++++++++++++++- > utilities/ovn-nbctl.8.xml | 7 ++- > utilities/ovn-nbctl.c | 98 ++++++++++++++++++++++++++++----------- > 4 files changed, 126 insertions(+), 28 deletions(-) > > diff --git a/NEWS b/NEWS > index 699217ca9..486f12963 100644 > --- a/NEWS > +++ b/NEWS > @@ -31,6 +31,9 @@ Post v25.03.0 > - The ovn-controller option ovn-ofctrl-wait-before-clear is no longer > supported. It will be ignored if used. ovn-controller will > automatically care about proper delay before clearing lflow. > + - Added a new ACL option "--all" to "acl-list" command. When set, > + "acl-list" command will list port groups associated with each port > of the > + target logical switch. > > OVN v25.03.0 - 07 Mar 2025 > -------------------------- > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > index 6e1f3fd45..317e33c0c 100644 > --- a/tests/ovn-nbctl.at > +++ b/tests/ovn-nbctl.at > @@ -304,7 +304,51 @@ dnl Test when same name exists in logical switches > and portgroups > AT_CHECK([ovn-nbctl create port_group name=ls0], [0], [ignore]) > AT_CHECK([ovn-nbctl acl-add ls0 to-lport 100 ip drop], [1], [], [stderr]) > AT_CHECK([grep 'exists in both' stderr], [0], [ignore]) > -AT_CHECK([ovn-nbctl --type=port-group acl-add ls0 to-lport 100 ip drop], > [0], [ignore])]) > +AT_CHECK([ovn-nbctl --type=port-group acl-add ls0 to-lport 100 ip drop], > [0], [ignore]) > + > +dnl Test using all option to show port group acls for a logical switch > +AT_CHECK([ovn-nbctl ls-del ls1], [0], [ignore]) > I think there has been some misunderstanding, this AT_CHECK can be rewritten as the following: 'check ovn-nbctl ls-add ls1' That applies to all AT_CHECK calls added by this commit that don't require to check the output, so basically everything except 'ovn-nbctl --all acl-list ls1'. > +AT_CHECK([ovn-nbctl ls-add ls1], [0], [ignore]) > +AT_CHECK([ovn-nbctl lsp-add ls1 lp0], [0], [ignore]) > +AT_CHECK([ovn-nbctl lsp-add ls1 lp1], [0], [ignore]) > +AT_CHECK([ovn-nbctl lsp-add ls1 lp2], [0], [ignore]) > + > +AT_CHECK([ovn-nbctl pg-del pg0], [0], [ignore]) > +AT_CHECK([ovn-nbctl pg-add pg0], [0], [ignore]) > +AT_CHECK([ovn-nbctl pg-set-ports pg0 lp0], [0], [ignore]) > +AT_CHECK([ovn-nbctl pg-add pg1], [0], [ignore]) > +AT_CHECK([ovn-nbctl pg-set-ports pg1 lp1], [0], [ignore]) > +AT_CHECK([ovn-nbctl pg-add pg2], [0], [ignore]) > +AT_CHECK([ovn-nbctl pg-set-ports pg2 lp0 lp2 lp1], [0], [ignore]) > + > +AT_CHECK([ovn-nbctl --type=switch acl-add ls1 to-lport 100 ip drop], [0], > [ignore]) > +AT_CHECK([ovn-nbctl --type=switch acl-add ls1 from-lport 500 ip drop], > [0], [ignore]) > +AT_CHECK([ovn-nbctl --type=port-group acl-add pg0 to-lport 144 ip drop], > [0], [ignore]) > +AT_CHECK([ovn-nbctl --type=port-group acl-add pg0 to-lport 11 ip drop], > [0], [ignore]) > +AT_CHECK([ovn-nbctl --type=port-group acl-add pg1 to-lport 12 ip drop], > [0], [ignore]) > +AT_CHECK([ovn-nbctl --type=port-group acl-add pg2 to-lport 99 ip drop], > [0], [ignore]) > +AT_CHECK([ovn-nbctl --type=port-group acl-add pg2 from-lport 100 ip > drop], [0], [ignore]) > +AT_CHECK([ovn-nbctl --type=port-group acl-add pg2 from-lport 98 ip drop], > [0], [ignore]) > + > +AT_CHECK([ovn-nbctl --all acl-list ls1], [0], [dnl > +from-lport 500 (ip) drop > + to-lport 100 (ip) drop > + to-lport 144 (ip) drop [[pg0]] > + to-lport 11 (ip) drop [[pg0]] > + to-lport 12 (ip) drop [[pg1]] > +from-lport 100 (ip) drop [[pg2]] > +from-lport 98 (ip) drop [[pg2]] > + to-lport 99 (ip) drop [[pg2]] > +]) > + > +AT_CHECK([ovn-nbctl acl-del ls1], [0], [ignore]) > +AT_CHECK([ovn-nbctl acl-del pg0], [0], [ignore]) > +AT_CHECK([ovn-nbctl acl-del pg1], [0], [ignore]) > +AT_CHECK([ovn-nbctl acl-del pg2], [0], [ignore]) > +AT_CHECK([ovn-nbctl pg-del pg0], [0], [ignore]) > +AT_CHECK([ovn-nbctl pg-del pg1], [0], [ignore]) > +AT_CHECK([ovn-nbctl pg-del pg2], [0], [ignore]) > +AT_CHECK([ovn-nbctl ls-del ls1], [0], [ignore])]) > > dnl --------------------------------------------------------------------- > > diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml > index 9f9547bdb..8569559ab 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>--sample-new=</code><var>sample</var>] > [<code>--sample-est=</code><var>sample</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> > + <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>--sample-new=</code><var>sample</var>] > [<code>--sample-est=</code><var>sample</var>] [<code>--may-exist</code>] > [<code>--apply-after-lb</code>] [<code>--tier</code>] [<code>--all</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> > @@ -441,6 +441,11 @@ > value. For more information about ACL tiers, see the > documentation > for the <code>ovn-nb</code>(5) database. > </p> > + <p> > + The <code>--all</code> option will list the port group ACLs > + associated with each logical switch port, when a logical switch > + is provided as an argument. > + </p> > </dd> > > <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> > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index 329c28cee..911869417 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -50,6 +50,7 @@ > #include "openvswitch/vlog.h" > #include "bitmap.h" > #include "vec.h" > +#include "uuidset.h" > > VLOG_DEFINE_THIS_MODULE(nbctl); > > @@ -57,6 +58,8 @@ VLOG_DEFINE_THIS_MODULE(nbctl); > * change the database at all? */ > static bool force_wait = false; > > +static int port_name_cmp(const void *s1_, const void *s2_); > + > nit: Not needed anymore. > static char * > string_ptr(char *ptr) > { > @@ -2222,31 +2225,11 @@ acl_cmd_get_pg_or_ls(struct ctl_context *ctx, > return NULL; > } > > -static void > -nbctl_acl_list(struct ctl_context *ctx) > +static inline void > +nbctl_acl_print(struct ctl_context *ctx, const struct nbrec_acl **acls, > + size_t n_acls, const char *pg_name) > { > - const struct nbrec_logical_switch *ls = NULL; > - const struct nbrec_port_group *pg = NULL; > - const struct nbrec_acl **acls; > - size_t i; > - > - char *error = acl_cmd_get_pg_or_ls(ctx, &ls, &pg); > - if (error) { > - ctx->error = error; > - return; > - } > - > - size_t n_acls = pg ? pg->n_acls : ls->n_acls; > - struct nbrec_acl **nb_acls = pg ? pg->acls : ls->acls; > - > - acls = xmalloc(sizeof *acls * n_acls); > - for (i = 0; i < n_acls; i++) { > - acls[i] = nb_acls[i]; > - } > - > - qsort(acls, n_acls, sizeof *acls, acl_cmp); > - > - for (i = 0; i < n_acls; i++) { > + for (size_t i = 0; i < n_acls; i++) { > const struct nbrec_acl *acl = acls[i]; > ds_put_format(&ctx->output, "%10s %5"PRId64" (%s) %s", > acl->direction, acl->priority, acl->match, > @@ -2271,10 +2254,70 @@ nbctl_acl_list(struct ctl_context *ctx) > if (smap_get_bool(&acl->options, "apply-after-lb", false)) { > ds_put_cstr(&ctx->output, " [after-lb]"); > } > + if (pg_name) { > + ds_put_format(&ctx->output, " [%s]", pg_name); > + } > ds_put_cstr(&ctx->output, "\n"); > } > +} > + > +static void > +nbctl_acl_list(struct ctl_context *ctx) > +{ > + const struct nbrec_logical_switch *ls = NULL; > + const struct nbrec_port_group *pg = NULL; > + const struct nbrec_acl **acls; > + > + char *error = acl_cmd_get_pg_or_ls(ctx, &ls, &pg); > + if (error) { > + ctx->error = error; > + return; > + } > + > + size_t n_acls = pg ? pg->n_acls : ls->n_acls; > + struct nbrec_acl **nb_acls = pg ? pg->acls : ls->acls; > > + acls = xmalloc(sizeof *acls * n_acls); > + for (size_t i = 0; i < n_acls; i++) { > + acls[i] = nb_acls[i]; > + } > + > + qsort(acls, n_acls, sizeof *acls, acl_cmp); > + nbctl_acl_print(ctx, acls, n_acls, NULL); > free(acls); > + if (shash_find(&ctx->options, "--all") && ls) { > + struct uuidset ports = UUIDSET_INITIALIZER(&ports); > + for (size_t i = 0; i < ls->n_ports; i++) { > + uuidset_insert(&ports, &ls->ports[i]->header_.uuid); > + } > + > + struct shash pg_map = SHASH_INITIALIZER(&pg_map); > + const struct nbrec_port_group *iter; > + NBREC_PORT_GROUP_FOR_EACH (iter, ctx->idl) { > + for (size_t i = 0; i < iter->n_ports; i++) { > + if (uuidset_find(&ports, &iter->ports[i]->header_.uuid)) { > + shash_add(&pg_map, iter->name, iter); > + break; > + } > + } > + } > + > + const struct shash_node **pg_nodes = shash_sort(&pg_map); > + for (size_t i = 0; i < shash_count(&pg_map); i++) { > + iter = shash_find_data(&pg_map, pg_nodes[i]->name); > There is no need to call shash_find_data, the data are available from the shash_node diretly: 'iter = pg_nodes[i]->data;' > + acls = xmalloc(sizeof *acls * iter->n_acls); > + for (size_t j = 0; j < iter->n_acls; j++) { > + acls[j] = iter->acls[j]; > + } > + > + qsort(acls, iter->n_acls, sizeof *acls, acl_cmp); > + nbctl_acl_print(ctx, acls, iter->n_acls, iter->name); > + free(acls); > + } > + > Missing free of the pg_nodes, that's also why one of the tests fails. > + uuidset_destroy(&ports); > + shash_destroy(&pg_map); > + } > } > > static int > @@ -2347,9 +2390,12 @@ nbctl_pre_acl(struct ctl_context *ctx) > { > ovsdb_idl_add_column(ctx->idl, &nbrec_port_group_col_name); > ovsdb_idl_add_column(ctx->idl, &nbrec_port_group_col_acls); > + ovsdb_idl_add_column(ctx->idl, &nbrec_port_group_col_ports); > > + ovsdb_idl_add_column(ctx->idl, &nbrec_logical_switch_port_col_name); > ovsdb_idl_add_column(ctx->idl, &nbrec_logical_switch_col_name); > ovsdb_idl_add_column(ctx->idl, &nbrec_logical_switch_col_acls); > + ovsdb_idl_add_column(ctx->idl, &nbrec_logical_switch_col_ports); > > ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_direction); > ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_priority); > @@ -8226,8 +8272,8 @@ static const struct ctl_command_syntax > nbctl_commands[] = { > "--apply-after-lb,--tier=,--sample-new=,--sample-est=", RW }, > { "acl-del", 1, 4, "{SWITCH | PORTGROUP} [DIRECTION [PRIORITY > MATCH]]", > 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 }, > + { "acl-list", 1, 2, "{SWITCH | PORTGROUP}", > + nbctl_pre_acl_list, nbctl_acl_list, NULL, "--all,--type=", RO }, > > /* qos commands. */ > { "qos-add", 5, 7, > -- > 2.49.0 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
