Repository: mdlayher/netlink
Pull request #143 (2019-06-05 16:49:03)

netlink: add netNS type for network namespace manipulation
I was a little leery of directly opening files and passing back the file descriptor without managing the entire lifetime of the file (how long does it continue to live?), so I decided to evaluate things a bit more and see what I could come up with.

The netNS type is a wrapper around network namespace operations which hopefully makes things clear and makes the lifetime of these network namespace files easy to reason about.


Patch:
diff --git a/conn_linux.go b/conn_linux.go
index 2c3005b..d9a6d21 100644
--- a/conn_linux.go
+++ b/conn_linux.go
@@ -622,10 +622,11 @@ func newLockedNetNSGoroutine(netNS int) (*lockedNetNSGoroutine, error) {
 	// Any bare syscall errors (e.g. setns) should be wrapped with
 	// os.NewSyscallError for the remainder of this function.
 
-	callerNS, err := getThreadNetNS()
+	callerNS, err := threadNetNS()
 	if err != nil {
 		return nil, err
 	}
+	defer callerNS.Close()
 
 	g := &lockedNetNSGoroutine{
 		doneC: make(chan struct{}),
@@ -641,6 +642,10 @@ func newLockedNetNSGoroutine(netNS int) (*lockedNetNSGoroutine, error) {
 		// messages to the wrong places.
 		// See: http://lists.infradead.org/pipermail/libnl/2017-February/002293.html.
 		//
+		//
+		// In addition, the OS thread must also remain locked because we attempt
+		// to manipulate the network namespace of the thread within this goroutine.
+		//
 		// The intent is to never unlock the OS thread, so that the thread
 		// will terminate when the goroutine exits starting in Go 1.10:
 		// https://go-review.googlesource.com/c/go/+/46038.
@@ -655,11 +660,12 @@ func newLockedNetNSGoroutine(netNS int) (*lockedNetNSGoroutine, error) {
 		defer g.wg.Done()
 
 		// Get the current namespace of the thread the goroutine is locked to.
-		origNetNS, err := getThreadNetNS()
+		threadNS, err := threadNetNS()
 		if err != nil {
 			errC <- err
 			return
 		}
+		defer threadNS.Close()
 
 		// Attempt to set the network namespace of the current thread to either:
 		// - the namespace referred to by the provided file descriptor from config
@@ -669,12 +675,12 @@ func newLockedNetNSGoroutine(netNS int) (*lockedNetNSGoroutine, error) {
 		explicitNS := true
 		if netNS == 0 {
 			explicitNS = false
-			netNS = callerNS
+			netNS = int(callerNS.FD())
 		}
 
 		// Only return an error if the network namespace was explicitly
 		// configured; implicit configuration by zero value should be ignored.
-		err = setThreadNetNS(netNS)
+		err = threadNS.Set(netNS)
 		switch {
 		case err != nil && explicitNS:
 			errC <- err
@@ -682,7 +688,7 @@ func newLockedNetNSGoroutine(netNS int) (*lockedNetNSGoroutine, error) {
 		case err == nil:
 			// If the thread's namespace has been successfully manipulated,
 			// make sure we change it back when the goroutine returns.
-			defer setThreadNetNS(origNetNS)
+			defer threadNS.Restore()
 		default:
 			// We couldn't successfully set the namespace, but the caller didn't
 			// explicitly ask for it to be set either. Continue.
diff --git a/netns_linux.go b/netns_linux.go
index f617c10..ca44514 100644
--- a/netns_linux.go
+++ b/netns_linux.go
@@ -9,19 +9,36 @@ import (
 	"golang.org/x/sys/unix"
 )
 
-// getThreadNetNS gets the network namespace file descriptor of the thread the current goroutine
-// is running on. Make sure to call runtime.LockOSThread() before this so the goroutine does not
-// get scheduled on another thread in the meantime.
-func getThreadNetNS() (int, error) {
-	file, err := os.Open(fmt.Sprintf("/proc/%d/task/%d/ns/net", unix.Getpid(), unix.Gettid()))
+// A netNS is a handle that can manipulate network namespaces.
+//
+// Operations performed on a netNS must use runtime.LockOSThread before
+// manipulating any network namespaces.
+type netNS struct {
+	f *os.File
+}
+
+// threadNetNS constructs a netNS using the network namespace of the calling
+// thread. If the namespace is not the default namespace, runtime.LockOSThread
+// should be invoked first.
+func threadNetNS() (*netNS, error) {
+	f, err := os.Open(fmt.Sprintf("/proc/self/task/%d/ns/net", unix.Gettid()))
 	if err != nil {
-		return -1, err
+		return nil, err
 	}
-	return int(file.Fd()), nil
+
+	return &netNS{f: f}, nil
 }
 
-// setThreadNetNS sets the network namespace of the thread of the current goroutine to
-// the namespace described by the user-provided file descriptor.
-func setThreadNetNS(fd int) error {
+// Close releases the handle to a network namespace.
+func (n *netNS) Close() error { return n.f.Close() }
+
+// FD returns a file descriptor which represents the network namespace.
+func (n *netNS) FD() int { return int(n.f.Fd()) }
+
+// Restore restores the original network namespace for the calling thread.
+func (n *netNS) Restore() error { return n.Set(n.FD()) }
+
+// Set sets a new network namespace for the current thread using fd.
+func (n *netNS) Set(fd int) error {
 	return os.NewSyscallError("setns", unix.Setns(fd, unix.CLONE_NEWNET))
 }


Test patch:
diff --git a/conn_linux_gteq_1.12_integration_test.go b/conn_linux_gteq_1.12_integration_test.go
index f03086b..cd847b1 100644
--- a/conn_linux_gteq_1.12_integration_test.go
+++ b/conn_linux_gteq_1.12_integration_test.go
@@ -156,23 +156,37 @@ func TestIntegrationConnNetNSImplicit(t *testing.T) {
 	runtime.LockOSThread()
 	defer runtime.UnlockOSThread()
 
-	origNS, err := netlink.GetThreadNetNS()
+	threadNS, err := netlink.ThreadNetNS()
 	if err != nil {
 		t.Fatalf("failed to get current network namespace: %v", err)
 	}
+	defer threadNS.Close()
 
-	defer func() {
-		if err := netlink.SetThreadNetNS(origNS); err != nil {
-			t.Fatalf("failed to restore original network namespace: %v", err)
-		}
-	}()
-
-	if err := netlink.SetThreadNetNS(int(f.Fd())); err != nil {
+	if err := threadNS.Set(int(f.Fd())); err != nil {
 		t.Fatalf("failed to enter new network namespace: %v", err)
 	}
 
-	// Any netlink connections created beyond this point should set themselves
-	// into the new namespace automatically as well.
+	// A newly created netlink connection should enter the new network namespace
+	// associated with this thread automatically.
+	if !findLink(t, ifName) {
+		t.Fatalf("did not find interface %q in namespace %q", ifName, ns)
+	}
+
+	// Return to the default namespace.
+	//
+	// A newly created netlink connection should NOT find the link because it
+	// is now in the default namespace.
+	if err := threadNS.Restore(); err != nil {
+		t.Fatalf("failed to restore original network namespace: %v", err)
+	}
+
+	if findLink(t, ifName) {
+		t.Fatalf("found interface %q in default namespace", ifName)
+	}
+}
+
+func findLink(t *testing.T, name string) bool {
+	t.Helper()
 
 	c, err := rtnl.Dial(nil)
 	if err != nil {
@@ -187,15 +201,13 @@ func TestIntegrationConnNetNSImplicit(t *testing.T) {
 
 	var found bool
 	for _, ifi := range ifis {
-		if ifi.Name == ifName {
+		if ifi.Name == name {
 			found = true
 			break
 		}
 	}
 
-	if !found {
-		t.Fatalf("did not find interface %q in namespace %q", ifName, ns)
-	}
+	return found
 }
 
 func mustBeTimeoutNetError(t *testing.T, err error) {
diff --git a/export_linux_test.go b/export_linux_test.go
index bf6446b..984ed8f 100644
--- a/export_linux_test.go
+++ b/export_linux_test.go
@@ -4,8 +4,17 @@ package netlink
 
 // This file exports certain identifiers for use in tests.
 
-// GetThreadNetNS wraps the internal getThreadNetNS for use in tests.
-func GetThreadNetNS() (int, error) { return getThreadNetNS() }
+// A NetNS wraps an internal netNS.
+type NetNS struct {
+	*netNS
+}
 
-// SetThreadNetNS wraps the internal setThreadNetNS for use in tests.
-func SetThreadNetNS(fd int) error { return setThreadNetNS(fd) }
+// ThreadNetNS wraps the internal threadNetNS.
+func ThreadNetNS() (*NetNS, error) {
+	ns, err := threadNetNS()
+	if err != nil {
+		return nil, err
+	}
+
+	return &NetNS{netNS: ns}, nil
+}