You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jojy Varghese <jo...@mesosphere.io> on 2015/07/24 02:32:56 UTC
Review Request 36757: Added https support.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/
-----------------------------------------------------------
Review request for mesos, Joris Van Remoortere and Timothy Chen.
Bugs: MESOS-3093
https://issues.apache.org/jira/browse/MESOS-3093
Repository: mesos
Description
-------
Current http implementation lacks a https interface. This change exposes
SSL socket for "https" URL scheme.
JIRA: MESOS-3093
Diffs
-----
3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
Diff: https://reviews.apache.org/r/36757/diff/
Testing
-------
make check
Thanks,
Jojy Varghese
Re: Review Request 36757: Added https support.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/#review92852
-----------------------------------------------------------
LGTM, just some minor comments.
3rdparty/libprocess/src/http.cpp (line 707)
<https://reviews.apache.org/r/36757/#comment147101>
Explicitly capturing url by reference should be fine here ?
If not, just pass in the scheme as argument to reduce overhead of copying the entire URL struct.
3rdparty/libprocess/src/http.cpp (line 708)
<https://reviews.apache.org/r/36757/#comment147107>
Personally speaking, I don't mind the usage of this lambda here assuming this is the only place where we create sockets.
Do we have a separate function for POST too that would also need this logic or it just calls into this function ?
3rdparty/libprocess/src/tests/ssl_tests.cpp (line 974)
<https://reviews.apache.org/r/36757/#comment147102>
nitpick:
s/getUrl/url
3rdparty/libprocess/src/tests/ssl_tests.cpp (line 980)
<https://reviews.apache.org/r/36757/#comment147104>
Won't just http::get(url); suffice here minus the empty default headers ?
3rdparty/libprocess/src/tests/ssl_tests.cpp (line 985)
<https://reviews.apache.org/r/36757/#comment147105>
Nitpick : s/string/const string
3rdparty/libprocess/src/tests/ssl_tests.cpp (line 988)
<https://reviews.apache.org/r/36757/#comment147100>
Can we also check if the response returned is what you had sent/expected i.e. status code is 200 ?
- Anand Mazumdar
On July 24, 2015, 12:32 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36757/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 12:32 a.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Timothy Chen.
>
>
> Bugs: MESOS-3093
> https://issues.apache.org/jira/browse/MESOS-3093
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Current http implementation lacks a https interface. This change exposes
> SSL socket for "https" URL scheme.
>
> JIRA: MESOS-3093
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36757/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 36757: Added https support.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On July 24, 2015, 3:58 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp, line 989
> > <https://reviews.apache.org/r/36757/diff/1-3/?file=1020505#file1020505line989>
> >
> > AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response);
The pattern "ASSERT_EQ(http::statuses[200], response.get().status)" is the one adopted in http_tests.cpp. Not sure if I want to have a new pattern for https tests.
> On July 24, 2015, 3:58 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp, line 1015
> > <https://reviews.apache.org/r/36757/diff/1-3/?file=1020505#file1020505line1015>
> >
> > s/{}/None()
> > s/payload/None()
> >
> > You should be able to do without setting headers and body here.
It will work but the test here is for having "some" payload and https not crapping out for actual POST data.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/#review92921
-----------------------------------------------------------
On July 24, 2015, 3:23 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36757/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 3:23 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Timothy Chen.
>
>
> Bugs: MESOS-3093
> https://issues.apache.org/jira/browse/MESOS-3093
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Current http implementation lacks a https interface. This change exposes
> SSL socket for "https" URL scheme.
>
> JIRA: MESOS-3093
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36757/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 36757: Added https support.
Posted by Anand Mazumdar <ma...@gmail.com>.
> On July 24, 2015, 3:58 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp, line 1015
> > <https://reviews.apache.org/r/36757/diff/1-3/?file=1020505#file1020505line1015>
> >
> > s/{}/None()
> > s/payload/None()
> >
> > You should be able to do without setting headers and body here.
>
> Jojy Varghese wrote:
> It will work but the test here is for having "some" payload and https not crapping out for actual POST data.
I see, my bad. Mind atleast removing the default headers here ?
> On July 24, 2015, 3:58 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp, line 989
> > <https://reviews.apache.org/r/36757/diff/1-3/?file=1020505#file1020505line989>
> >
> > AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response);
>
> Jojy Varghese wrote:
> The pattern "ASSERT_EQ(http::statuses[200], response.get().status)" is the one adopted in http_tests.cpp. Not sure if I want to have a new pattern for https tests.
There are not many places left in our code that still use this old style. ( All the remaining ones left would eventually get cleaned up ).
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/#review92921
-----------------------------------------------------------
On July 24, 2015, 5:49 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36757/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 5:49 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Timothy Chen.
>
>
> Bugs: MESOS-3093
> https://issues.apache.org/jira/browse/MESOS-3093
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Current http implementation lacks a https interface. This change exposes
> SSL socket for "https" URL scheme.
>
> JIRA: MESOS-3093
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36757/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 36757: Added https support.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/#review92921
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/tests/ssl_tests.cpp (line 988)
<https://reviews.apache.org/r/36757/#comment147189>
AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response);
3rdparty/libprocess/src/tests/ssl_tests.cpp (line 1014)
<https://reviews.apache.org/r/36757/#comment147191>
s/{}/None()
s/payload/None()
You should be able to do without setting headers and body here.
3rdparty/libprocess/src/tests/ssl_tests.cpp (line 1022)
<https://reviews.apache.org/r/36757/#comment147190>
Same as above.
- Anand Mazumdar
On July 24, 2015, 3:23 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36757/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 3:23 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Timothy Chen.
>
>
> Bugs: MESOS-3093
> https://issues.apache.org/jira/browse/MESOS-3093
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Current http implementation lacks a https interface. This change exposes
> SSL socket for "https" URL scheme.
>
> JIRA: MESOS-3093
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36757/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 36757: Added https support.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/#review93327
-----------------------------------------------------------
Ship it!
- Timothy Chen
On July 27, 2015, 1:56 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36757/
> -----------------------------------------------------------
>
> (Updated July 27, 2015, 1:56 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Timothy Chen.
>
>
> Bugs: MESOS-3093
> https://issues.apache.org/jira/browse/MESOS-3093
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Current http implementation lacks a https interface. This change exposes
> SSL socket for "https" URL scheme.
>
> JIRA: MESOS-3093
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36757/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 36757: Added https support.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/#review93122
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36712, 36757]
All tests passed.
- Mesos ReviewBot
On July 27, 2015, 1:56 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36757/
> -----------------------------------------------------------
>
> (Updated July 27, 2015, 1:56 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Timothy Chen.
>
>
> Bugs: MESOS-3093
> https://issues.apache.org/jira/browse/MESOS-3093
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Current http implementation lacks a https interface. This change exposes
> SSL socket for "https" URL scheme.
>
> JIRA: MESOS-3093
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36757/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 36757: Added https support.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/
-----------------------------------------------------------
(Updated July 27, 2015, 1:56 p.m.)
Review request for mesos, Joris Van Remoortere and Timothy Chen.
Changes
-------
review comments addressed.
Bugs: MESOS-3093
https://issues.apache.org/jira/browse/MESOS-3093
Repository: mesos
Description
-------
Current http implementation lacks a https interface. This change exposes
SSL socket for "https" URL scheme.
JIRA: MESOS-3093
Diffs (updated)
-----
3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
Diff: https://reviews.apache.org/r/36757/diff/
Testing
-------
make check
Thanks,
Jojy Varghese
Re: Review Request 36757: Added https support.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On July 26, 2015, 4:21 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp, lines 1025-1029
> > <https://reviews.apache.org/r/36757/diff/4/?file=1021132#file1021132line1025>
> >
> > You're going to get rid of this anyway, but for future reference: please use full words for variable names e.g. `responseBuffer`
>
> Jojy Varghese wrote:
> I will create tickets(JIRA) to address it?
Created MESOS-3153
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/#review93049
-----------------------------------------------------------
On July 27, 2015, 1:56 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36757/
> -----------------------------------------------------------
>
> (Updated July 27, 2015, 1:56 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Timothy Chen.
>
>
> Bugs: MESOS-3093
> https://issues.apache.org/jira/browse/MESOS-3093
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Current http implementation lacks a https interface. This change exposes
> SSL socket for "https" URL scheme.
>
> JIRA: MESOS-3093
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36757/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 36757: Added https support.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On July 26, 2015, 4:21 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp, lines 1025-1029
> > <https://reviews.apache.org/r/36757/diff/4/?file=1021132#file1021132line1025>
> >
> > You're going to get rid of this anyway, but for future reference: please use full words for variable names e.g. `responseBuffer`
I will create tickets(JIRA) to address it?
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/#review93049
-----------------------------------------------------------
On July 27, 2015, 1:56 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36757/
> -----------------------------------------------------------
>
> (Updated July 27, 2015, 1:56 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Timothy Chen.
>
>
> Bugs: MESOS-3093
> https://issues.apache.org/jira/browse/MESOS-3093
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Current http implementation lacks a https interface. This change exposes
> SSL socket for "https" URL scheme.
>
> JIRA: MESOS-3093
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36757/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 36757: Added https support.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/#review93049
-----------------------------------------------------------
Can you add (or at least make TODOs for) tests for the negative cases? http -> ssl socket, https -> raw socket.
After the remaining open issues this should look pretty good.
3rdparty/libprocess/src/tests/ssl_tests.cpp (lines 1025 - 1029)
<https://reviews.apache.org/r/36757/#comment147319>
You're going to get rid of this anyway, but for future reference: please use full words for variable names e.g. `responseBuffer`
- Joris Van Remoortere
On July 24, 2015, 5:49 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36757/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 5:49 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Timothy Chen.
>
>
> Bugs: MESOS-3093
> https://issues.apache.org/jira/browse/MESOS-3093
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Current http implementation lacks a https interface. This change exposes
> SSL socket for "https" URL scheme.
>
> JIRA: MESOS-3093
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36757/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 36757: Added https support.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/#review92962
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36712, 36757]
All tests passed.
- Mesos ReviewBot
On July 24, 2015, 5:49 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36757/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 5:49 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Timothy Chen.
>
>
> Bugs: MESOS-3093
> https://issues.apache.org/jira/browse/MESOS-3093
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Current http implementation lacks a https interface. This change exposes
> SSL socket for "https" URL scheme.
>
> JIRA: MESOS-3093
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36757/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 36757: Added https support.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On July 24, 2015, 7:23 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp, line 1020
> > <https://reviews.apache.org/r/36757/diff/4/?file=1021132#file1021132line1020>
> >
> > Do you want to check that this payload and content-type actually make their way to the socket by doing a read and parsing into a request?
After dicussing with you, decided to create a separate ticket that address this.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/#review92966
-----------------------------------------------------------
On July 27, 2015, 1:56 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36757/
> -----------------------------------------------------------
>
> (Updated July 27, 2015, 1:56 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Timothy Chen.
>
>
> Bugs: MESOS-3093
> https://issues.apache.org/jira/browse/MESOS-3093
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Current http implementation lacks a https interface. This change exposes
> SSL socket for "https" URL scheme.
>
> JIRA: MESOS-3093
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36757/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 36757: Added https support.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On July 24, 2015, 7:23 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp, lines 1025-1029
> > <https://reviews.apache.org/r/36757/diff/4/?file=1021132#file1021132line1025>
> >
> > Why do you need a std::stringstream for this? We can simplify with just a string, and also construct it like the protocol exposes:
> >
> > string buffer =
> > "HTTP/1.1 200 OK\r\n" +
> > "Content-Length : " + stringify(data.length()) + "\r\n" +
> > "\r\n" +
> > data;
> >
> > Same for test above too.
Only for efficiency reasons. string+ operator will create temp objects.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/#review92966
-----------------------------------------------------------
On July 24, 2015, 5:49 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36757/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 5:49 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Timothy Chen.
>
>
> Bugs: MESOS-3093
> https://issues.apache.org/jira/browse/MESOS-3093
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Current http implementation lacks a https interface. This change exposes
> SSL socket for "https" URL scheme.
>
> JIRA: MESOS-3093
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36757/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 36757: Added https support.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On July 24, 2015, 7:23 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp, line 1020
> > <https://reviews.apache.org/r/36757/diff/4/?file=1021132#file1021132line1020>
> >
> > Do you want to check that this payload and content-type actually make their way to the socket by doing a read and parsing into a request?
>
> Jojy Varghese wrote:
> After dicussing with you, decided to create a separate ticket that address this.
Created MESOS-3153
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/#review92966
-----------------------------------------------------------
On July 27, 2015, 1:56 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36757/
> -----------------------------------------------------------
>
> (Updated July 27, 2015, 1:56 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Timothy Chen.
>
>
> Bugs: MESOS-3093
> https://issues.apache.org/jira/browse/MESOS-3093
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Current http implementation lacks a https interface. This change exposes
> SSL socket for "https" URL scheme.
>
> JIRA: MESOS-3093
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36757/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 36757: Added https support.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/#review92966
-----------------------------------------------------------
3rdparty/libprocess/src/http.cpp (line 707)
<https://reviews.apache.org/r/36757/#comment147245>
We should do s/getSocket/createSocket/ to match the fact that you're calling 'Socket::create' under the covers, and the variable below is called 'create', etc. Also, any reason to name the function at all? We'd like to move to a static initialization world where we do stuff like this but it'll just look like:
Try<Socket> create = [&url]() -> Try<Socket> {
if (url.scheme == "http") {
return Socket::create(Socket::POLL);
}
#ifdef USE_SSL_SOCKET
if (url.scheme == "https") {
return Socket::create(Socket::SSL);
}
#endif
return Error("Unsupported URL scheme");
}();
3rdparty/libprocess/src/tests/ssl_tests.cpp (line 1020)
<https://reviews.apache.org/r/36757/#comment147243>
Do you want to check that this payload and content-type actually make their way to the socket by doing a read and parsing into a request?
3rdparty/libprocess/src/tests/ssl_tests.cpp (lines 1025 - 1029)
<https://reviews.apache.org/r/36757/#comment147241>
Why do you need a std::stringstream for this? We can simplify with just a string, and also construct it like the protocol exposes:
string buffer =
"HTTP/1.1 200 OK\r\n" +
"Content-Length : " + stringify(data.length()) + "\r\n" +
"\r\n" +
data;
Same for test above too.
3rdparty/libprocess/src/tests/ssl_tests.cpp (line 1030)
<https://reviews.apache.org/r/36757/#comment147244>
Moot point if we just use a string (see comment above) but you should not have needed the `c_str()` here.
Also, looks like we need a const& Future::get in our future (pun intended).
- Benjamin Hindman
On July 24, 2015, 5:49 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36757/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 5:49 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Timothy Chen.
>
>
> Bugs: MESOS-3093
> https://issues.apache.org/jira/browse/MESOS-3093
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Current http implementation lacks a https interface. This change exposes
> SSL socket for "https" URL scheme.
>
> JIRA: MESOS-3093
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36757/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 36757: Added https support.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/
-----------------------------------------------------------
(Updated July 24, 2015, 5:49 p.m.)
Review request for mesos, Joris Van Remoortere and Timothy Chen.
Changes
-------
added tests for server response validation.
Bugs: MESOS-3093
https://issues.apache.org/jira/browse/MESOS-3093
Repository: mesos
Description
-------
Current http implementation lacks a https interface. This change exposes
SSL socket for "https" URL scheme.
JIRA: MESOS-3093
Diffs (updated)
-----
3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
Diff: https://reviews.apache.org/r/36757/diff/
Testing
-------
make check
Thanks,
Jojy Varghese
Re: Review Request 36757: Added https support.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/#review92924
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36712, 36757]
All tests passed.
- Mesos ReviewBot
On July 24, 2015, 3:23 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36757/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 3:23 p.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Timothy Chen.
>
>
> Bugs: MESOS-3093
> https://issues.apache.org/jira/browse/MESOS-3093
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Current http implementation lacks a https interface. This change exposes
> SSL socket for "https" URL scheme.
>
> JIRA: MESOS-3093
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36757/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 36757: Added https support.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/
-----------------------------------------------------------
(Updated July 24, 2015, 3:23 p.m.)
Review request for mesos, Joris Van Remoortere and Timothy Chen.
Changes
-------
minor optimization.
Bugs: MESOS-3093
https://issues.apache.org/jira/browse/MESOS-3093
Repository: mesos
Description
-------
Current http implementation lacks a https interface. This change exposes
SSL socket for "https" URL scheme.
JIRA: MESOS-3093
Diffs (updated)
-----
3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
Diff: https://reviews.apache.org/r/36757/diff/
Testing
-------
make check
Thanks,
Jojy Varghese
Re: Review Request 36757: Added https support.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/#review92861
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36712, 36757]
All tests passed.
- Mesos ReviewBot
On July 24, 2015, 2:28 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36757/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 2:28 a.m.)
>
>
> Review request for mesos, Joris Van Remoortere and Timothy Chen.
>
>
> Bugs: MESOS-3093
> https://issues.apache.org/jira/browse/MESOS-3093
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Current http implementation lacks a https interface. This change exposes
> SSL socket for "https" URL scheme.
>
> JIRA: MESOS-3093
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
> 3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
>
> Diff: https://reviews.apache.org/r/36757/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 36757: Added https support.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36757/
-----------------------------------------------------------
(Updated July 24, 2015, 2:28 a.m.)
Review request for mesos, Joris Van Remoortere and Timothy Chen.
Changes
-------
review comments; added test for post.
Bugs: MESOS-3093
https://issues.apache.org/jira/browse/MESOS-3093
Repository: mesos
Description
-------
Current http implementation lacks a https interface. This change exposes
SSL socket for "https" URL scheme.
JIRA: MESOS-3093
Diffs (updated)
-----
3rdparty/libprocess/src/http.cpp d1685799f4c53e067d0812e037e171324ee7195f
3rdparty/libprocess/src/tests/ssl_tests.cpp 2fe50601615b0bee57bd3e05dc9c932f93ca7477
Diff: https://reviews.apache.org/r/36757/diff/
Testing
-------
make check
Thanks,
Jojy Varghese