Skip to content

Commit

Permalink
Merge pull request #5533 from hashicorp/pr-5184
Browse files Browse the repository at this point in the history 
provider/aws: Fix EC2 Classic SG Rule issue
  • Loading branch information
@catsby
catsby committed Mar 10, 2016
2 parents 0b9537b + f96ec46 commit 239b3e4
Show file tree
Hide file tree
Showing 7 changed files with 400 additions and 34 deletions.
6 changes: 5 additions & 1 deletion builtin/providers/aws/resource_aws_elb.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
31 changes: 16 additions & 15 deletions builtin/providers/aws/resource_aws_security_group.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand All @@ -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
Expand Down Expand Up @@ -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)
}
}
Expand Down
211 changes: 207 additions & 4 deletions builtin/providers/aws/resource_aws_security_group_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
},
},
},
Expand All @@ -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"),
},
},
},
Expand All @@ -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 {
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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"
}
}
`
40 changes: 35 additions & 5 deletions builtin/providers/aws/structure.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down
Loading

0 comments on commit 239b3e4

Please sign in to comment.