From 7e0a340baf0226ea63691262c67c3cef62a6d6da Mon Sep 17 00:00:00 2001 From: Jesse Szwedko Date: Thu, 11 Jun 2015 14:57:43 -0700 Subject: [PATCH 01/11] Consider security groups with source security groups when hashing Previously they would conflict you had multiple security group rules with the same ingress or egress ports but different source security groups because only the CIDR blocks were considered (which are empty when using source security groups). Updated to include migrations (from clint@ctshryock.com) Signed-off-by: Clint Shryock --- .../aws/resource_aws_security_group_rule.go | 62 +++++++-- ...esource_aws_security_group_rule_migrate.go | 118 ++++++++++++++++++ .../resource_aws_security_group_rule_test.go | 44 ++++++- 3 files changed, 211 insertions(+), 13 deletions(-) create mode 100644 builtin/providers/aws/resource_aws_security_group_rule_migrate.go diff --git a/builtin/providers/aws/resource_aws_security_group_rule.go b/builtin/providers/aws/resource_aws_security_group_rule.go index 270b41ed24dd..45ecd2c6aa17 100644 --- a/builtin/providers/aws/resource_aws_security_group_rule.go +++ b/builtin/providers/aws/resource_aws_security_group_rule.go @@ -21,6 +21,9 @@ func resourceAwsSecurityGroupRule() *schema.Resource { Read: resourceAwsSecurityGroupRuleRead, Delete: resourceAwsSecurityGroupRuleDelete, + SchemaVersion: 1, + MigrateState: resourceAwsSecurityGroupRuleMigrateState, + Schema: map[string]*schema.Schema{ "type": &schema.Schema{ Type: schema.TypeString, @@ -48,10 +51,11 @@ func resourceAwsSecurityGroupRule() *schema.Resource { }, "cidr_blocks": &schema.Schema{ - Type: schema.TypeList, - Optional: true, - ForceNew: true, - Elem: &schema.Schema{Type: schema.TypeString}, + Type: schema.TypeList, + Optional: true, + ForceNew: true, + Elem: &schema.Schema{Type: schema.TypeString}, + ConflictsWith: []string{"source_security_group_id"}, }, "security_group_id": &schema.Schema{ @@ -61,10 +65,11 @@ func resourceAwsSecurityGroupRule() *schema.Resource { }, "source_security_group_id": &schema.Schema{ - Type: schema.TypeString, - Optional: true, - ForceNew: true, - Computed: true, + Type: schema.TypeString, + Optional: true, + ForceNew: true, + Computed: true, + ConflictsWith: []string{"cidr_blocks"}, }, "self": &schema.Schema{ @@ -263,15 +268,32 @@ func findResourceSecurityGroup(conn *ec2.EC2, id string) (*ec2.SecurityGroup, er return resp.SecurityGroups[0], nil } +// byUserIDAndGroup implements sort.Interface for []*ec2.UserIDGroupPairs based on +// UserID and then the GroupID or GroupName field (only one should be set). +type byUserIDAndGroup []*ec2.UserIDGroupPair + +func (b byUserIDAndGroup) Len() int { return len(b) } +func (b byUserIDAndGroup) Swap(i, j int) { b[i], b[j] = b[j], b[i] } +func (b byUserIDAndGroup) Less(i, j int) bool { + if b[i].GroupID != nil && b[j].GroupID != nil { + return *b[i].GroupID < *b[j].GroupID + } + if b[i].GroupName != nil && b[j].GroupName != nil { + return *b[i].GroupName < *b[j].GroupName + } + + panic("mismatched security group rules, may be a terraform bug") +} + func ipPermissionIDHash(ruleType string, ip *ec2.IPPermission) string { var buf bytes.Buffer - // for egress rules, an TCP rule of -1 is automatically added, in which case - // the to and from ports will be nil. We don't record this rule locally. - if ip.IPProtocol != nil && *ip.IPProtocol != "-1" { + if ip.FromPort != nil && *ip.FromPort > 0 { buf.WriteString(fmt.Sprintf("%d-", *ip.FromPort)) + } + if ip.ToPort != nil && *ip.ToPort > 0 { buf.WriteString(fmt.Sprintf("%d-", *ip.ToPort)) - buf.WriteString(fmt.Sprintf("%s-", *ip.IPProtocol)) } + buf.WriteString(fmt.Sprintf("%s-", *ip.IPProtocol)) buf.WriteString(fmt.Sprintf("%s-", ruleType)) // We need to make sure to sort the strings below so that we always @@ -288,6 +310,22 @@ func ipPermissionIDHash(ruleType string, ip *ec2.IPPermission) string { } } + if len(ip.UserIDGroupPairs) > 0 { + sort.Sort(byUserIDAndGroup(ip.UserIDGroupPairs)) + for _, pair := range ip.UserIDGroupPairs { + if pair.GroupID != nil { + buf.WriteString(fmt.Sprintf("%s-", *pair.GroupID)) + } else { + buf.WriteString("-") + } + if pair.GroupName != nil { + buf.WriteString(fmt.Sprintf("%s-", *pair.GroupName)) + } else { + buf.WriteString("-") + } + } + } + return fmt.Sprintf("sg-%d", hashcode.String(buf.String())) } diff --git a/builtin/providers/aws/resource_aws_security_group_rule_migrate.go b/builtin/providers/aws/resource_aws_security_group_rule_migrate.go new file mode 100644 index 000000000000..012bffb0d2e3 --- /dev/null +++ b/builtin/providers/aws/resource_aws_security_group_rule_migrate.go @@ -0,0 +1,118 @@ +package aws + +import ( + "fmt" + "log" + "strconv" + "strings" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/hashicorp/terraform/terraform" +) + +func resourceAwsSecurityGroupRuleMigrateState( + v int, is *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) { + switch v { + case 0: + log.Println("[INFO] Found AWS Security Group State v0; migrating to v1") + return migrateSGRuleStateV0toV1(is, meta) + default: + return is, fmt.Errorf("Unexpected schema version: %d", v) + } + + return is, nil +} + +func migrateSGRuleStateV0toV1(is *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) { + if is.Empty() { + log.Println("[DEBUG] Empty InstanceState; nothing to migrate.") + return is, nil + } + + conn := meta.(*AWSClient).ec2conn + sg_id := is.Attributes["security_group_id"] + sg, err := findResourceSecurityGroup(conn, sg_id) + if err != nil { + return nil, fmt.Errorf("[WARN] Error finding security group for Security Group migration") + } + + perm, err := migrateExpandIPPerm(is.Attributes, sg) + + if err != nil { + return nil, fmt.Errorf("[WARN] Error making new IP Permission in Security Group migration") + } + + log.Printf("[DEBUG] Attributes before migration: %#v", is.Attributes) + newID := ipPermissionIDHash(is.Attributes["type"], perm) + is.Attributes["id"] = newID + is.ID = newID + log.Printf("[DEBUG] Attributes after migration: %#v, new id: %s", is.Attributes, newID) + return is, nil +} + +func migrateExpandIPPerm(attrs map[string]string, sg *ec2.SecurityGroup) (*ec2.IPPermission, error) { + var perm ec2.IPPermission + tp, err := strconv.Atoi(attrs["to_port"]) + if err != nil { + return nil, fmt.Errorf("Error converting to_port in Security Group migration") + } + + fp, err := strconv.Atoi(attrs["from_port"]) + if err != nil { + return nil, fmt.Errorf("Error converting from_port in Security Group migration") + } + + perm.ToPort = aws.Long(int64(tp)) + perm.FromPort = aws.Long(int64(fp)) + perm.IPProtocol = aws.String(attrs["protocol"]) + + groups := make(map[string]bool) + if attrs["self"] == "true" { + if sg.VPCID != nil && *sg.VPCID != "" { + groups[*sg.GroupID] = true + } else { + groups[*sg.GroupName] = true + } + } + + if attrs["source_security_group_id"] != "" { + groups[attrs["source_security_group_id"]] = true + } + + if len(groups) > 0 { + perm.UserIDGroupPairs = make([]*ec2.UserIDGroupPair, len(groups)) + // build string list of group name/ids + var gl []string + for k, _ := range groups { + gl = append(gl, k) + } + + for i, name := range gl { + perm.UserIDGroupPairs[i] = &ec2.UserIDGroupPair{ + GroupID: aws.String(name), + } + + if sg.VPCID == nil || *sg.VPCID == "" { + perm.UserIDGroupPairs[i].GroupID = nil + perm.UserIDGroupPairs[i].GroupName = aws.String(name) + perm.UserIDGroupPairs[i].UserID = nil + } + } + } + + var cb []string + for k, v := range attrs { + if k != "cidr_blocks.#" && strings.HasPrefix(k, "cidr_blocks") { + cb = append(cb, v) + } + } + if len(cb) > 0 { + perm.IPRanges = make([]*ec2.IPRange, len(cb)) + for i, v := range cb { + perm.IPRanges[i] = &ec2.IPRange{CIDRIP: aws.String(v)} + } + } + + return &perm, nil +} diff --git a/builtin/providers/aws/resource_aws_security_group_rule_test.go b/builtin/providers/aws/resource_aws_security_group_rule_test.go index f322ba44eaf4..2b99b0c159f5 100644 --- a/builtin/providers/aws/resource_aws_security_group_rule_test.go +++ b/builtin/providers/aws/resource_aws_security_group_rule_test.go @@ -44,6 +44,46 @@ func TestIpPermissionIDHash(t *testing.T) { }, } + vpc_security_group_source := &ec2.IPPermission{ + IPProtocol: aws.String("tcp"), + FromPort: aws.Long(int64(80)), + ToPort: aws.Long(int64(8000)), + UserIDGroupPairs: []*ec2.UserIDGroupPair{ + &ec2.UserIDGroupPair{ + UserID: aws.String("987654321"), + GroupID: aws.String("sg-12345678"), + }, + &ec2.UserIDGroupPair{ + UserID: aws.String("123456789"), + GroupID: aws.String("sg-987654321"), + }, + &ec2.UserIDGroupPair{ + UserID: aws.String("123456789"), + GroupID: aws.String("sg-12345678"), + }, + }, + } + + security_group_source := &ec2.IPPermission{ + IPProtocol: aws.String("tcp"), + FromPort: aws.Long(int64(80)), + ToPort: aws.Long(int64(8000)), + UserIDGroupPairs: []*ec2.UserIDGroupPair{ + &ec2.UserIDGroupPair{ + UserID: aws.String("987654321"), + GroupName: aws.String("my-security-group"), + }, + &ec2.UserIDGroupPair{ + UserID: aws.String("123456789"), + GroupName: aws.String("my-security-group"), + }, + &ec2.UserIDGroupPair{ + UserID: aws.String("123456789"), + GroupName: aws.String("my-other-security-group"), + }, + }, + } + // hardcoded hashes, to detect future change cases := []struct { Input *ec2.IPPermission @@ -53,12 +93,14 @@ func TestIpPermissionIDHash(t *testing.T) { {simple, "ingress", "sg-82613597"}, {egress, "egress", "sg-363054720"}, {egress_all, "egress", "sg-857124156"}, + {vpc_security_group_source, "egress", "sg-1900174468"}, + {security_group_source, "egress", "sg-1409919872"}, } for _, tc := range cases { actual := ipPermissionIDHash(tc.Type, tc.Input) if actual != tc.Output { - t.Fatalf("input: %s - %#v\noutput: %s", tc.Type, tc.Input, actual) + t.Errorf("input: %s - %#v\noutput: %s", tc.Type, tc.Input, actual) } } } From 8a21bd23eaa97116df4bd29e594ebf49931b97e2 Mon Sep 17 00:00:00 2001 From: Clint Shryock Date: Tue, 16 Jun 2015 15:09:33 -0500 Subject: [PATCH 02/11] fix existing tests --- .../aws/resource_aws_security_group_rule_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/builtin/providers/aws/resource_aws_security_group_rule_test.go b/builtin/providers/aws/resource_aws_security_group_rule_test.go index 2b99b0c159f5..f7f5769e9b8a 100644 --- a/builtin/providers/aws/resource_aws_security_group_rule_test.go +++ b/builtin/providers/aws/resource_aws_security_group_rule_test.go @@ -7,6 +7,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" + "github.com/aws/aws-sdk-go/aws/awsutil" "github.com/aws/aws-sdk-go/service/ec2" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/terraform" @@ -92,15 +93,15 @@ func TestIpPermissionIDHash(t *testing.T) { }{ {simple, "ingress", "sg-82613597"}, {egress, "egress", "sg-363054720"}, - {egress_all, "egress", "sg-857124156"}, - {vpc_security_group_source, "egress", "sg-1900174468"}, - {security_group_source, "egress", "sg-1409919872"}, + {egress_all, "egress", "sg-2766285362"}, + {vpc_security_group_source, "egress", "sg-2661404947"}, + {security_group_source, "egress", "sg-1841245863"}, } for _, tc := range cases { actual := ipPermissionIDHash(tc.Type, tc.Input) if actual != tc.Output { - t.Errorf("input: %s - %#v\noutput: %s", tc.Type, tc.Input, actual) + t.Errorf("input: %s - %s\noutput: %s", tc.Type, awsutil.StringValue(tc.Input), actual) } } } From b25fb8a55d89b2452c7f08cfef58ad0ffacb8eda Mon Sep 17 00:00:00 2001 From: Clint Shryock Date: Tue, 16 Jun 2015 15:54:27 -0500 Subject: [PATCH 03/11] remove meta usage, stub test --- ...esource_aws_security_group_rule_migrate.go | 27 ++-------- ...ce_aws_security_group_rule_migrate_test.go | 50 +++++++++++++++++++ 2 files changed, 55 insertions(+), 22 deletions(-) create mode 100644 builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go diff --git a/builtin/providers/aws/resource_aws_security_group_rule_migrate.go b/builtin/providers/aws/resource_aws_security_group_rule_migrate.go index 012bffb0d2e3..30eb8c09f054 100644 --- a/builtin/providers/aws/resource_aws_security_group_rule_migrate.go +++ b/builtin/providers/aws/resource_aws_security_group_rule_migrate.go @@ -16,7 +16,7 @@ func resourceAwsSecurityGroupRuleMigrateState( switch v { case 0: log.Println("[INFO] Found AWS Security Group State v0; migrating to v1") - return migrateSGRuleStateV0toV1(is, meta) + return migrateSGRuleStateV0toV1(is) default: return is, fmt.Errorf("Unexpected schema version: %d", v) } @@ -24,20 +24,13 @@ func resourceAwsSecurityGroupRuleMigrateState( return is, nil } -func migrateSGRuleStateV0toV1(is *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) { +func migrateSGRuleStateV0toV1(is *terraform.InstanceState) (*terraform.InstanceState, error) { if is.Empty() { log.Println("[DEBUG] Empty InstanceState; nothing to migrate.") return is, nil } - conn := meta.(*AWSClient).ec2conn - sg_id := is.Attributes["security_group_id"] - sg, err := findResourceSecurityGroup(conn, sg_id) - if err != nil { - return nil, fmt.Errorf("[WARN] Error finding security group for Security Group migration") - } - - perm, err := migrateExpandIPPerm(is.Attributes, sg) + perm, err := migrateExpandIPPerm(is.Attributes) if err != nil { return nil, fmt.Errorf("[WARN] Error making new IP Permission in Security Group migration") @@ -51,7 +44,7 @@ func migrateSGRuleStateV0toV1(is *terraform.InstanceState, meta interface{}) (*t return is, nil } -func migrateExpandIPPerm(attrs map[string]string, sg *ec2.SecurityGroup) (*ec2.IPPermission, error) { +func migrateExpandIPPerm(attrs map[string]string) (*ec2.IPPermission, error) { var perm ec2.IPPermission tp, err := strconv.Atoi(attrs["to_port"]) if err != nil { @@ -69,11 +62,7 @@ func migrateExpandIPPerm(attrs map[string]string, sg *ec2.SecurityGroup) (*ec2.I groups := make(map[string]bool) if attrs["self"] == "true" { - if sg.VPCID != nil && *sg.VPCID != "" { - groups[*sg.GroupID] = true - } else { - groups[*sg.GroupName] = true - } + groups[attrs["security_group_id"]] = true } if attrs["source_security_group_id"] != "" { @@ -92,12 +81,6 @@ func migrateExpandIPPerm(attrs map[string]string, sg *ec2.SecurityGroup) (*ec2.I perm.UserIDGroupPairs[i] = &ec2.UserIDGroupPair{ GroupID: aws.String(name), } - - if sg.VPCID == nil || *sg.VPCID == "" { - perm.UserIDGroupPairs[i].GroupID = nil - perm.UserIDGroupPairs[i].GroupName = aws.String(name) - perm.UserIDGroupPairs[i].UserID = nil - } } } diff --git a/builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go b/builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go new file mode 100644 index 000000000000..b9ff41b91d12 --- /dev/null +++ b/builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go @@ -0,0 +1,50 @@ +package aws + +import ( + "testing" + + "github.com/hashicorp/terraform/terraform" +) + +func TestAWSSecurityGroupRuleMigrateState(t *testing.T) { + cases := map[string]struct { + StateVersion int + Attributes map[string]string + Expected string + Meta interface{} + }{ + "v0_1": { + StateVersion: 0, + Attributes: map[string]string{ + // EBS + "self": "true", + }, + Expected: "sg-1234", + }, + "v0_2": { + StateVersion: 0, + Attributes: map[string]string{ + // EBS + "self": "false", + }, + Expected: "sg-1235", + }, + } + + for tn, tc := range cases { + is := &terraform.InstanceState{ + ID: "sg-12333", + Attributes: tc.Attributes, + } + is, err := resourceAwsSecurityGroupRuleMigrateState( + tc.StateVersion, is, tc.Meta) + + if err != nil { + t.Fatalf("bad: %s, err: %#v", tn, err) + } + + if is.ID != tc.Expected { + t.Fatalf("bad sg rule id: %s\n\n expected: %s", is.ID, tc.Expected) + } + } +} From 2d06c81e4b6a4d907809cf2fb0dffea701a1a889 Mon Sep 17 00:00:00 2001 From: Clint Shryock Date: Tue, 16 Jun 2015 16:15:07 -0500 Subject: [PATCH 04/11] update test --- ...ce_aws_security_group_rule_migrate_test.go | 36 ++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go b/builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go index b9ff41b91d12..960cceaa493d 100644 --- a/builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go +++ b/builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go @@ -7,6 +7,11 @@ import ( ) func TestAWSSecurityGroupRuleMigrateState(t *testing.T) { + // "id":"sg-4235098228", "from_port":"0", "source_security_group_id":"sg-11877275"} + + // 2015/06/16 16:04:21 terraform-provider-aws: 2015/06/16 16:04:21 [DEBUG] Attributes after migration: + + // map[string]string{"from_port":"0", "source_security_group_id":"sg-11877275", "id":"sg-3766347571", "security_group_id":"sg-13877277", "cidr_blocks.#":"0", "type":"ingress", "protocol":"-1", "self":"false", "to_port":"0"}, new id: sg-3766347571 cases := map[string]struct { StateVersion int Attributes map[string]string @@ -16,24 +21,31 @@ func TestAWSSecurityGroupRuleMigrateState(t *testing.T) { "v0_1": { StateVersion: 0, Attributes: map[string]string{ - // EBS - "self": "true", - }, - Expected: "sg-1234", - }, - "v0_2": { - StateVersion: 0, - Attributes: map[string]string{ - // EBS - "self": "false", + "self": "false", + "to_port": "0", + "security_group_id": "sg-13877277", + "cidr_blocks.#": "0", + "type": "ingress", + "protocol": "-1", + "id": "sg-4235098228", + "from_port": "0", + "source_security_group_id": "sg-11877275", }, - Expected: "sg-1235", + Expected: "sg-3766347571", }, + // "v0_2": { + // StateVersion: 0, + // Attributes: map[string]string{ + // // EBS + // "self": "false", + // }, + // Expected: "sg-1235", + // }, } for tn, tc := range cases { is := &terraform.InstanceState{ - ID: "sg-12333", + ID: "sg-4235098228", Attributes: tc.Attributes, } is, err := resourceAwsSecurityGroupRuleMigrateState( From 3bf89fb81ec70f8a1fd530a01d25e37a2ce64c28 Mon Sep 17 00:00:00 2001 From: Clint Shryock Date: Tue, 16 Jun 2015 16:21:46 -0500 Subject: [PATCH 05/11] update tests with another example --- ...ce_aws_security_group_rule_migrate_test.go | 30 ++++++++++++------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go b/builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go index 960cceaa493d..33783828f55c 100644 --- a/builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go +++ b/builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go @@ -14,12 +14,14 @@ func TestAWSSecurityGroupRuleMigrateState(t *testing.T) { // map[string]string{"from_port":"0", "source_security_group_id":"sg-11877275", "id":"sg-3766347571", "security_group_id":"sg-13877277", "cidr_blocks.#":"0", "type":"ingress", "protocol":"-1", "self":"false", "to_port":"0"}, new id: sg-3766347571 cases := map[string]struct { StateVersion int + ID string Attributes map[string]string Expected string Meta interface{} }{ "v0_1": { StateVersion: 0, + ID: "sg-4235098228", Attributes: map[string]string{ "self": "false", "to_port": "0", @@ -27,25 +29,33 @@ func TestAWSSecurityGroupRuleMigrateState(t *testing.T) { "cidr_blocks.#": "0", "type": "ingress", "protocol": "-1", - "id": "sg-4235098228", "from_port": "0", "source_security_group_id": "sg-11877275", }, Expected: "sg-3766347571", }, - // "v0_2": { - // StateVersion: 0, - // Attributes: map[string]string{ - // // EBS - // "self": "false", - // }, - // Expected: "sg-1235", - // }, + "v0_2": { + StateVersion: 0, + ID: "sg-1021609891", + Attributes: map[string]string{ + "security_group_id": "sg-0981746d", + "from_port": "0", + "to_port": "0", + "type": "ingress", + "self": "false", + "protocol": "-1", + "cidr_blocks.0": "172.16.1.0/24", + "cidr_blocks.1": "172.16.2.0/24", + "cidr_blocks.2": "172.16.3.0/24", + "cidr_blocks.3": "172.16.4.0/24", + "cidr_blocks.#": "4"}, + Expected: "sg-4100229787", + }, } for tn, tc := range cases { is := &terraform.InstanceState{ - ID: "sg-4235098228", + ID: tc.ID, Attributes: tc.Attributes, } is, err := resourceAwsSecurityGroupRuleMigrateState( From c1cdac1f76c65b4e80bf020d3839df348dcdacf0 Mon Sep 17 00:00:00 2001 From: Clint Shryock Date: Tue, 16 Jun 2015 16:30:33 -0500 Subject: [PATCH 06/11] clean up old, incompatible test --- .../providers/aws/resource_aws_security_group_rule_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/builtin/providers/aws/resource_aws_security_group_rule_test.go b/builtin/providers/aws/resource_aws_security_group_rule_test.go index f7f5769e9b8a..595886e184d0 100644 --- a/builtin/providers/aws/resource_aws_security_group_rule_test.go +++ b/builtin/providers/aws/resource_aws_security_group_rule_test.go @@ -184,9 +184,9 @@ func TestAccAWSSecurityGroupRule_MultiIngress(t *testing.T) { var group ec2.SecurityGroup testMultiRuleCount := func(*terraform.State) error { - if len(group.IPPermissions) != 3 { + if len(group.IPPermissions) != 2 { return fmt.Errorf("Wrong Security Group rule count, expected %d, got %d", - 3, len(group.IPPermissions)) + 2, len(group.IPPermissions)) } var rule *ec2.IPPermission @@ -438,7 +438,6 @@ resource "aws_security_group_rule" "ingress_1" { cidr_blocks = ["10.0.0.0/8"] security_group_id = "${aws_security_group.web.id}" - source_security_group_id = "${aws_security_group.worker.id}" } resource "aws_security_group_rule" "ingress_2" { From 359826be263afb64435ac4a1e2094308704d6d83 Mon Sep 17 00:00:00 2001 From: Clint Shryock Date: Tue, 16 Jun 2015 16:38:26 -0500 Subject: [PATCH 07/11] clean up some conflicts with --- .../providers/aws/resource_aws_security_group_rule.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/builtin/providers/aws/resource_aws_security_group_rule.go b/builtin/providers/aws/resource_aws_security_group_rule.go index 45ecd2c6aa17..b12884e59532 100644 --- a/builtin/providers/aws/resource_aws_security_group_rule.go +++ b/builtin/providers/aws/resource_aws_security_group_rule.go @@ -51,11 +51,10 @@ func resourceAwsSecurityGroupRule() *schema.Resource { }, "cidr_blocks": &schema.Schema{ - Type: schema.TypeList, - Optional: true, - ForceNew: true, - Elem: &schema.Schema{Type: schema.TypeString}, - ConflictsWith: []string{"source_security_group_id"}, + Type: schema.TypeList, + Optional: true, + ForceNew: true, + Elem: &schema.Schema{Type: schema.TypeString}, }, "security_group_id": &schema.Schema{ From 640836ee58d9d3e92a75f7c6e27953eb16e2f911 Mon Sep 17 00:00:00 2001 From: Clint Shryock Date: Wed, 17 Jun 2015 09:35:50 -0500 Subject: [PATCH 08/11] rename method, update docs --- .../aws/resource_aws_security_group_rule.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/builtin/providers/aws/resource_aws_security_group_rule.go b/builtin/providers/aws/resource_aws_security_group_rule.go index b12884e59532..aa634bdf1bfa 100644 --- a/builtin/providers/aws/resource_aws_security_group_rule.go +++ b/builtin/providers/aws/resource_aws_security_group_rule.go @@ -267,13 +267,13 @@ func findResourceSecurityGroup(conn *ec2.EC2, id string) (*ec2.SecurityGroup, er return resp.SecurityGroups[0], nil } -// byUserIDAndGroup implements sort.Interface for []*ec2.UserIDGroupPairs based on -// UserID and then the GroupID or GroupName field (only one should be set). -type byUserIDAndGroup []*ec2.UserIDGroupPair +// ByGroupPair implements sort.Interface for []*ec2.UserIDGroupPairs based on +// GroupID or GroupName field (only one should be set). +type ByGroupPair []*ec2.UserIDGroupPair -func (b byUserIDAndGroup) Len() int { return len(b) } -func (b byUserIDAndGroup) Swap(i, j int) { b[i], b[j] = b[j], b[i] } -func (b byUserIDAndGroup) Less(i, j int) bool { +func (b ByGroupPair) Len() int { return len(b) } +func (b ByGroupPair) Swap(i, j int) { b[i], b[j] = b[j], b[i] } +func (b ByGroupPair) Less(i, j int) bool { if b[i].GroupID != nil && b[j].GroupID != nil { return *b[i].GroupID < *b[j].GroupID } @@ -310,7 +310,7 @@ func ipPermissionIDHash(ruleType string, ip *ec2.IPPermission) string { } if len(ip.UserIDGroupPairs) > 0 { - sort.Sort(byUserIDAndGroup(ip.UserIDGroupPairs)) + sort.Sort(ByGroupPair(ip.UserIDGroupPairs)) for _, pair := range ip.UserIDGroupPairs { if pair.GroupID != nil { buf.WriteString(fmt.Sprintf("%s-", *pair.GroupID)) From 24ee2e5d53f67ec8e7d7b59cbe13f5bec61e698e Mon Sep 17 00:00:00 2001 From: Clint Shryock Date: Thu, 18 Jun 2015 08:39:08 -0500 Subject: [PATCH 09/11] remove debugging --- .../aws/resource_aws_security_group_rule_migrate_test.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go b/builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go index 33783828f55c..664f05039387 100644 --- a/builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go +++ b/builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go @@ -7,11 +7,6 @@ import ( ) func TestAWSSecurityGroupRuleMigrateState(t *testing.T) { - // "id":"sg-4235098228", "from_port":"0", "source_security_group_id":"sg-11877275"} - - // 2015/06/16 16:04:21 terraform-provider-aws: 2015/06/16 16:04:21 [DEBUG] Attributes after migration: - - // map[string]string{"from_port":"0", "source_security_group_id":"sg-11877275", "id":"sg-3766347571", "security_group_id":"sg-13877277", "cidr_blocks.#":"0", "type":"ingress", "protocol":"-1", "self":"false", "to_port":"0"}, new id: sg-3766347571 cases := map[string]struct { StateVersion int ID string From 645a5aa55bf7060b86d0e40919803039801688f0 Mon Sep 17 00:00:00 2001 From: Clint Shryock Date: Fri, 19 Jun 2015 11:23:59 -0500 Subject: [PATCH 10/11] add warning message to explain scenario of conflicting rules --- .../aws/resource_aws_security_group_rule.go | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/builtin/providers/aws/resource_aws_security_group_rule.go b/builtin/providers/aws/resource_aws_security_group_rule.go index aa634bdf1bfa..6a99fb84d82d 100644 --- a/builtin/providers/aws/resource_aws_security_group_rule.go +++ b/builtin/providers/aws/resource_aws_security_group_rule.go @@ -94,6 +94,7 @@ func resourceAwsSecurityGroupRuleCreate(d *schema.ResourceData, meta interface{} ruleType := d.Get("type").(string) + var autherr error switch ruleType { case "ingress": log.Printf("[DEBUG] Authorizing security group %s %s rule: %s", @@ -109,13 +110,7 @@ func resourceAwsSecurityGroupRuleCreate(d *schema.ResourceData, meta interface{} req.GroupName = sg.GroupName } - _, err := conn.AuthorizeSecurityGroupIngress(req) - - if err != nil { - return fmt.Errorf( - "Error authorizing security group %s rules: %s", - "rules", err) - } + _, autherr = conn.AuthorizeSecurityGroupIngress(req) case "egress": log.Printf("[DEBUG] Authorizing security group %s %s rule: %#v", @@ -126,18 +121,28 @@ func resourceAwsSecurityGroupRuleCreate(d *schema.ResourceData, meta interface{} IPPermissions: []*ec2.IPPermission{perm}, } - _, err = conn.AuthorizeSecurityGroupEgress(req) - - if err != nil { - return fmt.Errorf( - "Error authorizing security group %s rules: %s", - "rules", err) - } + _, autherr = conn.AuthorizeSecurityGroupEgress(req) default: return fmt.Errorf("Security Group Rule must be type 'ingress' or type 'egress'") } + if autherr != nil { + if awsErr, ok := autherr.(awserr.Error); ok { + if awsErr.Code() == "InvalidPermission.Duplicate" { + return fmt.Errorf(`[WARN] A duplicate Security Group rule was found. This may be +a side effect of a now-fixed Terraform issue causing two security groups with +identical attributes but different source_security_group_ids to overwrite each +other in the state. See https://github.com/hashicorp/teraform/pull/2376 for more +information and instructions for recovery. Error message: %s`, awsErr.Message()) + } + } + + return fmt.Errorf( + "Error authorizing security group rule type %s: %s", + ruleType, autherr) + } + d.SetId(ipPermissionIDHash(ruleType, perm)) return resourceAwsSecurityGroupRuleRead(d, meta) From 44eb55f8f66788b9a2e709848b7cdb3f3d81665d Mon Sep 17 00:00:00 2001 From: Clint Shryock Date: Fri, 19 Jun 2015 11:50:10 -0500 Subject: [PATCH 11/11] update link to actually work --- builtin/providers/aws/resource_aws_security_group_rule.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/providers/aws/resource_aws_security_group_rule.go b/builtin/providers/aws/resource_aws_security_group_rule.go index 6a99fb84d82d..ce8b20498aaf 100644 --- a/builtin/providers/aws/resource_aws_security_group_rule.go +++ b/builtin/providers/aws/resource_aws_security_group_rule.go @@ -133,7 +133,7 @@ func resourceAwsSecurityGroupRuleCreate(d *schema.ResourceData, meta interface{} return fmt.Errorf(`[WARN] A duplicate Security Group rule was found. This may be a side effect of a now-fixed Terraform issue causing two security groups with identical attributes but different source_security_group_ids to overwrite each -other in the state. See https://github.com/hashicorp/teraform/pull/2376 for more +other in the state. See https://github.com/hashicorp/terraform/pull/2376 for more information and instructions for recovery. Error message: %s`, awsErr.Message()) } }