-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NETOBSERV-759: decorate flows with agent IP #78
Conversation
only for Protocol Buffers encoding. IPFIX is still missing.
Codecov Report
@@ Coverage Diff @@
## main #78 +/- ##
==========================================
+ Coverage 36.71% 38.02% +1.31%
==========================================
Files 26 28 +2
Lines 1841 1959 +118
==========================================
+ Hits 676 745 +69
- Misses 1134 1176 +42
- Partials 31 38 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@mariomac can you provide some motivation for this enrichment :-) --- we have the network Interface ID --- can't we get that data from k8s and not send that field over and over for each flow? |
@eranra We want to provide the IP of the reporter to help us deciding whether a flow is going outside the node or comes from another node (see: netobserv/flowlogs-pipeline#350) Also, flows can be also captured from virtual interfaces or physical interfaces that do no lead to external traffic. |
New image: ["quay.io/netobserv/netobserv-ebpf-agent:a05f544"]. It will expire after two weeks. |
@eranra do you have strong concerns about this? |
tested and works as expected |
@@ -12,6 +12,9 @@ import ( | |||
|
|||
var ilog = logrus.WithField("component", "exporter/IPFIXProto") | |||
|
|||
// TODO: encode also the equivalent of the Protobuf's AgentIP field in a format that is binary- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For information, in goflow's IPFIX this was called "SamplerAddress": https://github.com/cloudflare/goflow#output-format
In IPFIX spec (https://www.iana.org/assignments/ipfix/ipfix.xhtml) this is exporterIPv4Address
/ exporterIPv6Address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it what you already have in exporter *ipfixExporter.ExportingProcess
?
@praveingk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jotak the ExportingProcess has a net.Conn, which could be used to get the info. Yes, exporterIPv4(/6)Address seems right though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, the ExportingProcess could be used only if the AgentIPIface is set as "External"
type InterfaceNamer func(ifIndex int) string | ||
|
||
// Decorate adds to the flows extra metadata fields that are not directly fetched by eBPF: | ||
// - The interface name (corresponding to the interface index in the flow). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mariomac the interface name was already in the flows, is it just that you refactored at the same time the way it is added, right? Or did you change the flow content wrt interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was added in an earlier stage in the flow creation, but the constructor required to inject an external component (interfaceNamer) and I think that moving the extra decoration to another stage makes the code cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
Merging to unblock other PRs/tests and, if @praveingk has any concern about the current PR, we can address them into another PR. |
@jotak I am actually on vacation and not in front of the computer ( maybe a meeting could be helpful on that) --- and I understand the value of knowing which agent reported about a flow record --- I think that this might be concluded differently from the interface ID that we already reported, so this makes this field redundant. I am not super opinionated on this addition, I think this adds data over and over in each flow record we report, so it makes sense to think if we need that. |
* NETOBSERV-759: decorate flows with agent IP only for Protocol Buffers encoding. IPFIX is still missing. * Fix GRPC export + test
Only for Protocol Buffers encoding. IPFIX is still missing.
This PR creates a new "Decorator" instance and moves there also the interface naming process.
At least for OVN-K, the default
external
IP parsing coincides with the Node IP without any kind of tuning from the NOO: