KMS: on-demand key rotation #12342
KMS: on-demand key rotation #12342
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.
|
I have read the CLA Document and I hereby sign the CLA |
…n automatic rotation is enabled and disabled.
There was a problem hiding this comment.
Nice work! This is great, thanks for adding a new operation and improving parity! Great set of snapshot tests.
I have a few comments about the new attribute on the key that could maybe be on the store itself, another about returning None values from the provider, and maybe we could add a bit more snapshot match in order to reduce the manual assert.
Thanks again for the very thorough testing! 🚀
…n `OnDemandRotationStartDate` only while it's "in progress"
| @@ -284,6 +284,7 @@ def __init__( | |||
| self.crypto_key = KmsCryptoKey(self.metadata.get("KeySpec"), custom_key_material) | |||
| self.rotation_period_in_days = 365 | |||
| self.next_rotation_date = None | |||
| self.on_demand_rotation_start_date = None | |||
There was a problem hiding this comment.
For simplicity current implementation is limited to tracking a single on demand rotation.
As part of implementing ListKeyRotations, the model can be evolved to track many.
There was a problem hiding this comment.
After some discussion we've decided not to replicate AWS behavior for on_demand_rotation_start_date, which means LocalStack will not return this field. Since we don't consider it essential to emulate and it's only used in the get_key_rotation_status function, please remove support for it from your code, along with all related tests.
To fix tests you can use poll_condition to wait until OnDemandRotationStartDate is no longer in the response, just like you did in the test_rotate_key_on_demand_should_return_rotation_start_date_only_while_its_in_progress.
There was a problem hiding this comment.
This, along with @bentsku comment, are the last two things we need to address. After that, we're good to merge the PR 🚢
…ecessary to do it explicitly in `rotate_key_on_demand()`.
Motivation
Motivated by this enhancement request: #10723
Changes
Implementation of the on-demand key rotation feature in KMS.
As per AWS's specs, on-demand key rotation is only allowed for:
TODO
What's left to do:
ListKeyRotationsmay be required too, because it'd allow to properly test this functionality)