Build other eBPF programs

afdesk commented 3 years ago

Hello!

Motivation

I am interested in using driverkit to build other eBPF programs besides Falco (currently working with Tracee).

Feature

I would like to propose some changes to make driverkit more generic, and I can implement those if the maintainers agree:

add custom templates for the ebpf builders
use different docker images for the ebpf builders
add options to disable some actions for the driverkit docker process (for prepare driver config and makefile templates)

And the main change: implement a way to use linux headers for all the kernel releases.

Now driverkit is looking for distros from ubuntu repositories (https://mirrors.edge.kernel.org/ubuntu/pool/main/l/linux) with next versions of kernels: ... 4.4.x, 4.15.x, 5.4.x, 5.8.x, 5.11.x. We need to support all the kernel versions.

Happy to hear what you think and if you have other suggestions for supporting this use case!

leodido commented 3 years ago

Hey, @afdesk I like where this idea seems to go!

I would like to propose some changes to make driverkit more generic, and I can implement those if the maintainers agree:

add custom templates for the ebpf builders

+1

use different docker images for the ebpf builders

I guess you're referring to builder.Dockerfile right?

If so, agree! +1 Having a mechanism to select a different "builder container image" can be also useful for things like Linuxkit (see this branch I worked on a bit in the past).

add options to disable some actions for the driverkit docker process (for prepare driver config and makefile templates)

Could you elaborate more on what do you mean here?

I want to be sure I understood correctly :)

And the main change: implement a way to use linux headers for all the kernel releases.

Could you please extend on this too?

Now driverkit is looking for distros from ubuntu repositories (https://mirrors.edge.kernel.org/ubuntu/pool/main/l/linux) with next versions of kernels: ... 4.4.x, 4.15.x, 5.4.x, 5.8.x, 5.11.x. We need to support all the kernel versions.

The mirrors/repositories where to look can be easily added, anyway.

Happy to hear what you think and if you have other suggestions for supporting this use case!

Generally speaking, I love the concept of generalizing it and I would love to have such contributions!

afdesk commented 3 years ago

Hi @leodido! Nice to read your opinion!

I'll try to write more details about my needs.

use different docker images for the ebpf builders I guess you're referring to builder.Dockerfile right?

I want to set a custom image name to builderBaseImage variable. this change will allow us to use preconfigured docker image for building the ebpf program.

add options to disable some actions for the driverkit docker process (for prepare driver config and makefile templates) Could you elaborate more on what do you mean here?

the docker processor creates kernel.config, module-Makefile and module-driver-config.h. These files don't need for our the ebpf program. I would like to have an option for disabling these renders.

And the main change: implement a way to use linux headers for all the kernel releases. Could you please extend on this too?

This is our main goal. I understood that we can add new mirrors for the mirrors/repositories, but these new endpoinds should contain files/folders with the same format and we have to set all these arguments (Fullversion, kernelVersion etc):

/linux-headers-*Fullversion*FullExtraversion*_*Fullversion*-*firstExtra*.*kernelVersion*_amd64.deb",

I would like to use the repositories with all versions of the kernel headers where the folder/file names have different format. Example for ubuntu linux headers kernel.ubuntu.com/~kernel-ppa/mainline/.

Also, It will be well if we can use this builder without PatchLevel by default. we can use the last patch for the specif kernel version of linux headers.

Maybe I missed something about drivekit...

Thanks for your time!

leodido commented 3 years ago

Hi @leodido! Nice to read your opinion!

Nice to see you interested in improving and generalizing it!

I'll try to write more details about my needs.

use different docker images for the ebpf builders I guess you're referring to builder.Dockerfile right?

I want to set a custom image name to builderBaseImage variable. this change will allow us to use preconfigured docker image for building the ebpf program.

Makes sense.

add options to disable some actions for the driverkit docker process (for prepare driver config and makefile templates) Could you elaborate more on what do you mean here?

the docker processor creates kernel.config, module-Makefile and module-driver-config.h. These files don't need for our the ebpf program. I would like to have an option for disabling these renders.

Understood. And I do agree.

The only thing I would argue is: in order to avoid an explosion of option/parameter combinations that may or may not be required with a given builder base image, wouldn't be optimal to think of a way to strip down the whole option selection to a single "configuration"?

Example: when using builder base image X you also get option A, B, and C enabled by default.

Otherwise, a lot of conditional validators may be required on options A, B, and C to disallow users to use them with builder not requiring them.

Asking what you think 'cause I know (by experience) that too generic isn't always good, just that ;)

And the main change: implement a way to use linux headers for all the kernel releases. Could you please extend on this too?

This is our main goal. I understood that we can add new mirrors for the mirrors/repositories, but these new endpoinds should contain files/folders with the same format and we have to set all these arguments (Fullversion, kernelVersion etc):
/linux-headers-*Fullversion*FullExtraversion*_*Fullversion*-*firstExtra*.*kernelVersion*_amd64.deb",
I would like to use the repositories with all versions of the kernel headers where the folder/file names have different format. Example for ubuntu linux headers kernel.ubuntu.com/~kernel-ppa/mainline/.

If I don't recall it wrong we've handled a similar requirements in other builders different from the ubuntu ones.

In any case, creating another ubuntu builder type that fetches the things with a different logic is probably the easiest way to go.

Also, It will be well if we can use this builder without PatchLevel by default. we can use the last patch for the specif kernel version of linux headers.

Creating another builder type would ease also the handling of this requirement, IMO.

Maybe I missed something about drivekit...

I honestly don't think you missed anything important! :)

Thanks for your time!

afdesk commented 3 years ago

Hi @leodido! I needed time to learn driverkit options.

in order to avoid an explosion of option/parameter combinations that may or may not be required with a given builder base image,

we will use an image name to define a scope of options and a template for ebpf builder, right?

creating another ubuntu builder type that fetches the things with a different logic is probably the easiest way to go.

If you agree, I can write and offer my custom builder for our ebpf program.

Also I think that the best place for new options is ConfigOptions.

It seems that I need to add next fields:

builderType - a ubuntu builder type, i think about two options: stable, all. is it ok?
dockerImage - docker images for the ebpf builders

maybe this text is a little crumpled.

afdesk commented 3 years ago

Hi @leodido! There is a new idea for customizing driverkit. For options/parameters I would use environment variables rather than yaml settings. I'll take a fewer changes.

what do you think about it? thanks!

afdesk commented 3 years ago

Hi @leodido! I've created a little sample with changes which we need for building the custom ebpf program (for tracee). You can look the PR here: github.com/afdesk/my-driverkit/pull/2/files

I'd like to know your opinion about merging such changes into driverkit. Thank you very much!

itaysk commented 3 years ago

Hi @ledido, can you please comment?

afdesk commented 3 years ago

Hi @leodido! I've updated a PR, and did code more customized. Now I use next command to build the ebpf program:

DRIVERKIT_BUILDER_IMAGE=aquasec/tracee-ebpf-builder:v1 DRIVERKIT_UBUNTU_TEMPLATE_PATH=traceeUbuntuTemplate.sh ./driverkit docker -c ubuntu.yaml

ubuntu.yaml:

kernelrelease: 5.4.0-51-generic
kernelversion: 56
target: ubuntu-generic
output:
  probe: dist/tracee.bpf.generic.0.o
driverversion: dev
timeout: 120

traceeUbuntuTemplate.sh:

#!/bin/bash
rm -Rf {{ .DriverBuildDir }}
mkdir {{ .DriverBuildDir }}
cd {{ .DriverBuildDir }}
mkdir bpf
{{range $url := .KernelDownloadURLS}}
curl --silent -o kernel.deb -SL {{ $url }}
ar x kernel.deb
tar -xf data.tar.*
{{end}}
cd /tracee
KERN_HEADERS={{ .DriverBuildDir }}/usr/src/{{ .KernelHeadersPattern }} KERN_RELEASE=generic make bpf
cp /tracee/dist/tracee.bpf.generic.0.o /tmp/driver/bpf/probe.o

afdesk commented 3 years ago

I've added a support for kernel headers from ubuntu kernel mainline here is a new PR: github.com/afdesk/my-driverkit/pull/3/files.

there is no need to specify kernelversion. sample of ubuntu.yaml:

kernelrelease: 4.19.5
#kernelrelease: 4.18-rc4
target: ubuntu-generic
output:
  probe: dist/tracee.bpf.generic.0.o
driverversion: dev
timeout: 120

leodido commented 3 years ago

Ok @afdesk I've taken a look.

First of all thanks for approaching this. As said, I'd love to have a more generic/abstract driverkit!

Some thoughts.

Using environment variables

To be totally honest, I'd prefer if we could keep (and I think it's totally doable) the driverkit CLI interface "stable". Meaning, not introducing different usage patterns for different use cases.

What I mean is that, the driverkit CLI already supports automatic environment variables for RootOptions fields.

Here's ⬇️ where the automatic env vars get set up.

https://github.com/falcosecurity/driverkit/blob/af65823ae6f64158e5f03e2cd1f1a5311ecc3937/cmd/root.go#L217-L218

As you can see in the CLI test suite, the environment variables get merged with the CLI flags.

https://github.com/falcosecurity/driverkit/blob/af65823ae6f64158e5f03e2cd1f1a5311ecc3937/cmd/cli_test.go#L112-L130

https://github.com/falcosecurity/driverkit/blob/af65823ae6f64158e5f03e2cd1f1a5311ecc3937/cmd/cli_test.go#L144-L160

Furthermore, an override mechanism is already built in so that the following precedence applies: flags > env vars > YAML config file.

Which means, if you run:

DRIVERKIT_KERNELVERSION=60 ./_output/bin/driverkit docker -c cmd/testdata/configs/1.yaml --loglevel=debug --dryrun

The driverkit tool will use (see logs):

kernelversion=60

While, if you run:

DRIVERKIT_KERNELVERSION=60 ./_output/bin/driverkit docker -c cmd/testdata/configs/1.yaml --loglevel=debug --dryrun --kernelversion=61

The driverkit tool will use (see logs):

kernelversion=61

So, I believe we should not change this behaviour for other use cases. Instead, we should use it to implement new use cases like yours :) and benefit from it giving always the same experience to users disregarding the builder in use.

Builder image

Now, about the builder image (and the relative shell script to use).

My idea here was to use the same concepts I expressed above.

We could define a new option - let's say .BuilderImage - with the default being the current builder image. It's still not clear to me if it best belongs to ConfigOptions (probably this) or to RootOptions.

Then we could even pre-define in the source code a map (BuilderImage, Target) -> (ShellTemplate).

An instance of this map would be:

(aquasec/tracee-ebpf-builder:v1, ubuntu-mainline) -> traceeUbuntuTemplate.sh

Conclusions

What do you think? :)

These are all the feedback that came to my mind in this late Friday afternoon 😱 😂

Feel free to contact me on the Kubernetes slack (either privately or on the Falco channel) if you need.

afdesk commented 3 years ago

Hi @leodido! thanks for your answer.

yes, I agree with you about environment variable. I'll change it. it was the fastest way to show and test our ideas.

also, i'll try to add a new option into ConfigOptions. and I agree too about a new target type ubuntu-mainline. I'll add it and retest.

Your feedback is really useful, will change my code )

itaysk commented 3 years ago

Hi @leodido , so we have a POC that is implementing those changes. before we make a PR, we wanted to suggest a small feature that will possibly help us and others with building other programs: right now driverkit expects the builder image to contain the source code of the probe. We prefer not to build and store a new builder image for every commit in the probe source code. Currently our builder image contains just the tooling required to build the code, and the code itself is mounted (-v) into the builder when the container starts. So we were thinking about allowing the driverkit user to specify volumes that needs to be mounted into the container when it starts. WDYT?

leodido commented 3 years ago

Hey!

I think it's a good idea, tbh. L.

On Fri, Jul 9, 2021 at 4:22 PM Itay Shakury @.***> wrote:

Hi @leodido https://github.com/leodido , so we have a POC that is implementing those changes. before we make a PR, we wanted to suggest a small feature that will possible help us and others with building other programs: right now driverkit expects the builder image to contain the source code of the probe. We prefer not to build and store a new builder image for every commit in the probe source code. Currently our builder image contains just the tooling required to build the code, and the code itself is mounted (-v) into the builder when the container starts. So we were thinking about allowing the driverkit user to specify volumes that needs to be mounted into the container when it starts https://github.com/falcosecurity/driverkit/blob/af65823ae6f64158e5f03e2cd1f1a5311ecc3937/pkg/driverbuilder/docker.go#L111. WDYT?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/falcosecurity/driverkit/issues/100#issuecomment-877225893, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA5J4ZSF5GXYN372FAZ4C3TW4AZTANCNFSM445ZXSRQ .

itaysk commented 3 years ago

Great! any special guidance on how you would like to see the configuration exposed? cc: @afdesk

afdesk commented 3 years ago

Hi @leodido. I could add a new config option volume to rootOpts. is it ok? thanks!

leodido commented 3 years ago

Hello folks,

now that I'm reasoning more about it lemme do an observation...

We prefer not to build and store a new builder image for every commit in the probe source code.

I'm not getting why you would need to build and store a new builder image for every commit/version of your probe.

The current approach (and that's the reason there is the driverversion option) is to download the probe source code into the container of the builder image.

https://github.com/falcosecurity/driverkit/blob/af65823ae6f64158e5f03e2cd1f1a5311ecc3937/pkg/driverbuilder/builder/centos.go#L161

https://github.com/falcosecurity/driverkit/blob/af65823ae6f64158e5f03e2cd1f1a5311ecc3937/pkg/driverbuilder/builder/builders.go#L48-L50

You could do the same... If not, why?

I agree that the volume approach generally sounds correct and it's a good idea. Anyway, if we introduce it, the driverversion has no more reasons to stay and driverkit loses the concept of the version the user is building. Which is something I'd personally avoid.

poiana commented 3 years ago

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

itaysk commented 3 years ago

We will probably not pursue this route anymore since we came up with a better approach for us (GitHub.com/aquasecurity/btfhub). So we can close this one

poiana commented 2 years ago

Stale issues rot after 30d of inactivity.

Mark the issue as fresh with /remove-lifecycle rotten.

Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle rotten

poiana commented 2 years ago

Rotten issues close after 30d of inactivity.

Reopen the issue with /reopen.

Mark the issue as fresh with /remove-lifecycle rotten.

Provide feedback via https://github.com/falcosecurity/community. /close

poiana commented 2 years ago

@poiana: Closing this issue.

In response to [this](https://github.com/falcosecurity/driverkit/issues/100#issuecomment-990573653): >Rotten issues close after 30d of inactivity. > >Reopen the issue with `/reopen`. > >Mark the issue as fresh with `/remove-lifecycle rotten`. > >Provide feedback via https://github.com/falcosecurity/community. >/close 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.

falcosecurity / driverkit