mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.
Date Sat, 12 Dec 2015 01:37:16 GMT

-----------------------------------------------------------
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 <alexander@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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message