Open Stack

Ok, those were some good points. I personally like the approach of letting
each implementation specify it's own set of supported protocols.

I'll change my patch to simply convert all protocols to names (more
readable).


Thanks
Arvind

On 9/11/13 3:06 PM, "Justin Hammond" <justin.hammond at RACKSPACE.COM> wrote:

>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
>
>
>_______________________________________________
>OpenStack-dev mailing list
>OpenStack-dev at lists.openstack.org
>http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev