As it seems the review is no longer the place for this discussion, I will copy/paste my inline comments here:
I dislike the idea of passing magical numbers around to define protocols (defined or otherwise). I believe there should be a common set of protocols with their numbers mapped (such as this constants business) and a well defined way to validate/list said common constants. If a plugin wishes to add support for a protocol outside of the common case, it should be added to the list in a pluggable manner. Ex: common defines the constants 1, 6, 17 to be valid but my_cool_plugin wants to support 42. It should be my plugin's responsibility to add 42 to the list of valid protocols by appending to the list given a pluggable interface to do so. I do not believe plugins should continue to update the common.constants file with new protocols, but I do believe explicitly stating which protocols are valid is better than allowing users to possibly submit protocols erroneously. If the plugins use a system such as this, it is possible that new, common, protocols can be found to be core. See NETWORK_TYPE constants. tl;dr: magic constants are no good, but values should be validated in a pluggable and explicit manner. On 9/11/13 10:40 AM, "Akihiro Motoki" <amot...@gmail.com> wrote: >Hi all, > >Arvind, thank you for initiate the discussion about the ip protocol in >security group rules. >I think the discussion point can be broken down into: > >(a) how to specify ip protocol : by name, number, or both >(b) what ip protocols can be specified: known protocols only, all >protocols (or some subset of protocols including unknown protocols) > where "known protocols" is defined as a list in Neutron (a list >of constants or a configurable list) > >------ >(b) is the main topic Arvind and I discussed in the review. >If only known protocols are allowed, we cannot allow protocols which >are not listed in the known protocol list. >For instance, if "tcp", "udp" and "icmp" are registered as known >protocols (this is the current neutron implementation), >a tenant cannot allow "stcp" or "gre". > >Pros of "known protocols only" is the infrastructure provider can >control which protocols are allowed. >Cons is that users cannot use ip protocols not listed in a known list >and a provider needs to maintain a known protocol list. >Pros and cons of "all protocols allowed" is vice versa. > >If a list of known protocols is configurable, we can cover both cases, >e.g., an empty list or a list ["ANY"] means all protocols are allowed. >The question in this case is what is the best default value. > >My preference is to allow all protocols. At least a list of known >protocols needs to be configurable. >In my principle, a virtual network should be able to convery any type >of IP protocols in a virtual network. This is the reason of my >preference. > >----- >Regarding (a), if a name and a number refer to a same protocol, it >should be considered as identical. >For example, ip protocol number 6 is "tcp", so ip protocol number 6 >and protocol name "tcp" should be regarded as same. >My preference is to allow both name and number of IP protocol. This >will be achieved by Arvind's patch under the review. >"name" representation is easy to understand in general, but >maintaining all protocol names is a tough work. >This is the reason of my preference. > > >I understand there is a topic whether a list of known protocols should >contain name only or accepts both name and number. >I don't discuss it here because it is a simple question once we have a >consensus on the above two topic. > >Thanks, >Akihiro > >On Wed, Sep 11, 2013 at 11:15 PM, Arvind Somya (asomya) ><aso...@cisco.com> wrote: >> Hello all >> >> I have a patch in review where Akihiro made some comments about only >> restricting protocols by names and allowing all protocol numbers when >> creating security group rules. I personally disagree with this approach >>as >> names and numbers are just a textual/integer representation of a common >> protocol. The end result is going to be the same in both cases. >> >> https://review.openstack.org/#/c/43725/ >> >> Akihiro suggested a community discussion around this issue before the >>patch >> is accepted upstream. I hope this e-mail gets the ball rolling on that. >>I >> would like to hear the community's opinion on this issue and any >> pros/cons/pitfalls of either approach. >> >> Thanks >> Arvind >> >> _______________________________________________ >> OpenStack-dev mailing list >> OpenStack-dev@lists.openstack.org >> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >> > > > >-- >Akihiro MOTOKI <amot...@gmail.com> > >_______________________________________________ >OpenStack-dev mailing list >OpenStack-dev@lists.openstack.org >http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev