This is an automatically generated e-mail. To reply, visit:

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 

commit 6bd9b65b9d93f154ff9187fb5e5136262ede043c
Author: Alexander Rojas <alexan...@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

    Review: https://reviews.apache.org/r/38000

3rdparty/libprocess/include/process/event.hpp (line 121)

    const ref here

3rdparty/libprocess/include/process/process.hpp (lines 237 - 238)

    One newline here :)

3rdparty/libprocess/src/authentication_router.hpp (lines 52 - 53)

    "... rathern than having the authentication router maintain the mapping"

3rdparty/libprocess/src/process.cpp (lines 238 - 256)

    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 
    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):
    Future<Option<AuthenticationResult>> authenticate(const Request& request)
      Future<Option<AuthenticationResult>> authentication =
      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)

    Should we s/authorized/authenticated/ here since that's really what this 

3rdparty/libprocess/src/process.cpp (line 2498)

    Double // here?

3rdparty/libprocess/src/process.cpp (line 3367)

    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)

    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 

3rdparty/libprocess/src/tests/http_tests.cpp (line 76)

    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)

    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)

    Why call this 'addAuthenticator' instead of 'setAuthenticator' to match the 
interface we're wrapping?

3rdparty/libprocess/src/tests/http_tests.cpp (lines 1227 - 1229)

    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)

    How about s/registeredRealms/realms/ ?
    Also should this be private?

3rdparty/libprocess/src/tests/http_tests.cpp (line 1238)

    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)

    What does a disabled endpoint mean?

3rdparty/libprocess/src/tests/http_tests.cpp (line 1255)

    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)

    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)

    Hm.. I thought I already commented on avoiding "auth" abbreviations in 
previous reviews?

3rdparty/libprocess/src/tests/http_tests.cpp (line 1271)

    What does disabled endpoint mean here..?

3rdparty/libprocess/src/tests/http_tests.cpp (line 1284)

    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)

    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)

    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)

    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)

    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)

    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)

    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

Reply via email to