fix(core): use TaskExit type to unmarshal containerd exit events#2527
Conversation
|
Hi @Aryan-sharma11 @rksharma95, gentle ping on this PR would appreciate your review when possible. Thanks! |
There was a problem hiding this comment.
Pull request overview
One-line bug fix in the containerd event handler: the /tasks/exit case was unmarshaling event payloads into apievents.TaskStart instead of apievents.TaskExit. Because the two protobuf types share field 1 (ContainerID) but differ in subsequent field numbers, GetPid() always returned 0, so the pid == exitTask.GetPid() guard failed and stale entries accumulated in dm.Containers and the eBPF NsMap.
Changes:
- Swap
apievents.TaskStart{}forapievents.TaskExit{}in the/tasks/exitbranch ofhandleContainerdEvent. - Add unit tests demonstrating the proto field-number mismatch and verifying the fix.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| KubeArmor/core/containerdHandler.go | Use correct TaskExit proto type when unmarshaling /tasks/exit events so GetPid() returns the actual exiting pid. |
| KubeArmor/core/containerdHandler_test.go | New unit tests covering the bug, edge case (pid=0), shared ContainerID field, and unchanged TaskStart handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Athang69
force-pushed
the
fix/containerd-exit-v2
branch
from
6de4b64 to
85962ae
Compare
|
Raised PR #2605 for addressing the failing CI test |
|
@Athang69, kindly rebase this PR. |
Athang69
force-pushed
the
fix/containerd-exit-v2
branch
from
23c5096 to
7b22ae9
Compare
|
@Aryan-sharma11 done. |
|
@rksharma95 PTAL whenever convenient. |
|
This PR has received the required approvals. Could someone merge it when convenient? |
Aryan-sharma11
force-pushed
the
fix/containerd-exit-v2
branch
from
ddbd3e5 to
9e56cac
Compare
/tasks/exit handler was using apievents.TaskStart instead of apievents.TaskExit causing GetPid() to always return 0 and stale entries in dm.Containers and eBPF NsMap. Fixes kubearmor#2526 Signed-off-by: Athang69 <[email protected]>
Signed-off-by: Athang69 <[email protected]>
Aryan-sharma11
force-pushed
the
fix/containerd-exit-v2
branch
from
9e56cac to
da24840
Compare
|
LGTM! Thanks for the PR @Athang69 |
Purpose of PR?:
/tasks/exit handler was using apievents.TaskStart instead of apievents.TaskExit causing GetPid() to always return 0, so destroy was never triggered leaving stale entries in dm.Containers and eBPF NsMap.
Fixes #2526
Does this PR introduce a breaking change?
No
If the changes in this PR are manually verified, list down the scenarios covered:
Container exit event correctly unmarshaled, GetPid() returns correct pid, destroy path triggers correctly, /tasks/start handling unaffected.
Additional information for reviewer? :
One line fix - apievents.TaskStart to apievents.TaskExit in /tasks/exit case. ContainerID is field1 in both types so container lookup worked fine making the bug subtle - only the pid comparison was silently broken. Added containerdHandler_test.go with 4 unit tests proving the mismatch and verifying the fix.
Checklist:
<type>(<scope>): <subject>