update: optimize memory allocation and refactor next/next_batch helpers

[jasondellaluce]

Signed-off-by: Jason Dellaluce jasondellaluce@gmail.com Co-authored-by: Leonardo Grasso me@leonardograsso.com

What type of PR is this?

/kind design

/kind feature

Any specific area of the project related to this PR?

/area plugin-sdk

What this PR does / why we need it: This PR refactors the sdk helpers used in the next and next_batch plugin symbols. The new design proposes the usage of a new object interface PluginEvents that brings the following value propositions:

• It's easier to use and more elastic. It does not force developers to use the current callback method of the NextBatch wrapper, and can be used inside next too.
• Allocates memory only once, and does not perform any buffer copy on the fly. The benchmarks bundled in the PR shows that in the general case the next_batch flow performs zero memory allocations.
• Hides the complexity of the underlying C-memory management. At the same time, the object gives pointer access to the C buffers for advanced uses or for passing them to the plugin framework.
• Enables safe and protected memory accesses by implementing the io.Reader and io.Writer interfaces. For instence, one may access event data in write-only mode during the next flow, and in read-only mode in the field extraction flow.
• Disambiguates the C memory governance in the plugin framework. C-memory related to the plugin events is totally managed inside the plugin by the sdk.

Additional context: See https://github.com/falcosecurity/plugin-sdk-go/issues/9.

Additional notes for the reviewer: The changes of this PR affect the go sdk only, but need few minor changes inside libs too. This is due to the fact that libscap currently frees the produced event data at each iteration of next, which makes it impossible to reuse the same memory buffer and makes the memory governance ambiguous. In order for this to work, the only code locations to remove in libs would be the following (we refer to the plugin-system-api-additions wip branch of libs:

• libscap/scap.c#L1710
• libscap/scap.c#L1801

Here's the patch 👇🏼 intended to be used on top of https://github.com/falcosecurity/libs/pull/93 memory.patch.txt

Which issue(s) this PR fixes:

Fixes https://github.com/falcosecurity/plugin-sdk-go/issues/9

Does this PR introduce a user-facing change?:

update: optimize memory allocation and refactor next/next_batch helpers

[poiana]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jasondellaluce To complete the pull request process, please assign kris-nova after the PR has been reviewed. You can assign the PR to them by writing /assign @kris-nova in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/falcosecurity/plugin-sdk-go/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

[poiana]

Welcome @jasondellaluce! It looks like this is your first PR to falcosecurity/plugin-sdk-go 🎉

[poiana]

Hi @jasondellaluce. Thanks for your PR.

I'm waiting for a falcosecurity member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

[leogr]

/ok-to-test

[leogr]

/test build-plugin-sdk-go

[mstemm]

The code changes look okay, but I'm wondering if we should add this kind of inconsistency to the API. Previously, the rules for memory handling were straightforward--every struct/string/extracted field/etc was assumed to be allocated before being passed to the framework. Now, events will be an exception to that rule.

Is that going to be too confusing for plugin authors?

Also, if we're going to change the rules for memory handling, we need to also update plugin_info.h, the docs, and the plugin developer's guide to reflect this.

[leogr]

The code changes look okay, but I'm wondering if we should add this kind of inconsistency to the API. Previously, the rules for memory handling were straightforward--every struct/string/extracted field/etc was assumed to be allocated before being passed to the framework. Now, events will be an exception to that rule.

IMHO, we already introduced a kind of inconsistency. Indeed, if we look a the developer guide, it says:

Every function that returns or populates a char * or a struct pointer must allocate the memory for the string/struct (typically malloc()). The plugin framework will free the allocated memory using plugin_free_mem().

But later:

It is the plugin's responsibility to allocate plugin state (memory, open files, etc) and free that state later in `plugin_destroy`.

Thus, with this PR we could even reduce the confusion. We could say that the plugin framework will free the allocated memory only for char * and nothing more. Furthermore, this solution comes with a significant performance improvement (also for plugins written in C). Finally, I think we should, in general, prefer flexibility and performance improvement over conventions.

Also, if we're going to change the rules for memory handling, we need to also update plugin_info.h, the docs, and the plugin developer's guide to reflect this.

That's correct. We will update the patch to reflect this.

We can also provide a patch for the developer guide. Anyway, we planned to introduce other significant changes. Maybe we want to update it later?

[jasondellaluce]

FYI me and @leogr just updated the libs patch in the description. We updated the docs in plugin_info.h to be more explicit on whether the return values are allocated/deallocated by the plugin or by the framework. Note that we haven't changed the docs of extract_fields yet, as we plan to propose a different approach for that in the future.

[leogr]

Hey @mstemm

Here is the patch for the docs :point_right: https://github.com/falcosecurity/falco-website/pull/493#pullrequestreview-781907448

[leogr]

/hold

[jasondellaluce]

retired: a new proposal will come soon.