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(pkg,cmd): refactored builder script logic. #324

Merged
merged 5 commits into from
Mar 25, 2024

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Mar 13, 2024

What type of PR is this?

/kind cleanup
/kind feature

Any specific area of the project related to this PR?

/area cmd
/area pkg

What this PR does / why we need it:

Builder script has been split in 3 different scripts:

  • download libs
  • download headers
  • build

This way, we can reuse download libs script among all of them. Moreover, it is useful to have a download headers script that is invokeable by itself, because it has the logic to download and extract headers for a given distro.

Finally, fixed a couple of things with local builder:

  • redirect stderr to stdout so that we catch errors too while building
  • pre initialize envMap to an empty map, instead of nil
  • allow to automatically download kernel headers

Basically, what we want to achieve here is to allow falcoctl driver loader to be able to fetch KernelURLs using driverkit libraries, and then extract kernel urls using the newly provided templated bash scripts (note: this is a distro specific knowledge, we cannot just "untar" the headers), and finally pass the KERNELDIR env variable back to local build processor.
In the end, this approach will enable us to drop the kernel headers dependency for Falco installations on distros supported by driverkit, at least when the headers are actually found by driverkit itself.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Note that ~400Locs come from new _kernel.sh templates licenses.

Does this PR introduce a user-facing change?:

NONE

@@ -72,7 +72,7 @@ func NewLocalCmd(rootCommand *RootCmd, rootOpts *RootOptions, rootFlags *pflag.F
})
flagSet.BoolVar(&opts.useDKMS, "dkms", false, "Enforce usage of DKMS to build the kernel module.")
flagSet.StringVar(&opts.srcDir, "src-dir", "", "Enforce usage of local source dir to build drivers.")
flagSet.StringToStringVar(&opts.envMap, "env", nil, "Env variables to be enforced during the driver build.")
flagSet.StringToStringVar(&opts.envMap, "env", make(map[string]string), "Env variables to be enforced during the driver build.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default to an empty map instead of nil. Not a big deal anyway.

@@ -21,6 +21,9 @@ import (
"github.com/falcosecurity/driverkit/pkg/kernelrelease"
)

//go:embed templates/alinux_kernel.sh
var alinuxKernelTemplate 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.

Each builder does now expose a kernel download/extract template too.

parsed, err := t.Parse(b.TemplateScript())
// TemplateDataSpecifier is an optional interface implemented by builders
// to specify a custom template data instead of the default one.
type TemplateDataSpecifier interface {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TemplateData method is now optional since config.ToTemplateData is enough most of the time (for now, the only exception is local builder; and i think it will remain it).

@@ -305,13 +368,12 @@ func Targets() []string {
func (c Config) toTemplateData(b Builder, kr kernelrelease.KernelRelease) commonTemplateData {
c.setGCCVersion(b, kr)
return commonTemplateData{
DriverBuildDir: DriverDirectory,
ModuleDownloadURL: fmt.Sprintf("%s/%s.tar.gz", c.DownloadBaseURL, c.DriverVersion),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ModuleDownloadURL is only needed at libs download time, therefore it is now part of libsDownloadTemplateData.

@@ -48,7 +37,11 @@ mkdir -p build && cd build
echo "* Building kmod with DKMS"
# Build the module using DKMS
echo "#!/usr/bin/env bash" > "/tmp/falco-dkms-make"
echo "make CC={{ .GCCVersion }} \$@" >> "/tmp/falco-dkms-make"
if [[ -z "${KERNELDIR}" ]]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Manage kerneldir in dkms script.

@FedeDP FedeDP changed the title new(pkg,cmd): refactored builder script logic. wip: new(pkg,cmd): refactored builder script logic. Mar 13, 2024
Builder script has been split in 3 different scripts:
* download libs
* download headers
* build

This way, we can reuse `download libs` script among all of them.
Moreover, it is useful to have a download headers script that is invokeable by itself,
because it has the logic to download and extract headers for a given distro.

Finally, fixed a couple of things with local builder:
* redirect stderr to stdout so that we catch errors too while building
* pre initialize envMap to an empty map, instead of nil
* manage KERNELDIR env var, if set, while building with dkms

The last point allows for consumer to pass `KERNELDIR` inside `envMap` local builder processor
argument to customize the build.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP FedeDP force-pushed the new/refactor_builder_logic branch from 4153ffa to ec18417 Compare March 13, 2024 12:13
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
sourcedir=$(find . -type d -name "{{ .KernelHeadersPattern }}" | head -n 1 | xargs readlink -f)

# Patch makefile to avoid using absolute `/usr/src` path; instead use `..` relative one.
sed -i 's/\/usr\/src/../g' $sourcedir/Makefile
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In master, we used to install everything under /usr/src.
This is not convenient in case anybody is going to run this script on localhost (ie: not through docker); we now avoid that.
Instead, we need to patch main headers Makefile to avoid absolute path inclusion of secondary makefile.
I tested on multiple versions and this assumption holds true.

Moreover, let KernelDownloadScript method take just kernelUrls instead of full build config.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
FedeDP added a commit to falcosecurity/falcoctl that referenced this pull request Mar 14, 2024
Also, bumped driverkit to falcosecurity/driverkit#324 HEAD.

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

FedeDP commented Mar 14, 2024

Here is the output for falcoctl leveraging the new kernel downloader script: falcosecurity/falcoctl#476 (comment)

FedeDP added a commit to falcosecurity/falcoctl that referenced this pull request Mar 14, 2024
Also, bumped driverkit to falcosecurity/driverkit#324 HEAD.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
FedeDP added a commit to falcosecurity/falcoctl that referenced this pull request Mar 14, 2024
Also, bumped driverkit to falcosecurity/driverkit#324 HEAD.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP FedeDP changed the title wip: new(pkg,cmd): refactored builder script logic. new(pkg,cmd): refactored builder script logic. Mar 14, 2024
@@ -117,7 +127,8 @@ func (bp *KubernetesBuildProcessor) buildModule(b *builder.Build) error {

buildCmd := []string{
"/bin/bash",
"/driverkit/driverkit.sh",
"-l",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same fix as #321

srcDir string
envMap map[string]string
useDKMS bool
downloadHeaders bool
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Local cmd now supports automatically downloading kernel headers for the build.
Note that it is not a fatal error if kernel headers are not found; it will just continue trying with locally installed ones, if any.

…in local builder.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP FedeDP force-pushed the new/refactor_builder_logic branch from 723d1bb to 98d1211 Compare March 14, 2024 10:13
mv usr/src/kernels/*/* /tmp/kernel

# exit value
echo /tmp/kernel
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugliest part of the PR...the _kernel.sh template scripts MUST return the path to be used as KERNELDIR by echoing it to stdout, since it will then be used as env for the build script.

This means that the _kernel.sh script CANNOT echo anything else, since their stdout must just return the kerneldir path.
I could not find any better way to achieve this (well, we could create an output file, but that would be still ugly imho; and then we've got a new file to be cleaned up too).

if err != nil {
return err
}

runCmd :=
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 create a new cmd that makes required scripts executables and then :

@@ -39,9 +47,50 @@ func (lbp *LocalBuildProcessor) String() string {
func (lbp *LocalBuildProcessor) Start(b *builder.Build) error {
slog.Debug("doing a new local build")

if lbp.useDKMS {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the check from above, so that external library users gain it for free.

// We don't want to download headers
kr := b.KernelReleaseFromBuildConfig()

if lbp.downloadHeaders {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Download headers support; super simple:

  • now local command too requires a valid target type
  • if the builder is not found, we just skip kernel headers download. Should never happen inside driverkit because target CLI option is validated, but can happen when used by external library users
  • once created the target, we just call the KernelDownloadScript method to generate the kernel download/extracting script for the target distro
  • we run the script capturing the output, see again https://github.com/falcosecurity/driverkit/pull/324/files#r1524730462
  • we add the KERNELDIR env var to the localbuilderprocessor env, that will then be used when running the local.sh build script.

@@ -86,6 +135,20 @@ func (lbp *LocalBuildProcessor) Start(b *builder.Build) error {
// Fetch paths were kmod and probe will be built
srcModulePath := vv.GetModuleFullPath(c, kr)
srcProbePath := vv.GetProbeFullPath(c)

if len(lbp.srcDir) == 0 {
slog.Info("Downloading driver sources.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Downloading driver sources is now done in place using the new LibsDownloadScript, just like docker build processor does.

} else {
cmd.Stderr = cmd.Stdout // redirect stderr to stdout so that we catch it
Copy link
Contributor Author

@FedeDP FedeDP Mar 14, 2024

Choose a reason for hiding this comment

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

Redirect stderr to stdout. This is an improvement.

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

FedeDP commented Mar 25, 2024

/cc @EXONER4TED

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

@poiana
Copy link

poiana commented Mar 25, 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

@poiana poiana merged commit 0f094fc into master Mar 25, 2024
5 checks passed
@poiana poiana deleted the new/refactor_builder_logic branch March 25, 2024 14:28
FedeDP added a commit to falcosecurity/falcoctl that referenced this pull request Mar 25, 2024
Also, bumped driverkit to falcosecurity/driverkit#324 HEAD.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
poiana pushed a commit to falcosecurity/falcoctl that referenced this pull request Mar 28, 2024
Also, bumped driverkit to falcosecurity/driverkit#324 HEAD.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
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