fix(networkstream): key stream entities by container ID outside Kubernetes - #887
Merged
matthyx merged 1 commit intoAug 12, 2026
Merged
Conversation
AlonLiwsky
added
the
ai-assisted
📝 WalkthroughWalkthroughThe network stream now keys events by container ID, maps host traffic to the node entity, creates temporary non-Kubernetes entities, preserves pre-announcement traffic, prunes idle unannounced entities, and guards endpoint enrichment when Kubernetes inventory is unavailable. ChangesNetwork stream entity keying
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NetworkEvent
participant NetworkStream
participant EntityResolver
participant KubernetesInventory
participant ContainerCallback
NetworkEvent->>NetworkStream: submit network or DNS event
NetworkStream->>EntityResolver: resolve container ID
EntityResolver->>KubernetesInventory: inspect inventory when available
EntityResolver-->>NetworkStream: return node, existing, new, or no entity
ContainerCallback->>NetworkStream: announce container
NetworkStream-->>ContainerCallback: preserve and enrich recorded traffic
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
matthyx
reviewed
Aug 12, 2026
matthyx
left a comment
matthyx
left a comment
Contributor
There was a problem hiding this comment.
Review
Went through entityForEventLocked, the unannouncedEntities prune path, the ContainerCallback merge, and the k8sInventory nil-guard in buildNetworkEvent, plus the doc changes. Checked out fix/networkstream-ecs-entity-keying locally and ran the package suite (go test -race -v ./pkg/networkstream/...): all 15 new tests + the existing suite pass, no races. go build ./..., gofmt -l, and golangci-lint run ./pkg/networkstream/... are clean on the touched files (the only lint hits are pre-existing SA1019 deprecation warnings in wire.go/wire_test.go, untouched by this PR).
Logic checks out:
- The node-collapse condition (
entityID == "" || entityID == armotypes.HostContainerID) correctly drops thek8sObjectCache == nildisjunct, andcfg.KubernetesModeis the right signal to gate creation-on-first-sight vs. drop-on-miss — it's the same flag that already gates buildingk8sInventoryin the constructor, so it's consistent with an existing invariant rather than a new assumption. unannouncedEntitiesbookkeeping is correct: cleared on announce (both directions of the announce/first-event race), cleared on remove, and the prune loop deletes from the map it's ranging over, which Go permits.entityForEventLockedrestoring the node entity asHost(not inventing a container keyed by the node name) closes a latent gap in the old code too — previously, if the host container'sRemoveContainerfired, host traffic was silently dropped until the next flush interval recreated the node entity.docs/CONFIGURATION.md: confirmedhostMalwareSensorEnabled/hostNetworkSensorEnableddon't exist anywhere in the tree, andhostMonitoringEnableddoes (pkg/config/config.go,pkg/containerwatcher/v2/container_watcher_collection.go), so that table edit is accurate.
No code-correctness blockers found. Two things stand in the way of merging, unrelated to the change itself:
- DCO check is failing (
action_required) — the commit needs aSigned-off-bytrailer (git commit --amend -s). - This PR is still a draft and
build-and-push-imageis red — the log showsUsername and password requiredon thequay.iologin step, which looks like registry credentials not being available to a fork-sourced workflow run rather than anything introduced by this diff.
I'll hold off on approving until it's marked ready for review and the DCO check is green — happy to re-review immediately once those are resolved.
…netes The node fallback fired whenever the Kubernetes object cache was absent. That cache is nil on every non-Kubernetes install, so on host and ECS agents every container's traffic collapsed onto the single node entity, and no per-container entity existed at all. Collapse onto the node only for an empty container ID or the host sentinel. Outside Kubernetes also create a container entity on its first event -- nothing announces containers there, so those events were dropped -- and prune such an entity once it goes an interval without traffic, so a host with container churn does not accumulate dead ones. In Kubernetes an unknown entity still means removed-or-ignored rather than never-announced, so it is still dropped and that path is unchanged. Also in passing: - Keep traffic recorded before a container is announced. The watcher dispatches ContainerCallback to a worker pool while events flow straight through, so a container's first egress routinely lands first; the announcement now supplies identity over the existing event maps instead of replacing them. - Nil-guard the k8s inventory lookups in buildNetworkEvent, which only exist in Kubernetes mode. - Drop hostNetworkSensorEnabled and hostMalwareSensorEnabled from docs/CONFIGURATION.md. Neither key exists anywhere in the codebase; hostMonitoringEnabled is the real one and was missing. Signed-off-by: Alon Liwsky <40373481+AlonLiwsky@users.noreply.github.com>
AlonLiwsky
force-pushed
the
fix/networkstream-ecs-entity-keying
branch
from
August 12, 2026 14:10
f2b5e96 to
bc687e2
Compare
AlonLiwsky
marked this pull request as ready for review
August 12, 2026 14:11
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@pkg/networkstream/v1/network_stream_test.go`:
- Around line 347-355: Isolate each endpoint-kind case in the test loop by
creating a fresh NetworkStream and associated state inside each subtest, or by
assigning distinct endpoint identities. Ensure each invocation of
ns.handleNetworkEvent independently verifies that its event is recorded with an
outbound length of 1, without relying on state from the other case.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 066e6074-085c-4af0-898a-3c023b3a6c1b
📒 Files selected for processing (4)
docs/CONFIGURATION.mddocs/features/network-stream-entity-keying.mdpkg/networkstream/v1/network_stream.gopkg/networkstream/v1/network_stream_test.go
Comment on lines
+347
to
+355
| for _, kind := range []types.EndpointKind{types.EndpointKindPod, types.EndpointKindService} { | ||
| t.Run(string(kind), func(t *testing.T) { | ||
| event := outboundEvent(101, "1.2.3.4", 443) | ||
| event.DstEndpoint = types.L3Endpoint{Addr: "1.2.3.4", Kind: kind} | ||
|
|
||
| assert.NotPanics(t, func() { ns.handleNetworkEvent(event, nil) }) | ||
| assert.Len(t, ns.networkEventsStorage.Entities[testNodeName].Outbound, 1, | ||
| "the connection is still recorded, only its Kubernetes enrichment is missing") | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Isolate the pod and service cases.
Both subtests share one NetworkStream and one endpoint identity. The pod case can satisfy the service case's assert.Len(..., 1) assertion. Create the stream inside each subtest, or use distinct endpoint identities. Then each case proves that its event is recorded.
Proposed test isolation
func TestBuildNetworkEvent_PodEndpointWithoutK8sInventory(t *testing.T) {
- ns := newTestStream(t, processtree.NewProcessTreeManagerMock())
- require.Nil(t, ns.k8sInventory)
-
for _, kind := range []types.EndpointKind{types.EndpointKindPod, types.EndpointKindService} {
t.Run(string(kind), func(t *testing.T) {
+ ns := newTestStream(t, processtree.NewProcessTreeManagerMock())
+ require.Nil(t, ns.k8sInventory)
event := outboundEvent(101, "1.2.3.4", 443)
event.DstEndpoint = types.L3Endpoint{Addr: "1.2.3.4", Kind: kind}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, kind := range []types.EndpointKind{types.EndpointKindPod, types.EndpointKindService} { | |
| t.Run(string(kind), func(t *testing.T) { | |
| event := outboundEvent(101, "1.2.3.4", 443) | |
| event.DstEndpoint = types.L3Endpoint{Addr: "1.2.3.4", Kind: kind} | |
| assert.NotPanics(t, func() { ns.handleNetworkEvent(event, nil) }) | |
| assert.Len(t, ns.networkEventsStorage.Entities[testNodeName].Outbound, 1, | |
| "the connection is still recorded, only its Kubernetes enrichment is missing") | |
| }) | |
| for _, kind := range []types.EndpointKind{types.EndpointKindPod, types.EndpointKindService} { | |
| t.Run(string(kind), func(t *testing.T) { | |
| ns := newTestStream(t, processtree.NewProcessTreeManagerMock()) | |
| require.Nil(t, ns.k8sInventory) | |
| event := outboundEvent(101, "1.2.3.4", 443) | |
| event.DstEndpoint = types.L3Endpoint{Addr: "1.2.3.4", Kind: kind} | |
| assert.NotPanics(t, func() { ns.handleNetworkEvent(event, nil) }) | |
| assert.Len(t, ns.networkEventsStorage.Entities[testNodeName].Outbound, 1, | |
| "the connection is still recorded, only its Kubernetes enrichment is missing") | |
| }) |
🤖 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 `@pkg/networkstream/v1/network_stream_test.go` around lines 347 - 355, Isolate
each endpoint-kind case in the test loop by creating a fresh NetworkStream and
associated state inside each subtest, or by assigning distinct endpoint
identities. Ensure each invocation of ns.handleNetworkEvent independently
verifies that its event is recorded with an outbound length of 1, without
relying on state from the other case.
matthyx
approved these changes
Aug 12, 2026
matthyx
left a comment
matthyx
left a comment
Contributor
There was a problem hiding this comment.
Re-checked: DCO now passes and the PR is out of draft. Diffed the current head (bc687e2) against what I reviewed before — identical, so this was just the sign-off amend, no code changes to re-review.
build-and-push-image is still red on the same quay.io login step (Username and password required), but that's a registry-credentials gap that looks generic to fork-sourced PRs in this repo, not something this diff affects — branch protection doesn't require that check, and it's orthogonal to the change under review.
Approving.
entlein
pushed a commit
to k8sstormcenter/node-agent
that referenced
this pull request
Aug 15, 2026
…netes (kubescape#887) The node fallback fired whenever the Kubernetes object cache was absent. That cache is nil on every non-Kubernetes install, so on host and ECS agents every container's traffic collapsed onto the single node entity, and no per-container entity existed at all. Collapse onto the node only for an empty container ID or the host sentinel. Outside Kubernetes also create a container entity on its first event -- nothing announces containers there, so those events were dropped -- and prune such an entity once it goes an interval without traffic, so a host with container churn does not accumulate dead ones. In Kubernetes an unknown entity still means removed-or-ignored rather than never-announced, so it is still dropped and that path is unchanged. Also in passing: - Keep traffic recorded before a container is announced. The watcher dispatches ContainerCallback to a worker pool while events flow straight through, so a container's first egress routinely lands first; the announcement now supplies identity over the existing event maps instead of replacing them. - Nil-guard the k8s inventory lookups in buildNetworkEvent, which only exist in Kubernetes mode. - Drop hostNetworkSensorEnabled and hostMalwareSensorEnabled from docs/CONFIGURATION.md. Neither key exists anywhere in the codebase; hostMonitoringEnabled is the real one and was missing. Signed-off-by: Alon Liwsky <40373481+AlonLiwsky@users.noreply.github.com> Signed-off-by: entlein <einentlein@gmail.com>
entlein
pushed a commit
to k8sstormcenter/node-agent
that referenced
this pull request
Aug 26, 2026
…netes (kubescape#887) The node fallback fired whenever the Kubernetes object cache was absent. That cache is nil on every non-Kubernetes install, so on host and ECS agents every container's traffic collapsed onto the single node entity, and no per-container entity existed at all. Collapse onto the node only for an empty container ID or the host sentinel. Outside Kubernetes also create a container entity on its first event -- nothing announces containers there, so those events were dropped -- and prune such an entity once it goes an interval without traffic, so a host with container churn does not accumulate dead ones. In Kubernetes an unknown entity still means removed-or-ignored rather than never-announced, so it is still dropped and that path is unchanged. Also in passing: - Keep traffic recorded before a container is announced. The watcher dispatches ContainerCallback to a worker pool while events flow straight through, so a container's first egress routinely lands first; the announcement now supplies identity over the existing event maps instead of replacing them. - Nil-guard the k8s inventory lookups in buildNetworkEvent, which only exist in Kubernetes mode. - Drop hostNetworkSensorEnabled and hostMalwareSensorEnabled from docs/CONFIGURATION.md. Neither key exists anywhere in the codebase; hostMonitoringEnabled is the real one and was missing. Signed-off-by: Alon Liwsky <40373481+AlonLiwsky@users.noreply.github.com> Signed-off-by: entlein <einentlein@gmail.com>
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
2 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.
Summary
Outside Kubernetes the network stream folded every container's traffic onto the single node entity, so no per-container entity existed and per-workload attribution was impossible. This keys entities by container ID instead — collapsing onto the node only for traffic that is not a container's — and creates the entity on first sight where nothing announces containers. The Kubernetes path is unchanged.
Overview
handleNetworkEventandhandleDnsEventdecide which stream entity a connection belongs to. Today the fallback onto the node entity fires wheneverns.k8sObjectCache == nil:That cache is a Kubernetes API cache. In this repo's binary it is always present, but embedders that run the sensor outside a cluster construct the stream with a nil cache — and there every event, including every container's, is folded onto the single node entity. No per-container entity is ever created, so every container on the machine is indistinguishable in the stream and per-workload attribution downstream is impossible.
After this change the collapse happens only for traffic that is not a container's: an empty container ID, or the
"host"sentinel thatprocfs.goassigns to processes outside every tracked mount namespace. Everything else keys by container ID.Keying alone is not enough, because an event for an entity that does not exist is dropped, and outside Kubernetes nothing subscribes this package's
ContainerCallback— so no container entity is ever announced. WhenkubernetesModeis off, the entity is therefore created on the container's first event, and pruned once it goes an interval without traffic, so a machine with container churn does not accumulate dead ones.In Kubernetes it keeps dropping, and that is deliberate. A missing entity in a cluster does not mean "never announced"; it means the container was removed or is ignored. Removal is not quiet, either: the container collection publishes
EventTypeRemoveContainerbefore the container leaves the collection and then holds it in a 2-second cache for enrichers, while the watcher dispatches this callback onto a worker pool. So events for a container that is already gone keep arriving for a while afterwards. Creating entities for those would put a container carrying no pod or workload identity into the payload, for a pod that no longer exists, on every pod termination.Two smaller fixes ride along:
containerCallbacksubmits its subscribers to a worker pool while events flow straight through, so a container's first outbound connection routinely lands before itsAddContainerreaches this package. The announcement now fills identity in over the event maps already there instead of replacing them. The node entity is excluded — it is not announced, so it has no such window, and excluding it keeps that path byte-identical.k8sInventorylookups inbuildNetworkEventare nil-guarded. The inventory is only built whenkubernetesModeis set, so a pod- or service-kind endpoint arriving anywhere else would dereference a nil interface.Additional Information
Effect on Kubernetes: none. The removed
k8sObjectCache == nildisjunct never fired there, and every new behaviour is either behind!cfg.KubernetesModeor scoped away from the node entity.TestHandleNetworkEvent_KeyingIgnoresK8sObjectCacheandTestHandleNetworkEvent_KubernetesStillDropsUnknownEntitiespin that.One known gap is deliberately not addressed here.
ContainerCallbackdeletes a stopped container's entity along with any traffic it had not yet flushed, losing up to one interval — which matters most for a short-lived container that makes a single outbound connection. That behaviour is pre-existing and untouched by this PR: it is neither introduced nor widened here. But per-container entities make it matter more, so I will follow up with a separate PR that retires the entity at the next flush instead of deleting it. I am keeping it separate because that one is a Kubernetes behaviour change and deserves its own review rather than riding inside a change whose point is that Kubernetes is unaffected.Docs: adds
docs/features/network-stream-entity-keying.md. Also dropshostNetworkSensorEnabledandhostMalwareSensorEnabledfromdocs/CONFIGURATION.md— neither key exists anywhere in the codebase, whilehostMonitoringEnabled, which does, was missing from the table.How to Test
This package does not build on macOS, since inspektor-gadget is Linux-only. On a Mac:
On Linux,
go test -race ./pkg/networkstream/....15 new tests (18 cases with subtests) cover the keying, the platform gate, entity creation and pruning, the announcement merge in both arrival orders, node-entity restoration, and the nil-inventory guard. I checked each one earns its place by reverting the corresponding behaviour individually and confirming that test — and only that test — fails. The rest of the unit suite is unaffected.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation