You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rojas <al...@mesosphere.io> on 2015/12/07 15:14:37 UTC
Re: Review Request 38000: Introduced support for user interaction
with HTTP AuthenticationRouter.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38000/
-----------------------------------------------------------
(Updated Dec. 7, 2015, 3:14 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Bugs: MESOS-3233
https://issues.apache.org/jira/browse/MESOS-3233
Repository: mesos
Description
-------
Adds functions which allow libprocess users to register HTTP authenticators.
Overloads `ProcesBase::route()` to allow for registering of authenticating endpoints.
Includes tests.
Diffs (updated)
-----
3rdparty/libprocess/include/process/event.hpp a03824c061c4a0eb865b163999a763635e56744c
3rdparty/libprocess/include/process/process.hpp 81c094414d4d5ac5eb593df2a6d14aaacb19a826
3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d
3rdparty/libprocess/src/tests/http_tests.cpp 2de75ca1c7e224c36b534c368e7379dc158aa5bb
Diff: https://reviews.apache.org/r/38000/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 38000: Introduced support for user interaction
with HTTP AuthenticationRouter.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On Dec. 7, 2015, 11:51 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 3218-3220
> > <https://reviews.apache.org/r/38000/diff/19/?file=1155108#file1155108line3218>
> >
> > If this is an internal invariant, how about adding a `CHECK` here?
How would the check be liked? if you read the comment it either has a principal because otherwise authentication failed or there is no principal because authentication wasn't needed at all. I changed the comment though.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38000/#review109203
-----------------------------------------------------------
On Dec. 7, 2015, 3:14 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38000/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2015, 3:14 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-3233
> https://issues.apache.org/jira/browse/MESOS-3233
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds functions which allow libprocess users to register HTTP authenticators.
> Overloads `ProcesBase::route()` to allow for registering of authenticating endpoints.
> Includes tests.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/event.hpp a03824c061c4a0eb865b163999a763635e56744c
> 3rdparty/libprocess/include/process/process.hpp 81c094414d4d5ac5eb593df2a6d14aaacb19a826
> 3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d
> 3rdparty/libprocess/src/tests/http_tests.cpp 2de75ca1c7e224c36b534c368e7379dc158aa5bb
>
> Diff: https://reviews.apache.org/r/38000/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 38000: Introduced support for user interaction
with HTTP AuthenticationRouter.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38000/#review109203
-----------------------------------------------------------
3rdparty/libprocess/include/process/process.hpp (lines 261 - 263)
<https://reviews.apache.org/r/38000/#comment168678>
I'm not sure how the comment is related to the typedef. Do you mean one of the parameter is `principal`? Mind explaining it then?
3rdparty/libprocess/include/process/process.hpp (line 264)
<https://reviews.apache.org/r/38000/#comment168682>
Given the second argument is optional (and assuming it's principal), I believe `AuthenticatedHttpRequestHandler` may also be responsible for requests that do not require authentication (e.g. no authenticator for the realm). If my understanding is correct, naming the handler with `Authenticated*` can be misleading.
3rdparty/libprocess/src/process.cpp (lines 3218 - 3220)
<https://reviews.apache.org/r/38000/#comment168691>
If this is an internal invariant, how about adding a `CHECK` here?
3rdparty/libprocess/src/tests/http_tests.cpp (lines 20 - 23)
<https://reviews.apache.org/r/38000/#comment168693>
Sort alphabetically
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1314 - 1315)
<https://reviews.apache.org/r/38000/#comment168694>
Blank line or swap.
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1357 - 1358)
<https://reviews.apache.org/r/38000/#comment168696>
Blank line or swap.
3rdparty/libprocess/src/tests/http_tests.cpp (line 1373)
<https://reviews.apache.org/r/38000/#comment168698>
Let's definitely have a comment here why there is sleep. Also, why not `os::sleep()`?
- Alexander Rukletsov
On Dec. 7, 2015, 2:14 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38000/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2015, 2:14 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-3233
> https://issues.apache.org/jira/browse/MESOS-3233
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds functions which allow libprocess users to register HTTP authenticators.
> Overloads `ProcesBase::route()` to allow for registering of authenticating endpoints.
> Includes tests.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/event.hpp a03824c061c4a0eb865b163999a763635e56744c
> 3rdparty/libprocess/include/process/process.hpp 81c094414d4d5ac5eb593df2a6d14aaacb19a826
> 3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d
> 3rdparty/libprocess/src/tests/http_tests.cpp 2de75ca1c7e224c36b534c368e7379dc158aa5bb
>
> Diff: https://reviews.apache.org/r/38000/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 38000: Introduced support for user interaction
with HTTP AuthenticationRouter.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On Dec. 9, 2015, 12:19 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 1339-1343
> > <https://reviews.apache.org/r/38000/diff/21/?file=1155956#file1155956line1339>
> >
> > Hm.. it looks like you're using the principals here to check that the requests arrive to the endpoint in the right order?
> >
> > How about avoiding the captures here and just specifying "principal1" and "principal2" in the expectation?
> >
> > Also, you're returning "1" and "2" but that's not checked at all? Did you mean to check that the responses below are ordered correctly?
It is not enough to just set the principal in the expectation, since the expectation only says that the function will be called with the given parameters, but mentions nothing about the ordering.
> On Dec. 9, 2015, 12:19 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 1373-1375
> > <https://reviews.apache.org/r/38000/diff/21/?file=1155956#file1155956line1373>
> >
> > It's not clear why the sleep is here. If I had to guess, it looks like you wanted to ensure that the both authentication calls were made at this point? If so, we don't need a sleep for this.
> >
> > Let's try to avoid the sleep here if possible.
When pipelining is not enforced, without the sleep the test is just flaky, since often the messages will be executed in the right order. I can remove it, but as I mentioned, the test passes from failing everytime to only sometimes.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38000/#review109405
-----------------------------------------------------------
On Dec. 8, 2015, 3:38 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38000/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 3:38 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-3233
> https://issues.apache.org/jira/browse/MESOS-3233
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds functions which allow libprocess users to register HTTP authenticators.
> Overloads `ProcesBase::route()` to allow for registering of authenticating endpoints.
> Includes tests.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/event.hpp a03824c061c4a0eb865b163999a763635e56744c
> 3rdparty/libprocess/include/process/process.hpp 81c094414d4d5ac5eb593df2a6d14aaacb19a826
> 3rdparty/libprocess/src/authentication_router.hpp 5777deafd7420324627802a7ab9da9aaa2b46825
> 3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d
> 3rdparty/libprocess/src/tests/http_tests.cpp 2de75ca1c7e224c36b534c368e7379dc158aa5bb
>
> Diff: https://reviews.apache.org/r/38000/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 38000: Introduced support for user interaction
with HTTP AuthenticationRouter.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38000/#review109405
-----------------------------------------------------------
Second part of my earlier review, this time I went throug the test code.
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1214 - 1216)
<https://reviews.apache.org/r/38000/#comment168899>
Looking at this, it's a little strange that realm comes after help.
Also, what's "mesos"? :)
In the interest of making this as readable as possible, how about we just use "realm" in the tests here? Why do anything less direct?
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1226 - 1250)
<https://reviews.apache.org/r/38000/#comment168906>
After you re-use HttpProxy, the only thing that should be in this fixture is setting the authenticator during setup and unsetting during teardown.
How about naming this fixture 'HttpAuthenticationTest'? I can't quite tell why some names have http as the test case name, and some don't:
```
AuthenticationTest. HTTPAuthenticationNoAuthenticator
AuthenticationTest, AuthenticationNoCredentials
AuthenticationTest, AuthenticationSuccess
AuthenticationTest, DelayedAuthentication
```
How about the following, also note I tried not to repeat authentication since it's clear from the test fixture name:
```
HttpAuthenticationTest, Disabled
HttpAuthenticationTest, Unauthorized
HttpAuthenticationTest, Forbidden
HttpAuthenticationTest, Authenticated
HttpAuthenticationTest, Pipelining
```
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1253 - 1254)
<https://reviews.apache.org/r/38000/#comment168909>
An endpoint doesn't really request authentication, it just specifies a realm, no? I imagine that the wiring of authenticators to realms isn't tightly tied to the route() calls.
How about:
```
Ensures that when there is no authenticator for
a realm, requests are not authenticated (i.e. the
principal is None).
```
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1258 - 1259)
<https://reviews.apache.org/r/38000/#comment168912>
Ugh.
I suspected I was going to see something like this when I saw your test fixture. If we're going to put things in test fixtures I would suggest making them flexible and explicit, rather than implicit and static.
For example, rather than statically deciding to add an authenticator to the "mesos" realm (which let's just call "realm"!), we can have an explicit method for adding an authenticator:
```
addAuthenticator("realm", Owned<Authenticator>(new MockAuthenticator());
```
When a test calls this, the test fixture will keep track of the set of realms that need to be unset, so that we can clean up in TearDown.
Sound good?
3rdparty/libprocess/src/tests/http_tests.cpp (line 1270)
<https://reviews.apache.org/r/38000/#comment168914>
Have you looked at gtest.hpp in libprocess?
We have: AWAIT_EXPECT_RESPONSE_STATUS_EQ
We should update that assertion to check both the code and status, not sure why we manually check both all over the place.
3rdparty/libprocess/src/tests/http_tests.cpp (line 1273)
<https://reviews.apache.org/r/38000/#comment168917>
No CHECKs in tests please :(
Also, you don't need to do this AFAICT, you can just do the following above to simplify this:
```
EXPECT_CALL((*process.get()), handler1(_, Option<string>::none()));
```
Does that not work?
3rdparty/libprocess/src/tests/http_tests.cpp (line 1281)
<https://reviews.apache.org/r/38000/#comment168932>
Let's avoid the "auth" abbreviation entirely, can you do a sweep? I also see it in the tests below.
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1295 - 1297)
<https://reviews.apache.org/r/38000/#comment168947>
Why is this EXPECT_SOME_EQ instead of EXPECT_EQ? Aren't both of these options?
3rdparty/libprocess/src/tests/http_tests.cpp (line 1305)
<https://reviews.apache.org/r/38000/#comment168930>
In the interest of making this more readable:
s/"foo"/"principal"/
3rdparty/libprocess/src/tests/http_tests.cpp (line 1309)
<https://reviews.apache.org/r/38000/#comment168933>
Why is there an underscore here?
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1310 - 1311)
<https://reviews.apache.org/r/38000/#comment168934>
Do you need to capture? It looks like you can just place the value in the expectation:
```
EXPECT_CALL((*process.get()), handler1(_, "principal"));
```
Not sure if it can implicitly construct the option here, might need to do that explicitly.
3rdparty/libprocess/src/tests/http_tests.cpp (line 1316)
<https://reviews.apache.org/r/38000/#comment168931>
Can you just do "user:password" here? The comment seems sufficient to me
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1320 - 1322)
<https://reviews.apache.org/r/38000/#comment168935>
If you don't need the capture the option, can you use the AWAIT_EXPECT_RESPONSE_STATUS_EQ here? Ditto elsehwere
3rdparty/libprocess/src/tests/http_tests.cpp (line 1323)
<https://reviews.apache.org/r/38000/#comment168945>
If you have two options, you can just use EXPECT_EQ against the options directly:
```
EXPECT_EQ(authResult.principal, principal);
```
Although I think you don't need this per my other comment.
3rdparty/libprocess/src/tests/http_tests.cpp (line 1332)
<https://reviews.apache.org/r/38000/#comment168936>
This confused me a bit, how about:
```
// We satisfy the authentication futures in reverse
// order. Libprocess should not re-order requests
// when this occurs.
```
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1339 - 1343)
<https://reviews.apache.org/r/38000/#comment168937>
Hm.. it looks like you're using the principals here to check that the requests arrive to the endpoint in the right order?
How about avoiding the captures here and just specifying "principal1" and "principal2" in the expectation?
Also, you're returning "1" and "2" but that's not checked at all? Did you mean to check that the responses below are ordered correctly?
3rdparty/libprocess/src/tests/http_tests.cpp (line 1359)
<https://reviews.apache.org/r/38000/#comment168938>
Ditto `username:password` here to be more direct, since you already have a comment above.
3rdparty/libprocess/src/tests/http_tests.cpp (line 1366)
<https://reviews.apache.org/r/38000/#comment168939>
newline here
3rdparty/libprocess/src/tests/http_tests.cpp (line 1369)
<https://reviews.apache.org/r/38000/#comment168940>
No auth abbreviations please :)
3rdparty/libprocess/src/tests/http_tests.cpp (line 1370)
<https://reviews.apache.org/r/38000/#comment168942>
Instead of "foo" and "bar", how about "principal1" and "principal2"?
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1373 - 1375)
<https://reviews.apache.org/r/38000/#comment168941>
It's not clear why the sleep is here. If I had to guess, it looks like you wanted to ensure that the both authentication calls were made at this point? If so, we don't need a sleep for this.
Let's try to avoid the sleep here if possible.
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1381 - 1387)
<https://reviews.apache.org/r/38000/#comment168943>
Can you use the other macro I mentioned earlier here as well?
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1389 - 1390)
<https://reviews.apache.org/r/38000/#comment168944>
If you have two options, you can just use EXPECT_EQ against the options directly:
```
EXPECT_EQ(authResult1.principal, principal1);
```
Although I think you don't need this per my other comment.
- Ben Mahler
On Dec. 8, 2015, 2:38 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38000/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 2:38 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-3233
> https://issues.apache.org/jira/browse/MESOS-3233
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds functions which allow libprocess users to register HTTP authenticators.
> Overloads `ProcesBase::route()` to allow for registering of authenticating endpoints.
> Includes tests.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/event.hpp a03824c061c4a0eb865b163999a763635e56744c
> 3rdparty/libprocess/include/process/process.hpp 81c094414d4d5ac5eb593df2a6d14aaacb19a826
> 3rdparty/libprocess/src/authentication_router.hpp 5777deafd7420324627802a7ab9da9aaa2b46825
> 3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d
> 3rdparty/libprocess/src/tests/http_tests.cpp 2de75ca1c7e224c36b534c368e7379dc158aa5bb
>
> Diff: https://reviews.apache.org/r/38000/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 38000: Introduced support for user interaction
with HTTP AuthenticationRouter.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38000/#review109962
-----------------------------------------------------------
Ship it!
Ship It!
- Bernd Mathiske
On Dec. 11, 2015, 6:01 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38000/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2015, 6:01 a.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-3233
> https://issues.apache.org/jira/browse/MESOS-3233
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds functions which allow libprocess users to register HTTP authenticators.
> Overloads `ProcesBase::route()` to allow for registering of authenticating endpoints.
> Includes tests.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/event.hpp a03824c061c4a0eb865b163999a763635e56744c
> 3rdparty/libprocess/include/process/http.hpp c9e38e5a382f29f87a2ed62602cae3d62f8e16cc
> 3rdparty/libprocess/include/process/process.hpp 81c094414d4d5ac5eb593df2a6d14aaacb19a826
> 3rdparty/libprocess/src/authentication_router.hpp 5777deafd7420324627802a7ab9da9aaa2b46825
> 3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d
> 3rdparty/libprocess/src/tests/http_tests.cpp 2de75ca1c7e224c36b534c368e7379dc158aa5bb
>
> Diff: https://reviews.apache.org/r/38000/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 38000: Introduced support for user interaction
with HTTP AuthenticationRouter.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38000/#review110006
-----------------------------------------------------------
Ship it!
I'll make the changes below in order to avoid more round trips on reviewboard. The biggest change was related to simplifying the pipelining code between the http proxy and the process manager, hope you like it better now!
Take a look over the comments, let me know if you think we should follow up on anything:
```
commit 6bd9b65b9d93f154ff9187fb5e5136262ede043c
Author: Alexander Rojas <al...@mesosphere.io>
Date: Fri Dec 11 12:30:37 2015 -0800
Allow users of libprocess to perform authentication of HTTP requests.
This change integrates the previously added AuthenticationRouter into
libprocess in order to allow users to authenticate HTTP requests. Now,
when making a 'route', a security 'realm' can be specified. The user
of libprocess determines which Authenticator will be used for each
'realm'.
Review: https://reviews.apache.org/r/38000
```
3rdparty/libprocess/include/process/event.hpp (line 121)
<https://reviews.apache.org/r/38000/#comment169838>
const ref here
3rdparty/libprocess/include/process/process.hpp (lines 237 - 238)
<https://reviews.apache.org/r/38000/#comment169759>
One newline here :)
3rdparty/libprocess/src/authentication_router.hpp (lines 52 - 53)
<https://reviews.apache.org/r/38000/#comment169760>
"... rathern than having the authentication router maintain the mapping"
3rdparty/libprocess/src/process.cpp (lines 238 - 256)
<https://reviews.apache.org/r/38000/#comment169783>
Hm.. it looks like the complexity here is coming from the fact that we currently use the router outside of the HttpProxy and we wanted to pipeline per-UPID.
We can do a big simplification here by using the router in the http proxy itself and punting on the per-UPID pipelining in favor of per-socket pipelining, since that was just a performance optimization (not worth the complexity cost after seeing how the code turned out).
We end up with the following in the proxy (comments omitted):
```
HttpProxy
{
Future<Option<AuthenticationResult>> authenticate(const Request& request)
{
Future<Option<AuthenticationResult>> authentication =
authentication_router->authenticate(request);
return authentications.add<Option<AuthenticationResult>>(
[=]() { return authentication; });
}
Sequence authentications;
}
```
And we don't have to have any cleanup code. Then the process manager code looks like:
```
dispatch(proxy, &HttpProxy::authenticate, *request)
.onAny([this, request, promise, receiver](
const Future<Option<AuthenticationResult>>& authentication) {
```
Which looks a lot better! :)
3rdparty/libprocess/src/process.cpp (line 2485)
<https://reviews.apache.org/r/38000/#comment169803>
Should we s/authorized/authenticated/ here since that's really what this means?
3rdparty/libprocess/src/process.cpp (line 2498)
<https://reviews.apache.org/r/38000/#comment169802>
Double // here?
3rdparty/libprocess/src/process.cpp (line 3367)
<https://reviews.apache.org/r/38000/#comment169839>
We probably need a little comment here and in removeEndpoint that we don't need to wait for the operations to complete.
3rdparty/libprocess/src/tests/http_tests.cpp (lines 51 - 54)
<https://reviews.apache.org/r/38000/#comment169807>
Why did you introduce these when all of the other tests are still using the http:: prefix? This makes it really inconsistent: for example, if I look at the mock method introduced to HttpProcess, it is the only one without the http qualifiers.
3rdparty/libprocess/src/tests/http_tests.cpp (line 76)
<https://reviews.apache.org/r/38000/#comment169809>
Let's just use "real" inline to keep it simple, no need to introduce a global :)
3rdparty/libprocess/src/tests/http_tests.cpp (lines 105 - 109)
<https://reviews.apache.org/r/38000/#comment169811>
At this point we would just wrap with the open paren, but this fits on one line when we just use "realm" instead of the global.
3rdparty/libprocess/src/tests/http_tests.cpp (line 1219)
<https://reviews.apache.org/r/38000/#comment169814>
Why call this 'addAuthenticator' instead of 'setAuthenticator' to match the interface we're wrapping?
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1227 - 1229)
<https://reviews.apache.org/r/38000/#comment169836>
We actually have to wait here, otherwise we can trigger the leak of a mock object if we exit after leaving tear down but before the unset takes place, leading to:
```
../../../3rdparty/libprocess/src/tests/http_tests.cpp:1334: ERROR: this mock object (used in test HttpAuthenticationTest.Pipelining) should be deleted but never is. Its address is @0x7feca1603300.
```
This means we'll actually have to expose the Futures up on the set/unset functions. That makes sense anyway since the callers will potentially want to ensure that authentication is set up before they start setting up routes.
3rdparty/libprocess/src/tests/http_tests.cpp (line 1232)
<https://reviews.apache.org/r/38000/#comment169815>
How about s/registeredRealms/realms/ ?
Also should this be private?
3rdparty/libprocess/src/tests/http_tests.cpp (line 1238)
<https://reviews.apache.org/r/38000/#comment169825>
Hm.. didn't I already comment on not repeating HttpAuthentication in both the test fixture and test case name here?
3rdparty/libprocess/src/tests/http_tests.cpp (line 1245)
<https://reviews.apache.org/r/38000/#comment169820>
What does a disabled endpoint mean?
3rdparty/libprocess/src/tests/http_tests.cpp (line 1255)
<https://reviews.apache.org/r/38000/#comment169826>
This looks to be more about testing Unauthorized, since we're not exercising any credential checking code?
Should we also have a test for Forbidden?
3rdparty/libprocess/src/tests/http_tests.cpp (line 1257)
<https://reviews.apache.org/r/38000/#comment169817>
We haven't really been using auto for cases like this (also not super intuitive whether we should have used `auto` or `auto*` here..)
Ditto elsewhere
3rdparty/libprocess/src/tests/http_tests.cpp (line 1264)
<https://reviews.apache.org/r/38000/#comment169821>
Hm.. I thought I already commented on avoiding "auth" abbreviations in previous reviews?
3rdparty/libprocess/src/tests/http_tests.cpp (line 1271)
<https://reviews.apache.org/r/38000/#comment169824>
What does disabled endpoint mean here..?
3rdparty/libprocess/src/tests/http_tests.cpp (line 1284)
<https://reviews.apache.org/r/38000/#comment169828>
It's a little confusing that this is talking about credentials when they are irrelevant to the test, maybe we shouldn't mention them here especially since we already have a comment saying they are ignored.
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1302 - 1305)
<https://reviews.apache.org/r/38000/#comment169831>
Why don't we just remove this to avoid confusion and say:
```
// Note that we don't bother pretending to specify a valid
// 'Authorization' header since we force authentication success.
```
3rdparty/libprocess/src/tests/http_tests.cpp (line 1318)
<https://reviews.apache.org/r/38000/#comment169832>
Hm.. how about s/DelayedAuthentication/Pipelining/ to tell the reader about what we're interested in testing here (pipelining), not how we're testing it (by delaying authentication).
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1321 - 1323)
<https://reviews.apache.org/r/38000/#comment169819>
Why the indirection with the global rather than the following straightforward line?
```
addAuthenticator("realm", Owned<Authenticator>(authenticator));
```
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1339 - 1340)
<https://reviews.apache.org/r/38000/#comment169835>
To avoid ordering assumptions here when we go to check the options below, let's use FutureArg and replace EXPECT_EQ with AWAIT_EXPECT_EQ below
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1353 - 1357)
<https://reviews.apache.org/r/38000/#comment169833>
Ditto here about not bothering with this, and leaving a comment:
```
// Note that we don't bother pretending to specify a valid
// 'Authorization' header since we force authentication success.
```
3rdparty/libprocess/src/tests/http_tests.cpp (line 1368)
<https://reviews.apache.org/r/38000/#comment169834>
How about s/Result// ?
- Ben Mahler
On Dec. 11, 2015, 2:01 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38000/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2015, 2:01 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-3233
> https://issues.apache.org/jira/browse/MESOS-3233
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds functions which allow libprocess users to register HTTP authenticators.
> Overloads `ProcesBase::route()` to allow for registering of authenticating endpoints.
> Includes tests.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/event.hpp a03824c061c4a0eb865b163999a763635e56744c
> 3rdparty/libprocess/include/process/http.hpp c9e38e5a382f29f87a2ed62602cae3d62f8e16cc
> 3rdparty/libprocess/include/process/process.hpp 81c094414d4d5ac5eb593df2a6d14aaacb19a826
> 3rdparty/libprocess/src/authentication_router.hpp 5777deafd7420324627802a7ab9da9aaa2b46825
> 3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d
> 3rdparty/libprocess/src/tests/http_tests.cpp 2de75ca1c7e224c36b534c368e7379dc158aa5bb
>
> Diff: https://reviews.apache.org/r/38000/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 38000: Introduced support for user interaction
with HTTP AuthenticationRouter.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38000/
-----------------------------------------------------------
(Updated Dec. 11, 2015, 3:01 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Changes after bernd's review.
Bugs: MESOS-3233
https://issues.apache.org/jira/browse/MESOS-3233
Repository: mesos
Description
-------
Adds functions which allow libprocess users to register HTTP authenticators.
Overloads `ProcesBase::route()` to allow for registering of authenticating endpoints.
Includes tests.
Diffs (updated)
-----
3rdparty/libprocess/include/process/event.hpp a03824c061c4a0eb865b163999a763635e56744c
3rdparty/libprocess/include/process/http.hpp c9e38e5a382f29f87a2ed62602cae3d62f8e16cc
3rdparty/libprocess/include/process/process.hpp 81c094414d4d5ac5eb593df2a6d14aaacb19a826
3rdparty/libprocess/src/authentication_router.hpp 5777deafd7420324627802a7ab9da9aaa2b46825
3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d
3rdparty/libprocess/src/tests/http_tests.cpp 2de75ca1c7e224c36b534c368e7379dc158aa5bb
Diff: https://reviews.apache.org/r/38000/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 38000: Introduced support for user interaction
with HTTP AuthenticationRouter.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38000/
-----------------------------------------------------------
(Updated Dec. 10, 2015, 3:25 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Changes after ben's review.
Bugs: MESOS-3233
https://issues.apache.org/jira/browse/MESOS-3233
Repository: mesos
Description
-------
Adds functions which allow libprocess users to register HTTP authenticators.
Overloads `ProcesBase::route()` to allow for registering of authenticating endpoints.
Includes tests.
Diffs (updated)
-----
3rdparty/libprocess/include/process/event.hpp a03824c061c4a0eb865b163999a763635e56744c
3rdparty/libprocess/include/process/http.hpp 3f33b46527e07ed73621e07e9c5d40594e32f6a9
3rdparty/libprocess/include/process/process.hpp 81c094414d4d5ac5eb593df2a6d14aaacb19a826
3rdparty/libprocess/src/authentication_router.hpp 5777deafd7420324627802a7ab9da9aaa2b46825
3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d
3rdparty/libprocess/src/tests/http_tests.cpp 2de75ca1c7e224c36b534c368e7379dc158aa5bb
Diff: https://reviews.apache.org/r/38000/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 38000: Introduced support for user interaction
with HTTP AuthenticationRouter.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On Dec. 8, 2015, 8:52 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/process.hpp, lines 65-86
> > <https://reviews.apache.org/r/38000/diff/19/?file=1155107#file1155107line65>
> >
> > Why isn't this in the http header? It looks like firewall belongs there too since it's more aptly namespaced as 'http::Firewall'?
I will move the firewall functions in a different request.
> On Dec. 8, 2015, 8:52 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 1205-1223
> > <https://reviews.apache.org/r/38000/diff/21/?file=1155956#file1155956line1205>
> >
> > Can you instead just add another handler to the existing HttpProcess above, rather than introducing another Process here?
> >
> > There is already a handler for 'auth', so we can add one called '/authenticated' and document why '/auth' is still there (looks like it is going to be obviated by this?).
About this one, there is one test that checks authentication which is handled through the auth method. That test (and its functionality) become obsolete with this patch, should I just removed?
> On Dec. 8, 2015, 8:52 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 2400-2402
> > <https://reviews.apache.org/r/38000/diff/21/?file=1155955#file1155955line2400>
> >
> > Why are you dispatching here rather than calling directly? Note though that I'm not sure we should keep `schedule`.
Mostly because BenH has strongly discourage me from calling methods directly from processes. Moreover, once the cleaning of unused sequences is implemented, is imposible to guarantee that requests are being done from the same thread always, so we may have concurrency issues.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38000/#review109243
-----------------------------------------------------------
On Dec. 8, 2015, 3:38 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38000/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 3:38 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-3233
> https://issues.apache.org/jira/browse/MESOS-3233
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds functions which allow libprocess users to register HTTP authenticators.
> Overloads `ProcesBase::route()` to allow for registering of authenticating endpoints.
> Includes tests.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/event.hpp a03824c061c4a0eb865b163999a763635e56744c
> 3rdparty/libprocess/include/process/process.hpp 81c094414d4d5ac5eb593df2a6d14aaacb19a826
> 3rdparty/libprocess/src/authentication_router.hpp 5777deafd7420324627802a7ab9da9aaa2b46825
> 3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d
> 3rdparty/libprocess/src/tests/http_tests.cpp 2de75ca1c7e224c36b534c368e7379dc158aa5bb
>
> Diff: https://reviews.apache.org/r/38000/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 38000: Introduced support for user interaction
with HTTP AuthenticationRouter.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38000/#review109243
-----------------------------------------------------------
This is starting to shape up, thanks Alexander. As we discussed offline, just wanted to surface the main bits of feedback so you can start addressing them. I will follow up with feedback for the tests, wasn't able to do a complete pass in one sitting.
3rdparty/libprocess/include/process/process.hpp (lines 65 - 86)
<https://reviews.apache.org/r/38000/#comment168723>
Why isn't this in the http header? It looks like firewall belongs there too since it's more aptly namespaced as 'http::Firewall'?
3rdparty/libprocess/include/process/process.hpp (lines 68 - 73)
<https://reviews.apache.org/r/38000/#comment168724>
How about the following to be a bit more succinct?
```
/**
* Sets (or overwrites) the authenticator for the realm.
*
* Every incoming HTTP request to an endpoint associated
* with the realm will be authenticated with the given
* authenticator.
*/
```
3rdparty/libprocess/include/process/process.hpp (lines 79 - 82)
<https://reviews.apache.org/r/38000/#comment168725>
Not sure we need to say "still", how about:
```
/**
* Unsets the authenticator for the realm.
*
* Any endpoint mapped to the realm will no
* longer be authenticated.
*/
```
3rdparty/libprocess/include/process/process.hpp (lines 261 - 263)
<https://reviews.apache.org/r/38000/#comment168726>
I'm guessing this is about introducing a type to make this more explicit in the signature, not type safety:
```
TODO(arojas): Consider introducing an `authentication::Principal` type.
```
Similarly, we probably need a TODO on top of the `route()` declarations for an `authentication::Realm` type?
3rdparty/libprocess/include/process/process.hpp (lines 261 - 264)
<https://reviews.apache.org/r/38000/#comment168820>
Looks like this needs javadoc like the existing typedef, then you can remove the `/* principal */` that you've added here.
3rdparty/libprocess/include/process/process.hpp (lines 372 - 374)
<https://reviews.apache.org/r/38000/#comment168863>
There is a typo in here :)
Also, could you try to avoid the run on sentence? How about saying something about http request handlers are equivalent to authenticated http request handlers with a none principal, so we convert them.
3rdparty/libprocess/src/authentication_router.hpp (lines 52 - 56)
<https://reviews.apache.org/r/38000/#comment168864>
The second sentence here confused me (e.g. the current implementation of what?), so how about we just write the first sentence and let the reader figure out the implications? Note that the implications could change over time so it's possible for it to go out of sync and cause more confusion.
3rdparty/libprocess/src/process.cpp (lines 231 - 232)
<https://reviews.apache.org/r/38000/#comment168869>
We really need to help the reader understand this!
```
// Http pipelining requires that when we perform http
// authentication, we need to ensure that authentication
// results are processed in sequential order. Otherwise,
// we may send responses out-of-order!
//
// The ordering needs to be respected within a Process, but
// not across Processes since dispatches are asynchronous.
//
// Note that this needs to be done explicitly here
// because the authentication router does not guarantee
// ordered completion of its Futures (it doesn't have the
// knowledge of UPIDs necessary to do it in a fine-grained
// manner).
hashmap<UPID, Sequence> authenticationPipelines; // Or,
// hashmap<UPID, Sequence> authenticationSequences;
```
Also, it would be great to manage this correctly and erase entries when they are not relevant any more. As we discussed:
* Store the last future from the sequence alongside here.
* Set up a callback on future completion, the callback should check for equality against the current last. If equal, the entry can be erased.
Anything I'm missing there?
3rdparty/libprocess/src/process.cpp (line 2399)
<https://reviews.apache.org/r/38000/#comment168871>
"within a Process' execution context"
3rdparty/libprocess/src/process.cpp (lines 2400 - 2402)
<https://reviews.apache.org/r/38000/#comment168870>
Why are you dispatching here rather than calling directly? Note though that I'm not sure we should keep `schedule`.
3rdparty/libprocess/src/process.cpp (line 2447)
<https://reviews.apache.org/r/38000/#comment168872>
3rdparty/libprocess/src/process.cpp (lines 2447 - 2455)
<https://reviews.apache.org/r/38000/#comment168873>
The first comment here should match the first comment from the other blocks, no? How about:
```
// Authentication succeeded.
const Option<string>& principal = authentication.get()->principal;
CHECK_SOME(principal); // The authentication router validates this.
// TODO(benh): Use the sender PID in order to capture
// happens-before timing relationships for testing.
deliver(receiver, new HttpEvent(request, promise, principal));
```
3rdparty/libprocess/src/process.cpp (line 3219)
<https://reviews.apache.org/r/38000/#comment168874>
s/ignored/None/
But does this comment even belong here? Seems it belongs alongside the route / handler declarations.
3rdparty/libprocess/src/tests/http_tests.cpp (line 21)
<https://reviews.apache.org/r/38000/#comment168875>
What is this for? :o
3rdparty/libprocess/src/tests/http_tests.cpp (lines 43 - 45)
<https://reviews.apache.org/r/38000/#comment168877>
Why aren't these alphabetical?
3rdparty/libprocess/src/tests/http_tests.cpp (line 45)
<https://reviews.apache.org/r/38000/#comment168876>
Why did you abbreviate to 'auth' here? It makes it ambiguous as to whether you're referring to authentication or authorization.
3rdparty/libprocess/src/tests/http_tests.cpp (lines 1205 - 1223)
<https://reviews.apache.org/r/38000/#comment168879>
Can you instead just add another handler to the existing HttpProcess above, rather than introducing another Process here?
There is already a handler for 'auth', so we can add one called '/authenticated' and document why '/auth' is still there (looks like it is going to be obviated by this?).
3rdparty/libprocess/src/tests/http_tests.cpp (line 1235)
<https://reviews.apache.org/r/38000/#comment168881>
Please add the `()` when you use `new`
- Ben Mahler
On Dec. 8, 2015, 2:38 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38000/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 2:38 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-3233
> https://issues.apache.org/jira/browse/MESOS-3233
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds functions which allow libprocess users to register HTTP authenticators.
> Overloads `ProcesBase::route()` to allow for registering of authenticating endpoints.
> Includes tests.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/event.hpp a03824c061c4a0eb865b163999a763635e56744c
> 3rdparty/libprocess/include/process/process.hpp 81c094414d4d5ac5eb593df2a6d14aaacb19a826
> 3rdparty/libprocess/src/authentication_router.hpp 5777deafd7420324627802a7ab9da9aaa2b46825
> 3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d
> 3rdparty/libprocess/src/tests/http_tests.cpp 2de75ca1c7e224c36b534c368e7379dc158aa5bb
>
> Diff: https://reviews.apache.org/r/38000/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 38000: Introduced support for user interaction
with HTTP AuthenticationRouter.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38000/
-----------------------------------------------------------
(Updated Dec. 8, 2015, 3:38 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Bugs: MESOS-3233
https://issues.apache.org/jira/browse/MESOS-3233
Repository: mesos
Description
-------
Adds functions which allow libprocess users to register HTTP authenticators.
Overloads `ProcesBase::route()` to allow for registering of authenticating endpoints.
Includes tests.
Diffs (updated)
-----
3rdparty/libprocess/include/process/event.hpp a03824c061c4a0eb865b163999a763635e56744c
3rdparty/libprocess/include/process/process.hpp 81c094414d4d5ac5eb593df2a6d14aaacb19a826
3rdparty/libprocess/src/authentication_router.hpp 5777deafd7420324627802a7ab9da9aaa2b46825
3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d
3rdparty/libprocess/src/tests/http_tests.cpp 2de75ca1c7e224c36b534c368e7379dc158aa5bb
Diff: https://reviews.apache.org/r/38000/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 38000: Introduced support for user interaction
with HTTP AuthenticationRouter.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38000/
-----------------------------------------------------------
(Updated Dec. 8, 2015, 11:23 a.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Alex's review.
Bugs: MESOS-3233
https://issues.apache.org/jira/browse/MESOS-3233
Repository: mesos
Description
-------
Adds functions which allow libprocess users to register HTTP authenticators.
Overloads `ProcesBase::route()` to allow for registering of authenticating endpoints.
Includes tests.
Diffs (updated)
-----
3rdparty/libprocess/include/process/event.hpp a03824c061c4a0eb865b163999a763635e56744c
3rdparty/libprocess/include/process/process.hpp 81c094414d4d5ac5eb593df2a6d14aaacb19a826
3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d
3rdparty/libprocess/src/tests/http_tests.cpp 2de75ca1c7e224c36b534c368e7379dc158aa5bb
Diff: https://reviews.apache.org/r/38000/diff/
Testing
-------
make check
Thanks,
Alexander Rojas