[ovs-dev] [PATCH 1/2] ovn: Support port groups in ACLs

Mon Apr 23 08:16:22 UTC 2018

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 <zhouhan at gmail.com> wrote:

>
>
> On Fri, Mar 2, 2018 at 7:26 AM, Guru Shetty <guru at ovn.org> wrote:
> >
> >
> >
> > On 1 March 2018 at 15:43, Han Zhou <zhouhan at gmail.com> wrote:
> >>
> >>
> >>
> >> On Thu, Mar 1, 2018 at 12:26 PM, Guru Shetty <guru at ovn.org> wrote:
> >> >
> >> >
> >> >
> >> > On 1 March 2018 at 12:21, Han Zhou <zhouhan at gmail.com> wrote:
> >> >>
> >> >>
> >> >>
> >> >> On Thu, Mar 1, 2018 at 12:13 PM, Guru Shetty <guru at ovn.org> wrote:
> >> >> >
> >> >> >
> >> >> >
> >> >> > On 28 February 2018 at 19:37, Han Zhou <zhouhan at 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 <dalvarez at redhat.com>
> >> >> >> Reported-at: https://mail.openvswitch.org/
> pipermail/ovs-discuss/2018-February/046166.html
> >> >> >> Signed-off-by: Han Zhou <hzhou8 at 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
>
>