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.

Sign up for GitHub

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

Jump to bottom

Handle Access denied errors when listing resources by triggering alerts #152

Merged
merged 2 commits into from
Jan 29, 2021
Merged

Handle Access denied errors when listing resources by triggering alerts #152

merged 2 commits into from
Jan 29, 2021

Conversation

moadibfr
Copy link
Contributor

@moadibfr moadibfr commented Jan 25, 2021 •
edited
Loading

Q A
πŸ› Bug fix? no
πŸš€ New feature? yes
⚠ Deprecations? no
❌ BC Break yes
πŸ”— Related issues #9
❓ Documentation no

Description

Send alert and ignore resource when driftctl doesn't have the rights to list them

Screenshot from 2021-01-27 19-15-37

@moadibfr moadibfr requested a review from a team as a code owner January 25, 2021 17:15
@moadibfr moadibfr marked this pull request as draft January 25, 2021 17:15
@codecov
Copy link

codecov bot commented Jan 25, 2021 •
edited
Loading

Codecov Report

Merging #152 (61b81c2) into main (43dcb86) will increase coverage by 1.93%.
The diff coverage is 34.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #152      +/-   ##
==========================================
+ Coverage   67.79%   69.72%   +1.93%     
==========================================
  Files         188      190       +2     
  Lines        4241     4364     +123     
==========================================
+ Hits         2875     3043     +168     
- Misses       1055     1081      +26     
+ Partials      311      240      -71
Impacted Files Coverage Ξ”
pkg/remote/aws/init.go 0.00% <ΓΈ> (ΓΈ)
pkg/scanner.go 0.00% <0.00%> (ΓΈ)
pkg/remote/aws/db_instance_supplier.go 66.66% <16.66%> (+7.40%) ⬆️
pkg/remote/aws/ec2_ami_supplier.go 66.66% <16.66%> (-2.57%) ⬇️
pkg/remote/aws/ec2_ebs_snapshot_supplier.go 74.35% <16.66%> (+2.93%) ⬆️
pkg/remote/aws/ec2_ebs_volume_supplier.go 72.22% <16.66%> (+3.47%) ⬆️
pkg/remote/aws/ec2_eip_association_supplier.go 68.75% <16.66%> (+4.46%) ⬆️
pkg/remote/aws/ec2_eip_supplier.go 68.75% <16.66%> (+4.46%) ⬆️
pkg/remote/aws/ec2_instance_supplier.go 73.68% <16.66%> (+3.09%) ⬆️
pkg/remote/aws/ec2_key_pair_supplier.go 62.96% <16.66%> (-2.26%) ⬇️
... and 52 more

@moadibfr moadibfr force-pushed the fea/alert_res_list branch 2 times, most recently from 88c3d56 to cf0f033 Compare January 25, 2021 18:32
eliecharra
eliecharra reviewed Jan 26, 2021
View reviewed changes
Copy link
Contributor

@eliecharra eliecharra left a comment

Choose a reason for hiding this comment

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

First pass and got and architectural question

pkg/remote/aws/aws_error_handler.go Outdated Show resolved Hide resolved
pkg/remote/aws/db_instance_supplier.go Outdated Show resolved Hide resolved
pkg/remote/aws/db_instance_supplier_test.go Show resolved Hide resolved
@moadibfr moadibfr marked this pull request as ready for review January 27, 2021 18:06
@moadibfr moadibfr force-pushed the fea/alert_res_list branch 2 times, most recently from de179a2 to 6f116cb Compare January 27, 2021 18:15
eliecharra
eliecharra suggested changes Jan 28, 2021
View reviewed changes
pkg/error/aws_list_error_handler.go Outdated Show resolved Hide resolved
pkg/remote/aws/error.go Outdated Show resolved Hide resolved
pkg/remote/aws/error.go Outdated Show resolved Hide resolved
go.sum Outdated Show resolved Hide resolved
pkg/error/aws_list_error_handler.go Outdated Show resolved Hide resolved
pkg/remote/aws/error.go Outdated Show resolved Hide resolved
pkg/remote/aws/error.go Outdated Show resolved Hide resolved
pkg/remote/aws/iam_access_key_supplier_test.go Show resolved Hide resolved
pkg/remote/aws/iam_role_supplier.go Show resolved Hide resolved
pkg/remote/aws/route_table_supplier.go Show resolved Hide resolved
@moadibfr moadibfr mentioned this pull request Jan 28, 2021
Closed
wbeuil
wbeuil reviewed Jan 28, 2021
View reviewed changes
pkg/error/aws_list_error_handler.go Outdated Show resolved Hide resolved
pkg/error/aws_list_error_handler.go Outdated Show resolved Hide resolved
pkg/remote/aws/db_instance_supplier_test.go Show resolved Hide resolved
pkg/remote/aws/error.go Outdated Show resolved Hide resolved
pkg/remote/aws/error.go Outdated Show resolved Hide resolved
pkg/remote/aws/error.go Outdated Show resolved Hide resolved
pkg/remote/aws/s3_bucket_notification_supplier.go Outdated Show resolved Hide resolved
eliecharra
eliecharra suggested changes Jan 28, 2021
View reviewed changes
Copy link
Contributor

@eliecharra eliecharra left a comment

Choose a reason for hiding this comment

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

Awesome, almost good

pkg/remote/aws/iam_user_policy_supplier.go Outdated Show resolved Hide resolved
pkg/remote/resource_list_error_handler.go Outdated Show resolved Hide resolved
eliecharra
eliecharra suggested changes Jan 28, 2021
View reviewed changes
Copy link
Contributor

@eliecharra eliecharra left a comment

Choose a reason for hiding this comment

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

One test case to fix, rename file to reflect changes on HandleResourceEnumerationError naming and we are good to go !

@eliecharra eliecharra added the kind/enhancement New feature or improvement label Jan 28, 2021
@eliecharra eliecharra changed the title alert and ignore when listing resources is forbidden Handle Access denied errors when listing resources by triggering alerts Jan 28, 2021
@eliecharra eliecharra merged commit 5be7f5b into main Jan 29, 2021
@eliecharra eliecharra deleted the fea/alert_res_list branch January 29, 2021 15:31
@eliecharra eliecharra linked an issue Jan 29, 2021 that may be closed by this pull request
Insufficient permissions trigger an alert #9
Closed
@eliecharra eliecharra added this to the v0.4.0 milestone Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eliecharra eliecharra eliecharra approved these changes

@wbeuil wbeuil Awaiting requested review from wbeuil

Assignees
No one assigned
Labels
kind/enhancement New feature or improvement
Projects
None yet
Milestone
v0.4.0
Development

Successfully merging this pull request may close these issues.

Insufficient permissions trigger an alert
3 participants
@moadibfr @eliecharra @wbeuil