Pullthrough broken registry server #13283
Conversation
|
@mfojtik FYI |
|
|
||
| // Check whether remoteReader is seekable. The remoteReader' Seek method must work: ServeContent uses | ||
| // a seek to the end of the content to determine its size. | ||
| if _, err := remoteReader.Seek(0, os.SEEK_END); err != nil { |
There was a problem hiding this comment.
This seems fairly fragile - how do we actually verify this doesn't regress in the future?
There was a problem hiding this comment.
@smarterclayton This test is about the acknowledgment that the remoteReader is really io.ReadSeeker. Acording documentation the http.ServeContent uses a seek to the end of the content to determine its size.
Basically, the problem hidden deeply inside the docker's client library. They return an object with io.ReadSeeker interface which in fact isn't seekable. I can understand them, because if they don't know the final size of the content, it's impossible to move through it.
So the regression can't happen until the http.ServeContent accepts io.ReadSeeker as argument. If it stops doing that, then it will regress in golang.
| return distribution.Descriptor{}, err | ||
| } | ||
|
|
||
| if contentHandled { | ||
| return desc, nil | ||
| } |
There was a problem hiding this comment.
This should set Content-Length as well.
There was a problem hiding this comment.
Only when ServeContent doesn't run.
There was a problem hiding this comment.
@smarterclayton ServeContent will replace anyway.
legionus
force-pushed
the
dockerregistry-pullthrough-broken-registry
branch
2 times, most recently
from
da363f7 to
baf3234
Compare
|
Comments addressed. |
| @@ -104,10 +107,40 @@ func (pbs *pullthroughBlobStore) Get(ctx context.Context, dgst digest.Digest) ([ | |||
| // setResponseHeaders sets the appropriate content serving headers | |||
| func setResponseHeaders(w http.ResponseWriter, length int64, mediaType string, digest digest.Digest) { | |||
| w.Header().Set("Content-Type", mediaType) | |||
| w.Header().Set("Content-Length", fmt.Sprintf("%d", length)) | |||
There was a problem hiding this comment.
Let http.ServeContent set this. The value will be shorter than length for range requests. Set it only in case http.ServeContent doesn't run.
Signed-off-by: Gladkov Alexey <[email protected]>
legionus
force-pushed
the
dockerregistry-pullthrough-broken-registry
branch
from
baf3234 to
f298117
Compare
|
[test] |
|
Flake #13288 |
|
[test] |
|
Evaluated for origin test up to f298117 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/181/) (Base Commit: f617108) |
|
[merge] |
|
LGTM |
|
Evaluated for origin merge up to f298117 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/79/) (Base Commit: c9eff77) (Image: devenv-rhel7_6067) |
If remote registry server does not return all necessary headers (such as Content-Range or Content-Length) we can not use
http.ServeContentto pull through it. In this case we fallback toio.Copy.