> On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: > >
Thanks a bunch for your thorrough review Vinod - much appreciated. > 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? That would render our tests broken (those that reset the credentials). The concurrency is covered by a Lock within that SASL aux plugin code. > On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: > > src/authentication/cram_md5/auxprop.hpp, line 100 > > <https://reviews.apache.org/r/27760/diff/17/?file=869279#file869279line100> > > > > if you protect secrets::load() by Once, as suggested above, do we need > > this? See above > On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: > > src/authentication/cram_md5/authenticator.hpp, line 254 > > <https://reviews.apache.org/r/27760/diff/17/?file=869277#file869277line254> > > > > Do we need this in the header? Can it be moved to cpp? Good catch, artefact from recent refactoring. Thank you! > On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: > > src/authentication/cram_md5/authenticator.cpp, lines 54-59 > > <https://reviews.apache.org/r/27760/diff/17/?file=869278#file869278line54> > > > > How about: > > > > ``` > > sessions.put(pid, session); > > > > return session->authenticate() > > .onAny(defer(..)); > > > > ``` Nice, yes! > On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: > > src/authentication/cram_md5/authenticator.cpp, line 436 > > <https://reviews.apache.org/r/27760/diff/17/?file=869278#file869278line436> > > > > What do you mean by "within our Once covered case"? > > > > Also, just put "subsequent" on the next line. Rephreased that one to: ``` // The 'error' is set, if at all, only once per (os-) process. // To allow subsequent calls to return the possibly set Error // object, we make this a static pointer. ``` > On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: > > src/master/master.hpp, line 655 > > <https://reviews.apache.org/r/27760/diff/17/?file=869281#file869281line655> > > > > make this Option<Authenticator*> Neat, much nicer. > On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: > > src/authentication/cram_md5/authenticator.cpp, line 70 > > <https://reviews.apache.org/r/27760/diff/17/?file=869278#file869278line70> > > > > Can you add a comment on when this is possible? It shouldn't :) .... replaced by ``` CHECK(sessions.contains(pid)); ``` > 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? 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; } ``` > 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? 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. ``` > On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 3870-3873 > > <https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3870> > > > > Did you mix up "inner" and "outer" here? I rephrased it, maybe this is better? ``` // The inner future may get satisfied by an empty option, hinting // that the authentication has gotten refused. // The outer future will be ready only if we have entirely // authenticated successfully. ``` > 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()? `__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. - Till ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73310 ----------------------------------------------------------- On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27760/ > ----------------------------------------------------------- > > (Updated Feb. 19, 2015, 2:02 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 d372404 > src/authentication/cram_md5/authenticator.hpp 7578ea1 > src/authentication/cram_md5/authenticator.cpp PRE-CREATION > src/authentication/cram_md5/auxprop.hpp d036b11 > src/authentication/cram_md5/auxprop.cpp 5ff9755 > src/master/master.hpp 6a39df0 > src/master/master.cpp f10a3cf > src/tests/cram_md5_authentication_tests.cpp dd102dc > > Diff: https://reviews.apache.org/r/27760/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Till Toenshoff > >
