----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38000/#review110006 -----------------------------------------------------------
Ship it! I'll make the changes below in order to avoid more round trips on reviewboard. The biggest change was related to simplifying the pipelining code between the http proxy and the process manager, hope you like it better now! Take a look over the comments, let me know if you think we should follow up on anything: ``` commit 6bd9b65b9d93f154ff9187fb5e5136262ede043c Author: Alexander Rojas <[email protected]> Date: Fri Dec 11 12:30:37 2015 -0800 Allow users of libprocess to perform authentication of HTTP requests. This change integrates the previously added AuthenticationRouter into libprocess in order to allow users to authenticate HTTP requests. Now, when making a 'route', a security 'realm' can be specified. The user of libprocess determines which Authenticator will be used for each 'realm'. Review: https://reviews.apache.org/r/38000 ``` 3rdparty/libprocess/include/process/event.hpp (line 121) <https://reviews.apache.org/r/38000/#comment169838> const ref here 3rdparty/libprocess/include/process/process.hpp (lines 237 - 238) <https://reviews.apache.org/r/38000/#comment169759> One newline here :) 3rdparty/libprocess/src/authentication_router.hpp (lines 52 - 53) <https://reviews.apache.org/r/38000/#comment169760> "... rathern than having the authentication router maintain the mapping" 3rdparty/libprocess/src/process.cpp (lines 238 - 256) <https://reviews.apache.org/r/38000/#comment169783> Hm.. it looks like the complexity here is coming from the fact that we currently use the router outside of the HttpProxy and we wanted to pipeline per-UPID. We can do a big simplification here by using the router in the http proxy itself and punting on the per-UPID pipelining in favor of per-socket pipelining, since that was just a performance optimization (not worth the complexity cost after seeing how the code turned out). We end up with the following in the proxy (comments omitted): ``` HttpProxy { Future<Option<AuthenticationResult>> authenticate(const Request& request) { Future<Option<AuthenticationResult>> authentication = authentication_router->authenticate(request); return authentications.add<Option<AuthenticationResult>>( [=]() { return authentication; }); } Sequence authentications; } ``` And we don't have to have any cleanup code. Then the process manager code looks like: ``` dispatch(proxy, &HttpProxy::authenticate, *request) .onAny([this, request, promise, receiver]( const Future<Option<AuthenticationResult>>& authentication) { ``` Which looks a lot better! :) 3rdparty/libprocess/src/process.cpp (line 2485) <https://reviews.apache.org/r/38000/#comment169803> Should we s/authorized/authenticated/ here since that's really what this means? 3rdparty/libprocess/src/process.cpp (line 2498) <https://reviews.apache.org/r/38000/#comment169802> Double // here? 3rdparty/libprocess/src/process.cpp (line 3367) <https://reviews.apache.org/r/38000/#comment169839> We probably need a little comment here and in removeEndpoint that we don't need to wait for the operations to complete. 3rdparty/libprocess/src/tests/http_tests.cpp (lines 51 - 54) <https://reviews.apache.org/r/38000/#comment169807> Why did you introduce these when all of the other tests are still using the http:: prefix? This makes it really inconsistent: for example, if I look at the mock method introduced to HttpProcess, it is the only one without the http qualifiers. 3rdparty/libprocess/src/tests/http_tests.cpp (line 76) <https://reviews.apache.org/r/38000/#comment169809> Let's just use "real" inline to keep it simple, no need to introduce a global :) 3rdparty/libprocess/src/tests/http_tests.cpp (lines 105 - 109) <https://reviews.apache.org/r/38000/#comment169811> At this point we would just wrap with the open paren, but this fits on one line when we just use "realm" instead of the global. 3rdparty/libprocess/src/tests/http_tests.cpp (line 1219) <https://reviews.apache.org/r/38000/#comment169814> Why call this 'addAuthenticator' instead of 'setAuthenticator' to match the interface we're wrapping? 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1227 - 1229) <https://reviews.apache.org/r/38000/#comment169836> We actually have to wait here, otherwise we can trigger the leak of a mock object if we exit after leaving tear down but before the unset takes place, leading to: ``` ../../../3rdparty/libprocess/src/tests/http_tests.cpp:1334: ERROR: this mock object (used in test HttpAuthenticationTest.Pipelining) should be deleted but never is. Its address is @0x7feca1603300. ``` This means we'll actually have to expose the Futures up on the set/unset functions. That makes sense anyway since the callers will potentially want to ensure that authentication is set up before they start setting up routes. 3rdparty/libprocess/src/tests/http_tests.cpp (line 1232) <https://reviews.apache.org/r/38000/#comment169815> How about s/registeredRealms/realms/ ? Also should this be private? 3rdparty/libprocess/src/tests/http_tests.cpp (line 1238) <https://reviews.apache.org/r/38000/#comment169825> Hm.. didn't I already comment on not repeating HttpAuthentication in both the test fixture and test case name here? 3rdparty/libprocess/src/tests/http_tests.cpp (line 1245) <https://reviews.apache.org/r/38000/#comment169820> What does a disabled endpoint mean? 3rdparty/libprocess/src/tests/http_tests.cpp (line 1255) <https://reviews.apache.org/r/38000/#comment169826> This looks to be more about testing Unauthorized, since we're not exercising any credential checking code? Should we also have a test for Forbidden? 3rdparty/libprocess/src/tests/http_tests.cpp (line 1257) <https://reviews.apache.org/r/38000/#comment169817> We haven't really been using auto for cases like this (also not super intuitive whether we should have used `auto` or `auto*` here..) Ditto elsewhere 3rdparty/libprocess/src/tests/http_tests.cpp (line 1264) <https://reviews.apache.org/r/38000/#comment169821> Hm.. I thought I already commented on avoiding "auth" abbreviations in previous reviews? 3rdparty/libprocess/src/tests/http_tests.cpp (line 1271) <https://reviews.apache.org/r/38000/#comment169824> What does disabled endpoint mean here..? 3rdparty/libprocess/src/tests/http_tests.cpp (line 1284) <https://reviews.apache.org/r/38000/#comment169828> It's a little confusing that this is talking about credentials when they are irrelevant to the test, maybe we shouldn't mention them here especially since we already have a comment saying they are ignored. 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1302 - 1305) <https://reviews.apache.org/r/38000/#comment169831> Why don't we just remove this to avoid confusion and say: ``` // Note that we don't bother pretending to specify a valid // 'Authorization' header since we force authentication success. ``` 3rdparty/libprocess/src/tests/http_tests.cpp (line 1318) <https://reviews.apache.org/r/38000/#comment169832> Hm.. how about s/DelayedAuthentication/Pipelining/ to tell the reader about what we're interested in testing here (pipelining), not how we're testing it (by delaying authentication). 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1321 - 1323) <https://reviews.apache.org/r/38000/#comment169819> Why the indirection with the global rather than the following straightforward line? ``` addAuthenticator("realm", Owned<Authenticator>(authenticator)); ``` 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1339 - 1340) <https://reviews.apache.org/r/38000/#comment169835> To avoid ordering assumptions here when we go to check the options below, let's use FutureArg and replace EXPECT_EQ with AWAIT_EXPECT_EQ below 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1353 - 1357) <https://reviews.apache.org/r/38000/#comment169833> Ditto here about not bothering with this, and leaving a comment: ``` // Note that we don't bother pretending to specify a valid // 'Authorization' header since we force authentication success. ``` 3rdparty/libprocess/src/tests/http_tests.cpp (line 1368) <https://reviews.apache.org/r/38000/#comment169834> How about s/Result// ? - Ben Mahler On Dec. 11, 2015, 2:01 p.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38000/ > ----------------------------------------------------------- > > (Updated Dec. 11, 2015, 2:01 p.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till > Toenshoff. > > > Bugs: MESOS-3233 > https://issues.apache.org/jira/browse/MESOS-3233 > > > Repository: mesos > > > Description > ------- > > Adds functions which allow libprocess users to register HTTP authenticators. > Overloads `ProcesBase::route()` to allow for registering of authenticating > endpoints. > Includes tests. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/event.hpp > a03824c061c4a0eb865b163999a763635e56744c > 3rdparty/libprocess/include/process/http.hpp > c9e38e5a382f29f87a2ed62602cae3d62f8e16cc > 3rdparty/libprocess/include/process/process.hpp > 81c094414d4d5ac5eb593df2a6d14aaacb19a826 > 3rdparty/libprocess/src/authentication_router.hpp > 5777deafd7420324627802a7ab9da9aaa2b46825 > 3rdparty/libprocess/src/process.cpp > e93709d3bb4ac588457bb9331fc05ec5ab539f6d > 3rdparty/libprocess/src/tests/http_tests.cpp > 2de75ca1c7e224c36b534c368e7379dc158aa5bb > > Diff: https://reviews.apache.org/r/38000/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
