mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
Date Thu, 18 Dec 2014 02:43:40 GMT

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

Ship it!


Looks pretty good to me. Several nits about comments and log messages. The only bug I found
is a send() to `pid` instead of `from`.


src/authentication/authenticator.hpp
<https://reviews.apache.org/r/27760/#comment108552>

    s/getting//



src/authentication/authenticator.hpp
<https://reviews.apache.org/r/27760/#comment108553>

    s/e.g.//
    s/the connection to a database/a database connection/



src/authentication/authenticator.hpp
<https://reviews.apache.org/r/27760/#comment108558>

    s/getting//



src/authentication/authenticator.hpp
<https://reviews.apache.org/r/27760/#comment108561>

    Do we really want to reference the 'master' here? Authenticator could conceivably be used
by anything that wants to authenticate an authenticatee. Perhaps slave will want to authenticate
its executors at some point.
    s/by the master//
    Same applies to other 'master' references in this file.



src/authentication/cram_md5/authenticator.hpp
<https://reviews.apache.org/r/27760/#comment108570>

    Might we want a boolean or a Once to ensure that initialize only gets called once per
authenticator? Or am I just being paranoid?



src/authentication/cram_md5/authenticator.hpp
<https://reviews.apache.org/r/27760/#comment108565>

    isNone()?



src/authentication/cram_md5/authenticator.hpp
<https://reviews.apache.org/r/27760/#comment108567>

    What makes these "registration" credentials? Couldn't these authenticators be used for
anything requiring authentication?
    Maybe just remove the comment.



src/authentication/cram_md5/authenticator.hpp
<https://reviews.apache.org/r/27760/#comment108574>

    Does this call have a return status we should be checking?



src/master/master.cpp
<https://reviews.apache.org/r/27760/#comment108577>

    Maybe use the actual DEFAULT_AUTHENTICATOR string instead of 'CRAM-MD5'



src/master/master.cpp
<https://reviews.apache.org/r/27760/#comment108581>

    Maybe preface with "Cannot start master with authentication enabled: "



src/master/master.cpp
<https://reviews.apache.org/r/27760/#comment108580>

    Won't this warning show up on every default master start, with --credentials unspecified
and the default cram_md5 authenticator? Seems a bit verbose for the default state.
    Maybe only warn if credentials is set or authenticator is not default.



src/master/master.cpp
<https://reviews.apache.org/r/27760/#comment108584>

    Maybe worth clarifying (in a comment) that `from` will be the authenticatee process, while
`pid` is the actual framework/slave process being authenticated.



src/master/master.cpp
<https://reviews.apache.org/r/27760/#comment108583>

    !!! send(from, message); ?



src/master/master.cpp
<https://reviews.apache.org/r/27760/#comment108585>

    I wonder if authenticatorSessions should put/erase `from` instead of `pid`, since the
session is actually with the authenticatee (`from`). What would be the effect of such a change?



src/tests/cram_md5_authentication_tests.cpp
<https://reviews.apache.org/r/27760/#comment108588>

    ...when the Authenticator Session is destroyed...



src/tests/cram_md5_authentication_tests.cpp
<https://reviews.apache.org/r/27760/#comment108589>

    ...should fail the default (cram_md5) authenticator initialization.


- Adam B


On Nov. 17, 2014, 5:13 p.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27760/
> -----------------------------------------------------------
> 
> (Updated Nov. 17, 2014, 5:13 p.m.)
> 
> 
> Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2050
>     https://issues.apache.org/jira/browse/MESOS-2050
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> The initial design and implementation of the authenticator module interface caused issues
and was not optimal for heavy lifting setup of external dependencies. By introducing a two
fold design, this has been decoupled from the authentication message processing. The new design
also gets us back on track to the goal of makeing SASL a soft dependency of mesos.
> 
> 
> Diffs
> -----
> 
>   src/authentication/authenticator.hpp 460494a 
>   src/authentication/cram_md5/authenticator.hpp d739a02 
>   src/master/master.hpp ece36c3 
>   src/master/master.cpp b5fa8d1 
>   src/tests/cram_md5_authentication_tests.cpp a356aa1 
> 
> Diff: https://reviews.apache.org/r/27760/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>


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