You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jan Schlicht <ja...@mesosphere.io> on 2017/01/13 11:28:14 UTC
Review Request 55496: Fixed parsing of proxy CONNECT responses.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55496/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Gilbert Song, and Jie Yu.
Bugs: MESOS-6010
https://issues.apache.org/jira/browse/MESOS-6010
Repository: mesos
Description
-------
When behind a proxy, access to websites using SSL can be done by using
HTTP CONNECT tunneling. This is, for example, used when we pull images
from a Docker repository. The HTTP response of such a CONNECT request
surfaced a problem with our HTTP parser, because it doesn't have neither
any headers nor a message body. The parser expected a 'Content-Length'
header to be set in every HTTP response. We fix this by assuming that
a HTTP response doesn't have a message body when the 'Content-Length'
header isn't set.
Diffs
-----
3rdparty/libprocess/src/decoder.hpp 4c779d42548958e610142438a57529ccb4478053
3rdparty/libprocess/src/tests/decoder_tests.cpp 87563f4fcdfaa2a33c4482533ddff24e062e603a
Diff: https://reviews.apache.org/r/55496/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 55496: Fixed parsing of proxy CONNECT responses.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55496/#review161537
-----------------------------------------------------------
3rdparty/libprocess/src/decoder.hpp (line 416)
<https://reviews.apache.org/r/55496/#comment232837>
If you made `encoding` an `Option<string>` and dropped the `getOrElse` unwrapping, you could directly compare it against `Some("chunked")` below.
- Benjamin Bannier
On Jan. 13, 2017, 2:48 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55496/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2017, 2:48 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Gilbert Song, and Jie Yu.
>
>
> Bugs: MESOS-6010
> https://issues.apache.org/jira/browse/MESOS-6010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When behind a proxy, access to websites using SSL can be done by using
> HTTP CONNECT tunneling. This is, for example, used when we pull images
> from a Docker repository. The HTTP response of such a CONNECT request
> surfaced a problem with our HTTP parser, because it doesn't have neither
> any headers nor a message body. The parser expected a 'Content-Length'
> header to be set in every HTTP response. We fix this by assuming that
> a HTTP response doesn't have a message body when the 'Content-Length'
> header isn't set.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/decoder.hpp 4c779d42548958e610142438a57529ccb4478053
> 3rdparty/libprocess/src/tests/decoder_tests.cpp 87563f4fcdfaa2a33c4482533ddff24e062e603a
>
> Diff: https://reviews.apache.org/r/55496/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 55496: Fixed parsing of proxy CONNECT responses.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55496/#review161653
-----------------------------------------------------------
3rdparty/libprocess/src/decoder.hpp (lines 414 - 423)
<https://reviews.apache.org/r/55496/#comment232923>
This sounds like a hack. See my comments in the ticket. The http parser seemed to do the right thing in the sense that if there is no content length or tranfer encoding, it'll try to read until EOF.
In http::decodeResponses, we don't actually call `decode("", 0)` to signal the EOF, we should fix that first.
Then, I think we should workaround the problem at the callsite (i.e., docker fetcher).
What I am thinking right now is that we do some special handling when we know there is an HTTPS_PROXY. We can parse the 200 response, the body of which will be the actual response from the remote server. We need to call parse again on that.
- Jie Yu
On Jan. 13, 2017, 3:51 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55496/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2017, 3:51 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Gilbert Song, and Jie Yu.
>
>
> Bugs: MESOS-6010
> https://issues.apache.org/jira/browse/MESOS-6010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When behind a proxy, access to websites using SSL can be done by using
> HTTP CONNECT tunneling. This is, for example, used when we pull images
> from a Docker repository. The HTTP response of such a CONNECT request
> surfaced a problem with our HTTP parser, because it doesn't have neither
> any headers nor a message body. The parser expected a 'Content-Length'
> header to be set in every HTTP response. We fix this by assuming that
> a HTTP response doesn't have a message body when the 'Content-Length'
> header isn't set.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/decoder.hpp 4c779d42548958e610142438a57529ccb4478053
> 3rdparty/libprocess/src/tests/decoder_tests.cpp 87563f4fcdfaa2a33c4482533ddff24e062e603a
>
> Diff: https://reviews.apache.org/r/55496/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 55496: Added support for HTTP responses with
unspecified length.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55496/#review161870
-----------------------------------------------------------
Fix it, then Ship it!
Let's add a unit test in `3rdparty/libprocess/src/tests/decoder_tests.cpp`.
3rdparty/libprocess/src/http.cpp (lines 767 - 770)
<https://reviews.apache.org/r/55496/#comment233149>
In fact, I would always feed an EOF to the decoder, even if `responses` is non empty:
```
vector<Response> result;
auto appendResult = [=](const dequeue<http::Response*>& responses) mutable {
foreach (Response* response, responses) {
result.push_back(*response);
delete response;
}
};
appendResult(decoder.decode(s.data(), s.length()));
appendResult(decoder.decode("", 0));
if (decoder.failed()) {
return Error("Decoding failed");
}
if (responses.empty()) {
return Error("No response decoded");
}
return result;
```
- Jie Yu
On Jan. 17, 2017, 2:35 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55496/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2017, 2:35 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Gilbert Song, and Jie Yu.
>
>
> Bugs: MESOS-6010
> https://issues.apache.org/jira/browse/MESOS-6010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Some HTTP responses might not set the 'Content-Length' header. This
> usually means that the message body should be read until EOF. This
> will be done by an additional call of 'decoder.decode' with an empty
> string.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp 8895f0cfadf1cca9714fc7110ed0914e3db18983
>
> Diff: https://reviews.apache.org/r/55496/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 55496: Added support for HTTP responses with
unspecified length.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55496/
-----------------------------------------------------------
(Updated Jan. 18, 2017, 2:44 p.m.)
Review request for mesos, Benjamin Bannier, Gilbert Song, and Jie Yu.
Changes
-------
Added a unit test and addressed issues.
Bugs: MESOS-6010
https://issues.apache.org/jira/browse/MESOS-6010
Repository: mesos
Description
-------
Some HTTP responses might not set the 'Content-Length' header. This
usually means that the message body should be read until EOF. This
will be done by an additional call of 'decoder.decode' with an empty
string.
Diffs (updated)
-----
3rdparty/libprocess/src/http.cpp e2e9b8f7563f8756ee406fecca5900d644834974
3rdparty/libprocess/src/tests/decoder_tests.cpp 87563f4fcdfaa2a33c4482533ddff24e062e603a
Diff: https://reviews.apache.org/r/55496/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 55496: Added support for HTTP responses with
unspecified length.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55496/
-----------------------------------------------------------
(Updated Jan. 17, 2017, 3:35 p.m.)
Review request for mesos, Benjamin Bannier, Gilbert Song, and Jie Yu.
Changes
-------
Chose a different approach, proxied responses are now handled at the caller, as Jie suggested.
Summary (updated)
-----------------
Added support for HTTP responses with unspecified length.
Bugs: MESOS-6010
https://issues.apache.org/jira/browse/MESOS-6010
Repository: mesos
Description (updated)
-------
Some HTTP responses might not set the 'Content-Length' header. This
usually means that the message body should be read until EOF. This
will be done by an additional call of 'decoder.decode' with an empty
string.
Diffs (updated)
-----
3rdparty/libprocess/src/http.cpp 8895f0cfadf1cca9714fc7110ed0914e3db18983
Diff: https://reviews.apache.org/r/55496/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 55496: Fixed parsing of proxy CONNECT responses.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55496/
-----------------------------------------------------------
(Updated Jan. 13, 2017, 4:51 p.m.)
Review request for mesos, Benjamin Bannier, Gilbert Song, and Jie Yu.
Changes
-------
Addressed issues.
Bugs: MESOS-6010
https://issues.apache.org/jira/browse/MESOS-6010
Repository: mesos
Description
-------
When behind a proxy, access to websites using SSL can be done by using
HTTP CONNECT tunneling. This is, for example, used when we pull images
from a Docker repository. The HTTP response of such a CONNECT request
surfaced a problem with our HTTP parser, because it doesn't have neither
any headers nor a message body. The parser expected a 'Content-Length'
header to be set in every HTTP response. We fix this by assuming that
a HTTP response doesn't have a message body when the 'Content-Length'
header isn't set.
Diffs (updated)
-----
3rdparty/libprocess/src/decoder.hpp 4c779d42548958e610142438a57529ccb4478053
3rdparty/libprocess/src/tests/decoder_tests.cpp 87563f4fcdfaa2a33c4482533ddff24e062e603a
Diff: https://reviews.apache.org/r/55496/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 55496: Fixed parsing of proxy CONNECT responses.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55496/
-----------------------------------------------------------
(Updated Jan. 13, 2017, 2:48 p.m.)
Review request for mesos, Benjamin Bannier, Gilbert Song, and Jie Yu.
Changes
-------
Fixed problems with chunked transfer encodings.
Bugs: MESOS-6010
https://issues.apache.org/jira/browse/MESOS-6010
Repository: mesos
Description
-------
When behind a proxy, access to websites using SSL can be done by using
HTTP CONNECT tunneling. This is, for example, used when we pull images
from a Docker repository. The HTTP response of such a CONNECT request
surfaced a problem with our HTTP parser, because it doesn't have neither
any headers nor a message body. The parser expected a 'Content-Length'
header to be set in every HTTP response. We fix this by assuming that
a HTTP response doesn't have a message body when the 'Content-Length'
header isn't set.
Diffs (updated)
-----
3rdparty/libprocess/src/decoder.hpp 4c779d42548958e610142438a57529ccb4478053
3rdparty/libprocess/src/tests/decoder_tests.cpp 87563f4fcdfaa2a33c4482533ddff24e062e603a
Diff: https://reviews.apache.org/r/55496/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 55496: Fixed parsing of proxy CONNECT responses.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55496/#review161516
-----------------------------------------------------------
Don't merge yet, this seems to break chunked transfers.
- Jan Schlicht
On Jan. 13, 2017, 12:28 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55496/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2017, 12:28 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Gilbert Song, and Jie Yu.
>
>
> Bugs: MESOS-6010
> https://issues.apache.org/jira/browse/MESOS-6010
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When behind a proxy, access to websites using SSL can be done by using
> HTTP CONNECT tunneling. This is, for example, used when we pull images
> from a Docker repository. The HTTP response of such a CONNECT request
> surfaced a problem with our HTTP parser, because it doesn't have neither
> any headers nor a message body. The parser expected a 'Content-Length'
> header to be set in every HTTP response. We fix this by assuming that
> a HTTP response doesn't have a message body when the 'Content-Length'
> header isn't set.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/decoder.hpp 4c779d42548958e610142438a57529ccb4478053
> 3rdparty/libprocess/src/tests/decoder_tests.cpp 87563f4fcdfaa2a33c4482533ddff24e062e603a
>
> Diff: https://reviews.apache.org/r/55496/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>