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