Merge pull request #251 from nkubala/error_handling

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/analyze_test.go

@@ -29,11 +29,11 @@ var analyzeArgNumTests = []testpair{
 func TestAnalyzeArgNum(t *testing.T) {
 	for _, test := range analyzeArgNumTests {
 		err := checkAnalyzeArgNum(test.input)
-		if (err == nil) != test.expected_output {
-			if test.expected_output {
-				t.Errorf("Got unexpected error: %s", err)
-			} else {
+		if (err == nil) != test.shouldError {
+			if test.shouldError {
 				t.Errorf("Expected error but got none")
+			} else {
+				t.Errorf("Got unexpected error: %s", err)
 			}
 		}
 	}

cmd/diff.go

@@ -17,15 +17,16 @@ limitations under the License.
 package cmd
 
 import (
-	"errors"
 	"fmt"
 	"os"
+	"strings"
 	"sync"
 
 	"github.com/GoogleContainerTools/container-diff/cmd/util/output"
 	"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"
 )
@@ -66,50 +67,74 @@ func checkFilenameFlag(_ []string) error {
 			return nil
 		}
 	}
-	return errors.New("Please include --types=file with the --filename flag")
+	return errors.New("please include --types=file with the --filename flag")
+}
+
+// processImage is a concurrency-friendly wrapper around getImageForName
+func processImage(imageName string, imageMap map[string]*pkgutil.Image, wg *sync.WaitGroup, errChan chan<- error) {
+	defer wg.Done()
+	image, err := getImageForName(imageName)
+	if err != nil {
+		errChan <- fmt.Errorf("error retrieving image %s: %s", imageName, err)
+	}
+	imageMap[imageName] = &image
+}
+
+// collects errors from a channel and combines them
+// assumes channel has already been closed
+func readErrorsFromChannel(c chan error) error {
+	errs := []string{}
+	for {
+		err, ok := <-c
+		if !ok {
+			break
+		}
+		errs = append(errs, err.Error())
+	}
+
+	if len(errs) > 0 {
+		return errors.New(strings.Join(errs, "\n"))
+	}
+	return nil
 }
 
 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: {},
-	}
-	// 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)
-	}
+	imageMap := map[string]*pkgutil.Image{}
+	errChan := make(chan error, 2)
+
+	go processImage(image1Arg, imageMap, &wg, errChan)
+	go processImage(image2Arg, imageMap, &wg, errChan)
+
 	wg.Wait()
+	close(errChan)
 
 	if noCache && !save {
 		defer pkgutil.CleanupImage(*imageMap[image1Arg])
 		defer pkgutil.CleanupImage(*imageMap[image2Arg])
 	}
 
+	if err := readErrorsFromChannel(errChan); err != nil {
+		return err
+	}
+
 	logrus.Info("computing diffs")
 	req := differs.DiffRequest{
 		Image1:    *imageMap[image1Arg],
 		Image2:    *imageMap[image2Arg],
 		DiffTypes: diffTypes}
 	diffs, err := req.GetDiff()
 	if err != nil {
-		return fmt.Errorf("Could not retrieve diff: %s", err)
+		return fmt.Errorf("could not retrieve diff: %s", err)
 	}
 	outputResults(diffs)
 
@@ -122,7 +147,7 @@ func diffImages(image1Arg, image2Arg string, diffArgs []string) error {
 	}
 
 	if noCache && save {
-		logrus.Infof("Images were saved at %s and %s", imageMap[image1Arg].FSPath,
+		logrus.Infof("images were saved at %s and %s", imageMap[image1Arg].FSPath,
 			imageMap[image2Arg].FSPath)
 	}
 	return nil

cmd/diff_test.go

@@ -21,21 +21,46 @@ import (
 )
 
 var diffArgNumTests = []testpair{
-	{[]string{}, false},
-	{[]string{"one"}, false},
-	{[]string{"one", "two"}, true},
-	{[]string{"one", "two", "three"}, false},
+	{[]string{}, true},
+	{[]string{"one"}, true},
+	{[]string{"one", "two"}, false},
+	{[]string{"one", "two", "three"}, true},
 }
 
 func TestDiffArgNum(t *testing.T) {
 	for _, test := range diffArgNumTests {
 		err := checkDiffArgNum(test.input)
-		if (err == nil) != test.expected_output {
-			if test.expected_output {
-				t.Errorf("Got unexpected error: %s", err)
-			} else {
-				t.Errorf("Expected error but got none")
-			}
+		checkError(t, err, test.shouldError)
+	}
+}
+
+type imageDiff struct {
+	image1      string
+	image2      string
+	shouldError bool
+}
+
+var imageDiffs = []imageDiff{
+	{"", "", true},
+	{"gcr.io/google-appengine/python", "gcr.io/google-appengine/debian9", false},
+	{"gcr.io/google-appengine/python", "cats", true},
+}
+
+func TestDiffImages(t *testing.T) {
+	for _, test := range imageDiffs {
+		err := diffImages(test.image1, test.image2, []string{"apt"})
+		checkError(t, err, test.shouldError)
+		err = diffImages(test.image1, test.image2, []string{"metadata"})
+		checkError(t, err, test.shouldError)
+	}
+}
+
+func checkError(t *testing.T, err error, shouldError bool) {
+	if (err == nil) == shouldError {
+		if shouldError {
+			t.Errorf("expected error but got none")
+		} else {
+			t.Errorf("got unexpected error: %s", err)
 		}
 	}
 }

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"
@@ -131,6 +132,9 @@ func checkIfValidAnalyzer(_ []string) error {
 	return nil
 }
 
+// getImageForName infers the source of an image and retrieves a v1.Image reference to it.
+// Once a reference is obtained, it attempts to unpack the v1.Image's reader's contents
+// into a temp directory on the local filesystem.
 func getImageForName(imageName string) (pkgutil.Image, error) {
 	logrus.Infof("retrieving image: %s", imageName)
 	var img v1.Image
@@ -139,17 +143,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 +162,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 +192,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 +201,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 +219,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,

cmd/root_test.go

@@ -17,6 +17,6 @@ limitations under the License.
 package cmd
 
 type testpair struct {
-	input           []string
-	expected_output bool
+	input       []string
+	shouldError bool
 }