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

@@ -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

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" {