> On Dec. 2, 2015, 4:02 p.m., Alexander Rukletsov wrote: > > src/master/master.cpp, lines 2769-2770 > > <https://reviews.apache.org/r/39987/diff/10/?file=1150202#file1150202line2769> > > > > Is my understanding is correct, we do not allow reserving resources > > neither for operators nor for frameworks without a principal. Why do we > > have a path for `ACL::Entity::Any` here then? If you plan to reserve it for > > future use cases or just be more general — it's fine, but let's leave a fat > > comment that this is not possible currently (or how it's possible if my > > understanding is not correct). > > > > A follow-up question: Is it something we have agreed and maybe even > > documented somewhere, that certain actions require principal? I'm thinking > > about authz for quota and whether we have to make `principal` required > > there.
Yea this is an artifact of doing validation after authorization. We're doing it in that order here to maintain consistency with the order for LAUNCH operations, which must validate after authorization because their validation requires knowledge of the tasks that may have already been launched within a particular operation. You're right that if `!principal.isSome()` here, we know that this operation will fail. The validation routine will catch that and return the proper error, but how best to handle it here? In order to ensure that `validate()` is able to report the correct error, perhaps we should just `return true;` when `!principal.isSome()`. This is less than ideal, but perhaps would suffice along with a fat comment, as you suggested :-) - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39987/#review108655 ----------------------------------------------------------- On Dec. 2, 2015, 6 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39987/ > ----------------------------------------------------------- > > (Updated Dec. 2, 2015, 6 p.m.) > > > Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff. > > > Bugs: MESOS-3062 > https://issues.apache.org/jira/browse/MESOS-3062 > > > Repository: mesos > > > Description > ------- > > Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve. > Note: this review is continued from https://reviews.apache.org/r/37125/ > > > Diffs > ----- > > src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe > src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 > > Diff: https://reviews.apache.org/r/39987/diff/ > > > Testing > ------- > > This is the third in a chain of 5 patches. `make check` was used to test > after all patches were applied. > > > Thanks, > > Greg Mann > >
