Merge pull request #686 from mars1024/cleanup/skel_error

skel: clean up errors in skel and add some well-known error codes

SPEC.md

@@ -758,4 +758,8 @@ Error codes 1-99 must not be used other than as specified here.
 - `1` - Incompatible CNI version
 - `2` - Unsupported field in network configuration. The error message must contain the key and value of the unsupported field.
 - `3` - Container unknown or does not exist. This error implies the runtime does not need to perform any container network cleanup (for example, calling the `DEL` action on the container).
+- `4` - Invalid necessary environment variables, like CNI_COMMAND, CNI_CONTAINERID, etc. The error message must contain the names of invalid variables.
+- `5` - I/O failure. For example, failed to read network config bytes from stdin.
+- `6` - Failed to decode content. For example, failed to unmarshal network config from bytes or failed to decode version info from string.
+- `7` - Invalid network config. If some validations on network configs do not pass, this error will be raised.
 - `11` - Try again later. If the plugin detects some transient condition that should clear up, it can use this code to notify the runtime it should re-try the operation later.

pkg/skel/skel.go

@@ -62,7 +62,7 @@ func (e missingEnvError) Error() string {
 	return e.msg
 }
 
-func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, error) {
+func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, *types.Error) {
 	var cmd, contID, netns, ifName, args, path string
 
 	vars := []struct {
@@ -138,7 +138,7 @@ func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, error) {
 
 	if len(argsMissing) > 0 {
 		joined := strings.Join(argsMissing, ",")
-		return "", nil, missingEnvError{fmt.Sprintf("required env variables [%s] missing", joined)}
+		return "", nil, types.NewError(types.ErrInvalidEnvironmentVariables, fmt.Sprintf("required env variables [%s] missing", joined), "")
 	}
 
 	if cmd == "VERSION" {
@@ -147,7 +147,7 @@ func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, error) {
 
 	stdinData, err := ioutil.ReadAll(t.Stdin)
 	if err != nil {
-		return "", nil, fmt.Errorf("error reading from stdin: %v", err)
+		return "", nil, types.NewError(types.ErrIOFailure, fmt.Sprintf("error reading from stdin: %v", err), "")
 	}
 
 	cmdArgs := &CmdArgs{
@@ -168,32 +168,36 @@ func createTypedError(f string, args ...interface{}) *types.Error {
 	}
 }
 
-func (t *dispatcher) checkVersionAndCall(cmdArgs *CmdArgs, pluginVersionInfo version.PluginInfo, toCall func(*CmdArgs) error) error {
+func (t *dispatcher) checkVersionAndCall(cmdArgs *CmdArgs, pluginVersionInfo version.PluginInfo, toCall func(*CmdArgs) error) *types.Error {
 	configVersion, err := t.ConfVersionDecoder.Decode(cmdArgs.StdinData)
 	if err != nil {
-		return err
+		return types.NewError(types.ErrDecodingFailure, err.Error(), "")
 	}
 	verErr := t.VersionReconciler.Check(configVersion, pluginVersionInfo)
 	if verErr != nil {
-		return &types.Error{
-			Code:    types.ErrIncompatibleCNIVersion,
-			Msg:     "incompatible CNI versions",
-			Details: verErr.Details(),
+		return types.NewError(types.ErrIncompatibleCNIVersion, "incompatible CNI versions", verErr.Details())
+	}
+
+	if err = toCall(cmdArgs); err != nil {
+		if e, ok := err.(*types.Error); ok {
+			// don't wrap Error in Error
+			return e
 		}
+		return types.NewError(types.ErrInternal, err.Error(), "")
 	}
 
-	return toCall(cmdArgs)
+	return nil
 }
 
-func validateConfig(jsonBytes []byte) error {
+func validateConfig(jsonBytes []byte) *types.Error {
 	var conf struct {
 		Name string `json:"name"`
 	}
 	if err := json.Unmarshal(jsonBytes, &conf); err != nil {
-		return fmt.Errorf("error reading network config: %s", err)
+		return types.NewError(types.ErrDecodingFailure, fmt.Sprintf("error unmarshall network config: %v", err), "")
 	}
 	if conf.Name == "" {
-		return fmt.Errorf("missing network name")
+		return types.NewError(types.ErrInvalidNetworkConfig, "missing network name", "")
 	}
 	return nil
 }
@@ -202,17 +206,17 @@ func (t *dispatcher) pluginMain(cmdAdd, cmdCheck, cmdDel func(_ *CmdArgs) error,
 	cmd, cmdArgs, err := t.getCmdArgsFromEnv()
 	if err != nil {
 		// Print the about string to stderr when no command is set
-		if _, ok := err.(missingEnvError); ok && t.Getenv("CNI_COMMAND") == "" && about != "" {
-			fmt.Fprintln(t.Stderr, about)
+		if err.Code == types.ErrInvalidEnvironmentVariables && t.Getenv("CNI_COMMAND") == "" && about != "" {
+			_, _ = fmt.Fprintln(t.Stderr, about)
 			return nil
 		}
-		return createTypedError(err.Error())
+		return err
 	}
 
 	if cmd != "VERSION" {
 		err = validateConfig(cmdArgs.StdinData)
 		if err != nil {
-			return createTypedError(err.Error())
+			return err
 		}
 	}
 
@@ -222,45 +226,37 @@ func (t *dispatcher) pluginMain(cmdAdd, cmdCheck, cmdDel func(_ *CmdArgs) error,
 	case "CHECK":
 		configVersion, err := t.ConfVersionDecoder.Decode(cmdArgs.StdinData)
 		if err != nil {
-			return createTypedError(err.Error())
+			return types.NewError(types.ErrDecodingFailure, err.Error(), "")
 		}
 		if gtet, err := version.GreaterThanOrEqualTo(configVersion, "0.4.0"); err != nil {
-			return createTypedError(err.Error())
+			return types.NewError(types.ErrDecodingFailure, err.Error(), "")
 		} else if !gtet {
-			return &types.Error{
-				Code: types.ErrIncompatibleCNIVersion,
-				Msg:  "config version does not allow CHECK",
-			}
+			return types.NewError(types.ErrIncompatibleCNIVersion, "config version does not allow CHECK", "")
 		}
 		for _, pluginVersion := range versionInfo.SupportedVersions() {
 			gtet, err := version.GreaterThanOrEqualTo(pluginVersion, configVersion)
 			if err != nil {
-				return createTypedError(err.Error())
+				return types.NewError(types.ErrDecodingFailure, err.Error(), "")
 			} else if gtet {
 				if err := t.checkVersionAndCall(cmdArgs, versionInfo, cmdCheck); err != nil {
-					return createTypedError(err.Error())
+					return err
 				}
 				return nil
 			}
 		}
-		return &types.Error{
-			Code: types.ErrIncompatibleCNIVersion,
-			Msg:  "plugin version does not allow CHECK",
-		}
+		return types.NewError(types.ErrIncompatibleCNIVersion, "plugin version does not allow CHECK", "")
 	case "DEL":
 		err = t.checkVersionAndCall(cmdArgs, versionInfo, cmdDel)
 	case "VERSION":
-		err = versionInfo.Encode(t.Stdout)
+		if err := versionInfo.Encode(t.Stdout); err != nil {
+			return types.NewError(types.ErrIOFailure, err.Error(), "")
+		}
 	default:
-		return createTypedError("unknown CNI_COMMAND: %v", cmd)
+		return types.NewError(types.ErrInvalidEnvironmentVariables, fmt.Sprintf("unknown CNI_COMMAND: %v", cmd), "")
 	}
 
 	if err != nil {
-		if e, ok := err.(*types.Error); ok {
-			// don't wrap Error in Error
-			return e
-		}
-		return createTypedError(err.Error())
+		return err
 	}
 	return nil
 }

pkg/skel/skel_test.go

@@ -95,7 +95,7 @@ var _ = Describe("dispatching to the correct callback", func() {
 		err := dispatch.pluginMain(cmdAdd.Func, cmdCheck.Func, cmdDel.Func, versionInfo, "")
 		if isRequired {
 			Expect(err).To(Equal(&types.Error{
-				Code: 100,
+				Code: types.ErrInvalidEnvironmentVariables,
 				Msg:  "required env variables [" + envVar + "] missing",
 			}))
 		} else {
@@ -143,7 +143,7 @@ var _ = Describe("dispatching to the correct callback", func() {
 				Expect(err).To(HaveOccurred())
 
 				Expect(err).To(Equal(&types.Error{
-					Code: 100,
+					Code: types.ErrInvalidEnvironmentVariables,
 					Msg:  "required env variables [CNI_NETNS,CNI_IFNAME,CNI_PATH] missing",
 				}))
 			})
@@ -236,7 +236,7 @@ var _ = Describe("dispatching to the correct callback", func() {
 				Expect(err).To(HaveOccurred())
 
 				Expect(err).To(Equal(&types.Error{
-					Code: 100,
+					Code: types.ErrInvalidEnvironmentVariables,
 					Msg:  "required env variables [CNI_NETNS,CNI_IFNAME,CNI_PATH] missing",
 				}))
 			})
@@ -269,10 +269,10 @@ var _ = Describe("dispatching to the correct callback", func() {
 
 		Context("when the config has a bad version", func() {
 			It("immediately returns a useful error", func() {
-				dispatch.Stdin = strings.NewReader(`{ "cniVersion": "adsfsadf", "some": "config" }`)
+				dispatch.Stdin = strings.NewReader(`{ "cniVersion": "adsfsadf", "some": "config", "name": "test" }`)
 				versionInfo = version.PluginSupports("0.1.0", "0.2.0", "0.3.0")
 				err := dispatch.pluginMain(cmdAdd.Func, cmdCheck.Func, cmdDel.Func, versionInfo, "")
-				Expect(err.Code).To(Equal(uint(100)))
+				Expect(err.Code).To(Equal(uint(types.ErrDecodingFailure)))
 				Expect(cmdAdd.CallCount).To(Equal(0))
 				Expect(cmdCheck.CallCount).To(Equal(0))
 				Expect(cmdDel.CallCount).To(Equal(0))
@@ -281,10 +281,10 @@ var _ = Describe("dispatching to the correct callback", func() {
 
 		Context("when the plugin has a bad version", func() {
 			It("immediately returns a useful error", func() {
-				dispatch.Stdin = strings.NewReader(`{ "cniVersion": "0.4.0", "some": "config" }`)
+				dispatch.Stdin = strings.NewReader(`{ "cniVersion": "0.4.0", "some": "config", "name": "test" }`)
 				versionInfo = version.PluginSupports("0.1.0", "0.2.0", "adsfasdf")
 				err := dispatch.pluginMain(cmdAdd.Func, cmdCheck.Func, cmdDel.Func, versionInfo, "")
-				Expect(err.Code).To(Equal(uint(100)))
+				Expect(err.Code).To(Equal(uint(types.ErrDecodingFailure)))
 				Expect(cmdAdd.CallCount).To(Equal(0))
 				Expect(cmdCheck.CallCount).To(Equal(0))
 				Expect(cmdDel.CallCount).To(Equal(0))
@@ -385,7 +385,7 @@ var _ = Describe("dispatching to the correct callback", func() {
 			err := dispatch.pluginMain(cmdAdd.Func, cmdCheck.Func, cmdDel.Func, versionInfo, "")
 
 			Expect(err).To(Equal(&types.Error{
-				Code: 100,
+				Code: types.ErrInvalidEnvironmentVariables,
 				Msg:  "unknown CNI_COMMAND: NOPE",
 			}))
 		})
@@ -417,7 +417,7 @@ var _ = Describe("dispatching to the correct callback", func() {
 			Expect(cmdAdd.CallCount).To(Equal(0))
 			Expect(cmdDel.CallCount).To(Equal(0))
 			Expect(err).To(Equal(&types.Error{
-				Code: 100,
+				Code: types.ErrInvalidEnvironmentVariables,
 				Msg:  "required env variables [CNI_COMMAND] missing",
 			}))
 		})
@@ -439,7 +439,7 @@ var _ = Describe("dispatching to the correct callback", func() {
 			err := dispatch.pluginMain(cmdAdd.Func, cmdCheck.Func, cmdDel.Func, versionInfo, "")
 
 			Expect(err).To(Equal(&types.Error{
-				Code: 100,
+				Code: types.ErrIOFailure,
 				Msg:  "error reading from stdin: banana",
 			}))
 		})
@@ -473,7 +473,7 @@ var _ = Describe("dispatching to the correct callback", func() {
 				err := dispatch.pluginMain(cmdAdd.Func, cmdCheck.Func, cmdDel.Func, versionInfo, "")
 
 				Expect(err).To(Equal(&types.Error{
-					Code: 100,
+					Code: types.ErrInternal,
 					Msg:  "potato",
 				}))
 			})

pkg/types/types.go

@@ -133,9 +133,16 @@ func (r *Route) String() string {
 // Well known error codes
 // see https://github.com/containernetworking/cni/blob/master/SPEC.md#well-known-error-codes
 const (
-	ErrUnknown                uint = iota // 0
-	ErrIncompatibleCNIVersion             // 1
-	ErrUnsupportedField                   // 2
+	ErrUnknown                     uint = iota // 0
+	ErrIncompatibleCNIVersion                  // 1
+	ErrUnsupportedField                        // 2
+	ErrUnknownContainer                        // 3
+	ErrInvalidEnvironmentVariables             // 4
+	ErrIOFailure                               // 5
+	ErrDecodingFailure                         // 6
+	ErrInvalidNetworkConfig                    // 7
+	ErrTryAgainLater               uint = 11
+	ErrInternal                    uint = 999
 )
 
 type Error struct {
@@ -144,6 +151,14 @@ type Error struct {
 	Details string `json:"details,omitempty"`
 }
 
+func NewError(code uint, msg, details string) *Error {
+	return &Error{
+		Code:    code,
+		Msg:     msg,
+		Details: details,
+	}
+}
+
 func (e *Error) Error() string {
 	details := ""
 	if e.Details != "" {

pkg/types/types_test.go

@@ -137,5 +137,10 @@ var _ = Describe("Types", func() {
 				})
 			})
 		})
+
+		It("NewError method", func() {
+			err := types.NewError(1234, "some message", "some details")
+			Expect(err).To(Equal(example))
+		})
 	})
 })

plugins/test/noop/noop_test.go

@@ -22,6 +22,7 @@ import (
 	"strings"
 
 	"github.com/containernetworking/cni/pkg/skel"
+	"github.com/containernetworking/cni/pkg/types"
 	"github.com/containernetworking/cni/pkg/version"
 	noop_debug "github.com/containernetworking/cni/plugins/test/noop/debug"
 	. "github.com/onsi/ginkgo"
@@ -219,7 +220,7 @@ var _ = Describe("No-op plugin", func() {
 			session, err := gexec.Start(cmd, GinkgoWriter, GinkgoWriter)
 			Expect(err).NotTo(HaveOccurred())
 			Eventually(session).Should(gexec.Exit(1))
-			Expect(session.Out.Contents()).To(MatchJSON(`{ "code": 100, "msg": "banana" }`))
+			Expect(session.Out.Contents()).To(MatchJSON(fmt.Sprintf(`{ "code": %d, "msg": "banana" }`, types.ErrInternal)))
 		})
 	})