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

Include resource ID in scanning errors to ignore a particular resource #905

Merged
merged 3 commits into from Aug 4, 2021

Conversation

sundowndev
Copy link
Contributor

@sundowndev sundowndev commented Aug 2, 2021

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

Description

When we encounter a Access Denied error on a particular resource, it creates an alert and ignore all resources of the same type. With this fix, we create an alert including the resource type & ID so we can only ignore that one.

What changed

  • Moved RemoteAccessDeniedAlert into new pkg/remote/alerts package
  • Moved RemoteAWSTerraform & RemoteGithubTerraform into pkg/remote/common/providers.go
  • Deleted SupplierError and moved its methods into ResourceScanningError
  • Added fields resourceType & resourceName in ResourceScanningError
  • Added few unit tests to avoid regression and increase coverage

@sundowndev sundowndev added kind/bug Something isn't working priority/0 labels Aug 2, 2021
@codecov
Copy link

codecov bot commented Aug 2, 2021

Codecov Report

Merging #905 (4e3d352) into main (62e0efe) will increase coverage by 0.03%.
The diff coverage is 72.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #905      +/-   ##
==========================================
+ Coverage   81.66%   81.70%   +0.03%     
==========================================
  Files         232      233       +1     
  Lines        7566     7588      +22     
==========================================
+ Hits         6179     6200      +21     
- Misses       1159     1161       +2     
+ Partials      228      227       -1     
Impacted Files Coverage Ξ”
pkg/remote/aws/dynamodb_table_details_fetcher.go 77.77% <0.00%> (ΓΈ)
...ote/aws/ec2_default_route_table_details_fetcher.go 77.77% <0.00%> (ΓΈ)
pkg/remote/aws/ec2_route_details_fetcher.go 81.81% <0.00%> (ΓΈ)
...aws/ec2_route_table_association_details_fetcher.go 77.77% <0.00%> (ΓΈ)
pkg/remote/aws/iam_access_key_details_fetcher.go 77.77% <0.00%> (ΓΈ)
pkg/remote/aws/iam_role_enumerator.go 92.85% <0.00%> (ΓΈ)
.../aws/iam_role_policy_attachment_details_fetcher.go 78.94% <0.00%> (ΓΈ)
.../aws/iam_user_policy_attachment_details_fetcher.go 78.94% <0.00%> (ΓΈ)
pkg/remote/aws/init.go 0.00% <0.00%> (ΓΈ)
...kg/remote/aws/s3_bucket_analytic_detail_fetcher.go 77.77% <0.00%> (ΓΈ)
... and 75 more

@sundowndev sundowndev changed the title Use resource name for resource-specific errors Include resource ID in scanning errors to ignore a particular resource Aug 3, 2021
@sundowndev sundowndev marked this pull request as ready for review August 3, 2021 13:02
@sundowndev sundowndev requested a review from a team as a code owner August 3, 2021 13:02
@eliecharra eliecharra added this to the v0.14 milestone Aug 4, 2021
@eliecharra eliecharra added this to Review in driftctl Aug 4, 2021
eliecharra
eliecharra previously approved these changes Aug 4, 2021
Copy link
Contributor

@wbeuil wbeuil left a comment

Choose a reason for hiding this comment

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

Small changes to make

Copy link
Contributor

@wbeuil wbeuil left a comment

Choose a reason for hiding this comment

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

Nice work !

@sundowndev sundowndev merged commit 75cd450 into main Aug 4, 2021
driftctl automation moved this from Review to Done Aug 4, 2021
@sundowndev sundowndev deleted the fix/listedTypeAlerts branch August 4, 2021 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants