provider/aws: Fix EC2 Classic SG Rule issue

[catsby]

Fixes an issue where security groups would fail to update after applying an
initial security_group, because we were improperly saving the id of the group
and not the name (EC2 Classic only).

This is a PR combining #4983 and #5184 . It's majority @ephemeralsnow's work.

Fixing:

• Trying to use security_group_rule with ec2 classic blows up #4249
• Error authorizing security group ingress rules: InvalidGroup.NotFound in v0.6.12 #5464

[phinze]

Can you help me read the unit test diffs here? Trying to figure out the narrative of "when I do this, then I get this" and having trouble. Could use a bit of hand holding. ☺️

[catsby]

This chuck matches up with the newly added ec2.IpPermission block:

        &ec2.IpPermission{
            IpProtocol: aws.String("tcp"),
            FromPort:   aws.Int64(int64(443)),
            ToPort:     aws.Int64(int64(443)),
            UserIdGroupPairs: []*ec2.UserIdGroupPair{
                // Classic
                &ec2.UserIdGroupPair{
                    UserId:    aws.String("12345"),
                    GroupId:   aws.String("sg-33333"),
                    GroupName: aws.String("ec2_classic"),
                },
                &ec2.UserIdGroupPair{
                    UserId:    aws.String("amazon-elb"),
                    GroupId:   aws.String("sg-d2c979d3"),
                    GroupName: aws.String("amazon-elb-sg"),
                },
            },
        }

It represents a security group in EC2 Classic with two name references, one to a group named ec2_classic and another is the special Amazon ELB security group.

1: https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_security_group_test.go#L18

[phinze]

I'm curious - why are all these SG ids changing?

[catsby]

These are typo fixes. The original tests never checked the second IP Pair returned, but now they do. As such, I'm sure this typo was revealed (you can't add the same security group twice)

[phinze]

Ok @catsby a few inline comments for ya! ⛵

[catsby]

Test results:

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSSecurityGroup_' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
/Users/clint/Projects/Go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSSecurityGroup_ -timeout 120m
=== RUN   TestAccAWSSecurityGroup_basic
--- PASS: TestAccAWSSecurityGroup_basic (8.10s)
=== RUN   TestAccAWSSecurityGroup_namePrefix
--- PASS: TestAccAWSSecurityGroup_namePrefix (5.27s)
=== RUN   TestAccAWSSecurityGroup_self
--- PASS: TestAccAWSSecurityGroup_self (7.21s)
=== RUN   TestAccAWSSecurityGroup_vpc
--- PASS: TestAccAWSSecurityGroup_vpc (16.12s)
=== RUN   TestAccAWSSecurityGroup_vpcNegOneIngress
--- PASS: TestAccAWSSecurityGroup_vpcNegOneIngress (14.99s)
=== RUN   TestAccAWSSecurityGroup_MultiIngress
--- PASS: TestAccAWSSecurityGroup_MultiIngress (14.16s)
=== RUN   TestAccAWSSecurityGroup_Change
--- PASS: TestAccAWSSecurityGroup_Change (12.17s)
=== RUN   TestAccAWSSecurityGroup_generatedName
--- PASS: TestAccAWSSecurityGroup_generatedName (7.95s)
=== RUN   TestAccAWSSecurityGroup_DefaultEgress
--- PASS: TestAccAWSSecurityGroup_DefaultEgress (21.59s)
=== RUN   TestAccAWSSecurityGroup_drift
--- PASS: TestAccAWSSecurityGroup_drift (7.30s)
=== RUN   TestAccAWSSecurityGroup_drift_complex
--- PASS: TestAccAWSSecurityGroup_drift_complex (12.67s)
=== RUN   TestAccAWSSecurityGroup_tags
--- PASS: TestAccAWSSecurityGroup_tags (12.47s)
=== RUN   TestAccAWSSecurityGroup_CIDRandGroups
--- PASS: TestAccAWSSecurityGroup_CIDRandGroups (12.45s)
=== RUN   TestAccAWSSecurityGroup_ingressWithCidrAndSGs
--- PASS: TestAccAWSSecurityGroup_ingressWithCidrAndSGs (12.71s)
=== RUN   TestAccAWSSecurityGroup_ingressWithCidrAndSGs_classic
--- PASS: TestAccAWSSecurityGroup_ingressWithCidrAndSGs_classic (9.74s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    174.928s

[phinze]

LGTM

[xsb]

I can confirm that this fixes my problem in #5532

I also noticed that EC2-Classic forces you to use SG names instead of SG ids but this is not documented in the Terraform documentation. Having a field called source_security_group_id is a bit confusing in this case.

[ephemeralsnow]

@catsby Thank you for your support.

[catsby]

@ephemeralsnow thank you for your contribution! We ❤️ our contributors and users 😄

[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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.