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 37999: Introduced an Authenticator interface and an AuthenticationRouter in libprocess.
Date Wed, 18 Nov 2015 14:31:00 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review107003
-----------------------------------------------------------

Ship it!


Was great sitting down and going over all of this stuff with you over the past week, thanks
for the patience! Just some final adjustments that we discussed based on pulling this down
and getting ready to commit.


3rdparty/libprocess/include/process/authenticator.hpp (line 52)
<https://reviews.apache.org/r/37999/#comment165889>

    Let's maybe say here that this allows us to implement different authenticators based on
the scheme and include the examples below (Basic, Digest, SPNEGO, etc).



3rdparty/libprocess/include/process/authenticator.hpp (line 65)
<https://reviews.apache.org/r/37999/#comment165883>

    Whoops, stale now that you have the namespace.



3rdparty/libprocess/include/process/http.hpp (lines 545 - 552)
<https://reviews.apache.org/r/37999/#comment165890>

    As we discussed, should we have a TODO to remove these?



3rdparty/libprocess/src/authentication_router.hpp (lines 35 - 37)
<https://reviews.apache.org/r/37999/#comment165891>

    A little bit stale now that we called this router instead of realm manager, right?



3rdparty/libprocess/src/authentication_router.hpp (line 48)
<https://reviews.apache.org/r/37999/#comment165896>

    How about unset for symettry? I see why you called this 'set' and 'unset' and that does
describe the single authenticator per-realm constraint. Thinking over this again though, let's
also add a small comment to describe this constraint.



3rdparty/libprocess/src/authentication_router.hpp (line 57)
<https://reviews.apache.org/r/37999/#comment165897>

    Whoops, "either" here is orphaned.



3rdparty/libprocess/src/authentication_router.hpp (line 69)
<https://reviews.apache.org/r/37999/#comment165881>

    Whoops, stale comment.



3rdparty/libprocess/src/authentication_router.cpp (line 96)
<https://reviews.apache.org/r/37999/#comment165910>

    Hm.. perhaps we also need something here to mention that we're assuming absolute paths,
which is ensured by libprocess. Should consider maybe returning a failure here when a relative
path is detected.



3rdparty/libprocess/src/authentication_router.cpp (line 107)
<https://reviews.apache.org/r/37999/#comment165911>

    Whoops, should have a space before the brace here.



3rdparty/libprocess/src/authentication_router.cpp (line 111)
<https://reviews.apache.org/r/37999/#comment165913>

    Perhaps we should handle the "error" logging case first, as that tends to be how we structure
the code and we can avoid an else more clearly



3rdparty/libprocess/src/authentication_router.cpp (lines 114 - 116)
<https://reviews.apache.org/r/37999/#comment165912>

    How about we validate that the Result is valid before we send it up to libprocess? We
can CHECK validity in libprocess since we control the code here, and avoid having to do validity
checking (which we weren't really doing currently).



3rdparty/libprocess/src/authentication_router.cpp (line 136)
<https://reviews.apache.org/r/37999/#comment165916>

    Why the 'if' here? 'process' should not be null here, unless there is a bug. We could
add a check but for now I'll just remove the 'if' guard to be consistent with the rest of
the process wrapper code FWICT.



3rdparty/libprocess/src/process.cpp (line 60)
<https://reviews.apache.org/r/37999/#comment165918>

    What was this for? The result? Hm.. should be ok in this case to just include the router
since we are only using signatures from the router.



3rdparty/libprocess/src/process.cpp (line 109)
<https://reviews.apache.org/r/37999/#comment165917>

    Alphabetical?



3rdparty/libprocess/src/process.cpp (line 128)
<https://reviews.apache.org/r/37999/#comment165919>

    We should consider just making this authentication::Router, but I'll leave it for now.



3rdparty/libprocess/src/process.cpp (line 953)
<https://reviews.apache.org/r/37999/#comment165920>

    Even though there whould probably be just one comment above all these saying we're creating
the globals, let's add another comment here to be consistent with the current code:
    
    ```
    // Create the global HTTP authentication router.
    authentication_router = new AuthenticationRouter();
    ```



3rdparty/libprocess/src/process_reference.hpp (line 52)
<https://reviews.apache.org/r/37999/#comment165882>

    This should be in a separate patch, much like we did for the promise setting bug we noticed.


- Ben Mahler


On Nov. 18, 2015, 10:43 a.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
> 
> (Updated Nov. 18, 2015, 10:43 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3231
>     https://issues.apache.org/jira/browse/MESOS-3231
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The Authenticator interface allows us to implement different
> authenticators based on the scheme (e.g. Basic, Digest, SPNEGO).
> The AuthenticationRouter manages the authentication realms and
> the mapping from endpoints to realms. It is then used by the
> ProcessManager to route requests to the authenticator for the
> realm, if applicable.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am cdefa37528ea69422978a9772f955042e882dde4 
>   3rdparty/libprocess/include/Makefile.am e6be2c4db121585bbe7f3c0627de048adc7cfb2c 
>   3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/event.hpp 28ce1928877084f0e1a73fdad789224c86e53f46

>   3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a

>   3rdparty/libprocess/src/CMakeLists.txt fb9bd04832779ac43151f2feb3dfbf58cb996434 
>   3rdparty/libprocess/src/authentication_router.hpp PRE-CREATION 
>   3rdparty/libprocess/src/authentication_router.cpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp 7abdf21a5784920251c3627f9820c12fdc356c6e 
>   3rdparty/libprocess/src/process_reference.hpp e6110bba2a54948be68e58ab9de988565b7d95a8

> 
> Diff: https://reviews.apache.org/r/37999/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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