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

Added exception for security update error handling. #9977

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+30 −1
Open

Added exception for security update error handling. #9977

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
66e21c6
Added exception for security update error handling.
sachin-sandhu Jun 12, 2024
d658b26
Linting and fixes.
sachin-sandhu Jun 12, 2024
098fe4c
exception backtrace setup and error context assignment
sachin-sandhu Jun 13, 2024
6b80e43
Add exception to security update
sachin-sandhu Jun 18, 2024
d3ed7ea
Add exception to security update
sachin-sandhu Jun 18, 2024
28f3b30
Fixes formatting and tests
sachin-sandhu Jun 23, 2024
cdc9c91
Fixes dependabot file updater docker error.
sachin-sandhu Jun 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions updater/lib/dependabot/updater/operations/create_security_update_pull_request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,17 @@ module Operations
class CreateSecurityUpdatePullRequest
include SecurityUpdateHelpers

class SecurityUpdateError < StandardError
Copy link
Member

Choose a reason for hiding this comment

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

should we call this TransitiveSecurityUpdateError for specificity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i matched with class name convention, specificity would be a good idea for sure. ill wait for someone to review once to be sure in case im missing anything.

def initialize(message:, error_context:)
super(message)
@error_context = error_context
end

def sentry_context
{ extra: @error_context }
end
end

def self.applies_to?(job:)
return false if job.updating_a_pull_request?
# If we haven't been given data for the vulnerable dependency,
Expand Down Expand Up @@ -69,6 +80,15 @@ def check_and_create_pr_with_error_handling(dependency)
error_type: "inconsistent_registry_response",
Copy link
Member

Choose a reason for hiding this comment

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

I think the error is surfaced here.

can we detect the actual transitive failure mode and add a specific error class for that scenario? That way we are not swallowing up all errors.

I would expect something like this:

raise Dependabot::SecurityUpdateFailureDueToTransitiveDependency.new(...
...

That way, we can ensure we are actually separating out that error category.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abdulapopoola , im working on this

error_detail: e.message
)
rescue StandardError
log_error_for_security_update_failure(dependency: dependency)
end

def log_error_for_security_update_failure(dependency:)
raise SecurityUpdateError.new(
message: "Security Update Error, No files were updated!",
Copy link
Member

Choose a reason for hiding this comment

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

🤔 Rescuing StandardError is a pretty broad approach since it will catch all subclasses of StandardError. That would mean that almost any error that is encountered in #check_and_create_pr_with_error_handling will be logged as SecurityUpdateError, "Security Update Error, No files were updated!".

I think it would be wise to preserve some context. Maybe that happens in the message (i.e. we include the original error's class and/or message), or maybe that's passed along in the error_context.

Copy link
Contributor Author

@sachin-sandhu sachin-sandhu Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@landongrindheim , i have redone the code , earlier issue was that due to a double exception throw (StandardError and then SecurityUpdateError), there was an issue related with double log of sentry issue. so i didn't opt for a double exception.

I have raised a new exception class (SecurityUpdateError) and maintained the context info from thrown exception that should maintain the error_context from older exception. I have also setup the exception backtrack from SecurityUpdateError to thrown exception which should maintain the stack trace. It should start logging security updates error under new category.

image

Copy link
Member

Choose a reason for hiding this comment

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

I think what Landon was saying is still the case, any error that raises here will now be raised as a SecurityUpdateError instead of its native error class?

error_context: dependency
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand what dependency provides in terms of context. Could you share what we'd be sending to an error tracking service and how it would help us and/or users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@landongrindheim , i have changed error context to back to thrown exception object. it should maintain the older context information but with a newer exception class.

)
rescue StandardError => e
error_handler.handle_dependency_error(error: e, dependency: dependency)
end
Expand Down
Loading