> 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()`.
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. > On Sept. 19, 2017, 10:38 p.m., Gastón Kleiman wrote: > > src/tests/slave_authorization_tests.cpp > > Line 1266 (original), 1266-1268 (patched) > > <https://reviews.apache.org/r/61925/diff/2/?file=1815902#file1815902line1270> > > > > do we need `containers.json` too? I don't think `containers.json` is a valid route? - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61925/#review185728 ----------------------------------------------------------- On Sept. 14, 2017, 1:50 a.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61925/ > ----------------------------------------------------------- > > (Updated Sept. 14, 2017, 1:50 a.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 > 30eceae0920351cffc9c3393c6d08917c4041c1a > > > Diff: https://reviews.apache.org/r/61925/diff/2/ > > > Testing > ------- > > `make check` > > > Thanks, > > Greg Mann > >
