Add support for getting links in different namespaces - #1158
Conversation
📝 WalkthroughWalkthroughAdded NSID-aware link lookup APIs (by index and by name) that target a specific network namespace via IFLA_TARGET_NETNSID, updated execGetLink to map ENODEV to LinkNotFoundError, and added tests plus a test helper to create and inspect a link inside a namespaced environment. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant H as Handle
participant S as NetlinkSocket
participant K as Kernel
C->>H: LinkByNameAndNsid(name, nsid) / LinkByIndexAndNsid(index, nsid)
H->>H: Build GETLINK message\nset IFLA_TARGET_NETNSID\n(optional) add VF filter\n(optional) set IFLA_IFNAME/ALT_IFNAME
H->>S: send NETLINK GETLINK request
S->>K: deliver request to kernel
K-->>S: reply (link attrs) or error (e.g., ENODEV)
S-->>H: return response
H->>H: execGetLink parses response\nif ENODEV -> LinkNotFoundError
H-->>C: return Link or LinkNotFoundError
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@link_linux.go`:
- Around line 2142-2165: The LinkByNameAndNsid method currently falls back to
linkByNameDump when h.options.lookupByDump is true, which ignores the requested
target NSID; remove that fallback so LinkByNameAndNsid always constructs and
sends the RTM_GETLINK request with the IFLA_TARGET_NETNSID attribute (as already
done in the function) and, if necessary, detect/report unsupported kernels by
returning a clear error instead of calling linkByNameDump; update references to
h.options.lookupByDump in LinkByNameAndNsid and ensure the function always uses
the netlink request path that adds nl.NewRtAttr(unix.IFLA_TARGET_NETNSID, ...)
(and preserves the IFLA_EXT_MASK/IFLA_IFNAME/IFLA_ALT_IFNAME logic).
In `@link_test.go`:
- Around line 2223-2251: Add a kernel-version guard to both
TestLinkByIndexAndNsid and TestLinkByNameAndNsid by calling minKernelRequired(t,
5, 0) at the start of each test (before other setup like
t.Cleanup(setUpNetlinkTest(t))) so the tests are skipped on kernels older than
5.0 which lack IFLA_TARGET_NETNSID.
In `@netlink_test.go`:
- Around line 308-318: The NsHandle returned by netns.GetFromName (ns) is not
closed, leaking file descriptors; after obtaining the integer fd (int(ns)) and
calling GetNetNsIdByFd, close the handle (call ns.Close() or defer ns.Close()
after extracting the fd) before returning from the test; ensure you convert ns
to an int first and then close the NsHandle so you don't use the closed handle
when calling GetNetNsIdByFd (reference symbols: netns.GetFromName, ns,
GetNetNsIdByFd, ns.Close()).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@netlink_test.go`:
- Around line 268-321: The test hard-fails when the host's iproute2 lacks "ip
netns set ... auto" or "ip --json" support; update setUpNamespaceWithLink to run
those ip commands with CombinedOutput(), inspect the output/error for strings
like "unknown" or "Usage" (or any indication of unsupported flags), and call
t.Skipf(...) with a clear message when detected instead of t.Fatalf;
specifically replace the exec.Command(...).Run() calls for "ip netns set ...
auto" and the "ip --json link show" invocation with CombinedOutput checks and
skip behavior so older iproute2 versions gracefully skip the test.
🧹 Nitpick comments (1)
link_linux.go (1)
2116-2164: Add GoDoc for the new exported NSID APIs.
These are exported functions/methods, so adding brief doc comments keeps the public surface consistent and avoids lint noise.✍️ Suggested GoDoc
+// LinkByIndexAndNsid finds a link by index in the specified netnsid. func LinkByIndexAndNsid(index int, nsid int) (Link, error) { return pkgHandle.LinkByIndexAndNsid(index, nsid) } @@ +// LinkByNameAndNsid finds a link by name in the specified netnsid. func LinkByNameAndNsid(name string, nsid int) (Link, error) { return pkgHandle.LinkByNameAndNsid(name, nsid) }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@link_linux.go`:
- Around line 2140-2146: The doc comment for the functions LinkByNameAndNsid
incorrectly references "LinkByIndexAndName"; update the comments above both the
package-level LinkByNameAndNsid and the method (h *Handle) LinkByNameAndNsid to
accurately describe that they find a link by name in a different namespace and
return a Link (or pointer to the object as appropriate), ensuring the comment
text matches the function names LinkByNameAndNsid and clarifies the parameters
(name string, nsid int) and return values.
In `@netlink_test.go`:
- Around line 268-333: The helper setUpNamespaceWithLink currently returns
cleanupNs but calls t.Skip/t.Fatalf after setUpNamedNetlinkTest, leaving
cleanupNs uncalled on early exits; update the function so that any early-exit
path after cleanupNs is obtained (every branch that calls t.Skip or t.Fatalf)
invokes cleanupNs() before exiting (or call defer cleanupNs() immediately after
obtaining it and adjust returns so you don’t run it twice), e.g., ensure
cleanupNs() is executed before the error branches around the "ip netns set", "ip
link add", "ip --json link show", and json/unmarshal checks so the namespace and
OS-thread state are always cleaned up even on failures.
This commit adds two functions, `LinkByIndexAndNsid` and `LinkByNameAndNsid` which look up a link in a different namespace, identified by its nsid. This is particularly useful for getting the peer of a veth interface. The function `LinkByNameAndNsid` does not support the fallback functionality of getting the link from a dump, as the dump only contains links in the current namespace. Signed-off-by: Gwendolyn <me@gwendolyn.dev>
|
To avoid confusion: the AI summary is wrong, I did not do any changes to |
|
I may be missing some context, but why not define your own |
|
From what I understand, opening a handle to another namespace requires a file descriptor for that namespace (which can be acquired from a PID via |
|
Thanks @gwenya Next, I am trying to understand, does it even make sense to have this new method under Handle, should we just add a package level method? If you see a use case for it, let's add it to Handle as well. |
|
@aboch I am not entirely certain but I think it has to be under handle, since the netns IDs are relative to the namespace they are used from. I.e. namespace A might have ID 0 from the perspective of namespace B but ID 1 from the perspective of namespace C. I do think that we probably don't actually need the method to get by name and netns id, since the only use case I am aware of is getting the peer of a veth interface, which would always be by index and netns id |
3 participants
Rtnetlink supports getting links from different namespaces by sending
IFLA_TARGET_NETNSIDwith aRTM_GETLINKrequest.This is particularly useful for getting the peer of a veth, as the kernel tells us the netnsid of the peer but opening a netlink socket in that namespace requires a file descriptor into the namespace, and getting such a file descriptor from a netnsid requires enumerating all namespaces.
This PR adds two functions:
LinkByNameAndNsidandLinkByIndexAndNsid. These functions are duplicates ofLinkByNameandLinkByIndex, but specify theIFLA_TARGET_NETNSIDproperty in the request. The functionLinkByNameAndNsiddoes not support the fallback functionality of getting the link from a dump, as the dump only includes links in the current namespace.I am writing the nsid with
nl.Uint32Attr(), but it is actually a signed 32 bit int. I can't find any function to write signed int attributes, I'm not sure if it makes a difference since go's cast should keep the bits the same (I think).Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.