[NETOBSERV-237] gRPC+PB flow ingest/decoder for NetObserv eBPF Agent #160
Conversation
| return out | ||
| } | ||
|
|
||
| func ipToStr(ip *pbflow.IP) string { |
There was a problem hiding this comment.
If ipToStr and macToStr are generic, maybe move to some utility or common file
There was a problem hiding this comment.
I kept them as private functions because they rely on very concrete implementations of the encoding, as they were defined by us. I can move it to public functions if you think it's better to do it, but maybe they won't be used anywhere else and will increase the surface of the public API.
There was a problem hiding this comment.
@mariomac ok ... so if they are specific to the protocol you are correct and they should be kept in this context ... the only sub-idea would be to move to a file called decode_protobuf_utils.go but this is really minor idea. resolving the conversation :-)
| } | ||
|
|
||
| // Decode decodes input strings to a list of flow entries | ||
| func (c *Protobuf) Decode(in []interface{}) []config.GenericMap { |
There was a problem hiding this comment.
Are you using "c *Protobuf" in the function ???
There was a problem hiding this comment.
No, it is not used, but the API conventions of the flowlog pipeline require that Decode is run as a method of a type implementing the Decoder interface.
There was a problem hiding this comment.
k, it was strange to me that we do not have any private variable in Protobuf --- initially I asked myself why you decided to use the letter c but this is again minor.
There was a problem hiding this comment.
copy/paste issues. Changing to p or even _, as it is unused.
| if flow == nil { | ||
| return config.GenericMap{} | ||
| } | ||
| out := config.GenericMap{ |
There was a problem hiding this comment.
Should this be built somehow dynamic from https://github.com/netobserv/netobserv-agent/blob/main/proto/flow.proto ????
There was a problem hiding this comment.
That would involve flattening/changing the flow.proto definition to match the map structure, and then create some sort of code generation or use reflection to create the map. Since the number of fields is still relatively small, I'd keep using this explicit conversion, and consider another approach in the future if we feel that the fields grow until making this conversion unmaintainable.
There was a problem hiding this comment.
@mariomac this is exactly my fear :-( .... that this will grow and no one will be able to find where this came from and why this is not working. Any chance to someone connect that to the source (i.e. to the https://github.com/netobserv/netobserv-agent) project ... if not in code then at least with some remarks maybe?
There was a problem hiding this comment.
Yeah, actually the *pbflow.Records type is imported from the github.com/netobserv/netobserv-agent directly, so IDE navigation leads you to the original protobuf definition (at least in Goland IDE).
But I agree that we should investigate a more automatic way to map protobuf --> generic map fields that does not penalize readability nor performance.
eranra
left a comment
eranra
left a comment
There was a problem hiding this comment.
One high-level comment ... now with the new e2e testing framework maybe it will make sense to deploy some eBPF agent and FLP as another e2e test and have a working end-to-end test on kind ... maybe this is early and should not block this PR but a thing we should probably do in future PRs.
|
@eranra good point. There is already a task about that: https://issues.redhat.com/browse/NETOBSERV-199 but we didn't know exactly how to do it. We can implement the tests in the E2E framework. Let's see if Kind+Github hosts allow installing the agent as a privileged Pod that can send eBPF bytecode to the kernel. |
Codecov Report
@@ Coverage Diff @@
## main #160 +/- ##
==========================================
+ Coverage 58.47% 59.02% +0.54%
==========================================
Files 51 53 +2
Lines 2950 3043 +93
==========================================
+ Hits 1725 1796 +71
- Misses 1113 1128 +15
- Partials 112 119 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
3 participants
No description provided.