Merge pull request #174 from stevvooe/registry-interface-improvements

Add error return to Repository method on Registry

errors.go

@@ -0,0 +1,109 @@
+package distribution
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/docker/distribution/digest"
+	"github.com/docker/distribution/manifest"
+)
+
+var (
+	// ErrLayerExists returned when layer already exists
+	ErrLayerExists = fmt.Errorf("layer exists")
+
+	// ErrLayerTarSumVersionUnsupported when tarsum is unsupported version.
+	ErrLayerTarSumVersionUnsupported = fmt.Errorf("unsupported tarsum version")
+
+	// ErrLayerUploadUnknown returned when upload is not found.
+	ErrLayerUploadUnknown = fmt.Errorf("layer upload unknown")
+
+	// ErrLayerClosed returned when an operation is attempted on a closed
+	// Layer or LayerUpload.
+	ErrLayerClosed = fmt.Errorf("layer closed")
+)
+
+// ErrRepositoryUnknown is returned if the named repository is not known by
+// the registry.
+type ErrRepositoryUnknown struct {
+	Name string
+}
+
+func (err ErrRepositoryUnknown) Error() string {
+	return fmt.Sprintf("unknown respository name=%s", err.Name)
+}
+
+// ErrRepositoryNameInvalid should be used to denote an invalid repository
+// name. Reason may set, indicating the cause of invalidity.
+type ErrRepositoryNameInvalid struct {
+	Name   string
+	Reason error
+}
+
+func (err ErrRepositoryNameInvalid) Error() string {
+	return fmt.Sprintf("repository name %q invalid: %v", err.Name, err.Reason)
+}
+
+// ErrManifestUnknown is returned if the manifest is not known by the
+// registry.
+type ErrManifestUnknown struct {
+	Name string
+	Tag  string
+}
+
+func (err ErrManifestUnknown) Error() string {
+	return fmt.Sprintf("unknown manifest name=%s tag=%s", err.Name, err.Tag)
+}
+
+// ErrUnknownManifestRevision is returned when a manifest cannot be found by
+// revision within a repository.
+type ErrUnknownManifestRevision struct {
+	Name     string
+	Revision digest.Digest
+}
+
+func (err ErrUnknownManifestRevision) Error() string {
+	return fmt.Sprintf("unknown manifest name=%s revision=%s", err.Name, err.Revision)
+}
+
+// ErrManifestUnverified is returned when the registry is unable to verify
+// the manifest.
+type ErrManifestUnverified struct{}
+
+func (ErrManifestUnverified) Error() string {
+	return fmt.Sprintf("unverified manifest")
+}
+
+// ErrManifestVerification provides a type to collect errors encountered
+// during manifest verification. Currently, it accepts errors of all types,
+// but it may be narrowed to those involving manifest verification.
+type ErrManifestVerification []error
+
+func (errs ErrManifestVerification) Error() string {
+	var parts []string
+	for _, err := range errs {
+		parts = append(parts, err.Error())
+	}
+
+	return fmt.Sprintf("errors verifying manifest: %v", strings.Join(parts, ","))
+}
+
+// ErrUnknownLayer returned when layer cannot be found.
+type ErrUnknownLayer struct {
+	FSLayer manifest.FSLayer
+}
+
+func (err ErrUnknownLayer) Error() string {
+	return fmt.Sprintf("unknown layer %v", err.FSLayer.BlobSum)
+}
+
+// ErrLayerInvalidDigest returned when tarsum check fails.
+type ErrLayerInvalidDigest struct {
+	Digest digest.Digest
+	Reason error
+}
+
+func (err ErrLayerInvalidDigest) Error() string {
+	return fmt.Sprintf("invalid digest for referenced layer: %v, %v",
+		err.Digest, err.Reason)
+}

layer.go

@@ -1,84 +0,0 @@
-package distribution
-
-import (
-	"fmt"
-	"io"
-	"time"
-
-	"github.com/docker/distribution/digest"
-	"github.com/docker/distribution/manifest"
-)
-
-// Layer provides a readable and seekable layer object. Typically,
-// implementations are *not* goroutine safe.
-type Layer interface {
-	// http.ServeContent requires an efficient implementation of
-	// ReadSeeker.Seek(0, os.SEEK_END).
-	io.ReadSeeker
-	io.Closer
-
-	// Digest returns the unique digest of the blob, which is the tarsum for
-	// layers.
-	Digest() digest.Digest
-
-	// CreatedAt returns the time this layer was created.
-	CreatedAt() time.Time
-}
-
-// LayerUpload provides a handle for working with in-progress uploads.
-// Instances can be obtained from the LayerService.Upload and
-// LayerService.Resume.
-type LayerUpload interface {
-	io.WriteSeeker
-	io.ReaderFrom
-	io.Closer
-
-	// UUID returns the identifier for this upload.
-	UUID() string
-
-	// StartedAt returns the time this layer upload was started.
-	StartedAt() time.Time
-
-	// Finish marks the upload as completed, returning a valid handle to the
-	// uploaded layer. The digest is validated against the contents of the
-	// uploaded layer.
-	Finish(digest digest.Digest) (Layer, error)
-
-	// Cancel the layer upload process.
-	Cancel() error
-}
-
-var (
-	// ErrLayerExists returned when layer already exists
-	ErrLayerExists = fmt.Errorf("layer exists")
-
-	// ErrLayerTarSumVersionUnsupported when tarsum is unsupported version.
-	ErrLayerTarSumVersionUnsupported = fmt.Errorf("unsupported tarsum version")
-
-	// ErrLayerUploadUnknown returned when upload is not found.
-	ErrLayerUploadUnknown = fmt.Errorf("layer upload unknown")
-
-	// ErrLayerClosed returned when an operation is attempted on a closed
-	// Layer or LayerUpload.
-	ErrLayerClosed = fmt.Errorf("layer closed")
-)
-
-// ErrUnknownLayer returned when layer cannot be found.
-type ErrUnknownLayer struct {
-	FSLayer manifest.FSLayer
-}
-
-func (err ErrUnknownLayer) Error() string {
-	return fmt.Sprintf("unknown layer %v", err.FSLayer.BlobSum)
-}
-
-// ErrLayerInvalidDigest returned when tarsum check fails.
-type ErrLayerInvalidDigest struct {
-	Digest digest.Digest
-	Reason error
-}
-
-func (err ErrLayerInvalidDigest) Error() string {
-	return fmt.Sprintf("invalid digest for referenced layer: %v, %v",
-		err.Digest, err.Reason)
-}

notifications/listener_test.go

@@ -21,7 +21,11 @@ func TestListener(t *testing.T) {
 		ops: make(map[string]int),
 	}
 	ctx := context.Background()
-	repository := Listen(registry.Repository(ctx, "foo/bar"), tl)
+	repository, err := registry.Repository(ctx, "foo/bar")
+	if err != nil {
+		t.Fatalf("unexpected error getting repo: %v", err)
+	}
+	repository = Listen(repository, tl)
 
 	// Now take the registry through a number of operations
 	checkExerciseRepository(t, repository)

registry.go

@@ -1,19 +1,20 @@
 package distribution
 
 import (
+	"io"
+	"time"
+
 	"github.com/docker/distribution/digest"
 	"github.com/docker/distribution/manifest"
 	"golang.org/x/net/context"
 )
 
-// TODO(stevvooe): These types need to be moved out of the storage package.
-
 // Registry represents a collection of repositories, addressable by name.
 type Registry interface {
 	// Repository should return a reference to the named repository. The
 	// registry may or may not have the repository but should always return a
 	// reference.
-	Repository(ctx context.Context, name string) Repository
+	Repository(ctx context.Context, name string) (Repository, error)
 }
 
 // Repository is a named collection of manifests and layers.
@@ -82,3 +83,42 @@ type LayerService interface {
 	// before proceeding.
 	Resume(uuid string) (LayerUpload, error)
 }
+
+// Layer provides a readable and seekable layer object. Typically,
+// implementations are *not* goroutine safe.
+type Layer interface {
+	// http.ServeContent requires an efficient implementation of
+	// ReadSeeker.Seek(0, os.SEEK_END).
+	io.ReadSeeker
+	io.Closer
+
+	// Digest returns the unique digest of the blob, which is the tarsum for
+	// layers.
+	Digest() digest.Digest
+
+	// CreatedAt returns the time this layer was created.
+	CreatedAt() time.Time
+}
+
+// LayerUpload provides a handle for working with in-progress uploads.
+// Instances can be obtained from the LayerService.Upload and
+// LayerService.Resume.
+type LayerUpload interface {
+	io.WriteSeeker
+	io.ReaderFrom
+	io.Closer
+
+	// UUID returns the identifier for this upload.
+	UUID() string
+
+	// StartedAt returns the time this layer upload was started.
+	StartedAt() time.Time
+
+	// Finish marks the upload as completed, returning a valid handle to the
+	// uploaded layer. The digest is validated against the contents of the
+	// uploaded layer.
+	Finish(digest digest.Digest) (Layer, error)
+
+	// Cancel the layer upload process.
+	Cancel() error
+}

registry/api/v2/names.go

@@ -6,6 +6,10 @@ import (
 	"strings"
 )
 
+// TODO(stevvooe): Move these definitions back to an exported package. While
+// they are used with v2 definitions, their relevance expands beyond.
+// "distribution/names" is a candidate package.
+
 const (
 	// RepositoryNameComponentMinLength is the minimum number of characters in a
 	// single repository name slash-delimited component
@@ -37,10 +41,6 @@ var RepositoryNameComponentRegexp = regexp.MustCompile(`[a-z0-9]+(?:[._-][a-z0-9
 // RepositoryNameComponentRegexp which must completely match the content
 var RepositoryNameComponentAnchoredRegexp = regexp.MustCompile(`^` + RepositoryNameComponentRegexp.String() + `$`)
 
-// TODO(stevvooe): RepositoryName needs to be limited to some fixed length.
-// Looking path prefixes and s3 limitation of 1024, this should likely be
-// around 512 bytes. 256 bytes might be more manageable.
-
 // RepositoryNameRegexp builds on RepositoryNameComponentRegexp to allow 1 to
 // 5 path components, separated by a forward slash.
 var RepositoryNameRegexp = regexp.MustCompile(`(?:` + RepositoryNameComponentRegexp.String() + `/){0,4}` + RepositoryNameComponentRegexp.String())

registry/handlers/app.go

@@ -227,10 +227,30 @@ func (app *App) dispatcher(dispatch dispatchFunc) http.Handler {
 			return
 		}
 
-		// decorate the authorized repository with an event bridge.
+		if app.nameRequired(r) {
+			repository, err := app.registry.Repository(context, getName(context))
+
+			if err != nil {
+				ctxu.GetLogger(context).Errorf("error resolving repository: %v", err)
+
+				switch err := err.(type) {
+				case distribution.ErrRepositoryUnknown:
+					context.Errors.Push(v2.ErrorCodeNameUnknown, err)
+				case distribution.ErrRepositoryNameInvalid:
+					context.Errors.Push(v2.ErrorCodeNameInvalid, err)
+				}
+
+				w.WriteHeader(http.StatusBadRequest)
+				serveJSON(w, context.Errors)
+				return
+			}
+
+			// assign and decorate the authorized repository with an event bridge.
 			context.Repository = notifications.Listen(
-			app.registry.Repository(context, getName(context)),
+				repository,
 				app.eventBridge(context, r))
+		}
+
 		handler := dispatch(context, r)
 
 		ssrw := &singleStatusResponseWriter{ResponseWriter: w}
@@ -318,9 +338,7 @@ func (app *App) authorized(w http.ResponseWriter, r *http.Request, context *Cont
 		}
 	} else {
 		// Only allow the name not to be set on the base route.
-		route := mux.CurrentRoute(r)
+		if app.nameRequired(r) {
-
-		if route == nil || route.GetName() != v2.RouteNameBase {
 			// For this to be properly secured, repo must always be set for a
 			// resource that may make a modification. The only condition under
 			// which name is not set and we still allow access is when the
@@ -378,6 +396,12 @@ func (app *App) eventBridge(ctx *Context, r *http.Request) notifications.Listene
 	return notifications.NewBridge(ctx.urlBuilder, app.events.source, actor, request, app.events.sink)
 }
 
+// nameRequired returns true if the route requires a name.
+func (app *App) nameRequired(r *http.Request) bool {
+	route := mux.CurrentRoute(r)
+	return route == nil || route.GetName() != v2.RouteNameBase
+}
+
 // apiBase implements a simple yes-man for doing overall checks against the
 // api. This can support auth roundtrips to support docker login.
 func apiBase(w http.ResponseWriter, r *http.Request) {

registry/handlers/images.go

@@ -10,7 +10,6 @@ import (
 	"github.com/docker/distribution/digest"
 	"github.com/docker/distribution/manifest"
 	"github.com/docker/distribution/registry/api/v2"
-	"github.com/docker/distribution/registry/storage"
 	"github.com/gorilla/handlers"
 )
 
@@ -70,12 +69,12 @@ func (imh *imageManifestHandler) PutImageManifest(w http.ResponseWriter, r *http
 		// TODO(stevvooe): These error handling switches really need to be
 		// handled by an app global mapper.
 		switch err := err.(type) {
-		case storage.ErrManifestVerification:
+		case distribution.ErrManifestVerification:
 			for _, verificationError := range err {
 				switch verificationError := verificationError.(type) {
 				case distribution.ErrUnknownLayer:
 					imh.Errors.Push(v2.ErrorCodeBlobUnknown, verificationError.FSLayer)
-				case storage.ErrManifestUnverified:
+				case distribution.ErrManifestUnverified:
 					imh.Errors.Push(v2.ErrorCodeManifestUnverified)
 				default:
 					if verificationError == digest.ErrDigestInvalidFormat {
@@ -104,7 +103,7 @@ func (imh *imageManifestHandler) DeleteImageManifest(w http.ResponseWriter, r *h
 	manifests := imh.Repository.Manifests()
 	if err := manifests.Delete(imh.Tag); err != nil {
 		switch err := err.(type) {
-		case storage.ErrUnknownManifest:
+		case distribution.ErrManifestUnknown:
 			imh.Errors.Push(v2.ErrorCodeManifestUnknown, err)
 			w.WriteHeader(http.StatusNotFound)
 		default:

registry/handlers/tags.go

@@ -4,8 +4,8 @@ import (
 	"encoding/json"
 	"net/http"
 
+	"github.com/docker/distribution"
 	"github.com/docker/distribution/registry/api/v2"
-	"github.com/docker/distribution/registry/storage"
 	"github.com/gorilla/handlers"
 )
 
@@ -38,7 +38,7 @@ func (th *tagsHandler) GetTags(w http.ResponseWriter, r *http.Request) {
 	tags, err := manifests.Tags()
 	if err != nil {
 		switch err := err.(type) {
-		case storage.ErrUnknownRepository:
+		case distribution.ErrRepositoryUnknown:
 			w.WriteHeader(404)
 			th.Errors.Push(v2.ErrorCodeNameUnknown, map[string]string{"name": th.Repository.Name()})
 		default:

registry/storage/layer_test.go

@@ -36,7 +36,11 @@ func TestSimpleLayerUpload(t *testing.T) {
 	imageName := "foo/bar"
 	driver := inmemory.New()
 	registry := NewRegistryWithDriver(driver)
-	ls := registry.Repository(ctx, imageName).Layers()
+	repository, err := registry.Repository(ctx, imageName)
+	if err != nil {
+		t.Fatalf("unexpected error getting repo: %v", err)
+	}
+	ls := repository.Layers()
 
 	h := sha256.New()
 	rd := io.TeeReader(randomDataReader, h)
@@ -140,7 +144,11 @@ func TestSimpleLayerRead(t *testing.T) {
 	imageName := "foo/bar"
 	driver := inmemory.New()
 	registry := NewRegistryWithDriver(driver)
-	ls := registry.Repository(ctx, imageName).Layers()
+	repository, err := registry.Repository(ctx, imageName)
+	if err != nil {
+		t.Fatalf("unexpected error getting repo: %v", err)
+	}
+	ls := repository.Layers()
 
 	randomLayerReader, tarSumStr, err := testutil.CreateRandomTarFile()
 	if err != nil {
@@ -245,7 +253,11 @@ func TestLayerUploadZeroLength(t *testing.T) {
 	imageName := "foo/bar"
 	driver := inmemory.New()
 	registry := NewRegistryWithDriver(driver)
-	ls := registry.Repository(ctx, imageName).Layers()
+	repository, err := registry.Repository(ctx, imageName)
+	if err != nil {
+		t.Fatalf("unexpected error getting repo: %v", err)
+	}
+	ls := repository.Layers()
 
 	upload, err := ls.Upload()
 	if err != nil {

registry/storage/manifeststore.go

@@ -2,69 +2,13 @@ package storage
 
 import (
 	"fmt"
-	"strings"
 
 	"github.com/docker/distribution"
 	ctxu "github.com/docker/distribution/context"
-	"github.com/docker/distribution/digest"
 	"github.com/docker/distribution/manifest"
 	"github.com/docker/libtrust"
 )
 
-// ErrUnknownRepository is returned if the named repository is not known by
-// the registry.
-type ErrUnknownRepository struct {
-	Name string
-}
-
-func (err ErrUnknownRepository) Error() string {
-	return fmt.Sprintf("unknown respository name=%s", err.Name)
-}
-
-// ErrUnknownManifest is returned if the manifest is not known by the
-// registry.
-type ErrUnknownManifest struct {
-	Name string
-	Tag  string
-}
-
-func (err ErrUnknownManifest) Error() string {
-	return fmt.Sprintf("unknown manifest name=%s tag=%s", err.Name, err.Tag)
-}
-
-// ErrUnknownManifestRevision is returned when a manifest cannot be found by
-// revision within a repository.
-type ErrUnknownManifestRevision struct {
-	Name     string
-	Revision digest.Digest
-}
-
-func (err ErrUnknownManifestRevision) Error() string {
-	return fmt.Sprintf("unknown manifest name=%s revision=%s", err.Name, err.Revision)
-}
-
-// ErrManifestUnverified is returned when the registry is unable to verify
-// the manifest.
-type ErrManifestUnverified struct{}
-
-func (ErrManifestUnverified) Error() string {
-	return fmt.Sprintf("unverified manifest")
-}
-
-// ErrManifestVerification provides a type to collect errors encountered
-// during manifest verification. Currently, it accepts errors of all types,
-// but it may be narrowed to those involving manifest verification.
-type ErrManifestVerification []error
-
-func (errs ErrManifestVerification) Error() string {
-	var parts []string
-	for _, err := range errs {
-		parts = append(parts, err.Error())
-	}
-
-	return fmt.Sprintf("errors verifying manifest: %v", strings.Join(parts, ","))
-}
-
 type manifestStore struct {
 	repository *repository
 
@@ -147,7 +91,7 @@ func (ms *manifestStore) Delete(tag string) error {
 // registry only tries to store valid content, leaving trust policies of that
 // content up to consumers.
 func (ms *manifestStore) verifyManifest(tag string, mnfst *manifest.SignedManifest) error {
-	var errs ErrManifestVerification
+	var errs distribution.ErrManifestVerification
 	if mnfst.Name != ms.repository.Name() {
 		// TODO(stevvooe): This needs to be an exported error
 		errs = append(errs, fmt.Errorf("repository name does not match manifest name"))
@@ -161,10 +105,10 @@ func (ms *manifestStore) verifyManifest(tag string, mnfst *manifest.SignedManife
 	if _, err := manifest.Verify(mnfst); err != nil {
 		switch err {
 		case libtrust.ErrMissingSignatureKey, libtrust.ErrInvalidJSONContent, libtrust.ErrMissingSignatureKey:
-			errs = append(errs, ErrManifestUnverified{})
+			errs = append(errs, distribution.ErrManifestUnverified{})
 		default:
 			if err.Error() == "invalid signature" { // TODO(stevvooe): This should be exported by libtrust
-				errs = append(errs, ErrManifestUnverified{})
+				errs = append(errs, distribution.ErrManifestUnverified{})
 			} else {
 				errs = append(errs, err)
 			}

registry/storage/manifeststore_test.go

@@ -6,6 +6,7 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/docker/distribution"
 	"github.com/docker/distribution/digest"
 	"github.com/docker/distribution/manifest"
 	"github.com/docker/distribution/registry/storage/driver/inmemory"
@@ -20,7 +21,10 @@ func TestManifestStorage(t *testing.T) {
 	tag := "thetag"
 	driver := inmemory.New()
 	registry := NewRegistryWithDriver(driver)
-	repo := registry.Repository(ctx, name)
+	repo, err := registry.Repository(ctx, name)
+	if err != nil {
+		t.Fatalf("unexpected error getting repo: %v", err)
+	}
 	ms := repo.Manifests()
 
 	exists, err := ms.Exists(tag)
@@ -34,7 +38,7 @@ func TestManifestStorage(t *testing.T) {
 
 	if _, err := ms.Get(tag); true {
 		switch err.(type) {
-		case ErrUnknownManifest:
+		case distribution.ErrManifestUnknown:
 			break
 		default:
 			t.Fatalf("expected manifest unknown error: %#v", err)

registry/storage/registry.go

@@ -2,6 +2,7 @@ package storage
 
 import (
 	"github.com/docker/distribution"
+	"github.com/docker/distribution/registry/api/v2"
 	storagedriver "github.com/docker/distribution/registry/storage/driver"
 	"golang.org/x/net/context"
 )
@@ -36,12 +37,19 @@ func NewRegistryWithDriver(driver storagedriver.StorageDriver) distribution.Regi
 // Repository returns an instance of the repository tied to the registry.
 // Instances should not be shared between goroutines but are cheap to
 // allocate. In general, they should be request scoped.
-func (reg *registry) Repository(ctx context.Context, name string) distribution.Repository {
+func (reg *registry) Repository(ctx context.Context, name string) (distribution.Repository, error) {
+	if err := v2.ValidateRespositoryName(name); err != nil {
+		return nil, distribution.ErrRepositoryNameInvalid{
+			Name:   name,
+			Reason: err,
+		}
+	}
+
 	return &repository{
 		ctx:      ctx,
 		registry: reg,
 		name:     name,
-	}
+	}, nil
 }
 
 // repository provides name-scoped access to various services.

registry/storage/revisionstore.go

@@ -5,6 +5,7 @@ import (
 	"path"
 
 	"github.com/Sirupsen/logrus"
+	"github.com/docker/distribution"
 	"github.com/docker/distribution/digest"
 	"github.com/docker/distribution/manifest"
 	"github.com/docker/libtrust"
@@ -40,7 +41,7 @@ func (rs *revisionStore) get(revision digest.Digest) (*manifest.SignedManifest,
 	if exists, err := rs.exists(revision); err != nil {
 		return nil, err
 	} else if !exists {
-		return nil, ErrUnknownManifestRevision{
+		return nil, distribution.ErrUnknownManifestRevision{
 			Name:     rs.Name(),
 			Revision: revision,
 		}

registry/storage/tagstore.go

@@ -3,6 +3,7 @@ package storage
 import (
 	"path"
 
+	"github.com/docker/distribution"
 	"github.com/docker/distribution/digest"
 	storagedriver "github.com/docker/distribution/registry/storage/driver"
 )
@@ -26,7 +27,7 @@ func (ts *tagStore) tags() ([]string, error) {
 	if err != nil {
 		switch err := err.(type) {
 		case storagedriver.PathNotFoundError:
-			return nil, ErrUnknownRepository{Name: ts.name}
+			return nil, distribution.ErrRepositoryUnknown{Name: ts.name}
 		default:
 			return nil, err
 		}
@@ -104,7 +105,7 @@ func (ts *tagStore) resolve(tag string) (digest.Digest, error) {
 	if exists, err := exists(ts.driver, currentPath); err != nil {
 		return "", err
 	} else if !exists {
-		return "", ErrUnknownManifest{Name: ts.Name(), Tag: tag}
+		return "", distribution.ErrManifestUnknown{Name: ts.Name(), Tag: tag}
 	}
 
 	revision, err := ts.blobStore.readlink(currentPath)