Move filtered registry search out of the image service by corhere · Pull Request #45086 · moby/moby

corhere · 2023-03-01T00:41:10Z

Related to daemon: split image search to a separate service #44032

Move filtered registry search logic out of the image service as querying a remote registry is unrelated to the functionality of the image service. Move the logic into the registry service as it is the source of truth for the registry configuration, which makes it the most cohesive place to put it which does not require more extensive refactoring.

thaJeztah · 2023-03-01T11:39:13Z

I'm leaning towards keeping this more separate from the registry; "search" is a bit of an odd one, because search is not part of the OCI distribution specification.

Search was implemented in "docker registry v1" (the Docker Index (hence index.docker.io), which later became Docker Hub). When the registry V2 specification was created (which became the OCI Distribution Spec), search was deliberately left out of the implementation, because indexing and searching content was a separate concern, and it became clear that "what can be searched", "how" can be searched (and what properties to search for) could be very opinionated (different providers could have different things to search for).

So search did not become part of the OCI specs.

The search functionality in the daemon is still in place mostly for backward compatibility, and because the old (v1) search API is the only documented search API, so some other registry implementations also added support for it (by lack of other specifications for searching).

That said; some registries (including Docker Hub) provide other (more advanced) search functionality, and there's been some talk about defining a OCI spec for searching/indexing (not sure if that ever got actual progress though).

With the above in mind, I'd like to keep it separate with the potential to;

Allow other implementations to be added (haven't given this much thought, but somehow making it pluggable?)
(there's been various discussions about "better search" for images on Docker Hub at least; Docker Hub already has V2 and V3 search engines, which could provide some of that, but not sure if their API is publicly documented).
Probably for backward-compat, the "fallback" would be to use the V1 search

In either case; my thinking was to "try to keep it as isolated as possible"

corhere · 2023-03-01T14:37:24Z

I'm leaning towards keeping this more separate from the registry; "search" is a bit of an odd one, because search is not part of the OCI distribution specification.

The search interface is separated from the image distribution interface. The implementation is tied to the registry service, but that was already the case. Think of this PR as more of "getting search out of the wrong place to unblock containerd integration" than "getting search into the perfect place." We can still make #44032 happen; this PR just takes the pressure off.

rumpl

LGTM, a great first step, and love that it unblocks containerd a bit. Thanks

tianon

I agree that somewhere else is probably better, but also that "registry" seems like a more appropriate home than the image store for this Hub-specific behavior (given Hub is a registry, even if this isn't an OCI-specified endpoint). 😅

(In other words, this seems like a very small, easy net win on the path to that larger reconsideration ❤️)

neersighted

Looks like a sensible cleanup/separation of concerns to me, and eases the containerd integration work. Needs a rebase, but otherwise LGTM.

SearchRegistryForImages does not make sense as part of the image service interface. The implementation just wraps the search API of the registry service to filter the results client-side. It has nothing to do with local image storage, and the implementation of search does not need to change when changing which backend (graph driver vs. containerd snapshotter) is used for local image storage. Filtering of the search results is an implementation detail: the consumer of the results does not care which actor does the filtering so long as the results are filtered as requested. Move filtering into the exported API of the registry service to hide the implementation details. Only one thing---the registry service implementation---would need to change in order to support server-side filtering of search results if Docker Hub or other registry servers were to add support for it to their APIs. Use a fake registry server in the search unit tests to avoid having to mock out the registry API client. Signed-off-by: Cory Snider <[email protected]>

Move interface definitions to the packages which use the registry service. https://github.com/golang/go/wiki/CodeReviewComments#interfaces Signed-off-by: Cory Snider <[email protected]>

thaJeztah · 2023-03-15T14:17:01Z

registry/search.go

+
+	"github.com/docker/docker/api/types/filters"
+	"github.com/docker/docker/api/types/registry"
+	"github.com/docker/docker/dockerversion"


This import causes the CLI to also pull in docker/cli@a5d122d (commit vendoring is from a temporary branch, so link will likely be broken at some point);

github.com/docker/docker/dockerversion

-> github.com/docker/docker/pkg/useragent

-> github.com/docker/docker/pkg/parsers/kernel

-> golang.org/x/sys/windows/registry

We'll probably need to have a look at passing the dockerversion / dockerversion.DockerUserAgent(ctx) somehow

/cc @corhere @neersighted

Ah, shoot, interesting.

Yeah, TBH, the whole dockerversion.DockerUserAgent() (or more specifically, why it's added here?) is a bit weird. If I'm not mistaken, it takes the client's user-agent, and appends the "daemon" user-agent (so that both the "upstream" client (e.g. docker CLI), and the daemon are included.

Overall that seems fine, but definitely feels like something that should be set when constructing the service (or perhaps to be passed when calling it). Haven't looked how we handle this for the other "registry" cases (docker pull / docker push).

Quick search for that;

When Authenticating;

moby/daemon/auth.go

Lines 11 to 13 in a082bbc

func (daemon *Daemon) AuthenticateToRegistry(ctx context.Context, authConfig *registry.AuthConfig) (string, string, error) {

return daemon.registryService.Auth(ctx, authConfig, dockerversion.DockerUserAgent(ctx))

}

Distribution sets it up as part of the transport;

moby/distribution/registry.go

Lines 100 to 101 in a082bbc

modifiers := registry.Headers(dockerversion.DockerUserAgent(ctx), metaHeaders)

authTransport := transport.NewTransport(base, modifiers...)

And for plugins;

moby/plugin/registry.go

Lines 30 to 34 in a082bbc

func (pm *Manager) newResolver(ctx context.Context, tracker docker.StatusTracker, auth *registry.AuthConfig, headers http.Header, httpFallback bool) (remotes.Resolver, error) {

if headers == nil {

headers = http.Header{}

}

headers.Add("User-Agent", dockerversion.DockerUserAgent(ctx))

So looks like it's a bit "all over the place"

corhere added area/api API area/distribution Image Distribution status/2-code-review area/images Image Distribution kind/refactor PR's that refactor, or clean-up code containerd-integration Issues and PRs related to containerd integration labels Mar 1, 2023

corhere requested review from rumpl, thaJeztah, tianon and vvoland March 1, 2023 00:41

corhere changed the title ~~Move filtered registry search into registry service~~ Move filtered registry search out of the image service Mar 1, 2023

corhere force-pushed the search-in-registry-service branch from adfc66d to 55213f7 Compare March 1, 2023 22:42

rumpl approved these changes Mar 2, 2023

View reviewed changes

corhere requested a review from neersighted March 2, 2023 20:47

tianon approved these changes Mar 10, 2023

View reviewed changes

neersighted approved these changes Mar 10, 2023

View reviewed changes

neersighted added this to the v-next milestone Mar 10, 2023

corhere added 2 commits March 10, 2023 18:36

corhere force-pushed the search-in-registry-service branch from 55213f7 to 7b3acdf Compare March 10, 2023 23:38

vvoland approved these changes Mar 15, 2023

View reviewed changes

neersighted merged commit 1c84f63 into moby:master Mar 15, 2023

neersighted added status/4-merge and removed status/2-code-review labels Mar 15, 2023

thaJeztah reviewed Mar 15, 2023

View reviewed changes

thaJeztah mentioned this pull request Mar 15, 2023

vendor: update docker/docker to master (v24.0.0-dev) docker/cli#4010

Merged

corhere deleted the search-in-registry-service branch March 15, 2023 17:04

thaJeztah mentioned this pull request Mar 21, 2023

registry/search: pass User-Agent through headers #45185

Merged

This was referenced Mar 22, 2023

Remove registry Service use docker/cli#4111

Closed

cli/trust: remove special handling for "plugin" Class docker/cli#4114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move filtered registry search out of the image service#45086

Move filtered registry search out of the image service#45086
neersighted merged 2 commits intomoby:masterfrom
corhere:search-in-registry-service

corhere commented Mar 1, 2023 •

edited

Loading

Uh oh!

thaJeztah commented Mar 1, 2023

Uh oh!

corhere commented Mar 1, 2023

Uh oh!

rumpl left a comment

Uh oh!

tianon left a comment

Uh oh!

neersighted left a comment

Uh oh!

thaJeztah Mar 15, 2023

Uh oh!

neersighted Mar 15, 2023

Uh oh!

thaJeztah Mar 15, 2023

Uh oh!

thaJeztah Mar 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	func (daemon Daemon) AuthenticateToRegistry(ctx context.Context, authConfig registry.AuthConfig) (string, string, error) {
	return daemon.registryService.Auth(ctx, authConfig, dockerversion.DockerUserAgent(ctx))
	}

	modifiers := registry.Headers(dockerversion.DockerUserAgent(ctx), metaHeaders)
	authTransport := transport.NewTransport(base, modifiers...)

	func (pm Manager) newResolver(ctx context.Context, tracker docker.StatusTracker, auth registry.AuthConfig, headers http.Header, httpFallback bool) (remotes.Resolver, error) {
	if headers == nil {
	headers = http.Header{}
	}
	headers.Add("User-Agent", dockerversion.DockerUserAgent(ctx))

Conversation

corhere commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Mar 1, 2023

Uh oh!

corhere commented Mar 1, 2023

Uh oh!

rumpl left a comment

Choose a reason for hiding this comment

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

neersighted left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

neersighted Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

corhere commented Mar 1, 2023 •

edited

Loading