[ovs-dev,1/2] ovn: support applying ACLs to port groups

Although port group can be used in match conditions of ACLs, it is
still inconvenient for clients to figure out the lswitches that each
ACL should be applied to.

This patch supports applying ACLs to port groups directly instead of
applying to each related lswitch individually. It provides convenience
for clients such as k8s and OpenStack Neutron.

Requested-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-March/344856.html
Requested-by: Guru Shetty <guru@ovn.org>
Requested-by: Daniel Alvarez Sanchez <dalvarez@redhat.com>
Signed-off-by: Han Zhou <hzhou8@ebay.com>
---
 NEWS                    |   3 +-
 ovn/northd/ovn-northd.c | 422 +++++++++++++++++++++++++++++++-----------------
 ovn/ovn-nb.ovsschema    |   9 +-
 ovn/ovn-nb.xml          |  19 ++-
 tests/ovn.at            | 229 ++++++++++++++++++++++++++
 5 files changed, 526 insertions(+), 156 deletions(-)

On 22 April 2018 at 09:52, Han Zhou <zhouhan@gmail.com> wrote:

> Although port group can be used in match conditions of ACLs, it is
> still inconvenient for clients to figure out the lswitches that each
> ACL should be applied to.
>
> This patch supports applying ACLs to port groups directly instead of
> applying to each related lswitch individually. It provides convenience
> for clients such as k8s and OpenStack Neutron.
>
> Requested-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-March/
> 344856.html
> Requested-by: Guru Shetty <guru@ovn.org>
> Requested-by: Daniel Alvarez Sanchez <dalvarez@redhat.com>
> Signed-off-by: Han Zhou <hzhou8@ebay.com>
>

Thanks for the patch. I have not tested it, but just looked through the
code and it looks like what I had in mind.
So, you can technically use address sets or port groups in the ACLs inside
a port group, right?



> ---
>  NEWS                    |   3 +-
>  ovn/northd/ovn-northd.c | 422 ++++++++++++++++++++++++++++++
> +-----------------
>  ovn/ovn-nb.ovsschema    |   9 +-
>  ovn/ovn-nb.xml          |  19 ++-
>  tests/ovn.at            | 229 ++++++++++++++++++++++++++
>  5 files changed, 526 insertions(+), 156 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index cd4ffbb..8601cfc 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -22,7 +22,8 @@ Post-v2.9.0
>         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 as well as
> address
> -       sets that are automatically generated by Port_Groups.
> +       sets that are automatically generated by Port_Groups.  ACLs can be
> +       applied directly to Port_Groups as well.
>
>  v2.9.0 - 19 Feb 2018
>  --------------------
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index ce472a5..cd01756 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -3162,7 +3162,259 @@ build_reject_acl_rules(struct ovn_datapath *od,
> struct hmap *lflows,
>  }
>
>  static void
> -build_acls(struct ovn_datapath *od, struct hmap *lflows)
> +consider_acl(struct hmap *lflows, struct ovn_datapath *od,
> +             struct nbrec_acl *acl, bool has_stateful)
> +{
> +    bool ingress = !strcmp(acl->direction, "from-lport") ? true :false;
> +    enum ovn_stage stage = ingress ? S_SWITCH_IN_ACL : S_SWITCH_OUT_ACL;
> +
> +    char *stage_hint = xasprintf("%08x", acl->header_.uuid.parts[0]);
> +    if (!strcmp(acl->action, "allow")
> +        || !strcmp(acl->action, "allow-related")) {
> +        /* If there are any stateful flows, we must even commit "allow"
> +         * actions.  This is because, while the initiater's
> +         * direction may not have any stateful rules, the server's
> +         * may and then its return traffic would not have an
> +         * associated conntrack entry and would return "+invalid". */
> +        if (!has_stateful) {
> +            struct ds actions = DS_EMPTY_INITIALIZER;
> +            build_acl_log(&actions, acl);
> +            ds_put_cstr(&actions, "next;");
> +            ovn_lflow_add_with_hint(lflows, od, stage,
> +                                    acl->priority + OVN_ACL_PRI_OFFSET,
> +                                    acl->match, ds_cstr(&actions),
> +                                    stage_hint);
> +            ds_destroy(&actions);
> +        } else {
> +            struct ds match = DS_EMPTY_INITIALIZER;
> +            struct ds actions = DS_EMPTY_INITIALIZER;
> +
> +            /* Commit the connection tracking entry if it's a new
> +             * connection that matches this ACL.  After this commit,
> +             * the reply traffic is allowed by a flow we create at
> +             * priority 65535, defined earlier.
> +             *
> +             * It's also possible that a known connection was marked for
> +             * deletion after a policy was deleted, but the policy was
> +             * re-added while that connection is still known.  We catch
> +             * that case here and un-set ct_label.blocked (which will be
> done
> +             * by ct_commit in the "stateful" stage) to indicate that the
> +             * connection should be allowed to resume.
> +             */
> +            ds_put_format(&match, "((ct.new && !ct.est)"
> +                                  " || (!ct.new && ct.est && !ct.rpl "
> +                                       "&& ct_label.blocked == 1)) "
> +                                  "&& (%s)", acl->match);
> +            ds_put_cstr(&actions, REGBIT_CONNTRACK_COMMIT" = 1; ");
> +            build_acl_log(&actions, acl);
> +            ds_put_cstr(&actions, "next;");
> +            ovn_lflow_add_with_hint(lflows, od, stage,
> +                                    acl->priority + OVN_ACL_PRI_OFFSET,
> +                                    ds_cstr(&match),
> +                                    ds_cstr(&actions),
> +                                    stage_hint);
> +
> +            /* Match on traffic in the request direction for an
> established
> +             * connection tracking entry that has not been marked for
> +             * deletion.  There is no need to commit here, so we can just
> +             * proceed to the next table. We use this to ensure that this
> +             * connection is still allowed by the currently defined
> +             * policy. */
> +            ds_clear(&match);
> +            ds_clear(&actions);
> +            ds_put_format(&match,
> +                          "!ct.new && ct.est && !ct.rpl"
> +                          " && ct_label.blocked == 0 && (%s)",
> +                          acl->match);
> +
> +            build_acl_log(&actions, acl);
> +            ds_put_cstr(&actions, "next;");
> +            ovn_lflow_add_with_hint(lflows, od, stage,
> +                                    acl->priority + OVN_ACL_PRI_OFFSET,
> +                                    ds_cstr(&match), ds_cstr(&actions),
> +                                    stage_hint);
> +
> +            ds_destroy(&match);
> +            ds_destroy(&actions);
> +        }
> +    } else if (!strcmp(acl->action, "drop")
> +               || !strcmp(acl->action, "reject")) {
> +        struct ds match = DS_EMPTY_INITIALIZER;
> +        struct ds actions = DS_EMPTY_INITIALIZER;
> +
> +        /* The implementation of "drop" differs if stateful ACLs are in
> +         * use for this datapath.  In that case, the actions differ
> +         * depending on whether the connection was previously committed
> +         * to the connection tracker with ct_commit. */
> +        if (has_stateful) {
> +            /* If the packet is not part of an established connection,
> then
> +             * we can simply reject/drop it. */
> +            ds_put_cstr(&match,
> +                        "(!ct.est || (ct.est && ct_label.blocked == 1))");
> +            if (!strcmp(acl->action, "reject")) {
> +                build_reject_acl_rules(od, lflows, stage, acl, &match,
> +                                       &actions);
> +            } else {
> +                ds_put_format(&match, " && (%s)", acl->match);
> +                build_acl_log(&actions, acl);
> +                ds_put_cstr(&actions, "/* drop */");
> +                ovn_lflow_add(lflows, od, stage,
> +                              acl->priority + OVN_ACL_PRI_OFFSET,
> +                              ds_cstr(&match), ds_cstr(&actions));
> +            }
> +            /* For an existing connection without ct_label set, we've
> +             * encountered a policy change. ACLs previously allowed
> +             * this connection and we committed the connection tracking
> +             * entry.  Current policy says that we should drop this
> +             * connection.  First, we set bit 0 of ct_label to indicate
> +             * that this connection is set for deletion.  By not
> +             * specifying "next;", we implicitly drop the packet after
> +             * updating conntrack state.  We would normally defer
> +             * ct_commit() to the "stateful" stage, but since we're
> +             * rejecting/dropping the packet, we go ahead and do it here.
> +             */
> +            ds_clear(&match);
> +            ds_clear(&actions);
> +            ds_put_cstr(&match, "ct.est && ct_label.blocked == 0");
> +            ds_put_cstr(&actions, "ct_commit(ct_label=1/1); ");
> +            if (!strcmp(acl->action, "reject")) {
> +                build_reject_acl_rules(od, lflows, stage, acl, &match,
> +                                       &actions);
> +            } else {
> +                ds_put_format(&match, " && (%s)", acl->match);
> +                build_acl_log(&actions, acl);
> +                ds_put_cstr(&actions, "/* drop */");
> +                ovn_lflow_add(lflows, od, stage,
> +                              acl->priority + OVN_ACL_PRI_OFFSET,
> +                              ds_cstr(&match), ds_cstr(&actions));
> +            }
> +        } else {
> +            /* There are no stateful ACLs in use on this datapath,
> +             * so a "reject/drop" ACL is simply the "reject/drop"
> +             * logical flow action in all cases. */
> +            if (!strcmp(acl->action, "reject")) {
> +                build_reject_acl_rules(od, lflows, stage, acl, &match,
> +                                       &actions);
> +            } else {
> +                build_acl_log(&actions, acl);
> +                ds_put_cstr(&actions, "/* drop */");
> +                ovn_lflow_add(lflows, od, stage,
> +                              acl->priority + OVN_ACL_PRI_OFFSET,
> +                              acl->match, ds_cstr(&actions));
> +            }
> +        }
> +        ds_destroy(&match);
> +        ds_destroy(&actions);
> +    }
> +    free(stage_hint);
> +}
> +
> +struct ovn_port_group_ls {
> +    struct hmap_node key_node;  /* Index on 'key'. */
> +    struct uuid key;            /* nb_ls->header_.uuid. */
> +    const struct nbrec_logical_switch *nb_ls;
> +};
> +
> +struct ovn_port_group {
> +    struct hmap_node key_node;  /* Index on 'key'. */
> +    struct uuid key;            /* nb_pg->header_.uuid. */
> +    const struct nbrec_port_group *nb_pg;
> +    struct hmap nb_lswitches;   /* NB lswitches related to the port group
> */
> +    size_t n_acls;              /* Number of ACLs applied to the port
> group */
> +    struct nbrec_acl **acls;    /* ACLs applied to the port group */
> +};
> +
> +static struct ovn_port_group *
> +ovn_port_group_create(struct hmap *pgs,
> +                      const struct nbrec_port_group *nb_pg)
> +{
> +    struct ovn_port_group *pg = xzalloc(sizeof *pg);
> +    pg->key = nb_pg->header_.uuid;
> +    pg->nb_pg = nb_pg;
> +    pg->n_acls = nb_pg->n_acls;
> +    pg->acls = nb_pg->acls;
> +    hmap_init(&pg->nb_lswitches);
> +    hmap_insert(pgs, &pg->key_node, uuid_hash(&pg->key));
> +    return pg;
> +}
> +
> +static void
> +ovn_port_group_ls_add(struct ovn_port_group *pg,
> +                      const struct nbrec_logical_switch *nb_ls)
> +{
> +    struct ovn_port_group_ls *pg_ls = xzalloc(sizeof *pg_ls);
> +    pg_ls->key = nb_ls->header_.uuid;
> +    pg_ls->nb_ls = nb_ls;
> +    hmap_insert(&pg->nb_lswitches, &pg_ls->key_node,
> uuid_hash(&pg_ls->key));
> +}
> +
> +static struct ovn_port_group_ls *
> +ovn_port_group_ls_find(struct ovn_port_group *pg, const struct uuid
> *ls_uuid)
> +{
> +    struct ovn_port_group_ls *pg_ls;
> +
> +    HMAP_FOR_EACH_WITH_HASH (pg_ls, key_node, uuid_hash(ls_uuid),
> +                             &pg->nb_lswitches) {
> +        if (uuid_equals(ls_uuid, &pg_ls->key)) {
> +            return pg_ls;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static void
> +ovn_port_group_destroy(struct hmap *pgs, struct ovn_port_group *pg)
> +{
> +    if (pg) {
> +        hmap_remove(pgs, &pg->key_node);
> +        struct ovn_port_group_ls *ls;
> +        HMAP_FOR_EACH_POP (ls, key_node, &pg->nb_lswitches) {
> +            free(ls);
> +        }
> +        hmap_destroy(&pg->nb_lswitches);
> +        free(pg);
> +    }
> +}
> +
> +static void
> +build_port_group_lswitches(struct northd_context *ctx, struct hmap *pgs,
> +                           struct hmap *ports)
> +{
> +    hmap_init(pgs);
> +
> +    const struct nbrec_port_group *nb_pg;
> +    NBREC_PORT_GROUP_FOR_EACH (nb_pg, ctx->ovnnb_idl) {
> +        struct ovn_port_group *pg = ovn_port_group_create(pgs, nb_pg);
> +        for (size_t i = 0; i < nb_pg->n_ports; i++) {
> +            struct ovn_port *op = ovn_port_find(ports,
> nb_pg->ports[i]->name);
> +            if (!op) {
> +                static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(1, 1);
> +                VLOG_ERR_RL(&rl, "lport %s in port group %s not found.",
> +                            nb_pg->ports[i]->name,
> +                            nb_pg->name);
> +                continue;
> +            }
> +
> +            if (!op->od->nbs) {
> +                static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(1, 1);
> +                VLOG_WARN_RL(&rl, "lport %s in port group %s has no
> lswitch.",
> +                             nb_pg->ports[i]->name,
> +                             nb_pg->name);
> +                continue;
> +            }
> +
> +            struct ovn_port_group_ls *pg_ls =
> +                ovn_port_group_ls_find(pg, &op->od->nbs->header_.uuid);
> +            if (!pg_ls) {
> +                ovn_port_group_ls_add(pg, op->od->nbs);
> +            }
> +        }
> +    }
> +}
> +
> +static void
> +build_acls(struct ovn_datapath *od, struct hmap *lflows,
> +           struct hmap *port_groups)
>  {
>      bool has_stateful = has_stateful_acl(od);
>
> @@ -3263,148 +3515,15 @@ build_acls(struct ovn_datapath *od, struct hmap
> *lflows)
>      /* Ingress or Egress ACL Table (Various priorities). */
>      for (size_t i = 0; i < od->nbs->n_acls; i++) {
>          struct nbrec_acl *acl = od->nbs->acls[i];
> -        bool ingress = !strcmp(acl->direction, "from-lport") ? true
> :false;
> -        enum ovn_stage stage = ingress ? S_SWITCH_IN_ACL :
> S_SWITCH_OUT_ACL;
> -
> -        char *stage_hint = xasprintf("%08x", acl->header_.uuid.parts[0]);
> -        if (!strcmp(acl->action, "allow")
> -            || !strcmp(acl->action, "allow-related")) {
> -            /* If there are any stateful flows, we must even commit
> "allow"
> -             * actions.  This is because, while the initiater's
> -             * direction may not have any stateful rules, the server's
> -             * may and then its return traffic would not have an
> -             * associated conntrack entry and would return "+invalid". */
> -            if (!has_stateful) {
> -                struct ds actions = DS_EMPTY_INITIALIZER;
> -                build_acl_log(&actions, acl);
> -                ds_put_cstr(&actions, "next;");
> -                ovn_lflow_add_with_hint(lflows, od, stage,
> -                                        acl->priority +
> OVN_ACL_PRI_OFFSET,
> -                                        acl->match, ds_cstr(&actions),
> -                                        stage_hint);
> -                ds_destroy(&actions);
> -            } else {
> -                struct ds match = DS_EMPTY_INITIALIZER;
> -                struct ds actions = DS_EMPTY_INITIALIZER;
> -
> -                /* Commit the connection tracking entry if it's a new
> -                 * connection that matches this ACL.  After this commit,
> -                 * the reply traffic is allowed by a flow we create at
> -                 * priority 65535, defined earlier.
> -                 *
> -                 * It's also possible that a known connection was marked
> for
> -                 * deletion after a policy was deleted, but the policy was
> -                 * re-added while that connection is still known.  We
> catch
> -                 * that case here and un-set ct_label.blocked (which will
> be done
> -                 * by ct_commit in the "stateful" stage) to indicate that
> the
> -                 * connection should be allowed to resume.
> -                 */
> -                ds_put_format(&match, "((ct.new && !ct.est)"
> -                                      " || (!ct.new && ct.est && !ct.rpl "
> -                                           "&& ct_label.blocked == 1)) "
> -                                      "&& (%s)", acl->match);
> -                ds_put_cstr(&actions, REGBIT_CONNTRACK_COMMIT" = 1; ");
> -                build_acl_log(&actions, acl);
> -                ds_put_cstr(&actions, "next;");
> -                ovn_lflow_add_with_hint(lflows, od, stage,
> -                                        acl->priority +
> OVN_ACL_PRI_OFFSET,
> -                                        ds_cstr(&match),
> -                                        ds_cstr(&actions),
> -                                        stage_hint);
> -
> -                /* Match on traffic in the request direction for an
> established
> -                 * connection tracking entry that has not been marked for
> -                 * deletion.  There is no need to commit here, so we can
> just
> -                 * proceed to the next table. We use this to ensure that
> this
> -                 * connection is still allowed by the currently defined
> -                 * policy. */
> -                ds_clear(&match);
> -                ds_clear(&actions);
> -                ds_put_format(&match,
> -                              "!ct.new && ct.est && !ct.rpl"
> -                              " && ct_label.blocked == 0 && (%s)",
> -                              acl->match);
> -
> -                build_acl_log(&actions, acl);
> -                ds_put_cstr(&actions, "next;");
> -                ovn_lflow_add_with_hint(lflows, od, stage,
> -                                        acl->priority +
> OVN_ACL_PRI_OFFSET,
> -                                        ds_cstr(&match),
> ds_cstr(&actions),
> -                                        stage_hint);
> -
> -                ds_destroy(&match);
> -                ds_destroy(&actions);
> -            }
> -        } else if (!strcmp(acl->action, "drop")
> -                   || !strcmp(acl->action, "reject")) {
> -            struct ds match = DS_EMPTY_INITIALIZER;
> -            struct ds actions = DS_EMPTY_INITIALIZER;
> -
> -            /* The implementation of "drop" differs if stateful ACLs are
> in
> -             * use for this datapath.  In that case, the actions differ
> -             * depending on whether the connection was previously
> committed
> -             * to the connection tracker with ct_commit. */
> -            if (has_stateful) {
> -                /* If the packet is not part of an established
> connection, then
> -                 * we can simply reject/drop it. */
> -                ds_put_cstr(&match,
> -                            "(!ct.est || (ct.est && ct_label.blocked ==
> 1))");
> -                if (!strcmp(acl->action, "reject")) {
> -                    build_reject_acl_rules(od, lflows, stage, acl, &match,
> -                                           &actions);
> -                } else {
> -                    ds_put_format(&match, " && (%s)", acl->match);
> -                    build_acl_log(&actions, acl);
> -                    ds_put_cstr(&actions, "/* drop */");
> -                    ovn_lflow_add(lflows, od, stage,
> -                                  acl->priority + OVN_ACL_PRI_OFFSET,
> -                                  ds_cstr(&match), ds_cstr(&actions));
> -                }
> -                /* For an existing connection without ct_label set, we've
> -                 * encountered a policy change. ACLs previously allowed
> -                 * this connection and we committed the connection
> tracking
> -                 * entry.  Current policy says that we should drop this
> -                 * connection.  First, we set bit 0 of ct_label to
> indicate
> -                 * that this connection is set for deletion.  By not
> -                 * specifying "next;", we implicitly drop the packet after
> -                 * updating conntrack state.  We would normally defer
> -                 * ct_commit() to the "stateful" stage, but since we're
> -                 * rejecting/dropping the packet, we go ahead and do it
> here.
> -                 */
> -                ds_clear(&match);
> -                ds_clear(&actions);
> -                ds_put_cstr(&match, "ct.est && ct_label.blocked == 0");
> -                ds_put_cstr(&actions, "ct_commit(ct_label=1/1); ");
> -                if (!strcmp(acl->action, "reject")) {
> -                    build_reject_acl_rules(od, lflows, stage, acl, &match,
> -                                           &actions);
> -                } else {
> -                    ds_put_format(&match, " && (%s)", acl->match);
> -                    build_acl_log(&actions, acl);
> -                    ds_put_cstr(&actions, "/* drop */");
> -                    ovn_lflow_add(lflows, od, stage,
> -                                  acl->priority + OVN_ACL_PRI_OFFSET,
> -                                  ds_cstr(&match), ds_cstr(&actions));
> -                }
> -            } else {
> -                /* There are no stateful ACLs in use on this datapath,
> -                 * so a "reject/drop" ACL is simply the "reject/drop"
> -                 * logical flow action in all cases. */
> -                if (!strcmp(acl->action, "reject")) {
> -                    build_reject_acl_rules(od, lflows, stage, acl, &match,
> -                                           &actions);
> -                } else {
> -                    build_acl_log(&actions, acl);
> -                    ds_put_cstr(&actions, "/* drop */");
> -                    ovn_lflow_add(lflows, od, stage,
> -                                  acl->priority + OVN_ACL_PRI_OFFSET,
> -                                  acl->match, ds_cstr(&actions));
> -                }
> +        consider_acl(lflows, od, acl, has_stateful);
> +    }
> +    struct ovn_port_group *pg;
> +    HMAP_FOR_EACH (pg, key_node, port_groups) {
> +        if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) {
> +            for (size_t i = 0; i < pg->n_acls; i++) {
> +                consider_acl(lflows, od, pg->acls[i], has_stateful);
>              }
> -            ds_destroy(&match);
> -            ds_destroy(&actions);
>          }
> -        free(stage_hint);
>      }
>
>      /* Add 34000 priority flow to allow DHCP reply from ovn-controller to
> all
> @@ -3633,7 +3752,8 @@ build_stateful(struct ovn_datapath *od, struct hmap
> *lflows)
>
>  static void
>  build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
> -                    struct hmap *lflows, struct hmap *mcgroups)
> +                    struct hmap *port_groups, struct hmap *lflows,
> +                    struct hmap *mcgroups)
>  {
>      /* This flow table structure is documented in ovn-northd(8), so please
>       * update ovn-northd.8.xml if you change anything. */
> @@ -3652,7 +3772,7 @@ build_lswitch_flows(struct hmap *datapaths, struct
> hmap *ports,
>          build_pre_acls(od, lflows);
>          build_pre_lb(od, lflows);
>          build_pre_stateful(od, lflows);
> -        build_acls(od, lflows);
> +        build_acls(od, lflows, port_groups);
>          build_qos(od, lflows);
>          build_lb(od, lflows);
>          build_stateful(od, lflows);
> @@ -6069,12 +6189,12 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>   * constructing their contents based on the OVN_NB database. */
>  static void
>  build_lflows(struct northd_context *ctx, struct hmap *datapaths,
> -             struct hmap *ports)
> +             struct hmap *ports, struct hmap *port_groups)
>  {
>      struct hmap lflows = HMAP_INITIALIZER(&lflows);
>      struct hmap mcgroups = HMAP_INITIALIZER(&mcgroups);
>
> -    build_lswitch_flows(datapaths, ports, &lflows, &mcgroups);
> +    build_lswitch_flows(datapaths, ports, port_groups, &lflows,
> &mcgroups);
>      build_lrouter_flows(datapaths, ports, &lflows);
>
>      /* Push changes to the Logical_Flow table to database. */
> @@ -6421,6 +6541,7 @@ sync_dns_entries(struct northd_context *ctx, struct
> hmap *datapaths)
>      hmap_destroy(&dns_map);
>  }
>
> +
>
>  static void
>  ovnnb_db_run(struct northd_context *ctx, struct chassis_index
> *chassis_index,
> @@ -6429,16 +6550,23 @@ ovnnb_db_run(struct northd_context *ctx, struct
> chassis_index *chassis_index,
>      if (!ctx->ovnsb_txn || !ctx->ovnnb_txn) {
>          return;
>      }
> -    struct hmap datapaths, ports;
> +    struct hmap datapaths, ports, port_groups;
>      build_datapaths(ctx, &datapaths);
>      build_ports(ctx, &datapaths, chassis_index, &ports);
>      build_ipam(&datapaths, &ports);
> -    build_lflows(ctx, &datapaths, &ports);
> +    build_port_group_lswitches(ctx, &port_groups, &ports);
> +    build_lflows(ctx, &datapaths, &ports, &port_groups);
>
>      sync_address_sets(ctx);
>      sync_port_groups(ctx);
>      sync_dns_entries(ctx, &datapaths);
>
> +    struct ovn_port_group *pg, *next_pg;
> +    HMAP_FOR_EACH_SAFE (pg, next_pg, key_node, &port_groups) {
> +        ovn_port_group_destroy(&port_groups, pg);
> +    }
> +    hmap_destroy(&port_groups);
> +
>      struct ovn_datapath *dp, *next_dp;
>      HMAP_FOR_EACH_SAFE (dp, next_dp, key_node, &datapaths) {
>          ovn_datapath_destroy(&datapaths, dp);
> diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
> index 2d09282..8e6ddec 100644
> --- a/ovn/ovn-nb.ovsschema
> +++ b/ovn/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>  {
>      "name": "OVN_Northbound",
> -    "version": "5.10.0",
> -    "cksum": "222987318 18430",
> +    "version": "5.11.0",
> +    "cksum": "1149260021 18713",
>      "tables": {
>          "NB_Global": {
>              "columns": {
> @@ -122,6 +122,11 @@
>                                             "refType": "weak"},
>                                     "min": 0,
>                                     "max": "unlimited"}},
> +                "acls": {"type": {"key": {"type": "uuid",
> +                                          "refTable": "ACL",
> +                                          "refType": "strong"},
> +                                  "min": 0,
> +                                  "max": "unlimited"}},
>                  "external_ids": {
>                      "type": {"key": "string", "value": "string",
>                               "min": 0, "max": "unlimited"}}},
> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> index 62d5a07..6aed610 100644
> --- a/ovn/ovn-nb.xml
> +++ b/ovn/ovn-nb.xml
> @@ -963,6 +963,12 @@
>        The logical switch ports belonging to the group in uuids.
>      </column>
>
> +    <column name="acls">
> +      Access control rules that apply to the port group. Applying an ACL
> +      to a port group has the same effect as applying the ACL to all
> logical
> +      lswitches that the ports of the port group belong to.
> +    </column>
> +
>      <group title="Common Columns">
>        <column name="external_ids">
>          See <em>External IDs</em> at the beginning of this document.
> @@ -1030,12 +1036,13 @@
>    <table name="ACL" title="Access Control List (ACL) rule">
>      <p>
>        Each row in this table represents one ACL rule for a logical switch
> -      that points to it through its <ref column="acls"/> column.  The <ref
> -      column="action"/> column for the highest-<ref column="priority"/>
> -      matching row in this table determines a packet's treatment.  If no
> row
> -      matches, packets are allowed by default.  (Default-deny treatment is
> -      possible: add a rule with <ref column="priority"/> 0,
> <code>0</code> as
> -      <ref column="match"/>, and <code>deny</code> as <ref
> column="action"/>.)
> +      or a port group that points to it through its <ref column="acls"/>
> +      column.  The <ref column="action"/> column for the
> +      highest-<ref column="priority"/> matching row in this table
> determines a
> +      packet's treatment.  If no row matches, packets are allowed by
> default.
> +      (Default-deny treatment is possible: add a rule with
> +      <ref column="priority"/> 0, <code>0</code> as <ref column="match"/>,
> +      and <code>deny</code> as <ref column="action"/>.)
>      </p>
>
>      <column name="priority">
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 4a53165..95f747a 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -9798,3 +9798,232 @@ done
>  # Gracefully terminate daemons
>  OVN_CLEANUP([hv1], [hv2], [hv3])
>  AT_CLEANUP
> +
> +AT_SETUP([ovn -- ACLs on Port Groups])
> +AT_KEYWORDS([ovnpg_acl])
> +AT_SKIP_IF([test $HAVE_PYTHON = no])
> +ovn_start
> +
> +# Logical network:
> +#
> +# Three logical switches ls1, ls2, ls3.
> +# One logical router lr0 connected to ls[123],
> +# with nine subnets, three per logical switch:
> +#
> +#    lrp11 on ls1 for subnet 192.168.11.0/24
> +#    lrp12 on ls1 for subnet 192.168.12.0/24
> +#    lrp13 on ls1 for subnet 192.168.13.0/24
> +#    ...
> +#    lrp33 on ls3 for subnet 192.168.33.0/24
> +#
> +# 27 VIFs, 9 per LS, 3 per subnet: lp[123][123][123], where the first two
> +# digits are the subnet and the last digit distinguishes the VIF.
> +#
> +# This test will create two port groups and ACLs will be applied on them.
> +
> +get_lsp_uuid () {
> +    ovn-nbctl lsp-list ls${1%??} | grep lp$1 | awk '{ print $1 }'
> +}
> +
> +pg1_ports=
> +pg2_ports=
> +for i in 1 2 3; do
> +    ovn-nbctl ls-add ls$i
> +    for j in 1 2 3; do
> +        for k in 1 2 3; do
> +            ovn-nbctl \
> +                -- lsp-add ls$i lp$i$j$k \
> +                -- lsp-set-addresses lp$i$j$k "f0:00:00:00:0$i:$j$k \
> +                   192.168.$i$j.$k"
> +            # logical ports lp[12]?1 belongs to port group pg1
> +            if test $i != 3 && test $k == 1; then
> +                pg1_ports="$pg1_ports `get_lsp_uuid $i$j$k`"
> +            fi
> +            # logical ports lp[23]?2 belongs to port group pg2
> +            if test $i != 1 && test $k == 2; then
> +                pg2_ports="$pg2_ports `get_lsp_uuid $i$j$k`"
> +            fi
> +        done
> +    done
> +done
> +
> +ovn-nbctl lr-add lr0
> +for i in 1 2 3; do
> +    for j in 1 2 3; do
> +        ovn-nbctl lrp-add lr0 lrp$i$j 00:00:00:00:ff:$i$j
> 192.168.$i$j.254/24
> +        ovn-nbctl \
> +            -- lsp-add ls$i lrp$i$j-attachment \
> +            -- set Logical_Switch_Port lrp$i$j-attachment type=router \
> +                             options:router-port=lrp$i$j \
> +                             addresses='"00:00:00:00:ff:'$i$j'"'
> +    done
> +done
> +
> +pg1_uuid=`ovn-nbctl create Port_Group name=pg1 ports="$pg1_ports"`
> +ovn-nbctl create Port_Group name=pg2 ports="$pg2_ports"
> +
> +# create ACLs on pg1 to drop traffic from pg2 to pg1
> +ovn-nbctl --id=@acl1 create acl priority=1001 direction=to-lport \
> +        match='"outport == @pg1"' action=drop \
> +        -- add port_group $pg1_uuid acls @acl1
> +ovn-nbctl --id=@acl2 create acl priority=1002 direction=to-lport \
> +        match='"outport == @pg1 && ip4.src == $pg2_ip4"'
> action=allow-related \
> +        -- add port_group $pg1_uuid acls @acl2
> +
> +# Physical network:
> +#
> +# Three hypervisors hv[123].
> +# lp?1[123] spread across hv[123]: lp?11 on hv1, lp?12 on hv2, lp?13 on
> hv3.
> +# lp?2[123] spread across hv[23]: lp?21 and lp?22 on hv2, lp?23 on hv3.
> +# lp?3[123] all on hv3.
> +
> +# Given the name of a logical port, prints the name of the hypervisor
> +# on which it is located.
> +vif_to_hv() {
> +    case $1 in dnl (
> +        ?11) echo 1 ;; dnl (
> +        ?12 | ?21 | ?22) echo 2 ;; dnl (
> +        ?13 | ?23 | ?3?) echo 3 ;;
> +    esac
> +}
> +
> +# Given the name of a logical port, prints the name of its logical router
> +# port, e.g. "vif_to_lrp 123" yields 12.
> +vif_to_lrp() {
> +    echo ${1%?}
> +}
> +
> +# Given the name of a logical port, prints the name of its logical
> +# switch, e.g. "vif_to_ls 123" yields 1.
> +vif_to_ls() {
> +    echo ${1%??}
> +}
> +
> +net_add n1
> +for i in 1 2 3; do
> +    sim_add hv$i
> +    as hv$i
> +    ovs-vsctl add-br br-phys
> +    ovn_attach n1 br-phys 192.168.0.$i
> +done
> +for i in 1 2 3; do
> +    for j in 1 2 3; do
> +        for k in 1 2 3; do
> +            hv=`vif_to_hv $i$j$k`
> +                as hv$hv ovs-vsctl \
> +                -- add-port br-int vif$i$j$k \
> +                -- set Interface vif$i$j$k \
> +                    external-ids:iface-id=lp$i$j$k \
> +                    options:tx_pcap=hv$hv/vif$i$j$k-tx.pcap \
> +                    options:rxq_pcap=hv$hv/vif$i$j$k-rx.pcap \
> +                    ofport-request=$i$j$k
> +        done
> +    done
> +done
> +
> +# 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).
> +OVN_POPULATE_ARP
> +
> +# Allow some time for ovn-northd and ovn-controller to catch up.
> +# XXX This should be more systematic.
> +sleep 1
> +
> +# test_ip INPORT SRC_MAC DST_MAC SRC_IP DST_IP OUTPORT...
> +#
> +# This shell function causes a packet to be received on INPORT.  The
> packet's
> +# content has Ethernet destination DST and source SRC (each exactly 12 hex
> +# digits) and Ethernet type ETHTYPE (4 hex digits).  The OUTPORTs (zero or
> +# more) list the VIFs on which the packet should be received.  INPORT and
> the
> +# OUTPORTs are specified as logical switch port numbers, e.g. 123 for
> vif123.
> +for i in 1 2 3; do
> +    for j in 1 2 3; do
> +        for k in 1 2 3; do
> +            : > $i$j$k.expected
> +        done
> +    done
> +done
> +test_ip() {
> +    # This packet has bad checksums but logical L3 routing doesn't check.
> +    local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5
> +    local packet=${dst_mac}${src_mac}08004500001c0000000040110000${
> src_ip}${dst_ip}0035111100080000
> +    shift; shift; shift; shift; shift
> +    hv=hv`vif_to_hv $inport`
> +    as $hv ovs-appctl netdev-dummy/receive vif$inport $packet
> +    #as $hv ovs-appctl ofproto/trace br-int in_port=$inport $packet
> +    in_ls=`vif_to_ls $inport`
> +    in_lrp=`vif_to_lrp $inport`
> +    for outport; do
> +        out_ls=`vif_to_ls $outport`
> +        if test $in_ls = $out_ls; then
> +            # Ports on the same logical switch receive exactly the same
> packet.
> +            echo $packet
> +        else
> +            # Routing decrements TTL and updates source and dest MAC
> +            # (and checksum).
> +            out_lrp=`vif_to_lrp $outport`
> +            echo f00000000${outport}00000000ff$
> {out_lrp}08004500001c00000000"3f1101"00${src_ip}${dst_ip}0035111100080000
> +        fi >> $outport.expected
> +    done
> +}
> +
> +as hv1 ovs-vsctl --columns=name,ofport list interface
> +as hv1 ovn-sbctl list port_binding
> +as hv1 ovn-sbctl list datapath_binding
> +as hv1 ovn-sbctl list port_group
> +as hv1 ovn-sbctl list address_set
> +as hv1 ovn-sbctl dump-flows
> +as hv1 ovs-ofctl dump-flows br-int
> +
> +# Send IP packets between all pairs of source and destination ports,
> +# packets matches ACL1 but not ACL2 should be dropped
> +ip_to_hex() {
> +    printf "%02x%02x%02x%02x" "$@"
> +}
> +for is in 1 2 3; do
> +  for js in 1 2 3; do
> +    for ks in 1 2 3; do
> +      bcast=
> +      s=$is$js$ks
> +      smac=f00000000$s
> +      sip=`ip_to_hex 192 168 $is$js $ks`
> +      for id in 1 2 3; do
> +          for jd in 1 2 3; do
> +              for kd in 1 2 3; do
> +                d=$id$jd$kd
> +                dip=`ip_to_hex 192 168 $id$jd $kd`
> +                if test $is = $id; then dmac=f00000000$d; else
> dmac=00000000ff$is$js; fi
> +                if test $d != $s; then unicast=$d; else unicast=; fi
> +
> +                # packets matches ACL1 but not ACL2 should be dropped
> +                if test $id != 3 && test $kd == 1; then
> +                    if test $is == 1 || test $ks != 2; then
> +                        unicast=
> +                    fi
> +                fi
> +                test_ip $s $smac $dmac $sip $dip $unicast #1
> +              done
> +          done
> +        done
> +      done
> +  done
> +done
> +
> +# Allow some time for packet forwarding.
> +# XXX This can be improved.
> +sleep 1
> +
> +# Now check the packets actually received against the ones expected.
> +for i in 1 2 3; do
> +    for j in 1 2 3; do
> +        for k in 1 2 3; do
> +            OVN_CHECK_PACKETS([hv`vif_to_hv $i$j$k`/vif$i$j$k-tx.pcap],
> +                              [$i$j$k.expected])
> +        done
> +    done
> +done
> +
> +# Gracefully terminate daemons
> +OVN_CLEANUP([hv1], [hv2], [hv3])
> +AT_CLEANUP
> --
> 2.1.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On Mon, Apr 30, 2018 at 2:43 PM, Guru Shetty <guru@ovn.org> wrote:

>
>
> On 22 April 2018 at 09:52, Han Zhou <zhouhan@gmail.com> wrote:
>
>> Although port group can be used in match conditions of ACLs, it is
>> still inconvenient for clients to figure out the lswitches that each
>> ACL should be applied to.
>>
>> This patch supports applying ACLs to port groups directly instead of
>> applying to each related lswitch individually. It provides convenience
>> for clients such as k8s and OpenStack Neutron.
>>
>> Requested-at: https://mail.openvswitch.org/p
>> ipermail/ovs-dev/2018-March/344856.html
>> Requested-by: Guru Shetty <guru@ovn.org>
>> Requested-by: Daniel Alvarez Sanchez <dalvarez@redhat.com>
>> Signed-off-by: Han Zhou <hzhou8@ebay.com>
>>
>
> Thanks for the patch. I have not tested it, but just looked through the
> code and it looks like what I had in mind.
> So, you can technically use address sets or port groups in the ACLs inside
> a port group, right?
>
> Hi Guru, yes, you are right :)

>
>
>> ---
>>  NEWS                    |   3 +-
>>  ovn/northd/ovn-northd.c | 422 ++++++++++++++++++++++++++++++
>> +-----------------
>>  ovn/ovn-nb.ovsschema    |   9 +-
>>  ovn/ovn-nb.xml          |  19 ++-
>>  tests/ovn.at            | 229 ++++++++++++++++++++++++++
>>  5 files changed, 526 insertions(+), 156 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index cd4ffbb..8601cfc 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -22,7 +22,8 @@ Post-v2.9.0
>>         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 as well as
>> address
>> -       sets that are automatically generated by Port_Groups.
>> +       sets that are automatically generated by Port_Groups.  ACLs can be
>> +       applied directly to Port_Groups as well.
>>
>>  v2.9.0 - 19 Feb 2018
>>  --------------------
>> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
>> index ce472a5..cd01756 100644
>> --- a/ovn/northd/ovn-northd.c
>> +++ b/ovn/northd/ovn-northd.c
>> @@ -3162,7 +3162,259 @@ build_reject_acl_rules(struct ovn_datapath *od,
>> struct hmap *lflows,
>>  }
>>
>>  static void
>> -build_acls(struct ovn_datapath *od, struct hmap *lflows)
>> +consider_acl(struct hmap *lflows, struct ovn_datapath *od,
>> +             struct nbrec_acl *acl, bool has_stateful)
>> +{
>> +    bool ingress = !strcmp(acl->direction, "from-lport") ? true :false;
>> +    enum ovn_stage stage = ingress ? S_SWITCH_IN_ACL : S_SWITCH_OUT_ACL;
>> +
>> +    char *stage_hint = xasprintf("%08x", acl->header_.uuid.parts[0]);
>> +    if (!strcmp(acl->action, "allow")
>> +        || !strcmp(acl->action, "allow-related")) {
>> +        /* If there are any stateful flows, we must even commit "allow"
>> +         * actions.  This is because, while the initiater's
>> +         * direction may not have any stateful rules, the server's
>> +         * may and then its return traffic would not have an
>> +         * associated conntrack entry and would return "+invalid". */
>> +        if (!has_stateful) {
>> +            struct ds actions = DS_EMPTY_INITIALIZER;
>> +            build_acl_log(&actions, acl);
>> +            ds_put_cstr(&actions, "next;");
>> +            ovn_lflow_add_with_hint(lflows, od, stage,
>> +                                    acl->priority + OVN_ACL_PRI_OFFSET,
>> +                                    acl->match, ds_cstr(&actions),
>> +                                    stage_hint);
>> +            ds_destroy(&actions);
>> +        } else {
>> +            struct ds match = DS_EMPTY_INITIALIZER;
>> +            struct ds actions = DS_EMPTY_INITIALIZER;
>> +
>> +            /* Commit the connection tracking entry if it's a new
>> +             * connection that matches this ACL.  After this commit,
>> +             * the reply traffic is allowed by a flow we create at
>> +             * priority 65535, defined earlier.
>> +             *
>> +             * It's also possible that a known connection was marked for
>> +             * deletion after a policy was deleted, but the policy was
>> +             * re-added while that connection is still known.  We catch
>> +             * that case here and un-set ct_label.blocked (which will be
>> done
>> +             * by ct_commit in the "stateful" stage) to indicate that the
>> +             * connection should be allowed to resume.
>> +             */
>> +            ds_put_format(&match, "((ct.new && !ct.est)"
>> +                                  " || (!ct.new && ct.est && !ct.rpl "
>> +                                       "&& ct_label.blocked == 1)) "
>> +                                  "&& (%s)", acl->match);
>> +            ds_put_cstr(&actions, REGBIT_CONNTRACK_COMMIT" = 1; ");
>> +            build_acl_log(&actions, acl);
>> +            ds_put_cstr(&actions, "next;");
>> +            ovn_lflow_add_with_hint(lflows, od, stage,
>> +                                    acl->priority + OVN_ACL_PRI_OFFSET,
>> +                                    ds_cstr(&match),
>> +                                    ds_cstr(&actions),
>> +                                    stage_hint);
>> +
>> +            /* Match on traffic in the request direction for an
>> established
>> +             * connection tracking entry that has not been marked for
>> +             * deletion.  There is no need to commit here, so we can just
>> +             * proceed to the next table. We use this to ensure that this
>> +             * connection is still allowed by the currently defined
>> +             * policy. */
>> +            ds_clear(&match);
>> +            ds_clear(&actions);
>> +            ds_put_format(&match,
>> +                          "!ct.new && ct.est && !ct.rpl"
>> +                          " && ct_label.blocked == 0 && (%s)",
>> +                          acl->match);
>> +
>> +            build_acl_log(&actions, acl);
>> +            ds_put_cstr(&actions, "next;");
>> +            ovn_lflow_add_with_hint(lflows, od, stage,
>> +                                    acl->priority + OVN_ACL_PRI_OFFSET,
>> +                                    ds_cstr(&match), ds_cstr(&actions),
>> +                                    stage_hint);
>> +
>> +            ds_destroy(&match);
>> +            ds_destroy(&actions);
>> +        }
>> +    } else if (!strcmp(acl->action, "drop")
>> +               || !strcmp(acl->action, "reject")) {
>> +        struct ds match = DS_EMPTY_INITIALIZER;
>> +        struct ds actions = DS_EMPTY_INITIALIZER;
>> +
>> +        /* The implementation of "drop" differs if stateful ACLs are in
>> +         * use for this datapath.  In that case, the actions differ
>> +         * depending on whether the connection was previously committed
>> +         * to the connection tracker with ct_commit. */
>> +        if (has_stateful) {
>> +            /* If the packet is not part of an established connection,
>> then
>> +             * we can simply reject/drop it. */
>> +            ds_put_cstr(&match,
>> +                        "(!ct.est || (ct.est && ct_label.blocked ==
>> 1))");
>> +            if (!strcmp(acl->action, "reject")) {
>> +                build_reject_acl_rules(od, lflows, stage, acl, &match,
>> +                                       &actions);
>> +            } else {
>> +                ds_put_format(&match, " && (%s)", acl->match);
>> +                build_acl_log(&actions, acl);
>> +                ds_put_cstr(&actions, "/* drop */");
>> +                ovn_lflow_add(lflows, od, stage,
>> +                              acl->priority + OVN_ACL_PRI_OFFSET,
>> +                              ds_cstr(&match), ds_cstr(&actions));
>> +            }
>> +            /* For an existing connection without ct_label set, we've
>> +             * encountered a policy change. ACLs previously allowed
>> +             * this connection and we committed the connection tracking
>> +             * entry.  Current policy says that we should drop this
>> +             * connection.  First, we set bit 0 of ct_label to indicate
>> +             * that this connection is set for deletion.  By not
>> +             * specifying "next;", we implicitly drop the packet after
>> +             * updating conntrack state.  We would normally defer
>> +             * ct_commit() to the "stateful" stage, but since we're
>> +             * rejecting/dropping the packet, we go ahead and do it here.
>> +             */
>> +            ds_clear(&match);
>> +            ds_clear(&actions);
>> +            ds_put_cstr(&match, "ct.est && ct_label.blocked == 0");
>> +            ds_put_cstr(&actions, "ct_commit(ct_label=1/1); ");
>> +            if (!strcmp(acl->action, "reject")) {
>> +                build_reject_acl_rules(od, lflows, stage, acl, &match,
>> +                                       &actions);
>> +            } else {
>> +                ds_put_format(&match, " && (%s)", acl->match);
>> +                build_acl_log(&actions, acl);
>> +                ds_put_cstr(&actions, "/* drop */");
>> +                ovn_lflow_add(lflows, od, stage,
>> +                              acl->priority + OVN_ACL_PRI_OFFSET,
>> +                              ds_cstr(&match), ds_cstr(&actions));
>> +            }
>> +        } else {
>> +            /* There are no stateful ACLs in use on this datapath,
>> +             * so a "reject/drop" ACL is simply the "reject/drop"
>> +             * logical flow action in all cases. */
>> +            if (!strcmp(acl->action, "reject")) {
>> +                build_reject_acl_rules(od, lflows, stage, acl, &match,
>> +                                       &actions);
>> +            } else {
>> +                build_acl_log(&actions, acl);
>> +                ds_put_cstr(&actions, "/* drop */");
>> +                ovn_lflow_add(lflows, od, stage,
>> +                              acl->priority + OVN_ACL_PRI_OFFSET,
>> +                              acl->match, ds_cstr(&actions));
>> +            }
>> +        }
>> +        ds_destroy(&match);
>> +        ds_destroy(&actions);
>> +    }
>> +    free(stage_hint);
>> +}
>> +
>> +struct ovn_port_group_ls {
>> +    struct hmap_node key_node;  /* Index on 'key'. */
>> +    struct uuid key;            /* nb_ls->header_.uuid. */
>> +    const struct nbrec_logical_switch *nb_ls;
>> +};
>> +
>> +struct ovn_port_group {
>> +    struct hmap_node key_node;  /* Index on 'key'. */
>> +    struct uuid key;            /* nb_pg->header_.uuid. */
>> +    const struct nbrec_port_group *nb_pg;
>> +    struct hmap nb_lswitches;   /* NB lswitches related to the port
>> group */
>> +    size_t n_acls;              /* Number of ACLs applied to the port
>> group */
>> +    struct nbrec_acl **acls;    /* ACLs applied to the port group */
>> +};
>> +
>> +static struct ovn_port_group *
>> +ovn_port_group_create(struct hmap *pgs,
>> +                      const struct nbrec_port_group *nb_pg)
>> +{
>> +    struct ovn_port_group *pg = xzalloc(sizeof *pg);
>> +    pg->key = nb_pg->header_.uuid;
>> +    pg->nb_pg = nb_pg;
>> +    pg->n_acls = nb_pg->n_acls;
>> +    pg->acls = nb_pg->acls;
>> +    hmap_init(&pg->nb_lswitches);
>> +    hmap_insert(pgs, &pg->key_node, uuid_hash(&pg->key));
>> +    return pg;
>> +}
>> +
>> +static void
>> +ovn_port_group_ls_add(struct ovn_port_group *pg,
>> +                      const struct nbrec_logical_switch *nb_ls)
>> +{
>> +    struct ovn_port_group_ls *pg_ls = xzalloc(sizeof *pg_ls);
>> +    pg_ls->key = nb_ls->header_.uuid;
>> +    pg_ls->nb_ls = nb_ls;
>> +    hmap_insert(&pg->nb_lswitches, &pg_ls->key_node,
>> uuid_hash(&pg_ls->key));
>> +}
>> +
>> +static struct ovn_port_group_ls *
>> +ovn_port_group_ls_find(struct ovn_port_group *pg, const struct uuid
>> *ls_uuid)
>> +{
>> +    struct ovn_port_group_ls *pg_ls;
>> +
>> +    HMAP_FOR_EACH_WITH_HASH (pg_ls, key_node, uuid_hash(ls_uuid),
>> +                             &pg->nb_lswitches) {
>> +        if (uuid_equals(ls_uuid, &pg_ls->key)) {
>> +            return pg_ls;
>> +        }
>> +    }
>> +    return NULL;
>> +}
>> +
>> +static void
>> +ovn_port_group_destroy(struct hmap *pgs, struct ovn_port_group *pg)
>> +{
>> +    if (pg) {
>> +        hmap_remove(pgs, &pg->key_node);
>> +        struct ovn_port_group_ls *ls;
>> +        HMAP_FOR_EACH_POP (ls, key_node, &pg->nb_lswitches) {
>> +            free(ls);
>> +        }
>> +        hmap_destroy(&pg->nb_lswitches);
>> +        free(pg);
>> +    }
>> +}
>> +
>> +static void
>> +build_port_group_lswitches(struct northd_context *ctx, struct hmap *pgs,
>> +                           struct hmap *ports)
>> +{
>> +    hmap_init(pgs);
>> +
>> +    const struct nbrec_port_group *nb_pg;
>> +    NBREC_PORT_GROUP_FOR_EACH (nb_pg, ctx->ovnnb_idl) {
>> +        struct ovn_port_group *pg = ovn_port_group_create(pgs, nb_pg);
>> +        for (size_t i = 0; i < nb_pg->n_ports; i++) {
>> +            struct ovn_port *op = ovn_port_find(ports,
>> nb_pg->ports[i]->name);
>> +            if (!op) {
>> +                static struct vlog_rate_limit rl =
>> VLOG_RATE_LIMIT_INIT(1, 1);
>> +                VLOG_ERR_RL(&rl, "lport %s in port group %s not found.",
>> +                            nb_pg->ports[i]->name,
>> +                            nb_pg->name);
>> +                continue;
>> +            }
>> +
>> +            if (!op->od->nbs) {
>> +                static struct vlog_rate_limit rl =
>> VLOG_RATE_LIMIT_INIT(1, 1);
>> +                VLOG_WARN_RL(&rl, "lport %s in port group %s has no
>> lswitch.",
>> +                             nb_pg->ports[i]->name,
>> +                             nb_pg->name);
>> +                continue;
>> +            }
>> +
>> +            struct ovn_port_group_ls *pg_ls =
>> +                ovn_port_group_ls_find(pg, &op->od->nbs->header_.uuid);
>> +            if (!pg_ls) {
>> +                ovn_port_group_ls_add(pg, op->od->nbs);
>> +            }
>> +        }
>> +    }
>> +}
>> +
>> +static void
>> +build_acls(struct ovn_datapath *od, struct hmap *lflows,
>> +           struct hmap *port_groups)
>>  {
>>      bool has_stateful = has_stateful_acl(od);
>>
>> @@ -3263,148 +3515,15 @@ build_acls(struct ovn_datapath *od, struct hmap
>> *lflows)
>>      /* Ingress or Egress ACL Table (Various priorities). */
>>      for (size_t i = 0; i < od->nbs->n_acls; i++) {
>>          struct nbrec_acl *acl = od->nbs->acls[i];
>> -        bool ingress = !strcmp(acl->direction, "from-lport") ? true
>> :false;
>> -        enum ovn_stage stage = ingress ? S_SWITCH_IN_ACL :
>> S_SWITCH_OUT_ACL;
>> -
>> -        char *stage_hint = xasprintf("%08x", acl->header_.uuid.parts[0]);
>> -        if (!strcmp(acl->action, "allow")
>> -            || !strcmp(acl->action, "allow-related")) {
>> -            /* If there are any stateful flows, we must even commit
>> "allow"
>> -             * actions.  This is because, while the initiater's
>> -             * direction may not have any stateful rules, the server's
>> -             * may and then its return traffic would not have an
>> -             * associated conntrack entry and would return "+invalid". */
>> -            if (!has_stateful) {
>> -                struct ds actions = DS_EMPTY_INITIALIZER;
>> -                build_acl_log(&actions, acl);
>> -                ds_put_cstr(&actions, "next;");
>> -                ovn_lflow_add_with_hint(lflows, od, stage,
>> -                                        acl->priority +
>> OVN_ACL_PRI_OFFSET,
>> -                                        acl->match, ds_cstr(&actions),
>> -                                        stage_hint);
>> -                ds_destroy(&actions);
>> -            } else {
>> -                struct ds match = DS_EMPTY_INITIALIZER;
>> -                struct ds actions = DS_EMPTY_INITIALIZER;
>> -
>> -                /* Commit the connection tracking entry if it's a new
>> -                 * connection that matches this ACL.  After this commit,
>> -                 * the reply traffic is allowed by a flow we create at
>> -                 * priority 65535, defined earlier.
>> -                 *
>> -                 * It's also possible that a known connection was marked
>> for
>> -                 * deletion after a policy was deleted, but the policy
>> was
>> -                 * re-added while that connection is still known.  We
>> catch
>> -                 * that case here and un-set ct_label.blocked (which
>> will be done
>> -                 * by ct_commit in the "stateful" stage) to indicate
>> that the
>> -                 * connection should be allowed to resume.
>> -                 */
>> -                ds_put_format(&match, "((ct.new && !ct.est)"
>> -                                      " || (!ct.new && ct.est && !ct.rpl
>> "
>> -                                           "&& ct_label.blocked == 1)) "
>> -                                      "&& (%s)", acl->match);
>> -                ds_put_cstr(&actions, REGBIT_CONNTRACK_COMMIT" = 1; ");
>> -                build_acl_log(&actions, acl);
>> -                ds_put_cstr(&actions, "next;");
>> -                ovn_lflow_add_with_hint(lflows, od, stage,
>> -                                        acl->priority +
>> OVN_ACL_PRI_OFFSET,
>> -                                        ds_cstr(&match),
>> -                                        ds_cstr(&actions),
>> -                                        stage_hint);
>> -
>> -                /* Match on traffic in the request direction for an
>> established
>> -                 * connection tracking entry that has not been marked for
>> -                 * deletion.  There is no need to commit here, so we can
>> just
>> -                 * proceed to the next table. We use this to ensure that
>> this
>> -                 * connection is still allowed by the currently defined
>> -                 * policy. */
>> -                ds_clear(&match);
>> -                ds_clear(&actions);
>> -                ds_put_format(&match,
>> -                              "!ct.new && ct.est && !ct.rpl"
>> -                              " && ct_label.blocked == 0 && (%s)",
>> -                              acl->match);
>> -
>> -                build_acl_log(&actions, acl);
>> -                ds_put_cstr(&actions, "next;");
>> -                ovn_lflow_add_with_hint(lflows, od, stage,
>> -                                        acl->priority +
>> OVN_ACL_PRI_OFFSET,
>> -                                        ds_cstr(&match),
>> ds_cstr(&actions),
>> -                                        stage_hint);
>> -
>> -                ds_destroy(&match);
>> -                ds_destroy(&actions);
>> -            }
>> -        } else if (!strcmp(acl->action, "drop")
>> -                   || !strcmp(acl->action, "reject")) {
>> -            struct ds match = DS_EMPTY_INITIALIZER;
>> -            struct ds actions = DS_EMPTY_INITIALIZER;
>> -
>> -            /* The implementation of "drop" differs if stateful ACLs are
>> in
>> -             * use for this datapath.  In that case, the actions differ
>> -             * depending on whether the connection was previously
>> committed
>> -             * to the connection tracker with ct_commit. */
>> -            if (has_stateful) {
>> -                /* If the packet is not part of an established
>> connection, then
>> -                 * we can simply reject/drop it. */
>> -                ds_put_cstr(&match,
>> -                            "(!ct.est || (ct.est && ct_label.blocked ==
>> 1))");
>> -                if (!strcmp(acl->action, "reject")) {
>> -                    build_reject_acl_rules(od, lflows, stage, acl,
>> &match,
>> -                                           &actions);
>> -                } else {
>> -                    ds_put_format(&match, " && (%s)", acl->match);
>> -                    build_acl_log(&actions, acl);
>> -                    ds_put_cstr(&actions, "/* drop */");
>> -                    ovn_lflow_add(lflows, od, stage,
>> -                                  acl->priority + OVN_ACL_PRI_OFFSET,
>> -                                  ds_cstr(&match), ds_cstr(&actions));
>> -                }
>> -                /* For an existing connection without ct_label set, we've
>> -                 * encountered a policy change. ACLs previously allowed
>> -                 * this connection and we committed the connection
>> tracking
>> -                 * entry.  Current policy says that we should drop this
>> -                 * connection.  First, we set bit 0 of ct_label to
>> indicate
>> -                 * that this connection is set for deletion.  By not
>> -                 * specifying "next;", we implicitly drop the packet
>> after
>> -                 * updating conntrack state.  We would normally defer
>> -                 * ct_commit() to the "stateful" stage, but since we're
>> -                 * rejecting/dropping the packet, we go ahead and do it
>> here.
>> -                 */
>> -                ds_clear(&match);
>> -                ds_clear(&actions);
>> -                ds_put_cstr(&match, "ct.est && ct_label.blocked == 0");
>> -                ds_put_cstr(&actions, "ct_commit(ct_label=1/1); ");
>> -                if (!strcmp(acl->action, "reject")) {
>> -                    build_reject_acl_rules(od, lflows, stage, acl,
>> &match,
>> -                                           &actions);
>> -                } else {
>> -                    ds_put_format(&match, " && (%s)", acl->match);
>> -                    build_acl_log(&actions, acl);
>> -                    ds_put_cstr(&actions, "/* drop */");
>> -                    ovn_lflow_add(lflows, od, stage,
>> -                                  acl->priority + OVN_ACL_PRI_OFFSET,
>> -                                  ds_cstr(&match), ds_cstr(&actions));
>> -                }
>> -            } else {
>> -                /* There are no stateful ACLs in use on this datapath,
>> -                 * so a "reject/drop" ACL is simply the "reject/drop"
>> -                 * logical flow action in all cases. */
>> -                if (!strcmp(acl->action, "reject")) {
>> -                    build_reject_acl_rules(od, lflows, stage, acl,
>> &match,
>> -                                           &actions);
>> -                } else {
>> -                    build_acl_log(&actions, acl);
>> -                    ds_put_cstr(&actions, "/* drop */");
>> -                    ovn_lflow_add(lflows, od, stage,
>> -                                  acl->priority + OVN_ACL_PRI_OFFSET,
>> -                                  acl->match, ds_cstr(&actions));
>> -                }
>> +        consider_acl(lflows, od, acl, has_stateful);
>> +    }
>> +    struct ovn_port_group *pg;
>> +    HMAP_FOR_EACH (pg, key_node, port_groups) {
>> +        if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) {
>> +            for (size_t i = 0; i < pg->n_acls; i++) {
>> +                consider_acl(lflows, od, pg->acls[i], has_stateful);
>>              }
>> -            ds_destroy(&match);
>> -            ds_destroy(&actions);
>>          }
>> -        free(stage_hint);
>>      }
>>
>>      /* Add 34000 priority flow to allow DHCP reply from ovn-controller
>> to all
>> @@ -3633,7 +3752,8 @@ build_stateful(struct ovn_datapath *od, struct hmap
>> *lflows)
>>
>>  static void
>>  build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
>> -                    struct hmap *lflows, struct hmap *mcgroups)
>> +                    struct hmap *port_groups, struct hmap *lflows,
>> +                    struct hmap *mcgroups)
>>  {
>>      /* This flow table structure is documented in ovn-northd(8), so
>> please
>>       * update ovn-northd.8.xml if you change anything. */
>> @@ -3652,7 +3772,7 @@ build_lswitch_flows(struct hmap *datapaths, struct
>> hmap *ports,
>>          build_pre_acls(od, lflows);
>>          build_pre_lb(od, lflows);
>>          build_pre_stateful(od, lflows);
>> -        build_acls(od, lflows);
>> +        build_acls(od, lflows, port_groups);
>>          build_qos(od, lflows);
>>          build_lb(od, lflows);
>>          build_stateful(od, lflows);
>> @@ -6069,12 +6189,12 @@ build_lrouter_flows(struct hmap *datapaths,
>> struct hmap *ports,
>>   * constructing their contents based on the OVN_NB database. */
>>  static void
>>  build_lflows(struct northd_context *ctx, struct hmap *datapaths,
>> -             struct hmap *ports)
>> +             struct hmap *ports, struct hmap *port_groups)
>>  {
>>      struct hmap lflows = HMAP_INITIALIZER(&lflows);
>>      struct hmap mcgroups = HMAP_INITIALIZER(&mcgroups);
>>
>> -    build_lswitch_flows(datapaths, ports, &lflows, &mcgroups);
>> +    build_lswitch_flows(datapaths, ports, port_groups, &lflows,
>> &mcgroups);
>>      build_lrouter_flows(datapaths, ports, &lflows);
>>
>>      /* Push changes to the Logical_Flow table to database. */
>> @@ -6421,6 +6541,7 @@ sync_dns_entries(struct northd_context *ctx, struct
>> hmap *datapaths)
>>      hmap_destroy(&dns_map);
>>  }
>>
>> +
>>
>>  static void
>>  ovnnb_db_run(struct northd_context *ctx, struct chassis_index
>> *chassis_index,
>> @@ -6429,16 +6550,23 @@ ovnnb_db_run(struct northd_context *ctx, struct
>> chassis_index *chassis_index,
>>      if (!ctx->ovnsb_txn || !ctx->ovnnb_txn) {
>>          return;
>>      }
>> -    struct hmap datapaths, ports;
>> +    struct hmap datapaths, ports, port_groups;
>>      build_datapaths(ctx, &datapaths);
>>      build_ports(ctx, &datapaths, chassis_index, &ports);
>>      build_ipam(&datapaths, &ports);
>> -    build_lflows(ctx, &datapaths, &ports);
>> +    build_port_group_lswitches(ctx, &port_groups, &ports);
>> +    build_lflows(ctx, &datapaths, &ports, &port_groups);
>>
>>      sync_address_sets(ctx);
>>      sync_port_groups(ctx);
>>      sync_dns_entries(ctx, &datapaths);
>>
>> +    struct ovn_port_group *pg, *next_pg;
>> +    HMAP_FOR_EACH_SAFE (pg, next_pg, key_node, &port_groups) {
>> +        ovn_port_group_destroy(&port_groups, pg);
>> +    }
>> +    hmap_destroy(&port_groups);
>> +
>>      struct ovn_datapath *dp, *next_dp;
>>      HMAP_FOR_EACH_SAFE (dp, next_dp, key_node, &datapaths) {
>>          ovn_datapath_destroy(&datapaths, dp);
>> diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
>> index 2d09282..8e6ddec 100644
>> --- a/ovn/ovn-nb.ovsschema
>> +++ b/ovn/ovn-nb.ovsschema
>> @@ -1,7 +1,7 @@
>>  {
>>      "name": "OVN_Northbound",
>> -    "version": "5.10.0",
>> -    "cksum": "222987318 18430",
>> +    "version": "5.11.0",
>> +    "cksum": "1149260021 18713",
>>      "tables": {
>>          "NB_Global": {
>>              "columns": {
>> @@ -122,6 +122,11 @@
>>                                             "refType": "weak"},
>>                                     "min": 0,
>>                                     "max": "unlimited"}},
>> +                "acls": {"type": {"key": {"type": "uuid",
>> +                                          "refTable": "ACL",
>> +                                          "refType": "strong"},
>> +                                  "min": 0,
>> +                                  "max": "unlimited"}},
>>                  "external_ids": {
>>                      "type": {"key": "string", "value": "string",
>>                               "min": 0, "max": "unlimited"}}},
>> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
>> index 62d5a07..6aed610 100644
>> --- a/ovn/ovn-nb.xml
>> +++ b/ovn/ovn-nb.xml
>> @@ -963,6 +963,12 @@
>>        The logical switch ports belonging to the group in uuids.
>>      </column>
>>
>> +    <column name="acls">
>> +      Access control rules that apply to the port group. Applying an ACL
>> +      to a port group has the same effect as applying the ACL to all
>> logical
>> +      lswitches that the ports of the port group belong to.
>> +    </column>
>> +
>>      <group title="Common Columns">
>>        <column name="external_ids">
>>          See <em>External IDs</em> at the beginning of this document.
>> @@ -1030,12 +1036,13 @@
>>    <table name="ACL" title="Access Control List (ACL) rule">
>>      <p>
>>        Each row in this table represents one ACL rule for a logical switch
>> -      that points to it through its <ref column="acls"/> column.  The
>> <ref
>> -      column="action"/> column for the highest-<ref column="priority"/>
>> -      matching row in this table determines a packet's treatment.  If no
>> row
>> -      matches, packets are allowed by default.  (Default-deny treatment
>> is
>> -      possible: add a rule with <ref column="priority"/> 0,
>> <code>0</code> as
>> -      <ref column="match"/>, and <code>deny</code> as <ref
>> column="action"/>.)
>> +      or a port group that points to it through its <ref column="acls"/>
>> +      column.  The <ref column="action"/> column for the
>> +      highest-<ref column="priority"/> matching row in this table
>> determines a
>> +      packet's treatment.  If no row matches, packets are allowed by
>> default.
>> +      (Default-deny treatment is possible: add a rule with
>> +      <ref column="priority"/> 0, <code>0</code> as <ref
>> column="match"/>,
>> +      and <code>deny</code> as <ref column="action"/>.)
>>      </p>
>>
>>      <column name="priority">
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index 4a53165..95f747a 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -9798,3 +9798,232 @@ done
>>  # Gracefully terminate daemons
>>  OVN_CLEANUP([hv1], [hv2], [hv3])
>>  AT_CLEANUP
>> +
>> +AT_SETUP([ovn -- ACLs on Port Groups])
>> +AT_KEYWORDS([ovnpg_acl])
>> +AT_SKIP_IF([test $HAVE_PYTHON = no])
>> +ovn_start
>> +
>> +# Logical network:
>> +#
>> +# Three logical switches ls1, ls2, ls3.
>> +# One logical router lr0 connected to ls[123],
>> +# with nine subnets, three per logical switch:
>> +#
>> +#    lrp11 on ls1 for subnet 192.168.11.0/24
>> +#    lrp12 on ls1 for subnet 192.168.12.0/24
>> +#    lrp13 on ls1 for subnet 192.168.13.0/24
>> +#    ...
>> +#    lrp33 on ls3 for subnet 192.168.33.0/24
>> +#
>> +# 27 VIFs, 9 per LS, 3 per subnet: lp[123][123][123], where the first two
>> +# digits are the subnet and the last digit distinguishes the VIF.
>> +#
>> +# This test will create two port groups and ACLs will be applied on them.
>> +
>> +get_lsp_uuid () {
>> +    ovn-nbctl lsp-list ls${1%??} | grep lp$1 | awk '{ print $1 }'
>> +}
>> +
>> +pg1_ports=
>> +pg2_ports=
>> +for i in 1 2 3; do
>> +    ovn-nbctl ls-add ls$i
>> +    for j in 1 2 3; do
>> +        for k in 1 2 3; do
>> +            ovn-nbctl \
>> +                -- lsp-add ls$i lp$i$j$k \
>> +                -- lsp-set-addresses lp$i$j$k "f0:00:00:00:0$i:$j$k \
>> +                   192.168.$i$j.$k"
>> +            # logical ports lp[12]?1 belongs to port group pg1
>> +            if test $i != 3 && test $k == 1; then
>> +                pg1_ports="$pg1_ports `get_lsp_uuid $i$j$k`"
>> +            fi
>> +            # logical ports lp[23]?2 belongs to port group pg2
>> +            if test $i != 1 && test $k == 2; then
>> +                pg2_ports="$pg2_ports `get_lsp_uuid $i$j$k`"
>> +            fi
>> +        done
>> +    done
>> +done
>> +
>> +ovn-nbctl lr-add lr0
>> +for i in 1 2 3; do
>> +    for j in 1 2 3; do
>> +        ovn-nbctl lrp-add lr0 lrp$i$j 00:00:00:00:ff:$i$j
>> 192.168.$i$j.254/24
>> +        ovn-nbctl \
>> +            -- lsp-add ls$i lrp$i$j-attachment \
>> +            -- set Logical_Switch_Port lrp$i$j-attachment type=router \
>> +                             options:router-port=lrp$i$j \
>> +                             addresses='"00:00:00:00:ff:'$i$j'"'
>> +    done
>> +done
>> +
>> +pg1_uuid=`ovn-nbctl create Port_Group name=pg1 ports="$pg1_ports"`
>> +ovn-nbctl create Port_Group name=pg2 ports="$pg2_ports"
>> +
>> +# create ACLs on pg1 to drop traffic from pg2 to pg1
>> +ovn-nbctl --id=@acl1 create acl priority=1001 direction=to-lport \
>> +        match='"outport == @pg1"' action=drop \
>> +        -- add port_group $pg1_uuid acls @acl1
>> +ovn-nbctl --id=@acl2 create acl priority=1002 direction=to-lport \
>> +        match='"outport == @pg1 && ip4.src == $pg2_ip4"'
>> action=allow-related \
>> +        -- add port_group $pg1_uuid acls @acl2
>> +
>> +# Physical network:
>> +#
>> +# Three hypervisors hv[123].
>> +# lp?1[123] spread across hv[123]: lp?11 on hv1, lp?12 on hv2, lp?13 on
>> hv3.
>> +# lp?2[123] spread across hv[23]: lp?21 and lp?22 on hv2, lp?23 on hv3.
>> +# lp?3[123] all on hv3.
>> +
>> +# Given the name of a logical port, prints the name of the hypervisor
>> +# on which it is located.
>> +vif_to_hv() {
>> +    case $1 in dnl (
>> +        ?11) echo 1 ;; dnl (
>> +        ?12 | ?21 | ?22) echo 2 ;; dnl (
>> +        ?13 | ?23 | ?3?) echo 3 ;;
>> +    esac
>> +}
>> +
>> +# Given the name of a logical port, prints the name of its logical router
>> +# port, e.g. "vif_to_lrp 123" yields 12.
>> +vif_to_lrp() {
>> +    echo ${1%?}
>> +}
>> +
>> +# Given the name of a logical port, prints the name of its logical
>> +# switch, e.g. "vif_to_ls 123" yields 1.
>> +vif_to_ls() {
>> +    echo ${1%??}
>> +}
>> +
>> +net_add n1
>> +for i in 1 2 3; do
>> +    sim_add hv$i
>> +    as hv$i
>> +    ovs-vsctl add-br br-phys
>> +    ovn_attach n1 br-phys 192.168.0.$i
>> +done
>> +for i in 1 2 3; do
>> +    for j in 1 2 3; do
>> +        for k in 1 2 3; do
>> +            hv=`vif_to_hv $i$j$k`
>> +                as hv$hv ovs-vsctl \
>> +                -- add-port br-int vif$i$j$k \
>> +                -- set Interface vif$i$j$k \
>> +                    external-ids:iface-id=lp$i$j$k \
>> +                    options:tx_pcap=hv$hv/vif$i$j$k-tx.pcap \
>> +                    options:rxq_pcap=hv$hv/vif$i$j$k-rx.pcap \
>> +                    ofport-request=$i$j$k
>> +        done
>> +    done
>> +done
>> +
>> +# 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).
>> +OVN_POPULATE_ARP
>> +
>> +# Allow some time for ovn-northd and ovn-controller to catch up.
>> +# XXX This should be more systematic.
>> +sleep 1
>> +
>> +# test_ip INPORT SRC_MAC DST_MAC SRC_IP DST_IP OUTPORT...
>> +#
>> +# This shell function causes a packet to be received on INPORT.  The
>> packet's
>> +# content has Ethernet destination DST and source SRC (each exactly 12
>> hex
>> +# digits) and Ethernet type ETHTYPE (4 hex digits).  The OUTPORTs (zero
>> or
>> +# more) list the VIFs on which the packet should be received.  INPORT
>> and the
>> +# OUTPORTs are specified as logical switch port numbers, e.g. 123 for
>> vif123.
>> +for i in 1 2 3; do
>> +    for j in 1 2 3; do
>> +        for k in 1 2 3; do
>> +            : > $i$j$k.expected
>> +        done
>> +    done
>> +done
>> +test_ip() {
>> +    # This packet has bad checksums but logical L3 routing doesn't check.
>> +    local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5
>> +    local packet=${dst_mac}${src_mac}08004500001c0000000040110000${src
>> _ip}${dst_ip}0035111100080000
>> +    shift; shift; shift; shift; shift
>> +    hv=hv`vif_to_hv $inport`
>> +    as $hv ovs-appctl netdev-dummy/receive vif$inport $packet
>> +    #as $hv ovs-appctl ofproto/trace br-int in_port=$inport $packet
>> +    in_ls=`vif_to_ls $inport`
>> +    in_lrp=`vif_to_lrp $inport`
>> +    for outport; do
>> +        out_ls=`vif_to_ls $outport`
>> +        if test $in_ls = $out_ls; then
>> +            # Ports on the same logical switch receive exactly the same
>> packet.
>> +            echo $packet
>> +        else
>> +            # Routing decrements TTL and updates source and dest MAC
>> +            # (and checksum).
>> +            out_lrp=`vif_to_lrp $outport`
>> +            echo f00000000${outport}00000000ff$
>> {out_lrp}08004500001c00000000"3f1101"00${src_ip}${dst_ip}0035111100080000
>> +        fi >> $outport.expected
>> +    done
>> +}
>> +
>> +as hv1 ovs-vsctl --columns=name,ofport list interface
>> +as hv1 ovn-sbctl list port_binding
>> +as hv1 ovn-sbctl list datapath_binding
>> +as hv1 ovn-sbctl list port_group
>> +as hv1 ovn-sbctl list address_set
>> +as hv1 ovn-sbctl dump-flows
>> +as hv1 ovs-ofctl dump-flows br-int
>> +
>> +# Send IP packets between all pairs of source and destination ports,
>> +# packets matches ACL1 but not ACL2 should be dropped
>> +ip_to_hex() {
>> +    printf "%02x%02x%02x%02x" "$@"
>> +}
>> +for is in 1 2 3; do
>> +  for js in 1 2 3; do
>> +    for ks in 1 2 3; do
>> +      bcast=
>> +      s=$is$js$ks
>> +      smac=f00000000$s
>> +      sip=`ip_to_hex 192 168 $is$js $ks`
>> +      for id in 1 2 3; do
>> +          for jd in 1 2 3; do
>> +              for kd in 1 2 3; do
>> +                d=$id$jd$kd
>> +                dip=`ip_to_hex 192 168 $id$jd $kd`
>> +                if test $is = $id; then dmac=f00000000$d; else
>> dmac=00000000ff$is$js; fi
>> +                if test $d != $s; then unicast=$d; else unicast=; fi
>> +
>> +                # packets matches ACL1 but not ACL2 should be dropped
>> +                if test $id != 3 && test $kd == 1; then
>> +                    if test $is == 1 || test $ks != 2; then
>> +                        unicast=
>> +                    fi
>> +                fi
>> +                test_ip $s $smac $dmac $sip $dip $unicast #1
>> +              done
>> +          done
>> +        done
>> +      done
>> +  done
>> +done
>> +
>> +# Allow some time for packet forwarding.
>> +# XXX This can be improved.
>> +sleep 1
>> +
>> +# Now check the packets actually received against the ones expected.
>> +for i in 1 2 3; do
>> +    for j in 1 2 3; do
>> +        for k in 1 2 3; do
>> +            OVN_CHECK_PACKETS([hv`vif_to_hv $i$j$k`/vif$i$j$k-tx.pcap],
>> +                              [$i$j$k.expected])
>> +        done
>> +    done
>> +done
>> +
>> +# Gracefully terminate daemons
>> +OVN_CLEANUP([hv1], [hv2], [hv3])
>> +AT_CLEANUP
>> --
>> 2.1.0
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
>

On Sun, Apr 22, 2018 at 09:52:34AM -0700, Han Zhou wrote:
> Although port group can be used in match conditions of ACLs, it is
> still inconvenient for clients to figure out the lswitches that each
> ACL should be applied to.
> 
> This patch supports applying ACLs to port groups directly instead of
> applying to each related lswitch individually. It provides convenience
> for clients such as k8s and OpenStack Neutron.
> 
> Requested-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-March/344856.html
> Requested-by: Guru Shetty <guru@ovn.org>
> Requested-by: Daniel Alvarez Sanchez <dalvarez@redhat.com>
> Signed-off-by: Han Zhou <hzhou8@ebay.com>

Thanks a lot.  I applied this to master.