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 Tue, 08 Dec 2015 23:19:32 GMT

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


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