provider/aws: Update Security Group Rules to Version 2

builtin/providers/aws/resource_aws_security_group_rule.go

@@ -20,7 +20,7 @@ func resourceAwsSecurityGroupRule() *schema.Resource {
 		Read:   resourceAwsSecurityGroupRuleRead,
 		Delete: resourceAwsSecurityGroupRuleDelete,
 
-		SchemaVersion: 1,
+                SchemaVersion: 2,
 		MigrateState:  resourceAwsSecurityGroupRuleMigrateState,
 
 		Schema: map[string]*schema.Schema{
@@ -67,14 +67,15 @@ func resourceAwsSecurityGroupRule() *schema.Resource {
 				Optional:      true,
 				ForceNew:      true,
 				Computed:      true,
-				ConflictsWith: []string{"cidr_blocks"},
+                                ConflictsWith: []string{"cidr_blocks", "self"},
 			},
 
 			"self": &schema.Schema{
-				Type:     schema.TypeBool,
+                                Type:          schema.TypeBool,
-				Optional: true,
+                                Optional:      true,
-				Default:  false,
+                                Default:       false,
-				ForceNew: true,
+                                ForceNew:      true,
+                                ConflictsWith: []string{"cidr_blocks"},
 			},
 		},
 	}
@@ -142,7 +143,7 @@ information and instructions for recovery. Error message: %s`, awsErr.Message())
 			ruleType, autherr)
 	}
 
-	d.SetId(ipPermissionIDHash(ruleType, perm))
+        d.SetId(ipPermissionIDHash(sg_id, ruleType, perm))
 
 	return resourceAwsSecurityGroupRuleRead(d, meta)
 }
@@ -158,24 +159,67 @@ func resourceAwsSecurityGroupRuleRead(d *schema.ResourceData, meta interface{})
 	}
 
 	var rule *ec2.IpPermission
+        var rules []*ec2.IpPermission
 	ruleType := d.Get("type").(string)
-	var rl []*ec2.IpPermission
 	switch ruleType {
 	case "ingress":
-		rl = sg.IpPermissions
+                rules = sg.IpPermissions
 	default:
-		rl = sg.IpPermissionsEgress
+                rules = sg.IpPermissionsEgress
 	}
 
-	for _, r := range rl {
+        p := expandIPPerm(d, sg)
-		if d.Id() == ipPermissionIDHash(ruleType, r) {
+
-			rule = r
+        if len(rules) == 0 {
+                return fmt.Errorf("No IPPerms")
+        }
+
+        for _, r := range rules {
+                if r.ToPort != nil && *p.ToPort != *r.ToPort {
+                        continue
+                }
+
+                if r.FromPort != nil && *p.FromPort != *r.FromPort {
+                        continue
+                }
+
+                if r.IpProtocol != nil && *p.IpProtocol != *r.IpProtocol {
+                        continue
+                }
+
+                remaining := len(p.IpRanges)
+                for _, ip := range p.IpRanges {
+                        for _, rip := range r.IpRanges {
+                                if *ip.CidrIp == *rip.CidrIp {
+                                        remaining--
+                                }
+                        }
+                }
+
+                if remaining > 0 {
+                        continue
 		}
+
+                remaining = len(p.UserIdGroupPairs)
+                for _, ip := range p.UserIdGroupPairs {
+                        for _, rip := range r.UserIdGroupPairs {
+                                if *ip.GroupId == *rip.GroupId {
+                                        remaining--
+                                }
+                        }
+                }
+
+                if remaining > 0 {
+                        continue
+                }
+
+                log.Printf("[DEBUG] Found rule for Security Group Rule (%s): %s", d.Id(), r)
+                rule = r
 	}
 
 	if rule == nil {
-		log.Printf("[DEBUG] Unable to find matching %s Security Group Rule for Group %s",
+                log.Printf("[DEBUG] Unable to find matching %s Security Group Rule (%s) for Group %s",
-			ruleType, sg_id)
+                        ruleType, d.Id(), sg_id)
 		d.SetId("")
 		return nil
 	}
@@ -186,14 +230,14 @@ func resourceAwsSecurityGroupRuleRead(d *schema.ResourceData, meta interface{})
 	d.Set("type", ruleType)
 
 	var cb []string
-	for _, c := range rule.IpRanges {
+        for _, c := range p.IpRanges {
 		cb = append(cb, *c.CidrIp)
 	}
 
 	d.Set("cidr_blocks", cb)
 
-	if len(rule.UserIdGroupPairs) > 0 {
+        if len(p.UserIdGroupPairs) > 0 {
-		s := rule.UserIdGroupPairs[0]
+                s := p.UserIdGroupPairs[0]
 		d.Set("source_security_group_id", *s.GroupId)
 	}
 
@@ -285,8 +329,9 @@ func (b ByGroupPair) Less(i, j int) bool {
 	panic("mismatched security group rules, may be a terraform bug")
 }
 
-func ipPermissionIDHash(ruleType string, ip *ec2.IpPermission) string {
+func ipPermissionIDHash(sg_id, ruleType string, ip *ec2.IpPermission) string {
 	var buf bytes.Buffer
+        buf.WriteString(fmt.Sprintf("%s-", sg_id))
 	if ip.FromPort != nil && *ip.FromPort > 0 {
 		buf.WriteString(fmt.Sprintf("%d-", *ip.FromPort))
 	}

builtin/providers/aws/resource_aws_security_group_rule_migrate.go

@@ -17,6 +17,12 @@ func resourceAwsSecurityGroupRuleMigrateState(
 	case 0:
 		log.Println("[INFO] Found AWS Security Group State v0; migrating to v1")
 		return migrateSGRuleStateV0toV1(is)
+        case 1:
+                log.Println("[INFO] Found AWS Security Group State v1; migrating to v2")
+                // migrating to version 2 of the schema is the same as 0->1, since the
+                // method signature has changed now and will use the security group id in
+                // the hash
+                return migrateSGRuleStateV0toV1(is)
 	default:
 		return is, fmt.Errorf("Unexpected schema version: %d", v)
 	}
@@ -37,7 +43,7 @@ func migrateSGRuleStateV0toV1(is *terraform.InstanceState) (*terraform.InstanceS
 	}
 
 	log.Printf("[DEBUG] Attributes before migration: %#v", is.Attributes)
-	newID := ipPermissionIDHash(is.Attributes["type"], perm)
+        newID := ipPermissionIDHash(is.Attributes["security_group_id"], is.Attributes["type"], perm)
 	is.Attributes["id"] = newID
 	is.ID = newID
 	log.Printf("[DEBUG] Attributes after migration: %#v, new id: %s", is.Attributes, newID)

builtin/providers/aws/resource_aws_security_group_rule_test.go

@@ -2,7 +2,7 @@ package aws
 
 import (
 	"fmt"
-	"reflect"
+        "log"
 	"testing"
 
 	"github.com/aws/aws-sdk-go/aws"
@@ -90,15 +90,15 @@ func TestIpPermissionIDHash(t *testing.T) {
 		Type   string
 		Output string
 	}{
-		{simple, "ingress", "sg-82613597"},
+                {simple, "ingress", "sg-3403497314"},
-		{egress, "egress", "sg-363054720"},
+                {egress, "egress", "sg-1173186295"},
-		{egress_all, "egress", "sg-2766285362"},
+                {egress_all, "egress", "sg-766323498"},
-		{vpc_security_group_source, "egress", "sg-2661404947"},
+                {vpc_security_group_source, "egress", "sg-351225364"},
-		{security_group_source, "egress", "sg-1841245863"},
+                {security_group_source, "egress", "sg-2198807188"},
 	}
 
 	for _, tc := range cases {
-		actual := ipPermissionIDHash(tc.Type, tc.Input)
+                actual := ipPermissionIDHash("sg-12345", tc.Type, tc.Input)
 		if actual != tc.Output {
 			t.Errorf("input: %s - %s\noutput: %s", tc.Type, tc.Input, actual)
 		}
@@ -132,7 +132,7 @@ func TestAccAWSSecurityGroupRule_Ingress_VPC(t *testing.T) {
 				Config: testAccAWSSecurityGroupRuleIngressConfig,
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckAWSSecurityGroupRuleExists("aws_security_group.web", &group),
-					testAccCheckAWSSecurityGroupRuleAttributes(&group, "ingress"),
+                                        testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.ingress_1", &group, nil, "ingress"),
 					resource.TestCheckResourceAttr(
 						"aws_security_group_rule.ingress_1", "from_port", "80"),
 					testRuleCount,
@@ -169,7 +169,7 @@ func TestAccAWSSecurityGroupRule_Ingress_Classic(t *testing.T) {
 				Config: testAccAWSSecurityGroupRuleIngressClassicConfig,
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckAWSSecurityGroupRuleExists("aws_security_group.web", &group),
-					testAccCheckAWSSecurityGroupRuleAttributes(&group, "ingress"),
+                                        testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.ingress_1", &group, nil, "ingress"),
 					resource.TestCheckResourceAttr(
 						"aws_security_group_rule.ingress_1", "from_port", "80"),
 					testRuleCount,
@@ -231,7 +231,7 @@ func TestAccAWSSecurityGroupRule_Egress(t *testing.T) {
 				Config: testAccAWSSecurityGroupRuleEgressConfig,
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckAWSSecurityGroupRuleExists("aws_security_group.web", &group),
-					testAccCheckAWSSecurityGroupRuleAttributes(&group, "egress"),
+                                        testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.egress_1", &group, nil, "egress"),
 				),
 			},
 		},
@@ -256,6 +256,92 @@ func TestAccAWSSecurityGroupRule_SelfReference(t *testing.T) {
 	})
 }
 
+// testing partial match implementation
+func TestAccAWSSecurityGroupRule_PartialMatching_Basic(t *testing.T) {
+        var group ec2.SecurityGroup
+
+        p := ec2.IpPermission{
+                FromPort:   aws.Int64(80),
+                ToPort:     aws.Int64(80),
+                IpProtocol: aws.String("tcp"),
+                IpRanges: []*ec2.IpRange{
+                        &ec2.IpRange{CidrIp: aws.String("10.0.2.0/24")},
+                        &ec2.IpRange{CidrIp: aws.String("10.0.3.0/24")},
+                        &ec2.IpRange{CidrIp: aws.String("10.0.4.0/24")},
+                },
+        }
+
+        o := ec2.IpPermission{
+                FromPort:   aws.Int64(80),
+                ToPort:     aws.Int64(80),
+                IpProtocol: aws.String("tcp"),
+                IpRanges: []*ec2.IpRange{
+                        &ec2.IpRange{CidrIp: aws.String("10.0.5.0/24")},
+                },
+        }
+
+        resource.Test(t, resource.TestCase{
+                PreCheck:     func() { testAccPreCheck(t) },
+                Providers:    testAccProviders,
+                CheckDestroy: testAccCheckAWSSecurityGroupRuleDestroy,
+                Steps: []resource.TestStep{
+                        resource.TestStep{
+                                Config: testAccAWSSecurityGroupRulePartialMatching,
+                                Check: resource.ComposeTestCheckFunc(
+                                        testAccCheckAWSSecurityGroupRuleExists("aws_security_group.web", &group),
+                                        testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.ingress", &group, &p, "ingress"),
+                                        testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.other", &group, &o, "ingress"),
+                                        testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.nat_ingress", &group, &o, "ingress"),
+                                ),
+                        },
+                },
+        })
+}
+
+func TestAccAWSSecurityGroupRule_PartialMatching_Source(t *testing.T) {
+        var group ec2.SecurityGroup
+        var nat ec2.SecurityGroup
+        var p ec2.IpPermission
+
+        // This function creates the expected IPPermission with the group id from an
+        // external security group, needed because Security Group IDs are generated on
+        // AWS side and can't be known ahead of time.
+        setupSG := func(*terraform.State) error {
+                if nat.GroupId == nil {
+                        return fmt.Errorf("Error: nat group has nil GroupID")
+                }
+
+                p = ec2.IpPermission{
+                        FromPort:   aws.Int64(80),
+                        ToPort:     aws.Int64(80),
+                        IpProtocol: aws.String("tcp"),
+                        UserIdGroupPairs: []*ec2.UserIdGroupPair{
+                                &ec2.UserIdGroupPair{GroupId: nat.GroupId},
+                        },
+                }
+
+                return nil
+        }
+
+        resource.Test(t, resource.TestCase{
+                PreCheck:     func() { testAccPreCheck(t) },
+                Providers:    testAccProviders,
+                CheckDestroy: testAccCheckAWSSecurityGroupRuleDestroy,
+                Steps: []resource.TestStep{
+                        resource.TestStep{
+                                Config: testAccAWSSecurityGroupRulePartialMatching_Source,
+                                Check: resource.ComposeTestCheckFunc(
+                                        testAccCheckAWSSecurityGroupRuleExists("aws_security_group.web", &group),
+                                        testAccCheckAWSSecurityGroupRuleExists("aws_security_group.nat", &nat),
+                                        setupSG,
+                                        testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.source_ingress", &group, &p, "ingress"),
+                                ),
+                        },
+                },
+        })
+
+}
+
 func testAccCheckAWSSecurityGroupRuleDestroy(s *terraform.State) error {
 	conn := testAccProvider.Meta().(*AWSClient).ec2conn
 
@@ -319,14 +405,27 @@ func testAccCheckAWSSecurityGroupRuleExists(n string, group *ec2.SecurityGroup)
 	}
 }
 
-func testAccCheckAWSSecurityGroupRuleAttributes(group *ec2.SecurityGroup, ruleType string) resource.TestCheckFunc {
+func testAccCheckAWSSecurityGroupRuleAttributes(n string, group *ec2.SecurityGroup, p *ec2.IpPermission, ruleType string) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
-		p := &ec2.IpPermission{
+                rs, ok := s.RootModule().Resources[n]
-			FromPort:   aws.Int64(80),
+                if !ok {
-			ToPort:     aws.Int64(8000),
+                        return fmt.Errorf("Security Group Rule Not found: %s", n)
-			IpProtocol: aws.String("tcp"),
+                }
-			IpRanges:   []*ec2.IpRange{&ec2.IpRange{CidrIp: aws.String("10.0.0.0/8")}},
+
+                if rs.Primary.ID == "" {
+                        return fmt.Errorf("No Security Group Rule is set")
 		}
+
+                if p == nil {
+                        p = &ec2.IpPermission{
+                                FromPort:   aws.Int64(80),
+                                ToPort:     aws.Int64(8000),
+                                IpProtocol: aws.String("tcp"),
+                                IpRanges:   []*ec2.IpRange{&ec2.IpRange{CidrIp: aws.String("10.0.0.0/8")}},
+                        }
+                }
+
+                var matchingRule *ec2.IpPermission
 		var rules []*ec2.IpPermission
 		if ruleType == "ingress" {
 			rules = group.IpPermissions
@@ -338,15 +437,53 @@ func testAccCheckAWSSecurityGroupRuleAttributes(group *ec2.SecurityGroup, ruleTy
 			return fmt.Errorf("No IPPerms")
 		}
 
-		// Compare our ingress
+                for _, r := range rules {
-		if !reflect.DeepEqual(rules[0], p) {
+                        if r.ToPort != nil && *p.ToPort != *r.ToPort {
-			return fmt.Errorf(
+                                continue
-				"Got:\n\n%#v\n\nExpected:\n\n%#v\n",
+                        }
-				rules[0],
+
-				p)
+                        if r.FromPort != nil && *p.FromPort != *r.FromPort {
+                                continue
+                        }
+
+                        if r.IpProtocol != nil && *p.IpProtocol != *r.IpProtocol {
+                                continue
+                        }
+
+                        remaining := len(p.IpRanges)
+                        for _, ip := range p.IpRanges {
+                                for _, rip := range r.IpRanges {
+                                        if *ip.CidrIp == *rip.CidrIp {
+                                                remaining--
+                                        }
+                                }
+                        }
+
+                        if remaining > 0 {
+                                continue
+                        }
+
+                        remaining = len(p.UserIdGroupPairs)
+                        for _, ip := range p.UserIdGroupPairs {
+                                for _, rip := range r.UserIdGroupPairs {
+                                        if *ip.GroupId == *rip.GroupId {
+                                                remaining--
+                                        }
+                                }
+                        }
+
+                        if remaining > 0 {
+                                continue
+                        }
+                        matchingRule = r
 		}
 
-		return nil
+                if matchingRule != nil {
+                        log.Printf("[DEBUG] Matching rule found : %s", matchingRule)
+                        return nil
+                }
+
+                return fmt.Errorf("Error here\n\tlooking for %s, wasn't found in %s", p, rules)
 	}
 }
 
@@ -480,3 +617,104 @@ resource "aws_security_group_rule" "self" {
   security_group_id = "${aws_security_group.web.id}"
 }
 `
+
+const testAccAWSSecurityGroupRulePartialMatching = `
+resource "aws_vpc" "default" {
+  cidr_block = "10.0.0.0/16"
+  tags {
+    Name = "tf-sg-rule-bug"
+  }
+}
+
+resource "aws_security_group" "web" {
+    name = "tf-other"
+    vpc_id = "${aws_vpc.default.id}"
+    tags {
+        Name        = "tf-other-sg"
+    }
+}
+
+resource "aws_security_group" "nat" {
+    name = "tf-nat"
+    vpc_id = "${aws_vpc.default.id}"
+    tags {
+        Name        = "tf-nat-sg"
+    }
+}
+
+resource "aws_security_group_rule" "ingress" {
+    type        = "ingress"
+    from_port   = 80
+    to_port     = 80
+    protocol    = "tcp"
+    cidr_blocks = ["10.0.2.0/24", "10.0.3.0/24", "10.0.4.0/24"]
+
+   security_group_id = "${aws_security_group.web.id}"
+}
+
+resource "aws_security_group_rule" "other" {
+    type        = "ingress"
+    from_port   = 80
+    to_port     = 80
+    protocol    = "tcp"
+    cidr_blocks = ["10.0.5.0/24"]
+
+   security_group_id = "${aws_security_group.web.id}"
+}
+
+// same a above, but different group, to guard against bad hashing
+resource "aws_security_group_rule" "nat_ingress" {
+    type        = "ingress"
+    from_port   = 80
+    to_port     = 80
+    protocol    = "tcp"
+    cidr_blocks = ["10.0.2.0/24", "10.0.3.0/24", "10.0.4.0/24"]
+
+   security_group_id = "${aws_security_group.nat.id}"
+}
+`
+
+const testAccAWSSecurityGroupRulePartialMatching_Source = `
+resource "aws_vpc" "default" {
+  cidr_block = "10.0.0.0/16"
+  tags {
+    Name = "tf-sg-rule-bug"
+  }
+}
+
+resource "aws_security_group" "web" {
+    name = "tf-other"
+    vpc_id = "${aws_vpc.default.id}"
+    tags {
+        Name        = "tf-other-sg"
+    }
+}
+
+resource "aws_security_group" "nat" {
+    name = "tf-nat"
+    vpc_id = "${aws_vpc.default.id}"
+    tags {
+        Name        = "tf-nat-sg"
+    }
+}
+
+resource "aws_security_group_rule" "source_ingress" {
+    type        = "ingress"
+    from_port   = 80
+    to_port     = 80
+    protocol    = "tcp"
+
+                source_security_group_id = "${aws_security_group.nat.id}"
+   security_group_id = "${aws_security_group.web.id}"
+}
+
+resource "aws_security_group_rule" "other_ingress" {
+    type        = "ingress"
+    from_port   = 80
+    to_port     = 80
+    protocol    = "tcp"
+    cidr_blocks = ["10.0.2.0/24", "10.0.3.0/24", "10.0.4.0/24"]
+
+   security_group_id = "${aws_security_group.web.id}"
+}
+`