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.

Sign up for GitHub

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

libpod/image: Use ParseNormalizedNamed in RepoDigests #2106

Closed
wants to merge 1 commit into from
+76 −6
Closed

libpod/image: Use ParseNormalizedNamed in RepoDigests #2106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion cmd/podman/sign.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,10 @@ func signCmd(c *cli.Context) error {
sigStoreDir = SignatureStoreDir
}

repos := newImage.RepoDigests()
repos, err := newImage.RepoDigests()
if err != nil {
return errors.Wrapf(err, "error calculating repo digests for %s", signimage)
}
if len(repos) == 0 {
logrus.Errorf("no repodigests associated with the image %s", signimage)
continue
Expand Down
18 changes: 15 additions & 3 deletions libpod/image/image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,12 +305,24 @@ func (i *Image) Names() []string {
}

// RepoDigests returns a string array of repodigests associated with the image
func (i *Image) RepoDigests() []string {
func (i *Image) RepoDigests() ([]string, error) {
var repoDigests []string
digest := i.Digest()

for _, name := range i.Names() {
repoDigests = append(repoDigests, strings.SplitN(name, ":", 2)[0]+"@"+i.Digest().String())
named, err := reference.ParseNormalizedNamed(name)
if err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW c/image/storage silently ignores errors (= .Names entries not in the c/image/docker/reference format).

Names are not documented to be in any particular format, so hard failures here may not be appropriate — OTOH maybe we should tighten the definition of Names instead. I don’t know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Names are not documented to be in any particular format, so hard failures here may not be appropriate...

I don't have an opinion on what ParseNormaliedNamed wants to consider an error, but if it returns an error, I think we should be passing it back up the chain. I certainly don't want different opinions on what names are considered valid living at different places in the stack.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I’m saying that the existing opinion/practice is that non-ParseNormalizedNamed names are 1) allowed and 2) silently ignored, so if you ”don’t want different opinions” (which I agree with) the right thing to do is to silently ignore such names as well.

I do support tightening the definition, but if that should happen, it needs be a separate PR set across c/storage (documenting the field as such)+c/image+libpod (using it) and maybe others.

return nil, err
}

canonical, err := reference.WithDigest(reference.TrimNamed(named), digest)
if err != nil {
return nil, err
}

repoDigests = append(repoDigests, canonical.String())
}
return repoDigests
return repoDigests, nil
}

// Created returns the time the image was created
Expand Down
46 changes: 46 additions & 0 deletions libpod/image/image_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"testing"

"github.com/containers/storage"
"github.com/opencontainers/go-digest"
"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -192,6 +193,51 @@ func TestImage_MatchRepoTag(t *testing.T) {
cleanup(workdir, ir)
}

// TestImage_RepoDigests tests RepoDigest generation.
func TestImage_RepoDigests(t *testing.T) {
dgst, err := digest.Parse("sha256:7173b809ca12ec5dee4506cd86be934c4596dd234ee82c0662eac04a8c2c71dc")
if err != nil {
t.Fatal(err)
}

for _, test := range []struct {
name string
names []string
expected []string
}{
{
name: "empty",
names: []string{},
expected: nil,
},
{
name: "tagged",
names: []string{"docker.io/library/busybox:latest"},
expected: []string{"docker.io/library/busybox@sha256:7173b809ca12ec5dee4506cd86be934c4596dd234ee82c0662eac04a8c2c71dc"},
Copy link
Member

Choose a reason for hiding this comment

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

How often does Busybox update (such that this would no longer pull the same image)?

Can we throw a comment on here that this needs updating when busybox bumps its image version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How often does Busybox update...

This digest is hard-coded in the test itself; it is completely decoupled from any real-world BusyBox image. So no need to bump anything.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry, I meant the line above - busybox:latest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry, I meant the line above - busybox:latest

Still just a string I'm hard-coding for these unit tests with no relation to real-world images. The new unit tests should just be exercising vendored string-manipulation code; they aren't hitting the network or anything.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, should be fine then, thanks

},
{
name: "digest",
names: []string{"docker.io/library/busybox@sha256:7173b809ca12ec5dee4506cd86be934c4596dd234ee82c0662eac04a8c2c71dc"},
expected: []string{"docker.io/library/busybox@sha256:7173b809ca12ec5dee4506cd86be934c4596dd234ee82c0662eac04a8c2c71dc"},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test fails with the doubled @sha256 unless we also include the ParseNormalizedNamed change this commit makes to RepoDigests().

},
} {
t.Run(test.name, func(t *testing.T) {
image := &Image{
image: &storage.Image{
Names: test.names,
Digest: dgst,
},
}
actual, err := image.RepoDigests()
if err != nil {
t.Fatal(err)
}

assert.Equal(t, test.expected, actual)
})
}
}

// Test_splitString tests the splitString function in image that
// takes input and splits on / and returns the last array item
func Test_splitString(t *testing.T) {
Expand Down
13 changes: 11 additions & 2 deletions pkg/varlinkapi/images.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,18 @@ func (i *LibpodAPI) ListImages(call iopodman.VarlinkCall) error {
for _, image := range images {
labels, _ := image.Labels(getContext())
containers, _ := image.Containers()
repoDigests, err := image.RepoDigests()
if err != nil {
return err
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dunno why everyone else is ignoring errors here, but it's been that way since 39a7a77 (#669). @baude, do you remember your motivation?

Copy link
Member

Choose a reason for hiding this comment

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

image.Labels(...) seems to ignore errors as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image.Labels(...) seems to ignore errors as well.

Yeah, I left the existing error-ignores alone, since I don't understand their motivation. I'm happy to make these all consistent though (one way or the other) if someone can explain the motivation.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like we don't want to fatally fail when trying to get some information on an image, but it's not commented at all so I'm not sure. @baude this intentional?

}

size, _ := image.Size(getContext())

i := iopodman.ImageInList{
Id: image.ID(),
ParentId: image.Parent,
RepoTags: image.Names(),
RepoDigests: image.RepoDigests(),
RepoDigests: repoDigests,
Created: image.Created().String(),
Size: int64(*size),
VirtualSize: image.VirtualSize,
Expand All @@ -73,6 +78,10 @@ func (i *LibpodAPI) GetImage(call iopodman.VarlinkCall, name string) error {
if err != nil {
return err
}
repoDigests, err := newImage.RepoDigests()
if err != nil {
return err
}
size, err := newImage.Size(getContext())
if err != nil {
return err
Expand All @@ -82,7 +91,7 @@ func (i *LibpodAPI) GetImage(call iopodman.VarlinkCall, name string) error {
Id: newImage.ID(),
ParentId: newImage.Parent,
RepoTags: newImage.Names(),
RepoDigests: newImage.RepoDigests(),
RepoDigests: repoDigests,
Created: newImage.Created().String(),
Size: int64(*size),
VirtualSize: newImage.VirtualSize,
Expand Down