terraform-aws-rds-cluster
terraform-aws-rds-cluster copied to clipboard
cloudposse
Use aws/rds as default KMS key
If the user enables storage or performance insights encryption without specifying a KMS key, use the AWS-managed aws/rds one.
There's one thing I'm checking. Does the aws/rds key always exist in AWS accounts, or is it only created when needed? If the latter, loading it as a data source could fail in new accounts, leading to a chicken and egg problem with this module attempting to deploy a cluster in a new account. I've asked AWS support for clarification.
Unfortunately, AWS Support confirmed the key is created on-demand: https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/Overview.Encryption.html#Overview.Encryption.Enabling
When you create an encrypted DB instance, you can choose a customer managed CMK or the AWS managed CMK for Amazon RDS to encrypt your DB instance. If you don't specify the key identifier for a customer managed CMK, Amazon RDS uses the AWS managed CMK for your new DB instance. Amazon RDS creates an AWS managed CMK for Amazon RDS for your AWS account. Your AWS account has a different AWS managed CMK for Amazon RDS for each AWS Region.
So, it's probably not OK for this module to require the aws/rds key. Instead, I will change the logic so this key is only looked up if the user enables encryption without specifying keys (a good optimisation anyway), and document the restriction. I'll also investigate if there's a simpler way to create this key than "create an instance with encryption enabled but no key specified".
It might also be possible to take advantage of this automatic creation. Terraform can specify storage_encrypted = true and no KMS key, and the AWS API will set the KMS key to aws/rds. But will that break on updates... hmm 🤔
I've updated this PR so that the aws/rds key is only loaded with a data source if required, and mentioned the "must already exist" restriction in the docs.
Unfortunately, I don't think it's possible to have this module create the key for you. You can create a db instance with storage_enabled = true and kms_key_arn = null, but Terraform will detect kms_key_arn as changed on subsequent runs.
This pull request is now in conflict. Could you fix it @alexjurkiewicz? 🙏
![mergify[bot] avatar](https://avatars.githubusercontent.com/in/10562?v=4 "Published by a pub.dev verified publisher"){kind=small}
This pull request is now in conflict. Could you fix it @alexjurkiewicz? 🙏
@alexjurkiewicz discussing this one internally with the team. Now that we know those AWS KMS Keys are not around by default in new accounts... I'm wondering if we should just use cloudposse/kms-key/aws in the cases where no key is passed but storage_encrypted is true. Let me know your thoughts and I'll also discuss with the team.
Thanks!
That makes sense. A KMS key (without rotation) is $1/mo, which is nothing compared to the cost of an RDS cluster.
d1a677f:
Create KMS keys if needed
Rather than rely on aws/rds, which isn't guaranteed to exist (it's created in each account/region when you create an RDS resource with encryption and don't specify an explicit KMS key).
Create separate keys for storage and performance insights, adding a hardcoded policy to both (copied from aws/rds). Output the key ARNs as well.
Finally, split KMS logic out to a separate file, because the policy is so large it makes main.tf hard to read otherwise.
This pull request is now in conflict. Could you fix it @alexjurkiewicz? 🙏
This pull request is now in conflict. Could you fix it @alexjurkiewicz? 🙏
@alexjurkiewicz
TestExamplesComplete 2021-10-18T19:34:21Z command.go:121: on ../../kms.tf line 90, in module "performance_insights_kms_key":
TestExamplesComplete 2021-10-18T19:34:21Z command.go:121: 90: for_each = local.storage_create_kms_key ? [1] : []
This pull request is now in conflict. Could you fix it @alexjurkiewicz? 🙏
This pull request is now in conflict. Could you fix it @alexjurkiewicz? 🙏
@alexjurkiewicz friendly ping on this one -- Still want to move this forward or should we close it?
