From 19a93a6e3d4213c56583bb0c843cf9e33d379752 Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Thu, 10 Nov 2016 15:14:33 -0800 Subject: [PATCH] Update pull error handling Translate pull errors to provide a more consistent and user friendly error message. Signed-off-by: Derek McGowan (github: dmcgowan) --- distribution/errors.go | 34 ++++++++++++++++++++ distribution/pull.go | 4 +-- integration-cli/docker_cli_by_digest_test.go | 2 +- integration-cli/docker_cli_pull_test.go | 23 +++++-------- 4 files changed, 45 insertions(+), 18 deletions(-) diff --git a/distribution/errors.go b/distribution/errors.go index 1e7630e380..ed8c3c0534 100644 --- a/distribution/errors.go +++ b/distribution/errors.go @@ -5,11 +5,14 @@ import ( "strings" "syscall" + "github.com/Sirupsen/logrus" "github.com/docker/distribution/registry/api/errcode" "github.com/docker/distribution/registry/api/v2" "github.com/docker/distribution/registry/client" "github.com/docker/distribution/registry/client/auth" "github.com/docker/docker/distribution/xfer" + "github.com/docker/docker/reference" + "github.com/pkg/errors" ) // ErrNoSupport is an error type used for errors indicating that an operation @@ -56,6 +59,37 @@ func shouldV2Fallback(err errcode.Error) bool { return false } +func translatePullError(err error, ref reference.Named) error { + switch v := err.(type) { + case errcode.Errors: + if len(v) != 0 { + for _, extra := range v[1:] { + logrus.Infof("Ignoring extra error returned from registry: %v", extra) + } + return translatePullError(v[0], ref) + } + case errcode.Error: + var newErr error + switch v.Code { + case errcode.ErrorCodeDenied: + // ErrorCodeDenied is used when access to the repository was denied + newErr = errors.Errorf("repository %s not found: does not exist or no read access", ref.Name()) + case v2.ErrorCodeManifestUnknown: + newErr = errors.Errorf("manifest for %s not found", ref.String()) + case v2.ErrorCodeNameUnknown: + newErr = errors.Errorf("repository %s not found", ref.Name()) + } + if newErr != nil { + logrus.Infof("Translating %q to %q", err, newErr) + return newErr + } + case xfer.DoNotRetry: + return translatePullError(v.Err, ref) + } + + return err +} + // continueOnError returns true if we should fallback to the next endpoint // as a result of this error. func continueOnError(err error) bool { diff --git a/distribution/pull.go b/distribution/pull.go index 4fab438800..5307d4ccc9 100644 --- a/distribution/pull.go +++ b/distribution/pull.go @@ -168,7 +168,7 @@ func Pull(ctx context.Context, ref reference.Named, imagePullConfig *ImagePullCo continue } logrus.Errorf("Not continuing with pull after error: %v", err) - return err + return translatePullError(err, ref) } imagePullConfig.ImageEventLogger(ref.String(), repoInfo.Name(), "pull") @@ -179,7 +179,7 @@ func Pull(ctx context.Context, ref reference.Named, imagePullConfig *ImagePullCo lastErr = fmt.Errorf("no endpoints found for %s", ref.String()) } - return lastErr + return translatePullError(lastErr, ref) } // writeStatus writes a status message to out. If layersDownloaded is true, the diff --git a/integration-cli/docker_cli_by_digest_test.go b/integration-cli/docker_cli_by_digest_test.go index a04b04998d..c2d85461a8 100644 --- a/integration-cli/docker_cli_by_digest_test.go +++ b/integration-cli/docker_cli_by_digest_test.go @@ -114,7 +114,7 @@ func testPullByDigestNoFallback(c *check.C) { imageReference := fmt.Sprintf("%s@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", repoName) out, _, err := dockerCmdWithError("pull", imageReference) c.Assert(err, checker.NotNil, check.Commentf("expected non-zero exit status and correct error message when pulling non-existing image")) - c.Assert(out, checker.Contains, "manifest unknown", check.Commentf("expected non-zero exit status and correct error message when pulling non-existing image")) + c.Assert(out, checker.Contains, fmt.Sprintf("manifest for %s not found", imageReference), check.Commentf("expected non-zero exit status and correct error message when pulling non-existing image")) } func (s *DockerRegistrySuite) TestPullByDigestNoFallback(c *check.C) { diff --git a/integration-cli/docker_cli_pull_test.go b/integration-cli/docker_cli_pull_test.go index 8a86cd39f6..c89adae0c6 100644 --- a/integration-cli/docker_cli_pull_test.go +++ b/integration-cli/docker_cli_pull_test.go @@ -48,12 +48,12 @@ func (s *DockerHubPullSuite) TestPullNonExistingImage(c *check.C) { } entries := []entry{ - {"library/asdfasdf", "asdfasdf", "foobar"}, - {"library/asdfasdf", "library/asdfasdf", "foobar"}, - {"library/asdfasdf", "asdfasdf", ""}, - {"library/asdfasdf", "asdfasdf", "latest"}, - {"library/asdfasdf", "library/asdfasdf", ""}, - {"library/asdfasdf", "library/asdfasdf", "latest"}, + {"asdfasdf", "asdfasdf", "foobar"}, + {"asdfasdf", "library/asdfasdf", "foobar"}, + {"asdfasdf", "asdfasdf", ""}, + {"asdfasdf", "asdfasdf", "latest"}, + {"asdfasdf", "library/asdfasdf", ""}, + {"asdfasdf", "library/asdfasdf", "latest"}, } // The option field indicates "-a" or not. @@ -98,18 +98,11 @@ func (s *DockerHubPullSuite) TestPullNonExistingImage(c *check.C) { for record := range recordChan { if len(record.option) == 0 { c.Assert(record.err, checker.NotNil, check.Commentf("expected non-zero exit status when pulling non-existing image: %s", record.out)) - // Hub returns 401 rather than 404 for nonexistent repos over - // the v2 protocol - but we should end up falling back to v1, - // which does return a 404. - tag := record.e.tag - if tag == "" { - tag = "latest" - } - c.Assert(record.out, checker.Contains, fmt.Sprintf("Error: image %s:%s not found", record.e.repo, tag), check.Commentf("expected image not found error messages")) + c.Assert(record.out, checker.Contains, fmt.Sprintf("repository %s not found: does not exist or no read access", record.e.repo), check.Commentf("expected image not found error messages")) } else { // pull -a on a nonexistent registry should fall back as well c.Assert(record.err, checker.NotNil, check.Commentf("expected non-zero exit status when pulling non-existing image: %s", record.out)) - c.Assert(record.out, checker.Contains, fmt.Sprintf("Error: image %s not found", record.e.repo), check.Commentf("expected image not found error messages")) + c.Assert(record.out, checker.Contains, fmt.Sprintf("repository %s not found", record.e.repo), check.Commentf("expected image not found error messages")) c.Assert(record.out, checker.Not(checker.Contains), "unauthorized", check.Commentf(`message should not contain "unauthorized"`)) } }