> On Sept. 26, 2013, 1:27 a.m., Benjamin Hindman wrote: > > include/mesos/scheduler.hpp, line 367 > > <https://reviews.apache.org/r/14292/diff/2/?file=357192#file357192line367> > > > > Any particular reason you added the space?
that was a mistake. fixed. > On Sept. 26, 2013, 1:27 a.m., Benjamin Hindman wrote: > > src/java/src/org/apache/mesos/MesosSchedulerDriver.java, line 127 > > <https://reviews.apache.org/r/14292/diff/2/?file=357199#file357199line127> > > > > 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. FWIW, that was the route I originally envisioned. But that is not going to work if we are working with old jar and new libs. AFAICT, GetField() would throw an exception if the field doesn't exist. From what I have read online, it seemed like an involved and potentially unsafe process to suppress that exception and move on. http://www.ibm.com/developerworks/library/j-jni/#exceptions Thoughts? > On Sept. 26, 2013, 1:27 a.m., Benjamin Hindman wrote: > > src/master/flags.hpp, line 99 > > <https://reviews.apache.org/r/14292/diff/2/?file=357200#file357200line99> > > > > s/auth/authenticate/ !? Pretty pretty please? :) > On Sept. 26, 2013, 1:27 a.m., Benjamin Hindman wrote: > > src/master/flags.hpp, line 110 > > <https://reviews.apache.org/r/14292/diff/2/?file=357200#file357200line110> > > > > Do we want to allow strings without the 'file://' too? I know we didn't > > do this for the whitelist but it seems nice. ;) Not just whitelist, but our "--zk" flag also expects a file path to start with "file://" :( To be consistent, and not change the already exposed semantics of zk, I would leave it as is. I'm ok with stripping "file://", if present, when I do sanity checking in master.cpp so as to "silently" allow paths without the prefix. Thoughts? > On Sept. 26, 2013, 1:27 a.m., Benjamin Hindman wrote: > > src/master/master.cpp, line 295 > > <https://reviews.apache.org/r/14292/diff/2/?file=357202#file357202line295> > > > > ... requires a credentials file (see --credentials flag)" done > On Sept. 26, 2013, 1:27 a.m., Benjamin Hindman wrote: > > src/master/master.cpp, lines 311-312 > > <https://reviews.apache.org/r/14292/diff/2/?file=357202#file357202line311> > > > > 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. Good point. Added a TODO. > On Sept. 26, 2013, 1:27 a.m., Benjamin Hindman wrote: > > src/master/master.cpp, lines 314-315 > > <https://reviews.apache.org/r/14292/diff/2/?file=357202#file357202line314> > > > > You could probably: > > > > foreach (const string& line, strings::tokenize(read.get(), "\n") { > > ...; > > } done > On Sept. 26, 2013, 1:27 a.m., Benjamin Hindman wrote: > > src/master/master.cpp, line 554 > > <https://reviews.apache.org/r/14292/diff/2/?file=357202#file357202line554> > > > > 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. ;) Thanks for catching this! Will add a test. P.S: If you can help me out my sending a review out for HostPort I will use it :) > On Sept. 26, 2013, 1:27 a.m., Benjamin Hindman wrote: > > src/master/master.cpp, line 1596 > > <https://reviews.apache.org/r/14292/diff/2/?file=357202#file357202line1596> > > > > Do we want to remove the authenticator from authenticating and delete > > it? it was being lazily deleted in authenticate() but i agree doing it here is good. fixed. > On Sept. 26, 2013, 1:27 a.m., Benjamin Hindman wrote: > > src/sched/sched.cpp, line 266 > > <https://reviews.apache.org/r/14292/diff/2/?file=357205#file357205line266> > > > > And maybe a retry attempts limit? sorry, that was what i meant. fixed. > On Sept. 26, 2013, 1:27 a.m., Benjamin Hindman wrote: > > src/sched/sched.cpp, line 93 > > <https://reviews.apache.org/r/14292/diff/2/?file=357205#file357205line93> > > > > Can we take an optional credential? > On Sept. 26, 2013, 1:27 a.m., Benjamin Hindman wrote: > > src/sched/sched.cpp, lines 283-290 > > <https://reviews.apache.org/r/14292/diff/2/?file=357205#file357205line283> > > > > 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? I will think about this once I integrate SASL. This was stop gap for the tests to pass before SASL integration. > On Sept. 26, 2013, 1:27 a.m., Benjamin Hindman wrote: > > include/mesos/scheduler.hpp, lines 331-334 > > <https://reviews.apache.org/r/14292/diff/2/?file=357192#file357192line331> > > > > 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. > On Sept. 26, 2013, 1:27 a.m., Benjamin Hindman wrote: > > src/master/master.cpp, line 1587 > > <https://reviews.apache.org/r/14292/diff/2/?file=357202#file357202line1587> > > > > 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. Added a TODO for now. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14292/#review26401 ----------------------------------------------------------- On Sept. 26, 2013, 1:28 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14292/ > ----------------------------------------------------------- > > (Updated Sept. 26, 2013, 1:28 a.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. > > > 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/allocator_tests.cpp c57da6eb3c431b47468b6a6941c3de06af9209e5 > src/tests/allocator_zookeeper_tests.cpp > 6e3214c15c14dc8ba82082738c172c8833cd0887 > src/tests/authentication_tests.cpp PRE-CREATION > src/tests/exception_tests.cpp 3fc1ac32d553644080a88f04f22077691ae1820b > src/tests/fault_tolerance_tests.cpp > 10e52c401476eb8416361de49b8e4061bb7ac4f3 > src/tests/gc_tests.cpp e404de3bfacfcac5515995f1b45c3d39181e138f > src/tests/isolator_tests.cpp cd3b3000060b379ef10e38a2a98a2eebe69d90fc > src/tests/master_detector_tests.cpp > 2d140ba1a364a7af4d643951d6016ac17dd10526 > src/tests/master_tests.cpp 52f09d4f1ddeabcc1a797a13fae9641b72425dd5 > src/tests/mesos.hpp 8fbd56c8dd438a08673b54630bfe25d60ad5ee0e > src/tests/mesos.cpp 776cb0f13d10b4ae437fe9a3c97dc8b3481290af > src/tests/resource_offers_tests.cpp > 3888e461de5c8fa807cff2fd2bd7ca12c704823a > src/tests/slave_recovery_tests.cpp 48b2e6380a9ae688291992f3bf25c3cc473bc808 > src/tests/status_update_manager_tests.cpp > cf420e4764356402f05b27c3b8e8802c21a58f8e > > Diff: https://reviews.apache.org/r/14292/diff/ > > > Testing > ------- > > make check > > All our tests do auth now. yay! > > Also added couple specific tests. Will likely add more after integrating with > SASL authenticator. > > > Thanks, > > Vinod Kone > >
