Merge branch 'improve-group-ingress-handling'

* improve-group-ingress-handling:
  Add securitygroup acceptance tests
  Update readonly securitygroup properties to match implementation
  Allow for comparison of ingress rules
  Update ec2_securitygroup_spec with correct params
  allow self referencing security groups

Conflicts:
	spec/acceptance/fixtures/securitygroup.pp.tmpl
	spec/acceptance/securitygroup_spec.rb

* Added check to ensure security group is not part of VPC from
  Gareth's tests.  Updated template includes ingress rules like
  the tests that I built.

* closes #14

lib/puppet/provider/ec2_securitygroup/v2.rb

@@ -14,7 +14,7 @@ def self.instances
     end.flatten
   end
 
-  read_only(:region)
+  read_only(:region, :ingress, :description)
 
   def self.prefetch(resources)
     instances.each do |prov|
@@ -24,11 +24,28 @@ def self.prefetch(resources)
     end
   end
 
+  def self.format_ingress_rules(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
+        {
+          'security_group' => rule.user_id_group_pairs.first.group_name
+        }
+      end
+    end.uniq
+  end
+
   def self.security_group_to_hash(region, group)
     {
       name: group[:group_name],
       description: group[:description],
       ensure: :present,
+      ingress: format_ingress_rules(group),
       region: region,
     }
   end

lib/puppet/type/ec2_securitygroup.rb

@@ -17,8 +17,16 @@
     end
   end
 
-  newparam(:ingress, :array_mathching => :all) do
+  newproperty(:ingress, :array_matching => :all) do
     desc 'rules for ingress traffic'
+    def insync?(is)
+      should == stringify_values(is)
+    end
+    def stringify_values(rules)
+      rules.collect do |obj|
+        obj.each { |k,v| obj[k] = v.to_s }
+      end
+    end
   end
 
   newparam(:tags, :array_matching => :all) do
@@ -32,11 +40,15 @@
     end
   end
 
+  def should_autorequire?(rule)
+    !rule.nil? and rule.key? 'security_group' and rule['security_group'] != name
+  end
+
   autorequire(:ec2_securitygroup) do
     rules = self[:ingress]
     rules = [rules] unless rules.is_a?(Array)
     rules.collect do |rule|
-      rule['security_group'] if !rule.nil? and rule.key? 'security_group'
+      rule['security_group'] if should_autorequire?(rule)
     end
   end
 end

spec/acceptance/fixtures/securitygroup.pp.tmpl

@@ -1,7 +1,16 @@
 ec2_securitygroup { '{{name}}':
   ensure => {{ensure}},
-  region => '{{region}}',
   description => '{{description}}',
+  region => '{{region}}',
+  ingress     => [
+    {{#ingress}}
+      {
+        {{#values}}
+          {{k}} => '{{v}}',
+        {{/values}}
+      },
+    {{/ingress}}
+  ],
   tags            => {
   {{#tags}}
     {{k}} => '{{v}}',

spec/acceptance/securitygroup_spec.rb

@@ -23,14 +23,56 @@ def has_matching_tags(group, tags)
     expect(symmetric_difference).to be_empty
   end
 
+  def get_group_permission(ip_permissions, group, protocol)
+    ip_permissions.detect do |perm|
+      pairs = perm[:user_id_group_pairs]
+      pairs.any? do |pair|
+        pair.group_name == group && perm[:ip_protocol] == protocol
+      end
+    end
+  end
+
+  # a fairly naive matching algorithm, since the shape of ip_permissions is
+  # quite different than the shape of our ingress rules
+  def has_ingress_rule(rule, ip_permissions)
+    if (rule.has_key? :security_group)
+      group_name = rule[:security_group]
+      # a match occurs when AWS has a TCP / UDP / ICMP perm setup for group
+      tcp_perm = get_group_permission(ip_permissions, group_name, 'tcp')
+      udp_perm = get_group_permission(ip_permissions, group_name, 'udp')
+      icmp_perm = get_group_permission(ip_permissions, group_name, 'icmp')
+      match = !tcp_perm.nil? && !udp_perm.nil? && !icmp_perm.nil?
+      expect(match).to eq(true), "Could not find ingress rule for #{group_name}"
+    else
+      match = ip_permissions.any? do |perm|
+        rule[:protocol] == perm[:ip_protocol] &&
+        rule[:port] == perm[:from_port] &&
+        rule[:port] == perm[:to_port] &&
+        perm[:ip_ranges].any? { |ip| ip[:cidr_ip] == rule[:cidr] }
+      end
+      msg = "Could not find ingress rule for #{rule[:protocol]} port #{rule[:port]} and CIDR #{rule[:cidr]}"
+      expect(match).to eq(true), msg
+    end
+  end
+
   describe 'should create a new security group' do
 
     before(:all) do
+      @name = "#{PuppetManifest.env_id}-#{SecureRandom.uuid}"
       @config = {
-        :name => "#{PuppetManifest.env_id}-#{SecureRandom.uuid}",
-        :region => @default_region,
+        :name => @name,
         :ensure => 'present',
-        :description => 'short lived group created by acceptance tests',
+        :description => 'aws acceptance securitygroup',
+        :region => @default_region,
+        :ingress => [
+          {
+            :security_group => @name,
+          },{
+            :protocol => 'tcp',
+            :port     => 22,
+            :cidr     => '0.0.0.0/0'
+          }
+        ],
         :tags => {
           :department => 'engineering',
           :project    => 'cloud',
@@ -45,6 +87,8 @@ def has_matching_tags(group, tags)
     after(:all) do
       new_config = @config.update({:ensure => 'absent'})
       PuppetManifest.new(@template, new_config).apply
+
+      expect(find_group(@config[:name])).to be_nil
     end
 
     it "with the specified name" do
@@ -63,6 +107,58 @@ def has_matching_tags(group, tags)
       expect(@group.description).to eq(@config[:description])
     end
 
+    it "with the specified ingress rules" do
+      # perform a naive match
+      @config[:ingress].all? { |rule| has_ingress_rule(rule, @group.ip_permissions)}
+    end
+
+  end
+
+  describe 'should create a new securitygroup' do
+
+    before(:each) do
+      @name = "#{PuppetManifest.env_id}-#{SecureRandom.uuid}"
+      @config = {
+        :name => @name,
+        :ensure => 'present',
+        :description => 'aws acceptance sg',
+        :region => @default_region,
+        :ingress => [
+          {
+            :security_group => @name,
+          },{
+            :protocol => 'tcp',
+            :port     => 22,
+            :cidr     => '0.0.0.0/0'
+          }
+        ],
+        :tags => {
+          :department => 'engineering',
+          :project    => 'cloud',
+          :created_by => 'aws-acceptance'
+        }
+      }
+
+      PuppetManifest.new(@template, @config).apply
+      @group = find_group(@config[:name])
+    end
+
+    after(:each) do
+      @config[:ensure] = 'absent'
+      PuppetManifest.new(@template, @config).apply
+    end
+
+    it 'that can have tags changed' do
+      pending 'changing tags not yet supported for security groups'
+      has_matching_tags(@group, @config[:tags])
+
+      tags = {:created_by => 'aws-tests', :foo => 'bar'}
+      @config[:tags].update(tags)
+
+      PuppetManifest.new(@template, @config).apply
+      @group = find_group(@config[:name])
+      has_matching_tags(@group, @config[:tags])
+    end
   end
 
 end

spec/spec_helper_acceptance.rb

@@ -5,10 +5,7 @@ class PuppetManifest < Mustache
   def initialize(file, config)
     @template_file = File.join(Dir.getwd, 'spec', 'acceptance', 'fixtures', file)
     config.each do |key, value|
-      config_value = value
-      if (value.class == Hash)
-        config_value = value.map { |k, v| { :k => k, :v => v }}
-      end
+      config_value = self.class.to_generalized_data(value)
       instance_variable_set("@#{key}".to_sym, config_value)
       self.class.send(:attr_accessor, key)
     end
@@ -18,6 +15,38 @@ def apply
     system("bundle exec puppet apply -e \"#{manifest}\" --modulepath ../")
   end
 
+  def self.to_generalized_data(val)
+    case val
+    when Hash
+      to_generalized_hash_list(val)
+    when Array
+      to_generalized_array_list(val)
+    else
+      val
+    end
+  end
+
+  # returns an array of :k =>, :v => hashes given a Hash
+  # { :a => 'b', :c => 'd' } -> [{:k => 'a', :v => 'b'}, {:k => 'c', :v => 'd'}]
+  def self.to_generalized_hash_list(hash)
+    hash.map { |k, v| { :k => k, :v => v }}
+  end
+
+  # necessary to build like [{ :values => Array }] rather than [[]] when there
+  # are nested hashes, for the sake of Mustache being able to render
+  # otherwise, simply return the item
+  def self.to_generalized_array_list(arr)
+    arr.map do |item|
+      if item.class == Hash
+        {
+          :values => to_generalized_hash_list(item)
+        }
+      else
+        item
+      end
+    end
+  end
+
   def self.env_id
     @env_id ||= (
       ENV['BUILD_DISPLAY_NAME'] ||

spec/unit/type/ec2_securitygroup_spec.rb

@@ -7,15 +7,16 @@
   let :params do
     [
       :name,
-      :ingress
+      :tags,
     ]
   end
 
   let :properties do
     [
       :ensure,
       :description,
-      :region
+      :region,
+      :ingress,
     ]
   end