----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55887/#review165999 -----------------------------------------------------------
src/slave/slave.cpp (line 1662) <https://reviews.apache.org/r/55887/#comment237880> Although I can tell what `transaction` means as an analogy here, perhaps just saying "because they are either a single task or tasks in a single task group" is more direct? src/slave/slave.cpp (line 1663) <https://reviews.apache.org/r/55887/#comment237920> We can also comment that ``` Note that we only report the first error we encounter for the task group. ``` src/slave/slave.cpp (lines 1669 - 1670) <https://reviews.apache.org/r/55887/#comment237899> We have already said "If any of the authorizations fails, we fail all the tasks" above. src/slave/slave.cpp (line 1671) <https://reviews.apache.org/r/55887/#comment237895> In this case I think we don't need an `await`, we can use a `collect` because of the "failing all tasks when one authorization fails" semantics. src/slave/slave.cpp (line 1689) <https://reviews.apache.org/r/55887/#comment237913> s/taskUser/user/ would be shorter but still sufficient? src/slave/slave.cpp (line 1706) <https://reviews.apache.org/r/55887/#comment237919> s/Status Update/status update/ src/slave/slave.cpp (lines 1710 - 1711) <https://reviews.apache.org/r/55887/#comment237917> Two-space indentation. src/slave/slave.cpp (line 1713) <https://reviews.apache.org/r/55887/#comment237918> s/tasks(s) // because it's already in `taskOrTaskGroup`? Also s/WARNING/ERROR/ because this is actually an error, if we do log it. src/slave/slave.cpp (line 1722) <https://reviews.apache.org/r/55887/#comment237938> Why not TASK_ERROR? src/slave/slave.cpp (lines 1735 - 1739) <https://reviews.apache.org/r/55887/#comment237897> The way it is written, `Slave::run()` (and the slave actor) is going to be blocked by `authorized.get()` which is not good. We should introduce a continuation method `_run` to put the rest of the method and contents in the lambda above and rename the current `_run` as `__run`. Then at the end of run(): ``` collect(authorizations) .onAny(defer(self(), &Self::_run, ...)); ``` src/slave/slave.cpp (line 6161) <https://reviews.apache.org/r/55887/#comment237898> This method is identitcal to the one on the master and at first glance people would probably wonder why this is done again on the agent. We could use the space to explain why we do the identitcal thing here for run tasks while not for other things. The reason as I see is that 1. In general we shouldn't need to re-authorize actions that have already been authorized by the master. To prevent a client from spoofing as the master and exploiting the agent, the agent should authenticate the client as the master. 2. We specially re-authorize the `RUN_TASK` action even though the same ACL is on the master because a) in cases where hosts have heterogeneous user-account configurations, it makes sense to set the ACL on the agent instead of on the master; 2) compared to other actions such as killing a task and shutting down a framework, it's a greater security risk if malicious tasks are launched as a superuser on the agent. - Jiang Yan Xu On Feb. 7, 2017, 7:53 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55887/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2017, 7:53 p.m.) > > > Review request for mesos, Adam B, Alexander Rojas, and Jiang Yan Xu. > > > Bugs: MESOS-6953 > https://issues.apache.org/jira/browse/MESOS-6953 > > > Repository: mesos > > > Description > ------- > > Added support for action `run_tasks` on the agent's flag `acl`. Based on > the ACL configured for `run_tasks`, a task to be launched on the agent > will be (dis)allowed to launch on the agent. > If a task or task group cannot be launched due to failed authorization, > a `TASK_FAILED` Status Update shall be sent with a reason code of > `REASON_TASK_UNAUTHORIZED` or `REASON_TASK_GROUP_UNAUTHORIZED` as > applicable. > Note that in case of a task group, all tasks fail if any of the tasks > within the task group encounter the authorization error. > > > Diffs > ----- > > src/slave/slave.hpp 5049eb783b8ad7b9599f20c3701f7d3d654b4491 > src/slave/slave.cpp 92564ff8fff06d1cb17192d374d355b4bb7d39d8 > > Diff: https://reviews.apache.org/r/55887/diff/ > > > Testing > ------- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
