Merge pull request #38 from Iristyle/fix-ingress-insync-detection

(fix) Order independent ingress insync detection

lib/puppet/type/ec2_securitygroup.rb

@@ -20,8 +20,17 @@
   newproperty(:ingress, :array_matching => :all) do
     desc 'rules for ingress traffic'
     def insync?(is)
-      should == stringify_values(is)
+      order_ingress(should) == 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']] }
+
+      groups + ports
+    end
+
     def stringify_values(rules)
       rules.collect do |obj|
         obj.each { |k,v| obj[k] = v.to_s }

spec/acceptance/instance_spec.rb

@@ -103,7 +103,7 @@ def has_matching_tags(instance, tags)
     end
 
     def expect_failed_apply(config)
-      success = PuppetManifest.new(@template, config).apply
+      success = PuppetManifest.new(@template, config).apply[:exit_status].success?
       expect(success).to eq(false)
 
       expect(@ec2.get_instances(config[:name])).to be_empty

spec/acceptance/securitygroup_spec.rb

@@ -114,7 +114,8 @@ def has_ingress_rule(rule, ip_permissions)
 
     it 'and should not be able to modify the ingress rules and recreate the security group' do
       new_config = @config.dup.update({:ingress => [{ :security_group => @name }]})
-      expect(PuppetManifest.new(@template, new_config).apply).to eq(false)
+      success = PuppetManifest.new(@template, new_config).apply[:exit_status].success?
+      expect(success).to eq(false)
 
       # should still have the original rules
       @group = find_group(@config[:name])
@@ -149,11 +150,74 @@ def has_ingress_rule(rule, ip_permissions)
       end
 
       it 'and should not fail to be applied multiple times' do
-        expect(PuppetManifest.new(@template, @config_2).apply).to eq(true)
+        success = PuppetManifest.new(@template, @config_2).apply[:exit_status].success?
+        expect(success).to eq(true)
       end
     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 => [
+          {
+            :protocol => 'udp',
+            :port     => 22,
+            :cidr     => '0.0.0.0/0'
+          },
+          {
+            :protocol => 'tcp',
+            :port     => 443,
+            :cidr     => '0.0.0.0/0'
+          },{
+            :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
+      expect { find_group(@config[:name]) }.to raise_error(Aws::EC2::Errors::InvalidGroupNotFound)
+    end
+
+    def expect_rule_matches(ingress_rule, ip_permission)
+      expect(ingress_rule[:protocol]).to eq(ip_permission.ip_protocol)
+      expect(ingress_rule[:port]).to eq(ip_permission.to_port)
+    end
+
+    it 'and does not emit change notifications on a second run when the manifest ingress rule ordering does not match the one returned by AWS' do
+      output = PuppetManifest.new(@template, @config).apply[:output]
+      @group = find_group(@config[:name])
+
+      # Puppet code not loaded, so can't call format_ingress_rules on ec2_securitygroup type
+      expect_rule_matches(@config[:ingress][2], @group[:ip_permissions][0])
+      expect_rule_matches(@config[:ingress][1], @group[:ip_permissions][1])
+      expect_rule_matches(@config[:ingress][0], @group[:ip_permissions][2])
+
+      # should still be considered insync despite ordering differences
+      changed = output.any? { |l| l.match('ingress changed') }
+      expect(changed).to eq(false)
+    end
+  end
+
   describe 'should create a new securitygroup' do
 
     before(:each) do

spec/spec_helper_acceptance.rb

@@ -1,5 +1,6 @@
 require 'aws-sdk-core'
 require 'mustache'
+require 'open3'
 
 class PuppetManifest < Mustache
   def initialize(file, config)
@@ -12,7 +13,19 @@ def initialize(file, config)
   end
   def apply
     manifest = self.render.gsub("\n", '')
-    system("bundle exec puppet apply --detailed-exitcodes -e \"#{manifest}\" --modulepath ../")
+    cmd = "bundle exec puppet apply --detailed-exitcodes -e \"#{manifest}\" --modulepath ../"
+    result = { output: [], exit_status: nil }
+
+    Open3.popen2e(cmd) do |stdin, stdout_err, wait_thr|
+      while line = stdout_err.gets
+        result[:output].push(line)
+        puts line
+      end
+
+      result[:exit_status] = wait_thr.value
+    end
+
+    result
   end
 
   def self.to_generalized_data(val)