Permalink
Browse files

ovn: Support port groups in ACLs

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 <dalvarez@redhat.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-February/046166.html
Tested-by: Mark Michelson <mmichels@redhat.com>
Reviewed-by: Mark Michelson <mmichels@redhat.com>
Reviewed-by: Daniel Alvarez <dalvarez@redhat.com>
Signed-off-by: Han Zhou <hzhou8@ebay.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
  • Loading branch information...
hzhou8 authored and blp committed Apr 5, 2018
1 parent 6040586 commit 3d2848bafa93a2b483a4504c5de801454671dccf
1 NEWS
@@ -20,6 +20,7 @@ Post-v2.9.0
* implemented icmp4/icmp6/tcp_reset actions in order to drop the packet
and reply with a RST for TCP or ICMPv4/ICMPv6 unreachable message for
other IPv4/IPv6-based protocols whenever a reject ACL rule is hit.
* ACL match conditions can now match on Port_Groups.

v2.9.0 - 19 Feb 2018
--------------------
@@ -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 */
@@ -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. */
@@ -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);
}

@@ -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);
@@ -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;
}
@@ -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,
@@ -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
@@ -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);
}
Oops, something went wrong.

0 comments on commit 3d2848b

Please sign in to comment.