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