diff mbox series

[ovs-dev,2/2] ovn-nbctl: Support ACL commands on port groups.

Message ID 1524415955-78908-2-git-send-email-hzhou8@ebay.com
State Superseded
Headers show
Series [ovs-dev,1/2] ovn: support applying ACLs to port groups | expand

Commit Message

Han Zhou April 22, 2018, 4:52 p.m. UTC 
The new option --port-group is supported for ovn-nbctl ACL related
commands. User can now ovn-nbctl to add/delete/list ACLs on port
groups. E.g.

ovn-nbctl --port-group acl-add port_group1 to-lport 1000 \
    'outport == @port_group1 && ip4.src == $port_group1_ip4' \
     allow-related

Signed-off-by: Han Zhou <hzhou8@ebay.com>
---
 ovn/utilities/ovn-nbctl.8.xml |  24 +++---
 ovn/utilities/ovn-nbctl.c     | 167 +++++++++++++++++++++++++++++++-----------
 tests/ovn-nbctl.at            |  62 +++++++++-------
 tests/ovn.at                  |  11 +--
 4 files changed, 173 insertions(+), 91 deletions(-)

Comments

Ben Pfaff May 9, 2018, 6:13 p.m. UTC | #1 
On Sun, Apr 22, 2018 at 09:52:35AM -0700, Han Zhou wrote:
> The new option --port-group is supported for ovn-nbctl ACL related
> commands. User can now ovn-nbctl to add/delete/list ACLs on port
> groups. E.g.
> 
> ovn-nbctl --port-group acl-add port_group1 to-lport 1000 \
>     'outport == @port_group1 && ip4.src == $port_group1_ip4' \
>      allow-related
> 
> Signed-off-by: Han Zhou <hzhou8@ebay.com>

Thanks for working on making ovn-nbctl more useful here.

The documentation is pretty inconsistent about whether it mentions
--port-group.

I think that in most cases the names of port groups and lswitches are
going to be different.  As a user interface convenience, I suggest that
there be *two* options: --port-group and --lswitch (or whatever).  If
either one is given, then the command works with that kind of entity.
If neither one is given, then the command works with whichever one
actually exists with the given name, or exits with an error if both
exist.

Thanks,

Ben.
Han Zhou May 9, 2018, 10:11 p.m. UTC | #2 
On Wed, May 9, 2018 at 11:13 AM, Ben Pfaff <blp@ovn.org> wrote:
>
> On Sun, Apr 22, 2018 at 09:52:35AM -0700, Han Zhou wrote:
> > The new option --port-group is supported for ovn-nbctl ACL related
> > commands. User can now ovn-nbctl to add/delete/list ACLs on port
> > groups. E.g.
> >
> > ovn-nbctl --port-group acl-add port_group1 to-lport 1000 \
> >     'outport == @port_group1 && ip4.src == $port_group1_ip4' \
> >      allow-related
> >
> > Signed-off-by: Han Zhou <hzhou8@ebay.com>
>
> Thanks for working on making ovn-nbctl more useful here.
>
> The documentation is pretty inconsistent about whether it mentions
> --port-group.
>
> I think that in most cases the names of port groups and lswitches are
> going to be different.  As a user interface convenience, I suggest that
> there be *two* options: --port-group and --lswitch (or whatever).  If
> either one is given, then the command works with that kind of entity.
> If neither one is given, then the command works with whichever one
> actually exists with the given name, or exits with an error if both
> exist.
>
This is a good suggestion. Then would it be better to have just one option
e.g. --acl-type (or just --type), and the value can be "port-group" or
"lswitch"? If the option is not provided, the command works with whichever
exists or error out if both exist. What do you think?

Thanks,
Han
Ben Pfaff May 9, 2018, 10:20 p.m. UTC | #3 
On May 9, 2018 3:11:19 PM PDT, Han Zhou <zhouhan@gmail.com> wrote:
>On Wed, May 9, 2018 at 11:13 AM, Ben Pfaff <blp@ovn.org> wrote:
>>
>> On Sun, Apr 22, 2018 at 09:52:35AM -0700, Han Zhou wrote:
>> > The new option --port-group is supported for ovn-nbctl ACL related
>> > commands. User can now ovn-nbctl to add/delete/list ACLs on port
>> > groups. E.g.
>> >
>> > ovn-nbctl --port-group acl-add port_group1 to-lport 1000 \
>> >     'outport == @port_group1 && ip4.src == $port_group1_ip4' \
>> >      allow-related
>> >
>> > Signed-off-by: Han Zhou <hzhou8@ebay.com>
>>
>> Thanks for working on making ovn-nbctl more useful here.
>>
>> The documentation is pretty inconsistent about whether it mentions
>> --port-group.
>>
>> I think that in most cases the names of port groups and lswitches are
>> going to be different.  As a user interface convenience, I suggest
>that
>> there be *two* options: --port-group and --lswitch (or whatever).  If
>> either one is given, then the command works with that kind of entity.
>> If neither one is given, then the command works with whichever one
>> actually exists with the given name, or exits with an error if both
>> exist.
>>
>This is a good suggestion. Then would it be better to have just one
>option
>e.g. --acl-type (or just --type), and the value can be "port-group" or
>"lswitch"? If the option is not provided, the command works with
>whichever
>exists or error out if both exist. What do you think?
>
>Thanks,
>Han

Sure, that's fine too.
Han Zhou May 10, 2018, 6:35 a.m. UTC | #4 
On Wed, May 9, 2018 at 3:20 PM, Ben Pfaff <blp@ovn.org> wrote:
>
> On May 9, 2018 3:11:19 PM PDT, Han Zhou <zhouhan@gmail.com> wrote:
>>
>>
>>
>> On Wed, May 9, 2018 at 11:13 AM, Ben Pfaff <blp@ovn.org> wrote:
>> >
>> > On Sun, Apr 22, 2018 at 09:52:35AM -0700, Han Zhou wrote:
>> > > The new option --port-group is supported for ovn-nbctl ACL related
>> > > commands. User can now ovn-nbctl to add/delete/list ACLs on port
>> > > groups. E.g.
>> > >
>> > > ovn-nbctl --port-group acl-add port_group1 to-lport 1000 \
>> > >     'outport == @port_group1 && ip4.src == $port_group1_ip4' \
>> > >      allow-related
>> > >
>> > > Signed-off-by: Han Zhou <hzhou8@ebay.com>
>> >
>> > Thanks for working on making ovn-nbctl more useful here.
>> >
>> > The documentation is pretty inconsistent about whether it mentions
>> > --port-group.
>> >
>> > I think that in most cases the names of port groups and lswitches are
>> > going to be different.  As a user interface convenience, I suggest that
>> > there be *two* options: --port-group and --lswitch (or whatever).  If
>> > either one is given, then the command works with that kind of entity.
>> > If neither one is given, then the command works with whichever one
>> > actually exists with the given name, or exits with an error if both
>> > exist.
>> >
>> This is a good suggestion. Then would it be better to have just one
option e.g. --acl-type (or just --type), and the value can be "port-group"
or "lswitch"? If the option is not provided, the command works with
whichever exists or error out if both exist. What do you think?
>>
>> Thanks,
>> Han
>
>
> Sure, that's fine too.
Nice, I submitted v2: https://patchwork.ozlabs.org/patch/911290/
diff mbox series

Patch

diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index bfbd842..6cbef91 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -74,12 +74,12 @@ 
       </dd>
     </dl>
 
-    <h1>Logical Switch ACL Commands</h1>
+    <h1>ACL Commands</h1>
     <dl>
-      <dt>[<code>--log</code>] [<code>--severity=</code><var>severity</var>] [<code>--name=</code><var>name</var>] [<code>--may-exist</code>] <code>acl-add</code> <var>switch</var> <var>direction</var> <var>priority</var> <var>match</var> <var>verdict</var></dt>
+      <dt>[<code>--log</code>] [<code>--severity=</code><var>severity</var>] [<code>--name=</code><var>name</var>] [<code>--may-exist</code>] <code>acl-add</code> {<var>switch</var> | <var>port_group</var>} <var>direction</var> <var>priority</var> <var>match</var> <var>verdict</var></dt>
       <dd>
         <p>
-          Adds the specified ACL to <var>switch</var>.
+          Adds the specified ACL to <var>switch</var>/<var>port_group</var>.
           <var>direction</var> must be either <code>from-lport</code> or
           <code>to-lport</code>.  <var>priority</var> must be between
           <code>0</code> and <code>32767</code>, inclusive.  A full
@@ -101,19 +101,19 @@ 
         </p>
       </dd>
 
-      <dt><code>acl-del</code> <var>switch</var> [<var>direction</var> [<var>priority</var> <var>match</var>]]</dt>
+      <dt><code>acl-del</code> {<var>switch</var> | <var>port_group</var>} [<var>direction</var> [<var>priority</var> <var>match</var>]]</dt>
       <dd>
-        Deletes ACLs from <var>switch</var>.  If only
-        <var>switch</var> is supplied, all the ACLs from the logical
-        switch are deleted.  If <var>direction</var> is also specified,
-        then all the flows in that direction will be deleted from the
-        logical switch.  If all the fields are given, then a single flow
-        that matches all the fields will be deleted.
+        Deletes ACLs from <var>switch</var>/<var>port_group</var>.  If only
+        <var>switch</var>/<var>port_group</var> is supplied, all the ACLs from
+        the logical switch or port group are deleted.  If <var>direction</var>
+        is also specified, then all the flows in that direction will be deleted
+        from the logical switch/port group.  If all the fields are given, then
+        a single flow that matches all the fields will be deleted.
       </dd>
 
-      <dt><code>acl-list</code> <var>switch</var></dt>
+      <dt><code>acl-list</code> {<var>switch</var> | <var>port_group</var>} </dt>
       <dd>
-        Lists the ACLs on <var>switch</var>.
+        Lists the ACLs on <var>switch</var>/<var>port_group</var>.
       </dd>
     </dl>
 
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 5c18161..08019d2 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -343,11 +343,12 @@  Logical switch commands:\n\
 \n\
 ACL commands:\n\
   [--log] [--severity=SEVERITY] [--name=NAME] [--may-exist]\n\
-  acl-add SWITCH DIRECTION PRIORITY MATCH ACTION\n\
+  acl-add {SWITCH | PORTGROUP} DIRECTION PRIORITY MATCH ACTION\n\
                             add an ACL to SWITCH\n\
-  acl-del SWITCH [DIRECTION [PRIORITY MATCH]]\n\
+  acl-del {SWITCH | PORTGROUP} [DIRECTION [PRIORITY MATCH]]\n\
                             remove ACLs from SWITCH\n\
-  acl-list SWITCH           print ACLs for SWITCH\n\
+  acl-list {SWITCH | PORTGROUP}\n\
+                            print ACLs for SWITCH\n\
 \n\
 QoS commands:\n\
   qos-add SWITCH DIRECTION PRIORITY MATCH [rate=RATE [burst=BURST]] [dscp=DSCP]\n\
@@ -598,6 +599,36 @@  lb_by_name_or_uuid(struct ctl_context *ctx, const char *id, bool must_exist)
     return lb;
 }
 
+static const struct nbrec_port_group *
+pg_by_name_or_uuid(struct ctl_context *ctx, const char *id, bool must_exist)
+{
+    const struct nbrec_port_group *pg = NULL;
+
+    struct uuid pg_uuid;
+    bool is_uuid = uuid_from_string(&pg_uuid, id);
+    if (is_uuid) {
+        pg = nbrec_port_group_get_for_uuid(ctx->idl, &pg_uuid);
+    }
+
+    if (!pg) {
+        const struct nbrec_port_group *iter;
+
+        NBREC_PORT_GROUP_FOR_EACH (iter, ctx->idl) {
+            if (!strcmp(iter->name, id)) {
+                pg = iter;
+                break;
+            }
+        }
+    }
+
+    if (!pg && must_exist) {
+        ctl_fatal("%s: port group %s not found", id,
+                  is_uuid ? "UUID" : "name");
+    }
+
+    return pg;
+}
+
 static void
 print_alias(const struct smap *external_ids, const char *key, struct ds *s)
 {
@@ -1372,20 +1403,29 @@  acl_cmp(const void *acl1_, const void *acl2_)
 static void
 nbctl_acl_list(struct ctl_context *ctx)
 {
-    const struct nbrec_logical_switch *ls;
+    const struct nbrec_logical_switch *ls = NULL;
+    const struct nbrec_port_group *pg = NULL;
     const struct nbrec_acl **acls;
     size_t i;
 
-    ls = ls_by_name_or_uuid(ctx, ctx->argv[1], true);
+    bool is_port_group = shash_find(&ctx->options, "--port-group") != NULL;
+    if (is_port_group) {
+        pg = pg_by_name_or_uuid(ctx, ctx->argv[1], true);
+    } else {
+        ls = ls_by_name_or_uuid(ctx, ctx->argv[1], true);
+    }
 
-    acls = xmalloc(sizeof *acls * ls->n_acls);
-    for (i = 0; i < ls->n_acls; i++) {
-        acls[i] = ls->acls[i];
+    size_t n_acls = is_port_group ? pg->n_acls : ls->n_acls;
+    struct nbrec_acl **nb_acls = is_port_group ? pg->acls : ls->acls;
+
+    acls = xmalloc(sizeof *acls * n_acls);
+    for (i = 0; i < n_acls; i++) {
+        acls[i] = nb_acls[i];
     }
 
-    qsort(acls, ls->n_acls, sizeof *acls, acl_cmp);
+    qsort(acls, n_acls, sizeof *acls, acl_cmp);
 
-    for (i = 0; i < ls->n_acls; i++) {
+    for (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,
@@ -1455,10 +1495,16 @@  parse_priority(const char *arg)
 static void
 nbctl_acl_add(struct ctl_context *ctx)
 {
-    const struct nbrec_logical_switch *ls;
+    const struct nbrec_logical_switch *ls = NULL;
+    const struct nbrec_port_group *pg = NULL;
     const char *action = ctx->argv[5];
 
-    ls = ls_by_name_or_uuid(ctx, ctx->argv[1], true);
+    bool is_port_group = shash_find(&ctx->options, "--port-group") != NULL;
+    if (is_port_group) {
+        pg = pg_by_name_or_uuid(ctx, ctx->argv[1], true);
+    } else {
+        ls = ls_by_name_or_uuid(ctx, ctx->argv[1], true);
+    }
 
     const char *direction = parse_direction(ctx->argv[2]);
     int64_t priority = parse_priority(ctx->argv[3]);
@@ -1495,9 +1541,11 @@  nbctl_acl_add(struct ctl_context *ctx)
         nbrec_acl_set_name(acl, name);
     }
 
-    /* Check if same acl already exists for the ls */
-    for (size_t i = 0; i < ls->n_acls; i++) {
-        if (!acl_cmp(&ls->acls[i], &acl)) {
+    /* Check if same acl already exists for the ls/portgroup */
+    size_t n_acls = is_port_group ? pg->n_acls : ls->n_acls;
+    struct nbrec_acl **acls = is_port_group ? pg->acls : ls->acls;
+    for (size_t i = 0; i < n_acls; i++) {
+        if (!acl_cmp(&acls[i], &acl)) {
             bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
             if (!may_exist) {
                 ctl_fatal("Same ACL already existed on the ls %s.",
@@ -1507,45 +1555,68 @@  nbctl_acl_add(struct ctl_context *ctx)
         }
     }
 
-    /* Insert the acl into the logical switch. */
-    nbrec_logical_switch_verify_acls(ls);
-    struct nbrec_acl **new_acls = xmalloc(sizeof *new_acls * (ls->n_acls + 1));
-    nullable_memcpy(new_acls, ls->acls, sizeof *new_acls * ls->n_acls);
-    new_acls[ls->n_acls] = acl;
-    nbrec_logical_switch_set_acls(ls, new_acls, ls->n_acls + 1);
+    /* Insert the acl into the logical switch/port group. */
+    struct nbrec_acl **new_acls = xmalloc(sizeof *new_acls * (n_acls + 1));
+    nullable_memcpy(new_acls, acls, sizeof *new_acls * n_acls);
+    new_acls[n_acls] = acl;
+    if (is_port_group) {
+        nbrec_port_group_verify_acls(pg);
+        nbrec_port_group_set_acls(pg, new_acls, n_acls + 1);
+    } else {
+        nbrec_logical_switch_verify_acls(ls);
+        nbrec_logical_switch_set_acls(ls, new_acls, n_acls + 1);
+    }
     free(new_acls);
 }
 
 static void
 nbctl_acl_del(struct ctl_context *ctx)
 {
-    const struct nbrec_logical_switch *ls;
-    ls = ls_by_name_or_uuid(ctx, ctx->argv[1], true);
+    const struct nbrec_logical_switch *ls = NULL;
+    const struct nbrec_port_group *pg = NULL;
+    bool is_port_group = shash_find(&ctx->options, "--port-group") != NULL;
+    if (is_port_group) {
+        pg = pg_by_name_or_uuid(ctx, ctx->argv[1], true);
+    } else {
+        ls = ls_by_name_or_uuid(ctx, ctx->argv[1], true);
+    }
 
     if (ctx->argc == 2) {
         /* If direction, priority, and match are not specified, delete
          * all ACLs. */
-        nbrec_logical_switch_verify_acls(ls);
-        nbrec_logical_switch_set_acls(ls, NULL, 0);
+        if (is_port_group) {
+            nbrec_port_group_verify_acls(pg);
+            nbrec_port_group_set_acls(pg, NULL, 0);
+        } else {
+            nbrec_logical_switch_verify_acls(ls);
+            nbrec_logical_switch_set_acls(ls, NULL, 0);
+        }
         return;
     }
 
     const char *direction = parse_direction(ctx->argv[2]);
 
+    size_t n_acls = is_port_group ? pg->n_acls : ls->n_acls;
+    struct nbrec_acl **acls = is_port_group ? pg->acls : ls->acls;
     /* If priority and match are not specified, delete all ACLs with the
      * specified direction. */
     if (ctx->argc == 3) {
-        struct nbrec_acl **new_acls = xmalloc(sizeof *new_acls * ls->n_acls);
+        struct nbrec_acl **new_acls = xmalloc(sizeof *new_acls * n_acls);
 
-        int n_acls = 0;
-        for (size_t i = 0; i < ls->n_acls; i++) {
-            if (strcmp(direction, ls->acls[i]->direction)) {
-                new_acls[n_acls++] = ls->acls[i];
+        int n_new_acls = 0;
+        for (size_t i = 0; i < n_acls; i++) {
+            if (strcmp(direction, acls[i]->direction)) {
+                new_acls[n_new_acls++] = acls[i];
             }
         }
 
-        nbrec_logical_switch_verify_acls(ls);
-        nbrec_logical_switch_set_acls(ls, new_acls, n_acls);
+        if (is_port_group) {
+            nbrec_port_group_verify_acls(pg);
+            nbrec_port_group_set_acls(pg, new_acls, n_new_acls);
+        } else {
+            nbrec_logical_switch_verify_acls(ls);
+            nbrec_logical_switch_set_acls(ls, new_acls, n_new_acls);
+        }
         free(new_acls);
         return;
     }
@@ -1557,17 +1628,23 @@  nbctl_acl_del(struct ctl_context *ctx)
     }
 
     /* Remove the matching rule. */
-    for (size_t i = 0; i < ls->n_acls; i++) {
-        struct nbrec_acl *acl = ls->acls[i];
+    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) &&
              !strcmp(direction, acl->direction)) {
             struct nbrec_acl **new_acls
-                = xmemdup(ls->acls, sizeof *new_acls * ls->n_acls);
-            new_acls[i] = ls->acls[ls->n_acls - 1];
-            nbrec_logical_switch_verify_acls(ls);
-            nbrec_logical_switch_set_acls(ls, new_acls,
-                                          ls->n_acls - 1);
+                = xmemdup(acls, sizeof *new_acls * n_acls);
+            new_acls[i] = acls[n_acls - 1];
+            if (is_port_group) {
+                nbrec_port_group_verify_acls(pg);
+                nbrec_port_group_set_acls(pg, new_acls,
+                                          n_acls - 1);
+            } else {
+                nbrec_logical_switch_verify_acls(ls);
+                nbrec_logical_switch_set_acls(ls, new_acls,
+                                              n_acls - 1);
+            }
             free(new_acls);
             return;
         }
@@ -3911,11 +3988,13 @@  static const struct ctl_command_syntax nbctl_commands[] = {
     { "ls-list", 0, 0, "", NULL, nbctl_ls_list, NULL, "", RO },
 
     /* acl commands. */
-    { "acl-add", 5, 6, "SWITCH DIRECTION PRIORITY MATCH ACTION", NULL,
-      nbctl_acl_add, NULL, "--log,--may-exist,--name=,--severity=", RW },
-    { "acl-del", 1, 4, "SWITCH [DIRECTION [PRIORITY MATCH]]", NULL,
-      nbctl_acl_del, NULL, "", RW },
-    { "acl-list", 1, 1, "SWITCH", NULL, nbctl_acl_list, NULL, "", RO },
+    { "acl-add", 5, 6, "{SWITCH | PORTGROUP} DIRECTION PRIORITY MATCH ACTION",
+      NULL, nbctl_acl_add, NULL,
+      "--log,--port-group,--may-exist,--name=,--severity=", RW },
+    { "acl-del", 1, 4, "{SWITCH | PORTGROUP} [DIRECTION [PRIORITY MATCH]]",
+      NULL, nbctl_acl_del, NULL, "--port-group", RW },
+    { "acl-list", 1, 1, "{SWITCH | PORTGROUP}",
+      NULL, nbctl_acl_list, NULL, "--port-group", RO },
 
     /* qos commands. */
     { "qos-add", 5, 7,
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 514e7e7..6bd8b8d 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -178,22 +178,19 @@  AT_CLEANUP
 
 dnl ---------------------------------------------------------------------
 
-AT_SETUP([ovn-nbctl - ACLs])
-OVN_NBCTL_TEST_START
-
-AT_CHECK([ovn-nbctl ls-add ls0])
-AT_CHECK([ovn-nbctl --log acl-add ls0 from-lport 600 udp drop])
-AT_CHECK([ovn-nbctl --log --name=test --severity=info acl-add ls0 to-lport 500 udp drop])
-AT_CHECK([ovn-nbctl acl-add ls0 from-lport 400 tcp drop])
-AT_CHECK([ovn-nbctl acl-add ls0 to-lport 300 tcp drop])
-AT_CHECK([ovn-nbctl acl-add ls0 from-lport 200 ip drop])
-AT_CHECK([ovn-nbctl acl-add ls0 to-lport 100 ip drop])
-dnl Add duplicated ACL
-AT_CHECK([ovn-nbctl acl-add ls0 to-lport 100 ip drop], [1], [], [stderr])
-AT_CHECK([grep 'already existed' stderr], [0], [ignore])
-AT_CHECK([ovn-nbctl --may-exist acl-add ls0 to-lport 100 ip drop])
-
-AT_CHECK([ovn-nbctl acl-list ls0], [0], [dnl
+m4_define([OVN_NBCTL_TEST_ACL],
+  [AT_CHECK([ovn-nbctl $2 --log acl-add $1 from-lport 600 udp drop])
+   AT_CHECK([ovn-nbctl $2 --log --name=test --severity=info acl-add $1 to-lport 500 udp drop])
+   AT_CHECK([ovn-nbctl $2 acl-add $1 from-lport 400 tcp drop])
+   AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 300 tcp drop])
+   AT_CHECK([ovn-nbctl $2 acl-add $1 from-lport 200 ip drop])
+   AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 100 ip drop])
+   dnl Add duplicated ACL
+   AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 100 ip drop], [1], [], [stderr])
+   AT_CHECK([grep 'already existed' stderr], [0], [ignore])
+   AT_CHECK([ovn-nbctl $2 --may-exist acl-add $1 to-lport 100 ip drop])
+
+   AT_CHECK([ovn-nbctl $2 acl-list $1], [0], [dnl
 from-lport   600 (udp) drop log()
 from-lport   400 (tcp) drop
 from-lport   200 (ip) drop
@@ -202,29 +199,38 @@  from-lport   200 (ip) drop
   to-lport   100 (ip) drop
 ])
 
-dnl Delete in one direction.
-AT_CHECK([ovn-nbctl acl-del ls0 to-lport])
-AT_CHECK([ovn-nbctl acl-list ls0], [0], [dnl
+   dnl Delete in one direction.
+   AT_CHECK([ovn-nbctl $2 acl-del $1 to-lport])
+   AT_CHECK([ovn-nbctl $2 acl-list $1], [0], [dnl
 from-lport   600 (udp) drop log()
 from-lport   400 (tcp) drop
 from-lport   200 (ip) drop
 ])
 
-dnl Delete all ACLs.
-AT_CHECK([ovn-nbctl acl-del ls0])
-AT_CHECK([ovn-nbctl acl-list ls0], [0], [dnl
+   dnl Delete all ACLs.
+   AT_CHECK([ovn-nbctl $2 acl-del $1])
+   AT_CHECK([ovn-nbctl $2 acl-list $1], [0], [dnl
 ])
 
-AT_CHECK([ovn-nbctl acl-add ls0 from-lport 600 udp drop])
-AT_CHECK([ovn-nbctl acl-add ls0 from-lport 400 tcp drop])
-AT_CHECK([ovn-nbctl acl-add ls0 from-lport 200 ip drop])
+   AT_CHECK([ovn-nbctl $2 acl-add $1 from-lport 600 udp drop])
+   AT_CHECK([ovn-nbctl $2 acl-add $1 from-lport 400 tcp drop])
+   AT_CHECK([ovn-nbctl $2 acl-add $1 from-lport 200 ip drop])
 
-dnl Delete a single flow.
-AT_CHECK([ovn-nbctl acl-del ls0 from-lport 400 tcp])
-AT_CHECK([ovn-nbctl acl-list ls0], [0], [dnl
+   dnl Delete a single flow.
+   AT_CHECK([ovn-nbctl $2 acl-del $1 from-lport 400 tcp])
+   AT_CHECK([ovn-nbctl $2 acl-list $1], [0], [dnl
 from-lport   600 (udp) drop
 from-lport   200 (ip) drop
 ])
+])
+
+AT_SETUP([ovn-nbctl - ACLs])
+OVN_NBCTL_TEST_START
+
+AT_CHECK([ovn-nbctl ls-add ls0])
+OVN_NBCTL_TEST_ACL([ls0])
+AT_CHECK([ovn-nbctl create port_group name=pg0], [0], [ignore])
+OVN_NBCTL_TEST_ACL([pg0], [--port-group])
 
 OVN_NBCTL_TEST_STOP
 AT_CLEANUP
diff --git a/tests/ovn.at b/tests/ovn.at
index 95f747a..6a6300b 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -9859,16 +9859,13 @@  for i in 1 2 3; do
     done
 done
 
-pg1_uuid=`ovn-nbctl create Port_Group name=pg1 ports="$pg1_ports"`
+ovn-nbctl create Port_Group name=pg1 ports="$pg1_ports"
 ovn-nbctl create Port_Group name=pg2 ports="$pg2_ports"
 
 # create ACLs on pg1 to drop traffic from pg2 to pg1
-ovn-nbctl --id=@acl1 create acl priority=1001 direction=to-lport \
-        match='"outport == @pg1"' action=drop \
-        -- add port_group $pg1_uuid acls @acl1
-ovn-nbctl --id=@acl2 create acl priority=1002 direction=to-lport \
-        match='"outport == @pg1 && ip4.src == $pg2_ip4"' action=allow-related \
-        -- add port_group $pg1_uuid acls @acl2
+ovn-nbctl --port-group acl-add pg1 to-lport 1001 'outport == @pg1' drop
+ovn-nbctl --port-group acl-add pg1 to-lport 1002 \
+        'outport == @pg1 && ip4.src == $pg2_ip4' allow-related
 
 # Physical network:
 #