diff --git a/builtin/providers/aws/resource_aws_elb.go b/builtin/providers/aws/resource_aws_elb.go index bbd4cc4ce..ca9cfdf50 100644 --- a/builtin/providers/aws/resource_aws_elb.go +++ b/builtin/providers/aws/resource_aws_elb.go @@ -341,7 +341,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 diff --git a/builtin/providers/aws/resource_aws_security_group.go b/builtin/providers/aws/resource_aws_security_group.go index fba2e1604..c74cf9661 100644 --- a/builtin/providers/aws/resource_aws_security_group.go +++ b/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) } } diff --git a/builtin/providers/aws/resource_aws_security_group_test.go b/builtin/providers/aws/resource_aws_security_group_test.go index 71d3c39ce..af89df87a 100644 --- a/builtin/providers/aws/resource_aws_security_group_test.go +++ b/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("foo"), + 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{ + 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" + } +} +` diff --git a/builtin/providers/aws/structure.go b/builtin/providers/aws/structure.go index f4c7785a3..391829292 100644 --- a/builtin/providers/aws/structure.go +++ b/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 { @@ -321,11 +321,41 @@ func flattenHealthCheck(check *elb.HealthCheck) []map[string]interface{} { return result } -// Flattens an array of UserSecurityGroups into a []string -func flattenSecurityGroups(list []*ec2.UserIdGroupPair) []string { - result := make([]string, 0, len(list)) +// Flattens an array of UserSecurityGroups into a []*ec2.GroupIdentifier +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 + } + // userid nil here for same vpc groups + + vpc := g.GroupName == nil || *g.GroupName == "" + var id *string + if vpc { + id = g.GroupId + } else { + id = g.GroupName + } + + // id is groupid for vpcs + // id is groupname for non vpc (classic) + + 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 } diff --git a/builtin/providers/aws/structure_test.go b/builtin/providers/aws/structure_test.go index 898d93ce7..74e7ca206 100644 --- a/builtin/providers/aws/structure_test.go +++ b/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) { @@ -722,3 +769,85 @@ func TestFlattenAsgEnabledMetrics(t *testing.T) { t.Fatalf("expected id to be GroupMaxSize, but was %s", result[1]) } } + +func TestFlattenSecurityGroups(t *testing.T) { + cases := []struct { + ownerId *string + pairs []*ec2.UserIdGroupPair + expected []*ec2.GroupIdentifier + }{ + // simple, no user id included (we ignore it mostly) + { + ownerId: aws.String("user1234"), + pairs: []*ec2.UserIdGroupPair{ + &ec2.UserIdGroupPair{ + GroupId: aws.String("sg-12345"), + }, + }, + expected: []*ec2.GroupIdentifier{ + &ec2.GroupIdentifier{ + GroupId: aws.String("sg-12345"), + }, + }, + }, + // include the owner id, but keep it consitent with the same account. Tests + // EC2 classic situation + { + ownerId: aws.String("user1234"), + pairs: []*ec2.UserIdGroupPair{ + &ec2.UserIdGroupPair{ + GroupId: aws.String("sg-12345"), + UserId: aws.String("user1234"), + }, + }, + expected: []*ec2.GroupIdentifier{ + &ec2.GroupIdentifier{ + GroupId: aws.String("sg-12345"), + }, + }, + }, + + // include the owner id, but from a different account. This is reflects + // EC2 Classic when refering to groups by name + { + ownerId: aws.String("user1234"), + pairs: []*ec2.UserIdGroupPair{ + &ec2.UserIdGroupPair{ + GroupId: aws.String("sg-12345"), + GroupName: aws.String("somegroup"), // GroupName is only included in Classic + UserId: aws.String("user4321"), + }, + }, + expected: []*ec2.GroupIdentifier{ + &ec2.GroupIdentifier{ + GroupId: aws.String("sg-12345"), + GroupName: aws.String("user4321/somegroup"), + }, + }, + }, + + // include the owner id, but from a different account. This reflects in + // EC2 VPC when refering to groups by id + { + ownerId: aws.String("user1234"), + pairs: []*ec2.UserIdGroupPair{ + &ec2.UserIdGroupPair{ + GroupId: aws.String("sg-12345"), + UserId: aws.String("user4321"), + }, + }, + expected: []*ec2.GroupIdentifier{ + &ec2.GroupIdentifier{ + GroupId: aws.String("user4321/sg-12345"), + }, + }, + }, + } + + for _, c := range cases { + out := flattenSecurityGroups(c.pairs, c.ownerId) + if !reflect.DeepEqual(out, c.expected) { + t.Fatalf("Error matching output and expected: %#v vs %#v", out, c.expected) + } + } +} diff --git a/website/source/docs/providers/aws/r/elb.html.markdown b/website/source/docs/providers/aws/r/elb.html.markdown index 997d7274d..a127bb006 100644 --- a/website/source/docs/providers/aws/r/elb.html.markdown +++ b/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. diff --git a/website/source/docs/providers/aws/r/security_group.html.markdown b/website/source/docs/providers/aws/r/security_group.html.markdown index b054cc1e7..c34c689b1 100644 --- a/website/source/docs/providers/aws/r/security_group.html.markdown +++ b/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.