-
Notifications
You must be signed in to change notification settings - Fork 108
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
avoid ipv6 loopback address reported as src or dst of span #1021
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1021 +/- ##
==========================================
+ Coverage 79.06% 81.64% +2.58%
==========================================
Files 137 137
Lines 10923 10923
==========================================
+ Hits 8636 8918 +282
+ Misses 1761 1497 -264
+ Partials 526 508 -18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
e30ea1f
to
ee3f85f
Compare
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.
LGTM! Any concern from the rest of team members?
pkg/internal/ebpf/common/common.go
Outdated
@@ -231,10 +231,21 @@ func cstr(chars []uint8) string { | |||
} | |||
|
|||
func (connInfo *BPFConnInfo) reqHostInfo() (source, target string) { | |||
oneAddress := [16]uint8{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1} |
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.
This is technically the IPv6 Loopback address. I'm still concerned that we have an event with it, but I don't mind putting a check to verify it's valid.
The Go library code does it like this:
func (ip IP) IsLoopback() bool {
if ip4 := ip.To4(); ip4 != nil {
return ip4[0] == 127
}
return ip.Equal(IPv6loopback)
}
So I think all we need to do is:
if src.Equal(net.IPv6loopback) {
src = ""
}
if dst.Equal(net.IPv6loopback) {
dst = ""
}
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.
that is much cleaner, updated ....
on another note, does it make sense to report ipv4 loopback addresses in traces?
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.
I'd say it might have sense. E.g. if you run Beyla on a baremetal host that runs multiple services.
You can always use filters to remove local traces: https://grafana.com/docs/beyla/latest/configure/options/#filter-metrics-and-traces-by-attribute-values
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.
LGTM! Thanks for your contribution @esara !
address first part of #895