Re: [ovs-dev] [PATCH 3/3] ovn: Add address_set() support for ACLs.

[Russell Bryant]

On Tue, Apr 12, 2016 at 2:17 PM, Han Zhou <zhou...@gmail.com> wrote:

>
>
> On Tue, Apr 12, 2016 at 11:02 AM, Russell Bryant <russ...@ovn.org> wrote:
> >
> > On Tue, Apr 12, 2016 at 1:35 PM, Mickey Spiegel <emspi...@us.ibm.com>
> wrote:
> >
> > > One comment below.
> > >
> > > -"dev" <dev-boun...@openvswitch.org> wrote: -
> > >
> > > >To: Ben Pfaff <b...@ovn.org>
> > > >From: Russell Bryant
> > > >Sent by: "dev"
> > > >Date: 04/12/2016 09:37AM
> > > >Cc: ovs dev <dev@openvswitch.org>
> > > >Subject: Re: [ovs-dev] [PATCH 3/3] ovn: Add address_set() support for
> > > >ACLs.
> > > >
> > >
> > > >On Mon, Apr 11, 2016 at 12:08 PM, Ben Pfaff <b...@ovn.org> wrote:
> > > >
> > > >> On Tue, Apr 05, 2016 at 05:24:19PM -0400, Russell Bryant wrote:
> > > >> > This feature was originally proposed here:
> > > >> >
> > > >> > http://openvswitch.org/pipermail/dev/2016-March/067440.html
> > > >> >
> > > >> > A common use case for OVN ACLs involves needing to match a set of
> IP
> > > >> > addresses.
> > > >> >
> > > >> > outport == "lp1" && ip4.src == {10.0.0.5, 10.0.0.25, 10.0.0.50}
> > > >> >
> > > >> > This example match only has 3 addresses, but it could easily have
> > > >> > hundreds of addresses. In some cases, the same large set of
> addresses
> > > >> > needs to be used in several ACLs.
> > > >> >
> > > >> > This patch adds a new Address_Set table to OVN_Northbound so that
> a
> > > set
> > > >> > of addresses can be specified once and then referred to by name in
> > > ACLs.
> > > >> > To recreate the above example, you would first create an address
> set:
> > > >> >
> > > >> > $ ovn-nbctl create Address_Set name=set1
> > > >> addresses=10.0.0.5,10.0.0.25,10.0.0.50
> > > >> >
> > > >> > Then you can refer to this address set by name in an ACL match:
> > > >> >
> > > >> > outport == "lp1" && ip4.src == address_set(set1)
> > > >> >
> > > >> > Signed-off-by: Russell Bryant <russ...@ovn.org>
> > > >>
> > > >> It might be worth making address sets scoped somehow. Otherwise it
> > > >> might eventually become a bottleneck if there are many address sets
> that
> > > >> a given hypervisor does not need to know about.
> > > >>
> > > >
> > > >Good point.
> > > >
> > > >The primary use case I have for this is to use in the match column of
> OVN
> > > >ACLs. ACLs are scoped to logical switches, so we could do the same for
> > > >address sets. Having them scoped equally makes sense to me.
> > > >
> > > >The only downside I can think of is that it will cause OpenStack to
> have
> > > to
> > > >duplicate the same address set across multiple logical switches in
> some
> > > >cases. I think that's probably OK though. It's still a huge
> improvement
> > > >over what it does today.
> > >
> > > Thinking of the use of remote_group_id in OpenStack security groups, I
> > > believe it would be common for the same address set to be used in all
> > > logical switches that are connected to the same logical router. Since
> there
> > > is no notion of project in OVN, it is not so obvious what to scope
> address
> > > sets to.
> > >
> >
> > Yes, that's right.  I was just assuming that the common case was not
> having
> > having that many networks, so that we still got a significant
> improvement.
> >
> > One option would be to skip trying to scope it for now and revisit if it
> > becomes a bottleneck.
> >
>
> Agree, it would be better not scope address set to lswitches.
>
> >
> > > Would it be good enough to have controllers check to see if the address
> > > set is used by any of the local logical switches?
> > >
> >
> > That's not easy to do.  That check means scanning all logical flows to
> see
> > if the address set is referenced.  We need to load all address sets
> before
> > processing logical flows so that we can actually make sense of the
> address
> > set reference when we hit one.
> >
>
> Since we will know which acls are using an address set, and acls are
> scoped to lswitches, then we can deduce the lswitches that uses an address
> set. How about storing this information in a new column of address set
> table in SB, so that ovn-controller can utilize this information for
> optimization? (Of course this is not required in the first version).
>

That's a clever idea.  I'll think about that some more when working on the
next revision.

-- 
Russell Bryant
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/3] ovn: Add address_set() support for ACLs.

[Ben Pfaff]

On Tue, Apr 12, 2016 at 11:17:30AM -0700, Han Zhou wrote:
> On Tue, Apr 12, 2016 at 11:02 AM, Russell Bryant  wrote:
> >
> > On Tue, Apr 12, 2016 at 1:35 PM, Mickey Spiegel 
> wrote:
> > Yes, that's right.  I was just assuming that the common case was not
> having
> > having that many networks, so that we still got a significant improvement.
> >
> > One option would be to skip trying to scope it for now and revisit if it
> > becomes a bottleneck.
> >
> 
> Agree, it would be better not scope address set to lswitches.

I'm fine with postponing the idea of scoping address sets.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/3] ovn: Add address_set() support for ACLs.

[Han Zhou]

On Tue, Apr 12, 2016 at 11:02 AM, Russell Bryant <russ...@ovn.org> wrote:
>
> On Tue, Apr 12, 2016 at 1:35 PM, Mickey Spiegel <emspi...@us.ibm.com>
wrote:
>
> > One comment below.
> >
> > -"dev" <dev-boun...@openvswitch.org> wrote: -
> >
> > >To: Ben Pfaff <b...@ovn.org>
> > >From: Russell Bryant
> > >Sent by: "dev"
> > >Date: 04/12/2016 09:37AM
> > >Cc: ovs dev <dev@openvswitch.org>
> > >Subject: Re: [ovs-dev] [PATCH 3/3] ovn: Add address_set() support for
> > >ACLs.
> > >
> >
> > >On Mon, Apr 11, 2016 at 12:08 PM, Ben Pfaff <b...@ovn.org> wrote:
> > >
> > >> On Tue, Apr 05, 2016 at 05:24:19PM -0400, Russell Bryant wrote:
> > >> > This feature was originally proposed here:
> > >> >
> > >> > http://openvswitch.org/pipermail/dev/2016-March/067440.html
> > >> >
> > >> > A common use case for OVN ACLs involves needing to match a set of
IP
> > >> > addresses.
> > >> >
> > >> > outport == "lp1" && ip4.src == {10.0.0.5, 10.0.0.25, 10.0.0.50}
> > >> >
> > >> > This example match only has 3 addresses, but it could easily have
> > >> > hundreds of addresses. In some cases, the same large set of
addresses
> > >> > needs to be used in several ACLs.
> > >> >
> > >> > This patch adds a new Address_Set table to OVN_Northbound so that a
> > set
> > >> > of addresses can be specified once and then referred to by name in
> > ACLs.
> > >> > To recreate the above example, you would first create an address
set:
> > >> >
> > >> > $ ovn-nbctl create Address_Set name=set1
> > >> addresses=10.0.0.5,10.0.0.25,10.0.0.50
> > >> >
> > >> > Then you can refer to this address set by name in an ACL match:
> > >> >
> > >> > outport == "lp1" && ip4.src == address_set(set1)
> > >> >
> > >> > Signed-off-by: Russell Bryant <russ...@ovn.org>
> > >>
> > >> It might be worth making address sets scoped somehow. Otherwise it
> > >> might eventually become a bottleneck if there are many address sets
that
> > >> a given hypervisor does not need to know about.
> > >>
> > >
> > >Good point.
> > >
> > >The primary use case I have for this is to use in the match column of
OVN
> > >ACLs. ACLs are scoped to logical switches, so we could do the same for
> > >address sets. Having them scoped equally makes sense to me.
> > >
> > >The only downside I can think of is that it will cause OpenStack to
have
> > to
> > >duplicate the same address set across multiple logical switches in some
> > >cases. I think that's probably OK though. It's still a huge improvement
> > >over what it does today.
> >
> > Thinking of the use of remote_group_id in OpenStack security groups, I
> > believe it would be common for the same address set to be used in all
> > logical switches that are connected to the same logical router. Since
there
> > is no notion of project in OVN, it is not so obvious what to scope
address
> > sets to.
> >
>
> Yes, that's right.  I was just assuming that the common case was not
having
> having that many networks, so that we still got a significant improvement.
>
> One option would be to skip trying to scope it for now and revisit if it
> becomes a bottleneck.
>

Agree, it would be better not scope address set to lswitches.

>
> > Would it be good enough to have controllers check to see if the address
> > set is used by any of the local logical switches?
> >
>
> That's not easy to do.  That check means scanning all logical flows to see
> if the address set is referenced.  We need to load all address sets before
> processing logical flows so that we can actually make sense of the address
> set reference when we hit one.
>

Since we will know which acls are using an address set, and acls are scoped
to lswitches, then we can deduce the lswitches that uses an address set.
How about storing this information in a new column of address set table in
SB, so that ovn-controller can utilize this information for optimization?
(Of course this is not required in the first version).

> --
> Russell Bryant
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev




--
Best regards,
Han
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/3] ovn: Add address_set() support for ACLs.

[Russell Bryant]

On Tue, Apr 12, 2016 at 1:35 PM, Mickey Spiegel <emspi...@us.ibm.com> wrote:

> One comment below.
>
> -"dev" <dev-boun...@openvswitch.org> wrote: -
>
> >To: Ben Pfaff <b...@ovn.org>
> >From: Russell Bryant
> >Sent by: "dev"
> >Date: 04/12/2016 09:37AM
> >Cc: ovs dev <dev@openvswitch.org>
> >Subject: Re: [ovs-dev] [PATCH 3/3] ovn: Add address_set() support for
> >ACLs.
> >
>
> >On Mon, Apr 11, 2016 at 12:08 PM, Ben Pfaff <b...@ovn.org> wrote:
> >
> >> On Tue, Apr 05, 2016 at 05:24:19PM -0400, Russell Bryant wrote:
> >> > This feature was originally proposed here:
> >> >
> >> > http://openvswitch.org/pipermail/dev/2016-March/067440.html
> >> >
> >> > A common use case for OVN ACLs involves needing to match a set of IP
> >> > addresses.
> >> >
> >> > outport == "lp1" && ip4.src == {10.0.0.5, 10.0.0.25, 10.0.0.50}
> >> >
> >> > This example match only has 3 addresses, but it could easily have
> >> > hundreds of addresses. In some cases, the same large set of addresses
> >> > needs to be used in several ACLs.
> >> >
> >> > This patch adds a new Address_Set table to OVN_Northbound so that a
> set
> >> > of addresses can be specified once and then referred to by name in
> ACLs.
> >> > To recreate the above example, you would first create an address set:
> >> >
> >> > $ ovn-nbctl create Address_Set name=set1
> >> addresses=10.0.0.5,10.0.0.25,10.0.0.50
> >> >
> >> > Then you can refer to this address set by name in an ACL match:
> >> >
> >> > outport == "lp1" && ip4.src == address_set(set1)
> >> >
> >> > Signed-off-by: Russell Bryant <russ...@ovn.org>
> >>
> >> It might be worth making address sets scoped somehow. Otherwise it
> >> might eventually become a bottleneck if there are many address sets that
> >> a given hypervisor does not need to know about.
> >>
> >
> >Good point.
> >
> >The primary use case I have for this is to use in the match column of OVN
> >ACLs. ACLs are scoped to logical switches, so we could do the same for
> >address sets. Having them scoped equally makes sense to me.
> >
> >The only downside I can think of is that it will cause OpenStack to have
> to
> >duplicate the same address set across multiple logical switches in some
> >cases. I think that's probably OK though. It's still a huge improvement
> >over what it does today.
>
> Thinking of the use of remote_group_id in OpenStack security groups, I
> believe it would be common for the same address set to be used in all
> logical switches that are connected to the same logical router. Since there
> is no notion of project in OVN, it is not so obvious what to scope address
> sets to.
>

Yes, that's right.  I was just assuming that the common case was not having
having that many networks, so that we still got a significant improvement.

One option would be to skip trying to scope it for now and revisit if it
becomes a bottleneck.


> Would it be good enough to have controllers check to see if the address
> set is used by any of the local logical switches?
>

That's not easy to do.  That check means scanning all logical flows to see
if the address set is referenced.  We need to load all address sets before
processing logical flows so that we can actually make sense of the address
set reference when we hit one.

-- 
Russell Bryant
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/3] ovn: Add address_set() support for ACLs.

[Mickey Spiegel]

One comment below.

-"dev" <dev-boun...@openvswitch.org> wrote: -

>To: Ben Pfaff <b...@ovn.org>
>From: Russell Bryant 
>Sent by: "dev" 
>Date: 04/12/2016 09:37AM
>Cc: ovs dev <dev@openvswitch.org>
>Subject: Re: [ovs-dev] [PATCH 3/3] ovn: Add address_set() support for
>ACLs.
>
>On Mon, Apr 11, 2016 at 12:08 PM, Ben Pfaff <b...@ovn.org> wrote:
>
>> On Tue, Apr 05, 2016 at 05:24:19PM -0400, Russell Bryant wrote:
>> > This feature was originally proposed here:
>> >
>> >   http://openvswitch.org/pipermail/dev/2016-March/067440.html
>> >
>> > A common use case for OVN ACLs involves needing to match a set of IP
>> > addresses.
>> >
>> >outport == "lp1" && ip4.src == {10.0.0.5, 10.0.0.25, 10.0.0.50}
>> >
>> > This example match only has 3 addresses, but it could easily have
>> > hundreds of addresses.  In some cases, the same large set of addresses
>> > needs to be used in several ACLs.
>> >
>> > This patch adds a new Address_Set table to OVN_Northbound so that a set
>> > of addresses can be specified once and then referred to by name in ACLs.
>> > To recreate the above example, you would first create an address set:
>> >
>> >   $ ovn-nbctl create Address_Set name=set1
>> addresses=10.0.0.5,10.0.0.25,10.0.0.50
>> >
>> > Then you can refer to this address set by name in an ACL match:
>> >
>> >   outport == "lp1" && ip4.src == address_set(set1)
>> >
>> > Signed-off-by: Russell Bryant <russ...@ovn.org>
>>
>> It might be worth making address sets scoped somehow.  Otherwise it
>> might eventually become a bottleneck if there are many address sets that
>> a given hypervisor does not need to know about.
>>
>
>Good point.
>
>The primary use case I have for this is to use in the match column of OVN
>ACLs.  ACLs are scoped to logical switches, so we could do the same for
>address sets.  Having them scoped equally makes sense to me.
>
>The only downside I can think of is that it will cause OpenStack to have to
>duplicate the same address set across multiple logical switches in some
>cases.  I think that's probably OK though.  It's still a huge improvement
>over what it does today.

Thinking of the use of remote_group_id in OpenStack security groups, I believe 
it would be common for the same address set to be used in all logical switches 
that are connected to the same logical router. Since there is no notion of 
project in OVN, it is not so obvious what to scope address sets to.
Would it be good enough to have controllers check to see if the address set is 
used by any of the local logical switches?

Mickey

>> I am not sure that I am in favor of the restriction that all of the
>> addresses in a given set have the same form, described here:
>>
>> +  Each row in this table represents a named set of addresses.
>> +  An address set may contain MAC, IPv4, or IPv6 addresses, but
>> +  a single address set must contain addresses all of the same
>type.
>>
>> This restriction might be surprising to someone already familiar
>with
>> the OVN matching language, which otherwise doesn't have such a
>> restriction.
>>
>
>I made up the restriction, mainly because I wasn't sure how mixing
>types
>would be useful.  I can certainly drop it.  I don't think there's
>actually
>anything in the code trying to enforce it, anyway.
>
>
>> I don't see anything that indicates that an address set can include
>CIDR
>> or bitwise matches, e.g. 192.168.0.0/24 or
>> 01:00:00:00:00:00/01:00:00:00:00:00.  I don't know why that would
>be
>> restricted.
>>
>
>I just didn't think of it.  Explicitly including and testing that
>makes
>sense.
>
>
>> I haven't done a detailed review yet but it sounds like you plan to
>post
>> a v2.
>>
>
>Thanks for the feedback!  I'll incorporate your comments on both
>patches in
>the next revision (v3).
>
>-- 
>Russell Bryant
>___
>dev mailing list
>dev@openvswitch.org
>http://openvswitch.org/mailman/listinfo/dev
>

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/3] ovn: Add address_set() support for ACLs.

[Russell Bryant]

On Mon, Apr 11, 2016 at 12:08 PM, Ben Pfaff  wrote:

> On Tue, Apr 05, 2016 at 05:24:19PM -0400, Russell Bryant wrote:
> > This feature was originally proposed here:
> >
> >   http://openvswitch.org/pipermail/dev/2016-March/067440.html
> >
> > A common use case for OVN ACLs involves needing to match a set of IP
> > addresses.
> >
> >outport == "lp1" && ip4.src == {10.0.0.5, 10.0.0.25, 10.0.0.50}
> >
> > This example match only has 3 addresses, but it could easily have
> > hundreds of addresses.  In some cases, the same large set of addresses
> > needs to be used in several ACLs.
> >
> > This patch adds a new Address_Set table to OVN_Northbound so that a set
> > of addresses can be specified once and then referred to by name in ACLs.
> > To recreate the above example, you would first create an address set:
> >
> >   $ ovn-nbctl create Address_Set name=set1
> addresses=10.0.0.5,10.0.0.25,10.0.0.50
> >
> > Then you can refer to this address set by name in an ACL match:
> >
> >   outport == "lp1" && ip4.src == address_set(set1)
> >
> > Signed-off-by: Russell Bryant 
>
> It might be worth making address sets scoped somehow.  Otherwise it
> might eventually become a bottleneck if there are many address sets that
> a given hypervisor does not need to know about.
>

Good point.

The primary use case I have for this is to use in the match column of OVN
ACLs.  ACLs are scoped to logical switches, so we could do the same for
address sets.  Having them scoped equally makes sense to me.

The only downside I can think of is that it will cause OpenStack to have to
duplicate the same address set across multiple logical switches in some
cases.  I think that's probably OK though.  It's still a huge improvement
over what it does today.


> I am not sure that I am in favor of the restriction that all of the
> addresses in a given set have the same form, described here:
>
> +  Each row in this table represents a named set of addresses.
> +  An address set may contain MAC, IPv4, or IPv6 addresses, but
> +  a single address set must contain addresses all of the same type.
>
> This restriction might be surprising to someone already familiar with
> the OVN matching language, which otherwise doesn't have such a
> restriction.
>

I made up the restriction, mainly because I wasn't sure how mixing types
would be useful.  I can certainly drop it.  I don't think there's actually
anything in the code trying to enforce it, anyway.


> I don't see anything that indicates that an address set can include CIDR
> or bitwise matches, e.g. 192.168.0.0/24 or
> 01:00:00:00:00:00/01:00:00:00:00:00.  I don't know why that would be
> restricted.
>

I just didn't think of it.  Explicitly including and testing that makes
sense.


> I haven't done a detailed review yet but it sounds like you plan to post
> a v2.
>

Thanks for the feedback!  I'll incorporate your comments on both patches in
the next revision (v3).

-- 
Russell Bryant
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/3] ovn: Add address_set() support for ACLs.

[Ben Pfaff]

On Tue, Apr 05, 2016 at 05:24:19PM -0400, Russell Bryant wrote:
> This feature was originally proposed here:
> 
>   http://openvswitch.org/pipermail/dev/2016-March/067440.html
> 
> A common use case for OVN ACLs involves needing to match a set of IP
> addresses.
> 
>outport == "lp1" && ip4.src == {10.0.0.5, 10.0.0.25, 10.0.0.50}
> 
> This example match only has 3 addresses, but it could easily have
> hundreds of addresses.  In some cases, the same large set of addresses
> needs to be used in several ACLs.
> 
> This patch adds a new Address_Set table to OVN_Northbound so that a set
> of addresses can be specified once and then referred to by name in ACLs.
> To recreate the above example, you would first create an address set:
> 
>   $ ovn-nbctl create Address_Set name=set1 
> addresses=10.0.0.5,10.0.0.25,10.0.0.50
> 
> Then you can refer to this address set by name in an ACL match:
> 
>   outport == "lp1" && ip4.src == address_set(set1)
> 
> Signed-off-by: Russell Bryant 

It might be worth making address sets scoped somehow.  Otherwise it
might eventually become a bottleneck if there are many address sets that
a given hypervisor does not need to know about.

I am not sure that I am in favor of the restriction that all of the
addresses in a given set have the same form, described here:

+  Each row in this table represents a named set of addresses.
+  An address set may contain MAC, IPv4, or IPv6 addresses, but
+  a single address set must contain addresses all of the same type.

This restriction might be surprising to someone already familiar with
the OVN matching language, which otherwise doesn't have such a
restriction.

I don't see anything that indicates that an address set can include CIDR
or bitwise matches, e.g. 192.168.0.0/24 or
01:00:00:00:00:00/01:00:00:00:00:00.  I don't know why that would be
restricted.

I haven't done a detailed review yet but it sounds like you plan to post
a v2.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 3/3] ovn: Add address_set() support for ACLs.

[Han Zhou]

On Wednesday, April 6, 2016, Russell Bryant > wrote:

>
>
> On Tue, Apr 5, 2016 at 10:03 PM, Han Zhou  wrote:
>
>>
>>
>> On Tue, Apr 5, 2016 at 2:24 PM, Russell Bryant  wrote:
>> >> +/* Return true if the address sets match, false otherwise. */
>> > +static bool
>> > +address_sets_match(struct address_set *addr_set,
>> > +   const struct sbrec_address_set *addr_set_rec)
>> > +{
>> > +char **addrs1;
>> > +char **addrs2;
>> > +
>> > +if (addr_set->n_addresses != addr_set_rec->n_addresses) {
>> > +return false;
>> > +}
>> > +size_t n_addresses = addr_set->n_addresses;
>> > +
>> > +addrs1 = xmemdup(addr_set->addresses,
>> > + n_addresses * sizeof addr_set->addresses[0]);
>> > +addrs2 = xmemdup(addr_set_rec->addresses,
>> > + n_addresses * sizeof addr_set_rec->addresses[0]);
>>
>> This xmemdup might have some problems. Firstly, IPv6 address string size
>> is variant, so we cannot use the size of the first element to calculate the
>> total size. Secondly, since it is configurable, there can be mistakes in
>> the address format, or someone can even attack purposely.
>>
>
> It's not really obvious what's going on here, but it's not duplicating the
> actual strings.  it's duplicating an array of pointers so that we can sort
> them.  It's still pointing to the original strings.
>
Sorry, I misinterpreted the code :(


>
> The same pattern is used in ovn-nbctl and ovn-sbctl.
>
>
>> Thanks Russell. I haven't yet completed the review, just some comments
>> inlined. One additional thing came to my mind is that, even if it is much
>> more efficent than having IP addresses on each ACL, it is still O(nLogn)
>> considering the comparisons when handling the address updates. Ideally it
>> should be O(n) for adding/removing an IP in a set, but I understand that we
>> don't have the semantics for adding/deleting an element in a Set column.
>> Not sure if this would still create scale problems when the list grows to
>> hundreds or thousands of addresses. I just raise the concern, but no good
>> ideas yet.
>>
>
> Do you mean from the Neutron plugin?  Being able to say "add this element
> to a set" would be great.  It has come up before.  I was expecting it would
> get added at some point.  I copied Andy on this point.
>

I meant both neutron plugin and in ovn itself. I think it would help in
both cases. But in ovn more changes are required to make it O(1): physical
flow translation for address-set needs to be incremental, which is not a
small change.

>
> The code in ovn-controller could certainly be a lot more efficient as well
> since we're having to do a full comparison of sets to see if anything has
> changed.  Change tracking would help here, especially since it's well
> isolated. I opted for the less efficient route to start with to make sure
> we at least started with something that we knew worked.
>
Completely understand!

> --
> Russell Bryant
>


-- 
Best regards,
Han
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/3] ovn: Add address_set() support for ACLs.

[Russell Bryant]

On Tue, Apr 5, 2016 at 5:24 PM, Russell Bryant  wrote:

> This feature was originally proposed here:
>
>   http://openvswitch.org/pipermail/dev/2016-March/067440.html
>
> A common use case for OVN ACLs involves needing to match a set of IP
> addresses.
>
>outport == "lp1" && ip4.src == {10.0.0.5, 10.0.0.25, 10.0.0.50}
>
> This example match only has 3 addresses, but it could easily have
> hundreds of addresses.  In some cases, the same large set of addresses
> needs to be used in several ACLs.
>
> This patch adds a new Address_Set table to OVN_Northbound so that a set
> of addresses can be specified once and then referred to by name in ACLs.
> To recreate the above example, you would first create an address set:
>
>   $ ovn-nbctl create Address_Set name=set1
> addresses=10.0.0.5,10.0.0.25,10.0.0.50
>
> Then you can refer to this address set by name in an ACL match:
>
>   outport == "lp1" && ip4.src == address_set(set1)
>
> Signed-off-by: Russell Bryant 


I have documentation updates for this patch that I forgot to commit before
sending these patches.  I'll include them in v2.

-- 
Russell Bryant
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/3] ovn: Add address_set() support for ACLs.

[Russell Bryant]

On Tue, Apr 5, 2016 at 10:03 PM, Han Zhou  wrote:

>
>
> On Tue, Apr 5, 2016 at 2:24 PM, Russell Bryant  wrote:
> >> +/* Return true if the address sets match, false otherwise. */
> > +static bool
> > +address_sets_match(struct address_set *addr_set,
> > +   const struct sbrec_address_set *addr_set_rec)
> > +{
> > +char **addrs1;
> > +char **addrs2;
> > +
> > +if (addr_set->n_addresses != addr_set_rec->n_addresses) {
> > +return false;
> > +}
> > +size_t n_addresses = addr_set->n_addresses;
> > +
> > +addrs1 = xmemdup(addr_set->addresses,
> > + n_addresses * sizeof addr_set->addresses[0]);
> > +addrs2 = xmemdup(addr_set_rec->addresses,
> > + n_addresses * sizeof addr_set_rec->addresses[0]);
>
> This xmemdup might have some problems. Firstly, IPv6 address string size
> is variant, so we cannot use the size of the first element to calculate the
> total size. Secondly, since it is configurable, there can be mistakes in
> the address format, or someone can even attack purposely.
>

It's not really obvious what's going on here, but it's not duplicating the
actual strings.  it's duplicating an array of pointers so that we can sort
them.  It's still pointing to the original strings.

The same pattern is used in ovn-nbctl and ovn-sbctl.


> Thanks Russell. I haven't yet completed the review, just some comments
> inlined. One additional thing came to my mind is that, even if it is much
> more efficent than having IP addresses on each ACL, it is still O(nLogn)
> considering the comparisons when handling the address updates. Ideally it
> should be O(n) for adding/removing an IP in a set, but I understand that we
> don't have the semantics for adding/deleting an element in a Set column.
> Not sure if this would still create scale problems when the list grows to
> hundreds or thousands of addresses. I just raise the concern, but no good
> ideas yet.
>

Do you mean from the Neutron plugin?  Being able to say "add this element
to a set" would be great.  It has come up before.  I was expecting it would
get added at some point.  I copied Andy on this point.

The code in ovn-controller could certainly be a lot more efficient as well
since we're having to do a full comparison of sets to see if anything has
changed.  Change tracking would help here, especially since it's well
isolated.  I opted for the less efficient route to start with to make sure
we at least started with something that we knew worked.

-- 
Russell Bryant
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/3] ovn: Add address_set() support for ACLs.

[Han Zhou]

On Tue, Apr 5, 2016 at 2:24 PM, Russell Bryant  wrote:
>
> This feature was originally proposed here:
>
>   http://openvswitch.org/pipermail/dev/2016-March/067440.html
>
> A common use case for OVN ACLs involves needing to match a set of IP
> addresses.
>
>outport == "lp1" && ip4.src == {10.0.0.5, 10.0.0.25, 10.0.0.50}
>
> This example match only has 3 addresses, but it could easily have
> hundreds of addresses.  In some cases, the same large set of addresses
> needs to be used in several ACLs.
>
> This patch adds a new Address_Set table to OVN_Northbound so that a set
> of addresses can be specified once and then referred to by name in ACLs.
> To recreate the above example, you would first create an address set:
>
>   $ ovn-nbctl create Address_Set name=set1
addresses=10.0.0.5,10.0.0.25,10.0.0.50
>
> Then you can refer to this address set by name in an ACL match:
>
>   outport == "lp1" && ip4.src == address_set(set1)
>
> Signed-off-by: Russell Bryant 
> ---
>  ovn/controller/lflow.c| 155
+-
>  ovn/northd/ovn-northd.c   |  42 +
>  ovn/ovn-nb.ovsschema  |  12 +++-
>  ovn/ovn-nb.xml|  24 +++
>  ovn/ovn-sb.ovsschema  |  12 +++-
>  ovn/ovn-sb.xml|   9 +++
>  ovn/utilities/ovn-nbctl.c |   4 ++
>  ovn/utilities/ovn-sbctl.c |   4 ++
>  tests/ovn.at  |  10 +++
>  9 files changed, 267 insertions(+), 5 deletions(-)
>
> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> index 287ffd3..00b9e67 100644
> --- a/ovn/controller/lflow.c
> +++ b/ovn/controller/lflow.c
> @@ -27,6 +27,7 @@
>  #include "ovn/lib/ovn-sb-idl.h"
>  #include "packets.h"
>  #include "simap.h"
> +#include "sset.h"
>
>  VLOG_DEFINE_THIS_MODULE(lflow);
>
> @@ -35,6 +36,9 @@ VLOG_DEFINE_THIS_MODULE(lflow);
>  /* Contains "struct expr_symbol"s for fields supported by OVN lflows. */
>  static struct shash symtab;
>
> +/* Contains an internal expr datastructure that represents an address
set. */
> +static struct shash expr_address_sets;
> +
>  static void
>  add_logical_register(struct shash *symtab, enum mf_field_id id)
>  {
> @@ -48,6 +52,7 @@ void
>  lflow_init(void)
>  {
>  shash_init();
> +shash_init(_address_sets);
>
>  /* Reserve a pair of registers for the logical inport and outport.
A full
>   * 32-bit register each is bigger than we need, but the expression
code
> @@ -157,6 +162,150 @@ lflow_init(void)
>  expr_symtab_add_field(, "sctp.src", MFF_SCTP_SRC, "sctp",
false);
>  expr_symtab_add_field(, "sctp.dst", MFF_SCTP_DST, "sctp",
false);
>  }
> +
> +/* Details of an address set currently in address_sets. We keep a cached
> + * copy of sets still in their string form here to make it easier to
compare
> + * with the current values in the OVN_Southbound database. */
> +struct address_set {
> +char **addresses;
> +size_t n_addresses;
> +};
> +
> +/* struct address_set instances for address sets currently in the symtab,
> + * hashed on the address set name. */
> +static struct shash local_address_sets =
SHASH_INITIALIZER(_address_sets);
> +
> +static int
> +addr_cmp(const void *p1, const void *p2)
> +{
> +const char *s1 = p1;
> +const char *s2 = p2;
> +return strcmp(s1, s2);
> +}
> +
> +/* Return true if the address sets match, false otherwise. */
> +static bool
> +address_sets_match(struct address_set *addr_set,
> +   const struct sbrec_address_set *addr_set_rec)
> +{
> +char **addrs1;
> +char **addrs2;
> +
> +if (addr_set->n_addresses != addr_set_rec->n_addresses) {
> +return false;
> +}
> +size_t n_addresses = addr_set->n_addresses;
> +
> +addrs1 = xmemdup(addr_set->addresses,
> + n_addresses * sizeof addr_set->addresses[0]);
> +addrs2 = xmemdup(addr_set_rec->addresses,
> + n_addresses * sizeof addr_set_rec->addresses[0]);

This xmemdup might have some problems. Firstly, IPv6 address string size is
variant, so we cannot use the size of the first element to calculate the
total size. Secondly, since it is configurable, there can be mistakes in
the address format, or someone can even attack purposely.

> +
> +qsort(addrs1, n_addresses, sizeof *addrs1, addr_cmp);
> +qsort(addrs2, n_addresses, sizeof *addrs2, addr_cmp);
> +
> +bool res = true;
> +size_t i;
> +for (i = 0; i <  n_addresses; i++) {
> +if (strcmp(addrs1[i], addrs2[i])) {
> +res = false;
> +break;
> +}
> +}
> +
> +free(addrs1);
> +free(addrs2);
> +
> +return res;
> +}
> +
> +static void
> +address_set_destroy(struct address_set *addr_set)
> +{
> +size_t i;
> +for (i = 0; i < addr_set->n_addresses; i++) {
> +free(addr_set->addresses[i]);
> +}
> +if (addr_set->n_addresses) {
> +free(addr_set->addresses);
> +}
> +free(addr_set);
> +}
> +
> +static void
> +update_address_sets(struct 

[ovs-dev] [PATCH 3/3] ovn: Add address_set() support for ACLs.

[Russell Bryant]

This feature was originally proposed here:

  http://openvswitch.org/pipermail/dev/2016-March/067440.html

A common use case for OVN ACLs involves needing to match a set of IP
addresses.

   outport == "lp1" && ip4.src == {10.0.0.5, 10.0.0.25, 10.0.0.50}

This example match only has 3 addresses, but it could easily have
hundreds of addresses.  In some cases, the same large set of addresses
needs to be used in several ACLs.

This patch adds a new Address_Set table to OVN_Northbound so that a set
of addresses can be specified once and then referred to by name in ACLs.
To recreate the above example, you would first create an address set:

  $ ovn-nbctl create Address_Set name=set1 
addresses=10.0.0.5,10.0.0.25,10.0.0.50

Then you can refer to this address set by name in an ACL match:

  outport == "lp1" && ip4.src == address_set(set1)

Signed-off-by: Russell Bryant 
---
 ovn/controller/lflow.c| 155 +-
 ovn/northd/ovn-northd.c   |  42 +
 ovn/ovn-nb.ovsschema  |  12 +++-
 ovn/ovn-nb.xml|  24 +++
 ovn/ovn-sb.ovsschema  |  12 +++-
 ovn/ovn-sb.xml|   9 +++
 ovn/utilities/ovn-nbctl.c |   4 ++
 ovn/utilities/ovn-sbctl.c |   4 ++
 tests/ovn.at  |  10 +++
 9 files changed, 267 insertions(+), 5 deletions(-)

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 287ffd3..00b9e67 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -27,6 +27,7 @@
 #include "ovn/lib/ovn-sb-idl.h"
 #include "packets.h"
 #include "simap.h"
+#include "sset.h"
 
 VLOG_DEFINE_THIS_MODULE(lflow);
 
@@ -35,6 +36,9 @@ VLOG_DEFINE_THIS_MODULE(lflow);
 /* Contains "struct expr_symbol"s for fields supported by OVN lflows. */
 static struct shash symtab;
 
+/* Contains an internal expr datastructure that represents an address set. */
+static struct shash expr_address_sets;
+
 static void
 add_logical_register(struct shash *symtab, enum mf_field_id id)
 {
@@ -48,6 +52,7 @@ void
 lflow_init(void)
 {
 shash_init();
+shash_init(_address_sets);
 
 /* Reserve a pair of registers for the logical inport and outport.  A full
  * 32-bit register each is bigger than we need, but the expression code
@@ -157,6 +162,150 @@ lflow_init(void)
 expr_symtab_add_field(, "sctp.src", MFF_SCTP_SRC, "sctp", false);
 expr_symtab_add_field(, "sctp.dst", MFF_SCTP_DST, "sctp", false);
 }
+
+/* Details of an address set currently in address_sets. We keep a cached
+ * copy of sets still in their string form here to make it easier to compare
+ * with the current values in the OVN_Southbound database. */
+struct address_set {
+char **addresses;
+size_t n_addresses;
+};
+
+/* struct address_set instances for address sets currently in the symtab,
+ * hashed on the address set name. */
+static struct shash local_address_sets = 
SHASH_INITIALIZER(_address_sets);
+
+static int
+addr_cmp(const void *p1, const void *p2)
+{
+const char *s1 = p1;
+const char *s2 = p2;
+return strcmp(s1, s2);
+}
+
+/* Return true if the address sets match, false otherwise. */
+static bool
+address_sets_match(struct address_set *addr_set,
+   const struct sbrec_address_set *addr_set_rec)
+{
+char **addrs1;
+char **addrs2;
+
+if (addr_set->n_addresses != addr_set_rec->n_addresses) {
+return false;
+}
+size_t n_addresses = addr_set->n_addresses;
+
+addrs1 = xmemdup(addr_set->addresses,
+ n_addresses * sizeof addr_set->addresses[0]);
+addrs2 = xmemdup(addr_set_rec->addresses,
+ n_addresses * sizeof addr_set_rec->addresses[0]);
+
+qsort(addrs1, n_addresses, sizeof *addrs1, addr_cmp);
+qsort(addrs2, n_addresses, sizeof *addrs2, addr_cmp);
+
+bool res = true;
+size_t i;
+for (i = 0; i <  n_addresses; i++) {
+if (strcmp(addrs1[i], addrs2[i])) {
+res = false;
+break;
+}
+}
+
+free(addrs1);
+free(addrs2);
+
+return res;
+}
+
+static void
+address_set_destroy(struct address_set *addr_set)
+{
+size_t i;
+for (i = 0; i < addr_set->n_addresses; i++) {
+free(addr_set->addresses[i]);
+}
+if (addr_set->n_addresses) {
+free(addr_set->addresses);
+}
+free(addr_set);
+}
+
+static void
+update_address_sets(struct controller_ctx *ctx)
+{
+/* Remember the names of all address sets currently in expr_address_sets
+ * so we can detect address sets that have been deleted. */
+struct sset cur_address_sets = SSET_INITIALIZER(_address_sets);
+
+struct shash_node *node;
+SHASH_FOR_EACH (node, _address_sets) {
+sset_add(_address_sets, node->name);
+}
+
+/* Iterate address sets in the southbound database.  Create and update the
+ * corresponding symtab entries as necessary. */
+const struct sbrec_address_set *addr_set_rec;
+SBREC_ADDRESS_SET_FOR_EACH (addr_set_rec, ctx->ovnsb_idl) {
+