search

falcosecurity / libs

libsinsp, libscap, the kernel module driver, and the eBPF driver sources
https://falcosecurity.github.io/libs/
Apache License 2.0
231 stars 164 forks source link

plugin api: passing state to more interface methods and fixing event_to_string #292

Closed jasondellaluce closed 2 years ago

jasondellaluce commented 2 years ago

Motivation 1

The plugin API have both function symbols that accept a plugin state/instance as function parameters, and symbols that take no parameters at all. The symbols with no parameters are meant to just return "static" information that describe the plugin at load-time, such as its name, version, required API version, etc. However, some of them are in the general case not a simple description of plugins, but also describe part of their behavior. Accordingly, the first proposal of this issue is to change the plugin API to add a ss_plugin_t* s function parameter to the symbols get_event_source, get_fields, get_extract_event_sources.

In the context of plugin development, receiving a plugin state allows implementing a more object-oriented design to languages that allow it (e.g. C++ and Go, which we officially support). In this perspective, this design change would make it easier for both the plugin developers and SDK developers to have a more consistent design. As a developer of the SDK Go, I felt some friction in this aspect. Another key benefit is that the information returned by those event could potentially be influenced by the configuration passed through the init() symbol. This is coherent with the current plugin loading logic, which calls init() prior to any of the symbols mentioned above. Lastly, this is also more coherent to the changes of https://github.com/falcosecurity/libs/pull/274 and the proposal of https://github.com/falcosecurity/libs/issues/259, which abolish the concept of "plugin type" and open the door to the development of more polymorphic plugins.

Motivation 2

The second proposal of this issue is to change the parameters of event_to_string to accept a const ss_plugin_event *evt instead of sparse information about events (e.g. its data only). This caused some trouble while implementing the SDK Go as we realized that extraction caching could not be applied at all to the result of ss_plugin_event due to the lack of any metadata about the event such as the event ID or its timestamp. The current design of this symbol makes it kind of incomplete and generally brings to bad performance.

Feature

This issue proposes to change the API function symbols mentioned above as follows:

This is a clear breaking change towards the plugin API, however this is also the right time in which we can introduce them because we already bumping the API version to 1.0.0 (see https://github.com/falcosecurity/falco/issues/1948).

zuc commented 2 years ago

+1 Both points (1. in particular) make a lot of sense to me.

FedeDP commented 2 years ago

I am all in for these changes! I think they provide better future-proofing for the plugins API, with less chance of breaking the API. I am not sure about get_id changes: do we really need them? Aside from that, :+1: from me!

jasondellaluce commented 2 years ago

Uh, I've included get_id by mistake, thanks for noticing!

leogr commented 2 years ago

I'm all in with the second proposal that's really needed. Furthermore, I would make event_to_string optional since not all consumers use it, and plugins authors do not understand why they should implement such a symbol. :+1:

On the other hand, I'm not convinced of removing the "static" requirement from symbols like get_event_source, get_fields, get_extract_event_sources. By design, we forced them to be static-like functions. If a plugin returned different fields sets based on the passed configuration, it would be impossible to ensure that a rule file is compatible with a plugin. Today we can statically check the compatibility because each plugin version comes with a specific set of fields, and a rule file can declare the minimum plugin version. If the plugin could return a different fields list based on a runtime configuration, this mechanism would be completely broken.

A similar issue could happen with get_event_source. What will happen if a plugin always returns the same ID but different event source names (based on a runtime configuration)? I guess it would break some of the plugin framework assumptions. We allow several IDs to map to the same event source (so multiple plugins can provide the same event source), but not the other way around.

jasondellaluce commented 2 years ago

I see your point and I agree! I didn't think of all the possible scenarios, thanks for the detail overview. I opened #309 to update the definitions of event_to_string as agreed!