Move filtered registry search out of image service

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 <csnider@mirantis.com>

api/server/router/image/backend.go

@@ -39,5 +39,8 @@ type importExportBackend interface {
 type registryBackend interface {
 	PullImage(ctx context.Context, image, tag string, platform *specs.Platform, metaHeaders map[string][]string, authConfig *registry.AuthConfig, outStream io.Writer) error
 	PushImage(ctx context.Context, image, tag string, metaHeaders map[string][]string, authConfig *registry.AuthConfig, outStream io.Writer) error
-	SearchRegistryForImages(ctx context.Context, searchFilters filters.Args, term string, limit int, authConfig *registry.AuthConfig, metaHeaders map[string][]string) (*registry.SearchResults, error)
+}
+
+type Searcher interface {
+	Search(ctx context.Context, searchFilters filters.Args, term string, limit int, authConfig *registry.AuthConfig, metaHeaders map[string][]string) ([]registry.SearchResult, error)
 }

api/server/router/image/image.go

@@ -10,6 +10,7 @@ import (
 // imageRouter is a router to talk with the image controller
 type imageRouter struct {
 	backend          Backend
+	searcher         Searcher
 	referenceBackend reference.Store
 	imageStore       image.Store
 	layerStore       layer.Store
@@ -17,9 +18,10 @@ type imageRouter struct {
 }
 
 // NewRouter initializes a new image router
-func NewRouter(backend Backend, referenceBackend reference.Store, imageStore image.Store, layerStore layer.Store) router.Router {
+func NewRouter(backend Backend, searcher Searcher, referenceBackend reference.Store, imageStore image.Store, layerStore layer.Store) router.Router {
 	ir := &imageRouter{
 		backend:          backend,
+		searcher:         searcher,
 		referenceBackend: referenceBackend,
 		imageStore:       imageStore,
 		layerStore:       layerStore,

api/server/router/image/image_routes.go

@@ -415,11 +415,11 @@ func (ir *imageRouter) getImagesSearch(ctx context.Context, w http.ResponseWrite
 	// For a search it is not an error if no auth was given. Ignore invalid
 	// AuthConfig to increase compatibility with the existing API.
 	authConfig, _ := registry.DecodeAuthConfig(r.Header.Get(registry.AuthHeader))
-	query, err := ir.backend.SearchRegistryForImages(ctx, searchFilters, r.Form.Get("term"), limit, authConfig, headers)
+	res, err := ir.searcher.Search(ctx, searchFilters, r.Form.Get("term"), limit, authConfig, headers)
 	if err != nil {
 		return err
 	}
-	return httputils.WriteJSON(w, http.StatusOK, query.Results)
+	return httputils.WriteJSON(w, http.StatusOK, res)
 }
 
 func (ir *imageRouter) postImagesPrune(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error {

cmd/dockerd/daemon.go

@@ -511,6 +511,7 @@ func initRouter(opts routerOptions) {
 		container.NewRouter(opts.daemon, decoder, opts.daemon.RawSysInfo().CgroupUnified),
 		image.NewRouter(
 			opts.daemon.ImageService(),
+			opts.daemon.RegistryService(),
 			opts.daemon.ReferenceStore,
 			opts.daemon.ImageService().DistributionServices().ImageStore,
 			opts.daemon.ImageService().DistributionServices().LayerStore,

daemon/containerd/image_search.go

@@ -1,19 +0,0 @@
-package containerd
-
-import (
-	"context"
-	"errors"
-
-	"github.com/docker/docker/api/types/filters"
-	"github.com/docker/docker/api/types/registry"
-	"github.com/docker/docker/errdefs"
-)
-
-// SearchRegistryForImages queries the registry for images matching
-// term. authConfig is used to login.
-//
-// TODO: this could be implemented in a registry service instead of the image
-// service.
-func (i *ImageService) SearchRegistryForImages(ctx context.Context, searchFilters filters.Args, term string, limit int, authConfig *registry.AuthConfig, metaHeaders map[string][]string) (*registry.SearchResults, error) {
-	return nil, errdefs.NotImplemented(errors.New("not implemented"))
-}

daemon/daemon.go

@@ -1459,6 +1459,11 @@ func (daemon *Daemon) ImageService() ImageService {
 	return daemon.imageService
 }
 
+// RegistryService returns the Daemon's RegistryService
+func (daemon *Daemon) RegistryService() registry.Service {
+	return daemon.registryService
+}
+
 // BuilderBackend returns the backend used by builder
 func (daemon *Daemon) BuilderBackend() builder.Backend {
 	return struct {

daemon/image_service.go

@@ -73,7 +73,6 @@ type ImageService interface {
 	// Other
 
 	GetRepository(ctx context.Context, ref reference.Named, authConfig *registry.AuthConfig) (distribution.Repository, error)
-	SearchRegistryForImages(ctx context.Context, searchFilters filters.Args, term string, limit int, authConfig *registry.AuthConfig, headers map[string][]string) (*registry.SearchResults, error)
 	DistributionServices() images.DistributionServices
 	Children(id image.ID) []image.ID
 	Cleanup() error

daemon/images/image_search.go

@@ -1,85 +0,0 @@
-package images // import "github.com/docker/docker/daemon/images"
-
-import (
-	"context"
-	"strconv"
-
-	"github.com/docker/docker/api/types/filters"
-	"github.com/docker/docker/api/types/registry"
-	"github.com/docker/docker/dockerversion"
-	"github.com/docker/docker/errdefs"
-	"github.com/pkg/errors"
-)
-
-var acceptedSearchFilterTags = map[string]bool{
-	"is-automated": true,
-	"is-official":  true,
-	"stars":        true,
-}
-
-// SearchRegistryForImages queries the registry for images matching
-// term. authConfig is used to login.
-//
-// TODO: this could be implemented in a registry service instead of the image
-// service.
-func (i *ImageService) SearchRegistryForImages(ctx context.Context, searchFilters filters.Args, term string, limit int,
-	authConfig *registry.AuthConfig,
-	headers map[string][]string) (*registry.SearchResults, error) {
-	if err := searchFilters.Validate(acceptedSearchFilterTags); err != nil {
-		return nil, err
-	}
-
-	isAutomated, err := searchFilters.GetBoolOrDefault("is-automated", false)
-	if err != nil {
-		return nil, err
-	}
-	isOfficial, err := searchFilters.GetBoolOrDefault("is-official", false)
-	if err != nil {
-		return nil, err
-	}
-
-	hasStarFilter := 0
-	if searchFilters.Contains("stars") {
-		hasStars := searchFilters.Get("stars")
-		for _, hasStar := range hasStars {
-			iHasStar, err := strconv.Atoi(hasStar)
-			if err != nil {
-				return nil, errdefs.InvalidParameter(errors.Wrapf(err, "invalid filter 'stars=%s'", hasStar))
-			}
-			if iHasStar > hasStarFilter {
-				hasStarFilter = iHasStar
-			}
-		}
-	}
-
-	unfilteredResult, err := i.registryService.Search(ctx, term, limit, authConfig, dockerversion.DockerUserAgent(ctx), headers)
-	if err != nil {
-		return nil, err
-	}
-
-	filteredResults := []registry.SearchResult{}
-	for _, result := range unfilteredResult.Results {
-		if searchFilters.Contains("is-automated") {
-			if isAutomated != result.IsAutomated {
-				continue
-			}
-		}
-		if searchFilters.Contains("is-official") {
-			if isOfficial != result.IsOfficial {
-				continue
-			}
-		}
-		if searchFilters.Contains("stars") {
-			if result.StarCount < hasStarFilter {
-				continue
-			}
-		}
-		filteredResults = append(filteredResults, result)
-	}
-
-	return &registry.SearchResults{
-		Query:      unfilteredResult.Query,
-		NumResults: len(filteredResults),
-		Results:    filteredResults,
-	}, nil
-}

registry/search.go

@@ -0,0 +1,139 @@
+package registry // import "github.com/docker/docker/registry"
+
+import (
+	"context"
+	"net/http"
+	"strconv"
+	"strings"
+
+	"github.com/docker/docker/api/types/filters"
+	"github.com/docker/docker/api/types/registry"
+	"github.com/docker/docker/dockerversion"
+	"github.com/docker/docker/errdefs"
+
+	"github.com/docker/distribution/registry/client/auth"
+	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
+)
+
+var acceptedSearchFilterTags = map[string]bool{
+	"is-automated": true,
+	"is-official":  true,
+	"stars":        true,
+}
+
+// Search queries the public registry for repositories matching the specified
+// search term and filters.
+func (s *defaultService) Search(ctx context.Context, searchFilters filters.Args, term string, limit int, authConfig *registry.AuthConfig, headers map[string][]string) ([]registry.SearchResult, error) {
+	if err := searchFilters.Validate(acceptedSearchFilterTags); err != nil {
+		return nil, err
+	}
+
+	isAutomated, err := searchFilters.GetBoolOrDefault("is-automated", false)
+	if err != nil {
+		return nil, err
+	}
+	isOfficial, err := searchFilters.GetBoolOrDefault("is-official", false)
+	if err != nil {
+		return nil, err
+	}
+
+	hasStarFilter := 0
+	if searchFilters.Contains("stars") {
+		hasStars := searchFilters.Get("stars")
+		for _, hasStar := range hasStars {
+			iHasStar, err := strconv.Atoi(hasStar)
+			if err != nil {
+				return nil, errdefs.InvalidParameter(errors.Wrapf(err, "invalid filter 'stars=%s'", hasStar))
+			}
+			if iHasStar > hasStarFilter {
+				hasStarFilter = iHasStar
+			}
+		}
+	}
+
+	unfilteredResult, err := s.searchUnfiltered(ctx, term, limit, authConfig, dockerversion.DockerUserAgent(ctx), headers)
+	if err != nil {
+		return nil, err
+	}
+
+	filteredResults := []registry.SearchResult{}
+	for _, result := range unfilteredResult.Results {
+		if searchFilters.Contains("is-automated") {
+			if isAutomated != result.IsAutomated {
+				continue
+			}
+		}
+		if searchFilters.Contains("is-official") {
+			if isOfficial != result.IsOfficial {
+				continue
+			}
+		}
+		if searchFilters.Contains("stars") {
+			if result.StarCount < hasStarFilter {
+				continue
+			}
+		}
+		filteredResults = append(filteredResults, result)
+	}
+
+	return filteredResults, nil
+}
+
+func (s *defaultService) searchUnfiltered(ctx context.Context, term string, limit int, authConfig *registry.AuthConfig, userAgent string, headers map[string][]string) (*registry.SearchResults, error) {
+	// TODO Use ctx when searching for repositories
+	if hasScheme(term) {
+		return nil, invalidParamf("invalid repository name: repository name (%s) should not have a scheme", term)
+	}
+
+	indexName, remoteName := splitReposSearchTerm(term)
+
+	// Search is a long-running operation, just lock s.config to avoid block others.
+	s.mu.RLock()
+	index, err := newIndexInfo(s.config, indexName)
+	s.mu.RUnlock()
+
+	if err != nil {
+		return nil, err
+	}
+	if index.Official {
+		// If pull "library/foo", it's stored locally under "foo"
+		remoteName = strings.TrimPrefix(remoteName, "library/")
+	}
+
+	endpoint, err := newV1Endpoint(index, userAgent, headers)
+	if err != nil {
+		return nil, err
+	}
+
+	var client *http.Client
+	if authConfig != nil && authConfig.IdentityToken != "" && authConfig.Username != "" {
+		creds := NewStaticCredentialStore(authConfig)
+		scopes := []auth.Scope{
+			auth.RegistryScope{
+				Name:    "catalog",
+				Actions: []string{"search"},
+			},
+		}
+
+		modifiers := Headers(userAgent, nil)
+		v2Client, err := v2AuthHTTPClient(endpoint.URL, endpoint.client.Transport, modifiers, creds, scopes)
+		if err != nil {
+			return nil, err
+		}
+		// Copy non transport http client features
+		v2Client.Timeout = endpoint.client.Timeout
+		v2Client.CheckRedirect = endpoint.client.CheckRedirect
+		v2Client.Jar = endpoint.client.Jar
+
+		logrus.Debugf("using v2 client for search to %s", endpoint.URL)
+		client = v2Client
+	} else {
+		client = endpoint.client
+		if err := authorizeClient(client, authConfig, endpoint); err != nil {
+			return nil, err
+		}
+	}
+
+	return newSession(client, endpoint).searchRepositories(remoteName, limit)
+}

daemon/images/image_search_test.go → registry/search_test.go

@@ -1,44 +1,26 @@
-package images // import "github.com/docker/docker/daemon/images"
+package registry // import "github.com/docker/docker/registry"
 
 import (
 	"context"
-	"errors"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
 	"testing"
 
 	"github.com/docker/docker/api/types/filters"
 	"github.com/docker/docker/api/types/registry"
 	"github.com/docker/docker/errdefs"
-	registrypkg "github.com/docker/docker/registry"
 	"gotest.tools/v3/assert"
 )
 
-type fakeService struct {
-	registrypkg.Service
-	shouldReturnError bool
-
-	term    string
-	results []registry.SearchResult
-}
-
-func (s *fakeService) Search(ctx context.Context, term string, limit int, authConfig *registry.AuthConfig, userAgent string, headers map[string][]string) (*registry.SearchResults, error) {
-	if s.shouldReturnError {
-		return nil, errdefs.Unknown(errors.New("search unknown error"))
-	}
-	return &registry.SearchResults{
-		Query:      s.term,
-		NumResults: len(s.results),
-		Results:    s.results,
-	}, nil
-}
-
-func TestSearchRegistryForImagesErrors(t *testing.T) {
+func TestSearchErrors(t *testing.T) {
 	errorCases := []struct {
 		filtersArgs       filters.Args
 		shouldReturnError bool
 		expectedError     string
 	}{
 		{
-			expectedError:     "search unknown error",
+			expectedError:     "Unexpected status code 500",
 			shouldReturnError: true,
 		},
 		{
@@ -82,12 +64,20 @@ func TestSearchRegistryForImagesErrors(t *testing.T) {
 	for _, tc := range errorCases {
 		tc := tc
 		t.Run(tc.expectedError, func(t *testing.T) {
-			daemon := &ImageService{
-				registryService: &fakeService{
-					shouldReturnError: tc.shouldReturnError,
-				},
-			}
-			_, err := daemon.SearchRegistryForImages(context.Background(), tc.filtersArgs, "term", 0, nil, map[string][]string{})
+			srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				if !tc.shouldReturnError {
+					t.Errorf("unexpected HTTP request")
+				}
+				http.Error(w, "no search for you", http.StatusInternalServerError)
+			}))
+			defer srv.Close()
+
+			// Construct the search term by cutting the 'http://' prefix off srv.URL.
+			term := srv.URL[7:] + "/term"
+
+			reg, err := NewService(ServiceOptions{})
+			assert.NilError(t, err)
+			_, err = reg.Search(context.Background(), tc.filtersArgs, term, 0, nil, map[string][]string{})
 			assert.ErrorContains(t, err, tc.expectedError)
 			if tc.shouldReturnError {
 				assert.Check(t, errdefs.IsUnknown(err), "got: %T: %v", err, err)
@@ -98,8 +88,8 @@ func TestSearchRegistryForImagesErrors(t *testing.T) {
 	}
 }
 
-func TestSearchRegistryForImages(t *testing.T) {
-	term := "term"
+func TestSearch(t *testing.T) {
+	const term = "term"
 	successCases := []struct {
 		name            string
 		filtersArgs     filters.Args
@@ -348,17 +338,24 @@ func TestSearchRegistryForImages(t *testing.T) {
 	for _, tc := range successCases {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
-			daemon := &ImageService{
-				registryService: &fakeService{
-					term:    term,
-					results: tc.registryResults,
-				},
-			}
-			results, err := daemon.SearchRegistryForImages(context.Background(), tc.filtersArgs, term, 0, nil, map[string][]string{})
+			srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				w.Header().Set("Content-type", "application/json")
+				json.NewEncoder(w).Encode(registry.SearchResults{
+					Query:      term,
+					NumResults: len(tc.registryResults),
+					Results:    tc.registryResults,
+				})
+			}))
+			defer srv.Close()
+
+			// Construct the search term by cutting the 'http://' prefix off srv.URL.
+			searchTerm := srv.URL[7:] + "/" + term
+
+			reg, err := NewService(ServiceOptions{})
+			assert.NilError(t, err)
+			results, err := reg.Search(context.Background(), tc.filtersArgs, searchTerm, 0, nil, map[string][]string{})
 			assert.NilError(t, err)
-			assert.Equal(t, results.Query, term)
-			assert.Equal(t, results.NumResults, len(tc.expectedResults))
-			assert.DeepEqual(t, results.Results, tc.expectedResults)
+			assert.DeepEqual(t, results, tc.expectedResults)
 		})
 	}
 }

registry/service.go

@@ -3,13 +3,12 @@ package registry // import "github.com/docker/docker/registry"
 import (
 	"context"
 	"crypto/tls"
-	"net/http"
 	"net/url"
 	"strings"
 	"sync"
 
 	"github.com/docker/distribution/reference"
-	"github.com/docker/distribution/registry/client/auth"
+	"github.com/docker/docker/api/types/filters"
 	"github.com/docker/docker/api/types/registry"
 	"github.com/docker/docker/errdefs"
 	"github.com/sirupsen/logrus"
@@ -21,7 +20,7 @@ type Service interface {
 	LookupPullEndpoints(hostname string) (endpoints []APIEndpoint, err error)
 	LookupPushEndpoints(hostname string) (endpoints []APIEndpoint, err error)
 	ResolveRepository(name reference.Named) (*RepositoryInfo, error)
-	Search(ctx context.Context, term string, limit int, authConfig *registry.AuthConfig, userAgent string, headers map[string][]string) (*registry.SearchResults, error)
+	Search(ctx context.Context, searchFilters filters.Args, term string, limit int, authConfig *registry.AuthConfig, headers map[string][]string) ([]registry.SearchResult, error)
 	ServiceConfig() *registry.ServiceConfig
 	LoadAllowNondistributableArtifacts([]string) error
 	LoadMirrors([]string) error
@@ -129,66 +128,6 @@ func splitReposSearchTerm(reposName string) (string, string) {
 	return nameParts[0], nameParts[1]
 }
 
-// Search queries the public registry for images matching the specified
-// search terms, and returns the results.
-func (s *defaultService) Search(ctx context.Context, term string, limit int, authConfig *registry.AuthConfig, userAgent string, headers map[string][]string) (*registry.SearchResults, error) {
-	// TODO Use ctx when searching for repositories
-	if hasScheme(term) {
-		return nil, invalidParamf("invalid repository name: repository name (%s) should not have a scheme", term)
-	}
-
-	indexName, remoteName := splitReposSearchTerm(term)
-
-	// Search is a long-running operation, just lock s.config to avoid block others.
-	s.mu.RLock()
-	index, err := newIndexInfo(s.config, indexName)
-	s.mu.RUnlock()
-
-	if err != nil {
-		return nil, err
-	}
-	if index.Official {
-		// If pull "library/foo", it's stored locally under "foo"
-		remoteName = strings.TrimPrefix(remoteName, "library/")
-	}
-
-	endpoint, err := newV1Endpoint(index, userAgent, headers)
-	if err != nil {
-		return nil, err
-	}
-
-	var client *http.Client
-	if authConfig != nil && authConfig.IdentityToken != "" && authConfig.Username != "" {
-		creds := NewStaticCredentialStore(authConfig)
-		scopes := []auth.Scope{
-			auth.RegistryScope{
-				Name:    "catalog",
-				Actions: []string{"search"},
-			},
-		}
-
-		modifiers := Headers(userAgent, nil)
-		v2Client, err := v2AuthHTTPClient(endpoint.URL, endpoint.client.Transport, modifiers, creds, scopes)
-		if err != nil {
-			return nil, err
-		}
-		// Copy non transport http client features
-		v2Client.Timeout = endpoint.client.Timeout
-		v2Client.CheckRedirect = endpoint.client.CheckRedirect
-		v2Client.Jar = endpoint.client.Jar
-
-		logrus.Debugf("using v2 client for search to %s", endpoint.URL)
-		client = v2Client
-	} else {
-		client = endpoint.client
-		if err := authorizeClient(client, authConfig, endpoint); err != nil {
-			return nil, err
-		}
-	}
-
-	return newSession(client, endpoint).searchRepositories(remoteName, limit)
-}
-
 // ResolveRepository splits a repository name into its components
 // and configuration of the associated registry.
 func (s *defaultService) ResolveRepository(name reference.Named) (*RepositoryInfo, error) {

registry/session.go

@@ -206,10 +206,10 @@ func (r *session) searchRepositories(term string, limit int) (*registry.SearchRe
 	}
 	defer res.Body.Close()
 	if res.StatusCode != http.StatusOK {
-		return nil, &jsonmessage.JSONError{
+		return nil, errdefs.Unknown(&jsonmessage.JSONError{
 			Message: fmt.Sprintf("Unexpected status code %d", res.StatusCode),
 			Code:    res.StatusCode,
-		}
+		})
 	}
 	result := new(registry.SearchResults)
 	return result, errors.Wrap(json.NewDecoder(res.Body).Decode(result), "error decoding registry search results")