Skip to content

new: falcoctl driver loader#2905

Merged
poiana merged 16 commits intomasterfrom
new/falcoctl_driver_loader
Dec 11, 2023
Merged

new: falcoctl driver loader#2905
poiana merged 16 commits intomasterfrom
new/falcoctl_driver_loader

Conversation

@FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Nov 9, 2023

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area build

What this PR does / why we need it:

This PR drops old falco-driver-loader script in favor of new falcoctl driver command.

Which issue(s) this PR fixes:

Fixes #2675

Special notes for your reviewer:

This is wip because falcoctl's PR (falcosecurity/falcoctl#343) is still to be merged and this will need some more work.
I opened this one to give an idea of the final look.

Does this PR introduce a user-facing change?:

new!: dropped falco-driver-loader script in favor of new falcoctl driver command

@poiana poiana requested review from Kaizhe and leogr November 9, 2023 11:34
@poiana poiana added the size/XXL label Nov 9, 2023
FedeDP
FedeDP commented Nov 9, 2023
View reviewed changes
cmake/modules/falcoctl.cmake Outdated
falcoctl
URL "https://github.com/falcosecurity/falcoctl/releases/download/v${FALCOCTL_VERSION}/falcoctl_${FALCOCTL_VERSION}_${FALCOCTL_SYSTEM_NAME}_${FALCOCTL_SYSTEM_PROC_GO}.tar.gz"
URL_HASH "SHA256=${FALCOCTL_HASH}"
URL "https://github.com/falcosecurity/falcoctl/archive/555594a2860284947ff83eefd4bd9a5abc6e9fe1.zip"
Copy link
Contributor Author

@FedeDP FedeDP Nov 9, 2023

Choose a reason for hiding this comment

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

Download zip from my own falcoctl PR.

docker/driver-loader-legacy/docker-entrypoint.sh Outdated
done

/usr/bin/falco-driver-loader "$@"
/usr/bin/falcoctl driver config "$@"
Copy link
Contributor Author

@FedeDP FedeDP Nov 9, 2023

Choose a reason for hiding this comment

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

This will need some more work, i am not sure what we want to do.

Copy link
Contributor Author

@FedeDP FedeDP Nov 9, 2023

Choose a reason for hiding this comment

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

Same goes for other occurrences.

Copy link
Member

@Andreagit97 Andreagit97 Nov 10, 2023

Choose a reason for hiding this comment

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

This will need some more work, i am not sure what we want to do.

What do you mean? what do we miss here?

Copy link
Contributor Author

@FedeDP FedeDP Nov 10, 2023

Choose a reason for hiding this comment

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

Oh the parameters being passed to falco-driver-loader were surely different from the one being passed to falcoctl driver; also config might be wrong there, since we may want to let users decide which subcmd to invoke.

Copy link
Contributor Author

@FedeDP FedeDP Dec 1, 2023

Choose a reason for hiding this comment

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

We decided to reimplement a similar switch logic to the one supported by falco-driver-loader (https://github.com/falcosecurity/falco/blob/master/scripts/falco-driver-loader#L752) managing each flag.

scripts/debian/postinst.in Outdated
# If needed, try to load/compile the driver through falco-driver-loader
# If needed, try to load/compile the driver through falcoctl
echo "[POST-INSTALL] Configure falcoctl driver type:"
falcoctl driver config --type $chosen_driver
Copy link
Contributor Author

@FedeDP FedeDP Nov 9, 2023

Choose a reason for hiding this comment

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

We now configure the desired driver type, and then build/download the driver using falcoctl.

scripts/falcoctl/falcoctl.yaml.in
Copy link
Contributor Author

@FedeDP FedeDP Nov 9, 2023

Choose a reason for hiding this comment

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

falcoctl config file must now be configured properly.

leogr
leogr reviewed Nov 9, 2023
View reviewed changes
Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

Nice!

Just did a first look and left a few comments.

@FedeDP
Copy link
Contributor Author

FedeDP commented Nov 10, 2023

Note: once #2413 gets merged, this should be at least ready for local testing with the CI produced packages.

@FedeDP FedeDP mentioned this pull request Nov 10, 2023
Closed
6 tasks
@FedeDP
Copy link
Contributor Author

FedeDP commented Nov 10, 2023

build-packages is failing with:

CMake Error at /__w/falco/falco/build/cmake_install.cmake:294 (file):
  file INSTALL cannot find
  "/__w/falco/falco/build/falcoctl-prefix/src/falcoctl/falcoctl": No such
  file or directory.

that is because our falcoctl.cmake file expects falcoctl releases (and is not able to build falcoctl from sources since it would add the huge dep on go), but is now pointing to a falcoctl commit hash zip file.
I would love to fix the CI to be able to test; perhaps we will need to tag a falcoctl alpha release once falcosecurity/falcoctl#343 is merged.

@FedeDP
Copy link
Contributor Author

FedeDP commented Nov 14, 2023 •
edited
Loading

TODO:

  • once new: driver selection in falco.yaml #2413 is merged, update deb/rpm scripts using new chosen_driver names (ie: "kmod", "ebpf" and "modern-ebpf").
  • also, are per-driver systemd units still needed? Btw they should enforce a driver.kind, like -o driver.kind=kmod
    -> we might want to deprecate current units and add a new falco@.service templated unit that enables the requested driver.

@leogr leogr added this to the 0.37.0 milestone Nov 24, 2023
@FedeDP FedeDP mentioned this pull request Nov 27, 2023
Merged
@FedeDP FedeDP force-pushed the new/falcoctl_driver_loader branch from 96a4b38 to d2ebe43 Compare November 27, 2023 14:02
@FedeDP
Copy link
Contributor Author

FedeDP commented Nov 27, 2023

also, are per-driver systemd units still needed? Btw they should enforce a driver.kind, like -o driver.kind=kmod
-> we might want to deprecate current units and add a new falco@.service templated unit that enables the requested driver.

Done.
About the single unit (deprecating existing ones), i haven't got strong opinions.

@FedeDP
Copy link
Contributor Author

FedeDP commented Nov 27, 2023

We just need an alpha tag of falcoctl 0.7.0 to be tested within this PR.

@FedeDP
Copy link
Contributor Author

FedeDP commented Nov 27, 2023

Done: https://github.com/falcosecurity/falcoctl/releases/tag/v0.7.0-alpha1

This was referenced Nov 28, 2023
Merged
Merged
@FedeDP
Copy link
Contributor Author

FedeDP commented Nov 29, 2023

falcosecurity/testing#34 + falcosecurity/testing#33 should allow test-dev-packages checks to pass fine.

@FedeDP FedeDP force-pushed the new/falcoctl_driver_loader branch 3 times, most recently from c294151 to 05dd35a Compare December 1, 2023 12:17
@FedeDP
Copy link
Contributor Author

FedeDP commented Dec 1, 2023

The only failing test left is

TestFalcoLegacyBPF

that will be fixed by falcosecurity/testing#34

@FedeDP FedeDP force-pushed the new/falcoctl_driver_loader branch from 05dd35a to 5a919ff Compare December 4, 2023 11:23
@FedeDP
new(docker,scripts): dropped falco-driver-loader in favor of new `fal…
ec4f26b 
…coctl driver` command.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
FedeDP
FedeDP commented Dec 6, 2023
View reviewed changes
cmake/modules/falcoctl.cmake Outdated
string(TOLOWER ${CMAKE_HOST_SYSTEM_NAME} FALCOCTL_SYSTEM_NAME)

set(FALCOCTL_VERSION "0.6.2")
set(FALCOCTL_VERSION "0.7.0-alpha2")
Copy link
Contributor Author

@FedeDP FedeDP Dec 6, 2023

Choose a reason for hiding this comment

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

Will need to bump to beta1 once it is out.

@FedeDP FedeDP force-pushed the new/falcoctl_driver_loader branch from d3422d6 to c071285 Compare December 6, 2023 10:41
FedeDP added 3 commits December 6, 2023 12:16
@FedeDP
chore(scripts): use new default value for falcoctl driver.host-root c…
278bc1c 
…onfig key.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP
chore(scripts): integrate back master changes FALCO_DRIVER_CHOICE a…
65f88ab 
…nd `FALCOCTL_ENABLED` .

Also, env variables always have precedence over dialog (ie: if they are set, we always skip dialog).

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP
chore(cmake): bumped falcoctl to v0.7.0-beta1.
678ca39 
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP FedeDP force-pushed the new/falcoctl_driver_loader branch from c071285 to 678ca39 Compare December 6, 2023 11:17
FedeDP
FedeDP commented Dec 6, 2023
View reviewed changes
scripts/debian/postinst.in
CHOICE=4
;;
esac
if [ -z $CHOICE ] && [ -x /usr/bin/dialog ] && [ "${FALCO_FRONTEND}" != "noninteractive" ]; then
Copy link
Contributor Author

@FedeDP FedeDP Dec 6, 2023

Choose a reason for hiding this comment

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

Env var has always precedence.

Copy link
Member

@leogr leogr Dec 11, 2023

Choose a reason for hiding this comment

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

I think this needs to be documented cc @falcosecurity/falco-website-maintainers

@FedeDP
update(submodules): bumped falcosecurity-testing to latest dev.
ac71895 
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP
Copy link
Contributor Author

FedeDP commented Dec 6, 2023

Bumped falcosecurity-testing to latest dev to (hopefully) fixup dev test-packages ci.

FedeDP added 2 commits December 6, 2023 13:49
@FedeDP
update(cmake): bump falcoctl to 0.7.0-beta2.
c78be5b 
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP
update(cmake): bumped falcoctl to v0.7.0-beta3.
d2795c5 
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP
Copy link
Contributor Author

FedeDP commented Dec 6, 2023

Ops, we need falcosecurity/testing#37 too ;)

@FedeDP
update(submodules): bumped falcosecurity/testing to latest master.
73be5e9 
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP
Copy link
Contributor Author

FedeDP commented Dec 6, 2023

Also need falcosecurity/falcoctl#369 because compiled eBPF probe is expected to be symlinked to /root/.falco/falco-bpf.o : https://github.com/falcosecurity/falco/blob/master/scripts/falco-driver-loader#L662 and https://github.com/falcosecurity/falco/blob/master/falco.yaml#L321

@FedeDP
update(cmake): bumped falcoctl to v0.7.0-beta4.
dc49d5d 
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP FedeDP changed the title wip: new: falcoctl driver loader new: falcoctl driver loader Dec 6, 2023
@FedeDP
Copy link
Contributor Author

FedeDP commented Dec 6, 2023

Tests are finally passing! 🚀

@FedeDP FedeDP mentioned this pull request Dec 7, 2023
Closed
leogr
leogr approved these changes Dec 11, 2023
View reviewed changes
Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

Just a couple of minor issues found (see comments below), which we can fix in a follow-up PR.
Thus, it's ok for me to merge this so we can start more accurate testing.

docker/driver-loader-legacy/docker-entrypoint.sh
print_usage() {
echo ""
echo "Usage:"
echo " falco-driver-loader [driver] [options]"
Copy link
Member

@leogr leogr Dec 11, 2023

Choose a reason for hiding this comment

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

Suggested change
echo " falco-driver-loader [driver] [options]"
echo " falco-driver-loader [driver] [options]"

This is misleading since there's no falco-driver-loader executable anymore.

Still trying to figure out how to fix it. May we print the container image usage help message instead? 🤔

Copy link
Contributor Author

@FedeDP FedeDP Dec 11, 2023

Choose a reason for hiding this comment

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

You are right! Will fix in a follow up PR

docker/driver-loader/docker-entrypoint.sh
print_usage() {
echo ""
echo "Usage:"
echo " falco-driver-loader [driver] [options]"
Copy link
Member

@leogr leogr Dec 11, 2023

Choose a reason for hiding this comment

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

Same as above

docker/falco/docker-entrypoint.sh
print_usage() {
echo ""
echo "Usage:"
echo " falco-driver-loader [driver] [options]"
Copy link
Member

@leogr leogr Dec 11, 2023

Choose a reason for hiding this comment

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

same as above

scripts/debian/postinst.in
CHOICE=4
;;
esac
if [ -z $CHOICE ] && [ -x /usr/bin/dialog ] && [ "${FALCO_FRONTEND}" != "noninteractive" ]; then
Copy link
Member

@leogr leogr Dec 11, 2023

Choose a reason for hiding this comment

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

I think this needs to be documented cc @falcosecurity/falco-website-maintainers

@poiana poiana added the lgtm label Dec 11, 2023
Andreagit97
Andreagit97 approved these changes Dec 11, 2023
View reviewed changes
Copy link
Member

@Andreagit97 Andreagit97 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
Contributor

poiana commented Dec 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [Andreagit97,FedeDP,leogr]

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 e177898 into master Dec 11, 2023
@poiana poiana deleted the new/falcoctl_driver_loader branch December 11, 2023 15:37
@FedeDP FedeDP mentioned this pull request Dec 12, 2023
Merged
This was referenced Feb 27, 2026
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leogr leogr leogr approved these changes

@Andreagit97 Andreagit97 Andreagit97 approved these changes

@Kaizhe Kaizhe Awaiting requested review from Kaizhe

Assignees

@leogr leogr

@Andreagit97 Andreagit97

Labels

approved area/build dco-signoff: yes kind/feature lgtm release-note size/XXL

Projects

Falco Roadmap
Archived in project

Milestone

0.37.0

Development

Successfully merging this pull request may close these issues.

Deprecate falco-driver-loader

4 participants

@FedeDP @poiana @leogr @Andreagit97