fix: map network event pktType to direction in CEL rules engine#841
Conversation
|
Warning Review limit reached
More reviews will be available in 51 minutes and 50 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthrough
Network Direction Inference
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Reviewed the fix end-to-end. The approach is sound and the chain works: for network events the production type is DatasourceEvent, whose GetPktType() reads the egress field (egress==1 -> OUTGOING, else HOST), so the new fallback yields outbound/inbound, and that value reaches CEL via the cel.go "direction" field getter (x.Raw.GetDirection()). I confirmed consts.Inbound=="inbound" / Outbound=="outbound", matching R1077's direction == 'outbound', and that the prefilter does not drop network events on direction (rule_manager.go only calls SetDirection for HTTP events, and prefilter line 254 short-circuits when e.Dir == DirNone). No hard blockers — but one test-coverage gap is worth fixing before merge, plus two minor nits inline.
Main concern (please address): the regression test does not cover the regressed code path. TestStructEventGetDirection only exercises StructEvent, but the production network path uses DatasourceEvent, and StructEvent.PktType is never populated for network events outside tests. So the identical block in datasource_event.go — the code that actually fixes #840 — and its real wiring through the egress field are untested. Consider a table test on DatasourceEvent.GetDirection() that sets egress in Data and asserts inbound/outbound.
|
|
||
| func (e *DatasourceEvent) GetDirection() consts.NetworkDirection { | ||
| if e.Direction == "" && e.EventType == NetworkEventType { | ||
| if strings.EqualFold(e.GetPktType(), "OUTGOING") { |
There was a problem hiding this comment.
This is the block that actually fixes #840 in production (network events flow through DatasourceEvent), yet there's no test for it — the new test only covers StructEvent. Worth a DatasourceEvent.GetDirection() test that populates the egress field.
Nit: prefer the existing OutgoingPktType constant (events.go) over the "OUTGOING" string literal.
|
|
||
| func (e *StructEvent) GetDirection() consts.NetworkDirection { | ||
| if e.Direction == "" && e.EventType == NetworkEventType { | ||
| if strings.EqualFold(e.GetPktType(), "OUTGOING") { |
There was a problem hiding this comment.
Nit: this block is duplicated verbatim in datasource_event.go; a shared helper (e.g. directionFromPktType(pktType)) would keep the two in sync. Also prefer the OutgoingPktType constant over the "OUTGOING" literal.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/utils/events_test.go (1)
27-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a mixed/lowercase
pktTypetest case for theEqualFoldcontract.Current cases validate mapping but not the case-insensitive requirement that
GetDirection()implements. A lowercase (or mixed-case)outgoingcase would prevent regressions.🤖 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/utils/events_test.go` around lines 27 - 35, Add a mixed/lowercase pktType test to cover the case-insensitive behavior in GetDirection. Extend the existing NetworkEventType cases in events_test.go by adding a StructEvent with pktType in lowercase or mixed case (for example, outgoing) and assert it still maps to consts.Outbound, so the EqualFold contract is explicitly verified and protected from regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/utils/events_test.go`:
- Around line 27-35: Add a mixed/lowercase pktType test to cover the
case-insensitive behavior in GetDirection. Extend the existing NetworkEventType
cases in events_test.go by adding a StructEvent with pktType in lowercase or
mixed case (for example, outgoing) and assert it still maps to consts.Outbound,
so the EqualFold contract is explicitly verified and protected from regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9b599d3-bf50-467c-a565-aad303e799a4
📒 Files selected for processing (3)
pkg/utils/datasource_event.gopkg/utils/events_test.gopkg/utils/struct_event.go
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
force-pushed
the
fix/network-event-direction
branch
from
3d350fc to
f1ffac6
Compare
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
1 participant
This PR fixes a bug where
event.directionon a network event is always empty, causing rules evaluatingevent.direction == 'outbound'(such as R1077) to never trigger. It falls back to mapping Inspektor Gadget'spktType(OUTGOINGvs others) to direction ife.Directionis empty.Closes #840
Summary by CodeRabbit
Bug Fixes
Tests