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

Add support for netkit device #1257

Merged
merged 1 commit into from
Mar 12, 2024
Merged

Add support for netkit device #1257

merged 1 commit into from
Mar 12, 2024

Conversation

hemanthmalla
Copy link
Member

@hemanthmalla hemanthmalla commented Dec 6, 2023

Adds support for attaching bpf programs to netkit devices using bpf links.

Generated code is currently based on 6.7-rc4

TODO :

  • Decide on a way to create netkit device in CI for testing.
  • Re-gen code once 6.7 is out and validate for any changes.

@chent1996
Copy link
Contributor

Thanks for this great work!

@borkmann
Copy link
Member

borkmann commented Jan 8, 2024

@hemanthmalla happy new year! Given 6.7 is officially released now, could you move forward with the PR? Thanks! :)

@hemanthmalla
Copy link
Member Author

@lmb I remember reading somewhere, that cilium/ebpf prefers not to have libs like vishvananda/netlink as dependencies. Do you have thoughts on how we should go about creating a netkit device in CI for tests?
Is shelling out to do something like ip link add nk0 type netkit the best option here ?

@ti-mo
Copy link
Collaborator

ti-mo commented Jan 29, 2024

@lmb I remember reading somewhere, that cilium/ebpf prefers not to have libs like vishvananda/netlink as dependencies. Do you have thoughts on how we should go about creating a netkit device in CI for tests? Is shelling out to do something like ip link add nk0 type netkit the best option here ?

Interesting, I never realized we attached to ifindex 1 in the XDP tests, but that obviously won't work for netkit. I think newer versions of Go automatically prune testing-only module dependencies, so technically there wouldn't be any downside to importing e.g. jsimonetti/rtnetlink. (e.g. vendoring the lib doesn't automatically pull in quicktest either, afaik)

I think we can soften our stance on importing a netlink lib for testing if this is the case. Not sure if we can do the same for examples. I think shelling out to ip would be strictly worse, since we'll need to deal with the various failure cases then. Using a library would make it easier to degrade gracefully (e.g. skip a test if creating a netkit dev returns EINVAL etc.) without resorting to screen scraping ip. I'd like to avoid that.

@hemanthmalla hemanthmalla marked this pull request as ready for review February 2, 2024 04:50
@hemanthmalla hemanthmalla requested a review from a team as a code owner February 2, 2024 04:50
lmb
lmb previously requested changes Feb 2, 2024
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.

Looks fine, only some small requests. Thanks!

link/netkit.go Show resolved Hide resolved
link/netkit_test.go Outdated Show resolved Hide resolved
link/netkit.go Show resolved Hide resolved
Copy link
Contributor

@rgo3 rgo3 left a comment

Choose a reason for hiding this comment

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

LGTM, just one small nit.

link/netkit.go Show resolved Hide resolved
Signed-off-by: Hemanth Malla <hemanth.malla@datadoghq.com>
@hemanthmalla
Copy link
Member Author

Test failures seem flaky to me. Previously only ci / Run tests on previous stable Go failed, re-triggered the tests on same commit changes and now Run tests on pre-built kernel (6.7) seems to fail. How do I re-trigger just the failed test ?

@hemanthmalla hemanthmalla requested review from ti-mo and lmb March 4, 2024 15:08
@ti-mo ti-mo dismissed lmb’s stale review March 12, 2024 08:24

Feedback addressed

@ti-mo ti-mo merged commit c5e9cb3 into cilium:main Mar 12, 2024
15 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.

6 participants