On Thu, Jun 23, 2016 at 1:05 AM, <bscha...@redhat.com> wrote:
> From: Russell Bryant <russ...@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 <russ...@ovn.org>
> Signed-off-by: Babu Shanmugam <bscha...@redhat.com>
> ---
> ovn/controller/lflow.c | 155
> +++++++++++++++++++++++++++++++++++++++++++++-
> ovn/northd/ovn-northd.c | 42 +++++++++++++
> ovn/ovn-nb.ovsschema | 10 ++-
> ovn/ovn-nb.xml | 28 +++++++++
> 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, 280 insertions(+), 4 deletions(-)
>
> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> index 263cba6..85bb35b 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 data structure 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)
>
- make both parameters const (const struct sbrec_address_set *)
- why not call these addr_set1 and addr_set2 ?
> +{
> + 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);
>
This sset is not an address_set, but address_set names (or keys).
How about renaming this variable to cur_address_set_names ?
> +
> + 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_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_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_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_address_sets, sset_node);
> + }
> +
> + sset_destroy(&cur_address_sets);
> +}
>
> struct lookup_port_aux {
> const struct lport_index *lports;
> @@ -297,7 +446,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);
> @@ -428,6 +578,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);
> @@ -438,4 +589,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/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 17713ec..52c2c38 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -2491,6 +2491,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);
> + }
> +
>
I'm still wrapping my head around ctx and txn but I thought of asking a
dumb question here:
In cases when hash_find_and_delete returns a non-null sb_address_set, is
there a need
to clean anything up before calling sbrec_address_set_set_addresses? If so,
this override may be causing a leak?
> + 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)
> @@ -2503,6 +2539,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);
> @@ -2747,6 +2785,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..b4607f8 100644
> --- a/ovn/ovn-nb.xml
> +++ b/ovn/ovn-nb.xml
> @@ -484,6 +484,34 @@
> </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 also IPv4 + IPv6 cidr (w.x.y.z/N), right?
+ </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 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 e9353f3..361f1d7 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 7789cdd..1b2a657 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 c0ee518..39fdd81 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 4f72107..59f9307 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -649,6 +649,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
>
it would be good to add some more tests to exercise the delete and update
codepaths.
If you think it would help, I could take a stab at them.
>
> # Pre-populate the hypervisors' ARP tables so that we don't lose any
> # packets for ARP resolution (native tunneling doesn't queue packets
> @@ -779,9 +781,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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
