You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Adam B <ad...@mesosphere.io> on 2014/04/01 08:28:00 UTC
Re: Review Request 18381: Added authentication support for slaves.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/
-----------------------------------------------------------
(Updated March 31, 2014, 11:27 p.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Adding back the disconnect(Slave) patch this one depends on.
(Somehow got removed again when I updated the diff.)
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
Re: Review Request 18381: Added authentication support for slaves.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/#review39403
-----------------------------------------------------------
Patch looks great!
Reviews applied: [18381]
All tests passed.
- Mesos ReviewBot
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
>
>
Re: Review Request 18381: Added authentication support for slaves.
Posted by Vinod Kone <vi...@gmail.com>.
> On April 12, 2014, 2:54 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 640
> > <https://reviews.apache.org/r/18381/diff/7/?file=540073#file540073line640>
> >
> > s/master.get()/self()/
>
> Adam B wrote:
> Slave::shutdown() exits without terminating if (from && master != from), so I either need to pass in master or nothing.
> I thought master was appropriate, since we actually got back a future==false (authentication failure) response from the master, as opposed to a slave-side error.
> Would you rather I pass nothing? Or alter Slave::shutdown() to also allow (from == self())?
>
> Vinod Kone wrote:
> s/self()/UPID()/
>
> this is how we manually call remote message handlers.
>
> Adam B wrote:
> To be clear, you're suggesting I call shutdown(UPID(), "msg"); which will provide a default/empty pid rather than master's?
correct.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/#review40197
-----------------------------------------------------------
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
>
>
Re: Review Request 18381: Added authentication support for slaves.
Posted by Adam B <ad...@mesosphere.io>.
> On April 11, 2014, 7:54 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 371
> > <https://reviews.apache.org/r/18381/diff/7/?file=540073#file540073line371>
> >
> > s/error/message/
> >
> > What happens if the slaves are upgraded before the master?
>
> Adam B wrote:
> If the Slaves are upgraded before the master, then the master would only send ShutdownMessages without the 'message' field.
> What I hoped would happen: Since the 'message' field is optional, install should pass None() to shutdown as an Option, and no error would be printed.
> After investigating, I now realize that it will default to an empty string, and I'll need to handle that instead of isNone(). Correct?
>
> I still think it would be nice if ProtobufProcess::install() could convert an optional protobuf into an Option of that type.
>
> Vinod Kone wrote:
> Yes. I think it will send it as an empty string. So just use string param instead of Option<string>.
Done.
> On April 11, 2014, 7:54 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 640
> > <https://reviews.apache.org/r/18381/diff/7/?file=540073#file540073line640>
> >
> > s/master.get()/self()/
>
> Adam B wrote:
> Slave::shutdown() exits without terminating if (from && master != from), so I either need to pass in master or nothing.
> I thought master was appropriate, since we actually got back a future==false (authentication failure) response from the master, as opposed to a slave-side error.
> Would you rather I pass nothing? Or alter Slave::shutdown() to also allow (from == self())?
>
> Vinod Kone wrote:
> s/self()/UPID()/
>
> this is how we manually call remote message handlers.
To be clear, you're suggesting I call shutdown(UPID(), "msg"); which will provide a default/empty pid rather than master's?
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/#review40197
-----------------------------------------------------------
On April 2, 2014, 9:44 p.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18381/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 9:44 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 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
>
>
Re: Review Request 18381: Added authentication support for slaves.
Posted by Vinod Kone <vi...@gmail.com>.
> On April 12, 2014, 2:54 a.m., Vinod Kone wrote:
> > src/master/master.cpp, line 2678
> > <https://reviews.apache.org/r/18381/diff/7/?file=540066#file540066line2678>
> >
> > 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.
>
> Adam B wrote:
> Added a TODO in master.hpp. Would need significant refactoring to share code across the Framework and Slave structs, deferring to a later review.
i just meant the consolidation of the verbs. we should either call it deactivate or disconnect for both frameworks and slaves. TODO is fine.
> On April 12, 2014, 2:54 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 371
> > <https://reviews.apache.org/r/18381/diff/7/?file=540073#file540073line371>
> >
> > s/error/message/
> >
> > What happens if the slaves are upgraded before the master?
>
> Adam B wrote:
> If the Slaves are upgraded before the master, then the master would only send ShutdownMessages without the 'message' field.
> What I hoped would happen: Since the 'message' field is optional, install should pass None() to shutdown as an Option, and no error would be printed.
> After investigating, I now realize that it will default to an empty string, and I'll need to handle that instead of isNone(). Correct?
>
> I still think it would be nice if ProtobufProcess::install() could convert an optional protobuf into an Option of that type.
Yes. I think it will send it as an empty string. So just use string param instead of Option<string>.
> On April 12, 2014, 2:54 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 590
> > <https://reviews.apache.org/r/18381/diff/7/?file=540073#file540073line590>
> >
> > I know this is copy pasted from sched.cpp but can this be CHECK_NOTNULL(authenticatee)?
>
> Adam B wrote:
> We actually want to make sure that authenticatee IS null before we create a new Authenticatee. Looks like there's no CHECK_ISNULL(), so I'll leave this as is.
i'm clearly blind. CHECK is fine here.
> On April 12, 2014, 2:54 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 640
> > <https://reviews.apache.org/r/18381/diff/7/?file=540073#file540073line640>
> >
> > s/master.get()/self()/
>
> Adam B wrote:
> Slave::shutdown() exits without terminating if (from && master != from), so I either need to pass in master or nothing.
> I thought master was appropriate, since we actually got back a future==false (authentication failure) response from the master, as opposed to a slave-side error.
> Would you rather I pass nothing? Or alter Slave::shutdown() to also allow (from == self())?
s/self()/UPID()/
this is how we manually call remote message handlers.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/#review40197
-----------------------------------------------------------
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
>
>
Re: Review Request 18381: Added authentication support for slaves.
Posted by Adam B <ad...@mesosphere.io>.
> On April 11, 2014, 7:54 p.m., Vinod Kone wrote:
> > 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.
Thanks for reviewing! This reply responds to most of your comments. I still need to pull readCredentials and ShutdownMessage into separate reviews and then update this one as the main diff, depending on those two.
> On April 11, 2014, 7:54 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 3400
> > <https://reviews.apache.org/r/18381/diff/7/?file=540066#file540066line3400>
> >
> > shouldn't this also be done in Master::disconnect(Slave*) similar to how we do it in Master::deactivate(Framework*) ?
Oops. Moved that into the disconnect(Slave) review, had it removed, and forgot to add it back here. Fixed.
> On April 11, 2014, 7:54 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 371
> > <https://reviews.apache.org/r/18381/diff/7/?file=540073#file540073line371>
> >
> > s/error/message/
> >
> > What happens if the slaves are upgraded before the master?
If the Slaves are upgraded before the master, then the master would only send ShutdownMessages without the 'message' field.
What I hoped would happen: Since the 'message' field is optional, install should pass None() to shutdown as an Option, and no error would be printed.
After investigating, I now realize that it will default to an empty string, and I'll need to handle that instead of isNone(). Correct?
I still think it would be nice if ProtobufProcess::install() could convert an optional protobuf into an Option of that type.
> On April 11, 2014, 7:54 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 590
> > <https://reviews.apache.org/r/18381/diff/7/?file=540073#file540073line590>
> >
> > I know this is copy pasted from sched.cpp but can this be CHECK_NOTNULL(authenticatee)?
We actually want to make sure that authenticatee IS null before we create a new Authenticatee. Looks like there's no CHECK_ISNULL(), so I'll leave this as is.
> On April 11, 2014, 7:54 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 640
> > <https://reviews.apache.org/r/18381/diff/7/?file=540073#file540073line640>
> >
> > s/master.get()/self()/
Slave::shutdown() exits without terminating if (from && master != from), so I either need to pass in master or nothing.
I thought master was appropriate, since we actually got back a future==false (authentication failure) response from the master, as opposed to a slave-side error.
Would you rather I pass nothing? Or alter Slave::shutdown() to also allow (from == self())?
> On April 11, 2014, 7:54 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 2678
> > <https://reviews.apache.org/r/18381/diff/7/?file=540066#file540066line2678>
> >
> > 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.
Added a TODO in master.hpp. Would need significant refactoring to share code across the Framework and Slave structs, deferring to a later review.
> On April 11, 2014, 7:54 p.m., Vinod Kone wrote:
> > src/sasl/authenticatee.hpp, lines 404-407
> > <https://reviews.apache.org/r/18381/diff/7/?file=540068#file540068line404>
> >
> > why the change?
> On April 11, 2014, 7:54 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 2666
> > <https://reviews.apache.org/r/18381/diff/7/?file=540066#file540066line2666>
> >
> > 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().
Fair enough. I was just worried that every time we register a slave, we would unnecessarily iterate through the list of all frameworks, which could be rather large. But I suppose slaves don't register that frequently (except on master failover), so speed is less important there.
At least this is better than the other way around, where registering new frameworks (which needs to be fast for interactive jobs) would unnecessarily iterate through thousands of slaves.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/#review40197
-----------------------------------------------------------
On April 2, 2014, 9:44 p.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18381/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 9:44 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 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
>
>
Re: Review Request 18381: Added authentication support for slaves.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 18381: Added authentication support for slaves.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/#review40253
-----------------------------------------------------------
Bad patch!
Reviews applied: [20302, 20301, 18381]
Failed command: make -j3 check GTEST_FILTER='' >/dev/null
Error:
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'
zip_safe flag not set; analyzing archive contents...
WARNING: '.' not a valid package name; please use only.-separated package names in setup.py
package init file 'src/__init__.py' not found (or not a regular file)
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
zip_safe flag not set; analyzing archive contents...
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
tests/authentication_tests.cpp: In member function 'virtual void AuthenticationTest_MasterFailoverDuringSlaveAuthentication_Test::TestBody()':
tests/authentication_tests.cpp:530:59: error: no matching function for call to 'AuthenticationTest_MasterFailoverDuringSlaveAuthentication_Test::StartSlave(process::Owned<mesos::internal::MasterDetector>, mesos::internal::slave::Flags&)'
tests/authentication_tests.cpp:530:59: note: candidates are:
./tests/mesos.hpp:100:44: note: virtual Try<process::PID<mesos::internal::slave::Slave> > mesos::internal::tests::MesosTest::StartSlave(const Option<mesos::internal::slave::Flags>&)
./tests/mesos.hpp:100:44: note: candidate expects 1 argument, 2 provided
./tests/mesos.hpp:104:44: note: virtual Try<process::PID<mesos::internal::slave::Slave> > mesos::internal::tests::MesosTest::StartSlave(mesos::internal::tests::MockExecutor*, const Option<mesos::internal::slave::Flags>&)
./tests/mesos.hpp:104:44: note: no known conversion for argument 1 from 'process::Owned<mesos::internal::MasterDetector>' to 'mesos::internal::tests::MockExecutor*'
./tests/mesos.hpp:109:44: note: virtual Try<process::PID<mesos::internal::slave::Slave> > mesos::internal::tests::MesosTest::StartSlave(mesos::internal::slave::Containerizer*, const Option<mesos::internal::slave::Flags>&)
./tests/mesos.hpp:109:44: note: no known conversion for argument 1 from 'process::Owned<mesos::internal::MasterDetector>' to 'mesos::internal::slave::Containerizer*'
./tests/mesos.hpp:114:44: note: virtual Try<process::PID<mesos::internal::slave::Slave> > mesos::internal::tests::MesosTest::StartSlave(mesos::internal::slave::Containerizer*, mesos::internal::MasterDetector*, const Option<mesos::internal::slave::Flags>&)
./tests/mesos.hpp:114:44: note: no known conversion for argument 1 from 'process::Owned<mesos::internal::MasterDetector>' to 'mesos::internal::slave::Containerizer*'
./tests/mesos.hpp:120:44: note: virtual Try<process::PID<mesos::internal::slave::Slave> > mesos::internal::tests::MesosTest::StartSlave(mesos::internal::MasterDetector*, const Option<mesos::internal::slave::Flags>&)
./tests/mesos.hpp:120:44: note: no known conversion for argument 1 from 'process::Owned<mesos::internal::MasterDetector>' to 'mesos::internal::MasterDetector*'
./tests/mesos.hpp:126:44: note: virtual Try<process::PID<mesos::internal::slave::Slave> > mesos::internal::tests::MesosTest::StartSlave(mesos::internal::tests::MockExecutor*, mesos::internal::MasterDetector*, const Option<mesos::internal::slave::Flags>&)
./tests/mesos.hpp:126:44: note: no known conversion for argument 1 from 'process::Owned<mesos::internal::MasterDetector>' to 'mesos::internal::tests::MockExecutor*'
tests/authentication_tests.cpp: In member function 'virtual void AuthenticationTest_LeaderElectionDuringSlaveAuthentication_Test::TestBody()':
tests/authentication_tests.cpp:610:59: error: no matching function for call to 'AuthenticationTest_LeaderElectionDuringSlaveAuthentication_Test::StartSlave(process::Owned<mesos::internal::MasterDetector>, mesos::internal::slave::Flags&)'
tests/authentication_tests.cpp:610:59: note: candidates are:
./tests/mesos.hpp:100:44: note: virtual Try<process::PID<mesos::internal::slave::Slave> > mesos::internal::tests::MesosTest::StartSlave(const Option<mesos::internal::slave::Flags>&)
./tests/mesos.hpp:100:44: note: candidate expects 1 argument, 2 provided
./tests/mesos.hpp:104:44: note: virtual Try<process::PID<mesos::internal::slave::Slave> > mesos::internal::tests::MesosTest::StartSlave(mesos::internal::tests::MockExecutor*, const Option<mesos::internal::slave::Flags>&)
./tests/mesos.hpp:104:44: note: no known conversion for argument 1 from 'process::Owned<mesos::internal::MasterDetector>' to 'mesos::internal::tests::MockExecutor*'
./tests/mesos.hpp:109:44: note: virtual Try<process::PID<mesos::internal::slave::Slave> > mesos::internal::tests::MesosTest::StartSlave(mesos::internal::slave::Containerizer*, const Option<mesos::internal::slave::Flags>&)
./tests/mesos.hpp:109:44: note: no known conversion for argument 1 from 'process::Owned<mesos::internal::MasterDetector>' to 'mesos::internal::slave::Containerizer*'
./tests/mesos.hpp:114:44: note: virtual Try<process::PID<mesos::internal::slave::Slave> > mesos::internal::tests::MesosTest::StartSlave(mesos::internal::slave::Containerizer*, mesos::internal::MasterDetector*, const Option<mesos::internal::slave::Flags>&)
./tests/mesos.hpp:114:44: note: no known conversion for argument 1 from 'process::Owned<mesos::internal::MasterDetector>' to 'mesos::internal::slave::Containerizer*'
./tests/mesos.hpp:120:44: note: virtual Try<process::PID<mesos::internal::slave::Slave> > mesos::internal::tests::MesosTest::StartSlave(mesos::internal::MasterDetector*, const Option<mesos::internal::slave::Flags>&)
./tests/mesos.hpp:120:44: note: no known conversion for argument 1 from 'process::Owned<mesos::internal::MasterDetector>' to 'mesos::internal::MasterDetector*'
./tests/mesos.hpp:126:44: note: virtual Try<process::PID<mesos::internal::slave::Slave> > mesos::internal::tests::MesosTest::StartSlave(mesos::internal::tests::MockExecutor*, mesos::internal::MasterDetector*, const Option<mesos::internal::slave::Flags>&)
./tests/mesos.hpp:126:44: note: no known conversion for argument 1 from 'process::Owned<mesos::internal::MasterDetector>' to 'mesos::internal::tests::MockExecutor*'
make[3]: *** [tests/mesos_tests-authentication_tests.o] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [check-am] Error 2
make[1]: *** [check] Error 2
make: *** [check-recursive] Error 1
- Mesos ReviewBot
On April 14, 2014, 6:34 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18381/
> -----------------------------------------------------------
>
> (Updated April 14, 2014, 6:34 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.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/master/flags.hpp 024f86d
> src/master/master.hpp fef59c9
> src/master/master.cpp 3c3c989
> src/sasl/authenticatee.hpp 42a4eba
> src/slave/flags.hpp d5c54c0
> src/slave/slave.hpp 08f6005
> src/slave/slave.cpp cddb241
> src/tests/authentication_tests.cpp 127c5e6
> src/tests/mesos.cpp ae3aeee
> 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
>
>
Re: Review Request 18381: Added authentication support for slaves.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/#review40266
-----------------------------------------------------------
Patch looks great!
Reviews applied: [20302, 20301, 18381]
All tests passed.
- Mesos ReviewBot
On April 14, 2014, 4:09 p.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18381/
> -----------------------------------------------------------
>
> (Updated April 14, 2014, 4:09 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.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/master/flags.hpp 024f86d
> src/master/master.hpp fef59c9
> src/master/master.cpp 3c3c989
> src/sasl/authenticatee.hpp 42a4eba
> src/slave/flags.hpp d5c54c0
> src/slave/slave.hpp 08f6005
> src/slave/slave.cpp cddb241
> src/tests/authentication_tests.cpp 127c5e6
> src/tests/mesos.cpp a9844e4
> 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
>
>
Re: Review Request 18381: Added authentication support for slaves.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/#review41074
-----------------------------------------------------------
Hey Adam, sorry for the delay on this. I'm a bit backed up at work. I will try to get to these before EOW. Hope thats ok?
- Vinod Kone
On April 22, 2014, 2:50 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18381/
> -----------------------------------------------------------
>
> (Updated April 22, 2014, 2:50 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.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/master/flags.hpp acf3963
> src/master/master.hpp f567a43
> src/master/master.cpp 0335b34
> src/sasl/authenticatee.hpp 42a4eba
> src/slave/flags.hpp d5c54c0
> src/slave/slave.hpp 438e5b5
> src/slave/slave.cpp b3c4285
> src/tests/authentication_tests.cpp 127c5e6
> src/tests/mesos.cpp a9844e4
> 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
>
>
Re: Review Request 18381: Added authentication support for slaves.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/#review41496
-----------------------------------------------------------
hey adam. mind rebasing this off the latest master?
- Vinod Kone
On April 22, 2014, 2:50 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18381/
> -----------------------------------------------------------
>
> (Updated April 22, 2014, 2:50 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.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/master/flags.hpp acf3963
> src/master/master.hpp f567a43
> src/master/master.cpp 0335b34
> src/sasl/authenticatee.hpp 42a4eba
> src/slave/flags.hpp d5c54c0
> src/slave/slave.hpp 438e5b5
> src/slave/slave.cpp b3c4285
> src/tests/authentication_tests.cpp 127c5e6
> src/tests/mesos.cpp a9844e4
> 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
>
>
Re: Review Request 18381: Added authentication support for slaves.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/#review41493
-----------------------------------------------------------
Ship it!
Thanks Adam for the patience and the hard work. I'll commit this now.
src/tests/mesos.cpp
<https://reviews.apache.org/r/18381/#comment74937>
thank you :)
- Vinod Kone
On April 22, 2014, 2:50 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18381/
> -----------------------------------------------------------
>
> (Updated April 22, 2014, 2:50 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.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/master/flags.hpp acf3963
> src/master/master.hpp f567a43
> src/master/master.cpp 0335b34
> src/sasl/authenticatee.hpp 42a4eba
> src/slave/flags.hpp d5c54c0
> src/slave/slave.hpp 438e5b5
> src/slave/slave.cpp b3c4285
> src/tests/authentication_tests.cpp 127c5e6
> src/tests/mesos.cpp a9844e4
> 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
>
>
Re: Review Request 18381: Added authentication support for slaves.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/
-----------------------------------------------------------
(Updated April 25, 2014, 12:54 p.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Rebased and retested after dependent patches were committed.
Bugs: MESOS-804
https://issues.apache.org/jira/browse/MESOS-804
Repository: mesos-git
Description
-------
Added authentication support for slaves.
Fixes MESOS-804.
Diffs (updated)
-----
include/mesos/mesos.proto d7fe068
src/master/flags.hpp 26a0492
src/master/master.hpp 9808378
src/master/master.cpp 890873b
src/sasl/authenticatee.hpp 42a4eba
src/slave/flags.hpp 55cf671
src/slave/slave.hpp 90b2c83
src/slave/slave.cpp 6bcf64f
src/tests/authentication_tests.cpp 127c5e6
src/tests/mesos.cpp ae926c9
src/tests/slave_recovery_tests.cpp 84b7796
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
Re: Review Request 18381: Added authentication support for slaves.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/
-----------------------------------------------------------
(Updated April 21, 2014, 7:50 p.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Rebased against latest master and dependent patches 20301, 20302 (both of which have ShipIts).
Vinod, please commit the dependent patches and let me know what's left before this one gets a ShipIt. Thanks!
Bugs: MESOS-804
https://issues.apache.org/jira/browse/MESOS-804
Repository: mesos-git
Description
-------
Added authentication support for slaves.
Fixes MESOS-804.
Diffs (updated)
-----
include/mesos/mesos.proto 37f8a7f
src/master/flags.hpp acf3963
src/master/master.hpp f567a43
src/master/master.cpp 0335b34
src/sasl/authenticatee.hpp 42a4eba
src/slave/flags.hpp d5c54c0
src/slave/slave.hpp 438e5b5
src/slave/slave.cpp b3c4285
src/tests/authentication_tests.cpp 127c5e6
src/tests/mesos.cpp a9844e4
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
Re: Review Request 18381: Added authentication support for slaves.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/
-----------------------------------------------------------
(Updated April 14, 2014, 9:09 a.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Rebased after BenH "Cleaned up master detector usage in tests."
Bugs: MESOS-804
https://issues.apache.org/jira/browse/MESOS-804
Repository: mesos-git
Description
-------
Added authentication support for slaves.
Fixes MESOS-804.
Diffs (updated)
-----
include/mesos/mesos.proto 37f8a7f
src/master/flags.hpp 024f86d
src/master/master.hpp fef59c9
src/master/master.cpp 3c3c989
src/sasl/authenticatee.hpp 42a4eba
src/slave/flags.hpp d5c54c0
src/slave/slave.hpp 08f6005
src/slave/slave.cpp cddb241
src/tests/authentication_tests.cpp 127c5e6
src/tests/mesos.cpp a9844e4
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
Re: Review Request 18381: Added authentication support for slaves.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/
-----------------------------------------------------------
(Updated April 13, 2014, 11:34 p.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Split off sub-reviews for credentials-reading and the shutdown message. Made changes in response to Vinod's comments.
Please review.
Bugs: MESOS-804
https://issues.apache.org/jira/browse/MESOS-804
Repository: mesos-git
Description (updated)
-------
Added authentication support for slaves.
Fixes MESOS-804.
Diffs (updated)
-----
include/mesos/mesos.proto 37f8a7f
src/master/flags.hpp 024f86d
src/master/master.hpp fef59c9
src/master/master.cpp 3c3c989
src/sasl/authenticatee.hpp 42a4eba
src/slave/flags.hpp d5c54c0
src/slave/slave.hpp 08f6005
src/slave/slave.cpp cddb241
src/tests/authentication_tests.cpp 127c5e6
src/tests/mesos.cpp ae3aeee
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
Re: Review Request 18381: Added authentication support for slaves.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/#review40185
-----------------------------------------------------------
Ping. Vinod, please review. Thanks!
- Adam B
On April 2, 2014, 9:44 p.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18381/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 9:44 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 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
>
>
Re: Review Request 18381: Added authentication support for slaves.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/
-----------------------------------------------------------
(Updated April 2, 2014, 9:44 p.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Removed dependency review 19006, since it has been submitted.
Vinod, please review.
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
Re: Review Request 18381: Added authentication support for slaves.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18381/#review39140
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19006, 18381]
All tests passed.
- Mesos ReviewBot
On April 1, 2014, 6:27 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18381/
> -----------------------------------------------------------
>
> (Updated April 1, 2014, 6:27 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
>
>