Skip to content

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 force-pushed the netkit branch 2 times, most recently from f806346 to ceb0c7d Compare February 2, 2024 01:16
@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!

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.

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
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