From 0f0df627cb59c027620a0d28296c764f1bc09602 Mon Sep 17 00:00:00 2001 From: Gareth Rushgrove Date: Mon, 15 Dec 2014 14:25:32 +0000 Subject: [PATCH 1/2] Sort security_group ip_permissions - In CI, a new failure occurred as a result of code introduced in the previous commit to order security group ingress rules as specified in a manifest, in the same manner as those returned as ip_permissions by AWS. It would appear that Rubys sorting algorithm doesn't necessarily match the behavior of AWS. To ensure that the two sets are sorted identically, perform the same sort operation on the AWS returned rules. By normalizing the sort operation, the code is more resilient to AWS ordering, and generally future-proof should that behavior change on the AWS side. --- lib/puppet/type/ec2_securitygroup.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/puppet/type/ec2_securitygroup.rb b/lib/puppet/type/ec2_securitygroup.rb index d225db8b..15b4143c 100644 --- a/lib/puppet/type/ec2_securitygroup.rb +++ b/lib/puppet/type/ec2_securitygroup.rb @@ -20,7 +20,7 @@ newproperty(:ingress, :array_matching => :all) do desc 'rules for ingress traffic' def insync?(is) - order_ingress(should) == stringify_values(is) + order_ingress(should) == order_ingress(stringify_values(is)) end def order_ingress(rules) From c729800e5ab93df9c2fc1113a13446b1e3da8980 Mon Sep 17 00:00:00 2001 From: Gareth Rushgrove Date: Mon, 24 Nov 2014 11:33:57 +0000 Subject: [PATCH 2/2] Allow changing ingress rules after security group creation * Updated the acceptance test to check both that rules are added * and that rules are deleted --- lib/puppet/provider/ec2_securitygroup/v2.rb | 40 +++++++++++++++++++-- spec/acceptance/securitygroup_spec.rb | 30 ++++++++++++---- 2 files changed, 61 insertions(+), 9 deletions(-) diff --git a/lib/puppet/provider/ec2_securitygroup/v2.rb b/lib/puppet/provider/ec2_securitygroup/v2.rb index d7ddb347..50908ac7 100644 --- a/lib/puppet/provider/ec2_securitygroup/v2.rb +++ b/lib/puppet/provider/ec2_securitygroup/v2.rb @@ -17,7 +17,7 @@ def self.instances end.flatten end - read_only(:region, :ingress, :description) + read_only(:region, :description) def self.prefetch(resources) instances.each do |prov| @@ -78,9 +78,17 @@ def create ) unless tags.empty? rules = resource[:ingress] - rules = [rules] unless rules.is_a?(Array) + authorize_ingress(rules) + end + + def authorize_ingress(new_rules, existing_rules=[]) + ec2 = ec2_client(resource[:region]) + new_rules = [new_rules] unless new_rules.is_a?(Array) + + to_create = new_rules - existing_rules + to_delete = existing_rules - new_rules - rules.reject(&:nil?).each do |rule| + to_create.reject(&:nil?).each do |rule| if rule.key? 'security_group' ec2.authorize_security_group_ingress( group_name: name, @@ -100,6 +108,32 @@ def create ) end end + + to_delete.reject(&:nil?).each do |rule| + if rule.key? 'security_group' + ec2.revoke_security_group_ingress( + group_name: name, + source_security_group_name: rule['security_group'] + ) + else + ec2.revoke_security_group_ingress( + group_name: name, + 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 + end + + end + + def ingress=(value) + authorize_ingress(value, @property_hash[:ingress]) end def destroy diff --git a/spec/acceptance/securitygroup_spec.rb b/spec/acceptance/securitygroup_spec.rb index a2bf7732..269c25c8 100644 --- a/spec/acceptance/securitygroup_spec.rb +++ b/spec/acceptance/securitygroup_spec.rb @@ -34,7 +34,7 @@ def get_group_permission(ip_permissions, group, protocol) # 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) + def check_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 @@ -42,7 +42,7 @@ def has_ingress_rule(rule, ip_permissions) 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}" + msg = "Could not find ingress rule for #{group_name}" else match = ip_permissions.any? do |perm| rule[:protocol] == perm[:ip_protocol] && @@ -51,10 +51,21 @@ def has_ingress_rule(rule, ip_permissions) 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 + [match, msg] end + def has_ingress_rule(rule, ip_permissions) + match, msg = check_ingress_rule(rule, ip_permissions) + expect(match).to eq(true), msg + end + + def doesnt_have_ingress_rule(rule, ip_permissions) + match, msg = check_ingress_rule(rule, ip_permissions) + expect(match).to eq(false), msg + end + + describe 'should create a new security group' do before(:all) do @@ -112,14 +123,21 @@ def has_ingress_rule(rule, ip_permissions) @config[:ingress].all? { |rule| has_ingress_rule(rule, @group.ip_permissions)} end - 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 }]}) + it 'should be able to modify the ingress rules and recreate the security group' do + new_rules = [{ + :protocol => 'tcp', + :port => 80, + :cidr => '0.0.0.0/0' + }] + new_config = @config.dup.update({:ingress => new_rules}) 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]) - @config[:ingress].all? { |rule| has_ingress_rule(rule, @group.ip_permissions)} + + new_rules.all? { |rule| has_ingress_rule(rule, @group.ip_permissions)} + @config[:ingress].all? { |rule| doesnt_have_ingress_rule(rule, @group.ip_permissions)} end describe 'that another group depends on in a secondary manifest' do