Merge pull request #269 from rosenhouse/skel-dependency-injection

Refactor skel with dependency injection

pkg/skel/skel.go

@@ -1,4 +1,4 @@
-// Copyright 2014 CNI authors
+// Copyright 2014-2016 CNI authors
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -18,6 +18,7 @@ package skel
 
 import (
 	"fmt"
+	"io"
 	"io/ioutil"
 	"log"
 	"os"
@@ -36,11 +37,15 @@ type CmdArgs struct {
 	StdinData   []byte
 }
 
+type dispatcher struct {
+	Getenv func(string) string
+	Stdin  io.Reader
+	Stderr io.Writer
+}
+
 type reqForCmdEntry map[string]bool
 
-// PluginMain is the "main" for a plugin. It accepts
+func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, error) {
-// two callback functions for add and del commands.
-func PluginMain(cmdAdd, cmdDel func(_ *CmdArgs) error) {
 	var cmd, contID, netns, ifName, args, path string
 
 	vars := []struct {
@@ -100,20 +105,22 @@ func PluginMain(cmdAdd, cmdDel func(_ *CmdArgs) error) {
 
 	argsMissing := false
 	for _, v := range vars {
-		*v.val = os.Getenv(v.name)
+		*v.val = t.Getenv(v.name)
-		if v.reqForCmd[cmd] && *v.val == "" {
+		if *v.val == "" {
-			log.Printf("%v env variable missing", v.name)
+			if v.reqForCmd[cmd] || v.name == "CNI_COMMAND" {
+				fmt.Fprintf(t.Stderr, "%v env variable missing\n", v.name)
 				argsMissing = true
 			}
 		}
+	}
 
 	if argsMissing {
-		dieMsg("required env variables missing")
+		return "", nil, fmt.Errorf("required env variables missing")
 	}
 
-	stdinData, err := ioutil.ReadAll(os.Stdin)
+	stdinData, err := ioutil.ReadAll(t.Stdin)
 	if err != nil {
-		dieMsg("error reading from stdin: %v", err)
+		return "", nil, fmt.Errorf("error reading from stdin: %v", err)
 	}
 
 	cmdArgs := &CmdArgs{
@@ -124,6 +131,21 @@ func PluginMain(cmdAdd, cmdDel func(_ *CmdArgs) error) {
 		Path:        path,
 		StdinData:   stdinData,
 	}
+	return cmd, cmdArgs, nil
+}
+
+func createTypedError(f string, args ...interface{}) *types.Error {
+	return &types.Error{
+		Code: 100,
+		Msg:  fmt.Sprintf(f, args...),
+	}
+}
+
+func (t *dispatcher) pluginMain(cmdAdd, cmdDel func(_ *CmdArgs) error) *types.Error {
+	cmd, cmdArgs, err := t.getCmdArgsFromEnv()
+	if err != nil {
+		return createTypedError(err.Error())
+	}
 
 	switch cmd {
 	case "ADD":
@@ -133,24 +155,32 @@ func PluginMain(cmdAdd, cmdDel func(_ *CmdArgs) error) {
 		err = cmdDel(cmdArgs)
 
 	default:
-		dieMsg("unknown CNI_COMMAND: %v", cmd)
+		return createTypedError("unknown CNI_COMMAND: %v", cmd)
 	}
 
 	if err != nil {
 		if e, ok := err.(*types.Error); ok {
 			// don't wrap Error in Error
-			dieErr(e)
+			return e
 		}
-		dieMsg(err.Error())
+		return createTypedError(err.Error())
 	}
+	return nil
 }
 
-func dieMsg(f string, args ...interface{}) {
+// PluginMain is the "main" for a plugin. It accepts
-	e := &types.Error{
+// two callback functions for add and del commands.
-		Code: 100,
+func PluginMain(cmdAdd, cmdDel func(_ *CmdArgs) error) {
-		Msg:  fmt.Sprintf(f, args...),
+	caller := dispatcher{
+		Getenv: os.Getenv,
+		Stdin:  os.Stdin,
+		Stderr: os.Stderr,
+	}
+
+	err := caller.pluginMain(cmdAdd, cmdDel)
+	if err != nil {
+		dieErr(err)
 	}
-	dieErr(e)
 }
 
 func dieErr(e *types.Error) {

pkg/skel/skel_test.go

@@ -15,70 +15,238 @@
 package skel
 
 import (
-	"os"
+	"bytes"
+	"errors"
+	"io"
+	"strings"
 
+	"github.com/containernetworking/cni/pkg/types"
+
+	"github.com/containernetworking/cni/pkg/testutils"
 	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
 )
 
-var _ = Describe("Skel", func() {
+type fakeCmd struct {
-	var (
+	CallCount int
-		fNoop = func(_ *CmdArgs) error { return nil }
+	Returns   struct {
-		// fErr    = func(_ *CmdArgs) error { return errors.New("dummy") }
+		Error error
-		envVars = []struct {
-			name string
-			val  string
-		}{
-			{"CNI_CONTAINERID", "dummy"},
-			{"CNI_NETNS", "dummy"},
-			{"CNI_IFNAME", "dummy"},
-			{"CNI_ARGS", "dummy"},
-			{"CNI_PATH", "dummy"},
 	}
+	Received struct {
+		CmdArgs *CmdArgs
+	}
+}
+
+func (c *fakeCmd) Func(args *CmdArgs) error {
+	c.CallCount++
+	c.Received.CmdArgs = args
+	return c.Returns.Error
+}
+
+var _ = Describe("dispatching to the correct callback", func() {
+	var (
+		environment     map[string]string
+		stdin           io.Reader
+		stderr          *bytes.Buffer
+		cmdAdd, cmdDel  *fakeCmd
+		dispatch        *dispatcher
+		expectedCmdArgs *CmdArgs
 	)
 
-	It("Must be possible to set the env vars", func() {
+	BeforeEach(func() {
-		for _, v := range envVars {
+		environment = map[string]string{
-			err := os.Setenv(v.name, v.val)
+			"CNI_COMMAND":     "ADD",
-			Expect(err).NotTo(HaveOccurred())
+			"CNI_CONTAINERID": "some-container-id",
+			"CNI_NETNS":       "/some/netns/path",
+			"CNI_IFNAME":      "eth0",
+			"CNI_ARGS":        "some;extra;args",
+			"CNI_PATH":        "/some/cni/path",
+		}
+		stdin = strings.NewReader(`{ "some": "config" }`)
+		stderr = &bytes.Buffer{}
+		dispatch = &dispatcher{
+			Getenv: func(key string) string { return environment[key] },
+			Stdin:  stdin,
+			Stderr: stderr,
+		}
+		cmdAdd = &fakeCmd{}
+		cmdDel = &fakeCmd{}
+		expectedCmdArgs = &CmdArgs{
+			ContainerID: "some-container-id",
+			Netns:       "/some/netns/path",
+			IfName:      "eth0",
+			Args:        "some;extra;args",
+			Path:        "/some/cni/path",
+			StdinData:   []byte(`{ "some": "config" }`),
 		}
 	})
 
-	Context("When dummy environment variables are passed", func() {
+	var envVarChecker = func(envVar string, isRequired bool) {
+		delete(environment, envVar)
 
-		It("should not fail with ADD and noop callback", func() {
+		err := dispatch.pluginMain(cmdAdd.Func, cmdDel.Func)
-			err := os.Setenv("CNI_COMMAND", "ADD")
+		if isRequired {
+			Expect(err).To(Equal(&types.Error{
+				Code: 100,
+				Msg:  "required env variables missing",
+			}))
+			Expect(stderr.String()).To(ContainSubstring(envVar + " env variable missing\n"))
+		} else {
 			Expect(err).NotTo(HaveOccurred())
-			PluginMain(fNoop, nil)
+		}
+	}
+
+	Context("when the CNI_COMMAND is ADD", func() {
+		It("extracts env vars and stdin data and calls cmdAdd", func() {
+			err := dispatch.pluginMain(cmdAdd.Func, cmdDel.Func)
+
+			Expect(err).NotTo(HaveOccurred())
+			Expect(cmdAdd.CallCount).To(Equal(1))
+			Expect(cmdDel.CallCount).To(Equal(0))
+			Expect(cmdAdd.Received.CmdArgs).To(Equal(expectedCmdArgs))
 		})
 
-		// TODO: figure out howto mock printing and os.Exit()
+		It("does not call cmdDel", func() {
-		// It("should fail with ADD and error callback", func() {
+			err := dispatch.pluginMain(cmdAdd.Func, cmdDel.Func)
-		// 	err := os.Setenv("CNI_COMMAND", "ADD")
-		// 	Expect(err).NotTo(HaveOccurred())
-		// 	PluginMain(fErr, nil)
-		// })
 
-		It("should not fail with DEL and noop callback", func() {
-			err := os.Setenv("CNI_COMMAND", "DEL")
 			Expect(err).NotTo(HaveOccurred())
-			PluginMain(nil, fNoop)
+			Expect(cmdDel.CallCount).To(Equal(0))
 		})
 
-		// TODO: figure out howto mock printing and os.Exit()
+		DescribeTable("required / optional env vars", envVarChecker,
-		// It("should fail with DEL and error callback", func() {
+			Entry("command", "CNI_COMMAND", true),
-		// 	err := os.Setenv("CNI_COMMAND", "DEL")
+			Entry("container id", "CNI_CONTAINER_ID", false),
-		// 	Expect(err).NotTo(HaveOccurred())
+			Entry("net ns", "CNI_NETNS", true),
-		// 	PluginMain(fErr, nil)
+			Entry("if name", "CNI_IFNAME", true),
-		// })
+			Entry("args", "CNI_ARGS", false),
+			Entry("path", "CNI_PATH", true),
+		)
 
-		It("should not fail with DEL and no NETNS and noop callback", func() {
+		Context("when multiple required env vars are missing", func() {
-			err := os.Setenv("CNI_COMMAND", "DEL")
+			BeforeEach(func() {
-			Expect(err).NotTo(HaveOccurred())
+				delete(environment, "CNI_NETNS")
-			err = os.Unsetenv("CNI_NETNS")
+				delete(environment, "CNI_IFNAME")
-			Expect(err).NotTo(HaveOccurred())
+				delete(environment, "CNI_PATH")
-			PluginMain(nil, fNoop)
 			})
 
+			It("reports that all of them are missing, not just the first", func() {
+				Expect(dispatch.pluginMain(cmdAdd.Func, cmdDel.Func)).NotTo(Succeed())
+				log := stderr.String()
+				Expect(log).To(ContainSubstring("CNI_NETNS env variable missing\n"))
+				Expect(log).To(ContainSubstring("CNI_IFNAME env variable missing\n"))
+				Expect(log).To(ContainSubstring("CNI_PATH env variable missing\n"))
+
+			})
+		})
+	})
+
+	Context("when the CNI_COMMAND is DEL", func() {
+		BeforeEach(func() {
+			environment["CNI_COMMAND"] = "DEL"
+		})
+
+		It("calls cmdDel with the env vars and stdin data", func() {
+			err := dispatch.pluginMain(cmdAdd.Func, cmdDel.Func)
+
+			Expect(err).NotTo(HaveOccurred())
+			Expect(cmdDel.CallCount).To(Equal(1))
+			Expect(cmdDel.Received.CmdArgs).To(Equal(expectedCmdArgs))
+		})
+
+		It("does not call cmdAdd", func() {
+			err := dispatch.pluginMain(cmdAdd.Func, cmdDel.Func)
+
+			Expect(err).NotTo(HaveOccurred())
+			Expect(cmdAdd.CallCount).To(Equal(0))
+		})
+
+		DescribeTable("required / optional env vars", envVarChecker,
+			Entry("command", "CNI_COMMAND", true),
+			Entry("container id", "CNI_CONTAINER_ID", false),
+			Entry("net ns", "CNI_NETNS", false),
+			Entry("if name", "CNI_IFNAME", true),
+			Entry("args", "CNI_ARGS", false),
+			Entry("path", "CNI_PATH", true),
+		)
+	})
+
+	Context("when the CNI_COMMAND is unrecognized", func() {
+		BeforeEach(func() {
+			environment["CNI_COMMAND"] = "NOPE"
+		})
+
+		It("does not call any cmd callback", func() {
+			dispatch.pluginMain(cmdAdd.Func, cmdDel.Func)
+
+			Expect(cmdAdd.CallCount).To(Equal(0))
+			Expect(cmdDel.CallCount).To(Equal(0))
+		})
+
+		It("returns an error", func() {
+			err := dispatch.pluginMain(cmdAdd.Func, cmdDel.Func)
+
+			Expect(err).To(Equal(&types.Error{
+				Code: 100,
+				Msg:  "unknown CNI_COMMAND: NOPE",
+			}))
+		})
+	})
+
+	Context("when stdin cannot be read", func() {
+		BeforeEach(func() {
+			dispatch.Stdin = &testutils.BadReader{}
+		})
+
+		It("does not call any cmd callback", func() {
+			dispatch.pluginMain(cmdAdd.Func, cmdDel.Func)
+
+			Expect(cmdAdd.CallCount).To(Equal(0))
+			Expect(cmdDel.CallCount).To(Equal(0))
+		})
+
+		It("wraps and returns the error", func() {
+			err := dispatch.pluginMain(cmdAdd.Func, cmdDel.Func)
+
+			Expect(err).To(Equal(&types.Error{
+				Code: 100,
+				Msg:  "error reading from stdin: banana",
+			}))
+		})
+	})
+
+	Context("when the callback returns an error", func() {
+		Context("when it is a typed Error", func() {
+			BeforeEach(func() {
+				cmdAdd.Returns.Error = &types.Error{
+					Code: 1234,
+					Msg:  "insufficient something",
+				}
+			})
+
+			It("returns the error as-is", func() {
+				err := dispatch.pluginMain(cmdAdd.Func, cmdDel.Func)
+
+				Expect(err).To(Equal(&types.Error{
+					Code: 1234,
+					Msg:  "insufficient something",
+				}))
+			})
+		})
+
+		Context("when it is an unknown error", func() {
+			BeforeEach(func() {
+				cmdAdd.Returns.Error = errors.New("potato")
+			})
+
+			It("wraps and returns the error", func() {
+				err := dispatch.pluginMain(cmdAdd.Func, cmdDel.Func)
+
+				Expect(err).To(Equal(&types.Error{
+					Code: 100,
+					Msg:  "potato",
+				}))
+			})
+		})
 	})
 })

pkg/testutils/bad_reader.go

@@ -0,0 +1,32 @@
+// Copyright 2016 CNI authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package testutils
+
+import "errors"
+
+type BadReader struct {
+	Error error
+}
+
+func (r *BadReader) Read(buffer []byte) (int, error) {
+	if r.Error != nil {
+		return 0, r.Error
+	}
+	return 0, errors.New("banana")
+}
+
+func (r *BadReader) Close() error {
+	return nil
+}