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 19:52:52 GMT

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


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