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

r/aws_lightsail_instance: Fix crash when encountering key-only tags #12346

Merged
merged 7 commits into from
Sep 1, 2020
Merged

r/aws_lightsail_instance: Fix crash when encountering key-only tags #12346

merged 7 commits into from
Sep 1, 2020

Conversation

stefansundin
Copy link
Contributor

@stefansundin stefansundin commented Mar 11, 2020

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" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Fixes this crash:

panic: runtime error: invalid memory address or nil pointer dereference
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: [signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x44409fa]
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4:
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: goroutine 184 [running]:
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags.KeyValueTags.Map(0xc00082ff20, 0xc00082ff20)
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: 	/opt/teamcity-agent/work/5d79fe75d4460a2f/src/github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags/key_value_tags.go:138 +0xda
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: github.com/terraform-providers/terraform-provider-aws/aws.resourceAwsLightsailInstanceRead(0xc000278e00, 0x51fa760, 0xc000180a00, 0xc000278e00, 0x0)
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: 	/opt/teamcity-agent/work/5d79fe75d4460a2f/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_lightsail_instance.go:216 +0x6eb
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: github.com/hashicorp/terraform-plugin-sdk/helper/schema.(*Resource).RefreshWithoutUpgrade(0xc000618200, 0xc000ff7c20, 0x51fa760, 0xc000180a00, 0xc00043cfb8, 0x0, 0x0)
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: 	/opt/teamcity-agent/work/5d79fe75d4460a2f/src/github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform-plugin-sdk/helper/schema/resource.go:455 +0x119
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: github.com/hashicorp/terraform-plugin-sdk/internal/helper/plugin.(*GRPCProviderServer).ReadResource(0xc00043cb00, 0x6e58080, 0xc000995440, 0xc000ff7b30, 0xc00043cb00, 0xc000995440, 0xc000a40a80)
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: 	/opt/teamcity-agent/work/5d79fe75d4460a2f/src/github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform-plugin-sdk/internal/helper/plugin/grpc_provider.go:525 +0x3d8
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: github.com/hashicorp/terraform-plugin-sdk/internal/tfplugin5._Provider_ReadResource_Handler(0x635f2e0, 0xc00043cb00, 0x6e58080, 0xc000995440, 0xc000917ec0, 0x0, 0x6e58080, 0xc000995440, 0xc00098e380, 0x1bc)
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: 	/opt/teamcity-agent/work/5d79fe75d4460a2f/src/github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform-plugin-sdk/internal/tfplugin5/tfplugin5.pb.go:3153 +0x217
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: google.golang.org/grpc.(*Server).processUnaryRPC(0xc0000d8000, 0x6e78cc0, 0xc000545e00, 0xc00092a200, 0xc0003c80f0, 0xa2400d0, 0x0, 0x0, 0x0)
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: 	/opt/teamcity-agent/work/5d79fe75d4460a2f/src/github.com/terraform-providers/terraform-provider-aws/vendor/google.golang.org/grpc/server.go:995 +0x460
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: google.golang.org/grpc.(*Server).handleStream(0xc0000d8000, 0x6e78cc0, 0xc000545e00, 0xc00092a200, 0x0)
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: 	/opt/teamcity-agent/work/5d79fe75d4460a2f/src/github.com/terraform-providers/terraform-provider-aws/vendor/google.golang.org/grpc/server.go:1275 +0xd97
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: google.golang.org/grpc.(*Server).serveStreams.func1.1(0xc0003b0800, 0xc0000d8000, 0x6e78cc0, 0xc000545e00, 0xc00092a200)
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: 	/opt/teamcity-agent/work/5d79fe75d4460a2f/src/github.com/terraform-providers/terraform-provider-aws/vendor/google.golang.org/grpc/server.go:710 +0xbb
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: created by google.golang.org/grpc.(*Server).serveStreams.func1
2020-03-10T19:07:52.845-0700 [DEBUG] plugin.terraform-provider-aws_v2.52.0_x4: 	/opt/teamcity-agent/work/5d79fe75d4460a2f/src/github.com/terraform-providers/terraform-provider-aws/vendor/google.golang.org/grpc/server.go:708 +0xa1

Creating tags with an empty string as a value appears to correctly create these key-only tags.

Documentation:

Release note for CHANGELOG:

resource/aws_lightsail_instance: Support key-only tags.

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSLightsailInstance_Tags'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSLightsailInstance_Tags -timeout 120m
=== RUN   TestAccAWSLightsailInstance_Tags
=== PAUSE TestAccAWSLightsailInstance_Tags
=== CONT  TestAccAWSLightsailInstance_Tags
--- PASS: TestAccAWSLightsailInstance_Tags (80.99s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	82.636s

…Creating tags with an empty string as a value appears to correctly create these key-only tags.
@stefansundin stefansundin requested a review from a team March 11, 2020 03:09
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. service/lightsail Issues and PRs that pertain to the lightsail service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. documentation Introduces or discusses updates to documentation. labels Mar 11, 2020
@@ -1782,7 +1782,13 @@ func LightsailKeyValueTags(tags []*lightsail.Tag) KeyValueTags {
m := make(map[string]*string, len(tags))

for _, tag := range tags {
m[aws.StringValue(tag.Key)] = tag.Value
if tag.Value == nil {
Copy link
Contributor

@ewbankkit ewbankkit Mar 12, 2020

Choose a reason for hiding this comment

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

@stefansundin Thanks for this.
This source file is actually generated and will be recreated if make gen is run.
This behaviour may well be occurring for other AWS services (I will do some spot tests) so should be fixed globally.

https://github.com/terraform-providers/terraform-provider-aws/blob/654cc23a38444d4836155d6a2243e3976b5f3992/aws/internal/keyvaluetags/generators/servicetags/main.go#L256-L265

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Updated. Let me know if I should make more changes.

@ewbankkit
Copy link
Contributor

@stefansundin I opened an issue to track this.

Closes #12368.

@bflad bflad linked an issue Mar 13, 2020 that may be closed by this pull request
@bflad bflad added bug Addresses a defect in current functionality. crash Results from or addresses a Terraform crash or kernel panic. and removed needs-triage Waiting for first response or review from a maintainer. labels Mar 13, 2020
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/XS Managed by automation to categorize the size of a PR. labels Mar 21, 2020
@stefansundin
Copy link
Contributor Author

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSLightsailInstance_Tags'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSLightsailInstance_Tags -timeout 120m
=== RUN   TestAccAWSLightsailInstance_Tags
=== PAUSE TestAccAWSLightsailInstance_Tags
=== CONT  TestAccAWSLightsailInstance_Tags
--- PASS: TestAccAWSLightsailInstance_Tags (80.99s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	82.636s

@ewbankkit
Copy link
Contributor

Verified acceptance test:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSLightsailInstance_Tags'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -count 1 -parallel 20 -run=TestAccAWSLightsailInstance_Tags -timeout 120m
=== RUN   TestAccAWSLightsailInstance_Tags
=== PAUSE TestAccAWSLightsailInstance_Tags
=== CONT  TestAccAWSLightsailInstance_Tags
--- PASS: TestAccAWSLightsailInstance_Tags (70.07s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	70.117s

@bflad
Copy link
Contributor

bflad commented Jun 24, 2020

Hi @stefansundin 👋 Thank you for reporting this and sorry you ran into trouble here.

Rather than adjusting all of the generated service functions, we would prefer to fix the underlying (keyvaluetags.KeyValueTags).Map() panic as part some work we are doing to add support for Auto Scaling service tags: https://github.com/terraform-providers/terraform-provider-aws/pull/13868/files#diff-d536593b39c59c8af54d5145c3e6206eL180

If there are other services that are distinguishing between nil/"" we can revisit this type of change on a per-service level.

Can you adjust this pull request to just being the aws_lightsail_instance testing and documentation updates and we will use this to verify Lightsail key-only tagging is working as expected? Thank you.

@bflad bflad self-assigned this Jun 24, 2020
@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jun 24, 2020
@stefansundin
Copy link
Contributor Author

stefansundin commented Jun 25, 2020

@bflad Do you want me to pull in master?

Edit: Nevermind, I did.

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jun 25, 2020
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. and removed size/XL Managed by automation to categorize the size of a PR. labels Jun 25, 2020
@bflad
Copy link
Contributor

bflad commented Jun 25, 2020

@stefansundin thanks so much! I was able to verify this updated test passes after #13868.

--- PASS: TestAccAWSLightsailInstance_Tags (75.80s)

We should expect to see the fix land in the next week or so.

@bflad bflad added this to the v3.5.0 milestone Sep 1, 2020
@bflad bflad merged commit fcac15f into hashicorp:master Sep 1, 2020
@ghost
Copy link

ghost commented Sep 3, 2020

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Oct 1, 2020

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 as resolved and limited conversation to collaborators Oct 1, 2020
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. documentation Introduces or discusses updates to documentation. service/lightsail Issues and PRs that pertain to the lightsail service. size/XS 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.

r/aws_lightsail_instance: Crash when adding tag with empty value
3 participants