Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: exec should return 200 if client doesn't upgrade proto #515

Merged
merged 1 commit into from
Jan 8, 2018
Merged

bugfix: exec should return 200 if client doesn't upgrade proto #515

merged 1 commit into from
Jan 8, 2018

Conversation

fuweid
Copy link
Contributor

@fuweid fuweid commented Jan 5, 2018

Signed-off-by: Wei Fu [email protected]

1.Describe what this PR did

If user calls /exec/execid/start without upgrade header, the pouchd should return 200 status.
But now, the pouchd always return 101 switch protocol. This PR is to fix this bug.

2.Does this pull request fix one issue?

fixes #494

3.Describe how you did it

For the AttachConfig, the Upgrade field should check header from request.

4.Describe how to verify it

I have added the test for this.

5.Special notes for reviews

In this PR, I upgrades the request.Hijack function and add more comments here:

For hijack request, the server can return 200 status or 101 switch proto.

If http-client doesn't switch proto, the Do function will return error caused by ErrPersistEOF.

As the RFC 2616, section 4.4 mentioned, if there is no Content-Length or chunked Transfer-Encoding on a *Response and the status is not 1xx, 204 or 304, then the body is unbounded. And the response is always terminated by the first empty line after the header fields.

In our case, the 200 response is like:

HTTP/1.1 200 OK
Content-Type: application/vnd.docker.raw-stream
<= first empty line
[raw-data]

The golang http client does follow the RFC 2616. More detail is here:

For this case, both status and header has been read into response. And the [raw-data] will be read from hijack.reader. So we can ignore the ErrPersistEOF here.

cc @allencloud and @Letty5411

@pouchrobot pouchrobot added kind/bug This is bug report for project size/L labels Jan 5, 2018
@codecov-io
Copy link

Codecov Report

Merging #515 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #515   +/-   ##
=======================================
  Coverage   18.04%   18.04%           
=======================================
  Files          35       35           
  Lines        1790     1790           
=======================================
  Hits          323      323           
  Misses       1432     1432           
  Partials       35       35

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 971b3b3...e0441ad. Read the comment docs.

@Letty5411
Copy link
Contributor

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jan 8, 2018
@Letty5411 Letty5411 merged commit 019edcd into AliyunContainerService:master Jan 8, 2018
@fuweid fuweid deleted the bugfix_exec_should_return_200_code_if_client_does_not_want_to_upgrade branch August 3, 2018 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants