Refactor DatasourceEvent methods to suppress invalid field length warnings#739
Merged
Refactor DatasourceEvent methods to suppress invalid field length warnings#739
Conversation
📝 WalkthroughWalkthroughReplaced many error-returning field reads with silent reads that return zero-values or nil; added a warning when a requested field is not found in the field-accessor cache; retained caching but changed cache-miss behavior to return missing/nil accessors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/utils/datasource_event.go (1)
415-424:⚠️ Potential issue | 🟡 MinorCannot distinguish between GID 0 (root) and field-read failure.
When the field read fails,
giddefaults to0and&gidis returned. Callers cannot distinguish between "user is root (GID 0)" and "field was missing/unreadable." The same issue applies toGetUidat lines 715-724.If this semantic distinction matters for security analysis, consider returning
nilon error instead.💡 Suggested approach
func (e *DatasourceEvent) GetGid() *uint32 { switch e.EventType { case SyscallEventType: return nil default: - gid, _ := e.getFieldAccessor("proc.creds.gid").Uint32(e.Data) - return &gid + gid, err := e.getFieldAccessor("proc.creds.gid").Uint32(e.Data) + if err != nil { + return nil + } + return &gid } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/datasource_event.go` around lines 415 - 424, The current GetGid (and similarly GetUid) returns &gid even when the field read failed because the Uint32 call's second return value is ignored, making GID 0 indistinguishable from a read error; change both DatasourceEvent.GetGid and DatasourceEvent.GetUid to capture the second return (ok/error) from e.getFieldAccessor("proc.creds.gid").Uint32(e.Data) (and the equivalent "proc.creds.uid"), and if the read failed return nil instead of &gid; keep the special-case SyscallEventType behavior but ensure you only return a pointer when the field read succeeded.
🧹 Nitpick comments (4)
pkg/utils/datasource_event.go (4)
171-176: Warning log may cause excessive noise for legitimately absent fields.When a field genuinely doesn't exist for a particular event type (e.g., DNS-specific fields on exec events), this warning will fire on every access since nil results aren't cached. Consider either:
- Caching which fields are known to be absent per event type, or
- Logging at Debug level instead of Warning
This could generate significant log volume in production with high event throughput.
♻️ Suggested approach to reduce log noise
field := e.Datasource.GetField(fieldName) if field == nil { - logger.L().Warning("field not found", helpers.String("field", fieldName), helpers.String("eventType", string(e.EventType))) - // Don't cache nil results - another data source may have this field + logger.L().Debug("field not found", helpers.String("field", fieldName), helpers.String("eventType", string(e.EventType))) + // Cache nil result to prevent repeated lookups and log spam + m.Store(fieldName, missingFieldAccessor) return missingFieldAccessor }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/datasource_event.go` around lines 171 - 176, The current logger.L().Warning call inside the branch where e.Datasource.GetField(fieldName) returns nil is noisy for legitimate absent fields (and those nil results aren't cached), so either change the log level to debug (logger.L().Debug) to reduce production noise, or implement a simple per-datasource/event-type absent-field cache keyed by (e.Datasource, e.EventType, fieldName) and return missingFieldAccessor while recording the absence to avoid repeated logs; locate the check around e.Datasource.GetField and missingFieldAccessor and update the logging or add the absent-field cache before returning.
193-743: Overall pattern review: Silent error suppression trades debuggability for tolerance.The refactor consistently ignores read errors via
_, _pattern across ~60 getters. This achieves the PR goal of suppressing field-length warnings but has trade-offs:
- Lost visibility: When a field exists but has corrupt/invalid data, no indication is logged
- Data quality: Zero-values flow into alerts and profiles (per context snippets)
- Debugging: Harder to trace issues back to field-read failures
If this is the intended behavior, consider adding a code comment at the top of the file documenting this contract: "Getters return zero-values on read failures to tolerate schema mismatches across datasource versions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/datasource_event.go` around lines 193 - 743, The getters on DatasourceEvent (e.g., GetAddresses, GetArgs, GetDstIP, GetPID, GetTimestamp and the many other Get* methods) intentionally ignore errors from getFieldAccessor calls and return zero-values; add a clear file-level comment at the top of pkg/utils/datasource_event.go that documents this contract (“Getters return zero-values on field-read failures to tolerate schema mismatches; errors are intentionally suppressed here”), reference the DatasourceEvent type and the use of getFieldAccessor, and briefly state that any change to this behavior must be handled centrally (e.g., by modifying getFieldAccessor or adding logging there) rather than altering each getter.
305-317: Port 0 returned on field failure may appear in alerts.When the port field read fails,
0is returned. Per context snippets 1 and 2, this value flows directly intobaseRuntimeAlert.Arguments["port"]andNetworkEntity.DstPort. Port 0 is technically valid but may indicate incomplete data. Consider whether callers should filter or flag this case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/datasource_event.go` around lines 305 - 317, GetDstPort currently returns 0 on read failure which can be mistaken for a real port; change its API to return (uint16, bool) so callers can detect failure: update func GetDstPort() to func GetDstPort() (uint16, bool), use the ok value from e.getFieldAccessor(...).Uint16(e.Data) and return (port, true) on success and (0, false) on failure (also log a warning via logger.L()). Then update callers that populate baseRuntimeAlert.Arguments["port"] and NetworkEntity.DstPort to check the bool and either omit the port argument or set an explicit sentinel/marker when false instead of inserting 0. Ensure references to EventType, getFieldAccessor, Data, baseRuntimeAlert.Arguments and NetworkEntity.DstPort are updated accordingly.
269-290: Document the contract for endpoint data when field reads fail.Field reads in
GetDstEndpoint()silently default to zero values (returningAddr: "0.0.0.0",Port: 0,Proto: 0when reads fail). All 11 callers—including profile managers, network stream handlers, and CEL evaluation—use these values directly in alerts and profiles without validation.Consider whether downstream consumers should distinguish "field missing" from "actual zero value". If treating missing fields as zeros is acceptable, document this contract explicitly so maintainers understand the implications.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/datasource_event.go` around lines 269 - 290, GetDstEndpoint silently treats failed field reads as zero values which propagates "0.0.0.0"/0/0 into callers; update the function's doc comment for GetDstEndpoint to clearly state that reads missing/invalid fields are returned as zero-values (Addr "0.0.0.0", Port 0, Proto 0, empty labels) and that callers should validate these values if they need to distinguish "missing" vs "actual zero". Reference the function name GetDstEndpoint, the returned type types.L4Endpoint/types.L3Endpoint, and helpers rawIPv4ToString and parseStringToMap in the comment so maintainers understand which fields are affected and can opt to change the API later (e.g., return an error or use option flags) if a different contract is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/utils/datasource_event.go`:
- Around line 198-201: GetArgs currently returns []string{""} when the
underlying "args" field is empty because it blindly splits the string; update
DatasourceEvent.GetArgs to mirror the GetAddresses fix: retrieve the args string
via e.getFieldAccessor("args").String(e.Data), check for empty string (or trim
if needed) and return an empty slice if empty, otherwise return
strings.Split(args, igconsts.ArgsSeparator); reference function GetArgs, method
getFieldAccessor("args"), property e.Data and constant igconsts.ArgsSeparator
when making the change.
- Around line 193-196: GetAddresses currently calls getFieldAccessor(...).String
and then strings.Split which returns []string{""} for an empty input; update
DatasourceEvent.GetAddresses to handle empty or whitespace-only address strings
by first trimming the raw addresses value and returning an empty slice if it's
empty, otherwise split on ',' and trim each entry and filter out any empty
results (so GetAddresses and callers like dnsEvent.GetAddresses() never receive
a single empty-string address). Ensure you reference GetAddresses,
e.getFieldAccessor("addresses"), and e.Data when implementing this check and
filtering.
---
Outside diff comments:
In `@pkg/utils/datasource_event.go`:
- Around line 415-424: The current GetGid (and similarly GetUid) returns &gid
even when the field read failed because the Uint32 call's second return value is
ignored, making GID 0 indistinguishable from a read error; change both
DatasourceEvent.GetGid and DatasourceEvent.GetUid to capture the second return
(ok/error) from e.getFieldAccessor("proc.creds.gid").Uint32(e.Data) (and the
equivalent "proc.creds.uid"), and if the read failed return nil instead of &gid;
keep the special-case SyscallEventType behavior but ensure you only return a
pointer when the field read succeeded.
---
Nitpick comments:
In `@pkg/utils/datasource_event.go`:
- Around line 171-176: The current logger.L().Warning call inside the branch
where e.Datasource.GetField(fieldName) returns nil is noisy for legitimate
absent fields (and those nil results aren't cached), so either change the log
level to debug (logger.L().Debug) to reduce production noise, or implement a
simple per-datasource/event-type absent-field cache keyed by (e.Datasource,
e.EventType, fieldName) and return missingFieldAccessor while recording the
absence to avoid repeated logs; locate the check around e.Datasource.GetField
and missingFieldAccessor and update the logging or add the absent-field cache
before returning.
- Around line 193-743: The getters on DatasourceEvent (e.g., GetAddresses,
GetArgs, GetDstIP, GetPID, GetTimestamp and the many other Get* methods)
intentionally ignore errors from getFieldAccessor calls and return zero-values;
add a clear file-level comment at the top of pkg/utils/datasource_event.go that
documents this contract (“Getters return zero-values on field-read failures to
tolerate schema mismatches; errors are intentionally suppressed here”),
reference the DatasourceEvent type and the use of getFieldAccessor, and briefly
state that any change to this behavior must be handled centrally (e.g., by
modifying getFieldAccessor or adding logging there) rather than altering each
getter.
- Around line 305-317: GetDstPort currently returns 0 on read failure which can
be mistaken for a real port; change its API to return (uint16, bool) so callers
can detect failure: update func GetDstPort() to func GetDstPort() (uint16,
bool), use the ok value from e.getFieldAccessor(...).Uint16(e.Data) and return
(port, true) on success and (0, false) on failure (also log a warning via
logger.L()). Then update callers that populate
baseRuntimeAlert.Arguments["port"] and NetworkEntity.DstPort to check the bool
and either omit the port argument or set an explicit sentinel/marker when false
instead of inserting 0. Ensure references to EventType, getFieldAccessor, Data,
baseRuntimeAlert.Arguments and NetworkEntity.DstPort are updated accordingly.
- Around line 269-290: GetDstEndpoint silently treats failed field reads as zero
values which propagates "0.0.0.0"/0/0 into callers; update the function's doc
comment for GetDstEndpoint to clearly state that reads missing/invalid fields
are returned as zero-values (Addr "0.0.0.0", Port 0, Proto 0, empty labels) and
that callers should validate these values if they need to distinguish "missing"
vs "actual zero". Reference the function name GetDstEndpoint, the returned type
types.L4Endpoint/types.L3Endpoint, and helpers rawIPv4ToString and
parseStringToMap in the comment so maintainers understand which fields are
affected and can opt to change the API later (e.g., return an error or use
option flags) if a different contract is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 839c20f9-86cc-40b2-b660-560d293c1c32
📒 Files selected for processing (1)
pkg/utils/datasource_event.go
…nings Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
force-pushed
the
simplify-datasource
branch
from
4c4ab52 to
9597da4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/utils/datasource_event.go (2)
275-287:⚠️ Potential issue | 🟠 Major
GetDstEndpointhardcodes IPv4 conversion regardless of endpoint version.The method always reads/converts
endpoint.addr_raw.v4, so non-IPv4 events can produce a bogus0.0.0.0address in alert metadata.🐛 Suggested fix (version-aware address decode)
func (e *DatasourceEvent) GetDstEndpoint() types.L4Endpoint { - addr, _ := e.getFieldAccessor("endpoint.addr_raw.v4").Uint32(e.Data) + version, _ := e.getFieldAccessor("endpoint.version").Uint8(e.Data) + addrStr := "" + switch version { + case 4: + addr, _ := e.getFieldAccessor("endpoint.addr_raw.v4").Uint32(e.Data) + addrStr = rawIPv4ToString(addr) + case 6: + addr, _ := e.getFieldAccessor("endpoint.addr_raw.v6").Bytes(e.Data) + addrStr = rawIPv6ToString(addr) + } kind, _ := e.getFieldAccessor("endpoint.k8s.kind").String(e.Data) name, _ := e.getFieldAccessor("endpoint.k8s.name").String(e.Data) namespace, _ := e.getFieldAccessor("endpoint.k8s.namespace").String(e.Data) podLabels, _ := e.getFieldAccessor("endpoint.k8s.labels").String(e.Data) - version, _ := e.getFieldAccessor("endpoint.version").Uint8(e.Data) port, _ := e.getFieldAccessor("endpoint.port").Uint16(e.Data) proto, _ := e.getFieldAccessor("endpoint.proto_raw").Uint16(e.Data) return types.L4Endpoint{ L3Endpoint: types.L3Endpoint{ - Addr: rawIPv4ToString(addr), + Addr: addrStr, Version: version,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/datasource_event.go` around lines 275 - 287, GetDstEndpoint currently always reads "endpoint.addr_raw.v4" and calls rawIPv4ToString, which yields incorrect addresses for IPv6; update GetDstEndpoint to branch on the endpoint version (the local variable version from getFieldAccessor("endpoint.version").Uint8) and for IPv4 use "endpoint.addr_raw.v4" + rawIPv4ToString(addr) and for IPv6 use "endpoint.addr_raw.v6" (via getFieldAccessor) and rawIPv6ToString on the retrieved value, leaving the rest of the L4Endpoint population unchanged; reference the functions/fields GetDstEndpoint, getFieldAccessor, rawIPv4ToString, rawIPv6ToString and the "endpoint.version" accessor so the correct addr field and conversion are selected based on version.
421-429:⚠️ Potential issue | 🟠 Major
GetGid/GetUidnow return&0on read failure, masking “unknown” as root.For non-syscall events, failed field reads now become non-nil pointers to zero. That conflates missing identity data with valid UID/GID
0and can skew policy/rule outcomes.✅ Suggested fix (preserve nil-on-missing semantics)
func (e *DatasourceEvent) GetGid() *uint32 { @@ default: - gid, _ := e.getFieldAccessor("proc.creds.gid").Uint32(e.Data) + gid, err := e.getFieldAccessor("proc.creds.gid").Uint32(e.Data) + if err != nil { + return nil + } return &gid } } @@ func (e *DatasourceEvent) GetUid() *uint32 { @@ default: - uid, _ := e.getFieldAccessor("proc.creds.uid").Uint32(e.Data) + uid, err := e.getFieldAccessor("proc.creds.uid").Uint32(e.Data) + if err != nil { + return nil + } return &uid } }Also applies to: 721-729
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/datasource_event.go` around lines 421 - 429, The current GetGid (and similarly GetUid) ignores the read error and returns a pointer to zero on failure, conflating a missing value with UID/GID 0; change the logic in GetGid (and the corresponding GetUid at the other block) to capture the error from e.getFieldAccessor(...).Uint32(e.Data) and return nil when that call fails, only returning &gid (or &uid) when err == nil so missing/failed reads remain nil rather than &0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/utils/datasource_event.go`:
- Around line 173-175: The warning path in getFieldAccessor currently logs every
missing field via logger.L().Warning and can flood logs; change it to
deduplicate warnings per eventType+field by tracking issued warnings (e.g., a
package-level concurrent-safe set like sync.Map or a map protected by
sync.RWMutex) keyed by eventType+fieldName, only calling logger.L().Warning the
first time a given key is seen, while still returning missingFieldAccessor and
not caching misses; reference getFieldAccessor and missingFieldAccessor when
adding the dedupe check around the existing logger.L().Warning call.
---
Outside diff comments:
In `@pkg/utils/datasource_event.go`:
- Around line 275-287: GetDstEndpoint currently always reads
"endpoint.addr_raw.v4" and calls rawIPv4ToString, which yields incorrect
addresses for IPv6; update GetDstEndpoint to branch on the endpoint version (the
local variable version from getFieldAccessor("endpoint.version").Uint8) and for
IPv4 use "endpoint.addr_raw.v4" + rawIPv4ToString(addr) and for IPv6 use
"endpoint.addr_raw.v6" (via getFieldAccessor) and rawIPv6ToString on the
retrieved value, leaving the rest of the L4Endpoint population unchanged;
reference the functions/fields GetDstEndpoint, getFieldAccessor,
rawIPv4ToString, rawIPv6ToString and the "endpoint.version" accessor so the
correct addr field and conversion are selected based on version.
- Around line 421-429: The current GetGid (and similarly GetUid) ignores the
read error and returns a pointer to zero on failure, conflating a missing value
with UID/GID 0; change the logic in GetGid (and the corresponding GetUid at the
other block) to capture the error from e.getFieldAccessor(...).Uint32(e.Data)
and return nil when that call fails, only returning &gid (or &uid) when err ==
nil so missing/failed reads remain nil rather than &0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7928f6db-ecab-44af-b289-e26223858743
📒 Files selected for processing (1)
pkg/utils/datasource_event.go
Comment on lines
+173
to
175
| logger.L().Warning("field not found", helpers.String("field", fieldName), helpers.String("eventType", string(e.EventType))) | ||
| // Don't cache nil results - another data source may have this field | ||
| return missingFieldAccessor |
There was a problem hiding this comment.
Missing-field warning path can flood logs on hot event streams.
getFieldAccessor logs every miss but intentionally does not cache miss results. With many silent getter calls, this can produce high-volume warning spam and hide actionable signals.
💡 Suggested mitigation (log once per eventType+field)
var (
fieldCaches = sync.Map{}
+ missingFieldWarned = sync.Map{}
)
@@
field := e.Datasource.GetField(fieldName)
if field == nil {
- logger.L().Warning("field not found", helpers.String("field", fieldName), helpers.String("eventType", string(e.EventType)))
+ warnKey := string(e.EventType) + "|" + fieldName
+ if _, loaded := missingFieldWarned.LoadOrStore(warnKey, struct{}{}); !loaded {
+ logger.L().Warning("field not found", helpers.String("field", fieldName), helpers.String("eventType", string(e.EventType)))
+ }
// Don't cache nil results - another data source may have this field
return missingFieldAccessor
}📝 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
| logger.L().Warning("field not found", helpers.String("field", fieldName), helpers.String("eventType", string(e.EventType))) | |
| // Don't cache nil results - another data source may have this field | |
| return missingFieldAccessor | |
| field := e.Datasource.GetField(fieldName) | |
| if field == nil { | |
| warnKey := string(e.EventType) + "|" + fieldName | |
| if _, loaded := missingFieldWarned.LoadOrStore(warnKey, struct{}{}); !loaded { | |
| logger.L().Warning("field not found", helpers.String("field", fieldName), helpers.String("eventType", string(e.EventType))) | |
| } | |
| // Don't cache nil results - another data source may have this field | |
| return missingFieldAccessor | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/utils/datasource_event.go` around lines 173 - 175, The warning path in
getFieldAccessor currently logs every missing field via logger.L().Warning and
can flood logs; change it to deduplicate warnings per eventType+field by
tracking issued warnings (e.g., a package-level concurrent-safe set like
sync.Map or a map protected by sync.RWMutex) keyed by eventType+fieldName, only
calling logger.L().Warning the first time a given key is seen, while still
returning missingFieldAccessor and not caching misses; reference
getFieldAccessor and missingFieldAccessor when adding the dedupe check around
the existing logger.L().Warning call.
This was referenced Mar 17, 2026
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.
Summary by CodeRabbit