id security groups primarily by name (falling back to vpc_name / group_name) and much more

lib/puppet/provider/ec2_securitygroup/v2.rb

@@ -18,7 +18,7 @@ def self.instances
     end.flatten
   end
 
-  read_only(:region, :description)
+  read_only(:name, :region, :description)
 
   def self.prefetch(resources)
     instances.each do |prov|
@@ -28,38 +28,50 @@ def self.prefetch(resources)
     end
   end
 
-  def self.format_ingress_rules(client, group)
+  def self.id_to_name(region, group_id)
+    ec2 = ec2_client(region)
+
+    group_response = ec2.describe_security_groups(
+      filters: [{name: 'group-id', values: [group_id]}])
+
+    group_response.data.security_groups.first.group_name
+  end
+
+  def self.format_ingress_rules(region, group)
     group[:ip_permissions].collect do |rule|
-      if rule.user_id_group_pairs.empty?
-        {
-          'protocol' => rule.ip_protocol,
-          'port' => rule.to_port.to_i,
-          'cidr' => rule.ip_ranges.first.cidr_ip
-        }
-      else
-        rule.user_id_group_pairs.collect do |security_group|
-          name = security_group.group_name
-          if name.nil?
-            group_response = client.describe_security_groups(
-              group_ids: [security_group.group_id]
-            )
-            name = group_response.data.security_groups.first.group_name
-          end
-          {
-            'security_group' => name
-          }
+      {}.tap do |h|
+        h['protocol'] = rule.ip_protocol
+
+        h['cidr'] = rule.ip_ranges.map(&:cidr_ip)
+        case h['cidr'].size
+        when 0 then h.delete('cidr')
+        when 1 then h['cidr'] = h['cidr'].first
+        end
+
+        h['port'] = [rule.from_port, rule.to_port].compact.map(&:to_s).uniq
+        case h['port'].size
+        when 0 then h.delete('port')
+        when 1 then h['port'] = h['port'].first
+        end
+
+        h['security_group'] = rule.user_id_group_pairs.
+          map {|ug| ug[:group_name] || id_to_name(region, ug[:group_id]) }.compact.
+          reject {|g| group.group_name == g}
+        case h['security_group'].size
+        when 0 then h.delete('security_group')
+        when 1 then h['security_group'] = h['security_group'].first
         end
       end
-    end.flatten.uniq
+    end.flatten.uniq.compact
   end
 
   def self.security_group_to_hash(region, group)
     ec2 = ec2_client(region)
     vpc_name = nil
     if group.vpc_id
-      vpc_response = ec2.describe_vpcs(
-        vpc_ids: [group.vpc_id]
-      )
+      vpc_response = ec2.describe_vpcs(filters: [
+        {name: 'vpc-id', values: [group.vpc_id]}
+      ])
       vpc_name = if vpc_response.data.vpcs.empty?
         nil
       elsif vpc_response.data.vpcs.first.to_hash.keys.include?(:group_name)
@@ -69,17 +81,20 @@ def self.security_group_to_hash(region, group)
         vpc_name_tag ? vpc_name_tag.value : nil
       end
     end
-    {
-      id: group.group_id,
-      name: group[:group_name],
-      id: group[:group_id],
+
+    group_tags     = tags_from_data(group.tags)
+    group_name_tag = group_tags.delete('Name')
+
+    { id: group.group_id,
+      name: group_name_tag ||
+        (vpc_name ? "#{vpc_name}/#{group[:group_name]}" : group.group_id),
+      group_name: group[:group_name],
       description: group[:description],
       ensure: :present,
-      ingress: format_ingress_rules(ec2, group),
+      ingress: format_ingress_rules(region, group),
       vpc: vpc_name,
       region: region,
-      tags: tags_for(group),
-    }
+      tags: group_tags }
   end
 
   def exists?
@@ -92,7 +107,7 @@ def create
     Puppet.info("Creating security group #{name} in region #{resource[:region]}")
     ec2 = ec2_client(resource[:region])
     config = {
-      group_name: name,
+      group_name: resource[:group_name],
       description: resource[:description]
     }
 
@@ -108,92 +123,65 @@ def create
     end
 
     response = ec2.create_security_group(config)
-
-    ec2.create_tags(
-      resources: [response.group_id],
-      tags: tags_for_resource
-    ) if resource[:tags]
+    ec2.create_tags(resources: [response.group_id], tags: tags_for_resource)
 
     @property_hash[:id] = response.group_id
-    rules = resource[:ingress]
-    authorize_ingress(rules)
+    authorize_ingress(resource[:ingress])
     @property_hash[:ensure] = :present
   end
 
+  def id_or_name_to_id(group_id_or_name)
+    return group_id_or_name if group_id_or_name =~ /^sg-/
+    return @property_hash[:id] if group_id_or_name == @property_hash[:group_name]
+
+    ec2 = ec2_client(@property_hash[:region])
+
+    group_response = ec2.describe_security_groups(
+      filters: [{name: 'group-name', values: [group_id_or_name]}])
+
+    if group_response.data.security_groups.count == 0
+      fail("No groups found with name: '#{group_id_or_name}'")
+    elsif group_response.data.security_groups.count > 1
+      Puppet.warning "Multiple groups found called #{group_id_or_name}"
+    end
+
+    group_response.data.security_groups.first.group_id
+  end
+
+  def rule_to_permission(rule)
+    # fallback to current group id if cidr is absent
+    sg = rule['security_group'] || (rule['cidr'] ? nil : @property_hash[:id])
+
+    { ip_protocol: rule['protocol'],
+      from_port:   Array(rule['port']).first,
+      to_port:     Array(rule['port']).last,
+      ip_ranges:   Array(rule['cidr']).map {|c| {cidr_ip: c}},
+      user_id_group_pairs:
+        Array(sg).map{|sgi| {group_id: id_or_name_to_id(sgi)} }}.
+      delete_if {|k,v| v.is_a?(Array) && v.empty?}.
+      delete_if {|k,v| v.nil?}
+  end
+
   def authorize_ingress(new_rules, existing_rules=[])
-    ec2 = ec2_client(resource[:region])
+    require 'pry'
+    binding.pry
+    ec2 = ec2_client(@property_hash[:region])
     new_rules = [new_rules] unless new_rules.is_a?(Array)
 
     to_create = new_rules - existing_rules
     to_delete = existing_rules - new_rules
 
-
-    to_create.reject(&:nil?).each do |rule|
-      if rule.key? 'security_group'
-        source_group_name = rule['security_group']
-        filters = [ {name: 'group-name', values: [source_group_name]} ]
-        if vpc_only_account?
-          response = ec2.describe_security_groups(group_ids: [@property_hash[:id]])
-          vpc_id = response.data.security_groups.first.vpc_id
-          filters.push( {name: 'vpc-id', values: [vpc_id]} )
-        end
-        group_response = ec2.describe_security_groups(filters: filters)
-        match_count = group_response.data.security_groups.count
-        fail("No groups found called #{source_group_name}") if match_count == 0
-        source_group_id = group_response.data.security_groups.first.group_id
-        Puppet.warning "#{match_count} groups found called #{source_group_name}, using #{source_group_id}" if match_count > 1
-
-        permissions = ['tcp', 'udp', 'icmp'].collect do |protocol|
-          {
-            ip_protocol: protocol,
-            to_port: protocol == 'icmp' ? -1 : 65535,
-            from_port: protocol == 'icmp' ? -1 : 1,
-            user_id_group_pairs: [{
-              group_id: source_group_id
-            }]
-          }
-        end
-
-        ec2.authorize_security_group_ingress(
-          group_id: @property_hash[:id],
-          ip_permissions: permissions
-        )
-      else
-        ec2.authorize_security_group_ingress(
-          group_id: @property_hash[:id],
-          ip_permissions: [{
-            ip_protocol: rule['protocol'],
-            to_port: rule['port'].to_i,
-            from_port: rule['port'].to_i,
-            ip_ranges: [{
-              cidr_ip: rule['cidr']
-            }]
-          }]
-        )
-      end
+    to_create.compact.each do |rule|
+      ec2.authorize_security_group_ingress(
+        group_id: @property_hash[:id],
+        ip_permissions: [rule_to_permission(rule)])
     end
 
-    to_delete.reject(&:nil?).each do |rule|
-      if rule.key? 'security_group'
-         ec2.revoke_security_group_ingress(
-          group_id: @property_hash[:id],
-          source_security_group_name: rule['security_group']
-        )
-      else
-        ec2.revoke_security_group_ingress(
-          group_id: @property_hash[:id],
-          ip_permissions: [{
-            ip_protocol: rule['protocol'],
-            to_port: rule['port'].to_i,
-            from_port: rule['port'].to_i,
-            ip_ranges: [{
-              cidr_ip: rule['cidr']
-            }]
-          }]
-        )
-      end
+    to_delete.compact.each do |rule|
+      ec2.revoke_security_group_ingress(
+        group_id: @property_hash[:id],
+        ip_permissions: [rule_to_permission(rule)])
     end
-
   end
 
   def ingress=(value)

lib/puppet/type/ec2_securitygroup.rb

@@ -6,7 +6,11 @@
   ensurable
 
   newparam(:name, namevar: true) do
-    desc 'the name of the security group'
+    desc 'the Name tag of the security group'
+  end
+
+  newparam(:group_name) do
+    desc 'the name of the security group in AWS'
     validate do |value|
       fail Puppet::Error, 'Security groups must have a name' if value == ''
     end
@@ -22,21 +26,23 @@
   newproperty(:ingress, :array_matching => :all) do
     desc 'rules for ingress traffic'
     def insync?(is)
-      order_ingress(should) == order_ingress(stringify_values(is))
+      order_ingress(stringify_values(should)) == order_ingress(stringify_values(is))
     end
 
     def order_ingress(rules)
-      groups, ports = rules.partition { |rule| rule['security_group'] }
-      groups.sort_by! { |group| group['security_group'] }
-      ports.sort! { |a, b| [a['protocol'], a['port']] <=> [b['protocol'], b['port']] }
+      cidrs, groups = rules.partition { |rule| rule['cidr'] }
+      groups.sort_by! do |g|
+        %w{security_group protocol port}.map{|k| g[k] || ' '}.flatten.join '!'
+      end
+      cidrs.sort_by! do |g|
+        %w{cidr protocol port}.map{|k| g[k] || ' '}.flatten.join '!'
+      end
 
-      groups + ports
+      groups + cidrs
     end
 
     def stringify_values(rules)
-      rules.collect do |obj|
-        obj.each { |k,v| obj[k] = v.to_s }
-      end
+      rules.map {|rule| rule.inject({}) { |h,kv| h.merge!(Hash[*kv]) } }
     end
   end
 

lib/puppet_x/puppetlabs/aws.rb

@@ -1,5 +1,13 @@
 require 'aws-sdk-core'
 
+if ENV['AWS_DEBUG'] == '1'
+  require 'logger'
+  logger = Logger.new($stdout)
+  logger.formatter = proc {|severity, datetime, progname, msg| msg }
+  Aws.config[:logger] = logger
+  Aws.config[:log_formatter] = Seahorse::Client::Logging::Formatter.colored
+end
+
 module PuppetX
   module Puppetlabs
     class Aws < Puppet::Provider
@@ -91,11 +99,11 @@ def self.name_from_tag(item)
       end
 
       def self.tags_for(item)
-        tags = {}
-        item.tags.each do |tag|
-          tags[tag.key] = tag.value unless tag.key == 'Name'
-        end
-        tags
+        tags_from_data(item.tags)
+      end
+
+      def self.tags_from_data(tags)
+        tags.inject({}){|th,kv| th.merge!(kv.key => kv.value)}
       end
 
       def tags=(value)