testing a fix - #60
Merged
Merged
Conversation
Signed-off-by: entlein <einentlein@gmail.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
entlein
added a commit
that referenced
this pull request
Aug 15, 2026
commit 4342b248418563a23eee0600ad94a7f6bb8b0d2f
Author: entlein <einentlein@gmail.com>
Date: Sat Aug 15 13:57:38 2026 +0200
test(e2e): fix issue-79 ladder measurement - read all node-agent pods, match containerName exactly, race-free readiness wait
The ladder under-counted to 0/N while the node-agent logs showed 5/5 R0001
for both the init and the ephemeral container: it read only one DaemonSet
pod's logs (the workload can land on any node), its grep could not match the
alert JSON's containerName field, and its readiness wait raced pod creation.
commit f7bcbd7631d67e0ab813f5364ea959819aa9f86f
Author: k8sstormcenter-bot <k8sstormcenter@users.noreply.github.com>
Date: Sat Aug 15 12:33:23 2026 +0200
fix(objectcache): do not evict status-lagged containers from the profile cache
The ContainerProfileCache reconciler classified any cache entry whose
container was absent from the pod's published status lists as reaped and
evicted it (reconciler.go isContainerTerminated). But kubelet publishes
the status groups incrementally: a just-attached ephemeral container has
no ephemeralContainerStatuses entry for several seconds while it is
already running and traced, and an entry added before the pod reached
the k8s cache carries an empty PodUID, which made the (Name, PodUID)
pre-running fallback unreachable for init containers. Eviction is
permanent (no re-add path exists), so every ProfileDependency=Required
rule (R0001/R0003/R0004) was silently suppressed for the container's
whole life: total alert loss for ephemeral containers, intermittent
exec-alert loss for init containers (issue #79, CI run 31846699597).
Evidence (live rig, issue #79): ephemeral container adopted at +1s,
evicted at the next reconciler tick +3s (entries 2->1), exec events
verifiably reached ReportEnrichedEvent at +75s and were dropped by the
Required-profile gate; the exec gadget's mntns filter map contained the
container's mntns the whole time (kernel/tracer exonerated).
Fix:
- treat absence from the status lists as reaped only when the pod SPEC
does not name the container either; a status entry with the same name
under a different non-empty ContainerID still evicts (replaced
instance)
- allow the pre-running (Name, PodUID) fallback to match when the
stored PodUID is empty
- backfill PodUID from the container runtime metadata when the pod is
not yet in the k8s cache at entry-build time
New tests fail on the pre-fix code and pass with the fix:
TestReconcilerKeepsJustAttachedEphemeralContainer,
TestReconcilerKeepsInitContainerWithEmptyStoredPodUID. Regression
guards (both-ways green): eviction after published termination, gone
from spec+status, replaced instance. Full objectcache, rulemanager and
containerprofilemanager suites pass unchanged.
commit 523246a94a53eee02a13b1cf48634a9754c98c75
Author: entlein <einentlein@gmail.com>
Date: Sat Aug 15 12:33:24 2026 +0200
fix(profilecache): do not classify status-lagged containers as reaped
The reconciler evicted any cache entry whose container was absent from
all published status lists once any statuses existed. A just-attached
ephemeral container is exactly that: the pod spec already declares it
while kubelet publishes its ephemeralContainerStatuses entry seconds
later. The freshly-adopted profile entry was evicted on the next tick,
nothing re-added it, and every ProfileDependency=Required rule was
suppressed for the container's entire life — zero alerts of any class
(issue #79 T5, ephemeral 0/N). Init-container entries created before the
pod reached the k8s cache (empty PodUID, status ContainerID not yet
published) hit the same branch, contributing to the init intermittency.
Observed: ephemeral container adopted +1s after attach; reconciler tick
3s later logged entries_before=2 entries_after=1; zero alerts over the
container's 75s life while the same pod alerted for its other containers.
Fix: absence from published statuses only counts as reaped when the
container is also absent from the pod SPEC (containers, initContainers,
ephemeralContainers). Additionally, the termination mark introduced with
the removal grace now resets when a marked container is observed alive
again, so a later genuine termination gets a full grace window.
Tests (red pre-fix): TestReconciler_KeepsEphemeralContainerAwaitingStatus,
TestReconciler_KeepsInitContainerAwaitingStatusWithEmptyPodUID,
TestReconciler_TerminationMarkResetsWhenContainerReappears; negative
contract TestReconciler_EvictsContainerRemovedFromSpecAndStatus.
Regression: go test -race ./pkg/objectcache/... ./pkg/containerwatcher/v2/
./pkg/rulemanager/... passes.
commit d9cf31923e68c95d1a68416529648c8890e57f53
Author: entlein <einentlein@gmail.com>
Date: Sat Aug 15 12:32:19 2026 +0200
test(profilecache): pin reconciler classification of status-lagged containers
Failing tests for the ephemeral total-loss leg of issue #79: the
reconciler classifies a container that is absent from all published
status lists as reaped, but a just-attached ephemeral container is
exactly that (kubelet publishes ephemeralContainerStatuses seconds after
the attach), and an init container whose entry carries an empty PodUID
hits the same branch while its status has no ContainerID yet. The entry
is evicted, nothing re-adds it, and every ProfileDependency=Required
rule is suppressed for the container's entire life.
Live-cluster evidence: ephemeral container adopted at +1s, reconciler
tick 3s later (entries_before=2 entries_after=1), zero alerts of any
class over its 75s life while the same pod alerted for other containers.
Contract pinned: a container still declared in the pod SPEC without a
published status is not reaped; absent from both spec and status is;
a termination mark resets when the container is observed alive again.
commit bcfbcde2c2fcfd3bde9bb07d72503254836dc8c6
Author: entlein <einentlein@gmail.com>
Date: Sat Aug 15 12:30:13 2026 +0200
test(e2e): add issue-79 end-of-life exec-delivery ladder script
Deterministic rig-side measurement for acceptance tests T4/T5: N repeated
init runs (terminal forbidden exec after a configurable runway) and N
ephemeral-container runs (terminal whoami+id), each asserting R0001
delivery via node-agent logs. Exits non-zero unless both legs are N/N.
commit eaca1294bd6ad32ec59db4248df7f56f912c6113
Author: entlein <einentlein@gmail.com>
Date: Sat Aug 15 12:28:45 2026 +0200
fix(containerwatcher,profilecache): grace period for events in flight at container end-of-life
An event emitted during a container's life can be processed after the
container's removal: the ordered event queue (50ms collection tick +
batching) and the worker pool delay evaluation past teardown. For a
container whose final process performs the exec and exits immediately
(init container with a terminal exec, ephemeral debug container), the
alert-carrying exec event loses this race and is dropped.
Observed failure: run 31846699597, Test_48_MultiSubtypeGroupedProfileDocument,
init container setup (sh -c "sleep 75; /usr/bin/id"): remove processed
22:44:26, terminal exec evaluated afterwards, zero R0001 while 98 R0003
fired during the container's life (assertion: 'id is not in the setup
section (initContainers)', component_test.go:3488). Same-shaped loss for
the ephemeral leg (R0001(debug,id)=0, remove 22:46:15).
Root cause, two drop points on the remove path:
1. EventHandlerFactory.ProcessEvent resolved container info only from the
live container collection plus a lazily-populated cache, silently
dropping events for just-removed containers that never had a prior
event processed.
2. ContainerProfileCache deleted the projected-profile entry immediately
(async) on the remove callback, so rules with ProfileDependency=Required
suppressed in-flight events as profile_incomplete.
Fix: keep container info and the projected profile resolvable for a
10s grace after removal, then evict:
- the factory now receives container lifecycle callbacks, warms its
lookup cache on add, and defers eviction by the grace period;
- the profile cache defers deleteContainer by the grace period and the
reconciler's terminated-eviction honors the same grace (mark on first
Terminated observation, evict on a later tick), so a reconciler tick
landing inside the window cannot reintroduce the race.
Tests: TestProcessEvent_DeliversEventForJustRemovedContainer{,_NoPriorEvent},
TestProcessEvent_RemovedContainerEvictedAfterGrace,
TestProjectedProfile_{SurvivesContainerRemovalGrace,EvictedAfterRemovalGrace},
TestReconciler_HonorsRemovalGraceForTerminatedContainer (all red on the
pre-fix code); TestReconcilerEvictsTerminatedContainer,
TestInitContainerEvictionViaRemoveEvent,
TestMissedRemoveEventEvictedByReconciler updated to the graced contract.
Regression: go test ./pkg/containerwatcher/... ./pkg/objectcache/...
./pkg/rulemanager/... passes (tracers field tests skipped locally: they
require the tracers.tar gadget bundle, unavailable off-CI); -race clean
on both touched packages.
commit b898329886daba36d8126148b66e94cf7fce6aa0
Author: entlein <einentlein@gmail.com>
Date: Sat Aug 15 12:21:59 2026 +0200
test(containerwatcher,profilecache): pin event delivery across container end-of-life
Failing tests for the teardown race behind issue #79 (init/ephemeral exec
loss): events emitted during a container's life are dropped when processed
after the container's removal.
- TestProcessEvent_DeliversEventForJustRemovedContainer[_NoPriorEvent]:
EventHandlerFactory.ProcessEvent silently drops events whose container
has left the live collection (evidence: run 31846699597, Test_48, init
container terminal exec at 22:44:26, remove processed 22:44:26, zero
R0001; ladder run1 total loss).
- TestProjectedProfile_SurvivesContainerRemovalGrace: the projected
profile is deleted immediately on the remove callback, so in-flight
events lose profile resolution and ProfileDependency=Required rules
suppress as profile_incomplete.
Both tests fail on current code by design; the fix must provide a
removal grace window covering the event pipeline delay.
commit f4dc94f6d0d6dee51be80596c4226c9601f72294
Author: entlein <einentlein@gmail.com>
Date: Sat Aug 15 00:50:08 2026 +0200
test(component): widen Test_48 init margin to 100s for CI runner variance
Init-phase enforcement is proven (interactive validation: adoption 3s after
deploy, init R0001 fires), but CI runners intermittently take longer than
75s to complete the first adoption; give the init container a 100s runway.
commit 0dbaeaf475c8fde427c50b37bab6372cbdffb18b
Author: entlein <einentlein@gmail.com>
Date: Sat Aug 15 00:27:26 2026 +0200
test(component): downgrade Test_48 ephemeral assertions to a tracked known limitation
The ephemeral debug container is adopted (section selected, monitor started,
tracers attached) but no events from it ever reach the rule engine - verified
in CI and interactively on a fresh cluster (zero exec/syscall/capability
events while the container demonstrably ran to completion). The
ephemeralContainers profile-selection contract stays covered by the cache
unit tests; event delivery for ephemeral containers is a container-watcher/
tracer scope issue independent of profile projection, tracked as follow-up.
Log the counts and self-signal when tracing starts working so the assertions
can be promoted back.
commit 7f5451de636b46005e9ef87859dd40a19fce1ebe
Author: entlein <einentlein@gmail.com>
Date: Sat Aug 15 00:00:09 2026 +0200
fix(containerwatcher): populate shared data for init containers during the init phase
getSharedWatchedContainerData refused to proceed while the pod phase was
Pending - but a pod executing its init containers is Pending by definition,
so shared data (and with it authored-profile adoption and rule enforcement)
could only arrive after the init phase completed. An init container could
never be enforced during its own execution; observed as an authored init
section whose forbidden binary produced no alert because adoption landed
seconds after the init container had already exited.
The phase gate existed because ImageID is empty in containerStatuses while
the pod is pending. Check that directly: when the pod is Pending, proceed as
soon as THIS container's status entry (containers, initContainers, or
ephemeralContainers) carries a non-empty ImageID, and keep retrying
otherwise.
commit bea971ddac9aef6840a17d1d2f0cffdc22efd417
Author: entlein <einentlein@gmail.com>
Date: Fri Aug 14 23:29:40 2026 +0200
test(component): size Test_48 waits to the observed init/ephemeral adoption latency
The per-section enforcement now works end to end, but the init and ephemeral
containers ran their forbidden binary 30s after start while authored-profile
adoption for those containers has been observed to take 60-70s on a loaded
runner - the exec raced adoption and produced no alert. Lengthen the in-pod
sleeps to 75s (fixture command, matching profile args) and the test's waits
accordingly.
commit abf33931e520b00b588b5205db2be0b5347481f4
Author: entlein <einentlein@gmail.com>
Date: Fri Aug 14 22:53:17 2026 +0200
chore: bump storage pin to the protobuf-marshaller regeneration
commit 422655f44939ab49f242ff1c41aed66ccb05f234
Author: entlein <einentlein@gmail.com>
Date: Fri Aug 14 21:59:26 2026 +0200
test(component): assert the grouped document round-trips before driving traffic
If a storage-side write path strips the subtype groups, Test_48's per-section
assertions fail later with misleading R0001 noise. Assert the served document
still carries all three groups (and the app section's execs) right after
apply, so a storage regression fails fast with the actual cause.
commit f8e00be1d6ed6d7949e9a25ab752cac1fa96ad8d
Author: entlein <einentlein@gmail.com>
Date: Fri Aug 14 21:18:55 2026 +0200
chore: bump storage pin to the subtype-group deflation fix
commit 801058d56d47e834562d425547c4cfe590667a09
Author: entlein <einentlein@gmail.com>
Date: Fri Aug 14 20:35:55 2026 +0200
test: renumber the multi-subtype component test to Test_48
Test_37 is already claimed by the signed-bundle overlay test in the fork CI
harness matrix; renumber to the next free slot so both suites can coexist.
commit 22a05c45903d580c2259c65bdf8bc52fa8f639a1
Author: entlein <einentlein@gmail.com>
Date: Fri Aug 14 17:17:51 2026 +0200
feat(containerprofilecache): recover container subtypes via grouped authored documents
The AP/NN decommission dropped the container subtype groups (containers/
initContainers/ephemeralContainers) the legacy specs carried: the flat
ContainerProfile could not describe a pod's init or ephemeral containers, and
the multi-container component test only exercised two REGULAR containers - so
the subtype contract was silently broken.
Storage now restores the subtype groups on ContainerProfileSpec (fork storage
module, pinned via replace); this consumes them:
- resolveAuthoredContainerSection maps an authored document to the per-
container view: flat documents pass through (single-container convention
unchanged); grouped documents select this container's section by name
across all three subtype groups, inheriting pod-level architectures and
the workload selector. A grouped document that does not cover the container
resolves to nil - never enforce a sibling's section - with a Warning on the
add path.
- Wired into both the add path (tryPopulateEntry) and the refresh path
(refreshOneEntry), before the learned-annotation validation.
- Unit tests: section selection across all three groups, flat pass-through,
uncovered-container nil (TestResolveAuthoredContainerSection), and an
add-path test proving a regular+init+ephemeral trio each adopt only their
own section from one shared document
(TestUserDefinedCP_GroupedDocumentPerSubtype).
Component test: Test_37_MultiSubtypeGroupedProfileDocument binds one grouped
document to a pod with a regular container, an init container whose startup
command runs a binary its section forbids (the init phase itself must alert),
and a runtime-attached ephemeral container (new
TestWorkload.AddEphemeralContainer helper using the ephemeralcontainers
subresource). Per-section allow/forbid is asserted for all three subtypes.
Test_36 and Test_37 are added to the component-tests matrix - Test_36 was
never listed, so it never ran in CI.
commit a60131bbe9b9c58f8cfc5f8de13d490abecd571a
Author: entlein <einentlein@gmail.com>
Date: Fri Aug 14 17:02:59 2026 +0200
feat(containerprofilecache): make authored-profile adoption visible (log + metric)
Adopting a user-authored ContainerProfile as the authoritative base for a
container silently switches what the rule engine enforces; the legacy overlay
path emitted metrics at the equivalent point, and the unresolved case already
has container_profile_user_defined_unresolved_total. Close the asymmetry:
- Info log naming the namespace/profile when an authored CP is adopted on the
add path.
- New counter (prometheus container_profile_user_defined_adopted_total,
OTEL node_agent.container_profile.user_defined_adopted.total), implemented
across the prometheus/OTEL/noop/mock managers.
gofmt applied to the touched metrics files (they were unformatted; no CI job
runs gofmt).
commit f9922e300a6defc31f9a7e9ecefdbedc499c49a4
Author: entlein <einentlein@gmail.com>
Date: Fri Aug 14 17:00:14 2026 +0200
test(projection): pin '*' exec-path classification as Pattern
The wildcard-classification fix routes any '*'- or dynamic-segment-bearing
entry on a path surface into Patterns instead of Values, but only the Opens
surface had a pinning test. Execs paths flow through the same projection, so
add the mirror case: a '*'-bearing exec path and a dynamic-segment exec path
are Patterns, a literal exec path stays a Value.
commit 48a67048265ca0901b38b678128d01c10b20c906
Author: entlein <einentlein@gmail.com>
Date: Fri Aug 14 16:59:30 2026 +0200
docs(containerprofilecache): update doc comments to the ContainerProfile-only design; restore network-wildcards fixture README
The AP/NN decommission left several doc comments describing the removed
overlay architecture: refreshAllEntries claimed to fast-skip on
UserAPRV/UserNNRV (fields that no longer exist), addContainer and
tryPopulateEntry said they fetch user-authored AP/NN CRDs, the workloadName
naming note still described the AP/NN aggregation target, and namespacedName
was documented as a legacy-CRD identifier. All now describe the authored
ContainerProfile flow that actually runs. Also drop a review-metadata
reference from a projection_apply comment.
tests/resources/network-wildcards/README.md was deleted with the legacy
fixtures, but the directory's ContainerProfile fixtures are still consumed by
containerprofilenetwork/fixtures_test.go and were left undocumented. Restore
the README updated to the ContainerProfile fixture shape and current test
names, including fixture 00 which postdates the deleted version.
No functional change; go build and the package tests are unchanged.
commit 8e44c662d68bc9d3cd895e4a76c5b7ed5799af4e
Author: ConstanzeTU <74674840+ConstanzeTU@users.noreply.github.com>
Date: Fri Aug 7 12:16:00 2026 +0200
ci: add dispatchable build workflow for the storage-triggered rebuild chain
The storage build dispatches this repo's build.yaml on the matching branch to
rebuild node-agent with the freshly built storage (go mod replace via
STORAGE_REF). This branch previously carried only the upstream .github, so the
dispatch failed with 'Workflow does not have workflow_dispatch'. Add the
dispatchable build workflow so the same-branch rebuild chain resolves.
commit a7345fea0661c955c2590ce67fec3078880c37bd
Author: Duck <70207455+entlein@users.noreply.github.com>
Date: Thu Aug 6 10:48:45 2026 +0200
Feat/celnetwork (#62)
* cel/containerprofilenetwork: add cp.was_selector_in_{ingress,egress}
Runtime label-based network matching, complementing was_address_in_*: resolve a
peer IP to its pod via the k8s object cache and match it against the profile
ingress/egress podSelector + namespaceSelector. Matches the peer by IDENTITY, so
it survives pod IP churn (learned IPs go stale on reschedule; selectors do not).
- projection carries each neighbor selector (IngressPeers/EgressPeers) instead of
dropping it at extract time
- wasSelectorIn shared impl + was_selector_in_ingress / was_selector_in_egress
direction wrappers, registered under the cp network library with cost estimates
- unit test for the podSelector + namespaceSelector match logic
* cel/containerprofilenetwork: match peer selectors on gadget-enriched labels
was_selector_in_{ingress,egress} resolved the peer IP to a pod through the
node-local K8s object cache (its pod watch is filtered by spec.nodeName), so
it could only match peers scheduled on the same node as the workload. A
matching peer on any other node did not resolve, was treated as unknown, and
alerted — ingress detection worked intra-node but not inter-node.
Match instead on the peer identity the network gadget's IP resolver already
attaches to the event (namespace + pod labels), resolved against a
cluster-wide pod inventory before the event reaches CEL. The IP-to-pod lookup
is dropped entirely; matching on peer identity is stable across pod IP churn
and holds across nodes.
- utils/cel: expose event.dstNamespace and event.dstPodLabels
- was_selector_in_{ingress,egress}: signature is now
(containerId, namespace, podLabels); the map argument bypasses the scalar
function cache
- add a compile guard that type-checks the selector rule expressions against
the real event object type
---------
Co-authored-by: ConstanzeTU <74674840+ConstanzeTU@users.noreply.github.com>
commit 2002fc54a732f825249d4355a0a5172b8206f71a
Author: Duck <70207455+entlein@users.noreply.github.com>
Date: Thu Aug 6 10:36:56 2026 +0200
testing a fix (#60)
Signed-off-by: entlein <einentlein@gmail.com>
commit 6365bdb76a6c24dad8564091fe709dd6dee9698a
Merge: 61d695f5 0212cdc1
Author: ConstanzeTU <74674840+ConstanzeTU@users.noreply.github.com>
Date: Thu Aug 6 10:35:22 2026 +0200
Merge remote-tracking branch 'origin/migrate/sbob' into mirrormain
commit 61d695f58f70cadcb151b81c0b664062029ffca9
Merge: 9393b35c 064da675
Author: Alon Liwsky <40373481+AlonLiwsky@users.noreply.github.com>
Date: Thu Aug 6 09:42:53 2026 +0300
Merge pull request #879 from AlonLiwsky/feat/networkstream-process-attribution
feat(networkstream): process attribution on the network stream (SUB-7786)
commit 064da6757fe5706fcb73646029c23999b4507958
Author: Alon <alon@armosec.io>
Date: Wed Aug 5 17:20:01 2026 +0300
fix(networkstream): draw the random child count once; name the tests in the doc table (SUB-7786)
Two CodeRabbit nitpicks, both valid.
The randomised estimator generator had rng.Intn(6) in the loop condition, so Go redrew
the bound before every iteration instead of once. The loop still terminated, but the
child count was no longer uniform over 0..5: each step continues with probability
(5-c)/6, making 5 children ~1.5% likely instead of ~16.7% and dropping the mean from
2.5 to ~1.8. Fan-out is one of the inputs that drives a tree into the region where the
childrenMap key made the estimate undercount, which is the whole reason this generator
was strengthened, so it was starving its own purpose. Drawn once now.
The 4,000-connection row in the doc looks like it contradicts the two tests that drive
comparable process counts and assert the payload stays UNDER the limit. It does not: the
tests build synthetic events carrying only a ref and a key, ~94 B each, where a
production entry is ~530 B -- the tests bound what the code emits, the table adds the
real-world entry weight on top. Named both tests in the caption and spelled out the
difference so a reader does not mistake the residual for a failing bound.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
commit 8bc4f41e1b38eddb1b778aa5570b4825ff68620a
Author: Alon <alon@armosec.io>
Date: Wed Aug 5 17:11:53 2026 +0300
docs(networkstream): correct the saturated-tree breach threshold to ~3,600 (SUB-7786)
The previous commit corrected the saturated-tree figure from 2.27 to 1.91 MiB but left
the threshold that had been derived from the old figure at ~3,000, so the paragraph
contradicted its own arithmetic: 3.75 - 1.91 leaves 1.84 MiB, which is ~3,640 entries,
not ~2,900. The ~2,900 only followed from the retired 2.27 MiB.
Measured directly this time rather than derived from another derived number: a
saturated tree map marshals to 1,978,505 B (1.89 MiB), leaving 3,932,160 - 1,978,505 =
1,953,655 B (1.86 MiB), so ~3,690 entries at 530 B. Two independent measurements put it
at ~3,580 and ~3,600, so the doc states ~3,600 -- rounding low is the right direction
for a breach threshold. Corrected in all three places: the table caption, the residual
table row and the derivation.
Added the rule that produced both of the errors in this chain, so the next person does
not repeat it: derive the threshold from the measured tree total, never from a payload
figure that already contains entries.
Unaffected: entries alone still breach at 3,932,160 / 530 = ~7,400, and the operator
conclusion -- that maxProcessTreeBytes is a real lever because trees are roughly half
the payload in that regime -- holds unchanged at ~3,600.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
commit 892df8b63b4742bb10118e17929fc1366b7e68d7
Author: Alon <alon@armosec.io>
Date: Wed Aug 5 16:01:32 2026 +0300
docs(networkstream): correct the payload-residual arithmetic and name the operator lever (SUB-7786)
Addresses matthyx's correction. The residual paragraph said the connection entries
alone breach the message limit at ~3,000 connections and that no tree budget can help
there. The number was right but the attribution and the conclusion were both wrong.
~3,000 is the breach point WITH the tree budget saturated, not entries alone. The
usable budget is 3.75 MiB of JSON (5 MiB after base64 x1.333); at 4,000 connections
entries are 2.02 MiB and saturated trees ~1.91 MiB, so backing the trees out leaves
room for ~2,900 entries. Entries alone would not breach until 3,932,160 / 530 = ~7,400.
That makes "no tree budget can help" the damaging part: trees are roughly half the
payload in that regime, so tightening maxProcessTreeBytes moves the breach point out
toward ~7,400 as the budget approaches zero. As written, an operator whose node was
dropping traffic would have concluded the budget was not a lever when it is the fastest
one available -- no protocol change, no backend coordination. Both numbers are now
tabulated with what each one means.
Splitting the message stays the long-term fix, since tightening the budget buys
headroom by shipping fewer trees -- the attribution this feature exists to deliver.
Recorded one prerequisite before anyone builds it: the container-profile precedent
reacts to a synchronous HTTP 413 from storage's QueueManager, whereas this path posts
elsewhere, handles no 413 anywhere, and the 5 MiB cap is a broker limit whose
enforcement point SUB-7850 lists as unverified. If it is enforced downstream the sensor
sees 200 OK and never learns, so reactive splitting is unavailable and the split has to
be decided before the first send.
Also corrects the 4,000-connection row from 5.72 to 5.24 MiB: it double-counted the
entry cost, adding a modelled 530 B per entry on top of a measured payload that already
contained ~94 B per synthetic entry. Same class of error as the one above -- mixing a
measured figure with a modelled one -- so the table now says which is which.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
commit 9393b35c5cd34f831aa36c6755e4a151245efa4d
Author: Matthias Bertschy <matthias.bertschy@gmail.com>
Date: Wed Aug 5 14:17:18 2026 +0200
feat(otel): add pod-level memory metric, harden cgroup scope resolution (#883)
* feat(otel): add pod-level memory metric, harden cgroup scope resolution
Adds node_agent.pod.memory.cgroup_bytes/cgroup_limit_bytes, read from
the parent kubepods-*-pod<UID>.slice cgroup rather than the container
.scope dir, so third-party sidecars with no OTEL instrumentation of
their own (e.g. clamav, gated behind malwareDetection) are covered by
the same read the kernel already aggregates at that level. The
existing node_agent.process.memory.* container-scoped metrics are
unchanged.
Also fixes two defects in findCgroupScopeDir surfaced while
investigating a live cgroup_bytes < rss_bytes anomaly:
- matched container IDs via an unanchored substring, now a
delimited-segment match
- accepted the first name-matching .scope dir without checking it
actually held memory.current, silently caching a 0 read on
cgroup-v1/hybrid hosts where the walk reaches another controller
subtree first; now keeps walking until a real match is found
Per-pod verification against production telemetry confirmed the
scope of this: 241/690 (35%) of live pods currently report
cgroup_bytes == 0 while rss_bytes > 0 (the second defect's
signature), and 13/690 (2%) show a genuine per-pod inversion. Scoped
to the systemd cgroup driver; cgroupfs hosts are a tracked follow-up.
Docs updated in docs/metrics-migration.md.
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
* fix(otel): stop cgroup scope resolution falling through to node-wide numbers
Addresses review feedback on #883 (both blockers):
1. resolveCgroupMemoryPaths fell through to the unscoped strategies
(parse /proc/self/cgroup, fixed v1 mount path) whenever the known-
container-ID scoped lookup failed. Those strategies are only valid
when the caller doesn't know its own container ID, or mounts its
own namespaced cgroup root (the sbom-scanner sidecar topology) --
for the main agent, which bind-mounts the host's cgroup tree, that
same root is the whole node. On any host where the scoped lookup
failed, this would have silently reported node-wide memory as the
container's own -- a wrong-but-plausible number, worse than the 0
it was meant to replace. A known container ID that can't be
scope-resolved now returns "", "" directly; the unscoped strategies
only run when ownContainerID == "".
2. findCgroupScopeDir now properly supports cgroup v1: it accepts a
verified directory with either memory.current (v2) or
memory.usage_in_bytes (v1), and resolveCgroupMemoryPaths reads
whichever filename pair is actually present. v1 hosts get correctly
container-scoped numbers instead of always reading 0. Also adds a
generous sentinel threshold so a v1 "unlimited" limit (a huge
near-MaxInt64 value) reports as 0, matching the v2 "max" convention,
instead of a misleading large number.
Also raises the pod-level resolver's rejection log from Debug to
Warning (agreed in review -- it resolves once per process, so no
volume concern, and it's the only signal that resolution silently
degraded).
docs/metrics-migration.md softened: the 35%-of-fleet figure cited
during development is real telemetry, but the causal attribution to
the v1/hybrid defect specifically wasn't confirmed (a simpler
empty-container-ID explanation is equally consistent with the same
observed zeros, and no cgroup-version signal exists in current
telemetry to distinguish them). This fix addresses both candidate
mechanisms either way, and makes the true split observable post-deploy
instead of asserted pre-deploy.
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
* fix(otel): thread hostCgroupMounted explicitly, close the residual node-wide gap
Addresses the second review round on #883.
Residual blocker: the previous fix inferred "is the unscoped fallback
safe" from ownContainerID == "", conflating two different topologies:
"caller doesn't know its container ID" and "caller mounts its own
namespaced cgroup root". Only the second makes the unscoped strategies
(parse /proc/self/cgroup, fixed v1 mount path) safe. The Kubernetes
DaemonSet bind-mounts the HOST's cgroup tree and can still hit
ownContainerID == "" (a permanently-cached early-startup race in
resolveOwnContainerID, before this pod's own ContainerStatuses entry
exists) -- and when it does, the old gate still fell through and would
have silently reported the whole node's memory as this container's
own, exactly the failure mode the previous commit claimed to close.
Fixed by passing the topology explicitly instead of inferring it:
hostCgroupMounted bool, threaded through NewOTELMetricsManager ->
registerResourceMetrics -> RegisterProcessMemoryMetrics/
RegisterPodMemoryMetrics -> readCgroupMem/readPodCgroupMem ->
resolveCgroupMemoryPaths(Under). cmd/main.go (the only caller in this
repo) passes true; the sbom-scanner sidecar's direct call passes
false. When hostCgroupMounted is true, only the scoped lookup (by
container ID) is ever tried; if it fails, this now returns "", ""
unconditionally -- the unscoped strategies never run, regardless of
why the container ID wasn't known.
New pinning test reproduces the reviewer's exact PoC: an empty
container ID on the host-mounted topology, with a v1 fixed-mount file
present at root, must still return "", "" rather than read it.
Also, from the same review round:
- Closed the observability gap: resolveOwnContainerID's two silent ""
returns now log Warning, and RegisterPodMemoryMetrics logs Warning
when an unresolved container ID reaches the DaemonSet topology
specifically (still silent for topologies with no pod concept at
all, so this doesn't add noise for cmd/host or cmd/ecs).
- Closed the pod-level cgroup-v1 gap found as a side effect of the
container-level v1 support: the pod-level resolver now also accepts
memory.usage_in_bytes at the verified parent slice, not just
memory.current. cgroupfs remains the only declared non-goal.
- Moved the cgroupScopeDir package-var publish out of the testable
..Under core and into the real resolveCgroupMemoryPaths wrapper, so
test invocations with synthetic roots no longer leave stale state in
the global (test-pollution nit).
docs/metrics-migration.md updated to describe the topology-explicit
model and the new observability signals.
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
---------
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
commit 24bf616f0872aa1dba5c00c6bdf24ebdc0ade98e
Author: Alon <alon@armosec.io>
Date: Wed Aug 5 13:57:15 2026 +0300
test+refactor(networkstream): pin the ranking order, derive the budget counters (SUB-7786)
Addresses matthyx's review.
1. The ranking order was not pinned. Inverting the estBytes comparator left the whole
suite green, TestSelectProcessTrees_PrefersSmallestTrees included, because every tree
in it is the same size -- ordering only decided WHICH equal-sized trees shipped, and
the beacon's small tree landed in the leftover budget either way. His test mixes two
size populations so the shipped COUNT depends on the order, which is the property
smallest-first exists for. Verified: ships 126 (= maxProcessTreeBytes/smallEst) as
written, 34 with the sort inverted, and the displacement assertion trips too.
2. The skip-vs-break branch is unreachable, so the comment claiming otherwise was
wrong. His proof holds: with the ascending sort, if candidate i does not fit then
used+est_i > budget, and for every j>i we have est_j >= est_i while used never
decreases, so nothing after the first miss can fit. Kept the skip as defensive against
a future ordering change, but it no longer claims to be load-bearing, and the test
named for it now states what it does and does not prove.
3. The drop counters are now derived -- len(candidates) minus len(processes), and total
connections minus shipped -- rather than accumulated in the packing loop. They are the
decision input for whether the budget needs raising, so they should not depend on the
loop body: as written before, a future early exit would have logged treesDropped 0
while dropping hundreds, with nothing asserting otherwise. Extracted selectWithinBudget
so the counters are testable, and pinned the identities that must hold regardless.
4. processNodeOverheadBytes' comment claimed "~360 measured" while the constant is 320.
He was right that the comment is the stale part: measured 133 bytes for a node with
every numeric at max and strings empty, plus ~16 for the childrenMap wrapper, so ~149
actual. The comment now says 320 is deliberate headroom -- absorbing UniqueID and
future fields -- rather than pretending to be a measurement.
Also documented two costs he raised that are real but out of scope here. The ref lookup
is on the packet path for every event including duplicates (unavoidable: the dedup key
contains the ref), and under exec churn his benchmark puts it at ~3.9x on that path --
write-lock contention on the manager mutex. The fix is a dedicated lock or atomic read
for the creator's pidStartTimeNs side map, which lives in pkg/processtree/creator and
belongs to the workstream that owns it. And the budget bounds the wire but not the
heap: storage retains one entry per process per endpoint per interval, each pinning a
tree, which cannot be capped without reintroducing the connection drops this fixes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
commit 86ea7a082dc6e389b6fcad6a11dae77164f2fd3d
Author: Alon <alon@armosec.io>
Date: Wed Aug 5 10:13:25 2026 +0300
fix(networkstream): charge the childrenMap key and escape containerID in the estimate (SUB-7786)
Addresses CodeRabbit's childrenMap-key finding. A child's comm is emitted TWICE --
as its own field and inside the parent's childrenMap key, which CommPID.MarshalText
renders as comm<U+241F>pid -- and the estimate charged it once, funding the second
copy out of per-node slack. estimateTreeBytes also charged containerID with a bare
len(), three lines above a comment saying never to do that; it was the last one.
Neither is reachable in production: comm is only ever 15 bytes because every source
is a kernel TASK_COMM_LEN buffer (eBPF GetComm, procfs stat.Comm), and container IDs
are hex. Measured at that bound the old accounting stays positive by +170 to +7291,
so CodeRabbit's Major severity is overstated -- I could not reproduce an underestimate
with a 15-byte comm. But the estimate is a BOUND, and it must not rest on an
invariant nothing in this repo enforces: remove the kernel's comm limit and the old
accounting runs 48% under (est 1,323,856 vs 2,531,171 marshalled), which is exactly
the silent over-limit message the budget exists to prevent.
Two corrections to the finding: the CommPID separator is U+241F (3 bytes), not '/',
and the deficit needs a fully-populated node, not any node.
The guard tests could not have caught this -- their children set four fields, so the
slack was never consumed; 200k randomised trees found nothing. The generator now
builds fully-populated children with escape-heavy 15-byte comms, and the table adds
the cases that actually discriminate (unbounded comm, both child shapes, escape-heavy
containerID), all three verified to fail without the fix.
Recalibrated: a realistic 10-node chain now estimates 5683 (was 5102, ~33% over
marshalled), so the budget holds ~461 trees rather than ~513. Still 1.6x the largest
batch observed, and the 283-connection worst case still ships every tree.
Two pre-existing assertions had tight constants tied to the old per-node accounting;
both now state their intent proportionally instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
commit a086e68ee86f45a858ce05df8da574bd4c990797
Author: Alon <alon@armosec.io>
Date: Tue Aug 4 18:16:56 2026 +0300
style(networkstream): trim comments, keeping the reasoning in the feature doc (SUB-7786)
The comments had grown to restate measurements, production numbers, calibration
tables and history inline -- all of which is already in
docs/features/network-stream-process-attribution.md, where it belongs and can be
kept current. Inline, it just made the code harder to read.
Kept only what a reader needs at that spot: the invariants that cause a bug if
violated (never scale StartTimeNs, never write to a shared tree node, don't
replace the copier with DeepCopy, take the ref before the mutex) and one pointer
to the doc. 419 -> 287 comment lines, no behaviour change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
commit ba68c0678df1c75bef54eb50725ab6dcb20b6e3a
Author: Alon <alon@armosec.io>
Date: Tue Aug 4 11:35:06 2026 +0300
test(networkstream): randomised guard that the tree estimate never undercounts (SUB-7786)
The budget's soundness is a property over ALL inputs, not the shapes I happened to
enumerate, and process argv is arbitrary kernel bytes. 400 randomised trees built
from bytes weighted toward what JSON escapes -- <, >, &, control bytes, the short
escapes, and mostly-invalid high bytes -- each checked against real json.Marshal
output. Fixed seed, so a failure reproduces.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
commit 733d9bac0c58874e0656c1bd4098cfb1a68af699
Author: Alon <alon@armosec.io>
Date: Tue Aug 4 11:29:34 2026 +0300
fix(networkstream): make the tree budget an actual bound (JSON escaping) (SUB-7786)
The budget was not a bound. estimateTreeBytes charged len(s), but that is not what
encoding/json emits: it escapes " and \\, every control byte, and -- because
Marshal enables HTML escaping -- <, > and &, which real command lines are full of
(sh -c 'cmd > /dev/null 2>&1'). Invalid UTF-8 is replaced byte-for-byte with the
6-byte \ufffd, and process argv is arbitrary kernel bytes, not guaranteed UTF-8.
Each such byte costs up to six where len() counted one.
Measured: 629 processes with 1 KB of non-UTF-8 argv each estimated at 772 KB --
29% of the budget -- so the under-budget fast path shipped every tree, dropped
nothing and logged nothing, while the real message was 5.37 MB after base64. The
broker rejects that, the snapshot is dropped, and the node loses its ENTIRE
interval of traffic. Reachable by an ordinary shell loop, and reachable
deliberately by anyone who can exec in a container on the node -- which made it a
detection-evasion primitive with a wider blast radius than the data-loss bug this
branch fixes. It also undercounted ordinary traffic: a realistic single node
estimated 372 against 395 marshalled.
escapedLen now charges the true escaped cost, rounding every escape up to 6 bytes,
and processNodeOverheadBytes goes 200 -> 320, measured against a fully-populated
node rather than a sparse test one. The estimator now overestimates by ~19% for
realistic trees. TestEstimateTreeBytes_NeverUnderestimates enforces the direction
across 15 shapes -- escape-heavy, non-UTF-8, wide, over-deep, legacy Children --
and fails on 10 of them with the old len()-based estimate.
Ranking changed from most-connections-first to smallest-tree-first. The old
rationale was backwards for the threat it named: a low-and-slow beacon opens
exactly one connection per interval, so it sorted last and lost its tree first --
the precise case reputation attribution exists to catch. Smallest-first maximises
the number of processes keeping a tree, which is the best objective available when
the sensor cannot know which process matters.
Also closes test gaps a mutation pass found: the determinism contract was unpinned
(removing both ref tie-breaks passed the suite), as were skip-vs-break packing, the
estimator's legacy-Children branch, the wrapper-field copy and countConnections.
All numbers in the code comments and the feature doc are re-derived from one
measured set -- the previous 4,000-connection row claimed 3.85 MB using lean test
entries when production-weight entries put it at 6.01 MB, over the limit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
commit b32773d5d1fdf7480ce2fb187569f4e13358c7e5
Author: Alon <alon@armosec.io>
Date: Tue Aug 4 10:37:59 2026 +0300
fix(networkstream): calibrate the tree budget against measured production traffic (SUB-7786)
The 1.5 MiB budget was picked from arithmetic, and it was wrong: it binds at
213 of 283 trees, and 283 connections is the largest batch ever observed in
production. It would therefore clip trees routinely on the busiest nodes --
degrading exactly the attribution this work adds -- while the payload at that
point is only 48% of the 5 MiB limit.
Recalibrated to 2.5 MiB using measured inputs rather than assumptions. The
reputation consumer gives the missing number: network_reputation_events_in_total
over the topic's message count puts a batch at ~42 connections in prod-eu (~32 in
prod-us), which against a 29 KB mean message makes a connection ~530 bytes of JSON
without its tree. Taking the worst case the budget exists for -- every connection
from a distinct process, so trees scale 1:1 -- 283 connections with p90 (~7 KB)
trees now ships all 283 trees at 2.50 MB after base64, 48% of the limit. The
budget binds above ~360 distinct processes at p90 and ~1280 at median, so it stays
a safety valve rather than a routine limiter.
TestBuildWireStream_ObservedWorstCaseFitsBudget pins the calibration itself and
fails at 1.5 MiB, so a future change cannot silently start clipping observed
traffic.
Also fixes the test helper that made the original numbers untrustworthy: it piled
the whole target size into one command line, which the 1 KB cap then truncated, so
it produced ~1.2 KB trees however large a size it was asked for -- and the
calibration test passed at both budgets because of it. It now builds a chain the
way a real tree gets big, and TestBigTree_ReachesRequestedSize keeps it honest.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
commit 0dd6419850290b2eb8bb24c290a8a28394821e6e
Author: Alon <alon@armosec.io>
Date: Tue Aug 4 10:07:27 2026 +0300
feat(networkstream): bound the process trees per message, and make the size observable (SUB-7786)
Attribution changed what sizes the payload. The old batch key collapsed every
process reaching one endpoint into a single entry carrying no tree, so size
tracked distinct ENDPOINTS. The new key splits per process and each distinct
process contributes a tree, so size tracks distinct PROCESSES that connected --
and nothing bounded that. SUB-7850's analysis modelled bytes per connection at a
fixed 283 connections, which is precisely the quantity the key change stops
holding fixed, so the multiplier on connection count was never budgeted.
Measured with ~2 KB trees: 4,000 distinct processes produce 4.99 MB of JSON, or
6.65 MB once the synchronizer envelope's base64 applies -- over the 5 MiB limit.
That is not graceful: sendNetworkEvent gets a non-2xx, Start() logs it and drops
the snapshot, so the node loses its ENTIRE interval of traffic. Reachable on a
node with heavy short-lived process churn.
maxProcessTreeBytes budgets the trees at 1.5 MiB of estimated bytes. A byte
budget rather than a tree count, because tree size still varies ~2.5x under the
command-line cap. Connections are never dropped -- that is the data loss this
change exists to fix -- only trees, and the refs stay put, so pid identity
survives and ProcessTreeFor returns nil for them as specified. Candidates rank by
connection count (highest fan-out is both the costliest attribution to lose and
the shape reputation cares about), ties broken on the ref so the payload never
depends on map iteration order.
How often this binds in reality is NOT knowable from current data: today's
sensor strips trees and emits no process identity, so distinct-processes-per-batch
exists in no message. Fleet mean today is ~30 KB (pulsar_average_msg_size on
network-stream-v1), ~175x under the limit -- comfortable baseline, but silent on
the new multiplier. So both the budget firing and any payload above 2 MiB now log
with their shape, which is what makes the question answerable after rollout.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
commit dee5d909c57f0a6b7f06206ae95c11b665744a12
Author: Alon <alon@armosec.io>
Date: Mon Aug 3 19:08:17 2026 +0300
fix(networkstream): address code review (SUB-7786)
Derive the wire copy BEFORE handing the snapshot to the notification channel.
Both were already outside the lock and buildWireStream is read-only, so this was
not live -- but the ordering made the producer re-read maps the consumer already
owned, resting on an out-of-repo guarantee that the consumer never writes to what
it receives. Reversing the two lines removes the dependency entirely.
capTreeCopy now copies the ProcessTree wrapper wholesale instead of field-listing
it, so a field added to the wrapper later cannot be silently dropped -- the exact
trap the three existing process copiers fell into. It also no longer assumes the
inner copy is non-nil.
Tests: two of the claims were not actually pinned. TestNoTickScaling asserted only
that buildWireStream does not rewrite an already-correct literal, so it passed
with a /10_000_000 injected into processRefFor, where such a bug would live; it
now runs the whole producer path. The shutdown-honouring select added in ad55206c
was covered by nothing -- every channel test buffers so the producer never blocks
-- so a plain blocking send passed the suite; TestFlush_BlockedChannelSendHonoursShutdown enters the blocked path and fails against that mutation. Also replaced a
fixed sleep with a poll on an observable signal, and covered the unattributed DNS
key and the nil-manager branch.
Docs: the key-collision claim was categorical but holds only for the structured
IP form, not for unstructured DNS names; and the lock-discipline table described
the flush while omitting that handleNetworkEvent holds the same mutex across an
unbounded net.LookupAddr (pre-existing, untouched here).
Dropped the two stale armoapi-go v0.0.696 go.sum lines.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
commit 0b30903a0e1fadb76cc89519758c664365f96609
Author: Alon <alon@armosec.io>
Date: Mon Aug 3 18:07:16 2026 +0300
fix(networkstream): let the flush honour shutdown while blocked on the channel (SUB-7786)
Moving the notification-channel send out of eventsStorageMutex removed the stall
on event recording but left the send itself blocking. A consumer that stops
reading would pin the flush goroutine past ctx cancellation, which the previous
under-the-lock send could not select against. The send stays blocking, so a slow
consumer still applies backpressure rather than silently losing traffic.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
commit 29f246e184efbdbacaf4764b8f21e6852d11cd73
Author: Alon <alon@armosec.io>
Date: Mon Aug 3 18:01:49 2026 +0300
feat(networkstream): ship trees once per process, capped, with the attribution marker (SUB-7786)
buildWireStream derives the HTTP payload from the flush snapshot: per-event trees
move into the message-scoped Processes map, one copy per distinct ProcessRef, and
each event keeps only its ref. Trees dominate the payload, so shipping them once
per process rather than once per connection is the size decision; the duplicated
ref bytes are budgeted. ProcessAttributionVersion is stamped unconditionally --
an empty Processes map is not a capability signal, so a sensor that ran and found
nothing must stay distinguishable from one that predates attribution.
On collision the deeper chain wins: the tree cache TTL (1 min) is shorter than
the flush interval (2 min), so two lookups for one process inside one interval
can return chains with different ancestry resolved. First-wins would discard the
richer chain and make the payload depend on map iteration order.
Command lines are capped at 1024 bytes with a visible marker, never splitting a
rune. cmdline is ~40% of a tree's bytes and unbounded, so this is what bounds the
payload tail rather than an optimisation.
The copy is read-only by hand rather than via armotypes.Process.DeepCopy, which
mutates its receiver: it calls MigrateToMap -- allocating ChildrenMap and nilling
Children -- on itself and on every child it recurses into. Those nodes are shared
with the process-tree manager's LRU cache, the legacy alert paths and the
notification-channel consumer, so writing to them from the flush goroutine would
be a data race that also strips the uncapped values those consumers rely on.
copyCappedProcess normalises the deprecated Children slice on read instead, and
every walk is depth-bounded.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
commit 6f5c8dd44d2c0d52889226d2e6281491b7a039a1
Author: Alon <alon@armosec.io>
Date: Mon Aug 3 17:52:59 2026 +0300
refactor(networkstream): independent flush snapshot; channel keeps trees, sleep removed (SUB-7786)
The flush handed the notification channel the LIVE storage struct, then -- still
holding eventsStorageMutex -- slept 100 ms and stripped every process tree from
the maps the consumer was reading. private-node-agent's host network sensor
reads outbound.ProcessTree off that channel, so the sleep was the only thing
standing between it and having its data erased mid-read. A test proved worse: an
event recorded 300 ms AFTER the flush appeared in the already-delivered
snapshot, because the clear loop and the consumer shared the same maps.
snapshotNetworkStream now allocates its own event maps, so the consumer's view
is immune to everything the producer does next. Trees are shared by pointer, not
walked: a tree is immutable once attached, which keeps the lock body to
O(entities + connections) struct copies. Both sends move outside the lock, and
the 100 ms sleep is deleted rather than shortened -- there is no shared state
left to race on. removeProcessTreeFromEvents goes with it; the wire copy is
where trees leave the payload from here.
Adds docs/features/network-stream-process-attribution.md, covering the emitted
schema, the batch key, the channel contract and the lock discipline.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
commit 8631fa10cd521960af548c45e328821ccd31c89e
Author: Alon <alon@armosec.io>
Date: Mon Aug 3 17:34:47 2026 +0300
feat(networkstream): per-connection process ref + process-aware batch key (SUB-7786)
The per-batch connection key was address/port/protocol with first-writer-wins,
so a second process connecting to the same endpoint was silently DISCARDED --
data loss, not merely misattribution. The same held for two processes resolving
one domain. The key now carries the process ref, so distinct processes coexist
while first-writer-wins is retained per process.
The ref is appended, never reordered: an unattributed key stays byte-identical
to the old format and can never collide with an attributed one. StartTimeNs is
boot-relative nanoseconds emitted verbatim; zero is legal pid-only identity.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
commit a4b27b6436e6e89581a3f15da08495ec23c97de5
Author: Alon <alon@armosec.io>
Date: Mon Aug 3 17:25:42 2026 +0300
chore: pin armoapi-go v0.0.742 for NetworkStream process attribution (SUB-7786)
v0.0.696 predates the process-attribution schema (ProcessRef,
NetworkStream.Processes, ProcessAttributionVersion). v0.0.742 is the exact
tag carrying it — v0.0.741 does not, so a newer tag is not evidence.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Alon <alon@armosec.io>
commit 5b5ff1c230e933bb679c05c9fa30f7cdccbc485e
Merge: 8866b6c8 dd16818c
Author: Alon Liwsky <40373481+AlonLiwsky@users.noreply.github.com>
Date: Wed Aug 5 09:24:55 2026 +0300
Merge pull request #876 from AlonLiwsky/fix/processtree-pid-recycle-lifecycle
fix(processtree): correct the exit-cleanup lifecycle under shutdown and pid reuse (SUB-7847, SUB-7846)
commit dd16818c09301abf5720f6248d5c545cbaa8b988
Author: Alon <alon@armosec.io>
Date: Mon Aug 3 18:42:02 2026 +0300
fix(processtree): harden the tree against process-id reuse (SUB-7846)
The kernel recycles pids, and the tree keeps an exited process's node for up
to exitCleanup::cleanupDelay. For that window a pid can be held by a LIVE
process while the tree still holds the DEAD one's node under it. Nothing
noticed, and two things went wrong. Both are reproduced in
pid_reuse_test.go, not inferred — the delayed-deletion half was found by
matthyx reviewing #873 and reproduced on that PR's head:
- handleForkEvent fills only EMPTY fields and a stale node is not empty, so
a recycled pid kept the predecessor's Comm, Cmdline, Path, Cwd, Uid and
Gid. A runtime alert of any type could name the wrong command.
- exitByPid matches on pid alone, so the dead process's delayed exit deleted
whatever node held that pid by then — a live process — reparenting its
children around it and taking its start time with it.
Three layers, each falling through to today's behaviour when its input is
unknown, and none of which ever KEEPS a node it cannot prove is newer:
1. A fork or exec on a pid with a pending exit retires the predecessor. The
event proves a new live incarnation, since a zombie can neither fork nor
exec, so no clock is needed. Deliberately NOT applied to procfs events:
/proc lists zombies, so there a pending exit can belong to the same
incarnation and proves nothing.
2. A delayed exit skips a node whose recorded creation postdates the exit
event's arrival. Both sides are boot-relative — the side map is procfs
ticks x 10^7, the arrival is CLOCK_BOOTTIME — so there is no btime skew
and no wall-clock margin. pendingExit.Timestamp and .StartTimeNs stay
wall-clock with their existing jobs and are not used here.
3. The procfs scan rebuilds a node whose start time changed. Two readings of
one process yield identical ticks, so a different non-zero value is proof
with no tolerance needed. The rebuild re-applies the Kubernetes
host-process policy, which the first-sighting path enforces and an
unconditional rebuild would bypass.
Layer 1 implements the shape matthyx proposed on SUB-7846, with one
difference. His snippet forces ok = false from the branch where a stale node
was found; the guard here runs before the lookup instead, so ok is false
naturally and no stale proc pointer exists. That also covers a case the
in-branch position misses: a pending exit can outlive its node, and the
recycled fork then builds a fresh node that the stale entry deletes at the
next cleanup. TestPidReuse_ForkAfterExit_ConsumesPendingExitWithNoNode fails
if the guard is gated on an existing node.
Retiring means the full teardown, NOT delete(pt.pendingExits, pid). That
simplification passes the obvious tests while leaving the dead process's
children linked to the pid, so the new process silently inherits them and
the tree claims an unrelated live process is their parent. It is quieter
than the bug it replaces.
TestPidReuse_ForkAfterExit_DoesNotInheritDeadChildren fails if anyone tries
it.
removeProcessNode extracts the teardown so the exit path and the procfs
rebuild cannot drift; it is independent of pendingExits because the rebuild
needs it for a pid with no pending exit at all. The extraction landed with
all 24 exit-manager and reparenting tests green before layer 3 used it. It
returns false when reparenting fails, and both callers then leave their
bookkeeping alone so the operation degrades to today's retry-next-tick. A
nil reparenting strategy takes the same failure path rather than being
dereferenced: unreachable today, since NewReparentingLogic ends in an
unconditional `return rl, nil`, but the agent has no recover() anywhere, so
the panic would end the process rather than one goroutine.
Under pt.mutex the additions are a map lookup and an integer comparison, and
no /proc read is added. The one syscall this work needs — CLOCK_BOOTTIME for
layer 2 — is read in handleExitEvent BEFORE the lock and passed into
addPendingExit, because unix.ClockGettime is a raw syscall rather than a vDSO
call and exits are about as frequent as forks. Same pattern, and same reason,
as the fork path's ~7.5 us start-time read.
forceCleanupOldest loses its second, redundant pass over the same pending
exits. That pass was silent while a repeat call always found the node gone;
now that a guard can keep a node and consume its pending entry, it would log
a warning for every node kept.
This is shared process-tree lifecycle: GetContainerProcessTree serves the
rule manager and every exporter, so this path sits behind every alert type.
Same-incarnation behaviour is therefore pinned by controls —
ProcfsSameStartTime_MergesAsToday keeps the merge semantics and children,
DelayedExit_NormalExitStillDeletes keeps ordinary exits deleting (without
it, an inverted comparison would leak every exited node and nothing would
catch it), and DelayedExit_UnknownStartTimeStillDeletes keeps the no-data
case byte-for-byte today's logic. Each guard was mutation-tested: deleting
it must turn a named test red.
The exit-manager lifecycle doc's SUB-7846 caveat is updated here rather than
in the preceding commit, where it was still accurate and where this doc does
not yet exist to link to.
Three limitations are documented rather than fixed. Layer 1 cannot
distinguish a recycle from a reordered exit, since the tracers do not
preserve kernel order across one queue. Layer 3 declines when the side map
holds no prior value, which lets layer 2 keep a node the scan merged rather
than rebuilt. And when a fork is processed BEFORE the exit it follows, all
three layers decline and both original bugs survive — confirmed against the
implementation, and left alone because the window is about one drain batch
and closing it would make the guards depend on ordering they cannot verify.
exitByPid settles the node-absent case before the arrival comparison, so the
guard cannot return while leaving an orphaned side-map entry. Unreachable
today, since side-map entries only e…
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
1 participant
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.
Overview