| Message ID | 20180620021859.20503-1-dalvarez@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] ovn-northd: Apply pre ACLs when using Port Groups | expand |
On Wed, Jun 20, 2018 at 3:18 AM, Daniel Alvarez <dalvarez@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. > > As a follow up, we could enhance this patch by creating an index > from lswitch to port groups. > > Signed-off-by: Daniel Alvarez <dalvarez@redhat.com> > --- > ovn/northd/ovn-northd.c | 100 +++++++++++++++++++++++++++--------------------- > tests/ovn.at | 2 +- > 2 files changed, 57 insertions(+), 45 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); > diff --git a/tests/ovn.at b/tests/ovn.at > index 6553d17c6..93644b023 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: > # > -- > 2.15.1 (Apple Git-101) > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Acked-by: Lucas Alvares Gomes <lucasagomes@gmail.com> Thanks for the patch, I just pulled it in a test environment along with the port group support series for networking-ovn [0], gave it a go and it works. [stack@host11 devstack]$ sudo ip net e ovnmeta-7b7909ae-b000-4196-9433-6a6dea0f352e ssh cirros@10.0.0.3 The authenticity of host '10.0.0.3 (10.0.0.3)' can't be established. RSA key fingerprint is SHA256:hECvgpUHbfO+hrwfOFmh6DYZxTL0p8gjBPkl8BwSNxA. RSA key fingerprint is MD5:5c:12:10:b4:ca:a6:33:cc:52:8b:9b:06:c1:28:4f:2c. Are you sure you want to continue connecting (yes/no)? yes Warning: Permanently added '10.0.0.3' (RSA) to the list of known hosts. cirros@10.0.0.3's password: $ curl 169.254.169.254 1.0 2007-01-19 2007-03-01 2007-08-29 2007-10-10 2007-12-15 2008-02-01 2008-09-01 2009-04-04 latest$ [0] https://review.openstack.org/#/c/570186/
On Tue, Jun 19, 2018 at 7:18 PM, Daniel Alvarez <dalvarez@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. > > As a follow up, we could enhance this patch by creating an index > from lswitch to port groups. > > Signed-off-by: Daniel Alvarez <dalvarez@redhat.com> > --- > ovn/northd/ovn-northd.c | 100 +++++++++++++++++++++++++++--------------------- > tests/ovn.at | 2 +- > 2 files changed, 57 insertions(+), 45 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); > diff --git a/tests/ovn.at b/tests/ovn.at > index 6553d17c6..93644b023 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: > # > -- > 2.15.1 (Apple Git-101) > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Acked-by: Han Zhou <hzhou8@ebay.com> p.s. I added a follow up patch to ensure the test coverage for the bug: https://patchwork.ozlabs.org/patch/933050/ Please review it, too.
On Wed, Jun 20, 2018 at 04:18:59AM +0200, Daniel Alvarez 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. > > As a follow up, we could enhance this patch by creating an index > from lswitch to port groups. > > Signed-off-by: Daniel Alvarez <dalvarez@redhat.com> Thanks. I applied this to master. If you want it backported, let me know.
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); diff --git a/tests/ovn.at b/tests/ovn.at index 6553d17c6..93644b023 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: #
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. As a follow up, we could enhance this patch by creating an index from lswitch to port groups. Signed-off-by: Daniel Alvarez <dalvarez@redhat.com> --- ovn/northd/ovn-northd.c | 100 +++++++++++++++++++++++++++--------------------- tests/ovn.at | 2 +- 2 files changed, 57 insertions(+), 45 deletions(-)