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

@@ -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

spec/acceptance/securitygroup_spec.rb

@@ -34,15 +34,15 @@ 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
       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}"
+      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