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/17 14:40:16 UTC
Review Request 55621: Fixed issues with the Docker fetcher when using
a proxy.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55621/
-----------------------------------------------------------
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 behing a proxy, 'curl' uses HTTP CONNECT tunneling to access
HTTPS. This lead to problems with our HTTP parser because the response
of a 'CONNECT' doesn't have neither headers nor a body.
Diffs
-----
src/uri/fetchers/docker.cpp 3f38dddfb4c089322fe4e13b1ef2070b4835885c
Diff: https://reviews.apache.org/r/55621/diff/
Testing
-------
Running ./bin/mesos-tests.sh behind a proxy.
For example by running:
```
docker run -d -p 3128:3128 minumum2scp/squid
export https_proxy=127.0.0.1:3128
./bin/mesos-tests.sh
```
Without this diff, tests cases in the `DockerFetcherPluginTest` fixture should fail.
Thanks,
Jan Schlicht
Re: Review Request 55621: Fixed issues with the Docker fetcher when
using a proxy.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55621/#review161886
-----------------------------------------------------------
Patch looks great!
Reviews applied: [55496, 55621]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos ReviewBot
On Jan. 17, 2017, 2:53 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55621/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2017, 2:53 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 behing a proxy, 'curl' uses HTTP CONNECT tunneling to access
> HTTPS. This lead to problems with our HTTP parser because the response
> of a 'CONNECT' doesn't have neither headers nor a body.
>
>
> Diffs
> -----
>
> src/uri/fetchers/docker.cpp 3f38dddfb4c089322fe4e13b1ef2070b4835885c
>
> Diff: https://reviews.apache.org/r/55621/diff/
>
>
> Testing
> -------
>
> ./bin/mesos-test.sh without a proxy (to test that it's not breaking existing behavior)
> ./bin/mesos-tests.sh behind a proxy.
> For example by running:
> ```
> docker run -d -p 3128:3128 minumum2scp/squid
> export https_proxy=127.0.0.1:3128
> ./bin/mesos-tests.sh
> ```
> Without this diff, tests cases in the `DockerFetcherPluginTest` fixture should fail.
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 55621: Fixed issues with the Docker fetcher when
using a proxy.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On Jan. 17, 2017, 7:11 p.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp, line 171
> > <https://reviews.apache.org/r/55621/diff/1/?file=1606547#file1606547line171>
> >
> > I think we need a case insensitive os::getenv because one can use `HTTPS_PROXY` as well.
> >
> > Maybe introduce a default parameter `Option<bool> caseSensitive` in os::getenv, default to None(). None() means use system default.
> >
> > If you feel the above is too much for now, add a helper to do that for now in this file and add a TODO.
Making `os::getenv` case insensitive wouldn't do the trick, as they aren't case insensitive in Linux and both the lower-case as well as the upper-case variable could be defined. For curl the lower case version has precedence (see https://curl.haxx.se/docs/manpage.html in the 'Environment' section), but this may not be the general case.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55621/#review161893
-----------------------------------------------------------
On Jan. 17, 2017, 3:53 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55621/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2017, 3:53 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 behing a proxy, 'curl' uses HTTP CONNECT tunneling to access
> HTTPS. This lead to problems with our HTTP parser because the response
> of a 'CONNECT' doesn't have neither headers nor a body.
>
>
> Diffs
> -----
>
> src/uri/fetchers/docker.cpp 3f38dddfb4c089322fe4e13b1ef2070b4835885c
>
> Diff: https://reviews.apache.org/r/55621/diff/
>
>
> Testing
> -------
>
> ./bin/mesos-test.sh without a proxy (to test that it's not breaking existing behavior)
> ./bin/mesos-tests.sh behind a proxy.
> For example by running:
> ```
> docker run -d -p 3128:3128 minumum2scp/squid
> export https_proxy=127.0.0.1:3128
> ./bin/mesos-tests.sh
> ```
> Without this diff, tests cases in the `DockerFetcherPluginTest` fixture should fail.
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 55621: Fixed issues with the Docker fetcher when
using a proxy.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On Jan. 17, 2017, 7:11 p.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp, line 171
> > <https://reviews.apache.org/r/55621/diff/1/?file=1606547#file1606547line171>
> >
> > I think we need a case insensitive os::getenv because one can use `HTTPS_PROXY` as well.
> >
> > Maybe introduce a default parameter `Option<bool> caseSensitive` in os::getenv, default to None(). None() means use system default.
> >
> > If you feel the above is too much for now, add a helper to do that for now in this file and add a TODO.
>
> Jan Schlicht wrote:
> Making `os::getenv` case insensitive wouldn't do the trick, as they aren't case insensitive in Linux and both the lower-case as well as the upper-case variable could be defined. For curl the lower case version has precedence (see https://curl.haxx.se/docs/manpage.html in the 'Environment' section), but this may not be the general case.
But, of course, we'll have to check for `HTTPS_PROXY` here. I'll add that.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55621/#review161893
-----------------------------------------------------------
On Jan. 17, 2017, 3:53 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55621/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2017, 3:53 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 behing a proxy, 'curl' uses HTTP CONNECT tunneling to access
> HTTPS. This lead to problems with our HTTP parser because the response
> of a 'CONNECT' doesn't have neither headers nor a body.
>
>
> Diffs
> -----
>
> src/uri/fetchers/docker.cpp 3f38dddfb4c089322fe4e13b1ef2070b4835885c
>
> Diff: https://reviews.apache.org/r/55621/diff/
>
>
> Testing
> -------
>
> ./bin/mesos-test.sh without a proxy (to test that it's not breaking existing behavior)
> ./bin/mesos-tests.sh behind a proxy.
> For example by running:
> ```
> docker run -d -p 3128:3128 minumum2scp/squid
> export https_proxy=127.0.0.1:3128
> ./bin/mesos-tests.sh
> ```
> Without this diff, tests cases in the `DockerFetcherPluginTest` fixture should fail.
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 55621: Fixed issues with the Docker fetcher when
using a proxy.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55621/#review161893
-----------------------------------------------------------
src/uri/fetchers/docker.cpp (line 168)
<https://reviews.apache.org/r/55621/#comment233171>
I'll add a TODO here to alert that this is a work around.
src/uri/fetchers/docker.cpp (line 171)
<https://reviews.apache.org/r/55621/#comment233170>
I think we need a case insensitive os::getenv because one can use `HTTPS_PROXY` as well.
Maybe introduce a default parameter `Option<bool> caseSensitive` in os::getenv, default to None(). None() means use system default.
If you feel the above is too much for now, add a helper to do that for now in this file and add a TODO.
src/uri/fetchers/docker.cpp (line 173)
<https://reviews.apache.org/r/55621/#comment233173>
I would also check if the response code is 200 and there is no content length and transfer encoding.
- Jie Yu
On Jan. 17, 2017, 2:53 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55621/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2017, 2:53 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 behing a proxy, 'curl' uses HTTP CONNECT tunneling to access
> HTTPS. This lead to problems with our HTTP parser because the response
> of a 'CONNECT' doesn't have neither headers nor a body.
>
>
> Diffs
> -----
>
> src/uri/fetchers/docker.cpp 3f38dddfb4c089322fe4e13b1ef2070b4835885c
>
> Diff: https://reviews.apache.org/r/55621/diff/
>
>
> Testing
> -------
>
> ./bin/mesos-test.sh without a proxy (to test that it's not breaking existing behavior)
> ./bin/mesos-tests.sh behind a proxy.
> For example by running:
> ```
> docker run -d -p 3128:3128 minumum2scp/squid
> export https_proxy=127.0.0.1:3128
> ./bin/mesos-tests.sh
> ```
> Without this diff, tests cases in the `DockerFetcherPluginTest` fixture should fail.
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 55621: Fixed issues with the Docker fetcher when
using a proxy.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55621/
-----------------------------------------------------------
(Updated Jan. 18, 2017, 2:45 p.m.)
Review request for mesos, Benjamin Bannier, Gilbert Song, and Jie Yu.
Changes
-------
Addressed issues, added more checks for CONNECT response.
Bugs: MESOS-6010
https://issues.apache.org/jira/browse/MESOS-6010
Repository: mesos
Description
-------
When behing a proxy, 'curl' uses HTTP CONNECT tunneling to access
HTTPS. This lead to problems with our HTTP parser because the response
of a 'CONNECT' doesn't have neither headers nor a body.
Diffs (updated)
-----
src/uri/fetchers/docker.cpp 027e7480bd91038b2e51031dc77aad779d6d5a88
Diff: https://reviews.apache.org/r/55621/diff/
Testing
-------
./bin/mesos-test.sh without a proxy (to test that it's not breaking existing behavior)
./bin/mesos-tests.sh behind a proxy.
For example by running:
```
docker run -d -p 3128:3128 minumum2scp/squid
export https_proxy=127.0.0.1:3128
./bin/mesos-tests.sh
```
Without this diff, tests cases in the `DockerFetcherPluginTest` fixture should fail.
Thanks,
Jan Schlicht
Re: Review Request 55621: Fixed issues with the Docker fetcher when
using a proxy.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55621/
-----------------------------------------------------------
(Updated Jan. 17, 2017, 3:53 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 behing a proxy, 'curl' uses HTTP CONNECT tunneling to access
HTTPS. This lead to problems with our HTTP parser because the response
of a 'CONNECT' doesn't have neither headers nor a body.
Diffs
-----
src/uri/fetchers/docker.cpp 3f38dddfb4c089322fe4e13b1ef2070b4835885c
Diff: https://reviews.apache.org/r/55621/diff/
Testing (updated)
-------
./bin/mesos-test.sh without a proxy (to test that it's not breaking existing behavior)
./bin/mesos-tests.sh behind a proxy.
For example by running:
```
docker run -d -p 3128:3128 minumum2scp/squid
export https_proxy=127.0.0.1:3128
./bin/mesos-tests.sh
```
Without this diff, tests cases in the `DockerFetcherPluginTest` fixture should fail.
Thanks,
Jan Schlicht