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

Han Zhou zhouhan at gmail.com
Sun Apr 22 17:17:42 UTC 2018


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


More information about the dev mailing list