resource/aws_security_group_rule: Prevent crash when reading rules from groups containing an ALL/-1 protocol rule

[bflad]

Fixes #6416

The rule searching logic was missing nil checks. This crash was only triggered under the context of other rules existing in the group having FromPort and ToPort in comparison to the local rule not having FromPort and ToPort.

This also prevents an errant difference when to_port is specified as 65535 in an ALL/-1 protocol rule, which is allowed by the API and previously supported in Terraform without this difference.

Previously:

--- FAIL: TestAccAWSSecurityGroupRule_Description_AllPorts_ToPort65535 (15.97s)
    testing.go:538: Step 0 error: After applying this step and refreshing, the plan was not empty:

        DIFF:

        DESTROY/CREATE: aws_security_group_rule.test
          cidr_blocks.#:            "1" => "1"
          cidr_blocks.0:            "0.0.0.0/0" => "0.0.0.0/0"
          description:              "description1" => "description1"
          from_port:                "0" => "0"
          protocol:                 "-1" => "-1"
          security_group_id:        "sg-023cc8b0fbfab4a05" => "sg-023cc8b0fbfab4a05"
          self:                     "false" => "false"
          source_security_group_id: "" => "<computed>"
          to_port:                  "0" => "65535" (forces new resource)
          type:                     "ingress" => "ingress"

=== CONT  TestAccAWSSecurityGroupRule_MultipleRuleSearching_AllProtocolCrash
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x2f6c5fc]

goroutine 576 [running]:
github.com/terraform-providers/terraform-provider-aws/aws.findRuleMatch(0xc000608500, 0xc0006f4390, 0x2, 0x2, 0x1, 0x0)
	/Users/bflad/go/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_security_group_rule.go:433 +0x6c
github.com/terraform-providers/terraform-provider-aws/aws.resourceAwsSecurityGroupRuleCreate.func1(0xc00070eb90)
	/Users/bflad/go/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_security_group_rule.go:227 +0x2bf

Output from acceptance testing:

--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidTypeError (1.53s)
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidCIDR (1.70s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_VPC (22.23s)
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription (21.89s)
--- PASS: TestAccAWSSecurityGroupRule_Egress (25.56s)
--- PASS: TestAccAWSSecurityGroupRule_MultipleRuleSearching_AllProtocolCrash (26.06s)
--- PASS: TestAccAWSSecurityGroupRule_Issue5310 (26.90s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Classic (27.31s)
--- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts (34.32s)
--- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts_ToPort65535 (36.12s)
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription_updates (35.41s)
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription_updates (37.68s)
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription (39.34s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Ipv6 (41.93s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Protocol (42.58s)
--- PASS: TestAccAWSSecurityGroupRule_SelfReference (43.87s)
--- PASS: TestAccAWSSecurityGroupRule_SelfSource (45.94s)
--- PASS: TestAccAWSSecurityGroupRule_MultiIngress (25.31s)
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_Source (48.62s)
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_basic (51.84s)
--- PASS: TestAccAWSSecurityGroupRule_PrefixListEgress (57.50s)
--- PASS: TestAccAWSSecurityGroupRule_MultiDescription (88.64s)
--- PASS: TestAccAWSSecurityGroupRule_Race (274.35s)

[tombuildsstuff]

one minor 🤔 for future enhancement but this otherwise LGTM 👍

[tombuildsstuff]

in the future it'd be good for protocolForValue to return a *string so we can easily detect this is invalid?

[bflad]

protocolForValue() returns the lowercased input string if it cannot find a match. Since we're specifically checking for a known value (should probably be a constant 🤔 ), I don't think it matters too much in this context since we do not have the opportunity to throw an error (other than panic()) within a DiffSuppressFunc. Definitely open to chatting about this further if I'm missing something (🥁). 😄

[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!