Merge remote-tracking branch 'ctiwald/ct/fix-protocol-problem'

* ctiwald/ct/fix-protocol-problem:
  aws: Document the odd protocol = "-1" behavior in security groups.
  aws: Fixup structure_test to handle new expandIPPerms behavior.
  aws: Add security group acceptance tests for protocol -1 fixes.
  aws: error on expndIPPerms(...) if our ports and protocol conflict.

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())
+		remove, err := expandIPPerms(group, os.Difference(ns).List())
-		add := expandIPPerms(group, ns.Difference(os).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.

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"

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
+// Takes the result of flatmap.Expand for an array of ingress/egress security
-// security group rules and returns EC2 API compatible objects
+// 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

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{

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`.