From 791d259e55b3e5131d0f8acc00143acbb707ea7b Mon Sep 17 00:00:00 2001 From: Gabe Rosenhouse Date: Wed, 13 Jul 2016 22:12:06 -0400 Subject: [PATCH 1/4] pkg/skel: refactor to use dependency injection Extract dependencies on os to enable more complete unit test coverage --- pkg/skel/skel.go | 57 ++++++--- pkg/skel/skel_test.go | 232 +++++++++++++++++++++++++++++------- pkg/testutils/bad_reader.go | 32 +++++ 3 files changed, 263 insertions(+), 58 deletions(-) create mode 100644 pkg/testutils/bad_reader.go diff --git a/pkg/skel/skel.go b/pkg/skel/skel.go index 9cf03917..484ef374 100644 --- a/pkg/skel/skel.go +++ b/pkg/skel/skel.go @@ -18,6 +18,7 @@ package skel import ( "fmt" + "io" "io/ioutil" "log" "os" @@ -36,11 +37,14 @@ type CmdArgs struct { StdinData []byte } +type dispatcher struct { + Getenv func(string) string + Stdin io.Reader +} + 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) { +func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, error) { var cmd, contID, netns, ifName, args, path string vars := []struct { @@ -100,20 +104,21 @@ 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 == "" { log.Printf("%v env variable missing", v.name) + // TODO: test this logging ^^^ and log to stderr instead of stdout 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 +129,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 +153,31 @@ 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{}) { - e := &types.Error{ - Code: 100, - Msg: fmt.Sprintf(f, args...), +// PluginMain is the "main" for a plugin. It accepts +// two callback functions for add and del commands. +func PluginMain(cmdAdd, cmdDel func(_ *CmdArgs) error) { + caller := dispatcher{ + Getenv: os.Getenv, + Stdin: os.Stdin, + } + + err := caller.pluginMain(cmdAdd, cmdDel) + if err != nil { + dieErr(err) } - dieErr(e) } func dieErr(e *types.Error) { diff --git a/pkg/skel/skel_test.go b/pkg/skel/skel_test.go index a52e0145..8a69d556 100644 --- a/pkg/skel/skel_test.go +++ b/pkg/skel/skel_test.go @@ -15,70 +15,216 @@ package skel import ( - "os" + "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 { + CallCount int + Returns struct { + Error error + } + 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 ( - fNoop = func(_ *CmdArgs) error { return nil } - // fErr = func(_ *CmdArgs) error { return errors.New("dummy") } - envVars = []struct { - name string - val string - }{ - {"CNI_CONTAINERID", "dummy"}, - {"CNI_NETNS", "dummy"}, - {"CNI_IFNAME", "dummy"}, - {"CNI_ARGS", "dummy"}, - {"CNI_PATH", "dummy"}, - } + environment map[string]string + stdin io.Reader + cmdAdd, cmdDel *fakeCmd + dispatch *dispatcher + expectedCmdArgs *CmdArgs ) - It("Must be possible to set the env vars", func() { - for _, v := range envVars { - err := os.Setenv(v.name, v.val) - Expect(err).NotTo(HaveOccurred()) + BeforeEach(func() { + environment = map[string]string{ + "CNI_COMMAND": "ADD", + "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" }`) + dispatch = &dispatcher{ + Getenv: func(key string) string { return environment[key] }, + Stdin: stdin, + } + 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 := os.Setenv("CNI_COMMAND", "ADD") + err := dispatch.pluginMain(cmdAdd.Func, cmdDel.Func) + if isRequired { + Expect(err).To(Equal(&types.Error{ + Code: 100, + Msg: "required env variables missing", + })) + } 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("should fail with ADD and error callback", func() { - // err := os.Setenv("CNI_COMMAND", "ADD") - // Expect(err).NotTo(HaveOccurred()) - // PluginMain(fErr, nil) - // }) + It("does not call cmdDel", func() { + err := dispatch.pluginMain(cmdAdd.Func, cmdDel.Func) - 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() - // It("should fail with DEL and error callback", func() { - // err := os.Setenv("CNI_COMMAND", "DEL") - // Expect(err).NotTo(HaveOccurred()) - // PluginMain(fErr, nil) - // }) + DescribeTable("required / optional env vars", envVarChecker, + // TODO: Entry("command", "CNI_COMMAND", true), + Entry("container id", "CNI_CONTAINER_ID", false), + Entry("net ns", "CNI_NETNS", true), + 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() { - err := os.Setenv("CNI_COMMAND", "DEL") - Expect(err).NotTo(HaveOccurred()) - err = os.Unsetenv("CNI_NETNS") - Expect(err).NotTo(HaveOccurred()) - PluginMain(nil, fNoop) + 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, + // TODO: 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", + })) + }) + }) }) }) diff --git a/pkg/testutils/bad_reader.go b/pkg/testutils/bad_reader.go new file mode 100644 index 00000000..b3c0e97d --- /dev/null +++ b/pkg/testutils/bad_reader.go @@ -0,0 +1,32 @@ +// Copyright 2014 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 +} From c17e70075933cea07d6c239fec18d4f30beae5be Mon Sep 17 00:00:00 2001 From: Gabe Rosenhouse Date: Wed, 13 Jul 2016 22:24:34 -0400 Subject: [PATCH 2/4] pkg/skel: missing env var log lines appear in stderr Previously, the log lines appeared in stdout before the JSON encoding of the error message. That would break JSON parsing of stdout. Instead, we use stderr for these unstructured logs, consistent with the CNI spec. --- pkg/skel/skel.go | 5 +++-- pkg/skel/skel_test.go | 22 ++++++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/pkg/skel/skel.go b/pkg/skel/skel.go index 484ef374..bed405c3 100644 --- a/pkg/skel/skel.go +++ b/pkg/skel/skel.go @@ -40,6 +40,7 @@ type CmdArgs struct { type dispatcher struct { Getenv func(string) string Stdin io.Reader + Stderr io.Writer } type reqForCmdEntry map[string]bool @@ -106,8 +107,7 @@ func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, error) { for _, v := range vars { *v.val = t.Getenv(v.name) if v.reqForCmd[cmd] && *v.val == "" { - log.Printf("%v env variable missing", v.name) - // TODO: test this logging ^^^ and log to stderr instead of stdout + fmt.Fprintf(t.Stderr, "%v env variable missing\n", v.name) argsMissing = true } } @@ -172,6 +172,7 @@ func PluginMain(cmdAdd, cmdDel func(_ *CmdArgs) error) { caller := dispatcher{ Getenv: os.Getenv, Stdin: os.Stdin, + Stderr: os.Stderr, } err := caller.pluginMain(cmdAdd, cmdDel) diff --git a/pkg/skel/skel_test.go b/pkg/skel/skel_test.go index 8a69d556..9974c9a5 100644 --- a/pkg/skel/skel_test.go +++ b/pkg/skel/skel_test.go @@ -15,6 +15,7 @@ package skel import ( + "bytes" "errors" "io" "strings" @@ -47,6 +48,7 @@ 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 @@ -62,9 +64,11 @@ var _ = Describe("dispatching to the correct callback", func() { "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{} @@ -87,6 +91,7 @@ var _ = Describe("dispatching to the correct callback", func() { Code: 100, Msg: "required env variables missing", })) + Expect(stderr.String()).To(ContainSubstring(envVar + " env variable missing\n")) } else { Expect(err).NotTo(HaveOccurred()) } @@ -117,6 +122,23 @@ var _ = Describe("dispatching to the correct callback", func() { Entry("args", "CNI_ARGS", false), Entry("path", "CNI_PATH", true), ) + + Context("when multiple required env vars are missing", func() { + BeforeEach(func() { + delete(environment, "CNI_NETNS") + delete(environment, "CNI_IFNAME") + delete(environment, "CNI_PATH") + }) + + 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() { From f4364185253eaa18125611ee987435286bb3cfdf Mon Sep 17 00:00:00 2001 From: Gabe Rosenhouse Date: Wed, 13 Jul 2016 22:54:22 -0400 Subject: [PATCH 3/4] pkg/skel: improve error message for missing CNI_COMMAND env var This makes the error message for missing CNI_COMMAND consistent with that of other required environment variables. --- pkg/skel/skel.go | 8 +++++--- pkg/skel/skel_test.go | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/pkg/skel/skel.go b/pkg/skel/skel.go index bed405c3..4325ec69 100644 --- a/pkg/skel/skel.go +++ b/pkg/skel/skel.go @@ -106,9 +106,11 @@ func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, error) { argsMissing := false for _, v := range vars { *v.val = t.Getenv(v.name) - if v.reqForCmd[cmd] && *v.val == "" { - fmt.Fprintf(t.Stderr, "%v env variable missing\n", v.name) - argsMissing = true + if *v.val == "" { + if v.reqForCmd[cmd] || v.name == "CNI_COMMAND" { + fmt.Fprintf(t.Stderr, "%v env variable missing\n", v.name) + argsMissing = true + } } } diff --git a/pkg/skel/skel_test.go b/pkg/skel/skel_test.go index 9974c9a5..39df2716 100644 --- a/pkg/skel/skel_test.go +++ b/pkg/skel/skel_test.go @@ -115,7 +115,7 @@ var _ = Describe("dispatching to the correct callback", func() { }) DescribeTable("required / optional env vars", envVarChecker, - // TODO: Entry("command", "CNI_COMMAND", true), + Entry("command", "CNI_COMMAND", true), Entry("container id", "CNI_CONTAINER_ID", false), Entry("net ns", "CNI_NETNS", true), Entry("if name", "CNI_IFNAME", true), @@ -162,7 +162,7 @@ var _ = Describe("dispatching to the correct callback", func() { }) DescribeTable("required / optional env vars", envVarChecker, - // TODO: Entry("command", "CNI_COMMAND", true), + Entry("command", "CNI_COMMAND", true), Entry("container id", "CNI_CONTAINER_ID", false), Entry("net ns", "CNI_NETNS", false), Entry("if name", "CNI_IFNAME", true), From a2aff8c6a8c919cb5793ad1738ed7fdbc58350da Mon Sep 17 00:00:00 2001 From: Gabe Rosenhouse Date: Thu, 14 Jul 2016 16:09:27 -0700 Subject: [PATCH 4/4] misc: fix up copyright dates --- pkg/skel/skel.go | 2 +- pkg/testutils/bad_reader.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/skel/skel.go b/pkg/skel/skel.go index 4325ec69..d6f0f4cf 100644 --- a/pkg/skel/skel.go +++ b/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. diff --git a/pkg/testutils/bad_reader.go b/pkg/testutils/bad_reader.go index b3c0e97d..ca06c5e9 100644 --- a/pkg/testutils/bad_reader.go +++ b/pkg/testutils/bad_reader.go @@ -1,4 +1,4 @@ -// Copyright 2014 CNI authors +// 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.