----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review61413 -----------------------------------------------------------
src/authentication/cram_md5/authenticator.hpp <https://reviews.apache.org/r/27760/#comment102985> Now that I think about it, the --authenticate flags are the real on/off switch for authentication, so if either authentication is enabled, and CRAM is the authenticator you selected, then you should require credentials to be loaded, and not specifying credentials should be an authenticator initialization error. src/authentication/cram_md5/authenticator.hpp <https://reviews.apache.org/r/27760/#comment102997> If this is an initialize error, then you shouldn't need to test a boolean or send an AuthErrMsg here. src/authentication/cram_md5/authenticator.hpp <https://reviews.apache.org/r/27760/#comment102996> If this becomes an initialize error, then you shouldn't need to check a boolean or send an AuthErrMsg here. src/authentication/cram_md5/authenticator.hpp <https://reviews.apache.org/r/27760/#comment102989> This just keeps looking weird to me, and a quick $ grep -rn "^[^(]*) : " src/ only shows one other result: src/slave/containerizer/external_containerizer.cpp:242: const Flags& _flags) : flags(_flags) {} More common examples result from $ grep -B1 -rn "^ *: [A-Z].*)$" src/ or $ grep -A1 -rn "::.*($" src/ Also, you should indent the 'const process::UPID& pid' by four spaces, and ': AuthenticatorSession(pid)' by only two. src/master/master.cpp <https://reviews.apache.org/r/27760/#comment102986> Why not use Master::finalize()? src/master/master.cpp <https://reviews.apache.org/r/27760/#comment102994> Should we even bother reading this flag or reading in the credentials file if neither --authenticate flag is set? src/master/master.cpp <https://reviews.apache.org/r/27760/#comment102995> Should we bother reading this flag, or checking the module manager for an authenticator, or loading the authenticator, if neither --authenticate flag is set? src/master/master.cpp <https://reviews.apache.org/r/27760/#comment102990> I don't see how in your current code you could get authenticator==NULL while the master stays alive? Wouldn't it have exited during Master::initialize? If we do make loading an authenticator conditional on the --authenticate flags, then this situation could arise when the master isn't set up for authentication but the authenticatee tries anyway. In this case, you don't have to tell them what authenticator name (if any) wasn't initialized, just that there is "No authenticator loaded" src/tests/cram_md5_authentication_tests.cpp <https://reviews.apache.org/r/27760/#comment102992> Wouldn't you want to delete the session before the authenticator, since it was created after the authenticator? Ditto for other tests. - Adam B On Nov. 13, 2014, 3:52 p.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27760/ > ----------------------------------------------------------- > > (Updated Nov. 13, 2014, 3:52 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 47f3bc9 > src/master/master.cpp 0f89d1f > src/tests/cram_md5_authentication_tests.cpp a356aa1 > > Diff: https://reviews.apache.org/r/27760/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Till Toenshoff > >
