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:10 UTC

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

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


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