> On Dec. 13, 2017, 3:54 a.m., Alexander Rojas wrote: > > src/master/master.cpp > > Lines 6138-6141 (original), 6169-6173 (patched) > > <https://reviews.apache.org/r/64515/diff/1/?file=1912811#file1912811line6169> > > > > How about: > > > > _Not authorized to register agent with(out) principal XXX providing the > > resrouces YYYY_
It now reads "Not authorized to register agent providing resources YYYY with principal XXXX" or "Not authorized to register agent providing resources YYYY with without a principal" I am putting "agent" together with "providing resources" because they together are the objects. > On Dec. 13, 2017, 3:54 a.m., Alexander Rojas wrote: > > src/tests/master_authorization_tests.cpp > > Lines 2538-2539 (patched) > > <https://reviews.apache.org/r/64515/diff/1/?file=1912812#file1912812line2538> > > > > Instead of the things in the parenthesis you can add: > > > > ```c++ > > { > > mesos::ACL::ReserveResources* acl = acls.add_reserve_resources(); > > acl->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal()); > > acl->mutable_roles()->set_type(ACL::Entity::NONE); > > } > > ``` > > > > And add a comment when starting the agent indicating that the agent is > > registering with `DEFAULT_CREDENTIAL.principal()` The test above `UnauthorizedToStaticallyReserveResources` has exactly this. I am just using different scenarios to broaden the test coverage. > On Dec. 13, 2017, 3:54 a.m., Alexander Rojas wrote: > > src/tests/master_authorization_tests.cpp > > Lines 2571 (patched) > > <https://reviews.apache.org/r/64515/diff/1/?file=1912812#file1912812line2571> > > > > Please add a test where the agent tries to register with > > `high-security-role` and succeeds. Added test `AuthorizedToStaticallyReserveRole`. > On Dec. 13, 2017, 3:54 a.m., Alexander Rojas wrote: > > src/tests/master_authorization_tests.cpp > > Lines 2577 (patched) > > <https://reviews.apache.org/r/64515/diff/1/?file=1912812#file1912812line2577> > > > > this test can be merged with > > `MasterAuthorizationTest.UnauthorizedToStaticallyReserveResources` and you > > save a restart of a master. You mean because the ACLs are set up the same way, we can have one test with two scenarios, agent1 unable to register for reason1, agent2 able to register for reason2? I think that's OK but in this case it's easier to understand the two cases individually with smaller and independent tests? - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64515/#review193670 ----------------------------------------------------------- On Jan. 3, 2018, 11 a.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64515/ > ----------------------------------------------------------- > > (Updated Jan. 3, 2018, 11 a.m.) > > > Review request for mesos, Alexander Rojas and James Peach. > > > Bugs: MESOS-8306 > https://issues.apache.org/jira/browse/MESOS-8306 > > > Repository: mesos > > > Description > ------- > > Used `reserve_resources` ACL for static reservations. > > > Diffs > ----- > > include/mesos/authorizer/acls.proto > d869820491f1b81f81e1157493aa73e32735b9c3 > src/master/master.hpp 8fe9420dbe03ea2cefc6a40b0f64284aa9fe7915 > src/master/master.cpp 282fdf8ac38e3613c621c1c8e5c50f27bde9dafd > src/tests/master_authorization_tests.cpp > 676543a5ad1bb5d47011fc2a8b05dfaaeef18c64 > > > Diff: https://reviews.apache.org/r/64515/diff/3/ > > > Testing > ------- > > make check. > > > Thanks, > > Jiang Yan Xu > >
