ociclient.makeError obscures StatusCode in a basic stringified error unless registry response is specifically-crafted JSON

[tianon]

I don't know if it's intentional (it seems like it's not, given how the function is structured), but ociclient.makeError does a lot of return fmt.Errorf(...) for many cases that seem like they should be returning a wrapped wireError, wireErrors, or ociregistry.Error instead so consumers can still access the original status code.

oci/ociregistry/ociclient/error.go

Lines 127 to 172 in 1f693b7

	// makeError forms an error from a non-OK response.
	func makeError(resp *http.Response) error {
	if resp.Request.Method == "HEAD" {
	// When we've made a HEAD request, we can't see any of
	// the actual error, so we'll have to make up something
	// from the HTTP status.
	var err error
	switch resp.StatusCode {
	case http.StatusNotFound:
	err = ociregistry.ErrNameUnknown
	case http.StatusUnauthorized:
	err = ociregistry.ErrUnauthorized
	case http.StatusForbidden:
	err = ociregistry.ErrDenied
	case http.StatusTooManyRequests:
	err = ociregistry.ErrTooManyRequests
	case http.StatusBadRequest:
	err = ociregistry.ErrUnsupported
	default:
	return fmt.Errorf("error response: %v", resp.Status)
	}
	return fmt.Errorf("error response: %v: %w", resp.Status, err)
	}
	if !isJSONMediaType(resp.Header.Get("Content-Type")) || resp.Request.Method == "HEAD" {
	// TODO include some of the body in this case?
	data, _ := io.ReadAll(resp.Body)
	return fmt.Errorf("error response: %v; body: %q", resp.Status, data)
	}
	data, err := io.ReadAll(io.LimitReader(resp.Body, errorBodySizeLimit+1))
	if err != nil {
	return fmt.Errorf("%s: cannot read error body: %v", resp.Status, err)
	}
	if len(data) > errorBodySizeLimit {
	// TODO include some part of the body
	return fmt.Errorf("error body too large")
	}
	var errs wireErrors
	if err := json.Unmarshal(data, &errs); err != nil {
	return fmt.Errorf("%s: malformed error response: %v", resp.Status, err)
	}
	if len(errs.Errors) == 0 {
	return fmt.Errorf("%s: no errors in body (probably a server issue)", resp.Status)
	}
	errs.httpStatusCode = resp.StatusCode
	return &errs
	}

I don't think there's anything in the distribution spec that mandates the structure of non-200 status codes from a registry, but I might be very wrong there! (from what I can see, this matches the implementation over in the server half of this library, but I'm using ociclient against non-cue-labs registries 😇)

The specific case I'm running into is a request from a registry that's returning a 404, but it becomes a basic fmt.Errorf("404 Not Found: ...") (with the added bug that I think my registry must be returning a "JSON-y" Content-Type: value because the ociclient.makeError code is trying to Unmarshal it 😅), so I can't identify/adjust my client behavior based on the 404 without doing a string match, which isn't ideal (and seems counter to the design of ociregistry.ErrNameUnknown and friends, and even the wireError/wireErrors wrappers that allow direct access to the HTTP status code).

At first, I thought the Errors slice on wrapErrors was generic enough that I would've proposed moving up the var errs wireErrors (and population of httpStatusCode on it) to the top of the function, and then every return fmt.Errorf(...) instead be something like errs.Errors = append(errs.Errors, fmt.Errorf(...)); return &errs or something (perhaps even the HEAD error translation code for the !isJSONMediaType(resp.Header.Get("Content-Type")) case, maybe with a second error added to the Errors slice if you wanted to still include the errant body), but I see it's not a generic errors wrapper (which is fair, since it then wouldn't Unmarshal correctly), so I'm not sure what to propose here except that it'd be really nice if there were some way to access StatusCode on errors from the registry in ociclient.

[rogpeppe]

Yup. I've definitely been thinking that the errors support needs to be improved. Thanks for creating the issue!

Some random thoughts:

• it should be straightforward for HTTP middleware to construct a well-formed OCI error
• we should make it possible to access the original HTTP status code when applicable (e.g. when invoking ociclient)
• the spec doesn't make it entirely clear what the behaviour should be when faced with arbitrary HTTP status codes but no JSON error body (or an incompatible JSON body)
• perhaps it should be possible for a registry Interface implementation to return an error that will result in a particular HTTP status code when ociserver sees it. Although this might be considered to be abstraction-breaking: thought required.
• it would be nice to share a uniform error type (including the wire representation) between ociclient and ociserver

[tianon]

I don't think there's anything in the distribution spec that mandates the structure of non-200 status codes from a registry, but I might be very wrong there!

Just to correct myself here -- this is related to opencontainers/distribution-spec#178 + opencontainers/distribution-spec#280 (ala opencontainers/distribution-spec#443).

If we go back in time a bit to before the spec got "cleaned up", we get:

https://github.com/opencontainers/distribution-spec/blob/a6e5b091b1468662730ab1e5be55c61838643ab4/spec.md#on-failure-bad-request

(Which clearly describes the structured errors that ociclient is reading/parsing 😄 ❤️)

All that got removed before 1.0 of the distribution-spec, for reasons that apparently weren't made clear in writing anywhere (as seen in issue 443 there).

[mvdan]

Should the spec add those structured errors back, or should we remove support for them if the spec dropped them on purpose?

[tianon]

IMO the spec should add them back, even if only as informational/SHOULD/MAY since they are in common use and documenting common use for interoperability is the literal point of the spec existing in the first place, but I'm not a maintainer of the distribution spec (only image and runtime). 🙈

However, @jonjohnsonjr there is a maintainer of the distribution spec, and I don't think I'm putting words in his mouth if I say he does think the spec would have value in having things like those structured errors back (opencontainers/distribution-spec#496 being some pretty compelling evidence towards that -- presumably closed because he was just restoring previously in-the-spec content and didn't think it should have too many cycles spent nit-picking, but that's definitely me projecting / trying to read between the lines). ❤️