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

update: support running multiple event sources in parallel #2182

Merged
merged 18 commits into from
Sep 12, 2022

Conversation

jasondellaluce
Copy link
Contributor

@jasondellaluce jasondellaluce commented Aug 30, 2022

What type of PR is this?

/kind design

/kind feature

Any specific area of the project related to this PR?

/area engine

What this PR does / why we need it:

See #2074. This is the last piece of "glue code" that uses all the preliminary work to support multiple parallel event processing loops coming from different sources.

From the UX known so far for Falco, this is a big no-op. However, this now supports loading multiple plugin with event sourcing capability and running all of them in parallel along with the traditional syscall event source. Each event source runs in its dedicated thread and does not influence the others. For example, this re-enables having both the k8s_audit and the syscall event sources on the same Falco instance.

By default, Falco enables the syscall event source and all the event sources implemented by the loaded plugin. The set of event sources enabled for event processing can be influenced with the --enable-source and --disable-source CLI options.

Which issue(s) this PR fixes:

Special notes for your reviewer:

There is a user-facing change that needed to be introduced in the stats writer (-s and --stats-interval CLI options). Considering that multiple event source can now run in parallel, the old stats formatting didn't make sense anymore. As such, the periodic sample is now partitioned by event source. Here's an example of the expected output:

{
  "sample": 71,
  "k8s_audit": {
    "cur": {
      "events": 1
    },
    "delta": {
      "events": 1
    }
  },
  "syscall": {
    "cur": {
      "drop_pct": 0,
      "drops": 0,
      "events": 9525,
      "preemptions": 0
    },
    "delta": {
      "drop_pct": 0,
      "drops": 0,
      "events": 137,
      "preemptions": 0
    }
  }
}

Does this PR introduce a user-facing change?:

new: support running multiple event sources in parallel
update(userspace/falco)!: adapt stats writer for multiple parallel event sources

Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

First-pass light review; i will surely need a second deeper review.

This is just useful for me to clear up some notions about the impl.


if(m_state->config->m_json_output)
// add syscall as last source
add_source_to_engine(falco_common::syscall_source);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to always add syscall_source unconditionally, without checking if it is loaded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point I'm gonna try to respond to this point and some of the other questions below. So, for event sources we maintain in the app state three things:

  • loaded_sources: a set of names all loaded event sources. By default this contains syscall that is provided out-of-the-box by Falco, and the event sources coming from the loaded plugins.
  • enabled_sources: a set of names all event sources and that are enabled for event processing (either in capture/offline or in live mode). By default this is the same as loaded_sources, and can then be changed with the --enable-source and --disable-source options.
  • sources: a list indexed by source name that contains all information mapped to a given source. For example, we map 1 inspector for each source name, 1 filtercheck list, and so on

The action of adding an event source to the rule engine relies on the loaded_sources list, because the rule engine must be aware of all sources no matter if they are actually enabled for event processing or not, because otherwise it will not be able to load the configured rulesets. Doing otherwise will deteriorate the UX as Falco would stop with error at startup if configured with even a single rule attached to a non-enabled event source. This is also why the syscall event source is added unconditionally.

Copy link
Contributor Author

@jasondellaluce jasondellaluce Aug 30, 2022

Choose a reason for hiding this comment

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

Note that the "all enabled by default" and the --disable-source options behavior are the same as when we had k8s_audit support bundled into Falco. I thought it made sense to reproduce the same UX (we didn't have --enable-source at that time, which now make things easier).

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense! Thanks Jason!

userspace/falco/app_actions/load_plugins.cpp Show resolved Hide resolved
userspace/falco/app_actions/load_plugins.cpp Show resolved Hide resolved
userspace/falco/app_actions/process_events.cpp Outdated Show resolved Hide resolved
@jasondellaluce
Copy link
Contributor Author

/milestone 0.33.0

@poiana poiana added this to the 0.33.0 milestone Aug 31, 2022
@jasondellaluce jasondellaluce force-pushed the refactor/multi-src-glue-code branch 2 times, most recently from 8989a6e to 85c45bb Compare September 6, 2022 09:00
@FedeDP
Copy link
Contributor

FedeDP commented Sep 9, 2022

Local build is failing with

In file included from /home/federico/Work/falco/userspace/engine/filter_evttype_resolver.cpp:17:
/home/federico/Work/falco/userspace/engine/filter_evttype_resolver.h: In static member function ‘static void falco_event_types::check_range(uint16_t)’:
/home/federico/Work/falco/userspace/engine/filter_evttype_resolver.h:36:36: error: ‘range_error’ is not a member of ‘std’
36 |                         throw std::range_error("invalid event type");

Do i miss anything?
This is a clean build!

@jasondellaluce
Copy link
Contributor Author

Local build is failing with

In file included from /home/federico/Work/falco/userspace/engine/filter_evttype_resolver.cpp:17:
/home/federico/Work/falco/userspace/engine/filter_evttype_resolver.h: In static member function ‘static void falco_event_types::check_range(uint16_t)’:
/home/federico/Work/falco/userspace/engine/filter_evttype_resolver.h:36:36: error: ‘range_error’ is not a member of ‘std’
36 |                         throw std::range_error("invalid event type");

Do i miss anything? This is a clean build!

Huh, I think this happens after rebasing on top of #2151. Does this happen on master too?

@FedeDP
Copy link
Contributor

FedeDP commented Sep 9, 2022

Does this happen on master too?

Confirm! Thanks! Weird that the issue is not spotted by the CI though.

@FedeDP
Copy link
Contributor

FedeDP commented Sep 9, 2022

Fixed by #2197

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
…esults

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
…ector only

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
…e private methods

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Now, the action is in charge of loading all plugins and initializing:
- the offline inspector
- the list of loaded event sources
- the list of loaded plugins and their config

After this action runs, plugins are loaded but not yet initialized.

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
… in the right order

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
…tor action

Now, the action takes care of inizializing all app inspectors
(just one in capture mode, one for each evt source in live mode), and of
registering and initializing all loaded plugins in the right inspector as needed.
The plugin initialization logic, which also involves the filtercheck list
population and checks, was moved and refactored from the previous
implementation of the load_plugins action.

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
…action

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
… loops

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
…ctors

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
…ent sources

Signed-off-by: Jason Dellaluce <jasondellaluce@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>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
@jasondellaluce jasondellaluce force-pushed the refactor/multi-src-glue-code branch from 85c45bb to f5e3d0b Compare September 12, 2022 12:39
@jasondellaluce
Copy link
Contributor Author

Fixed by #2197

Tests are back being all-green!

Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link
Contributor

poiana commented Sep 12, 2022

LGTM label has been added.

Git tree hash: 4ecde754214d0334b75968c12d0a49938fb37653

Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

This is awesome! 🤩

@poiana
Copy link
Contributor

poiana commented Sep 12, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, jasondellaluce, leogr

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 [FedeDP,jasondellaluce,leogr]

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

@poiana poiana merged commit 9c184af into master Sep 12, 2022
@poiana poiana deleted the refactor/multi-src-glue-code branch September 12, 2022 14:14
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