-
-
Notifications
You must be signed in to change notification settings - Fork 178
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
feat: support enabling bucket key encryption #176
Conversation
💥 This pull request now has conflicts. Could you fix it @tiagoposse? 🙏 |
conflict was fixed and requested changes implemented |
anything missing? |
/terratest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah typo in the type
. Please commit the changes and re-run the make readme
workflow locally + push the changes. Thanks!
@tiagoposse this is annoying and stupid, but can you add a description to all variables in the
|
@tiagoposse Thank you for this PR. I am, however, not inclined to accept it, because bucket keys add complexity, reduce auditability, and for the kind of usage Terraform makes of the S3 bucket, makes practically no difference in cost or availability. IIRC, Terraform makes 1 object read and 1 object write for each plan or apply. Maybe I'm wrong and it makes 4 reads and 4 writes due to locks and intermediate steps, for a total of 8 KMS operations. Then maybe, if you are using Cloud Posse modules that cause reads of the S3-stored state of other root modules, we can get up to 20. In the biggest usage I have seen, a company runs about 1,600 In exchange for saving $0.10, you lose CloudTrail access logs and the ability to scope KMS permissions to the object level, because when using bucket keys, those things happen at the bucket level. See Changes to note before enabling an S3 Bucket Key for more details. The real use case for bucket keys is when you are storing something like a data lake with millions of Parquet files and uploading large numbers of files in a batch, to the point where you are running into the KMS API rate limits (10,000 to 100,000 per second). By contrast, Terraform uses so few operations that, in my opinion, a bucket key is just not an option worth supporting. In fact, having it as an option seems counter-productive because it then becomes another thing people have to think about and make a decision about. |
Thank you for this @Nuru. Your points make sense and I do agree that losing access logs is not worth it to save 10c. This PR was purely to offer the option for smaller use cases (we for example use this in buckets hosting states for the development environment). But I see how this can increase complexity and create confusion. If the final decision is to not accept it, than so be it. If the final decision is to accept it, I'll happily perform the requested changes. |
Closing, because we are not going to add this feature for reasons explained above. |
what
Allow the user to enable the usage of a bucket key when using server side encryption
why
Helps reduce costs when using s3 backend
references