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

Han Zhou zhouhan at gmail.com
Sun Apr 22 16:52:35 UTC 2018


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 at 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(-)

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:
 #
-- 
2.1.0



More information about the dev mailing list