ROX-33614: Update Falco to 0.23.1#2976
Open
Stringy wants to merge 7 commits into
Open
Conversation
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Stringy
force-pushed
the
giles/claude-update-falco-skill
branch
5 times, most recently
from
March 16, 2026 08:20
c2385e2 to
e647cac
Compare
Stringy
changed the title
Stringy
marked this pull request as ready for review
March 16, 2026 08:53
ovalenti
changed the title
Stringy
force-pushed
the
giles/claude-update-falco-skill
branch
from
June 4, 2026 12:58
70ea6aa to
cd3770b
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR centralizes container ID resolution through cgroup parsing, removes container metadata wiring, updates process, network, event, and system-inspector paths, migrates collector tests to the threadinfo factory API, and adds host PID integration coverage. ChangesContainer ID and process inspection
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant HostPIDTest
participant ContainerExecutor
participant Collector
participant Sensor
HostPIDTest->>ContainerExecutor: Start container with PidMode host
ContainerExecutor->>Collector: Create nginx and sleep processes
Collector->>Sensor: Report container-associated processes
Sensor-->>HostPIDTest: Return nginx and sleep processes
HostPIDTest->>Sensor: Query processes with empty container ID
Sensor-->>HostPIDTest: Return no host processes
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Stringy
force-pushed
the
giles/claude-update-falco-skill
branch
from
June 5, 2026 08:53
544901c to
23c2967
Compare
Molter73
reviewed
Jun 15, 2026
Molter73
left a comment
Molter73
left a comment
Member
There was a problem hiding this comment.
I haven't gone through the changes under .claude or AGENTS.md, I think those probably should moved to their own PR.
Even without those there are plenty of comments, the falco side looks fine though.
|
|
||
| CLOG(DEBUG) << "Process (" << signal->container_id() << ": " << signal->pid() << "): " | ||
| << signal->name() << "[" << container_metadata_.GetNamespace(event) << "] " | ||
| << signal->name() |
Member
There was a problem hiding this comment.
Suggested change
| << signal->name() | |
| << signal->name() << "[" << container_id << "] " |
Member
There was a problem hiding this comment.
Actually, I'm an idiot, the container ID is clearly printed in the line above this one 🤦🏻
Can we go back to not having this here?
Member
There was a problem hiding this comment.
Why do the timeouts in this test need to be bumped from 5 to 30 seconds?
- Update falcosecurity-libs from 0.18.1 to 0.23.1 - Fix BPF verifier failures on older kernels (4.18) - Fix clang-format lint in Utility.cpp - Skip fd-based execs (/dev/fd/N) in exepath fallback - Disable TOCTOU 64-bit progs for missing syscalls - Remove container plugin, use built-in container ID lookups - Add analyze-ci Claude skill - Add update-falco-libs Claude skill Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use event type constants instead of strncmp for send/recv detection - Revert extract() back to extract_single() (still available in pinned falco) - Change null filter_check guards to asserts (fatal on init failure) - Move default_formatter_ back to initializer list - Remove try/catch around container filter (let exception propagate) - Use GetContainerID helper in Process.cpp instead of inline cgroup loop - Restore container ID in operator<< and debug log output
Stringy
force-pushed
the
giles/claude-update-falco-skill
branch
from
July 8, 2026 13:19
23c2967 to
6d57248
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
collector/lib/ContainerMetadata.cpp (1)
10-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the unused
EventExtractorsetup fromContainerMetadata
GetNamespace()andGetContainerLabel()no longer useevent_extractor_orinspector_, so the constructor’sEventExtractor::Init()call is dead work. Remove the cached members and simplify the constructor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@collector/lib/ContainerMetadata.cpp` around lines 10 - 28, ContainerMetadata still initializes and stores EventExtractor/inspector even though GetNamespace() and GetContainerLabel() no longer use them, so remove the unused member setup. Simplify the ContainerMetadata::ContainerMetadata constructor by dropping the EventExtractor::Init(inspector) call and any related cached state, and update the class to only keep the symbols actually used by GetNamespace(const std::string&) and GetContainerLabel().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@collector/lib/ContainerMetadata.cpp`:
- Around line 10-28: ContainerMetadata still initializes and stores
EventExtractor/inspector even though GetNamespace() and GetContainerLabel() no
longer use them, so remove the unused member setup. Simplify the
ContainerMetadata::ContainerMetadata constructor by dropping the
EventExtractor::Init(inspector) call and any related cached state, and update
the class to only keep the symbols actually used by GetNamespace(const
std::string&) and GetContainerLabel().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2f9927b3-1b5c-4b33-bdf1-1bb0f2447a31
📒 Files selected for processing (22)
collector/CMakeLists.txtcollector/lib/CollectorService.cppcollector/lib/ContainerEngine.hcollector/lib/ContainerInfoInspector.cppcollector/lib/ContainerInfoInspector.hcollector/lib/ContainerMetadata.cppcollector/lib/NetworkSignalHandler.cppcollector/lib/Process.cppcollector/lib/ProcessSignalFormatter.cppcollector/lib/ProcessSignalFormatter.hcollector/lib/Utility.cppcollector/lib/Utility.hcollector/lib/system-inspector/EventExtractor.cppcollector/lib/system-inspector/EventExtractor.hcollector/lib/system-inspector/Service.cppcollector/lib/system-inspector/Service.hcollector/test/ProcessSignalFormatterTest.cppcollector/test/SystemInspectorServiceTest.cppcollector/test/UtilityTest.cppfalcosecurity-libsintegration-tests/suites/k8s/namespace.gointegration-tests/suites/udp_networkflow.go
💤 Files with no reviewable changes (3)
- collector/lib/ContainerEngine.h
- collector/lib/system-inspector/Service.h
- collector/lib/ContainerInfoInspector.h
The sinsp filter parser treats the RHS of '!=' as a bare string value, not a field reference. Using val(proc.vpid) tells the parser to resolve proc.vpid as a field extraction at runtime, enabling the field-to-field comparison needed to filter host processes. Also fix clang-format alignment on the FIELD_CSTR macro.
Molter73
reviewed
Jul 14, 2026
Molter73
left a comment
Molter73
left a comment
Member
There was a problem hiding this comment.
Overall looks good, I would like to get confirmation on the sinsp header removal comment and I believe there is still one comment from my previous review that has not been addressed.
Comment on lines
+8
to
+10
| if (!check) { | ||
| CLOG(FATAL) << "Filter check not available for field: " << wrapper->event_name; | ||
| } |
Member
There was a problem hiding this comment.
[nitpick]
Suggested change
| if (!check) { | |
| CLOG(FATAL) << "Filter check not available for field: " << wrapper->event_name; | |
| } | |
| CLOG_IF(!check, FATAL) << "Filter check not available for field: " << wrapper->event_name; |
Comment on lines
+65
to
+68
| auto factory = std::make_shared<sinsp_filter_factory>( | ||
| inspector_.get(), EventExtractor::FilterList()); | ||
| sinsp_filter_compiler compiler(factory, "proc.pid != val(proc.vpid)"); | ||
| inspector_->set_filter(compiler.compile(), "proc.pid != val(proc.vpid)"); |
Member
There was a problem hiding this comment.
This should be equivalent:
Suggested change
| auto factory = std::make_shared<sinsp_filter_factory>( | |
| inspector_.get(), EventExtractor::FilterList()); | |
| sinsp_filter_compiler compiler(factory, "proc.pid != val(proc.vpid)"); | |
| inspector_->set_filter(compiler.compile(), "proc.pid != val(proc.vpid)"); | |
| inspector_->set_filter("proc.pid != val(proc.vpid)"); |
Comment on lines
+61
to
+63
| // Filter out host processes. In containers, pid != vpid due to PID | ||
| // namespacing. This is a built-in sinsp field that doesn't require | ||
| // any plugin. |
Member
There was a problem hiding this comment.
I think this may still need addressing, was a manual test at least run to verify if the behavior is consistent?
Comment on lines
+84
to
+94
| auto type = evt->get_type(); | ||
| bool is_send_recv = (type >= PPME_SOCKET_SENDTO_E && type <= PPME_SOCKET_RECVFROM_X) || | ||
| (type >= PPME_SOCKET_SENDMSG_E && type <= PPME_SOCKET_RECVMMSG_X); | ||
| if (!is_send_recv) { | ||
| if (fd_info->is_socket_failed()) { | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| if (fd_info->is_socket_pending()) { | ||
| return std::nullopt; | ||
| } |
Member
There was a problem hiding this comment.
Might want to do something closer to what falco does:
Suggested change
| auto type = evt->get_type(); | |
| bool is_send_recv = (type >= PPME_SOCKET_SENDTO_E && type <= PPME_SOCKET_RECVFROM_X) || | |
| (type >= PPME_SOCKET_SENDMSG_E && type <= PPME_SOCKET_RECVMMSG_X); | |
| if (!is_send_recv) { | |
| if (fd_info->is_socket_failed()) { | |
| return std::nullopt; | |
| } | |
| if (fd_info->is_socket_pending()) { | |
| return std::nullopt; | |
| } | |
| switch (evt->get_type()) { | |
| case PPME_SOCKET_SENDTO_E: | |
| case PPME_SOCKET_SENDTO_X: | |
| case PPME_SOCKET_RECV_E: | |
| case PPME_SOCKET_RECV_X: | |
| case PPME_SOCKET_RECVFROM_E: | |
| case PPME_SOCKET_RECVFROM_X: | |
| case PPME_SOCKET_SENDMSG_E: | |
| case PPME_SOCKET_SENDMSG_X: | |
| case PPME_SOCKET_SENDMMSG_E: | |
| case PPME_SOCKET_SENDMMSG_X: | |
| case PPME_SOCKET_RECVMSG_E: | |
| case PPME_SOCKET_RECVMSG_X: | |
| case PPME_SOCKET_RECVMMSG_E: | |
| case PPME_SOCKET_RECVMMSG_X: | |
| if (fd_info->is_socket_failed()) { | |
| return std::nullopt; | |
| } | |
| if (fd_info->is_socket_pending()) { | |
| return std::nullopt; | |
| } | |
| break; | |
| default: | |
| break; |
It's more verbose, but also more explicit about what operations should make these checks. The compiler should still be able to optimize the case conditions to range checks.
|
|
||
| #include <uuid/uuid.h> | ||
|
|
||
| #include <libsinsp/sinsp.h> |
Member
There was a problem hiding this comment.
Oh lord, no, please let's not start polluting the code base with sinsp headers again, we even have a proposal to stop doing this (#3502).
AFAICT, this is only needed for the sinsp_thread_manager definition, can we use a forward definition in the header file like we do with sinsp and sinsp_threadinfo?
Member
There was a problem hiding this comment.
Actually, I tried locally removing it and it just compiled, so maybe it is not needed at all.
|
|
||
| CLOG(DEBUG) << "Process (" << signal->container_id() << ": " << signal->pid() << "): " | ||
| << signal->name() << "[" << container_metadata_.GetNamespace(event) << "] " | ||
| << signal->name() |
Member
There was a problem hiding this comment.
Actually, I'm an idiot, the container ID is clearly printed in the line above this one 🤦🏻
Can we go back to not having this here?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration-tests/suites/host_pid_process.go`:
- Around line 78-85: Update TestNoHostProcesses to create or trigger a known
host-process event before asserting, then poll Sensor().Processes("") with a
bounded timeout until processing completes. Remove the unconditional 10-second
sleep and ensure the assertion verifies the triggered host process is excluded
rather than passing when no event was observed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ce967c2c-1f28-4c48-a915-8bf30ffbf2cc
📒 Files selected for processing (10)
collector/lib/NetworkSignalHandler.cppcollector/lib/ProcessSignalFormatter.cppcollector/lib/system-inspector/EventExtractor.cppcollector/lib/system-inspector/Service.cppintegration-tests/integration_test.gointegration-tests/pkg/config/container_config.gointegration-tests/pkg/executor/executor_cri.gointegration-tests/pkg/executor/executor_docker_api.gointegration-tests/suites/host_pid_process.gointegration-tests/suites/udp_networkflow.go
🚧 Files skipped from review as they are similar to previous changes (5)
- collector/lib/system-inspector/EventExtractor.cpp
- collector/lib/NetworkSignalHandler.cpp
- collector/lib/system-inspector/Service.cpp
- integration-tests/suites/udp_networkflow.go
- collector/lib/ProcessSignalFormatter.cpp
Comment on lines
+78
to
+85
| func (s *HostPidProcessTestSuite) TestNoHostProcesses() { | ||
| // Give collector some time to process events, then check that no | ||
| // host processes (empty container ID) have been reported. | ||
| time.Sleep(10 * time.Second) | ||
|
|
||
| hostProcesses := s.Sensor().Processes("") | ||
| s.Assert().Empty(hostProcesses, | ||
| "Expected no host processes (empty container ID) to be reported, but found %d", len(hostProcesses)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make host-process exclusion deterministic.
Line [81] adds a fixed 10-second delay, but the test never creates or waits for a known host-process event. If none arrives during that window, Processes("") is empty and the test passes without validating filtering. Trigger a deterministic host process and poll with a bounded timeout instead of sleeping unconditionally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@integration-tests/suites/host_pid_process.go` around lines 78 - 85, Update
TestNoHostProcesses to create or trigger a known host-process event before
asserting, then poll Sensor().Processes("") with a bounded timeout until
processing completes. Remove the unconditional 10-second sleep and ensure the
assertion verifies the triggered host process is excluded rather than passing
when no event was observed.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Falco PR: stackrox/falcosecurity-libs#97
The changes in this PR relate to the uplift of Falco to the latest tagged version, 0.23.1. This is a significant upgrade, jumping from 0.18.1 up to this latest version and contains the following notable changes to Collector:
This work was performed primarily by Claude, with oversight from me. This was something of an experiment and so I got Claude to do as much of the work as possible, with my role being that of the driver; steering Claude when it got muddled or hyper focused on the wrong fix.
The update and rebase itself was relatively straight forward, resulting in new locally-built and locally-verified builds in just a couple of hours (i.e. builds that built and ran locally, passing the integration tests on my Fedora 42 x86 machine.) The bottle neck became the CI and getting that work fed back into Claude to perform the diagnostics and fixes.
As a result, I have added two Claude skills as part of this PR which allow Claude to (1) perform Falco updates in the future and (2) inspect the state of CI and investigate test failures or BPF verifiers issues.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Built and tested locally, all unit & integration tests passing. CI handles the remainder of our test matrix.