From 776a4ffbe8b0bd5a68ad97a705d7535cef196cb5 Mon Sep 17 00:00:00 2001 From: Richard Scothern Date: Tue, 11 Aug 2015 11:00:30 -0700 Subject: [PATCH] Change some incorrect error types in proxy stores from API errors to distribution errors. Fill in missing checks for mutations on a registry pull-through cache. Add unit tests and update documentation. Also, give v2.ErrorCodeUnsupported an HTTP status code, previously it was defaulting to 500, now its 405 Method Not Allowed. Signed-off-by: Richard Scothern --- blobs.go | 3 -- docs/spec/api.md | 57 +++++++++++++++++++++++- errors.go | 4 ++ registry/api/errcode/register.go | 2 +- registry/api/v2/descriptors.go | 27 +++++++++++- registry/handlers/api_test.go | 65 ++++++++++++++++++++++++++++ registry/handlers/blob.go | 13 +++--- registry/handlers/blobupload.go | 9 +++- registry/handlers/images.go | 8 ++-- registry/proxy/proxymanifeststore.go | 5 +-- 10 files changed, 174 insertions(+), 19 deletions(-) diff --git a/blobs.go b/blobs.go index 556bf93e1..2087d0f9e 100644 --- a/blobs.go +++ b/blobs.go @@ -27,9 +27,6 @@ var ( // ErrBlobInvalidLength returned when the blob has an expected length on // commit, meaning mismatched with the descriptor or an invalid value. ErrBlobInvalidLength = errors.New("blob invalid length") - - // ErrUnsupported returned when an unsupported operation is attempted - ErrUnsupported = errors.New("unsupported operation") ) // ErrBlobInvalidDigest returned when digest check fails. diff --git a/docs/spec/api.md b/docs/spec/api.md index 40e21e413..81450657f 100644 --- a/docs/spec/api.md +++ b/docs/spec/api.md @@ -1731,6 +1731,24 @@ The error codes that may be included in the response body are enumerated below: +###### On Failure: Not allowed + +``` +405 Method Not Allowed +``` + +Manifest put is not allowed because the registry is configured as a pull-through cache or for some other reason + + + +The error codes that may be included in the response body are enumerated below: + +|Code|Message|Description| +|----|-------|-----------| +| `UNSUPPORTED` | The operation is unsupported. | The operation was unsupported due to a missing implementation or invalid set of parameters. | + + + #### DELETE Manifest @@ -1871,6 +1889,24 @@ The error codes that may be included in the response body are enumerated below: +###### On Failure: Not allowed + +``` +405 Method Not Allowed +``` + +Manifest delete is not allowed because the registry is configured as a pull-through cache or `delete` has been disabled. + + + +The error codes that may be included in the response body are enumerated below: + +|Code|Message|Description| +|----|-------|-----------| +| `UNSUPPORTED` | The operation is unsupported. | The operation was unsupported due to a missing implementation or invalid set of parameters. | + + + ### Blob @@ -2323,7 +2359,7 @@ Content-Type: application/json; charset=utf-8 } ``` -Delete is not enabled on the registry +Blob delete is not allowed because the registry is configured as a pull-through cache or `delete` has been disabled @@ -2456,6 +2492,24 @@ The error codes that may be included in the response body are enumerated below: +###### On Failure: Not allowed + +``` +405 Method Not Allowed +``` + +Blob upload is not allowed because the registry is configured as a pull-through cache or for some other reason + + + +The error codes that may be included in the response body are enumerated below: + +|Code|Message|Description| +|----|-------|-----------| +| `UNSUPPORTED` | The operation is unsupported. | The operation was unsupported due to a missing implementation or invalid set of parameters. | + + + ##### Initiate Resumable Blob Upload ``` @@ -3129,6 +3183,7 @@ The error codes that may be included in the response body are enumerated below: | `DIGEST_INVALID` | provided digest did not match uploaded content | When a blob is uploaded, the registry will check that the content matches the digest provided by the client. The error may include a detail structure with the key "digest", including the invalid digest string. This error may also be returned when a manifest includes an invalid layer digest. | | `NAME_INVALID` | invalid repository name | Invalid repository name encountered either during manifest validation or any API operation. | | `BLOB_UPLOAD_INVALID` | blob upload invalid | The blob upload encountered an error and can no longer proceed. | +| `UNSUPPORTED` | The operation is unsupported. | The operation was unsupported due to a missing implementation or invalid set of parameters. | diff --git a/errors.go b/errors.go index 53def4b8c..4511fde67 100644 --- a/errors.go +++ b/errors.go @@ -7,6 +7,10 @@ import ( "github.com/docker/distribution/digest" ) +// ErrUnsupported is returned when an unimplemented or unsupported action is +// performed +var ErrUnsupported = fmt.Errorf("operation unsupported") + // ErrRepositoryUnknown is returned if the named repository is not known by // the registry. type ErrRepositoryUnknown struct { diff --git a/registry/api/errcode/register.go b/registry/api/errcode/register.go index e1c93f38f..f3062ffaf 100644 --- a/registry/api/errcode/register.go +++ b/registry/api/errcode/register.go @@ -30,7 +30,7 @@ var ( Message: "The operation is unsupported.", Description: `The operation was unsupported due to a missing implementation or invalid set of parameters.`, - HTTPStatusCode: http.StatusBadRequest, + HTTPStatusCode: http.StatusMethodNotAllowed, }) // ErrorCodeUnauthorized is returned if a request is not authorized. diff --git a/registry/api/v2/descriptors.go b/registry/api/v2/descriptors.go index 09289b96b..c5630fed2 100644 --- a/registry/api/v2/descriptors.go +++ b/registry/api/v2/descriptors.go @@ -689,6 +689,14 @@ var routeDescriptors = []RouteDescriptor{ Format: errorsBody, }, }, + { + Name: "Not allowed", + Description: "Manifest put is not allowed because the registry is configured as a pull-through cache or for some other reason", + StatusCode: http.StatusMethodNotAllowed, + ErrorCodes: []errcode.ErrorCode{ + errcode.ErrorCodeUnsupported, + }, + }, }, }, }, @@ -757,6 +765,14 @@ var routeDescriptors = []RouteDescriptor{ Format: errorsBody, }, }, + { + Name: "Not allowed", + Description: "Manifest delete is not allowed because the registry is configured as a pull-through cache or `delete` has been disabled.", + StatusCode: http.StatusMethodNotAllowed, + ErrorCodes: []errcode.ErrorCode{ + errcode.ErrorCodeUnsupported, + }, + }, }, }, }, @@ -967,7 +983,7 @@ var routeDescriptors = []RouteDescriptor{ }, }, { - Description: "Delete is not enabled on the registry", + Description: "Blob delete is not allowed because the registry is configured as a pull-through cache or `delete` has been disabled", StatusCode: http.StatusMethodNotAllowed, Body: BodyDescriptor{ ContentType: "application/json; charset=utf-8", @@ -1051,6 +1067,14 @@ var routeDescriptors = []RouteDescriptor{ }, }, unauthorizedResponsePush, + { + Name: "Not allowed", + Description: "Blob upload is not allowed because the registry is configured as a pull-through cache or for some other reason", + StatusCode: http.StatusMethodNotAllowed, + ErrorCodes: []errcode.ErrorCode{ + errcode.ErrorCodeUnsupported, + }, + }, }, }, { @@ -1389,6 +1413,7 @@ var routeDescriptors = []RouteDescriptor{ ErrorCodeDigestInvalid, ErrorCodeNameInvalid, ErrorCodeBlobUploadInvalid, + errcode.ErrorCodeUnsupported, }, Body: BodyDescriptor{ ContentType: "application/json; charset=utf-8", diff --git a/registry/handlers/api_test.go b/registry/handlers/api_test.go index c484835fd..4c700e062 100644 --- a/registry/handlers/api_test.go +++ b/registry/handlers/api_test.go @@ -1001,6 +1001,21 @@ type testEnv struct { builder *v2.URLBuilder } +func newTestEnvMirror(t *testing.T, deleteEnabled bool) *testEnv { + config := configuration.Configuration{ + Storage: configuration.Storage{ + "inmemory": configuration.Parameters{}, + "delete": configuration.Parameters{"enabled": deleteEnabled}, + }, + Proxy: configuration.Proxy{ + RemoteURL: "http://example.com", + }, + } + + return newTestEnvWithConfig(t, &config) + +} + func newTestEnv(t *testing.T, deleteEnabled bool) *testEnv { config := configuration.Configuration{ Storage: configuration.Storage{ @@ -1378,3 +1393,53 @@ func createRepository(env *testEnv, t *testing.T, imageName string, tag string) "Docker-Content-Digest": []string{dgst.String()}, }) } + +// Test mutation operations on a registry configured as a cache. Ensure that they return +// appropriate errors. +func TestRegistryAsCacheMutationAPIs(t *testing.T) { + deleteEnabled := true + env := newTestEnvMirror(t, deleteEnabled) + + imageName := "foo/bar" + tag := "latest" + manifestURL, err := env.builder.BuildManifestURL(imageName, tag) + if err != nil { + t.Fatalf("unexpected error building base url: %v", err) + } + + // Manifest upload + unsignedManifest := &manifest.Manifest{ + Versioned: manifest.Versioned{ + SchemaVersion: 1, + }, + Name: imageName, + Tag: tag, + FSLayers: []manifest.FSLayer{}, + } + resp := putManifest(t, "putting unsigned manifest", manifestURL, unsignedManifest) + checkResponse(t, "putting signed manifest to cache", resp, errcode.ErrorCodeUnsupported.Descriptor().HTTPStatusCode) + + // Manifest Delete + resp, err = httpDelete(manifestURL) + checkResponse(t, "deleting signed manifest from cache", resp, errcode.ErrorCodeUnsupported.Descriptor().HTTPStatusCode) + + // Blob upload initialization + layerUploadURL, err := env.builder.BuildBlobUploadURL(imageName) + if err != nil { + t.Fatalf("unexpected error building layer upload url: %v", err) + } + + resp, err = http.Post(layerUploadURL, "", nil) + if err != nil { + t.Fatalf("unexpected error starting layer push: %v", err) + } + defer resp.Body.Close() + + checkResponse(t, fmt.Sprintf("starting layer push to cache %v", imageName), resp, errcode.ErrorCodeUnsupported.Descriptor().HTTPStatusCode) + + // Blob Delete + blobURL, err := env.builder.BuildBlobURL(imageName, digest.DigestSha256EmptyTar) + resp, err = httpDelete(blobURL) + checkResponse(t, "deleting blob from cache", resp, errcode.ErrorCodeUnsupported.Descriptor().HTTPStatusCode) + +} diff --git a/registry/handlers/blob.go b/registry/handlers/blob.go index fd514ec08..4a923aa51 100644 --- a/registry/handlers/blob.go +++ b/registry/handlers/blob.go @@ -76,16 +76,17 @@ func (bh *blobHandler) DeleteBlob(w http.ResponseWriter, r *http.Request) { err := blobs.Delete(bh, bh.Digest) if err != nil { switch err { - case distribution.ErrBlobUnknown: - w.WriteHeader(http.StatusNotFound) - bh.Errors = append(bh.Errors, v2.ErrorCodeBlobUnknown) case distribution.ErrUnsupported: - w.WriteHeader(http.StatusMethodNotAllowed) bh.Errors = append(bh.Errors, errcode.ErrorCodeUnsupported) + return + case distribution.ErrBlobUnknown: + bh.Errors = append(bh.Errors, v2.ErrorCodeBlobUnknown) + return default: - bh.Errors = append(bh.Errors, errcode.ErrorCodeUnknown) + bh.Errors = append(bh.Errors, err) + context.GetLogger(bh).Errorf("Unknown error deleting blob: %s", err.Error()) + return } - return } w.Header().Set("Content-Length", "0") diff --git a/registry/handlers/blobupload.go b/registry/handlers/blobupload.go index 1d1c1009d..bbb70b59d 100644 --- a/registry/handlers/blobupload.go +++ b/registry/handlers/blobupload.go @@ -117,8 +117,13 @@ type blobUploadHandler struct { func (buh *blobUploadHandler) StartBlobUpload(w http.ResponseWriter, r *http.Request) { blobs := buh.Repository.Blobs(buh) upload, err := blobs.Create(buh) + if err != nil { - buh.Errors = append(buh.Errors, errcode.ErrorCodeUnknown.WithDetail(err)) + if err == distribution.ErrUnsupported { + buh.Errors = append(buh.Errors, errcode.ErrorCodeUnsupported) + } else { + buh.Errors = append(buh.Errors, errcode.ErrorCodeUnknown.WithDetail(err)) + } return } @@ -227,6 +232,8 @@ func (buh *blobUploadHandler) PutBlobUploadComplete(w http.ResponseWriter, r *ht buh.Errors = append(buh.Errors, v2.ErrorCodeDigestInvalid.WithDetail(err)) default: switch err { + case distribution.ErrUnsupported: + buh.Errors = append(buh.Errors, errcode.ErrorCodeUnsupported) case distribution.ErrBlobInvalidLength, distribution.ErrBlobDigestUnsupported: buh.Errors = append(buh.Errors, v2.ErrorCodeBlobUploadInvalid.WithDetail(err)) default: diff --git a/registry/handlers/images.go b/registry/handlers/images.go index f53543993..f4f0db890 100644 --- a/registry/handlers/images.go +++ b/registry/handlers/images.go @@ -154,6 +154,10 @@ func (imh *imageManifestHandler) PutImageManifest(w http.ResponseWriter, r *http if err := manifests.Put(&manifest); err != nil { // TODO(stevvooe): These error handling switches really need to be // handled by an app global mapper. + if err == distribution.ErrUnsupported { + imh.Errors = append(imh.Errors, errcode.ErrorCodeUnsupported) + return + } switch err := err.(type) { case distribution.ErrManifestVerification: for _, verificationError := range err { @@ -210,14 +214,12 @@ func (imh *imageManifestHandler) DeleteImageManifest(w http.ResponseWriter, r *h return case distribution.ErrBlobUnknown: imh.Errors = append(imh.Errors, v2.ErrorCodeManifestUnknown) - w.WriteHeader(http.StatusNotFound) return case distribution.ErrUnsupported: imh.Errors = append(imh.Errors, errcode.ErrorCodeUnsupported) - w.WriteHeader(http.StatusMethodNotAllowed) + return default: imh.Errors = append(imh.Errors, errcode.ErrorCodeUnknown) - w.WriteHeader(http.StatusBadRequest) return } } diff --git a/registry/proxy/proxymanifeststore.go b/registry/proxy/proxymanifeststore.go index 8921998a7..e314e84f1 100644 --- a/registry/proxy/proxymanifeststore.go +++ b/registry/proxy/proxymanifeststore.go @@ -7,7 +7,6 @@ import ( "github.com/docker/distribution/context" "github.com/docker/distribution/digest" "github.com/docker/distribution/manifest" - "github.com/docker/distribution/registry/api/errcode" "github.com/docker/distribution/registry/client" "github.com/docker/distribution/registry/proxy/scheduler" ) @@ -147,9 +146,9 @@ func manifestDigest(sm *manifest.SignedManifest) (digest.Digest, error) { } func (pms proxyManifestStore) Put(manifest *manifest.SignedManifest) error { - return errcode.ErrorCodeUnsupported + return distribution.ErrUnsupported } func (pms proxyManifestStore) Delete(dgst digest.Digest) error { - return errcode.ErrorCodeUnsupported + return distribution.ErrUnsupported }