"dev" <dev-boun...@openvswitch.org> wrote on 04/07/2016 10:46:45 AM:
> From: Russell Bryant <russ...@ovn.org> > To: dev@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-boun...@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 <russ...@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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev