On Tue, Apr 5, 2016 at 2:24 PM, Russell Bryant <russ...@ovn.org> wrote:
>
> 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>
> ---
> ovn/controller/lflow.c | 155
+++++++++++++++++++++++++++++++++++++++++++++-
> ovn/northd/ovn-northd.c | 42 +++++++++++++
> ovn/ovn-nb.ovsschema | 12 +++-
> ovn/ovn-nb.xml | 24 +++++++
> ovn/ovn-sb.ovsschema | 12 +++-
> ovn/ovn-sb.xml | 9 +++
> ovn/utilities/ovn-nbctl.c | 4 ++
> ovn/utilities/ovn-sbctl.c | 4 ++
> tests/ovn.at | 10 +++
> 9 files changed, 267 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
> @@ -27,6 +27,7 @@
> #include "ovn/lib/ovn-sb-idl.h"
> #include "packets.h"
> #include "simap.h"
> +#include "sset.h"
>
> VLOG_DEFINE_THIS_MODULE(lflow);
>
> @@ -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. */
> +static struct shash expr_address_sets;
> +
> static void
> add_logical_register(struct shash *symtab, enum mf_field_id id)
> {
> @@ -48,6 +52,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
> @@ -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]);
This xmemdup might have some problems. Firstly, IPv6 address string size is
variant, so we cannot use the size of the first element to calculate the
total size. Secondly, since it is configurable, there can be mistakes in
the address format, or someone can even attack purposely.
> +
> + 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 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_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;
> @@ -303,7 +452,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);
> @@ -434,6 +584,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);
Just to remind @Ryan that the incremental flow computing has one more input
:)
> add_logical_flows(ctx, lports, mcgroups, local_datapaths,
> patched_datapaths, ct_zones, flow_table);
> add_neighbor_flows(ctx, lports, flow_table);
> @@ -444,4 +595,6 @@ lflow_destroy(void)
> {
> expr_symtab_destroy(&symtab);
> shash_destroy(&symtab);
> + expr_address_sets_destroy(&expr_address_sets);
> + shash_destroy(&expr_address_sets);
> }
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 4b1d611..f954591 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -2175,6 +2175,42 @@ 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. */
> +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)
> @@ -2187,6 +2223,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);
> @@ -2432,6 +2470,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 40a7a97..c26d8ae 100644
> --- a/ovn/ovn-nb.ovsschema
> +++ b/ovn/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Northbound",
> - "version": "2.0.2",
> - "cksum": "4289495412 4436",
> + "version": "2.1.0",
> + "cksum": "2262966655 4761",
> "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 e65bc3a..e383623 100644
> --- a/ovn/ovn-nb.xml
> +++ b/ovn/ovn-nb.xml
> @@ -491,6 +491,30 @@
> </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, but
> + a single address set must contain addresses all of the same type.
> + </p>
> +
> + <p>
> + Address sets are a work in progress and are not yet available for
use.
> + </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 06e8a07..22f7ad0 100644
> --- a/ovn/ovn-sb.ovsschema
> +++ b/ovn/ovn-sb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Southbound",
> - "version": "1.3.0",
> - "cksum": "654726257 5528",
> + "version": "1.4.0",
> + "cksum": "2272541964 5853",
> "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 efd2f9a..4d4ca41 100644
> --- a/ovn/ovn-sb.xml
> +++ b/ovn/ovn-sb.xml
> @@ -249,6 +249,15 @@
> </column>
> </table>
>
> + <table name="Address_Set" title="Address Sets">
> + <p>
> + See the documentation for <ref table="Address_Set"
db="OVN_Northbound"/>.
> + </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.
> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
> index bdad368..041125e 100644
> --- a/ovn/utilities/ovn-nbctl.c
> +++ b/ovn/utilities/ovn-nbctl.c
> @@ -1101,6 +1101,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 a257729..6b0e4f1 100644
> --- a/ovn/utilities/ovn-sbctl.c
> +++ b/ovn/utilities/ovn-sbctl.c
> @@ -776,6 +776,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 d10bc46..46e7828 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -618,6 +618,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 ==
address_set(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
> @@ -746,9 +748,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
droped
> + # 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
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
Thanks Russell. I haven't yet completed the review, just some comments
inlined. One additional thing came to my mind is that, even if it is much
more efficent than having IP addresses on each ACL, it is still O(nLogn)
considering the comparisons when handling the address updates. Ideally it
should be O(n) for adding/removing an IP in a set, but I understand that we
don't have the semantics for adding/deleting an element in a Set column.
Not sure if this would still create scale problems when the list grows to
hundreds or thousands of addresses. I just raise the concern, but no good
ideas yet.
--
Best regards,
Han
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
