----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57535/#review169325 -----------------------------------------------------------
src/master/master.hpp Lines 684-686 (patched) <https://reviews.apache.org/r/57535/#comment241677> Could you leave a TODO here to update this function to use `Principal` when MESOS-7202 is resolved? src/master/master.hpp Lines 684-685 (patched) <https://reviews.apache.org/r/57535/#comment241683> Could you tweak this comment a bit? "Returns whether the principal is authorized to (re-)register an agent." seems a bit more accurate to me? src/master/master.hpp Lines 686 (patched) <https://reviews.apache.org/r/57535/#comment241675> 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'. src/master/master.cpp Lines 3642 (patched) <https://reviews.apache.org/r/57535/#comment241676> Ditto src/master/master.cpp Lines 5419-5420 (patched) <https://reviews.apache.org/r/57535/#comment241684> Fits on one line. src/master/master.cpp Lines 5423-5424 (patched) <https://reviews.apache.org/r/57535/#comment241685> s/to to/to/ Also, could you make this logging a bit more explicit for the NONE principal case? Just to be clear that it's not a principal called 'none', might be better if it printed "Not authorized to register as agent without a principal" or something similar. src/master/master.cpp Lines 5437-5442 (patched) <https://reviews.apache.org/r/57535/#comment241688> 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. src/master/master.cpp Lines 5674-5675 (patched) <https://reviews.apache.org/r/57535/#comment241687> Fits on one line. src/master/master.cpp Lines 5678-5679 (patched) <https://reviews.apache.org/r/57535/#comment241686> s/to to/to/ Also, same comment regarding the NONE case. src/tests/master_authorization_tests.cpp Lines 2354 (patched) <https://reviews.apache.org/r/57535/#comment241689> s/it is/verify that it is/ src/tests/master_authorization_tests.cpp Lines 2357-2362 (patched) <https://reviews.apache.org/r/57535/#comment241690> 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? - Greg Mann On March 15, 2017, 1:09 a.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57535/ > ----------------------------------------------------------- > > (Updated March 15, 2017, 1:09 a.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 > >
