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

wip: Handle sinsp::next errors in the inspect loop #746

Closed
wants to merge 1 commit into from

Conversation

fntlnz
Copy link
Contributor

@fntlnz fntlnz commented Jul 31, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:

The problem was that when entering the event processing loop for sinsp events
sinsp calls the sinsp_parser that when doing the process_event call throws an exception like the one observed by OP in #683 .
While an error while parsing an event from sinsp is certainly representing a problem for the event itself it should not stop falco from operating its service.

This PR adds an error detection mechanism to allow falco to log when errors in handling the current event happen while calling sinsp::next, in that way the user
is informed with a log of type error.

This kind of behavior should be more common when dealing with non syscall events like container information coming from a webserver or a socket (like the CRI implementation) because
the service might not be able to respond correctly to a request or the parser can't parse the response.

Which issue(s) this PR fixes:

This is related to #683 - it doesn't actually fix the underlaying problem of not being able to parse huge json files but at least solves the problem of having falco crashing when that happens.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Instead of crashing, now Falco will report the error when an internal error occurs while handling an event to be inspected. The log line will be of type error and will contain the string `Error handling inspector event`

Signed-off-by: Lorenzo Fontana <lo@linux.com>
@poiana
Copy link

poiana commented Jul 31, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: fntlnz

If they are not already assigned, you can assign the PR to them by writing /assign @fntlnz in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fntlnz
Copy link
Contributor Author

fntlnz commented Jul 31, 2019

/hold

@fntlnz
Copy link
Contributor Author

fntlnz commented Jul 31, 2019

While this is a solution to the problem, the fact that the sinsp exceptions are so generic (there's only one of them) doesn't allow us to rethrow the ones that should make falco die in the end and be handled by the main program loop.

@fntlnz
Copy link
Contributor Author

fntlnz commented Jul 31, 2019

This can't be merged in the way it is because sinsp_exception is too generic to be catched so we will need to propose a change in sinsp to make a parser specific exception for parser errors.

The parser in sinsp is located here: https://github.com/draios/sysdig/blob/dev/userspace/libsinsp/parsers.cpp

@fntlnz fntlnz changed the title Handle sinsp::next errors in the inspect loop wip: Handle sinsp::next errors in the inspect loop Jul 31, 2019
@leodido
Copy link
Member

leodido commented Aug 1, 2019

I agree, we need a exception hierarchy in libsinsp so to be able to distinguish them here.

@fntlnz fntlnz added this to the 0.18.0 milestone Aug 22, 2019
@fntlnz fntlnz self-assigned this Aug 23, 2019
@leodido leodido modified the milestones: 0.18.0, 0.19.0 Oct 3, 2019
@fntlnz
Copy link
Contributor Author

fntlnz commented Oct 8, 2019

Closing this in favor of #758

@leodido leodido closed this Oct 8, 2019
@fntlnz fntlnz removed this from the 0.19.0 milestone Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants