From a3638072985a0cb71ff561ad5e5bbc2454f81c1f Mon Sep 17 00:00:00 2001 From: DCCooper <1866858@gmail.com> Date: Wed, 8 Dec 2021 12:51:20 +0800 Subject: [PATCH 2/2] isula-build: fix problems found by code review Signed-off-by: DCCooper <1866858@gmail.com> --- daemon/load.go | 67 ++++++++++++++++++++++++------------------------- daemon/save.go | 79 ++++++++++++++++++++-------------------------------------- image/image.go | 3 ++- util/cipher.go | 8 +++++- 4 files changed, 69 insertions(+), 88 deletions(-) diff --git a/daemon/load.go b/daemon/load.go index 378325c..894159b 100644 --- a/daemon/load.go +++ b/daemon/load.go @@ -69,9 +69,9 @@ type separatorLoad struct { } type loadOptions struct { + logEntry *logrus.Entry path string format string - logEntry *logrus.Entry sep separatorLoad } @@ -355,7 +355,7 @@ func (s *separatorLoad) getTarballInfo() error { return errors.Wrap(err, "join manifest file path failed") } - var t = make(map[string]tarballInfo) + var t = make(map[string]tarballInfo, 1) if err = util.LoadJSONFile(manifest, &t); err != nil { return errors.Wrap(err, "load manifest file failed") } @@ -370,7 +370,7 @@ func (s *separatorLoad) getTarballInfo() error { } func (s *separatorLoad) constructTarballInfo() (err error) { - s.log.Infof("construct image tarball info for %s", s.appName) + s.log.Infof("Construct image tarball info for %s", s.appName) // fill up path for separator // this case should not happened since client side already check this flag if len(s.appName) == 0 { @@ -408,26 +408,25 @@ func (s *separatorLoad) tarballCheckSum() error { return nil } - // app image tarball can not be empty - if len(s.appPath) == 0 { - return errors.New("app image tarball path can not be empty") - } - if err := util.CheckSum(s.appPath, s.info.AppHash); err != nil { - return errors.Wrapf(err, "check sum for file %q failed", s.appPath) - } - - // base image tarball can not be empty - if len(s.basePath) == 0 { - return errors.New("base image tarball path can not be empty") - } - if err := util.CheckSum(s.basePath, s.info.BaseHash); err != nil { - return errors.Wrapf(err, "check sum for file %q failed", s.basePath) - } - - // lib image may be empty image - if len(s.libPath) != 0 { - if err := util.CheckSum(s.libPath, s.info.LibHash); err != nil { - return errors.Wrapf(err, "check sum for file %q failed", s.libPath) + type checkInfo struct { + path string + hash string + str string + canBeEmpty bool + } + checkLen := 3 + var checkList = make([]checkInfo, 0, checkLen) + checkList = append(checkList, checkInfo{path: s.basePath, hash: s.info.BaseHash, canBeEmpty: false, str: "base image"}) + checkList = append(checkList, checkInfo{path: s.libPath, hash: s.info.LibHash, canBeEmpty: true, str: "lib image"}) + checkList = append(checkList, checkInfo{path: s.appPath, hash: s.info.AppHash, canBeEmpty: false, str: "app image"}) + for _, p := range checkList { + if len(p.path) == 0 && !p.canBeEmpty { + return errors.Errorf("%s tarball path can not be empty", p.str) + } + if len(p.path) != 0 { + if err := util.CheckSum(p.path, p.hash); err != nil { + return errors.Wrapf(err, "check sum for file %q failed", p.path) + } } } @@ -457,18 +456,18 @@ func (s *separatorLoad) unpackTarballs() error { return errors.Wrap(err, "failed to make temporary directories") } - // unpack base first and the later images will be moved here - if err := util.UnpackFile(s.basePath, s.tmpDir.base, archive.Gzip, false); err != nil { - return errors.Wrapf(err, "unpack base tarball %q failed", s.basePath) - } - - if err := util.UnpackFile(s.appPath, s.tmpDir.app, archive.Gzip, false); err != nil { - return errors.Wrapf(err, "unpack app tarball %q failed", s.appPath) - } + type unpackInfo struct{ path, dir, str string } + unpackLen := 3 + var unpackList = make([]unpackInfo, 0, unpackLen) + unpackList = append(unpackList, unpackInfo{path: s.basePath, dir: s.tmpDir.base, str: "base image"}) + unpackList = append(unpackList, unpackInfo{path: s.appPath, dir: s.tmpDir.app, str: "app image"}) + unpackList = append(unpackList, unpackInfo{path: s.libPath, dir: s.tmpDir.lib, str: "lib image"}) - if len(s.libPath) != 0 { - if err := util.UnpackFile(s.libPath, s.tmpDir.lib, archive.Gzip, false); err != nil { - return errors.Wrapf(err, "unpack lib tarball %q failed", s.libPath) + for _, p := range unpackList { + if len(p.path) != 0 { + if err := util.UnpackFile(p.path, p.dir, archive.Gzip, false); err != nil { + return errors.Wrapf(err, "unpack %s tarball %q failed", p.str, p.path) + } } } diff --git a/daemon/save.go b/daemon/save.go index f14a485..7a110bd 100644 --- a/daemon/save.go +++ b/daemon/save.go @@ -77,9 +77,9 @@ type saveOptions struct { } type separatorSave struct { + log *logrus.Entry renameData []renames tmpDir imageTmpDir - log *logrus.Entry base string lib string dest string @@ -190,7 +190,7 @@ func (b *Backend) Save(req *pb.SaveRequest, stream pb.Control_SaveServer) (err e }).Info("SaveRequest received") opts := b.getSaveOptions(req) - if err = opts.check(); err != nil { + if err = opts.manage(); err != nil { return errors.Wrap(err, "check save options failed") } @@ -278,17 +278,17 @@ func messageHandler(stream pb.Control_SaveServer, cliLogger *logger.Logger) func } } -func (opts *saveOptions) check() error { +func (opts *saveOptions) manage() error { if err := opts.checkImageNameIsID(); err != nil { return err } - if err := opts.checkFormat(); err != nil { + if err := opts.setFormat(); err != nil { return err } if err := opts.filterImageName(); err != nil { return err } - if err := opts.checkRenameFile(); err != nil { + if err := opts.loadRenameFile(); err != nil { return err } @@ -318,7 +318,7 @@ func (opts *saveOptions) checkImageNameIsID() error { return nil } -func (opts *saveOptions) checkFormat() error { +func (opts *saveOptions) setFormat() error { switch opts.format { case constant.DockerTransport: opts.format = constant.DockerArchiveTransport @@ -337,7 +337,7 @@ func (opts *saveOptions) filterImageName() error { return nil } - visitedImage := make(map[string]bool) + visitedImage := make(map[string]bool, 1) for _, imageName := range opts.oriImgList { if _, exists := visitedImage[imageName]; exists { continue @@ -351,8 +351,7 @@ func (opts *saveOptions) filterImageName() error { finalImage, ok := opts.finalImageSet[img.ID] if !ok { - finalImage = &savedImage{exist: true} - finalImage.tags = []reference.NamedTagged{} + finalImage = &savedImage{exist: true, tags: []reference.NamedTagged{}} opts.finalImageOrdered = append(opts.finalImageOrdered, img.ID) } @@ -369,7 +368,7 @@ func (opts *saveOptions) filterImageName() error { return nil } -func (opts *saveOptions) checkRenameFile() error { +func (opts *saveOptions) loadRenameFile() error { if len(opts.sep.renameFile) != 0 { var reName []renames if err := util.LoadJSONFile(opts.sep.renameFile, &reName); err != nil { @@ -494,12 +493,11 @@ func (s *separatorSave) adjustLayers() ([]imageManifest, error) { return man, nil } -func separateImage(opt saveOptions) error { +func separateImage(opt saveOptions) (err error) { s := &opt.sep s.log.Infof("Start saving separated images %v", opt.oriImgList) - var errList []error - if err := os.MkdirAll(s.dest, constant.DefaultRootDirMode); err != nil { + if err = os.MkdirAll(s.dest, constant.DefaultRootDirMode); err != nil { return err } @@ -507,30 +505,26 @@ func separateImage(opt saveOptions) error { if tErr := os.RemoveAll(s.tmpDir.root); tErr != nil && !os.IsNotExist(tErr) { s.log.Warnf("Removing save tmp directory %q failed: %v", s.tmpDir.root, tErr) } - if len(errList) != 0 { + if err != nil { if rErr := os.RemoveAll(s.dest); rErr != nil && !os.IsNotExist(rErr) { s.log.Warnf("Removing save dest directory %q failed: %v", s.dest, rErr) } } }() - if err := util.UnpackFile(opt.outputPath, s.tmpDir.untar, archive.Gzip, true); err != nil { - errList = append(errList, err) + if err = util.UnpackFile(opt.outputPath, s.tmpDir.untar, archive.Gzip, true); err != nil { return errors.Wrapf(err, "unpack %q failed", opt.outputPath) } - manifest, err := s.adjustLayers() - if err != nil { - errList = append(errList, err) - return errors.Wrap(err, "adjust layers failed") + manifest, aErr := s.adjustLayers() + if aErr != nil { + return errors.Wrap(aErr, "adjust layers failed") } - imgInfos, err := s.constructImageInfos(manifest, opt.localStore) - if err != nil { - errList = append(errList, err) - return errors.Wrap(err, "process image infos failed") + imgInfos, cErr := s.constructImageInfos(manifest, opt.localStore) + if cErr != nil { + return errors.Wrap(cErr, "process image infos failed") } - if err := s.processImageLayers(imgInfos); err != nil { - errList = append(errList, err) + if err = s.processImageLayers(imgInfos); err != nil { return err } @@ -552,7 +546,7 @@ func (s *separatorSave) processImageLayers(imgInfos map[string]imageInfo) error sort.Strings(sortedKey) for _, k := range sortedKey { info := imgInfos[k] - if err := s.clearDirs(true); err != nil { + if err := s.clearTempDirs(); err != nil { return errors.Wrap(err, "clear tmp dirs failed") } var t tarballInfo @@ -584,32 +578,13 @@ func (s *separatorSave) processImageLayers(imgInfos map[string]imageInfo) error return nil } -func (s *separatorSave) clearDirs(reCreate bool) error { - tmpDir := s.tmpDir - dirs := []string{tmpDir.base, tmpDir.app, tmpDir.lib} - var mkTmpDirs = func(dirs []string) error { - for _, dir := range dirs { - if err := os.MkdirAll(dir, constant.DefaultRootDirMode); err != nil { - return err - } - } - return nil - } - - var rmTmpDirs = func(dirs []string) error { - for _, dir := range dirs { - if err := os.RemoveAll(dir); err != nil { - return err - } +func (s *separatorSave) clearTempDirs() error { + dirs := []string{s.tmpDir.base, s.tmpDir.app, s.tmpDir.lib} + for _, dir := range dirs { + if err := os.RemoveAll(dir); err != nil { + return err } - return nil - } - - if err := rmTmpDirs(dirs); err != nil { - return err - } - if reCreate { - if err := mkTmpDirs(dirs); err != nil { + if err := os.MkdirAll(dir, constant.DefaultRootDirMode); err != nil { return err } } diff --git a/image/image.go b/image/image.go index b24cb41..37cd7fa 100644 --- a/image/image.go +++ b/image/image.go @@ -626,7 +626,8 @@ func GetNamedTaggedReference(image string) (reference.NamedTagged, string, error return nil, "", nil } - if slashLastIndex, sepLastIndex := strings.LastIndex(image, "/"), strings.LastIndex(image, ":"); sepLastIndex == -1 || (sepLastIndex < slashLastIndex) { + slashLastIndex, sepLastIndex := strings.LastIndex(image, "/"), strings.LastIndex(image, ":") + if sepLastIndex == -1 || (sepLastIndex < slashLastIndex) { image = fmt.Sprintf("%s:%s", image, constant.DefaultTag) } diff --git a/util/cipher.go b/util/cipher.go index a5e3125..67cb52b 100644 --- a/util/cipher.go +++ b/util/cipher.go @@ -212,6 +212,9 @@ func GenRSAPublicKeyFile(key *rsa.PrivateKey, path string) error { if err := pem.Encode(file, block); err != nil { return err } + if cErr := file.Close(); cErr != nil { + return cErr + } return nil } @@ -230,7 +233,10 @@ func ReadPublicKey(path string) (rsa.PublicKey, error) { if err != nil { return rsa.PublicKey{}, err } - key := pubInterface.(*rsa.PublicKey) + key, ok := pubInterface.(*rsa.PublicKey) + if !ok { + return rsa.PublicKey{}, errors.New("failed to find public key type") + } return *key, nil } -- 1.8.3.1