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 - rds - add consecutive daily snapshot count filter #7190

Merged
merged 6 commits into from
Mar 31, 2022

Conversation

jasgrover
Copy link
Contributor

Adding a new filter for 'consecutive-snapshots' daily snapshots to rds and rdscluster resources so that we can determine the number of consecutive daily snapshot each resource of above mentioned type has . The compliance reporting requires that all RDS ( cluster and instance ) resources should have at least 7 days worth of backups. This filter allows to calculate the number of daily consecutive snapshots.

Sample policy

     - name: rds-backup-test-daily
       resource: rds
       filters:
             - type: consecutive-snapshots
               days: 7
             - DBInstanceStatus: available
       actions:
          - type: notify
        

I had to close the previous PR (referenced below ) and recreate this PR to resolve the CLA issues with commit ids. Please see the following CR for notes /review by @ajkerrigan and @darrendao.

#7165

@jasgrover jasgrover requested a review from kapilt as a code owner March 29, 2022 21:36
@jasgrover jasgrover changed the title recreated fork and changes to resolve CLA issues aws - rds - add consecutive daily snapshot count filter Mar 29, 2022
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.

Glad to see you got the CLA issues sorted out (apologies for the hassle - we should be able to trigger CLA checks without forcing you to open a new PR!).

This looks good, thanks! Just a couple nits and a question inline. And it looks like there's an extra pair of recorded test data files for test_rds_snapshot_count_filter that got nested under the test_rdscluster_snapshot_count_filter directory (here and here).

c7n/resources/rds.py Outdated Show resolved Hide resolved
c7n/resources/rds.py Show resolved Hide resolved
c7n/resources/rds.py Outdated Show resolved Hide resolved
c7n/resources/rds.py Outdated Show resolved Hide resolved
c7n/resources/rds.py Outdated Show resolved Hide resolved
@jasgrover
Copy link
Contributor Author

@ajkerrigan The reason tests are failing because the filter looks for n (2) number of snapshots going back from present date and the test replay data becomes obsolete after passing of the day from commit... reckon will have to reframe the test.

@jasgrover
Copy link
Contributor Author

@ajkerrigan .. fixed the tests

@ajkerrigan
Copy link
Member

@ajkerrigan .. fixed the tests

I think you had the right idea validating matches in your original tests, but like you discovered they were making assumptions about the current date. A frozen date should let us keep your original test logic.

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 your work on this @jasgrover , and @darrendao for the assist. Looks good! 👍

@ajkerrigan ajkerrigan merged commit 74b9cbe into cloud-custodian:master Mar 31, 2022
@jasgrover
Copy link
Contributor Author

Thanks for your work on this @jasgrover , and @darrendao for the assist. Looks good! 👍

Many thanks for the excellent advise and assistance all the way !!

@jasgrover jasgrover deleted the rds_snapcount branch April 1, 2022 16:22
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.

2 participants