Remove global DefaultRegistryClient #13356
Conversation
| accessController, err := newAccessController(options) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| ctx := context.WithValue(context.Background(), "http.request", req) |
There was a problem hiding this comment.
Please, use github.com/docker/distribution/context.WithRequest
pkg/dockerregistry/server/context.go
Outdated
|
|
||
| type contextKey string | ||
|
|
||
| var registryClientKey contextKey = "registryClient" |
There was a problem hiding this comment.
Historically, these things are stored in the util.go. Please either move context variables from util.go there or move this variables there.
pkg/dockerregistry/server/auth.go
Outdated
| @@ -201,6 +196,11 @@ func TokenRealm(options map[string]interface{}) (*url.URL, error) { | |||
| func newAccessController(options map[string]interface{}) (registryauth.AccessController, error) { | |||
| log.Info("Using Origin Auth handler") | |||
|
|
|||
| ctx, ok := options["_context"].(context.Context) | |||
There was a problem hiding this comment.
Can you write a comment why we can't use parent context ? It will help us not to forget that it cannot be used.
There was a problem hiding this comment.
There is no parent context.
There was a problem hiding this comment.
Ooops. Sorry. I looked at WithUserClient, but was thinking about this change :)
pkg/dockerregistry/server/auth.go
Outdated
| @@ -201,6 +196,11 @@ func TokenRealm(options map[string]interface{}) (*url.URL, error) { | |||
| func newAccessController(options map[string]interface{}) (registryauth.AccessController, error) { | |||
| log.Info("Using Origin Auth handler") | |||
|
|
|||
| ctx, ok := options["_context"].(context.Context) | |||
There was a problem hiding this comment.
Add constant for _context.
|
[test] |
|
@legionus all comments addressed. |
pkg/dockerregistry/server/auth.go
Outdated
| @@ -45,6 +45,8 @@ const ( | |||
|
|
|||
| RealmKey = "realm" | |||
| TokenRealmKey = "tokenrealm" | |||
|
|
|||
| accessControllerOptionContext = "_context" | |||
There was a problem hiding this comment.
Make it public and use it in pkg/cmd/dockerregistry/dockerregistry.go
Godoc please :)
|
Flake #12784 |
pkg/dockerregistry/server/auth.go
Outdated
| @@ -201,6 +196,11 @@ func TokenRealm(options map[string]interface{}) (*url.URL, error) { | |||
| func newAccessController(options map[string]interface{}) (registryauth.AccessController, error) { | |||
| log.Info("Using Origin Auth handler") | |||
|
|
|||
| ctx, ok := options["_context"].(context.Context) | |||
There was a problem hiding this comment.
Why hiding the client into a context. Can the registry client be passed directly in an option?
There was a problem hiding this comment.
I should've replace log.Info with context.GetLogger(ctx), but I forged to do that. And I think it's convenient to have the application context in all hooks, not only in the middlewares.
There was a problem hiding this comment.
I don't think the context makes sense in functions where you don't deal with a request that can be canceled or measured.
The newAccessController is invoked just once for the lifetime of the registry. I don't see a value in passing a context to the access controller upon init. Especially when all the other requests it will deal with will have context of their own - and completely different.
There was a problem hiding this comment.
I've replaced Context by AccessControllerParams.
|
|
|
Flake #13385 |
pkg/dockerregistry/server/context.go
Outdated
| // WithRemoteBlobAccessCheckEnabled returns a new Context that allows | ||
| // blobDescriptorService to stat remote blobs. It is useful only in case | ||
| // of manifest verification. | ||
| func WithRemoteBlobAccessCheckEnabled(parent context.Context, enable bool) context.Context { |
There was a problem hiding this comment.
Does it make sense to export these if others are private?
pkg/dockerregistry/server/context.go
Outdated
| return context.WithValue(ctx, registryClientKey, client) | ||
| } | ||
|
|
||
| // RegistryClientFrom returns the registry client stored in ctx, if present. |
| spec := search[repo] | ||
| spec, ok := search[repo] | ||
| if !ok { | ||
| continue |
There was a problem hiding this comment.
Did this crash for you? It would indicate an error in identifyCandidateRepositories.
There was a problem hiding this comment.
Yes, it crashed. It was removed by delete call few lines before because there was too many open files in proxyStat handler.
|
Flake #12784 |
Signed-off-by: Oleg Bulatov <[email protected]>
|
Evaluated for origin test up to a7ca6e1 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/225/) (Base Commit: 0009bfa) |
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/225/) (Base Commit: 0009bfa) (Image: devenv-rhel7_6070) |
|
Evaluated for origin merge up to a7ca6e1 |
Global variables really complicate tests and make them impossible to run concurrently. So I've tried to remove one of a such variable.