----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22222/#review46508 -----------------------------------------------------------
Not sure I like the combined JSON format, rather than multiple files with plain-text formatting. I'm just thinking what if somebody wants authentication for registration/http but not the other, or wants principal/secret auth for one, but principal-only (implicit token, a la kerberos/OAuth) for the other. I guess 'secret' is optional, so you could still combine them and just ignore the 'secret' sometimes, but then we'll need another differentiator to specify what kind of authentication(s) to do. include/mesos/mesos.proto <https://reviews.apache.org/r/22222/#comment81916> (appellation) include/mesos/mesos.proto <https://reviews.apache.org/r/22222/#comment81917> "registration"? Since these credentials are currently used by the slave/framework to allow registration with the master? src/credentials/credentials.hpp <https://reviews.apache.org/r/22222/#comment81921> To quote Vinod's comment on my original RR (r/20301): "with the name space change you can s/readCredentials/read/" since credentials::readCredentials() sounds redundant. src/credentials/credentials.hpp <https://reviews.apache.org/r/22222/#comment81918> So now credentials should be stored in JSON format, with entries for identification/registration, http, and whatever else comes along? This could get messy quick. How many more credentials types do we expect? What if somebody wants to use kerberos for registration, but flatfile for http? Or OAuth for http and flatfile for registration? src/credentials/credentials.hpp <https://reviews.apache.org/r/22222/#comment81919> So, if the credentials file is valid JSON, but not a valid Credentials message, you will fall through and treat it like a plain-text file? src/credentials/credentials.hpp <https://reviews.apache.org/r/22222/#comment81920> I think we usually prefer the '+' at the end of the line above, rather than starting the next line. src/credentials/credentials.hpp <https://reviews.apache.org/r/22222/#comment81922> Why create a separate readCredential() method as well? That's what I had in my original r/18381 before Vinod suggested I "Just use readCredentials and let the caller check the returned vector is of size 1." src/master/flags.hpp <https://reviews.apache.org/r/22222/#comment81923> If this is all supposed to be one sentence (Either ... or ...), then get rid of the '.'s and remove the capitalization. src/master/flags.hpp <https://reviews.apache.org/r/22222/#comment81924> "Or a path to a JSON-formatted file containing credentials for identification/registration and http authentication." src/master/flags.hpp <https://reviews.apache.org/r/22222/#comment81925> Perhaps we also need an example for the plain-text file now? src/slave/flags.hpp <https://reviews.apache.org/r/22222/#comment81927> Weird wrapping. src/slave/flags.hpp <https://reviews.apache.org/r/22222/#comment81928> So verbose. src/tests/authentication_tests.cpp <https://reviews.apache.org/r/22222/#comment81929> Why CreateMasterFlags now, if you're not changing them? src/tests/credentials_tests.cpp <https://reviews.apache.org/r/22222/#comment81934> StartMaster() will call CreateMasterFlags() for you. src/tests/credentials_tests.cpp <https://reviews.apache.org/r/22222/#comment81935> Ditto src/tests/mesos.cpp <https://reviews.apache.org/r/22222/#comment81931> ? What does this comment mean? src/tests/mesos.cpp <https://reviews.apache.org/r/22222/#comment81932> Why does this need to be turned JSON? src/tests/mesos.cpp <https://reviews.apache.org/r/22222/#comment81933> s/credentials/slave credential/ - Adam B On June 20, 2014, 11:08 a.m., Isabel Jimenez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22222/ > ----------------------------------------------------------- > > (Updated June 20, 2014, 11:08 a.m.) > > > Review request for mesos, Benjamin Hindman, Dominic Hamon, and Till Toenshoff. > > > Bugs: MESOS-1391 > https://issues.apache.org/jira/browse/MESOS-1391 > > > Repository: mesos-git > > > Description > ------- > > After Ben's comments, regroup of all types of authentication in one common > definition, this is a draft for this Issue, here are some questions: > > - Do I replace credentials flag completely or if this slight duplication will > suffice for now ? > - If I don't replace it, for now as it is in this patch, the flag is useless, > I should give it to the sasl/authenticator but I would really appreciate some > comments for that part. > > > Diffs > ----- > > include/mesos/mesos.proto 2f6be05 > src/Makefile.am b1b7d2d > src/credentials/credentials.hpp 98b9088 > src/master/flags.hpp 47bb0dc > src/master/master.hpp b56e9f4 > src/master/master.cpp dcf28ad > src/sasl/authenticator.hpp 365db5f > src/slave/flags.hpp 3b8ba08 > src/slave/slave.cpp ed3483f > src/tests/authentication_tests.cpp 5cf2da4 > src/tests/credentials_tests.cpp PRE-CREATION > src/tests/mesos.cpp 1037420 > > Diff: https://reviews.apache.org/r/22222/diff/ > > > Testing > ------- > > > Thanks, > > Isabel Jimenez > >
