diff mbox

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

Message ID 1467018052-18740-3-git-send-email-bschanmu@redhat.com
State Superseded
Headers show

Commit Message

Babu Shanmugam June 27, 2016, 9 a.m. UTC 
From: Russell Bryant <russell@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@ovn.org>
Signed-off-by: Babu Shanmugam <bschanmu@redhat.com>
Co-authored-by: Flavio Fernandes <flavio@flaviof.com>
Signed-off-by: Flavio Fernandes <flavio@flaviof.com>
---
 ovn/controller/lflow.c    | 155 +++++++++++++++++++++++++++++++++++++++++++++-
 ovn/lib/expr.c            |   7 ++-
 ovn/northd/ovn-northd.c   |  43 +++++++++++++
 ovn/ovn-nb.ovsschema      |  10 ++-
 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, 299 insertions(+), 6 deletions(-)

Comments

Zong Kai LI June 27, 2016, 3:20 p.m. UTC | #1 
>
> +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) {
> +        node = shash_find(&local_address_sets, addr_set_rec->name);
> +        struct address_set *addr_set = node ? node->data : NULL;
> +
>

How about using shash_find_data here?
struct address_set *addr_set = shash_find_data(&local_address_sets,
addr_set_rec->name);

+
> +    /* 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);
>

Trivial: this seems unnecessary, sset_destroy will call sset_clear, and
sset_clear will call sset_delete.


> +    }
> +
> +    sset_destroy(&cur_addr_set_names);
> +}
>
> +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);
>

Trivial: this seems unnecessary, shash_destroy will delete and free node in
sb_address_sets.

+        shash_delete(&sb_address_sets, node);
> +    }
> +    shash_destroy(&sb_address_sets);
> +}
>
>
> +  <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>
> +
>

Thanks for updating this.
Babu Shanmugam June 28, 2016, 7:19 a.m. UTC | #2 
Hi Zong Kai LI,
My comments are in-lined below.

On Monday 27 June 2016 08:50 PM, Zong Kai LI wrote:
>
>     +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) {
>     +        node = shash_find(&local_address_sets, addr_set_rec->name);
>     +        struct address_set *addr_set = node ? node->data : NULL;
>     +
>
> How about using shash_find_data here?
> struct address_set *addr_set = shash_find_data(&local_address_sets, 
> addr_set_rec->name);

I agree.

>
>     +
>     +    /* 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);
>
>
> Trivial: this seems unnecessary, sset_destroy will call sset_clear, 
> and sset_clear will call sset_delete.

sset_destroy will have to loop the set from the beginning to delete the 
entries. Since we are anyway looping it before destroy(), it might save 
some minimal computing time. What do you think?

>     +   }
>     +
>     +    sset_destroy(&cur_addr_set_names);
>     +}
>
>     +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);
>
>
> Trivial: this seems unnecessary, shash_destroy will delete and free 
> node in sb_address_sets.
>

Same as the previous answer. Your suggestions are most welcome.

>     +        shash_delete(&sb_address_sets, node);
>     +    }
>     +    shash_destroy(&sb_address_sets);
>     +}
>
>
>     +  <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>
>     +
>
>
> Thanks for updating this.
Thank you for reporting :)

--
Babu
diff mbox

Patch

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 433df70..8fb35ac 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) {
+        node = shash_find(&local_address_sets, addr_set_rec->name);
+        struct address_set *addr_set = node ? node->data : NULL;
+
+        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 762fbaf..59e3f42 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -741,11 +741,14 @@  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;
+    }
+
+    if (!assign_constant_set_type(ctx, cs, EXPR_C_INTEGER)) {
         return false;
     }
 
-    cs->type = EXPR_C_INTEGER;
     size_t n_values = cs->n_values + addr_set->n_values;
     if (n_values >= *allocated_values) {
         cs->values = xrealloc(cs->values, n_values * sizeof *cs->values);
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..e5ae043 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,14 @@ 
                              "min": 0, "max": "unlimited"}}},
             "indexes": [["name"]],
             "isRoot": false},
+        "Address_Set": {
+            "columns": {
+                "name": {"type": "string"},
+                "addresses": {"type": {"key": "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