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

[Han Zhou]

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.
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?

This should be able to work without breaking the existing mechanism of
specifying ACLs in lswitches. So existing ACL users should not be affected.

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.
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.