> 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. > > Vinod Kone wrote: > Sorry, I don't think I follow. Can you elaborate on what specific tests > fail and why? > > Till Toenshoff wrote: > Our tests all run within the same OS-process. A ONCE-gatekeeper on > credential loading within this process would indeed allow this only a single > time. The CRAM-MD5 tests do attempt to re/load the credentials; see > cram_md5_authentication.cpp #95, #140, ... > These tests make sense given that we want to test the behavior of the > authenticator/authenticatee when using non matching credentials.
I don't see them reloading out of band. In other words, before your patch we used to call secrets::load() directly from tests, which I agree has thread safety issues if secrets::load() itself is not protected. But now, with your patch, all the loading happens via Authenticator->initialize(). So I'm trying to understand why the protection in Authenticator initialize is not enough. Sorry, if I'm mising something. > On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: > > src/master/master.cpp, line 3842 > > <https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3842> > > > > I think seeing this log line in master log would not reveal much > > information on what's happening. > > > > How about > > > > LOG(ERROR) << "Received authentication request from " << pid > > << " but authenticator is not loaded"; AuthenticationErrorMessage is only used by authenticator. How about sending FrameworkError message instead? That way the framework will atleast abort and know that it is doing something wrong. With AuthenticationErrorMessage it will just blindly retry. > On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 3897-3898 > > <https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3897> > > > > Why do we need this 'onAny'? Why can't 'authenticated' be set in > > __authenticate()? > > Till Toenshoff wrote: > `__authenticated` is needed to capture (at least) a discard on the outer > future. I have rearranged the code of both continuations to be more explicit > about that. A Future::discard() *does not* result in a future being DISCARDED (and onAny callbacks being called). You want an onDiscard() handler (instead of onAny) to capture that case. > On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 3952-3954 > > <https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3952> > > > > Can you rephrase these log messages to something meaninguful when one > > looks at master log? > > Till Toenshoff wrote: > How about this? > ``` > if (authenticate.isDiscarded()) { > LOG(WARNING) << "Authentication for " << pid > << " has pending cancel request"; > } else if (authenticate.discard()) { > LOG(WARNING) << "Requested to cancel authentication for " << pid; > } > ``` > > Adam B wrote: > How about: "Tried to cancel authentication session for [pid], but it was > already cancelled" and "Cancelling authentication session for PID"? How are you cancelling the authentication here? - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73310 ----------------------------------------------------------- On March 3, 2015, 2:58 p.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27760/ > ----------------------------------------------------------- > > (Updated March 3, 2015, 2:58 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 > > > 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 d299f07 > 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 ce0e0b3 > src/master/master.cpp 53c8696 > src/tests/cram_md5_authentication_tests.cpp 92a89c5 > > Diff: https://reviews.apache.org/r/27760/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Till Toenshoff > >
