----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18381/#review36419 -----------------------------------------------------------
src/master/flags.hpp <https://reviews.apache.org/r/18381/#comment67352> have defaults as "false"? simply do a validation in master that they don't conflict. src/master/master.hpp <https://reviews.apache.org/r/18381/#comment67397> Since slaves and frameworks have different pids why do we want different hashmaps for frameworks and slaves? src/master/master.cpp <https://reviews.apache.org/r/18381/#comment67375> New line. Also, this file doesn't use multi-line style comments. Can yo just use "//"? src/master/master.cpp <https://reviews.apache.org/r/18381/#comment67399> Hmm. This is getting complicated. Can you actually not do the flag deprecation stuff in this review. Lets do that in a subsequent review. IOW, keep "authenticate" flag as is and simply add "authenticate_slaves" flag. You can add a TODO for the rename. src/master/master.cpp <https://reviews.apache.org/r/18381/#comment67386> s/frameworks but not slaves/ ? src/master/master.cpp <https://reviews.apache.org/r/18381/#comment67387> s/slaves/slaves but not frameworks/ ? src/master/master.cpp <https://reviews.apache.org/r/18381/#comment67382> s/Nothing/ No frameworks or slaves/ src/master/master.cpp <https://reviews.apache.org/r/18381/#comment67398> Love this cleanup. Can you do this (deactivate(Slave*)) in its own review? Also, we already have a deactivateSlave() that is called by the SlaveObserver. We need to reconcile these two. src/master/master.cpp <https://reviews.apache.org/r/18381/#comment67400> Thank you! src/master/master.cpp <https://reviews.apache.org/r/18381/#comment67402> indentation. More importantly can we just leverage the SlaveShutdown message? src/master/master.cpp <https://reviews.apache.org/r/18381/#comment67404> indentation. src/master/master.cpp <https://reviews.apache.org/r/18381/#comment67410> Good point. If someone can spoof the "pid" in the authenticate message then it might deactivate a legitimate framework. Please create a new ticket so that we can discuss possible solutions. src/master/master.cpp <https://reviews.apache.org/r/18381/#comment67413> Why did you kill the "erase" here? src/master/master.cpp <https://reviews.apache.org/r/18381/#comment67412> put each arg in a different line if they don't fit in one. src/master/master.cpp <https://reviews.apache.org/r/18381/#comment67420> There's a lot of code duplication here. I'm not convinced that differentiating framework/slave authentication has any benefits. Either way we need to figure out to factor out some of this common code. src/messages/messages.proto <https://reviews.apache.org/r/18381/#comment67418> Can we make this optional? It will decouple scheduler and master deploys. src/sasl/authenticatee.hpp <https://reviews.apache.org/r/18381/#comment67421> Why this refactoring? src/slave/slave.cpp <https://reviews.apache.org/r/18381/#comment67426> Both master and slave use this. We should really refactor this into a common function that both can use. We could probably stick it in sasl/common.hpp? src/slave/slave.cpp <https://reviews.apache.org/r/18381/#comment67424> How about "Not expecting multiple credentials" ? src/slave/slave.cpp <https://reviews.apache.org/r/18381/#comment67428> formatting. src/slave/slave.cpp <https://reviews.apache.org/r/18381/#comment67430> formatting. src/slave/slave.cpp <https://reviews.apache.org/r/18381/#comment67429> new line. src/slave/slave.cpp <https://reviews.apache.org/r/18381/#comment67431> new line. src/slave/slave.cpp <https://reviews.apache.org/r/18381/#comment67432> new line. src/slave/slave.cpp <https://reviews.apache.org/r/18381/#comment67433> new line. src/slave/slave.cpp <https://reviews.apache.org/r/18381/#comment67434> Instead of a new message how about adding an "optional string message" to the shutdown method? src/slave/slave.cpp <https://reviews.apache.org/r/18381/#comment67435> thank you :) src/tests/examples_tests.cpp <https://reviews.apache.org/r/18381/#comment67436> Why is this disabled? Point to a JIRA. src/tests/slave_recovery_tests.cpp <https://reviews.apache.org/r/18381/#comment67438> Why is this disabled? - Vinod Kone On March 5, 2014, 7:12 p.m., Adam B wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18381/ > ----------------------------------------------------------- > > (Updated March 5, 2014, 7:12 p.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-804 > https://issues.apache.org/jira/browse/MESOS-804 > > > Repository: mesos-git > > > Description > ------- > > Added authentication support for slaves. > Fixes MESOS-804. > > Open Issues: > - Should AuthenticateMessage be replaced with AuthenticateFrameworkMessage, > or specify an Authenticatee type as coded here? > - removeSlave vs. deactivate(Slave): Some uses of removeSlave might benefit > from just deactivating if checkpointing is enabled. > - We currently deactivate a registered slave/framework when a new > authenticate message comes in, even if the new authentication message is a > failure/fake. Will file a new JIRA for this security hole. > - When multiple entries for the same principal exist in the credentials file, > only the last entry is used. Acceptable behavior, but shouldn't this be > documented? > > > Diffs > ----- > > src/master/flags.hpp 159b2de > src/master/master.hpp 49a3e15 > src/master/master.cpp f7ba9aa > src/messages/messages.proto c26a3d0 > src/sasl/authenticatee.hpp 42a4eba > src/sched/sched.cpp 00f6307 > src/slave/flags.hpp e4d98a5 > src/slave/slave.hpp 01b80df > src/slave/slave.cpp b350df4 > src/tests/authentication_tests.cpp 127c5e6 > src/tests/examples_tests.cpp 34f0233 > src/tests/mesos.cpp 96adeac > src/tests/sasl_tests.cpp 945426d > src/tests/slave_recovery_tests.cpp 40a9599 > > Diff: https://reviews.apache.org/r/18381/diff/ > > > Testing > ------- > > make check; manually tested flatfile slave authentication success/failure. > Added new slave authentication unit tests in authentication_tests.cpp. > > > Thanks, > > Adam B > >
