> On Sept. 19, 2017, 10:38 p.m., Gastón Kleiman wrote: > > src/slave/http.cpp > > Lines 1951-1961 (original), 1868-1879 (patched) > > <https://reviews.apache.org/r/61925/diff/2/?file=1815901#file1815901line1958> > > > > Why do you prefer to reimplement part of `authorizeEndpoint()` here? > > > > If we do this, we might want to log the same thing that method logs: > > > > ``` > > LOG(INFO) << "Authorizing principal '" > > << (principal.isSome() ? stringify(principal.get()) : "ANY") > > << "' to " << method > > << " the '" << endpoint << "' endpoint"; > > ``` > > > > And we should also update `Http::containers()`. > > Greg Mann wrote: > If we switch to using the `AuthorizationAcceptor` exclusively, then we'll > be able to remove the authorization helpers like `authorizeEndpoint()` > entirely. In the meantime, there will be some code duplication. Seem > reasonable? > > Yep, looks like I missed `containers()`, will update.
Yea I think we could add some debug logging to the `AuthorizationAcceptor`. However, I'd say `INFO` is overkill. `VLOG(1)` should be enough I think. I'll follow-up with a patch to add more logging. - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61925/#review185728 ----------------------------------------------------------- On Sept. 21, 2017, 4:52 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61925/ > ----------------------------------------------------------- > > (Updated Sept. 21, 2017, 4:52 p.m.) > > > Review request for mesos, Anand Mazumdar, Alexander Rojas, and Gastón Kleiman. > > > Bugs: MESOS-7914 > https://issues.apache.org/jira/browse/MESOS-7914 > > > Repository: mesos > > > Description > ------- > > This patch updates the agent endpoint handlers to make use of > the `AuthorizationAcceptor` exclusively for authorization, > eliminating the need to explicitly create authorization > subjects and objects. > > Endpoint-related slave authorization tests are also updated to > accommodate this change. > > > Diffs > ----- > > src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b > src/slave/http.cpp 3ea7829df8c1c35d2fa3a44f19a60b7e261042ce > src/tests/slave_authorization_tests.cpp > 4c7d37fd14c8f3e7a52d35fb685fb8f05cba1e70 > > > Diff: https://reviews.apache.org/r/61925/diff/3/ > > > Testing > ------- > > `make check` > > > Thanks, > > Greg Mann > >
