Merge pull request #273 from daveseff/securitygroup_egress

Adding egress rules for VPC security groups.

Author: gregohardy

README.md

@@ -879,6 +879,11 @@ Rules for ingress traffic.
 
 Accepts an array.
 
+##### `egress`
+Optional.
+
+Rules for egress traffic. Accepts an array. If no egress rules are specified, the security group will default to ALL outbound traffic allowed.
+
 ##### `id`
 
 Read-only.

lib/puppet/provider/ec2_securitygroup/v2.rb

@@ -43,7 +43,7 @@ def self.prefetch(resources)
     end
   end
 
-  def self.prepare_ingress_rule_for_puppet(region, rule, groups, group = nil, cidr = nil)
+  def self.prepare_rule_for_puppet(region, rule, groups, group = nil, cidr = nil)
     config = {
       'protocol' => rule.ip_protocol,
       'from_port' => rule.from_port.to_i,
@@ -60,17 +60,22 @@ def self.prepare_ingress_rule_for_puppet(region, rule, groups, group = nil, cidr
     config
   end
 
-  def self.format_ingress_rules(region, group, groups)
+  def self.format_rules(region, direction, group, groups)
     rules = []
-    group[:ip_permissions].each do |rule|
+    if direction == :ingress
+      group_rules = group[:ip_permissions]
+    elsif direction == :egress
+      group_rules = group[:ip_permissions_egress]
+    end
+    group_rules.each do |rule|
       addition = []
       rule.user_id_group_pairs.each do |security_group|
-        addition << prepare_ingress_rule_for_puppet(region, rule, groups, security_group)
+        addition << prepare_rule_for_puppet(region, rule, groups, security_group)
       end
       rule.ip_ranges.each do |cidr|
-        addition << prepare_ingress_rule_for_puppet(region, rule, groups, nil, cidr)
+        addition << prepare_rule_for_puppet(region, rule, groups, nil, cidr)
       end
-      addition << prepare_ingress_rule_for_puppet(region, rule, groups) if addition.empty?
+      addition << prepare_rule_for_puppet(region, rule, groups) if addition.empty?
       rules << addition
     end
     rules.flatten.uniq.compact
@@ -82,7 +87,8 @@ def self.security_group_to_hash(region, group, groups, vpcs)
       name: group.group_name,
       description: group.description,
       ensure: :present,
-      ingress: format_ingress_rules(region, group, groups),
+      ingress: format_rules(region, :ingress, group, groups),
+      egress: format_rules(region, :egress, group, groups),
       vpc: vpcs[group.vpc_id],
       vpc_id: group.vpc_id,
       region: region,
@@ -127,12 +133,14 @@ def create
     ) if resource[:tags]
 
     @property_hash[:id] = response.group_id
-    rules = resource[:ingress]
-    authorize_ingress(rules)
+    ingress_rules = resource[:ingress]
+    egress_rules = resource[:egress]
+    authorize_rules(ingress_rules, :ingress)
+    authorize_rules(egress_rules, :egress)
     @property_hash[:ensure] = :present
   end
 
-  def prepare_ingress_for_api(rule)
+  def prepare_rule_for_api(rule)
     ec2 = ec2_client(resource[:region])
     from_port ||= rule['from_port'] || rule['port'] || 1
     to_port ||= rule['to_port'] || rule['port'] || 65535
@@ -185,27 +193,37 @@ def prepare_ingress_for_api(rule)
     rule_hash[:ip_permissions].any? ? rule_hash : nil
   end
 
-  def authorize_ingress(new_rules, existing_rules=[])
+  def authorize_rules(new_rules, direction, existing_rules=[])
     ec2 = ec2_client(resource[:region])
     new_rules = [new_rules] unless new_rules.is_a?(Array)
 
     parser = PuppetX::Puppetlabs::AwsIngressRulesParser.new(new_rules)
     to_create = parser.rules_to_create(existing_rules)
     to_delete = parser.rules_to_delete(existing_rules)
 
-    to_delete.compact.each do |rule|
-      prepared_rule = prepare_ingress_for_api(rule) and
-        ec2.revoke_security_group_ingress(prepared_rule)
+    to_delete.reject(&:nil?).each do |rule|
+      if direction == :ingress
+        ec2.revoke_security_group_ingress(prepare_rule_for_api(rule))
+      elsif direction == :egress
+        ec2.revoke_security_group_egress(prepare_rule_for_api(rule))
+      end
     end
 
-    to_create.compact.each do |rule|
-      prepared_rule = prepare_ingress_for_api(rule) and
-        ec2.authorize_security_group_ingress(prepared_rule)
+    to_create.each do |rule|
+      if direction == :ingress
+        ec2.authorize_security_group_ingress(prepare_rule_for_api(rule))
+      elsif direction == :egress
+        ec2.authorize_security_group_egress(prepare_rule_for_api(rule))
+      end
     end
   end
 
   def ingress=(value)
-    authorize_ingress(value, @property_hash[:ingress])
+    authorize_rules(value, :ingress, @property_hash[:ingress])
+  end
+
+  def egress=(value)
+    authorize_rules(value, :egress, @property_hash[:egress])
   end
 
   def destroy

lib/puppet/type/ec2_securitygroup.rb

@@ -34,6 +34,21 @@ def insync?(is)
     end
   end
 
+  newproperty(:egress, :array_matching => :all) do
+    desc 'rules for egress traffic'
+    def insync?(is)
+      for_comparison = Marshal.load(Marshal.dump(should))
+      parser = PuppetX::Puppetlabs::AwsIngressRulesParser.new(for_comparison)
+      to_create = parser.rules_to_create(is)
+      to_delete = parser.rules_to_delete(is)
+      to_create.empty? && to_delete.empty?
+    end
+
+    validate do |value|
+      fail 'egress should be a Hash' unless value.is_a?(Hash)
+    end
+  end
+
   newproperty(:tags, :parent => PuppetX::Property::AwsTag) do
     desc 'the tags for the security group'
   end

spec/acceptance/securitygroup_spec.rb

@@ -33,8 +33,8 @@ def get_group_permission(ip_permissions, group_name, protocol, from_port, to_por
   end
 
   # a fairly naive matching algorithm, since the shape of ip_permissions is
-  # quite different than the shape of our ingress rules
-  def check_ingress_rule(rule, ip_permissions)
+  # quite different than the shape of our rules
+  def check_rule(rule, ip_permissions)
     if (rule.has_key? :security_group)
       group_name = rule[:security_group]
       protocols = rule[:protocol] || ['tcp', 'udp', 'icmp']
@@ -43,7 +43,7 @@ def check_ingress_rule(rule, ip_permissions)
         to_port = rule[:port] || rule[:to_port] || (protocol == 'icmp' ? -1 : 65535)
         get_group_permission(ip_permissions, group_name, protocol, from_port, to_port)
       end
-      msg = "Could not find ingress rule for #{group_name}"
+      msg = "Could not find rule for #{group_name}"
     else
       protocol = rule[:protocol] || 'tcp'
       from_port = rule[:port] || rule[:from_port] || (protocol == 'icmp' ? -1 : 1)
@@ -55,18 +55,18 @@ def check_ingress_rule(rule, ip_permissions)
         perm[:ip_ranges].any? { |ip| ip[:cidr_ip] == rule[:cidr] }
       end
 
-      msg = "Could not find ingress rule for #{protocol} from port #{from_port} to #{to_port} with CIDR #{rule[:cidr]}"
+      msg = "Could not find rule for #{protocol} from port #{from_port} to #{to_port} with CIDR #{rule[:cidr]}"
     end
     [match, msg]
   end
 
-  def has_ingress_rule(rule, ip_permissions)
-    match, msg = check_ingress_rule(rule, ip_permissions)
+  def has_rule(rule, ip_permissions)
+    match, msg = check_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)
+  def doesnt_have_rule(rule, ip_permissions)
+    match, msg = check_rule(rule, ip_permissions)
     expect(match).to eq(false), msg
   end
 
@@ -89,6 +89,15 @@ def doesnt_have_ingress_rule(rule, ip_permissions)
             :cidr     => '0.0.0.0/0'
           }
         ],
+        :egress => [
+          {
+            :security_group => @name,
+          },{
+            :protocol => 'tcp',
+            :port     => 8080,
+            :cidr     => '0.0.0.0/0'
+          }
+        ],
         :tags => {
           :department => 'engineering',
           :project    => 'cloud',
@@ -125,13 +134,18 @@ def doesnt_have_ingress_rule(rule, ip_permissions)
 
     it "with the specified ingress rules" do
       # perform a naive match
-      @config[:ingress].all? { |rule| has_ingress_rule(rule, @group.ip_permissions)}
+      @config[:ingress].all? { |rule| has_rule(rule, @group.ip_permissions)}
+    end
+
+    it "with the specified egress rules" do
+      # perform a naive match
+      @config[:egress].all? { |rule| has_rule(rule, @group.ip_permissions)}
     end
 
     it 'should be able to modify the ingress rules and recreate the security group' do
       new_rules = [{
         :protocol => 'tcp',
-        :port     => 80,
+        :port     => 8080,
         :cidr     => '0.0.0.0/0'
       }]
       new_config = @config.dup.update({:ingress => new_rules})
@@ -141,8 +155,25 @@ def doesnt_have_ingress_rule(rule, ip_permissions)
       # should still have the original rules
       @group = get_group(@config[:name])
 
-      new_rules.all? { |rule| has_ingress_rule(rule, @group.ip_permissions)}
-      @config[:ingress].all? { |rule| doesnt_have_ingress_rule(rule, @group.ip_permissions)}
+      new_rules.all? { |rule| has_rule(rule, @group.ip_permissions)}
+      @config[:ingress].all? { |rule| doesnt_have_rule(rule, @group.ip_permissions)}
+    end
+
+    it 'should be able to modify the egress rules and recreate the security group' do
+      new_rules = [{
+        :protocol => 'tcp',
+        :port     => 80,
+        :cidr     => '0.0.0.0/0'
+      }]
+      new_config = @config.dup.update({:egress => new_rules})
+      result = PuppetManifest.new(@template, new_config).apply
+      expect(result.exit_code).to eq(2)
+
+      # should still have the original rules
+      @group = get_group(@config[:name])
+
+      new_rules.all? { |rule| has_rule(rule, @group.ip_permissions_egress)}
+      @config[:egress].all? { |rule| doesnt_have_rule(rule, @group.ip_permissions_egress)}
     end
 
     describe 'that another group depends on in a secondary manifest' do
@@ -204,6 +235,13 @@ def doesnt_have_ingress_rule(rule, ip_permissions)
             :cidr     => '0.0.0.0/0'
           },
         ],
+        :egress => [
+          {
+            :protocol => 'tcp',
+            :port     => 8080,
+            :cidr     => '0.0.0.0/0'
+          },
+        ],
         :tags => {
           :department => 'engineering',
           :project    => 'cloud',
@@ -425,7 +463,7 @@ def expect_rule_matches(ingress_rule, ip_permission)
     end
 
     it "with the specified ingress rules" do
-      @config[:ingress].all? { |rule| has_ingress_rule(rule, @group.ip_permissions)}
+      @config[:ingress].all? { |rule| has_rule(rule, @group.ip_permissions)}
     end
 
     rules_to_test = [
@@ -474,8 +512,8 @@ def expect_rule_matches(ingress_rule, ip_permission)
 
         @group = get_group(@config[:name])
 
-        new_rules.all? { |rule| has_ingress_rule(rule, @group.ip_permissions)}
-        @config[:ingress].all? { |rule| doesnt_have_ingress_rule(rule, @group.ip_permissions)}
+        new_rules.all? { |rule| has_rule(rule, @group.ip_permissions)}
+        @config[:ingress].all? { |rule| doesnt_have_rule(rule, @group.ip_permissions)}
       end
     end
 

spec/unit/type/ec2_securitygroup_spec.rb

@@ -16,6 +16,7 @@
       :description,
       :region,
       :ingress,
+      :egress,
       :tags,
       :id,
     ]
@@ -60,6 +61,10 @@
     expect(type_class).to require_hash_for('ingress')
   end
 
+  it "should require egress to be a hash" do
+    expect(type_class).to require_hash_for('egress')
+  end
+
    it "should require tags to be a hash" do
     expect(type_class).to require_hash_for('tags')
   end