I agree with you. Plugin was a mere example and it does make sense to allow the provider to define custom protocols.
+1 On 9/11/13 12:46 PM, "Akihiro Motoki" <amot...@gmail.com> wrote: >Hi Justin, > >My point is what > >On Thu, Sep 12, 2013 at 12:46 AM, Justin Hammond ><justin.hamm...@rackspace.com> wrote: >> 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. > >I agree that value should be validated appropriately in general. >A configurable list of allowed protocols looks good to me. > >> 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. > >I think this is just a case a backend plugin defines allowed protocols. > >I also see a different case: a cloud provider defines allowed protocols. >For example VLAN network type of OVS plugin can convey any type of packets >including GRE, STCP and so on if a provider wants to do so. >We need to allow a provider to configure the list. > >Considering the above, what we need to do looks: >(a) to validate values properly, >(b) to allow a plugin to define what protocols should be allowed > (I think we need two types of lists: possible protocols and >default allowed protocols) >(c) to allow a cloud provider (deployer) to customize allow protocols. > (Of course (c) is a subnet of "possible protocols" in (b)) > >Does it make sense? >The above is just a start point of the discussion and some list can be >omitted. > ># Whether (c) is needed or not depends on the default list of (b). ># If it is wide enough (c) is not needed. The current list of (b) is >[tcp, udp, icmp] ># and it looks too small set to me, so it is better to have (c) too. > >> 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. > >I think the situation is a bit different. What network types are >allowed is tightly >coupled with a plugin implementation, and a cloud provider choose a plugin >based on their needs. Thus the mechanism of NETWORK_TYPE constants >make sense to me too. > >> tl;dr: magic constants are no good, but values should be validated in a >> pluggable and explicit manner. > >As I said above, I agree it is important to validate values properly in >general. > >Thanks, >Akihiro > >> >> >> >> 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 > >-- >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