Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to input ct.label value for an acl

2019-02-07 Thread Ben Pfaff
That's helpful.

On Thu, Feb 07, 2019 at 09:32:33PM +, Ankur Sharma wrote:
> Hi Ben,
> 
> My bad, should have explained the use case as well.
> 
> Following is the use case I started with:
> a. By associating identifier with connection tracker we can facilitate 
> debugging from connection tracker side.
> b. For example, if a system has multiple OVN ACLs (Hundreds or Thousands), 
> and we see an unexpected FLOW getting established.
>  We can debug it from connection tracker side to see which "Allow" ACL, 
> let the packet passthrough.
> 
> 
> I will update the documentation with this use case as well.
> Please let me know your thoughts.
> 
> Regards,
> Ankur
> 
> -Original Message-
> From: Ben Pfaff  
> Sent: Wednesday, February 6, 2019 6:11 PM
> To: Ankur Sharma 
> Cc: ovs-dev@openvswitch.org
> Subject: Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to input 
> ct.label value for an acl
> 
> I'd like to hear some kind of overall use case here.  Sure, you can use it to 
> identify an OVN ACL, or a security group, or anything else.  How does that 
> contribute to a larger system?  There should be a hint to the reader about 
> how and why to use it.
> 
> On Wed, Feb 06, 2019 at 10:06:46PM +, Ankur Sharma wrote:
> > Reason for using 128 bits:
> > a. Connection tracker has only 2 fields for metadata, ct.mark(32 bits) and 
> > ct.label(128 bits).
> > b. Purpose of this series is to ensure that we use smaller field, i.e  
> > ct.mark for flags and use the bigger field, i.e ct.label for associating 
> > metadata with the ct entry.
> > c. An example of metadata could be a value which maps ct entry to 
> > corresponding OVN ACL or Security Group or both.
> > d. Yes, I agree that 128 could more than sufficient for most of the cases, 
> > but unless we see a use case of dividing ct.label in subfields, i thought 
> > we can leverage on full 128 bits.
> > This keeps implementation simple and  also keeps the interpretation of a 
> > connection tracking entry simple.
> > 
> > Please let me know, if it sounds reasonable.
> > 
> > Thanks
> > 
> > Regards,
> > Ankur
> > 
> > -Original Message-
> > From: Ben Pfaff 
> > Sent: Tuesday, February 5, 2019 1:40 PM
> > To: Ankur Sharma 
> > Cc: ovs-dev@openvswitch.org
> > Subject: Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to 
> > input ct.label value for an acl
> > 
> > On Fri, Jan 11, 2019 at 12:16:39AM +, Ankur Sharma wrote:
> > > This patch allows user to associate a value with acl, which will be 
> > > assigned to ct.label of the corresponding connection tracking entry.
> > > 
> > > This value can be used to map a ct entry with corresponding OVN ACL 
> > > or higher level constructs like security group.
> > > 
> > > signed-off-by: Ankur Sharma 
> > 
> > Thanks for the patch!
> > 
> > Please capitalize the "S" in "Signed-off-by".
> > 
> > This adds a column in ovn-sb.ovsschema, so it should increment the minor 
> > version (the y in x.y.z).
> > 
> > The documentation for the new column explains what it does, but it does not 
> > explain the purpose.  Why would a user set this column?  What are its 
> > effects?
> > 
> > The column is a string, but its value is an integer.  Maybe that is because 
> > OVSDB integer columns are limited to 64 bits, but this value can be 128 
> > bits.  That is a very large space.  What is the reason that so much space 
> > should be dedicated to this identifier?  Even 64 bits is more identifiers 
> > than any deployment will ever use, so there must be some other reason.
> > 
> > Please do not use // comments.
> > 
> > Please document the new option in the ovn-sbctl manpage.
> > 
> > Please add a NEWS item for the new feature.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to input ct.label value for an acl

2019-02-07 Thread Ankur Sharma
Hi Ben,

My bad, should have explained the use case as well.

Following is the use case I started with:
a. By associating identifier with connection tracker we can facilitate 
debugging from connection tracker side.
b. For example, if a system has multiple OVN ACLs (Hundreds or Thousands), and 
we see an unexpected FLOW getting established.
 We can debug it from connection tracker side to see which "Allow" ACL, let 
the packet passthrough.


I will update the documentation with this use case as well.
Please let me know your thoughts.

Regards,
Ankur

-Original Message-
From: Ben Pfaff  
Sent: Wednesday, February 6, 2019 6:11 PM
To: Ankur Sharma 
Cc: ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to input 
ct.label value for an acl

I'd like to hear some kind of overall use case here.  Sure, you can use it to 
identify an OVN ACL, or a security group, or anything else.  How does that 
contribute to a larger system?  There should be a hint to the reader about how 
and why to use it.

On Wed, Feb 06, 2019 at 10:06:46PM +, Ankur Sharma wrote:
> Reason for using 128 bits:
> a. Connection tracker has only 2 fields for metadata, ct.mark(32 bits) and 
> ct.label(128 bits).
> b. Purpose of this series is to ensure that we use smaller field, i.e  
> ct.mark for flags and use the bigger field, i.e ct.label for associating 
> metadata with the ct entry.
> c. An example of metadata could be a value which maps ct entry to 
> corresponding OVN ACL or Security Group or both.
> d. Yes, I agree that 128 could more than sufficient for most of the cases, 
> but unless we see a use case of dividing ct.label in subfields, i thought we 
> can leverage on full 128 bits.
> This keeps implementation simple and  also keeps the interpretation of a 
> connection tracking entry simple.
> 
> Please let me know, if it sounds reasonable.
> 
> Thanks
> 
> Regards,
> Ankur
> 
> -Original Message-
> From: Ben Pfaff 
> Sent: Tuesday, February 5, 2019 1:40 PM
> To: Ankur Sharma 
> Cc: ovs-dev@openvswitch.org
> Subject: Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to 
> input ct.label value for an acl
> 
> On Fri, Jan 11, 2019 at 12:16:39AM +, Ankur Sharma wrote:
> > This patch allows user to associate a value with acl, which will be 
> > assigned to ct.label of the corresponding connection tracking entry.
> > 
> > This value can be used to map a ct entry with corresponding OVN ACL 
> > or higher level constructs like security group.
> > 
> > signed-off-by: Ankur Sharma 
> 
> Thanks for the patch!
> 
> Please capitalize the "S" in "Signed-off-by".
> 
> This adds a column in ovn-sb.ovsschema, so it should increment the minor 
> version (the y in x.y.z).
> 
> The documentation for the new column explains what it does, but it does not 
> explain the purpose.  Why would a user set this column?  What are its effects?
> 
> The column is a string, but its value is an integer.  Maybe that is because 
> OVSDB integer columns are limited to 64 bits, but this value can be 128 bits. 
>  That is a very large space.  What is the reason that so much space should be 
> dedicated to this identifier?  Even 64 bits is more identifiers than any 
> deployment will ever use, so there must be some other reason.
> 
> Please do not use // comments.
> 
> Please document the new option in the ovn-sbctl manpage.
> 
> Please add a NEWS item for the new feature.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to input ct.label value for an acl

2019-02-06 Thread Ben Pfaff
I'd like to hear some kind of overall use case here.  Sure, you can use
it to identify an OVN ACL, or a security group, or anything else.  How
does that contribute to a larger system?  There should be a hint to the
reader about how and why to use it.

On Wed, Feb 06, 2019 at 10:06:46PM +, Ankur Sharma wrote:
> Reason for using 128 bits:
> a. Connection tracker has only 2 fields for metadata, ct.mark(32 bits) and 
> ct.label(128 bits).
> b. Purpose of this series is to ensure that we use smaller field, i.e  
> ct.mark for flags and use the bigger field, i.e ct.label for associating 
> metadata with the ct entry.
> c. An example of metadata could be a value which maps ct entry to 
> corresponding OVN ACL or Security Group or both.
> d. Yes, I agree that 128 could more than sufficient for most of the cases, 
> but unless we see a use case of dividing ct.label in subfields, i thought we 
> can leverage on full 128 bits.
> This keeps implementation simple and  also keeps the interpretation of a 
> connection tracking entry simple.
> 
> Please let me know, if it sounds reasonable.
> 
> Thanks
> 
> Regards,
> Ankur
> 
> -Original Message-
> From: Ben Pfaff  
> Sent: Tuesday, February 5, 2019 1:40 PM
> To: Ankur Sharma 
> Cc: ovs-dev@openvswitch.org
> Subject: Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to input 
> ct.label value for an acl
> 
> On Fri, Jan 11, 2019 at 12:16:39AM +, Ankur Sharma wrote:
> > This patch allows user to associate a value with acl, which will be 
> > assigned to ct.label of the corresponding connection tracking entry.
> > 
> > This value can be used to map a ct entry with corresponding OVN ACL or 
> > higher level constructs like security group.
> > 
> > signed-off-by: Ankur Sharma 
> 
> Thanks for the patch!
> 
> Please capitalize the "S" in "Signed-off-by".
> 
> This adds a column in ovn-sb.ovsschema, so it should increment the minor 
> version (the y in x.y.z).
> 
> The documentation for the new column explains what it does, but it does not 
> explain the purpose.  Why would a user set this column?  What are its effects?
> 
> The column is a string, but its value is an integer.  Maybe that is because 
> OVSDB integer columns are limited to 64 bits, but this value can be 128 bits. 
>  That is a very large space.  What is the reason that so much space should be 
> dedicated to this identifier?  Even 64 bits is more identifiers than any 
> deployment will ever use, so there must be some other reason.
> 
> Please do not use // comments.
> 
> Please document the new option in the ovn-sbctl manpage.
> 
> Please add a NEWS item for the new feature.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to input ct.label value for an acl

2019-02-06 Thread Ankur Sharma
Hi Ben,

Thanks a lot for review.
Sure, V2 will have all the comments addressed.

Reason for using 128 bits:
a. Connection tracker has only 2 fields for metadata, ct.mark(32 bits) and 
ct.label(128 bits).
b. Purpose of this series is to ensure that we use smaller field, i.e  ct.mark 
for flags and use the bigger field, i.e ct.label for associating metadata with 
the ct entry.
c. An example of metadata could be a value which maps ct entry to corresponding 
OVN ACL or Security Group or both.
d. Yes, I agree that 128 could more than sufficient for most of the cases, but 
unless we see a use case of dividing ct.label in subfields, i thought we can 
leverage on full 128 bits.
This keeps implementation simple and  also keeps the interpretation of a 
connection tracking entry simple.

Please let me know, if it sounds reasonable.

Thanks

Regards,
Ankur

-Original Message-
From: Ben Pfaff  
Sent: Tuesday, February 5, 2019 1:40 PM
To: Ankur Sharma 
Cc: ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to input 
ct.label value for an acl

On Fri, Jan 11, 2019 at 12:16:39AM +, Ankur Sharma wrote:
> This patch allows user to associate a value with acl, which will be 
> assigned to ct.label of the corresponding connection tracking entry.
> 
> This value can be used to map a ct entry with corresponding OVN ACL or 
> higher level constructs like security group.
> 
> signed-off-by: Ankur Sharma 

Thanks for the patch!

Please capitalize the "S" in "Signed-off-by".

This adds a column in ovn-sb.ovsschema, so it should increment the minor 
version (the y in x.y.z).

The documentation for the new column explains what it does, but it does not 
explain the purpose.  Why would a user set this column?  What are its effects?

The column is a string, but its value is an integer.  Maybe that is because 
OVSDB integer columns are limited to 64 bits, but this value can be 128 bits.  
That is a very large space.  What is the reason that so much space should be 
dedicated to this identifier?  Even 64 bits is more identifiers than any 
deployment will ever use, so there must be some other reason.

Please do not use // comments.

Please document the new option in the ovn-sbctl manpage.

Please add a NEWS item for the new feature.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to input ct.label value for an acl

2019-02-05 Thread Ben Pfaff
On Fri, Jan 11, 2019 at 12:16:39AM +, Ankur Sharma wrote:
> This patch allows user to associate a value with acl,
> which will be assigned to ct.label of the corresponding
> connection tracking entry.
> 
> This value can be used to map a ct entry with corresponding
> OVN ACL or higher level constructs like security group.
> 
> signed-off-by: Ankur Sharma 

Thanks for the patch!

Please capitalize the "S" in "Signed-off-by".

This adds a column in ovn-sb.ovsschema, so it should increment the minor
version (the y in x.y.z).

The documentation for the new column explains what it does, but it does
not explain the purpose.  Why would a user set this column?  What are
its effects?

The column is a string, but its value is an integer.  Maybe that is
because OVSDB integer columns are limited to 64 bits, but this value can
be 128 bits.  That is a very large space.  What is the reason that so
much space should be dedicated to this identifier?  Even 64 bits is more
identifiers than any deployment will ever use, so there must be some
other reason.

Please do not use // comments.

Please document the new option in the ovn-sbctl manpage.

Please add a NEWS item for the new feature.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev