----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18381/#review40197 -----------------------------------------------------------
Looking great Adam. Lets split this into 3 reviews (1) credential read refactor 2) slave shutdown message and 3) slave auth) as mentioned in the comments. src/master/master.cpp <https://reviews.apache.org/r/18381/#comment73111> can you pull out this refactor into its own review? src/master/master.cpp <https://reviews.apache.org/r/18381/#comment73110> why the change? src/master/master.cpp <https://reviews.apache.org/r/18381/#comment73112> This message will likely be printed by the slave, so it is not terribly useful to include its id and 'from' in this message. Just say "Slave is not authenticated". src/master/master.cpp <https://reviews.apache.org/r/18381/#comment73113> ditto. src/master/master.cpp <https://reviews.apache.org/r/18381/#comment73114> Do we really need an AuthenticateMessage::Type? Why not just go through 'frameworks' and if pid not found go through 'slaves'? Similar to what we do in Master::exited(). src/master/master.cpp <https://reviews.apache.org/r/18381/#comment73115> we should really consolidate 'deactivate' framework and 'disconnect' slave. They are essentially doing similar things to frameworks and slaves. maybe a TODO for now if you don't want to tackle that first. src/master/master.cpp <https://reviews.apache.org/r/18381/#comment73116> shouldn't this also be done in Master::disconnect(Slave*) similar to how we do it in Master::deactivate(Framework*) ? src/messages/messages.proto <https://reviews.apache.org/r/18381/#comment73117> Lets pull this change out in its own review. src/messages/messages.proto <https://reviews.apache.org/r/18381/#comment73125> s/error/message/ src/messages/messages.proto <https://reviews.apache.org/r/18381/#comment73118> Revert this change. I don't think its needed per my comments above. src/sasl/authenticatee.hpp <https://reviews.apache.org/r/18381/#comment73119> why the change? src/sasl/common.hpp <https://reviews.apache.org/r/18381/#comment73120> we don't do this in header files. src/sasl/common.hpp <https://reviews.apache.org/r/18381/#comment73122> If you kill the below function (see comments below) you can merge this into the read function above. Maybe you can also fold parse() method above into read()? src/sasl/common.hpp <https://reviews.apache.org/r/18381/#comment73121> Kill this. Just use readCredentials and let the caller check the returned vector is of size 1. src/slave/slave.hpp <https://reviews.apache.org/r/18381/#comment73123> should these be public? src/slave/slave.hpp <https://reviews.apache.org/r/18381/#comment73124> s/Future/process::Future/ src/slave/slave.cpp <https://reviews.apache.org/r/18381/#comment73126> s/error/message/ What happens if the slaves are upgraded before the master? src/slave/slave.cpp <https://reviews.apache.org/r/18381/#comment73129> kill this and include the message in the log message below. src/slave/slave.cpp <https://reviews.apache.org/r/18381/#comment73127> new line. src/slave/slave.cpp <https://reviews.apache.org/r/18381/#comment73130> I know this is copy pasted from sched.cpp but can this be CHECK_NOTNULL(authenticatee)? src/slave/slave.cpp <https://reviews.apache.org/r/18381/#comment73131> s/master.get()/self()/ src/slave/slave.cpp <https://reviews.apache.org/r/18381/#comment73132> How about LOG(WARNING) << ... << ... src/tests/slave_recovery_tests.cpp <https://reviews.apache.org/r/18381/#comment73133> new line. - Vinod Kone On April 3, 2014, 4:44 a.m., Adam B wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18381/ > ----------------------------------------------------------- > > (Updated April 3, 2014, 4:44 a.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 Questions: > - Should AuthenticateMessage be replaced with AuthenticateFrameworkMessage, > or specify an Authenticatee type as coded here? > - 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 > ----- > > include/mesos/mesos.proto 37f8a7f > src/master/flags.hpp 024f86d > src/master/master.hpp b6b9983 > src/master/master.cpp 5d0ddb0 > src/messages/messages.proto bba17a9 > src/sasl/authenticatee.hpp 42a4eba > src/sasl/common.hpp PRE-CREATION > src/sched/sched.cpp 3684cfe > src/slave/flags.hpp d5c54c0 > src/slave/slave.hpp 15e23ce > src/slave/slave.cpp 6d901dc > src/tests/authentication_tests.cpp 127c5e6 > src/tests/cluster.hpp 11684d9 > src/tests/mesos.cpp ae3aeee > src/tests/sasl_tests.cpp 945426d > src/tests/slave_recovery_tests.cpp 72b6d42 > > 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 > >
