DockerFetcher should return the original payload even if it is non-JSON#7564
DockerFetcher should return the original payload even if it is non-JSON#7564kzys wants to merge 1 commit intocontainerd:mainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
d45e67f to
13095a4
Compare
|
/retest |
remotes/docker/fetcher.go
Outdated
| var registryErr Errors | ||
|
|
||
| buf := &bytes.Buffer{} | ||
| _, err := io.Copy(buf, resp.Body) |
There was a problem hiding this comment.
Can we use io.LimitReader ?
There was a problem hiding this comment.
Sure. How about liming the body around 1024 bytes? It is pretty arbitrary though.
|
/retest |
remotes/docker/fetcher.go
Outdated
| if len(s) > 0 { | ||
| s = ": " + s | ||
| } | ||
| return fmt.Errorf("unexpected status code %v: %v%s", req.String(), resp.Status, s) |
There was a problem hiding this comment.
Wondering if it could make sense to return the response as-is in the "not valid JSON" case, wdyt?
(agreed on the limit reader though, to prevent really bad responses)
There was a problem hiding this comment.
Without unexpected status code ...? I'd like to keep the URL (from req.String) and HTTP status code. The response body may have these, but it isn't guaranteed.
There was a problem hiding this comment.
Yeah. Tricky one indeed. I guess either makes sense ("we couldn't parse; we don't know so here's what the registry told us" vs adding the extra information). I guess all bets are off in either case, so 🤷♂️.
|
We rejected a similar PR in the past due to the high likelihood that the body is HTML. Do we have more examples of the non-JSON return bodies and which registries they are coming from? |
|
No. We don't. That being said, if we have a bad HTML-returning registry, it would be better to tell callers that the registry is returning HTML rather than just having its HTTP code. |
0ee3dad to
a826b59
Compare
|
Agreed about better informing the callers. At some point these sort of responses from an HTTP endpoint need to go through more detailed debugging. Throwing the entire output into an error string is kinda risky. I would be more inclined to have an option which could write these out to a file or just have a better way of collecting an HTTP trace. Today I debug these using mitmproxy, but that only helps if I can reproduce the issue, we need a way for users to capture the trace. |
|
Hmm, right. I was mostly thinking about the DDoS risk (because of @AkihiroSuda's comment), but there is a potential information disclosure risk. Writing that to a local file could be abused and it doesn't seem like containerd's job. Tracing (like OpenTelemetry) is also not great due to sampling... |
|
Perhaps dumping first XX amount of data still wouldn't be "too bad". Seeing a |
|
Sorry. I should have started from the end of the last discussion.
So returning a typed error or logging the body (with the context's logger) seems better than just using fmt.Errorf(). |
remotes/docker/fetcher.go
Outdated
| }) | ||
| } | ||
|
|
||
| type HTTPError struct { |
There was a problem hiding this comment.
I didn't love to have this error under docker. Then I realized we have ErrUnexpectedStatus since #4523! So I'm going to update this PR tomorrow.
9af83b2 to
ba19663
Compare
8c14d0b to
5f6f3a0
Compare
While DockerHub returns the errors array, the fetcher should support non-JSON errors to return as much information as possible. Fixes containerd#4672. Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
|
This PR is stale because it has been open 90 days with no activity. This PR will be closed in 7 days unless new comments are made or the stale label is removed. |
|
This PR was closed because it has been stalled for 7 days with no activity. |
While DockerHub returns the errors array, the fetcher should support non-JSON errors to return as much information as possible.
Fixes #4672.