Errors returned are not typed making it difficult to distinguish for approriate handling.

[sjhx]

Specifically ImageReference.NewImageSource() error can indicate that the credentials are rejected (permanent) or alternatively that there is a transient network error e.g. "dial connection timeout", "tls timeout" or 429 "too many requests" (transient).

Without string matching it is not possible to distinguish one from another and in order to handle approriately.

Ref: IBM/portieris#238

[mtrmac]

Specifically for the Docker client, there is at least

image/docker/errors.go

Lines 12 to 23 in 122b16a

	var (
	// ErrV1NotSupported is returned when we're trying to talk to a
	// docker V1 registry.
	ErrV1NotSupported = errors.New("can't talk to a V1 docker registry")
	// ErrTooManyRequests is returned when the status code returned is 429
	ErrTooManyRequests = errors.New("too many requests to registry")
	)
	// ErrUnauthorizedForCredentials is returned when the status code returned is 401
	type ErrUnauthorizedForCredentials struct { // We only use a struct to allow a type assertion, without limiting the contents of the error otherwise.
	Err error
	}

. That’s not a general solution, but probably enough at least for the “credentials rejected, give up” case.

https://github.com/containers/common/tree/master/pkg/retry might be useful as well.

[jhart1685]

Another example of an error (from image/docker/errors.go) that should be typed is manifest unknown.

When writing an application (using this library), I want to be able to ignore cases where source image does not exist. Currently the only way to do this is with a string compare for manifest unknown.
It would be extremely helpful if the containers image library returned a specific error (or typed error) in this case.

[mtrmac]

That’s a reasonable request.

manifest unknown is actually, currently, typed as github.com/docker/distribution/registry/api/v2.ErrorCodeManifestUnknown — but that’s not an API commitment, and it’s not something we’d want to make an API commitment, because registries behave inconsistently in this (IIRC some just report a HTTP 404 status). So ideally c/image should hide these registry differences and provide a new error type (i.e. not the api/v2 one above).

But then there are semantic difficulties — some registries (in some configurations?) only ever report “unauthorized” / “denied”, never “manifest unknown”, because users who are not allowed to access images are not allowed to know whether that image exists either. So it’s unclear what the library can do about that.

Finally, this would require some kind of interoperability testing infrastructure (which we should have anyway, but sadly we don’t) to be able to make a reasonable API commitment of any kind.

[jhart1685]

Hi @mtrmac - thanks for the response, and apologies for the delay in getting back to you.
You make good points about differences between different registry implementations, the need for a new error type, and the requirement for interoperability testing.

I'm not sure I understand your first point though:

manifest unknown is actually, currently, typed as github.com/docker/distribution/registry/api/v2.ErrorCodeManifestUnknown — but that’s not an API commitment

Are you suggesting that (in some cases at least), we should get a typed error back from containers/image?

I've just tested with the latest docker distribution code, by running a plain, insecure registry locally (and pushed some images into it):

docker run -d -p 5001:5000 --restart always --name registry registry:latest

Here's a code snippet that will use copy.Image to copy an image from one reference in that registry to another:
https://gist.github.com/jhart1685/58af75f97a1022ae188adc49cab8d0a8
The code references a src manifest that does not exist (I checked that the code works with an existing src image).
In the error handling code, I iterate around an Unwrap, writing the error and error type at each point in the stack until it can no longer be unwrapped.

The output looks like:

*errors.withStack, initializing source docker://localhost:5001/idontexist:1: reading manifest 1 in localhost:5001/idontexist: manifest unknown: manifest unknown
*errors.withMessage, initializing source docker://localhost:5001/idontexist:1: reading manifest 1 in localhost:5001/idontexist: manifest unknown: manifest unknown
*errors.withStack, reading manifest 1 in localhost:5001/idontexist: manifest unknown: manifest unknown
*errors.withMessage, reading manifest 1 in localhost:5001/idontexist: manifest unknown: manifest unknown
errcode.Errors, manifest unknown: manifest unknown

No sign of a v2.ErrorCodeManifestUnknown in that stack.

[mtrmac]

@jhart1685 Yes, I’m basically suggesting that, except that ErrorCodeManifestUnknown is not quite an error. Compare the recently-added

image/docker/docker_client.go

Lines 923 to 934 in 29aec5f

	func isManifestUnknownError(err error) bool {
	var errs errcode.Errors
	if !errors.As(err, &errs) || len(errs) == 0 {
	return false
	}
	err = errs[0]
	ec, ok := err.(errcode.ErrorCoder)
	if !ok {
	return false
	}
	return ec.ErrorCode() == v2.ErrorCodeManifestUnknown
	}

. Caveats:

• It’s very likely that doesn’t work reliably against the full universe of registries.
• c/image does not document or promise that it will continue returning that return type; compare Remove github.com/docker/distribution/registry/client package #1487 which already proposes to change some parts of the returned error value.

[jhart1685]

@mtrmac - ok, thanks. Based on results I'm seeing, I don't believe that new isManifestUnknownError is in the code path for the cases I've been testing.
However, as you say, even if it were, we shouldn't be relying on it.