r/security_group_rule: parse description correctly

[jaymecd]

This PR is aimed to improve description discovery of Security Group Rule (with same protocol & port range) by matching aws_security_group_rule schema properties:

• has cidr_blocks - get description from matched item of IpRanges
• has ipv6_cidr_blocks - get description from matched item of Ipv6Ranges
• has prefix_list_ids - get description from matched item of PrefixListIds
• has source_security_group_id - get description from matched item of UserIdGroupPairs

How to reproduce the issue:

Running TestAccAWSSecurityGroupRule_MultiDescription test on master fails with:

testing.go:434: Step 0 error: After applying this step and refreshing, the plan was not empty:
    DIFF:

    UPDATE: aws_security_group_rule.ingress_rule_1
        description: "NAT SG Description" => "CIDR Description"
    UPDATE: aws_security_group_rule.ingress_rule_2
        description: "NAT SG Description" => "IPv6 CIDR Description"
    UPDATE: aws_security_group_rule.ingress_rule_4
        description: "NAT SG Description" => "Prefix List Description"

Configuration what causes it:

resource "aws_security_group" "test" {
  vpc_id = "${aws_vpc.default.id}"
}

resource "aws_security_group_rule" "ingress_22_target1" {
  security_group_id        = "${aws_security_group.test.id}"
  source_security_group_id = "${aws_security_group.target1.id}"
  description              = "Allow Target One"
  type                     = "ingress"
  protocol                 = "tcp"
  from_port                = "22"
  to_port                  = "22"
}

resource "aws_security_group_rule" "ingress_22_target2" {
  security_group_id = "${aws_security_group.test.id}"
  cidr_blocks       = ["10.0.2.0/24"]
  description       = "Allow Target Two"
  type              = "ingress"
  protocol          = "tcp"
  from_port         = "22"
  to_port           = "22"
}

Initialization goes as expected

$ terraform plan
$ terraform apply

...[clipped]...
aws_security_group_rule.ingress_22_target2: Creation complete after 1s (ID: sgrule-3731331332)
aws_security_group_rule.ingress_22_target1: Creation complete after 2s (ID: sgrule-4216459885)

Run plan again w/o modifications:

$ terraform plan

...[clipped]...
Terraform will perform the following actions:

  ~ aws_security_group_rule.ingress_22_target2
      description: "Allow Target One" => "Allow Target Two"

That's it, it tries to overwrite it, as parsed incorrectly.
Even after apply it would try to make the same

[radeksimko]

Hi @jaymecd
the mentioned test (and all other SG-related tests) do pass for us.

Can you provide some details about your AWS account in which is this test failing? e.g. output from http://docs.aws.amazon.com/cli/latest/reference/ec2/describe-account-attributes.html ?

Thanks.

[bflad]

As another data point, we have this same issue of the descriptions between multiple rules not matching correctly in our accounts (one VPC only, one EC2 classic enabled).

[jaymecd]

@radeksimko sure, but I doubt it's important here:

$ aws --region us-west-2 ec2 describe-account-attributes

{
    "AccountAttributes": [
        {
            "AttributeName": "supported-platforms",
            "AttributeValues": [
                {
                    "AttributeValue": "VPC"
                }
            ]
        },
        {
            "AttributeName": "vpc-max-security-groups-per-interface",
            "AttributeValues": [
                {
                    "AttributeValue": "5"
                }
            ]
        },
        {
            "AttributeName": "max-elastic-ips",
            "AttributeValues": [
                {
                    "AttributeValue": "5"
                }
            ]
        },
        {
            "AttributeName": "max-instances",
            "AttributeValues": [
                {
                    "AttributeValue": "20"
                }
            ]
        },
        {
            "AttributeName": "vpc-max-elastic-ips",
            "AttributeValues": [
                {
                    "AttributeValue": "5"
                }
            ]
        },
        {
            "AttributeName": "default-vpc",
            "AttributeValues": [
                {
                    "AttributeValue": "vpc-24323640"
                }
            ]
        }
    ]
}

As @bflad has mentioned, that happened on any kind of setup.

if SG has several rules of same type, protocol & port, but with different targets (cidr, sg & etc..) - only last rule description is used.
That conflicts with internal state (unique description per rule) and terraform reports difference.

[bflad]

Related: #2069

[jaymecd]

@radeksimko any plans to merge it into upcoming 1.3.0 version? tnx

[radeksimko]

All SG acceptance tests passing, the code looks reasonable, thanks 👍

TF_ACC=1 go test ./aws -v -run=TestAccAWSSecurityGroup -timeout 120m
=== RUN   TestAccAWSSecurityGroup_importBasic
--- PASS: TestAccAWSSecurityGroup_importBasic (87.14s)
=== RUN   TestAccAWSSecurityGroup_importIpv6
--- PASS: TestAccAWSSecurityGroup_importIpv6 (87.60s)
=== RUN   TestAccAWSSecurityGroup_importSelf
--- PASS: TestAccAWSSecurityGroup_importSelf (96.31s)
=== RUN   TestAccAWSSecurityGroup_importSourceSecurityGroup
--- PASS: TestAccAWSSecurityGroup_importSourceSecurityGroup (97.84s)
=== RUN   TestAccAWSSecurityGroup_importIPRangeAndSecurityGroupWithSameRules
--- PASS: TestAccAWSSecurityGroup_importIPRangeAndSecurityGroupWithSameRules (105.28s)
=== RUN   TestAccAWSSecurityGroup_importIPRangesWithSameRules
--- PASS: TestAccAWSSecurityGroup_importIPRangesWithSameRules (99.19s)
=== RUN   TestAccAWSSecurityGroup_importPrefixList
--- PASS: TestAccAWSSecurityGroup_importPrefixList (104.57s)
=== RUN   TestAccAWSSecurityGroupRule_Ingress_VPC
--- PASS: TestAccAWSSecurityGroupRule_Ingress_VPC (50.69s)
=== RUN   TestAccAWSSecurityGroupRule_Ingress_Protocol
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Protocol (90.31s)
=== RUN   TestAccAWSSecurityGroupRule_Ingress_Ipv6
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Ipv6 (91.24s)
=== RUN   TestAccAWSSecurityGroupRule_Ingress_Classic
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Classic (43.58s)
=== RUN   TestAccAWSSecurityGroupRule_MultiIngress
--- PASS: TestAccAWSSecurityGroupRule_MultiIngress (60.87s)
=== RUN   TestAccAWSSecurityGroupRule_Egress
--- PASS: TestAccAWSSecurityGroupRule_Egress (49.15s)
=== RUN   TestAccAWSSecurityGroupRule_SelfReference
--- PASS: TestAccAWSSecurityGroupRule_SelfReference (89.77s)
=== RUN   TestAccAWSSecurityGroupRule_ExpectInvalidTypeError
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidTypeError (3.64s)
=== RUN   TestAccAWSSecurityGroupRule_ExpectInvalidCIDR
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidCIDR (3.29s)
=== RUN   TestAccAWSSecurityGroupRule_PartialMatching_basic
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_basic (96.22s)
=== RUN   TestAccAWSSecurityGroupRule_PartialMatching_Source
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_Source (92.77s)
=== RUN   TestAccAWSSecurityGroupRule_Issue5310
--- PASS: TestAccAWSSecurityGroupRule_Issue5310 (36.30s)
=== RUN   TestAccAWSSecurityGroupRule_Race
--- PASS: TestAccAWSSecurityGroupRule_Race (695.11s)
=== RUN   TestAccAWSSecurityGroupRule_SelfSource
--- PASS: TestAccAWSSecurityGroupRule_SelfSource (86.66s)
=== RUN   TestAccAWSSecurityGroupRule_PrefixListEgress
--- PASS: TestAccAWSSecurityGroupRule_PrefixListEgress (95.84s)
=== RUN   TestAccAWSSecurityGroupRule_IngressDescription
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription (51.62s)
=== RUN   TestAccAWSSecurityGroupRule_EgressDescription
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription (52.16s)
=== RUN   TestAccAWSSecurityGroupRule_IngressDescription_updates
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription_updates (80.55s)
=== RUN   TestAccAWSSecurityGroupRule_EgressDescription_updates
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription_updates (79.57s)
=== RUN   TestAccAWSSecurityGroupRule_MultiDescription
--- PASS: TestAccAWSSecurityGroupRule_MultiDescription (183.84s)
=== RUN   TestAccAWSSecurityGroup_basic
--- PASS: TestAccAWSSecurityGroup_basic (82.81s)
=== RUN   TestAccAWSSecurityGroup_basicRuleDescription
--- PASS: TestAccAWSSecurityGroup_basicRuleDescription (111.51s)
=== RUN   TestAccAWSSecurityGroup_ipv6
--- PASS: TestAccAWSSecurityGroup_ipv6 (84.79s)
=== RUN   TestAccAWSSecurityGroup_tagsCreatedFirst
--- PASS: TestAccAWSSecurityGroup_tagsCreatedFirst (55.22s)
=== RUN   TestAccAWSSecurityGroup_namePrefix
--- PASS: TestAccAWSSecurityGroup_namePrefix (34.17s)
=== RUN   TestAccAWSSecurityGroup_self
--- PASS: TestAccAWSSecurityGroup_self (110.59s)
=== RUN   TestAccAWSSecurityGroup_vpc
--- PASS: TestAccAWSSecurityGroup_vpc (80.82s)
=== RUN   TestAccAWSSecurityGroup_vpcNegOneIngress
--- PASS: TestAccAWSSecurityGroup_vpcNegOneIngress (78.31s)
=== RUN   TestAccAWSSecurityGroup_vpcProtoNumIngress
--- PASS: TestAccAWSSecurityGroup_vpcProtoNumIngress (78.34s)
=== RUN   TestAccAWSSecurityGroup_MultiIngress
--- PASS: TestAccAWSSecurityGroup_MultiIngress (99.13s)
=== RUN   TestAccAWSSecurityGroup_Change
--- PASS: TestAccAWSSecurityGroup_Change (134.95s)
=== RUN   TestAccAWSSecurityGroup_ChangeRuleDescription
--- PASS: TestAccAWSSecurityGroup_ChangeRuleDescription (190.16s)
=== RUN   TestAccAWSSecurityGroup_generatedName
--- PASS: TestAccAWSSecurityGroup_generatedName (82.02s)
=== RUN   TestAccAWSSecurityGroup_DefaultEgress_VPC
--- PASS: TestAccAWSSecurityGroup_DefaultEgress_VPC (80.49s)
=== RUN   TestAccAWSSecurityGroup_DefaultEgress_Classic
--- PASS: TestAccAWSSecurityGroup_DefaultEgress_Classic (30.96s)
=== RUN   TestAccAWSSecurityGroup_drift
--- PASS: TestAccAWSSecurityGroup_drift (43.27s)
=== RUN   TestAccAWSSecurityGroup_drift_complex
--- PASS: TestAccAWSSecurityGroup_drift_complex (78.55s)
=== RUN   TestAccAWSSecurityGroup_invalidCIDRBlock
--- PASS: TestAccAWSSecurityGroup_invalidCIDRBlock (2.95s)
=== RUN   TestAccAWSSecurityGroup_tags
--- PASS: TestAccAWSSecurityGroup_tags (106.00s)
=== RUN   TestAccAWSSecurityGroup_CIDRandGroups
--- PASS: TestAccAWSSecurityGroup_CIDRandGroups (79.67s)
=== RUN   TestAccAWSSecurityGroup_ingressWithCidrAndSGs
--- PASS: TestAccAWSSecurityGroup_ingressWithCidrAndSGs (79.36s)
=== RUN   TestAccAWSSecurityGroup_ingressWithCidrAndSGs_classic
--- PASS: TestAccAWSSecurityGroup_ingressWithCidrAndSGs_classic (35.74s)
=== RUN   TestAccAWSSecurityGroup_egressWithPrefixList
--- PASS: TestAccAWSSecurityGroup_egressWithPrefixList (78.52s)
=== RUN   TestAccAWSSecurityGroup_ipv4andipv6Egress
--- PASS: TestAccAWSSecurityGroup_ipv4andipv6Egress (65.51s)
=== RUN   TestAccAWSSecurityGroup_failWithDiffMismatch
--- PASS: TestAccAWSSecurityGroup_failWithDiffMismatch (77.46s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	4692.435s

[bflad]

Thanks @jaymecd and @radeksimko for getting this in! 🎉

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!