Wow great job Han! I'll take a look ASAP, this is really useful indeed. Thanks! Daniel
On Sun, Apr 22, 2018 at 7:17 PM, Han Zhou <zhou...@gmail.com> wrote: > > > On Fri, Mar 2, 2018 at 7:26 AM, Guru Shetty <g...@ovn.org> wrote: > > > > > > > > On 1 March 2018 at 15:43, Han Zhou <zhou...@gmail.com> wrote: > >> > >> > >> > >> On Thu, Mar 1, 2018 at 12:26 PM, Guru Shetty <g...@ovn.org> wrote: > >> > > >> > > >> > > >> > On 1 March 2018 at 12:21, Han Zhou <zhou...@gmail.com> wrote: > >> >> > >> >> > >> >> > >> >> On Thu, Mar 1, 2018 at 12:13 PM, Guru Shetty <g...@ovn.org> wrote: > >> >> > > >> >> > > >> >> > > >> >> > On 28 February 2018 at 19:37, Han Zhou <zhou...@gmail.com> wrote: > >> >> >> > >> >> >> This patch enables using port group names in ACL match conditions. > >> >> >> Users can create a port group in northbound DB Port_Group table, > >> >> >> and then use the name of the port group in ACL match conditions > >> >> >> for "inport" or "outport". It can help reduce the number of ACLs > >> >> >> for CMS clients such as OpenStack Neutron, for the use cases > >> >> >> where a group of logical ports share same ACL rules except the > >> >> >> "inport"/"outport" part. Without this patch, the clients have to > >> >> >> create N (N = number of lports) ACLs, and this patch helps achieve > >> >> >> the same goal with only one ACL. E.g.: > >> >> >> > >> >> >> to-lport 1000 "outport == @port_group1 && ip4.src == {IP1, IP2, > ...}" allow-related > >> >> >> > >> >> >> There was a similar attempt by Zong Kai Li in 2016 [1]. This patch > >> >> >> takes a slightly different approach by using weak refs instead of > >> >> >> strings, which requires a new table instead of reusing the address > >> >> >> set table. This way it will also benefit for a follow up patch > that > >> >> >> enables generating address sets automatically from port groups to > >> >> >> avoid a lot a trouble from client perspective [2]. > >> >> >> > >> >> >> [1] https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/ > 077118.html > >> >> >> [2] https://mail.openvswitch.org/pipermail/ovs-discuss/2018- > February/046260.html > >> >> >> > >> >> >> Reported-by: Daniel Alvarez Sanchez <dalva...@redhat.com> > >> >> >> Reported-at: https://mail.openvswitch.org/ > pipermail/ovs-discuss/2018-February/046166.html > >> >> >> Signed-off-by: Han Zhou <hzh...@ebay.com> > >> >> > > >> >> > > >> >> > Wouldn't it be more complete and useful if we add the acl to a > port group too? And then internally, you decide which switches you want to > add the ACL to. > >> >> > > >> >> > For e.g: ovn-nbctl --port-group add-acl port_group1 to-lport 1000 > "outport == @port_group1 && ip4.src == {IP1, IP2, ...}" allow-related > >> >> > > >> >> > This way, the client does not have to keep track of all the > logical switches it needs to apply an ACL to. Thoughts? > >> >> > > >> >> Yes, this is a good idea. Since it is only about the ovn-nbctl tool > improvement, it can be a follow up patch. > >> > > >> > > >> > What if we have something like a acl column in the port_group table > so that we just have one entry in OVN NB database? Logically, we apply a > ACL to a security group instead of a logical switch. And then ovn-northd > decided which logical switches to apply it to. Would that make difference > in performance? It does reduce the size of the NB database. Any drawbacks? > >> > > >> Ok, I thought you were talking about ovn-nbctl tool only. Now I get > your point. I think it is a good idea, since it is a common work for > different clients: figuring out which lswitches are needed for each group > of ACLs. > > > > Right. And sending in multiple ACLs and deleting multiple ACLs instead > of just one ACL with this approach. > > > > > >> > >> So it makes sense to simplify clients implementation and support the > feature in OVN. I think it would be better to have 2 columns for ACLs on > port-groups, one for to-lports, the other for from-lports. And the match > condition "outport/inport == @<port group name>" should be automatically > added by northd when processing, instead of filling in the redundant > information by clients. Would this sounds better? > > > > > > I don't have a strong opinion either way. Doing as you suggest makes it > simpler, but probably a little harder explaining in documentation as there > is a general difference with lswitch ACL. > > > >> > >> > >> This should be able to work without breaking the existing mechanism of > specifying ACLs in lswitches. So existing ACL users should not be affected. > > > > Agreed. > > > >> > >> > >> Performance could be better in clients (networking-ovn, kubernetes-ovn, > etc.), since there is no need to figuring out the lswitch list to apply > ACLs, although not sure how much improvement it could be. > > > > For ovn-kubernetes, it makes quite a bit of difference as we don't need > to send ACL addition to multiple switches (which can be as many nodes in > the cluster) > > > > > >> > >> Performance impact from ovn-northd perspective is not sure, because > there are less data to process from OVN-NB, but more processing needed for > the port-group attached ACLs handling. > >> > >> I can work on it as a follow up patch on top of the current > implementation. > > > > Thanks! Looking forward to it! > > > > > > I just submitted the patch for this: https://patchwork.ozlabs.org/ > project/openvswitch/list/?series=40294 > > Guru, I did it the way you suggested (instead of my proposal of > automatically generating outport/inport) because I find it more consistent > with the current way. > Daniel, this addresses the same you mentioned at: > https://mail.openvswitch.org/pipermail/ovs-dev/2018-March/345145.html > > Please take a look. > > Thanks, > Han > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev