providers/aws: security group ingress rules can be updated

CHANGELOG.md

@@ -23,6 +23,8 @@ IMPROVEMENTS:
     can be used to configure custom providers and provisioners. [GH-192]
   * providers/aws: EIPs now expose `allocation_id` and `public_ip`
       attributes.
+  * providers/aws: Security group rules can be updated without a
+      destroy/create.
 
 BUG FIXES:
 

builtin/providers/aws/resource_aws_security_group.go

@@ -3,6 +3,7 @@ package aws
 import (
 	"fmt"
 	"log"
+	"reflect"
 	"time"
 
 	"github.com/hashicorp/terraform/helper/resource"
@@ -141,6 +142,77 @@ func resourceAwsSecurityGroupCreate(d *schema.ResourceData, meta interface{}) er
 }
 
 func resourceAwsSecurityGroupUpdate(d *schema.ResourceData, meta interface{}) error {
+	p := meta.(*ResourceProvider)
+	ec2conn := p.ec2conn
+
+	sgRaw, _, err := SGStateRefreshFunc(ec2conn, d.Id())()
+	if err != nil {
+		return err
+	}
+	if sgRaw == nil {
+		d.SetId("")
+		return nil
+	}
+	group := sgRaw.(*ec2.SecurityGroupInfo).SecurityGroup
+
+	if d.HasChange("ingress") {
+		o, n := d.GetChange("ingress")
+		if o == nil {
+			o = []interface{}{}
+		}
+		if n == nil {
+			n = []interface{}{}
+		}
+
+		oldRules := expandIPPerms(o.([]interface{}))
+		newRules := expandIPPerms(n.([]interface{}))
+
+		var add, remove []ec2.IPPerm
+		for _, p := range newRules {
+			// Check if we have had this rule before
+			exists := false
+			for _, old := range oldRules {
+				if reflect.DeepEqual(old, p) {
+					exists = true
+					break
+				}
+			}
+			if exists {
+				continue
+			}
+			add = append(add, p)
+		}
+		for _, p := range oldRules {
+			// Check if we have this rule to add
+			exists := false
+			for _, n := range newRules {
+				if reflect.DeepEqual(n, p) {
+					exists = true
+					break
+				}
+			}
+			if exists {
+				continue
+			}
+			remove = append(remove, p)
+		}
+
+		// TODO: We need to handle partial state better in the in-between
+		// in this update.
+
+		// Authorize the new rules
+		_, err := ec2conn.AuthorizeSecurityGroup(group, add)
+		if err != nil {
+			return fmt.Errorf("Error authorizing security group ingress rules: %s", err)
+		}
+
+		// Revoke the old rules
+		_, err = ec2conn.RevokeSecurityGroup(group, remove)
+		if err != nil {
+			return fmt.Errorf("Error authorizing security group ingress rules: %s", err)
+		}
+	}
+
 	return nil
 }
 

builtin/providers/aws/resource_aws_security_group_test.go

@@ -99,12 +99,31 @@ func TestAccAWSSecurityGroup_MultiIngress(t *testing.T) {
 					testAccCheckAWSSecurityGroupExists("aws_security_group.web", &group),
 				),
 			},
+		},
+	})
+}
+
+func TestAccAWSSecurityGroup_Change(t *testing.T) {
+	var group ec2.SecurityGroupInfo
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckAWSSecurityGroupDestroy,
+		Steps: []resource.TestStep{
 			resource.TestStep{
-				Config: testAccAWSSecurityGroupConfigMultiIngress,
+				Config: testAccAWSSecurityGroupConfig,
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckAWSSecurityGroupExists("aws_security_group.web", &group),
 				),
 			},
+			resource.TestStep{
+				Config: testAccAWSSecurityGroupConfigChange,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSSecurityGroupExists("aws_security_group.web", &group),
+					testAccCheckAWSSecurityGroupAttributesChanged(&group),
+				),
+			},
 		},
 	})
 }
@@ -210,6 +229,55 @@ func testAccCheckAWSSecurityGroupAttributes(group *ec2.SecurityGroupInfo) resour
 	}
 }
 
+func testAccCheckAWSSecurityGroupAttributesChanged(group *ec2.SecurityGroupInfo) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		p := []ec2.IPPerm{
+			ec2.IPPerm{
+				FromPort:  80,
+				ToPort:    9000,
+				Protocol:  "tcp",
+				SourceIPs: []string{"10.0.0.0/8"},
+			},
+			ec2.IPPerm{
+				FromPort:  80,
+				ToPort:    1234,
+				Protocol:  "tcp",
+				SourceIPs: []string{"10.0.0.0/8"},
+			},
+		}
+
+		if group.Name != "terraform_acceptance_test_example" {
+			return fmt.Errorf("Bad name: %s", group.Name)
+		}
+
+		if group.Description != "Used in the terraform acceptance tests" {
+			return fmt.Errorf("Bad description: %s", group.Description)
+		}
+
+		// Compare our ingress
+		if len(group.IPPerms) != 2 {
+			return fmt.Errorf(
+				"Got:\n\n%#v\n\nExpected:\n\n%#v\n",
+				group.IPPerms,
+				p)
+		}
+
+		if group.IPPerms[0].ToPort == 1234 {
+			group.IPPerms[1], group.IPPerms[0] =
+				group.IPPerms[0], group.IPPerms[1]
+		}
+
+		if !reflect.DeepEqual(group.IPPerms, p) {
+			return fmt.Errorf(
+				"Got:\n\n%#v\n\nExpected:\n\n%#v\n",
+				group.IPPerms,
+				p)
+		}
+
+		return nil
+	}
+}
+
 const testAccAWSSecurityGroupConfig = `
 resource "aws_security_group" "web" {
     name = "terraform_acceptance_test_example"
@@ -224,6 +292,27 @@ resource "aws_security_group" "web" {
 }
 `
 
+const testAccAWSSecurityGroupConfigChange = `
+resource "aws_security_group" "web" {
+    name = "terraform_acceptance_test_example"
+    description = "Used in the terraform acceptance tests"
+
+    ingress {
+        protocol = "tcp"
+        from_port = 80
+        to_port = 9000
+        cidr_blocks = ["10.0.0.0/8"]
+    }
+
+    ingress {
+        protocol = "tcp"
+        from_port = 80
+        to_port = 1234
+        cidr_blocks = ["10.0.0.0/8"]
+    }
+}
+`
+
 const testAccAWSSecurityGroupConfigVpc = `
 resource "aws_vpc" "foo" {
 	cidr_block = "10.1.0.0/16"