Skip to content
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

Merged
merged 1 commit into from May 15, 2024
Merged

Conversation

brlbil
Copy link
Contributor

@brlbil brlbil commented May 10, 2024

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 PR adds a test suite for the netkit link and pulls in the github.com/jsimonetti/rtnetlink/v2 package for testing.

Fixes: #1350

@brlbil brlbil marked this pull request as ready for review May 10, 2024 19:28
@brlbil brlbil requested review from mmat11 and a team as code owners May 10, 2024 19:28
Copy link
Collaborator

@lmb lmb left a 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 Show resolved Hide resolved
blackhole := driver.NetkitPolicyDrop
err = conn.Link.New(&rtnetlink.LinkMessage{
Family: unix.AF_UNSPEC,
Index: 1000,
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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.

Copy link
Collaborator

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?

Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Push changes.

Copy link
Collaborator

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.

@brlbil brlbil force-pushed the pr/brlbil/add_netkit_test branch from 9bf62a3 to fab29fe Compare May 14, 2024 20:44
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>
@lmb lmb force-pushed the pr/brlbil/add_netkit_test branch from fab29fe to 45ae194 Compare May 15, 2024 10:03
@lmb lmb merged commit 43c4809 into cilium:main May 15, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test suite for netkit devices
3 participants