> On Feb. 20, 2015, 2: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. > ```
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. > On Feb. 20, 2015, 2: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? > > Till Toenshoff wrote: > 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. > ``` FTFY: "The inner future will be satisfied when the authentication process has completed and returned a result; however, it could return an Option None, indicating that authentication was denied. The outer future will be ready only if/when authentication has succeeded." Why did we go for an Option<string> rather than a Try<string>? Seems like Try would better reflect the difference between success/failure than some/none does. > On Feb. 20, 2015, 2: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; > } > ``` How about: "Tried to cancel authentication session for [pid], but it was already cancelled" and "Cancelling authentication session for PID"? - Adam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73310 ----------------------------------------------------------- On Feb. 19, 2015, 6:02 a.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27760/ > ----------------------------------------------------------- > > (Updated Feb. 19, 2015, 6:02 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 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 > >
