Change errors structure to handle them gracefully #390

boaz0 · 2017-12-13T21:12:02Z

Using errors.Cause one can understand what was the cause of the failure. Unfortunately, when the error is returned due to status code different than 203, 200 or 201 errors.Cause is a verbose string.

This patch suggests to set the cause of the error to be the staus text.

For example, if failed to upload a layer, instead of having the following:
Error initiating layer upload to /path/to/upload, status 401
This error message will be returned:
Error initiating layer upload to /path/to/upload": Unauthorized

It's then easier to handle this error by reading errors.Cause:

if errors.Cause(err) == http.StatusText(http.StatusUnauthorized) {
    return errors.New("You are not authorized. Missing credentials?")
}

This can help me to fix containers/buildah#254.

boaz0 · 2017-12-14T09:24:18Z

/cc @rhatdan @TomSweeneyRedHat WDYT?

TomSweeneyRedHat · 2017-12-14T13:47:42Z

docker/docker_client.go

@@ -363,7 +363,7 @@ func (c *dockerClient) detectProperties(ctx context.Context) error {
 		defer resp.Body.Close()
 		logrus.Debugf("Ping %s status %d", url, resp.StatusCode)
 		if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusUnauthorized {
-			return errors.Errorf("error pinging repository, response code %d", resp.StatusCode)
+			return errors.Wrapf(errors.New(http.StatusText(resp.StatusCode)), "Error pingin repository")


nit "pingin" to "pinging"

runcom · 2017-12-14T13:54:09Z

LGTM

TomSweeneyRedHat · 2017-12-14T14:17:09Z

The change LGTM with the exception of the nit, but I think I'd rather not remove anything from the original message. I personally like the status codes, but see the value in the text. I think I'd prefer from your example:

Error initiating layer upload to /path/to/upload, status 401: Unauthorized.

Also you didn't hit up all the occurrences in this project, should we? @nalind WDYT?

rhatdan · 2017-12-14T14:32:42Z

With the errors wrap won't we end up with the 401 anyways if you print out the error?

rhatdan · 2017-12-14T14:33:04Z

LGTM with the pingin fix.

boaz0 · 2017-12-14T15:26:47Z

With the errors wrap won't we end up with the 401 anyways if you print out the error?

The http.StatusText function takes the res.StatusCode and returns a text for the HTTP status code so no.

Here are the texts for each status-code from golang source code

Maybe I should change it to
"Error initiating layer upload to /path/to/upload: HTTP response "
so it will be clear it is related to the request.

@TomSweeneyRedHat thank you for noticing! Working on it.

@runcom thanks for the review.

nalind · 2017-12-14T19:15:00Z

I like this.

rhatdan · 2018-01-02T13:08:37Z

@mtrmac PTAL and get this merged.

mtrmac · 2018-01-02T13:48:49Z

It's then easier to handle this error by reading errors.Cause:

if errors.Cause(err) == http.StatusText(http.StatusUnauthorized) {
    return errors.New("You are not authorized. Missing credentials?")
}

This still relies on a string comparison, which is fragile (it is non-obvious that the string is an API commitment), and invisible to tools. I’d much prefer using a specific error value, or type, for this.

In fact, focusing on the 401 status which seems to be the core concern of containers/buildah#254 , there already are two plausible options:

Many of the clients for the docker/distribution API (but not signature lookaside) can, and probably should, call github.com/docker/distribution/registry/client.HandleErrorResponse, to get nicer and more detailed error messages. That function can also return well-typed github.com/docker/distribution/registry/api/errcode.ErrorCodeUnauthorized and github.com/docker/distribution/registry/client.UnexpectedHTTPResponseError.
We also have c/i/docker.ErrUnauthorizedForCredentials, which is returned if the server is using Authorization: Bearer. It seems very reasonable to return the same value for Authorization: Basic as well.

In the worst case, a new type c/i/docker.UnexpectedHTTPStatus int type implementing error would still be cleaner than the string comparison proposed above.

TomSweeneyRedHat · 2018-01-02T15:16:23Z

@ripcurld0 this needs to be rebased.

boaz0 · 2018-01-07T13:13:46Z

@mtrmac Thanks a lot for the review.
I decided to go with HandleErrorResponse as you suggested. Do you mind re-review it again?

By the way, we're not vendoring this package, and we're relying on other people to do that. Should I leave it as it is or we need to vendor this package?

@TomSweeneyRedHat I have rebased the patch. Thank you so much for noticing.

boaz0 · 2018-01-11T07:24:11Z

ping @mtrmac 🖌 ♻️

mtrmac · 2018-01-15T21:53:07Z

👍 Thanks! Please rebase once more.

@runcom you might want to take another look.

If commiting or reading Docker image fails the error message is verbose and thus it's hard to handle the error gracefully. This patch attempts to solve in the following way: The `docker/distribution/registry/client.HandleErrorResponse` takes a `http.Response` and returns a new error containing a human-readable error message alongside with the status code. In order to handle such errors, verify the error type and handle it: if respErr, ok := errors.Cause(err).(errcode.Error); ok { // handle the error here. } This change is going to apply on docker images. Signed-off-by: Boaz Shuster <[email protected]>

runcom · 2018-01-15T22:30:07Z

LGTM

boaz0 · 2018-01-15T23:41:25Z

Thank you so much.

mtrmac · 2018-01-16T13:49:13Z

By the way, we're not vendoring this package, and we're relying on other people to do that. Should I leave it as it is or we need to vendor this package?

FWIW this needs to stay as it is because c/image is a library package; types in vendored packages are not visible to users. See e.g. opencontainers/image-spec#528 (comment) , and this would equally apply to the error types returned by HandleErrorResponse.

rhatdan · 2018-01-16T15:39:03Z

@ripcurld0 Could you vendor this into buildah?

boaz0 · 2018-01-16T15:57:48Z

@rhatdan consider this done 😉

TomSweeneyRedHat reviewed Dec 14, 2017

View reviewed changes

boaz0 mentioned this pull request Jan 1, 2018

Commit has misleading error when creds are required but not supplied containers/buildah#254

Closed

boaz0 mentioned this pull request Jan 7, 2018

Handle commit error gracefully containers/buildah#385

Closed

runcom merged commit 46d75e3 into containers:master Jan 15, 2018

boaz0 deleted the errors_cause_for_download branch January 15, 2018 23:41

boaz0 mentioned this pull request Jan 16, 2018

vendor containers/image to 386d6c33c9d622ed84baf14f4b1ff1be86800ccd containers/buildah#393

Closed

This was referenced Jan 24, 2018

Return errcode from HasBlob rather than error #409

Closed

Add functionality to search registries #406

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change errors structure to handle them gracefully #390

Change errors structure to handle them gracefully #390

boaz0 commented Dec 13, 2017

boaz0 commented Dec 14, 2017

TomSweeneyRedHat Dec 14, 2017

runcom commented Dec 14, 2017

TomSweeneyRedHat commented Dec 14, 2017

rhatdan commented Dec 14, 2017

rhatdan commented Dec 14, 2017

boaz0 commented Dec 14, 2017 •

edited

Loading

nalind commented Dec 14, 2017

rhatdan commented Jan 2, 2018

mtrmac commented Jan 2, 2018 •

edited

Loading

TomSweeneyRedHat commented Jan 2, 2018

boaz0 commented Jan 7, 2018 •

edited

Loading

boaz0 commented Jan 11, 2018 •

edited

Loading

mtrmac commented Jan 15, 2018 •

edited by runcom

Loading

runcom commented Jan 15, 2018

boaz0 commented Jan 15, 2018

mtrmac commented Jan 16, 2018

rhatdan commented Jan 16, 2018

boaz0 commented Jan 16, 2018

Change errors structure to handle them gracefully #390

Change errors structure to handle them gracefully #390

Conversation

boaz0 commented Dec 13, 2017

boaz0 commented Dec 14, 2017

TomSweeneyRedHat Dec 14, 2017

Choose a reason for hiding this comment

runcom commented Dec 14, 2017

TomSweeneyRedHat commented Dec 14, 2017

rhatdan commented Dec 14, 2017

rhatdan commented Dec 14, 2017

boaz0 commented Dec 14, 2017 • edited Loading

nalind commented Dec 14, 2017

rhatdan commented Jan 2, 2018

mtrmac commented Jan 2, 2018 • edited Loading

TomSweeneyRedHat commented Jan 2, 2018

boaz0 commented Jan 7, 2018 • edited Loading

boaz0 commented Jan 11, 2018 • edited Loading

mtrmac commented Jan 15, 2018 • edited by runcom Loading

runcom commented Jan 15, 2018

boaz0 commented Jan 15, 2018

mtrmac commented Jan 16, 2018

rhatdan commented Jan 16, 2018

boaz0 commented Jan 16, 2018

boaz0 commented Dec 14, 2017 •

edited

Loading

mtrmac commented Jan 2, 2018 •

edited

Loading

boaz0 commented Jan 7, 2018 •

edited

Loading

boaz0 commented Jan 11, 2018 •

edited

Loading

mtrmac commented Jan 15, 2018 •

edited by runcom

Loading