Handle error gracefully when we can't retrieve an image #251

nkubala · 2018-07-26T19:28:54Z

This fixes the error handling for processing images before we do the diff or analysis. Errors are propagated back through a channel to the main loop, which will only execute the diff/analysis if we didn't receive any errors back from the image processing. Error handling also happens in the main loop now, so we'll get consistent results across diff types.

This also changes a bunch of error messages to add some more context when we do receive an error.

➜ ./out/container-diff diff dogs gcr.io/google-appengine/python:latest
2018/07/26 12:27:12 No matching credentials found for index.docker.io, falling back on anonymous
ERRO[0001] error retrieving image dogs: UNAUTHORIZED: "authentication required" 
➜ ./out/container-diff analyze dogs                                   
2018/07/26 12:27:16 No matching credentials found for index.docker.io, falling back on anonymous
ERRO[0000] error retrieving image dogs: UNAUTHORIZED: "authentication required"

Fixes #250

bobcatfish

Thanks for jumping on this so quickly @nkubala!

I wonder if there is any chance to add some test coverage in the process :D

bobcatfish · 2018-07-27T00:55:31Z

cmd/diff.go

+	defer wg.Done()
+	image, err := getImageForName(imageName)
+	if image.Image == nil {
+		errChan <- fmt.Errorf("error retrieving image %s: %s", imageName, err.Error())


I'm interested in why call err.Error() in this case vs. passing err to be formatted as '%s'

technically the format types don't match (string vs error), but this seems to work so I'll change it back

bobcatfish · 2018-07-27T00:57:52Z

cmd/diff.go

+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 {


2 questions:

do you want to propagate the error to errChan anytime err is not nil? e.g. if err != nil || image.Image == nil

what if image.Image is nil AND err is nil too? (calling err.Error() on it)

Ok this logic isn't great so I'm gonna rework it a bit. Digging into the go-containerregistry code, if we get an error returned back from any of the three sources (tar/daemon/remote), the image returned will always be nil. So it's equivalent to just change this back to a vanilla if err != nil check.

The reason I was checking for image.Image == nil is to avoid putting the image into the returned map so we didn't attempt to clean up an empty path, but that logic is already happening in pkgutil.CleanupImage so this is safe. I moved the deferred calls up before we return the errors, and we're good.

TLDR answers for your questions: 1) yes, and 2) not possible

bobcatfish · 2018-07-27T01:09:47Z

cmd/diff.go

@@ -69,33 +70,60 @@ 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) {


Is there a name for this function that might be a bit more specific to what this function is doing? I wasn't sure what processImage meant when I was reading this.

It seems like the purpose of this function is to look up sometthing about the image (im not sure what tho - are we actually getting the bytes of the image itself, or looking up some metadata or something else?) and put that something into imageMap

So some other ideas:

AddImageToMap

GetMetadataForImage

FindImageSource

DownloadImage

(You might have some other name ideas tho since you probably know what exactly getImageForName is doing)

This function is essentially just a concurrent wrapper around getImageForName, which retrieves a reference to an image and a reader for the actual bytes of the image. I can change this to getImage (the imageMap isn't really a useful struct here so I don't want to refer to it), but the reason I called it processImage is because getImageForName actually does a bit of image processing, i.e. unpacks the image filesystem to the local filesystem. I should probably just add some comments explaining what each function does :)

bobcatfish · 2018-07-27T01:11:37Z

cmd/diff.go

-		}(imageArg, imageMap)
+	img2, ok := imageMap[image2Arg]
+	if !ok {
+		return fmt.Errorf("cannot find image %s", image2Arg)
 	}


what would it mean if processImage returned for both images without errors, but one or both of these checks failed?

removed these since this logic is duplicated elsewhere

bobcatfish · 2018-07-27T01:11:54Z

cmd/diff.go

+
+	if len(errs) > 0 {
+		return errors.New(strings.Join(errs, "\n"))
+	}


this logic to collect together errors into one error seems like a good candidate for being moved into a separate function

agreed, done

bobcatfish

Nice, looks great! Thanks for adding the extra docstrings and tests, much clearer to me now how this works :D

bobcatfish · 2018-07-30T17:36:50Z

cmd/analyze_test.go

-			if test.expected_output {
-				t.Errorf("Got unexpected error: %s", err)
-			} else {
+		if (err == nil) != test.shouldError {


nice, shouldError is a way better name!

bobcatfish · 2018-07-30T17:37:13Z

cmd/diff.go


 	if noCache && !save {
 		defer pkgutil.CleanupImage(*imageMap[image1Arg])
 		defer pkgutil.CleanupImage(*imageMap[image2Arg])
 	}

+	if err := readErrorsFromChannel(errChan); err != nil {


niiiiiiiice

bobcatfish · 2018-07-30T17:37:52Z

cmd/root.go

@@ -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.


ty ty, makes sense now!

Handle error gracefully when we can't retrieve an image

8621bc5

nkubala requested a review from bobcatfish July 26, 2018 19:29

fix channel read

ee51ff8

bobcatfish reviewed Jul 27, 2018

View reviewed changes

nkubala added 4 commits July 27, 2018 11:56

improve error handling logic when retrieving images

6a7a550

move error combining logic into helper function

7024d3b

add comments for functions

265b622

add tests

9c47911

bobcatfish approved these changes Jul 30, 2018

View reviewed changes

nkubala merged commit c255953 into GoogleContainerTools:master Jul 30, 2018

nkubala deleted the error_handling branch July 30, 2018 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle error gracefully when we can't retrieve an image #251

Handle error gracefully when we can't retrieve an image #251

nkubala commented Jul 26, 2018

bobcatfish left a comment

bobcatfish Jul 27, 2018

nkubala Jul 27, 2018

bobcatfish Jul 27, 2018

nkubala Jul 27, 2018

bobcatfish Jul 27, 2018

nkubala Jul 27, 2018

bobcatfish Jul 27, 2018

nkubala Jul 27, 2018

bobcatfish Jul 27, 2018

nkubala Jul 27, 2018

bobcatfish left a comment

bobcatfish Jul 30, 2018

bobcatfish Jul 30, 2018

bobcatfish Jul 30, 2018

Handle error gracefully when we can't retrieve an image #251

Handle error gracefully when we can't retrieve an image #251

Conversation

nkubala commented Jul 26, 2018

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment