mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vinod Kone" <vinodk...@gmail.com>
Subject Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
Date Wed, 04 Mar 2015 21:16:15 GMT


> On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
> > src/authentication/cram_md5/authenticator.cpp, line 449
> > <https://reviews.apache.org/r/27760/diff/17/?file=869278#file869278line449>
> >
> >     Shouldn't this be protected by once() to avoid 2 different threads loading secrets
at the same time?
> 
> Till Toenshoff wrote:
>     That would render our tests broken (those that reset the credentials). The concurrency
is covered by a Lock within that SASL aux plugin code.
> 
> Till Toenshoff wrote:
>     Let me know if this is ok, please.
> 
> Vinod Kone wrote:
>     Sorry, I don't think I follow. Can you elaborate on what specific tests fail and
why?
> 
> Till Toenshoff wrote:
>     Our tests all run within the same OS-process. A ONCE-gatekeeper on credential loading
within this process would indeed allow this only a single time. The CRAM-MD5 tests do attempt
to re/load the credentials; see cram_md5_authentication.cpp #95, #140, ... 
>     These tests make sense given that we want to test the behavior of the authenticator/authenticatee
when using non matching credentials.

I don't see them reloading out of band. In other words, before your patch we used to call
secrets::load() directly from tests, which I agree has thread safety issues if secrets::load()
itself is not protected. But now, with your patch, all the loading happens via Authenticator->initialize().
So I'm trying to understand why the protection in Authenticator initialize is not enough.
Sorry, if I'm mising something.


> On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 3842
> > <https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3842>
> >
> >     I think seeing this log line in master log would not reveal much information
on what's happening.
> >     
> >      How about
> >      
> >      LOG(ERROR) << "Received authentication request from " << pid
> >                 << " but authenticator is not loaded";

AuthenticationErrorMessage is only used by authenticator. How about sending FrameworkError
message instead? That way the framework will atleast abort and know that it is doing something
wrong. With AuthenticationErrorMessage it will just blindly retry.


> On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 3897-3898
> > <https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3897>
> >
> >     Why do we need this 'onAny'? Why can't 'authenticated' be set in __authenticate()?
> 
> Till Toenshoff wrote:
>     `__authenticated` is needed to capture (at least) a discard on the outer future.
I have rearranged the code of both continuations to be more explicit about that.

A Future::discard() *does not* result in a future being DISCARDED (and onAny callbacks being
called). You want an onDiscard() handler (instead of onAny) to capture that case.


> On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 3952-3954
> > <https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line3952>
> >
> >     Can you rephrase these log messages to something meaninguful when one looks
at master log?
> 
> Till Toenshoff wrote:
>     How about this?
>     ```
>         if (authenticate.isDiscarded()) {
>           LOG(WARNING) << "Authentication for " << pid
>                        << " has pending cancel request";
>         } else if (authenticate.discard()) {
>           LOG(WARNING) << "Requested to cancel authentication for " << pid;
>         }
>      ```
> 
> Adam B wrote:
>     How about: "Tried to cancel authentication session for [pid], but it was already
cancelled" and "Cancelling authentication session for PID"?

How are you cancelling the authentication here?


- Vinod


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27760/#review73310
-----------------------------------------------------------


On March 3, 2015, 2:58 p.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27760/
> -----------------------------------------------------------
> 
> (Updated March 3, 2015, 2:58 p.m.)
> 
> 
> Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2050
>     https://issues.apache.org/jira/browse/MESOS-2050
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The initial design and implementation of the authenticator module interface caused issues
and was not optimal for heavy lifting setup of external dependencies. By introducing a two
fold design, this has been decoupled from the authentication message processing. The new design
also gets us back on track to the goal of makeing SASL a soft dependency of mesos.
> 
> 
> Diffs
> -----
> 
>   include/mesos/authentication/authenticator.hpp f66217a 
>   src/Makefile.am d299f07 
>   src/authentication/cram_md5/authenticator.hpp c6f465f 
>   src/authentication/cram_md5/authenticator.cpp PRE-CREATION 
>   src/authentication/cram_md5/auxprop.hpp b894386 
>   src/authentication/cram_md5/auxprop.cpp cf503a2 
>   src/master/master.hpp ce0e0b3 
>   src/master/master.cpp 53c8696 
>   src/tests/cram_md5_authentication_tests.cpp 92a89c5 
> 
> Diff: https://reviews.apache.org/r/27760/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>


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