Use the new error package

This is the first step in converting out static strings into well-defined error types. This shows just a few examples of it to get a feel for how things will look. Once we agree on the basic outline we can then work on converting the rest of the code over. Signed-off-by: Doug Davis <dug@us.ibm.com>
2022-11-09 12:21:53 -05:00 · 2015-07-21 13:30:32 -07:00 · 2015-07-21 13:30:32 -07:00 · 628b9a41b0
commit 628b9a41b0
parent 8e05a3e30f
9 changed files with 286 additions and 37 deletions
--- a/api/errors/README.md
+++ b/api/errors/README.md
@ -0,0 +1,58 @@
+Docker 'errors' package
+=======================
+
+This package contains all of the error messages generated by the Docker
+engine that might be exposed via the Docker engine's REST API.
+
+Each top-level engine package will have its own file in this directory
+so that there's a clear grouping of errors, instead of just one big
+file. The errors for each package are defined here instead of within
+their respective package structure so that Docker CLI code that may need
+to import these error definition files will not need to know or understand
+the engine's package/directory structure. In other words, all they should
+need to do is import `.../docker/api/errors` and they will automatically
+pick up all Docker engine defined errors.  This also gives the engine
+developers the freedom to change the engine packaging structure (e.g. to
+CRUD packages) without worrying about breaking existing clients.
+
+These errors are defined using the 'errcode' package. The `errcode`  package
+allows for each error to be typed and include all information necessary to
+have further processing done on them if necessary.  In particular, each error
+includes:
+
+* Value - a unique string (in all caps) associated with this error.
+Typically, this string is the same name as the variable name of the error
+(w/o the `ErrorCode` text) but in all caps.
+
+* Message - the human readable sentence that will be displayed for this
+error. It can contain '%s' substitutions that allows for the code generating
+the error to specify values that will be inserted in the string prior to
+being displayed to the end-user. The `WithArgs()` function can be used to
+specify the insertion strings.  Note, the evaluation of the strings will be
+done at the time `WithArgs()` is called.
+
+* Description - additional human readable text to further explain the
+circumstances of the error situation.
+
+* HTTPStatusCode - when the error is returned back to a CLI, this value
+will be used to populate the HTTP status code. If not present the default
+value will be `StatusInternalServerError`, 500.
+
+Not all errors generated within the engine's executable will be propagated
+back to the engine's API layer. For example, it is expected that errors
+generated by vendored code (under `docker/vendor`) and packaged code
+(under `docker/pkg`) will be converted into errors defined by this package.
+
+When processing an errcode error, if you are looking for a particular
+error then you can do something like:
+
+```
+import derr "github.com/docker/docker/api/errors"
+
+...
+
+err := someFunc()
+if err.ErrorCode() == derr.ErrorCodeNoSuchContainer {
+	...
+}
+```
--- a/api/errors/builder.go
+++ b/api/errors/builder.go
@ -0,0 +1,93 @@
+package errors
+
+// This file contains all of the errors that can be generated from the
+// docker/builder component.
+
+import (
+	"net/http"
+
+	"github.com/docker/distribution/registry/api/errcode"
+)
+
+var (
+	// ErrorCodeAtLeastOneArg is generated when the parser comes across a
+	// Dockerfile command that doesn't have any args.
+	ErrorCodeAtLeastOneArg = errcode.Register(errGroup, errcode.ErrorDescriptor{
+		Value:          "ATLEASTONEARG",
+		Message:        "%s requires at least one argument",
+		Description:    "The specified command requires at least one argument",
+		HTTPStatusCode: http.StatusInternalServerError,
+	})
+
+	// ErrorCodeExactlyOneArg is generated when the parser comes across a
+	// Dockerfile command that requires exactly one arg but got less/more.
+	ErrorCodeExactlyOneArg = errcode.Register(errGroup, errcode.ErrorDescriptor{
+		Value:          "EXACTLYONEARG",
+		Message:        "%s requires exactly one argument",
+		Description:    "The specified command requires exactly one argument",
+		HTTPStatusCode: http.StatusInternalServerError,
+	})
+
+	// ErrorCodeAtLeastTwoArgs is generated when the parser comes across a
+	// Dockerfile command that requires at least two args but got less.
+	ErrorCodeAtLeastTwoArgs = errcode.Register(errGroup, errcode.ErrorDescriptor{
+		Value:          "ATLEASTTWOARGS",
+		Message:        "%s requires at least two arguments",
+		Description:    "The specified command requires at least two arguments",
+		HTTPStatusCode: http.StatusInternalServerError,
+	})
+
+	// ErrorCodeTooManyArgs is generated when the parser comes across a
+	// Dockerfile command that has more args than it should
+	ErrorCodeTooManyArgs = errcode.Register(errGroup, errcode.ErrorDescriptor{
+		Value:          "TOOMANYARGS",
+		Message:        "Bad input to %s, too many args",
+		Description:    "The specified command was passed too many arguments",
+		HTTPStatusCode: http.StatusInternalServerError,
+	})
+
+	// ErrorCodeChainOnBuild is generated when the parser comes across a
+	// Dockerfile command that is trying to chain ONBUILD commands.
+	ErrorCodeChainOnBuild = errcode.Register(errGroup, errcode.ErrorDescriptor{
+		Value:          "CHAINONBUILD",
+		Message:        "Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed",
+		Description:    "ONBUILD Dockerfile commands aren't allow on ONBUILD commands",
+		HTTPStatusCode: http.StatusInternalServerError,
+	})
+
+	// ErrorCodeBadOnBuildCmd is generated when the parser comes across a
+	// an ONBUILD Dockerfile command with an invalid trigger/command.
+	ErrorCodeBadOnBuildCmd = errcode.Register(errGroup, errcode.ErrorDescriptor{
+		Value:          "BADONBUILDCMD",
+		Message:        "%s isn't allowed as an ONBUILD trigger",
+		Description:    "The specified ONBUILD command isn't allowed",
+		HTTPStatusCode: http.StatusInternalServerError,
+	})
+
+	// ErrorCodeMissingFrom is generated when the Dockerfile is missing
+	// a FROM command.
+	ErrorCodeMissingFrom = errcode.Register(errGroup, errcode.ErrorDescriptor{
+		Value:          "MISSINGFROM",
+		Message:        "Please provide a source image with `from` prior to run",
+		Description:    "The Dockerfile is missing a FROM command",
+		HTTPStatusCode: http.StatusInternalServerError,
+	})
+
+	// ErrorCodeNotOnWindows is generated when the specified Dockerfile
+	// command is not supported on Windows.
+	ErrorCodeNotOnWindows = errcode.Register(errGroup, errcode.ErrorDescriptor{
+		Value:          "NOTONWINDOWS",
+		Message:        "%s is not supported on Windows",
+		Description:    "The specified Dockerfile command is not supported on Windows",
+		HTTPStatusCode: http.StatusInternalServerError,
+	})
+
+	// ErrorCodeVolumeEmpty is generated when the specified Volume string
+	// is empty.
+	ErrorCodeVolumeEmpty = errcode.Register(errGroup, errcode.ErrorDescriptor{
+		Value:          "VOLUMEEMPTY",
+		Message:        "Volume specified can not be an empty string",
+		Description:    "The specified volume can not be an empty string",
+		HTTPStatusCode: http.StatusInternalServerError,
+	})
+)
--- a/api/errors/daemon.go
+++ b/api/errors/daemon.go
@ -0,0 +1,21 @@
+package errors
+
+// This file contains all of the errors that can be generated from the
+// docker/daemon component.
+
+import (
+	"net/http"
+
+	"github.com/docker/distribution/registry/api/errcode"
+)
+
+var (
+	// ErrorCodeNoSuchContainer is generated when we look for a container by
+	// name or ID and we can't find it.
+	ErrorCodeNoSuchContainer = errcode.Register(errGroup, errcode.ErrorDescriptor{
+		Value:          "NOSUCHCONTAINER",
+		Message:        "no such id: %s",
+		Description:    "The specified container can not be found",
+		HTTPStatusCode: http.StatusNotFound,
+	})
+)
--- a/api/errors/error.go
+++ b/api/errors/error.go
@ -0,0 +1,6 @@
+package errors
+
+// This file contains all of the errors that can be generated from the
+// docker engine but are not tied to any specific top-level component.
+
+const errGroup = "engine"
--- a/api/server/image.go
+++ b/api/server/image.go
@ -3,6 +3,7 @@ package server
 import (
 	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@ -335,7 +336,7 @@ func (s *Server) postBuild(version version.Version, w http.ResponseWriter, r *ht
 			return err
 		}
 		sf := streamformatter.NewJSONStreamFormatter()
-		w.Write(sf.FormatError(err))
+		w.Write(sf.FormatError(errors.New(utils.GetErrorMessage(err))))
 	}
 	return nil
 }
--- a/api/server/server.go
+++ b/api/server/server.go
@ -14,6 +14,7 @@ import (
 	"github.com/gorilla/mux"

 	"github.com/Sirupsen/logrus"
+	"github.com/docker/distribution/registry/api/errcode"
 	"github.com/docker/docker/api"
 	"github.com/docker/docker/autogen/dockerversion"
 	"github.com/docker/docker/daemon"
@ -187,28 +188,71 @@ func httpError(w http.ResponseWriter, err error) {
 		logrus.WithFields(logrus.Fields{"error": err, "writer": w}).Error("unexpected HTTP error handling")
 		return
 	}
+
 	statusCode := http.StatusInternalServerError
-	// FIXME: this is brittle and should not be necessary.
-	// If we need to differentiate between different possible error types, we should
-	// create appropriate error types with clearly defined meaning.
-	errStr := strings.ToLower(err.Error())
-	for keyword, status := range map[string]int{
-		"not found":             http.StatusNotFound,
-		"no such":               http.StatusNotFound,
-		"bad parameter":         http.StatusBadRequest,
-		"conflict":              http.StatusConflict,
-		"impossible":            http.StatusNotAcceptable,
-		"wrong login/password":  http.StatusUnauthorized,
-		"hasn't been activated": http.StatusForbidden,
-	} {
-		if strings.Contains(errStr, keyword) {
-			statusCode = status
-			break
+	errMsg := err.Error()
+
+	// Based on the type of error we get we need to process things
+	// slightly differently to extract the error message.
+	// In the 'errcode.*' cases there are two different type of
+	// error that could be returned. errocode.ErrorCode is the base
+	// type of error object - it is just an 'int' that can then be
+	// used as the look-up key to find the message. errorcode.Error
+	// extends errorcode.Error by adding error-instance specific
+	// data, like 'details' or variable strings to be inserted into
+	// the message.
+	//
+	// Ideally, we should just be able to call err.Error() for all
+	// cases but the errcode package doesn't support that yet.
+	//
+	// Additionally, in both errcode cases, there might be an http
+	// status code associated with it, and if so use it.
+	switch err.(type) {
+	case errcode.ErrorCode:
+		daError, _ := err.(errcode.ErrorCode)
+		statusCode = daError.Descriptor().HTTPStatusCode
+		errMsg = daError.Message()
+
+	case errcode.Error:
+		// For reference, if you're looking for a particular error
+		// then you can do something like :
+		//   import ( derr "github.com/docker/docker/api/errors" )
+		//   if daError.ErrorCode() == derr.ErrorCodeNoSuchContainer { ... }
+
+		daError, _ := err.(errcode.Error)
+		statusCode = daError.ErrorCode().Descriptor().HTTPStatusCode
+		errMsg = daError.Message
+
+	default:
+		// This part of will be removed once we've
+		// converted everything over to use the errcode package
+
+		// FIXME: this is brittle and should not be necessary.
+		// If we need to differentiate between different possible error types,
+		// we should create appropriate error types with clearly defined meaning
+		errStr := strings.ToLower(err.Error())
+		for keyword, status := range map[string]int{
+			"not found":             http.StatusNotFound,
+			"no such":               http.StatusNotFound,
+			"bad parameter":         http.StatusBadRequest,
+			"conflict":              http.StatusConflict,
+			"impossible":            http.StatusNotAcceptable,
+			"wrong login/password":  http.StatusUnauthorized,
+			"hasn't been activated": http.StatusForbidden,
+		} {
+			if strings.Contains(errStr, keyword) {
+				statusCode = status
+				break
+			}
 		}
 	}

+	if statusCode == 0 {
+		statusCode = http.StatusInternalServerError
+	}
+
 	logrus.WithFields(logrus.Fields{"statusCode": statusCode, "err": err}).Error("HTTP Error")
-	http.Error(w, err.Error(), statusCode)
+	http.Error(w, errMsg, statusCode)
 }

 // writeJSON writes the value v to the http response stream as json with standard
--- a/builder/dispatchers.go
+++ b/builder/dispatchers.go
@ -18,6 +18,7 @@ import (
 	"strings"

 	"github.com/Sirupsen/logrus"
+	derr "github.com/docker/docker/api/errors"
 	flag "github.com/docker/docker/pkg/mflag"
 	"github.com/docker/docker/pkg/nat"
 	"github.com/docker/docker/runconfig"
@ -41,12 +42,12 @@ func nullDispatch(b *builder, args []string, attributes map[string]bool, origina
 //
 func env(b *builder, args []string, attributes map[string]bool, original string) error {
 	if len(args) == 0 {
-		return fmt.Errorf("ENV requires at least one argument")
+		return derr.ErrorCodeAtLeastOneArg.WithArgs("ENV")
 	}

 	if len(args)%2 != 0 {
 		// should never get here, but just in case
-		return fmt.Errorf("Bad input to ENV, too many args")
+		return derr.ErrorCodeTooManyArgs.WithArgs("ENV")
 	}

 	if err := b.BuilderFlags.Parse(); err != nil {
@ -100,7 +101,7 @@ func env(b *builder, args []string, attributes map[string]bool, original string)
 // Sets the maintainer metadata.
 func maintainer(b *builder, args []string, attributes map[string]bool, original string) error {
 	if len(args) != 1 {
-		return fmt.Errorf("MAINTAINER requires exactly one argument")
+		return derr.ErrorCodeExactlyOneArg.WithArgs("MAINTAINER")
 	}

 	if err := b.BuilderFlags.Parse(); err != nil {
@ -117,11 +118,11 @@ func maintainer(b *builder, args []string, attributes map[string]bool, original
 //
 func label(b *builder, args []string, attributes map[string]bool, original string) error {
 	if len(args) == 0 {
-		return fmt.Errorf("LABEL requires at least one argument")
+		return derr.ErrorCodeAtLeastOneArg.WithArgs("LABEL")
 	}
 	if len(args)%2 != 0 {
 		// should never get here, but just in case
-		return fmt.Errorf("Bad input to LABEL, too many args")
+		return derr.ErrorCodeTooManyArgs.WithArgs("LABEL")
 	}

 	if err := b.BuilderFlags.Parse(); err != nil {
@ -153,7 +154,7 @@ func label(b *builder, args []string, attributes map[string]bool, original strin
 //
 func add(b *builder, args []string, attributes map[string]bool, original string) error {
 	if len(args) < 2 {
-		return fmt.Errorf("ADD requires at least two arguments")
+		return derr.ErrorCodeAtLeastTwoArgs.WithArgs("ADD")
 	}

 	if err := b.BuilderFlags.Parse(); err != nil {
@ -169,7 +170,7 @@ func add(b *builder, args []string, attributes map[string]bool, original string)
 //
 func dispatchCopy(b *builder, args []string, attributes map[string]bool, original string) error {
 	if len(args) < 2 {
-		return fmt.Errorf("COPY requires at least two arguments")
+		return derr.ErrorCodeAtLeastTwoArgs.WithArgs("COPY")
 	}

 	if err := b.BuilderFlags.Parse(); err != nil {
@ -185,7 +186,7 @@ func dispatchCopy(b *builder, args []string, attributes map[string]bool, origina
 //
 func from(b *builder, args []string, attributes map[string]bool, original string) error {
 	if len(args) != 1 {
-		return fmt.Errorf("FROM requires one argument")
+		return derr.ErrorCodeExactlyOneArg.WithArgs("FROM")
 	}

 	if err := b.BuilderFlags.Parse(); err != nil {
@ -233,7 +234,7 @@ func from(b *builder, args []string, attributes map[string]bool, original string
 //
 func onbuild(b *builder, args []string, attributes map[string]bool, original string) error {
 	if len(args) == 0 {
-		return fmt.Errorf("ONBUILD requires at least one argument")
+		return derr.ErrorCodeAtLeastOneArg.WithArgs("ONBUILD")
 	}

 	if err := b.BuilderFlags.Parse(); err != nil {
@ -243,9 +244,9 @@ func onbuild(b *builder, args []string, attributes map[string]bool, original str
 	triggerInstruction := strings.ToUpper(strings.TrimSpace(args[0]))
 	switch triggerInstruction {
 	case "ONBUILD":
-		return fmt.Errorf("Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed")
+		return derr.ErrorCodeChainOnBuild
 	case "MAINTAINER", "FROM":
-		return fmt.Errorf("%s isn't allowed as an ONBUILD trigger", triggerInstruction)
+		return derr.ErrorCodeBadOnBuildCmd.WithArgs(triggerInstruction)
 	}

 	original = regexp.MustCompile(`(?i)^\s*ONBUILD\s*`).ReplaceAllString(original, "")
@ -260,7 +261,7 @@ func onbuild(b *builder, args []string, attributes map[string]bool, original str
 //
 func workdir(b *builder, args []string, attributes map[string]bool, original string) error {
 	if len(args) != 1 {
-		return fmt.Errorf("WORKDIR requires exactly one argument")
+		return derr.ErrorCodeExactlyOneArg.WithArgs("WORKDIR")
 	}

 	if err := b.BuilderFlags.Parse(); err != nil {
@ -317,7 +318,7 @@ func workdir(b *builder, args []string, attributes map[string]bool, original str
 //
 func run(b *builder, args []string, attributes map[string]bool, original string) error {
 	if b.image == "" && !b.noBaseImage {
-		return fmt.Errorf("Please provide a source image with `from` prior to run")
+		return derr.ErrorCodeMissingFrom
 	}

 	if err := b.BuilderFlags.Parse(); err != nil {
@ -467,7 +468,7 @@ func expose(b *builder, args []string, attributes map[string]bool, original stri
 	portsTab := args

 	if len(args) == 0 {
-		return fmt.Errorf("EXPOSE requires at least one argument")
+		return derr.ErrorCodeAtLeastOneArg.WithArgs("EXPOSE")
 	}

 	if err := b.BuilderFlags.Parse(); err != nil {
@ -506,11 +507,11 @@ func expose(b *builder, args []string, attributes map[string]bool, original stri
 //
 func user(b *builder, args []string, attributes map[string]bool, original string) error {
 	if runtime.GOOS == "windows" {
-		return fmt.Errorf("USER is not supported on Windows")
+		return derr.ErrorCodeNotOnWindows.WithArgs("USER")
 	}

 	if len(args) != 1 {
-		return fmt.Errorf("USER requires exactly one argument")
+		return derr.ErrorCodeExactlyOneArg.WithArgs("USER")
 	}

 	if err := b.BuilderFlags.Parse(); err != nil {
@ -527,10 +528,10 @@ func user(b *builder, args []string, attributes map[string]bool, original string
 //
 func volume(b *builder, args []string, attributes map[string]bool, original string) error {
 	if runtime.GOOS == "windows" {
-		return fmt.Errorf("VOLUME is not supported on Windows")
+		return derr.ErrorCodeNotOnWindows.WithArgs("VOLUME")
 	}
 	if len(args) == 0 {
-		return fmt.Errorf("VOLUME requires at least one argument")
+		return derr.ErrorCodeAtLeastOneArg.WithArgs("VOLUME")
 	}

 	if err := b.BuilderFlags.Parse(); err != nil {
@ -543,7 +544,7 @@ func volume(b *builder, args []string, attributes map[string]bool, original stri
 	for _, v := range args {
 		v = strings.TrimSpace(v)
 		if v == "" {
-			return fmt.Errorf("Volume specified can not be an empty string")
+			return derr.ErrorCodeVolumeEmpty
 		}
 		b.Config.Volumes[v] = struct{}{}
 	}
--- a/daemon/daemon.go
+++ b/daemon/daemon.go
@ -15,6 +15,7 @@ import (

 	"github.com/Sirupsen/logrus"
 	"github.com/docker/docker/api"
+	derr "github.com/docker/docker/api/errors"
 	"github.com/docker/docker/daemon/events"
 	"github.com/docker/docker/daemon/execdriver"
 	"github.com/docker/docker/daemon/execdriver/execdrivers"
@ -125,6 +126,10 @@ func (daemon *Daemon) Get(prefixOrName string) (*Container, error) {

 	containerId, indexError := daemon.idIndex.Get(prefixOrName)
 	if indexError != nil {
+		// When truncindex defines an error type, use that instead
+		if strings.Contains(indexError.Error(), "no such id") {
+			return nil, derr.ErrorCodeNoSuchContainer.WithArgs(prefixOrName)
+		}
 		return nil, indexError
 	}
 	return daemon.containers.Get(containerId), nil
--- a/utils/utils.go
+++ b/utils/utils.go
@ -13,6 +13,7 @@ import (
 	"runtime"
 	"strings"

+	"github.com/docker/distribution/registry/api/errcode"
 	"github.com/docker/docker/autogen/dockerversion"
 	"github.com/docker/docker/pkg/archive"
 	"github.com/docker/docker/pkg/fileutils"
@ -286,3 +287,22 @@ func ImageReference(repo, ref string) string {
 func DigestReference(ref string) bool {
 	return strings.Contains(ref, ":")
 }
+
+// GetErrorMessage returns the human readable message associated with
+// the passed-in error. In some cases the default Error() func returns
+// something that is less than useful so based on its types this func
+// will go and get a better piece of text.
+func GetErrorMessage(err error) string {
+	switch err.(type) {
+	case errcode.Error:
+		e, _ := err.(errcode.Error)
+		return e.Message
+
+	case errcode.ErrorCode:
+		ec, _ := err.(errcode.ErrorCode)
+		return ec.Message()
+
+	default:
+		return err.Error()
+	}
+}