TrueCloudLab/distribution
13
0
Fork 3
Code Issues 2 Pull requests Projects Releases 2 Packages Wiki Activity Actions

Fix for issue 664: https://github.com/docker/distribution/issues/664

Browse source 
Errors thrown by storage drivers don't have the name of the driver, causing user
confusion about whether the error is coming from Docker or from a storage driver.
This change adds the storage driver name to each error message.

This required changing ErrUnsupportedDriver to a type, leading to code changes
whenever ErrUnsupportedDriver is used.  The tests check whether the driver name
appears in the error message.

Signed-off-by: Amit Shukla <amit.shukla@docker.com>
This commit is contained in:
amitshukla 2015-10-02 16:19:06 -07:00 committed by Richard Scothern
parent a9da0e5100
commit 9637cb40cd
10 changed files with 99 additions and 44 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
registry/storage/blobserver.go
View file

@ -36,16 +36,15 @@ func (bs *blobServer) ServeBlob(ctx context.Context, w http.ResponseWriter, r *h
redirectURL, err := bs.driver.URLFor(ctx, path, map[string]interface{}{"method": r.Method})
switch err {
case nil:
if err == nil {
if bs.redirect {
// Redirect to storage URL.
http.Redirect(w, r, redirectURL, http.StatusTemporaryRedirect)
return err
}
}
fallthrough
case driver.ErrUnsupportedMethod:
if _, ok := err.(*driver.ErrUnsupportedMethod); ok {
// Fallback to serving the content directly.
br, err := newFileReader(ctx, bs.driver, path, desc.Size)
if err != nil {

71
registry/storage/driver/base/base.go
View file

@ -50,16 +50,40 @@ type Base struct {
storagedriver.StorageDriver
}
// Format errors received from the storage driver
func (base *Base) setDriverName(e error) error {
if e != nil {
if actualErr, ok := e.(storagedriver.ErrUnsupportedMethod); ok {
actualErr.DriverName = base.StorageDriver.Name()
return actualErr
}
if actualErr, ok := e.(storagedriver.PathNotFoundError); ok {
actualErr.DriverName = base.StorageDriver.Name()
return actualErr
}
if actualErr, ok := e.(storagedriver.InvalidPathError); ok {
actualErr.DriverName = base.StorageDriver.Name()
return actualErr
}
if actualErr, ok := e.(storagedriver.InvalidOffsetError); ok {
actualErr.DriverName = base.StorageDriver.Name()
return actualErr
}
}
return e
}
// GetContent wraps GetContent of underlying storage driver.
func (base *Base) GetContent(ctx context.Context, path string) ([]byte, error) {
ctx, done := context.WithTrace(ctx)
defer done("%s.GetContent(%q)", base.Name(), path)
if !storagedriver.PathRegexp.MatchString(path) {
return nil, storagedriver.InvalidPathError{Path: path}
return nil, storagedriver.InvalidPathError{Path: path, DriverName: base.StorageDriver.Name()}
}
return base.StorageDriver.GetContent(ctx, path)
b, e := base.StorageDriver.GetContent(ctx, path)
return b, base.setDriverName(e)
}
// PutContent wraps PutContent of underlying storage driver.
@ -68,10 +92,10 @@ func (base *Base) PutContent(ctx context.Context, path string, content []byte) e
defer done("%s.PutContent(%q)", base.Name(), path)
if !storagedriver.PathRegexp.MatchString(path) {
return storagedriver.InvalidPathError{Path: path}
return storagedriver.InvalidPathError{Path: path, DriverName: base.StorageDriver.Name()}
}
return base.StorageDriver.PutContent(ctx, path, content)
return base.setDriverName(base.StorageDriver.PutContent(ctx, path, content))
}
// ReadStream wraps ReadStream of underlying storage driver.
@ -80,14 +104,15 @@ func (base *Base) ReadStream(ctx context.Context, path string, offset int64) (io
defer done("%s.ReadStream(%q, %d)", base.Name(), path, offset)
if offset < 0 {
return nil, storagedriver.InvalidOffsetError{Path: path, Offset: offset}
return nil, storagedriver.InvalidOffsetError{Path: path, Offset: offset, DriverName: base.StorageDriver.Name()}
}
if !storagedriver.PathRegexp.MatchString(path) {
return nil, storagedriver.InvalidPathError{Path: path}
return nil, storagedriver.InvalidPathError{Path: path, DriverName: base.StorageDriver.Name()}
}
return base.StorageDriver.ReadStream(ctx, path, offset)
rc, e := base.StorageDriver.ReadStream(ctx, path, offset)
return rc, base.setDriverName(e)
}
// WriteStream wraps WriteStream of underlying storage driver.
@ -96,14 +121,15 @@ func (base *Base) WriteStream(ctx context.Context, path string, offset int64, re
defer done("%s.WriteStream(%q, %d)", base.Name(), path, offset)
if offset < 0 {
return 0, storagedriver.InvalidOffsetError{Path: path, Offset: offset}
return 0, storagedriver.InvalidOffsetError{Path: path, Offset: offset, DriverName: base.StorageDriver.Name()}
}
if !storagedriver.PathRegexp.MatchString(path) {
return 0, storagedriver.InvalidPathError{Path: path}
return 0, storagedriver.InvalidPathError{Path: path, DriverName: base.StorageDriver.Name()}
}
return base.StorageDriver.WriteStream(ctx, path, offset, reader)
i64, e := base.StorageDriver.WriteStream(ctx, path, offset, reader)
return i64, base.setDriverName(e)
}
// Stat wraps Stat of underlying storage driver.
@ -112,10 +138,11 @@ func (base *Base) Stat(ctx context.Context, path string) (storagedriver.FileInfo
defer done("%s.Stat(%q)", base.Name(), path)
if !storagedriver.PathRegexp.MatchString(path) {
return nil, storagedriver.InvalidPathError{Path: path}
return nil, storagedriver.InvalidPathError{Path: path, DriverName: base.StorageDriver.Name()}
}
return base.StorageDriver.Stat(ctx, path)
fi, e := base.StorageDriver.Stat(ctx, path)
return fi, base.setDriverName(e)
}
// List wraps List of underlying storage driver.
@ -124,10 +151,11 @@ func (base *Base) List(ctx context.Context, path string) ([]string, error) {
defer done("%s.List(%q)", base.Name(), path)
if !storagedriver.PathRegexp.MatchString(path) && path != "/" {
return nil, storagedriver.InvalidPathError{Path: path}
return nil, storagedriver.InvalidPathError{Path: path, DriverName: base.StorageDriver.Name()}
}
return base.StorageDriver.List(ctx, path)
str, e := base.StorageDriver.List(ctx, path)
return str, base.setDriverName(e)
}
// Move wraps Move of underlying storage driver.
@ -136,12 +164,12 @@ func (base *Base) Move(ctx context.Context, sourcePath string, destPath string)
defer done("%s.Move(%q, %q", base.Name(), sourcePath, destPath)
if !storagedriver.PathRegexp.MatchString(sourcePath) {
return storagedriver.InvalidPathError{Path: sourcePath}
return storagedriver.InvalidPathError{Path: sourcePath, DriverName: base.StorageDriver.Name()}
} else if !storagedriver.PathRegexp.MatchString(destPath) {
return storagedriver.InvalidPathError{Path: destPath}
return storagedriver.InvalidPathError{Path: destPath, DriverName: base.StorageDriver.Name()}
}
return base.StorageDriver.Move(ctx, sourcePath, destPath)
return base.setDriverName(base.StorageDriver.Move(ctx, sourcePath, destPath))
}
// Delete wraps Delete of underlying storage driver.
@ -150,10 +178,10 @@ func (base *Base) Delete(ctx context.Context, path string) error {
defer done("%s.Delete(%q)", base.Name(), path)
if !storagedriver.PathRegexp.MatchString(path) {
return storagedriver.InvalidPathError{Path: path}
return storagedriver.InvalidPathError{Path: path, DriverName: base.StorageDriver.Name()}
}
return base.StorageDriver.Delete(ctx, path)
return base.setDriverName(base.StorageDriver.Delete(ctx, path))
}
// URLFor wraps URLFor of underlying storage driver.
@ -162,8 +190,9 @@ func (base *Base) URLFor(ctx context.Context, path string, options map[string]in
defer done("%s.URLFor(%q)", base.Name(), path)
if !storagedriver.PathRegexp.MatchString(path) {
return "", storagedriver.InvalidPathError{Path: path}
return "", storagedriver.InvalidPathError{Path: path, DriverName: base.StorageDriver.Name()}
}
return base.StorageDriver.URLFor(ctx, path, options)
str, e := base.StorageDriver.URLFor(ctx, path, options)
return str, base.setDriverName(e)
}

2
registry/storage/driver/filesystem/driver.go
View file

@ -248,7 +248,7 @@ func (d *driver) Delete(ctx context.Context, subPath string) error {
// URLFor returns a URL which may be used to retrieve the content stored at the given path.
// May return an UnsupportedMethodErr in certain StorageDriver implementations.
func (d *driver) URLFor(ctx context.Context, path string, options map[string]interface{}) (string, error) {
return "", storagedriver.ErrUnsupportedMethod
return "", new(storagedriver.ErrUnsupportedMethod)
}
// fullPath returns the absolute path of a key within the Driver's storage.

2
registry/storage/driver/inmemory/driver.go
View file

@ -258,5 +258,5 @@ func (d *driver) Delete(ctx context.Context, path string) error {
// URLFor returns a URL which may be used to retrieve the content stored at the given path.
// May return an UnsupportedMethodErr in certain StorageDriver implementations.
func (d *driver) URLFor(ctx context.Context, path string, options map[string]interface{}) (string, error) {
return "", storagedriver.ErrUnsupportedMethod
return "", new(storagedriver.ErrUnsupportedMethod)
}

2
registry/storage/driver/oss/oss.go
View file

@ -748,7 +748,7 @@ func (d *driver) URLFor(ctx context.Context, path string, options map[string]int
if ok {
methodString, ok = method.(string)
if !ok || (methodString != "GET" && methodString != "HEAD") {
return "", storagedriver.ErrUnsupportedMethod
return "", new(storagedriver.ErrUnsupportedMethod)
}
}

2
registry/storage/driver/rados/rados.go
View file

@ -496,7 +496,7 @@ func (d *driver) Delete(ctx context.Context, objectPath string) error {
// URLFor returns a URL which may be used to retrieve the content stored at the given path.
// May return an UnsupportedMethodErr in certain StorageDriver implementations.
func (d *driver) URLFor(ctx context.Context, path string, options map[string]interface{}) (string, error) {
return "", storagedriver.ErrUnsupportedMethod
return "", new(storagedriver.ErrUnsupportedMethod)
}
// Generate a blob identifier

2
registry/storage/driver/s3/s3.go
View file

@ -759,7 +759,7 @@ func (d *driver) URLFor(ctx context.Context, path string, options map[string]int
if ok {
methodString, ok = method.(string)
if !ok || (methodString != "GET" && methodString != "HEAD") {
return "", storagedriver.ErrUnsupportedMethod
return "", new(storagedriver.ErrUnsupportedMethod)
}
}

18
registry/storage/driver/storagedriver.go
View file

@ -1,7 +1,6 @@
package driver
import (
"errors"
"fmt"
"io"
"regexp"
@ -93,24 +92,32 @@ type StorageDriver interface {
var PathRegexp = regexp.MustCompile(`^(/[A-Za-z0-9._-]+)+$`)
// ErrUnsupportedMethod may be returned in the case where a StorageDriver implementation does not support an optional method.
var ErrUnsupportedMethod = errors.New("unsupported method")
type ErrUnsupportedMethod struct {
DriverName string
}
func (err ErrUnsupportedMethod) Error() string {
return fmt.Sprintf("[%s] unsupported method", err.DriverName)
}
// PathNotFoundError is returned when operating on a nonexistent path.
type PathNotFoundError struct {
Path string
DriverName string
}
func (err PathNotFoundError) Error() string {
return fmt.Sprintf("Path not found: %s", err.Path)
return fmt.Sprintf("[%s] Path not found: %s", err.DriverName, err.Path)
}
// InvalidPathError is returned when the provided path is malformed.
type InvalidPathError struct {
Path string
DriverName string
}
func (err InvalidPathError) Error() string {
return fmt.Sprintf("Invalid path: %s", err.Path)
return fmt.Sprintf("[%s] Invalid path: %s", err.DriverName, err.Path)
}
// InvalidOffsetError is returned when attempting to read or write from an
@ -118,8 +125,9 @@ func (err InvalidPathError) Error() string {
type InvalidOffsetError struct {
Path string
Offset int64
DriverName string
}
func (err InvalidOffsetError) Error() string {
return fmt.Sprintf("Invalid offset: %d for path: %s", err.Offset, err.Path)
return fmt.Sprintf("[%s] Invalid offset: %d for path: %s", err.DriverName, err.Offset, err.Path)
}

6
registry/storage/driver/swift/swift.go
View file

@ -658,14 +658,14 @@ func (d *driver) Delete(ctx context.Context, path string) error {
// URLFor returns a URL which may be used to retrieve the content stored at the given path.
func (d *driver) URLFor(ctx context.Context, path string, options map[string]interface{}) (string, error) {
if d.SecretKey == "" {
return "", storagedriver.ErrUnsupportedMethod
return "", storagedriver.ErrUnsupportedMethod{}
}
methodString := "GET"
method, ok := options["method"]
if ok {
if methodString, ok = method.(string); !ok {
return "", storagedriver.ErrUnsupportedMethod
return "", storagedriver.ErrUnsupportedMethod{}
}
}
@ -684,7 +684,7 @@ func (d *driver) URLFor(ctx context.Context, path string, options map[string]int
}
if !supported {
return "", storagedriver.ErrUnsupportedMethod
return "", storagedriver.ErrUnsupportedMethod{}
}
expiresTime := time.Now().Add(20 * time.Minute)

23
registry/storage/driver/testsuites/testsuites.go
View file

@ -10,6 +10,7 @@ import (
"os"
"path"
"sort"
"strings"
"sync"
"testing"
"time"
@ -145,10 +146,12 @@ func (suite *DriverSuite) TestInvalidPaths(c *check.C) {
defer suite.StorageDriver.Delete(suite.ctx, firstPart(filename))
c.Assert(err, check.NotNil)
c.Assert(err, check.FitsTypeOf, storagedriver.InvalidPathError{})
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
_, err = suite.StorageDriver.GetContent(suite.ctx, filename)
c.Assert(err, check.NotNil)
c.Assert(err, check.FitsTypeOf, storagedriver.InvalidPathError{})
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
}
}
@ -205,6 +208,7 @@ func (suite *DriverSuite) TestReadNonexistent(c *check.C) {
_, err := suite.StorageDriver.GetContent(suite.ctx, filename)
c.Assert(err, check.NotNil)
c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{})
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
}
// TestWriteReadStreams1 tests a simple write-read streaming workflow.
@ -321,6 +325,7 @@ func (suite *DriverSuite) TestReadStreamWithOffset(c *check.C) {
c.Assert(err.(storagedriver.InvalidOffsetError).Offset, check.Equals, int64(-1))
c.Assert(err.(storagedriver.InvalidOffsetError).Path, check.Equals, filename)
c.Assert(reader, check.IsNil)
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
// Read past the end of the content and make sure we get a reader that
// returns 0 bytes and io.EOF
@ -443,6 +448,7 @@ func (suite *DriverSuite) testContinueStreamAppend(c *check.C, chunkSize int64)
c.Assert(err, check.FitsTypeOf, storagedriver.InvalidOffsetError{})
c.Assert(err.(storagedriver.InvalidOffsetError).Path, check.Equals, filename)
c.Assert(err.(storagedriver.InvalidOffsetError).Offset, check.Equals, int64(-1))
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
}
// TestReadNonexistentStream tests that reading a stream for a nonexistent path
@ -453,10 +459,12 @@ func (suite *DriverSuite) TestReadNonexistentStream(c *check.C) {
_, err := suite.StorageDriver.ReadStream(suite.ctx, filename, 0)
c.Assert(err, check.NotNil)
c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{})
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
_, err = suite.StorageDriver.ReadStream(suite.ctx, filename, 64)
c.Assert(err, check.NotNil)
c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{})
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
}
// TestList checks the returned list of keys after populating a directory tree.
@ -517,6 +525,7 @@ func (suite *DriverSuite) TestMove(c *check.C) {
_, err = suite.StorageDriver.GetContent(suite.ctx, sourcePath)
c.Assert(err, check.NotNil)
c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{})
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
}
// TestMoveOverwrite checks that a moved object no longer exists at the source
@ -546,6 +555,7 @@ func (suite *DriverSuite) TestMoveOverwrite(c *check.C) {
_, err = suite.StorageDriver.GetContent(suite.ctx, sourcePath)
c.Assert(err, check.NotNil)
c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{})
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
}
// TestMoveNonexistent checks that moving a nonexistent key fails and does not
@ -563,6 +573,7 @@ func (suite *DriverSuite) TestMoveNonexistent(c *check.C) {
err = suite.StorageDriver.Move(suite.ctx, sourcePath, destPath)
c.Assert(err, check.NotNil)
c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{})
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
received, err := suite.StorageDriver.GetContent(suite.ctx, destPath)
c.Assert(err, check.IsNil)
@ -600,6 +611,7 @@ func (suite *DriverSuite) TestDelete(c *check.C) {
_, err = suite.StorageDriver.GetContent(suite.ctx, filename)
c.Assert(err, check.NotNil)
c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{})
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
}
// TestURLFor checks that the URLFor method functions properly, but only if it
@ -614,7 +626,7 @@ func (suite *DriverSuite) TestURLFor(c *check.C) {
c.Assert(err, check.IsNil)
url, err := suite.StorageDriver.URLFor(suite.ctx, filename, nil)
if err == storagedriver.ErrUnsupportedMethod {
if _, ok := err.(*storagedriver.ErrUnsupportedMethod); ok {
return
}
c.Assert(err, check.IsNil)
@ -628,7 +640,7 @@ func (suite *DriverSuite) TestURLFor(c *check.C) {
c.Assert(read, check.DeepEquals, contents)
url, err = suite.StorageDriver.URLFor(suite.ctx, filename, map[string]interface{}{"method": "HEAD"})
if err == storagedriver.ErrUnsupportedMethod {
if _, ok := err.(*storagedriver.ErrUnsupportedMethod); ok {
return
}
c.Assert(err, check.IsNil)
@ -644,6 +656,7 @@ func (suite *DriverSuite) TestDeleteNonexistent(c *check.C) {
err := suite.StorageDriver.Delete(suite.ctx, filename)
c.Assert(err, check.NotNil)
c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{})
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
}
// TestDeleteFolder checks that deleting a folder removes all child elements.
@ -671,6 +684,7 @@ func (suite *DriverSuite) TestDeleteFolder(c *check.C) {
_, err = suite.StorageDriver.GetContent(suite.ctx, path.Join(dirname, filename1))
c.Assert(err, check.NotNil)
c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{})
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
_, err = suite.StorageDriver.GetContent(suite.ctx, path.Join(dirname, filename2))
c.Assert(err, check.IsNil)
@ -684,14 +698,17 @@ func (suite *DriverSuite) TestDeleteFolder(c *check.C) {
_, err = suite.StorageDriver.GetContent(suite.ctx, path.Join(dirname, filename1))
c.Assert(err, check.NotNil)
c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{})
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
_, err = suite.StorageDriver.GetContent(suite.ctx, path.Join(dirname, filename2))
c.Assert(err, check.NotNil)
c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{})
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
_, err = suite.StorageDriver.GetContent(suite.ctx, path.Join(dirname, filename3))
c.Assert(err, check.NotNil)
c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{})
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
}
// TestStatCall runs verifies the implementation of the storagedriver's Stat call.
@ -707,11 +724,13 @@ func (suite *DriverSuite) TestStatCall(c *check.C) {
fi, err := suite.StorageDriver.Stat(suite.ctx, dirPath)
c.Assert(err, check.NotNil)
c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{})
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
c.Assert(fi, check.IsNil)
fi, err = suite.StorageDriver.Stat(suite.ctx, filePath)
c.Assert(err, check.NotNil)
c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{})
c.Assert(strings.Contains(err.Error(), suite.Name()), check.Equals, true)
c.Assert(fi, check.IsNil)
err = suite.StorageDriver.PutContent(suite.ctx, filePath, content)