Merge pull request #5329 from hashicorp/b-5310

provider/aws: Fix crash creating rules in aws SGs

builtin/providers/aws/resource_aws_security_group_rule.go

@@ -149,7 +149,9 @@ information and instructions for recovery. Error message: %s`, awsErr.Message())
 			ruleType, autherr)
 	}
 
-	d.SetId(ipPermissionIDHash(sg_id, ruleType, perm))
+	id := ipPermissionIDHash(sg_id, ruleType, perm)
+	d.SetId(id)
+	log.Printf("[DEBUG] Security group rule ID set to %s", id)
 
 	return resourceAwsSecurityGroupRuleRead(d, meta)
 }
@@ -164,6 +166,8 @@ func resourceAwsSecurityGroupRuleRead(d *schema.ResourceData, meta interface{})
 		return nil
 	}
 
+	isVPC := sg.VpcId != nil && *sg.VpcId != ""
+
 	var rule *ec2.IpPermission
 	var rules []*ec2.IpPermission
 	ruleType := d.Get("type").(string)
@@ -215,8 +219,14 @@ func resourceAwsSecurityGroupRuleRead(d *schema.ResourceData, meta interface{})
 		remaining = len(p.UserIdGroupPairs)
 		for _, ip := range p.UserIdGroupPairs {
 			for _, rip := range r.UserIdGroupPairs {
-				if *ip.GroupId == *rip.GroupId {
-					remaining--
+				if isVPC {
+					if *ip.GroupId == *rip.GroupId {
+						remaining--
+					}
+				} else {
+					if *ip.GroupName == *rip.GroupName {
+						remaining--
+					}
 				}
 			}
 		}
@@ -250,7 +260,11 @@ func resourceAwsSecurityGroupRuleRead(d *schema.ResourceData, meta interface{})
 
 	if len(p.UserIdGroupPairs) > 0 {
 		s := p.UserIdGroupPairs[0]
-		d.Set("source_security_group_id", *s.GroupId)
+		if isVPC {
+			d.Set("source_security_group_id", *s.GroupId)
+		} else {
+			d.Set("source_security_group_id", *s.GroupName)
+		}
 	}
 
 	return nil
@@ -406,6 +420,7 @@ func expandIPPerm(d *schema.ResourceData, sg *ec2.SecurityGroup) (*ec2.IpPermiss
 	}
 
 	if v, ok := d.GetOk("self"); ok && v.(bool) {
+		// if sg.GroupId != nil {
 		if sg.VpcId != nil && *sg.VpcId != "" {
 			groups[*sg.GroupId] = true
 		} else {

builtin/providers/aws/resource_aws_security_group_rule_test.go

@@ -343,6 +343,24 @@ func TestAccAWSSecurityGroupRule_PartialMatching_Source(t *testing.T) {
 	})
 }
 
+func TestAccAWSSecurityGroupRule_Issue5310(t *testing.T) {
+	var group ec2.SecurityGroup
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckAWSSecurityGroupRuleDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccAWSSecurityGroupRuleIssue5310,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSSecurityGroupRuleExists("aws_security_group.issue_5310", &group),
+				),
+			},
+		},
+	})
+}
+
 func TestAccAWSSecurityGroupRule_Race(t *testing.T) {
 	var group ec2.SecurityGroup
 
@@ -527,6 +545,26 @@ resource "aws_security_group_rule" "ingress_1" {
 }
 `
 
+const testAccAWSSecurityGroupRuleIssue5310 = `
+provider "aws" {
+        region = "us-east-1"
+}
+
+resource "aws_security_group" "issue_5310" {
+    name = "terraform-test-issue_5310"
+    description = "SG for test of issue 5310"
+}
+
+resource "aws_security_group_rule" "issue_5310" {
+    type = "ingress"
+    from_port = 0
+    to_port = 65535
+    protocol = "tcp"
+    security_group_id = "${aws_security_group.issue_5310.id}"
+    self = true
+}
+`
+
 const testAccAWSSecurityGroupRuleIngressClassicConfig = `
 provider "aws" {
         region = "us-east-1"