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 64515: Used `reserve_resources` ACL for static reservations.
Date Wed, 03 Jan 2018 19:01:08 GMT


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


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