Skip to content

Commit

Permalink
Fixup search
Browse files Browse the repository at this point in the history 
podman-remote search had some FIXMEs in tests that were failing.
So I reworked the search handler to use the local abi.  This
means the podman search and podman-remote search will use the
same functions.

While doing this, I noticed we were just outputing errors via
logrus.Error rather then returning them, which works ok for
podman but the messages get lost on podman-remote.  Changed
the code to actually return the error messages to the caller.

This allows us to turn on the remaining podman-remote FIXME
tests.

Signed-off-by: Daniel J Walsh <[email protected]>
  • Loading branch information
@rhatdan
rhatdan committed Jan 25, 2021
1 parent 3f5af4e commit 74a63df
Show file tree
Hide file tree
Showing 6 changed files with 62 additions and 163 deletions.
28 changes: 17 additions & 11 deletions libpod/image/search.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ func SearchImages(term string, options SearchOptions) ([]SearchResult, error) {
searchImageInRegistryHelper := func(index int, registry string) {
defer sem.Release(1)
defer wg.Done()
searchOutput := searchImageInRegistry(term, registry, options)
data[index] = searchOutputData{data: searchOutput}
searchOutput, err := searchImageInRegistry(term, registry, options)
data[index] = searchOutputData{data: searchOutput, err: err}
}

ctx := context.Background()
Expand All @@ -116,13 +116,21 @@ func SearchImages(term string, options SearchOptions) ([]SearchResult, error) {

wg.Wait()
results := []SearchResult{}
var lastError error
for _, d := range data {
if d.err != nil {
return nil, d.err
if lastError != nil {
logrus.Errorf("%v", lastError)
}
lastError = d.err
continue
}
results = append(results, d.data...)
}
return results, nil
if len(results) > 0 {
return results, nil
}
return results, lastError
}

// getRegistries returns the list of registries to search, depending on an optional registry specification
Expand All @@ -140,7 +148,7 @@ func getRegistries(registry string) ([]string, error) {
return registries, nil
}

func searchImageInRegistry(term string, registry string, options SearchOptions) []SearchResult {
func searchImageInRegistry(term string, registry string, options SearchOptions) ([]SearchResult, error) {
// Max number of queries by default is 25
limit := maxQueries
if options.Limit > 0 {
Expand All @@ -156,16 +164,14 @@ func searchImageInRegistry(term string, registry string, options SearchOptions)
if options.ListTags {
results, err := searchRepositoryTags(registry, term, sc, options)
if err != nil {
logrus.Errorf("error listing registry tags %q: %v", registry, err)
return []SearchResult{}
return []SearchResult{}, err
}
return results
return results, nil
}

results, err := docker.SearchRegistry(context.TODO(), sc, registry, term, limit)
if err != nil {
logrus.Errorf("error searching registry %q: %v", registry, err)
return []SearchResult{}
return []SearchResult{}, err
}
index := registry
arr := strings.Split(registry, ".")
Expand Down Expand Up @@ -219,7 +225,7 @@ func searchImageInRegistry(term string, registry string, options SearchOptions)
}
paramsArr = append(paramsArr, params)
}
return paramsArr
return paramsArr, nil
}

func searchRepositoryTags(registry, term string, sc *types.SystemContext, options SearchOptions) ([]SearchResult, error) {
Expand Down
84 changes: 31 additions & 53 deletions pkg/api/handlers/compat/images_search.go
View file
Original file line number Diff line number Diff line change
@@ -1,25 +1,30 @@
package compat

import (
"fmt"
"net/http"
"strconv"

"github.com/containers/image/v5/types"
"github.com/containers/podman/v2/libpod/image"
"github.com/containers/podman/v2/libpod"
"github.com/containers/podman/v2/libpod/define"
"github.com/containers/podman/v2/pkg/api/handlers/utils"
"github.com/containers/podman/v2/pkg/auth"
"github.com/docker/docker/api/types/registry"
"github.com/containers/podman/v2/pkg/domain/entities"
"github.com/containers/podman/v2/pkg/domain/infra/abi"
"github.com/gorilla/schema"
"github.com/pkg/errors"
)

func SearchImages(w http.ResponseWriter, r *http.Request) {
runtime := r.Context().Value("runtime").(*libpod.Runtime)
decoder := r.Context().Value("decoder").(*schema.Decoder)
query := struct {
Term string `json:"term"`
Limit int `json:"limit"`
NoTrunc bool `json:"noTrunc"`
Filters map[string][]string `json:"filters"`
TLSVerify bool `json:"tlsVerify"`
ListTags bool `json:"listTags"`
}{
// This is where you can override the golang default value for one of fields
}
Expand All @@ -29,67 +34,40 @@ func SearchImages(w http.ResponseWriter, r *http.Request) {
return
}

filter := image.SearchFilter{}
if len(query.Filters) > 0 {
if len(query.Filters["stars"]) > 0 {
stars, err := strconv.Atoi(query.Filters["stars"][0])
if err != nil {
utils.InternalServerError(w, err)
return
}
filter.Stars = stars
}
if len(query.Filters["is-official"]) > 0 {
isOfficial, err := strconv.ParseBool(query.Filters["is-official"][0])
if err != nil {
utils.InternalServerError(w, err)
return
}
filter.IsOfficial = types.NewOptionalBool(isOfficial)
}
if len(query.Filters["is-automated"]) > 0 {
isAutomated, err := strconv.ParseBool(query.Filters["is-automated"][0])
if err != nil {
utils.InternalServerError(w, err)
return
}
filter.IsAutomated = types.NewOptionalBool(isAutomated)
}
}
options := image.SearchOptions{
Filter: filter,
Limit: query.Limit,
}

if _, found := r.URL.Query()["tlsVerify"]; found {
options.InsecureSkipTLSVerify = types.NewOptionalBool(!query.TLSVerify)
}

_, authfile, key, err := auth.GetCredentials(r)
if err != nil {
utils.Error(w, "failed to retrieve repository credentials", http.StatusBadRequest, errors.Wrapf(err, "failed to parse %q header for %s", key, r.URL.String()))
return
}
defer auth.RemoveAuthfile(authfile)
options.Authfile = authfile

results, err := image.SearchImages(query.Term, options)
filters := []string{}
for key, val := range query.Filters {
filters = append(filters, fmt.Sprintf("%s=%s", key, val[0]))
}

options := entities.ImageSearchOptions{
Authfile: authfile,
Limit: query.Limit,
NoTrunc: query.NoTrunc,
ListTags: query.ListTags,
Filters: filters,
}
if _, found := r.URL.Query()["tlsVerify"]; found {
options.SkipTLSVerify = types.NewOptionalBool(!query.TLSVerify)
}
ir := abi.ImageEngine{Libpod: runtime}
reports, err := ir.Search(r.Context(), query.Term, options)
if err != nil {
utils.BadRequest(w, "term", query.Term, err)
utils.Error(w, "Something went wrong.", http.StatusInternalServerError, err)
return
}

compatResults := make([]registry.SearchResult, 0, len(results))
for _, result := range results {
compatResult := registry.SearchResult{
Name: result.Name,
Description: result.Description,
StarCount: result.Stars,
IsAutomated: result.Automated == "[OK]",
IsOfficial: result.Official == "[OK]",
if !utils.IsLibpodRequest(r) {
if len(reports) == 0 {
utils.ImageNotFound(w, query.Term, define.ErrNoSuchImage)
return
}
compatResults = append(compatResults, compatResult)
}

utils.WriteResponse(w, http.StatusOK, compatResults)
utils.WriteResponse(w, http.StatusOK, reports)
}
86 changes: 0 additions & 86 deletions pkg/api/handlers/libpod/images.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -589,92 +589,6 @@ func UntagImage(w http.ResponseWriter, r *http.Request) {
utils.WriteResponse(w, http.StatusCreated, "")
}

func SearchImages(w http.ResponseWriter, r *http.Request) {
decoder := r.Context().Value("decoder").(*schema.Decoder)
query := struct {
Term string `json:"term"`
Limit int `json:"limit"`
NoTrunc bool `json:"noTrunc"`
Filters map[string][]string `json:"filters"`
TLSVerify bool `json:"tlsVerify"`
ListTags bool `json:"listTags"`
}{
// This is where you can override the golang default value for one of fields
}

if err := decoder.Decode(&query, r.URL.Query()); err != nil {
utils.Error(w, "Something went wrong.", http.StatusBadRequest, errors.Wrapf(err, "failed to parse parameters for %s", r.URL.String()))
return
}

filter := image.SearchFilter{}
if len(query.Filters) > 0 {
if len(query.Filters["stars"]) > 0 {
stars, err := strconv.Atoi(query.Filters["stars"][0])
if err != nil {
utils.InternalServerError(w, err)
return
}
filter.Stars = stars
}
if len(query.Filters["is-official"]) > 0 {
isOfficial, err := strconv.ParseBool(query.Filters["is-official"][0])
if err != nil {
utils.InternalServerError(w, err)
return
}
filter.IsOfficial = types.NewOptionalBool(isOfficial)
}
if len(query.Filters["is-automated"]) > 0 {
isAutomated, err := strconv.ParseBool(query.Filters["is-automated"][0])
if err != nil {
utils.InternalServerError(w, err)
return
}
filter.IsAutomated = types.NewOptionalBool(isAutomated)
}
}
options := image.SearchOptions{
Limit: query.Limit,
NoTrunc: query.NoTrunc,
ListTags: query.ListTags,
Filter: filter,
}

if _, found := r.URL.Query()["tlsVerify"]; found {
options.InsecureSkipTLSVerify = types.NewOptionalBool(!query.TLSVerify)
}

_, authfile, key, err := auth.GetCredentials(r)
if err != nil {
utils.Error(w, "failed to retrieve repository credentials", http.StatusBadRequest, errors.Wrapf(err, "failed to parse %q header for %s", key, r.URL.String()))
return
}
defer auth.RemoveAuthfile(authfile)
options.Authfile = authfile

searchResults, err := image.SearchImages(query.Term, options)
if err != nil {
utils.BadRequest(w, "term", query.Term, err)
return
}
// Convert from image.SearchResults to entities.ImageSearchReport. We don't
// want to leak any low-level packages into the remote client, which
// requires converting.
reports := make([]entities.ImageSearchReport, len(searchResults))
for i := range searchResults {
reports[i].Index = searchResults[i].Index
reports[i].Name = searchResults[i].Name
reports[i].Description = searchResults[i].Description
reports[i].Stars = searchResults[i].Stars
reports[i].Official = searchResults[i].Official
reports[i].Automated = searchResults[i].Automated
reports[i].Tag = searchResults[i].Tag
}

utils.WriteResponse(w, http.StatusOK, reports)
}

// ImagesBatchRemove is the endpoint for batch image removal.
func ImagesBatchRemove(w http.ResponseWriter, r *http.Request) {
runtime := r.Context().Value("runtime").(*libpod.Runtime)
Expand Down
2 changes: 1 addition & 1 deletion pkg/api/server/register_images.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1019,7 +1019,7 @@ func (s *APIServer) registerImagesHandlers(r *mux.Router) error {
// $ref: "#/responses/DocsSearchResponse"
// 500:
// $ref: '#/responses/InternalError'
r.Handle(VersionedPath("/libpod/images/search"), s.APIHandler(libpod.SearchImages)).Methods(http.MethodGet)
r.Handle(VersionedPath("/libpod/images/search"), s.APIHandler(compat.SearchImages)).Methods(http.MethodGet)
// swagger:operation GET /libpod/images/{name:.*}/get libpod libpodExportImage
// ---
// tags:
Expand Down
13 changes: 3 additions & 10 deletions test/e2e/search_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,6 @@ registries = ['{{.Host}}:{{.Port}}']`
})

It("podman search doesn't attempt HTTP if force secure is true", func() {
SkipIfRemote("FIXME This should work on podman-remote")
if podmanTest.Host.Arch == "ppc64le" {
Skip("No registry image for ppc64le")
}
Expand All @@ -324,15 +323,11 @@ registries = ['{{.Host}}:{{.Port}}']`
registryFileTmpl.Execute(&buffer, registryEndpoints[5])
podmanTest.setRegistriesConfigEnv(buffer.Bytes())
ioutil.WriteFile(fmt.Sprintf("%s/registry5.conf", tempdir), buffer.Bytes(), 0644)
if IsRemote() {
podmanTest.RestartRemoteService()
defer podmanTest.RestartRemoteService()
}

search := podmanTest.Podman([]string{"search", image, "--tls-verify=true"})
search.WaitWithDefaultTimeout()

Expect(search.ExitCode()).To(Equal(0))
Expect(search.ExitCode()).To(Equal(125))
Expect(search.OutputToString()).Should(BeEmpty())
match, _ := search.ErrorGrepString("error")
Expect(match).Should(BeTrue())
Expand All @@ -342,7 +337,6 @@ registries = ['{{.Host}}:{{.Port}}']`
})

It("podman search doesn't attempt HTTP if registry is not listed as insecure", func() {
SkipIfRemote("FIXME This should work on podman-remote")
if podmanTest.Host.Arch == "ppc64le" {
Skip("No registry image for ppc64le")
}
Expand Down Expand Up @@ -376,7 +370,7 @@ registries = ['{{.Host}}:{{.Port}}']`
search := podmanTest.Podman([]string{"search", image})
search.WaitWithDefaultTimeout()

Expect(search.ExitCode()).To(Equal(0))
Expect(search.ExitCode()).To(Equal(125))
Expect(search.OutputToString()).Should(BeEmpty())
match, _ := search.ErrorGrepString("error")
Expect(match).Should(BeTrue())
Expand All @@ -386,7 +380,6 @@ registries = ['{{.Host}}:{{.Port}}']`
})

It("podman search doesn't attempt HTTP if one registry is not listed as insecure", func() {
SkipIfRemote("FIXME This should work on podman-remote")
if podmanTest.Host.Arch == "ppc64le" {
Skip("No registry image for ppc64le")
}
Expand Down Expand Up @@ -431,7 +424,7 @@ registries = ['{{.Host}}:{{.Port}}']`
search := podmanTest.Podman([]string{"search", "my-alpine"})
search.WaitWithDefaultTimeout()

Expect(search.ExitCode()).To(Equal(0))
Expect(search.ExitCode()).To(Equal(125))
Expect(search.OutputToString()).Should(BeEmpty())
match, _ := search.ErrorGrepString("error")
Expect(match).Should(BeTrue())
Expand Down
12 changes: 10 additions & 2 deletions test/python/docker/test_images.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,16 @@ def test_list_images(self):

def test_search_image(self):
"""Search for image"""
for r in self.client.images.search("libpod/alpine"):
self.assertIn("quay.io/libpod/alpine", r["Name"])
for r in self.client.images.search("alpine"):
self.assertIn("alpine", r["Name"])

def test_search_bogus_image(self):
"""Search for bogus image should throw exception"""
try:
r = self.client.images.search("bogus/bogus")
except:
return
self.assertTrue(len(r)==0)

def test_remove_image(self):
"""Remove image"""
Expand Down

0 comments on commit 74a63df

Please sign in to comment.