From 9ac929597b791653e19d4fd2501434af3f109dc1 Mon Sep 17 00:00:00 2001 From: Iristyle Date: Fri, 12 Dec 2014 17:35:57 -0800 Subject: [PATCH] (fix) Order independent ingress insync detection - Previously, it was possible to trivially generate spurious change notifications when the specified order of ingress rules inside a manifest did not match the order returned by AWS. AWS always returns an ordered set of ip_permissions, first by protocol, then by port. Since the conversion to group authorizations is lossy, segregate and individually sort those rules by group name. - Verify with an additional acceptance test that orders ingress rules within a manifest differently than what AWS returns, which would have previously caused such change notifications. - To support scanning Puppet apply output, it was necessary to change from system() to Open3.popen2e, and PuppetManifest.apply was updated to return a hash with the output lines and exit_status rather than simply returning a true / false success value. Several callsites were updated accordingly. --- lib/puppet/type/ec2_securitygroup.rb | 11 ++++- spec/acceptance/instance_spec.rb | 2 +- spec/acceptance/securitygroup_spec.rb | 68 ++++++++++++++++++++++++++- spec/spec_helper_acceptance.rb | 15 +++++- 4 files changed, 91 insertions(+), 5 deletions(-) diff --git a/lib/puppet/type/ec2_securitygroup.rb b/lib/puppet/type/ec2_securitygroup.rb index 2fa07dcb..d225db8b 100644 --- a/lib/puppet/type/ec2_securitygroup.rb +++ b/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 } diff --git a/spec/acceptance/instance_spec.rb b/spec/acceptance/instance_spec.rb index 2123d378..993eb46f 100644 --- a/spec/acceptance/instance_spec.rb +++ b/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 diff --git a/spec/acceptance/securitygroup_spec.rb b/spec/acceptance/securitygroup_spec.rb index b5bb5943..a2bf7732 100644 --- a/spec/acceptance/securitygroup_spec.rb +++ b/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 diff --git a/spec/spec_helper_acceptance.rb b/spec/spec_helper_acceptance.rb index 656d98aa..9ef48ffb 100644 --- a/spec/spec_helper_acceptance.rb +++ b/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)