diff --git a/builtin/providers/aws/resource_aws_security_group_rule.go b/builtin/providers/aws/resource_aws_security_group_rule.go index 97b6d4025..bd40c284f 100644 --- a/builtin/providers/aws/resource_aws_security_group_rule.go +++ b/builtin/providers/aws/resource_aws_security_group_rule.go @@ -20,7 +20,7 @@ func resourceAwsSecurityGroupRule() *schema.Resource { Read: resourceAwsSecurityGroupRuleRead, Delete: resourceAwsSecurityGroupRuleDelete, - SchemaVersion: 1, + SchemaVersion: 2, MigrateState: resourceAwsSecurityGroupRuleMigrateState, Schema: map[string]*schema.Schema{ @@ -67,14 +67,15 @@ func resourceAwsSecurityGroupRule() *schema.Resource { Optional: true, ForceNew: true, Computed: true, - ConflictsWith: []string{"cidr_blocks"}, + ConflictsWith: []string{"cidr_blocks", "self"}, }, "self": &schema.Schema{ - Type: schema.TypeBool, - Optional: true, - Default: false, - ForceNew: true, + Type: schema.TypeBool, + Optional: true, + Default: false, + ForceNew: true, + ConflictsWith: []string{"cidr_blocks"}, }, }, } @@ -142,7 +143,7 @@ information and instructions for recovery. Error message: %s`, awsErr.Message()) ruleType, autherr) } - d.SetId(ipPermissionIDHash(ruleType, perm)) + d.SetId(ipPermissionIDHash(sg_id, ruleType, perm)) return resourceAwsSecurityGroupRuleRead(d, meta) } @@ -158,24 +159,67 @@ func resourceAwsSecurityGroupRuleRead(d *schema.ResourceData, meta interface{}) } var rule *ec2.IpPermission + var rules []*ec2.IpPermission ruleType := d.Get("type").(string) - var rl []*ec2.IpPermission switch ruleType { case "ingress": - rl = sg.IpPermissions + rules = sg.IpPermissions default: - rl = sg.IpPermissionsEgress + rules = sg.IpPermissionsEgress } - for _, r := range rl { - if d.Id() == ipPermissionIDHash(ruleType, r) { - rule = r + p := expandIPPerm(d, sg) + + if len(rules) == 0 { + return fmt.Errorf("No IPPerms") + } + + for _, r := range rules { + if r.ToPort != nil && *p.ToPort != *r.ToPort { + continue + } + + if r.FromPort != nil && *p.FromPort != *r.FromPort { + continue + } + + if r.IpProtocol != nil && *p.IpProtocol != *r.IpProtocol { + continue + } + + remaining := len(p.IpRanges) + for _, ip := range p.IpRanges { + for _, rip := range r.IpRanges { + if *ip.CidrIp == *rip.CidrIp { + remaining-- + } + } + } + + if remaining > 0 { + continue } + + remaining = len(p.UserIdGroupPairs) + for _, ip := range p.UserIdGroupPairs { + for _, rip := range r.UserIdGroupPairs { + if *ip.GroupId == *rip.GroupId { + remaining-- + } + } + } + + if remaining > 0 { + continue + } + + log.Printf("[DEBUG] Found rule for Security Group Rule (%s): %s", d.Id(), r) + rule = r } if rule == nil { - log.Printf("[DEBUG] Unable to find matching %s Security Group Rule for Group %s", - ruleType, sg_id) + log.Printf("[DEBUG] Unable to find matching %s Security Group Rule (%s) for Group %s", + ruleType, d.Id(), sg_id) d.SetId("") return nil } @@ -186,14 +230,14 @@ func resourceAwsSecurityGroupRuleRead(d *schema.ResourceData, meta interface{}) d.Set("type", ruleType) var cb []string - for _, c := range rule.IpRanges { + for _, c := range p.IpRanges { cb = append(cb, *c.CidrIp) } d.Set("cidr_blocks", cb) - if len(rule.UserIdGroupPairs) > 0 { - s := rule.UserIdGroupPairs[0] + if len(p.UserIdGroupPairs) > 0 { + s := p.UserIdGroupPairs[0] d.Set("source_security_group_id", *s.GroupId) } @@ -285,8 +329,9 @@ func (b ByGroupPair) Less(i, j int) bool { panic("mismatched security group rules, may be a terraform bug") } -func ipPermissionIDHash(ruleType string, ip *ec2.IpPermission) string { +func ipPermissionIDHash(sg_id, ruleType string, ip *ec2.IpPermission) string { var buf bytes.Buffer + buf.WriteString(fmt.Sprintf("%s-", sg_id)) if ip.FromPort != nil && *ip.FromPort > 0 { buf.WriteString(fmt.Sprintf("%d-", *ip.FromPort)) } 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 98ecced70..3dd6f5f72 100644 --- a/builtin/providers/aws/resource_aws_security_group_rule_migrate.go +++ b/builtin/providers/aws/resource_aws_security_group_rule_migrate.go @@ -17,6 +17,12 @@ func resourceAwsSecurityGroupRuleMigrateState( case 0: log.Println("[INFO] Found AWS Security Group State v0; migrating to v1") return migrateSGRuleStateV0toV1(is) + case 1: + log.Println("[INFO] Found AWS Security Group State v1; migrating to v2") + // migrating to version 2 of the schema is the same as 0->1, since the + // method signature has changed now and will use the security group id in + // the hash + return migrateSGRuleStateV0toV1(is) default: return is, fmt.Errorf("Unexpected schema version: %d", v) } @@ -37,7 +43,7 @@ func migrateSGRuleStateV0toV1(is *terraform.InstanceState) (*terraform.InstanceS } log.Printf("[DEBUG] Attributes before migration: %#v", is.Attributes) - newID := ipPermissionIDHash(is.Attributes["type"], perm) + newID := ipPermissionIDHash(is.Attributes["security_group_id"], is.Attributes["type"], perm) is.Attributes["id"] = newID is.ID = newID log.Printf("[DEBUG] Attributes after migration: %#v, new id: %s", is.Attributes, newID) 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 c160703f3..a00385ba7 100644 --- a/builtin/providers/aws/resource_aws_security_group_rule_test.go +++ b/builtin/providers/aws/resource_aws_security_group_rule_test.go @@ -2,7 +2,7 @@ package aws import ( "fmt" - "reflect" + "log" "testing" "github.com/aws/aws-sdk-go/aws" @@ -90,15 +90,15 @@ func TestIpPermissionIDHash(t *testing.T) { Type string Output string }{ - {simple, "ingress", "sg-82613597"}, - {egress, "egress", "sg-363054720"}, - {egress_all, "egress", "sg-2766285362"}, - {vpc_security_group_source, "egress", "sg-2661404947"}, - {security_group_source, "egress", "sg-1841245863"}, + {simple, "ingress", "sg-3403497314"}, + {egress, "egress", "sg-1173186295"}, + {egress_all, "egress", "sg-766323498"}, + {vpc_security_group_source, "egress", "sg-351225364"}, + {security_group_source, "egress", "sg-2198807188"}, } for _, tc := range cases { - actual := ipPermissionIDHash(tc.Type, tc.Input) + actual := ipPermissionIDHash("sg-12345", tc.Type, tc.Input) if actual != tc.Output { t.Errorf("input: %s - %s\noutput: %s", tc.Type, tc.Input, actual) } @@ -132,7 +132,7 @@ func TestAccAWSSecurityGroupRule_Ingress_VPC(t *testing.T) { Config: testAccAWSSecurityGroupRuleIngressConfig, Check: resource.ComposeTestCheckFunc( testAccCheckAWSSecurityGroupRuleExists("aws_security_group.web", &group), - testAccCheckAWSSecurityGroupRuleAttributes(&group, "ingress"), + testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.ingress_1", &group, nil, "ingress"), resource.TestCheckResourceAttr( "aws_security_group_rule.ingress_1", "from_port", "80"), testRuleCount, @@ -169,7 +169,7 @@ func TestAccAWSSecurityGroupRule_Ingress_Classic(t *testing.T) { Config: testAccAWSSecurityGroupRuleIngressClassicConfig, Check: resource.ComposeTestCheckFunc( testAccCheckAWSSecurityGroupRuleExists("aws_security_group.web", &group), - testAccCheckAWSSecurityGroupRuleAttributes(&group, "ingress"), + testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.ingress_1", &group, nil, "ingress"), resource.TestCheckResourceAttr( "aws_security_group_rule.ingress_1", "from_port", "80"), testRuleCount, @@ -231,7 +231,7 @@ func TestAccAWSSecurityGroupRule_Egress(t *testing.T) { Config: testAccAWSSecurityGroupRuleEgressConfig, Check: resource.ComposeTestCheckFunc( testAccCheckAWSSecurityGroupRuleExists("aws_security_group.web", &group), - testAccCheckAWSSecurityGroupRuleAttributes(&group, "egress"), + testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.egress_1", &group, nil, "egress"), ), }, }, @@ -256,6 +256,92 @@ func TestAccAWSSecurityGroupRule_SelfReference(t *testing.T) { }) } +// testing partial match implementation +func TestAccAWSSecurityGroupRule_PartialMatching_Basic(t *testing.T) { + var group ec2.SecurityGroup + + p := ec2.IpPermission{ + FromPort: aws.Int64(80), + ToPort: aws.Int64(80), + IpProtocol: aws.String("tcp"), + IpRanges: []*ec2.IpRange{ + &ec2.IpRange{CidrIp: aws.String("10.0.2.0/24")}, + &ec2.IpRange{CidrIp: aws.String("10.0.3.0/24")}, + &ec2.IpRange{CidrIp: aws.String("10.0.4.0/24")}, + }, + } + + o := ec2.IpPermission{ + FromPort: aws.Int64(80), + ToPort: aws.Int64(80), + IpProtocol: aws.String("tcp"), + IpRanges: []*ec2.IpRange{ + &ec2.IpRange{CidrIp: aws.String("10.0.5.0/24")}, + }, + } + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSSecurityGroupRuleDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccAWSSecurityGroupRulePartialMatching, + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSSecurityGroupRuleExists("aws_security_group.web", &group), + testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.ingress", &group, &p, "ingress"), + testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.other", &group, &o, "ingress"), + testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.nat_ingress", &group, &o, "ingress"), + ), + }, + }, + }) +} + +func TestAccAWSSecurityGroupRule_PartialMatching_Source(t *testing.T) { + var group ec2.SecurityGroup + var nat ec2.SecurityGroup + var p ec2.IpPermission + + // This function creates the expected IPPermission with the group id from an + // external security group, needed because Security Group IDs are generated on + // AWS side and can't be known ahead of time. + setupSG := func(*terraform.State) error { + if nat.GroupId == nil { + return fmt.Errorf("Error: nat group has nil GroupID") + } + + p = ec2.IpPermission{ + FromPort: aws.Int64(80), + ToPort: aws.Int64(80), + IpProtocol: aws.String("tcp"), + UserIdGroupPairs: []*ec2.UserIdGroupPair{ + &ec2.UserIdGroupPair{GroupId: nat.GroupId}, + }, + } + + return nil + } + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSSecurityGroupRuleDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccAWSSecurityGroupRulePartialMatching_Source, + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSSecurityGroupRuleExists("aws_security_group.web", &group), + testAccCheckAWSSecurityGroupRuleExists("aws_security_group.nat", &nat), + setupSG, + testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.source_ingress", &group, &p, "ingress"), + ), + }, + }, + }) + +} + func testAccCheckAWSSecurityGroupRuleDestroy(s *terraform.State) error { conn := testAccProvider.Meta().(*AWSClient).ec2conn @@ -319,14 +405,27 @@ func testAccCheckAWSSecurityGroupRuleExists(n string, group *ec2.SecurityGroup) } } -func testAccCheckAWSSecurityGroupRuleAttributes(group *ec2.SecurityGroup, ruleType string) resource.TestCheckFunc { +func testAccCheckAWSSecurityGroupRuleAttributes(n string, group *ec2.SecurityGroup, p *ec2.IpPermission, ruleType string) resource.TestCheckFunc { return func(s *terraform.State) error { - p := &ec2.IpPermission{ - FromPort: aws.Int64(80), - ToPort: aws.Int64(8000), - IpProtocol: aws.String("tcp"), - IpRanges: []*ec2.IpRange{&ec2.IpRange{CidrIp: aws.String("10.0.0.0/8")}}, + rs, ok := s.RootModule().Resources[n] + if !ok { + return fmt.Errorf("Security Group Rule Not found: %s", n) + } + + if rs.Primary.ID == "" { + return fmt.Errorf("No Security Group Rule is set") } + + if p == nil { + p = &ec2.IpPermission{ + FromPort: aws.Int64(80), + ToPort: aws.Int64(8000), + IpProtocol: aws.String("tcp"), + IpRanges: []*ec2.IpRange{&ec2.IpRange{CidrIp: aws.String("10.0.0.0/8")}}, + } + } + + var matchingRule *ec2.IpPermission var rules []*ec2.IpPermission if ruleType == "ingress" { rules = group.IpPermissions @@ -338,15 +437,53 @@ func testAccCheckAWSSecurityGroupRuleAttributes(group *ec2.SecurityGroup, ruleTy return fmt.Errorf("No IPPerms") } - // Compare our ingress - if !reflect.DeepEqual(rules[0], p) { - return fmt.Errorf( - "Got:\n\n%#v\n\nExpected:\n\n%#v\n", - rules[0], - p) + for _, r := range rules { + if r.ToPort != nil && *p.ToPort != *r.ToPort { + continue + } + + if r.FromPort != nil && *p.FromPort != *r.FromPort { + continue + } + + if r.IpProtocol != nil && *p.IpProtocol != *r.IpProtocol { + continue + } + + remaining := len(p.IpRanges) + for _, ip := range p.IpRanges { + for _, rip := range r.IpRanges { + if *ip.CidrIp == *rip.CidrIp { + remaining-- + } + } + } + + if remaining > 0 { + continue + } + + remaining = len(p.UserIdGroupPairs) + for _, ip := range p.UserIdGroupPairs { + for _, rip := range r.UserIdGroupPairs { + if *ip.GroupId == *rip.GroupId { + remaining-- + } + } + } + + if remaining > 0 { + continue + } + matchingRule = r } - return nil + if matchingRule != nil { + log.Printf("[DEBUG] Matching rule found : %s", matchingRule) + return nil + } + + return fmt.Errorf("Error here\n\tlooking for %s, wasn't found in %s", p, rules) } } @@ -480,3 +617,104 @@ resource "aws_security_group_rule" "self" { security_group_id = "${aws_security_group.web.id}" } ` + +const testAccAWSSecurityGroupRulePartialMatching = ` +resource "aws_vpc" "default" { + cidr_block = "10.0.0.0/16" + tags { + Name = "tf-sg-rule-bug" + } +} + +resource "aws_security_group" "web" { + name = "tf-other" + vpc_id = "${aws_vpc.default.id}" + tags { + Name = "tf-other-sg" + } +} + +resource "aws_security_group" "nat" { + name = "tf-nat" + vpc_id = "${aws_vpc.default.id}" + tags { + Name = "tf-nat-sg" + } +} + +resource "aws_security_group_rule" "ingress" { + type = "ingress" + from_port = 80 + to_port = 80 + protocol = "tcp" + cidr_blocks = ["10.0.2.0/24", "10.0.3.0/24", "10.0.4.0/24"] + + security_group_id = "${aws_security_group.web.id}" +} + +resource "aws_security_group_rule" "other" { + type = "ingress" + from_port = 80 + to_port = 80 + protocol = "tcp" + cidr_blocks = ["10.0.5.0/24"] + + security_group_id = "${aws_security_group.web.id}" +} + +// same a above, but different group, to guard against bad hashing +resource "aws_security_group_rule" "nat_ingress" { + type = "ingress" + from_port = 80 + to_port = 80 + protocol = "tcp" + cidr_blocks = ["10.0.2.0/24", "10.0.3.0/24", "10.0.4.0/24"] + + security_group_id = "${aws_security_group.nat.id}" +} +` + +const testAccAWSSecurityGroupRulePartialMatching_Source = ` +resource "aws_vpc" "default" { + cidr_block = "10.0.0.0/16" + tags { + Name = "tf-sg-rule-bug" + } +} + +resource "aws_security_group" "web" { + name = "tf-other" + vpc_id = "${aws_vpc.default.id}" + tags { + Name = "tf-other-sg" + } +} + +resource "aws_security_group" "nat" { + name = "tf-nat" + vpc_id = "${aws_vpc.default.id}" + tags { + Name = "tf-nat-sg" + } +} + +resource "aws_security_group_rule" "source_ingress" { + type = "ingress" + from_port = 80 + to_port = 80 + protocol = "tcp" + + source_security_group_id = "${aws_security_group.nat.id}" + security_group_id = "${aws_security_group.web.id}" +} + +resource "aws_security_group_rule" "other_ingress" { + type = "ingress" + from_port = 80 + to_port = 80 + protocol = "tcp" + cidr_blocks = ["10.0.2.0/24", "10.0.3.0/24", "10.0.4.0/24"] + + security_group_id = "${aws_security_group.web.id}" +} +`