data-source/aws_kms_alias: Prevent crash on aliases without target key #3203

bflad · 2018-01-30T17:03:11Z

According to the KMS API documentation, AWS service KMS aliases (e.g. alias/aws/redshift) do not return TargetKeyId. This PR adds testing against those aliases and ensures the data source does not return target_key_arn or target_key_id in that scenario.

New acceptance test without data source changes:

make testacc TEST=./aws TESTARGS='-run=TestAccDataSourceAwsKmsAlias'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccDataSourceAwsKmsAlias -timeout 120m
=== RUN   TestAccDataSourceAwsKmsAlias_AwsService
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x25b13d3]

goroutine 143 [running]:
github.com/terraform-providers/terraform-provider-aws/aws.dataSourceAwsKmsAliasRead(0xc420209110, 0x3584220, 0xc420280780, 0xc420209110, 0x0)
	/Users/bflad/go/src/github.com/terraform-providers/terraform-provider-aws/aws/data_source_aws_kms_alias.go:76 +0x3b3
github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/helper/schema.(*Resource).ReadDataApply(0xc42034c000, 0xc4205bde60, 0x3584220, 0xc420280780, 0xc420264988, 0xc4205b2601, 0xc4205a7680)
	/Users/bflad/go/src/github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/helper/schema/resource.go:292 +0x8b
github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/helper/schema.(*Provider).ReadDataApply(0xc4200f8fc0, 0xc420793e50, 0xc4205bde60, 0x0, 0x400, 0x44)
	/Users/bflad/go/src/github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/helper/schema/provider.go:426 +0x9a
github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/terraform.(*EvalReadDataApply).Eval(0xc4205bdc00, 0x580b380, 0xc4203141a0, 0x2, 0x2, 0x3985d99, 0x4)
	/Users/bflad/go/src/github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/terraform/eval_read_data.go:122 +0x100
github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/terraform.EvalRaw(0x57fb860, 0xc4205bdc00, 0x580b380, 0xc4203141a0, 0x0, 0x0, 0x0, 0x0)
	/Users/bflad/go/src/github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/terraform/eval.go:53 +0x173
github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/terraform.(*EvalSequence).Eval(0xc4205bdc20, 0x580b380, 0xc4203141a0, 0x2, 0x2, 0x3985d99, 0x4)
	/Users/bflad/go/src/github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/terraform/eval_sequence.go:14 +0x7e
github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/terraform.EvalRaw(0x57fba60, 0xc4205bdc20, 0x580b380, 0xc4203141a0, 0x33c2ec0, 0x580c3c5, 0x3171b80, 0xc4204fc1d0)
	/Users/bflad/go/src/github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/terraform/eval.go:53 +0x173
github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/terraform.Eval(0x57fba60, 0xc4205bdc20, 0x580b380, 0xc4203141a0, 0xc4205bdc20, 0x57fba60, 0xc4205bdc20, 0x108c434)
	/Users/bflad/go/src/github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/terraform/eval.go:34 +0x4d
github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/terraform.(*Graph).walk.func1(0x38c5680, 0xc42059cb30, 0x0, 0x0)
	/Users/bflad/go/src/github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/terraform/graph.go:126 +0xd0d
github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/dag.(*Walker).walkVertex(0xc4202087e0, 0x38c5680, 0xc42059cb30, 0xc42053f340)
	/Users/bflad/go/src/github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/dag/walk.go:387 +0x3c1
created by github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/dag.(*Walker).Update
	/Users/bflad/go/src/github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/dag/walk.go:310 +0x1364
exit status 2
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	3.447s
make: *** [testacc] Error 1

After:

 make testacc TEST=./aws TESTARGS='-run=TestAccDataSourceAwsKmsAlias'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccDataSourceAwsKmsAlias -timeout 120m
=== RUN   TestAccDataSourceAwsKmsAlias_AwsService
--- PASS: TestAccDataSourceAwsKmsAlias_AwsService (9.54s)
=== RUN   TestAccDataSourceAwsKmsAlias_CMK
--- PASS: TestAccDataSourceAwsKmsAlias_CMK (41.27s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	50.859s

Ninir

Hey @bflad,

Good job fixing that! 🚀 🚗
Just left a nitpick, non-blocking at all to me!

$ make testacc TEST=./aws TESTARGS='-run=TestAccDataSourceAwsKmsAlias'                                                         
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccDataSourceAwsKmsAlias -timeout 120m
=== RUN   TestAccDataSourceAwsKmsAlias_AwsService
--- PASS: TestAccDataSourceAwsKmsAlias_AwsService (24.56s)
=== RUN   TestAccDataSourceAwsKmsAlias_CMK
--- PASS: TestAccDataSourceAwsKmsAlias_CMK (54.35s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	78.963s

Ninir · 2018-01-30T22:42:35Z

aws/data_source_aws_kms_alias.go

-		Resource:  fmt.Sprintf("key/%s", *alias.TargetKeyId),
-	}
-	d.Set("target_key_arn", targetKeyARN.String())
+	// AWS service aliases do not return TargetKeyId:


Nitpick: I would probably enhance this message so that we add the context ... do not return TargetKeyId when not associated with a Customer Managed Key (CMK). What do you think?

Agree with @Ninir, there are cases where the key is associated, it is just not the case all the time

I'll update this comment 👍

ColinHebert · 2018-01-31T03:14:49Z

Sweet! I know it's a bit demanding but can we get that one released in a 1.8.1 release?

…etKeyId

bflad · 2018-02-09T22:41:55Z

This has been released in terraform-provider-aws version 1.9.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-08T17:17:23Z

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!

data-source/aws_kms_alias: Prevent crash on AWS service aliases

fac79d5

bflad added bug Addresses a defect in current functionality. crash Results from or addresses a Terraform crash or kernel panic. service/kms Issues and PRs that pertain to the kms service. labels Jan 30, 2018

bflad added this to the v1.9.0 milestone Jan 30, 2018

bflad requested review from radeksimko, jen20 and Ninir January 30, 2018 17:03

ghost added the size/L Managed by automation to categorize the size of a PR. label Jan 30, 2018

bflad mentioned this pull request Jan 30, 2018

datasource/aws_kms_alias: panic: runtime error: invalid memory address or nil pointer dereference #3189

Closed

Ninir approved these changes Jan 30, 2018

View reviewed changes

data-source/aws_kms_alias: Improve comment about aliases missing Targ…

19b34df

…etKeyId

ghost added the size/L Managed by automation to categorize the size of a PR. label Jan 31, 2018

bflad changed the title ~~data-source/aws_kms_alias: Prevent crash on AWS service aliases~~ data-source/aws_kms_alias: Prevent crash on aliases without target key Jan 31, 2018

bflad merged commit 4cf4201 into master Jan 31, 2018

bflad deleted the b-aws_kms_alias-fix-aws-alias-crash branch January 31, 2018 14:04

bflad added a commit that referenced this pull request Jan 31, 2018

Update CHANGELOG for #3203

d70769f

bflad added a commit that referenced this pull request Jan 31, 2018

Update CHANGELOG for #3211 and fix #3203 location

6cec9a1

handlerbot mentioned this pull request Feb 5, 2018

Support ARNs for existing KMS keys #2009

Closed

ghost locked and limited conversation to collaborators Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

data-source/aws_kms_alias: Prevent crash on aliases without target key #3203

data-source/aws_kms_alias: Prevent crash on aliases without target key #3203

bflad commented Jan 30, 2018

Ninir left a comment

Ninir Jan 30, 2018

ColinHebert Jan 31, 2018

bflad Jan 31, 2018

ColinHebert commented Jan 31, 2018

bflad commented Feb 9, 2018

ghost commented Apr 8, 2020

data-source/aws_kms_alias: Prevent crash on aliases without target key #3203

data-source/aws_kms_alias: Prevent crash on aliases without target key #3203

Conversation

bflad commented Jan 30, 2018

Ninir left a comment

Choose a reason for hiding this comment

Ninir Jan 30, 2018

Choose a reason for hiding this comment

ColinHebert Jan 31, 2018

Choose a reason for hiding this comment

bflad Jan 31, 2018

Choose a reason for hiding this comment

ColinHebert commented Jan 31, 2018

bflad commented Feb 9, 2018

ghost commented Apr 8, 2020