From 8621bc5d693f507a5a5f3cfa99d1315701dcf6bb Mon Sep 17 00:00:00 2001 From: Nick Kubala Date: Thu, 26 Jul 2018 12:22:19 -0700 Subject: [PATCH 1/6] Handle error gracefully when we can't retrieve an image --- cmd/analyze.go | 10 ++++---- cmd/diff.go | 61 ++++++++++++++++++++++++++++++++-------------- cmd/root.go | 30 +++++++++++++---------- differs/differs.go | 12 ++++----- 4 files changed, 71 insertions(+), 42 deletions(-) diff --git a/cmd/analyze.go b/cmd/analyze.go index 95a5e184..e8000968 100644 --- a/cmd/analyze.go +++ b/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,19 +56,19 @@ 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{ @@ -76,7 +76,7 @@ func analyzeImage(imageName string, analyzerArgs []string) error { 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") diff --git a/cmd/diff.go b/cmd/diff.go index 61eceab5..e9f5cc35 100644 --- a/cmd/diff.go +++ b/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,10 +69,23 @@ 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 @@ -80,22 +93,34 @@ func diffImages(image1Arg, image2Arg string, diffArgs []string) error { 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 { diff --git a/cmd/root.go b/cmd/root.go index e2a949da..528ae2ff 100644 --- a/cmd/root.go +++ b/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, diff --git a/differs/differs.go b/differs/differs.go index c8c617b2..19913cb4 100644 --- a/differs/differs.go +++ b/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 } From ee51ff8f13c7a59da1e07cc43116200a96c7ee5e Mon Sep 17 00:00:00 2001 From: Nick Kubala Date: Thu, 26 Jul 2018 14:06:50 -0700 Subject: [PATCH 2/6] fix channel read --- cmd/diff.go | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/cmd/diff.go b/cmd/diff.go index e9f5cc35..2ef1e8ce 100644 --- a/cmd/diff.go +++ b/cmd/diff.go @@ -19,6 +19,7 @@ package cmd import ( "fmt" "os" + "strings" "sync" "github.com/GoogleContainerTools/container-diff/cmd/util/output" @@ -100,17 +101,19 @@ func diffImages(image1Arg, image2Arg string, diffArgs []string) error { go processImage(image2Arg, imageMap, &wg, errChan) wg.Wait() - err, ok := <-errChan - errs := []error{} - if ok { - errs = append(errs, err) + close(errChan) + + errs := []string{} + for { + err, ok := <-errChan + if !ok { + break + } + errs = append(errs, err.Error()) } + if len(errs) > 0 { - errStr := "" - for _, err := range errs { - errStr = errStr + err.Error() - } - return errors.New(errStr) + return errors.New(strings.Join(errs, "\n")) } img1, ok := imageMap[image1Arg] From 6a7a550f3746d575716ecf310890039872079d10 Mon Sep 17 00:00:00 2001 From: Nick Kubala Date: Fri, 27 Jul 2018 11:54:03 -0700 Subject: [PATCH 3/6] improve error handling logic when retrieving images --- cmd/diff.go | 35 +++++++++++------------------------ 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/cmd/diff.go b/cmd/diff.go index 2ef1e8ce..f30b5eef 100644 --- a/cmd/diff.go +++ b/cmd/diff.go @@ -67,18 +67,14 @@ 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") } 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) + errChan <- fmt.Errorf("error retrieving image %s: %s", imageName, err) } imageMap[imageName] = &image } @@ -103,6 +99,11 @@ func diffImages(image1Arg, image2Arg string, diffArgs []string) error { wg.Wait() close(errChan) + if noCache && !save { + defer pkgutil.CleanupImage(*imageMap[image1Arg]) + defer pkgutil.CleanupImage(*imageMap[image2Arg]) + } + errs := []string{} for { err, ok := <-errChan @@ -116,28 +117,14 @@ func diffImages(image1Arg, image2Arg string, diffArgs []string) error { return errors.New(strings.Join(errs, "\n")) } - 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) - } - - if noCache && !save { - defer pkgutil.CleanupImage(*imageMap[image1Arg]) - defer pkgutil.CleanupImage(*imageMap[image2Arg]) - } - logrus.Info("computing diffs") req := differs.DiffRequest{ - Image1: *img1, - Image2: *img2, + 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) @@ -150,7 +137,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 From 7024d3b46b41db9188e58b9cee366f96d287b731 Mon Sep 17 00:00:00 2001 From: Nick Kubala Date: Fri, 27 Jul 2018 12:04:37 -0700 Subject: [PATCH 4/6] move error combining logic into helper function --- cmd/diff.go | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/cmd/diff.go b/cmd/diff.go index f30b5eef..c6f91ef2 100644 --- a/cmd/diff.go +++ b/cmd/diff.go @@ -79,6 +79,24 @@ func processImage(imageName string, imageMap map[string]*pkgutil.Image, wg *sync 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 { @@ -104,17 +122,8 @@ func diffImages(image1Arg, image2Arg string, diffArgs []string) error { defer pkgutil.CleanupImage(*imageMap[image2Arg]) } - errs := []string{} - for { - err, ok := <-errChan - if !ok { - break - } - errs = append(errs, err.Error()) - } - - if len(errs) > 0 { - return errors.New(strings.Join(errs, "\n")) + if err := readErrorsFromChannel(errChan); err != nil { + return err } logrus.Info("computing diffs") From 265b622caa075666a91480e2c1dbca6b97078be0 Mon Sep 17 00:00:00 2001 From: Nick Kubala Date: Fri, 27 Jul 2018 12:08:07 -0700 Subject: [PATCH 5/6] add comments for functions --- cmd/diff.go | 1 + cmd/root.go | 3 +++ 2 files changed, 4 insertions(+) diff --git a/cmd/diff.go b/cmd/diff.go index c6f91ef2..8596c559 100644 --- a/cmd/diff.go +++ b/cmd/diff.go @@ -70,6 +70,7 @@ func checkFilenameFlag(_ []string) error { 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) diff --git a/cmd/root.go b/cmd/root.go index 528ae2ff..491a0ed5 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -132,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 From 9c4791164690216bc7257b083e09a5b0d7329bb9 Mon Sep 17 00:00:00 2001 From: Nick Kubala Date: Fri, 27 Jul 2018 14:18:56 -0700 Subject: [PATCH 6/6] add tests --- cmd/analyze_test.go | 8 ++++---- cmd/diff_test.go | 45 +++++++++++++++++++++++++++++++++++---------- cmd/root_test.go | 4 ++-- 3 files changed, 41 insertions(+), 16 deletions(-) diff --git a/cmd/analyze_test.go b/cmd/analyze_test.go index b53ff1a4..827a9fb9 100644 --- a/cmd/analyze_test.go +++ b/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) } } } diff --git a/cmd/diff_test.go b/cmd/diff_test.go index a5112ea0..144ceb23 100644 --- a/cmd/diff_test.go +++ b/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) } } } diff --git a/cmd/root_test.go b/cmd/root_test.go index 24a0f798..319f7315 100644 --- a/cmd/root_test.go +++ b/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 }