-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add Terminator support for KMS and allow KMS in the CI account #106
Conversation
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.
I chatted with mattclay about this one and we have some questions about kms pricing (the docs are a little vague on whether the free tier includes CreateKey requests). We've opened a support case and are waiting on that before we want to proceed with this PR.
Closing and re-opening to update CI status. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@mattclay Got a reply from AWS Support, the KMS 20k request free tier includes key creation, which should cover our normal CI usage. We don't currently make any of the excluded calls that I can find.
|
@jillr So as long as all of our KMS requests in a year (not just create_key) stay within the 20K free-tier we shouldn't see any charges? Do we have an idea of how many requests we'll be making per test run? |
@mattclay Note: The free tier is 20k/month rather than per year. Running terminator every 10 minutes (4000 times a month) is going to result in around 10k calls (list+list_aliases). By my count it's ~480 API calls per test-run (think I could get that down). But terminator will then need to describe that key every run for 7 days (a CMK can only be scheduled for deletion in 7 days, not deleted instantly). This will also then result in an additional 1000 describe API calls on the key. For a total of around 9000 API calls as a result of each aws_kms CI job. Once we're over 20k, it's 6¢ per 20k API calls. We're seeing around 200 (total) CI jobs per month, if all of these ran the KMS suite this would result in a charge of around $5/month. We could also switch it over to being marked as an 'unstable' test so it's only run when someone's making changes to the KMS modules themselves, at which point I suspect we'd be seeing something around the 30¢/month as a peak cost during months where someone actively works on the KMS modules |
@tremble Thanks for the correction and analysis. My concern is specifically with the CMK charges since they're higher than the others ($1/mo). As you pointed out, due to the number of terminator API calls, we will exceed the free-tier. Reading the pricing page (https://aws.amazon.com/kms/pricing/), the important details seem to be:
Based on that, it seems we'll pay for a CMK only for the 1 - 2 hours it exists and is not pending deletion. That last part about the deletion is something I missed in my earlier reading. So it does seem like the cost for this should be reasonable. |
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.
Tested against community.aws/pull/63, other than sns_topic (which is unsupported due to missing permissions in this repo) looks good. I don't think we need to hold up this change on making sns_topic supported.
@@ -32,7 +18,16 @@ Statement: | |||
- 'arn:aws:iam::aws:policy/service-role/AmazonEC2ContainerServiceRole' | |||
- 'arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole' | |||
|
|||
- Sid: AllowGlobalUnrestrictedResourceActionsWhichIncurNoFees | |||
# Legacy - We need to backport ansible-collections/community.aws/63 or | |||
# wait until community.aws drops CI support for Ansible 2.9 |
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.
@tremble We don't currently have a sunset timeline for 2.9 support, would you mind opening that backport when 63 merges? It'll ultimately be up to core to accept but I'd rather not have extraneous roles hanging around forever if we can avoid it.
A number of services support enabling encryption using KMS, ensure we can add test cases for it
For example
ansible-collections/community.aws#42