fix(userspace/libsinsp): let plugins parse events before eventually filtering them out through inspector global filter #2182

FedeDP · 2024-12-04T10:17:16Z

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area libsinsp

Does this PR require a change in the driver versions?

What this PR does / why we need it:

Before, it was the sinsp parser that decided if an event was to be filtered out given the inspector global filter.
This led to a problem where the plugins were actually able to parse the event, but then the event was filtered out in any case.
For example, if we had a filter on a plugin field, we evaluated the filter before the plugin parsed the event, but if the plugin field extraction relied on the parsing code to eg: write a value to a foreign key, the filter would always filter out the event.
Eg (leveraging #2179):

sudo ./libsinsp/examples/sinsp-example -p "/home/federico/Work/container/libcontainer.so" -m -f "container_plugin.id!=host"

The container_plugin.id!=host filter would have been evaluated before plugin parsers were called, leading to container_plugin.id being host instead of the real container id for the very first event in a container (ie: the clone).

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

FedeDP · 2024-12-04T10:17:28Z

/milestone 0.20.0
/cc @jasondellaluce

FedeDP · 2024-12-04T10:19:27Z

userspace/libsinsp/parsers.cpp

-	}
-
-	evt->set_filtered_out(false);
+	const uint16_t etype = evt->get_scap_evt()->type;


Now the parser is only responsible for the big switch case (ie: the actual parsing).

All of the code below has been moved to a RAII oriented structure under sinsp.h: struct sinsp_evt_filter.

FedeDP · 2024-12-04T10:20:03Z

userspace/libsinsp/parsers.h

@@ -36,6 +36,7 @@ class sinsp_parser {
 	void process_event(sinsp_evt* evt);
 	void event_cleanup(sinsp_evt* evt);

+	bool reset(sinsp_evt* evt);


Since it is now called by sinsp, moved to public.

FedeDP · 2024-12-04T10:20:33Z

userspace/libsinsp/sinsp.cpp

+			//
+			// Run the state engine
+			//
+			m_parser->process_event(evt);


Keep old behavior: only process event by sinsp if the event has not been filtered out.

FedeDP · 2024-12-04T10:21:00Z

userspace/libsinsp/sinsp.cpp

+		// the internal implementation of libsinsp.
+		for(auto& pp : m_plugin_parsers) {
+			// todo(jason): should we log parsing errors here?
+			pp.process_event(evt, m_event_sources);


For plugins, as stated by the comment, we cannot know in advance if an event will modify its state; therefore always call the plugin parsers.

github-actions · 2024-12-04T10:23:29Z

Perf diff from master - unit tests

     6.02%     +2.14%  [.] sinsp::next
     4.90%     +1.37%  [.] sinsp_evt::get_type
     3.53%     -0.94%  [.] sinsp_thread_manager::get_thread_ref
     3.33%     -0.85%  [.] sinsp_parser::process_event
     2.58%     -0.85%  [.] sinsp::fetch_next_event
     0.63%     +0.67%  [.] libsinsp::state::stl_container_table_adapter<std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, libsinsp::state::value_table_entry_adapter<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, libsinsp::state::value_table_entry_adapter<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >::dynamic_fields_t>::stl_container_table_adapter
     3.64%     -0.67%  [.] sinsp_thread_manager::find_thread
     0.63%     +0.48%  [.] sinsp_evt::get_direction
     1.76%     -0.44%  [.] 0x00000000000eb3b0
     1.63%     +0.42%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release

Heap diff from master - unit tests

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Benchmarks diff from master

Comparing gbench_data.json to /root/actions-runner/_work/libs/libs/build/gbench_data.json
Benchmark                                                         Time             CPU      Time Old      Time New       CPU Old       CPU New
----------------------------------------------------------------------------------------------------------------------------------------------
BM_sinsp_split_mean                                            -0.0342         -0.0342           152           146           152           146
BM_sinsp_split_median                                          -0.0349         -0.0349           152           147           152           147
BM_sinsp_split_stddev                                          -0.6196         -0.6201             2             1             2             1
BM_sinsp_split_cv                                              -0.6062         -0.6067             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_mean                  -0.0485         -0.0485            60            57            60            57
BM_sinsp_concatenate_paths_relative_path_median                -0.0477         -0.0477            60            57            60            57
BM_sinsp_concatenate_paths_relative_path_stddev                +0.2594         +0.2574             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_cv                    +0.3235         +0.3214             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_mean                     -0.0150         -0.0150            25            25            25            25
BM_sinsp_concatenate_paths_empty_path_median                   -0.0147         -0.0147            25            25            25            25
BM_sinsp_concatenate_paths_empty_path_stddev                   -0.0661         -0.0664             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_cv                       -0.0518         -0.0522             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_mean                  -0.1200         -0.1200            64            56            64            56
BM_sinsp_concatenate_paths_absolute_path_median                -0.1195         -0.1195            63            56            63            56
BM_sinsp_concatenate_paths_absolute_path_stddev                -0.0001         -0.0003             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_cv                    +0.1364         +0.1360             0             0             0             0
BM_sinsp_split_container_image_mean                            +0.0073         +0.0073           389           392           389           392
BM_sinsp_split_container_image_median                          +0.0091         +0.0091           389           392           389           392
BM_sinsp_split_container_image_stddev                          +0.1052         +0.1051             3             4             3             4
BM_sinsp_split_container_image_cv                              +0.0973         +0.0971             0             0             0             0

userspace/libsinsp/sinsp.h

codecov · 2024-12-04T10:29:08Z

Codecov Report

Attention: Patch coverage is 87.80488% with 5 lines in your changes missing coverage. Please review.

Project coverage is 75.17%. Comparing base (556d868) to head (b162571).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
userspace/libsinsp/sinsp.cpp	87.50%	5 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2182   +/-   ##
=======================================
  Coverage   75.16%   75.17%           
=======================================
  Files         257      257           
  Lines       33711    33723   +12     
  Branches     5744     5743    -1     
=======================================
+ Hits        25339    25350   +11     
- Misses       8372     8373    +1

Flag	Coverage Δ
libsinsp	`75.17% <87.80%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…iltering them out through inspector global filter. Signed-off-by: Federico Di Pierro <[email protected]>

…p.cpp. Signed-off-by: Federico Di Pierro <[email protected]> Co-authored-by: Jason Dellaluce <[email protected]>

Andreagit97

/approve

poiana · 2024-12-04T12:13:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP

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:

~~OWNERS~~ [Andreagit97,FedeDP]

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

poiana added release-note-none dco-signoff: yes kind/bug Something isn't working area/libsinsp approved labels Dec 4, 2024

poiana requested review from gnosek and hbrueckner December 4, 2024 10:17

poiana requested a review from jasondellaluce December 4, 2024 10:17

poiana added this to the 0.20.0 milestone Dec 4, 2024

poiana added the size/L label Dec 4, 2024

FedeDP commented Dec 4, 2024

View reviewed changes

jasondellaluce reviewed Dec 4, 2024

View reviewed changes

userspace/libsinsp/sinsp.h Outdated Show resolved Hide resolved

jasondellaluce approved these changes Dec 4, 2024

View reviewed changes

poiana assigned jasondellaluce Dec 4, 2024

poiana added the lgtm label Dec 4, 2024

FedeDP and others added 2 commits December 4, 2024 12:53

fix(userspace/libsinsp): let plugins parse events before eventually f…

0b581f9

…iltering them out through inspector global filter. Signed-off-by: Federico Di Pierro <[email protected]>

chore(userspace/libsinsp): move sinsp_evt_filter definition to sins…

b162571

…p.cpp. Signed-off-by: Federico Di Pierro <[email protected]> Co-authored-by: Jason Dellaluce <[email protected]>

FedeDP force-pushed the fix/sinsp_filter_after_plugin_parse branch from 28ae697 to b162571 Compare December 4, 2024 11:53

Andreagit97 approved these changes Dec 4, 2024

View reviewed changes

poiana assigned Andreagit97 Dec 4, 2024

poiana merged commit 82f0d4b into master Dec 4, 2024
47 of 49 checks passed

poiana deleted the fix/sinsp_filter_after_plugin_parse branch December 4, 2024 12:37

FedeDP mentioned this pull request Dec 9, 2024

Reimplement containers metadata support as a plugin falcosecurity/falco#3403

Open

This was referenced Dec 9, 2024

wip: chore(userspace/libsinsp): move user group manager on container_id changed refresh to a RAII object #2192

Closed

chore(userspace/libsinsp): move user group manager on container_id changed refresh to a RAII object #2194

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(userspace/libsinsp): let plugins parse events before eventually filtering them out through inspector global filter #2182

fix(userspace/libsinsp): let plugins parse events before eventually filtering them out through inspector global filter #2182

FedeDP commented Dec 4, 2024 •

edited

Loading

FedeDP commented Dec 4, 2024

FedeDP Dec 4, 2024

FedeDP Dec 4, 2024

FedeDP Dec 4, 2024

FedeDP Dec 4, 2024

FedeDP Dec 4, 2024

github-actions bot commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

Andreagit97 left a comment

poiana commented Dec 4, 2024

fix(userspace/libsinsp): let plugins parse events before eventually filtering them out through inspector global filter #2182

fix(userspace/libsinsp): let plugins parse events before eventually filtering them out through inspector global filter #2182

Conversation

FedeDP commented Dec 4, 2024 • edited Loading

FedeDP commented Dec 4, 2024

FedeDP Dec 4, 2024

Choose a reason for hiding this comment

FedeDP Dec 4, 2024

Choose a reason for hiding this comment

FedeDP Dec 4, 2024

Choose a reason for hiding this comment

FedeDP Dec 4, 2024

Choose a reason for hiding this comment

FedeDP Dec 4, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 4, 2024 • edited Loading

Perf diff from master - unit tests

Heap diff from master - unit tests

Heap diff from master - scap file

Benchmarks diff from master

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

Andreagit97 left a comment

Choose a reason for hiding this comment

poiana commented Dec 4, 2024

FedeDP commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading