From 138a2ec6a5d3af3350d12f0ea28724a44dfd3f16 Mon Sep 17 00:00:00 2001
From: Gareth Rushgrove <gareth@morethanseven.net>
Date: Thu, 19 Mar 2015 10:45:40 +0000
Subject: [PATCH] Allow setting ingress rules for default security groups in
 VPC

Due to default security groups all being named default we couldn't
reference them previously due to unique resouce naming conflicts. This
patch allows for a composite namevar only in the case of the default
group. Note that the composite name populates the VPC field
automatically, so you don't have to duplicate the information in a
separate property.
---
 examples/vpc-example/init.pp                | 13 +++++++++
 lib/puppet/provider/ec2_securitygroup/v2.rb |  9 ++++--
 lib/puppet/type/ec2_securitygroup.rb        | 32 +++++++++++++++++++--
 spec/acceptance/fixtures/vpc.pp.tmpl        | 21 ++++++++++++++
 spec/acceptance/vpc_spec.rb                 | 15 ++++++++--
 spec/unit/type/ec2_securitygroup_spec.rb    |  6 ++++
 6 files changed, 88 insertions(+), 8 deletions(-)

diff --git a/examples/vpc-example/init.pp b/examples/vpc-example/init.pp
index 90fe710e..2ff1c318 100644
--- a/examples/vpc-example/init.pp
+++ b/examples/vpc-example/init.pp
@@ -18,6 +18,19 @@
   }]
 }
 
+ec2_securitygroup { 'sample-vpc::default':
+  ensure      => present,
+  region      => 'sa-east-1',
+  description => 'default VPC security group',
+  ingress     => [{
+    protocol => 'tcp',
+    port     => 22,
+    cidr     => '0.0.0.0/0'
+  },{
+    security_group => 'default',
+  }],
+}
+
 ec2_vpc_subnet { 'sample-subnet':
   ensure            => present,
   region            => 'sa-east-1',
diff --git a/lib/puppet/provider/ec2_securitygroup/v2.rb b/lib/puppet/provider/ec2_securitygroup/v2.rb
index a410c73d..64ba6dbc 100644
--- a/lib/puppet/provider/ec2_securitygroup/v2.rb
+++ b/lib/puppet/provider/ec2_securitygroup/v2.rb
@@ -86,10 +86,13 @@ def self.security_group_to_hash(region, group)
         vpc_name_tag ? vpc_name_tag.value : nil
       end
     end
+    name = group[:group_name]
+    name = "#{vpc_name}::#{name}" if vpc_name && name == 'default'
     {
-      id: group.group_id,
-      name: group.group_name,
-      description: group.description,
+      name: name,
+      group_name: group[:group_name],
+      id: group[:group_id],
+      description: group[:description],
       ensure: :present,
       ingress: format_ingress_rules(ec2, group),
       vpc: vpc_name,
diff --git a/lib/puppet/type/ec2_securitygroup.rb b/lib/puppet/type/ec2_securitygroup.rb
index 74014975..916010e0 100644
--- a/lib/puppet/type/ec2_securitygroup.rb
+++ b/lib/puppet/type/ec2_securitygroup.rb
@@ -6,14 +6,20 @@
 
   ensurable
 
-  newparam(:name, namevar: true) do
-    desc 'the name of the security group'
+  newparam(:name) do
+    desc 'the name of the security group resource'
+    isnamevar
     validate do |value|
       fail 'security groups must have a name' if value == ''
       fail 'name should be a String' unless value.is_a?(String)
     end
   end
 
+  newparam(:group_name) do
+    desc 'the name of the security group'
+    isnamevar
+  end
+
   newproperty(:region) do
     desc 'the region in which to launch the security group'
     validate do |value|
@@ -51,6 +57,7 @@ def insync?(is)
 
   newproperty(:vpc) do
     desc 'A VPC to which the group should be associated'
+    isnamevar
     validate do |value|
       fail 'vpc should be a String' unless value.is_a?(String)
     end
@@ -71,4 +78,25 @@ def should_autorequire?(rule)
   autorequire(:ec2_vpc) do
     self[:vpc]
   end
+
+  # When you create a VPC you automatically get a security group called default. You can't change the name.
+  # This lack of uniqueness makes managing these default security groups difficult. Enter a composite namevar.
+  # We support two name formats:
+  #
+  #   1. {some-security-group}
+  #   2. {some-vpc-name}::default
+  #
+  # Note that we only support prefixing a security group name with the vpc name for the default security group
+  # at this point. This avoids the issue of otherwise needing to store the resources in two places for non-default
+  # VPC security groups.
+  #
+  # In the case of a a default security group, we maintain the full name (including the VPC name) in the name property
+  # as otherwise it won't be unique and uniqueness and composite namevars are fun.
+  def self.title_patterns
+    [
+      [ /^(([\w\-]+)::(default))$/, [ [ :name, lambda {|x| x} ], [ :vpc, lambda {|x| x} ], [ :group_name, lambda {|x| x} ] ] ],
+      [ /^((.*))$/, [ [ :name, lambda {|x| x} ], [ :group_name, lambda {|x| x} ] ] ]
+    ]
+  end
+
 end
diff --git a/spec/acceptance/fixtures/vpc.pp.tmpl b/spec/acceptance/fixtures/vpc.pp.tmpl
index 04cbb629..3fd492be 100644
--- a/spec/acceptance/fixtures/vpc.pp.tmpl
+++ b/spec/acceptance/fixtures/vpc.pp.tmpl
@@ -11,6 +11,27 @@ ec2_vpc { '{{name}}-vpc':
   },
 }
 
+ec2_securitygroup { '{{name}}-vpc::default':
+  ensure      => {{ensure}},
+  region      => '{{region}}',
+  description => 'default VPC security group',
+  ingress     => [
+    {{#security_group_ingress}}
+      {
+        {{#values}}
+          {{k}} => '{{v}}',
+        {{/values}}
+      },
+    {{/security_group_ingress}}
+  ],
+  tags        => {
+  {{#tags}}
+    {{k}} => '{{v}}',
+  {{/tags}}
+  },
+  require     => Ec2_vpc['{{name}}-vpc'],
+}
+
 ec2_securitygroup { '{{name}}-sg':
   ensure      => {{ensure}},
   description => 'VPC accceptance test',
diff --git a/spec/acceptance/vpc_spec.rb b/spec/acceptance/vpc_spec.rb
index fa783a47..eb715b27 100644
--- a/spec/acceptance/vpc_spec.rb
+++ b/spec/acceptance/vpc_spec.rb
@@ -50,10 +50,12 @@ def find_instance(name)
     finder(name, 'get_instances')
   end
 
-  def find_security_group(name)
-    group_response = @aws.ec2_client.describe_security_groups(filters: [
+  def find_security_group(name, vpc_id=nil)
+    filters = [
       {name: 'group-name', values: [name]}
-    ])
+    ]
+    filters << {name: 'vpc-id', values: [vpc_id]} unless vpc_id.nil?
+    group_response = @aws.ec2_client.describe_security_groups(filters: filters)
     items = group_response.data.security_groups
     expect(items.count).to eq(1)
     items.first
@@ -118,6 +120,7 @@ def generate_ip
       @vgw = find_vpn_gateway("#{@name}-vgw")
       @cgw = find_customer_gateway("#{@name}-cgw")
       @sg = find_security_group("#{@name}-sg")
+      @dsg = find_security_group('default', @vpc.vpc_id)
     end
 
     after(:all) do
@@ -155,6 +158,12 @@ def generate_ip
       expect(@aws.tag_difference(@sg, @config[:tags])).to be_empty
     end
 
+    it 'should manage the default VPC security group' do
+      expect(@dsg).not_to be_nil
+      expect(@dsg.vpc_id).to eq(@vpc.vpc_id)
+      expect(@aws.tag_difference(@dsg, @config[:tags])).to be_empty
+    end
+
     it 'should create a route table' do
       table = find_route_table("#{@name}-routes")
       expect(table).not_to be_nil
diff --git a/spec/unit/type/ec2_securitygroup_spec.rb b/spec/unit/type/ec2_securitygroup_spec.rb
index ce5c56b2..7074328a 100644
--- a/spec/unit/type/ec2_securitygroup_spec.rb
+++ b/spec/unit/type/ec2_securitygroup_spec.rb
@@ -67,4 +67,10 @@
     expect(type_class).to order_tags_on_output
   end
 
+  it 'should extract the vpc from a composite name' do
+    sg = type_class.new({name: 'sample::default'})
+    expect(sg[:vpc]).to eq('sample')
+    expect(sg[:group_name]).to eq('default')
+  end
+
 end