> On June 2, 2017, 4:16 a.m., Greg Mann wrote: > > src/master/http.cpp > > Lines 4778 (patched) > > <https://reviews.apache.org/r/59101/diff/4/?file=1731542#file1731542line4778> > > > > 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.
I'm in for returning a warning, however all the filtering endpoints assume `false` in the case of error. Examples: [`approveViewExecutorInfo`](https://github.com/apache/mesos/blob/master/src/common/http.cpp#L830), [`approveViewFrameworkInfo`](https://github.com/apache/mesos/blob/master/src/common/http.cpp#L812), [`approveViewTaskInfo`](https://github.com/apache/mesos/blob/master/src/common/http.cpp#L850), [`approveViewTask`](https://github.com/apache/mesos/blob/master/src/common/http.cpp#L869), [`approveViewFlags`](https://github.com/apache/mesos/blob/master/src/common/http.cpp#L888), [`approveViewRole`](https://github.com/apache/mesos/blob/master/src/common/http.cpp#L945) - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59101/#review176722 ----------------------------------------------------------- On June 1, 2017, 4: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, 4: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 > >
