> On Nov. 14, 2014, 8:10 a.m., Adam B wrote: > > src/master/master.cpp, lines 314-317 > > <https://reviews.apache.org/r/27760/diff/5/?file=762954#file762954line314> > > > > Why not use Master::finalize()?
Much better indeed. > On Nov. 14, 2014, 8:10 a.m., Adam B wrote: > > src/authentication/cram_md5/authenticator.hpp, lines 501-502 > > <https://reviews.apache.org/r/27760/diff/5/?file=762952#file762952line501> > > > > 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. As correctly noted by Vinod and also as described in the master flags, "authenticate_xxx" is a MAY/MUST not a OFF/ON. > On Nov. 14, 2014, 8:10 a.m., Adam B wrote: > > src/authentication/cram_md5/authenticator.hpp, line 537 > > <https://reviews.apache.org/r/27760/diff/5/?file=762952#file762952line537> > > > > If this is an initialize error, then you shouldn't need to test a > > boolean or send an AuthErrMsg here. Narf, indeed - this one errors out already in the master initializing. > On Nov. 14, 2014, 8:10 a.m., Adam B wrote: > > src/authentication/cram_md5/authenticator.hpp, line 546 > > <https://reviews.apache.org/r/27760/diff/5/?file=762952#file762952line546> > > > > If this becomes an initialize error, then you shouldn't need to check a > > boolean or send an AuthErrMsg here. I went a slightly different route now. When the initializing failed we either entirely fail the master (when authenticate_xxxx had been set) or show a warning and kill the authenticator instance. If then, later on an authentication request is received and we dont have an authenticator instance at hand, we LOG an error and send back an AuthenticationErrorMessage. > On Nov. 14, 2014, 8:10 a.m., Adam B wrote: > > src/authentication/cram_md5/authenticator.hpp, lines 559-560 > > <https://reviews.apache.org/r/27760/diff/5/?file=762952#file762952line559> > > > > 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. So clang-format +2 extra spaces on the parameter (not sure why clang-format doesnt get this right though). > On Nov. 14, 2014, 8:10 a.m., Adam B wrote: > > src/master/master.cpp, line 379 > > <https://reviews.apache.org/r/27760/diff/5/?file=762954#file762954line379> > > > > Should we even bother reading this flag or reading in the credentials > > file if neither --authenticate flag is set? > > Vinod Kone wrote: > the reason we originally loaded credentials despite "--authenticate" flag > being unset is so that it easy to upgrade scheduler and masters. > "--authenticate" means only only authenticated frameworks can register. not > specifying "--authenticate" means allowing authenticating or > non-authenticating frameworks to register. maybe the flag should've been more > explicitly named. That means we will have to load/initialize the authenticator in any case. If a user has not supplied any credentials, the authenticator has to LOG(ERROR) once an authentication request is received. This also means that we have to transmit an AuthenticationErrorMsg back to the authenticatee in such case. > On Nov. 14, 2014, 8:10 a.m., Adam B wrote: > > src/master/master.cpp, line 395 > > <https://reviews.apache.org/r/27760/diff/5/?file=762954#file762954line395> > > > > Should we bother reading this flag, or checking the module manager for > > an authenticator, or loading the authenticator, if neither --authenticate > > flag is set? See Vinod's comment. - Till ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review61413 ----------------------------------------------------------- On Nov. 13, 2014, 11: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, 11: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 > >
