From 5d029fb8078bb4d0b7a88ebb493af13f7f462a6d Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Fri, 13 Feb 2015 13:59:50 -0800 Subject: [PATCH] Add error return to Repository method on Registry The method (Registry).Repository may now return an error. This is too allow certain implementationt to validate the name or opt to not return a repository under certain conditions. In conjunction with this change, error declarations have been moved into a single file in the distribution package. Several error declarations that had remained in the storage package have been moved into distribution, as well. The declarations for Layer and LayerUpload have also been moved into the main registry file, as a result. Signed-off-by: Stephen J Day --- errors.go | 109 +++++++++++++++++++++++++ layer.go | 84 ------------------- notifications/listener_test.go | 6 +- registry.go | 46 ++++++++++- registry/api/v2/names.go | 8 +- registry/handlers/app.go | 38 +++++++-- registry/handlers/images.go | 7 +- registry/handlers/tags.go | 4 +- registry/storage/layer_test.go | 18 +++- registry/storage/manifeststore.go | 62 +------------- registry/storage/manifeststore_test.go | 8 +- registry/storage/registry.go | 12 ++- registry/storage/revisionstore.go | 3 +- registry/storage/tagstore.go | 5 +- 14 files changed, 236 insertions(+), 174 deletions(-) create mode 100644 errors.go delete mode 100644 layer.go diff --git a/errors.go b/errors.go new file mode 100644 index 000000000..7883b9f73 --- /dev/null +++ b/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) +} diff --git a/layer.go b/layer.go deleted file mode 100644 index dedc0538a..000000000 --- a/layer.go +++ /dev/null @@ -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) -} diff --git a/notifications/listener_test.go b/notifications/listener_test.go index 0f91a6a3f..c1cb1f703 100644 --- a/notifications/listener_test.go +++ b/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) diff --git a/registry.go b/registry.go index 1bb3b6299..cd3fb8028 100644 --- a/registry.go +++ b/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 +} diff --git a/registry/api/v2/names.go b/registry/api/v2/names.go index d05eeb6a2..ffac1858b 100644 --- a/registry/api/v2/names.go +++ b/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()) diff --git a/registry/handlers/app.go b/registry/handlers/app.go index 3a9f46a71..2202de4af 100644 --- a/registry/handlers/app.go +++ b/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. - context.Repository = notifications.Listen( - app.registry.Repository(context, getName(context)), - app.eventBridge(context, r)) + 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( + 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 route == nil || route.GetName() != v2.RouteNameBase { + if app.nameRequired(r) { // 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) { diff --git a/registry/handlers/images.go b/registry/handlers/images.go index 0e58984b0..de7b6dd6c 100644 --- a/registry/handlers/images.go +++ b/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: diff --git a/registry/handlers/tags.go b/registry/handlers/tags.go index 0a764693d..be84fae58 100644 --- a/registry/handlers/tags.go +++ b/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: diff --git a/registry/storage/layer_test.go b/registry/storage/layer_test.go index ec0186db5..ea101b53f 100644 --- a/registry/storage/layer_test.go +++ b/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 { diff --git a/registry/storage/manifeststore.go b/registry/storage/manifeststore.go index 998029058..765b5d056 100644 --- a/registry/storage/manifeststore.go +++ b/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) } diff --git a/registry/storage/manifeststore_test.go b/registry/storage/manifeststore_test.go index 1fd026629..d3a55ce55 100644 --- a/registry/storage/manifeststore_test.go +++ b/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) diff --git a/registry/storage/registry.go b/registry/storage/registry.go index 2983751a4..1a402f368 100644 --- a/registry/storage/registry.go +++ b/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. diff --git a/registry/storage/revisionstore.go b/registry/storage/revisionstore.go index b3ecd7117..e7122f3eb 100644 --- a/registry/storage/revisionstore.go +++ b/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, } diff --git a/registry/storage/tagstore.go b/registry/storage/tagstore.go index 6ae3e5f88..147623a29 100644 --- a/registry/storage/tagstore.go +++ b/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)