You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Isabel Jimenez <co...@isabeljimenez.com> on 2014/06/04 01:15:02 UTC
Review Request 22222: HTTP authentication flag for basic/digest HTTP
authentication
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22222/
-----------------------------------------------------------
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
-----
src/authentications/authentications.hpp PRE-CREATION
src/master/flags.hpp 4863359
src/master/master.cpp 766a0e3
src/messages/messages.proto 6f6e570
Diff: https://reviews.apache.org/r/22222/diff/
Testing
-------
Thanks,
Isabel Jimenez
Re: Review Request 22222: HTTP authentication flag for basic/digest HTTP
authentication
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22222/#review44821
-----------------------------------------------------------
Bad patch!
Reviews applied: [22222]
Failed command: make -j3 distcheck GTEST_FILTER='' >/dev/null
Error:
configure: WARNING: can not find python-boto
-------------------------------------------------------------------
mesos-ec2 services will not function.
-------------------------------------------------------------------
ev.c:1531:31: warning: 'ev_default_loop_ptr' initialized and declared 'extern' [enabled by default]
ev.c: In function 'evpipe_write':
ev.c:2160:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2172:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c: In function 'pipecb':
ev.c:2193:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2207:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
In file included from /usr/include/c++/4.6/ext/hash_set:61:0,
from src/glog/stl_logging.h:54,
from src/stl_logging_unittest.cc:34:
/usr/include/c++/4.6/backward/backward_warning.h:33:2: warning: #warning This file includes at least one deprecated or antiquated header which may be removed without further notice at a future date. Please use a non-deprecated interface with equivalent functionality instead. For a listing of replacement headers and interfaces, consult the file backward_warning.h. To disable this warning use -Wno-deprecated. [-Wcpp]
In file included from src/utilities.h:73:0,
from src/googletest.h:38,
from src/stl_logging_unittest.cc:48:
src/base/mutex.h:137:0: warning: "_XOPEN_SOURCE" redefined [enabled by default]
/usr/include/features.h:166:0: note: this is the location of the previous definition
warning: no files found matching 'Makefile' under directory 'docs'
warning: no files found matching 'indexsidebar.html' under directory 'docs'
ar: creating libleveldb.a
zip_safe flag not set; analyzing archive contents...
../../src/master/master.cpp:56:47: fatal error: authentications/authentications.hpp: No such file or directory
compilation terminated.
make[3]: *** [master/libmesos_no_3rdparty_la-master.lo] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [all] Error 2
make[1]: *** [all-recursive] Error 1
make: *** [distcheck] Error 1
- Mesos ReviewBot
On June 3, 2014, 11:15 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22222/
> -----------------------------------------------------------
>
> (Updated June 3, 2014, 11:15 p.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
> -----
>
> src/authentications/authentications.hpp PRE-CREATION
> src/master/flags.hpp 4863359
> src/master/master.cpp 766a0e3
> src/messages/messages.proto 6f6e570
>
> Diff: https://reviews.apache.org/r/22222/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 22222: HTTP authentication flag for basic/digest HTTP
authentication
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22222/#review46041
-----------------------------------------------------------
Patch looks great!
Reviews applied: [22222]
All tests passed.
- Mesos ReviewBot
On June 17, 2014, 10:31 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22222/
> -----------------------------------------------------------
>
> (Updated June 17, 2014, 10:31 p.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 709b8b1
> src/Makefile.am 3e623cc
> src/credentials/credentials.hpp 98b9088
> src/master/flags.hpp 780e219
> src/master/master.hpp 7a12185
> src/master/master.cpp 4a01b1a
> src/sasl/authenticator.hpp 365db5f
> src/slave/flags.hpp 3b8ba08
> src/slave/slave.hpp 3227e73
> src/slave/slave.cpp bc976b7
> src/tests/cluster.hpp 1c96ee7
> src/tests/credentials_tests.cpp PRE-CREATION
> src/tests/mesos.hpp 0b9b2f9
> src/tests/mesos.cpp 98a7c38
>
> Diff: https://reviews.apache.org/r/22222/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 22222: HTTP authentication flag for basic/digest HTTP
authentication
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On June 24, 2014, 9:58 a.m., Adam B wrote:
> > src/credentials/credentials.hpp, line 34
> > <https://reviews.apache.org/r/22222/diff/3/?file=614273#file614273line34>
> >
> > 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.
Thanks for the comments, I was trying to differentiate read for one credential from multiple. I kept just read for multiple credentials read.
> On June 24, 2014, 9:58 a.m., Adam B wrote:
> > src/credentials/credentials.hpp, lines 58-63
> > <https://reviews.apache.org/r/22222/diff/3/?file=614273#file614273line58>
> >
> > 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?
Yes and in that case an error will also be returned since it's not a valid text file either.
> On June 24, 2014, 9:58 a.m., Adam B wrote:
> > src/credentials/credentials.hpp, line 82
> > <https://reviews.apache.org/r/22222/diff/3/?file=614273#file614273line82>
> >
> > 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."
I think it is cleaner to make a read a single credential function, we gain taking away that size check.
> On June 24, 2014, 9:58 a.m., Adam B wrote:
> > src/tests/mesos.cpp, line 146
> > <https://reviews.apache.org/r/22222/diff/3/?file=614282#file614282line146>
> >
> > Why does this need to be turned JSON?
Ben make a good comment about making things easier if instead of a string miming the json format we just built it from the original message.
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22222/#review46508
-----------------------------------------------------------
On June 26, 2014, 5:50 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22222/
> -----------------------------------------------------------
>
> (Updated June 26, 2014, 5:50 p.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/credentials_tests.cpp PRE-CREATION
> src/tests/mesos.cpp 1037420
>
> Diff: https://reviews.apache.org/r/22222/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 22222: HTTP authentication flag for basic/digest HTTP
authentication
Posted by Adam B <ad...@mesosphere.io>.
> On June 24, 2014, 2:58 a.m., Adam B wrote:
> > 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.
>
> Benjamin Hindman wrote:
> The plan was to ultimately get away from plain-text formatting. While simpler, it's very implicit, hence the use of JSON to explicitly capture the structure.
>
> I agree that we'll need another differentiator for something like Keberos. The plan was to add more stuff to Credential, for example, one idea is to add an 'optional bool kerberos' which if set to true assumes that there is an implicit token that should be looked up.
Sounds reasonable to me. Just wanted to open up the discussion here and make sure we're not coding ourselves into a corner. The JSON format should be flexible enough to support multiple authentication types (even ones without explicit secrets), while being structured enough to explicitly capture the credential configurations better than plain-text.
> On June 24, 2014, 2:58 a.m., Adam B wrote:
> > include/mesos/mesos.proto, line 671
> > <https://reviews.apache.org/r/22222/diff/3/?file=614271#file614271line671>
> >
> > "registration"? Since these credentials are currently used by the slave/framework to allow registration with the master?
>
> Benjamin Hindman wrote:
> SGTM. The other suggestion was to call them 'sasl', since these are the credentials that we're giving to the SASL Authenticator.
I think I prefer naming the credentials after their intended purpose (registration, http, zookeeper, etc.) rather than the authentication mechanism used, since sasl could potentially be used for multiple credential purposes, but a master/slave will only provide a single set of credentials for a particular purpose.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22222/#review46508
-----------------------------------------------------------
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
>
>
Re: Review Request 22222: HTTP authentication flag for basic/digest HTTP
authentication
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On June 24, 2014, 9:58 a.m., Adam B wrote:
> > 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.
The plan was to ultimately get away from plain-text formatting. While simpler, it's very implicit, hence the use of JSON to explicitly capture the structure.
I agree that we'll need another differentiator for something like Keberos. The plan was to add more stuff to Credential, for example, one idea is to add an 'optional bool kerberos' which if set to true assumes that there is an implicit token that should be looked up.
> On June 24, 2014, 9:58 a.m., Adam B wrote:
> > include/mesos/mesos.proto, line 671
> > <https://reviews.apache.org/r/22222/diff/3/?file=614271#file614271line671>
> >
> > "registration"? Since these credentials are currently used by the slave/framework to allow registration with the master?
SGTM. The other suggestion was to call them 'sasl', since these are the credentials that we're giving to the SASL Authenticator.
> On June 24, 2014, 9:58 a.m., Adam B wrote:
> > src/credentials/credentials.hpp, lines 56-57
> > <https://reviews.apache.org/r/22222/diff/3/?file=614273#file614273line56>
> >
> > 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?
As mentioned above, I don't think we should use flat files. But mixing and matching Kerberos vs explicit secret is likely a valid use case. Do you have a suggestion for storing all these credentials? Just like we're sticking all the ACLs in a single JSON, the idea was to do the same here. But suggestions are welcome!
> On June 24, 2014, 9:58 a.m., Adam B wrote:
> > src/slave/flags.hpp, lines 252-255
> > <https://reviews.apache.org/r/22222/diff/3/?file=614278#file614278line252>
> >
> > So verbose.
But so explicit!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22222/#review46508
-----------------------------------------------------------
On June 20, 2014, 6:08 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22222/
> -----------------------------------------------------------
>
> (Updated June 20, 2014, 6:08 p.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
>
>
Re: Review Request 22222: HTTP authentication flag for basic/digest HTTP
authentication
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 22222: HTTP authentication flag for basic/digest HTTP
authentication
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22222/#review46952
-----------------------------------------------------------
Ship it!
I cleaned up a few style nits and the remaining issues and committed this, thanks Isabel! Please mark the issues as resolved.
- Benjamin Hindman
On June 26, 2014, 5:50 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22222/
> -----------------------------------------------------------
>
> (Updated June 26, 2014, 5:50 p.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/credentials_tests.cpp PRE-CREATION
> src/tests/mesos.cpp 1037420
>
> Diff: https://reviews.apache.org/r/22222/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 22222: HTTP authentication flag for basic/digest HTTP
authentication
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On June 27, 2014, 4:08 a.m., Dominic Hamon wrote:
> > src/master/master.hpp, line 373
> > <https://reviews.apache.org/r/22222/diff/4/?file=618541#file618541line373>
> >
> > i might be missing something, but why are these being stored in the master?
Yup, removed this for Isabel when I committed.
> On June 27, 2014, 4:08 a.m., Dominic Hamon wrote:
> > src/sasl/authenticator.hpp, line 476
> > <https://reviews.apache.org/r/22222/diff/4/?file=618543#file618543line476>
> >
> > should you check that a principal doesn't appear more than once?
These are the same semantics as before, so this was left as is.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22222/#review46832
-----------------------------------------------------------
On June 26, 2014, 5:50 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22222/
> -----------------------------------------------------------
>
> (Updated June 26, 2014, 5:50 p.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/credentials_tests.cpp PRE-CREATION
> src/tests/mesos.cpp 1037420
>
> Diff: https://reviews.apache.org/r/22222/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 22222: HTTP authentication flag for basic/digest HTTP
authentication
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22222/#review46832
-----------------------------------------------------------
src/master/master.hpp
<https://reviews.apache.org/r/22222/#comment82403>
i might be missing something, but why are these being stored in the master?
src/sasl/authenticator.hpp
<https://reviews.apache.org/r/22222/#comment82402>
should you check that a principal doesn't appear more than once?
- Dominic Hamon
On June 26, 2014, 10:50 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22222/
> -----------------------------------------------------------
>
> (Updated June 26, 2014, 10:50 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/credentials_tests.cpp PRE-CREATION
> src/tests/mesos.cpp 1037420
>
> Diff: https://reviews.apache.org/r/22222/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 22222: HTTP authentication flag for basic/digest HTTP
authentication
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22222/
-----------------------------------------------------------
(Updated June 26, 2014, 5:50 p.m.)
Review request for mesos, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
Changes
-------
Changes after Adam's review
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 (updated)
-----
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/credentials_tests.cpp PRE-CREATION
src/tests/mesos.cpp 1037420
Diff: https://reviews.apache.org/r/22222/diff/
Testing
-------
Thanks,
Isabel Jimenez
Re: Review Request 22222: HTTP authentication flag for basic/digest HTTP
authentication
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22222/
-----------------------------------------------------------
(Updated June 20, 2014, 6:08 p.m.)
Review request for mesos, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
Changes
-------
Changes after Ben's comments
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 (updated)
-----
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
Re: Review Request 22222: HTTP authentication flag for basic/digest HTTP
authentication
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22222/
-----------------------------------------------------------
(Updated June 17, 2014, 10:31 p.m.)
Review request for mesos, Benjamin Hindman, Dominic Hamon, and Till Toenshoff.
Changes
-------
Make some changes after Ben's comments. Have some doubts about slave credential json support, see comments on JIRA.
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 (updated)
-----
include/mesos/mesos.proto 709b8b1
src/Makefile.am 3e623cc
src/credentials/credentials.hpp 98b9088
src/master/flags.hpp 780e219
src/master/master.hpp 7a12185
src/master/master.cpp 4a01b1a
src/sasl/authenticator.hpp 365db5f
src/slave/flags.hpp 3b8ba08
src/slave/slave.hpp 3227e73
src/slave/slave.cpp bc976b7
src/tests/cluster.hpp 1c96ee7
src/tests/credentials_tests.cpp PRE-CREATION
src/tests/mesos.hpp 0b9b2f9
src/tests/mesos.cpp 98a7c38
Diff: https://reviews.apache.org/r/22222/diff/
Testing
-------
Thanks,
Isabel Jimenez
Re: Review Request 22222: HTTP authentication flag for basic/digest HTTP
authentication
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22222/#review44968
-----------------------------------------------------------
Let's definitely reuse the --credentials flag (and thus s/Authentications/Credentials). We can try parsing as a JSON object first and if that fails then fall back to parsing as before (and if that fails then exit and report the error).
src/authentications/authentications.hpp
<https://reviews.apache.org/r/22222/#comment79583>
You'll want to first try and parse to JSON (and if that fails fall back and parse as we've done in the past with --credentials since we want to override that flag).
src/messages/messages.proto
<https://reviews.apache.org/r/22222/#comment79584>
s/Authentications/Credentials/ here and everywhere else! See top-level comment.
src/messages/messages.proto
<https://reviews.apache.org/r/22222/#comment79579>
Plus +2 indents here please.
src/messages/messages.proto
<https://reviews.apache.org/r/22222/#comment79585>
So, I guess Credentials::HTTP ends up being a bit weird after all. :-( How do you feel about having people use 'principal' and 'secret' instead of 'username' and 'password'?
I think we should also use the public Credential protobuf too, no need to duplicate that here.
- Benjamin Hindman
On June 3, 2014, 11:15 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22222/
> -----------------------------------------------------------
>
> (Updated June 3, 2014, 11:15 p.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
> -----
>
> src/authentications/authentications.hpp PRE-CREATION
> src/master/flags.hpp 4863359
> src/master/master.cpp 766a0e3
> src/messages/messages.proto 6f6e570
>
> Diff: https://reviews.apache.org/r/22222/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>