incubator-deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Lutterkort <lut...@redhat.com>
Subject Re: [PATCH] implements 'firewalls' - ec2 security groups [revision 2]
Date Fri, 03 Jun 2011 09:52:27 GMT
ACK, with a few nits:

Before pushing, remove [revision 2] from the commit message. Also,
Gemfile needs a version constraint since we depend on a very new aws gem

On Mon, 2011-05-30 at 17:33 +0300, marios@redhat.com wrote:
> From: marios <marios@redhat.com>
> diff --git a/server/lib/deltacloud/base_driver/features.rb b/server/lib/deltacloud/base_driver/features.rb
> index 65c4cba..cb25a3b 100644
> --- a/server/lib/deltacloud/base_driver/features.rb
> +++ b/server/lib/deltacloud/base_driver/features.rb
> @@ -183,11 +183,11 @@ module Deltacloud
>        end
>      end
>  
> -    declare_feature :instances, :security_group do
> -      description "Put instance in one or more security groups on launch"
> +    declare_feature :instances, :firewall do
> +      description "Put instance in one or more firewalls (security groups) on launch"
>        operation :create do
> -        param :security_group, :array, :optional, nil,
> -        "Array of security group names"
> +        param :firewalls, :array, :optional, nil,
> +        "Array of firewall (security group) id"

How is that array encoded ? Should mention that in the description.
 
> diff --git a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> index 0c0471a..c3911cc 100644
> --- a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> +++ b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> @@ -201,7 +203,7 @@ module Deltacloud
>            instance_options.merge!(:key_name => opts[:keyname]) if opts[:keyname]
>            instance_options.merge!(:availability_zone => opts[:realm_id]) if opts[:realm_id]
>            instance_options.merge!(:instance_type => opts[:hwp_id]) if opts[:hwp_id]
&& opts[:hwp_id].length > 0
> -          instance_options.merge!(:group_ids => opts[:security_group]) if opts[:security_group]
> +          instance_options.merge!(:group_ids => opts[:firewall]) if opts[:firewall]

The feature declaration calls that param :firewalls, not :firewall

> @@ -516,6 +518,7 @@ module Deltacloud
>            end
>          end
>  
> +

Spurious whitespace change

> @@ -764,6 +834,82 @@ module Deltacloud
>            balancer
>          end
>  
> +        #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|
> +            source.each_pair do |key,value|
> +              sources_string<< "#{key}=#{value}&"
> +            end
> +            sources_string.chomp!("&")
> +            sources_string<<"|"
> +          end
> +          sources_string.chomp!("|")
> +          #"type=group&owner=123456789012&name=new_firewall|type=address&family=ipv4&address=192.168.1.1&prefix=24"
> +          id_string = "user #{user_id}:::protocol #{protocol}:::from_port #{from_port}:::to_port
#{to_port}:::sources #{sources_string}"
> +          Base64.encode64(id_string)

One way to shorten the id is to be less verbose in the fixed parts of
id_string.

> diff --git a/server/lib/deltacloud/helpers/conversion_helper.rb b/server/lib/deltacloud/helpers/conversion_helper.rb
> index 9a33482..c838b7b 100644
> --- a/server/lib/deltacloud/helpers/conversion_helper.rb
> +++ b/server/lib/deltacloud/helpers/conversion_helper.rb
> @@ -19,7 +19,8 @@ require 'deltacloud/base_driver'
>  module ConversionHelper
>  
>    def convert_to_json(type, obj)
> -    if ( [ :image, :realm, :instance, :storage_volume, :storage_snapshot, :hardware_profile,
:key, :bucket, :address ].include?( type ) )
> +    if ( [ :image, :realm, :instance, :storage_volume, :storage_snapshot, :hardware_profile,
:key, :bucket, :blob, :firewall, :load_balancer, :address ].include?( type ) )
> +

The addition of :blob and :load_balancer should go into a separate
patch. (and there's an extra empty line added)

> @@ -27,7 +28,11 @@ module ConversionHelper
>          type = type.to_s.pluralize
>        else
>          data = obj.to_hash
> -        data.merge!({ :href => self.send(:"#{type}_url", data[:id]) })
> +        if type == :blob
> +          data.merge!({ :href => self.send(:"bucket_url", "#{data[:bucket]}/#{data[:id]}"
) })
> +        else
> +          data.merge!({ :href => self.send(:"#{type}_url", data[:id]) })
> +        end

This should also go into a separate 'improve json for blobs' patch.

> diff --git a/server/lib/deltacloud/models/bucket.rb b/server/lib/deltacloud/models/bucket.rb
> index 304fc0b..faf0224 100644
> --- a/server/lib/deltacloud/models/bucket.rb
> +++ b/server/lib/deltacloud/models/bucket.rb
> @@ -24,7 +24,9 @@ class Bucket < BaseModel
>  
>    def to_hash
>      h = self.to_hash_original
> -    h[:blob_list] = self.blob_list.collect { |blob| { :id => blob, :href => "/api/buckets/#{self.id}/#{blob.id}"}}
> +    unless blob_list.nil?
> +      h[:blob_list] = self.blob_list.collect { |blob| { :id => blob, :href =>
"/api/buckets/#{self.id}/#{blob}"}}
> +    end

More for the 'improve json for blobs' patch.

> diff --git a/server/public/javascripts/application.js b/server/public/javascripts/application.js
> index 95c9bc2..1c66d78 100644
> --- a/server/public/javascripts/application.js
> +++ b/server/public/javascripts/application.js
> @@ -51,3 +51,38 @@ function less_fields()
>  		meta_params[0].value = eval(current_val)-1
>  	}
>  }
> +
> +var addresses = 0;
> +var groups = 0;
> +function make_fields(type)
> +{
> +	form = document.getElementById("new_rule_form")
> +  button = document.getElementById("submit_button")

Please no tabs, only spaces for indentation.

> diff --git a/server/server.rb b/server/server.rb
> index 86dd524..1ba166a 100644
> --- a/server/server.rb
> +++ b/server/server.rb
> @@ -768,7 +768,7 @@ get '/api/buckets/:bucket/:blob' do
>      respond_to do |format|
>        format.html { haml :"blobs/show" }
>        format.xml { haml :"blobs/show" }
> -      format.json { convert_to_json(blobs, @blob) }
> +      format.json { convert_to_json(:blob, @blob) }

More for the 'improve json for blobs' patch.

> @@ -853,6 +853,7 @@ collection :buckets do
>  
>  end
>  
> +

.. and another hunk that shouldn't be there.

David



Mime
View raw message