You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rojas <al...@mesosphere.io> on 2015/11/18 11:43:57 UTC
Re: Review Request 37999: Introduced an Authenticator interface and an
AuthenticationRouter in libprocess.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/
-----------------------------------------------------------
(Updated Nov. 18, 2015, 11:43 a.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Updated summary and description.
Summary (updated)
-----------------
Introduced an Authenticator interface and an AuthenticationRouter in libprocess.
Bugs: MESOS-3231
https://issues.apache.org/jira/browse/MESOS-3231
Repository: mesos
Description (updated)
-------
The Authenticator interface allows us to implement different
authenticators based on the scheme (e.g. Basic, Digest, SPNEGO).
The AuthenticationRouter manages the authentication realms and
the mapping from endpoints to realms. It is then used by the
ProcessManager to route requests to the authenticator for the
realm, if applicable.
Diffs
-----
3rdparty/libprocess/Makefile.am cdefa37528ea69422978a9772f955042e882dde4
3rdparty/libprocess/include/Makefile.am e6be2c4db121585bbe7f3c0627de048adc7cfb2c
3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
3rdparty/libprocess/include/process/event.hpp 28ce1928877084f0e1a73fdad789224c86e53f46
3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
3rdparty/libprocess/src/CMakeLists.txt fb9bd04832779ac43151f2feb3dfbf58cb996434
3rdparty/libprocess/src/authentication_router.hpp PRE-CREATION
3rdparty/libprocess/src/authentication_router.cpp PRE-CREATION
3rdparty/libprocess/src/process.cpp 7abdf21a5784920251c3627f9820c12fdc356c6e
3rdparty/libprocess/src/process_reference.hpp e6110bba2a54948be68e58ab9de988565b7d95a8
Diff: https://reviews.apache.org/r/37999/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 37999: Introduced an Authenticator interface and an
AuthenticationRouter in libprocess.
Posted by Ben Mahler <be...@gmail.com>.
> On Nov. 20, 2015, 2 p.m., Ben Mahler wrote:
> > For transparency we pulled out the libprocess integration because we realized that requests sent to the authentication router need to have authentication results satisfied in the same order in which the requests were sent. We're still thinking through how to solve this within libprocess, so for now we're just going to commit these interfaces (we don't expect these interfaces to change further for the MVP).
> >
> > I will update the description in the commit to reflect that this no longer includes the ProcessManager integration.
We're also pulling out the remaining changes to event.hpp and process.cpp in this patch, as they will make more sense alongside the subsequent integration patches.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review107365
-----------------------------------------------------------
On Nov. 20, 2015, 12:53 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2015, 12:53 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-3231
> https://issues.apache.org/jira/browse/MESOS-3231
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Authenticator interface allows us to implement different
> authenticators based on the scheme (e.g. Basic, Digest, SPNEGO).
> The AuthenticationRouter manages the authentication realms and
> the mapping from endpoints to realms. It is then used by the
> ProcessManager to route requests to the authenticator for the
> realm, if applicable.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am cdefa37528ea69422978a9772f955042e882dde4
> 3rdparty/libprocess/include/Makefile.am e6be2c4db121585bbe7f3c0627de048adc7cfb2c
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 60c615281f3810230bf6c17866f46eaa6855ca29
> 3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
> 3rdparty/libprocess/src/CMakeLists.txt fb9bd04832779ac43151f2feb3dfbf58cb996434
> 3rdparty/libprocess/src/authentication_router.hpp PRE-CREATION
> 3rdparty/libprocess/src/authentication_router.cpp PRE-CREATION
> 3rdparty/libprocess/src/process.cpp 7abdf21a5784920251c3627f9820c12fdc356c6e
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37999: Introduced an Authenticator interface and an
AuthenticationRouter in libprocess.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review107365
-----------------------------------------------------------
Ship it!
For transparency we pulled out the libprocess integration because we realized that requests sent to the authentication router need to have authentication results satisfied in the same order in which the requests were sent. We're still thinking through how to solve this within libprocess, so for now we're just going to commit these interfaces (we don't expect these interfaces to change further for the MVP).
I will update the description in the commit to reflect that this no longer includes the ProcessManager integration.
- Ben Mahler
On Nov. 20, 2015, 12:53 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2015, 12:53 p.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-3231
> https://issues.apache.org/jira/browse/MESOS-3231
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Authenticator interface allows us to implement different
> authenticators based on the scheme (e.g. Basic, Digest, SPNEGO).
> The AuthenticationRouter manages the authentication realms and
> the mapping from endpoints to realms. It is then used by the
> ProcessManager to route requests to the authenticator for the
> realm, if applicable.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am cdefa37528ea69422978a9772f955042e882dde4
> 3rdparty/libprocess/include/Makefile.am e6be2c4db121585bbe7f3c0627de048adc7cfb2c
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 60c615281f3810230bf6c17866f46eaa6855ca29
> 3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
> 3rdparty/libprocess/src/CMakeLists.txt fb9bd04832779ac43151f2feb3dfbf58cb996434
> 3rdparty/libprocess/src/authentication_router.hpp PRE-CREATION
> 3rdparty/libprocess/src/authentication_router.cpp PRE-CREATION
> 3rdparty/libprocess/src/process.cpp 7abdf21a5784920251c3627f9820c12fdc356c6e
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37999: Introduced an Authenticator interface and an
AuthenticationRouter in libprocess.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/
-----------------------------------------------------------
(Updated Nov. 20, 2015, 1:53 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Removes actual authentication logic from `ProcessManager::handle()` until delivering pipelining to processes is cleared.
Bugs: MESOS-3231
https://issues.apache.org/jira/browse/MESOS-3231
Repository: mesos
Description
-------
The Authenticator interface allows us to implement different
authenticators based on the scheme (e.g. Basic, Digest, SPNEGO).
The AuthenticationRouter manages the authentication realms and
the mapping from endpoints to realms. It is then used by the
ProcessManager to route requests to the authenticator for the
realm, if applicable.
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am cdefa37528ea69422978a9772f955042e882dde4
3rdparty/libprocess/include/Makefile.am e6be2c4db121585bbe7f3c0627de048adc7cfb2c
3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
3rdparty/libprocess/include/process/event.hpp 60c615281f3810230bf6c17866f46eaa6855ca29
3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
3rdparty/libprocess/src/CMakeLists.txt fb9bd04832779ac43151f2feb3dfbf58cb996434
3rdparty/libprocess/src/authentication_router.hpp PRE-CREATION
3rdparty/libprocess/src/authentication_router.cpp PRE-CREATION
3rdparty/libprocess/src/process.cpp 7abdf21a5784920251c3627f9820c12fdc356c6e
Diff: https://reviews.apache.org/r/37999/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 37999: Introduced an Authenticator interface and an
AuthenticationRouter in libprocess.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review107003
-----------------------------------------------------------
Ship it!
Was great sitting down and going over all of this stuff with you over the past week, thanks for the patience! Just some final adjustments that we discussed based on pulling this down and getting ready to commit.
3rdparty/libprocess/include/process/authenticator.hpp (line 52)
<https://reviews.apache.org/r/37999/#comment165889>
Let's maybe say here that this allows us to implement different authenticators based on the scheme and include the examples below (Basic, Digest, SPNEGO, etc).
3rdparty/libprocess/include/process/authenticator.hpp (line 65)
<https://reviews.apache.org/r/37999/#comment165883>
Whoops, stale now that you have the namespace.
3rdparty/libprocess/include/process/http.hpp (lines 545 - 552)
<https://reviews.apache.org/r/37999/#comment165890>
As we discussed, should we have a TODO to remove these?
3rdparty/libprocess/src/authentication_router.hpp (lines 35 - 37)
<https://reviews.apache.org/r/37999/#comment165891>
A little bit stale now that we called this router instead of realm manager, right?
3rdparty/libprocess/src/authentication_router.hpp (line 48)
<https://reviews.apache.org/r/37999/#comment165896>
How about unset for symettry? I see why you called this 'set' and 'unset' and that does describe the single authenticator per-realm constraint. Thinking over this again though, let's also add a small comment to describe this constraint.
3rdparty/libprocess/src/authentication_router.hpp (line 57)
<https://reviews.apache.org/r/37999/#comment165897>
Whoops, "either" here is orphaned.
3rdparty/libprocess/src/authentication_router.hpp (line 69)
<https://reviews.apache.org/r/37999/#comment165881>
Whoops, stale comment.
3rdparty/libprocess/src/authentication_router.cpp (line 96)
<https://reviews.apache.org/r/37999/#comment165910>
Hm.. perhaps we also need something here to mention that we're assuming absolute paths, which is ensured by libprocess. Should consider maybe returning a failure here when a relative path is detected.
3rdparty/libprocess/src/authentication_router.cpp (line 107)
<https://reviews.apache.org/r/37999/#comment165911>
Whoops, should have a space before the brace here.
3rdparty/libprocess/src/authentication_router.cpp (line 111)
<https://reviews.apache.org/r/37999/#comment165913>
Perhaps we should handle the "error" logging case first, as that tends to be how we structure the code and we can avoid an else more clearly
3rdparty/libprocess/src/authentication_router.cpp (lines 114 - 116)
<https://reviews.apache.org/r/37999/#comment165912>
How about we validate that the Result is valid before we send it up to libprocess? We can CHECK validity in libprocess since we control the code here, and avoid having to do validity checking (which we weren't really doing currently).
3rdparty/libprocess/src/authentication_router.cpp (line 136)
<https://reviews.apache.org/r/37999/#comment165916>
Why the 'if' here? 'process' should not be null here, unless there is a bug. We could add a check but for now I'll just remove the 'if' guard to be consistent with the rest of the process wrapper code FWICT.
3rdparty/libprocess/src/process.cpp (line 60)
<https://reviews.apache.org/r/37999/#comment165918>
What was this for? The result? Hm.. should be ok in this case to just include the router since we are only using signatures from the router.
3rdparty/libprocess/src/process.cpp (line 109)
<https://reviews.apache.org/r/37999/#comment165917>
Alphabetical?
3rdparty/libprocess/src/process.cpp (line 128)
<https://reviews.apache.org/r/37999/#comment165919>
We should consider just making this authentication::Router, but I'll leave it for now.
3rdparty/libprocess/src/process.cpp (line 953)
<https://reviews.apache.org/r/37999/#comment165920>
Even though there whould probably be just one comment above all these saying we're creating the globals, let's add another comment here to be consistent with the current code:
```
// Create the global HTTP authentication router.
authentication_router = new AuthenticationRouter();
```
3rdparty/libprocess/src/process_reference.hpp (line 52)
<https://reviews.apache.org/r/37999/#comment165882>
This should be in a separate patch, much like we did for the promise setting bug we noticed.
- Ben Mahler
On Nov. 18, 2015, 10:43 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Nov. 18, 2015, 10:43 a.m.)
>
>
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-3231
> https://issues.apache.org/jira/browse/MESOS-3231
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The Authenticator interface allows us to implement different
> authenticators based on the scheme (e.g. Basic, Digest, SPNEGO).
> The AuthenticationRouter manages the authentication realms and
> the mapping from endpoints to realms. It is then used by the
> ProcessManager to route requests to the authenticator for the
> realm, if applicable.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am cdefa37528ea69422978a9772f955042e882dde4
> 3rdparty/libprocess/include/Makefile.am e6be2c4db121585bbe7f3c0627de048adc7cfb2c
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 28ce1928877084f0e1a73fdad789224c86e53f46
> 3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
> 3rdparty/libprocess/src/CMakeLists.txt fb9bd04832779ac43151f2feb3dfbf58cb996434
> 3rdparty/libprocess/src/authentication_router.hpp PRE-CREATION
> 3rdparty/libprocess/src/authentication_router.cpp PRE-CREATION
> 3rdparty/libprocess/src/process.cpp 7abdf21a5784920251c3627f9820c12fdc356c6e
> 3rdparty/libprocess/src/process_reference.hpp e6110bba2a54948be68e58ab9de988565b7d95a8
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>