Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from
Feb 28, 2024

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Nov 10, 2023

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area pkg

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Docker images changes will be split to their own PR if maintainers agree this is the way to go.
Also: while testing i discovered that recent archlinux kmods fail to build (eg: against 6.6.1.arch1-1) even with master images and code.

level=DEBUG msg="\x02\x00\x00\x00\x00\x00\x00\x7f./tools/objtool/objtool: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.38' not found (required by ./tools/objtool/objtool)\n"

Will investigate further, but it seems we need a builder image with an updated glibc version.
Opened an issue to track that: #303

Does this PR introduce a user-facing change?:

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

git

# Install cmake3.x (on centos7 `cmake` package installs cmake2.x)
RUN curl -L -o /tmp/cmake.tar.gz https://github.com/Kitware/CMake/releases/download/v3.22.5/cmake-3.22.5-linux-$(uname -m).tar.gz; \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to manually install a cmake 3.x on centos:7.


cp /driverkit/module-Makefile {{ .DriverBuildDir }}/Makefile
bash /driverkit/fill-driver-config.sh {{ .DriverBuildDir }}
mv /tmp/module-download/*/* {{ .DriverBuildDir }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scripts are a bit different now:

  • we move all the stuff under the download libs repo to{{ .DriverBuildDir }} now, not only the driver subfolder
  • we run cmake with lots of things disabled. Unfortunately, given that libs only recently gained the ability to build the bpf probe in MINIMAL_BUILD mode, we need to configure the project in full mode, and this requires git because grpc and protobuf deps need to be fetched/cloned.
  • Then, building is just a matter of calling make driver and make bpf with the correct CC and KERNELDIR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually really like how much this all simplifies the code, and I think the trade offs are worth it.

@FedeDP
Copy link
Contributor Author

FedeDP commented Nov 15, 2023

cc @falcosecurity/driverkit-maintainers

@EXONER4TED
Copy link
Contributor

I think this is awesome! Which would you prefer though - we can put this through now, or do #308 first and then rebase this?

@FedeDP
Copy link
Contributor Author

FedeDP commented Dec 1, 2023

Let's go with #308 first! I will split the docker images cmake part from this so that we can actually test that this works perfectly fine using --builderimage auto:master

@FedeDP
Copy link
Contributor Author

FedeDP commented Dec 1, 2023

I will also need to update the new local builder to use cmake when downloading sources. (this is a reminder for myself!)

@FedeDP
Copy link
Contributor Author

FedeDP commented Dec 1, 2023

Split #309 out of this !

@FedeDP
Copy link
Contributor Author

FedeDP commented Dec 12, 2023

Need more fixes to the new local (as in "internal to containers") module and probe paths (that will now be /tmp/driver/build/driver/$drivername.ko and /tmp/driver/build/driver/bpf/probe.o).
Everything else seems working fine though ;)

@@ -55,6 +43,14 @@ type Config struct {
*Build
}

func (c Config) ToDriverFullPath() string {
return path.Join(DriverDirectory, "build", "driver", fmt.Sprintf("%s.ko", c.DriverName))
Copy link
Contributor Author

@FedeDP FedeDP Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We expect the built kmod with cmake to be under /tmp/driver/build/driver/$drivername.ko.

return path.Join(DriverDirectory, "build", "driver", fmt.Sprintf("%s.ko", c.DriverName))
}

func (c Config) ToProbeFullPath() string {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We expect the built probe with cmake to be under /tmp/driver/build/driver/bpf/probe.o.

{{ if .BuildModule }}
# Build the module
cd {{ .DriverBuildDir }}
make CC=/usr/bin/gcc-{{ .GCCVersion }} KERNELDIR=/tmp/kernel
mv {{ .ModuleDriverName }}.ko {{ .ModuleFullPath }}
Copy link
Contributor Author

@FedeDP FedeDP Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need the mv anymore since ModuleFullPath is already the correct path.

@@ -102,11 +102,11 @@ func (bp *KubernetesBuildProcessor) buildModule(b *builder.Build) error {
return err
}

if builder.ModuleFullPath != "" {
if c.ModuleFilePath != "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following are small fixes in the kubernetes builder. cc @Lowaiz

@@ -275,15 +255,15 @@ func (bp *KubernetesBuildProcessor) copyModuleAndProbeFromPodWithUID(ctx context
}
if p.Status.Phase == corev1.PodRunning {
slog.With(falcoBuilderUIDLabel, falcoBuilderUID).Info("start downloading module and probe from pod")
if builder.ModuleFullPath != "" {
err = copySingleFileFromPod(build.ModuleFilePath, bp.coreV1Client, bp.clientConfig, p.Namespace, p.Name, builder.ModuleFullPath, moduleLockFile)
if c.ModuleFilePath != "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same fix as above.

if err != nil {
return err
}
slog.Info("Kernel Module extraction successful")
}
if builder.ProbeFullPath != "" {
err = copySingleFileFromPod(build.ProbeFilePath, bp.coreV1Client, bp.clientConfig, p.Namespace, p.Name, builder.ProbeFullPath, probeLockFile)
if c.ProbeFilePath != "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same fix as above.

@FedeDP
Copy link
Contributor Author

FedeDP commented Dec 13, 2023

TODO: i need to also fix the local builder with local sources case; there, we won't need to configure anything since the project is already configured.

@FedeDP
Copy link
Contributor Author

FedeDP commented Dec 19, 2023

/hold for review

@EXONER4TED
Copy link
Contributor

So my vote here is to merge this, try it downstream, and then report back. I love this change, and we're ready to merge it. If something breaks downstream, we can handle it in a separate MR/release

@FedeDP
Copy link
Contributor Author

FedeDP commented Feb 28, 2024

Nice! I will make sure to rebase asap and let the flow go on :)

EXONER4TED
EXONER4TED previously approved these changes Feb 28, 2024
Copy link
Contributor

@EXONER4TED EXONER4TED left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

… to build kmod and bpf.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
…` in template scripts.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
…ute existing dir path.

Moreover, take into account srcDir in local builder: when src-dir is specified, sources do not
need to be configured through cmake.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
…e driverversions.

In CI, enable multiple driverversions to test that we do not break against
old driver versions.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Moreover, add back some now unused cmake variables, ie:
* PROBE_NAME
* PROBE_VERSION
* PROBE_DEVICE_NAME

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
…l` cmd.

Moreover, properly fill CmakeCmd for local target too.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Copy link
Contributor

@EXONER4TED EXONER4TED left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve again :)

@poiana
Copy link

poiana commented Feb 28, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: EXONER4TED, FedeDP

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:

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

@FedeDP
Copy link
Contributor Author

FedeDP commented Feb 28, 2024

/unhold

@poiana poiana merged commit 31c7e6a into master Feb 28, 2024
5 checks passed
@poiana poiana deleted the new/use_cmake branch February 28, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants