You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@deltacloud.apache.org by ma...@redhat.com on 2011/05/16 19:45:09 UTC

firewalls - ec2 security groups - revision 1

first revision of firewalls including suggestions by Michal:

  * in xml output: <rule id="XNlciAyOTc0Njc3OTc5NDU6Ojpwcm90b2NvbCB0Y3A6Ojpmcm9tX3BvcnQgMjQ="> rather than <rule> <id> theID </id> ...
  * safely do ... blocks around ec2 invocations in ec2_driver

Also, this version adds the html interface for creating new rules. I copy/paste notes from original message here for convenience (ammended for the above changes):


This patch implements 'firewalls' - which are ec2 security groups. Some notes:

 * This functionality relies on some modifications to the appoxy aws gem - we have outstanding pull requests https://github.com/appoxy/aws/pull/89
(earlier one for security groups parser already in https://github.com/appoxy/aws/pull/81). Until these commits are pulled into aws the only way to test is with my branch (https://github.com/marios/aws):

    mkdir delme; cd delme; git clone git://github.com/marios/aws.git ; cd aws ; gem build aws.gemspec;  sudo gem install aws-2.4.5.gem

(ignore the version numbering of the gem - its just a remnant from when I created my fork - latest from appoxy is 2.5.2)

=======================================================================

 * XML looks like:

<firewall href='http://localhost:3001/api/firewalls/new_firewall' id='new_firewall'>
  <name><![CDATA[new_firewall]]></name>
  <description><![CDATA[new_one]]></description>
  <owner_id>297467797945</owner_id>
  <rules>
    <rule id="dXNlciAyOTc0Njc3OTc5NDU6Ojpwcm90b2NvbCB0Y3A6Ojpmcm9tX3BvcnQgMjQ=">
      <allow_protocol>tcp</allow_protocol>
      <port_from>0</port_from>
      <port_to>65535</port_to>
      <direction>ingress</direction>
      <sources>
        <source name='new_firewall' owner='123456789012' type='group'></source>
        <source address='10.1.1.1' family='ipv4' prefix='24' type='address'></source>
        <source address='192.168.1.1' family='ipv4' prefix='24' type='address'></source>
      </sources>
    </rule>
  </rules>
</firewall>

=======================================================================

 * OPERATIONS: implemented GET/POST/DELETE [list, create, destroy] for firewalls (both html and xml interfaces), GET/POST/DELETE for firewall rules. You can also use curl rather than html interface if you prefer:

create firewall rule:
curl -F "protocol=tcp" -F "from_port=22" -F "to_port=22" -F "ip_address1=192.168.1.1/24" -F "ip_address2=10.1.1.1/24" -F "group1=new_group" -F "group1owner=123456789"   --user 'aws_key:aws_secret_key' http://localhost:3001/api/firewalls/default/rules?format=xml
(and can specify additional sources for a given rule using ip_addressN and groupN/groupNowner)

list firewalls: curl   --user 'aws_key:aws_secret_key' http://localhost:3001/api/firewalls?format=xml

create new firewall: curl -F "name=some_new_firewall" -F "description=gonna be deleted immediately"  --user 'aws_key:aws_secret_key' http://localhost:3001/api/firewalls?format=xml

delete a firewall: curl -X DELETE  --user 'aws_key:aws_secret_key' http://localhost:3001/api/firewalls/some_new_firewall?format=xml

delete firewall rule: curl -X DELETE --user 'aws_key:aws_secret_key' http://localhost:3001/api/firewalls/firewall_id/rule_id?format=xml

=======================================================================

* Firewall rule ids... amazon doesn't have any notion of an 'id' for a single firewall rule, rather each firewall rule is identified by its constituent parts (protocol, from&to ports, and sources [groups and ipaddress ranges]). In order to allow for a 'delete /api/firewalls/:firewall/:rule' type operation I use Base64.encode to encode a unique UID for each rule using 'aws_owner_id protocol from_port to_port sources' - but this results in rather ugly looking uids... discussion/suggestions welcome,

I'm sure theres more but this is already way too long, thanks to anyone brave enough to try this stuff out,

all the best, marios

Re: firewalls - ec2 security groups - revision 1

Posted by "marios@redhat.com" <ma...@redhat.com>.
aws gem pull request for the security groups code (required by our 
'firewalls') has been merged

(https://github.com/appoxy/aws/commit/f5a227af8049725fb5b1cfd4d52c4b1634534b83) 


Travis Reeder (https://github.com/treeder) has offered to make a new 
release with this code included - once that's done we can think about 
pushing this code to trunk.

SO, now seems a good time to ask for a review :) - anyone have any 
thoughts on this code? Until aws v.2.5.4 becomes available you can build 
it using:

mkdir tempdir; cd tempdir; git clone git://github.com/appoxy/aws.git; cd 
aws; gem build aws.gemspec; sudo  gem install aws-2.5.3.gem

  [OPTIONAL cleanup:  cd ../../ ; rm -rf ./tempdir/ ]

marios

On 16/05/11 20:45, marios@redhat.com wrote:
> first revision of firewalls including suggestions by Michal:
>
>    * in xml output:<rule id="XNlciAyOTc0Njc3OTc5NDU6Ojpwcm90b2NvbCB0Y3A6Ojpmcm9tX3BvcnQgMjQ=">  rather than<rule>  <id>  theID</id>  ...
>    * safely do ... blocks around ec2 invocations in ec2_driver
>
> Also, this version adds the html interface for creating new rules. I copy/paste notes from original message here for convenience (ammended for the above changes):
>
>
> This patch implements 'firewalls' - which are ec2 security groups. Some notes:
>
>   * This functionality relies on some modifications to the appoxy aws gem - we have outstanding pull requests https://github.com/appoxy/aws/pull/89
> (earlier one for security groups parser already in https://github.com/appoxy/aws/pull/81). Until these commits are pulled into aws the only way to test is with my branch (https://github.com/marios/aws):
>
>      mkdir delme; cd delme; git clone git://github.com/marios/aws.git ; cd aws ; gem build aws.gemspec;  sudo gem install aws-2.4.5.gem
>
> (ignore the version numbering of the gem - its just a remnant from when I created my fork - latest from appoxy is 2.5.2)
>
> =======================================================================
>
>   * XML looks like:
>
> <firewall href='http://localhost:3001/api/firewalls/new_firewall' id='new_firewall'>
>    <name><![CDATA[new_firewall]]></name>
>    <description><![CDATA[new_one]]></description>
>    <owner_id>297467797945</owner_id>
>    <rules>
>      <rule id="dXNlciAyOTc0Njc3OTc5NDU6Ojpwcm90b2NvbCB0Y3A6Ojpmcm9tX3BvcnQgMjQ=">
>        <allow_protocol>tcp</allow_protocol>
>        <port_from>0</port_from>
>        <port_to>65535</port_to>
>        <direction>ingress</direction>
>        <sources>
>          <source name='new_firewall' owner='123456789012' type='group'></source>
>          <source address='10.1.1.1' family='ipv4' prefix='24' type='address'></source>
>          <source address='192.168.1.1' family='ipv4' prefix='24' type='address'></source>
>        </sources>
>      </rule>
>    </rules>
> </firewall>
>
> =======================================================================
>
>   * OPERATIONS: implemented GET/POST/DELETE [list, create, destroy] for firewalls (both html and xml interfaces), GET/POST/DELETE for firewall rules. You can also use curl rather than html interface if you prefer:
>
> create firewall rule:
> curl -F "protocol=tcp" -F "from_port=22" -F "to_port=22" -F "ip_address1=192.168.1.1/24" -F "ip_address2=10.1.1.1/24" -F "group1=new_group" -F "group1owner=123456789"   --user 'aws_key:aws_secret_key' http://localhost:3001/api/firewalls/default/rules?format=xml
> (and can specify additional sources for a given rule using ip_addressN and groupN/groupNowner)
>
> list firewalls: curl   --user 'aws_key:aws_secret_key' http://localhost:3001/api/firewalls?format=xml
>
> create new firewall: curl -F "name=some_new_firewall" -F "description=gonna be deleted immediately"  --user 'aws_key:aws_secret_key' http://localhost:3001/api/firewalls?format=xml
>
> delete a firewall: curl -X DELETE  --user 'aws_key:aws_secret_key' http://localhost:3001/api/firewalls/some_new_firewall?format=xml
>
> delete firewall rule: curl -X DELETE --user 'aws_key:aws_secret_key' http://localhost:3001/api/firewalls/firewall_id/rule_id?format=xml
>
> =======================================================================
>
> * Firewall rule ids... amazon doesn't have any notion of an 'id' for a single firewall rule, rather each firewall rule is identified by its constituent parts (protocol, from&to ports, and sources [groups and ipaddress ranges]). In order to allow for a 'delete /api/firewalls/:firewall/:rule' type operation I use Base64.encode to encode a unique UID for each rule using 'aws_owner_id protocol from_port to_port sources' - but this results in rather ugly looking uids... discussion/suggestions welcome,
>
> I'm sure theres more but this is already way too long, thanks to anyone brave enough to try this stuff out,
>
> all the best, marios


Re: [PATCH] implements 'firewalls' - ec2 security groups [revision 1]

Posted by "marios@redhat.com" <ma...@redhat.com>.
On 30/05/11 12:17, Michal Fojtik wrote:
> On May 16, 2011, at 7:45 PM, marios@redhat.com wrote:
>
> Hi,
>
> Sorry that I need to NACK this patch again, but I have same doubts as
> with previous revision. Seems like you fixed safely..end blocks but:
>
> * Please rename Firewall_Rule to FirewallRule
> * Please move 'create/delete' rule to Rabbit
>
> Otherwise this patch looks good and works flawlessly for me :-)
>

k thanks for review - just checked the original review and realised that 
i forgot to scroll down :) so didn't see these comments before  - will 
send rev2 (also, still waiting for aws 2.5.4 - the aws firewall rule 
changes were pulled but will be in next gem release)


marios

>    -- Michal
>
>> From: marios<ma...@redhat.com>
>>
>>
>> Signed-off-by: marios<ma...@redhat.com>
>> ---
>> server/deltacloud.rb                             |    2 +
>> server/lib/deltacloud/base_driver/base_driver.rb |    6 +-
>> server/lib/deltacloud/base_driver/features.rb    |    8 +-
>> server/lib/deltacloud/drivers/ec2/ec2_driver.rb  |  152 +++++++++++++++++++++-
>> server/lib/deltacloud/models/firewall.rb         |   22 +++
>> server/lib/deltacloud/models/firewall_rule.rb    |   23 ++++
>> server/public/javascripts/application.js         |   37 ++++++
>> server/server.rb                                 |  105 +++++++++++++++
>> server/views/firewalls/index.html.haml           |   25 ++++
>> server/views/firewalls/index.xml.haml            |   23 ++++
>> server/views/firewalls/new.html.haml             |   11 ++
>> server/views/firewalls/new_rule.html.haml        |   27 ++++
>> server/views/firewalls/show.html.haml            |   41 ++++++
>> server/views/firewalls/show.xml.haml             |   21 +++
>> 14 files changed, 495 insertions(+), 8 deletions(-)
>> create mode 100644 server/lib/deltacloud/models/firewall.rb
>> create mode 100644 server/lib/deltacloud/models/firewall_rule.rb
>> create mode 100644 server/views/firewalls/index.html.haml
>> create mode 100644 server/views/firewalls/index.xml.haml
>> create mode 100644 server/views/firewalls/new.html.haml
>> create mode 100644 server/views/firewalls/new_rule.html.haml
>> create mode 100644 server/views/firewalls/show.html.haml
>> create mode 100644 server/views/firewalls/show.xml.haml
>>
>> diff --git a/server/deltacloud.rb b/server/deltacloud.rb
>> index 7caf34f..5628e31 100644
>> --- a/server/deltacloud.rb
>> +++ b/server/deltacloud.rb
>> @@ -36,6 +36,8 @@ require 'deltacloud/models/storage_volume'
>> require 'deltacloud/models/bucket'
>> require 'deltacloud/models/blob'
>> require 'deltacloud/models/load_balancer'
>> +require 'deltacloud/models/firewall'
>> +require 'deltacloud/models/firewall_rule'
>>
>> require 'deltacloud/validation'
>> require 'deltacloud/helpers'
>> diff --git a/server/lib/deltacloud/base_driver/base_driver.rb b/server/lib/deltacloud/base_driver/base_driver.rb
>> index d9ebd92..06363d9 100644
>> --- a/server/lib/deltacloud/base_driver/base_driver.rb
>> +++ b/server/lib/deltacloud/base_driver/base_driver.rb
>> @@ -183,8 +183,12 @@ module Deltacloud
>>        keys(credentials, opts).first if has_capability?(:keys)
>>      end
>>
>> +    def firewall(credentials, opts={})
>> +      firewalls(credentials, opts).first if has_capability?(:firewalls)
>> +    end
>> +
>>      MEMBER_SHOW_METHODS =
>> -      [ :realm, :image, :instance, :storage_volume, :bucket, :blob, :key ]
>> +      [ :realm, :image, :instance, :storage_volume, :bucket, :blob, :key, :firewall ]
>>
>>      def has_capability?(capability)
>>        if MEMBER_SHOW_METHODS.include?(capability.to_sym)
>> 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"
>>        end
>>      end
>>
>> diff --git a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
>> index 14c5829..5a6d34d 100644
>> --- a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
>> +++ b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
>> @@ -16,6 +16,7 @@
>>
>> require 'deltacloud/base_driver'
>> require 'aws'
>> +require 'base64'
>>
>> class Instance
>>    attr_accessor :keyname
>> @@ -33,12 +34,13 @@ module Deltacloud
>>        class EC2Driver<  Deltacloud::BaseDriver
>>
>>          def supported_collections
>> -          DEFAULT_COLLECTIONS + [ :keys, :buckets, :load_balancers, :addresses ]
>> +
>> +          DEFAULT_COLLECTIONS + [ :keys, :buckets, :load_balancers, :addresses, :firewalls ]
>>          end
>>
>>          feature :instances, :user_data
>>          feature :instances, :authentication_key
>> -        feature :instances, :security_group
>> +        feature :instances, :firewall
>>          feature :instances, :instance_count
>>          feature :images, :owner_id
>>          feature :buckets, :bucket_location
>> @@ -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]
>>            instance_options.merge!(
>>              :min_count =>  opts[:instance_count],
>>              :max_count =>  opts[:instance_count]
>> @@ -516,6 +518,7 @@ module Deltacloud
>>            end
>>          end
>>
>> +
>
> An unnecessary space here IMHO :-)
>
>>          def addresses(credentials, opts={})
>>            ec2 = new_client(credentials)
>>            address_id = (opts and opts[:id]) ? [opts[:id]] : []
>> @@ -571,6 +574,73 @@ module Deltacloud
>>            end
>>          end
>>
>> +#--
>> +#FIREWALLS - ec2 security groups
>> +#--
>> +			def firewalls(credentials, opts={})
>
> Also this seems like a formatting issue to me.
> Dunno if you're using tabs instead of spaces, but please use spaces.
>
> In Vim you can do it using:
>
> :set expandtab
> :set tabstop=2
>
>> +          ec2 = new_client(credentials)
>> +          the_firewalls = []
>> +          groups = []
>> +          safely do
>> +            if opts[:id]
>> +              groups = ec2.describe_security_groups([opts[:id]])
>> +            else
>> +              groups = ec2.describe_security_groups()
>> +            end
>> +          end
>> +          groups.each do |security_group|
>> +            the_firewalls<<  convert_security_group(security_group)
>> +          end
>> +          filter_on(the_firewalls, :id, opts)
>> +		  end
>> +
>> +#--
>> +#Create firewall
>> +#--
>> +        def create_firewall(credentials, opts={})
>> +          ec2 = new_client(credentials)
>> +          safely do
>> +            ec2.create_security_group(opts["name"], opts["description"])
>> +          end
>> +          Firewall.new( { :id=>opts["name"], :name=>opts["name"],
>> +                          :description =>  opts["description"], :owner_id =>  "", :rules =>  [] } )
>> +        end
>> +
>> +#--
>> +#Delete firewall
>> +#--
>> +        def delete_firewall(credentials, opts={})
>> +          ec2 = new_client(credentials)
>> +          safely do
>> +            ec2.delete_security_group(opts["id"])
>> +          end
>> +        end
>> +#--
>> +#Create firewall rule
>> +#--
>> +        def create_firewall_rule(credentials, opts={})
>> +          ec2 = new_client(credentials)
>> +          groups = []
>> +          opts['groups'].each do |k,v|
>> +            groups<<  {"group_name" =>  k, "owner" =>v}
>> +          end
>> +          safely do
>> +            ec2.manage_security_group_ingress(opts['firewall'], opts['from_port'], opts['to_port'], opts['protocol'],
>> +              "authorize", opts['addresses'], groups)
>> +          end
>> +        end
>> +#--
>> +#Delete firewall rule
>> +#--
>> +        def delete_firewall_rule(credentials, opts={})
>> +          ec2 = new_client(credentials)
>> +          firewall = opts[:firewall]
>> +          protocol, from_port, to_port, addresses, groups = firewall_rule_params(opts[:rule_id])
>> +          safely do
>> +            ec2.manage_security_group_ingress(firewall, from_port, to_port, protocol, "revoke", addresses, groups)
>> +          end
>> +        end
>> +
>>          def valid_credentials?(credentials)
>>            retval = true
>>            begin
>> @@ -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)
>> +        end
>> +
>> +        #extract params from uid
>> +        def firewall_rule_params(id)
>> +          param_string = Base64.decode64(id)
>> +          # "#{user_id}:::#{protocol}:::#{from_port}:::#{to_port}:::#{sources_string}"
>> +          params = param_string.split(":::")
>> +          protocol = params.grep(/protocol/).first.split(" ").last
>> +          from_port = params.grep(/from_port/).first.split(" ").last
>> +          to_port = params.grep(/to_port/).first.split(" ").last
>> +          sources = params.grep(/sources/).first.split(" ").last
>> +          addresses = []
>> +          groups = []
>> +          sources.split("|").each do |source|
>> +            current = source.split("&")
>> +            type = current.grep(/type/).first.split("=").last
>> +            case type
>> +              when 'group'
>> +                #type=group&owner=123456789012&name=default
>> +                name = current.grep(/name/).first.split("=").last
>> +                owner = current.grep(/owner/).first.split("=").last
>> +                groups<<  {'group_name' =>  name, 'owner' =>  owner}
>> +              when 'address'
>> +                #type=address&family=ipv4&address=10.1.1.1&prefix=24
>> +                address = current.grep(/address/).last.split("=").last
>> +                address<<"/#{current.grep(/prefix/).first.split("=").last}"
>> +                addresses<<  address
>> +            end
>> +          end
>> +          return protocol, from_port, to_port, addresses, groups
>> +        end
>> +
>> +        #Convert ec2 security group to server/lib/deltacloud/models/firewall
>> +        def convert_security_group(security_group)
>> +          rules = []
>> +          security_group[:aws_perms].each do |perm|
>> +            sources = []
>> +            perm[:groups].each do |group|
>> +              sources<<  {:type =>  "group", :name =>  group[:group_name], :owner =>  group[:owner]}
>> +            end
>> +            perm[:ip_ranges].each do |ip|
>> +              sources<<  {:type =>  "address", :family=>"ipv4",
>> +                          :address=>ip[:cidr_ip].split("/").first,
>> +                          :prefix=>ip[:cidr_ip].split("/").last}
>> +            end
>> +            rule_id = firewall_rule_id(security_group[:aws_owner], perm[:protocol],
>> +                                       perm[:from_port] , perm[:to_port], sources)
>> +            rules<<  Firewall_Rule.new({:id =>  rule_id,
>
>
> Could you please change the class name 'Firewall_Rule' to 'FirewallRule' ?
>
>> +                                        :allow_protocol =>  perm[:protocol],
>> +                                        :port_from =>  perm[:from_port],
>> +                                        :port_to =>  perm[:to_port],
>> +                                        :direction =>  'ingress',
>> +                                        :sources =>  sources})
>> +          end
>> +          Firewall.new(  {  :id =>  security_group[:aws_group_name],
>> +                            :name =>  security_group[:aws_group_name],
>> +                            :description =>  security_group[:aws_description],
>> +                            :owner_id =>  security_group[:aws_owner],
>> +                            :rules =>  rules
>> +                      }  )
>> +        end
>> +
>>          def convert_state(ec2_state)
>>            case ec2_state
>>              when "terminated"
>> diff --git a/server/lib/deltacloud/models/firewall.rb b/server/lib/deltacloud/models/firewall.rb
>> new file mode 100644
>> index 0000000..dc0ae3d
>> --- /dev/null
>> +++ b/server/lib/deltacloud/models/firewall.rb
>> @@ -0,0 +1,22 @@
>> +#
>> +# Licensed to the Apache Software Foundation (ASF) under one or more
>> +# contributor license agreements.  See the NOTICE file distributed with
>> +# this work for additional information regarding copyright ownership.  The
>> +# ASF licenses this file to you under the Apache License, Version 2.0 (the
>> +# "License"); you may not use this file except in compliance with the
>> +# License.  You may obtain a copy of the License at
>> +#
>> +#       http://www.apache.org/licenses/LICENSE-2.0
>> +#
>> +# Unless required by applicable law or agreed to in writing, software
>> +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
>> +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
>> +# License for the specific language governing permissions and limitations
>> +# under the License.
>> +
>> +class Firewall<  BaseModel
>> +  attr_accessor :name
>> +  attr_accessor :description
>> +  attr_accessor :owner_id
>> +  attr_accessor :rules
>> +end
>> \ No newline at end of file
>> diff --git a/server/lib/deltacloud/models/firewall_rule.rb b/server/lib/deltacloud/models/firewall_rule.rb
>> new file mode 100644
>> index 0000000..df353a5
>> --- /dev/null
>> +++ b/server/lib/deltacloud/models/firewall_rule.rb
>> @@ -0,0 +1,23 @@
>> +#
>> +# Licensed to the Apache Software Foundation (ASF) under one or more
>> +# contributor license agreements.  See the NOTICE file distributed with
>> +# this work for additional information regarding copyright ownership.  The
>> +# ASF licenses this file to you under the Apache License, Version 2.0 (the
>> +# "License"); you may not use this file except in compliance with the
>> +# License.  You may obtain a copy of the License at
>> +#
>> +#       http://www.apache.org/licenses/LICENSE-2.0
>> +#
>> +# Unless required by applicable law or agreed to in writing, software
>> +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
>> +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
>> +# License for the specific language governing permissions and limitations
>> +# under the License.
>> +
>> +class Firewall_Rule<  BaseModel
>> +  attr_accessor :allow_protocol # tcp/udp/icmp
>> +  attr_accessor :port_from
>> +  attr_accessor :port_to
>> +  attr_accessor :sources
>> +  attr_accessor :direction #ingress egress
>> +end
>> diff --git a/server/public/javascripts/application.js b/server/public/javascripts/application.js
>> index 95c9bc2..29102c0 100644
>> --- a/server/public/javascripts/application.js
>> +++ b/server/public/javascripts/application.js
>> @@ -51,3 +51,40 @@ 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")
>> +	if(type == "address")
>> +	{
>> +		name = "ip_address" + eval(++addresses)
>> +		create_rule_source_field(name, "Address " + eval(addresses) + " [use CIDR notation 0.0.0.0/0]", form, button)
>> +	}
>> +  else if(type == "group")
>> +	{
>> +		name = "group" + eval(++groups)
>> +		create_rule_source_field(name, "Name of group " + eval(groups), form, button)
>> +		name = "group" + eval(groups) + "owner"
>> +		create_rule_source_field(name, "Group " + eval(groups) + " owner (required)", form, button)
>> +	}
>> +}
>> +
>> +function create_rule_source_field(name, label, form, button)
>> +{
>> +		element = document.createElement("INPUT")
>> +		element.type = "input"
>> +		element.size = 35
>> +		element.name = name
>> +		text = document.createTextNode(label)
>> +		form.insertBefore(element, button)
>> +		form.insertBefore(text, element)
>> +		form.insertBefore(document.createElement('BR'), element)
>> +		form.insertBefore(document.createElement('BR'), button)
>> +		form.insertBefore(document.createElement('BR'), button)
>> +		form.insertBefore(document.createElement('BR'), button)
>> +}
>
> I think this is OK, but we are including a JQuery library, so you can also
> use JQuery helpers to manipulate with JavaScript. No coding doubts on this,
> it's perfectly valid JavaScript code, but using JQuery will shrink it and make
> it more readable (at least for non-javascript-gurus (like /me ;))
>
>> +
>> +
>> diff --git a/server/server.rb b/server/server.rb
>> index 4da8357..a318df6 100644
>> --- a/server/server.rb
>> +++ b/server/server.rb
>> @@ -843,6 +843,7 @@ collection :buckets do
>>
>> end
>>
>> +
>> get '/api/addresses/:id/associate' do
>>    @instances = driver.instances(credentials)
>>    @address = Address::new(:id =>  params[:id])
>> @@ -929,3 +930,107 @@ collection :addresses do
>>    end
>>
>> end
>> +
>> +#html for creating a new firewall
>> +get '/api/firewalls/new' do
>> +  respond_to do |format|
>> +    format.html { haml :"firewalls/new" }
>> +  end
>> +end
>> +
>> +#html for creating a new firewall rule
>> +get '/api/firewalls/:firewall/new_rule' do
>> +  @firewall_name = params[:firewall]
>> +  respond_to do |format|
>> +    format.html {haml :"firewalls/new_rule" }
>> +  end
>> +end
>> +
>> +#create new firewall rule
>> +post '/api/firewalls/:firewall/rules' do
>> +  opts = {}
>> +  opts['firewall'] = params[:firewall] # must specify group_name or group_id
>> +  opts['protocol'] = params[:protocol]
>> +  opts['from_port'] = params[:from_port]
>> +  opts['to_port'] = params[:to_port]
>> +  addresses =  params.inject([]){|result,current| result<<  current.last unless current.grep(/^ip[-_]address/i).empty?; result}
>> +  groups = {}
>> +  max_groups  = params.select{|k,v| k=~/^group/}.size/2
>> +  for i in (1..max_groups) do
>> +    groups.merge!({params["group#{i}"]=>params["group#{i}owner"]})
>> +  end
>> +  opts['addresses'] = addresses
>> +  opts['groups'] = groups
>> +  driver.create_firewall_rule(credentials, opts)
>> +  @firewall = driver.firewall(credentials, {:id =>  params[:firewall]})
>> +  respond_to do |format|
>> +    format.html {haml :"firewalls/show"}
>> +    format.xml do
>> +      response.status = 201 #created
>> +      haml :"firewalls/show"
>> +    end
>> +  end
>> +end
>
> Please move this to Rabbit instead of using Sinatra 'post/get' helpers.
> At least for parts where it's possible (like I know that we are not covering
> 'new' actions, since they are using only from HTML UI).
>
> I mentioned this in previous mail, but it was not fixed. If you have any good
> reason to keep it as it is please tell me.
>
>> +#delete a firewall rule
>> +delete '/api/firewalls/:firewall/:rule' do
>> +  opts = {}
>> +  opts[:firewall] = params[:firewall]
>> +  opts[:rule_id] = params[:rule]
>> +  driver.delete_firewall_rule(credentials, opts)
>> +  respond_to do |format|
>> +    format.html {redirect firewall_url(params[:firewall])}
>> +    format.xml {204}
>> +    format.json {204}
>> +  end
>> +end
>> +
>> +
>> +collection :firewalls do
>> +  description "Allow user to define firewall rules for an instance (ec2 security groups) eg expose ssh access [port 22, tcp]."
>> +
>> +  operation :index do
>> +    description 'List all firewalls'
>> +    with_capability :firewalls
>> +    control { filter_all(:firewalls) }
>> +  end
>> +
>> +  operation :show do
>> +    description 'Show details for a specific firewall - list all rules'
>> +    with_capability :firewall
>> +    param :id,            :string,    :required
>> +    control { show(:firewall) }
>> +  end
>> +
>> +  operation :create do
>> +    description 'Create a new firewall'
>> +    with_capability :create_firewall
>> +    param :name,          :string,    :required
>> +    param :description,   :string,    :required
>> +    control do
>> +      @firewall = driver.create_firewall(credentials, params )
>> +      respond_to do |format|
>> +        format.xml do
>> +          response.status = 201  # Created
>> +          haml :"firewalls/show"
>> +        end
>> +        format.html {haml :"firewalls/show"}
>> +      end
>> +    end
>> +  end
>> +
>> +  operation :destroy do
>> +    description 'Delete a specified firewall - error if firewall has rules'
>> +    with_capability :delete_firewall
>> +    param :id,            :string,    :required
>> +    control do
>> +      driver.delete_firewall(credentials, params)
>> +      respond_to do |format|
>> +        format.xml { 204 }
>> +        format.json {  204 }
>> +        format.html {  redirect(firewalls_url) }
>> +      end
>> +    end
>> +  end
>> +
>> +end #firewalls
>> diff --git a/server/views/firewalls/index.html.haml b/server/views/firewalls/index.html.haml
>> new file mode 100644
>> index 0000000..3312a32
>> --- /dev/null
>> +++ b/server/views/firewalls/index.html.haml
>> @@ -0,0 +1,25 @@
>> +%h1 Firewalls
>> +%br
>> +%p
>> +  =link_to 'Create new firewall', "/api/firewalls/new"
>> +%table.display
>> +  %thead
>> +    %tr
>> +      %th Id
>> +      %th Name
>> +      %th Description
>> +      %th Owner ID
>> +      %th Rules
>> +  %tbody
>> +    - @firewalls.each do |firewall|
>> +      %tr
>> +        %td
>> +          = link_to firewall.id, firewall_url(firewall.id)
>> +        %td
>> +          = firewall.name
>> +        %td
>> +          = firewall.description
>> +        %td
>> +          = firewall.owner_id
>> +        %td
>> +          = link_to 'view rules', firewall_url(firewall.id)
>> diff --git a/server/views/firewalls/index.xml.haml b/server/views/firewalls/index.xml.haml
>> new file mode 100644
>> index 0000000..f027785
>> --- /dev/null
>> +++ b/server/views/firewalls/index.xml.haml
>> @@ -0,0 +1,23 @@
>> +!!! XML
>> +%firewalls
>> +  - @firewalls.each do |firewall|
>> +    %firewall{:href =>  firewall_url(firewall.id), :id =>  firewall.id}
>> +      - firewall.attributes.select{ |attr| attr != :id&&  attr!= :rules}.each do |attribute|
>> +        - haml_tag("#{attribute}".tr('-', '_'), :<) do
>> +          - if [:name, :description].include?(attribute)
>> +            =cdata do
>> +              - haml_concat firewall.send(attribute)
>> +          - else
>> +            - haml_concat firewall.send(attribute)
>> +      %rules
>> +        - firewall.rules.each do |rule|
>> +          %rule{:id =>  rule.id}
>> +            - rule.attributes.select{|attr| attr != :sources&&  attr != :id}.each do |rule_attrib|
>> +              - haml_tag("#{rule_attrib}".tr('-', '_'), :<) do
>> +                - haml_concat rule.send(rule_attrib)
>> +            %sources
>> +              - rule.sources.each do |source|
>> +                - if source[:type] == "group"
>> +                  %source{:name =>  source[:name], :type=>  source[:type], :owner=>  source[:owner]}
>> +                - else
>> +                  %source{:prefix =>  source[:prefix], :address=>  source[:address], :family=>source[:family], :type =>  source[:type]}
>> \ No newline at end of file
>> diff --git a/server/views/firewalls/new.html.haml b/server/views/firewalls/new.html.haml
>> new file mode 100644
>> index 0000000..4a230a6
>> --- /dev/null
>> +++ b/server/views/firewalls/new.html.haml
>> @@ -0,0 +1,11 @@
>> +%h1 New Firewall
>> +
>> +%form{:action =>  firewalls_url, :method =>  :post}
>> +  %label
>> +    Firewall Name
>> +    %input{:name =>  'name', :size =>  25}/
>> +    %br
>> +  %label
>> +    Firewall Description
>> +    %input{:name =>  'description', :size =>  100}/
>> +  %input{:type =>  :submit, :name =>  "commit", :value=>"create"}
>> \ No newline at end of file
>> diff --git a/server/views/firewalls/new_rule.html.haml b/server/views/firewalls/new_rule.html.haml
>> new file mode 100644
>> index 0000000..a3bee24
>> --- /dev/null
>> +++ b/server/views/firewalls/new_rule.html.haml
>> @@ -0,0 +1,27 @@
>> +%h1 New Firewall Rule
>> +
>> +%form{ :action =>  "#{firewall_url(@firewall_name)}/rules", :id =>  "new_rule_form", :method =>  :post, :enctype =>  'multipart/form-data'}
>> +  %label
>> +    Protocol:
>> +    %br
>> +    %input{ :name =>  'protocol', :size =>  10}/
>> +    %br
>> +    %br
>> +  %label
>> +    From port:
>> +    %br
>> +    %input{ :name =>  'from_port', :size =>  10}/
>> +    %br
>> +    %br
>> +    To port:
>> +    %br
>> +    %input{ :name =>  'to_port', :size =>  10}/
>> +    %br
>> +    %br
>> +  %a{ :href =>  "javascript:;", :onclick =>  "make_fields('address');"} Add source IP address
>> +  %br
>> +  %a{ :href =>  "javascript:;", :onclick =>  "make_fields('group');"} Add source group
>> +  %br
>> +  %br
>> +  %input{ :type =>  :submit, :id =>  "submit_button", :name =>  "commit", :value =>  "create"}/
>> +
>> diff --git a/server/views/firewalls/show.html.haml b/server/views/firewalls/show.html.haml
>> new file mode 100644
>> index 0000000..e6a186f
>> --- /dev/null
>> +++ b/server/views/firewalls/show.html.haml
>> @@ -0,0 +1,41 @@
>> +%h1 Firewall
>> +%h2
>> +  = @firewall.id
>> +%dl
>> +  %di
>> +    %dt Name
>> +    %dd
>> +      = @firewall.name
>> +    %dt Owner
>> +    %dd
>> +      = @firewall.owner_id
>> +    %dt Description
>> +    %dd
>> +      = @firewall.description
>> +
>> +%h2
>> +  Rules
>> +  %br
>> +  %p
>> +    =link_to 'Create a new rule', "/api/firewalls/#{@firewall.name}/new_rule"
>> +%dl
>> +  - @firewall.rules.each do |rule|
>> +    %di
>> +      Rule
>> +      - rule.attributes.select{|attr| attr != :sources}.each do |attrib|
>> +        %dt #{attrib}
>> +        %dd
>> +          = rule.send(attrib)
>> +      %dt sources
>> +      %dd
>> +        - rule.sources.each do |source|
>> +          - if source[:type] == "group"
>> +            type: #{source[:type]}, name: #{source[:name]}, owner: #{source[:owner]}
>> +            %br
>> +          - else
>> +            type: #{source[:type]}, family: #{source[:family]}, address: #{source[:address]}, prefix: #{source[:prefix]}
>> +            %br
>> +      %dd
>> +        = link_to_action 'Delete Rule', destroy_firewall_url(@firewall.name) + "/#{rule.id}", :delete
>> +  %dd
>> +    = link_to_action 'Delete Firewall', destroy_firewall_url(@firewall.name), :delete
>> \ No newline at end of file
>> diff --git a/server/views/firewalls/show.xml.haml b/server/views/firewalls/show.xml.haml
>> new file mode 100644
>> index 0000000..9d1fc48
>> --- /dev/null
>> +++ b/server/views/firewalls/show.xml.haml
>> @@ -0,0 +1,21 @@
>> +!!! XML
>> +%firewall{:href =>  firewall_url(@firewall.id), :id =>  @firewall.id}
>> +  - @firewall.attributes.select{ |attr| attr != :id&&  attr!= :rules}.each do |attribute|
>> +    - haml_tag("#{attribute}".tr('-', '_'), :<) do
>> +      - if [:name, :description].include?(attribute)
>> +        =cdata do
>> +          - haml_concat @firewall.send(attribute)
>> +      - else
>> +        - haml_concat @firewall.send(attribute)
>> +  %rules
>> +    - @firewall.rules.each do |rule|
>> +      %rule{:id =>  rule.id}
>> +        - rule.attributes.select{|attr| attr != :sources&&  attr != :id}.each do |rule_attrib|
>> +          - haml_tag("#{rule_attrib}".tr('-', '_'), :<) do
>> +            - haml_concat rule.send(rule_attrib)
>> +        %sources
>> +          - rule.sources.each do |source|
>> +            - if source[:type] == "group"
>> +              %source{:name =>  source[:name], :type=>  source[:type], :owner=>source[:owner]}
>> +            - else
>> +              %source{:prefix =>  source[:prefix], :address=>  source[:address], :family=>source[:family], :type =>  source[:type]}
>> \ No newline at end of file
>> --
>> 1.7.3.4
>>
>
> ------------------------------------------------------
> Michal Fojtik, mfojtik@redhat.com
> Deltacloud API: http://deltacloud.org
>


Re: [PATCH] implements 'firewalls' - ec2 security groups [revision 1]

Posted by Michal Fojtik <mf...@redhat.com>.
On May 16, 2011, at 7:45 PM, marios@redhat.com wrote:

Hi,

Sorry that I need to NACK this patch again, but I have same doubts as
with previous revision. Seems like you fixed safely..end blocks but:

* Please rename Firewall_Rule to FirewallRule
* Please move 'create/delete' rule to Rabbit

Otherwise this patch looks good and works flawlessly for me :-)

  -- Michal

> From: marios <ma...@redhat.com>
> 
> 
> Signed-off-by: marios <ma...@redhat.com>
> ---
> server/deltacloud.rb                             |    2 +
> server/lib/deltacloud/base_driver/base_driver.rb |    6 +-
> server/lib/deltacloud/base_driver/features.rb    |    8 +-
> server/lib/deltacloud/drivers/ec2/ec2_driver.rb  |  152 +++++++++++++++++++++-
> server/lib/deltacloud/models/firewall.rb         |   22 +++
> server/lib/deltacloud/models/firewall_rule.rb    |   23 ++++
> server/public/javascripts/application.js         |   37 ++++++
> server/server.rb                                 |  105 +++++++++++++++
> server/views/firewalls/index.html.haml           |   25 ++++
> server/views/firewalls/index.xml.haml            |   23 ++++
> server/views/firewalls/new.html.haml             |   11 ++
> server/views/firewalls/new_rule.html.haml        |   27 ++++
> server/views/firewalls/show.html.haml            |   41 ++++++
> server/views/firewalls/show.xml.haml             |   21 +++
> 14 files changed, 495 insertions(+), 8 deletions(-)
> create mode 100644 server/lib/deltacloud/models/firewall.rb
> create mode 100644 server/lib/deltacloud/models/firewall_rule.rb
> create mode 100644 server/views/firewalls/index.html.haml
> create mode 100644 server/views/firewalls/index.xml.haml
> create mode 100644 server/views/firewalls/new.html.haml
> create mode 100644 server/views/firewalls/new_rule.html.haml
> create mode 100644 server/views/firewalls/show.html.haml
> create mode 100644 server/views/firewalls/show.xml.haml
> 
> diff --git a/server/deltacloud.rb b/server/deltacloud.rb
> index 7caf34f..5628e31 100644
> --- a/server/deltacloud.rb
> +++ b/server/deltacloud.rb
> @@ -36,6 +36,8 @@ require 'deltacloud/models/storage_volume'
> require 'deltacloud/models/bucket'
> require 'deltacloud/models/blob'
> require 'deltacloud/models/load_balancer'
> +require 'deltacloud/models/firewall'
> +require 'deltacloud/models/firewall_rule'
> 
> require 'deltacloud/validation'
> require 'deltacloud/helpers'
> diff --git a/server/lib/deltacloud/base_driver/base_driver.rb b/server/lib/deltacloud/base_driver/base_driver.rb
> index d9ebd92..06363d9 100644
> --- a/server/lib/deltacloud/base_driver/base_driver.rb
> +++ b/server/lib/deltacloud/base_driver/base_driver.rb
> @@ -183,8 +183,12 @@ module Deltacloud
>       keys(credentials, opts).first if has_capability?(:keys)
>     end
> 
> +    def firewall(credentials, opts={})
> +      firewalls(credentials, opts).first if has_capability?(:firewalls)
> +    end
> +
>     MEMBER_SHOW_METHODS =
> -      [ :realm, :image, :instance, :storage_volume, :bucket, :blob, :key ]
> +      [ :realm, :image, :instance, :storage_volume, :bucket, :blob, :key, :firewall ]
> 
>     def has_capability?(capability)
>       if MEMBER_SHOW_METHODS.include?(capability.to_sym)
> 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"
>       end
>     end
> 
> diff --git a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> index 14c5829..5a6d34d 100644
> --- a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> +++ b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> @@ -16,6 +16,7 @@
> 
> require 'deltacloud/base_driver'
> require 'aws'
> +require 'base64'
> 
> class Instance
>   attr_accessor :keyname
> @@ -33,12 +34,13 @@ module Deltacloud
>       class EC2Driver < Deltacloud::BaseDriver
> 
>         def supported_collections
> -          DEFAULT_COLLECTIONS + [ :keys, :buckets, :load_balancers, :addresses ]
> +
> +          DEFAULT_COLLECTIONS + [ :keys, :buckets, :load_balancers, :addresses, :firewalls ]
>         end
> 
>         feature :instances, :user_data
>         feature :instances, :authentication_key
> -        feature :instances, :security_group
> +        feature :instances, :firewall
>         feature :instances, :instance_count
>         feature :images, :owner_id
>         feature :buckets, :bucket_location
> @@ -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]
>           instance_options.merge!(
>             :min_count => opts[:instance_count],
>             :max_count => opts[:instance_count]
> @@ -516,6 +518,7 @@ module Deltacloud
>           end
>         end
> 
> +

An unnecessary space here IMHO :-)

>         def addresses(credentials, opts={})
>           ec2 = new_client(credentials)
>           address_id = (opts and opts[:id]) ? [opts[:id]] : []
> @@ -571,6 +574,73 @@ module Deltacloud
>           end
>         end
> 
> +#--
> +#FIREWALLS - ec2 security groups
> +#--
> +			def firewalls(credentials, opts={})

Also this seems like a formatting issue to me.
Dunno if you're using tabs instead of spaces, but please use spaces.

In Vim you can do it using:

:set expandtab
:set tabstop=2

> +          ec2 = new_client(credentials)
> +          the_firewalls = []
> +          groups = []
> +          safely do
> +            if opts[:id]
> +              groups = ec2.describe_security_groups([opts[:id]])
> +            else
> +              groups = ec2.describe_security_groups()
> +            end
> +          end
> +          groups.each do |security_group|
> +            the_firewalls << convert_security_group(security_group)
> +          end
> +          filter_on(the_firewalls, :id, opts)
> +		  end
> +
> +#--
> +#Create firewall
> +#--
> +        def create_firewall(credentials, opts={})
> +          ec2 = new_client(credentials)
> +          safely do
> +            ec2.create_security_group(opts["name"], opts["description"])
> +          end
> +          Firewall.new( { :id=>opts["name"], :name=>opts["name"],
> +                          :description => opts["description"], :owner_id => "", :rules => [] } )
> +        end
> +
> +#--
> +#Delete firewall
> +#--
> +        def delete_firewall(credentials, opts={})
> +          ec2 = new_client(credentials)
> +          safely do
> +            ec2.delete_security_group(opts["id"])
> +          end
> +        end
> +#--
> +#Create firewall rule
> +#--
> +        def create_firewall_rule(credentials, opts={})
> +          ec2 = new_client(credentials)
> +          groups = []
> +          opts['groups'].each do |k,v|
> +            groups << {"group_name" => k, "owner" =>v}
> +          end
> +          safely do
> +            ec2.manage_security_group_ingress(opts['firewall'], opts['from_port'], opts['to_port'], opts['protocol'],
> +              "authorize", opts['addresses'], groups)
> +          end
> +        end
> +#--
> +#Delete firewall rule
> +#--
> +        def delete_firewall_rule(credentials, opts={})
> +          ec2 = new_client(credentials)
> +          firewall = opts[:firewall]
> +          protocol, from_port, to_port, addresses, groups = firewall_rule_params(opts[:rule_id])
> +          safely do
> +            ec2.manage_security_group_ingress(firewall, from_port, to_port, protocol, "revoke", addresses, groups)
> +          end
> +        end
> +
>         def valid_credentials?(credentials)
>           retval = true
>           begin
> @@ -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)
> +        end
> +
> +        #extract params from uid
> +        def firewall_rule_params(id)
> +          param_string = Base64.decode64(id)
> +          # "#{user_id}:::#{protocol}:::#{from_port}:::#{to_port}:::#{sources_string}"
> +          params = param_string.split(":::")
> +          protocol = params.grep(/protocol/).first.split(" ").last
> +          from_port = params.grep(/from_port/).first.split(" ").last
> +          to_port = params.grep(/to_port/).first.split(" ").last
> +          sources = params.grep(/sources/).first.split(" ").last
> +          addresses = []
> +          groups = []
> +          sources.split("|").each do |source|
> +            current = source.split("&")
> +            type = current.grep(/type/).first.split("=").last
> +            case type
> +              when 'group'
> +                #type=group&owner=123456789012&name=default
> +                name = current.grep(/name/).first.split("=").last
> +                owner = current.grep(/owner/).first.split("=").last
> +                groups << {'group_name' => name, 'owner' => owner}
> +              when 'address'
> +                #type=address&family=ipv4&address=10.1.1.1&prefix=24
> +                address = current.grep(/address/).last.split("=").last
> +                address<<"/#{current.grep(/prefix/).first.split("=").last}"
> +                addresses << address
> +            end
> +          end
> +          return protocol, from_port, to_port, addresses, groups
> +        end
> +
> +        #Convert ec2 security group to server/lib/deltacloud/models/firewall
> +        def convert_security_group(security_group)
> +          rules = []
> +          security_group[:aws_perms].each do |perm|
> +            sources = []
> +            perm[:groups].each do |group|
> +              sources << {:type => "group", :name => group[:group_name], :owner => group[:owner]}
> +            end
> +            perm[:ip_ranges].each do |ip|
> +              sources << {:type => "address", :family=>"ipv4",
> +                          :address=>ip[:cidr_ip].split("/").first,
> +                          :prefix=>ip[:cidr_ip].split("/").last}
> +            end
> +            rule_id = firewall_rule_id(security_group[:aws_owner], perm[:protocol],
> +                                       perm[:from_port] , perm[:to_port], sources)
> +            rules << Firewall_Rule.new({:id => rule_id,


Could you please change the class name 'Firewall_Rule' to 'FirewallRule' ?

> +                                        :allow_protocol => perm[:protocol],
> +                                        :port_from => perm[:from_port],
> +                                        :port_to => perm[:to_port],
> +                                        :direction => 'ingress',
> +                                        :sources => sources})
> +          end
> +          Firewall.new(  {  :id => security_group[:aws_group_name],
> +                            :name => security_group[:aws_group_name],
> +                            :description => security_group[:aws_description],
> +                            :owner_id => security_group[:aws_owner],
> +                            :rules => rules
> +                      }  )
> +        end
> +
>         def convert_state(ec2_state)
>           case ec2_state
>             when "terminated"
> diff --git a/server/lib/deltacloud/models/firewall.rb b/server/lib/deltacloud/models/firewall.rb
> new file mode 100644
> index 0000000..dc0ae3d
> --- /dev/null
> +++ b/server/lib/deltacloud/models/firewall.rb
> @@ -0,0 +1,22 @@
> +#
> +# Licensed to the Apache Software Foundation (ASF) under one or more
> +# contributor license agreements.  See the NOTICE file distributed with
> +# this work for additional information regarding copyright ownership.  The
> +# ASF licenses this file to you under the Apache License, Version 2.0 (the
> +# "License"); you may not use this file except in compliance with the
> +# License.  You may obtain a copy of the License at
> +#
> +#       http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
> +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
> +# License for the specific language governing permissions and limitations
> +# under the License.
> +
> +class Firewall < BaseModel
> +  attr_accessor :name
> +  attr_accessor :description
> +  attr_accessor :owner_id
> +  attr_accessor :rules
> +end
> \ No newline at end of file
> diff --git a/server/lib/deltacloud/models/firewall_rule.rb b/server/lib/deltacloud/models/firewall_rule.rb
> new file mode 100644
> index 0000000..df353a5
> --- /dev/null
> +++ b/server/lib/deltacloud/models/firewall_rule.rb
> @@ -0,0 +1,23 @@
> +#
> +# Licensed to the Apache Software Foundation (ASF) under one or more
> +# contributor license agreements.  See the NOTICE file distributed with
> +# this work for additional information regarding copyright ownership.  The
> +# ASF licenses this file to you under the Apache License, Version 2.0 (the
> +# "License"); you may not use this file except in compliance with the
> +# License.  You may obtain a copy of the License at
> +#
> +#       http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
> +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
> +# License for the specific language governing permissions and limitations
> +# under the License.
> +
> +class Firewall_Rule < BaseModel
> +  attr_accessor :allow_protocol # tcp/udp/icmp
> +  attr_accessor :port_from
> +  attr_accessor :port_to
> +  attr_accessor :sources
> +  attr_accessor :direction #ingress egress
> +end
> diff --git a/server/public/javascripts/application.js b/server/public/javascripts/application.js
> index 95c9bc2..29102c0 100644
> --- a/server/public/javascripts/application.js
> +++ b/server/public/javascripts/application.js
> @@ -51,3 +51,40 @@ 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")
> +	if(type == "address")
> +	{
> +		name = "ip_address" + eval(++addresses)
> +		create_rule_source_field(name, "Address " + eval(addresses) + " [use CIDR notation 0.0.0.0/0]", form, button) 
> +	}
> +  else if(type == "group")
> +	{
> +		name = "group" + eval(++groups)
> +		create_rule_source_field(name, "Name of group " + eval(groups), form, button)
> +		name = "group" + eval(groups) + "owner"
> +		create_rule_source_field(name, "Group " + eval(groups) + " owner (required)", form, button)
> +	}
> +}
> +
> +function create_rule_source_field(name, label, form, button)
> +{
> +		element = document.createElement("INPUT")
> +		element.type = "input"
> +		element.size = 35
> +		element.name = name
> +		text = document.createTextNode(label)
> +		form.insertBefore(element, button)
> +		form.insertBefore(text, element)
> +		form.insertBefore(document.createElement('BR'), element)
> +		form.insertBefore(document.createElement('BR'), button)
> +		form.insertBefore(document.createElement('BR'), button)
> +		form.insertBefore(document.createElement('BR'), button)
> +}

I think this is OK, but we are including a JQuery library, so you can also
use JQuery helpers to manipulate with JavaScript. No coding doubts on this,
it's perfectly valid JavaScript code, but using JQuery will shrink it and make
it more readable (at least for non-javascript-gurus (like /me ;))

> +
> +
> diff --git a/server/server.rb b/server/server.rb
> index 4da8357..a318df6 100644
> --- a/server/server.rb
> +++ b/server/server.rb
> @@ -843,6 +843,7 @@ collection :buckets do
> 
> end
> 
> +
> get '/api/addresses/:id/associate' do
>   @instances = driver.instances(credentials)
>   @address = Address::new(:id => params[:id])
> @@ -929,3 +930,107 @@ collection :addresses do
>   end
> 
> end
> +
> +#html for creating a new firewall
> +get '/api/firewalls/new' do
> +  respond_to do |format|
> +    format.html { haml :"firewalls/new" }
> +  end
> +end
> +
> +#html for creating a new firewall rule
> +get '/api/firewalls/:firewall/new_rule' do
> +  @firewall_name = params[:firewall]
> +  respond_to do |format|
> +    format.html {haml :"firewalls/new_rule" }
> +  end
> +end
> +
> +#create new firewall rule
> +post '/api/firewalls/:firewall/rules' do
> +  opts = {}
> +  opts['firewall'] = params[:firewall] # must specify group_name or group_id
> +  opts['protocol'] = params[:protocol]
> +  opts['from_port'] = params[:from_port]
> +  opts['to_port'] = params[:to_port]
> +  addresses =  params.inject([]){|result,current| result << current.last unless current.grep(/^ip[-_]address/i).empty?; result}
> +  groups = {}
> +  max_groups  = params.select{|k,v| k=~/^group/}.size/2
> +  for i in (1..max_groups) do
> +    groups.merge!({params["group#{i}"]=>params["group#{i}owner"]})
> +  end
> +  opts['addresses'] = addresses
> +  opts['groups'] = groups
> +  driver.create_firewall_rule(credentials, opts)
> +  @firewall = driver.firewall(credentials, {:id => params[:firewall]})
> +  respond_to do |format|
> +    format.html {haml :"firewalls/show"}
> +    format.xml do
> +      response.status = 201 #created
> +      haml :"firewalls/show"
> +    end
> +  end
> +end

Please move this to Rabbit instead of using Sinatra 'post/get' helpers.
At least for parts where it's possible (like I know that we are not covering
'new' actions, since they are using only from HTML UI).

I mentioned this in previous mail, but it was not fixed. If you have any good
reason to keep it as it is please tell me.

> +#delete a firewall rule
> +delete '/api/firewalls/:firewall/:rule' do
> +  opts = {}
> +  opts[:firewall] = params[:firewall]
> +  opts[:rule_id] = params[:rule]
> +  driver.delete_firewall_rule(credentials, opts)
> +  respond_to do |format|
> +    format.html {redirect firewall_url(params[:firewall])}
> +    format.xml {204}
> +    format.json {204}
> +  end
> +end
> +
> +
> +collection :firewalls do
> +  description "Allow user to define firewall rules for an instance (ec2 security groups) eg expose ssh access [port 22, tcp]."
> +
> +  operation :index do
> +    description 'List all firewalls'
> +    with_capability :firewalls
> +    control { filter_all(:firewalls) }
> +  end
> +
> +  operation :show do
> +    description 'Show details for a specific firewall - list all rules'
> +    with_capability :firewall
> +    param :id,            :string,    :required
> +    control { show(:firewall) }
> +  end
> +
> +  operation :create do
> +    description 'Create a new firewall'
> +    with_capability :create_firewall
> +    param :name,          :string,    :required
> +    param :description,   :string,    :required
> +    control do
> +      @firewall = driver.create_firewall(credentials, params )
> +      respond_to do |format|
> +        format.xml do
> +          response.status = 201  # Created
> +          haml :"firewalls/show"
> +        end
> +        format.html {haml :"firewalls/show"}
> +      end
> +    end
> +  end
> +
> +  operation :destroy do
> +    description 'Delete a specified firewall - error if firewall has rules'
> +    with_capability :delete_firewall
> +    param :id,            :string,    :required
> +    control do
> +      driver.delete_firewall(credentials, params)
> +      respond_to do |format|
> +        format.xml { 204 }
> +        format.json {  204 }
> +        format.html {  redirect(firewalls_url) }
> +      end
> +    end
> +  end
> +
> +end #firewalls
> diff --git a/server/views/firewalls/index.html.haml b/server/views/firewalls/index.html.haml
> new file mode 100644
> index 0000000..3312a32
> --- /dev/null
> +++ b/server/views/firewalls/index.html.haml
> @@ -0,0 +1,25 @@
> +%h1 Firewalls
> +%br
> +%p
> +  =link_to 'Create new firewall', "/api/firewalls/new"
> +%table.display
> +  %thead
> +    %tr
> +      %th Id
> +      %th Name
> +      %th Description
> +      %th Owner ID
> +      %th Rules
> +  %tbody
> +    - @firewalls.each do |firewall|
> +      %tr
> +        %td
> +          = link_to firewall.id, firewall_url(firewall.id)
> +        %td
> +          = firewall.name
> +        %td
> +          = firewall.description
> +        %td
> +          = firewall.owner_id
> +        %td
> +          = link_to 'view rules', firewall_url(firewall.id)
> diff --git a/server/views/firewalls/index.xml.haml b/server/views/firewalls/index.xml.haml
> new file mode 100644
> index 0000000..f027785
> --- /dev/null
> +++ b/server/views/firewalls/index.xml.haml
> @@ -0,0 +1,23 @@
> +!!! XML
> +%firewalls
> +  - @firewalls.each do |firewall|
> +    %firewall{:href => firewall_url(firewall.id), :id => firewall.id}
> +      - firewall.attributes.select{ |attr| attr != :id && attr!= :rules}.each do |attribute|
> +        - haml_tag("#{attribute}".tr('-', '_'), :<) do
> +          - if [:name, :description].include?(attribute)
> +            =cdata do
> +              - haml_concat firewall.send(attribute)
> +          - else
> +            - haml_concat firewall.send(attribute)
> +      %rules
> +        - firewall.rules.each do |rule|
> +          %rule{:id => rule.id}
> +            - rule.attributes.select{|attr| attr != :sources && attr != :id}.each do |rule_attrib|
> +              - haml_tag("#{rule_attrib}".tr('-', '_'), :<) do
> +                - haml_concat rule.send(rule_attrib)
> +            %sources
> +              - rule.sources.each do |source|
> +                - if source[:type] == "group"
> +                  %source{:name => source[:name], :type=> source[:type], :owner=> source[:owner]}
> +                - else
> +                  %source{:prefix => source[:prefix], :address=> source[:address], :family=>source[:family], :type => source[:type]}
> \ No newline at end of file
> diff --git a/server/views/firewalls/new.html.haml b/server/views/firewalls/new.html.haml
> new file mode 100644
> index 0000000..4a230a6
> --- /dev/null
> +++ b/server/views/firewalls/new.html.haml
> @@ -0,0 +1,11 @@
> +%h1 New Firewall
> +
> +%form{:action => firewalls_url, :method => :post}
> +  %label
> +    Firewall Name
> +    %input{:name => 'name', :size => 25}/
> +    %br
> +  %label
> +    Firewall Description
> +    %input{:name => 'description', :size => 100}/
> +  %input{:type => :submit, :name => "commit", :value=>"create"}
> \ No newline at end of file
> diff --git a/server/views/firewalls/new_rule.html.haml b/server/views/firewalls/new_rule.html.haml
> new file mode 100644
> index 0000000..a3bee24
> --- /dev/null
> +++ b/server/views/firewalls/new_rule.html.haml
> @@ -0,0 +1,27 @@
> +%h1 New Firewall Rule
> +
> +%form{ :action => "#{firewall_url(@firewall_name)}/rules", :id => "new_rule_form", :method => :post, :enctype => 'multipart/form-data'}
> +  %label
> +    Protocol:
> +    %br
> +    %input{ :name => 'protocol', :size => 10}/
> +    %br 
> +    %br
> +  %label
> +    From port:
> +    %br
> +    %input{ :name => 'from_port', :size => 10}/
> +    %br
> +    %br
> +    To port:
> +    %br
> +    %input{ :name => 'to_port', :size => 10}/
> +    %br
> +    %br
> +  %a{ :href => "javascript:;", :onclick => "make_fields('address');"} Add source IP address
> +  %br
> +  %a{ :href => "javascript:;", :onclick => "make_fields('group');"} Add source group
> +  %br
> +  %br
> +  %input{ :type => :submit, :id => "submit_button", :name => "commit", :value => "create"}/
> +
> diff --git a/server/views/firewalls/show.html.haml b/server/views/firewalls/show.html.haml
> new file mode 100644
> index 0000000..e6a186f
> --- /dev/null
> +++ b/server/views/firewalls/show.html.haml
> @@ -0,0 +1,41 @@
> +%h1 Firewall
> +%h2
> +  = @firewall.id
> +%dl
> +  %di
> +    %dt Name
> +    %dd
> +      = @firewall.name
> +    %dt Owner
> +    %dd
> +      = @firewall.owner_id
> +    %dt Description
> +    %dd
> +      = @firewall.description
> +
> +%h2
> +  Rules
> +  %br
> +  %p
> +    =link_to 'Create a new rule', "/api/firewalls/#{@firewall.name}/new_rule"
> +%dl
> +  - @firewall.rules.each do |rule|
> +    %di
> +      Rule
> +      - rule.attributes.select{|attr| attr != :sources}.each do |attrib|
> +        %dt #{attrib}
> +        %dd
> +          = rule.send(attrib)
> +      %dt sources
> +      %dd
> +        - rule.sources.each do |source|
> +          - if source[:type] == "group"
> +            type: #{source[:type]}, name: #{source[:name]}, owner: #{source[:owner]}
> +            %br
> +          - else
> +            type: #{source[:type]}, family: #{source[:family]}, address: #{source[:address]}, prefix: #{source[:prefix]}
> +            %br
> +      %dd
> +        = link_to_action 'Delete Rule', destroy_firewall_url(@firewall.name) + "/#{rule.id}", :delete
> +  %dd
> +    = link_to_action 'Delete Firewall', destroy_firewall_url(@firewall.name), :delete
> \ No newline at end of file
> diff --git a/server/views/firewalls/show.xml.haml b/server/views/firewalls/show.xml.haml
> new file mode 100644
> index 0000000..9d1fc48
> --- /dev/null
> +++ b/server/views/firewalls/show.xml.haml
> @@ -0,0 +1,21 @@
> +!!! XML
> +%firewall{:href => firewall_url(@firewall.id), :id => @firewall.id}
> +  - @firewall.attributes.select{ |attr| attr != :id && attr!= :rules}.each do |attribute|
> +    - haml_tag("#{attribute}".tr('-', '_'), :<) do
> +      - if [:name, :description].include?(attribute)
> +        =cdata do
> +          - haml_concat @firewall.send(attribute)
> +      - else
> +        - haml_concat @firewall.send(attribute)
> +  %rules
> +    - @firewall.rules.each do |rule|
> +      %rule{:id => rule.id}
> +        - rule.attributes.select{|attr| attr != :sources && attr != :id}.each do |rule_attrib|
> +          - haml_tag("#{rule_attrib}".tr('-', '_'), :<) do
> +            - haml_concat rule.send(rule_attrib)
> +        %sources
> +          - rule.sources.each do |source|
> +            - if source[:type] == "group"
> +              %source{:name => source[:name], :type=> source[:type], :owner=>source[:owner]}
> +            - else
> +              %source{:prefix => source[:prefix], :address=> source[:address], :family=>source[:family], :type => source[:type]}
> \ No newline at end of file
> -- 
> 1.7.3.4
> 

------------------------------------------------------
Michal Fojtik, mfojtik@redhat.com
Deltacloud API: http://deltacloud.org


[PATCH] implements 'firewalls' - ec2 security groups [revision 1]

Posted by ma...@redhat.com.
From: marios <ma...@redhat.com>


Signed-off-by: marios <ma...@redhat.com>
---
 server/deltacloud.rb                             |    2 +
 server/lib/deltacloud/base_driver/base_driver.rb |    6 +-
 server/lib/deltacloud/base_driver/features.rb    |    8 +-
 server/lib/deltacloud/drivers/ec2/ec2_driver.rb  |  152 +++++++++++++++++++++-
 server/lib/deltacloud/models/firewall.rb         |   22 +++
 server/lib/deltacloud/models/firewall_rule.rb    |   23 ++++
 server/public/javascripts/application.js         |   37 ++++++
 server/server.rb                                 |  105 +++++++++++++++
 server/views/firewalls/index.html.haml           |   25 ++++
 server/views/firewalls/index.xml.haml            |   23 ++++
 server/views/firewalls/new.html.haml             |   11 ++
 server/views/firewalls/new_rule.html.haml        |   27 ++++
 server/views/firewalls/show.html.haml            |   41 ++++++
 server/views/firewalls/show.xml.haml             |   21 +++
 14 files changed, 495 insertions(+), 8 deletions(-)
 create mode 100644 server/lib/deltacloud/models/firewall.rb
 create mode 100644 server/lib/deltacloud/models/firewall_rule.rb
 create mode 100644 server/views/firewalls/index.html.haml
 create mode 100644 server/views/firewalls/index.xml.haml
 create mode 100644 server/views/firewalls/new.html.haml
 create mode 100644 server/views/firewalls/new_rule.html.haml
 create mode 100644 server/views/firewalls/show.html.haml
 create mode 100644 server/views/firewalls/show.xml.haml

diff --git a/server/deltacloud.rb b/server/deltacloud.rb
index 7caf34f..5628e31 100644
--- a/server/deltacloud.rb
+++ b/server/deltacloud.rb
@@ -36,6 +36,8 @@ require 'deltacloud/models/storage_volume'
 require 'deltacloud/models/bucket'
 require 'deltacloud/models/blob'
 require 'deltacloud/models/load_balancer'
+require 'deltacloud/models/firewall'
+require 'deltacloud/models/firewall_rule'
 
 require 'deltacloud/validation'
 require 'deltacloud/helpers'
diff --git a/server/lib/deltacloud/base_driver/base_driver.rb b/server/lib/deltacloud/base_driver/base_driver.rb
index d9ebd92..06363d9 100644
--- a/server/lib/deltacloud/base_driver/base_driver.rb
+++ b/server/lib/deltacloud/base_driver/base_driver.rb
@@ -183,8 +183,12 @@ module Deltacloud
       keys(credentials, opts).first if has_capability?(:keys)
     end
 
+    def firewall(credentials, opts={})
+      firewalls(credentials, opts).first if has_capability?(:firewalls)
+    end
+
     MEMBER_SHOW_METHODS =
-      [ :realm, :image, :instance, :storage_volume, :bucket, :blob, :key ]
+      [ :realm, :image, :instance, :storage_volume, :bucket, :blob, :key, :firewall ]
     
     def has_capability?(capability)
       if MEMBER_SHOW_METHODS.include?(capability.to_sym)
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"
       end
     end
 
diff --git a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
index 14c5829..5a6d34d 100644
--- a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
+++ b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
@@ -16,6 +16,7 @@
 
 require 'deltacloud/base_driver'
 require 'aws'
+require 'base64'
 
 class Instance
   attr_accessor :keyname
@@ -33,12 +34,13 @@ module Deltacloud
       class EC2Driver < Deltacloud::BaseDriver
 
         def supported_collections
-          DEFAULT_COLLECTIONS + [ :keys, :buckets, :load_balancers, :addresses ]
+
+          DEFAULT_COLLECTIONS + [ :keys, :buckets, :load_balancers, :addresses, :firewalls ]
         end
 
         feature :instances, :user_data
         feature :instances, :authentication_key
-        feature :instances, :security_group
+        feature :instances, :firewall
         feature :instances, :instance_count
         feature :images, :owner_id
         feature :buckets, :bucket_location
@@ -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]
           instance_options.merge!(
             :min_count => opts[:instance_count],
             :max_count => opts[:instance_count]
@@ -516,6 +518,7 @@ module Deltacloud
           end
         end
 
+
         def addresses(credentials, opts={})
           ec2 = new_client(credentials)
           address_id = (opts and opts[:id]) ? [opts[:id]] : []
@@ -571,6 +574,73 @@ module Deltacloud
           end
         end
 
+#--
+#FIREWALLS - ec2 security groups
+#--
+			def firewalls(credentials, opts={})
+          ec2 = new_client(credentials)
+          the_firewalls = []
+          groups = []
+          safely do
+            if opts[:id]
+              groups = ec2.describe_security_groups([opts[:id]])
+            else
+              groups = ec2.describe_security_groups()
+            end
+          end
+          groups.each do |security_group|
+            the_firewalls << convert_security_group(security_group)
+          end
+          filter_on(the_firewalls, :id, opts)
+		  end
+
+#--
+#Create firewall
+#--
+        def create_firewall(credentials, opts={})
+          ec2 = new_client(credentials)
+          safely do
+            ec2.create_security_group(opts["name"], opts["description"])
+          end
+          Firewall.new( { :id=>opts["name"], :name=>opts["name"],
+                          :description => opts["description"], :owner_id => "", :rules => [] } )
+        end
+
+#--
+#Delete firewall
+#--
+        def delete_firewall(credentials, opts={})
+          ec2 = new_client(credentials)
+          safely do
+            ec2.delete_security_group(opts["id"])
+          end
+        end
+#--
+#Create firewall rule
+#--
+        def create_firewall_rule(credentials, opts={})
+          ec2 = new_client(credentials)
+          groups = []
+          opts['groups'].each do |k,v|
+            groups << {"group_name" => k, "owner" =>v}
+          end
+          safely do
+            ec2.manage_security_group_ingress(opts['firewall'], opts['from_port'], opts['to_port'], opts['protocol'],
+              "authorize", opts['addresses'], groups)
+          end
+        end
+#--
+#Delete firewall rule
+#--
+        def delete_firewall_rule(credentials, opts={})
+          ec2 = new_client(credentials)
+          firewall = opts[:firewall]
+          protocol, from_port, to_port, addresses, groups = firewall_rule_params(opts[:rule_id])
+          safely do
+            ec2.manage_security_group_ingress(firewall, from_port, to_port, protocol, "revoke", addresses, groups)
+          end
+        end
+
         def valid_credentials?(credentials)
           retval = true
           begin
@@ -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)
+        end
+
+        #extract params from uid
+        def firewall_rule_params(id)
+          param_string = Base64.decode64(id)
+          # "#{user_id}:::#{protocol}:::#{from_port}:::#{to_port}:::#{sources_string}"
+          params = param_string.split(":::")
+          protocol = params.grep(/protocol/).first.split(" ").last
+          from_port = params.grep(/from_port/).first.split(" ").last
+          to_port = params.grep(/to_port/).first.split(" ").last
+          sources = params.grep(/sources/).first.split(" ").last
+          addresses = []
+          groups = []
+          sources.split("|").each do |source|
+            current = source.split("&")
+            type = current.grep(/type/).first.split("=").last
+            case type
+              when 'group'
+                #type=group&owner=123456789012&name=default
+                name = current.grep(/name/).first.split("=").last
+                owner = current.grep(/owner/).first.split("=").last
+                groups << {'group_name' => name, 'owner' => owner}
+              when 'address'
+                #type=address&family=ipv4&address=10.1.1.1&prefix=24
+                address = current.grep(/address/).last.split("=").last
+                address<<"/#{current.grep(/prefix/).first.split("=").last}"
+                addresses << address
+            end
+          end
+          return protocol, from_port, to_port, addresses, groups
+        end
+
+        #Convert ec2 security group to server/lib/deltacloud/models/firewall
+        def convert_security_group(security_group)
+          rules = []
+          security_group[:aws_perms].each do |perm|
+            sources = []
+            perm[:groups].each do |group|
+              sources << {:type => "group", :name => group[:group_name], :owner => group[:owner]}
+            end
+            perm[:ip_ranges].each do |ip|
+              sources << {:type => "address", :family=>"ipv4",
+                          :address=>ip[:cidr_ip].split("/").first,
+                          :prefix=>ip[:cidr_ip].split("/").last}
+            end
+            rule_id = firewall_rule_id(security_group[:aws_owner], perm[:protocol],
+                                       perm[:from_port] , perm[:to_port], sources)
+            rules << Firewall_Rule.new({:id => rule_id,
+                                        :allow_protocol => perm[:protocol],
+                                        :port_from => perm[:from_port],
+                                        :port_to => perm[:to_port],
+                                        :direction => 'ingress',
+                                        :sources => sources})
+          end
+          Firewall.new(  {  :id => security_group[:aws_group_name],
+                            :name => security_group[:aws_group_name],
+                            :description => security_group[:aws_description],
+                            :owner_id => security_group[:aws_owner],
+                            :rules => rules
+                      }  )
+        end
+
         def convert_state(ec2_state)
           case ec2_state
             when "terminated"
diff --git a/server/lib/deltacloud/models/firewall.rb b/server/lib/deltacloud/models/firewall.rb
new file mode 100644
index 0000000..dc0ae3d
--- /dev/null
+++ b/server/lib/deltacloud/models/firewall.rb
@@ -0,0 +1,22 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.  The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License.  You may obtain a copy of the License at
+#
+#       http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+class Firewall < BaseModel
+  attr_accessor :name
+  attr_accessor :description
+  attr_accessor :owner_id
+  attr_accessor :rules
+end
\ No newline at end of file
diff --git a/server/lib/deltacloud/models/firewall_rule.rb b/server/lib/deltacloud/models/firewall_rule.rb
new file mode 100644
index 0000000..df353a5
--- /dev/null
+++ b/server/lib/deltacloud/models/firewall_rule.rb
@@ -0,0 +1,23 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.  The
+# ASF licenses this file to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance with the
+# License.  You may obtain a copy of the License at
+#
+#       http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+class Firewall_Rule < BaseModel
+  attr_accessor :allow_protocol # tcp/udp/icmp
+  attr_accessor :port_from
+  attr_accessor :port_to
+  attr_accessor :sources
+  attr_accessor :direction #ingress egress
+end
diff --git a/server/public/javascripts/application.js b/server/public/javascripts/application.js
index 95c9bc2..29102c0 100644
--- a/server/public/javascripts/application.js
+++ b/server/public/javascripts/application.js
@@ -51,3 +51,40 @@ 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")
+	if(type == "address")
+	{
+		name = "ip_address" + eval(++addresses)
+		create_rule_source_field(name, "Address " + eval(addresses) + " [use CIDR notation 0.0.0.0/0]", form, button) 
+	}
+  else if(type == "group")
+	{
+		name = "group" + eval(++groups)
+		create_rule_source_field(name, "Name of group " + eval(groups), form, button)
+		name = "group" + eval(groups) + "owner"
+		create_rule_source_field(name, "Group " + eval(groups) + " owner (required)", form, button)
+	}
+}
+
+function create_rule_source_field(name, label, form, button)
+{
+		element = document.createElement("INPUT")
+		element.type = "input"
+		element.size = 35
+		element.name = name
+		text = document.createTextNode(label)
+		form.insertBefore(element, button)
+		form.insertBefore(text, element)
+		form.insertBefore(document.createElement('BR'), element)
+		form.insertBefore(document.createElement('BR'), button)
+		form.insertBefore(document.createElement('BR'), button)
+		form.insertBefore(document.createElement('BR'), button)
+}
+
+
diff --git a/server/server.rb b/server/server.rb
index 4da8357..a318df6 100644
--- a/server/server.rb
+++ b/server/server.rb
@@ -843,6 +843,7 @@ collection :buckets do
 
 end
 
+
 get '/api/addresses/:id/associate' do
   @instances = driver.instances(credentials)
   @address = Address::new(:id => params[:id])
@@ -929,3 +930,107 @@ collection :addresses do
   end
 
 end
+
+#html for creating a new firewall
+get '/api/firewalls/new' do
+  respond_to do |format|
+    format.html { haml :"firewalls/new" }
+  end
+end
+
+#html for creating a new firewall rule
+get '/api/firewalls/:firewall/new_rule' do
+  @firewall_name = params[:firewall]
+  respond_to do |format|
+    format.html {haml :"firewalls/new_rule" }
+  end
+end
+
+#create new firewall rule
+post '/api/firewalls/:firewall/rules' do
+  opts = {}
+  opts['firewall'] = params[:firewall] # must specify group_name or group_id
+  opts['protocol'] = params[:protocol]
+  opts['from_port'] = params[:from_port]
+  opts['to_port'] = params[:to_port]
+  addresses =  params.inject([]){|result,current| result << current.last unless current.grep(/^ip[-_]address/i).empty?; result}
+  groups = {}
+  max_groups  = params.select{|k,v| k=~/^group/}.size/2
+  for i in (1..max_groups) do
+    groups.merge!({params["group#{i}"]=>params["group#{i}owner"]})
+  end
+  opts['addresses'] = addresses
+  opts['groups'] = groups
+  driver.create_firewall_rule(credentials, opts)
+  @firewall = driver.firewall(credentials, {:id => params[:firewall]})
+  respond_to do |format|
+    format.html {haml :"firewalls/show"}
+    format.xml do
+      response.status = 201 #created
+      haml :"firewalls/show"
+    end
+  end
+end
+
+#delete a firewall rule
+delete '/api/firewalls/:firewall/:rule' do
+  opts = {}
+  opts[:firewall] = params[:firewall]
+  opts[:rule_id] = params[:rule]
+  driver.delete_firewall_rule(credentials, opts)
+  respond_to do |format|
+    format.html {redirect firewall_url(params[:firewall])}
+    format.xml {204}
+    format.json {204}
+  end
+end
+
+
+collection :firewalls do
+  description "Allow user to define firewall rules for an instance (ec2 security groups) eg expose ssh access [port 22, tcp]."
+
+  operation :index do
+    description 'List all firewalls'
+    with_capability :firewalls
+    control { filter_all(:firewalls) }
+  end
+
+  operation :show do
+    description 'Show details for a specific firewall - list all rules'
+    with_capability :firewall
+    param :id,            :string,    :required
+    control { show(:firewall) }
+  end
+
+  operation :create do
+    description 'Create a new firewall'
+    with_capability :create_firewall
+    param :name,          :string,    :required
+    param :description,   :string,    :required
+    control do
+      @firewall = driver.create_firewall(credentials, params )
+      respond_to do |format|
+        format.xml do
+          response.status = 201  # Created
+          haml :"firewalls/show"
+        end
+        format.html {haml :"firewalls/show"}
+      end
+    end
+  end
+
+  operation :destroy do
+    description 'Delete a specified firewall - error if firewall has rules'
+    with_capability :delete_firewall
+    param :id,            :string,    :required
+    control do
+      driver.delete_firewall(credentials, params)
+      respond_to do |format|
+        format.xml { 204 }
+        format.json {  204 }
+        format.html {  redirect(firewalls_url) }
+      end
+    end
+  end
+
+end #firewalls
diff --git a/server/views/firewalls/index.html.haml b/server/views/firewalls/index.html.haml
new file mode 100644
index 0000000..3312a32
--- /dev/null
+++ b/server/views/firewalls/index.html.haml
@@ -0,0 +1,25 @@
+%h1 Firewalls
+%br
+%p
+  =link_to 'Create new firewall', "/api/firewalls/new"
+%table.display
+  %thead
+    %tr
+      %th Id
+      %th Name
+      %th Description
+      %th Owner ID
+      %th Rules
+  %tbody
+    - @firewalls.each do |firewall|
+      %tr
+        %td
+          = link_to firewall.id, firewall_url(firewall.id)
+        %td
+          = firewall.name
+        %td
+          = firewall.description
+        %td
+          = firewall.owner_id
+        %td
+          = link_to 'view rules', firewall_url(firewall.id)
diff --git a/server/views/firewalls/index.xml.haml b/server/views/firewalls/index.xml.haml
new file mode 100644
index 0000000..f027785
--- /dev/null
+++ b/server/views/firewalls/index.xml.haml
@@ -0,0 +1,23 @@
+!!! XML
+%firewalls
+  - @firewalls.each do |firewall|
+    %firewall{:href => firewall_url(firewall.id), :id => firewall.id}
+      - firewall.attributes.select{ |attr| attr != :id && attr!= :rules}.each do |attribute|
+        - haml_tag("#{attribute}".tr('-', '_'), :<) do
+          - if [:name, :description].include?(attribute)
+            =cdata do
+              - haml_concat firewall.send(attribute)
+          - else
+            - haml_concat firewall.send(attribute)
+      %rules
+        - firewall.rules.each do |rule|
+          %rule{:id => rule.id}
+            - rule.attributes.select{|attr| attr != :sources && attr != :id}.each do |rule_attrib|
+              - haml_tag("#{rule_attrib}".tr('-', '_'), :<) do
+                - haml_concat rule.send(rule_attrib)
+            %sources
+              - rule.sources.each do |source|
+                - if source[:type] == "group"
+                  %source{:name => source[:name], :type=> source[:type], :owner=> source[:owner]}
+                - else
+                  %source{:prefix => source[:prefix], :address=> source[:address], :family=>source[:family], :type => source[:type]}
\ No newline at end of file
diff --git a/server/views/firewalls/new.html.haml b/server/views/firewalls/new.html.haml
new file mode 100644
index 0000000..4a230a6
--- /dev/null
+++ b/server/views/firewalls/new.html.haml
@@ -0,0 +1,11 @@
+%h1 New Firewall
+
+%form{:action => firewalls_url, :method => :post}
+  %label
+    Firewall Name
+    %input{:name => 'name', :size => 25}/
+    %br
+  %label
+    Firewall Description
+    %input{:name => 'description', :size => 100}/
+  %input{:type => :submit, :name => "commit", :value=>"create"}
\ No newline at end of file
diff --git a/server/views/firewalls/new_rule.html.haml b/server/views/firewalls/new_rule.html.haml
new file mode 100644
index 0000000..a3bee24
--- /dev/null
+++ b/server/views/firewalls/new_rule.html.haml
@@ -0,0 +1,27 @@
+%h1 New Firewall Rule
+
+%form{ :action => "#{firewall_url(@firewall_name)}/rules", :id => "new_rule_form", :method => :post, :enctype => 'multipart/form-data'}
+  %label
+    Protocol:
+    %br
+    %input{ :name => 'protocol', :size => 10}/
+    %br 
+    %br
+  %label
+    From port:
+    %br
+    %input{ :name => 'from_port', :size => 10}/
+    %br
+    %br
+    To port:
+    %br
+    %input{ :name => 'to_port', :size => 10}/
+    %br
+    %br
+  %a{ :href => "javascript:;", :onclick => "make_fields('address');"} Add source IP address
+  %br
+  %a{ :href => "javascript:;", :onclick => "make_fields('group');"} Add source group
+  %br
+  %br
+  %input{ :type => :submit, :id => "submit_button", :name => "commit", :value => "create"}/
+
diff --git a/server/views/firewalls/show.html.haml b/server/views/firewalls/show.html.haml
new file mode 100644
index 0000000..e6a186f
--- /dev/null
+++ b/server/views/firewalls/show.html.haml
@@ -0,0 +1,41 @@
+%h1 Firewall
+%h2
+  = @firewall.id
+%dl
+  %di
+    %dt Name
+    %dd
+      = @firewall.name
+    %dt Owner
+    %dd
+      = @firewall.owner_id
+    %dt Description
+    %dd
+      = @firewall.description
+
+%h2
+  Rules
+  %br
+  %p
+    =link_to 'Create a new rule', "/api/firewalls/#{@firewall.name}/new_rule"
+%dl
+  - @firewall.rules.each do |rule|
+    %di
+      Rule
+      - rule.attributes.select{|attr| attr != :sources}.each do |attrib|
+        %dt #{attrib}
+        %dd
+          = rule.send(attrib)
+      %dt sources
+      %dd
+        - rule.sources.each do |source|
+          - if source[:type] == "group"
+            type: #{source[:type]}, name: #{source[:name]}, owner: #{source[:owner]}
+            %br
+          - else
+            type: #{source[:type]}, family: #{source[:family]}, address: #{source[:address]}, prefix: #{source[:prefix]}
+            %br
+      %dd
+        = link_to_action 'Delete Rule', destroy_firewall_url(@firewall.name) + "/#{rule.id}", :delete
+  %dd
+    = link_to_action 'Delete Firewall', destroy_firewall_url(@firewall.name), :delete
\ No newline at end of file
diff --git a/server/views/firewalls/show.xml.haml b/server/views/firewalls/show.xml.haml
new file mode 100644
index 0000000..9d1fc48
--- /dev/null
+++ b/server/views/firewalls/show.xml.haml
@@ -0,0 +1,21 @@
+!!! XML
+%firewall{:href => firewall_url(@firewall.id), :id => @firewall.id}
+  - @firewall.attributes.select{ |attr| attr != :id && attr!= :rules}.each do |attribute|
+    - haml_tag("#{attribute}".tr('-', '_'), :<) do
+      - if [:name, :description].include?(attribute)
+        =cdata do
+          - haml_concat @firewall.send(attribute)
+      - else
+        - haml_concat @firewall.send(attribute)
+  %rules
+    - @firewall.rules.each do |rule|
+      %rule{:id => rule.id}
+        - rule.attributes.select{|attr| attr != :sources && attr != :id}.each do |rule_attrib|
+          - haml_tag("#{rule_attrib}".tr('-', '_'), :<) do
+            - haml_concat rule.send(rule_attrib)
+        %sources
+          - rule.sources.each do |source|
+            - if source[:type] == "group"
+              %source{:name => source[:name], :type=> source[:type], :owner=>source[:owner]}
+            - else
+              %source{:prefix => source[:prefix], :address=> source[:address], :family=>source[:family], :type => source[:type]}
\ No newline at end of file
-- 
1.7.3.4