ROX-30390: Add indicator key functions#16621
Conversation
|
Skipping CI for Draft Pull Request. |
|
This change is part of the following stack: Change managed by git-spice. |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider centralizing the repeated Key(h) switch logic into a helper or interface to reduce duplication across NetworkConn, ContainerEndpoint, and ProcessListening.
- The hashToHexString function can be simplified to fmt.Sprintf("%016x", h.Sum64()) instead of manually building a byte slice, which will improve readability.
- Rather than silently defaulting unknown HashingAlgo values to the hash implementation, consider validating the input and handling invalid algos explicitly to avoid unintentional fallbacks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider centralizing the repeated Key(h) switch logic into a helper or interface to reduce duplication across NetworkConn, ContainerEndpoint, and ProcessListening.
- The hashToHexString function can be simplified to fmt.Sprintf("%016x", h.Sum64()) instead of manually building a byte slice, which will improve readability.
- Rather than silently defaulting unknown HashingAlgo values to the hash implementation, consider validating the input and handling invalid algos explicitly to avoid unintentional fallbacks.
## Individual Comments
### Comment 1
<location> `sensor/common/networkflow/manager/indicator/key.go:125` </location>
<code_context>
+ }
+}
+
+func hashStrings(h hash.Hash64, strs ...string) {
+ for _, s := range strs {
+ _, _ = h.Write([]byte(s))
</code_context>
<issue_to_address>
Potential risk of hash collisions due to lack of delimiters between concatenated strings.
Add a delimiter or length prefix between each string when writing to the hash to prevent collisions, especially if strings may contain arbitrary data.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Images are ready for the commit at 7d81935. To use with deploy scripts, first |
Yes, it can, but |
I find this overkill. I can consider that once we add an env variable to switch the algorithm (that would come later). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #16621 +/- ##
==========================================
- Coverage 48.72% 48.66% -0.06%
==========================================
Files 2658 2661 +3
Lines 198307 198976 +669
==========================================
+ Hits 96625 96833 +208
- Misses 94112 94552 +440
- Partials 7570 7591 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/retest |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
/retest |
7d81935 to
d328baa
Compare
|
Images are ready for the commit at d9102ef. To use with deploy scripts, first |
|
Images are ready for the commit at f73527e. To use with deploy scripts, first |
lvalerom
left a comment
There was a problem hiding this comment.
Looks great! Love the bitwise operations ❤️
|
@vikin91: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
CI failures of and They must be unrelated to this particular change, so I allow myself to skip investigating them deeper. |
|
/retest |
Description
The goal of this PR is to add ID functions for all indicators. There are two functions:
keyString()that consumes more memory, but is CPU-optimized and can be used for clusters with little netFlow events,keyHash()that optimizes memory usage but consumes more CPU. It should be used as default, especially in clusters with many open endpoints, processes and network connections.The produced keys are not used in this PR. They will be used as deduper key in the next PR.
User-facing documentation
Testing and quality
Automated testing
How I validated my change