Conversation
Co-authored-by: jdettmannnava <145699825+jdettmannnava@users.noreply.github.com>
Co-authored-by: mianava <miaferguson@navapbc.com>
jdettmannnava
changed the title
lukey-luke
left a comment
lukey-luke
left a comment
There was a problem hiding this comment.
Thanks for working on this! I left a few comments here. I've also moved out the parameter store piece to a separate ticket per discussion this morning - DPC-5291
| Cleans up old ECR images while protecting images referenced by currently running ECS tasks. | ||
| """ | ||
|
|
||
| from argparse import ArgumentParser |
There was a problem hiding this comment.
Can you run pylint and pytest over the python files?
| protected = get_protected_image_refs(ecs_client) | ||
| for repo_name, strategies in REPO_STRATEGIES.items(): | ||
| if repo in ('all', repo_name): | ||
| print(repo_name) | ||
| for image in get_images_to_delete(ecr_client, repo_name, strategies, protected): | ||
| print(f' {image.tags or image.digest}') |
There was a problem hiding this comment.
Consider moving this to a shared function so the same code is used for lambda_handler. This would DRY up the code and ensure same code that is being dry run is used in production
| DPC_STRATEGIES = ( | ||
| (count_image_strategy, 'rls-r', 5,), | ||
| (days_older_than_strategy, '', 14,), | ||
| (days_older_than_strategy, None, 14,), |
There was a problem hiding this comment.
nit: was not immediately obvious what None meant here. This should cover untagged images, but renaming to something like tag_prefix would make it more clear
| 'repos': list(protected_refs)}) | ||
|
|
||
| for repo in all_repos: | ||
| for repo_name, strategies in REPO_STRATEGIES.items(): |
There was a problem hiding this comment.
This will result in only repositories that are listed in REPO_STRATEGIES getting logged.
Image deletion for this lambda-based approach should absolutely be opt in. However, if there are other repositories that are old, it might be helpful to still log these at an account-level and opt in more repositories.
I think there might still be value in retaining a baseline policy for sweeping images across the account that are not opted in, but older than a certain threshold. Even though it doesn't impact what runs for DPC for example, it does impact security scan results for the account, so I think it would be good to flag.
You could achieve this by using get_all_repos() and then filtering based on REPO_STRATEGIES. E.g. add another class state option
DELETE, PROTECT, or NOT_OPTED_IN
| 'dpc-web': DPC_STRATEGIES, | ||
| 'dpc-web-admin': DPC_STRATEGIES, | ||
| 'dpc-web-portal': DPC_STRATEGIES, | ||
| } |
There was a problem hiding this comment.
Per discussion this morning, managing the list in Python code should work just fine. It would probably be easier to access if these constants are moved to the top of code or a separate file and then flagged in README.md. This way any other team, know exactly where to look and can add their repositories
| ### To set up lifecycles for a given repository | ||
| - Add the repository name as a key in the REPO_STRATEGIES dictionary | ||
| - Add a tuple as the value for this key | ||
| - For each lifecycle, add the function (along with additional arguments) to the tuple. These functions will run in order. |
There was a problem hiding this comment.
👍 Thanks for updating the README to reflect new constants
2 participants
🎫 Ticket
https://jira.cms.gov/browse/DPC-5263
🛠 Changes
ℹ️ Context
The existing code for cleaning up images in ECR repositories did not match current needs.
🧪 Validation
count_image_strategyto 3 images on prod and worked as expected. (It only removed the oldest image, as the image protected by running is not included in the count.)days_older_than_strategyto 10 days (in test) and compared the results ofdpc-attributionto the images AWS would delete if the same change was made there. All images matched, except for one image that was currently running and was protected by the script.