You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@deltacloud.apache.org by sa...@eucalyptus.com on 2011/07/10 19:28:28 UTC
[PATCH 1/2] bug fix in ec2::firewall_rule_id
While preparing for the firewall support in eucalyptus driver, I found the bug in ec2 driver.
The issue is 'each_pair' doesn't guarantee that the rule_id string is concartenated in the right order.
Specifically I found the prefix and address is switched (e.g., 0, 0.0.0.0).
Re: [PATCH 1/2] bug fix in ec2::firewall_rule_id
Posted by Sang-Min Park <sa...@eucalyptus.com>.
thanks marios. I made the adjustment per your suggestions and committed.
--SM
On Mon, Jul 11, 2011 at 5:34 AM, marios@redhat.com <ma...@redhat.com>wrote:
> ACK - I like this approach; an earlier version was setting ID like:
>
> type=group&owner=123456789012&**name=new_firewall|type=**
> address&family=ipv4&address=**192.168.1.1&prefix=24"
>
> i.e. each value was prefixed with its field name but this made the ID
> string too long. I think your approach of building the ID by explicitly
> setting positions is safer.
>
> Can you please look at a couple formatting nits before you commit (you have
> commit right now I believe?):
>
> * trailing whitespaces lines 78, 90 92 of the patch (see David
> Lutterkort's email "Whitespace and you" on 03rd June 2011
> http://mail-archives.apache.**org/mod_mbox/incubator-**
> deltacloud-dev/201106.mbox/%**3C1307092263.2654.19.camel@**
> melon.watzmann.net%3E<http://mail-archives.apache.org/mod_mbox/incubator-deltacloud-dev/201106.mbox/%3C1307092263.2654.19.camel@melon.watzmann.net%3E>for editor settings etc - e.g. I couldn't apply your patch due to my git
> pre-commit hooks about whitespace (I edited the patch and then applied))
>
> * can you use spaces instead of tabs lines 79, 80, 82 of the patch (i know
> this sounds pedantic but different editors handle tabs in different ways so
> formatting gets messed around - the dev team uses 2 spaces/tab)
>
> * why do you prefer "sources_string.slice(0,**sources_string.length-1)"
> over "sources_string.chomp!(",")"
>
> marios
>
>
> On 10/07/11 20:28, sang-min.park@eucalyptus.com wrote:
>
>> From: Sang-Min Park<sp...@eucalyptus.com>
>>
>> ---
>> server/lib/deltacloud/drivers/**ec2/ec2_driver.rb | 12 ++++++------
>> 1 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/server/lib/deltacloud/**drivers/ec2/ec2_driver.rb
>> b/server/lib/deltacloud/**drivers/ec2/ec2_driver.rb
>> index 53f95b6..a21ba34 100644
>> --- a/server/lib/deltacloud/**drivers/ec2/ec2_driver.rb
>> +++ b/server/lib/deltacloud/**drivers/ec2/ec2_driver.rb
>> @@ -809,15 +809,15 @@ module Deltacloud
>> #generate uid from firewall rule parameters (amazon doesn't do
>> this for us
>> def firewall_rule_id(user_id, protocol, from_port, to_port,
>> sources)
>> sources_string = ""
>> - sources.each do |source|
>> - sources_string<<"@"
>> - source.each_pair do |key,value|
>> - sources_string<< "#{value},"
>> + sources.each do |source|
>> + if source[:type].to_s == "group"
>> + sources_string<< "@#{source[:type]},#{source[:**
>> owner]},#{source[:name]},"
>> + else
>> + sources_string<< "@#{source[:type]},#{source[:**
>> family]},#{source[:address]},#**{source[:prefix]},"
>> end
>> - sources_string.chomp!(",")
>> end
>> #sources_string is @group,297467797945,test@**address,ipv4,10.1.1.1,24
>> etc
>> - id_string = "#{user_id}~#{protocol}~#{**
>> from_port}~#{to_port}~#{**sources_string}"
>> + id_string = "#{user_id}~#{protocol}~#{**
>> from_port}~#{to_port}~#{**sources_string.slice(0,**
>> sources_string.length-1)}"
>> end
>>
>> #extract params from uid
>>
>
>
--
----------------------------------------------------
Sang-Min Park
Engineer
Eucalyptus Systems
Re: [PATCH 1/2] bug fix in ec2::firewall_rule_id
Posted by "marios@redhat.com" <ma...@redhat.com>.
ACK - I like this approach; an earlier version was setting ID like:
type=group&owner=123456789012&name=new_firewall|type=address&family=ipv4&address=192.168.1.1&prefix=24"
i.e. each value was prefixed with its field name but this made the ID
string too long. I think your approach of building the ID by explicitly
setting positions is safer.
Can you please look at a couple formatting nits before you commit (you
have commit right now I believe?):
* trailing whitespaces lines 78, 90 92 of the patch (see David
Lutterkort's email "Whitespace and you" on 03rd June 2011
http://mail-archives.apache.org/mod_mbox/incubator-deltacloud-dev/201106.mbox/%3C1307092263.2654.19.camel@melon.watzmann.net%3E
for editor settings etc - e.g. I couldn't apply your patch due to my git
pre-commit hooks about whitespace (I edited the patch and then applied))
* can you use spaces instead of tabs lines 79, 80, 82 of the patch (i
know this sounds pedantic but different editors handle tabs in different
ways so formatting gets messed around - the dev team uses 2 spaces/tab)
* why do you prefer "sources_string.slice(0,sources_string.length-1)"
over "sources_string.chomp!(",")"
marios
On 10/07/11 20:28, sang-min.park@eucalyptus.com wrote:
> From: Sang-Min Park<sp...@eucalyptus.com>
>
> ---
> server/lib/deltacloud/drivers/ec2/ec2_driver.rb | 12 ++++++------
> 1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> index 53f95b6..a21ba34 100644
> --- a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> +++ b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> @@ -809,15 +809,15 @@ module Deltacloud
> #generate uid from firewall rule parameters (amazon doesn't do this for us
> def firewall_rule_id(user_id, protocol, from_port, to_port, sources)
> sources_string = ""
> - sources.each do |source|
> - sources_string<<"@"
> - source.each_pair do |key,value|
> - sources_string<< "#{value},"
> + sources.each do |source|
> + if source[:type].to_s == "group"
> + sources_string<< "@#{source[:type]},#{source[:owner]},#{source[:name]},"
> + else
> + sources_string<< "@#{source[:type]},#{source[:family]},#{source[:address]},#{source[:prefix]},"
> end
> - sources_string.chomp!(",")
> end
> #sources_string is @group,297467797945,test@address,ipv4,10.1.1.1,24 etc
> - id_string = "#{user_id}~#{protocol}~#{from_port}~#{to_port}~#{sources_string}"
> + id_string = "#{user_id}~#{protocol}~#{from_port}~#{to_port}~#{sources_string.slice(0,sources_string.length-1)}"
> end
>
> #extract params from uid
[PATCH 1/2] bug fix in ec2::firewall_rule_id
Posted by sa...@eucalyptus.com.
From: Sang-Min Park <sp...@eucalyptus.com>
---
server/lib/deltacloud/drivers/ec2/ec2_driver.rb | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
index 53f95b6..a21ba34 100644
--- a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
+++ b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
@@ -809,15 +809,15 @@ module Deltacloud
#generate uid from firewall rule parameters (amazon doesn't do this for us
def firewall_rule_id(user_id, protocol, from_port, to_port, sources)
sources_string = ""
- sources.each do |source|
- sources_string<<"@"
- source.each_pair do |key,value|
- sources_string<< "#{value},"
+ sources.each do |source|
+ if source[:type].to_s == "group"
+ sources_string << "@#{source[:type]},#{source[:owner]},#{source[:name]},"
+ else
+ sources_string << "@#{source[:type]},#{source[:family]},#{source[:address]},#{source[:prefix]},"
end
- sources_string.chomp!(",")
end
#sources_string is @group,297467797945,test@address,ipv4,10.1.1.1,24 etc
- id_string = "#{user_id}~#{protocol}~#{from_port}~#{to_port}~#{sources_string}"
+ id_string = "#{user_id}~#{protocol}~#{from_port}~#{to_port}~#{sources_string.slice(0,sources_string.length-1)}"
end
#extract params from uid
--
1.7.4.1