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

Add plugins support #93

Merged
merged 13 commits into from
Oct 20, 2021
Merged

Add plugins support #93

merged 13 commits into from
Oct 20, 2021

Conversation

mstemm
Copy link
Contributor

@mstemm mstemm commented Oct 5, 2021

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:

See the plugin proposal doc.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new: add support for plugins, including loading plugins from shared libraries, opening a stream of events, fetching events, and extracting fields.

userspace/libscap/plugin_info.h Outdated Show resolved Hide resolved
userspace/libscap/plugin_info.h Show resolved Hide resolved
// of the API they run against, and the engine will take care of checking
// and enforcing compatibility.
//
char* (*get_required_api_version)();
Copy link
Contributor

Choose a reason for hiding this comment

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

I went with encoding the API version in a single uint64_t: https://github.com/falcosecurity/libs/pull/39/files#diff-71f79d5139ef868a1129c1c29f6325e0bd2d2e40951b9578fa57e2f1156a9118R14-R23

Can we decide on a single approach and (if we choose strings) provide some helpers to work with these? I'd rather not reinvent the wheel in the api versioning patch

Copy link
Member

Choose a reason for hiding this comment

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

An unit64_t cannot represent the full SemVer 2.0 specification 👇
https://semver.org/#backusnaur-form-grammar-for-valid-semver-versions

Although the current system does not fully support that, I expect things like 1.0.0-beta+exp.sha.5114f85 will become common when dealing with dev builds.

Thus, I strongly believe we have to use a string (at least on higher-level APIs like plugins).
I'm not sure if the same versioning semantic makes sense for the driver version too, but we can discuss that.

Anyhow, I fully agree with not reinventing the wheel (in case we want to use the same semantic). For example, having a single SemVer 2.0 parser with full capabilities would be great.

Copy link
Contributor

Choose a reason for hiding this comment

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

Point taken, though I'm not sure how much this really applies to compatibility checks. Major must be the same, minor must be equal or greater, patch can be whatever (with maybe a warning if it's smaller), right? We're not sorting any version numbers here.

Also, I assume framework 1.2.0 is compatible with a plugin using api 1.1.0 but not compatible with a plugin using api 1.3.0, not the other way round? Since there's no way for the plugin to know the framework api version, it's probably the only way.

Copy link
Member

Choose a reason for hiding this comment

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

Your examples are correct. Framework 1.2.0 must be compatible with a plugin using 1.1.0 because no breaking changes can be introduced when bumping the minor version number (the only exception is for the 0.x.y series; indeed, it would be a good idea starting from 0 😺 ).
However, if the framework version is 1.2.0 and the plugin requires 1.2.0-11+a0f7dc (assume we are working on not yet a released version for development purposes) then the version check must fail (since 1.2.0-11+a0f7dc > 1.2.0). The vice-versa must work instead. I know this is an extreme example, but it can happen (similar cases happened with Falco many times) and is particularly important because we want to publish dev builds.

My main point is that we (unfortunately) have to use a string anyway because it's the only way to represent the full SemVer specification (regardless of how we use it).

PS
That being said, I can understand that the full Semver would be cumbersome for the driver. Consider that we will likely never publish dev builds for drivers, and the frequency of the changes is very low. Thus, I believe it's acceptable to use a different approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe full semver is necessary for the driver but if we do implement it for the plugin interface, I'd rather use that implementation instead of maintaining a separate one, even though it's overkill for the driver api.

But coming back to your example, do you expect the api versions to be autogenerated (from git describe or whatever)? I'd expect api versions (as opposed to software versions) to only be incremented manually and e.g. your unreleased patch on top of api 1.2.0 would just use api version 1.3.0 (the number being set in stone only at merge time, since we can have multiple PRs affecting the api in flight).

Also, I'd strongly expect the api version to be independent of any consumer version (like the driver api). Am I wrong?

Copy link
Contributor

@gnosek gnosek Oct 12, 2021

Choose a reason for hiding this comment

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

In early versions, I don't expect API versions to be autogenerated, but it can happen later (that would help when we have multiple PRs in flight).

Would it though? When merging such PRs you still have to change the api version (you won't know the api version that your PR will end up as in in the master branch).

For example, say the current released api is 1.2.0. Whether two PRs will bump the api to 1.3.0-foo and 1.3.0-bar or just to 1.3.0, one of them will end up as api 1.3.0 (not prerelease), the other one as 1.4.0.

Please correct me if I'm wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Your example is one of the possible solutions.

My idea for the future is more sophisticated and mostly avoid manually bumping the version (I'm not yet sure it's feasible): a bot that automatically increments the API version. Since the bot can commit, it can bump the API version before merging/releasing.

With such a mechanism in place, we could use a different strategy.
For example, starting from 1.2.0 with one PR 1.3.0-foo, another PR 1.3.0-bar. When the first gets merged, the master changes 1.3.0-x. When the second gets merged, the master changes to 1.3.0-y. Finally, when releasing a new software version, the API version is bumped too accordingly to semver rules (so we will end up with 1.3.0 instead of 1.4.0).

N.B.: I'm not saying this is the best solution. It's just an example. I have not yet enough which solution would work best

Copy link
Contributor

Choose a reason for hiding this comment

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

when releasing a new software version

We don't have versioned falco-libs releases (yet?). Also, this ties the API version to software versions (unnecessarily IMHO).

Copy link
Member

@leogr leogr Oct 12, 2021

Choose a reason for hiding this comment

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

We don't have versioned falco-libs yet, but the plan for that is official.

Anyway, I'm not saying we have to tie the API version to software versions. I agree with you that it would be unnecessary.
I meant that the software version release is just a convenient point in time (for many other things too, like the changelog generation). It avoids having unreleased API versions and saves unnecessary version bumps. In your previous example, the 1.3.0 would be never released (since it would end up with 1.4.0 directly).

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we just have a different view on how important a release is. As long as I don't have to write a semver parser, your call :D

userspace/libscap/plugin_info.h Show resolved Hide resolved
userspace/libscap/plugin_info.h Outdated Show resolved Hide resolved
userspace/libsinsp/plugin.h Outdated Show resolved Hide resolved
userspace/libsinsp/plugin.h Outdated Show resolved Hide resolved
userspace/libsinsp/plugin.h Outdated Show resolved Hide resolved
userspace/libsinsp/plugin_evt_processor.cpp Outdated Show resolved Hide resolved
userspace/libsinsp/plugin_evt_processor.cpp Outdated Show resolved Hide resolved
@mstemm
Copy link
Contributor Author

mstemm commented Oct 15, 2021

I believe we've addressed all the feedback at this point. There are some additional changes to get plugins to play nicely with the filter/formatter factories in falco. It's the commits starting at "Allow for alt. list of filterchecks when creating filter/formatter ".

@leogr could you look at the new changes? @gnosek could you make sure we addressed all the feedback?

@poiana
Copy link
Contributor

poiana commented Oct 20, 2021

LGTM label has been added.

Git tree hash: 63c735a8e90364e5ff52856f2768a8a9d35e5383

@poiana
Copy link
Contributor

poiana commented Oct 20, 2021

[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:

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

userspace/libscap/scap.c Outdated Show resolved Hide resolved
mstemm and others added 10 commits October 20, 2021 08:02
Add support for opening plugins and sourcing events from plugins in
libscap:

- The structs in plugin_info represent the interface to a plugin
  dynamic library. Required enums/consts used for the plugin interface
  are defined here. The source_plugin_info/extractor_plugin_info
  structs contain the resolved functions.

- scap_open_plugin_int() handles the details of opening a plugin. It
  takes a source_plugin_info struct and calls the plugin's open()
  function.

- scap_next_plugin() calls the plugin's next() method to return
  events.

Capture file handling also has some changes, with the introduction of
plugin sources:

 - When writing capture files from plugin events, the resulting
   capture file does not have any of the following:
      - fd list
      - process list
      - machine info
      - interface list
      - user list

 - When reading capture files, libscap does not mandate any of the
   above blocks, as they could have been written by a libscap reading
   plugin events.

 - When reading capture files, if a section header block is found in
   the middle of the file, instead of returning SCAP_UNEXPECTED_BLOCK,
   read the section header, make no changes, and return
   SCAP_TIMEOUT. This allows a capture file to contain a section
   header block in the middle, which supports use cases involving
   concatenating .scap files from plugins into a single file.

Co-authored-by: Loris Degioanni <[email protected]>
Co-authored-by: Leonardo Grasso <[email protected]>
Signed-off-by: Mark Stemm <[email protected]>
libsinsp support for plugins involves:

- methods to load a set of plugins and associate them with an
  inspector (add_plugin, get_plugins, get_plugin_by_id,
  get_source_plugin_by_source, etc.)

- methods to open a stream of events from a plugin and check its
  progress (set_input_plugin/set_input_plugin_open_params,
  get_read_progress_plugin, etc.)

- plugin_evt_processor is forward-looking and will allow for parallel
  reading of events from plugins.

Co-authored-by: Loris Degioanni <[email protected]>
Co-authored-by: Leonardo Grasso <[email protected]>
Signed-off-by: Mark Stemm <[email protected]>
This is like evt.datetime but the the time part skips
nanoseconds. This can be used for events from plugins, which might not
need nanosecond resolution.

Co-authored-by: Loris Degioanni <[email protected]>
Co-authored-by: Leonardo Grasso <[email protected]>
Signed-off-by: Mark Stemm <[email protected]>
Was future-thinking towards parallel processing of events but we don't
need it for MVP.

Signed-off-by: Mark Stemm <[email protected]>
Add a plugin API function to free any memory allocated by the
plugin. The dynamic symbol is plugin_free_mem(). In create_plugin(),
try to resolve plugin_free_mem first and return an error if it can't
be found. Otherwise, save it in the plugin_info structs.

In other places, where the framework was calling free() call the
resolved symbol instead.

Signed-off-by: Mark Stemm <[email protected]>
Move the handling of async extraction from the plugin framework to
plugin-sdk-go. All go plugins now export a plugin_extract_fields
function in C. That function either uses the async extraction
interface implemented in plugin-sdk-go, or calls the (synchronous)
extraction functions.

Signed-off-by: Mark Stemm <[email protected]>
When creating filter/formatter factories, or when creating an
individual filter/formatter, allow specifying an alternate list of
available filterchecks.

This will allow segregating fields used by plugins, which only work
with a given plugin event, from fields used by syscalls, which work
with the "default" set of filterchecks in g_filterlist.

The sinsp_filter_check_list object is a good match for this, but it
represents both a list of filter_checks and also has the default set
of filterchecks for syscalls baked in.

With the notion of factories, we need the first part but don't want
the second. So make up a base class filter_check_list that has the
first part. sinsp_filter_check_list derives from the base class and
has the built-in filterchecks.

Also move it to its own file so it's possible to include the
declaration without including filterchecks.h.

Signed-off-by: Mark Stemm <[email protected]>
It took a while, but we remembered to finish moving the token_bucket
from falco engine to libs. There were 2 copies for a while.

This brings over one change from the falco copy--to have an optional
timer function.

Signed-off-by: Mark Stemm <[email protected]>
Split sinsp_evt_filter_check_event in two. (New)
sinsp_evt_filter_check_gen_event has fields for event number and
time. sinsp_evt_filter_check_event has fields for everything else.

Add sinsp_evt_filter_check_gen_event to the filtercheck list for
plugins so filters/conditions can have fields that work on time/event
number.

Signed-off-by: Mark Stemm <[email protected]>
Completely divorce plugin rc values from scap rc values.

Instead of being int32_t, plugin rc values are a ss_plugin_rc enum and
don't directly relate to SCAP_XXX values.

In a couple of locations in scap.c, plugin rcs do need to be mapped to
scap rcs to reflect opening sources, getting next event. Create a
function plugin_rc_to_scap_rc for that.

Also cleaned up a few other cases where ints were being used instead
of enums (plugin type, return value from next, etc.)

Signed-off-by: Mark Stemm <[email protected]>
@poiana poiana added the lgtm label Oct 20, 2021
@poiana
Copy link
Contributor

poiana commented Oct 20, 2021

LGTM label has been added.

Git tree hash: 12c1d7124447f27fb0009525246f7088fb8d6ad9

@poiana poiana merged commit a9dd4cf into master Oct 20, 2021
@poiana poiana deleted the add-plugins-support branch October 20, 2021 15:30
leogr pushed a commit to leogr/libs that referenced this pull request Jan 5, 2023
…falcosecurity#93)

- Falcosecurity/libs falcosecurity#416: Support execve exit and clone child exit events on ARM64
- Falcosecurity/libs falcosecurity#418: Enable 64BIT_ARGS_SINGLE_REGISER on ARM64
- Also, disable userspace workarounds ARM, which attempted to compensate
  for the missing execve/clone exit events
leogr pushed a commit to leogr/libs that referenced this pull request Jan 5, 2023
…falcosecurity#93)

- Falcosecurity/libs falcosecurity#416: Support execve exit and clone child exit events on ARM64
- Falcosecurity/libs falcosecurity#418: Enable 64BIT_ARGS_SINGLE_REGISER on ARM64
- Also, disable userspace workarounds ARM, which attempted to compensate
  for the missing execve/clone exit events
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.

4 participants