[ovs-dev] [PATCH] ovn-northd: Apply pre ACLs when using Port Groups

Han Zhou zhouhan at gmail.com
Wed Jun 20 00:46:17 UTC 2018


On Tue, Jun 19, 2018 at 5:27 PM, Han Zhou <zhouhan at gmail.com> wrote:
>
>
>
> On Tue, Jun 19, 2018 at 3:46 PM, Daniel Alvarez <dalvarez at redhat.com>
wrote:
> >
> > When using Port Groups, the pre ACLs were not applied so the
> > conntrack action was not performed. This patch takes Port Groups
> > into account when processing the pre ACLs.
> >
> > Signed-off-by: Daniel Alvarez <dalvarez at redhat.com>
> > ---
> >  ovn/northd/ovn-northd.c | 100
+++++++++++++++++++++++++++---------------------
> >  1 file changed, 56 insertions(+), 44 deletions(-)
> >
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index 72fe4e795..818ac59fa 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -2835,8 +2835,47 @@ build_dhcpv6_action(struct ovn_port *op, struct
in6_addr *offer_ip,
> >      return true;
> >  }
> >
> > +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 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 bool
> > -has_stateful_acl(struct ovn_datapath *od)
> > +has_stateful_acl(struct ovn_datapath *od, struct hmap *port_groups)
> >  {
> >      for (size_t i = 0; i < od->nbs->n_acls; i++) {
> >          struct nbrec_acl *acl = od->nbs->acls[i];
> > @@ -2845,13 +2884,25 @@ has_stateful_acl(struct ovn_datapath *od)
> >          }
> >      }
> >
> > +    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++) {
> > +                struct nbrec_acl *acl = pg->acls[i];
> > +                if (!strcmp(acl->action, "allow-related")) {
> > +                    return true;
> > +                }
> > +            }
> > +        }
> > +    }
> >      return false;
> >  }
> >
> >  static void
> > -build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
> > +build_pre_acls(struct ovn_datapath *od, struct hmap *lflows,
> > +               struct hmap *port_groups)
> >  {
> > -    bool has_stateful = has_stateful_acl(od);
> > +    bool has_stateful = has_stateful_acl(od, port_groups);
> >
> >      /* Ingress and Egress Pre-ACL Table (Priority 0): Packets are
> >       * allowed by default. */
> > @@ -3309,21 +3360,6 @@ consider_acl(struct hmap *lflows, struct
ovn_datapath *od,
> >      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)
> > @@ -3338,30 +3374,6 @@ ovn_port_group_create(struct hmap *pgs,
> >      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)
> >  {
> > @@ -3416,7 +3428,7 @@ static void
> >  build_acls(struct ovn_datapath *od, struct hmap *lflows,
> >             struct hmap *port_groups)
> >  {
> > -    bool has_stateful = has_stateful_acl(od);
> > +    bool has_stateful = has_stateful_acl(od, port_groups);
> >
> >      /* Ingress and Egress ACL Table (Priority 0): Packets are allowed
by
> >       * default.  A related rule at priority 1 is added below if there
> > @@ -3769,7 +3781,7 @@ build_lswitch_flows(struct hmap *datapaths,
struct hmap *ports,
> >              continue;
> >          }
> >
> > -        build_pre_acls(od, lflows);
> > +        build_pre_acls(od, lflows, port_groups);
> >          build_pre_lb(od, lflows);
> >          build_pre_stateful(od, lflows);
> >          build_acls(od, lflows, port_groups);
> > --
> > 2.15.1 (Apple Git-101)
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> Thanks for the fix!
>
> All looks good to me except that the test case "ovn -- ACLs on Port
Groups" is broken with this change. I think it is because conntrack is not
supported in the dummy datapath and so the stateful ACL would not work in
the test suites, and it was passing just because of this bug. So, to fix
the test case, you need below change:
> -------------><8--------------------8><----------
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 6553d17..93644b0 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -9981,7 +9981,7 @@ ovn-nbctl create Port_Group name=pg2
ports="$pg2_ports"
>  # create ACLs on pg1 to drop traffic from pg2 to pg1
>  ovn-nbctl acl-add pg1 to-lport 1001 'outport == @pg1' drop
>  ovn-nbctl --type=port-group acl-add pg1 to-lport 1002 \
> -        'outport == @pg1 && ip4.src == $pg2_ip4' allow-related
> +        'outport == @pg1 && ip4.src == $pg2_ip4' allow
>
>  # Physical network:
>  #
>
>
> Thanks,
> Han

One more thing, it may be worth to add a hash index from lswitch to
port-groups so that the look up in has_stateful_acl() can be more
efficient, when there are big number of port groups (e.g. in OpenStack each
tenant has its own default group). Of course this can be a follow up
improvement.


More information about the dev mailing list