cleanup(sinsp): move engine/platform selection code to libsinsp #1401

gnosek · 2023-10-10T14:39:00Z

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 API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-savefile

/area libscap-engine-udig

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it:

This PR continues the work of decoupling libscap from platform code. The motivation is to (eventually):

move all the parsing/data structure heavy code to C++
strip down libscap to the bare essentials for getting events
remove code duplication for platform data structures (scap_threadinfo vs sinsp_threadinfo etc.)
kill uthash :)

Baby steps though, this PR moves the code responsible for engine/platform selection to libsinsp (in particular, sinsp::open_* methods). All the platform code still lives in libscap for the time being.

I'm planning some follow up PRs (though this one feels okay as a standalone submission too):

make sinsp own the m_platform pointer (remove it from scap_t)
make savefile code independent of platform specifics (delegate platform data save/load to platforms)
build a C++ interface to the platform code
move over the platform code to sinsp (with no substantial modifications)
actually rewrite into C++; this is probably going to take a while :)

All but the last one are more or less done, for the actual rewrite I have some sketches but not yet finished code as I'd rather avoid rebasing the inevitable giant patch set over and over :)

In the end, I see the platform class as owning ~all the syscall parser code, with e.g. container/k8s support building on top of that. It's going to substantially change the center of gravity of libsinsp, which would become a pipeline for events into various "plugins" (using the term very loosely), but also enables us to e.g. support non-Linux OSes cleanly.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

This PR does not affect any sinsp APIs but does affect scap. Not sure whether we consider this an API change: does anybody use raw libscap in the real world?

Does this PR introduce a user-facing change?:

BREAKING CHANGE: scap_init (and related functions) no longer initialize the platform
BREAKING CHANGE: scap_mode_t and its values are now renamed to sinsp_mode_t and SINSP_MODE_*

FedeDP · 2023-10-17T07:47:57Z

/milestone 0.14.0

cmake/modules/engine_config.cmake

userspace/libsinsp/sinsp.cpp

leogr · 2023-10-17T08:20:16Z

This PR does not affect any sinsp APIs but does affect scap. Not sure whether we consider this an API change: does anybody use raw libscap in the real world?

I don't know, but I would still signal it as a breaking change in the release-note block
(we use ! or BREAKING CHANGE: as per https://www.conventionalcommits.org/en/v1.0.0/#specification point 13).

Signed-off-by: Grzegorz Nosek <[email protected]>

AS of now, libscap doesn't know anything about selecting engines or platforms and defers this to sinsp (or the consumer, when using scap directly) Signed-off-by: Grzegorz Nosek <[email protected]>

They are handled by the (Linux) platform, not the engine Signed-off-by: Grzegorz Nosek <[email protected]>

Andreagit97

Thank you for this!
/approve

poiana · 2023-10-30T09:17:30Z

LGTM label has been added.

Git tree hash: c01b0b03d91d8676f419a92de9db6efa494604b4

libscap doesn't need it any more and sinsp can pass it to the only place that needs it (sinsp::open_common) directly. Note: we could just also set m_mode in the individual sinsp::open_* methods but this way we save a tiny bit of code duplication. Signed-off-by: Grzegorz Nosek <[email protected]>

…form_vtable Signed-off-by: Grzegorz Nosek <[email protected]>

It matches the new role of the function better Signed-off-by: Grzegorz Nosek <[email protected]>

Signed-off-by: Grzegorz Nosek <[email protected]>

userspace/libsinsp/sinsp.h

Andreagit97 · 2023-10-30T10:02:22Z

userspace/libsinsp/sinsp.cpp

+	switch(mode)
+	{
+		case SCAP_MODE_PLUGIN:
+			platform = scap_generic_alloc_platform(::on_new_entry_from_proc, this);


Amazing thank you!

scap_mode_t is now named sinsp_mode_t to reflect that it's not used in libscap at all, and all the enum values are renamed from SCAP_MODE_* to SINSP_MODE_* Signed-off-by: Grzegorz Nosek <[email protected]>

Andreagit97 · 2023-10-30T10:04:26Z

if the CI becomes green we are good to go IMO

gnosek · 2023-10-30T13:34:42Z

the s390x build only took a bit over two and a half hours :D

Andreagit97

/approve

poiana · 2023-10-30T14:41:31Z

LGTM label has been added.

Git tree hash: 140f211f7235ebd1299c012fbbb667eeb0f697b6

FedeDP

/approve

poiana · 2023-10-30T20:24:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, gnosek

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~~ [Andreagit97,FedeDP,gnosek]

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

poiana added release-note-none kind/cleanup dco-signoff: yes kind/design area/libscap approved area/libsinsp labels Oct 10, 2023

poiana requested review from Andreagit97 and hbrueckner October 10, 2023 14:39

poiana added the size/XXL label Oct 10, 2023

gnosek force-pushed the platform-sinsp-selection branch from b6d9147 to fab324f Compare October 10, 2023 19:45

gnosek changed the title ~~cleanup(sinsp): move engine/platform selection code to libsinsp~~ wip: cleanup(sinsp): move engine/platform selection code to libsinsp Oct 11, 2023

poiana added the do-not-merge/work-in-progress label Oct 11, 2023

gnosek force-pushed the platform-sinsp-selection branch 6 times, most recently from 053a960 to 4401d55 Compare October 12, 2023 14:08

gnosek changed the title ~~wip: cleanup(sinsp): move engine/platform selection code to libsinsp~~ cleanup(sinsp): move engine/platform selection code to libsinsp Oct 12, 2023

poiana removed the do-not-merge/work-in-progress label Oct 12, 2023

gnosek mentioned this pull request Oct 17, 2023

cleanup(scap,sinsp): return event flags directly #1420

Merged

poiana added this to the 0.14.0 milestone Oct 17, 2023

FedeDP reviewed Oct 17, 2023

View reviewed changes

cmake/modules/engine_config.cmake Show resolved Hide resolved

userspace/libsinsp/sinsp.cpp Outdated Show resolved Hide resolved

userspace/libsinsp/sinsp.cpp Outdated Show resolved Hide resolved

gnosek force-pushed the platform-sinsp-selection branch from 4401d55 to ce02d53 Compare October 17, 2023 10:52

poiana added dco-signoff: no and removed dco-signoff: yes labels Oct 17, 2023

gnosek force-pushed the platform-sinsp-selection branch from ce02d53 to aeda51d Compare October 17, 2023 10:53

gnosek added 3 commits October 26, 2023 18:18

cleanup(scap, sinsp): remove proc callback params from scap_open_args

d7de595

Signed-off-by: Grzegorz Nosek <[email protected]>

cleanup(scap,sinsp): move engine selection to sinsp

46c477a

AS of now, libscap doesn't know anything about selecting engines or platforms and defers this to sinsp (or the consumer, when using scap directly) Signed-off-by: Grzegorz Nosek <[email protected]>

cleanup(scap,sinsp): remove scap_nodriver_engine_params

df24a91

They are handled by the (Linux) platform, not the engine Signed-off-by: Grzegorz Nosek <[email protected]>

Andreagit97 previously approved these changes Oct 30, 2023

View reviewed changes

poiana assigned Andreagit97 Oct 30, 2023

poiana added the lgtm label Oct 30, 2023

gnosek added 5 commits October 30, 2023 10:38

cleanup(scap): rename scap_linux_platform variable to scap_linux_plat…

146d472

…form_vtable Signed-off-by: Grzegorz Nosek <[email protected]>

cleanup(scap): rename scap_init_int to scap_init_engine

d9ff58c

It matches the new role of the function better Signed-off-by: Grzegorz Nosek <[email protected]>

docs(scap): document why we initialize the engine first

293d4a2

Signed-off-by: Grzegorz Nosek <[email protected]>

cleanup(scap): move scap_mode_t to sinsp

8ad59e0

Signed-off-by: Grzegorz Nosek <[email protected]>

gnosek dismissed Andreagit97’s stale review via af92f50 October 30, 2023 09:39

gnosek force-pushed the platform-sinsp-selection branch from 053d38f to af92f50 Compare October 30, 2023 09:39

poiana removed the lgtm label Oct 30, 2023

poiana requested a review from Andreagit97 October 30, 2023 09:39

Andreagit97 reviewed Oct 30, 2023

View reviewed changes

cleanup(sinsp): rename scap_mode_t and its values

7a197f5

scap_mode_t is now named sinsp_mode_t to reflect that it's not used in libscap at all, and all the enum values are renamed from SCAP_MODE_* to SINSP_MODE_* Signed-off-by: Grzegorz Nosek <[email protected]>

gnosek force-pushed the platform-sinsp-selection branch from af92f50 to 7a197f5 Compare October 30, 2023 10:04

Andreagit97 approved these changes Oct 30, 2023

View reviewed changes

poiana added the lgtm label Oct 30, 2023

FedeDP approved these changes Oct 30, 2023

View reviewed changes

poiana assigned FedeDP Oct 30, 2023

poiana merged commit cbab0d8 into falcosecurity:master Oct 30, 2023
21 checks passed

gnosek deleted the platform-sinsp-selection branch October 30, 2023 20:28

gnosek mentioned this pull request Oct 30, 2023

cleanup(scap): remove platform handle from scap_t #1458

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup(sinsp): move engine/platform selection code to libsinsp #1401

cleanup(sinsp): move engine/platform selection code to libsinsp #1401

gnosek commented Oct 10, 2023 •

edited

Loading

FedeDP commented Oct 17, 2023

leogr commented Oct 17, 2023

Andreagit97 left a comment

poiana commented Oct 30, 2023

Andreagit97 Oct 30, 2023

Andreagit97 commented Oct 30, 2023

gnosek commented Oct 30, 2023

Andreagit97 left a comment

poiana commented Oct 30, 2023

FedeDP left a comment

poiana commented Oct 30, 2023

cleanup(sinsp): move engine/platform selection code to libsinsp #1401

cleanup(sinsp): move engine/platform selection code to libsinsp #1401

Conversation

gnosek commented Oct 10, 2023 • edited Loading

FedeDP commented Oct 17, 2023

leogr commented Oct 17, 2023

Andreagit97 left a comment

Choose a reason for hiding this comment

poiana commented Oct 30, 2023

Andreagit97 Oct 30, 2023

Choose a reason for hiding this comment

Andreagit97 commented Oct 30, 2023

gnosek commented Oct 30, 2023

Andreagit97 left a comment

Choose a reason for hiding this comment

poiana commented Oct 30, 2023

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Oct 30, 2023

gnosek commented Oct 10, 2023 •

edited

Loading