Merge pull request containernetworking#131 from squeed/no-delete-ns

pkg/ns: remove namespace creation (and move to testutils)

pkg/ip/link_linux_test.go

@@ -25,6 +25,7 @@ import (
 
  "github.com/containernetworking/plugins/pkg/ip"
  "github.com/containernetworking/plugins/pkg/ns"
+ "github.com/containernetworking/plugins/pkg/testutils"
 
  "github.com/vishvananda/netlink"
 )
@@ -58,10 +59,10 @@ var _ = Describe("Link", func() {
  BeforeEach(func() {
  var err error
 
- hostNetNS, err = ns.NewNS()
+ hostNetNS, err = testutils.NewNS()
  Expect(err).NotTo(HaveOccurred())
 
- containerNetNS, err = ns.NewNS()
+ containerNetNS, err = testutils.NewNS()
  Expect(err).NotTo(HaveOccurred())
 
  fakeBytes := make([]byte, 20)

pkg/ipam/ipam_linux_test.go

@@ -21,6 +21,7 @@ import (
  "github.com/containernetworking/cni/pkg/types"
  "github.com/containernetworking/cni/pkg/types/current"
  "github.com/containernetworking/plugins/pkg/ns"
+ "github.com/containernetworking/plugins/pkg/testutils"
 
  "github.com/vishvananda/netlink"
 
@@ -48,7 +49,7 @@ var _ = Describe("ConfigureIface", func() {
  BeforeEach(func() {
  // Create a new NetNS so we don't modify the host
  var err error
- originalNS, err = ns.NewNS()
+ originalNS, err = testutils.NewNS()
  Expect(err).NotTo(HaveOccurred())
 
  err = originalNS.Do(func(ns.NetNS) error {

pkg/ns/README.md

@@ -12,10 +12,6 @@ For example, you cannot rely on the `ns.Set()` namespace being the current names
 The `ns.Do()` method provides **partial** control over network namespaces for you by implementing these strategies. All code dependent on a particular network namespace (including the root namespace) should be wrapped in the `ns.Do()` method to ensure the correct namespace is selected for the duration of your code. For example:
 
 ```go
-targetNs, err := ns.NewNS()
-if err != nil {
- return err
-}
 err = targetNs.Do(func(hostNs ns.NetNS) error {
  dummy := &netlink.Dummy{
  LinkAttrs: netlink.LinkAttrs{
@@ -26,11 +22,16 @@ err = targetNs.Do(func(hostNs ns.NetNS) error {
 })
 ```
 
-Note this requirement to wrap every network call is very onerous - any libraries you call might call out to network services such as DNS, and all such calls need to be protected after you call `ns.Do()`. The CNI plugins all exit very soon after calling `ns.Do()` which helps to minimize the problem.
+Note this requirement to wrap every network call is very onerous - any libraries you call might call out to network services such as DNS, and all such calls need to be protected after you call `ns.Do()`. All goroutines spawned from within the `ns.Do` will not inherit the new namespace. The CNI plugins all exit very soon after calling `ns.Do()` which helps to minimize the problem.
 
-Also: If the runtime spawns a new OS thread, it will inherit the network namespace of the parent thread, which may have been temporarily switched, and thus the new OS thread will be permanently "stuck in the wrong namespace".
+When a new thread is spawned in Linux, it inherits the namepaces of its parent. In versions of go **prior to 1.10**, if the runtime spawns a new OS thread, it picks the parent randomly. If the chosen parent thread has been moved to a new namespace (even temporarily), the new OS thread will be permanently "stuck in the wrong namespace", and goroutines will non-deterministically switch namespaces as they are rescheduled.
+
+In short, **there was no safe way to change network namespaces, even temporarily, from within a long-lived, multithreaded Go process**. If you wish to do this, you must use go 1.10 or greater. 
+
+
+### Creating network namespaces
+Earlier versions of this library managed namespace creation, but as CNI does not actually utilize this feature (and it was essentialy unmaintained), it was removed. If you're writing a container runtime, you should implement namespace management yourself. However, there are some gotchas when doing so, especially around handling `/var/run/netns`. A reasonably correct reference implementation, borrowed from `rkt`, can be found in `pkg/testutils/netns_linux.go` if you're in need of a source of inspiration.
 
-In short, **there is no safe way to change network namespaces from within a long-lived, multithreaded Go process**. If your daemon process needs to be namespace aware, consider spawning a separate process (like a CNI plugin) for each namespace.
 
 ### Further Reading
  - https://github.com/golang/go/wiki/LockOSThread

pkg/ns/ns_linux.go

@@ -15,10 +15,8 @@
 package ns
 
 import (
- "crypto/rand"
  "fmt"
  "os"
- "path"
  "runtime"
  "sync"
  "syscall"
@@ -38,82 +36,6 @@ func getCurrentThreadNetNSPath() string {
  return fmt.Sprintf("/proc/%d/task/%d/ns/net", os.Getpid(), unix.Gettid())
 }
 
-// Creates a new persistent network namespace and returns an object
-// representing that namespace, without switching to it
-func NewNS() (NetNS, error) {
- const nsRunDir = "/var/run/netns"
-
- b := make([]byte, 16)
- _, err := rand.Reader.Read(b)
- if err != nil {
- return nil, fmt.Errorf("failed to generate random netns name: %v", err)
- }
-
- err = os.MkdirAll(nsRunDir, 0755)
- if err != nil {
- return nil, err
- }
-
- // create an empty file at the mount point
- nsName := fmt.Sprintf("cni-%x-%x-%x-%x-%x", b[0:4], b[4:6], b[6:8], b[8:10], b[10:])
- nsPath := path.Join(nsRunDir, nsName)
- mountPointFd, err := os.Create(nsPath)
- if err != nil {
- return nil, err
- }
- mountPointFd.Close()
-
- // Ensure the mount point is cleaned up on errors; if the namespace
- // was successfully mounted this will have no effect because the file
- // is in-use
- defer os.RemoveAll(nsPath)
-
- var wg sync.WaitGroup
- wg.Add(1)
-
- // do namespace work in a dedicated goroutine, so that we can safely
- // Lock/Unlock OSThread without upsetting the lock/unlock state of
- // the caller of this function
- var fd *os.File
- go (func() {
- defer wg.Done()
- runtime.LockOSThread()
-
- var origNS NetNS
- origNS, err = GetNS(getCurrentThreadNetNSPath())
- if err != nil {
- return
- }
- defer origNS.Close()
-
- // create a new netns on the current thread
- err = unix.Unshare(unix.CLONE_NEWNET)
- if err != nil {
- return
- }
- defer origNS.Set()
-
- // bind mount the new netns from the current thread onto the mount point
- err = unix.Mount(getCurrentThreadNetNSPath(), nsPath, "none", unix.MS_BIND, "")
- if err != nil {
- return
- }
-
- fd, err = os.Open(nsPath)
- if err != nil {
- return
- }
- })()
- wg.Wait()
-
- if err != nil {
- unix.Unmount(nsPath, unix.MNT_DETACH)
- return nil, fmt.Errorf("failed to create namespace: %v", err)
- }
-
- return &netNS{file: fd, mounted: true}, nil
-}
-
 func (ns *netNS) Close() error {
  if err := ns.errorIfClosed(); err != nil {
  return err
@@ -124,16 +46,6 @@ func (ns *netNS) Close() error {
  }
  ns.closed = true
 
- if ns.mounted {
- if err := unix.Unmount(ns.file.Name(), unix.MNT_DETACH); err != nil {
- return fmt.Errorf("Failed to unmount namespace %s: %v", ns.file.Name(), err)
- }
- if err := os.RemoveAll(ns.file.Name()); err != nil {
- return fmt.Errorf("Failed to clean up namespace %s: %v", ns.file.Name(), err)
- }
- ns.mounted = false
- }
-
  return nil
 }
 
@@ -180,9 +92,8 @@ type NetNS interface {
 }
 
 type netNS struct {
- file *os.File
- mounted bool
- closed bool
+ file *os.File
+ closed bool
 }
 
 // netNS implements the NetNS interface

pkg/ns/ns_linux_test.go

@@ -22,6 +22,7 @@ import (
  "path/filepath"
 
  "github.com/containernetworking/plugins/pkg/ns"
+ "github.com/containernetworking/plugins/pkg/testutils"
  . "github.com/onsi/ginkgo"
  . "github.com/onsi/gomega"
  "golang.org/x/sys/unix"
@@ -65,16 +66,19 @@ var _ = Describe("Linux namespace operations", func() {
  BeforeEach(func() {
  var err error
 
- originalNetNS, err = ns.NewNS()
+ originalNetNS, err = testutils.NewNS()
  Expect(err).NotTo(HaveOccurred())
 
- targetNetNS, err = ns.NewNS()
+ targetNetNS, err = testutils.NewNS()
  Expect(err).NotTo(HaveOccurred())
  })
 
  AfterEach(func() {
- Expect(targetNetNS.Close()).To(Succeed())
- Expect(originalNetNS.Close()).To(Succeed())
+ targetNetNS.Close()
+ originalNetNS.Close()
+
+ Expect(testutils.UnmountNS(targetNetNS)).To(Succeed())
+ Expect(testutils.UnmountNS(originalNetNS)).To(Succeed())
  })
 
  It("executes the callback within the target network namespace", func() {
@@ -155,13 +159,14 @@ var _ = Describe("Linux namespace operations", func() {
 
  It("should not leak a closed netns onto any threads in the process", func() {
  By("creating a new netns")
- createdNetNS, err := ns.NewNS()
+ createdNetNS, err := testutils.NewNS()
  Expect(err).NotTo(HaveOccurred())
 
  By("discovering the inode of the created netns")
  createdNetNSInode, err := getInodeNS(createdNetNS)
  Expect(err).NotTo(HaveOccurred())
  createdNetNS.Close()
+ Expect(testutils.UnmountNS(createdNetNS)).NotTo(HaveOccurred())
 
  By("comparing against the netns inode of every thread in the process")
  for _, netnsPath := range allNetNSInCurrentProcess() {
@@ -188,7 +193,8 @@ var _ = Describe("Linux namespace operations", func() {
 
  Describe("closing a network namespace", func() {
  It("should prevent further operations", func() {
- createdNetNS, err := ns.NewNS()
+ createdNetNS, err := testutils.NewNS()
+ defer testutils.UnmountNS(createdNetNS)
  Expect(err).NotTo(HaveOccurred())
 
  err = createdNetNS.Close()
@@ -202,8 +208,9 @@ var _ = Describe("Linux namespace operations", func() {
  })
 
  It("should only work once", func() {
- createdNetNS, err := ns.NewNS()
+ createdNetNS, err := testutils.NewNS()
  Expect(err).NotTo(HaveOccurred())
+ defer testutils.UnmountNS(createdNetNS)
 
  err = createdNetNS.Close()
  Expect(err).NotTo(HaveOccurred())
@@ -216,7 +223,8 @@ var _ = Describe("Linux namespace operations", func() {
 
  Describe("IsNSorErr", func() {
  It("should detect a namespace", func() {
- createdNetNS, err := ns.NewNS()
+ createdNetNS, err := testutils.NewNS()
+ defer testutils.UnmountNS(createdNetNS)
  err = ns.IsNSorErr(createdNetNS.Path())
  Expect(err).NotTo(HaveOccurred())
  })