----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14292/#review26401 -----------------------------------------------------------
include/mesos/scheduler.hpp <https://reviews.apache.org/r/14292/#comment51563> I'd prefer an entirely separate constructor so we can differentiate when a user gave us a malformed credential versus when they gave us nothing. include/mesos/scheduler.hpp <https://reviews.apache.org/r/14292/#comment51564> Any particular reason you added the space? src/java/src/org/apache/mesos/MesosSchedulerDriver.java <https://reviews.apache.org/r/14292/#comment51565> I'd prefer to make credential "optional", which means setting it to null for now. src/java/src/org/apache/mesos/MesosSchedulerDriver.java <https://reviews.apache.org/r/14292/#comment51566> I think we can have one initialize and check if 'credential' is null and if so create a C++ MesosSchedulerDriver that doesn't pass a credential otherwise creating one that does pass a credential. src/master/flags.hpp <https://reviews.apache.org/r/14292/#comment51567> s/auth/authenticate/ !? Pretty pretty please? src/master/flags.hpp <https://reviews.apache.org/r/14292/#comment51568> Do we want to allow strings without the 'file://' too? I know we didn't do this for the whitelist but it seems nice. ;) src/master/master.cpp <https://reviews.apache.org/r/14292/#comment51569> ... requires a credentials file (see --credentials flag)" src/master/master.cpp <https://reviews.apache.org/r/14292/#comment51570> Awesome warning! I was thinking it would be cool to check the readability/writability of the credentials file too and print a warning if it's not very secure! ;) Maybe just add a TODO. src/master/master.cpp <https://reviews.apache.org/r/14292/#comment51571> You could probably: foreach (const string& line, strings::tokenize(read.get(), "\n") { ...; } src/master/master.cpp <https://reviews.apache.org/r/14292/#comment51572> authenticated.erase(HostPort(pid)); Seems like we should have a test which makes sure that an authenticated framework that fails over and tries to (re)register with bad (or missing) credentials can't (re)register! Or maybe this motivates promoting the host/port string to the HostPort struct sooner than later. ;) src/master/master.cpp <https://reviews.apache.org/r/14292/#comment51576> Do we want to do a delay too? I can imagine delay(timeout, self(), &Self::_authenticate, ...)); and then just checking if (future.isPending()) in _authenticate will imply that a timeout has occurred. src/master/master.cpp <https://reviews.apache.org/r/14292/#comment51574> This function has some incorrect indentation. src/master/master.cpp <https://reviews.apache.org/r/14292/#comment51573> Do we want to remove the authenticator from authenticating and delete it? src/master/master.cpp <https://reviews.apache.org/r/14292/#comment51575> Likewise here ... do we want to remove the authenticator from authenticating and delete it? src/sched/sched.cpp <https://reviews.apache.org/r/14292/#comment51577> Can we take an optional credential? src/sched/sched.cpp <https://reviews.apache.org/r/14292/#comment51578> What about delaying to _authenticate and checking if the future is pending? src/sched/sched.cpp <https://reviews.apache.org/r/14292/#comment51580> This function seems to have some weird indentation going on again. src/sched/sched.cpp <https://reviews.apache.org/r/14292/#comment51579> And maybe a retry attempts limit? src/sched/sched.cpp <https://reviews.apache.org/r/14292/#comment51581> Hmm, not sure what solutions you have in mind. One solution on the master side would be to defer a Master::registerFramework via 'then' on the future from the Authenticator.authenticate() ... other thoughts? src/sched/sched.cpp <https://reviews.apache.org/r/14292/#comment51582> I think we can make this optional! That way we only try and authenticate if someone gave us credentials. If the master requires authentication we'll just get a FrameworkErrorMessage. - Benjamin Hindman On Sept. 25, 2013, 10 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14292/ > ----------------------------------------------------------- > > (Updated Sept. 25, 2013, 10 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Bugs: MESOS-704 > https://issues.apache.org/jira/browse/MESOS-704 > > > Repository: mesos-git > > > Description > ------- > > Added authentication support for scheduler driver and master. > > TODO > --> Tests. > > > Diffs > ----- > > include/mesos/mesos.proto 8f845cc9b7cd491622cb29a69f6909170510a664 > include/mesos/scheduler.hpp cf3ecdaaf40fd878a80fe0b6f7e61a0997329cbd > src/Makefile.am 3eae9642b639e78f101e7fe580aa803beadbebda > src/authentication/authenticatee.hpp PRE-CREATION > src/authentication/authenticator.hpp PRE-CREATION > src/common/type_utils.hpp 674a8820c339c6446dfa7d444457477ab4512e79 > src/java/jni/construct.cpp b01bd7ae2eda2dc5e0dcd68848c65bd9f9ea81f0 > src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp > 6d2a03b6a88e71ac4e2e2d1ee8e15925e393ef3d > src/java/src/org/apache/mesos/MesosSchedulerDriver.java > 7ef1fe7755286bf92b94d7ece4f72d54e5b57a84 > src/master/flags.hpp d59e67d5b2799d6d7a37e9cfe7246ae7372091ac > src/master/master.hpp bd5cb1ff05967518d7dc7f3a9dc0d32c22eb9643 > src/master/master.cpp a49b17ef43fca5b385a89731ca8776a26b61399a > src/messages/messages.proto 4d400c2d8f6ecb9f7cf8635b2a98341206bbb6c5 > src/python/native/mesos_scheduler_driver_impl.cpp > f25d41d38caf2701813dbec0d342a3b327e9dedf > src/sched/sched.cpp c399f2481259683a8e178abb3478307042292f23 > src/tests/fault_tolerance_tests.cpp > 10e52c401476eb8416361de49b8e4061bb7ac4f3 > > Diff: https://reviews.apache.org/r/14292/diff/ > > > Testing > ------- > > make check > > Will update this review with auth specific tests. > > > Thanks, > > Vinod Kone > >
