Improved support for SecurityGroup IDs

This patch makes it possible to specify GroupID in the options hash to the
aws computre requests operating on security groups. This is needed since
when working with VPC you must use GroupID instead of name.

lib/fog/aws.rb

@@ -221,6 +221,10 @@ module Fog
         "vol-#{Fog::Mock.random_hex(8)}"
       end
 
+      def self.security_group_id
+        "sg-#{Fog::Mock.random_hex(8)}"
+      end
+
       def self.key_id(length=21)
         #Probably close enough
         Fog::Mock.random_selection('ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789',length)
@@ -230,5 +234,24 @@ module Fog
         "#{db_name}.#{Fog::Mock.random_letters(rand(12) + 4)}.#{region}.rds.amazonaws.com"
       end
     end
+
+    def self.parse_security_group_options(group_name, options)
+      if group_name.is_a?(Hash)
+        options = group_name
+      elsif group_name
+        if options.key?('GroupName')
+          raise Fog::Compute::AWS::Error, 'Arguments specified both group_name and GroupName in options'
+        end
+        options = options.clone
+        options['GroupName'] = group_name
+      end
+      if !options.key?('GroupName') && !options.key?('GroupId')
+        raise Fog::Compute::AWS::Error, 'Neither GroupName nor GroupId specified'
+      end
+      if options.key?('GroupName') && options.key?('GroupId')
+        raise Fog::Compute::AWS::Error, 'Both GroupName and GroupId specified'
+      end
+      options
+    end
   end
 end

lib/fog/aws/compute.rb

@@ -124,6 +124,7 @@ module Fog
                   'default' => {
                     'groupDescription'    => 'default group',
                     'groupName'           => 'default',
+                    'groupId'             => 'sg-11223344',
                     'ipPermissionsEgress' => [],
                     'ipPermissions'       => [
                       {

lib/fog/aws/requests/compute/authorize_security_group_ingress.rb

@@ -8,8 +8,10 @@ module Fog
         # Add permissions to a security group
         #
         # ==== Parameters
-        # * group_name<~String> - Name of group
+        # * group_name<~String> - Name of group, optional (can also be specifed as GroupName in options)
         # * options<~Hash>:
+        #   * 'GroupName'<~String> - Name of security group to modify
+        #   * 'GroupId'<~String> - Id of security group to modify
         #   * 'SourceSecurityGroupName'<~String> - Name of security group to authorize
         #   * 'SourceSecurityGroupOwnerId'<~String> - Name of owner to authorize
         #   or
@@ -39,11 +41,7 @@ module Fog
         #
         # {Amazon API Reference}[http://docs.amazonwebservices.com/AWSEC2/latest/APIReference/ApiReference-query-AuthorizeSecurityGroupIngress.html]
         def authorize_security_group_ingress(group_name, options = {})
-          if group_name.is_a?(Hash)
-            Fog::Logger.deprecation("Fog::AWS::Compute#authorize_security_group_ingress now requires the 'group_name' parameter. Only specifying an options hash is now deprecated [light_black](#{caller.first})[/]")
-            options = group_name
-            group_name = options.delete('GroupName')
-          end
+          options = Fog::AWS.parse_security_group_options(group_name, options)
 
           if ip_permissions = options.delete('IpPermissions')
             options.merge!(indexed_ip_permissions_params(ip_permissions))
@@ -51,7 +49,6 @@ module Fog
 
           request({
             'Action'    => 'AuthorizeSecurityGroupIngress',
-            'GroupName' => group_name,
             :idempotent => true,
             :parser     => Fog::Parsers::Compute::AWS::Basic.new
           }.merge!(options))
@@ -85,10 +82,11 @@ module Fog
       class Mock
 
         def authorize_security_group_ingress(group_name, options = {})
-          if group_name.is_a?(Hash)
-            Fog::Logger.deprecation("Fog::AWS::Compute#authorize_security_group_ingress now requires the 'group_name' parameter. Only specifying an options hash is now deprecated [light_black](#{caller.first})[/]")
-            options = group_name
-            group_name = options.delete('GroupName')
+          options = Fog::AWS.parse_security_group_options(group_name, options)
+          if options.key?('GroupName')
+            group_name = options['GroupName']
+          else
+            group_name = self.data[:security_groups].reject { |k,v| v['groupId'] != options['GroupId'] } .keys.first
           end
 
           verify_permission_options(options)
@@ -134,7 +132,7 @@ module Fog
         private
 
         def verify_permission_options(options)
-          if options.empty?
+          if options.size <= 1
             raise Fog::Compute::AWS::Error.new("InvalidRequest => The request received was invalid.")
           end
           if options['IpProtocol'] && !['tcp', 'udp', 'icmp'].include?(options['IpProtocol'])

lib/fog/aws/requests/compute/create_security_group.rb

@@ -10,6 +10,7 @@ module Fog
         # ==== Parameters
         # * group_name<~String> - Name of the security group.
         # * group_description<~String> - Description of group.
+        # * vpc_id<~String> - ID of the VPC
         #
         # ==== Returns
         # * response<~Excon::Response>:
@@ -38,6 +39,7 @@ module Fog
             data = {
               'groupDescription'    => description,
               'groupName'           => name,
+              'groupId'             => Fog::AWS::Mock.security_group_id,
               'ipPermissionsEgress' => [],
               'ipPermissions'       => [],
               'ownerId'             => self.data[:owner_id],

lib/fog/aws/requests/compute/delete_security_group.rb

@@ -8,7 +8,8 @@ module Fog
         # Delete a security group that you own
         #
         # ==== Parameters
-        # * group_name<~String> - Name of the security group.
+        # * group_name<~String> - Name of the security group, must be nil if id is specified
+        # * group_id<~String> - Id of the security group, must be nil if name is specified
         #
         # ==== Returns
         # * response<~Excon::Response>:
@@ -17,10 +18,20 @@ module Fog
         #     * 'return'<~Boolean> - success?
         #
         # {Amazon API Reference}[http://docs.amazonwebservices.com/AWSEC2/latest/APIReference/ApiReference-query-DeleteSecurityGroup.html]
-        def delete_security_group(name)
+        def delete_security_group(name, id = nil)
+          if name && id
+            raise Fog::Compute::AWS::Error.new("May not specify both group_name and group_id")
+          end
+          if name
+            type_id    = 'GroupName'
+            identifier = name
+          else
+            type_id    = 'GroupId'
+            identifier = id
+          end
           request(
             'Action'    => 'DeleteSecurityGroup',
-            'GroupName' => name,
+            type_id     => identifier,
             :idempotent => true,
             :parser     => Fog::Parsers::Compute::AWS::Basic.new
           )
@@ -29,11 +40,18 @@ module Fog
       end
 
       class Mock
-        def delete_security_group(name)
+        def delete_security_group(name, id = nil)
           if name == 'default'
             raise Fog::Compute::AWS::Error.new("InvalidGroup.Reserved => The security group 'default' is reserved")
           end
 
+          if name && id
+            raise Fog::Compute::AWS::Error.new("May not specify both group_name and group_id")
+          end
+          if id
+            name = self.data[:security_groups].reject { |k,v| v['groupId'] != id } .keys.first
+          end
+
           response = Excon::Response.new
           if self.data[:security_groups][name]
 

lib/fog/aws/requests/compute/describe_security_groups.rb

@@ -73,6 +73,8 @@ module Fog
             if permission_key = filter_key.split('ip-permission.')[1]
               if permission_key == 'group-name'
                 security_group_info = security_group_info.reject{|security_group| !security_group['ipPermissions']['groups'].detect {|group| [*filter_value].include?(group['groupName'])}}
+              elsif permission_key == 'group-id'
+                security_group_info = security_group_info.reject{|security_group| !security_group['ipPermissions']['groups'].detect {|group| [*filter_value].include?(group['groupId'])}}
               elsif permission_key == 'user-id'
                 security_group_info = security_group_info.reject{|security_group| !security_group['ipPermissions']['groups'].detect {|group| [*filter_value].include?(group['userId'])}}
               else

lib/fog/aws/requests/compute/request_spot_instances.rb

@@ -25,7 +25,8 @@ module Fog
         #   * 'LaunchSpecification.KeyName'<~String> - Name of a keypair to add to booting instances
         #   * 'LaunchSpecification.Monitoring.Enabled'<~Boolean> - Enables monitoring, defaults to disabled
         #   * 'LaunchSpecification.Placement.AvailabilityZone'<~String> - Placement constraint for instances
-        #   * 'LaunchSpecification.SecurityGroup'<~Array> or <~String> - Name of security group(s) for instances
+        #   * 'LaunchSpecification.SecurityGroup'<~Array> or <~String> - Name of security group(s) for instances, not supported in VPC
+        #   * 'LaunchSpecification.SecurityGroupId'<~Array> or <~String> - Id of security group(s) for instances, use this or LaunchSpecification.SecurityGroup
         #   * 'LaunchSpecification.UserData'<~String> -  Additional data to provide to booting instances
         #   * 'Type'<~String> - spot instance request type in ['one-time', 'persistent']
         #   * 'ValidFrom'<~Time> - start date for request
@@ -64,6 +65,9 @@ module Fog
           if security_groups = options.delete('LaunchSpecification.SecurityGroup')
             options.merge!(Fog::AWS.indexed_param('LaunchSpecification.SecurityGroup', [*security_groups]))
           end
+          if security_group_ids = options.delete('LaunchSpecification.SecurityGroupId')
+            options.merge!(Fog::AWS.indexed_param('LaunchSpecification.SecurityGroupId', [*security_group_ids]))
+          end
           if options['LaunchSpecification.UserData']
             options['LaunchSpecification.UserData'] = Base64.encode64(options['LaunchSpecification.UserData'])
           end

lib/fog/aws/requests/compute/revoke_security_group_ingress.rb

@@ -8,8 +8,10 @@ module Fog
         # Remove permissions from a security group
         #
         # ==== Parameters
-        # * group_name<~String> - Name of group
+        # * group_name<~String> - Name of group, optional (can also be specifed as GroupName in options)
         # * options<~Hash>:
+        #   * 'GroupName'<~String> - Name of security group to modify
+        #   * 'GroupId'<~String> - Id of security group to modify
         #   * 'SourceSecurityGroupName'<~String> - Name of security group to authorize
         #   * 'SourceSecurityGroupOwnerId'<~String> - Name of owner to authorize
         #   or
@@ -39,11 +41,7 @@ module Fog
         #
         # {Amazon API Reference}[http://docs.amazonwebservices.com/AWSEC2/latest/APIReference/ApiReference-query-RevokeSecurityGroupIngress.html]
         def revoke_security_group_ingress(group_name, options = {})
-          if group_name.is_a?(Hash)
-            Fog::Logger.deprecation("Fog::AWS::Compute#revoke_security_group_ingress now requires the 'group_name' parameter. Only specifying an options hash is now deprecated [light_black](#{caller.first})[/]")
-            options = group_name
-            group_name = options.delete('GroupName')
-          end
+          options = Fog::AWS.parse_security_group_options(group_name, options)
 
           if ip_permissions = options.delete('IpPermissions')
             options.merge!(indexed_ip_permissions_params(ip_permissions))
@@ -51,7 +49,6 @@ module Fog
 
           request({
             'Action'    => 'RevokeSecurityGroupIngress',
-            'GroupName' => group_name,
             :idempotent => true,
             :parser     => Fog::Parsers::Compute::AWS::Basic.new
           }.merge!(options))
@@ -62,10 +59,11 @@ module Fog
       class Mock
 
         def revoke_security_group_ingress(group_name, options = {})
-          if group_name.is_a?(Hash)
-            Fog::Logger.deprecation("Fog::AWS::Compute#revoke_security_group_ingress now requires the 'group_name' parameter. Only specifying an options hash is now deprecated [light_black](#{caller.first})[/]")
-            options = group_name
-            group_name = options.delete('GroupName')
+          options = Fog::AWS.parse_security_group_options(group_name, options)
+          if options.key?('GroupName')
+            group_name = options['GroupName']
+          else
+            group_name = self.data[:security_groups].reject { |k,v| v['groupId'] != options['GroupId'] } .keys.first
           end
 
           verify_permission_options(options)

lib/fog/aws/requests/compute/run_instances.rb

@@ -30,7 +30,8 @@ module Fog
         #     * 'Ebs.DeleteOnTermination'<~String> - specifies whether or not to delete the volume on instance termination
         #   * 'ClientToken'<~String> - unique case-sensitive token for ensuring idempotency
         #   * 'DisableApiTermination'<~Boolean> - specifies whether or not to allow termination of the instance from the api
-        #   * 'SecurityGroup'<~Array> or <~String> - Name of security group(s) for instances (you must omit this parameter if using Virtual Private Clouds)
+        #   * 'SecurityGroup'<~Array> or <~String> - Name of security group(s) for instances (not supported for VPC)
+        #   * 'SecurityGroupId'<~Array> or <~String> - id's of security group(s) for instances, use this or SecurityGroup
         #   * 'InstanceInitiatedShutdownBehaviour'<~String> - specifies whether volumes are stopped or terminated when instance is shutdown, in [stop, terminate]
         #   * 'InstanceType'<~String> - Type of instance to boot. Valid options
         #     in ['t1.micro', 'm1.small', 'm1.large', 'm1.xlarge', 'c1.medium', 'c1.xlarge', 'm2.xlarge', m2.2xlarge', 'm2.4xlarge', 'cc1.4xlarge', 'cg1.4xlarge']
@@ -97,6 +98,9 @@ module Fog
           if security_groups = options.delete('SecurityGroup')
             options.merge!(Fog::AWS.indexed_param('SecurityGroup', [*security_groups]))
           end
+          if security_group_ids = options.delete('SecurityGroupId')
+            options.merge!(Fog::AWS.indexed_param('SecurityGroupId', [*security_group_ids]))
+          end
           if options['UserData']
             options['UserData'] = Base64.encode64(options['UserData'])
           end

tests/aws/requests/compute/security_group_tests.rb

@@ -31,6 +31,7 @@ Shindo.tests('Fog::Compute[:aws] | security group requests', ['aws']) do
       Fog::Compute[:aws].create_security_group('fog_security_group_two', 'tests group').body
     end
 
+    group_id = Fog::Compute[:aws].describe_security_groups('group-name' => 'fog_security_group').body['securityGroupInfo'].first['groupId']
     to_be_revoked = []
     expected_permissions = []
 
@@ -63,6 +64,10 @@ Shindo.tests('Fog::Compute[:aws] | security group requests', ['aws']) do
       array_differences(expected_permissions, Fog::Compute[:aws].describe_security_groups('group-name' => 'fog_security_group').body['securityGroupInfo'].first['ipPermissions'])
     end
 
+    tests("#describe_security_groups('group-id' => '#{group_id}')").returns([]) do
+      array_differences(expected_permissions, Fog::Compute[:aws].describe_security_groups('group-id' => group_id).body['securityGroupInfo'].first['ipPermissions'])
+    end
+
     permission = { 'SourceSecurityGroupName' => 'fog_security_group_two', 'SourceSecurityGroupOwnerId' => @owner_id }
     tests("#authorize_security_group_ingress('fog_security_group', #{permission.inspect})").formats(AWS::Compute::Formats::BASIC) do
       Fog::Compute[:aws].authorize_security_group_ingress('fog_security_group', permission).body
@@ -256,6 +261,52 @@ Shindo.tests('Fog::Compute[:aws] | security group requests', ['aws']) do
       Fog::Compute[:aws].delete_security_group('fog_security_group_two').body
     end
 
+    # Create security group in VPC
+    tests("#create_security_group('vpc_security_group', 'tests group')").formats(AWS::Compute::Formats::BASIC) do
+      Fog::Compute[:aws].create_security_group('vpc_security_group', 'tests group', 'vpc-11223344').body
+    end
+
+    group_id = Fog::Compute[:aws].describe_security_groups('group-name' => 'vpc_security_group').body['securityGroupInfo'].first['groupId']
+    
+    # Access group with name in options array
+    permission = { 'IpProtocol' => 'tcp', 'FromPort' => '22', 'ToPort' => '22', 'CidrIp' => '10.0.0.0/8' }
+    expected_permissions = [
+      {"groups"=>[],
+        "ipRanges"=>[{"cidrIp"=>"10.0.0.0/8"}],
+        "ipProtocol"=>"tcp",
+        "fromPort"=>22,
+        "toPort"=>22}
+    ]
+
+    options = permission.clone
+    options['GroupName'] = 'vpc_security_group'
+    tests("#authorize_security_group_ingress(#{options.inspect})").formats(AWS::Compute::Formats::BASIC) do
+      Fog::Compute[:aws].authorize_security_group_ingress(options).body
+    end
+
+    tests("#describe_security_groups('group-name' => 'vpc_security_group')").returns([]) do
+      array_differences(expected_permissions, Fog::Compute[:aws].describe_security_groups('group-name' => 'vpc_security_group').body['securityGroupInfo'].first['ipPermissions'])
+    end
+
+    tests("#revoke_security_group_ingress(#{options.inspect})").formats(AWS::Compute::Formats::BASIC) do
+      Fog::Compute[:aws].revoke_security_group_ingress(options).body
+    end
+
+    # Access group with id in options array
+    options = permission.clone
+    options['GroupId'] = group_id
+    tests("#authorize_security_group_ingress(#{options.inspect})").formats(AWS::Compute::Formats::BASIC) do
+      Fog::Compute[:aws].authorize_security_group_ingress(options).body
+    end
+
+    tests("#describe_security_groups('group-name' => 'vpc_security_group')").returns([]) do
+      array_differences(expected_permissions, Fog::Compute[:aws].describe_security_groups('group-name' => 'vpc_security_group').body['securityGroupInfo'].first['ipPermissions'])
+    end
+
+    tests("#revoke_security_group_ingress(#{options.inspect})").formats(AWS::Compute::Formats::BASIC) do
+      Fog::Compute[:aws].revoke_security_group_ingress(options).body
+    end
+
   end
   tests('failure') do
 
@@ -358,6 +409,23 @@ Shindo.tests('Fog::Compute[:aws] | security group requests', ['aws']) do
     tests("#delete_security_group('default')").raises(Fog::Compute::AWS::Error) do
       Fog::Compute[:aws].delete_security_group('default')
     end
+
+    broken_params = [
+                     [ 'fog_security_group', { 'GroupName' => 'fog_security_group'}],
+                     [ 'fog_security_group', { 'GroupId' => 'sg-11223344'}],
+                     [ { 'GroupName' => 'fog_security_group', 'GroupId' => 'sg-11223344'}, nil]
+                    ]
+
+    broken_params.each do |list_elem|
+      tests("#authorize_security_group_ingress(#{list_elem[0].inspect}, #{list_elem[1].inspect})").raises(Fog::Compute::AWS::Error) do
+        Fog::Compute[:aws].authorize_security_group_ingress(list_elem[0], list_elem[1])
+      end
+
+      tests("#revoke_security_group_ingress(#{list_elem[0].inspect}, #{list_elem[1].inspect})").raises(Fog::Compute::AWS::Error) do
+        Fog::Compute[:aws].revoke_security_group_ingress(list_elem[0], list_elem[1])
+      end
+    end
+
   end
 
 end