-
Notifications
You must be signed in to change notification settings - Fork 697
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
link: add test suite for netkit #1463
Conversation
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.
Left some questions, thanks for putting in the work to contribute the support upstream!
@ti-mo remind me, adding a dependency to tests doesn't cause downstream consumers to depend on rtnetlink?
link/netkit_test.go
Outdated
blackhole := driver.NetkitPolicyDrop | ||
err = conn.Link.New(&rtnetlink.LinkMessage{ | ||
Family: unix.AF_UNSPEC, | ||
Index: 1000, |
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 means we can't call the function more than once per test or concurrently.
Is there a way to get the kernel to allocate an ID + retrieve the ID from the netlink answer somehow?
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 we do not provide the index the kernel will assign the index, the same goes for the name, just like the ip command line tool without giving any arguments. We are getting the link with net.InterfaceByName
so we do not need to provide the index. But even if we get rid of the index we still have a hard-coded interface name. I am not sure what we can do about that. If we do not give the name the kernel will assign names starting from nk0 and nk1, and whatever available next.
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.
Could we get the interface index from the calling test function? It would be the caller's responsibility to give a unique index for every call. Also, we would not need to call net.InterfaceByName
nor return the index from net.InterfaceByName
. Also, conn can come from the caller so that if we need to call the function more than once we would not need to create new connections every time. What do you think?
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.
Could we get the interface index from the calling test function? It would be the caller's responsibility to give a unique index for every call.
I like this, seems straightforward enough.
Also, conn can come from the caller so that if we need to call the function more than once we would not need to create new connections every time. What do you think?
That would make it more cumbersome to call (because we need another func to dial the conn). I'd just dial a new conn as needed until it turns out that doesn't work for some reason.
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 not generate a random string at the start of the function and use it as the interface name?
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.
Can do, ultimately this isn't going to happen often I'd guess. So manually allocating indices is probably not too much of a hassle.
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.
Push changes.
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.
Ended up adding a small sequence generator which allocates ids automatically.
9bf62a3
to
fab29fe
Compare
The netkit link support was added without a test suite due to a lack of support for driver-specific extractions in the `github.com/jsimonetti/rtnetlink` package. The driver support has been added to `rtnetlink`. This commit adds a test suite for the netkit link and pulls in the `github.com/jsimonetti/rtnetlink/v2` package for testing. Signed-off-by: Birol Bilgin <birolbilgin@gmail.com>
fab29fe
to
45ae194
Compare
The netkit link support was added without a test suite due to a lack of support for driver-specific extractions in the
github.com/jsimonetti/rtnetlink
package. The driver support has been added tortnetlink
. This PR adds a test suite for the netkit link and pulls in thegithub.com/jsimonetti/rtnetlink/v2
package for testing.Fixes: #1350