Fixes #799: AWS: filter ebs snapshots to just the current account and used snapshots (#793) #799
Conversation
Co-authored-by: Dallas Kaman <dallas.kaman@praetorian.com>
amlweems
force-pushed
the
ebs-snapshot-filter
branch
from
0818176
to
3b8ea97
Compare
|
I agree with this. It may be good as a followup later to also inspect the graph for EBS snapshots that aren't owned by the account, but are in use, to have fuller coverage. |
|
@amlweems Thank you! @ryan-lane Yes, it may be a better idea to filter to also include EBSVolumes -> EBSSnapshots in use by owned EC2Instances |
Yes, this looks good! I agree with @ryan-lane that we could also have a future PR in which we can populate EBS Snapshots not owned by the account but in use. I'd like @mpurusottamc to take a look at this as well since I had opened the PR for EBS Snapshots during my time at Cloudanix. Once the CLA check is okay, we can merge! |
|
@kedarghule I disagree, in that this PR should include both owned snapshots and snapshots in use by owned instances. This PR with just the filter for owned snapshots, I think, would be too much of a regression. |
Oh okay. I see your reasoning and I am inclined to agree. I do agree that we should show the snapshots in use by owned instances too. |
|
@kedarghule This change makes a lot of sense. It was a miss on our side during the initial checkin. Thanks @amlweems for correcting this. |
|
@amlweems Are you able to update this PR to include both owned snapshots and snapshots in use by owned instances? |
|
@ramonpetgrave64 Sure thing! I've just pushed a commit that adds a check for snapshots in use and fetches those in addition to any self owned snapshots. |
Thank you! |
|
@ramonpetgrave64 Thanks for the review! I've made both changes and signed the CLA. |
tests/integration/cartography/intel/aws/ec2/test_ec2_snapshots.py
Outdated
Show resolved
Hide resolved
tests/integration/cartography/intel/aws/ec2/test_ec2_snapshots.py
Outdated
Show resolved
Hide resolved
|
Also please update your branch with the latest from the main branch. |
|
@ramonpetgrave64 👍 fixed the non-blockers and updated with main |
tests/integration/cartography/intel/aws/ec2/test_ec2_snapshots.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com>
ramonpetgrave64
changed the title
ramonpetgrave64
changed the title
|
@amlweems Thank you again! Ideas, suggestions, and contributions are always welcome. |
* filter ebs snapshots to just the current account (lyft#793) Co-authored-by: Dallas Kaman <dallas.kaman@praetorian.com> * add snapshots_in_use to get_snapshots * filter duplicate snapshots in get_snapshots * add integration test for get_snapshots_in_use * small var name/comment updates * Update tests/integration/cartography/intel/aws/ec2/test_ec2_snapshots.py Co-authored-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com> * update from linter Co-authored-by: Dallas Kaman <dallas.kaman@praetorian.com> Co-authored-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com>
The
describe_snapshotscall returns approximately 40k public snapshots which creates extra nodes / relationships and generally causes performances issues with the graph. This PR filters snapshots byOwnerId`"self". This parameter is documented here and returns snapshots owned or explicitly granted to the caller's account.