[ovs-dev] [PATCH v5 2/2] ovn: Add address_set() support for ACLs.

bschanmu at redhat.com bschanmu at redhat.com
Tue Jun 28 07:29:01 UTC 2016


From: Russell Bryant <russell at ovn.org>

This feature was originally proposed here:

  http://openvswitch.org/pipermail/dev/2016-March/067440.html

A common use case for OVN ACLs involves needing to match a set of IP
addresses.

   outport == "lp1" && ip4.src == {10.0.0.5, 10.0.0.25, 10.0.0.50}

This example match only has 3 addresses, but it could easily have
hundreds of addresses.  In some cases, the same large set of addresses
needs to be used in several ACLs.

This patch adds a new Address_Set table to OVN_Northbound so that a set
of addresses can be specified once and then referred to by name in ACLs.
To recreate the above example, you would first create an address set:

  $ ovn-nbctl create Address_Set name=set1 addresses=10.0.0.5,10.0.0.25,10.0.0.50

Then you can refer to this address set by name in an ACL match:

  outport == "lp1" && ip4.src == $set1

Signed-off-by: Russell Bryant <russell at ovn.org>
Signed-off-by: Babu Shanmugam <bschanmu at redhat.com>
Co-authored-by: Flavio Fernandes <flavio at flaviof.com>
Signed-off-by: Flavio Fernandes <flavio at flaviof.com>
---
 ovn/controller/lflow.c    | 155 +++++++++++++++++++++++++++++++++++++++++++++-
 ovn/lib/expr.c            |   2 +-
 ovn/northd/ovn-northd.c   |  43 +++++++++++++
 ovn/ovn-nb.ovsschema      |  13 +++-
 ovn/ovn-nb.xml            |  32 ++++++++++
 ovn/ovn-sb.ovsschema      |  12 +++-
 ovn/ovn-sb.xml            |  19 ++++++
 ovn/utilities/ovn-nbctl.c |   4 ++
 ovn/utilities/ovn-sbctl.c |   4 ++
 tests/ovn.at              |  19 ++++++
 10 files changed, 298 insertions(+), 5 deletions(-)

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 433df70..7eebac8 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -28,6 +28,7 @@
 #include "ovn/lib/ovn-sb-idl.h"
 #include "packets.h"
 #include "simap.h"
+#include "sset.h"
 
 VLOG_DEFINE_THIS_MODULE(lflow);
 
@@ -36,6 +37,9 @@ VLOG_DEFINE_THIS_MODULE(lflow);
 /* Contains "struct expr_symbol"s for fields supported by OVN lflows. */
 static struct shash symtab;
 
+/* Contains an internal expr datastructure that represents an address set. */
+static struct shash expr_address_sets;
+
 static void
 add_logical_register(struct shash *symtab, enum mf_field_id id)
 {
@@ -49,6 +53,7 @@ void
 lflow_init(void)
 {
     shash_init(&symtab);
+    shash_init(&expr_address_sets);
 
     /* Reserve a pair of registers for the logical inport and outport.  A full
      * 32-bit register each is bigger than we need, but the expression code
@@ -158,6 +163,150 @@ lflow_init(void)
     expr_symtab_add_field(&symtab, "sctp.src", MFF_SCTP_SRC, "sctp", false);
     expr_symtab_add_field(&symtab, "sctp.dst", MFF_SCTP_DST, "sctp", false);
 }
+
+/* Details of an address set currently in address_sets. We keep a cached
+ * copy of sets still in their string form here to make it easier to compare
+ * with the current values in the OVN_Southbound database. */
+struct address_set {
+    char **addresses;
+    size_t n_addresses;
+};
+
+/* struct address_set instances for address sets currently in the symtab,
+ * hashed on the address set name. */
+static struct shash local_address_sets = SHASH_INITIALIZER(&local_address_sets);
+
+static int
+addr_cmp(const void *p1, const void *p2)
+{
+    const char *s1 = p1;
+    const char *s2 = p2;
+    return strcmp(s1, s2);
+}
+
+/* Return true if the address sets match, false otherwise. */
+static bool
+address_sets_match(const struct address_set *addr_set,
+                   const struct sbrec_address_set *addr_set_rec)
+{
+    char **addrs1;
+    char **addrs2;
+
+    if (addr_set->n_addresses != addr_set_rec->n_addresses) {
+        return false;
+    }
+    size_t n_addresses = addr_set->n_addresses;
+
+    addrs1 = xmemdup(addr_set->addresses,
+                     n_addresses * sizeof addr_set->addresses[0]);
+    addrs2 = xmemdup(addr_set_rec->addresses,
+                     n_addresses * sizeof addr_set_rec->addresses[0]);
+
+    qsort(addrs1, n_addresses, sizeof *addrs1, addr_cmp);
+    qsort(addrs2, n_addresses, sizeof *addrs2, addr_cmp);
+
+    bool res = true;
+    size_t i;
+    for (i = 0; i <  n_addresses; i++) {
+        if (strcmp(addrs1[i], addrs2[i])) {
+            res = false;
+            break;
+        }
+    }
+
+    free(addrs1);
+    free(addrs2);
+
+    return res;
+}
+
+static void
+address_set_destroy(struct address_set *addr_set)
+{
+    size_t i;
+    for (i = 0; i < addr_set->n_addresses; i++) {
+        free(addr_set->addresses[i]);
+    }
+    if (addr_set->n_addresses) {
+        free(addr_set->addresses);
+    }
+    free(addr_set);
+}
+
+static void
+update_address_sets(struct controller_ctx *ctx)
+{
+    /* Remember the names of all address sets currently in expr_address_sets
+     * so we can detect address sets that have been deleted. */
+    struct sset cur_addr_set_names = SSET_INITIALIZER(&cur_addr_set_names);
+
+    struct shash_node *node;
+    SHASH_FOR_EACH (node, &local_address_sets) {
+        sset_add(&cur_addr_set_names, node->name);
+    }
+
+    /* Iterate address sets in the southbound database.  Create and update the
+     * corresponding symtab entries as necessary. */
+    const struct sbrec_address_set *addr_set_rec;
+    SBREC_ADDRESS_SET_FOR_EACH (addr_set_rec, ctx->ovnsb_idl) {
+        struct address_set *addr_set =
+            shash_find_data(&local_address_sets, addr_set_rec->name);
+
+        bool create_set = false;
+        if (addr_set) {
+            /* This address set has already been added.  We must determine
+             * if the symtab entry needs to be updated due to a change. */
+            sset_find_and_delete(&cur_addr_set_names, addr_set_rec->name);
+            if (!address_sets_match(addr_set, addr_set_rec)) {
+                expr_macros_remove(&expr_address_sets, addr_set_rec->name);
+                address_set_destroy(addr_set);
+                addr_set = NULL;
+                create_set = true;
+            }
+        } else {
+            /* This address set is not yet in the symtab, so add it. */
+            create_set = true;
+        }
+
+        if (create_set) {
+            /* The address set is either new or has changed.  Create a symbol
+             * that resolves to the full set of addresses.  Store it in
+             * address_sets to remember that we created this symbol. */
+            addr_set = xzalloc(sizeof *addr_set);
+            addr_set->n_addresses = addr_set_rec->n_addresses;
+            if (addr_set_rec->n_addresses) {
+                addr_set->addresses = xmalloc(addr_set_rec->n_addresses
+                                              * sizeof addr_set->addresses[0]);
+                size_t i;
+                for (i = 0; i < addr_set_rec->n_addresses; i++) {
+                    addr_set->addresses[i] = xstrdup(addr_set_rec->addresses[i]);
+                }
+            }
+            shash_add(&local_address_sets, addr_set_rec->name, addr_set);
+
+            expr_macros_add(&expr_address_sets, addr_set_rec->name,
+                            (const char * const *) addr_set->addresses,
+                            addr_set->n_addresses);
+        }
+    }
+
+    /* Anything remaining in cur_addr_set_names refers to an address set that
+     * has been deleted from the southbound database.  We should delete
+     * the corresponding symtab entry. */
+    const char *cur_node, *next_node;
+    SSET_FOR_EACH_SAFE (cur_node, next_node, &cur_addr_set_names) {
+        expr_macros_remove(&expr_address_sets, cur_node);
+
+        struct address_set *addr_set
+            = shash_find_and_delete(&local_address_sets, cur_node);
+        address_set_destroy(addr_set);
+
+        struct sset_node *sset_node = SSET_NODE_FROM_NAME(cur_node);
+        sset_delete(&cur_addr_set_names, sset_node);
+    }
+
+    sset_destroy(&cur_addr_set_names);
+}
 
 struct lookup_port_aux {
     const struct lport_index *lports;
@@ -306,7 +455,8 @@ add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports,
         struct hmap matches;
         struct expr *expr;
 
-        expr = expr_parse_string(lflow->match, &symtab, NULL, &error);
+        expr = expr_parse_string(lflow->match, &symtab,
+                                 &expr_address_sets, &error);
         if (!error) {
             if (prereqs) {
                 expr = expr_combine(EXPR_T_AND, expr, prereqs);
@@ -439,6 +589,7 @@ lflow_run(struct controller_ctx *ctx, const struct lport_index *lports,
           const struct hmap *patched_datapaths,
           const struct simap *ct_zones, struct hmap *flow_table)
 {
+    update_address_sets(ctx);
     add_logical_flows(ctx, lports, mcgroups, local_datapaths,
                       patched_datapaths, ct_zones, flow_table);
     add_neighbor_flows(ctx, lports, flow_table);
@@ -449,4 +600,6 @@ lflow_destroy(void)
 {
     expr_symtab_destroy(&symtab);
     shash_destroy(&symtab);
+    expr_macros_destroy(&expr_address_sets);
+    shash_destroy(&expr_address_sets);
 }
diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index 2883ca9..59e3f42 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -741,7 +741,7 @@ parse_macros(struct expr_context *ctx, struct expr_constant_set *cs,
     struct expr_constant_set *addr_set
         = shash_find_data(ctx->macros, ctx->lexer->token.s);
     if (!addr_set) {
-        expr_syntax_error(ctx, "Unknown macro: '%s'", ctx->lexer->token.s);
+        expr_syntax_error(ctx, "Unknown macro: '%s'.", ctx->lexer->token.s);
         return false;
     }
 
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index c2cf15e..71aecae 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -2516,6 +2516,43 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths,
     }
     hmap_destroy(&mcgroups);
 }
+
+/* OVN_Northbound and OVN_Southbound have an identical Address_Set table.
+ * We always update OVN_Southbound to match the current data in
+ * OVN_Northbound, so that the address sets used in Logical_Flows in 
+ * OVN_Southbound is checked against the proper set.*/
+static void
+sync_address_sets(struct northd_context *ctx)
+{
+    struct shash sb_address_sets = SHASH_INITIALIZER(&sb_address_sets);
+
+    const struct sbrec_address_set *sb_address_set;
+    SBREC_ADDRESS_SET_FOR_EACH (sb_address_set, ctx->ovnsb_idl) {
+        shash_add(&sb_address_sets, sb_address_set->name, sb_address_set);
+    }
+
+    const struct nbrec_address_set *nb_address_set;
+    NBREC_ADDRESS_SET_FOR_EACH (nb_address_set, ctx->ovnnb_idl) {
+        sb_address_set = shash_find_and_delete(&sb_address_sets,
+                                               nb_address_set->name);
+        if (!sb_address_set) {
+            sb_address_set = sbrec_address_set_insert(ctx->ovnsb_txn);
+            sbrec_address_set_set_name(sb_address_set, nb_address_set->name);
+        }
+
+        sbrec_address_set_set_addresses(sb_address_set,
+                /* "char **" is not compatible with "const char **" */
+                (const char **) nb_address_set->addresses,
+                nb_address_set->n_addresses);
+    }
+
+    struct shash_node *node, *next;
+    SHASH_FOR_EACH_SAFE (node, next, &sb_address_sets) {
+        sbrec_address_set_delete(node->data);
+        shash_delete(&sb_address_sets, node);
+    }
+    shash_destroy(&sb_address_sets);
+}
 
 static void
 ovnnb_db_run(struct northd_context *ctx)
@@ -2528,6 +2565,8 @@ ovnnb_db_run(struct northd_context *ctx)
     build_ports(ctx, &datapaths, &ports);
     build_lflows(ctx, &datapaths, &ports);
 
+    sync_address_sets(ctx);
+
     struct ovn_datapath *dp, *next_dp;
     HMAP_FOR_EACH_SAFE (dp, next_dp, key_node, &datapaths) {
         ovn_datapath_destroy(&datapaths, dp);
@@ -2772,6 +2811,10 @@ main(int argc, char *argv[])
     add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_binding_col_mac);
     ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_port_binding_col_chassis);
 
+    ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_address_set);
+    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_address_set_col_name);
+    add_column_noalert(ovnsb_idl_loop.idl, &sbrec_address_set_col_addresses);
+
     /* Main loop. */
     exiting = false;
     while (!exiting) {
diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index 58f04b2..2b28d09 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
     "name": "OVN_Northbound",
     "version": "3.1.0",
-    "cksum": "1426508118 6135",
+    "cksum": "2796586351 6460",
     "tables": {
         "Logical_Switch": {
             "columns": {
@@ -48,6 +48,17 @@
                              "min": 0, "max": "unlimited"}}},
             "indexes": [["name"]],
             "isRoot": false},
+        "Address_Set": {
+            "columns": {
+                "name": {"type": "string"},
+                "addresses": {"type": {"key": "string",
+                                       "min": 0,
+                                       "max": "unlimited"}},
+                "external_ids": {
+                    "type": {"key": "string", "value": "string",
+                             "min": 0, "max": "unlimited"}}},
+            "indexes": [["name"]],
+            "isRoot": true},
         "ACL": {
             "columns": {
                 "priority": {"type": {"key": {"type": "integer",
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 6355c44..58a3a32 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -484,6 +484,38 @@
     </group>
   </table>
 
+  <table name="Address_Set" title="Address Sets">
+    <p>
+      Each row in this table represents a named set of addresses.
+      An address set may contain MAC, IPv4, or IPv6 addresses and cidrs.
+      The address set will ultimately be used in ACLs, where a certain
+      type of field such as ip4.src or ip6.src will be compared with the
+      address set. So, a single address set must contain addresses of the
+      same type.
+    </p>
+
+    <p>
+      Address sets can be used in the <ref column="match" table="ACL"/> column
+      of the <ref table="ACL"/> table.  For syntax information, see the details
+      of the expression language used for the <ref column="match"
+      table="Logical_Flow" db="OVN_Southbound"/> column in the <ref
+      table="Logical_Flow" db="OVN_Southbound"/> table of the <ref
+      db="OVN_Southbound"/> database.
+    </p>
+
+    <column name="name">
+      <p>
+        A name for the address set.  This must be unique among all address sets.
+      </p>
+    </column>
+
+    <column name="addresses">
+      <p>
+        The set of addresses.
+      </p>
+    </column>
+  </table>
+
   <table name="ACL" title="Access Control List (ACL) rule">
     <p>
       Each row in this table represents one ACL rule for a logical switch
diff --git a/ovn/ovn-sb.ovsschema b/ovn/ovn-sb.ovsschema
index a1343c9..aab4ef5 100644
--- a/ovn/ovn-sb.ovsschema
+++ b/ovn/ovn-sb.ovsschema
@@ -1,7 +1,7 @@
 {
     "name": "OVN_Southbound",
-    "version": "1.4.0",
-    "cksum": "198773462 6073",
+    "version": "1.5.0",
+    "cksum": "2807058982 6398",
     "tables": {
         "Chassis": {
             "columns": {
@@ -28,6 +28,14 @@
                                      "min": 0,
                                      "max": "unlimited"}},
                 "ip": {"type": "string"}}},
+        "Address_Set": {
+            "columns": {
+                "name": {"type": "string"},
+                "addresses": {"type": {"key": "string",
+                                       "min": 0,
+                                       "max": "unlimited"}}},
+            "indexes": [["name"]],
+            "isRoot": true},
         "Logical_Flow": {
             "columns": {
                 "logical_datapath": {"type": {"key": {"type": "uuid",
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index f330374..e6ccbcc 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -249,6 +249,17 @@
     </column>
   </table>
 
+  <table name="Address_Set" title="Address Sets">
+    <p>
+      See the documentation for the <ref table="Address_Set"
+      db="OVN_Northbound"/> table in the <ref db="OVN_Northbound"/> database
+      for details.
+    </p>
+
+    <column name="name"/>
+    <column name="addresses"/>
+  </table>
+
   <table name="Logical_Flow" title="Logical Network Flows">
     <p>
       Each row in this table represents one logical flow.
@@ -656,6 +667,14 @@
         </code>...<code></code>''.
       </p>
 
+      <p>
+        You may refer to a set of IPv4, IPv6, or MAC addresses stored in the
+        <ref table="Address_Set"/> table by its <ref column="name"
+        table="Address_Set"/>.  An <ref table="Address_Set"/> with a name
+        of <code>set1</code> can be referred to as
+        <code>$set1</code>.
+      </p>
+
       <p><em>Miscellaneous</em></p>
 
       <p>
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 345647a..c673e32 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -1866,6 +1866,10 @@ static const struct ctl_table_class tables[] = {
        NULL},
       {NULL, NULL, NULL}}},
 
+    {&nbrec_table_address_set,
+     {{&nbrec_table_address_set, &nbrec_address_set_col_name, NULL},
+      {NULL, NULL, NULL}}},
+
     {NULL, {{NULL, NULL, NULL}, {NULL, NULL, NULL}}}
 };
 
diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
index a12b247..d85efd5 100644
--- a/ovn/utilities/ovn-sbctl.c
+++ b/ovn/utilities/ovn-sbctl.c
@@ -779,6 +779,10 @@ static const struct ctl_table_class tables[] = {
      {{&sbrec_table_mac_binding, &sbrec_mac_binding_col_logical_port, NULL},
       {NULL, NULL, NULL}}},
 
+    {&sbrec_table_address_set,
+     {{&sbrec_table_address_set, &sbrec_address_set_col_name, NULL},
+      {NULL, NULL, NULL}}},
+
     {NULL, {{NULL, NULL, NULL}, {NULL, NULL, NULL}}}
 };
 
diff --git a/tests/ovn.at b/tests/ovn.at
index 840d0ef..3443708 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -259,6 +259,9 @@ ip4.src == ::1 => 128-bit constant is not compatible with 32-bit field ip4.src.
 1 == eth.type == 2 => Range expressions must have the form `x < field < y' or `x > field > y', with each `<' optionally replaced by `<=' or `>' by `>=').
 
 eth.dst[40] x => Extra tokens at end of input.
+
+ip4.src == {1.2.3.4, $set1, $unkownset} => Syntax error at `$unkownset' Unknown macro: 'unkownset'.
+eth.src == {$set3, badmac, 00:00:00:00:00:01} => Syntax error at `badmac' expecting constant.
 ]])
 sed 's/ =>.*//' test-cases.txt > input.txt
 sed 's/.* => //' test-cases.txt > expout
@@ -482,6 +485,12 @@ dl_src=00:00:00:00:00:01
 dl_src=00:00:00:00:00:02
 dl_src=00:00:00:00:00:03
 ])
+AT_CHECK([expr_to_flow 'eth.src == {00:00:00:00:00:01, $set3, ba:be:be:ef:de:ad, $set3}'], [0], [dnl
+dl_src=00:00:00:00:00:01
+dl_src=00:00:00:00:00:02
+dl_src=00:00:00:00:00:03
+dl_src=ba:be:be:ef:de:ad
+])
 AT_CLEANUP
 
 AT_SETUP([ovn -- action parsing])
@@ -674,6 +683,8 @@ done
 ovn-nbctl acl-add lsw0 from-lport 1000 'eth.type == 0x1234' drop
 ovn-nbctl acl-add lsw0 from-lport 1000 'eth.type == 0x1235 && inport == "lp11"' drop
 ovn-nbctl acl-add lsw0 to-lport 1000 'eth.type == 0x1236 && outport == "lp33"' drop
+ovn-nbctl create Address_Set name=set1 addresses=\"f0:00:00:00:00:11\",\"f0:00:00:00:00:21\",\"f0:00:00:00:00:31\"
+ovn-nbctl acl-add lsw0 to-lport 1000 'eth.type == 0x1237 && eth.src == $set1 && outport == "lp33"' drop
 
 # Pre-populate the hypervisors' ARP tables so that we don't lose any
 # packets for ARP resolution (native tunneling doesn't queue packets
@@ -804,9 +815,17 @@ for is in 1 2 3; do
 
                 if test $d != $s && test $s != 11; then acl2=$d; else acl2=; fi
                 if test $d != $s && test $d != 33; then acl3=$d; else acl3=; fi
+                if test $d == $s || (test $js == 1 && test $d == 33); then
+                    # Source of 11, 21, or 31 and dest of 33 should be dropped
+                    # due to the 4th ACL that uses address_set(set1).
+                    acl4=
+                else
+                    acl4=$d
+                fi
                 test_packet $s f000000000$d f000000000$s 1234        #7, acl1
                 test_packet $s f000000000$d f000000000$s 1235 $acl2  #7, acl2
                 test_packet $s f000000000$d f000000000$s 1236 $acl3  #7, acl3
+                test_packet $s f000000000$d f000000000$s 1237 $acl4  #7, acl4
 
                 test_packet $s f000000000$d f00000000055 810000091234      #4
                 test_packet $s f000000000$d 0100000000$s $s$d              #5
-- 
2.5.5




More information about the dev mailing list