> On Dec. 1, 2015, 2:24 p.m., Michael Park wrote: > > src/master/master.cpp, line 2771 > > <https://reviews.apache.org/r/39987/diff/9/?file=1149276#file1149276line2771> > > > > Why is it that we need to perform validation within authorization? We > > perform `authorizeTask` with a `TaskInfo` that has not been validated yet, > > and take care of it later on. Does that pattern not work here? > > Greg Mann wrote: > That pattern would work here, and the original patches I posted worked > that way. We ended up switching to this solution because it makes for a > cleaner implementation and eliminates some redundant code. However, it does > come at the cost of making error messaging more difficult (as you noted in > another patch) and decreasing the separation of functionality that can make > debugging easier. I don't have a strong preference for either path at the > moment. Perhaps Jie could chime in with his thoughts? > > Jie Yu wrote: > I looked at the code again. I guess the reason we validate the operation > before authorization is because we want to get all the principals from > resources in UNRESERVE, and we want to make sure resources in UNRESERVE > operation is valid. As MPark pointed out, coupling authorization with > validation does cause some confusion when generating error messages. How > about the following: > > 1) we don't perform validation in authorization. In > authorizeUnreserveResources, when we iterate all resources, we can do the > following as you previosly did (sorry about the back and forth, my bad). > ``` > foreach (const Resource& resource, unreserve.resources()) { > if (Resources::isDynamicallyReserved(resource)) { > request.mutable_reserver_principals()->add_values( > resource.reservation().principal()); > } > } > ``` > > 2) in Http::reserve and Http::unreserve, we still perform validation > first before calling master->authroizeXXX. > > 3) in `Master::_accept`, we perform operation validation on > RESERVE/UNRESERVE. > > Let me know if that's better or not? > > Greg Mann wrote: > No worries, Jie! I learned about `Future<>::repair()` in the process so I > consider it worthwhile :-) > > I have one question about your plan: for `LAUNCH` operations, tasks are > authorized before they get validated, and I'm wondering if it's worth making > this part of the interface consistent across all types of operations, i.e. > perhaps we should change the code for `LAUNCH` to validate before > authorization as well. If we decide to do this, I could do it here or in > another review. > > Greg Mann wrote: > Since the validation for `LAUNCH` needs to be aware of tasks that were > previously launched on the same operation, it makes sense in that case for > validation to occur after authorization. However, in general, I think it > makes more sense for the operation to be validated before authorization, so > I'll follow the pattern that Jie suggested above for > `RESERVE/UNRESERVE/CREATE/DESTROY`. This means that the interface will have > slightly different expectations regarding validation depending on what type > of operation is being performed. > > An alternative would be to validate all operations *after* authorization > is performed, which in general seems less intuitive to me, but would keep the > interface consistent... > > Jie Yu wrote: > Greg, the code sample I suggested above actually tried to keep the > interface consistent (validation is done separately and is decoupled from > authorization). Just want to double check with you. > > Greg Mann wrote: > Thanks for checking Jie, I think I misread your comments previously. > > Your suggested solution would keep the separation of validation and > authorization consistent, the difference is that for HTTP endpoint operations > validation would be done *before* authorization, while for framework > operations validation would be done *after* authorization. I guess the > question is, do we think it's worthwhile to establish a guideline like "you > can always assume an operation has been validated before the authorization > function is called" or vice-versa. Maybe it's inconsequential, since as the > code exists currently it doesn't really matter in what order the two > operations are performed.
Aha, now I get it. Thanks for explaining! Yeah, I think I am fine with either way. You can swap the order in http endpoint if the change is easy. Otherwise, I would probably just leave it as it is. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39987/#review108496 ----------------------------------------------------------- On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39987/ > ----------------------------------------------------------- > > (Updated Nov. 30, 2015, 8:35 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 > >
