Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resource/aws_instance: Add check for empty credit_specification block #9003

Merged
merged 1 commit into from
Jun 20, 2019

Conversation

nywilken
Copy link
Contributor

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

@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Jun 14, 2019
@nywilken nywilken requested a review from aeschright June 14, 2019 20:14
@ghost ghost added service/ec2 Issues and PRs that pertain to the ec2 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Jun 14, 2019
@nywilken nywilken added this to the v2.16.0 milestone Jun 14, 2019
credit_specification {
}
}
`, rInt, instance_type)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`, rInt, instance_type)
`, rInt, instance)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -3971,6 +4016,33 @@ resource "aws_instance" "foo" {
`, rInt)
}

func testAccInstanceConfig_creditSpecification_unknownCpuCredits(rInt int, instance_type string) string {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nywilken nywilken force-pushed the b-aws_instance-unknown-cpu_credits-check branch from 4806964 to 10732fc Compare June 18, 2019 03:02
@bflad bflad added bug Addresses a defect in current functionality. crash Results from or addresses a Terraform crash or kernel panic. labels Jun 20, 2019
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

resource "aws_subnet" "my_subnet" {
vpc_id = "${aws_vpc.my_vpc.id}"
cidr_block = "172.16.20.0/24"
availability_zone = "us-west-2a"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]}"

}

resource "aws_instance" "foo" {
ami = "ami-51537029" # us-west-2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

Acceptance tests after changes
```
--- PASS: TestAccAWSInstance_creditSpecification_unknownCpuCredits_t2
--- PASS: TestAccAWSInstance_creditSpecification_unknownCpuCredits_t3
```
@nywilken nywilken force-pushed the b-aws_instance-unknown-cpu_credits-check branch from 10732fc to b8084c0 Compare June 20, 2019 18:24
@nywilken nywilken merged commit b6d0c83 into master Jun 20, 2019
@nywilken nywilken deleted the b-aws_instance-unknown-cpu_credits-check branch June 20, 2019 18:31
@nywilken
Copy link
Contributor Author

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

nywilken added a commit that referenced this pull request Jun 20, 2019
@bflad
Copy link
Contributor

bflad commented Jun 20, 2019

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
Copy link

ghost commented Nov 3, 2019

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!

@ghost ghost locked and limited conversation to collaborators Nov 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. crash Results from or addresses a Terraform crash or kernel panic. service/ec2 Issues and PRs that pertain to the ec2 service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

* aws_instance.instance: unexpected EOF
3 participants