new: driver selection in falco.yaml by therealbobo · Pull Request #2413 · falcosecurity/falco

therealbobo · 2023-02-09T08:36:18Z

Signed-off-by: Roberto Scolaro roberto.scolaro21@gmail.com

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

/kind release

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area engine

/area tests

/area proposals

/area CI

What this PR does / why we need it:

It might be useful to configure the driver mode via the configuration file falco.yaml

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new: driver selection in falco.yaml

Andreagit97 · 2023-02-09T10:15:05Z

Thank you for this! @therealbobo
We discussed many times to uniform how we open engines and this could be a possible way
cc @leogr

leogr · 2023-02-21T15:00:14Z

Thank you for this! @therealbobo We discussed many times to uniform how we open engines and this could be a possible way cc @leogr

It could be a possible solution. However, how could that option interact with other ways to select the driver?
I specifically refer to the FALCO_BPF_PROBE env var and other esoteric options like -u or how we enable the gVisor support.

Let's discuss it!

therealbobo · 2023-02-21T17:43:32Z

I think that the driver selected in falco.yaml should be considered if and only if the driver to be used is not specified in any other way (e.g. env variable or args). Moreover, using the config file, it will possibile to simplify the service management. 🤔

poiana · 2023-08-01T13:32:32Z

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

FedeDP · 2023-11-06T08:15:38Z

/reopen

/remove-lifecycle stale

poiana · 2023-11-06T08:15:42Z

@FedeDP: Reopened this PR.

Details

In response to this:

/reopen

/remove-lifecycle stale

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

FedeDP · 2023-11-06T08:15:46Z

/milestone 0.37.0

FedeDP · 2023-11-06T08:16:14Z

We actually need this for the shiny new falco-driver-loader integrated inside falcoctl: falcosecurity/falcoctl#327

FedeDP · 2023-11-06T08:18:11Z

I'd suggest a rebase, and then:

DEPRECATION NOTICE for FALCO_BPF_PROBE
DEPRECATION NOTICE for cmd line options (note that -u was already dropped in the meantime, so i think that only the modern-bpf related one survives today)

Imho config option should always take precedence over deprecated cmdline/env var, since otherwise falcoctl would not be able to correctly instrument the right driver for Falco.

/cc @leogr

Andreagit97

LGMT!

leogr

Love this. It's a huge improvement, and we will finally get a bit of consistency across our configuration. So, first of all, a big thanks to @therealbobo, @Andreagit97, and @FedeDP 👏

The implementation is already good. I've just left some comments about minor issues I've found when reviewing.

Other than that, I want to ensure that a warning message is printed to the user any time a deprecated option is used (either command line or config). I've already identified a corner case (see my comment below) and I think another similar corner case can happen if the user modified both the old and the new field in the configuration file.

Ping me privately, I can provide more details if needed.

falco.yaml

userspace/falco/app/actions/helpers_inspector.cpp

leogr · 2023-11-23T13:30:19Z

userspace/falco/app/actions/load_config.cpp

+	// at least one change in the default config we don't allow to use the command line options.
+	if(s.config->m_changes_in_engine_config)
+	{
+		return run_result::ok(); 


I'm fine with this behavior. However, I don't like silently ignoring deprecated options here (users may not get what's happening).

So, when the user mistakenly uses BOTH the command line flag AND change the config, we should print something like:

The '-g,--gvisor-config' command line option is deprecated and will be removed in Falco version 0.38. Note that this option is ineffective when 'engine' settings are specified via the configuration file.

I'd add a single log, something like:

if(s.config->m_changes_in_engine_config) { falco_logger::log(falco_logger::level::WARNING, "Since the new 'engine' config key is being used, deprecated CLI options " "[-e,-g,--gvisor-config,--nodriver,--modern-bpf] and FALCO_BPF_PROBE environment variable will be ignored.\n"); return run_result::ok();

Added in latest commit; example output:

Thu Nov 23 15:15:50 2023: Falco version: 0.37.0-146+9bf0565 (x86_64) Thu Nov 23 15:15:50 2023: Falco initialized with configuration file: ../falco.yaml Thu Nov 23 15:15:50 2023: Since the new 'engine' config key is being used, deprecated CLI options [-e,-g,--gvisor-config,--nodriver,--modern-bpf] and FALCO_BPF_PROBE environment variable will be ignored. Thu Nov 23 15:15:50 2023: Loading rules from file ../rules/falco_rules.yaml

Ok. I was thinking of printing the warning only if one of those options was used. But your solution is fine anyway 👍

userspace/falco/app/actions/load_config.cpp

leogr · 2023-11-23T13:35:31Z

userspace/falco/app/actions/load_config.cpp

+
+	// If overridden from CLI options (soon to be removed),
+	// use the requested driver.
+	if (getenv(FALCO_BPF_ENV_VARIABLE))


What will happen when this gets merged, and perhaps some of the users will use the same env var in the config 👇 ?

engine: kind: kmod kmod: buf_size_preset: 4 drop_failed_exit: false ebpf: # path to the elf file to load. probe: ${FALCO_BPF_PROBE}

🤯

Nothing, i mean the deprecated env variable takes precedence.

Nothing, i mean the deprecated env variable takes precedence.

Will it print a warning even if it shouldn't? 🤔

Yes it will print the newly introduced warning about the fact that FALCO_BPF_PROBE env variable will be ignored, and that is correct(since we won't directly use the env variable but the new config key that resolves to the env variable value)

Oh i mis-read (i thought kind: bpf was in use). In this case, as i stated earlier, the deprecated env variable takes precedence (since the bpf kind configuration is not even parsed, because the kind is kmod).
No warning in this case (except for the FALCO_BPF_PROBE env var deprecation )

userspace/falco/configuration.cpp

userspace/falco/configuration.h

Also, properly warn the user that deprecated CLI options will be ignored when the new `engine` configuration key is in use. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

These macros will be used by other files so we need to share them Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

LucaGuerra

Thank you for this! 😮 LGTM, let's merge it so we can test it better. This is something we needed 😅

poiana · 2023-11-24T17:27:19Z

LGTM label has been added.

Details

Git tree hash: f99a5c87c0a21f7dc134467c3c73787ba75ee22f

leogr

For sure we need to deeply test it, but LGTM at the moment!

Thank you'll!

poiana · 2023-11-27T14:00:54Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,LucaGuerra,leogr]

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

Please see: falcosecurity/falco#2413. Signed-off-by: Aldo Lacuku <aldo@lacuku.eu>

poiana added do-not-merge/work-in-progress kind/feature dco-signoff: yes do-not-merge/release-note-label-needed labels Feb 9, 2023

poiana requested review from Andreagit97 and Kaizhe February 9, 2023 08:36

poiana added the size/M label Feb 9, 2023

therealbobo force-pushed the new/mode-in-config branch from beef31d to efc957c Compare February 9, 2023 08:38

therealbobo force-pushed the new/mode-in-config branch from efc957c to 4723660 Compare March 6, 2023 09:01

leogr added this to the 0.36.0 milestone May 3, 2023

poiana added the lifecycle/stale label Aug 1, 2023

Andreagit97 modified the milestones: 0.36.0, 0.37.0, TBD Aug 24, 2023

Andreagit97 mentioned this pull request Aug 31, 2023

Kernel module isn't installed to module dir when using falco-driver-loader --download #2574

Closed

therealbobo closed this Sep 4, 2023

poiana reopened this Nov 6, 2023

poiana removed the lifecycle/stale label Nov 6, 2023

poiana modified the milestones: TBD, 0.37.0 Nov 6, 2023

FedeDP force-pushed the new/mode-in-config branch from ef38a9f to 7b39c8e Compare November 22, 2023 14:35

Andreagit97 reviewed Nov 22, 2023

View reviewed changes

leogr reviewed Nov 23, 2023

View reviewed changes

FedeDP force-pushed the new/mode-in-config branch from eb0689d to a8749b6 Compare November 23, 2023 14:57

chore(userspace): small round of review-induced fixes.

8f3edcc

Also, properly warn the user that deprecated CLI options will be ignored when the new `engine` configuration key is in use. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

FedeDP force-pushed the new/mode-in-config branch from a8749b6 to 8f3edcc Compare November 23, 2023 15:38

leogr mentioned this pull request Nov 23, 2023

Emit a warning when trying to set a non-existing config key #2924

Closed

Andreagit97 added 5 commits November 23, 2023 19:34

cleanup: move some macros inside a shared file

231e134

These macros will be used by other files so we need to share them Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

new: add some deprecation warnings

4b083bb

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

fix: fix some broken tests

0c2f8c0

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

docs: improve a log

3dc8dad

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

cleanup: rename modern-ebpf into modern_ebpf

5ea1359

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

FedeDP mentioned this pull request Nov 24, 2023

new(cmd,internal/utils,pkg/driver): use correct engine.kind config key falcosecurity/falcoctl#357

Merged

LucaGuerra approved these changes Nov 24, 2023

View reviewed changes

poiana assigned LucaGuerra Nov 24, 2023

poiana added the lgtm label Nov 24, 2023

FedeDP mentioned this pull request Nov 27, 2023

Add use of FALCO_DRIVER_CHOICE and FALCOCTL_ENABLED env vars #2773

Merged

leogr approved these changes Nov 27, 2023

View reviewed changes

poiana assigned leogr Nov 27, 2023

poiana merged commit 00b7c56 into falcosecurity:master Nov 27, 2023

alacuku added a commit to alacuku/charts that referenced this pull request Jan 11, 2024

update(falco/driver)!: use the same names for drivers as falco

35be618

Please see: falcosecurity/falco#2413. Signed-off-by: Aldo Lacuku <aldo@lacuku.eu>

alacuku added a commit to alacuku/charts that referenced this pull request Jan 12, 2024

update(falco/driver)!: use the same names for drivers as falco

1a691f0

Please see: falcosecurity/falco#2413. Signed-off-by: Aldo Lacuku <aldo@lacuku.eu>

alacuku added a commit to alacuku/charts that referenced this pull request Jan 12, 2024

update(falco/driver)!: use the same names for drivers as falco

ee393b8

Please see: falcosecurity/falco#2413. Signed-off-by: Aldo Lacuku <aldo@lacuku.eu>

FedeDP mentioned this pull request Jan 17, 2024

[TRACKING] Breaking changes in Falco 0.37.0 #2763

Closed

6 tasks

alacuku added a commit to alacuku/charts that referenced this pull request Jan 23, 2024

update(falco/driver)!: use the same names for drivers as falco

b5f5da6

Please see: falcosecurity/falco#2413. Signed-off-by: Aldo Lacuku <aldo@lacuku.eu>

alacuku added a commit to alacuku/charts that referenced this pull request Jan 24, 2024

update(falco/driver)!: use the same names for drivers as falco

af47534

Please see: falcosecurity/falco#2413. Signed-off-by: Aldo Lacuku <aldo@lacuku.eu>

alacuku added a commit to alacuku/charts that referenced this pull request Jan 24, 2024

update(falco/driver)!: use the same names for drivers as falco

237bda7

Please see: falcosecurity/falco#2413. Signed-off-by: Aldo Lacuku <aldo@lacuku.eu>

leogr mentioned this pull request Apr 18, 2024

update(hooks/UseWasm): update scap option falcosecurity/falco-playground#57

Merged

Conversation

therealbobo commented Feb 9, 2023 • edited by FedeDP Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Andreagit97 commented Feb 9, 2023

Uh oh!

leogr commented Feb 21, 2023

Uh oh!

therealbobo commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poiana commented Aug 1, 2023

Uh oh!

FedeDP commented Nov 6, 2023

Uh oh!

poiana commented Nov 6, 2023

Uh oh!

FedeDP commented Nov 6, 2023

Uh oh!

FedeDP commented Nov 6, 2023

Uh oh!

FedeDP commented Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Andreagit97 left a comment

Choose a reason for hiding this comment

Uh oh!

leogr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FedeDP Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

LucaGuerra left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poiana commented Nov 24, 2023

Uh oh!

leogr left a comment

Choose a reason for hiding this comment

Uh oh!

poiana commented Nov 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

therealbobo commented Feb 9, 2023 •

edited by FedeDP

Loading

therealbobo commented Feb 21, 2023 •

edited

Loading

FedeDP commented Nov 6, 2023 •

edited

Loading

FedeDP Nov 23, 2023 •

edited

Loading

LucaGuerra left a comment •

edited

Loading