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

Ryan Moats rmoats at us.ibm.com
Thu Apr 7 19:52:47 UTC 2016


"dev" <dev-bounces at openvswitch.org> wrote on 04/07/2016 10:46:45 AM:

> From: Russell Bryant <russell at ovn.org>
> To: dev at openvswitch.org
> Date: 04/07/2016 10:47 AM
> Subject: [ovs-dev] [PATCH v2 3/3] ovn: Add address_set() support for
ACLs.
> Sent by: "dev" <dev-bounces at openvswitch.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 == address_set(set1)
>
> Signed-off-by: Russell Bryant <russell at ovn.org>
> ---

Yes, this works and yes, I like having the address set in both
northbound and southbound.  I've got two nits in the comments though:

>  ovn/controller/lflow.c    | 155 +++++++++++++++++++++++++++++++++++
> ++++++++++-
>  ovn/northd/ovn-northd.c   |  42 +++++++++++++
>  ovn/ovn-nb.ovsschema      |  12 +++-
>  ovn/ovn-nb.xml            |  29 +++++++++
>  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              |  10 +++
>  9 files changed, 282 insertions(+), 5 deletions(-)
>
> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> index 287ffd3..00b9e67 100644
> --- a/ovn/controller/lflow.c
> +++ b/ovn/controller/lflow.c
> @@ -35,6 +36,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. */

data structure?

> +static struct shash expr_address_sets;
> +
>  static void
>  add_logical_register(struct shash *symtab, enum mf_field_id id)
>  {
> @@ -157,6 +162,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(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_address_sets = SSET_INITIALIZER(&cur_address_sets);
> +
> +    struct shash_node *node;
> +    SHASH_FOR_EACH (node, &local_address_sets) {
> +        sset_add(&cur_address_sets, node->name);
> +    }
> +
> +    /* Iterate address sets in the southbound database.  Create
andupdate the

and update?

> +     * 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_address_sets, addr_set_rec->name);
> +            if (!address_sets_match(addr_set, addr_set_rec)) {
> +                expr_address_sets_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_address_sets_add(&expr_address_sets, addr_set_rec->
name,
> +                                  (const char * const *) addr_set->
addresses,
> +                                  addr_set->n_addresses);
> +        }
> +    }
> +
> +    /* Anything remaining in cur_address_sets 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_address_sets) {
> +        expr_address_sets_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_address_sets, sset_node);
> +    }
> +
> +    sset_destroy(&cur_address_sets);
> +}
>
>  struct lookup_port_aux {
>      const struct lport_index *lports;

Ryan



More information about the dev mailing list