Handle error gracefully when we can't retrieve an image

Author: nkubala

cmd/analyze.go

@@ -17,13 +17,13 @@ limitations under the License.
 package cmd
 
 import (
-	"errors"
 	"fmt"
 	"os"
 
 	"github.com/GoogleContainerTools/container-diff/cmd/util/output"
 	"github.com/GoogleContainerTools/container-diff/differs"
 	pkgutil "github.com/GoogleContainerTools/container-diff/pkg/util"
+	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 )
@@ -56,27 +56,27 @@ func checkAnalyzeArgNum(args []string) error {
 func analyzeImage(imageName string, analyzerArgs []string) error {
 	analyzeTypes, err := differs.GetAnalyzers(analyzerArgs)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "getting analyzers")
 	}
 
 	image, err := getImageForName(imageName)
 	if err != nil {
-		return err
+		return errors.Wrapf(err, "error retrieving image %s", imageName)
 	}
 
 	if noCache && !save {
 		defer pkgutil.CleanupImage(image)
 	}
 	if err != nil {
-		return fmt.Errorf("Error processing image: %s", err)
+		return fmt.Errorf("error processing image: %s", err)
 	}
 
 	req := differs.SingleRequest{
 		Image:        image,
 		AnalyzeTypes: analyzeTypes}
 	analyses, err := req.GetAnalysis()
 	if err != nil {
-		return fmt.Errorf("Error performing image analysis: %s", err)
+		return fmt.Errorf("error performing image analysis: %s", err)
 	}
 
 	logrus.Info("retrieving analyses")

cmd/diff.go

@@ -17,7 +17,6 @@ limitations under the License.
 package cmd
 
 import (
-	"errors"
 	"fmt"
 	"os"
 	"sync"
@@ -26,6 +25,7 @@ import (
 	"github.com/GoogleContainerTools/container-diff/differs"
 	pkgutil "github.com/GoogleContainerTools/container-diff/pkg/util"
 	"github.com/GoogleContainerTools/container-diff/util"
+	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 )
@@ -69,33 +69,58 @@ func checkFilenameFlag(_ []string) error {
 	return errors.New("Please include --types=file with the --filename flag")
 }
 
+func processImage(imageName string, imageMap map[string]*pkgutil.Image, wg *sync.WaitGroup, errChan chan<- error) {
+	defer wg.Done()
+	image, err := getImageForName(imageName)
+	if image.Image == nil {
+		errChan <- fmt.Errorf("error retrieving image %s: %s", imageName, err.Error())
+		return
+	}
+	if err != nil {
+		logrus.Warningf("diff may be inaccurate: %s", err)
+	}
+	imageMap[imageName] = &image
+}
+
 func diffImages(image1Arg, image2Arg string, diffArgs []string) error {
 	diffTypes, err := differs.GetAnalyzers(diffArgs)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "getting analyzers")
 	}
 
 	var wg sync.WaitGroup
 	wg.Add(2)
 
 	logrus.Infof("starting diff on images %s and %s, using differs: %s\n", image1Arg, image2Arg, diffArgs)
 
-	imageMap := map[string]*pkgutil.Image{
-		image1Arg: {},
-		image2Arg: {},
+	imageMap := map[string]*pkgutil.Image{}
+	errChan := make(chan error, 2)
+
+	go processImage(image1Arg, imageMap, &wg, errChan)
+	go processImage(image2Arg, imageMap, &wg, errChan)
+
+	wg.Wait()
+	err, ok := <-errChan
+	errs := []error{}
+	if ok {
+		errs = append(errs, err)
 	}
-	// TODO: fix error handling here
-	for imageArg := range imageMap {
-		go func(imageName string, imageMap map[string]*pkgutil.Image) {
-			defer wg.Done()
-			image, err := getImageForName(imageName)
-			imageMap[imageName] = &image
-			if err != nil {
-				logrus.Warningf("Diff may be inaccurate: %s", err)
-			}
-		}(imageArg, imageMap)
+	if len(errs) > 0 {
+		errStr := ""
+		for _, err := range errs {
+			errStr = errStr + err.Error()
+		}
+		return errors.New(errStr)
+	}
+
+	img1, ok := imageMap[image1Arg]
+	if !ok {
+		return fmt.Errorf("cannot find image %s", image1Arg)
+	}
+	img2, ok := imageMap[image2Arg]
+	if !ok {
+		return fmt.Errorf("cannot find image %s", image2Arg)
 	}
-	wg.Wait()
 
 	if noCache && !save {
 		defer pkgutil.CleanupImage(*imageMap[image1Arg])
@@ -104,8 +129,8 @@ func diffImages(image1Arg, image2Arg string, diffArgs []string) error {
 
 	logrus.Info("computing diffs")
 	req := differs.DiffRequest{
-		Image1:    *imageMap[image1Arg],
-		Image2:    *imageMap[image2Arg],
+		Image1:    *img1,
+		Image2:    *img2,
 		DiffTypes: diffTypes}
 	diffs, err := req.GetDiff()
 	if err != nil {

cmd/root.go

@@ -38,6 +38,7 @@ import (
 	"github.com/GoogleContainerTools/container-diff/util"
 	"github.com/google/go-containerregistry/pkg/v1"
 	homedir "github.com/mitchellh/go-homedir"
+	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
@@ -139,17 +140,17 @@ func getImageForName(imageName string) (pkgutil.Image, error) {
 		start := time.Now()
 		img, err = tarball.ImageFromPath(imageName, nil)
 		if err != nil {
-			return pkgutil.Image{}, err
+			return pkgutil.Image{}, errors.Wrap(err, "retrieving tar from path")
 		}
 		elapsed := time.Now().Sub(start)
-		logrus.Infof("retrieving image from tar took %f seconds", elapsed.Seconds())
+		logrus.Infof("retrieving image ref from tar took %f seconds", elapsed.Seconds())
 	} else if strings.HasPrefix(imageName, DaemonPrefix) {
 		// remove the daemon prefix
 		imageName = strings.Replace(imageName, DaemonPrefix, "", -1)
 
 		ref, err := name.ParseReference(imageName, name.WeakValidation)
 		if err != nil {
-			return pkgutil.Image{}, err
+			return pkgutil.Image{}, errors.Wrap(err, "parsing image reference")
 		}
 
 		start := time.Now()
@@ -158,28 +159,28 @@ func getImageForName(imageName string) (pkgutil.Image, error) {
 			Buffer: true,
 		})
 		if err != nil {
-			return pkgutil.Image{}, err
+			return pkgutil.Image{}, errors.Wrap(err, "retrieving image from daemon")
 		}
 		elapsed := time.Now().Sub(start)
-		logrus.Infof("retrieving image from daemon took %f seconds", elapsed.Seconds())
+		logrus.Infof("retrieving local image ref took %f seconds", elapsed.Seconds())
 	} else {
 		// either has remote prefix or has no prefix, in which case we force remote
 		imageName = strings.Replace(imageName, RemotePrefix, "", -1)
 		ref, err := name.ParseReference(imageName, name.WeakValidation)
 		if err != nil {
-			return pkgutil.Image{}, err
+			return pkgutil.Image{}, errors.Wrap(err, "parsing image reference")
 		}
 		auth, err := authn.DefaultKeychain.Resolve(ref.Context().Registry)
 		if err != nil {
-			return pkgutil.Image{}, err
+			return pkgutil.Image{}, errors.Wrap(err, "resolving auth")
 		}
 		start := time.Now()
 		img, err = remote.Image(ref, auth, http.DefaultTransport)
 		if err != nil {
-			return pkgutil.Image{}, err
+			return pkgutil.Image{}, errors.Wrap(err, "retrieving remote image")
 		}
 		elapsed := time.Now().Sub(start)
-		logrus.Infof("retrieving remote image took %f seconds", elapsed.Seconds())
+		logrus.Infof("retrieving remote image ref took %f seconds", elapsed.Seconds())
 	}
 
 	// create tempdir and extract fs into it
@@ -188,7 +189,7 @@ func getImageForName(imageName string) (pkgutil.Image, error) {
 		start := time.Now()
 		imgLayers, err := img.Layers()
 		if err != nil {
-			return pkgutil.Image{}, err
+			return pkgutil.Image{}, errors.Wrap(err, "getting image layers")
 		}
 		for _, layer := range imgLayers {
 			layerStart := time.Now()
@@ -197,12 +198,12 @@ func getImageForName(imageName string) (pkgutil.Image, error) {
 			if err != nil {
 				return pkgutil.Image{
 					Layers: layers,
-				}, err
+				}, errors.Wrap(err, "getting extract path for layer")
 			}
 			if err := pkgutil.GetFileSystemForLayer(layer, path, nil); err != nil {
 				return pkgutil.Image{
 					Layers: layers,
-				}, err
+				}, errors.Wrap(err, "getting filesystem for layer")
 			}
 			layers = append(layers, pkgutil.Layer{
 				FSPath: path,
@@ -215,12 +216,15 @@ func getImageForName(imageName string) (pkgutil.Image, error) {
 	}
 
 	path, err := getExtractPathForImage(imageName, img)
+	if err != nil {
+		return pkgutil.Image{}, err
+	}
 	// extract fs into provided dir
 	if err := pkgutil.GetFileSystemForImage(img, path, nil); err != nil {
 		return pkgutil.Image{
 			FSPath: path,
 			Layers: layers,
-		}, err
+		}, errors.Wrap(err, "getting filesystem for image")
 	}
 	return pkgutil.Image{
 		Image:  img,

differs/differs.go

@@ -63,13 +63,13 @@ func (req DiffRequest) GetDiff() (map[string]util.Result, error) {
 		if diff, err := differ.Diff(img1, img2); err == nil {
 			results[differ.Name()] = diff
 		} else {
-			logrus.Errorf("Error getting diff with %s: %s", differ.Name(), err)
+			logrus.Errorf("error getting diff with %s: %s", differ.Name(), err)
 		}
 	}
 
 	var err error
 	if len(results) == 0 {
-		err = fmt.Errorf("Could not perform diff on %v and %v", img1, img2)
+		err = fmt.Errorf("could not perform diff on %v and %v", img1, img2)
 	} else {
 		err = nil
 	}
@@ -87,13 +87,13 @@ func (req SingleRequest) GetAnalysis() (map[string]util.Result, error) {
 		if analysis, err := analyzer.Analyze(img); err == nil {
 			results[analyzeName] = analysis
 		} else {
-			logrus.Errorf("Error getting analysis with %s: %s", analyzeName, err)
+			logrus.Errorf("error getting analysis with %s: %s", analyzeName, err)
 		}
 	}
 
 	var err error
 	if len(results) == 0 {
-		err = fmt.Errorf("Could not perform analysis on %v", img)
+		err = fmt.Errorf("could not perform analysis on %v", img)
 	} else {
 		err = nil
 	}
@@ -107,11 +107,11 @@ func GetAnalyzers(analyzeNames []string) ([]Analyzer, error) {
 		if a, exists := Analyzers[name]; exists {
 			analyzeFuncs = append(analyzeFuncs, a)
 		} else {
-			return nil, fmt.Errorf("Unknown analyzer/differ specified: %s", name)
+			return nil, fmt.Errorf("unknown analyzer/differ specified: %s", name)
 		}
 	}
 	if len(analyzeFuncs) == 0 {
-		return nil, fmt.Errorf("No known analyzers/differs specified")
+		return nil, fmt.Errorf("no known analyzers/differs specified")
 	}
 	return analyzeFuncs, nil
 }