----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59101/#review176722 -----------------------------------------------------------
Could you update the description to reflect the fact that this patch adds authZ to both the v0 and v1 calls? In other patches as well, if necessary. src/master/http.cpp Lines 4714 (patched) <https://reviews.apache.org/r/59101/#comment250163> I think a comma after "none" makes this a bit more readable: "If none, an empty response will be returned." src/master/http.cpp Lines 4732-4733 (original), 4744-4753 (patched) <https://reviews.apache.org/r/59101/#comment250164> Ditto here regarding using `defer`. Also, I think you could chain this together to avoid nested lambdas: ``` return approver .then(defer( master->self(), [this](const Owned<ObjectApprover>& approver) -> Future<Response> { return _getMaintenanceStatus(approver) })) .then([request](const mesos::maintenance::ClusterStatus& status) -> Future<Response> { return OK(JSON::protobuf(status), request.url.query.get("jsonp")); }); ``` src/master/http.cpp Lines 4778 (patched) <https://reviews.apache.org/r/59101/#comment250165> In the agent's HTTP handlers, when `ObjectApprover::approved` returns an error, the handler returns a `Failure`. With this code as-is, if the authorizer is in a bad state and all `approved` calls return errors, the client would get a successful response with an empty body. Is that the behavior we want? I would expect that in the case where all `approved` calls return errors, the handler would return a `Failure`. I'm not sure about the case where only some `approved` calls return errors. Looking at the handler for '/containers' on the agent, we simply log a warning if authorization for one of the containers returns an error, so I think that endpoint may return a successful result when all `approved` calls return errors. I think at the very least, we should log a warning when `approved` returns an error, and we should consider if there's a good way to return an unsuccessful response to the client when all authorizations fail. src/master/http.cpp Lines 4803-4810 (original), 4838-4855 (patched) <https://reviews.apache.org/r/59101/#comment250166> Ditto here regarding using `defer` and chaining callbacks to avoid the nested lambdas. src/tests/api_tests.cpp Line 1299 (original), 1299 (patched) <https://reviews.apache.org/r/59101/#comment250168> Do you think we should update the v0 endpoint tests as well? src/tests/api_tests.cpp Lines 1365-1371 (patched) <https://reviews.apache.org/r/59101/#comment250167> Ditto here regarding `AWAIT_EXPECT_RESPONSE_STATUS_EQ`. - Greg Mann On June 1, 2017, 2:57 p.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59101/ > ----------------------------------------------------------- > > (Updated June 1, 2017, 2:57 p.m.) > > > Review request for mesos, Adam B, Greg Mann, and Till Toenshoff. > > > Bugs: MESOS-7415 > https://issues.apache.org/jira/browse/MESOS-7415 > > > Repository: mesos > > > Description > ------- > > Enables the use of authorization for the `GET_MAINTENANCE_STATUS` > v1 API call, using the ACL `GetMaintenanceStatus` and the action > of the same name as the API call. > > It also updates the ApiTests to take into account the authorization > case. > > > Diffs > ----- > > src/master/http.cpp 7060b8fa53e0682681c50e051908ffbbf50fb7da > src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 > src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09 > > > Diff: https://reviews.apache.org/r/59101/diff/4/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
