diff --git a/builtin/providers/aws/resource_aws_security_group.go b/builtin/providers/aws/resource_aws_security_group.go index cff8222e0..67648787d 100644 --- a/builtin/providers/aws/resource_aws_security_group.go +++ b/builtin/providers/aws/resource_aws_security_group.go @@ -439,8 +439,14 @@ func resourceAwsSecurityGroupUpdateRules( os := o.(*schema.Set) ns := n.(*schema.Set) - remove := expandIPPerms(group, os.Difference(ns).List()) - add := expandIPPerms(group, ns.Difference(os).List()) + remove, err := expandIPPerms(group, os.Difference(ns).List()) + if err != nil { + return err + } + add, err := expandIPPerms(group, ns.Difference(os).List()) + if err != nil { + return err + } // TODO: We need to handle partial state better in the in-between // in this update. diff --git a/builtin/providers/aws/resource_aws_security_group_test.go b/builtin/providers/aws/resource_aws_security_group_test.go index eb3f200f2..35f0d55da 100644 --- a/builtin/providers/aws/resource_aws_security_group_test.go +++ b/builtin/providers/aws/resource_aws_security_group_test.go @@ -142,6 +142,47 @@ func TestAccAWSSecurityGroup_vpc(t *testing.T) { }) } +func TestAccAWSSecurityGroup_vpcNegOneIngress(t *testing.T) { + var group ec2.SecurityGroup + + testCheck := func(*terraform.State) error { + if *group.VPCID == "" { + return fmt.Errorf("should have vpc ID") + } + + return nil + } + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSSecurityGroupDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccAWSSecurityGroupConfigVpcNegOneIngress, + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSSecurityGroupExists("aws_security_group.web", &group), + testAccCheckAWSSecurityGroupAttributesNegOneProtocol(&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.956249133.protocol", "-1"), + resource.TestCheckResourceAttr( + "aws_security_group.web", "ingress.956249133.from_port", "0"), + resource.TestCheckResourceAttr( + "aws_security_group.web", "ingress.956249133.to_port", "0"), + resource.TestCheckResourceAttr( + "aws_security_group.web", "ingress.956249133.cidr_blocks.#", "1"), + resource.TestCheckResourceAttr( + "aws_security_group.web", "ingress.956249133.cidr_blocks.0", "10.0.0.0/8"), + testCheck, + ), + }, + }, + }) +} func TestAccAWSSecurityGroup_MultiIngress(t *testing.T) { var group ec2.SecurityGroup @@ -344,6 +385,37 @@ func testAccCheckAWSSecurityGroupAttributes(group *ec2.SecurityGroup) resource.T } } +func testAccCheckAWSSecurityGroupAttributesNegOneProtocol(group *ec2.SecurityGroup) resource.TestCheckFunc { + return func(s *terraform.State) error { + p := &ec2.IPPermission{ + IPProtocol: aws.String("-1"), + IPRanges: []*ec2.IPRange{&ec2.IPRange{CIDRIP: aws.String("10.0.0.0/8")}}, + } + + 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") + } + + // Compare our ingress + if !reflect.DeepEqual(group.IPPermissions[0], p) { + return fmt.Errorf( + "Got:\n\n%#v\n\nExpected:\n\n%#v\n", + group.IPPermissions[0], + p) + } + + return nil + } +} + func TestAccAWSSecurityGroup_tags(t *testing.T) { var group ec2.SecurityGroup @@ -560,6 +632,24 @@ resource "aws_security_group" "web" { } ` +const testAccAWSSecurityGroupConfigVpcNegOneIngress = ` +resource "aws_vpc" "foo" { + cidr_block = "10.1.0.0/16" +} + +resource "aws_security_group" "web" { + name = "terraform_acceptance_test_example" + description = "Used in the terraform acceptance tests" + vpc_id = "${aws_vpc.foo.id}" + + ingress { + protocol = "-1" + from_port = 0 + to_port = 0 + cidr_blocks = ["10.0.0.0/8"] + } +} +` const testAccAWSSecurityGroupConfigMultiIngress = ` resource "aws_security_group" "worker" { name = "terraform_acceptance_test_example_1" diff --git a/builtin/providers/aws/structure.go b/builtin/providers/aws/structure.go index 533780037..269734aa7 100644 --- a/builtin/providers/aws/structure.go +++ b/builtin/providers/aws/structure.go @@ -42,10 +42,12 @@ func expandListeners(configured []interface{}) ([]*elb.Listener, error) { return listeners, nil } -// Takes the result of flatmap.Expand for an array of ingress/egress -// security group rules and returns EC2 API compatible objects +// Takes the result of flatmap.Expand for an array of ingress/egress security +// group rules and returns EC2 API compatible objects. This function will error +// if it finds invalid permissions input, namely a protocol of "-1" with either +// to_port or from_port set to a non-zero value. func expandIPPerms( - group *ec2.SecurityGroup, configured []interface{}) []*ec2.IPPermission { + group *ec2.SecurityGroup, configured []interface{}) ([]*ec2.IPPermission, error) { vpc := group.VPCID != nil perms := make([]*ec2.IPPermission, len(configured)) @@ -57,6 +59,17 @@ func expandIPPerms( perm.ToPort = aws.Long(int64(m["to_port"].(int))) perm.IPProtocol = aws.String(m["protocol"].(string)) + // When protocol is "-1", AWS won't store any ports for the + // rule, but also won't error if the user specifies ports other + // than '0'. Force the user to make a deliberate '0' port + // choice when specifying a "-1" protocol, and tell them about + // AWS's behavior in the error message. + if *perm.IPProtocol == "-1" && (*perm.FromPort != 0 || *perm.ToPort != 0) { + return nil, fmt.Errorf( + "from_port (%d) and to_port (%d) must both be 0 to use the the 'ALL' \"-1\" protocol!", + *perm.FromPort, *perm.ToPort) + } + var groups []string if raw, ok := m["security_groups"]; ok { list := raw.(*schema.Set).List() @@ -102,7 +115,7 @@ func expandIPPerms( perms[i] = &perm } - return perms + return perms, nil } // Takes the result of flatmap.Expand for an array of parameters and diff --git a/builtin/providers/aws/structure_test.go b/builtin/providers/aws/structure_test.go index 6ec80274c..2f45d643c 100644 --- a/builtin/providers/aws/structure_test.go +++ b/builtin/providers/aws/structure_test.go @@ -61,7 +61,10 @@ func TestexpandIPPerms(t *testing.T) { GroupID: aws.String("foo"), VPCID: aws.String("bar"), } - perms := expandIPPerms(group, expanded) + perms, err := expandIPPerms(group, expanded) + if err != nil { + t.Fatalf("error expanding perms: %v", err) + } expected := []ec2.IPPermission{ ec2.IPPermission{ @@ -117,6 +120,100 @@ func TestexpandIPPerms(t *testing.T) { } +func TestExpandIPPerms_NegOneProtocol(t *testing.T) { + hash := func(v interface{}) int { + return hashcode.String(v.(string)) + } + + expanded := []interface{}{ + map[string]interface{}{ + "protocol": "-1", + "from_port": 0, + "to_port": 0, + "cidr_blocks": []interface{}{"0.0.0.0/0"}, + "security_groups": schema.NewSet(hash, []interface{}{ + "sg-11111", + "foo/sg-22222", + }), + }, + } + group := &ec2.SecurityGroup{ + GroupID: aws.String("foo"), + VPCID: aws.String("bar"), + } + + perms, err := expandIPPerms(group, expanded) + if err != nil { + t.Fatalf("error expanding perms: %v", err) + } + + expected := []ec2.IPPermission{ + ec2.IPPermission{ + IPProtocol: aws.String("-1"), + FromPort: aws.Long(int64(0)), + ToPort: aws.Long(int64(0)), + IPRanges: []*ec2.IPRange{&ec2.IPRange{CIDRIP: aws.String("0.0.0.0/0")}}, + UserIDGroupPairs: []*ec2.UserIDGroupPair{ + &ec2.UserIDGroupPair{ + UserID: aws.String("foo"), + GroupID: aws.String("sg-22222"), + }, + &ec2.UserIDGroupPair{ + GroupID: aws.String("sg-22222"), + }, + }, + }, + } + + exp := expected[0] + perm := perms[0] + + if *exp.FromPort != *perm.FromPort { + t.Fatalf( + "Got:\n\n%#v\n\nExpected:\n\n%#v\n", + *perm.FromPort, + *exp.FromPort) + } + + if *exp.IPRanges[0].CIDRIP != *perm.IPRanges[0].CIDRIP { + t.Fatalf( + "Got:\n\n%#v\n\nExpected:\n\n%#v\n", + *perm.IPRanges[0].CIDRIP, + *exp.IPRanges[0].CIDRIP) + } + + if *exp.UserIDGroupPairs[0].UserID != *perm.UserIDGroupPairs[0].UserID { + t.Fatalf( + "Got:\n\n%#v\n\nExpected:\n\n%#v\n", + *perm.UserIDGroupPairs[0].UserID, + *exp.UserIDGroupPairs[0].UserID) + } + + // Now test the error case. This *should* error when either from_port + // or to_port is not zero, but protocal is "-1". + errorCase := []interface{}{ + map[string]interface{}{ + "protocol": "-1", + "from_port": 0, + "to_port": 65535, + "cidr_blocks": []interface{}{"0.0.0.0/0"}, + "security_groups": schema.NewSet(hash, []interface{}{ + "sg-11111", + "foo/sg-22222", + }), + }, + } + securityGroups := &ec2.SecurityGroup{ + GroupID: aws.String("foo"), + VPCID: aws.String("bar"), + } + + _, expandErr := expandIPPerms(securityGroups, errorCase) + if expandErr == nil { + t.Fatal("expandIPPerms should have errored!") + } +} + func TestExpandIPPerms_nonVPC(t *testing.T) { hash := schema.HashString @@ -141,7 +238,10 @@ func TestExpandIPPerms_nonVPC(t *testing.T) { group := &ec2.SecurityGroup{ GroupName: aws.String("foo"), } - perms := expandIPPerms(group, expanded) + perms, err := expandIPPerms(group, expanded) + if err != nil { + t.Fatalf("error expanding perms: %v", err) + } expected := []ec2.IPPermission{ ec2.IPPermission{ 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 ed389fd76..83757be39 100644 --- a/website/source/docs/providers/aws/r/security_group.html.markdown +++ b/website/source/docs/providers/aws/r/security_group.html.markdown @@ -79,7 +79,8 @@ The `ingress` block supports: * `cidr_blocks` - (Optional) List of CIDR blocks. Cannot be used with `security_groups`. * `from_port` - (Required) The start port. -* `protocol` - (Required) The protocol. +* `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`. @@ -91,7 +92,8 @@ The `egress` block supports: * `cidr_blocks` - (Optional) List of CIDR blocks. Cannot be used with `security_groups`. * `from_port` - (Required) The start port. -* `protocol` - (Required) The protocol. +* `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`.