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