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.

Author: garethr

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',

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,

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

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',

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

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