From 1379d78455b16f9b28e9b6062f74c47ae20d550e Mon Sep 17 00:00:00 2001 From: Gareth Rushgrove Date: Fri, 24 Oct 2014 10:41:59 +0100 Subject: [PATCH 1/5] allow self referencing security groups --- lib/puppet/type/ec2_securitygroup.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/puppet/type/ec2_securitygroup.rb b/lib/puppet/type/ec2_securitygroup.rb index b43c93f1..a1ee3312 100644 --- a/lib/puppet/type/ec2_securitygroup.rb +++ b/lib/puppet/type/ec2_securitygroup.rb @@ -32,11 +32,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 From eff79f877ddfab20a53e7cdd5278092a619648cf Mon Sep 17 00:00:00 2001 From: Iristyle Date: Thu, 30 Oct 2014 14:07:19 -0700 Subject: [PATCH 2/5] Update ec2_securitygroup_spec with correct params - :tags was missing --- spec/unit/type/ec2_securitygroup_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/unit/type/ec2_securitygroup_spec.rb b/spec/unit/type/ec2_securitygroup_spec.rb index d6c68d0a..6b1494b5 100644 --- a/spec/unit/type/ec2_securitygroup_spec.rb +++ b/spec/unit/type/ec2_securitygroup_spec.rb @@ -7,7 +7,8 @@ let :params do [ :name, - :ingress + :ingress, + :tags, ] end @@ -15,7 +16,7 @@ [ :ensure, :description, - :region + :region, ] end From 5ddfac2637ca690258fe2280360832f91d2bb325 Mon Sep 17 00:00:00 2001 From: Gareth Rushgrove Date: Fri, 24 Oct 2014 11:32:02 +0100 Subject: [PATCH 3/5] Allow for comparison of ingress rules Also outputs the rules in the puppet resource command, based on our simplified assumptions. --- lib/puppet/provider/ec2_securitygroup/v2.rb | 17 +++++++++++++++++ lib/puppet/type/ec2_securitygroup.rb | 10 +++++++++- spec/unit/type/ec2_securitygroup_spec.rb | 2 +- 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/lib/puppet/provider/ec2_securitygroup/v2.rb b/lib/puppet/provider/ec2_securitygroup/v2.rb index 64159375..ffae8a47 100644 --- a/lib/puppet/provider/ec2_securitygroup/v2.rb +++ b/lib/puppet/provider/ec2_securitygroup/v2.rb @@ -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 diff --git a/lib/puppet/type/ec2_securitygroup.rb b/lib/puppet/type/ec2_securitygroup.rb index a1ee3312..b91dd7b2 100644 --- a/lib/puppet/type/ec2_securitygroup.rb +++ b/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 diff --git a/spec/unit/type/ec2_securitygroup_spec.rb b/spec/unit/type/ec2_securitygroup_spec.rb index 6b1494b5..0628cc9b 100644 --- a/spec/unit/type/ec2_securitygroup_spec.rb +++ b/spec/unit/type/ec2_securitygroup_spec.rb @@ -7,7 +7,6 @@ let :params do [ :name, - :ingress, :tags, ] end @@ -17,6 +16,7 @@ :ensure, :description, :region, + :ingress, ] end From c2f2fe4c15bc8d19b8bfa3e2f4d90a85ddf7a827 Mon Sep 17 00:00:00 2001 From: Gareth Rushgrove Date: Fri, 24 Oct 2014 11:33:56 +0100 Subject: [PATCH 4/5] Update readonly securitygroup properties to match implementation --- lib/puppet/provider/ec2_securitygroup/v2.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/puppet/provider/ec2_securitygroup/v2.rb b/lib/puppet/provider/ec2_securitygroup/v2.rb index ffae8a47..12d437ee 100644 --- a/lib/puppet/provider/ec2_securitygroup/v2.rb +++ b/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| From 0347108ec681655efa013b311e3f729cff4fb00f Mon Sep 17 00:00:00 2001 From: Iristyle Date: Mon, 3 Nov 2014 10:31:33 -0800 Subject: [PATCH 5/5] Add securitygroup acceptance tests - Add basic Mustache template for the sake of acceptance runs - Tests ensure that security groups can be added / deleted, and that when added their name, tags, description and ingress rules match Ingress rule matching is a little naive and could be improved - Refactor acceptance test helper to further transform an array of hashes (like ingress rules) into a more generalized data structure that can be used by Mustache templates. Mustache doesn't seem to deal with nested arrays very well, but if the array is a list of hashes with a :value key, then that can be translated more easily into a template. --- .../acceptance/fixtures/securitygroup.pp.tmpl | 19 +++ spec/acceptance/securitygroup_spec.rb | 160 ++++++++++++++++++ spec/spec_helper_acceptance.rb | 37 +++- 3 files changed, 212 insertions(+), 4 deletions(-) create mode 100644 spec/acceptance/fixtures/securitygroup.pp.tmpl create mode 100644 spec/acceptance/securitygroup_spec.rb diff --git a/spec/acceptance/fixtures/securitygroup.pp.tmpl b/spec/acceptance/fixtures/securitygroup.pp.tmpl new file mode 100644 index 00000000..28510bd6 --- /dev/null +++ b/spec/acceptance/fixtures/securitygroup.pp.tmpl @@ -0,0 +1,19 @@ +ec2_securitygroup { '{{name}}': + ensure => {{ensure}}, + description => '{{description}}', + region => '{{region}}', + ingress => [ + {{#ingress}} + { + {{#values}} + {{k}} => '{{v}}', + {{/values}} + }, + {{/ingress}} + ], + tags => { + {{#tags}} + {{k}} => '{{v}}', + {{/tags}} + } +} diff --git a/spec/acceptance/securitygroup_spec.rb b/spec/acceptance/securitygroup_spec.rb new file mode 100644 index 00000000..2a3d4b34 --- /dev/null +++ b/spec/acceptance/securitygroup_spec.rb @@ -0,0 +1,160 @@ +require 'spec_helper_acceptance' +require 'securerandom' + +describe "ec2_securitygroup" do + + before(:all) do + @default_region = 'sa-east-1' + @ec2 = Ec2Helper.new(@default_region) + @template = 'securitygroup.pp.tmpl' + end + + def find_group(name) + groups = @ec2.get_groups(name) + expect(groups.count).to eq(1) + groups.first + end + + def has_matching_tags(group, tags) + group_tags = {} + group.tags.each { |s| group_tags[s.key.to_sym] = s.value if s.key != 'Name' } + + symmetric_difference = tags.to_set ^ group_tags.to_set + 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 => @name, + :ensure => 'present', + :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', + :created_by => 'aws-acceptance' + } + } + + PuppetManifest.new(@template, @config).apply + @group = find_group(@config[:name]) + end + + 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 + expect(@group.group_name).to eq(@config[:name]) + end + + it "with the specified tags" do + has_matching_tags(@group, @config[:tags]) + end + + it "with the specified description" do + 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 diff --git a/spec/spec_helper_acceptance.rb b/spec/spec_helper_acceptance.rb index 9af7c44e..ca26a924 100644 --- a/spec/spec_helper_acceptance.rb +++ b/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'] ||