Skip to content
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

aws - convert key alias to key id before cache lookup #8505

Merged
merged 16 commits into from May 3, 2023

Conversation

PratMis
Copy link
Collaborator

@PratMis PratMis commented Apr 19, 2023

Closes #8504

@PratMis PratMis requested a review from kapilt as a code owner April 19, 2023 03:42
@PratMis PratMis marked this pull request as draft April 19, 2023 04:13
@PratMis PratMis marked this pull request as ready for review April 19, 2023 17:18
Copy link
Member

@ajkerrigan ajkerrigan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, looks good 👍 . I added a test and made one tweak (comment inline), feedback or alternatives welcome on that.

c7n/filters/kms.py Outdated Show resolved Hide resolved
@PratMis
Copy link
Collaborator Author

PratMis commented Apr 27, 2023

Thanks for this, looks good 👍 . I added a test and made one tweak (comment inline), feedback or alternatives welcome on that.

Thanks @ajkerrigan , looks good to me. Sorry, I should have added a test 😄 . Also, the test failure here seems unrelated. Great idea of creating a new flipped dict. I guess it might come handy later

@ajkerrigan
Copy link
Member

Thanks for this, looks good +1 . I added a test and made one tweak (comment inline), feedback or alternatives welcome on that.

Thanks @ajkerrigan , looks good to me. Sorry, I should have added a test smile . Also, the test failure here seems unrelated. Great idea of creating a new flipped dict. I guess it might come handy later

Looks like with this change, we avoid some cases where we'd otherwise need to list all aliases. That secrets manager test had 2 list_aliases calls but only needed to use 1.

...but that might mess with cache reuse. Need to do some more testing with this sorry :-/

@PratMis
Copy link
Collaborator Author

PratMis commented May 1, 2023

Thanks for the feedback and changes @ajkerrigan . Let me get back to you on this. I'll test it in our environment to find out if its breaking something.

@PratMis
Copy link
Collaborator Author

PratMis commented May 3, 2023

Ok, I went ahead and tested this. The original issue doesn't seem to be happening anymore. When running it against a bunch of accounts, I also didn't see any errors.

@ajkerrigan
Copy link
Member

ajkerrigan commented May 3, 2023

Ok, I went ahead and tested this. The original issue doesn't seem to be happening anymore. When running it against a bunch of accounts, I also didn't see any errors.

That's great, thanks for confirming. I also added a test for that specific cache-related issue. Without the changes in this PR it fails and throws those warnings:

WARNING  custodian.filters:related.py:77 Resource sqs:https://sqs.us-east-1.amazonaws.com/644160558196/test-kms-cache-custom-key
references non existant Key: alias/kms-cache-check  

@ajkerrigan ajkerrigan merged commit 25241bd into cloud-custodian:main May 3, 2023
21 checks passed
@PratMis
Copy link
Collaborator Author

PratMis commented May 3, 2023

Thanks @ajkerrigan, learned something new about testing with cache enabled 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache lookup doesn't return correct results with kms key alias
2 participants