Merge pull request #239 from steveeJ/master

MAINTAINERS: remove @zachgersh, add @dcbw

CONTRIBUTING.md

@@ -1,11 +1,11 @@
 # How to Contribute
 
-cni is [Apache 2.0 licensed](LICENSE) and accepts contributions via GitHub
+CNI is [Apache 2.0 licensed](LICENSE) and accepts contributions via GitHub
 pull requests. This document outlines some of the conventions on development
 workflow, commit message formatting, contact points and other resources to make
 it easier to get your contribution accepted.
 
-We welcome improvements to documentation as well as to code.
+We gratefully welcome improvements to documentation as well as to code.
 
 # Certificate of Origin
 
@@ -41,7 +41,19 @@ This is a rough outline of how to prepare a contribution:
 - Make sure any new code files have a license header.
 - Submit a pull request to the original repository.
 
-Thanks for your contributions!
+# Acceptance policy
+
+These things will make a PR more likely to be accepted:
+
+ * a well-described requirement
+ * tests for new code
+ * tests for old code!
+ * new code follows the conventions in old code
+ * a good commit message (see below)
+
+In general, we will merge a PR once two maintainers have endorsed it.
+Trivial changes (e.g., corrections to spelling) may get waved through.
+For substantial changes, more people may become involved, and you might get asked to resubmit the PR or divide the changes into more than one PR.
 
 ### Format of the Commit Message
 

MAINTAINERS

@@ -1,5 +1,5 @@
+Dan Williams <dcbw@redhat.com> (@dcbw)
 Gabe Rosenhouse <grosenhouse@pivotal.io> (@rosenhouse)
 Michael Bridgen <michael@weave.works> (@squaremo)
 Stefan Junker <stefan.junker@coreos.com> (@steveeJ)
 Tom Denham <tom.denham@metaswitch.com> (@tomdee)
-Zach Gershman <zachgersh@gmail.com> (@zachgersh)

README.md

@@ -30,6 +30,7 @@ To avoid duplication, we think it is prudent to define a common interface betwee
 - [Weave - a multi-host Docker network](https://github.com/weaveworks/weave)
 - [Project Calico - a layer 3 virtual network](https://github.com/projectcalico/calico-cni)
 - [Contiv Networking - policy networking for various use cases](https://github.com/contiv/netplugin)
+- [Mesos - a distributed systems kernel](https://github.com/apache/mesos/blob/master/docs/cni.md)
 
 ## Contributing to CNI
 

ROADMAP.md

@@ -0,0 +1,33 @@
+# CNI Roadmap
+
+This document defines a high level roadmap for CNI development.
+The list below is not complete, and we advise to get the current project state from the [milestones defined in GitHub](https://github.com/containernetworking/cni/milestones).
+
+## CNI Milestones
+
+### [v0.2.0](https://github.com/containernetworking/cni/milestones/v0.2.0)
+
+* Signed release binaries
+* Introduction of a testing strategy/framework
+
+### [v0.3.0](https://github.com/containernetworking/cni/milestones/v0.3.0)
+
+* Further increase test coverage
+* Simpler default route handling in bridge plugin
+* Clarify project description, documentation and contribution guidelines
+
+### [v0.4.0](https://github.com/containernetworking/cni/milestones/v0.4.0)
+
+* Further increase test coverage
+* Simpler bridging of host interface
+* Improve IPAM allocator predictability
+* Allow in- and output of arbitrary K/V pairs for plugins
+
+### [v1.0.0](https://github.com/containernetworking/cni/milestones/v1.0.0)
+
+- Plugin composition functionality
+- IPv6 support
+- Stable SPEC
+- Strategy and tooling for backwards compatibility
+- Complete test coverage
+- Integrate build artefact generation with CI

pkg/ns/ns.go

@@ -20,7 +20,9 @@ import (
 	"os"
 	"path"
 	"runtime"
+	"strings"
 	"sync"
+	"syscall"
 
 	"golang.org/x/sys/unix"
 )
@@ -58,6 +60,7 @@ type NetNS interface {
 type netNS struct {
 	file    *os.File
 	mounted bool
+	closed  bool
 }
 
 func getCurrentThreadNetNSPath() string {
@@ -72,12 +75,63 @@ func GetCurrentNS() (NetNS, error) {
 	return GetNS(getCurrentThreadNetNSPath())
 }
 
+const (
+	// https://github.com/torvalds/linux/blob/master/include/uapi/linux/magic.h
+	NSFS_MAGIC   = 0x6e736673
+	PROCFS_MAGIC = 0x9fa0
+)
+
+type NSPathNotExistErr struct{ msg string }
+
+func (e NSPathNotExistErr) Error() string { return e.msg }
+
+type NSPathNotNSErr struct{ msg string }
+
+func (e NSPathNotNSErr) Error() string { return e.msg }
+
+func IsNSorErr(nspath string) error {
+	stat := syscall.Statfs_t{}
+	if err := syscall.Statfs(nspath, &stat); err != nil {
+		if os.IsNotExist(err) {
+			err = NSPathNotExistErr{msg: fmt.Sprintf("failed to Statfs %q: %v", nspath, err)}
+		} else {
+			err = fmt.Errorf("failed to Statfs %q: %v", nspath, err)
+		}
+		return err
+	}
+
+	switch stat.Type {
+	case PROCFS_MAGIC:
+		// Kernel < 3.19
+
+		validPathContent := "ns/"
+		validName := strings.Contains(nspath, validPathContent)
+		if !validName {
+			return NSPathNotNSErr{msg: fmt.Sprintf("path %q doesn't contain %q", nspath, validPathContent)}
+		}
+
+		return nil
+	case NSFS_MAGIC:
+		// Kernel >= 3.19
+
+		return nil
+	default:
+		return NSPathNotNSErr{msg: fmt.Sprintf("unknown FS magic on %q: %x", nspath, stat.Type)}
+	}
+}
+
 // Returns an object representing the namespace referred to by @path
 func GetNS(nspath string) (NetNS, error) {
+	err := IsNSorErr(nspath)
+	if err != nil {
+		return nil, err
+	}
+
 	fd, err := os.Open(nspath)
 	if err != nil {
 		return nil, err
 	}
+
 	return &netNS{file: fd}, nil
 }
 
@@ -165,8 +219,22 @@ func (ns *netNS) Fd() uintptr {
 	return ns.file.Fd()
 }
 
+func (ns *netNS) errorIfClosed() error {
+	if ns.closed {
+		return fmt.Errorf("%q has already been closed", ns.file.Name())
+	}
+	return nil
+}
+
 func (ns *netNS) Close() error {
-	ns.file.Close()
+	if err := ns.errorIfClosed(); err != nil {
+		return err
+	}
+
+	if err := ns.file.Close(); err != nil {
+		return fmt.Errorf("Failed to close %q: %v", ns.file.Name(), err)
+	}
+	ns.closed = true
 
 	if ns.mounted {
 		if err := unix.Unmount(ns.file.Name(), unix.MNT_DETACH); err != nil {
@@ -175,11 +243,17 @@ func (ns *netNS) Close() error {
 		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
 }
 
 func (ns *netNS) Do(toRun func(NetNS) error) error {
+	if err := ns.errorIfClosed(); err != nil {
+		return err
+	}
+
 	containedCall := func(hostNS NetNS) error {
 		threadNS, err := GetNS(getCurrentThreadNetNSPath())
 		if err != nil {
@@ -218,6 +292,10 @@ func (ns *netNS) Do(toRun func(NetNS) error) error {
 }
 
 func (ns *netNS) Set() error {
+	if err := ns.errorIfClosed(); err != nil {
+		return err
+	}
+
 	if _, _, err := unix.Syscall(unix.SYS_SETNS, ns.Fd(), uintptr(unix.CLONE_NEWNET), 0); err != 0 {
 		return fmt.Errorf("Error switching to ns %v: %v", ns.file.Name(), err)
 	}
@@ -230,7 +308,7 @@ func (ns *netNS) Set() error {
 func WithNetNSPath(nspath string, toRun func(NetNS) error) error {
 	ns, err := GetNS(nspath)
 	if err != nil {
-		return fmt.Errorf("Failed to open %v: %v", nspath, err)
+		return err
 	}
 	defer ns.Close()
 	return ns.Do(toRun)

pkg/ns/ns_test.go

@@ -17,6 +17,7 @@ package ns_test
 import (
 	"errors"
 	"fmt"
+	"io/ioutil"
 	"os"
 	"path/filepath"
 
@@ -169,6 +170,76 @@ var _ = Describe("Linux namespace operations", func() {
 					Expect(netnsInode).NotTo(Equal(createdNetNSInode))
 				}
 			})
+
+			It("fails when the path is not a namespace", func() {
+				tempFile, err := ioutil.TempFile("", "nstest")
+				Expect(err).NotTo(HaveOccurred())
+				defer tempFile.Close()
+
+				nspath := tempFile.Name()
+				defer os.Remove(nspath)
+
+				_, err = ns.GetNS(nspath)
+				Expect(err).To(HaveOccurred())
+				Expect(err).To(BeAssignableToTypeOf(ns.NSPathNotNSErr{}))
+				Expect(err).NotTo(BeAssignableToTypeOf(ns.NSPathNotExistErr{}))
+			})
+		})
+
+		Describe("closing a network namespace", func() {
+			It("should prevent further operations", func() {
+				createdNetNS, err := ns.NewNS()
+				Expect(err).NotTo(HaveOccurred())
+
+				err = createdNetNS.Close()
+				Expect(err).NotTo(HaveOccurred())
+
+				err = createdNetNS.Do(func(ns.NetNS) error { return nil })
+				Expect(err).To(HaveOccurred())
+
+				err = createdNetNS.Set()
+				Expect(err).To(HaveOccurred())
+			})
+
+			It("should only work once", func() {
+				createdNetNS, err := ns.NewNS()
+				Expect(err).NotTo(HaveOccurred())
+
+				err = createdNetNS.Close()
+				Expect(err).NotTo(HaveOccurred())
+
+				err = createdNetNS.Close()
+				Expect(err).To(HaveOccurred())
+			})
+		})
+	})
+
+	Describe("IsNSorErr", func() {
+		It("should detect a namespace", func() {
+			createdNetNS, err := ns.NewNS()
+			err = ns.IsNSorErr(createdNetNS.Path())
+			Expect(err).NotTo(HaveOccurred())
+		})
+
+		It("should refuse other paths", func() {
+			tempFile, err := ioutil.TempFile("", "nstest")
+			Expect(err).NotTo(HaveOccurred())
+			defer tempFile.Close()
+
+			nspath := tempFile.Name()
+			defer os.Remove(nspath)
+
+			err = ns.IsNSorErr(nspath)
+			Expect(err).To(HaveOccurred())
+			Expect(err).To(BeAssignableToTypeOf(ns.NSPathNotNSErr{}))
+			Expect(err).NotTo(BeAssignableToTypeOf(ns.NSPathNotExistErr{}))
+		})
+
+		It("should error on non-existing paths", func() {
+			err := ns.IsNSorErr("/tmp/IDoNotExist")
+			Expect(err).To(HaveOccurred())
+			Expect(err).To(BeAssignableToTypeOf(ns.NSPathNotExistErr{}))
+			Expect(err).NotTo(BeAssignableToTypeOf(ns.NSPathNotNSErr{}))
 		})
 	})
 })

pkg/skel/skel.go

@@ -36,6 +36,8 @@ type CmdArgs struct {
 	StdinData   []byte
 }
 
+type reqForCmdEntry map[string]bool
+
 // PluginMain is the "main" for a plugin. It accepts
 // two callback functions for add and del commands.
 func PluginMain(cmdAdd, cmdDel func(_ *CmdArgs) error) {
@@ -44,20 +46,62 @@ func PluginMain(cmdAdd, cmdDel func(_ *CmdArgs) error) {
 	vars := []struct {
 		name      string
 		val       *string
-		req  bool
+		reqForCmd reqForCmdEntry
 	}{
-		{"CNI_COMMAND", &cmd, true},
-		{"CNI_CONTAINERID", &contID, false},
-		{"CNI_NETNS", &netns, true},
-		{"CNI_IFNAME", &ifName, true},
-		{"CNI_ARGS", &args, false},
-		{"CNI_PATH", &path, true},
+		{
+			"CNI_COMMAND",
+			&cmd,
+			reqForCmdEntry{
+				"ADD": true,
+				"DEL": true,
+			},
+		},
+		{
+			"CNI_CONTAINERID",
+			&contID,
+			reqForCmdEntry{
+				"ADD": false,
+				"DEL": false,
+			},
+		},
+		{
+			"CNI_NETNS",
+			&netns,
+			reqForCmdEntry{
+				"ADD": true,
+				"DEL": false,
+			},
+		},
+		{
+			"CNI_IFNAME",
+			&ifName,
+			reqForCmdEntry{
+				"ADD": true,
+				"DEL": true,
+			},
+		},
+		{
+			"CNI_ARGS",
+			&args,
+			reqForCmdEntry{
+				"ADD": false,
+				"DEL": false,
+			},
+		},
+		{
+			"CNI_PATH",
+			&path,
+			reqForCmdEntry{
+				"ADD": true,
+				"DEL": true,
+			},
+		},
 	}
 
 	argsMissing := false
 	for _, v := range vars {
 		*v.val = os.Getenv(v.name)
-		if v.req && *v.val == "" {
+		if v.reqForCmd[cmd] && *v.val == "" {
 			log.Printf("%v env variable missing", v.name)
 			argsMissing = true
 		}

pkg/skel/skel_test.go

@@ -71,5 +71,14 @@ var _ = Describe("Skel", func() {
 		// 	Expect(err).NotTo(HaveOccurred())
 		// 	PluginMain(fErr, nil)
 		// })
+
+		It("should not fail with DEL and no NETNS and noop callback", func() {
+			err := os.Setenv("CNI_COMMAND", "DEL")
+			Expect(err).NotTo(HaveOccurred())
+			err = os.Unsetenv("CNI_NETNS")
+			Expect(err).NotTo(HaveOccurred())
+			PluginMain(nil, fNoop)
+		})
+
 	})
 })

plugins/main/bridge/bridge.go

@@ -289,6 +289,10 @@ func cmdDel(args *skel.CmdArgs) error {
 		return err
 	}
 
+	if args.Netns == "" {
+		return nil
+	}
+
 	var ipn *net.IPNet
 	err = ns.WithNetNSPath(args.Netns, func(_ ns.NetNS) error {
 		var err error

plugins/main/ipvlan/ipvlan.go

@@ -152,6 +152,10 @@ func cmdDel(args *skel.CmdArgs) error {
 		return err
 	}
 
+	if args.Netns == "" {
+		return nil
+	}
+
 	return ns.WithNetNSPath(args.Netns, func(_ ns.NetNS) error {
 		return ip.DelLinkByName(args.IfName)
 	})

plugins/main/macvlan/macvlan.go

@@ -170,6 +170,10 @@ func cmdDel(args *skel.CmdArgs) error {
 		return err
 	}
 
+	if args.Netns == "" {
+		return nil
+	}
+
 	return ns.WithNetNSPath(args.Netns, func(_ ns.NetNS) error {
 		return ip.DelLinkByName(args.IfName)
 	})

plugins/main/ptp/ptp.go

@@ -199,6 +199,10 @@ func cmdDel(args *skel.CmdArgs) error {
 		return err
 	}
 
+	if args.Netns == "" {
+		return nil
+	}
+
 	var ipn *net.IPNet
 	err := ns.WithNetNSPath(args.Netns, func(_ ns.NetNS) error {
 		var err error