provider/aws: Fix EC2 Classic SG Rule issue

Fixes an issue where security groups would fail to update after applying an
initial security_group, because we were improperly saving the id of the group
and not the name (EC2 Classic only).

This is a PR combining https://github.com/hashicorp/terraform/pull/4983 and
https://github.com/hashicorp/terraform/pull/5184 . It's majority
@ephemeralsnow's work.

builtin/providers/aws/resource_aws_elb.go

@@ -340,7 +340,11 @@ func resourceAwsElbRead(d *schema.ResourceData, meta interface{}) error {
 	d.Set("listener", flattenListeners(lb.ListenerDescriptions))
 	d.Set("security_groups", flattenStringList(lb.SecurityGroups))
 	if lb.SourceSecurityGroup != nil {
-		d.Set("source_security_group", lb.SourceSecurityGroup.GroupName)
+		group := lb.SourceSecurityGroup.GroupName
+		if lb.SourceSecurityGroup.OwnerAlias != nil && *lb.SourceSecurityGroup.OwnerAlias != "" {
+			group = aws.String(*lb.SourceSecurityGroup.OwnerAlias + "/" + *lb.SourceSecurityGroup.GroupName)
+		}
+		d.Set("source_security_group", group)
 
 		// Manually look up the ELB Security Group ID, since it's not provided
 		var elbVpc string

builtin/providers/aws/resource_aws_security_group.go

@@ -273,12 +273,8 @@ func resourceAwsSecurityGroupRead(d *schema.ResourceData, meta interface{}) erro
 
 	sg := sgRaw.(*ec2.SecurityGroup)
 
-	remoteIngressRules := resourceAwsSecurityGroupIPPermGather(d.Id(), sg.IpPermissions)
-	remoteEgressRules := resourceAwsSecurityGroupIPPermGather(d.Id(), sg.IpPermissionsEgress)
-
-	//
-	// TODO enforce the seperation of ips and security_groups in a rule block
-	//
+	remoteIngressRules := resourceAwsSecurityGroupIPPermGather(d.Id(), sg.IpPermissions, sg.OwnerId)
+	remoteEgressRules := resourceAwsSecurityGroupIPPermGather(d.Id(), sg.IpPermissionsEgress, sg.OwnerId)
 
 	localIngressRules := d.Get("ingress").(*schema.Set).List()
 	localEgressRules := d.Get("egress").(*schema.Set).List()
@@ -409,7 +405,7 @@ func resourceAwsSecurityGroupRuleHash(v interface{}) int {
 	return hashcode.String(buf.String())
 }
 
-func resourceAwsSecurityGroupIPPermGather(groupId string, permissions []*ec2.IpPermission) []map[string]interface{} {
+func resourceAwsSecurityGroupIPPermGather(groupId string, permissions []*ec2.IpPermission, ownerId *string) []map[string]interface{} {
 	ruleMap := make(map[string]map[string]interface{})
 	for _, perm := range permissions {
 		var fromPort, toPort int64
@@ -445,12 +441,9 @@ func resourceAwsSecurityGroupIPPermGather(groupId string, permissions []*ec2.IpP
 			m["cidr_blocks"] = list
 		}
 
-		var groups []string
-		if len(perm.UserIdGroupPairs) > 0 {
-			groups = flattenSecurityGroups(perm.UserIdGroupPairs)
-		}
-		for i, id := range groups {
-			if id == groupId {
+		groups := flattenSecurityGroups(perm.UserIdGroupPairs, ownerId)
+		for i, g := range groups {
+			if *g.GroupId == groupId {
 				groups[i], groups = groups[len(groups)-1], groups[:len(groups)-1]
 				m["self"] = true
 			}
@@ -464,7 +457,11 @@ func resourceAwsSecurityGroupIPPermGather(groupId string, permissions []*ec2.IpP
 			list := raw.(*schema.Set)
 
 			for _, g := range groups {
-				list.Add(g)
+				if g.GroupName != nil {
+					list.Add(*g.GroupName)
+				} else {
+					list.Add(*g.GroupId)
+				}
 			}
 
 			m["security_groups"] = list
@@ -531,12 +528,16 @@ func resourceAwsSecurityGroupUpdateRules(
 						GroupId:       group.GroupId,
 						IpPermissions: remove,
 					}
+					if group.VpcId == nil || *group.VpcId == "" {
+						req.GroupId = nil
+						req.GroupName = group.GroupName
+					}
 					_, err = conn.RevokeSecurityGroupIngress(req)
 				}
 
 				if err != nil {
 					return fmt.Errorf(
-						"Error authorizing security group %s rules: %s",
+						"Error revoking security group %s rules: %s",
 						ruleset, err)
 				}
 			}

builtin/providers/aws/resource_aws_security_group_test.go

@@ -24,7 +24,7 @@ func TestResourceAwsSecurityGroupIPPermGather(t *testing.T) {
 			IpRanges:   []*ec2.IpRange{&ec2.IpRange{CidrIp: aws.String("0.0.0.0/0")}},
 			UserIdGroupPairs: []*ec2.UserIdGroupPair{
 				&ec2.UserIdGroupPair{
-					GroupId: aws.String("sg-22222"),
+					GroupId: aws.String("sg-11111"),
 				},
 			},
 		},
@@ -33,8 +33,27 @@ func TestResourceAwsSecurityGroupIPPermGather(t *testing.T) {
 			FromPort:   aws.Int64(int64(80)),
 			ToPort:     aws.Int64(int64(80)),
 			UserIdGroupPairs: []*ec2.UserIdGroupPair{
+				// VPC
+				&ec2.UserIdGroupPair{
+					GroupId: aws.String("sg-22222"),
+				},
+			},
+		},
+		&ec2.IpPermission{
+			IpProtocol: aws.String("tcp"),
+			FromPort:   aws.Int64(int64(443)),
+			ToPort:     aws.Int64(int64(443)),
+			UserIdGroupPairs: []*ec2.UserIdGroupPair{
+				// Classic
 				&ec2.UserIdGroupPair{
-					GroupId: aws.String("foo"),
+					UserId:    aws.String("12345"),
+					GroupId:   aws.String("sg-33333"),
+					GroupName: aws.String("ec2_classic"),
+				},
+				&ec2.UserIdGroupPair{
+					UserId:    aws.String("amazon-elb"),
+					GroupId:   aws.String("sg-d2c979d3"),
+					GroupName: aws.String("amazon-elb-sg"),
 				},
 			},
 		},
@@ -53,12 +72,21 @@ func TestResourceAwsSecurityGroupIPPermGather(t *testing.T) {
 			"from_port": int64(80),
 			"to_port":   int64(80),
 			"security_groups": schema.NewSet(schema.HashString, []interface{}{
-				"foo",
+				"sg-22222",
+			}),
+		},
+		map[string]interface{}{
+			"protocol":  "tcp",
+			"from_port": int64(443),
+			"to_port":   int64(443),
+			"security_groups": schema.NewSet(schema.HashString, []interface{}{
+				"ec2_classic",
+				"amazon-elb/amazon-elb-sg",
 			}),
 		},
 	}
 
-	out := resourceAwsSecurityGroupIPPermGather("sg-22222", raw)
+	out := resourceAwsSecurityGroupIPPermGather("sg-11111", raw, aws.String("12345"))
 	for _, i := range out {
 		// loop and match rules, because the ordering is not guarneteed
 		for _, l := range local {
@@ -636,6 +664,94 @@ func TestAccAWSSecurityGroup_CIDRandGroups(t *testing.T) {
 	})
 }
 
+func TestAccAWSSecurityGroup_ingressWithCidrAndSGs(t *testing.T) {
+	var group ec2.SecurityGroup
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckAWSSecurityGroupDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccAWSSecurityGroupConfig_ingressWithCidrAndSGs,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSSecurityGroupExists("aws_security_group.web", &group),
+					testAccCheckAWSSecurityGroupSGandCidrAttributes(&group),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "name", "terraform_acceptance_test_example"),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "description", "Used in the terraform acceptance tests"),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "ingress.#", "2"),
+				),
+			},
+		},
+	})
+}
+
+// This test requires an EC2 Classic region
+func TestAccAWSSecurityGroup_ingressWithCidrAndSGs_classic(t *testing.T) {
+	var group ec2.SecurityGroup
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckAWSSecurityGroupDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccAWSSecurityGroupConfig_ingressWithCidrAndSGs_classic,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSSecurityGroupExists("aws_security_group.web", &group),
+					testAccCheckAWSSecurityGroupSGandCidrAttributes(&group),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "name", "terraform_acceptance_test_example"),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "description", "Used in the terraform acceptance tests"),
+					resource.TestCheckResourceAttr(
+						"aws_security_group.web", "ingress.#", "2"),
+				),
+			},
+		},
+	})
+}
+
+func testAccCheckAWSSecurityGroupSGandCidrAttributes(group *ec2.SecurityGroup) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		if *group.GroupName != "terraform_acceptance_test_example" {
+			return fmt.Errorf("Bad name: %s", *group.GroupName)
+		}
+
+		if *group.Description != "Used in the terraform acceptance tests" {
+			return fmt.Errorf("Bad description: %s", *group.Description)
+		}
+
+		if len(group.IpPermissions) == 0 {
+			return fmt.Errorf("No IPPerms")
+		}
+
+		if len(group.IpPermissions) != 2 {
+			return fmt.Errorf("Expected 2 ingress rules, got %d", len(group.IpPermissions))
+		}
+
+		for _, p := range group.IpPermissions {
+			if *p.FromPort == int64(22) {
+				if len(p.IpRanges) != 1 || p.UserIdGroupPairs != nil {
+					return fmt.Errorf("Found ip perm of 22, but not the right ipranges / pairs: %s", p)
+				}
+				continue
+			} else if *p.FromPort == int64(80) {
+				if len(p.IpRanges) != 1 || len(p.UserIdGroupPairs) != 1 {
+					return fmt.Errorf("Found ip perm of 80, but not the right ipranges / pairs: %s", p)
+				}
+				continue
+			}
+			return fmt.Errorf("Found a rouge rule")
+		}
+
+		return nil
+	}
+}
+
 func testAccCheckAWSSecurityGroupAttributesChanged(group *ec2.SecurityGroup) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		p := []*ec2.IpPermission{
@@ -1141,3 +1257,90 @@ resource "aws_security_group" "mixed" {
   }
 }
 `
+
+const testAccAWSSecurityGroupConfig_ingressWithCidrAndSGs = `
+resource "aws_security_group" "other_web" {
+  name        = "tf_other_acc_tests"
+  description = "Used in the terraform acceptance tests"
+
+  tags {
+    Name = "tf-acc-test"
+  }
+}
+
+resource "aws_security_group" "web" {
+  name        = "terraform_acceptance_test_example"
+  description = "Used in the terraform acceptance tests"
+
+  ingress {
+    protocol  = "tcp"
+    from_port = "22"
+    to_port   = "22"
+
+    cidr_blocks = [
+      "192.168.0.1/32",
+    ]
+  }
+
+  ingress {
+    protocol        = "tcp"
+    from_port       = 80
+    to_port         = 8000
+    cidr_blocks     = ["10.0.0.0/8"]
+    security_groups = ["${aws_security_group.other_web.id}"]
+  }
+
+  egress {
+    protocol    = "tcp"
+    from_port   = 80
+    to_port     = 8000
+    cidr_blocks = ["10.0.0.0/8"]
+  }
+
+  tags {
+    Name = "tf-acc-test"
+  }
+}
+`
+
+const testAccAWSSecurityGroupConfig_ingressWithCidrAndSGs_classic = `
+provider "aws" {
+	region = "us-east-1"
+}
+
+resource "aws_security_group" "other_web" {
+  name        = "tf_other_acc_tests"
+  description = "Used in the terraform acceptance tests"
+
+  tags {
+    Name = "tf-acc-test"
+  }
+}
+
+resource "aws_security_group" "web" {
+  name        = "terraform_acceptance_test_example"
+  description = "Used in the terraform acceptance tests"
+
+  ingress {
+    protocol  = "tcp"
+    from_port = "22"
+    to_port   = "22"
+
+    cidr_blocks = [
+      "192.168.0.1/32",
+    ]
+  }
+
+  ingress {
+    protocol        = "tcp"
+    from_port       = 80
+    to_port         = 8000
+    cidr_blocks     = ["10.0.0.0/8"]
+    security_groups = ["${aws_security_group.other_web.name}"]
+  }
+
+  tags {
+    Name = "tf-acc-test"
+  }
+}
+`

builtin/providers/aws/structure.go

@@ -137,7 +137,7 @@ func expandEcsLoadBalancers(configured []interface{}) []*ecs.LoadBalancer {
 // to_port or from_port set to a non-zero value.
 func expandIPPerms(
 	group *ec2.SecurityGroup, configured []interface{}) ([]*ec2.IpPermission, error) {
-	vpc := group.VpcId != nil
+	vpc := group.VpcId != nil && *group.VpcId != ""
 
 	perms := make([]*ec2.IpPermission, len(configured))
 	for i, mRaw := range configured {
@@ -322,10 +322,36 @@ func flattenHealthCheck(check *elb.HealthCheck) []map[string]interface{} {
 }
 
 // Flattens an array of UserSecurityGroups into a []string
-func flattenSecurityGroups(list []*ec2.UserIdGroupPair) []string {
-	result := make([]string, 0, len(list))
+func flattenSecurityGroups(list []*ec2.UserIdGroupPair, ownerId *string) []*ec2.GroupIdentifier {
+	result := make([]*ec2.GroupIdentifier, 0, len(list))
 	for _, g := range list {
-		result = append(result, *g.GroupId)
+		var userId *string
+		if g.UserId != nil && *g.UserId != "" && (ownerId == nil || *ownerId != *g.UserId) {
+			userId = g.UserId
+		}
+
+		vpc := g.GroupName == nil || *g.GroupName == ""
+		var id *string
+		if vpc {
+			id = g.GroupId
+		} else {
+			id = g.GroupName
+		}
+
+		if userId != nil {
+			id = aws.String(*userId + "/" + *id)
+		}
+
+		if vpc {
+			result = append(result, &ec2.GroupIdentifier{
+				GroupId: id,
+			})
+		} else {
+			result = append(result, &ec2.GroupIdentifier{
+				GroupId:   g.GroupId,
+				GroupName: id,
+			})
+		}
 	}
 	return result
 }

builtin/providers/aws/structure_test.go

@@ -82,7 +82,7 @@ func TestExpandIPPerms(t *testing.T) {
 					GroupId: aws.String("sg-22222"),
 				},
 				&ec2.UserIdGroupPair{
-					GroupId: aws.String("sg-22222"),
+					GroupId: aws.String("sg-11111"),
 				},
 			},
 		},
@@ -92,7 +92,7 @@ func TestExpandIPPerms(t *testing.T) {
 			ToPort:     aws.Int64(int64(-1)),
 			UserIdGroupPairs: []*ec2.UserIdGroupPair{
 				&ec2.UserIdGroupPair{
-					UserId: aws.String("foo"),
+					GroupId: aws.String("foo"),
 				},
 			},
 		},
@@ -122,6 +122,29 @@ func TestExpandIPPerms(t *testing.T) {
 			*exp.UserIdGroupPairs[0].UserId)
 	}
 
+	if *exp.UserIdGroupPairs[0].GroupId != *perm.UserIdGroupPairs[0].GroupId {
+		t.Fatalf(
+			"Got:\n\n%#v\n\nExpected:\n\n%#v\n",
+			*perm.UserIdGroupPairs[0].GroupId,
+			*exp.UserIdGroupPairs[0].GroupId)
+	}
+
+	if *exp.UserIdGroupPairs[1].GroupId != *perm.UserIdGroupPairs[1].GroupId {
+		t.Fatalf(
+			"Got:\n\n%#v\n\nExpected:\n\n%#v\n",
+			*perm.UserIdGroupPairs[1].GroupId,
+			*exp.UserIdGroupPairs[1].GroupId)
+	}
+
+	exp = expected[1]
+	perm = perms[1]
+
+	if *exp.UserIdGroupPairs[0].GroupId != *perm.UserIdGroupPairs[0].GroupId {
+		t.Fatalf(
+			"Got:\n\n%#v\n\nExpected:\n\n%#v\n",
+			*perm.UserIdGroupPairs[0].GroupId,
+			*exp.UserIdGroupPairs[0].GroupId)
+	}
 }
 
 func TestExpandIPPerms_NegOneProtocol(t *testing.T) {
@@ -161,7 +184,7 @@ func TestExpandIPPerms_NegOneProtocol(t *testing.T) {
 					GroupId: aws.String("sg-22222"),
 				},
 				&ec2.UserIdGroupPair{
-					GroupId: aws.String("sg-22222"),
+					GroupId: aws.String("sg-11111"),
 				},
 			},
 		},
@@ -256,7 +279,7 @@ func TestExpandIPPerms_nonVPC(t *testing.T) {
 					GroupName: aws.String("sg-22222"),
 				},
 				&ec2.UserIdGroupPair{
-					GroupName: aws.String("sg-22222"),
+					GroupName: aws.String("sg-11111"),
 				},
 			},
 		},
@@ -288,6 +311,30 @@ func TestExpandIPPerms_nonVPC(t *testing.T) {
 			*perm.IpRanges[0].CidrIp,
 			*exp.IpRanges[0].CidrIp)
 	}
+
+	if *exp.UserIdGroupPairs[0].GroupName != *perm.UserIdGroupPairs[0].GroupName {
+		t.Fatalf(
+			"Got:\n\n%#v\n\nExpected:\n\n%#v\n",
+			*perm.UserIdGroupPairs[0].GroupName,
+			*exp.UserIdGroupPairs[0].GroupName)
+	}
+
+	if *exp.UserIdGroupPairs[1].GroupName != *perm.UserIdGroupPairs[1].GroupName {
+		t.Fatalf(
+			"Got:\n\n%#v\n\nExpected:\n\n%#v\n",
+			*perm.UserIdGroupPairs[1].GroupName,
+			*exp.UserIdGroupPairs[1].GroupName)
+	}
+
+	exp = expected[1]
+	perm = perms[1]
+
+	if *exp.UserIdGroupPairs[0].GroupName != *perm.UserIdGroupPairs[0].GroupName {
+		t.Fatalf(
+			"Got:\n\n%#v\n\nExpected:\n\n%#v\n",
+			*perm.UserIdGroupPairs[0].GroupName,
+			*exp.UserIdGroupPairs[0].GroupName)
+	}
 }
 
 func TestExpandListeners(t *testing.T) {

website/source/docs/providers/aws/r/elb.html.markdown

@@ -66,7 +66,8 @@ The following arguments are supported:
 * `name` - (Optional) The name of the ELB. By default generated by terraform.
 * `access_logs` - (Optional) An Access Logs block. Access Logs documented below.
 * `availability_zones` - (Required for an EC2-classic ELB) The AZ's to serve traffic in.
-* `security_groups` - (Optional) A list of security group IDs to assign to the ELB.
+* `security_groups` - (Optional) A list of security group IDs to assign to the ELB. 
+  Only valid if creating an ELB within a VPC
 * `subnets` - (Required for a VPC ELB) A list of subnet IDs to attach to the ELB.
 * `instances` - (Optional) A list of instance ids to place in the ELB pool.
 * `internal` - (Optional) If true, ELB will be an internal ELB.

website/source/docs/providers/aws/r/security_group.html.markdown

@@ -83,26 +83,24 @@ assign a random, unique name
 
 The `ingress` block supports:
 
-* `cidr_blocks` - (Optional) List of CIDR blocks. Cannot be used with `security_groups`.
+* `cidr_blocks` - (Optional) List of CIDR blocks.
 * `from_port` - (Required) The start port.
 * `protocol` - (Required) The protocol. If you select a protocol of
 "-1", you must specify a "from_port" and "to_port" equal to 0.
 * `security_groups` - (Optional) List of security group Group Names if using
     EC2-Classic or the default VPC, or Group IDs if using a non-default VPC.
-    Cannot be used with `cidr_blocks`.
 * `self` - (Optional) If true, the security group itself will be added as
      a source to this ingress rule.
 * `to_port` - (Required) The end range port.
 
 The `egress` block supports:
 
-* `cidr_blocks` - (Optional) List of CIDR blocks. Cannot be used with `security_groups`.
+* `cidr_blocks` - (Optional) List of CIDR blocks.
 * `from_port` - (Required) The start port.
 * `protocol` - (Required) The protocol. If you select a protocol of
 "-1", you must specify a "from_port" and "to_port" equal to 0.
 * `security_groups` - (Optional) List of security group Group Names if using
     EC2-Classic or the default VPC, or Group IDs if using a non-default VPC.
-    Cannot be used with `cidr_blocks`.
 * `self` - (Optional) If true, the security group itself will be added as
      a source to this egress rule.
 * `to_port` - (Required) The end range port.