build(minimal-build): allow using eBPF driver with minimal builds #1325

Molter73 · 2023-09-05T13:32:01Z

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area build

Does this PR require a change in the driver versions?

What this PR does / why we need it:
Historically, the minimal build was only able to run using the kernel module driver, but since the addition of the modern probe this has also become an option for running with minimal builds. I believe there's value in allowing adopters of the libs to use the minimal build with the eBPF probe.

This change means that scap will always be linked against zlib and libelf, but I believe this is a worthwhile trade off.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Molter73 · 2023-09-05T13:32:53Z

I know we are in the middle of cutting a release, just wanted to open the PR so I don't forget, we can look into this at a later time.

Andreagit97 · 2023-09-05T14:56:01Z

Thank you @Molter73 this is really valuable! we will take a look ASAP, probably after the release :/

Andreagit97 · 2023-09-07T15:20:23Z

CMakeLists.txt

 option(MUSL_OPTIMIZED_BUILD "Enable if you want a musl optimized build" OFF)
 option(USE_BUNDLED_DRIVER "Use the driver/ subdirectory in the build process (only available in Linux)" ON)
 option(ENABLE_DRIVERS_TESTS "Enable driver tests (bpf, kernel module, modern bpf)" OFF)
 option(ENABLE_LIBSCAP_TESTS "Enable libscap unit tests" OFF)
 option(BUILD_SHARED_LIBS "Build libscap and libsinsp as shared libraries" OFF)
 option(ENABLE_VM_TESTS "Enable driver sanity tests" OFF)

+option(MINIMAL_BUILD "Produce a minimal build with only the essential features (no eBPF probe driver, no kubernetes, no mesos, no marathon and no container metadata)" OFF)


just a minor note on the fly on which maybe we want to start discussing:

What about having a unique MINIMAL_BUILD? Is not clear to me why bpf was excluded in principle 🤔

What about enabling also the modern probe in MINIMAL_BUILD? In the end is just another ebpf driver and could be really useful for example in Falco with minimal build because we can test it even if we have no prebuilt drivers

WDYT? @falcosecurity/libs-maintainers

What about enabling also the modern probe in MINIMAL_BUILD?

👍

We can also think of including zlib in the minimal build. I don't see it as a huge issue. wdyt?
cc @gnosek

What about having a unique MINIMAL_BUILD? Is not clear to me why bpf was excluded in principle 🤔

AFAICT, ebpf was excluded to allow for building without linking to zlib, doesn't seem to be any other reason for this. If we can directly add eBPF into the minimal build, this PR will become a lot smaller.

What about enabling also the modern probe in MINIMAL_BUILD?

Because of how everything is setup, it's already possible to set MINIMAL_BUILD and BUILD_LIBSCAP_MODERN_BPF to ON and everything will be setup and work correctly, so no change is needed for this to work (at least when I tried it, it worked this way).

Because of how everything is setup, it's already possible to set MINIMAL_BUILD and BUILD_LIBSCAP_MODERN_BPF to ON and everything will be setup and work correctly, so no change is needed for this to work (at least when I tried it, it worked this way).

oh, great news! never tried! thanks

AFAICT, ebpf was excluded to allow for building without linking to zlib, doesn't seem to be any other reason for this. If we can directly add eBPF into the minimal build, this PR will become a lot smaller.

To be honest i would go for it, so including also zlib, in this way we could avoid yet another cmake option :)

Just to say this out loud, my initial intention was to split the MINIMAL_BUILD into multiple options that could be toggled individually or set them all with MINIMAL_BUILD, but desisted because it would've been a lot of work and I only needed the minimal build to run with eBPF. We can discuss this alternative approach if it's something that would be interesting for the community.

yeah until we don't have a clear use case I would avoid putting this burden on your shoulders :) If we find alternative use cases i would be happy to consider this splitting you proposed!

gnosek · 2023-09-07T16:05:50Z

TBH I'm not sure why we even have a dedicated minimal build mode in the first place. Ideally, we'd have feature flags for all the major features that MINIMAL_BUILD disables and then you'd either disable e.g. zlib (which would take eBPF away AIUI) or not, independent from other features.

Until that day comes, 👍 from me, although the checks like https://github.com/falcosecurity/libs/pull/1325/files#diff-3ef409c4a80658bffe80055929e4bfd351c6dc05877b1eb1162b19b76a3c9842R24 start to become ugly

Molter73 · 2023-09-07T16:17:18Z

Until that day comes, 👍 from me, although the checks like https://github.com/falcosecurity/libs/pull/1325/files#diff-3ef409c4a80658bffe80055929e4bfd351c6dc05877b1eb1162b19b76a3c9842R24 start to become ugly

Yeah, it's ugly as hell, I apologize for it.

dwindsor · 2023-09-11T13:41:56Z

Hello! We definitely need something like this, as we're using the modern eBPF probe in our closed source application - meaning that we have to use MINIMAL_BUILD.

Currently, we're patching userspace/libscap/CMakeLists.txt as such, and it just works:

diff -purN orig/userspace/libscap/CMakeLists.txt patched/userspace/libscap/CMakeLists.txt
--- orig/userspace/libscap/CMakeLists.txt	2023-08-21 10:37:15.177875298 -0500
+++ patched/userspace/libscap/CMakeLists.txt	2023-08-21 10:54:44.366683611 -0500
@@ -21,9 +21,13 @@ option(BUILD_LIBSCAP_MODERN_BPF "Enable
 
 include(ExternalProject)
 
-if(WIN32 OR NOT MINIMAL_BUILD)
-	include(zlib)
-endif()
+#if(WIN32 OR NOT MINIMAL_BUILD OR BUILD_LIBSCAP_MODERN_BPF)
+#	include(zlib)
+#endif()

Andreagit97 · 2023-09-11T13:44:21Z

If we all agree here, I think we can go with only one MINIMAL_BUILD option which allows:

kmod
legacy bpf
modern bpf

FedeDP · 2023-09-11T13:45:59Z

Fully agree with Andrea! 👍 from me

Molter73 · 2023-09-11T15:21:21Z

If the general consensus is that we should just allow all drivers to be used with the MINIMAL_BUILD I'll gladly change the PR, I've already started looking into this.

Molter73 · 2023-09-12T13:26:03Z

I've changed the PR to always link scap against zlib and libelf, as well as always adding the bpf engine. IMO, it's a lot more streamlined, but let me know if you see any downsides to this approach and would like it implemented in some other way.

FedeDP · 2023-09-12T13:36:55Z

I love this, much simplier cmake files too.
I agree with @gnosek : sooner or later we'll want to directly expose BUILD_LIBSCAP_ENGINE_X options, and improve the situation. But today is not that day.
👍 for this approach from me!

Also, we are currently frozen for the 0.13 tag, but are nearing the code thaw; if we all agree, we could merge this one right after the thaw!

FedeDP · 2023-09-12T13:37:08Z

/usr/bin/ld: cannot find -l/usr/lib/x86_64-linux-gnu/libz.so: No such file or directory
/usr/bin/ld: cannot find -l/usr/lib/x86_64-linux-gnu/libelf.so: No such file or directory

😆

Molter73 · 2023-09-12T13:42:22Z

/usr/bin/ld: cannot find -l/usr/lib/x86_64-linux-gnu/libz.so: No such file or directory
/usr/bin/ld: cannot find -l/usr/lib/x86_64-linux-gnu/libelf.so: No such file or directory

😆

I really should've seen this coming, shouldn't I? 🤣

FedeDP · 2023-09-26T15:29:58Z

Hi @Molter73 can you rebase this one?

FedeDP · 2023-09-26T15:31:40Z

userspace/libscap/settings.h

@@ -20,8 +20,6 @@ limitations under the License.
 //
 #define INCLUDE_UNKNOWN_SOCKET_FDS

-#ifndef MINIMAL_BUILD
 #define USE_ZLIB


Can't we drop USE_ZLIB altogether since it is always true now?

At least on linux ;) i see it being used in scap_zlib.h as follows:

#if defined(USE_ZLIB) && !defined(UDIG) && !defined(_WIN32)

This can become

#if !defined(UDIG) && !defined(_WIN32)

(also, note that since we might drop udig support per #1368, the check would become even simpler!)

FedeDP · 2023-09-27T07:19:23Z

Hi @Molter73 can you rebase this one?

It seems there is no need :) About my other comment, we can fix that while removing UDIG support IMHO. Not a big deal.

FedeDP · 2023-09-27T07:20:03Z

cmake/modules/libscap.cmake

-			list(APPEND libscap_link_libraries ${install_lib_link_library})
+			if(${install_lib_link_library} MATCHES "/")
+				# We have a path. Convert it to -L<dir> + -l<lib>.
+				get_filename_component(scap_lib_dir ${install_lib_link_library} DIRECTORY)


FYI @geraldcombs

FedeDP

/approve

poiana · 2023-09-27T15:44:31Z

LGTM label has been added.

Git tree hash: 53b3a41d41f70213360d93f21c1dc2d691515589

Historically, the minimal build was only able to run using the kernel module driver, but since the addition of the modern probe this has also become an option for running with minimal builds. I believe there's value in allowing adopters of the libs to use the minimal build with the eBPF probe. This change means that scap will always be linked against zlib and libelf, but I believe this is a worthwhile trade off. Signed-off-by: Mauro Ezequiel Moltrasio <[email protected]>

Signed-off-by: Mauro Ezequiel Moltrasio <[email protected]>

Molter73 · 2023-09-29T07:58:21Z

Just finished rebasing, sorry for the delay, I've had a busy couple of days.

FedeDP

/approve

poiana · 2023-09-29T08:01:16Z

LGTM label has been added.

Git tree hash: abf233cbf6dbef80e89782719ea91f84ea899eee

Andreagit97

/approve

poiana · 2023-09-29T08:19:58Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,Molter73]

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/feature New feature or request dco-signoff: yes area/build size/M approved labels Sep 5, 2023

poiana requested review from hbrueckner and leogr September 5, 2023 13:32

Andreagit97 added this to the 0.14.0 milestone Sep 5, 2023

Andreagit97 reviewed Sep 7, 2023

View reviewed changes

Molter73 force-pushed the minimal-build-with-ebpf branch from 3bd8be7 to f972b8a Compare September 12, 2023 13:24

Molter73 requested a review from Andreagit97 September 18, 2023 10:32

FedeDP reviewed Sep 26, 2023

View reviewed changes

FedeDP reviewed Sep 27, 2023

View reviewed changes

FedeDP approved these changes Sep 27, 2023

View reviewed changes

poiana assigned FedeDP Sep 27, 2023

poiana added the lgtm label Sep 27, 2023

Molter73 added 2 commits September 29, 2023 09:57

build(scap): add libdirs to pkgconfig

083c886

Signed-off-by: Mauro Ezequiel Moltrasio <[email protected]>

Molter73 force-pushed the minimal-build-with-ebpf branch from cc1bc28 to 083c886 Compare September 29, 2023 07:57

poiana removed the lgtm label Sep 29, 2023

poiana requested a review from FedeDP September 29, 2023 07:57

FedeDP approved these changes Sep 29, 2023

View reviewed changes

poiana added the lgtm label Sep 29, 2023

Andreagit97 approved these changes Sep 29, 2023

View reviewed changes

poiana assigned Andreagit97 Sep 29, 2023

poiana merged commit 5333d0b into falcosecurity:master Sep 29, 2023

FedeDP mentioned this pull request Nov 10, 2023

new(docker,pkg/driverbuilder): use cmake instead of makefile template to build kmod and bpf falcosecurity/driverkit#302

Merged

jasondellaluce mentioned this pull request Nov 15, 2023

fix(cmake): solve win32 linking issues with zlib #1484

Merged

FedeDP mentioned this pull request Dec 1, 2023

new(docker): add cmake and git packages to builders images. falcosecurity/driverkit#309

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build(minimal-build): allow using eBPF driver with minimal builds #1325

build(minimal-build): allow using eBPF driver with minimal builds #1325

Molter73 commented Sep 5, 2023 •

edited

Loading

Molter73 commented Sep 5, 2023

Andreagit97 commented Sep 5, 2023

Andreagit97 Sep 7, 2023

leogr Sep 7, 2023

Molter73 Sep 7, 2023

Andreagit97 Sep 7, 2023

Molter73 Sep 7, 2023

Andreagit97 Sep 7, 2023

gnosek commented Sep 7, 2023

Molter73 commented Sep 7, 2023

dwindsor commented Sep 11, 2023 •

edited

Loading

Andreagit97 commented Sep 11, 2023

FedeDP commented Sep 11, 2023

Molter73 commented Sep 11, 2023

Molter73 commented Sep 12, 2023

FedeDP commented Sep 12, 2023

FedeDP commented Sep 12, 2023

Molter73 commented Sep 12, 2023

FedeDP commented Sep 26, 2023

FedeDP Sep 26, 2023

FedeDP Sep 26, 2023

FedeDP commented Sep 27, 2023

FedeDP Sep 27, 2023

geraldcombs Sep 27, 2023

FedeDP left a comment

poiana commented Sep 27, 2023

Molter73 commented Sep 29, 2023

FedeDP left a comment

poiana commented Sep 29, 2023

Andreagit97 left a comment

poiana commented Sep 29, 2023

build(minimal-build): allow using eBPF driver with minimal builds #1325

build(minimal-build): allow using eBPF driver with minimal builds #1325

Conversation

Molter73 commented Sep 5, 2023 • edited Loading

Molter73 commented Sep 5, 2023

Andreagit97 commented Sep 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnosek commented Sep 7, 2023

Molter73 commented Sep 7, 2023

dwindsor commented Sep 11, 2023 • edited Loading

Andreagit97 commented Sep 11, 2023

FedeDP commented Sep 11, 2023

Molter73 commented Sep 11, 2023

Molter73 commented Sep 12, 2023

FedeDP commented Sep 12, 2023

FedeDP commented Sep 12, 2023

Molter73 commented Sep 12, 2023

FedeDP commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FedeDP commented Sep 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Sep 27, 2023

Molter73 commented Sep 29, 2023

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Sep 29, 2023

Andreagit97 left a comment

Choose a reason for hiding this comment

poiana commented Sep 29, 2023

Molter73 commented Sep 5, 2023 •

edited

Loading

dwindsor commented Sep 11, 2023 •

edited

Loading