Dont parse errors as JSON unless Content-Type is set to JSON #4013
Conversation
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #4013 +/- ##
==========================================
+ Coverage 57.76% 57.79% +0.03%
==========================================
Files 108 108
Lines 10500 10507 +7
==========================================
+ Hits 6065 6073 +8
+ Misses 3762 3761 -1
Partials 673 673
☔ View full report in Codecov by Sentry. |
milosgajdos
force-pushed
the
nonjson-error-client
branch
from
158dc67
to
5e7ffe7
Compare
milosgajdos
added
conformance
There was a problem hiding this comment.
application/vnd.api+json (https://jsonapi.org) looks to be a new addition, which is not documented as part of the spec and, searching history in both this repository and moby/moby, is not something I think we ever supported.
Given that it's not part of the spec, I don't think we should add special handling for it (spec defines application/json, nothing else)
|
Addressed comments, PTAL @thaJeztah @corhere |
milosgajdos
force-pushed
the
nonjson-error-client
branch
2 times, most recently
from
8756096
to
eee27f9
Compare
milosgajdos
force-pushed
the
nonjson-error-client
branch
2 times, most recently
from
65273f0
to
886425d
Compare
|
DCO is moaning now, but I've no idea why 🤨 |
milosgajdos
force-pushed
the
nonjson-error-client
branch
from
886425d
to
4beb60b
Compare
Client attempts to parse the body of every error it receives as JSON regardless of the content-type. This commit rectifies by only parsing he error body as JSON if the Content-Type header is set to either "application/json" or "application/vnd.api+json". Signed-off-by: Milos Gajdos <milosthegajdos@gmail.com>
milosgajdos
force-pushed
the
nonjson-error-client
branch
from
4beb60b
to
45b7b9c
Compare
|
PTAL @thaJeztah @corhere |
| if contentType != "application/json" && contentType != "application/vnd.api+json" { | ||
| return makeError(statusCode, string(body)) | ||
| } | ||
|
|
||
| // For backward compatibility, handle irregularly formatted | ||
| // messages that contain a "details" field. | ||
| var detailsErr struct { | ||
| Details string `json:"details"` | ||
| } | ||
| err = json.Unmarshal(body, &detailsErr) |
There was a problem hiding this comment.
I believe we have to check for if len(body) == 0 as well. This is relevant for HEAD requests for example. I'm currently getting
time="2024-03-20T08:19:56.45287383Z" level=error msg="response completed with error" err.code=unknown err.detail="errors:
denied: requested access to the resource is denied
error parsing HTTP 401 response body: unexpected end of JSON input: ""
" err.message="unknown error" go.version=go1.20.8 http.request.host="<redacted>" http.request.id=8a2db3dd-a021-4864-8a56-307617a3cf26 http.request.method=HEAD http.request.remoteaddr="<redacted>" http.request.uri="/v2/<redacted>/<redacted>/blobs/sha256:<redacted>?ns=<redacted>" http.request.useragent="containerd/v1.6.28" http.response.contenttype="application/json; charset=utf-8" http.response.duration=213.502875ms http.response.status=500 http.response.written=264 vars.digest="sha256:<redacted>" vars.name="<redacted>"
in proxy mode against a non-existing image.
There was a problem hiding this comment.
Hm.. yes, looks like we may need to? (at least I don't think it hurts?). Wondering if in that case it should produce a generic error (based on status code)?
Contributions / PR welcome!
There was a problem hiding this comment.
Oh, I guess that last part may already be handled, as this is only for "details", so maybe that's not needed.
Client attempts to parse the body of every error it receives as
JSONregardless of theContent-Type.This PR rectifies that by only parsing the error body as
JSONif theContent-Type headeris set to eitherapplication/jsonorapplication/vnd.api+json.Closes #3962