diff --git a/builtin/providers/aws/resource_aws_security_group_rule.go b/builtin/providers/aws/resource_aws_security_group_rule.go index 270b41ed24dd..ce8b20498aaf 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, @@ -61,10 +64,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{ @@ -90,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", @@ -105,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", @@ -122,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/terraform/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) @@ -263,15 +272,32 @@ func findResourceSecurityGroup(conn *ec2.EC2, id string) (*ec2.SecurityGroup, er return resp.SecurityGroups[0], nil } +// ByGroupPair implements sort.Interface for []*ec2.UserIDGroupPairs based on +// GroupID or GroupName field (only one should be set). +type ByGroupPair []*ec2.UserIDGroupPair + +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 + } + 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 +314,22 @@ func ipPermissionIDHash(ruleType string, ip *ec2.IPPermission) string { } } + if len(ip.UserIDGroupPairs) > 0 { + sort.Sort(ByGroupPair(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..30eb8c09f054 --- /dev/null +++ b/builtin/providers/aws/resource_aws_security_group_rule_migrate.go @@ -0,0 +1,101 @@ +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) + default: + return is, fmt.Errorf("Unexpected schema version: %d", v) + } + + return is, nil +} + +func migrateSGRuleStateV0toV1(is *terraform.InstanceState) (*terraform.InstanceState, error) { + if is.Empty() { + log.Println("[DEBUG] Empty InstanceState; nothing to migrate.") + return is, nil + } + + perm, err := migrateExpandIPPerm(is.Attributes) + + 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) (*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" { + groups[attrs["security_group_id"]] = 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), + } + } + } + + 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_migrate_test.go b/builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go new file mode 100644 index 000000000000..664f05039387 --- /dev/null +++ b/builtin/providers/aws/resource_aws_security_group_rule_migrate_test.go @@ -0,0 +1,67 @@ +package aws + +import ( + "testing" + + "github.com/hashicorp/terraform/terraform" +) + +func TestAWSSecurityGroupRuleMigrateState(t *testing.T) { + 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", + "security_group_id": "sg-13877277", + "cidr_blocks.#": "0", + "type": "ingress", + "protocol": "-1", + "from_port": "0", + "source_security_group_id": "sg-11877275", + }, + Expected: "sg-3766347571", + }, + "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: tc.ID, + 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) + } + } +} 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..595886e184d0 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" @@ -44,6 +45,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 @@ -52,13 +93,15 @@ func TestIpPermissionIDHash(t *testing.T) { }{ {simple, "ingress", "sg-82613597"}, {egress, "egress", "sg-363054720"}, - {egress_all, "egress", "sg-857124156"}, + {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.Fatalf("input: %s - %#v\noutput: %s", tc.Type, tc.Input, actual) + t.Errorf("input: %s - %s\noutput: %s", tc.Type, awsutil.StringValue(tc.Input), actual) } } } @@ -141,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 @@ -395,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" {