Add ability to return all fields exported by a factory #75

mstemm · 2021-08-25T21:22:35Z

Add the ability to return all fields exported by a factory. This is
important for programs like falco that need to validate rule filter
expressions for various event sources, as well as print out sets of
supported fields.

Previously, falco did direct calls to
sinsp::get_filtercheck_fields_info but we're trying to standardize
everything to work through factories, to make it easier to support new
event sources. This PR supports that work.

Signed-off-by: Mark Stemm [email protected]

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area driver-kmod

/area driver-ebpf

/area libscap

/area libsinsp

/area tests

/area proposals

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new: add ability to return all fields exported by a filter factory. This will be used by Falco to support multiple event sources.

leogr · 2021-08-26T14:19:03Z

/ok-to-test

Take advantage of the changes in falcosecurity/libs#75 to have a general-purpose way to list fields for a given event source. in the engine, list_fields() now takes a source, iterates over filter factories, and calls get_fields() for each factory, printing the results. list_source_fields now calls the engine regardless of source. Signed-off-by: Mark Stemm <[email protected]>

gnosek

Hmm okay 🤷‍♂️

There's no user of this code added so I have ~zero context on what this is supposed to do

gnosek · 2021-09-09T13:23:17Z

userspace/libsinsp/gen_filter.h

@@ -205,4 +232,7 @@ class gen_event_filter_factory

 	// Create a new filtercheck
 	virtual gen_event_filter_check *new_filtercheck(const char *fldname) = 0;
+
+	// Return the set of fields supported by this factory
+	virtual std::list<filter_fieldclass_info> get_fields() = 0;


Why lists and not vectors?

No need for random access, just iteration.

Still, I don't think a list should be the default choice for a container, due to the horrible memory accesses. Probably doesn't matter much here, but maybe at least we'd get cheap .size() with a vector

According to https://stackoverflow.com/questions/228908/is-listsize-really-on .size() is supposed to be O(1) and actually is in gcc >= 5.0. Also, the only use of this method just iterates (once) anyway.

gnosek · 2021-09-09T13:24:01Z

userspace/libsinsp/filter.h

+	std::list<gen_event_filter_factory::filter_fieldclass_info> get_fields() override;
+


I assume this is the only subclass?

In the libraries, yes. In falco there's another subclass for "json_events", which are used by the k8s audit rules: https://github.com/leogr/falco/blob/new/plugin-system-api-additions/userspace/engine/json_evt.h#L376.

And after these changes are all merged, we'll be creating a subclass for plugins, so a given plugin can export the set of valid fields used by that plugin.

userspace/libsinsp/filter.cpp

gnosek · 2021-09-09T13:43:25Z

userspace/libsinsp/filter.cpp

+
+		for(int32_t k = 0; k < fci->m_nfields; k++)
+		{
+			const filtercheck_field_info* fld = &fci->m_fields[k];


fci->m_fields isn't a vector we could iterate over without an explicit index, is it?

that's right, looks like it's been a plain array the whole time: https://github.com/falcosecurity/libs/blob/factory-add-get-fields/userspace/libsinsp/sinsp.h#L143

:sadpanda: might be nice to convert it to a vector later.

gnosek · 2021-09-09T13:43:49Z

userspace/libsinsp/filter.cpp

+			if(fld->m_flags & EPF_TABLE_ONLY ||
+			   fld->m_flags & EPF_PRINT_ONLY)
+			{
+				continue;
+			}


What do these flags mean and why do we want this check here?

They are defined here: https://github.com/falcosecurity/libs/blob/factory-add-get-fields/userspace/libsinsp/event.h#L44. Generally if a field can't be used to filter events, we don't want to print it.

(And btw, this is just moving code from falco that did this in a specific way for syscall filterchecks. This PR makes it generic for a "factory" e.g. source of filters/filterchecks/fields).

Thanks for the explanation.

if a field can't be used to filter events, we don't want to print it

this should live as a comment above this if

Okay, added.

mstemm · 2021-09-10T19:37:32Z

By the way, all of these PRs are used in this Falco PR (falcosecurity/falco#1715), if you want to see how they are used.

leogr

/approve

LGTM, just a note since the next Falco release is very close.

The new get_fields() conflicts with the one already present here in Falco (so basically Falco won't compile until the signature gets adjusted in Falco).
I know that's addressed by falcosecurity/falco#1715. However, we likely have to upgrade the version of the libs in Falco soon and I'm not confident falcosecurity/falco#1715 will get merged in time.

Thus, I wouldn't risk blocking the Falco release. So,
/hold
for the moment.

poiana · 2021-09-16T14:53:24Z

LGTM label has been added.

Git tree hash: 883da564554e61b2aecfa056eadf99a7466a151d

mstemm · 2021-09-16T17:31:27Z

That's fine--I wouldn't merge any of #74, #75, #76, or #77 individually. Just merge them all at once (after the release, most likely) and then I can fix things up with the corresponding falco release.

leogr · 2021-09-17T07:14:09Z

That's fine--I wouldn't merge any of #74, #75, #76, or #77 individually. Just merge them all at once (after the release, most likely) and then I can fix things up with the corresponding falco release.

Perfect! I will review the other PRs soon.
Thank you.

mstemm · 2021-10-01T16:41:36Z

Falco 0.30.0 is released so I'm removing the hold.
/hold cancel

Add the ability to return all fields exported by a factory. This is important for programs like falco that need to validate rule filter expressions for various event sources, as well as print out sets of supported fields. Previously, falco did direct calls to sinsp::get_filtercheck_fields_info but we're trying to standardize everything to work through factories, to make it easier to support new event sources. This PR supports that work. Signed-off-by: Mark Stemm <[email protected]>

leogr

/approve

poiana · 2021-10-04T08:00:11Z

LGTM label has been added.

Git tree hash: f3f1fe505e425a28717cfb3c660ab20bf0034e88

poiana · 2021-10-04T08:00:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leogr, mstemm

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~~ [leogr]

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

Pinned to a commit that contains PRs falcosecurity/libs#74, falcosecurity/libs#75, falcosecurity/libs#76, falcosecurity/libs#77 Signed-off-by: Mark Stemm <[email protected]>

Take advantage of the changes in falcosecurity/libs#75 to have a general-purpose way to list fields for a given event source. in the engine, list_fields() now takes a source, iterates over filter factories, and calls get_fields() for each factory, printing the results. list_source_fields now calls the engine regardless of source. Signed-off-by: Mark Stemm <[email protected]>

poiana added release-note kind/feature New feature or request area/libsinsp dco-signoff: yes labels Aug 25, 2021

poiana requested review from gnosek and leogr August 25, 2021 21:22

poiana added the size/M label Aug 25, 2021

poiana added the ok-to-test label Aug 26, 2021

mstemm mentioned this pull request Aug 27, 2021

Gen events falco engine falcosecurity/falco#1715

Merged

gnosek reviewed Sep 9, 2021

View reviewed changes

poiana added dco-signoff: no and removed dco-signoff: yes labels Sep 10, 2021

mstemm force-pushed the factory-add-get-fields branch from 8038b1e to ff9868b Compare September 10, 2021 19:39

poiana added dco-signoff: yes dco-signoff: no and removed dco-signoff: no dco-signoff: yes labels Sep 10, 2021

mstemm force-pushed the factory-add-get-fields branch from a179dc7 to 59ff689 Compare September 13, 2021 14:48

poiana added dco-signoff: yes and removed dco-signoff: no labels Sep 13, 2021

leogr previously approved these changes Sep 16, 2021

View reviewed changes

poiana added the do-not-merge/hold label Sep 16, 2021

poiana assigned leogr Sep 16, 2021

poiana added the lgtm label Sep 16, 2021

poiana added the approved label Sep 16, 2021

gnosek previously approved these changes Oct 1, 2021

View reviewed changes

poiana assigned gnosek Oct 1, 2021

poiana removed the do-not-merge/hold label Oct 1, 2021

mstemm dismissed stale reviews from gnosek and leogr via 1fdf3af October 1, 2021 16:43

mstemm force-pushed the factory-add-get-fields branch from 59ff689 to 1fdf3af Compare October 1, 2021 16:43

poiana removed the lgtm label Oct 1, 2021

gnosek approved these changes Oct 1, 2021

View reviewed changes

leogr approved these changes Oct 4, 2021

View reviewed changes

poiana added the lgtm label Oct 4, 2021

poiana merged commit 59a8145 into master Oct 4, 2021

poiana deleted the factory-add-get-fields branch October 4, 2021 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to return all fields exported by a factory #75

Add ability to return all fields exported by a factory #75

mstemm commented Aug 25, 2021 •

edited by leogr

Loading

leogr commented Aug 26, 2021

gnosek left a comment

gnosek Sep 9, 2021

mstemm Sep 10, 2021

gnosek Sep 13, 2021

mstemm Sep 13, 2021

gnosek Sep 9, 2021

mstemm Sep 10, 2021

gnosek Sep 9, 2021

mstemm Sep 10, 2021

gnosek Sep 13, 2021

gnosek Sep 9, 2021

mstemm Sep 10, 2021

gnosek Sep 13, 2021

mstemm Sep 13, 2021

mstemm commented Sep 10, 2021

leogr left a comment

poiana commented Sep 16, 2021

mstemm commented Sep 16, 2021

leogr commented Sep 17, 2021

mstemm commented Oct 1, 2021

leogr left a comment

poiana commented Oct 4, 2021

poiana commented Oct 4, 2021

		std::list<gen_event_filter_factory::filter_fieldclass_info> get_fields() override;

Add ability to return all fields exported by a factory #75

Add ability to return all fields exported by a factory #75

Conversation

mstemm commented Aug 25, 2021 • edited by leogr Loading

leogr commented Aug 26, 2021

gnosek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstemm commented Sep 10, 2021

leogr left a comment

Choose a reason for hiding this comment

poiana commented Sep 16, 2021

mstemm commented Sep 16, 2021

leogr commented Sep 17, 2021

mstemm commented Oct 1, 2021

leogr left a comment

Choose a reason for hiding this comment

poiana commented Oct 4, 2021

poiana commented Oct 4, 2021

mstemm commented Aug 25, 2021 •

edited by leogr

Loading