Re: Review Request 36757: Added https support.
--- 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.
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.
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.
--- 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.
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.
--- 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.
--- 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.
---
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.
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.
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.
--- 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.
---
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:
TrySocket create = [url]() - TrySocket {
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.
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.
--- 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.
--- 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
Re: Review Request 36757: Added https support.
--- 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
