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>

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
 --------------------

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,
+void expr_const_sets_add(struct shash *const_sets, const char *name,
-                        const char * const *values, size_t n_values);
+                         const char * const *values, size_t n_values,
-void expr_addr_sets_remove(struct shash *addr_sets, const char *name);
+                         bool convert_to_integer);
-void expr_addr_sets_destroy(struct shash *addr_sets);
+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 */

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. */

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,
+                              &conj_id_ofs, addr_sets, port_groups,
-                              active_tunnels, local_lport_ids);
+                              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,
+                      group_table, meter_table, chassis, addr_sets,
-                      active_tunnels, local_lport_ids);
+                      port_groups, flow_table, active_tunnels,
+                      local_lport_ids);
     add_neighbor_flows(ctx, flow_table);
 }
 

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);

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;
     }

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,

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,
+        expr_const_sets_add(addr_sets, as->name,
-                           (const char *const *) as->addresses,
+                            (const char *const *) as->addresses,
-                           as->n_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,
+                              &meter_table, &addr_sets, &port_groups,
-                              &active_tunnels, &local_lport_ids);
+                              &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

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);
 }

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. */
 };
 
@@ -751,6 +752,36 @@ parse_addr_sets(struct expr_context *ctx, struct expr_constant_set *cs,
     return true;
 }
 
+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
+/* Adds an constant set named 'name' to 'const_sets', replacing any existing
- * address set entry with the given name. */
+ * constant set entry with the given name. */
 void
-expr_addr_sets_add(struct shash *addr_sets, const char *name,
+expr_const_sets_add(struct shash *const_sets, const char *name,
-                   const char *const *values, size_t n_values)
+                    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++) {
+    if (convert_to_integer) {
-        /* Use the lexer to convert each address set into the proper
+        cs->type = EXPR_C_INTEGER;
-         * integer format. */
+        for (size_t i = 0; i < n_values; i++) {
-        struct lexer lex;
+            /* Use the lexer to convert each constant set into the proper
-        lexer_init(&lex, values[i]);
+             * integer format. */
-        lexer_get(&lex);
+            struct lexer lex;
-        if (lex.token.type != LEX_T_INTEGER
+            lexer_init(&lex, values[i]);
-            && lex.token.type != LEX_T_MASKED_INTEGER) {
+            lexer_get(&lex);
-            VLOG_WARN("Invalid address set entry: '%s', token type: %d",
+            if (lex.token.type != LEX_T_INTEGER
-                      values[i], lex.token.type);
+                && lex.token.type != LEX_T_MASKED_INTEGER) {
-        } else {
+                VLOG_WARN("Invalid constant set entry: '%s', token type: %d",
-            union expr_constant *c = &cs->values[cs->n_values++];
+                          values[i], lex.token.type);
-            c->value = lex.token.value;
+            } else {
-            c->format = lex.token.format;
+                union expr_constant *c = &cs->values[cs->n_values++];
-            c->masked = lex.token.type == LEX_T_MASKED_INTEGER;
+                c->value = lex.token.value;
-            if (c->masked) {
+                c->format = lex.token.format;
-                c->mask = lex.token.mask;
+                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) {

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;

ovn/northd/ovn-northd.c

@@ -6221,6 +6221,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.
@@ -6337,6 +6380,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;
@@ -6934,6 +6978,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);

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"},

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.

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",

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>

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();
@@ -2100,7 +2118,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);

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.
@@ -1224,6 +1243,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).
@@ -1364,10 +1390,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

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_const_sets_add(addr_sets, "set1", addrs1, 3, true);
-    expr_addr_sets_add(addr_sets, "set2", addrs2, 3);
+    expr_const_sets_add(addr_sets, "set2", addrs2, 3, true);
-    expr_addr_sets_add(addr_sets, "set3", addrs3, 3);
+    expr_const_sets_add(addr_sets, "set3", addrs3, 3, true);
-    expr_addr_sets_add(addr_sets, "set4", addrs4, 0);
+    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);