From caf11183fa91a301402a00a302e81894861f9957 Mon Sep 17 00:00:00 2001
From: xingweizheng <xingweizheng@huawei.com>
Date: Sun, 22 Aug 2021 12:31:04 +0800
Subject: [PATCH 3/4] bugfix: fix save multiple tags single image failed

---
 daemon/save.go                     | 145 ++++++++++++++++++++++++-------------
 exporter/docker/archive/archive.go |  23 +-----
 image/image.go                     |  18 -----
 3 files changed, 95 insertions(+), 91 deletions(-)

diff --git a/daemon/save.go b/daemon/save.go
index de644c3..7ad1285 100644
--- a/daemon/save.go
+++ b/daemon/save.go
@@ -17,6 +17,7 @@ import (
 	"context"
 	"os"
 
+	"github.com/containers/image/v5/docker/reference"
 	"github.com/containers/image/v5/types"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -33,26 +34,30 @@ import (
 )
 
 type saveOptions struct {
-	sysCtx     *types.SystemContext
-	logger     *logger.Logger
-	localStore *store.Store
-	logEntry   *logrus.Entry
-	saveID     string
-	outputPath string
-	oriImgList []string
-	format     string
+	sysCtx            *types.SystemContext
+	localStore        *store.Store
+	saveID            string
+	format            string
+	oriImgList        []string
+	finalImageOrdered []string
+	finalImageSet     map[string][]reference.NamedTagged
+	outputPath        string
+	logger            *logger.Logger
+	logEntry          *logrus.Entry
 }
 
 func (b *Backend) getSaveOptions(req *pb.SaveRequest) saveOptions {
 	return saveOptions{
-		sysCtx:     image.GetSystemContext(),
-		logger:     logger.NewCliLogger(constant.CliLogBufferLen),
-		localStore: b.daemon.localStore,
-		saveID:     req.GetSaveID(),
-		outputPath: req.GetPath(),
-		oriImgList: req.GetImages(),
-		format:     req.GetFormat(),
-		logEntry:   logrus.WithFields(logrus.Fields{"SaveID": req.GetSaveID(), "Format": req.GetFormat()}),
+		sysCtx:            image.GetSystemContext(),
+		localStore:        b.daemon.localStore,
+		saveID:            req.GetSaveID(),
+		format:            req.GetFormat(),
+		oriImgList:        req.GetImages(),
+		finalImageOrdered: make([]string, 0),
+		finalImageSet:     make(map[string][]reference.NamedTagged),
+		outputPath:        req.GetPath(),
+		logger:            logger.NewCliLogger(constant.CliLogBufferLen),
+		logEntry:          logrus.WithFields(logrus.Fields{"SaveID": req.GetSaveID(), "Format": req.GetFormat()}),
 	}
 }
 
@@ -63,28 +68,14 @@ func (b *Backend) Save(req *pb.SaveRequest, stream pb.Control_SaveServer) error
 		"Format": req.GetFormat(),
 	}).Info("SaveRequest received")
 
-	var (
-		ok  bool
-		err error
-	)
-
+	var err error
 	opts := b.getSaveOptions(req)
 
-	switch opts.format {
-	case constant.DockerTransport:
-		opts.format = constant.DockerArchiveTransport
-	case constant.OCITransport:
-		opts.format = constant.OCIArchiveTransport
-	default:
-		return errors.New("wrong image format provided")
+	if err = checkFormatAndExpandTag(&opts); err != nil {
+		return err
 	}
-
-	for i, imageName := range opts.oriImgList {
-		nameWithTag, cErr := image.CheckAndAddDefaultTag(imageName, opts.localStore)
-		if cErr != nil {
-			return cErr
-		}
-		opts.oriImgList[i] = nameWithTag
+	if err = filterImageName(&opts); err != nil {
+		return err
 	}
 
 	defer func() {
@@ -98,26 +89,18 @@ func (b *Backend) Save(req *pb.SaveRequest, stream pb.Control_SaveServer) error
 	ctx := context.WithValue(stream.Context(), util.LogFieldKey(util.LogKeySessionID), opts.saveID)
 	eg, _ := errgroup.WithContext(ctx)
 
-	eg.Go(exportHandler(ctx, opts))
+	eg.Go(exportHandler(ctx, &opts))
 	eg.Go(messageHandler(stream, opts.logger))
-	errC := make(chan error, 1)
 
-	errC <- eg.Wait()
-	defer close(errC)
-
-	err, ok = <-errC
-	if !ok {
-		opts.logEntry.Info("Channel errC closed")
-		return nil
-	}
-	if err != nil {
+	if err = eg.Wait(); err != nil {
+		opts.logEntry.Warnf("Save stream closed with: %v", err)
 		return err
 	}
 
 	return nil
 }
 
-func exportHandler(ctx context.Context, opts saveOptions) func() error {
+func exportHandler(ctx context.Context, opts *saveOptions) func() error {
 	return func() error {
 		defer func() {
 			opts.logger.CloseContent()
@@ -129,18 +112,22 @@ func exportHandler(ctx context.Context, opts saveOptions) func() error {
 			}
 		}()
 
-		for _, imageID := range opts.oriImgList {
+		for _, imageID := range opts.finalImageOrdered {
+			copyCtx := *opts.sysCtx
+			// It's ok for DockerArchiveAdditionalTags == nil, as a result, no additional tags will be appended to the final archive file.
+			copyCtx.DockerArchiveAdditionalTags = opts.finalImageSet[imageID]
+
 			exOpts := exporter.ExportOptions{
 				Ctx:           ctx,
-				SystemContext: opts.sysCtx,
+				SystemContext: &copyCtx,
 				ExportID:      opts.saveID,
 				ReportWriter:  opts.logger,
 			}
 
 			if err := exporter.Export(imageID, exporter.FormatTransport(opts.format, opts.outputPath),
 				exOpts, opts.localStore); err != nil {
-				opts.logEntry.Errorf("Save Image %s output to %s failed with: %v", imageID, opts.format, err)
-				return errors.Wrapf(err, "save Image %s output to %s failed", imageID, opts.format)
+				opts.logEntry.Errorf("Save image %q in format %q failed: %v", imageID, opts.format, err)
+				return errors.Wrapf(err, "save image %q in format %q failed", imageID, opts.format)
 			}
 		}
 
@@ -164,3 +151,59 @@ func messageHandler(stream pb.Control_SaveServer, cliLogger *logger.Logger) func
 		return nil
 	}
 }
+
+func checkFormatAndExpandTag(opts *saveOptions) error {
+	switch opts.format {
+	case constant.DockerTransport:
+		opts.format = constant.DockerArchiveTransport
+	case constant.OCITransport:
+		opts.format = constant.OCIArchiveTransport
+	default:
+		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
+}
+
+func filterImageName(opts *saveOptions) error {
+	if opts.format == constant.OCIArchiveTransport {
+		opts.finalImageOrdered = opts.oriImgList
+		return nil
+	}
+
+	visitedImage := make(map[string]bool)
+	for _, imageName := range opts.oriImgList {
+		if _, exists := visitedImage[imageName]; exists {
+			continue
+		}
+		visitedImage[imageName] = true
+
+		_, img, err := image.FindImageLocally(opts.localStore, imageName)
+		if err != nil {
+			return errors.Wrapf(err, "filter image name failed when finding image name %q", imageName)
+		}
+		if _, ok := opts.finalImageSet[img.ID]; !ok {
+			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 {
+			continue
+		}
+		opts.finalImageSet[img.ID] = append(opts.finalImageSet[img.ID], tagged)
+	}
+
+	return nil
+}
diff --git a/exporter/docker/archive/archive.go b/exporter/docker/archive/archive.go
index 04654cf..cc6b872 100644
--- a/exporter/docker/archive/archive.go
+++ b/exporter/docker/archive/archive.go
@@ -21,9 +21,7 @@ import (
 	"github.com/containers/image/v5/docker/archive"
 	"github.com/containers/image/v5/transports/alltransports"
 	"github.com/containers/image/v5/types"
-	"github.com/docker/distribution/reference"
 	"github.com/pkg/errors"
-	"github.com/sirupsen/logrus"
 
 	constant "isula.org/isula-build"
 	"isula.org/isula-build/exporter"
@@ -91,32 +89,13 @@ func (d *dockerArchiveExporter) Init(opts exporter.ExportOptions, src, destSpec
 		DockerArchiveExporter.InitArchiveWriter(opts.ExportID, archWriter)
 	}
 
-	// There is a slightly difference between FindImageLocally and ParseImagesToReference to get a reference.
-	// FindImageLocally or FindImage, both result a reference with a nil named field of *storageReference.
-	// ParseImagesToReference returns a reference with non-nil named field of *storageReference that used to set destReference, if names is the format of name[:tag] with and without repository domain.
-
-	// If using  srcReferenceForDest to replace srcReference, When src is the format of name[:tag] without a registry domain name,
-	// in which time, cp.Image() will be called and new image source will call imageMatchesRepo() to check If image matches repository or not.
-	// ParseNormalizedNamed will finally called to add default docker.io/library/ prefix to name[:tag], return false result of the checking.
-	// As a result, we will get error of no image matching reference found.
 	srcReference, _, err = image.FindImageLocally(localStore, src)
 	if err != nil {
 		return errors.Wrapf(err, "find src image: %q failed with transport %q", src, d.Name())
 	}
 
-	imageReferenceForDest, _, err := image.ParseImagesToReference(localStore, []string{src})
-	if err != nil {
-		return errors.Wrapf(err, "parse image: %q to reference failed with transport %q", src, d.Name())
-	}
 	archiveWriter := DockerArchiveExporter.GetArchiveWriter(opts.ExportID)
-	nameAndTag, ok := imageReferenceForDest.DockerReference().(reference.NamedTagged)
-	// src is the format of ImageID, ok is false
-	if ok {
-		destReference, err = archiveWriter.NewReference(nameAndTag)
-	} else {
-		logrus.Infof("Transform image reference failed, use nil instead")
-		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 e06d253..5ae7245 100644
--- a/image/image.go
+++ b/image/image.go
@@ -504,8 +504,6 @@ func FindImageLocally(store *store.Store, image string) (types.ImageReference, *
 }
 
 // ParseImagesToReference get the image reference in store
-// When names is the format of ImageID (sha256), return ref with nil named field of *storageReference
-// When names is the format of name[:tag] with and without repository domain, such as registry.example.com/name:tag, name:tag, return corresponding ref with non-nil named field of *storageReference with and without domain
 func ParseImagesToReference(store *store.Store, names []string) (types.ImageReference, *storage.Image, error) {
 	var (
 		ref types.ImageReference
@@ -529,22 +527,6 @@ func ParseImagesToReference(store *store.Store, names []string) (types.ImageRefe
 				continue
 			}
 			img = img2
-
-			// For support export archive file, we need provide reference.Named field when names is the format of name[:tag] not the image ID
-			pRef, pErr := reference.Parse(name)
-			if pErr != nil {
-				return nil, nil, errors.Wrapf(pErr, "error parse name %q", name)
-			}
-			namedRef, isNamed := pRef.(reference.Named)
-			if !isNamed {
-				return nil, nil, errors.Errorf("reference %s has no name", pRef.String())
-			}
-
-			var nErr error
-			ref, nErr = is.Transport.NewStoreReference(store, namedRef, img2.ID)
-			if nErr != nil {
-				return nil, nil, errors.Wrap(nErr, "error get reference from store")
-			}
 		}
 		break
 	}
-- 
1.8.3.1

