-
Notifications
You must be signed in to change notification settings - Fork 151
Added functionality to clean-up old Object Storage keys created by linode-cli #824
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
Added functionality to clean-up old Object Storage keys created by linode-cli #824
Conversation
31ebdaa to
2826976
Compare
2148d91 to
5c7d0a3
Compare
3096e0c to
cc71d06
Compare
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.
Pull Request Overview
This PR implements automatic cleanup and rotation of object storage access keys created by linode-cli. The cleanup runs once every 24 hours (by default) before any object storage operation, removing keys older than 30 days and rotating keys older than 10 days. Users can configure the cleanup behavior or disable it entirely.
- Adds time-based key cleanup and rotation logic with configurable lifespan and rotation periods
- Introduces new CLI argument
--skip-key-cleanupto bypass the automatic cleanup process - Adds comprehensive integration tests covering cleanup, rotation, and conditional execution scenarios
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/integration/obj/test_obj_plugin.py | Adds integration tests for key cleanup, rotation, and conditional execution based on configuration |
| pyproject.toml | Adds pytimeparse dependency and pins packaging and rich versions |
| linodecli/plugins/obj/init.py | Implements key cleanup and rotation logic with configuration support |
| linodecli/configuration/config.py | Adds helper methods for plugin configuration management including type-safe value retrieval |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f35abb5 to
913c3e9
Compare
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.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
ezilber-akamai
left a comment
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.
Not sure if this is an issue with the way I am testing, but the Object Storage Key gets deleted and replaced when I follow the test steps in the description. This is the procedure I am following:
- Run
linode-cli obj ls - Observe that a key has been created in Cloud Manager
- Observe the contents of ~/.config/linode-cli
- The following fields are populated in this file
plugin-obj-last-key-cleanup-timestampplugin-obj-access-keyplugin-obj-secret-keyplugin-obj-cluster
- The following field does NOT show up in this file:
plugin-obj-key-lifespan
- The following fields are populated in this file
- Then I run this command:
linode-cli obj ls --force-key-cleanup --key-lifespan 10s- The CLI output suggests that the key has been deleted, but when I look in CM it seems that a new key appears in its place. Is this the intended behavior?
Is there something wrong with my procedure? Also, as a side note, whenever I run linode-cli obj ls, the output is just a blank line even if I have a key.
|
Hi @ezilber-akamai, this is expected behavior, the keys are cleaned up before executing the operation. If the key-lifespan is set to 10s, it means it will remove all access keys generated for that customer older than 10s, including the one generated by your linode-cli, so it will need to generate a new one (the default lifespan is 30 days). BTW, it should not rotate the access key if you set plugin-obj-access-key+plugin-obj-secret-key to a key that you created manually (so it doesn't have the "linode-cli-..." naming pattern). |
Understood. In that case it sounds like it is working properly. I will give a final review today. Thanks for clarifying! |
|
This implementation looks great, although I'm seeing the following test failures when running them locally: Since these tests are making heavy use of mocks, I'm wondering if the mocking isn't playing nice with the the subprocesses created by Would it make sense to move these tests to |
|
Hey @skulpok-akamai ! Just bumping my above message in case you missed it 👍 |
|
Just pushed up the testing changes recommended above as discussed in private. We'll just need a re-review from the DX team and this should be set. Thanks for the contribution! |
tests/unit/test_plugin_obj.py
Outdated
| assert helpers._denominate(1e23) == "90949470177.29 TB" | ||
|
|
||
|
|
||
| def test_obj_action_triggers_key_cleanup_and_deletes_stale_key( |
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.
These tests have a good bit of redundant logic that could be shared, but I think it's better for it to be implemented in every test here just for clarity.
|
|
||
| # When enabled, pylint would attempt to guess common misconfiguration and emit | ||
| # user-friendly hints instead of false-positive error messages. | ||
| suggestion-mode=yes |
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.
This is longer supported in pylint causing an error
|
Hi @lgarber-akamai, sorry, I’m on leave until 2nd January and have very limited availability. I see that you improved the tests, thanks for that! I also see that the PR has now been merged, which is great. I can work on further improving the tests when I’m back from leave if there’s still a need, I’ll get in touch then. |
📝 Description
This change adds automatic clean-up of object storage access keys created by linode-cli that are older than 30 days (by default). The clean-up runs once every 24 hours and is triggered before any object storage operation. This helps to remove stale keys and improve security by ensuring old, unused keys are regularly deleted.
✔️ How to Test
What are the steps to reproduce the issue or verify the changes?
linode-cli obj ls)