new(config): add `container_engines` config to falco.yaml #3266

incertum · 2024-06-27T18:50:26Z

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:

Fixes #3258

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(config): add `container_engines` config to falco.yaml

incertum · 2024-06-27T18:50:44Z

/milestone 0.39.0

incertum · 2024-06-27T18:52:16Z

Re unit tests: Curious to know where to add tests as for most configs we do not have any here except for example for engine_modern_config.yaml etc. And / or likely more tests in libs make more sense given we just call some API here?

userspace/falco/configuration.cpp

FedeDP · 2024-06-28T06:24:06Z

falco.yaml

+    enabled: true
+  cri:
+    enabled: true
+    cri: ["/run/containerd/containerd.sock", "/run/crio/crio.sock", "/run/k3s/containerd/containerd.sock"]


Suggested change

cri: ["/run/containerd/containerd.sock", "/run/crio/crio.sock", "/run/k3s/containerd/containerd.sock"]

sockets: ["/run/containerd/containerd.sock", "/run/crio/crio.sock", "/run/k3s/containerd/containerd.sock"]

FedeDP · 2024-06-28T06:24:14Z

falco.yaml

+  cri:
+    enabled: true
+    cri: ["/run/containerd/containerd.sock", "/run/crio/crio.sock", "/run/k3s/containerd/containerd.sock"]
+    disable-cri-async: false


Suggested change

disable-cri-async: false

disable-async: false

@leogr? Re both names we ok with having them different to the CLI args?

Btw ok with me.

FedeDP · 2024-06-28T06:25:21Z

I'd also add a debug print of enabled container engines during the startup of Falco ;)

FedeDP · 2024-06-28T06:27:35Z

And / or likely more tests in libs make more sense given we just call some API here?

Yes, agree! Also, you could add some tests to https://github.com/falcosecurity/testing/blob/main/tests/falco/config_test.go; for example you could test that enabling each container engine yields a debug log (aforementioned) with the requested engine enabled.

leogr · 2024-06-28T16:58:10Z

falco.yaml

+  rocket:
+    enabled: true


AFAIK rocket was the initial name, but its official name is rkt

Still, rkt was discontinued in 2020 (https://github.com/rkt/rkt), so I don't see any compelling reason to keep it.

@falcosecurity/libs-maintainers wdyt? 🤔

Oh I didn't know, yes can change it to rkt.

@falcosecurity/libs-maintainers wdyt? 🤔

Agree, we can clean it up; +1 from me!

Removed it. It's not relevant for Falco as you pointed out.

incertum · 2024-06-28T21:16:16Z

userspace/falco/app/actions/init_inspectors.cpp

 		}
 	}

 	// Decide whether to do sync or async for CRI metadata fetch
 	inspector->set_cri_async(!s.options.disable_cri_async);

+	if(s.options.disable_cri_async || s.config->m_container_engines_disable_cri_async)
+	{
+		falco_logger::log(falco_logger::level::DEBUG, "Disabling async lookups for 'CRI'");


Would be nice to extend the API and add a is_cri_async and a get_cri_socket_paths methods. If you also think it would be good I can do it during this dev cycle, but no rush.

falcosecurity/libs#2022 also added a label "good first issue" if someone wanted to get started helping us.

FedeDP · 2024-07-02T07:17:47Z

So, TestFalco_Legacy_TimeIso8601 is failing because it sees:

Fri Jun 28 21:37:49 2024: Enabled container engine 'docker'
        	            	Fri Jun 28 21:37:49 2024: Enabled container engine 'podman'
        	            	Fri Jun 28 21:37:49 2024: Enabled container engine 'CRI'
        	            	Fri Jun 28 21:37:49 2024: Enabled container engine 'lxc'
        	            	Fri Jun 28 21:37:49 2024: Enabled container engine 'libvirt_lxc'
        	            	Fri Jun 28 21:37:49 2024: Enabled container engine 'bpm'

log before the initial:

    	            	2024-06-28T21:37:49+0000: Falco version: 0.39.0-33+0dfb7b8 (x86_64)

string.
I don't get it though, since the Falco version one is logged by load_config that is the first app::run step, while your new logs come from init_inspectors step.

FedeDP · 2024-07-02T07:18:04Z

Aside from ☝️ , LGTM!

incertum · 2024-07-02T15:22:35Z

So, TestFalco_Legacy_TimeIso8601 is failing because it sees:

Fri Jun 28 21:37:49 2024: Enabled container engine 'docker'
        	            	Fri Jun 28 21:37:49 2024: Enabled container engine 'podman'
        	            	Fri Jun 28 21:37:49 2024: Enabled container engine 'CRI'
        	            	Fri Jun 28 21:37:49 2024: Enabled container engine 'lxc'
        	            	Fri Jun 28 21:37:49 2024: Enabled container engine 'libvirt_lxc'
        	            	Fri Jun 28 21:37:49 2024: Enabled container engine 'bpm'

log before the initial:

    	            	2024-06-28T21:37:49+0000: Falco version: 0.39.0-33+0dfb7b8 (x86_64)

string. I don't get it though, since the Falco version one is logged by load_config that is the first app::run step, while your new logs come from init_inspectors step.

The test should not be dependent on the order? If so, we should adjust the test anyways. Above ones are generated while parsing the config.

FedeDP · 2024-07-03T08:59:50Z

The test should not be dependent on the order?

Yes but the test enforces that the very first log line should be Falco version..., and that makes sense from an UX perspective IMHO.
I'd move the logs here from config loading to inspector init time (also if you think that makes sense because in that case, we would only log when the syscall inspector is actually in use, but we won't log if we are eg: plugin only).

FedeDP · 2024-07-04T07:07:55Z

We need to fix #3270 in order to fix CI, since centos7 is eol.

leogr · 2024-07-18T14:12:01Z

We need to fix #3270 in order to fix CI, since centos7 is eol.

Is #3274 enough? 🤔

FedeDP · 2024-08-26T15:07:20Z

Hey Melissa, we were finally able to bump libs in Falco master, can you rebase this one? :)

incertum · 2024-08-26T17:08:07Z

Hey Melissa, we were finally able to bump libs in Falco master, can you rebase this one? :)

Thanks @FedeDP

FedeDP · 2024-08-26T17:27:07Z

userspace/falco/configuration.cpp

-	m_metrics_include_empty_values(false)
+	m_metrics_include_empty_values(false),
+	m_container_engines_mask(0),
+	m_container_engines_cri_socket_paths({"/run/containerd/containerd.sock", "/run/crio/crio.sock","/run/k3s/containerd/containerd.sock"}),


Not for now but i'd expose these defaults as static strings from libs cri namespace, like: libsinsp::container_engines::cri::s_default_socket_paths.

FedeDP · 2024-08-26T17:27:48Z

userspace/falco/configuration.cpp

The schema_json in this file must be updated to match the newly added config key.

This will also fix CI of course.

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Co-authored-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Leonardo Grasso <me@leonardograsso.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

…pector Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

FedeDP

/approve

poiana · 2024-08-27T06:12:40Z

LGTM label has been added.

Git tree hash: 4a0d14e5afced1c00a54495c6fbb28b65032016c

poiana · 2024-08-27T06:12:40Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [FedeDP,incertum]

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

poiana added release-note kind/feature dco-signoff: yes area/engine labels Jun 27, 2024

poiana requested review from FedeDP and Kaizhe June 27, 2024 18:50

poiana added approved size/L labels Jun 27, 2024

poiana added this to the 0.39.0 milestone Jun 27, 2024

This was referenced Jun 27, 2024

Falco 0.38.0 - k8s specific fields are not populated any more #3243

Open

update(cri): cri-dockerd support falcosecurity/libs#1907

Merged

FedeDP reviewed Jun 28, 2024

View reviewed changes

leogr reviewed Jun 28, 2024

View reviewed changes

incertum commented Jun 28, 2024

View reviewed changes

leogr mentioned this pull request Jul 18, 2024

falco: allow multiple socket paths for containerd falcosecurity/charts#655

Open

incertum force-pushed the container-engines-config branch 2 times, most recently from b2ece6a to 425adc3 Compare August 26, 2024 17:04

FedeDP reviewed Aug 26, 2024

View reviewed changes

incertum and others added 5 commits August 26, 2024 17:40

new(config): add container_engines config to falco.yaml

16c248d

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

update(engine): address reviewers comments wrt container_engines config

ade4daa

Co-authored-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Leonardo Grasso <me@leonardograsso.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

update(engine): move some contaienr engines debug message to init ins…

946a7e2

…pector Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

chore: update desc in falco.yaml

6e438df

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

chore: updat config schema w/ container_engines

4363c7b

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

incertum force-pushed the container-engines-config branch from 425adc3 to 4363c7b Compare August 26, 2024 18:08

incertum mentioned this pull request Aug 26, 2024

Add public is_cri_async and a get_cri_socket_paths methods to CRI container engine falcosecurity/libs#2022

Open

FedeDP approved these changes Aug 27, 2024

View reviewed changes

poiana assigned FedeDP Aug 27, 2024

poiana added the lgtm label Aug 27, 2024

LucaGuerra approved these changes Aug 27, 2024

View reviewed changes

poiana assigned LucaGuerra Aug 27, 2024

poiana merged commit 8a3cb76 into falcosecurity:master Aug 27, 2024
33 checks passed

FedeDP mentioned this pull request Sep 17, 2024

chore(userspace/falco): deprecate cri related CLI options. #3329

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new(config): add `container_engines` config to falco.yaml #3266

new(config): add `container_engines` config to falco.yaml #3266

incertum commented Jun 27, 2024

incertum commented Jun 27, 2024

incertum commented Jun 27, 2024

FedeDP Jun 28, 2024

FedeDP Jun 28, 2024

incertum Jun 28, 2024

incertum Jun 28, 2024

FedeDP commented Jun 28, 2024

FedeDP commented Jun 28, 2024

leogr Jun 28, 2024

incertum Jun 28, 2024

FedeDP Jul 1, 2024

incertum Jul 1, 2024

incertum Jun 28, 2024

incertum Aug 26, 2024

FedeDP commented Jul 2, 2024

FedeDP commented Jul 2, 2024

incertum commented Jul 2, 2024

FedeDP commented Jul 3, 2024

FedeDP commented Jul 4, 2024

leogr commented Jul 18, 2024

FedeDP commented Aug 26, 2024

incertum commented Aug 26, 2024

FedeDP Aug 26, 2024

FedeDP Aug 26, 2024

FedeDP Aug 26, 2024

FedeDP left a comment

poiana commented Aug 27, 2024

poiana commented Aug 27, 2024

	cri: ["/run/containerd/containerd.sock", "/run/crio/crio.sock", "/run/k3s/containerd/containerd.sock"]
	sockets: ["/run/containerd/containerd.sock", "/run/crio/crio.sock", "/run/k3s/containerd/containerd.sock"]

new(config): add container_engines config to falco.yaml #3266

new(config): add container_engines config to falco.yaml #3266

Conversation

incertum commented Jun 27, 2024

incertum commented Jun 27, 2024

incertum commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FedeDP commented Jun 28, 2024

FedeDP commented Jun 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FedeDP commented Jul 2, 2024

FedeDP commented Jul 2, 2024

incertum commented Jul 2, 2024

FedeDP commented Jul 3, 2024

FedeDP commented Jul 4, 2024

leogr commented Jul 18, 2024

FedeDP commented Aug 26, 2024

incertum commented Aug 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Aug 27, 2024

poiana commented Aug 27, 2024

new(config): add `container_engines` config to falco.yaml #3266

new(config): add `container_engines` config to falco.yaml #3266