Skip to content

test: add container plugin correctness validator and CI - #3940

Open
robbycochran wants to merge 8 commits into
collector-container-pluginfrom
rc-pr3939-replay
Open

test: add container plugin correctness validator and CI#3940
robbycochran wants to merge 8 commits into
collector-container-pluginfrom
rc-pr3939-replay

Conversation

@robbycochran

@robbycochran robbycochran commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Description

Stacked on #3939 (collector-container-plugin). Adds an opt-in correctness validator that feeds synthetic events through the pinned Falco library, loads the production container plugin and checks Collector attribution, filtering and selected network handling. No live kernel capture or Kubernetes cluster is required.

  • 67 deterministic cases covering cgroup layouts, process creation and event ordering, exec refresh, process-ID reuse and connection tracking.
  • A reusable workflow called by main CI with the same builder-tag selection as unit tests. AMD64 and ARM64 run Debug builds with ASan/UBSan and upload logs/XML, preserving failing exit status. No Valgrind job is added.
  • A usage and extension guide, including how to add scenarios and the boundaries of synthetic replay.

This PR changes no production plugin logic and includes no candidate fixes. It remains a draft while the regression cases fail; findings and reproductions link back to the relevant reviews on #3939.

Validation

Local Linux corpus execution completed. Workflow lint and diff whitespace checks pass. CI for the updated ASan/UBSan workflow is pending. The target is opt-in, so normal unit-test builds remain unchanged.

skip-integration-tests avoids the broad VM/Kubernetes matrix for this test-only companion PR; the validator does not need cloud/runtime credentials.

@robbycochran

Copy link
Copy Markdown
Collaborator Author

Regression reproductions against #3939

These are local results against parent 249fe750e930248796254d7d6d7df3e02db4c1ce, with pinned Falco e61430ac73d78137344eda9a21e663cd897b4f59: 54 passing / 13 failing across 67 cases. They describe two root causes, not thirteen independent bugs. This PR supplies tests only; no candidate fixes are included. CI results for the simplified workflow are still pending.

1. A later cgroup entry erases an already found container ID

This reproduces the existing cgroup-iteration review comment on #3939.

Start a process with memory=/kubepods/burstable/pod123/<64-hex-ID> followed by cpuset=/. FindContainerID finds the container, then overwrites it with the empty result from the second entry. CacheContainerID stores host, and the plugin-backed filter rejects subsequent container activity. Reversing the entries preserves the ID.

Three cases cover the focused reproduction plus match-before-host and match-between-hosts corpus entries. The reverse-order case is a positive control. The tests exercise the actual capture callback and Collector attribution, not just the string parser.

bash /src/collector/test/plugin-replay/run-corpus.sh /build /tmp/cgroup-repro \
  --gtest_filter='*MatchingCgroup*:*MatchThenHost*:*MatchBetweenHosts*'

Any fix must preserve a successful match across unrelated entries. If the fix stops iteration early, distinguish a successful early stop from a read error: Falco's iteration return value alone does not distinguish them.

2. A child created from its parent's event has no cached identity

This provides an event-only reproduction of the broader uninitialized-field concern already raised on #3939, plus a downstream connection test.

  1. Start capture with a known host or hostPID-container parent.
  2. Feed the parent's successful fork/clone/clone3 event, without the child's corresponding event.
  3. Falco creates a valid child entry and populates its cgroups. The tests assert these prerequisites before checking the plugin.
  4. The plugin updates the event's parent entry, but the newly created child's cached ID remains empty.
  5. Feed subsequent child activity. A host child passes container.id != host because the cached value is empty, not host. For a hostPID container, the empty ID instead prevents correct connection attribution.

The production NetworkSignalHandler reproduction is particularly useful: with both fork events, it returns PROCESSED and records one connection under the expected container ID. Omit only the child's fork event and it returns IGNORED, leaving ConnectionTracker empty. A separate recovery case confirms that observing the child's event initializes its identity.

bash /src/collector/test/plugin-replay/run-corpus.sh /build /tmp/child-repro \
  --gtest_filter='ContainerPluginReplayTest.HostPIDConnectionTracked*'

Eight failures exercise the actual parent-created-child path: six parameterized cases, one installed-host-filter case and one downstream network case. Two additional failures insert a valid late-discovered process through Falco's real thread manager; these model /proc discovery output, not a live /proc lookup. They support the cache-lifecycle concern but should not be counted as independent event-only reproductions.

Mapping every empty cache value to host would address the host-filter symptom but still discard legitimate container activity. The correctness requirement is to establish the child's actual identity before using it for filtering or connection attribution.

Scope of these findings

Parent-only PID-namespace cases are excluded because their return value is namespace-local and cannot identify the global child entry. The valid parent-only reproductions above use host/hostPID processes. No test writes the plugin's cached ID or mocks callback ordering.

These tests establish behavior for supplied events, not how frequently live capture loses an event. They do not establish Sensor delivery or recovery of the reported CPU regression. Build and extension instructions live in the README.

@robbycochran
robbycochran marked this pull request as ready for review September 8, 2026 23:38
@robbycochran
robbycochran requested a review from a team as a code owner September 8, 2026 23:38
@codecov-commenter

codecov-commenter commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 27.33%. Comparing base (249fe75) to head (2704e8a).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@                     Coverage Diff                     @@
##           collector-container-plugin    #3940   +/-   ##
===========================================================
  Coverage                       27.33%   27.33%           
===========================================================
  Files                              94       94           
  Lines                            5399     5399           
  Branches                         2538     2538           
===========================================================
  Hits                             1476     1476           
  Misses                           3197     3197           
  Partials                          726      726           
Flag Coverage Δ
collector-unit-tests 27.33% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@robbycochran robbycochran added the run-benchmark Ask to run benchmark on a PR and compare it with the baseline label Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
VM Method Baseline CPU median (%) Test CPU median (%) CPU P-value

VM Method Baseline Memory median (MiB) Test Memory median (MiB) Memory P-value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-benchmark Ask to run benchmark on a PR and compare it with the baseline skip-integration-tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants