resource/aws_instance: Add check for empty credit_specification block

[nywilken]

Fixes: #6532

When given an empty credit_specification block (i.e cpu_credits is missing) the Terraform provider
will crash upon trying to type assert on the empty (nil) block. This check introduces a guard clause around the type assertion
to protect around the empty (nil) block. A warning is logged to indicated that a default value will be used in place of the missing
value.

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Release note for CHANGELOG:

* resource/aws_instance: Add check for empty credit_specification block

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSInstance_creditSpecification_unknown'

--- PASS: TestAccAWSInstance_creditSpecification_unknownCpuCredits_t2
--- PASS: TestAccAWSInstance_creditSpecification_unknownCpuCredits_t3

[nywilken]

Suggested change

	`, rInt, instance_type)
	`, rInt, instance)

[aeschright]

Having it match the attribute name makes the most sense to me.

[bflad]

Drive by comment: we historically have preferred naming these consistent with Terraform configuration argument and using mixed caps to match the Effective Go documentation. So in this case: instanceType 👍

golint would catch underscored variable names like this, e.g. one example on master:

aws/config_test.go:43:5: don't use underscores in Go names; var `test_ec2_describeAccountAttributes_response` should be `testEc2DescribeAccountAttributesResponse` (golint)
var test_ec2_describeAccountAttributes_response = `<DescribeAccountAttributesResponse xmlns="http://ec2.amazonaws.com/doc/2016-11-15/">
    ^

However there are a lot of linting failures we generally do not want to concern ourselves within golint, including its non-configurable initialisms and the underscore check for acceptance test function names (among others), which is why its not enabled by default in our pull request golangci-lint testing.

[bflad]

Submitted a pull request to the contributing guide to ensure it includes a link to Effective Go: #9031

[nywilken]

Drive by commenting - love it. Thanks for the additional context @bflad and good call out on the Effective Go resource.

[nywilken]

Suggested change

	func testAccInstanceConfig_creditSpecification_unknownCpuCredits(rInt int, instance_type string) string {
	func testAccInstanceConfig_creditSpecification_unknownCpuCredits(rInt int, instance string) string {

[bflad]

Overall this looks good to go. Since this pull request relates to fixing a panic within the provider, I have added the bug and crash labels.

I left some non-blocking nitpicks below, which are more likely to be addressed overall for this resource's acceptance testing via #4987, but left for context. Relating the to provided CHANGELOG note, I would slightly rephrase it to be a little more clear on the impact of this change from an operator perspective, so they can better access the criticality for their environment:

* resource/aws_instance: Prevent panic when `credit_specification` configuration block is missing arguments

The new acceptance testing looks good in the sense that it appropriately triggers the panic without the resource logic update:

=== CONT  TestAccAWSInstance_creditSpecification_unknownCpuCredits_t2
panic: interface conversion: interface {} is nil, not map[string]interface {}

goroutine 493 [running]:
github.com/terraform-providers/terraform-provider-aws/aws.buildAwsInstanceOpts(0xc0003b0770, 0x524f1c0, 0xc000572800, 0xc000044070, 0xc000044000, 0x68)
  /Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_instance.go:1767 +0x21e2
github.com/terraform-providers/terraform-provider-aws/aws.resourceAwsInstanceCreate(0xc0003b0770, 0x524f1c0, 0xc000572800, 0x2, 0xaa41a20)
  /Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_instance.go:514 +0x7c

Output from acceptance testing in AWS Commercial:

--- PASS: TestAccAWSInstance_addSecondaryInterface (104.72s)
--- PASS: TestAccAWSInstance_addSecurityGroupNetworkInterface (226.59s)
--- PASS: TestAccAWSInstance_associatePublic_defaultPrivate (71.07s)
--- PASS: TestAccAWSInstance_associatePublic_defaultPublic (272.77s)
--- PASS: TestAccAWSInstance_associatePublic_explicitPrivate (70.30s)
--- PASS: TestAccAWSInstance_associatePublic_explicitPublic (71.28s)
--- PASS: TestAccAWSInstance_associatePublic_overridePrivate (192.38s)
--- PASS: TestAccAWSInstance_associatePublic_overridePublic (71.09s)
--- PASS: TestAccAWSInstance_associatePublicIPAndPrivateIP (70.26s)
--- PASS: TestAccAWSInstance_basic (245.79s)
--- PASS: TestAccAWSInstance_blockDevices (77.46s)
--- PASS: TestAccAWSInstance_changeInstanceType (329.34s)
--- PASS: TestAccAWSInstance_creditSpecification_isNotAppliedToNonBurstable (100.23s)
--- PASS: TestAccAWSInstance_creditSpecification_standardCpuCredits (198.99s)
--- PASS: TestAccAWSInstance_creditSpecification_standardCpuCredits_t2Tot3Taint (491.78s)
--- PASS: TestAccAWSInstance_creditSpecification_unknownCpuCredits_t2 (111.36s)
--- PASS: TestAccAWSInstance_creditSpecification_unknownCpuCredits_t3 (314.59s)
--- PASS: TestAccAWSInstance_creditSpecification_unlimitedCpuCredits (189.95s)
--- PASS: TestAccAWSInstance_creditSpecification_unlimitedCpuCredits_t2Tot3Taint (329.66s)
--- PASS: TestAccAWSInstance_creditSpecification_unspecifiedDefaultsToStandard (272.23s)
--- PASS: TestAccAWSInstance_creditSpecification_updateCpuCredits (94.11s)
--- PASS: TestAccAWSInstance_creditSpecificationT3_standardCpuCredits (299.60s)
--- PASS: TestAccAWSInstance_creditSpecificationT3_unlimitedCpuCredits (329.36s)
--- PASS: TestAccAWSInstance_creditSpecificationT3_unspecifiedDefaultsToUnlimited (294.67s)
--- PASS: TestAccAWSInstance_creditSpecificationT3_updateCpuCredits (115.73s)
--- PASS: TestAccAWSInstance_disableApiTermination (117.01s)
--- PASS: TestAccAWSInstance_disappears (228.80s)
--- PASS: TestAccAWSInstance_forceNewAndTagsDrift (102.44s)
--- PASS: TestAccAWSInstance_getPasswordData_falseToTrue (358.65s)
--- PASS: TestAccAWSInstance_getPasswordData_trueToFalse (158.55s)
--- PASS: TestAccAWSInstance_GP2IopsDevice (67.39s)
--- PASS: TestAccAWSInstance_GP2WithIopsValue (71.16s)
--- PASS: TestAccAWSInstance_importBasic (208.29s)
--- PASS: TestAccAWSInstance_importInDefaultVpcBySgId (191.66s)
--- PASS: TestAccAWSInstance_importInDefaultVpcBySgName (188.75s)
--- PASS: TestAccAWSInstance_importInEc2Classic (86.71s)
--- PASS: TestAccAWSInstance_instanceProfileChange (263.92s)
--- PASS: TestAccAWSInstance_ipv6_supportAddressCount (71.50s)
--- PASS: TestAccAWSInstance_ipv6_supportAddressCountWithIpv4 (61.67s)
--- PASS: TestAccAWSInstance_ipv6AddressCountAndSingleAddressCausesError (6.79s)
--- PASS: TestAccAWSInstance_keyPairCheck (80.63s)
--- PASS: TestAccAWSInstance_multipleRegions (140.34s)
--- PASS: TestAccAWSInstance_NetworkInstanceRemovingAllSecurityGroups (75.05s)
--- PASS: TestAccAWSInstance_NetworkInstanceSecurityGroups (82.25s)
--- PASS: TestAccAWSInstance_NetworkInstanceVPCSecurityGroupIDs (182.64s)
--- PASS: TestAccAWSInstance_noAMIEphemeralDevices (178.69s)
--- PASS: TestAccAWSInstance_placementGroup (171.23s)
--- PASS: TestAccAWSInstance_primaryNetworkInterface (193.49s)
--- PASS: TestAccAWSInstance_primaryNetworkInterfaceSourceDestCheck (52.20s)
--- PASS: TestAccAWSInstance_privateIP (60.10s)
--- PASS: TestAccAWSInstance_rootBlockDeviceMismatch (171.60s)
--- PASS: TestAccAWSInstance_rootInstanceStore (66.85s)
--- PASS: TestAccAWSInstance_sourceDestCheck (127.13s)
--- PASS: TestAccAWSInstance_tags (114.50s)
--- PASS: TestAccAWSInstance_UserData_EmptyStringToUnspecified (69.06s)
--- PASS: TestAccAWSInstance_UserData_UnspecifiedToEmptyString (70.45s)
--- PASS: TestAccAWSInstance_userDataBase64 (230.08s)
--- PASS: TestAccAWSInstance_volumeTags (96.38s)
--- PASS: TestAccAWSInstance_volumeTagsComputed (91.09s)
--- PASS: TestAccAWSInstance_vpc (201.18s)
--- PASS: TestAccAWSInstance_withIamInstanceProfile (241.29s)
--- PASS: TestAccAWSInstanceDataSource_AzUserData (123.73s)
--- PASS: TestAccAWSInstanceDataSource_basic (251.58s)
--- PASS: TestAccAWSInstanceDataSource_blockDevices (95.21s)
--- PASS: TestAccAWSInstanceDataSource_creditSpecification (115.33s)
--- PASS: TestAccAWSInstanceDataSource_getPasswordData_falseToTrue (168.11s)
--- PASS: TestAccAWSInstanceDataSource_getPasswordData_trueToFalse (169.78s)
--- PASS: TestAccAWSInstanceDataSource_GetUserData (121.72s)
--- PASS: TestAccAWSInstanceDataSource_GetUserData_NoUserData (100.39s)
--- PASS: TestAccAWSInstanceDataSource_gp2IopsDevice (184.52s)
--- PASS: TestAccAWSInstanceDataSource_keyPair (92.32s)
--- PASS: TestAccAWSInstanceDataSource_PlacementGroup (185.53s)
--- PASS: TestAccAWSInstanceDataSource_privateIP (205.28s)
--- PASS: TestAccAWSInstanceDataSource_rootInstanceStore (103.60s)
--- PASS: TestAccAWSInstanceDataSource_SecurityGroups (125.04s)
--- PASS: TestAccAWSInstanceDataSource_tags (119.97s)
--- PASS: TestAccAWSInstanceDataSource_VPC (114.87s)
--- PASS: TestAccAWSInstanceDataSource_VPCSecurityGroups (73.45s)
--- PASS: TestAccAWSInstancesDataSource_basic (87.68s)
--- PASS: TestAccAWSInstancesDataSource_instance_state_names (87.66s)
--- PASS: TestAccAWSInstancesDataSource_tags (81.84s)

I'm going to elide the output from acceptance testing in AWS GovCloud (US) due to mostly being failures relating to #4987.

[bflad]

Our acceptance testing should prefer to remain AWS region and availability zone agnostic so it can be used against multiple AWS partitions or otherwise be redirected for region/AZ outages. See also: #4987 and this section under https://github.com/terraform-providers/terraform-provider-aws/blob/master/.github/CONTRIBUTING.md#acceptance-testing-guidelines

The below are location-based items that may be noted during review and are recommended for consistency with testing flexibility. Resource testing is expected to pass across multiple AWS environments supported by the Terraform AWS Provider (e.g. AWS Standard and AWS GovCloud (US)).

In this case, we can either allow EC2 to automatically select the subnet availability zone by omitting the argument or use the aws_availability_zones data source to select one in the available state, e.g.

data "aws_availability_zones" "current" {
  state = "available"
}

resource "aws_subnet" "my_subnet" {
  # ... other configuration ...
  availability_zone = "${data.aws_availability_zones.current.names[0]}"

[bflad]

Same note and references here about being AWS region agnostic. We can handle AMI lookup via the aws_ami data source against Amazon Linux AMIs that are generally available in all AWS partitions and regions. e.g. from the contributing guide

data "aws_ami" "amzn-ami-minimal-hvm-ebs" {
  most_recent = true
  owners      = ["amazon"]

  filter {
    name = "name"
    values = ["amzn-ami-minimal-hvm-*"]
  }
  filter {
    name = "root-device-type"
    values = ["ebs"]
  }
}

resource "aws_instance" "foo" {
  ami           = "${data.aws_ami.amzn-ami-minimal-hvm-ebs.id}"

[nywilken]

@bflad thanks the review and context around testing. I'll circle back on this resource with the details outlined in #4987 and the CONTRIBUTING guide.

[bflad]

This has been released in version 2.16.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

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