-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14292/#review26506
-----------------------------------------------------------
Ship it!
src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp
<https://reviews.apache.org/r/14292/#comment51685>
s/2//
src/java/src/org/apache/mesos/MesosSchedulerDriver.java
<https://reviews.apache.org/r/14292/#comment51686>
Put 'Scheduler scheduler,' on newline. Maybe reformat other constructor parameters too?
src/master/flags.hpp
<https://reviews.apache.org/r/14292/#comment51692>
Update this description based on what the actual code does?
src/master/master.hpp
<https://reviews.apache.org/r/14292/#comment51687>
s/ class/class/
src/master/master.hpp
<https://reviews.apache.org/r/14292/#comment51688>
Newline please.
src/master/master.hpp
<https://reviews.apache.org/r/14292/#comment51689>
Maybe a comment explaining that these are the credentials we accept for authentication?
src/master/master.hpp
<https://reviews.apache.org/r/14292/#comment51690>
Awesome comment!
src/master/master.hpp
<https://reviews.apache.org/r/14292/#comment51691>
Maybe a 'see above' comment for rationale for HostPort?
src/master/master.cpp
<https://reviews.apache.org/r/14292/#comment51693>
Maybe update the description in flags to note that you don't need the 'file://' prefix?
src/master/master.cpp
<https://reviews.apache.org/r/14292/#comment51695>
s/allows/allowing/
src/master/master.cpp
<https://reviews.apache.org/r/14292/#comment51694>
s/allows/allowing/ and s/ too//
src/master/master.cpp
<https://reviews.apache.org/r/14292/#comment51696>
s/file' "/file '"/
src/master/master.cpp
<https://reviews.apache.org/r/14292/#comment51697>
Maybe a LOG(WARNING) too?
src/master/master.cpp
<https://reviews.apache.org/r/14292/#comment51698>
LOG(WARNING) here too?
src/master/master.cpp
<https://reviews.apache.org/r/14292/#comment51699>
Maybe add a little more to the comment that explains how we're sure that a framework will
definitely attempt to re-authenticate before we consider it active again?
src/master/master.cpp
<https://reviews.apache.org/r/14292/#comment51700>
+1
src/master/master.cpp
<https://reviews.apache.org/r/14292/#comment51701>
+1 to only retrying from the scheduler. This will also simplify this code!
src/sasl/authenticator.hpp
<https://reviews.apache.org/r/14292/#comment51704>
s/user/principal/
src/sasl/authenticator.hpp
<https://reviews.apache.org/r/14292/#comment51702>
&
src/sasl/authenticator.hpp
<https://reviews.apache.org/r/14292/#comment51703>
Alternatively:
std::map<std::string, std::string> secrets;
foreach (const Credential& credential, credentials) {
secrets[credential.principal()] = credential.secret();
}
load(secrets);
Or just kill the other load altogether (but fix up the tests)!
src/sched/sched.cpp
<https://reviews.apache.org/r/14292/#comment51705>
Yes, I agree we should abort! In previous conversations I thought you meant abort the
process (not the scheduler driver)! Do you want to also invoke the 'error' callback with a
message? We should really add an 'abort' which takes an error message. ;)
src/sched/sched.cpp
<https://reviews.apache.org/r/14292/#comment51706>
+1
src/tests/authentication_tests.cpp
<https://reviews.apache.org/r/14292/#comment51707>
s/wit/with/
src/tests/authentication_tests.cpp
<https://reviews.apache.org/r/14292/#comment51708>
s/wit/with/ s/without/with/
- Benjamin Hindman
On Sept. 30, 2013, 2:46 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14292/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2013, 2:46 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 957576bbc1c73513a9591194d017f76fe562a616
> include/mesos/scheduler.hpp cf3ecdaaf40fd878a80fe0b6f7e61a0997329cbd
> src/Makefile.am ee336130ad93d8b524c841f75be36f00d4a2b147
> 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/python/native/mesos_scheduler_driver_impl.cpp f25d41d38caf2701813dbec0d342a3b327e9dedf
> src/sasl/authenticator.hpp 2f78cf0fdd97f0ddc3a6ebd162e6559497d708e4
> 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
> -------
>
> OSX:
> make check
> ./bin/mesos-tests.sh --gtest_filter="*FaultTolerance*" --gtest_repeat=1000 --gtest_break_on_failure
--gtest_shuffle
>
> Linux:
> make check GTEST_FILTER=''
>
> Couldn't run tests on linux because the boxes I've access to do not support SASL :/ It
all compiles fwiw.
>
> All our tests do auth now. yay!
>
>
> Thanks,
>
> Vinod Kone
>
>
|