From 18c554ee50d89ef03d9fbbb0768c5fdf76c5753e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Min=C3=A1=C5=99?= Date: Wed, 17 Aug 2016 15:49:45 +0200 Subject: [PATCH] registry: Suppress unnecessary unauthorized errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit During a cross-repo mount request, it's quite common to forbid access to a source repository picked by Docker daemon regardless of user having access there. This produces error messages that appear severe but they're not. This patch delays their logging until the very last repository access check and lowers their severity. Signed-off-by: Michal Minář --- pkg/dockerregistry/server/auth.go | 160 ++++++++++++------ pkg/dockerregistry/server/auth_test.go | 68 +++++--- pkg/dockerregistry/server/errorblobstore.go | 21 +-- .../server/errormanifestservice.go | 8 +- pkg/dockerregistry/server/errortagservice.go | 10 +- .../server/repositorymiddleware.go | 23 --- .../server/repositorymiddleware_test.go | 4 +- 7 files changed, 180 insertions(+), 114 deletions(-) diff --git a/pkg/dockerregistry/server/auth.go b/pkg/dockerregistry/server/auth.go index 53c14c47c411..d58f596eab11 100644 --- a/pkg/dockerregistry/server/auth.go +++ b/pkg/dockerregistry/server/auth.go @@ -40,6 +40,7 @@ func (d deferredErrors) Empty() bool { const ( OpenShiftAuth = "openshift" + defaultRealm = "origin" defaultTokenPath = "/openshift/token" defaultUserName = "anonymous" @@ -47,7 +48,7 @@ const ( TokenRealmKey = "tokenrealm" ) -// RegistryClient encapsulates getting access to the OpenShift API. +// RegistryClient encapsulates getting access to the master API. type RegistryClient interface { // Clients return the authenticated clients to use with the server. Clients() (client.Interface, kclientset.Interface, error) @@ -158,9 +159,9 @@ var _ registryauth.Challenge = &tokenAuthChallenge{} // Errors used and exported by this package. var ( // Challenging errors - ErrTokenRequired = errors.New("authorization header required") - ErrTokenInvalid = errors.New("failed to decode credentials") - ErrOpenShiftAccessDenied = errors.New("access denied") + ErrTokenRequired = errors.New("authorization header required") + ErrTokenInvalid = errors.New("failed to decode credentials") + ErrAccessDenied = errors.New("access denied") // Non-challenging errors ErrNamespaceRequired = errors.New("repository namespace required") @@ -168,6 +169,38 @@ var ( ErrUnsupportedResource = errors.New("unsupported resource") ) +// errAccessDenied is an internal error representing access denied resulting from a negotiation with master +// API. It needs to be wrapped using wrapErr() before passing up to request dispatcher to generate proper auth +// challenge error. +type errAccessDenied struct { + err error + reason string +} + +func (e *errAccessDenied) Error() string { + if e.err != nil { + return fmt.Sprintf("master API client error: %v", e.err) + } + if len(e.reason) > 0 { + return fmt.Sprintf("access denied: %s", e.reason) + } + return fmt.Sprintf("access denied") +} + +// apiError returns one of the exposed errors summarizing the one being wrapped that can be passed to +// dispatcher. +func (e *errAccessDenied) apiError() error { + return ErrAccessDenied +} + +func isErrAccessDenied(err error) bool { + if err == ErrAccessDenied { + return true + } + _, ok := err.(*errAccessDenied) + return ok +} + // TokenRealm returns the template URL to use as the token realm redirect. // An empty scheme/host in the returned URL means to match the scheme/host on incoming requests. func TokenRealm(options map[string]interface{}) (*url.URL, error) { @@ -201,7 +234,7 @@ func TokenRealm(options map[string]interface{}) (*url.URL, error) { func newAccessController(options map[string]interface{}) (registryauth.AccessController, error) { log.Info("Using Origin Auth handler") - realm, err := getStringOption("", RealmKey, "origin", options) + realm, err := getStringOption("", RealmKey, defaultRealm, options) if err != nil { return nil, err } @@ -269,37 +302,15 @@ func (ac *tokenAuthChallenge) SetHeaders(w http.ResponseWriter) { // wrapErr wraps errors related to authorization in an authChallenge error that will present a WWW-Authenticate challenge response func (ac *AccessController) wrapErr(ctx context.Context, err error) error { - switch err { - case ErrTokenRequired: - // Challenge for errors that involve missing tokens - if ac.tokenRealm == nil { - // Send the basic challenge if we don't have a place to redirect - return &authChallenge{realm: ac.realm, err: err} - } - - if len(ac.tokenRealm.Scheme) > 0 && len(ac.tokenRealm.Host) > 0 { - // Redirect to token auth if we've been given an absolute URL - return &tokenAuthChallenge{realm: ac.tokenRealm.String(), err: err} - } - - // Auto-detect scheme/host from request - req, reqErr := context.GetRequest(ctx) - if reqErr != nil { - return reqErr - } - scheme, host := httprequest.SchemeHost(req) - tokenRealmCopy := *ac.tokenRealm - if len(tokenRealmCopy.Scheme) == 0 { - tokenRealmCopy.Scheme = scheme - } - if len(tokenRealmCopy.Host) == 0 { - tokenRealmCopy.Host = host - } - return &tokenAuthChallenge{realm: tokenRealmCopy.String(), err: err} - case ErrTokenInvalid, ErrOpenShiftAccessDenied: + switch { + case err == ErrTokenRequired: + return wrapTokenRequiredError(ctx, ac.realm, ac.tokenRealm, err) + case err == ErrTokenInvalid: // Challenge for errors that involve tokens or access denied return &authChallenge{realm: ac.realm, err: err} - case ErrNamespaceRequired, ErrUnsupportedAction, ErrUnsupportedResource: + case isErrAccessDenied(err): + return &authChallenge{realm: ac.realm, err: ErrAccessDenied} + case err == ErrNamespaceRequired, err == ErrUnsupportedAction, err == ErrUnsupportedResource: // Malformed or unsupported request, no challenge return err default: @@ -308,6 +319,34 @@ func (ac *AccessController) wrapErr(ctx context.Context, err error) error { } } +func wrapTokenRequiredError(ctx context.Context, realm string, tokenRealm *url.URL, err error) error { + // Challenge for errors that involve missing tokens + if tokenRealm == nil { + // Send the basic challenge if we don't have a place to redirect + return &authChallenge{realm: realm, err: err} + } + + if len(tokenRealm.Scheme) > 0 && len(tokenRealm.Host) > 0 { + // Redirect to token auth if we've been given an absolute URL + return &tokenAuthChallenge{realm: tokenRealm.String(), err: err} + } + + // Auto-detect scheme/host from request + req, reqErr := context.GetRequest(ctx) + if reqErr != nil { + return reqErr + } + scheme, host := httprequest.SchemeHost(req) + tokenRealmCopy := *tokenRealm + if len(tokenRealmCopy.Scheme) == 0 { + tokenRealmCopy.Scheme = scheme + } + if len(tokenRealmCopy.Host) == 0 { + tokenRealmCopy.Host = host + } + return &tokenAuthChallenge{realm: tokenRealmCopy.String(), err: err} +} + // Authorized handles checking whether the given request is authorized // for actions on resources allowed by openshift. // Sources of access records: @@ -357,7 +396,7 @@ func (ac *AccessController) Authorized(ctx context.Context, accessRecords ...reg // Validate all requested accessRecords // Only return failure errors from this loop. Success should continue to validate all records for _, access := range accessRecords { - context.GetLogger(ctx).Debugf("Origin auth: checking for access to %s:%s:%s", access.Resource.Type, access.Resource.Name, access.Action) + context.GetLogger(ctx).Debugf("checking for access to %s:%s:%s", access.Resource.Type, access.Resource.Name, access.Action) switch access.Resource.Type { case "repository": @@ -385,15 +424,17 @@ func (ac *AccessController) Authorized(ctx context.Context, accessRecords ...reg continue } if err := verifyPruneAccess(ctx, osClient); err != nil { + context.GetLogger(ctx).Error(err.Error()) return nil, ac.wrapErr(ctx, err) } verifiedPrune = true default: if err := verifyImageStreamAccess(ctx, imageStreamNS, imageStreamName, verb, osClient); err != nil { if access.Action != "pull" { + context.GetLogger(ctx).Error(err.Error()) return nil, ac.wrapErr(ctx, err) } - possibleCrossMountErrors.Add(imageStreamNS, imageStreamName, ac.wrapErr(ctx, err)) + possibleCrossMountErrors.Add(imageStreamNS, imageStreamName, err) } } @@ -420,6 +461,7 @@ func (ac *AccessController) Authorized(ctx context.Context, accessRecords ...reg continue } if err := verifyPruneAccess(ctx, osClient); err != nil { + context.GetLogger(ctx).Error(err.Error()) return nil, ac.wrapErr(ctx, err) } verifiedPrune = true @@ -435,17 +477,21 @@ func (ac *AccessController) Authorized(ctx context.Context, accessRecords ...reg for namespaceAndName, err := range possibleCrossMountErrors { // If we have no push requests, this can't be a cross-mount request, so error if len(pushChecks) == 0 { - return nil, err + context.GetLogger(ctx).Error(err.Error()) + return nil, ac.wrapErr(ctx, err) } // If we also requested a push to this ns/name, this isn't a cross-mount request, so error if pushChecks[namespaceAndName] { - return nil, err + context.GetLogger(ctx).Error(err.Error()) + return nil, ac.wrapErr(ctx, err) } } // Conditionally add auth errors we want to handle later to the context if !possibleCrossMountErrors.Empty() { - context.GetLogger(ctx).Debugf("Origin auth: deferring errors: %#v", possibleCrossMountErrors) + for ns, err := range possibleCrossMountErrors { + context.GetLogger(ctx).Debugf("deferring error for namespace=%s: %s", ns, err.Error()) + } ctx = WithDeferredErrors(ctx, possibleCrossMountErrors) } // Always add a marker to the context so we know auth was run @@ -490,7 +536,7 @@ func verifyOpenShiftUser(ctx context.Context, client client.UsersInterface) (str if err != nil { context.GetLogger(ctx).Errorf("Get user failed with error: %s", err) if kerrors.IsUnauthorized(err) || kerrors.IsForbidden(err) { - return "", "", ErrOpenShiftAccessDenied + return "", "", ErrAccessDenied } return "", "", err } @@ -510,16 +556,14 @@ func verifyWithSAR(ctx context.Context, resource, namespace, name, verb string, response, err := client.LocalSubjectAccessReviews(namespace).Create(&sar) if err != nil { - context.GetLogger(ctx).Errorf("OpenShift client error: %s", err) if kerrors.IsUnauthorized(err) || kerrors.IsForbidden(err) { - return ErrOpenShiftAccessDenied + return &errAccessDenied{err: err} } return err } if !response.Allowed { - context.GetLogger(ctx).Errorf("OpenShift access denied: %s", response.Reason) - return ErrOpenShiftAccessDenied + return &errAccessDenied{reason: response.Reason} } return nil @@ -543,15 +587,35 @@ func verifyPruneAccess(ctx context.Context, client client.SubjectAccessReviews) } response, err := client.SubjectAccessReviews().Create(&sar) if err != nil { - context.GetLogger(ctx).Errorf("OpenShift client error: %s", err) if kerrors.IsUnauthorized(err) || kerrors.IsForbidden(err) { - return ErrOpenShiftAccessDenied + return &errAccessDenied{err: err} } return err } if !response.Allowed { - context.GetLogger(ctx).Errorf("OpenShift access denied: %s", response.Reason) - return ErrOpenShiftAccessDenied + return &errAccessDenied{reason: response.Reason} } return nil } + +func checkPendingErrors(ctx context.Context, namespace, name string) error { + if !AuthPerformed(ctx) { + return fmt.Errorf("openshift.auth.completed missing from context") + } + + deferredErrors, haveDeferredErrors := DeferredErrorsFrom(ctx) + if !haveDeferredErrors { + return nil + } + + repoErr, haveRepoErr := deferredErrors.Get(namespace, name) + if !haveRepoErr { + return nil + } + + context.GetLogger(ctx).Debugf("found deferred error for %s/%s: %s", namespace, name, repoErr.Error()) + if accessDenied, ok := repoErr.(*errAccessDenied); ok { + return accessDenied.apiError() + } + return repoErr +} diff --git a/pkg/dockerregistry/server/auth_test.go b/pkg/dockerregistry/server/auth_test.go index e220e8d585b8..9b6ec9f390e4 100644 --- a/pkg/dockerregistry/server/auth_test.go +++ b/pkg/dockerregistry/server/auth_test.go @@ -13,9 +13,11 @@ import ( "github.com/docker/distribution/registry/auth" kapi "k8s.io/kubernetes/pkg/api" + kerrors "k8s.io/kubernetes/pkg/api/errors" "k8s.io/kubernetes/pkg/apimachinery/registered" "k8s.io/kubernetes/pkg/client/restclient" "k8s.io/kubernetes/pkg/runtime" + "k8s.io/kubernetes/pkg/util/diff" "github.com/docker/distribution/context" "github.com/openshift/origin/pkg/api/latest" @@ -32,15 +34,27 @@ import ( // tests invalid/valid/scoped openshift tokens. func TestVerifyImageStreamAccess(t *testing.T) { tests := []struct { + name string openshiftResponse response - expectedError error + matchError func(err error) error }{ { + name: "invalid openshift bearer token", // Test invalid openshift bearer token openshiftResponse: response{401, "Unauthorized"}, - expectedError: ErrOpenShiftAccessDenied, + matchError: func(err error) error { + accessDenied, isAccessDenied := err.(*errAccessDenied) + if !isAccessDenied { + return fmt.Errorf("expected errAccessDenied, not %T", accessDenied) + } + if !kerrors.IsUnauthorized(accessDenied.err) { + return fmt.Errorf("expected unauthorized, not: %#+v", err) + } + return nil + }, }, { + name: "token not scoped for create operation", // Test valid openshift bearer token but token *not* scoped for create operation openshiftResponse: response{ 200, @@ -50,9 +64,15 @@ func TestVerifyImageStreamAccess(t *testing.T) { Reason: "not authorized!", }), }, - expectedError: ErrOpenShiftAccessDenied, + matchError: func(err error) error { + if !reflect.DeepEqual(&errAccessDenied{reason: "not authorized!"}, err) { + return fmt.Errorf("got unexpected error: %s", diff.ObjectGoPrintDiff(&errAccessDenied{reason: "not authorized!"}, err)) + } + return nil + }, }, { + name: "valid openshift bearer token", // Test valid openshift bearer token and token scoped for create operation openshiftResponse: response{ 200, @@ -62,25 +82,29 @@ func TestVerifyImageStreamAccess(t *testing.T) { Reason: "authorized!", }), }, - expectedError: nil, }, } for _, test := range tests { - ctx := context.Background() - server, _ := simulateOpenShiftMaster([]response{test.openshiftResponse}) - client, err := client.New(&restclient.Config{BearerToken: "magic bearer token", Host: server.URL}) - if err != nil { - t.Fatal(err) - } - err = verifyImageStreamAccess(ctx, "foo", "bar", "create", client) - if err == nil || test.expectedError == nil { - if err != test.expectedError { - t.Fatalf("verifyImageStreamAccess did not get expected error - got %s - expected %s", err, test.expectedError) + func() { + ctx := context.Background() + server, _ := simulateOpenShiftMaster([]response{test.openshiftResponse}) + defer server.Close() + + client, err := client.New(&restclient.Config{BearerToken: "magic bearer token", Host: server.URL}) + if err != nil { + t.Errorf("[%s] unexpected error: %v", test.name, err) + return } - } else if err.Error() != test.expectedError.Error() { - t.Fatalf("verifyImageStreamAccess did not get expected error - got %s - expected %s", err, test.expectedError) - } - server.Close() + err = verifyImageStreamAccess(ctx, "foo", "bar", "create", client) + if test.matchError == nil { + if err != nil { + t.Errorf("[%s] got unexpected error: %#+v", test.name, err) + return + } + } else if matchErr := test.matchError(err); matchErr != nil { + t.Errorf("[%s] %v", test.name, err) + } + }() } } @@ -194,7 +218,7 @@ func TestAccessController(t *testing.T) { "docker login with invalid openshift creds": { basicToken: "b3BlbnNoaWZ0OmF3ZXNvbWU=", openshiftResponses: []response{{403, ""}}, - expectedError: ErrOpenShiftAccessDenied, + expectedError: ErrAccessDenied, expectedChallenge: true, expectedHeaders: http.Header{"Www-Authenticate": []string{`Basic realm=myrealm,error="access denied"`}}, expectedActions: []string{"GET /oapi/v1/users/~ (Authorization=Bearer awesome)"}, @@ -241,7 +265,7 @@ func TestAccessController(t *testing.T) { {200, runtime.EncodeOrDie(kapi.Codecs.LegacyCodec(registered.GroupOrDie(kapi.GroupName).GroupVersions[0]), &userapi.User{ObjectMeta: kapi.ObjectMeta{Name: "usr1"}})}, {200, runtime.EncodeOrDie(kapi.Codecs.LegacyCodec(registered.GroupOrDie(kapi.GroupName).GroupVersions[0]), &api.SubjectAccessReviewResponse{Namespace: "foo", Allowed: false, Reason: "unauthorized!"})}, }, - expectedError: ErrOpenShiftAccessDenied, + expectedError: ErrAccessDenied, expectedChallenge: true, expectedHeaders: http.Header{"Www-Authenticate": []string{`Basic realm=myrealm,error="access denied"`}}, expectedActions: []string{ @@ -265,7 +289,7 @@ func TestAccessController(t *testing.T) { {200, runtime.EncodeOrDie(kapi.Codecs.LegacyCodec(registered.GroupOrDie(kapi.GroupName).GroupVersions[0]), &api.SubjectAccessReviewResponse{Namespace: "", Allowed: true, Reason: "authorized!"})}, {200, runtime.EncodeOrDie(kapi.Codecs.LegacyCodec(registered.GroupOrDie(kapi.GroupName).GroupVersions[0]), &api.SubjectAccessReviewResponse{Namespace: "baz", Allowed: false, Reason: "no!"})}, }, - expectedError: ErrOpenShiftAccessDenied, + expectedError: ErrAccessDenied, expectedChallenge: true, expectedHeaders: http.Header{"Www-Authenticate": []string{`Basic realm=myrealm,error="access denied"`}}, expectedActions: []string{ @@ -445,7 +469,7 @@ func TestAccessController(t *testing.T) { } else { challengeErr, isChallenge := err.(auth.Challenge) if test.expectedChallenge != isChallenge { - t.Errorf("%s: expected challenge=%v, accessController returned challenge=%v", k, test.expectedChallenge, isChallenge) + t.Errorf("%s: expected challenge=%v, accessController returned challenge=%v, error: %#+v", k, test.expectedChallenge, isChallenge, err) continue } if isChallenge { diff --git a/pkg/dockerregistry/server/errorblobstore.go b/pkg/dockerregistry/server/errorblobstore.go index dfee01828c07..27e8c83a909f 100644 --- a/pkg/dockerregistry/server/errorblobstore.go +++ b/pkg/dockerregistry/server/errorblobstore.go @@ -20,35 +20,35 @@ type errorBlobStore struct { var _ distribution.BlobStore = &errorBlobStore{} func (r *errorBlobStore) Stat(ctx context.Context, dgst digest.Digest) (distribution.Descriptor, error) { - if err := r.repo.checkPendingErrors(ctx); err != nil { + if err := checkPendingErrors(ctx, r.repo.namespace, r.repo.name); err != nil { return distribution.Descriptor{}, err } return r.store.Stat(WithRepository(ctx, r.repo), dgst) } func (r *errorBlobStore) Get(ctx context.Context, dgst digest.Digest) ([]byte, error) { - if err := r.repo.checkPendingErrors(ctx); err != nil { + if err := checkPendingErrors(ctx, r.repo.namespace, r.repo.name); err != nil { return nil, err } return r.store.Get(WithRepository(ctx, r.repo), dgst) } func (r *errorBlobStore) Open(ctx context.Context, dgst digest.Digest) (distribution.ReadSeekCloser, error) { - if err := r.repo.checkPendingErrors(ctx); err != nil { + if err := checkPendingErrors(ctx, r.repo.namespace, r.repo.name); err != nil { return nil, err } return r.store.Open(WithRepository(ctx, r.repo), dgst) } func (r *errorBlobStore) Put(ctx context.Context, mediaType string, p []byte) (distribution.Descriptor, error) { - if err := r.repo.checkPendingErrors(ctx); err != nil { + if err := checkPendingErrors(ctx, r.repo.namespace, r.repo.name); err != nil { return distribution.Descriptor{}, err } return r.store.Put(WithRepository(ctx, r.repo), mediaType, p) } func (r *errorBlobStore) Create(ctx context.Context, options ...distribution.BlobCreateOption) (distribution.BlobWriter, error) { - if err := r.repo.checkPendingErrors(ctx); err != nil { + if err := checkPendingErrors(ctx, r.repo.namespace, r.repo.name); err != nil { return nil, err } @@ -77,21 +77,21 @@ func (r *errorBlobStore) Create(ctx context.Context, options ...distribution.Blo } func (r *errorBlobStore) Resume(ctx context.Context, id string) (distribution.BlobWriter, error) { - if err := r.repo.checkPendingErrors(ctx); err != nil { + if err := checkPendingErrors(ctx, r.repo.namespace, r.repo.name); err != nil { return nil, err } return r.store.Resume(WithRepository(ctx, r.repo), id) } func (r *errorBlobStore) ServeBlob(ctx context.Context, w http.ResponseWriter, req *http.Request, dgst digest.Digest) error { - if err := r.repo.checkPendingErrors(ctx); err != nil { + if err := checkPendingErrors(ctx, r.repo.namespace, r.repo.name); err != nil { return err } return r.store.ServeBlob(WithRepository(ctx, r.repo), w, req, dgst) } func (r *errorBlobStore) Delete(ctx context.Context, dgst digest.Digest) error { - if err := r.repo.checkPendingErrors(ctx); err != nil { + if err := checkPendingErrors(ctx, r.repo.namespace, r.repo.name); err != nil { return err } return r.store.Delete(WithRepository(ctx, r.repo), dgst) @@ -108,7 +108,7 @@ func checkPendingCrossMountErrors(ctx context.Context, opts *distribution.Create if err != nil { return err } - return checkPendingErrors(context.GetLogger(ctx), ctx, namespace, name) + return checkPendingErrors(ctx, namespace, name) } // guardCreateOptions ensures the expected options type is passed, and optionally disables cross mounting @@ -151,7 +151,8 @@ func (f statCrossMountCreateOptions) Apply(v interface{}) error { if err != nil { context.GetLogger(f.ctx).Infof("cannot mount blob %s from repository %s: %v - disabling cross-repo mount", opts.Mount.From.Digest().String(), - opts.Mount.From.Name()) + opts.Mount.From.Name(), + err) opts.Mount.ShouldMount = false return nil } diff --git a/pkg/dockerregistry/server/errormanifestservice.go b/pkg/dockerregistry/server/errormanifestservice.go index 486333981604..2f1b7c917884 100644 --- a/pkg/dockerregistry/server/errormanifestservice.go +++ b/pkg/dockerregistry/server/errormanifestservice.go @@ -16,28 +16,28 @@ type errorManifestService struct { var _ distribution.ManifestService = &errorManifestService{} func (em *errorManifestService) Exists(ctx context.Context, dgst digest.Digest) (bool, error) { - if err := em.repo.checkPendingErrors(ctx); err != nil { + if err := checkPendingErrors(ctx, em.repo.namespace, em.repo.name); err != nil { return false, err } return em.manifests.Exists(ctx, dgst) } func (em *errorManifestService) Get(ctx context.Context, dgst digest.Digest, options ...distribution.ManifestServiceOption) (distribution.Manifest, error) { - if err := em.repo.checkPendingErrors(ctx); err != nil { + if err := checkPendingErrors(ctx, em.repo.namespace, em.repo.name); err != nil { return nil, err } return em.manifests.Get(ctx, dgst, options...) } func (em *errorManifestService) Put(ctx context.Context, manifest distribution.Manifest, options ...distribution.ManifestServiceOption) (digest.Digest, error) { - if err := em.repo.checkPendingErrors(ctx); err != nil { + if err := checkPendingErrors(ctx, em.repo.namespace, em.repo.name); err != nil { return "", err } return em.manifests.Put(ctx, manifest, options...) } func (em *errorManifestService) Delete(ctx context.Context, dgst digest.Digest) error { - if err := em.repo.checkPendingErrors(ctx); err != nil { + if err := checkPendingErrors(ctx, em.repo.namespace, em.repo.name); err != nil { return err } return em.manifests.Delete(ctx, dgst) diff --git a/pkg/dockerregistry/server/errortagservice.go b/pkg/dockerregistry/server/errortagservice.go index 017e06dc4ca6..1ca19af6602b 100644 --- a/pkg/dockerregistry/server/errortagservice.go +++ b/pkg/dockerregistry/server/errortagservice.go @@ -15,35 +15,35 @@ type errorTagService struct { var _ distribution.TagService = &errorTagService{} func (t *errorTagService) Get(ctx context.Context, tag string) (distribution.Descriptor, error) { - if err := t.repo.checkPendingErrors(ctx); err != nil { + if err := checkPendingErrors(ctx, t.repo.namespace, t.repo.name); err != nil { return distribution.Descriptor{}, err } return t.tags.Get(ctx, tag) } func (t *errorTagService) Tag(ctx context.Context, tag string, desc distribution.Descriptor) error { - if err := t.repo.checkPendingErrors(ctx); err != nil { + if err := checkPendingErrors(ctx, t.repo.namespace, t.repo.name); err != nil { return err } return t.tags.Tag(ctx, tag, desc) } func (t *errorTagService) Untag(ctx context.Context, tag string) error { - if err := t.repo.checkPendingErrors(ctx); err != nil { + if err := checkPendingErrors(ctx, t.repo.namespace, t.repo.name); err != nil { return err } return t.tags.Untag(ctx, tag) } func (t *errorTagService) All(ctx context.Context) ([]string, error) { - if err := t.repo.checkPendingErrors(ctx); err != nil { + if err := checkPendingErrors(ctx, t.repo.namespace, t.repo.name); err != nil { return nil, err } return t.tags.All(ctx) } func (t *errorTagService) Lookup(ctx context.Context, digest distribution.Descriptor) ([]string, error) { - if err := t.repo.checkPendingErrors(ctx); err != nil { + if err := checkPendingErrors(ctx, t.repo.namespace, t.repo.name); err != nil { return nil, err } return t.tags.Lookup(ctx, digest) diff --git a/pkg/dockerregistry/server/repositorymiddleware.go b/pkg/dockerregistry/server/repositorymiddleware.go index 8801c4f0a039..9d58db021dd1 100644 --- a/pkg/dockerregistry/server/repositorymiddleware.go +++ b/pkg/dockerregistry/server/repositorymiddleware.go @@ -390,26 +390,3 @@ func (r *repository) manifestFromImageWithCachedLayers(image *imageapi.Image, ca r.rememberLayersOfManifest(dgst, manifest, cacheName) return } - -func (r *repository) checkPendingErrors(ctx context.Context) error { - return checkPendingErrors(context.GetLogger(r.ctx), ctx, r.namespace, r.name) -} - -func checkPendingErrors(logger context.Logger, ctx context.Context, namespace, name string) error { - if !AuthPerformed(ctx) { - return fmt.Errorf("openshift.auth.completed missing from context") - } - - deferredErrors, haveDeferredErrors := DeferredErrorsFrom(ctx) - if !haveDeferredErrors { - return nil - } - - repoErr, haveRepoErr := deferredErrors.Get(namespace, name) - if !haveRepoErr { - return nil - } - - logger.Debugf("Origin auth: found deferred error for %s/%s: %v", namespace, name, repoErr) - return repoErr -} diff --git a/pkg/dockerregistry/server/repositorymiddleware_test.go b/pkg/dockerregistry/server/repositorymiddleware_test.go index 1486c7e41a63..53fcd2fb85f1 100644 --- a/pkg/dockerregistry/server/repositorymiddleware_test.go +++ b/pkg/dockerregistry/server/repositorymiddleware_test.go @@ -258,8 +258,8 @@ func TestRepositoryBlobStat(t *testing.T) { name: "deferred error", stat: "nm/is@" + testImages["nm/is:latest"][0].DockerImageLayers[0].Name, imageStreams: []imageapi.ImageStream{{ObjectMeta: kapi.ObjectMeta{Namespace: "nm", Name: "is"}}}, - deferredErrors: deferredErrors{"nm/is": ErrOpenShiftAccessDenied}, - expectedError: ErrOpenShiftAccessDenied, + deferredErrors: deferredErrors{"nm/is": ErrAccessDenied}, + expectedError: ErrAccessDenied, }, } { ref, err := reference.Parse(tc.stat)