> On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: > > src/authentication/cram_md5/authenticator.cpp, line 449 > > <https://reviews.apache.org/r/27760/diff/17/?file=869278#file869278line449> > > > > Shouldn't this be protected by once() to avoid 2 different threads > > loading secrets at the same time? > > Till Toenshoff wrote: > That would render our tests broken (those that reset the credentials). > The concurrency is covered by a Lock within that SASL aux plugin code. > > Till Toenshoff wrote: > Let me know if this is ok, please.
Sorry, I don't think I follow. Can you elaborate on what specific tests fail and why? > On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 466-467 > > <https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line466> > > > > why this if condition for logging? > > Till Toenshoff wrote: > Not sure I understand. Did you see the comment directly above those lines? > ``` > // A failure to initialize the authenticator does lead to > // unusable authentication but still allows non authenticating > // frameworks and slaves to connect. > ``` > > Adam B wrote: > I think (a few revisions back), the thought was that the default > parameters for a master are authenticate_frameworks/slaves=false && > credentials=none && authenticator=default(cram), at which point it seems > unnecessary to warn somebody that we didn't load the authenticator. Upon > returning to this decision, I see no reason not to log this message anytime > authentication is disabled, even if it's the default setting. Maybe it would > seem less harsh as an INFO in the default case, but a single WARN message on > master startup could be a gentle nudge to try out authentication. > > Till Toenshoff wrote: > Let's try to get this sorted out as well. Any actions needed? I just meant, that we should probably always log that warning irrespective of the 'if(credentials.isSome() || (authenticatorNames[0] != DEFAULT_AUTHENTICATOR)) ' condition. IIUC, the fact that we are initializing an authenticator, irrespective of whether 'credentials' is provided is because there could be authenticators which don't depend on 'credentials'. So, if there are initialization errors it is better to log them sooner than later when they do enable 'authenticate_frameworks' or 'authenticate_slaves'. Alternatively, I would just simplify this as follows: ``` if (intialize.isError()) { EXIT(1) << "Failed to initialize authenticator '" << authenticatorNames[0] << ": " << initialize.error(); } ``` - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73310 ----------------------------------------------------------- On Feb. 27, 2015, 2:28 a.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27760/ > ----------------------------------------------------------- > > (Updated Feb. 27, 2015, 2:28 a.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 > > > 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 > ----- > > include/mesos/authentication/authenticator.hpp f66217a > src/Makefile.am 17d0d7a > src/authentication/cram_md5/authenticator.hpp c6f465f > src/authentication/cram_md5/authenticator.cpp PRE-CREATION > src/authentication/cram_md5/auxprop.hpp b894386 > src/authentication/cram_md5/auxprop.cpp cf503a2 > src/master/master.hpp e288cdb > src/master/master.cpp 76e217d > src/tests/cram_md5_authentication_tests.cpp 92a89c5 > > Diff: https://reviews.apache.org/r/27760/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Till Toenshoff > >
