Acked-by: Daniel Alvarez <dalva...@redhat.com>
Thanks Han! Everything LGTM and the test pass okay against current master.
On Thu, Apr 5, 2018 at 2:51 AM, Han Zhou <zhou...@gmail.com> wrote:
> This patch enables using port group names in ACL match conditions.
> Users can create a port group in northbound DB Port_Group table,
> and then use the name of the port group in ACL match conditions
> for "inport" or "outport". It can help reduce the number of ACLs
> for CMS clients such as OpenStack Neutron, for the use cases
> where a group of logical ports share same ACL rules except the
> "inport"/"outport" part. Without this patch, the clients have to
> create N (N = number of lports) ACLs, and this patch helps achieve
> the same goal with only one ACL. E.g.:
>
> to-lport 1000 "outport == @port_group1 && ip4.src == {IP1, IP2, ...}"
> allow-related
>
> There was a similar attempt by Zong Kai Li in 2016 [1]. This patch
> takes a slightly different approach by using weak refs instead of
> strings, which requires a new table instead of reusing the address
> set table. This way it will also benefit for a follow up patch that
> enables generating address sets automatically from port groups to
> avoid a lot a trouble from client perspective [2].
>
> An extra benefit of this patch is that it could enable conjunctive
> match effectively. As reported at [3], this patch was tested together
> with the conjunctive match enhancement patch [4], and huge performance
> improvement (more than 10x faster) was seen because of this.
>
> [1] https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/077118.html
> [2] https://mail.openvswitch.org/pipermail/ovs-discuss/2018-
> February/046260.html
> [3] https://mail.openvswitch.org/pipermail/ovs-dev/2018-March/344873.html
> [4] https://patchwork.ozlabs.org/patch/874433/
>
> Reported-by: Daniel Alvarez Sanchez <dalva...@redhat.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-
> February/046166.html
> Tested-by: Mark Michelson <mmich...@redhat.com>
> Reviewed-by: Mark Michelson <mmich...@redhat.com>
> Reviewed-by: Daniel Alvarez <dalva...@redhat.com>
> Signed-off-by: Han Zhou <hzh...@ebay.com>
> ---
>
> Notes:
> v1->v2:
> - rebased on master
> - corrected typos in logs/comments
>
> NEWS | 2 +
> include/ovn/expr.h | 24 +++++---
> include/ovn/lex.h | 1 +
> ovn/controller/lflow.c | 16 +++--
> ovn/controller/lflow.h | 1 +
> ovn/controller/ofctrl.c | 6 +-
> ovn/controller/ofctrl.h | 3 +-
> ovn/controller/ovn-controller.c | 31 +++++++---
> ovn/lib/actions.c | 3 +-
> ovn/lib/expr.c | 127 ++++++++++++++++++++++++++++--
> ----------
> ovn/lib/lex.c | 20 +++++++
> ovn/northd/ovn-northd.c | 47 +++++++++++++++
> ovn/ovn-nb.ovsschema | 15 ++++-
> ovn/ovn-nb.xml | 30 ++++++++++
> ovn/ovn-sb.ovsschema | 10 +++-
> ovn/ovn-sb.xml | 20 +++++++
> ovn/utilities/ovn-trace.c | 27 +++++++--
> tests/ovn.at | 34 +++++++++++
> tests/test-ovn.c | 43 ++++++++++----
> 19 files changed, 381 insertions(+), 79 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 0cfcac5..ba9f0d8 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -15,6 +15,8 @@ Post-v2.9.0
> * OFPT_ROLE_STATUS is now available in OpenFlow 1.3.
> - Linux kernel 4.14
> * Add support for compiling OVS with the latest Linux 4.14 kernel
> + - OVN:
> + * Port_Group is supported in ACL match conditions.
>
> v2.9.0 - 19 Feb 2018
> --------------------
> diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> index 711713e..3995e62 100644
> --- a/include/ovn/expr.h
> +++ b/include/ovn/expr.h
> @@ -382,9 +382,11 @@ expr_from_node(const struct ovs_list *node)
> void expr_format(const struct expr *, struct ds *);
> void expr_print(const struct expr *);
> struct expr *expr_parse(struct lexer *, const struct shash *symtab,
> - const struct shash *addr_sets);
> + const struct shash *addr_sets,
> + const struct shash *port_groups);
> struct expr *expr_parse_string(const char *, const struct shash *symtab,
> const struct shash *addr_sets,
> + const struct shash *port_groups,
> char **errorp);
>
> struct expr *expr_clone(struct expr *);
> @@ -404,6 +406,7 @@ bool expr_is_normalized(const struct expr *);
>
> char *expr_parse_microflow(const char *, const struct shash *symtab,
> const struct shash *addr_sets,
> + const struct shash *port_groups,
> bool (*lookup_port)(const void *aux,
> const char *port_name,
> unsigned int *portp),
> @@ -486,19 +489,22 @@ void expr_constant_set_format(const struct
> expr_constant_set *, struct ds *);
> void expr_constant_set_destroy(struct expr_constant_set *cs);
>
>
> -/* Address sets.
> +/* Constant sets.
> *
> - * Instead of referring to a set of value as:
> + * For example, instead of referring to a set of IP addresses as:
> * {addr1, addr2, ..., addrN}
> * You can register a set of values and refer to them as:
> * $name
> - * The address set entries should all have integer/masked-integer values.
> - * The values that don't qualify are ignored.
> + *
> + * If convert_to_integer is true, the set must contain
> + * integer/masked-integer values. The values that don't qualify
> + * are ignored.
> */
>
> -void expr_addr_sets_add(struct shash *addr_sets, const char *name,
> - const char * const *values, size_t n_values);
> -void expr_addr_sets_remove(struct shash *addr_sets, const char *name);
> -void expr_addr_sets_destroy(struct shash *addr_sets);
> +void expr_const_sets_add(struct shash *const_sets, const char *name,
> + const char * const *values, size_t n_values,
> + bool convert_to_integer);
> +void expr_const_sets_remove(struct shash *const_sets, const char *name);
> +void expr_const_sets_destroy(struct shash *const_sets);
>
> #endif /* ovn/expr.h */
> diff --git a/include/ovn/lex.h b/include/ovn/lex.h
> index afa4a23..8d55857 100644
> --- a/include/ovn/lex.h
> +++ b/include/ovn/lex.h
> @@ -37,6 +37,7 @@ enum lex_type {
> LEX_T_INTEGER, /* 12345 or 1.2.3.4 or ::1 or
> 01:02:03:04:05 */
> LEX_T_MASKED_INTEGER, /* 12345/10 or 1.2.0.0/16 or ::2/127
> or... */
> LEX_T_MACRO, /* $NAME */
> + LEX_T_PORT_GROUP, /* @NAME */
> LEX_T_ERROR, /* invalid input */
>
> /* Bare tokens. */
> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> index df125b1..cc2a020 100644
> --- a/ovn/controller/lflow.c
> +++ b/ovn/controller/lflow.c
> @@ -70,6 +70,7 @@ static void consider_logical_flow(struct controller_ctx
> *ctx,
> struct hmap *nd_ra_opts,
> uint32_t *conj_id_ofs,
> const struct shash *addr_sets,
> + const struct shash *port_groups,
> struct hmap *flow_table,
> struct sset *active_tunnels,
> struct sset *local_lport_ids);
> @@ -149,6 +150,7 @@ add_logical_flows(struct controller_ctx *ctx,
> struct ovn_extend_table *meter_table,
> const struct sbrec_chassis *chassis,
> const struct shash *addr_sets,
> + const struct shash *port_groups,
> struct hmap *flow_table,
> struct sset *active_tunnels,
> struct sset *local_lport_ids)
> @@ -179,8 +181,8 @@ add_logical_flows(struct controller_ctx *ctx,
> lflow, local_datapaths,
> group_table, meter_table, chassis,
> &dhcp_opts, &dhcpv6_opts, &nd_ra_opts,
> - &conj_id_ofs, addr_sets, flow_table,
> - active_tunnels, local_lport_ids);
> + &conj_id_ofs, addr_sets, port_groups,
> + flow_table, active_tunnels,
> local_lport_ids);
> }
>
> dhcp_opts_destroy(&dhcp_opts);
> @@ -201,6 +203,7 @@ consider_logical_flow(struct controller_ctx *ctx,
> struct hmap *nd_ra_opts,
> uint32_t *conj_id_ofs,
> const struct shash *addr_sets,
> + const struct shash *port_groups,
> struct hmap *flow_table,
> struct sset *active_tunnels,
> struct sset *local_lport_ids)
> @@ -258,7 +261,8 @@ consider_logical_flow(struct controller_ctx *ctx,
> struct hmap matches;
> struct expr *expr;
>
> - expr = expr_parse_string(lflow->match, &symtab, addr_sets, &error);
> + expr = expr_parse_string(lflow->match, &symtab, addr_sets,
> port_groups,
> + &error);
> if (!error) {
> if (prereqs) {
> expr = expr_combine(EXPR_T_AND, expr, prereqs);
> @@ -450,13 +454,15 @@ lflow_run(struct controller_ctx *ctx,
> struct ovn_extend_table *group_table,
> struct ovn_extend_table *meter_table,
> const struct shash *addr_sets,
> + const struct shash *port_groups,
> struct hmap *flow_table,
> struct sset *active_tunnels,
> struct sset *local_lport_ids)
> {
> add_logical_flows(ctx, chassis_index, local_datapaths,
> - group_table, meter_table, chassis, addr_sets,
> flow_table,
> - active_tunnels, local_lport_ids);
> + group_table, meter_table, chassis, addr_sets,
> + port_groups, flow_table, active_tunnels,
> + local_lport_ids);
> add_neighbor_flows(ctx, flow_table);
> }
>
> diff --git a/ovn/controller/lflow.h b/ovn/controller/lflow.h
> index 22bf534..dcf2fe7 100644
> --- a/ovn/controller/lflow.h
> +++ b/ovn/controller/lflow.h
> @@ -69,6 +69,7 @@ void lflow_run(struct controller_ctx *,
> struct ovn_extend_table *group_table,
> struct ovn_extend_table *meter_table,
> const struct shash *addr_sets,
> + const struct shash *port_groups,
> struct hmap *flow_table,
> struct sset *active_tunnels,
> struct sset *local_lport_ids);
> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> index 83340bb..349de3a 100644
> --- a/ovn/controller/ofctrl.c
> +++ b/ovn/controller/ofctrl.c
> @@ -1147,7 +1147,8 @@ ofctrl_lookup_port(const void *br_int_, const char
> *port_name,
> * must free(). */
> char *
> ofctrl_inject_pkt(const struct ovsrec_bridge *br_int, const char *flow_s,
> - const struct shash *addr_sets)
> + const struct shash *addr_sets,
> + const struct shash *port_groups)
> {
> int version = rconn_get_version(swconn);
> if (version < 0) {
> @@ -1156,7 +1157,8 @@ ofctrl_inject_pkt(const struct ovsrec_bridge
> *br_int, const char *flow_s,
>
> struct flow uflow;
> char *error = expr_parse_microflow(flow_s, &symtab, addr_sets,
> - ofctrl_lookup_port, br_int,
> &uflow);
> + port_groups, ofctrl_lookup_port,
> + br_int, &uflow);
> if (error) {
> return error;
> }
> diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
> index d53bc68..a8b3afc 100644
> --- a/ovn/controller/ofctrl.h
> +++ b/ovn/controller/ofctrl.h
> @@ -47,7 +47,8 @@ struct ovn_flow *ofctrl_dup_flow(struct ovn_flow
> *source);
> void ofctrl_ct_flush_zone(uint16_t zone_id);
>
> char *ofctrl_inject_pkt(const struct ovsrec_bridge *br_int,
> - const char *flow_s, const struct shash
> *addr_sets);
> + const char *flow_s, const struct shash *addr_sets,
> + const struct shash *port_groups);
>
> /* Flow table interfaces to the rest of ovn-controller. */
> void ofctrl_add_flow(struct hmap *desired_flows, uint8_t table_id,
> diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-
> controller.c
> index fdefd3d..27a092d 100644
> --- a/ovn/controller/ovn-controller.c
> +++ b/ovn/controller/ovn-controller.c
> @@ -293,9 +293,22 @@ addr_sets_init(struct controller_ctx *ctx, struct
> shash *addr_sets)
> {
> const struct sbrec_address_set *as;
> SBREC_ADDRESS_SET_FOR_EACH (as, ctx->ovnsb_idl) {
> - expr_addr_sets_add(addr_sets, as->name,
> - (const char *const *) as->addresses,
> - as->n_addresses);
> + expr_const_sets_add(addr_sets, as->name,
> + (const char *const *) as->addresses,
> + as->n_addresses, true);
> + }
> +}
> +
> +/* Iterate port groups in the southbound database. Create and update the
> + * corresponding symtab entries as necessary. */
> +static void
> +port_groups_init(struct controller_ctx *ctx, struct shash *port_groups)
> +{
> + const struct sbrec_port_group *pg;
> + SBREC_PORT_GROUP_FOR_EACH (pg, ctx->ovnsb_idl) {
> + expr_const_sets_add(port_groups, pg->name,
> + (const char *const *) pg->ports,
> + pg->n_ports, false);
> }
> }
>
> @@ -703,6 +716,8 @@ main(int argc, char *argv[])
> if (br_int && chassis) {
> struct shash addr_sets = SHASH_INITIALIZER(&addr_sets);
> addr_sets_init(&ctx, &addr_sets);
> + struct shash port_groups = SHASH_INITIALIZER(&port_groups);
> + port_groups_init(&ctx, &port_groups);
>
> patch_run(&ctx, br_int, chassis);
>
> @@ -723,8 +738,8 @@ main(int argc, char *argv[])
> struct hmap flow_table =
> HMAP_INITIALIZER(&flow_table);
> lflow_run(&ctx, chassis,
> &chassis_index, &local_datapaths,
> &group_table,
> - &meter_table, &addr_sets, &flow_table,
> - &active_tunnels, &local_lport_ids);
> + &meter_table, &addr_sets, &port_groups,
> + &flow_table, &active_tunnels,
> &local_lport_ids);
>
> if (chassis_id) {
> bfd_run(&ctx, br_int, chassis, &local_datapaths,
> @@ -753,7 +768,7 @@ main(int argc, char *argv[])
>
> if (pending_pkt.conn) {
> char *error = ofctrl_inject_pkt(br_int,
> pending_pkt.flow_s,
> - &addr_sets);
> + &port_groups, &addr_sets);
> if (error) {
> unixctl_command_reply_error(pending_pkt.conn, error);
> free(error);
> @@ -767,8 +782,10 @@ main(int argc, char *argv[])
> update_sb_monitors(ctx.ovnsb_idl, chassis,
> &local_lports, &local_datapaths);
>
> - expr_addr_sets_destroy(&addr_sets);
> + expr_const_sets_destroy(&addr_sets);
> shash_destroy(&addr_sets);
> + expr_const_sets_destroy(&port_groups);
> + shash_destroy(&port_groups);
> }
>
> /* If we haven't handled the pending packet insertion
> diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> index fc5ace1..93f6081 100644
> --- a/ovn/lib/actions.c
> +++ b/ovn/lib/actions.c
> @@ -234,7 +234,8 @@ add_prerequisite(struct action_context *ctx, const
> char *prerequisite)
> struct expr *expr;
> char *error;
>
> - expr = expr_parse_string(prerequisite, ctx->pp->symtab, NULL,
> &error);
> + expr = expr_parse_string(prerequisite, ctx->pp->symtab, NULL, NULL,
> + &error);
> ovs_assert(!error);
> ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, expr);
> }
> diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
> index b0aa672..5840ef8 100644
> --- a/ovn/lib/expr.c
> +++ b/ovn/lib/expr.c
> @@ -464,6 +464,7 @@ struct expr_context {
> struct lexer *lexer; /* Lexer for pulling more tokens. */
> const struct shash *symtab; /* Symbol table. */
> const struct shash *addr_sets; /* Address set table. */
> + const struct shash *port_groups; /* Port group table. */
> bool not; /* True inside odd number of NOT
> operators. */
> };
>
> @@ -752,6 +753,36 @@ parse_addr_sets(struct expr_context *ctx, struct
> expr_constant_set *cs,
> }
>
> static bool
> +parse_port_group(struct expr_context *ctx, struct expr_constant_set *cs,
> + size_t *allocated_values)
> +{
> + struct expr_constant_set *port_group
> + = (ctx->port_groups
> + ? shash_find_data(ctx->port_groups, ctx->lexer->token.s)
> + : NULL);
> + if (!port_group) {
> + lexer_syntax_error(ctx->lexer, "expecting port group name");
> + return false;
> + }
> +
> + if (!assign_constant_set_type(ctx, cs, EXPR_C_STRING)) {
> + return false;
> + }
> +
> + size_t n_values = cs->n_values + port_group->n_values;
> + if (n_values >= *allocated_values) {
> + cs->values = xrealloc(cs->values, n_values * sizeof *cs->values);
> + *allocated_values = n_values;
> + }
> + for (size_t i = 0; i < port_group->n_values; i++) {
> + cs->values[cs->n_values++].string =
> + xstrdup(port_group->values[i].string);
> + }
> +
> + return true;
> +}
> +
> +static bool
> parse_constant(struct expr_context *ctx, struct expr_constant_set *cs,
> size_t *allocated_values)
> {
> @@ -788,6 +819,12 @@ parse_constant(struct expr_context *ctx, struct
> expr_constant_set *cs,
> }
> lexer_get(ctx->lexer);
> return true;
> + } else if (ctx->lexer->token.type == LEX_T_PORT_GROUP) {
> + if (!parse_port_group(ctx, cs, allocated_values)) {
> + return false;
> + }
> + lexer_get(ctx->lexer);
> + return true;
> } else {
> lexer_syntax_error(ctx->lexer, "expecting constant");
> return false;
> @@ -939,65 +976,74 @@ expr_constant_set_destroy(struct expr_constant_set
> *cs)
> }
> }
>
> -/* Adds an address set named 'name' to 'addr_sets', replacing any existing
> - * address set entry with the given name. */
> +/* Adds an constant set named 'name' to 'const_sets', replacing any
> existing
> + * constant set entry with the given name. */
> void
> -expr_addr_sets_add(struct shash *addr_sets, const char *name,
> - const char *const *values, size_t n_values)
> +expr_const_sets_add(struct shash *const_sets, const char *name,
> + const char *const *values, size_t n_values,
> + bool convert_to_integer)
> {
> /* Replace any existing entry for this name. */
> - expr_addr_sets_remove(addr_sets, name);
> + expr_const_sets_remove(const_sets, name);
>
> struct expr_constant_set *cs = xzalloc(sizeof *cs);
> - cs->type = EXPR_C_INTEGER;
> cs->in_curlies = true;
> cs->n_values = 0;
> cs->values = xmalloc(n_values * sizeof *cs->values);
> - for (size_t i = 0; i < n_values; i++) {
> - /* Use the lexer to convert each address set into the proper
> - * integer format. */
> - struct lexer lex;
> - lexer_init(&lex, values[i]);
> - lexer_get(&lex);
> - if (lex.token.type != LEX_T_INTEGER
> - && lex.token.type != LEX_T_MASKED_INTEGER) {
> - VLOG_WARN("Invalid address set entry: '%s', token type: %d",
> - values[i], lex.token.type);
> - } else {
> - union expr_constant *c = &cs->values[cs->n_values++];
> - c->value = lex.token.value;
> - c->format = lex.token.format;
> - c->masked = lex.token.type == LEX_T_MASKED_INTEGER;
> - if (c->masked) {
> - c->mask = lex.token.mask;
> + if (convert_to_integer) {
> + cs->type = EXPR_C_INTEGER;
> + for (size_t i = 0; i < n_values; i++) {
> + /* Use the lexer to convert each constant set into the proper
> + * integer format. */
> + struct lexer lex;
> + lexer_init(&lex, values[i]);
> + lexer_get(&lex);
> + if (lex.token.type != LEX_T_INTEGER
> + && lex.token.type != LEX_T_MASKED_INTEGER) {
> + VLOG_WARN("Invalid constant set entry: '%s', token type:
> %d",
> + values[i], lex.token.type);
> + } else {
> + union expr_constant *c = &cs->values[cs->n_values++];
> + c->value = lex.token.value;
> + c->format = lex.token.format;
> + c->masked = lex.token.type == LEX_T_MASKED_INTEGER;
> + if (c->masked) {
> + c->mask = lex.token.mask;
> + }
> }
> + lexer_destroy(&lex);
> + }
> + } else {
> + cs->type = EXPR_C_STRING;
> + for (size_t i = 0; i < n_values; i++) {
> + union expr_constant *c = &cs->values[cs->n_values++];
> + c->string = xstrdup(values[i]);
> }
> - lexer_destroy(&lex);
> }
>
> - shash_add(addr_sets, name, cs);
> + shash_add(const_sets, name, cs);
> }
>
> void
> -expr_addr_sets_remove(struct shash *addr_sets, const char *name)
> +expr_const_sets_remove(struct shash *const_sets, const char *name)
> {
> - struct expr_constant_set *cs = shash_find_and_delete(addr_sets,
> name);
> + struct expr_constant_set *cs = shash_find_and_delete(const_sets,
> name);
> if (cs) {
> expr_constant_set_destroy(cs);
> free(cs);
> }
> }
>
> -/* Destroy all contents of 'addr_sets'. */
> +/* Destroy all contents of 'const_sets'. */
> void
> -expr_addr_sets_destroy(struct shash *addr_sets)
> +expr_const_sets_destroy(struct shash *const_sets)
> {
> struct shash_node *node, *next;
>
> - SHASH_FOR_EACH_SAFE (node, next, addr_sets) {
> + SHASH_FOR_EACH_SAFE (node, next, const_sets) {
> struct expr_constant_set *cs = node->data;
>
> - shash_delete(addr_sets, node);
> + shash_delete(const_sets, node);
> expr_constant_set_destroy(cs);
> free(cs);
> }
> @@ -1214,11 +1260,13 @@ expr_parse__(struct expr_context *ctx)
> * lexer->error is NULL. */
> struct expr *
> expr_parse(struct lexer *lexer, const struct shash *symtab,
> - const struct shash *addr_sets)
> + const struct shash *addr_sets,
> + const struct shash *port_groups)
> {
> struct expr_context ctx = { .lexer = lexer,
> .symtab = symtab,
> - .addr_sets = addr_sets };
> + .addr_sets = addr_sets,
> + .port_groups = port_groups };
> return lexer->error ? NULL : expr_parse__(&ctx);
> }
>
> @@ -1230,13 +1278,15 @@ expr_parse(struct lexer *lexer, const struct shash
> *symtab,
> * error (with free()). */
> struct expr *
> expr_parse_string(const char *s, const struct shash *symtab,
> - const struct shash *addr_sets, char **errorp)
> + const struct shash *addr_sets,
> + const struct shash *port_groups,
> + char **errorp)
> {
> struct lexer lexer;
>
> lexer_init(&lexer, s);
> lexer_get(&lexer);
> - struct expr *expr = expr_parse(&lexer, symtab, addr_sets);
> + struct expr *expr = expr_parse(&lexer, symtab, addr_sets,
> port_groups);
> lexer_force_end(&lexer);
> *errorp = lexer_steal_error(&lexer);
> if (*errorp) {
> @@ -1460,7 +1510,7 @@ expr_get_level(const struct expr *expr)
> static enum expr_level
> expr_parse_level(const char *s, const struct shash *symtab, char **errorp)
> {
> - struct expr *expr = expr_parse_string(s, symtab, NULL, errorp);
> + struct expr *expr = expr_parse_string(s, symtab, NULL, NULL, errorp);
> enum expr_level level = expr ? expr_get_level(expr) : EXPR_L_NOMINAL;
> expr_destroy(expr);
> return level;
> @@ -1618,7 +1668,7 @@ parse_and_annotate(const char *s, const struct shash
> *symtab,
> char *error;
> struct expr *expr;
>
> - expr = expr_parse_string(s, symtab, NULL, &error);
> + expr = expr_parse_string(s, symtab, NULL, NULL, &error);
> if (expr) {
> expr = expr_annotate__(expr, symtab, nesting, &error);
> }
> @@ -3277,6 +3327,7 @@ expr_parse_microflow__(struct lexer *lexer,
> char * OVS_WARN_UNUSED_RESULT
> expr_parse_microflow(const char *s, const struct shash *symtab,
> const struct shash *addr_sets,
> + const struct shash *port_groups,
> bool (*lookup_port)(const void *aux,
> const char *port_name,
> unsigned int *portp),
> @@ -3286,7 +3337,7 @@ expr_parse_microflow(const char *s, const struct
> shash *symtab,
> lexer_init(&lexer, s);
> lexer_get(&lexer);
>
> - struct expr *e = expr_parse(&lexer, symtab, addr_sets);
> + struct expr *e = expr_parse(&lexer, symtab, addr_sets, port_groups);
> lexer_force_end(&lexer);
>
> if (e) {
> diff --git a/ovn/lib/lex.c b/ovn/lib/lex.c
> index 2f49af0..0514950 100644
> --- a/ovn/lib/lex.c
> +++ b/ovn/lib/lex.c
> @@ -231,6 +231,10 @@ lex_token_format(const struct lex_token *token,
> struct ds *s)
> ds_put_format(s, "$%s", token->s);
> break;
>
> + case LEX_T_PORT_GROUP:
> + ds_put_format(s, "@%s", token->s);
> + break;
> +
> case LEX_T_LPAREN:
> ds_put_cstr(s, "(");
> break;
> @@ -573,6 +577,18 @@ lex_parse_addr_set(const char *p, struct lex_token
> *token)
> return lex_parse_id(p, LEX_T_MACRO, token);
> }
>
> +static const char *
> +lex_parse_port_group(const char *p, struct lex_token *token)
> +{
> + p++;
> + if (!lex_is_id1(*p)) {
> + lex_error(token, "`@' must be followed by a valid identifier.");
> + return p;
> + }
> +
> + return lex_parse_id(p, LEX_T_PORT_GROUP, token);
> +}
> +
> /* Initializes 'token' and parses the first token from the beginning of
> * null-terminated string 'p' into 'token'. Stores a pointer to the
> start of
> * the token (after skipping white space and comments, if any) into
> '*startp'.
> @@ -747,6 +763,10 @@ next:
> p = lex_parse_addr_set(p, token);
> break;
>
> + case '@':
> + p = lex_parse_port_group(p, token);
> + break;
> +
> case ':':
> if (p[1] != ':') {
> token->type = LEX_T_COLON;
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 3963810..d4addf6 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -6178,6 +6178,49 @@ sync_address_sets(struct northd_context *ctx)
> shash_destroy(&sb_address_sets);
> }
>
> +/* Each port group in Port_Group table in OVN_Northbound has a
> corresponding
> + * entry in Port_Group table in OVN_Southbound. In OVN_Northbound the
> entries
> + * contains lport uuids, while in OVN_Southbound we store the lport names.
> + */
> +static void
> +sync_port_groups(struct northd_context *ctx)
> +{
> + struct shash sb_port_groups = SHASH_INITIALIZER(&sb_port_groups);
> +
> + const struct sbrec_port_group *sb_port_group;
> + SBREC_PORT_GROUP_FOR_EACH (sb_port_group, ctx->ovnsb_idl) {
> + shash_add(&sb_port_groups, sb_port_group->name, sb_port_group);
> + }
> +
> + const struct nbrec_port_group *nb_port_group;
> + NBREC_PORT_GROUP_FOR_EACH (nb_port_group, ctx->ovnnb_idl) {
> + sb_port_group = shash_find_and_delete(&sb_port_groups,
> + nb_port_group->name);
> + if (!sb_port_group) {
> + sb_port_group = sbrec_port_group_insert(ctx->ovnsb_txn);
> + sbrec_port_group_set_name(sb_port_group,
> nb_port_group->name);
> + }
> +
> + const char **nb_port_names = xcalloc(nb_port_group->n_ports,
> + sizeof *nb_port_names);
> + int i;
> + for (i = 0; i < nb_port_group->n_ports; i++) {
> + nb_port_names[i] = nb_port_group->ports[i]->name;
> + }
> + sbrec_port_group_set_ports(sb_port_group,
> + nb_port_names,
> + nb_port_group->n_ports);
> + free(nb_port_names);
> + }
> +
> + struct shash_node *node, *next;
> + SHASH_FOR_EACH_SAFE (node, next, &sb_port_groups) {
> + sbrec_port_group_delete(node->data);
> + shash_delete(&sb_port_groups, node);
> + }
> + shash_destroy(&sb_port_groups);
> +}
> +
> /*
> * struct 'dns_info' is used to sync the DNS records between OVN
> Northbound db
> * and Southbound db.
> @@ -6294,6 +6337,7 @@ ovnnb_db_run(struct northd_context *ctx, struct
> chassis_index *chassis_index,
> build_lflows(ctx, &datapaths, &ports);
>
> sync_address_sets(ctx);
> + sync_port_groups(ctx);
> sync_dns_entries(ctx, &datapaths);
>
> struct ovn_datapath *dp, *next_dp;
> @@ -6891,6 +6935,9 @@ main(int argc, char *argv[])
> 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);
> + ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_port_group);
> + add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_group_col_name);
> + add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_group_col_ports);
>
> ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_dns);
> add_column_noalert(ovnsb_idl_loop.idl, &sbrec_dns_col_datapaths);
> diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
> index 32f9d5a..2d09282 100644
> --- a/ovn/ovn-nb.ovsschema
> +++ b/ovn/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Northbound",
> "version": "5.10.0",
> - "cksum": "626737541 17810",
> + "cksum": "222987318 18430",
> "tables": {
> "NB_Global": {
> "columns": {
> @@ -114,6 +114,19 @@
> "min": 0, "max": "unlimited"}}},
> "indexes": [["name"]],
> "isRoot": true},
> + "Port_Group": {
> + "columns": {
> + "name": {"type": "string"},
> + "ports": {"type": {"key": {"type": "uuid",
> + "refTable":
> "Logical_Switch_Port",
> + "refType": "weak"},
> + "min": 0,
> + "max": "unlimited"}},
> + "external_ids": {
> + "type": {"key": "string", "value": "string",
> + "min": 0, "max": "unlimited"}}},
> + "indexes": [["name"]],
> + "isRoot": true},
> "Load_Balancer": {
> "columns": {
> "name": {"type": "string"},
> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> index 6bb93de..25ffeaa 100644
> --- a/ovn/ovn-nb.xml
> +++ b/ovn/ovn-nb.xml
> @@ -922,6 +922,36 @@
> </group>
> </table>
>
> + <table name="Port_Group" title="Port Groups">
> + <p>
> + Each row in this table represents a named group of logical switch
> ports.
> + </p>
> +
> + <p>
> + Port groups may 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">
> + A name for the port group. Names are ASCII and must match
> + <code>[a-zA-Z_.][a-zA-Z_.0-9]*</code>.
> + </column>
> +
> + <column name="ports">
> + The logical switch ports belonging to the group in uuids.
> + </column>
> +
> + <group title="Common Columns">
> + <column name="external_ids">
> + See <em>External IDs</em> at the beginning of this document.
> + </column>
> + </group>
> + </table>
> +
> <table name="Load_Balancer" title="load balancer">
> <p>
> Each row represents one load balancer.
> diff --git a/ovn/ovn-sb.ovsschema b/ovn/ovn-sb.ovsschema
> index 2abcc6b..9e271d4 100644
> --- a/ovn/ovn-sb.ovsschema
> +++ b/ovn/ovn-sb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Southbound",
> "version": "1.15.0",
> - "cksum": "70426956 13327",
> + "cksum": "1839738004 13639",
> "tables": {
> "SB_Global": {
> "columns": {
> @@ -55,6 +55,14 @@
> "max": "unlimited"}}},
> "indexes": [["name"]],
> "isRoot": true},
> + "Port_Group": {
> + "columns": {
> + "name": {"type": "string"},
> + "ports": {"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 6a8b818..bf6a680 100644
> --- a/ovn/ovn-sb.xml
> +++ b/ovn/ovn-sb.xml
> @@ -377,6 +377,18 @@
> <column name="addresses"/>
> </table>
>
> + <table name="Port_Group" title="Port Groups">
> + <p>
> + This table contains names for the logical switch ports in the
> + <ref db="OVN_Northbound"/> database that belongs to the same group
> + that is defined in <ref table="Port_Group" db="OVN_Northbound"/>
> + in the <ref db="OVN_Northbound"/> database.
> + </p>
> +
> + <column name="name"/>
> + <column name="ports"/>
> + </table>
> +
> <table name="Logical_Flow" title="Logical Network Flows">
> <p>
> Each row in this table represents one logical flow.
> @@ -770,6 +782,14 @@
> <code>$set1</code>.
> </p>
>
> + <p>
> + You may refer to a group of logical switch ports stored in the
> + <ref table="Port_Group"/> table by its <ref column="name"
> + table="Port_Group"/>. An <ref table="Port_Group"/> with a name
> + of <code>port_group1</code> can be referred to as
> + <code>@port_group1</code>.
> + </p>
> +
> <p><em>Miscellaneous</em></p>
>
> <p>
> diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
> index a392573..21136ef 100644
> --- a/ovn/utilities/ovn-trace.c
> +++ b/ovn/utilities/ovn-trace.c
> @@ -418,6 +418,9 @@ static struct shash symtab;
> /* Address sets. */
> static struct shash address_sets;
>
> +/* Port groups. */
> +static struct shash port_groups;
> +
> /* DHCP options. */
> static struct hmap dhcp_opts; /* Contains "struct gen_opts_map"s. */
> static struct hmap dhcpv6_opts; /* Contains "struct gen_opts_map"s. */
> @@ -697,9 +700,22 @@ read_address_sets(void)
>
> const struct sbrec_address_set *sbas;
> SBREC_ADDRESS_SET_FOR_EACH (sbas, ovnsb_idl) {
> - expr_addr_sets_add(&address_sets, sbas->name,
> + expr_const_sets_add(&address_sets, sbas->name,
> (const char *const *) sbas->addresses,
> - sbas->n_addresses);
> + sbas->n_addresses, true);
> + }
> +}
> +
> +static void
> +read_port_groups(void)
> +{
> + shash_init(&port_groups);
> +
> + const struct sbrec_port_group *sbpg;
> + SBREC_PORT_GROUP_FOR_EACH (sbpg, ovnsb_idl) {
> + expr_const_sets_add(&port_groups, sbpg->name,
> + (const char *const *) sbpg->ports,
> + sbpg->n_ports, false);
> }
> }
>
> @@ -796,7 +812,8 @@ read_flows(void)
>
> char *error;
> struct expr *match;
> - match = expr_parse_string(sblf->match, &symtab, &address_sets,
> &error);
> + match = expr_parse_string(sblf->match, &symtab, &address_sets,
> + &port_groups, &error);
> if (error) {
> VLOG_WARN("%s: parsing expression failed (%s)",
> sblf->match, error);
> @@ -937,6 +954,7 @@ read_db(void)
> read_ports();
> read_mcgroups();
> read_address_sets();
> + read_port_groups();
> read_gen_opts();
> read_flows();
> read_mac_bindings();
> @@ -2039,7 +2057,8 @@ trace(const char *dp_s, const char *flow_s)
>
> struct flow uflow;
> char *error = expr_parse_microflow(flow_s, &symtab, &address_sets,
> - ovntrace_lookup_port, dp, &uflow);
> + &port_groups, ovntrace_lookup_port,
> + dp, &uflow);
> if (error) {
> char *s = xasprintf("error parsing flow: %s\n", error);
> free(error);
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 5f985f3..458808c 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -304,6 +304,7 @@ inport = "eth0" => Syntax error at `=' expecting
> relational operator.
> 123 == 123 => Syntax error at `123' expecting field name.
>
> $name => Syntax error at `$name' expecting address set name.
> +@name => Syntax error at `@name' expecting port group name.
>
> 123 == xyzzy => Syntax error at `xyzzy' expecting field name.
> xyzzy == 1 => Syntax error at `xyzzy' expecting field name.
> @@ -657,6 +658,24 @@ ip,nw_src=8.0.0.0/8.0.0.0
> ])
> AT_CLEANUP
>
> +AT_SETUP([ovn -- converting expressions to flows -- port groups])
> +AT_KEYWORDS([expression])
> +expr_to_flow () {
> + echo "$1" | ovstest test-ovn expr-to-flows | sort
> +}
> +AT_CHECK([expr_to_flow 'outport == @pg1'], [0], [dnl
> +reg15=0x11
> +reg15=0x12
> +reg15=0x13
> +])
> +AT_CHECK([expr_to_flow 'outport == {@pg_empty}'], [0], [dnl
> +(no flows)
> +])
> +AT_CHECK([expr_to_flow 'outport == {"lsp1", @pg_empty}'], [0], [dnl
> +reg15=0x11
> +])
> +AT_CLEANUP
> +
> AT_SETUP([ovn -- action parsing])
> dnl Unindented text is input (a set of OVN logical actions).
> dnl Indented text is expected output.
> @@ -1204,6 +1223,13 @@ ovn-nbctl acl-add lsw0 to-lport 1000 'eth.type ==
> 0x1236 && outport == "lp33"' d
> 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
>
> +get_lsp_uuid () {
> + ovn-nbctl lsp-list lsw0 | grep $1 | awk '{ print $1 }'
> +}
> +
> +ovn-nbctl create Port_Group name=pg1 ports=`get_lsp_uuid
> lp22`,`get_lsp_uuid lp33`
> +ovn-nbctl acl-add lsw0 to-lport 1000 'eth.type == 0x1238 && outport ==
> @pg1' drop
> +
> # Pre-populate the hypervisors' ARP tables so that we don't lose any
> # packets for ARP resolution (native tunneling doesn't queue packets
> # for ARP resolution).
> @@ -1344,10 +1370,18 @@ for is in 1 2 3; do
> else
> acl4=$d
> fi
> + if test $d = $s || test $d = 22 || test $d = 33; then
> + # dest of 22 and 33 should be dropped
> + # due to the 5th ACL that uses port_group(pg1).
> + acl5=
> + else
> + acl5=$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 f000000000$s 1238 $acl5 #7,
> acl5
>
> test_packet $s f000000000$d f00000000055 810000091234
> #4
> test_packet $s f000000000$d 0100000000$s $s$d
> #5
> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> index 7452753..d4a5d59 100644
> --- a/tests/test-ovn.c
> +++ b/tests/test-ovn.c
> @@ -211,10 +211,24 @@ create_addr_sets(struct shash *addr_sets)
> };
> static const char *const addrs4[] = { NULL };
>
> - expr_addr_sets_add(addr_sets, "set1", addrs1, 3);
> - expr_addr_sets_add(addr_sets, "set2", addrs2, 3);
> - expr_addr_sets_add(addr_sets, "set3", addrs3, 3);
> - expr_addr_sets_add(addr_sets, "set4", addrs4, 0);
> + expr_const_sets_add(addr_sets, "set1", addrs1, 3, true);
> + expr_const_sets_add(addr_sets, "set2", addrs2, 3, true);
> + expr_const_sets_add(addr_sets, "set3", addrs3, 3, true);
> + expr_const_sets_add(addr_sets, "set4", addrs4, 0, true);
> +}
> +
> +static void
> +create_port_groups(struct shash *port_groups)
> +{
> + shash_init(port_groups);
> +
> + static const char *const pg1[] = {
> + "lsp1", "lsp2", "lsp3",
> + };
> + static const char *const pg2[] = { NULL };
> +
> + expr_const_sets_add(port_groups, "pg1", pg1, 3, false);
> + expr_const_sets_add(port_groups, "pg_empty", pg2, 0, false);
> }
>
> static bool
> @@ -245,23 +259,29 @@ test_parse_expr__(int steps)
> {
> struct shash symtab;
> struct shash addr_sets;
> + struct shash port_groups;
> struct simap ports;
> struct ds input;
>
> create_symtab(&symtab);
> create_addr_sets(&addr_sets);
> + create_port_groups(&port_groups);
>
> simap_init(&ports);
> simap_put(&ports, "eth0", 5);
> simap_put(&ports, "eth1", 6);
> simap_put(&ports, "LOCAL", ofp_to_u16(OFPP_LOCAL));
> + simap_put(&ports, "lsp1", 0x11);
> + simap_put(&ports, "lsp2", 0x12);
> + simap_put(&ports, "lsp3", 0x13);
>
> ds_init(&input);
> while (!ds_get_test_line(&input, stdin)) {
> struct expr *expr;
> char *error;
>
> - expr = expr_parse_string(ds_cstr(&input), &symtab, &addr_sets,
> &error);
> + expr = expr_parse_string(ds_cstr(&input), &symtab, &addr_sets,
> + &port_groups, &error);
> if (!error && steps > 0) {
> expr = expr_annotate(expr, &symtab, &error);
> }
> @@ -298,8 +318,10 @@ test_parse_expr__(int steps)
> simap_destroy(&ports);
> expr_symtab_destroy(&symtab);
> shash_destroy(&symtab);
> - expr_addr_sets_destroy(&addr_sets);
> + expr_const_sets_destroy(&addr_sets);
> shash_destroy(&addr_sets);
> + expr_const_sets_destroy(&port_groups);
> + shash_destroy(&port_groups);
> }
>
> static void
> @@ -373,7 +395,7 @@ test_evaluate_expr(struct ovs_cmdl_context *ctx)
> ovn_init_symtab(&symtab);
>
> struct flow uflow;
> - char *error = expr_parse_microflow(ctx->argv[1], &symtab, NULL,
> + char *error = expr_parse_microflow(ctx->argv[1], &symtab, NULL, NULL,
> lookup_atoi_cb, NULL, &uflow);
> if (error) {
> ovs_fatal(0, "%s", error);
> @@ -383,7 +405,7 @@ test_evaluate_expr(struct ovs_cmdl_context *ctx)
> while (!ds_get_test_line(&input, stdin)) {
> struct expr *expr;
>
> - expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, &error);
> + expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, NULL,
> &error);
> if (!error) {
> expr = expr_annotate(expr, &symtab, &error);
> }
> @@ -857,7 +879,8 @@ test_tree_shape_exhaustively(struct expr *expr,
> struct shash *symtab,
> expr_format(expr, &s);
>
> char *error;
> - modified = expr_parse_string(ds_cstr(&s), symtab, NULL,
> &error);
> + modified = expr_parse_string(ds_cstr(&s), symtab, NULL,
> + NULL, &error);
> if (error) {
> fprintf(stderr, "%s fails to parse (%s)\n",
> ds_cstr(&s), error);
> @@ -1163,7 +1186,7 @@ test_expr_to_packets(struct ovs_cmdl_context *ctx
> OVS_UNUSED)
> while (!ds_get_test_line(&input, stdin)) {
> struct flow uflow;
> char *error = expr_parse_microflow(ds_cstr(&input), &symtab,
> NULL,
> - lookup_atoi_cb, NULL, &uflow);
> + NULL, lookup_atoi_cb, NULL,
> &uflow);
> if (error) {
> puts(error);
> free(error);
> --
> 2.1.0
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
