From 305801f58f22c9a26bc05eaa800e399afb212fe3 Mon Sep 17 00:00:00 2001 From: Jake Sanders Date: Tue, 14 Nov 2017 16:06:17 -0800 Subject: [PATCH] Disambiguate mirror -> other endpoint fallbacks from V2 -> V1 Signed-off-by: Jake Sanders --- distribution/errors.go | 14 +++--- distribution/errors_test.go | 85 +++++++++++++++++++++++++++++++++++++ distribution/pull_v2.go | 2 +- distribution/push_v2.go | 2 +- 4 files changed, 96 insertions(+), 7 deletions(-) create mode 100644 distribution/errors_test.go diff --git a/distribution/errors.go b/distribution/errors.go index dd6ff0a9aa..355e9da1bd 100644 --- a/distribution/errors.go +++ b/distribution/errors.go @@ -126,21 +126,25 @@ func TranslatePullError(err error, ref reference.Named) error { // continueOnError returns true if we should fallback to the next endpoint // as a result of this error. -func continueOnError(err error) bool { +func continueOnError(err error, mirrorEndpoint bool) bool { switch v := err.(type) { case errcode.Errors: if len(v) == 0 { return true } - return continueOnError(v[0]) + return continueOnError(v[0], mirrorEndpoint) case ErrNoSupport: - return continueOnError(v.Err) + return continueOnError(v.Err, mirrorEndpoint) case errcode.Error: - return shouldV2Fallback(v) + return mirrorEndpoint || shouldV2Fallback(v) case *client.UnexpectedHTTPResponseError: return true case ImageConfigPullError: - return false + // ImageConfigPullError only happens with v2 images, v1 fallback is + // unnecessary. + // Failures from a mirror endpoint should result in fallback to the + // canonical repo. + return mirrorEndpoint case error: return !strings.Contains(err.Error(), strings.ToLower(syscall.ESRCH.Error())) } diff --git a/distribution/errors_test.go b/distribution/errors_test.go new file mode 100644 index 0000000000..09f7936421 --- /dev/null +++ b/distribution/errors_test.go @@ -0,0 +1,85 @@ +package distribution + +import ( + "errors" + "strings" + "syscall" + "testing" + + "github.com/docker/distribution/registry/api/errcode" + "github.com/docker/distribution/registry/api/v2" + "github.com/docker/distribution/registry/client" +) + +var always_continue = []error{ + &client.UnexpectedHTTPResponseError{}, + + // Some errcode.Errors that don't disprove the existence of a V1 image + errcode.Error{Code: errcode.ErrorCodeUnauthorized}, + errcode.Error{Code: v2.ErrorCodeManifestUnknown}, + errcode.Error{Code: v2.ErrorCodeNameUnknown}, + + errors.New("some totally unexpected error"), +} + +var continue_from_mirror_endpoint = []error{ + ImageConfigPullError{}, + + // Some other errcode.Error that doesn't indicate we should search for a V1 image. + errcode.Error{Code: errcode.ErrorCodeTooManyRequests}, +} + +var never_continue = []error{ + errors.New(strings.ToLower(syscall.ESRCH.Error())), // No such process +} + +func TestContinueOnError_NonMirrorEndpoint(t *testing.T) { + for _, err := range always_continue { + if !continueOnError(err, false) { + t.Errorf("Should continue from non-mirror endpoint: %T: '%s'", err, err.Error()) + } + } + + for _, err := range continue_from_mirror_endpoint { + if continueOnError(err, false) { + t.Errorf("Should only continue from mirror endpoint: %T: '%s'", err, err.Error()) + } + } +} + +func TestContinueOnError_MirrorEndpoint(t *testing.T) { + errs := []error{} + errs = append(errs, always_continue...) + errs = append(errs, continue_from_mirror_endpoint...) + for _, err := range errs { + if !continueOnError(err, true) { + t.Errorf("Should continue from mirror endpoint: %T: '%s'", err, err.Error()) + } + } +} + +func TestContinueOnError_NeverContinue(t *testing.T) { + for _, is_mirror_endpoint := range []bool{true, false} { + for _, err := range never_continue { + if continueOnError(err, is_mirror_endpoint) { + t.Errorf("Should never continue: %T: '%s'", err, err.Error()) + } + } + } +} + +func TestContinueOnError_UnnestsErrors(t *testing.T) { + // ContinueOnError should evaluate nested errcode.Errors. + + // Assumes that v2.ErrorCodeNameUnknown is a continueable error code. + err := errcode.Errors{errcode.Error{Code: v2.ErrorCodeNameUnknown}} + if !continueOnError(err, false) { + t.Fatal("ContinueOnError should unnest, base return value on errcode.Errors") + } + + // Assumes that errcode.ErrorCodeTooManyRequests is not a V1-fallback indication + err = errcode.Errors{errcode.Error{Code: errcode.ErrorCodeTooManyRequests}} + if continueOnError(err, false) { + t.Fatal("ContinueOnError should unnest, base return value on errcode.Errors") + } +} diff --git a/distribution/pull_v2.go b/distribution/pull_v2.go index c8d784ca63..35ff529b4f 100644 --- a/distribution/pull_v2.go +++ b/distribution/pull_v2.go @@ -74,7 +74,7 @@ func (p *v2Puller) Pull(ctx context.Context, ref reference.Named, platform strin if _, ok := err.(fallbackError); ok { return err } - if continueOnError(err) { + if continueOnError(err, p.endpoint.Mirror) { return fallbackError{ err: err, confirmedV2: p.confirmedV2, diff --git a/distribution/push_v2.go b/distribution/push_v2.go index 7ffce5b2af..2aecc183b0 100644 --- a/distribution/push_v2.go +++ b/distribution/push_v2.go @@ -67,7 +67,7 @@ func (p *v2Pusher) Push(ctx context.Context) (err error) { } if err = p.pushV2Repository(ctx); err != nil { - if continueOnError(err) { + if continueOnError(err, p.endpoint.Mirror) { return fallbackError{ err: err, confirmedV2: p.pushState.confirmedV2,