From d6c6a81dd3cb73685c5cdf029cf9dd5602d3d44d Mon Sep 17 00:00:00 2001 From: xingweizheng Date: Sun, 29 Aug 2021 01:07:58 +0800 Subject: [PATCH 4/5] clean code: all latest tag checks take the FindImage as the entrance --- builder/dockerfile/builder.go | 2 +- constant.go | 2 + daemon/push.go | 6 --- daemon/save.go | 24 ++++------ exporter/docker/archive/archive.go | 56 +++++++++++++--------- image/image.go | 40 ++++++++-------- image/image_test.go | 96 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 160 insertions(+), 66 deletions(-) diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index 1e1e24d..e28fac9 100644 --- a/builder/dockerfile/builder.go +++ b/builder/dockerfile/builder.go @@ -646,7 +646,7 @@ func CheckAndExpandTag(tag string) (reference.Named, string, error) { if sepLastIndex == -1 || (sepLastIndex < slashLastIndex) { // isula // localhost:5000/isula - newTag += ":latest" + newTag = fmt.Sprintf("%s:%s", newTag, constant.DefaultTag) } const longestTagFieldsLen = 3 diff --git a/constant.go b/constant.go index bfe399b..4d1596a 100644 --- a/constant.go +++ b/constant.go @@ -89,6 +89,8 @@ const ( IsuladTransport = "isulad" // ManifestTransport used to export manifest list ManifestTransport = "manifest" + // DefaultTag is latest + DefaultTag = "latest" ) var ( diff --git a/daemon/push.go b/daemon/push.go index d3f5571..ac05383 100644 --- a/daemon/push.go +++ b/daemon/push.go @@ -68,12 +68,6 @@ func (b *Backend) Push(req *pb.PushRequest, stream pb.Control_PushServer) error return err } - imageName, err := image.CheckAndAddDefaultTag(opt.imageName, opt.localStore) - if err != nil { - return err - } - opt.imageName = imageName - manifestType, gErr := exporter.GetManifestType(opt.format) if gErr != nil { return gErr diff --git a/daemon/save.go b/daemon/save.go index 35b67de..ee70691 100644 --- a/daemon/save.go +++ b/daemon/save.go @@ -16,6 +16,7 @@ package daemon import ( "context" "os" + "strings" "github.com/containers/image/v5/docker/reference" "github.com/containers/image/v5/types" @@ -76,7 +77,7 @@ func (b *Backend) Save(req *pb.SaveRequest, stream pb.Control_SaveServer) error var err error opts := b.getSaveOptions(req) - if err = checkFormatAndExpandTag(&opts); err != nil { + if err = checkFormat(&opts); err != nil { return err } if err = filterImageName(&opts); err != nil { @@ -159,7 +160,7 @@ func messageHandler(stream pb.Control_SaveServer, cliLogger *logger.Logger) func } } -func checkFormatAndExpandTag(opts *saveOptions) error { +func checkFormat(opts *saveOptions) error { switch opts.format { case constant.DockerTransport: opts.format = constant.DockerArchiveTransport @@ -169,14 +170,6 @@ func checkFormatAndExpandTag(opts *saveOptions) error { return errors.New("wrong image format provided") } - for i, imageName := range opts.oriImgList { - nameWithTag, err := image.CheckAndAddDefaultTag(imageName, opts.localStore) - if err != nil { - return errors.Wrapf(err, "check format and expand tag failed with image name %q", imageName) - } - opts.oriImgList[i] = nameWithTag - } - return nil } @@ -205,12 +198,11 @@ func filterImageName(opts *saveOptions) error { opts.finalImageOrdered = append(opts.finalImageOrdered, img.ID) } - ref, err := reference.Parse(imageName) - if err != nil { - return errors.Wrapf(err, "filter image name failed when parsing name %q", imageName) - } - tagged, withTag := ref.(reference.NamedTagged) - if withTag { + if !strings.HasPrefix(img.ID, imageName) { + tagged, _, err := image.GetNamedTaggedReference(imageName) + if err != nil { + return errors.Wrapf(err, "get named tagged reference failed when saving image %q", imageName) + } finalImage.tags = append(finalImage.tags, tagged) } opts.finalImageSet[img.ID] = finalImage diff --git a/exporter/docker/archive/archive.go b/exporter/docker/archive/archive.go index 36a2881..60e67fd 100644 --- a/exporter/docker/archive/archive.go +++ b/exporter/docker/archive/archive.go @@ -19,7 +19,6 @@ import ( "sync" "github.com/containers/image/v5/docker/archive" - "github.com/containers/image/v5/transports/alltransports" "github.com/containers/image/v5/types" "github.com/pkg/errors" @@ -50,24 +49,47 @@ func (d *dockerArchiveExporter) Name() string { } func (d *dockerArchiveExporter) Init(opts exporter.ExportOptions, src, destSpec string, localStore *store.Store) error { - var ( - srcReference types.ImageReference - destReference types.ImageReference - err error - ) + var archiveFilePath string + const partsNum = 2 - // src could be form of ImageID digest or name[:tag] + + // src is an imageid digest // destSpec could be "file:name:tag" or "file:name" or just "file" with transport "docker-archive", such as docker-archive:output.tar:name:tag - // When more than two parts, build must be called + if parts := strings.Split(destSpec, ":"); len(parts) < partsNum { + return errors.Errorf("image name %q is invalid", destSpec) + } else if len(parts) == partsNum { + archiveFilePath = strings.SplitN(destSpec, ":", partsNum)[1] + } else { + fileNameTag := strings.SplitN(destSpec, ":", partsNum)[1] + archiveFilePath = strings.SplitN(fileNameTag, ":", partsNum)[0] + } + + if DockerArchiveExporter.GetArchiveWriter(opts.ExportID) == nil { + archWriter, err := archive.NewWriter(opts.SystemContext, archiveFilePath) + if err != nil { + return errors.Wrapf(err, "create archive writer failed") + } + DockerArchiveExporter.InitArchiveWriter(opts.ExportID, archWriter) + } + + // when destSpec is more than two parts, build operation must be called if parts := strings.Split(destSpec, ":"); len(parts) > partsNum { - srcReference, _, err = image.FindImage(localStore, src) + srcReference, _, err := image.FindImage(localStore, src) if err != nil { return errors.Wrapf(err, "find src image: %q failed with transport %q", src, d.Name()) } - destReference, err = alltransports.ParseImageName(destSpec) + + // removing docker.io/library/ prefix by not using alltransports.ParseImageName + namedTagged, _, err := image.GetNamedTaggedReference(strings.Join(parts[2:], ":")) + if err != nil { + return errors.Wrapf(err, "get named tagged reference of image %q failed", strings.Join(parts[2:], ":")) + } + archiveWriter := DockerArchiveExporter.GetArchiveWriter(opts.ExportID) + destReference, err := archiveWriter.NewReference(namedTagged) if err != nil { return errors.Wrapf(err, "parse dest spec: %q failed with transport %q", destSpec, d.Name()) } + d.Lock() d.items[opts.ExportID] = exporter.Bus{ SrcRef: srcReference, @@ -79,23 +101,13 @@ func (d *dockerArchiveExporter) Init(opts exporter.ExportOptions, src, destSpec } // from build or save, we can get path from the other part - archiveFilePath := strings.SplitN(destSpec, ":", partsNum)[1] - - if DockerArchiveExporter.GetArchiveWriter(opts.ExportID) == nil { - archWriter, wErr := archive.NewWriter(opts.SystemContext, archiveFilePath) - if wErr != nil { - return errors.Wrapf(wErr, "create archive writer failed") - } - DockerArchiveExporter.InitArchiveWriter(opts.ExportID, archWriter) - } - - srcReference, _, err = image.FindImage(localStore, src) + srcReference, _, err := image.FindImage(localStore, src) if err != nil { return errors.Wrapf(err, "find src image: %q failed with transport %q", src, d.Name()) } archiveWriter := DockerArchiveExporter.GetArchiveWriter(opts.ExportID) - destReference, err = archiveWriter.NewReference(nil) + destReference, err := archiveWriter.NewReference(nil) if err != nil { return errors.Wrapf(err, "parse dest spec: %q failed", destSpec) } diff --git a/image/image.go b/image/image.go index b6210f2..5dda185 100644 --- a/image/image.go +++ b/image/image.go @@ -482,7 +482,7 @@ func FindImage(store *store.Store, image string) (types.ImageReference, *storage if err != nil { return nil, nil, errors.Wrapf(err, "failed to parse image %q in local store", localName) } - + return ref, img, nil } @@ -528,9 +528,8 @@ func tryResolveNameInStore(name string, store *store.Store) string { return img.ID } - defaultTag := "latest" - logrus.Infof("Try to find image: %s:%s in local storage", name, defaultTag) - img, err = store.Image(fmt.Sprintf("%s:%s", name, defaultTag)) + logrus.Infof("Try to find image: %s:%s in local storage", name, constant.DefaultTag) + img, err = store.Image(fmt.Sprintf("%s:%s", name, constant.DefaultTag)) if err != nil { return "" } @@ -621,25 +620,24 @@ func tryResolveNameInRegistries(name string, sc *types.SystemContext) ([]string, return candidates, constant.DockerTransport } -// CheckAndAddDefaultTag checks if src is format of repository[:tag], add default tag if src without tag -func CheckAndAddDefaultTag(imageName string, store *store.Store) (string, error) { - _, img, err := FindImage(store, imageName) - if err != nil { - return "", errors.Wrapf(err, "find src image: %q failed", imageName) +// GetNamedTaggedReference checks an image name, if it does not include a tag, default tag "latest" will be added to it. +func GetNamedTaggedReference(image string) (reference.NamedTagged, string, error) { + if image == "" { + return nil, "", nil } - defaultTag := "latest" - for _, name := range img.Names { - // for imageName is the format of repository[:tag] - if imageName == name { - return imageName, nil - } - // for name is the format of repository - if fmt.Sprintf("%s:%s", imageName, defaultTag) == name { - return name, nil - } + if slashLastIndex, sepLastIndex := strings.LastIndex(image, "/"), strings.LastIndex(image, ":"); sepLastIndex == -1 || (sepLastIndex < slashLastIndex) { + image = fmt.Sprintf("%s:%s", image, constant.DefaultTag) + } + + ref, err := reference.Parse(image) + if err != nil { + return nil, "", errors.Wrapf(err, "filter image name failed when parsing name %q", image) + } + tagged, withTag := ref.(reference.NamedTagged) + if !withTag { + return nil, "", errors.Errorf("image %q does not contain a tag even though the default tag is added", image) } - // for imageName is the format of imageID - return imageName, nil + return tagged, image, nil } diff --git a/image/image_test.go b/image/image_test.go index 43d936f..15b13e1 100644 --- a/image/image_test.go +++ b/image/image_test.go @@ -123,3 +123,99 @@ registries = [] assert.Assert(t, cmp.Contains(candidates, "localhost/busybox:latest")) assert.Equal(t, transport, constant.DockerTransport) } + +func TestGetNamedTaggedReference(t *testing.T) { + type testcase struct { + name string + tag string + output string + wantErr bool + errString string + } + testcases := []testcase{ + { + name: "test 1", + tag: "isula/test", + output: "isula/test:latest", + wantErr: false, + }, + { + name: "test 2", + tag: "localhost:5000/test", + output: "localhost:5000/test:latest", + wantErr: false, + }, + { + name: "test 3", + tag: "isula/test:latest", + output: "isula/test:latest", + wantErr: false, + }, + { + name: "test 4", + tag: "localhost:5000/test:latest", + output: "localhost:5000/test:latest", + wantErr: false, + }, + { + name: "test 5", + tag: "localhost:5000:aaa/test:latest", + output: "", + wantErr: true, + errString: "invalid reference format", + }, + { + name: "test 6", + tag: "localhost:5000:aaa/test", + output: "", + wantErr: true, + errString: "invalid reference format", + }, + { + name: "test 7", + tag: "localhost:5000/test:latest:latest", + output: "", + wantErr: true, + errString: "invalid reference format", + }, + { + name: "test 8", + tag: "test:latest:latest", + output: "", + wantErr: true, + errString: "invalid reference format", + }, + { + name: "test 9", + tag: "", + output: "", + wantErr: false, + }, + { + name: "test 10", + tag: "abc efg:latest", + output: "", + wantErr: true, + errString: "invalid reference format", + }, + { + name: "test 11", + tag: "abc!@#:latest", + output: "", + wantErr: true, + errString: "invalid reference format", + }, + } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + _, tag, err := GetNamedTaggedReference(tc.tag) + if !tc.wantErr { + assert.Equal(t, tag, tc.output, tc.name) + } + if tc.wantErr { + assert.ErrorContains(t, err, tc.errString) + } + }) + + } +} -- 1.8.3.1