[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