Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

image look up: consult registries.conf #7823

Merged
merged 2 commits into from
Oct 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 58 additions & 14 deletions libpod/image/image.go
Original file line number Diff line number Diff line change
Expand Up @@ -410,48 +410,92 @@ func (ir *Runtime) getLocalImage(inputName string) (string, *storage.Image, erro
if inputName == "" {
return "", nil, errors.Errorf("input name is blank")
}

// Check if the input name has a transport and if so strip it
dest, err := alltransports.ParseImageName(inputName)
if err == nil && dest.DockerReference() != nil {
inputName = dest.DockerReference().String()
}

img, err := ir.getImage(stripSha256(inputName))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes the only caller of getImage with a name AFAICS; the remaining know that they are using a full image ID; so, getImage should become getImageByID and use either NewStoreReference(store, nil, id) (more explicit about the intent) or perhaps store.Image (bypasses c/image, but allows names as input), without the overhead/risk of ParseStoreReference allowing other inputs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good catch. I can open a follow-up PR to clean up 👍

// Early check for fully-qualified images and (short) IDs.
img, err := ir.store.Image(stripSha256(inputName))
if err == nil {
return inputName, img, err
return inputName, img, nil
}

// container-storage wasn't able to find it in its current form
// check if the input name has a tag, and if not, run it through
// again
// Note that it's crucial to first decompose the image and check if
// it's a fully-qualified one or a "short name". The latter requires
// some normalization with search registries and the
// "localhost/prefix".
decomposedImage, err := decompose(inputName)
if err != nil {
// We may have a storage reference. We can't parse it to a
// reference before. Otherwise, we'd normalize "alpine" to
// "docker.io/library/alpine:latest" which would break the
// order in which we should query local images below.
if ref, err := is.Transport.ParseStoreReference(ir.store, inputName); err == nil {
img, err = is.Transport.GetStoreImage(ir.store, ref)
if err == nil {
return inputName, img, nil
}
}
return "", nil, err
}

// The image has a registry name in it and we made sure we looked for it locally
// with a tag. It cannot be local.
// The specified image is fully qualified, so it doesn't exist in the
// storage.
if decomposedImage.hasRegistry {
// However ... we may still need to normalize to docker.io:
// `docker.io/foo` -> `docker.io/library/foo`
if ref, err := is.Transport.ParseStoreReference(ir.store, inputName); err == nil {
img, err = is.Transport.GetStoreImage(ir.store, ref)
if err == nil {
return inputName, img, nil
}
}
return "", nil, errors.Wrapf(ErrNoSuchImage, imageError)
}
// if the image is saved with the repository localhost, searching with localhost prepended is necessary
// We don't need to strip the sha because we have already determined it is not an ID
ref, err := decomposedImage.referenceWithRegistry(DefaultLocalRegistry)

// "Short-name image", so let's try out certain prefixes:
// 1) DefaultLocalRegistry (i.e., "localhost/)
// 2) Unqualified-search registries from registries.conf
unqualifiedSearchRegistries, err := registries.GetRegistries()
if err != nil {
return "", nil, err
}
img, err = ir.getImage(ref.String())

for _, candidate := range append([]string{DefaultLocalRegistry}, unqualifiedSearchRegistries...) {
ref, err := decomposedImage.referenceWithRegistry(candidate)
if err != nil {
return "", nil, err
}
img, err := ir.store.Image(ref.String())
if err == nil {
return ref.String(), img, nil
}
}

// Backwards compat: normalize to docker.io as some users may very well
// rely on that.
ref, err := is.Transport.ParseStoreReference(ir.store, inputName)
if err == nil {
return inputName, img, err
img, err = is.Transport.GetStoreImage(ir.store, ref)
if err == nil {
return inputName, img, nil
}
}

// grab all the local images
// Last resort: look at the repotags of all images and try to find a
// match.
images, err := ir.GetImages()
if err != nil {
return "", nil, err
}

// check the repotags of all images for a match
decomposedImage, err = decompose(inputName)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already computed around line 430.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have added a comment. This was needed as the decomposedImage.referenceWithRegistry above has side effects.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can’t see any side effects — referenceWithRegistry just reads ip.hasRegistry and calls ip.unnormalizedRef.String() AFAICS; if it had side effects, would it even be possible to call it in a loop?

if err != nil {
return "", nil, err
}
repoImage, err := findImageInRepotags(decomposedImage, images)
if err == nil {
return inputName, repoImage, nil
Expand Down
5 changes: 5 additions & 0 deletions pkg/registries/registries.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
package registries

// TODO: this package should not exist anymore. Users should either use
// c/image's `sysregistriesv2` package directly OR, even better, we cache a
// config in libpod's image runtime so we don't need to parse the
// registries.conf files redundantly.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sysregistriesv2 is already caching the files.


import (
"os"
"path/filepath"
Expand Down
100 changes: 98 additions & 2 deletions test/e2e/pull_test.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
package integration

import (
"os"

"fmt"
"os"
"path/filepath"
"strings"

Expand Down Expand Up @@ -400,4 +399,101 @@ var _ = Describe("Podman pull", func() {
session.WaitWithDefaultTimeout()
Expect(session.ExitCode()).To(Not(Equal(0)))
})

It("podman pull + inspect from unqualified-search registry", func() {
// Regression test for #6381:
// Make sure that `pull shortname` and `inspect shortname`
// refer to the same image.

// We already tested pulling, so we can save some energy and
// just restore local artifacts and tag them.
podmanTest.RestoreArtifact(ALPINE)
podmanTest.RestoreArtifact(BB)

// What we want is at least two images which have the same name
// and are prefixed with two different unqualified-search
// registries from ../registries.conf.
//
// A `podman inspect $name` must yield the one from the _first_
// matching registry in the registries.conf.
getID := func(image string) string {
setup := podmanTest.PodmanNoCache([]string{"image", "inspect", image})
setup.WaitWithDefaultTimeout()
Expect(setup.ExitCode()).To(Equal(0))

data := setup.InspectImageJSON() // returns []inspect.ImageData
Expect(len(data)).To(Equal(1))
return data[0].ID
}

untag := func(image string) {
setup := podmanTest.PodmanNoCache([]string{"untag", image})
setup.WaitWithDefaultTimeout()
Expect(setup.ExitCode()).To(Equal(0))

setup = podmanTest.PodmanNoCache([]string{"image", "inspect", image})
setup.WaitWithDefaultTimeout()
Expect(setup.ExitCode()).To(Equal(0))

data := setup.InspectImageJSON() // returns []inspect.ImageData
Expect(len(data)).To(Equal(1))
Expect(len(data[0].RepoTags)).To(Equal(0))
}

tag := func(image, tag string) {
setup := podmanTest.PodmanNoCache([]string{"tag", image, tag})
setup.WaitWithDefaultTimeout()
Expect(setup.ExitCode()).To(Equal(0))
setup = podmanTest.PodmanNoCache([]string{"image", "exists", tag})
setup.WaitWithDefaultTimeout()
Expect(setup.ExitCode()).To(Equal(0))
}

image1 := getID(ALPINE)
image2 := getID(BB)

// $ head -n2 ../registries.conf
// [registries.search]
// registries = ['docker.io', 'quay.io', 'registry.fedoraproject.org']
registries := []string{"docker.io", "quay.io", "registry.fedoraproject.org"}
name := "foo/test:tag"
tests := []struct {
// tag1 has precedence (see list above) over tag2 when
// doing an inspect on "test:tag".
tag1, tag2 string
}{
{
fmt.Sprintf("%s/%s", registries[0], name),
fmt.Sprintf("%s/%s", registries[1], name),
},
{
fmt.Sprintf("%s/%s", registries[0], name),
fmt.Sprintf("%s/%s", registries[2], name),
},
{
fmt.Sprintf("%s/%s", registries[1], name),
fmt.Sprintf("%s/%s", registries[2], name),
},
}

for _, t := range tests {
// 1) untag both images
// 2) tag them according to `t`
// 3) make sure that an inspect of `name` returns `image1` with `tag1`
untag(image1)
untag(image2)
tag(image1, t.tag1)
tag(image2, t.tag2)

setup := podmanTest.PodmanNoCache([]string{"image", "inspect", name})
setup.WaitWithDefaultTimeout()
Expect(setup.ExitCode()).To(Equal(0))

data := setup.InspectImageJSON() // returns []inspect.ImageData
Expect(len(data)).To(Equal(1))
Expect(len(data[0].RepoTags)).To(Equal(1))
Expect(data[0].RepoTags[0]).To(Equal(t.tag1))
Expect(data[0].ID).To(Equal(image1))
}
})
})
1 change: 1 addition & 0 deletions test/registries.conf
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Note that changing the order here may break tests.
[registries.search]
registries = ['docker.io', 'quay.io', 'registry.fedoraproject.org']

Expand Down