fix: SSE-S3 example #132
fix: SSE-S3 example #132
Conversation
zacharyblasczyk
force-pushed
the
sse-s3-example
branch
from
e3544aa
to
3b4ad37
Compare
| @@ -37,7 +37,7 @@ module "file_storage" { | |||
| source = "../../modules/file_storage" | |||
|
|
|||
| namespace = var.namespace | |||
| sse_algorithm = "aws:kms" | |||
| sse_algorithm = var.sse_algorithm | |||
| kms_key_arn = var.create_kms_key ? aws_kms_key.key[0].arn : null | |||
There was a problem hiding this comment.
suggestion: when sse_algorithm is not aws:kms, it would a bit nicer to automatically disable the KMS key without requiring to also pass create_kms_key=false.
in the key resource and in the output with the key ARN, var.create_kms_key could be replaced by
locals {
should_create_kms_key = var.sse_algorithm == "aws:kms" && var.create_kms_key
}or similar
There was a problem hiding this comment.
I like this recommendation, but we have some use cases where organizations are using this module and bringing their own keys. For now this is the least breaking change.
There was a problem hiding this comment.
What would the breaking change be? Someone that had set sse_algorithm to AES256 and ended up with KMS-encryption with the default key?
There was a problem hiding this comment.
Rereading that code, I think it would be fine potentially for now.
However, there is a third option in the v5 module aws:kms:dsse which we will be upgrading to at some point. We would then need to reevaluate that ternary logic you proposed.
Having the user explicitly set var.create_kms_key is our opinionated approach to communicate expected behavior for now and unblocks us.
### [2.4.2](v2.4.1...v2.4.2) (2023-08-30) ### Bug Fixes * SSE-S3 example ([#132](#132)) ([627005b](627005b))
|
This PR is included in version 2.4.2 🎉 |
Now passing the
var.sse_algorithmin the file storage module which defaults to "aws:kms".Also added a sse-s3 example for customers that don't want to deal with kms.