-
Notifications
You must be signed in to change notification settings - Fork 892
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
update: improve support and tests for live-capture event selection #2432
Conversation
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
…sc and new engine features Co-authored-by: Melissa Kilby <melissa.kilby.oss@gmail.com> Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
The test now take in accoint pre/post-conditions of the actions, usage of the -A option, and the newly-introduced base_syscall user configuration. This also makes sure that the event selection properly handles generic events and options/configs precedence. Co-authored-by: Melissa Kilby <melissa.kilby.oss@gmail.com> Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
…e103d57f97d74dd374f Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
/milestone 0.35.0 |
Co-authored-by: Melissa Kilby <melissa.kilby.oss@gmail.com> Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasondellaluce nice work! Given master is currently a tad flawed, would vote for merging rather faster and then polishing in a follow up PR if needed. WDYT?
|
||
// selected events are the union of the rules events set and the | ||
// base events set (either the default or the user-defined one) | ||
s.selected_sc_set = rules_sc_set.merge(base_sc_set); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how now that we reverted to starting from sc_set we could simplify and clean up "base_syscalls" right away and be more consistent 🎉
@@ -163,11 +170,15 @@ void evttype_index_ruleset::add( | |||
wrap->filter = filter; | |||
if(rule.source == falco_common::syscall_source) | |||
{ | |||
wrap->evttypes = libsinsp::filter::ast::ppm_event_codes(condition.get()); | |||
wrap->sc_codes = libsinsp::filter::ast::ppm_sc_codes(condition.get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see some more libs fixes in falcosecurity/libs@338bff0 that aim to make sure we handle the tough corner cases and special snow flakes. This is probably a perfect reflection of why we were going back and forth between first resolving the evt.type string names to event codes vs syscall codes ... meaning we need a combination of the two approaches after all.
Co-authored-by: Melissa Kilby <melissa.kilby.oss@gmail.com> Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Just noticed that |
This event selection process only concerns events that could be ignored at kernel-level, and for now we have no way to express the selection of non-syscall events (e.g. |
Thank you Jason. Fair to say it was never needed? https://github.com/falcosecurity/falco/blob/0.34.1/userspace/falco/app_actions/configure_interesting_sets.cpp#L38 |
At the point in time of 0.34.0/.1, it was needed for printing the output of
At the current time, storing this in the app state does not have value anymore, as the event set is not used only by the Falco engine. |
cc @falcosecurity/falco-maintainers for visibility. |
…75375154a58 Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Thanks @jasondellaluce, also for expanding the unit tests! |
@jasondellaluce as we bump libs one more suggesting to update falco engine version as we will have new fields available as well, ty! |
…7badbbd43e Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
…9960ac38c6 Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Note, the |
…5e53053412 Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
LGTM label has been added. Git tree hash: 920c332289d1cb0bfeeb389368eff1e51bf213d9
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP, jasondellaluce 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 |
What type of PR is this?
/kind bug
/kind design
Any specific area of the project related to this PR?
/area engine
/area tests
What this PR does / why we need it:
This follows up the contributions of #2428 by fixing a design issue, adapting the codebase to the latest updates in libsinsp, and by growing the unit tests for the part of code responsible for event selection during live captures.
~400 lines of code of this PR are for tests.
Which issue(s) this PR fixes:
Special notes for your reviewer:
A lot changed this week, so here's quick recap for how Falco deals with events:
syscall
, Falco will behave as usualsyscall
event source:base_syscalls
configuration field.-A
flag is not set, performance-heavy syscalls are forcefully removed from the selected set (such as IO-intensive syscalls)What was missing? The current implementation incorrectly uses the ppm_event_code enumerative as single source of truth for defining the set of selected events. This has some faults, the first being that there can be information loss about generic events. Instead, Falco needs to operate with both ppm_sc_code and ppm_event_code for their different purposes:
ppm_sc_code
is a richer enumerative that has no information loss about generic events and includes syscalls-related events only. The unit test suite now checks that everything works as expected.What's still missing? The work and design discussion around falcosecurity/libs#889 will lead to fixing two remaining problems:
switch
,procexit
) and all the rest of the syscall events.None of this is blocking for Falco in its current feature development status. Point 1) has no impact because Falco does not rely on the libsinsp conversion utilities, but searches for ppm_sc_codes in the ruleset right on instead. Point 2) will require a minor refactor in Falco because we currently have tracepoint in a different selection sets, however this will only be non-functional changes.
Does this PR introduce a user-facing change?: