mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 18381: Added authentication support for slaves.
Date Sun, 13 Apr 2014 07:22:35 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message