From 2e94c22fe4f33bc61fb119945a413b92cee0dba9 Mon Sep 17 00:00:00 2001 From: "Yu Wang (UC)" Date: Wed, 11 Jan 2017 12:10:16 -0800 Subject: [PATCH 1/2] issue#2135 image pull returns 404 on manifest request if there is storage error When get manifest, the handler will try to retrieve it from storage driver. When storage driver is cloud storage, it can fail due to various reasons even if the manifest exists (like 500, 503, etc. from storage server). Currently manifest handler blindly return 404 which can be confusing to user. This change will return 404 if the manifest blob doesn't exist, and return 500 UnknownError for all other errors (consistent with the behavior of other handlers). Signed-off-by: Yu Wang (UC) --- registry/handlers/manifests.go | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/registry/handlers/manifests.go b/registry/handlers/manifests.go index 971dca712..75e3d1dd4 100644 --- a/registry/handlers/manifests.go +++ b/registry/handlers/manifests.go @@ -77,7 +77,11 @@ func (imh *manifestHandler) GetManifest(w http.ResponseWriter, r *http.Request) tags := imh.Repository.Tags(imh) desc, err := tags.Get(imh, imh.Tag) if err != nil { - imh.Errors = append(imh.Errors, v2.ErrorCodeManifestUnknown.WithDetail(err)) + if _, ok := err.(distribution.ErrTagUnknown); ok { + imh.Errors = append(imh.Errors, v2.ErrorCodeManifestUnknown.WithDetail(err)) + } else { + imh.Errors = append(imh.Errors, errcode.ErrorCodeUnknown.WithDetail(err)) + } return } imh.Digest = desc.Digest @@ -94,7 +98,11 @@ func (imh *manifestHandler) GetManifest(w http.ResponseWriter, r *http.Request) } manifest, err = manifests.Get(imh, imh.Digest, options...) if err != nil { - imh.Errors = append(imh.Errors, v2.ErrorCodeManifestUnknown.WithDetail(err)) + if _, ok := err.(distribution.ErrManifestUnknownRevision); ok { + imh.Errors = append(imh.Errors, v2.ErrorCodeManifestUnknown.WithDetail(err)) + } else { + imh.Errors = append(imh.Errors, errcode.ErrorCodeUnknown.WithDetail(err)) + } return } @@ -161,7 +169,11 @@ func (imh *manifestHandler) GetManifest(w http.ResponseWriter, r *http.Request) manifest, err = manifests.Get(imh, manifestDigest) if err != nil { - imh.Errors = append(imh.Errors, v2.ErrorCodeManifestUnknown.WithDetail(err)) + if _, ok := err.(distribution.ErrManifestUnknownRevision); ok { + imh.Errors = append(imh.Errors, v2.ErrorCodeManifestUnknown.WithDetail(err)) + } else { + imh.Errors = append(imh.Errors, errcode.ErrorCodeUnknown.WithDetail(err)) + } return } @@ -191,7 +203,11 @@ func (imh *manifestHandler) convertSchema2Manifest(schema2Manifest *schema2.Dese blobs := imh.Repository.Blobs(imh) configJSON, err := blobs.Get(imh, targetDescriptor.Digest) if err != nil { - imh.Errors = append(imh.Errors, v2.ErrorCodeManifestInvalid.WithDetail(err)) + if err == distribution.ErrBlobUnknown { + imh.Errors = append(imh.Errors, v2.ErrorCodeManifestInvalid.WithDetail(err)) + } else { + imh.Errors = append(imh.Errors, errcode.ErrorCodeUnknown.WithDetail(err)) + } return nil, err } From 0bb696c5bf138ee8d40fa9053e89b0c67429fdfa Mon Sep 17 00:00:00 2001 From: yuwaMSFT2 Date: Fri, 13 Jan 2017 15:07:28 -0800 Subject: [PATCH 2/2] add test for manifest handler where storage driver fails Signed-off-by: Yu Wang (UC) --- registry/handlers/api_test.go | 110 ++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/registry/handlers/api_test.go b/registry/handlers/api_test.go index e1112fde5..e2598f130 100644 --- a/registry/handlers/api_test.go +++ b/registry/handlers/api_test.go @@ -3,6 +3,7 @@ package handlers import ( "bytes" "encoding/json" + "errors" "fmt" "io" "io/ioutil" @@ -28,6 +29,8 @@ import ( "github.com/docker/distribution/reference" "github.com/docker/distribution/registry/api/errcode" "github.com/docker/distribution/registry/api/v2" + storagedriver "github.com/docker/distribution/registry/storage/driver" + "github.com/docker/distribution/registry/storage/driver/factory" _ "github.com/docker/distribution/registry/storage/driver/testdriver" "github.com/docker/distribution/testutil" "github.com/docker/libtrust" @@ -815,6 +818,93 @@ func TestManifestAPI(t *testing.T) { testManifestAPIManifestList(t, env2, schema2Args) } +// storageManifestErrDriverFactory implements the factory.StorageDriverFactory interface. +type storageManifestErrDriverFactory struct{} + +const ( + repositoryWithManifestNotFound = "manifesttagnotfound" + repositoryWithManifestInvalidPath = "manifestinvalidpath" + repositoryWithManifestBadLink = "manifestbadlink" + repositoryWithGenericStorageError = "genericstorageerr" +) + +func (factory *storageManifestErrDriverFactory) Create(parameters map[string]interface{}) (storagedriver.StorageDriver, error) { + // Initialize the mock driver + var errGenericStorage = errors.New("generic storage error") + return &mockErrorDriver{ + returnErrs: []mockErrorMapping{ + { + pathMatch: fmt.Sprintf("%s/_manifests/tags", repositoryWithManifestNotFound), + content: nil, + err: storagedriver.PathNotFoundError{}, + }, + { + pathMatch: fmt.Sprintf("%s/_manifests/tags", repositoryWithManifestInvalidPath), + content: nil, + err: storagedriver.InvalidPathError{}, + }, + { + pathMatch: fmt.Sprintf("%s/_manifests/tags", repositoryWithManifestBadLink), + content: []byte("this is a bad sha"), + err: nil, + }, + { + pathMatch: fmt.Sprintf("%s/_manifests/tags", repositoryWithGenericStorageError), + content: nil, + err: errGenericStorage, + }, + }, + }, nil +} + +type mockErrorMapping struct { + pathMatch string + content []byte + err error +} + +// mockErrorDriver implements StorageDriver to force storage error on manifest request +type mockErrorDriver struct { + storagedriver.StorageDriver + returnErrs []mockErrorMapping +} + +func (dr *mockErrorDriver) GetContent(ctx context.Context, path string) ([]byte, error) { + for _, returns := range dr.returnErrs { + if strings.Contains(path, returns.pathMatch) { + return returns.content, returns.err + } + } + return nil, errors.New("Unknown storage error") +} + +func TestGetManifestWithStorageError(t *testing.T) { + factory.Register("storagemanifesterror", &storageManifestErrDriverFactory{}) + config := configuration.Configuration{ + Storage: configuration.Storage{ + "storagemanifesterror": configuration.Parameters{}, + "maintenance": configuration.Parameters{"uploadpurging": map[interface{}]interface{}{ + "enabled": false, + }}, + }, + } + config.HTTP.Headers = headerConfig + env1 := newTestEnvWithConfig(t, &config) + defer env1.Shutdown() + + repo, _ := reference.ParseNamed(repositoryWithManifestNotFound) + testManifestWithStorageError(t, env1, repo, http.StatusNotFound, v2.ErrorCodeManifestUnknown) + + repo, _ = reference.ParseNamed(repositoryWithGenericStorageError) + testManifestWithStorageError(t, env1, repo, http.StatusInternalServerError, errcode.ErrorCodeUnknown) + + repo, _ = reference.ParseNamed(repositoryWithManifestInvalidPath) + testManifestWithStorageError(t, env1, repo, http.StatusInternalServerError, errcode.ErrorCodeUnknown) + + repo, _ = reference.ParseNamed(repositoryWithManifestBadLink) + testManifestWithStorageError(t, env1, repo, http.StatusInternalServerError, errcode.ErrorCodeUnknown) +} + func TestManifestDelete(t *testing.T) { schema1Repo, _ := reference.ParseNamed("foo/schema1") schema2Repo, _ := reference.ParseNamed("foo/schema2") @@ -852,6 +942,26 @@ func testManifestDeleteDisabled(t *testing.T, env *testEnv, imageName reference. checkResponse(t, "status of disabled delete of manifest", resp, http.StatusMethodNotAllowed) } +func testManifestWithStorageError(t *testing.T, env *testEnv, imageName reference.Named, expectedStatusCode int, expectedErrorCode errcode.ErrorCode) { + tag := "latest" + tagRef, _ := reference.WithTag(imageName, tag) + manifestURL, err := env.builder.BuildManifestURL(tagRef) + if err != nil { + t.Fatalf("unexpected error getting manifest url: %v", err) + } + + // ----------------------------- + // Attempt to fetch the manifest + resp, err := http.Get(manifestURL) + if err != nil { + t.Fatalf("unexpected error getting manifest: %v", err) + } + defer resp.Body.Close() + checkResponse(t, "getting non-existent manifest", resp, expectedStatusCode) + checkBodyHasErrorCodes(t, "getting non-existent manifest", resp, expectedErrorCode) + return +} + func testManifestAPISchema1(t *testing.T, env *testEnv, imageName reference.Named) manifestArgs { tag := "thetag" args := manifestArgs{imageName: imageName}