On Mon, Jun 30, 2025 at 04:33:33PM +0200, Dumitru Ceara wrote: > On 6/25/25 10:22 AM, Mairtin O'Loingsigh via dev 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, > > Thanks for this new version (and thanks, Ales, for reviews on the older > versions)! I think this is a feature that improves the life of people > debugging OVN ACLs. > > > v2: Code review changes > > v3: Cleanup autotest > > Add pg_nodes free > > Fix code nits > > > > NEWS | 3 ++ > > tests/ovn-nbctl.at | 44 +++++++++++++++++- > > utilities/ovn-nbctl.8.xml | 7 ++- > > utilities/ovn-nbctl.c | 96 ++++++++++++++++++++++++++++----------- > > 4 files changed, 122 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 > > Nit: s/will list port groups associated/will also list port group ACLs > associated/ > > > + target logical switch. > > > > OVN v25.03.0 - 07 Mar 2025 > > -------------------------- > > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > > index 6e1f3fd45..7f0bf79cb 100644 > > --- a/tests/ovn-nbctl.at > > +++ b/tests/ovn-nbctl.at > > @@ -304,7 +304,49 @@ 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 > > Nit: comments should be sentences (and should end with a '.'). Also it > might better if we s/all/--all/. > > > +check ovn-nbctl ls-del ls1 > > +check ovn-nbctl ls-add ls1 > > +check ovn-nbctl lsp-add ls1 lp0 > > +check ovn-nbctl lsp-add ls1 lp1 > > +check ovn-nbctl lsp-add ls1 lp2 > > +check ovn-nbctl pg-del pg0 > > +check ovn-nbctl pg-add pg0 > > +check ovn-nbctl pg-set-ports pg0 lp0 > > +check ovn-nbctl pg-add pg1 > > +check ovn-nbctl pg-set-ports pg1 lp1 > > +check ovn-nbctl pg-add pg2 > > +check ovn-nbctl pg-set-ports pg2 lp0 lp2 lp1 > > +check ovn-nbctl --type=switch acl-add ls1 to-lport 100 ip drop > > +check ovn-nbctl --type=switch acl-add ls1 from-lport 500 ip drop > > +check ovn-nbctl --type=port-group acl-add pg0 to-lport 144 ip drop > > +check ovn-nbctl --type=port-group acl-add pg0 to-lport 11 ip drop > > +check ovn-nbctl --type=port-group acl-add pg1 to-lport 12 ip drop > > +check ovn-nbctl --type=port-group acl-add pg2 to-lport 99 ip drop > > +check ovn-nbctl --type=port-group acl-add pg2 from-lport 100 ip drop > > +check ovn-nbctl --type=port-group acl-add pg2 from-lport 98 ip drop > > + > > +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]] > > +]) > > + > > +check ovn-nbctl acl-del ls1 > > +check ovn-nbctl acl-del pg0 > > +check ovn-nbctl acl-del pg1 > > +check ovn-nbctl acl-del pg2 > > +check ovn-nbctl pg-del pg0 > > +check ovn-nbctl pg-del pg1 > > +check ovn-nbctl pg-del pg2 > > +check ovn-nbctl ls-del ls1]) > > > > 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> > > The `--all` reference should actually be on the acl-list command. > > > <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 > > Nit: s/will list/will also list/ > > > + 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..6f52e921a 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); > > > > @@ -2222,31 +2223,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 > > Nit: we don't really need "inline". > > > +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 +2252,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; > > Nit: this can be declared below, where we actually use it first. > > > + > > + 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); > > Nit: I'd add a newline here. > > > + 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 = 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); > > + } > > + > > + uuidset_destroy(&ports); > > + shash_destroy(&pg_map); > > + free(pg_nodes); > > + } > > } > > > > static int > > @@ -2347,9 +2388,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 +8270,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, > > I took care of all the small comments I had above and applied the patch > to main. > > Thanks, > Dumitru > > Hi Dumitru,
Thanks for making these changes and applying the patch. Mairtin _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
