provider/aws: Enforced kms_key_* attributes to be ARNs #10356

Ninir · 2016-11-24T22:26:20Z

Reasonning

At the moment, it is possible to use the KMS Key ID, the KMS Key Alias or the KMS Key ARN.
Since AWS is sending back ARNs only, using something else than Kms Key ARNs means that resources are always flapping, thus triggering and update on each change.
Thus, this enforces kms_key_* attributes to be ARNs.

Related issues

Closes #10070
Fixes #9626

TODOs

Run aws_cloudtrail acceptance tests

# Basic tests
$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSCloudTrail_'         
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/24 23:21:02 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSCloudTrail_ -timeout 120m
=== RUN   TestAccAWSCloudTrail_importBasic
--- PASS: TestAccAWSCloudTrail_importBasic (62.72s)
=== RUN   TestAccAWSCloudTrail_basic
--- PASS: TestAccAWSCloudTrail_basic (99.45s)
=== RUN   TestAccAWSCloudTrail_enable_logging
--- PASS: TestAccAWSCloudTrail_enable_logging (138.17s)
=== RUN   TestAccAWSCloudTrail_is_multi_region
--- PASS: TestAccAWSCloudTrail_is_multi_region (135.11s)
=== RUN   TestAccAWSCloudTrail_logValidation
--- PASS: TestAccAWSCloudTrail_logValidation (97.07s)
=== RUN   TestAccAWSCloudTrail_tags
--- PASS: TestAccAWSCloudTrail_tags (145.30s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	677.878s

# KMS key id
$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSCloudTrail_kmsKey'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/25 19:10:19 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSCloudTrail_kmsKey -timeout 120m
=== RUN   TestAccAWSCloudTrail_kmsKey
--- PASS: TestAccAWSCloudTrail_kmsKey (299.83s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	299.859s

Run aws_db_instance acceptance tests

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSDBInstance_'                   
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/25 13:33:01 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSDBInstance_ -timeout 120m
=== RUN   TestAccAWSDBInstance_importBasic
--- PASS: TestAccAWSDBInstance_importBasic (732.75s)
=== RUN   TestAccAWSDBInstance_basic
--- PASS: TestAccAWSDBInstance_basic (614.40s)
=== RUN   TestAccAWSDBInstance_kmsKey
--- PASS: TestAccAWSDBInstance_kmsKey (577.55s)
=== RUN   TestAccAWSDBInstance_optionGroup
--- PASS: TestAccAWSDBInstance_optionGroup (727.13s)
=== RUN   TestAccAWSDBInstance_enhancedMonitoring
--- PASS: TestAccAWSDBInstance_enhancedMonitoring (867.78s)
=== RUN   TestAccAWSDBInstance_portUpdate
--- PASS: TestAccAWSDBInstance_portUpdate (657.96s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	4177.590s

Run aws_ebs_volume acceptance tests

# Basic checks
$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSEBSVolume_'  
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/25 11:27:11 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSEBSVolume_ -timeout 120m
=== RUN   TestAccAWSEBSVolume_importBasic
--- PASS: TestAccAWSEBSVolume_importBasic (24.97s)
=== RUN   TestAccAWSEBSVolume_basic
--- PASS: TestAccAWSEBSVolume_basic (28.72s)
=== RUN   TestAccAWSEBSVolume_NoIops
--- PASS: TestAccAWSEBSVolume_NoIops (25.17s)
=== RUN   TestAccAWSEBSVolume_withTags
--- PASS: TestAccAWSEBSVolume_withTags (25.61s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	104.500s

# KMS test
$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSEBSVolume_kmsKey'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/25 21:19:56 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSEBSVolume_kmsKey -timeout 120m
=== RUN   TestAccAWSEBSVolume_kmsKey
--- PASS: TestAccAWSEBSVolume_kmsKey (75.95s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	75.996s

Run aws_elastic_transcorder_pipeline acceptance tests

# Basic tests
$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSElasticTranscoderPipeline_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/25 11:33:07 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSElasticTranscoderPipeline_ -timeout 120m
=== RUN   TestAccAWSElasticTranscoderPipeline_basic
--- PASS: TestAccAWSElasticTranscoderPipeline_basic (44.63s)
=== RUN   TestAccAWSElasticTranscoderPipeline_notifications
--- PASS: TestAccAWSElasticTranscoderPipeline_notifications (68.14s)
=== RUN   TestAccAWSElasticTranscoderPipeline_withContentConfig
--- PASS: TestAccAWSElasticTranscoderPipeline_withContentConfig (71.72s)
=== RUN   TestAccAWSElasticTranscoderPipeline_withPermissions
--- PASS: TestAccAWSElasticTranscoderPipeline_withPermissions (43.74s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	228.272s

# KMS Key test
$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSElasticTranscoderPipeline_kmsKey'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/25 21:47:30 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSElasticTranscoderPipeline_kmsKey -timeout 120m
=== RUN   TestAccAWSElasticTranscoderPipeline_kmsKey
--- PASS: TestAccAWSElasticTranscoderPipeline_kmsKey (92.86s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	92.891s

Run aws_kinesis_firehose_delivery_stream acceptance tests

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSKinesisFirehoseDeliveryStream_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/25 11:45:49 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSKinesisFirehoseDeliveryStream_ -timeout 120m
=== RUN   TestAccAWSKinesisFirehoseDeliveryStream_s3basic
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3basic (122.45s)
=== RUN   TestAccAWSKinesisFirehoseDeliveryStream_s3WithCloudwatchLogging
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3WithCloudwatchLogging (130.77s)
=== RUN   TestAccAWSKinesisFirehoseDeliveryStream_s3ConfigUpdates
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3ConfigUpdates (265.22s)
=== RUN   TestAccAWSKinesisFirehoseDeliveryStream_RedshiftConfigUpdates
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_RedshiftConfigUpdates (785.81s)
=== RUN   TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchConfigUpdates
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchConfigUpdates (1496.10s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	2800.387s

Run aws_rds_cluster acceptance tests

# No KMS Key configurations
$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSRDSCluster_basic'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/25 14:45:28 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSRDSCluster_basic -timeout 120m
=== RUN   TestAccAWSRDSCluster_basic
--- PASS: TestAccAWSRDSCluster_basic (121.54s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	121.574s

# KMS Key configurations
$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSRDSCluster_kmsKey'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/25 14:49:44 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSRDSCluster_kmsKey -timeout 120m
=== RUN   TestAccAWSRDSCluster_kmsKey
--- PASS: TestAccAWSRDSCluster_kmsKey (142.18s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	142.211s

Run aws_redshift_cluster

# Basic test checking everything is ok
$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSRedshiftCluster_basic'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/25 15:00:08 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSRedshiftCluster_basic -timeout 120m
=== RUN   TestAccAWSRedshiftCluster_basic
--- PASS: TestAccAWSRedshiftCluster_basic (601.56s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	601.595s

# KMS Key test
$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSRedshiftCluster_kmsKey'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/25 16:52:19 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSRedshiftCluster_kmsKey -timeout 120m
=== RUN   TestAccAWSRedshiftCluster_kmsKey
--- PASS: TestAccAWSRedshiftCluster_kmsKey (634.58s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	634.611s

Run aws_s3_bucket_object acceptance tests

# Datasources tests
$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccDataSourceAWSS3BucketObject_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/24 23:50:45 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccDataSourceAWSS3BucketObject_ -timeout 120m
=== RUN   TestAccDataSourceAWSS3BucketObject_basic
--- PASS: TestAccDataSourceAWSS3BucketObject_basic (79.40s)
=== RUN   TestAccDataSourceAWSS3BucketObject_readableBody
--- PASS: TestAccDataSourceAWSS3BucketObject_readableBody (77.45s)
=== RUN   TestAccDataSourceAWSS3BucketObject_kmsEncrypted
--- PASS: TestAccDataSourceAWSS3BucketObject_kmsEncrypted (103.25s)
=== RUN   TestAccDataSourceAWSS3BucketObject_allParams
--- PASS: TestAccDataSourceAWSS3BucketObject_allParams (81.22s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	341.359s

# Resource tests
$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSS3BucketObject_'                 
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/25 21:53:41 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSS3BucketObject_ -timeout 120m
=== RUN   TestAccAWSS3BucketObject_source
--- PASS: TestAccAWSS3BucketObject_source (84.21s)
=== RUN   TestAccAWSS3BucketObject_content
--- PASS: TestAccAWSS3BucketObject_content (94.43s)
=== RUN   TestAccAWSS3BucketObject_withContentCharacteristics
--- PASS: TestAccAWSS3BucketObject_withContentCharacteristics (95.40s)
=== RUN   TestAccAWSS3BucketObject_updates
--- PASS: TestAccAWSS3BucketObject_updates (155.97s)
=== RUN   TestAccAWSS3BucketObject_updatesWithVersioning
--- PASS: TestAccAWSS3BucketObject_updatesWithVersioning (152.45s)
=== RUN   TestAccAWSS3BucketObject_kms
--- PASS: TestAccAWSS3BucketObject_kms (110.29s)
=== RUN   TestAccAWSS3BucketObject_acl
--- PASS: TestAccAWSS3BucketObject_acl (147.63s)
=== RUN   TestAccAWSS3BucketObject_storageClass
--- PASS: TestAccAWSS3BucketObject_storageClass (136.23s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	976.654s

Run aws_ses_receipt_rule acceptance tests

$make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSSESReceiptRule_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/24 23:34:57 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSSESReceiptRule_ -timeout 120m
=== RUN   TestAccAWSSESReceiptRule_basic
--- PASS: TestAccAWSSESReceiptRule_basic (31.32s)
=== RUN   TestAccAWSSESReceiptRule_order
--- PASS: TestAccAWSSESReceiptRule_order (36.77s)
=== RUN   TestAccAWSSESReceiptRule_actions
--- PASS: TestAccAWSSESReceiptRule_actions (48.94s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	117.060s

Update the documentation for each resource

stack72 · 2016-11-29T16:48:09Z

Hi @Ninir

Thanks for the work here - this seems sane to me! I have 1 request though, I think we should add some tests around the validateArn function - this will allow us to be able to test that the validations work without the need to run all of the acceptance tests

We do a similar thing for most of the validateFuncs

Hope this is ok?

Paul

Ninir · 2016-11-29T21:17:08Z

Hey @stack72
Thanks for the idea!

The validateArn function is already testing on its own as far as I saw. Do you mean having tests, thus having a config, that will expect errors such as ExpectError: regexp.MustCompile("X doesn't look like a valid ARN (Y)"),?

Thanks!

stack72 · 2016-11-30T10:07:15Z

@Ninir we have the acceptance tests and these are great - we usually try and add unit tests to the validate func as well

See https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_db_option_group.go#L347
https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_db_option_group_test.go#L163

As you can see, this tests that the values are as expected :) These tests get run at build time

Paul

Ninir · 2016-11-30T11:48:04Z

@stack72 Agreed on all of this :)

As far as I understand (hope I did well 😅 ), these unit tests are already implemented (thus why I didn't create them) here: https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/validators_test.go#L188

Sorry If I didn't get the point :)

stack72 · 2016-11-30T11:50:24Z

AAAHHHH!!! You just reimplemented a function that was already there - well in that case, LGTM!

Thanks for all of the work here

P.

Ninir · 2016-11-30T11:52:16Z

Yeah indeed! will make the PR description more helpful on that next time. Thanks for the quick answers @stack72 (as always :) )

* Added kms_key_* validation to force ARNs * Added Redshift Cluster KMS key test * Added cloudtrail kms key test * Added EBS volume kms key * Added Elastic Transcoder Pipeline kms key arn test

ghost · 2020-04-19T02:17:59Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

Ninir force-pushed the kms_key_attribute branch 3 times, most recently from 79f5321 to 5b3da09 Compare November 25, 2016 13:14

Added kms_key_* validation to force ARNs

71a447d

Ninir force-pushed the kms_key_attribute branch from 5b3da09 to 71a447d Compare November 25, 2016 13:16

Ninir mentioned this pull request Nov 25, 2016

Feature Change: aws_ebs_volume: Modify kms_key_id argument to kms_key_arn #10070

Closed

Ninir added 3 commits November 25, 2016 19:15

Added Redshift Cluster KMS key test

128ddd2

Added cloudtrail kms key test

7f0d372

Added EBS volume kms key

a0f429d

Ninir force-pushed the kms_key_attribute branch from 9d1fc30 to a0f429d Compare November 25, 2016 20:39

Added Elastic Transcoder Pipeline kms key arn test

2f89f64

Ninir changed the title ~~[WIP] Added kms_key_* validation to force ARNs~~ Enforced kms_key_* attributes to be ARNs Nov 25, 2016

Ninir changed the title ~~Enforced kms_key_* attributes to be ARNs~~ provider/aws: Enforced kms_key_* attributes to be ARNs Nov 28, 2016

stack72 added enhancement provider/aws waiting-response An issue/pull request is waiting for a response from the community labels Nov 29, 2016

stack72 merged commit 5481e9e into hashicorp:master Nov 30, 2016

Ninir deleted the kms_key_attribute branch November 30, 2016 11:52

Ninir mentioned this pull request Dec 19, 2016

provider/aws: Reworked validateArn function to handle empty values #10833

Merged

4 tasks

acdha mentioned this pull request Mar 2, 2020

Naming confusion in aws_db_instance hashicorp/terraform-provider-aws#7955

Closed

ghost locked and limited conversation to collaborators Apr 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/aws: Enforced kms_key_* attributes to be ARNs #10356

provider/aws: Enforced kms_key_* attributes to be ARNs #10356

Ninir commented Nov 24, 2016 •

edited

Loading

stack72 commented Nov 29, 2016

Ninir commented Nov 29, 2016 •

edited

Loading

stack72 commented Nov 30, 2016

Ninir commented Nov 30, 2016

stack72 commented Nov 30, 2016

Ninir commented Nov 30, 2016

ghost commented Apr 19, 2020

provider/aws: Enforced kms_key_* attributes to be ARNs #10356

provider/aws: Enforced kms_key_* attributes to be ARNs #10356

Conversation

Ninir commented Nov 24, 2016 • edited Loading

Reasonning

Related issues

TODOs

stack72 commented Nov 29, 2016

Ninir commented Nov 29, 2016 • edited Loading

stack72 commented Nov 30, 2016

Ninir commented Nov 30, 2016

stack72 commented Nov 30, 2016

Ninir commented Nov 30, 2016

ghost commented Apr 19, 2020

Ninir commented Nov 24, 2016 •

edited

Loading

Ninir commented Nov 29, 2016 •

edited

Loading