| Message ID | 20180619224602.18603-1-dalvarez@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] ovn-northd: Apply pre ACLs when using Port Groups | expand |
On Tue, Jun 19, 2018 at 3:46 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. > > Signed-off-by: Daniel Alvarez <dalvarez@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@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
On Tue, Jun 19, 2018 at 5:27 PM, Han Zhou <zhouhan@gmail.com> wrote: > > > > On Tue, Jun 19, 2018 at 3:46 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. > > > > Signed-off-by: Daniel Alvarez <dalvarez@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@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.
On Tue, Jun 19, 2018 at 05:27:20PM -0700, Han Zhou wrote: > 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: I would have guessed that conntrack works OK in the dummy datapath because dpif-netdev supports conntrack.
On Tue, Jun 19, 2018 at 5:49 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Tue, Jun 19, 2018 at 05:27:20PM -0700, Han Zhou wrote: > > 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: > > I would have guessed that conntrack works OK in the dummy datapath > because dpif-netdev supports conntrack. Ah, I admit that I am ignorant on this. I need to study more on it to understand why this test case doesn't work. Is there any tool/documentation/example on how to debug the dummy datapath conntrack, such as dumping the conntrack table entries? Thanks, Han
Thanks a lot Han for the review. Just sent the v2 with the test fixed. I'll leave the hash index for a follow up as I'm short in time but if you want to edit my patch feel free to do it or send another one. Thanks again for the Port Groups implementation! :) Cheers, Daniel On Wed, Jun 20, 2018 at 4:15 AM, Han Zhou <zhouhan@gmail.com> wrote: > > > On Tue, Jun 19, 2018 at 5:49 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > On Tue, Jun 19, 2018 at 05:27:20PM -0700, Han Zhou wrote: > > > 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: > > > > I would have guessed that conntrack works OK in the dummy datapath > > because dpif-netdev supports conntrack. > > Ah, I admit that I am ignorant on this. I need to study more on it to > understand why this test case doesn't work. Is there any > tool/documentation/example on how to debug the dummy datapath conntrack, > such as dumping the conntrack table entries? > > Thanks, > Han >
On Tue, Jun 19, 2018 at 7:15 PM, Han Zhou <zhouhan@gmail.com> wrote: > > > > On Tue, Jun 19, 2018 at 5:49 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > On Tue, Jun 19, 2018 at 05:27:20PM -0700, Han Zhou wrote: > > > 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: > > > > I would have guessed that conntrack works OK in the dummy datapath > > because dpif-netdev supports conntrack. > > Ah, I admit that I am ignorant on this. I need to study more on it to understand why this test case doesn't work. Is there any tool/documentation/example on how to debug the dummy datapath conntrack, such as dumping the conntrack table entries? > I checked more on this. I didn't find any tools to debug the user space conntrack module, but I found "ovs-appctl dpif/dump-flows" useful for debugging the dummy datapath. By dumping flow it shows the packets are dropped because the conntrack state for the packets are marked as "invalid". It is marked as invalid because the checksum we used in the test packets are all 0s and conn_key_extract() returns false. This is why stateful ACL seems not working in test cases, and probably this is why none of the ACL test cases were actually testing "allow-related". conn_key_extract() checks both L3 and L4 checksums. I tried with a single ICMP request packet with correct IP header checksum and ICMP checksum, and the packet went through the conntrack and was received successfully. So I think there are 2 alternatives to test stateful ACLs in test cases. option1: write a script to generate proper checksums, and use it in test cases when correct checksum is required option2: add an option to the dummy datapath to bypass checksum validation in connection tracking I will try option1 first, but any suggestions are welcome. Thanks, Han
On Thu, Jun 21, 2018 at 12:07 AM, Han Zhou <zhouhan@gmail.com> wrote: > > > > On Tue, Jun 19, 2018 at 7:15 PM, Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > On Tue, Jun 19, 2018 at 5:49 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > > > On Tue, Jun 19, 2018 at 05:27:20PM -0700, Han Zhou wrote: > > > > 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: > > > > > > I would have guessed that conntrack works OK in the dummy datapath > > > because dpif-netdev supports conntrack. > > > > Ah, I admit that I am ignorant on this. I need to study more on it to understand why this test case doesn't work. Is there any tool/documentation/example on how to debug the dummy datapath conntrack, such as dumping the conntrack table entries? > > > I checked more on this. I didn't find any tools to debug the user space conntrack module, but I found "ovs-appctl dpif/dump-flows" useful for debugging the dummy datapath. By dumping flow it shows the packets are dropped because the conntrack state for the packets are marked as "invalid". It is marked as invalid because the checksum we used in the test packets are all 0s and conn_key_extract() returns false. This is why stateful ACL seems not working in test cases, and probably this is why none of the ACL test cases were actually testing "allow-related". > > conn_key_extract() checks both L3 and L4 checksums. I tried with a single ICMP request packet with correct IP header checksum and ICMP checksum, and the packet went through the conntrack and was received successfully. So I think there are 2 alternatives to test stateful ACLs in test cases. > > option1: write a script to generate proper checksums, and use it in test cases when correct checksum is required > option2: add an option to the dummy datapath to bypass checksum validation in connection tracking > > I will try option1 first, but any suggestions are welcome. > As pointed out by Justin in today's ovn meeting, the "ovs-appctl -t ovn-controller inject-pkt" already takes care of checksum calculation, which can be used to replace the dummy/receive tool in tests when checksum is needed. So I tried this approach and it works pretty well: https://patchwork.ozlabs.org/patch/933050/ Thanks Justin!
> On Jun 21, 2018, at 7:45 PM, Han Zhou <zhouhan@gmail.com> wrote: > > As pointed out by Justin in today's ovn meeting, the "ovs-appctl -t ovn-controller inject-pkt" already takes care of checksum calculation, which can be used to replace the dummy/receive tool in tests when checksum is needed. > So I tried this approach and it works pretty well: https://patchwork.ozlabs.org/patch/933050/ Great! At the time I added that, I'd thought about updating the OVN tests to use the mechanism, since it's much easier to read than the hex representation. Unfortunately, I didn't have a chance, but I just added it back as a low-priority item on an internal to-do list I'm maintaining. Thanks for following up and letting me know it worked. --Justin
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);
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@redhat.com> --- ovn/northd/ovn-northd.c | 100 +++++++++++++++++++++++++++--------------------- 1 file changed, 56 insertions(+), 44 deletions(-)