Open Stack

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" <amotoki at gmail.com> wrote:

>Hi Justin,
>
>My point is what
>
>On Thu, Sep 12, 2013 at 12:46 AM, Justin Hammond
><justin.hammond at 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" <amotoki at 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)
>>><asomya at 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 at lists.openstack.org
>>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>>>
>>>
>>>
>>>
>>>--
>>>Akihiro MOTOKI <amotoki at gmail.com>
>>>
>>>_______________________________________________
>>>OpenStack-dev mailing list
>>>OpenStack-dev at lists.openstack.org
>>>http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>>
>> _______________________________________________
>> OpenStack-dev mailing list
>> OpenStack-dev at lists.openstack.org
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>-- 
>Akihiro MOTOKI <amotoki at gmail.com>
>
>_______________________________________________
>OpenStack-dev mailing list
>OpenStack-dev at lists.openstack.org
>http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev