-
Notifications
You must be signed in to change notification settings - Fork 31
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
Filter interfaces by their associated IPs #111
Conversation
👋 @masonj188 and I are from ngrok and we are starting to use netobserv to get better visibility into our edge traffic. Let us know if this is the sort of patch you are interested in accepting and we'll push it forward. In the future we'll continue to upstream changes we need to netobserv as well. |
/assign @msherif1234 |
Thanks @joewilliams and @masonj188 for your contribution I will definitely go over your changes, for now I just wanted to make sure u are considering single and dual stack clusters support and being able to handle both address-families IPs |
Hi @msherif1234, really appreciate the quick reply!
Yep! I pushed up some test cases that cover IPv6 addresses as well, should have included those earlier. The list of IPs given in the environment variable are parsed into netip.Prefix which should cover both IPv4 and IPv6 addresses. |
return ipIfaceFilter, nil | ||
} | ||
|
||
func (f *ipInterfaceFilter) Allowed(iface string) (bool, error) { |
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.
will this Allowed logic still work with using only interface names ?
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.
If I understand your question correctly, yep it should work. With these changes a filter just has to implement "Allowed" and will always receive an interface name from the interface monitor when an interface is found/added.
The ipInterfaceFilter receives the interface name, queries for which IPs the interface is associated with, and then checks if any of the IPs were the ones passed in through the "INTERFACE_IPS" env variable, if so it returns true (interface allowed), if not then it returns false.
ipIfaceFilter := ipInterfaceFilter{} | ||
ipIfaceFilter.ipsFromIface = ipsFromIface | ||
|
||
for _, ip := range ips { |
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.
do we need special handling for loopback IPs or LinkLocal ?
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 don't think so, the code uses net.InterfaceByName() and then calls Interface.Addrs() which is happy to return loopback or link-local addresses.
Just running some code that loops through interfaces and calls .Addrs()
on my machine:
if: lo
addrs: 127.0.0.1/8
if: lo
addrs: ::1/128
if: enp6s0
addrs: 10.0.0.100/24
if: enp6s0
addrs: fe80::b981:bd8e:91f6:b3e5/64
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #111 +/- ##
==========================================
- Coverage 31.75% 31.71% -0.05%
==========================================
Files 37 37
Lines 3288 3371 +83
==========================================
+ Hits 1044 1069 +25
- Misses 2185 2239 +54
- Partials 59 63 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
@joewilliams and @masonj188, I took a quick look at the code, and this will indeed ease configuration, I really like that. Are you talking about the need to configure specific IPs or about configuration by subnets/netmasks? We are talking about interfaces for specific subnets, right? Also, can you add needed changes to https://github.com/netobserv/netobserv-ebpf-agent/blob/main/docs/config.md to describe the updated capabilities THX for the contribution :-) |
@masonj188, any progress on this PR? I would very much like to see progress and this is a really good idea and really good code. :-) |
@eranra Thanks for the bump! Sorry this dropped off my radar for a bit - we've been using the the agent with this patch for quite a while now! The agent as a whole has been great, we really appreciate all the work all of you have put into it. I'll get some of the already mentioned things fixed here in the next hour. Outside of that, I am interested in getting your (and/or @msherif1234) opinion on supplying the subnet masks. As it's written right now you have to supply the exact IP+mask in CIDR notation for it to match an interface - I'm kind of thinking we should change that behavior. What if I changed the behavior to allow providing a list of plain IPs and/or subnets:
Essentially it would be "Allow interfaces with this provided IP" or "Allow interfaces with an associated IP in this subnet" respectively, and it would be fine to provide a mix of both, i.e. One reason I don't like having to supply the subnet mask with an IP is that we ran into the case where if your host is getting IPv6 addresses via DHCPv6 it can/will assign a /128 subnet to the interface even when the "real" subnet is a /64, it just gets that information from a Router Advertisement and updates the routing table, but never changes the interface assignment. It felt like a footgun to have to supply |
e178249
to
1f621da
Compare
pkg/agent/agent.go
Outdated
return nil, fmt.Errorf("configuring interface filters: %w", err) | ||
var filter InterfaceFilter | ||
|
||
if len(cfg.InterfaceIPs) > 0 { |
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.
do we need to flag an error if user supplied both cfg.InterfaceIPs
and cfg.Interfaces
?
/ok-to-test |
New image: quay.io/netobserv/netobserv-ebpf-agent:1733b58. It will expire after two weeks. |
1 similar comment
New image: quay.io/netobserv/netobserv-ebpf-agent:1733b58. It will expire after two weeks. |
/test all |
@msherif1234: No presubmit jobs available for netobserv/netobserv-ebpf-agent@main In response to this:
Instructions 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/test-infra repository. |
@masonj5n I would like to get this PR merged can u pls rebase it Thanks!! |
0f9b8fc
to
683f46c
Compare
Hi @msherif1234, thanks for the poke! I went ahead and made some changes so it matches my last comment - where you supply a list IP CIDRs/ranges and then any interface with an IP in those ranges will be listened on. Only difference from my comment is that if you want a single IP you'd provide a Also, it looks like a recent dependabot update bumped
|
70b810d
to
b90e9df
Compare
go.mod
Outdated
go 1.19 | ||
go 1.21 | ||
|
||
toolchain go1.21.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.
why u need this ? it seems to fail lint check in ur PR, pls run
make lint
on ur view
also main has the updated ipfix dependency and things compile just fine with go1.19 so I am not sure why we need to bump to go 1.21 ?
@jotak is there any concern advancing to go 1.21 the github checks for 1.19 and 1.20 ?
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 didn't realize this but the behavior changed in go 1.21 so that it's a compile error if a dependency has a go
directive with a higher go
version than what you're compiling it with:
From https://go.dev/doc/modules/gomod-ref#go:
The go directive sets the minimum version of Go required to use this module. Before Go 1.21, the directive was advisory only; now it is a mandatory requirement: Go toolchains refuse to use modules declaring newer Go versions.
So, I think things will fail to build on go 1.21 until the go.mod
is updated like I show here (because ipfix requires 1.21), but I did switch back to go 1.19 and was able to run go mod tidy
and go mod vendor
and run make build
, so I removed this change.
b90e9df
to
d1e5d69
Compare
/LGTM |
Thanks @masonj5n for your contribution!! |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: msherif1234 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
We have a use case where it's easy for us to pass the IP(s) of the interfaces we'd like to monitor as an environment variable but quite a bit harder to know the names of those interfaces in advance.
Here's a first pass at adding another filter type that allows/disallows interfaces based on whether it's associated with the passed in IPs. There's still some logic around the poller/watcher for interfaces that'd need some additions to also watch for when IPs are attached/unattached and I'm sure other things that would need updates too.
Is there any interest in adding a feature like this? Happy to change approach from what I have here or otherwise do what still needs done to get it merged if so!