From 06602d7346c3b579e5e7702743e0a572271ccbab Mon Sep 17 00:00:00 2001 From: Nikolai Zujev Date: Fri, 17 Nov 2017 16:44:20 +0100 Subject: [PATCH] r/security_group_rule: parse description correctly (#1959) * r/security_group_rule: parse description correctly * r/security_group_rule: test multi description * fix: use prefix_list_ids only within egress * fix: detect rule by Ipv6Ranges as well --- aws/resource_aws_security_group_rule.go | 90 ++++++-- aws/resource_aws_security_group_rule_test.go | 221 +++++++++++++++++++ 2 files changed, 291 insertions(+), 20 deletions(-) diff --git a/aws/resource_aws_security_group_rule.go b/aws/resource_aws_security_group_rule.go index d3ec4bc86ba..3bc4bd9a78c 100644 --- a/aws/resource_aws_security_group_rule.go +++ b/aws/resource_aws_security_group_rule.go @@ -281,7 +281,9 @@ func resourceAwsSecurityGroupRuleRead(d *schema.ResourceData, meta interface{}) if err := setFromIPPerm(d, sg, p); err != nil { return errwrap.Wrapf("Error setting IP Permission for Security Group Rule: {{err}}", err) } - setDescriptionFromIPPerm(d, rule) + + d.Set("description", descriptionFromIPPerm(d, rule)) + return nil } @@ -695,38 +697,86 @@ func setFromIPPerm(d *schema.ResourceData, sg *ec2.SecurityGroup, rule *ec2.IpPe return nil } -func setDescriptionFromIPPerm(d *schema.ResourceData, rule *ec2.IpPermission) { - var description string +func descriptionFromIPPerm(d *schema.ResourceData, rule *ec2.IpPermission) string { + // probe IpRanges + cidrIps := make(map[string]bool) + if raw, ok := d.GetOk("cidr_blocks"); ok { + for _, v := range raw.([]interface{}) { + cidrIps[v.(string)] = true + } + } + + if len(cidrIps) > 0 { + for _, c := range rule.IpRanges { + if _, ok := cidrIps[*c.CidrIp]; !ok { + continue + } - for _, c := range rule.IpRanges { - desc := aws.StringValue(c.Description) - if desc != "" { - description = desc + if desc := aws.StringValue(c.Description); desc != "" { + return desc + } } } - for _, ip := range rule.Ipv6Ranges { - desc := aws.StringValue(ip.Description) - if desc != "" { - description = desc + // probe Ipv6Ranges + cidrIpv6s := make(map[string]bool) + if raw, ok := d.GetOk("ipv6_cidr_blocks"); ok { + for _, v := range raw.([]interface{}) { + cidrIpv6s[v.(string)] = true } } - for _, p := range rule.PrefixListIds { - desc := aws.StringValue(p.Description) - if desc != "" { - description = desc + if len(cidrIpv6s) > 0 { + for _, ip := range rule.Ipv6Ranges { + if _, ok := cidrIpv6s[*ip.CidrIpv6]; !ok { + continue + } + + if desc := aws.StringValue(ip.Description); desc != "" { + return desc + } } } - if len(rule.UserIdGroupPairs) > 0 { - desc := aws.StringValue(rule.UserIdGroupPairs[0].Description) - if desc != "" { - description = desc + // probe PrefixListIds + listIds := make(map[string]bool) + if raw, ok := d.GetOk("prefix_list_ids"); ok { + for _, v := range raw.([]interface{}) { + listIds[v.(string)] = true + } + } + + if len(listIds) > 0 { + for _, p := range rule.PrefixListIds { + if _, ok := listIds[*p.PrefixListId]; !ok { + continue + } + + if desc := aws.StringValue(p.Description); desc != "" { + return desc + } + } + } + + // probe UserIdGroupPairs + groupIds := make(map[string]bool) + if raw, ok := d.GetOk("source_security_group_id"); ok { + groupIds[raw.(string)] = true + } + + if len(groupIds) > 0 { + for _, gp := range rule.UserIdGroupPairs { + if _, ok := groupIds[*gp.GroupId]; !ok { + continue + } + + if desc := aws.StringValue(gp.Description); desc != "" { + return desc + } } } - d.Set("description", description) + return "" } // Validates that either 'cidr_blocks', 'ipv6_cidr_blocks', 'self', or 'source_security_group_id' is set diff --git a/aws/resource_aws_security_group_rule_test.go b/aws/resource_aws_security_group_rule_test.go index 82eb446b338..5f5bbd4daad 100644 --- a/aws/resource_aws_security_group_rule_test.go +++ b/aws/resource_aws_security_group_rule_test.go @@ -676,6 +676,139 @@ func TestAccAWSSecurityGroupRule_EgressDescription_updates(t *testing.T) { }) } +func TestAccAWSSecurityGroupRule_MultiDescription(t *testing.T) { + var group ec2.SecurityGroup + var nat ec2.SecurityGroup + rInt := acctest.RandInt() + + rule1 := ec2.IpPermission{ + FromPort: aws.Int64(22), + ToPort: aws.Int64(22), + IpProtocol: aws.String("tcp"), + IpRanges: []*ec2.IpRange{ + {CidrIp: aws.String("0.0.0.0/0"), Description: aws.String("CIDR Description")}, + }, + } + + rule2 := ec2.IpPermission{ + FromPort: aws.Int64(22), + ToPort: aws.Int64(22), + IpProtocol: aws.String("tcp"), + Ipv6Ranges: []*ec2.Ipv6Range{ + {CidrIpv6: aws.String("::/0"), Description: aws.String("IPv6 CIDR Description")}, + }, + } + + var rule3 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") + } + + rule3 = ec2.IpPermission{ + FromPort: aws.Int64(22), + ToPort: aws.Int64(22), + IpProtocol: aws.String("tcp"), + UserIdGroupPairs: []*ec2.UserIdGroupPair{ + {GroupId: nat.GroupId, Description: aws.String("NAT SG Description")}, + }, + } + + return nil + } + + var endpoint ec2.VpcEndpoint + var rule4 ec2.IpPermission + + // This function creates the expected IPPermission with the prefix list ID from + // the VPC Endpoint created in the test + setupPL := func(*terraform.State) error { + conn := testAccProvider.Meta().(*AWSClient).ec2conn + prefixListInput := &ec2.DescribePrefixListsInput{ + Filters: []*ec2.Filter{ + {Name: aws.String("prefix-list-name"), Values: []*string{endpoint.ServiceName}}, + }, + } + + log.Printf("[DEBUG] Reading VPC Endpoint prefix list: %s", prefixListInput) + prefixListsOutput, err := conn.DescribePrefixLists(prefixListInput) + + if err != nil { + _, ok := err.(awserr.Error) + if !ok { + return fmt.Errorf("Error reading VPC Endpoint prefix list: %s", err.Error()) + } + } + + if len(prefixListsOutput.PrefixLists) != 1 { + return fmt.Errorf("There are multiple prefix lists associated with the service name '%s'. Unexpected", prefixListsOutput) + } + + rule4 = ec2.IpPermission{ + FromPort: aws.Int64(22), + ToPort: aws.Int64(22), + IpProtocol: aws.String("tcp"), + PrefixListIds: []*ec2.PrefixListId{ + {PrefixListId: prefixListsOutput.PrefixLists[0].PrefixListId, Description: aws.String("Prefix List Description")}, + }, + } + + return nil + } + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSSecurityGroupRuleDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSSecurityGroupRuleMultiDescription(rInt, "ingress"), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSSecurityGroupRuleExists("aws_security_group.worker", &group), + testAccCheckAWSSecurityGroupRuleExists("aws_security_group.nat", &nat), + testAccCheckVpcEndpointExists("aws_vpc_endpoint.s3-us-west-2", &endpoint), + + testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.ingress_rule_1", &group, &rule1, "ingress"), + resource.TestCheckResourceAttr("aws_security_group_rule.ingress_rule_1", "description", "CIDR Description"), + + testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.ingress_rule_2", &group, &rule2, "ingress"), + resource.TestCheckResourceAttr("aws_security_group_rule.ingress_rule_2", "description", "IPv6 CIDR Description"), + + setupSG, + testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.ingress_rule_3", &group, &rule3, "ingress"), + resource.TestCheckResourceAttr("aws_security_group_rule.ingress_rule_3", "description", "NAT SG Description"), + ), + }, + { + Config: testAccAWSSecurityGroupRuleMultiDescription(rInt, "egress"), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSSecurityGroupRuleExists("aws_security_group.worker", &group), + testAccCheckAWSSecurityGroupRuleExists("aws_security_group.nat", &nat), + testAccCheckVpcEndpointExists("aws_vpc_endpoint.s3-us-west-2", &endpoint), + + testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.egress_rule_1", &group, &rule1, "egress"), + resource.TestCheckResourceAttr("aws_security_group_rule.egress_rule_1", "description", "CIDR Description"), + + testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.egress_rule_2", &group, &rule2, "egress"), + resource.TestCheckResourceAttr("aws_security_group_rule.egress_rule_2", "description", "IPv6 CIDR Description"), + + setupSG, + testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.egress_rule_3", &group, &rule3, "egress"), + resource.TestCheckResourceAttr("aws_security_group_rule.egress_rule_3", "description", "NAT SG Description"), + + setupPL, + testAccCheckAWSSecurityGroupRuleAttributes("aws_security_group_rule.egress_rule_4", &group, &rule4, "egress"), + resource.TestCheckResourceAttr("aws_security_group_rule.egress_rule_4", "description", "Prefix List Description"), + ), + }, + }, + }) +} + func testAccCheckAWSSecurityGroupRuleDestroy(s *terraform.State) error { conn := testAccProvider.Meta().(*AWSClient).ec2conn @@ -797,6 +930,19 @@ func testAccCheckAWSSecurityGroupRuleAttributes(n string, group *ec2.SecurityGro continue } + remaining = len(p.Ipv6Ranges) + for _, ip := range p.Ipv6Ranges { + for _, rip := range r.Ipv6Ranges { + if *ip.CidrIpv6 == *rip.CidrIpv6 { + remaining-- + } + } + } + + if remaining > 0 { + continue + } + remaining = len(p.UserIdGroupPairs) for _, ip := range p.UserIdGroupPairs { for _, rip := range r.UserIdGroupPairs { @@ -1015,6 +1161,81 @@ resource "aws_security_group_rule" "ingress_2" { } ` +func testAccAWSSecurityGroupRuleMultiDescription(rInt int, rType string) string { + var b bytes.Buffer + b.WriteString(fmt.Sprintf(` + resource "aws_vpc" "tf_sgrule_description_test" { + cidr_block = "10.0.0.0/16" + tags { Name = "tf-sg-rule-description" } + } + + resource "aws_vpc_endpoint" "s3-us-west-2" { + vpc_id = "${aws_vpc.tf_sgrule_description_test.id}" + service_name = "com.amazonaws.us-west-2.s3" + } + + resource "aws_security_group" "worker" { + name = "terraform_test_%[1]d" + vpc_id = "${aws_vpc.tf_sgrule_description_test.id}" + description = "Used in the terraform acceptance tests" + tags { Name = "tf-sg-rule-description" } + } + + resource "aws_security_group" "nat" { + name = "terraform_test_%[1]d_nat" + vpc_id = "${aws_vpc.tf_sgrule_description_test.id}" + description = "Used in the terraform acceptance tests" + tags { Name = "tf-sg-rule-description" } + } + + resource "aws_security_group_rule" "%[2]s_rule_1" { + security_group_id = "${aws_security_group.worker.id}" + description = "CIDR Description" + type = "%[2]s" + protocol = "tcp" + from_port = 22 + to_port = 22 + cidr_blocks = ["0.0.0.0/0"] + } + + resource "aws_security_group_rule" "%[2]s_rule_2" { + security_group_id = "${aws_security_group.worker.id}" + description = "IPv6 CIDR Description" + type = "%[2]s" + protocol = "tcp" + from_port = 22 + to_port = 22 + ipv6_cidr_blocks = ["::/0"] + } + + resource "aws_security_group_rule" "%[2]s_rule_3" { + security_group_id = "${aws_security_group.worker.id}" + description = "NAT SG Description" + type = "%[2]s" + protocol = "tcp" + from_port = 22 + to_port = 22 + source_security_group_id = "${aws_security_group.nat.id}" + } + `, rInt, rType)) + + if rType == "egress" { + b.WriteString(fmt.Sprintf(` + resource "aws_security_group_rule" "egress_rule_4" { + security_group_id = "${aws_security_group.worker.id}" + description = "Prefix List Description" + type = "egress" + protocol = "tcp" + from_port = 22 + to_port = 22 + prefix_list_ids = ["${aws_vpc_endpoint.s3-us-west-2.prefix_list_id}"] + } + `)) + } + + return b.String() +} + // check for GH-1985 regression const testAccAWSSecurityGroupRuleConfigSelfReference = ` provider "aws" {