On 28 February 2018 at 19:37, 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]. > > [1] https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/077118.html > [2] https://mail.openvswitch.org/pipermail/ovs-discuss/2018- > February/046260.html > > Reported-by: Daniel Alvarez Sanchez <dalva...@redhat.com> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018- > February/046166.html > Signed-off-by: Han Zhou <hzh...@ebay.com> >
Wouldn't it be more complete and useful if we add the acl to a port group too? And then internally, you decide which switches you want to add the ACL to. For e.g: ovn-nbctl --port-group add-acl port_group1 to-lport 1000 "outport == @port_group1 && ip4.src == {IP1, IP2, ...}" allow-related This way, the client does not have to keep track of all the logical switches it needs to apply an ACL to. Thoughts? > --- > 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 4230189..da2c3ec 100644 > --- a/NEWS > +++ b/NEWS > @@ -14,6 +14,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 8d6d1b6..e3ee6b1 100644 > --- a/ovn/controller/ofctrl.c > +++ b/ovn/controller/ofctrl.c > @@ -1145,7 +1145,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) { > @@ -1154,7 +1155,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 7592bda..492e4fa 100644 > --- a/ovn/controller/ovn-controller.c > +++ b/ovn/controller/ovn-controller.c > @@ -288,9 +288,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); > } > } > > @@ -696,6 +709,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); > > @@ -713,8 +728,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, > @@ -740,7 +755,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); > @@ -754,8 +769,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 fde3bff..8845e77 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..1a33269 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 groups 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..9463640 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 d83681c..2924d8f 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -6135,6 +6135,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. > @@ -6251,6 +6294,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; > @@ -6848,6 +6892,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 b7a5b6b..83727c5 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 f000b16..2eac943 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 db39c49..5a9e73d 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(); > @@ -2009,7 +2027,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 8ee3bf0..c8ab7b7 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. > @@ -1194,6 +1213,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). > @@ -1334,10 +1360,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