-
Notifications
You must be signed in to change notification settings - Fork 920
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
wip: new(build,userspace): switch to use container plugin #3482
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
1575590
to
a52405c
Compare
/milestone 0.41.0 |
This PR may bring feature or behavior changes in the Falco engine and may require the engine version to be bumped. Please double check userspace/engine/falco_engine_version.h file. See versioning for FALCO_ENGINE_VERSION. /hold |
11663e9
to
03e2350
Compare
Going to remove the wip commit once falcosecurity/testing#69 gets merged. EDIT: removed |
624544b
to
03e2350
Compare
Now the arm64 test-dev-packages is down to 4 errors, 2 of them are related to the usage of the full branch name for libs and driver deps; they will be fixed once we properly use a commit hash. The remaining 2 are actually linked to the container plugin: it seems replaying a capture file we are seeing 2 container events instead of 3:
EDIT: on x86 we also have a couple of ASSERT triggering in libs code:
|
The failing tests come from the fact that we are not able to extract Indeed, we were previously setting tinfo on the generated container json event: https://github.com/falcosecurity/libs/blob/master/userspace/libsinsp/container.cpp#L287; i am trying to understand whether this is reproducible with a plugin ASYNCEVENT. EDIT: fixed; now only the x86 ASSERT fails remain:
|
It seems modern ebpf is hit too:
After adding: sinsp_fdinfo* sinsp_threadinfo::add_fd(int64_t fd, std::unique_ptr<sinsp_fdinfo> fdinfo) {
sinsp_fdtable* fd_table_ptr = get_fd_table();
if(fd_table_ptr == NULL) {
printf("NULL fd_table_ptr: '%s' '%s'\n", m_comm.c_str(), m_cmd_line.c_str());
return NULL;
}
Because the culprit is the podman service started by the podman SDK withing the go-worker of the plugin. Thus no plugin, no issue. I added a commit on top if my libs PR to drop the
Probably there was a time where we always returned
|
It will be shipped by default hence it is present in default config. Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
Also, default falco.yaml will only host container plugin configuration but won't enable the plugin. Instead, a configuration override file will be installed only on linux non-musl deployments, enabled the plugin. Signed-off-by: Federico Di Pierro <[email protected]>
… string. Also, remove `container_id container_name` fields from `-pc` output. These fields are now automatically appended since the `container` plugin marks them as suggested. Signed-off-by: Federico Di Pierro <[email protected]>
03e2350
to
e071b4a
Compare
1 failure left on x86_64:
No idea why this is failing. 2 failures on arm64:
Both of them are present in https://github.com/falcosecurity/falco/actions/runs/13330903373 too (ie: they do not seem to depend on the container work). Many failures on x86_64 musl; that's because we cannot load the plugin on musl, as already stated in the PR body. I think we will just need to remove the required check on it (or just stop testing musl build). |
b44acf7
to
5d0b262
Compare
falcosecurity/testing#71 fixes arm64 kmod and legacyBpf failures. |
5d0b262
to
bf07203
Compare
Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
/reopen |
@FedeDP: Failed to re-open PR: state cannot be changed. The new/container_plugin branch was force-pushed or recreated. In response to this:
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-sigs/prow repository. |
/reopen |
@FedeDP: Reopened this PR. In response to this:
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-sigs/prow repository. |
dec8857
to
c127177
Compare
Container plugin cannot be dynamically loaded on musl build, therefore some falcosecurity/testing tests are failing on it. Signed-off-by: Federico Di Pierro <[email protected]>
c127177
to
fd1ddd3
Compare
What type of PR is this?
/kind cleanup
/kind feature
Any specific area of the project related to this PR?
/area build
/area engine
What this PR does / why we need it:
This PR bumps Falco to HEAD of falcosecurity/libs#2207, enforcing the usage of the container plugin.
Keeping it
wip
until all issues are resolved.TODO:
test_falco_engine.extra_format_do_not_replace_container_info
falco_others.yaml
config (that we install on windows,osx and musl in place of falco.yaml), or we use something likeload_plugins: [@FALCO_DEFAULT_PLUGINS@]
and customizeFALCO_DEFAULT_PLUGINS
from within cmake -> we decided to add the plugin configuration in the main falco.yaml file but then install an override config file that loads it only on non-musl linux installationscontainer_engines:
key: drop in favor of plugin config -> implement TODOs or drop? It is notsandbox
level unfortunately, butIncubating
thus theoretically we'd need a deprecation period. -> before 1.0.0incubating
features can be dropped without deprecation period: https://github.com/falcosecurity/falco/blob/master/proposals/20231220-features-adoption-and-deprecation.md#before-falco-10While it should be up to falcoctl to install the
container
plugin, since it was previously a core feature of libraries, we decided to keep it installed by the Falco package so that host installations through packages won't lose such a feature by default (ie: without playing with falcoctl to install the plugin).Another solution could be to let rpm/deb install scripts leverage
falcoctl
to install the plugin, BUT thetar.gz
tarball would still be hit by the feature loss.Which issue(s) this PR fixes:
Refs #3403
Special notes for your reviewer:
The cares,grpc,openssl and curl cmake files were copy pasted from libs repo.
Does this PR introduce a user-facing change?: