mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 57535: Applied RegisterAgent ACL to the master.
Date Mon, 20 Mar 2017 06:17:56 GMT


> On March 17, 2017, 2:32 p.m., Greg Mann wrote:
> > src/master/master.hpp
> > Lines 684-686 (patched)
> > <https://reviews.apache.org/r/57535/diff/3/?file=1665114#file1665114line684>
> >
> >     Could you leave a TODO here to update this function to use `Principal` when
MESOS-7202 is resolved?

Will MESOS-7202 cover the master <-> agent protocol and the `authenticated`?


> On March 17, 2017, 2:32 p.m., Greg Mann wrote:
> > src/master/master.hpp
> > Lines 686 (patched)
> > <https://reviews.apache.org/r/57535/diff/3/?file=1665114#file1665114line686>
> >
> >     Should we do `authorizeAgent` since this is an entirely new function? I'm not
sure if we're trying to transition as we add new code; I only see one example in the agent
code, `getAgent()` in 'slave/http.cpp'.

`getAgents` directly corresponds to the new API `GET_AGENTS` so it makese more sense to use
the new terminology. AFAIK in other cases we maintain file level consistency and defer to
a later sweep.


> On March 17, 2017, 2:32 p.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 5437-5442 (patched)
> > <https://reviews.apache.org/r/57535/diff/3/?file=1665115#file1665115line5437>
> >
> >     I'm not confident in verifying this myself without some more effort; perhaps
Jie or Neil or somebody else familiar with the agent registration code path in the master
could verify that this is the case?
> >     
> >     I could have another look next week to dig in here a bit.

To add to the comment: `authenticated` supports an optional step (i.e., authentication) in
the (re-)registration path. If no authentication is done, the pid wouldn't be in `authenticated`
but the whole process still works by design. Therefore `authenticated` is checked in the beginning
and never checked again after this step. So the robustness of agent (re-)registration doesn't
care if `pid` is removed from `authenticated` (or not in it in the first place). In other
words, `pid` being removed from `authenticated` would suggest disconnection but this is not
the place to handle it.


> On March 17, 2017, 2:32 p.m., Greg Mann wrote:
> > src/tests/master_authorization_tests.cpp
> > Lines 2357-2362 (patched)
> > <https://reviews.apache.org/r/57535/diff/3/?file=1665116#file1665116line2357>
> >
> >     Are we sure that this represents a reregistration, rather than a registration
of a new agent? Perhaps you could do `EXPECT_CALL` on something in the agent's reregistration
code path to verify?

Good catch. I created a `slaveFlags` above but forgot to use it. I should've used `FUTURE_MESSAGE(Eq(SlaveReregisteredMessage().GetTypeName()),
_, _);` and that should confirm it.


- Jiang Yan


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


On March 14, 2017, 6:09 p.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57535/
> -----------------------------------------------------------
> 
> (Updated March 14, 2017, 6:09 p.m.)
> 
> 
> Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Greg Mann, and Vinod
Kone.
> 
> 
> Bugs: MESOS-7097
>     https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Applied RegisterAgent ACL to the master.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 
>   src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
>   src/tests/master_authorization_tests.cpp 1a0285a3f345ef21a8256d4123d8bb684f538da4 
> 
> 
> Diff: https://reviews.apache.org/r/57535/diff/3/
> 
> 
> Testing
> -------
> 
> make check.
> 
> The tests added here cover some basic scenarios, I have more tests but will add them
when MESOS-7244 is fixed.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


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