From 628b9a41b09fde3ce1493f7d4f1495b9afaa506c Mon Sep 17 00:00:00 2001 From: Doug Davis Date: Tue, 21 Jul 2015 13:30:32 -0700 Subject: [PATCH] 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 --- api/errors/README.md | 58 ++++++++++++++++++++++++++ api/errors/builder.go | 93 ++++++++++++++++++++++++++++++++++++++++++ api/errors/daemon.go | 21 ++++++++++ api/errors/error.go | 6 +++ api/server/image.go | 3 +- api/server/server.go | 78 +++++++++++++++++++++++++++-------- builder/dispatchers.go | 39 +++++++++--------- daemon/daemon.go | 5 +++ utils/utils.go | 20 +++++++++ 9 files changed, 286 insertions(+), 37 deletions(-) create mode 100644 api/errors/README.md create mode 100644 api/errors/builder.go create mode 100644 api/errors/daemon.go create mode 100644 api/errors/error.go diff --git a/api/errors/README.md b/api/errors/README.md new file mode 100644 index 0000000000..ae87aa098f --- /dev/null +++ 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 { + ... +} +``` diff --git a/api/errors/builder.go b/api/errors/builder.go new file mode 100644 index 0000000000..38d0d3c391 --- /dev/null +++ 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, + }) +) diff --git a/api/errors/daemon.go b/api/errors/daemon.go new file mode 100644 index 0000000000..20aa093e08 --- /dev/null +++ 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, + }) +) diff --git a/api/errors/error.go b/api/errors/error.go new file mode 100644 index 0000000000..37222d4438 --- /dev/null +++ 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" diff --git a/api/server/image.go b/api/server/image.go index c929fced37..ffac56c013 100644 --- 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 } diff --git a/api/server/server.go b/api/server/server.go index 66ba4e0842..a7e2a6db33 100644 --- 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 diff --git a/builder/dispatchers.go b/builder/dispatchers.go index a810e2cc6b..ff85e02834 100644 --- 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{}{} } diff --git a/daemon/daemon.go b/daemon/daemon.go index 4d6d43dc31..1261fb13a6 100644 --- 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 diff --git a/utils/utils.go b/utils/utils.go index 8c98d47210..c1065b08f1 100644 --- 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() + } +}