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/09/23 10:57:09 UTC
Re: Review Request 37999: Implemented http::AuthenticatorManager
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/
-----------------------------------------------------------
(Updated Sept. 23, 2015, 10:57 a.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
No longer WIP, opening to the community.
Summary (updated)
-----------------
Implemented http::AuthenticatorManager
Bugs: MESOS-3231
https://issues.apache.org/jira/browse/MESOS-3231
Repository: mesos
Description (updated)
-------
Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
Diffs
-----
3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
3rdparty/libprocess/include/process/http.hpp fbd6cf7967173495875a8ea90ed28ade88b982a2
3rdparty/libprocess/src/process.cpp 4afa30569b4d235637b49a624602e6b199c32e0e
Diff: https://reviews.apache.org/r/37999/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Alexander Rojas <al...@mesosphere.io>.
> On Oct. 21, 2015, 5:25 a.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 841-842
> > <https://reviews.apache.org/r/37999/diff/8/?file=1101615#file1101615line841>
> >
> > If an `authenticate`-request failed, the error message is never shown anywhere.
> >
> > I think we should add something that helps us debugging the authentication via logs.
> >
> > Something like this would possibly do:
> > ```
> > } else {
> > LOG(WARNING) << "Authentication failed: " << result.error();
> > }
> > ```
Using `VLOG` since we are in libprocess here.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review103347
-----------------------------------------------------------
On Oct. 20, 2015, noon, Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2015, noon)
>
>
> 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
> -------
>
> Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
>
> No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/include/process/http.hpp 591c1a959057155e1bf0f5bd73352e78d1c15cb3
> 3rdparty/libprocess/src/process.cpp 954d31424bc8f8ecfa78b80513c480f2514ec271
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review103347
-----------------------------------------------------------
3rdparty/libprocess/src/process.cpp (lines 841 - 842)
<https://reviews.apache.org/r/37999/#comment161360>
If an `authenticate`-request failed, the error message is never shown anywhere.
I think we should add something that helps us debugging the authentication via logs.
Something like this would possibly do:
```
} else {
LOG(WARNING) << "Authentication failed: " << result.error();
}
```
- Till Toenshoff
On Oct. 20, 2015, 10 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2015, 10 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
> -------
>
> Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
>
> No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/include/process/http.hpp 591c1a959057155e1bf0f5bd73352e78d1c15cb3
> 3rdparty/libprocess/src/process.cpp 954d31424bc8f8ecfa78b80513c480f2514ec271
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Alexander Rojas <al...@mesosphere.io>.
> On Nov. 6, 2015, 4:22 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/process.cpp, line 750
> > <https://reviews.apache.org/r/37999/diff/12/?file=1116827#file1116827line750>
> >
> > Shouldn't we push the realm as challenge here already?
> >
> > If we do this below only if all challenges are empty, we are missing the case where challenges are present some are and some are not.
>
> Alexander Rojas wrote:
> Well, as it is mentioned in the comment below either:
> 1. Authentication succeed, no need to add challenges, forward to the handler.
> 2. It is a multi-step authentication, the case is also handled in this block. If there is at least one multi-step, we only send the headers corresponding to them (and they come in the challenges part) in which case, challenges won't be null in the block below.
> 3. Authentication failed, we send all the challenges.
>
> So no, we don't push challenges here because we are still looking for at least one multi-step mechanism.
>
> Bernd Mathiske wrote:
> Just to clarify: so we are not expecting the user to switch to another still available scheme when there is a multi-step one? Why not advertise more choice to the user then?
First time he connects to the endpoint he gets all the options, but if he's in the middle of a multi-step it means that the user already started authentication and he already chose one. He will either fail (and get all the options) or succeed, in which case we don't need a new set.
I guess a user can change the authentication mechanism when he already started one… but I wouldn't trust such user anyway.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review105421
-----------------------------------------------------------
On Nov. 5, 2015, 6:07 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2015, 6:07 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
> -------
>
> Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
>
> No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Bernd Mathiske <be...@mesosphere.io>.
> On Nov. 6, 2015, 7:22 a.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/process.cpp, line 750
> > <https://reviews.apache.org/r/37999/diff/12/?file=1116827#file1116827line750>
> >
> > Shouldn't we push the realm as challenge here already?
> >
> > If we do this below only if all challenges are empty, we are missing the case where challenges are present some are and some are not.
>
> Alexander Rojas wrote:
> Well, as it is mentioned in the comment below either:
> 1. Authentication succeed, no need to add challenges, forward to the handler.
> 2. It is a multi-step authentication, the case is also handled in this block. If there is at least one multi-step, we only send the headers corresponding to them (and they come in the challenges part) in which case, challenges won't be null in the block below.
> 3. Authentication failed, we send all the challenges.
>
> So no, we don't push challenges here because we are still looking for at least one multi-step mechanism.
Just to clarify: so we are not expecting the user to switch to another still available scheme when there is a multi-step one? Why not advertise more choice to the user then?
- Bernd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review105421
-----------------------------------------------------------
On Nov. 5, 2015, 9:07 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2015, 9:07 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
> -------
>
> Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
>
> No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Alexander Rojas <al...@mesosphere.io>.
> On Nov. 6, 2015, 4:22 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/process.cpp, line 750
> > <https://reviews.apache.org/r/37999/diff/12/?file=1116827#file1116827line750>
> >
> > Shouldn't we push the realm as challenge here already?
> >
> > If we do this below only if all challenges are empty, we are missing the case where challenges are present some are and some are not.
Well, as it is mentioned in the comment below either:
1. Authentication succeed, no need to add challenges, forward to the handler.
2. It is a multi-step authentication, the case is also handled in this block. If there is at least one multi-step, we only send the headers corresponding to them (and they come in the challenges part) in which case, challenges won't be null in the block below.
3. Authentication failed, we send all the challenges.
So no, we don't push challenges here because we are still looking for at least one multi-step mechanism.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review105421
-----------------------------------------------------------
On Nov. 5, 2015, 6:07 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2015, 6:07 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
> -------
>
> Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
>
> No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review105421
-----------------------------------------------------------
3rdparty/libprocess/src/process.cpp (line 533)
<https://reviews.apache.org/r/37999/#comment164011>
Since we are in a cpp file, we can use "using std::unique_ptr" above and skip the qualifier "std::" here.
3rdparty/libprocess/src/process.cpp (line 696)
<https://reviews.apache.org/r/37999/#comment164013>
This seems to be a trick. Are we testing whether we are at "/" here? Ok, so you copied this code from somewhere else in the same file, but this is too obscure IMHO when written without any comment.
Please use something more obvious or comment on what this is doing.
3rdparty/libprocess/src/process.cpp (line 750)
<https://reviews.apache.org/r/37999/#comment164028>
Shouldn't we push the realm as challenge here already?
If we do this below only if all challenges are empty, we are missing the case where challenges are present some are and some are not.
3rdparty/libprocess/src/process.cpp (line 755)
<https://reviews.apache.org/r/37999/#comment164021>
s/on/one
3rdparty/libprocess/src/process.cpp (line 756)
<https://reviews.apache.org/r/37999/#comment164024>
s/was already/has already been
relevance to the present => use present perfect, not imperfect
3rdparty/libprocess/src/process.cpp (line 757)
<https://reviews.apache.org/r/37999/#comment164022>
s/executed/reached
- Bernd Mathiske
On Nov. 5, 2015, 9:07 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2015, 9:07 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
> -------
>
> Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
>
> No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> 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
>
>
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. 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: Implemented http::AuthenticatorManager
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. 18, 2015, 10:15 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
-------
Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
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 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: Implemented http::AuthenticatorManager
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. 17, 2015, 2:35 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Third round of reviews.
Bugs: MESOS-3231
https://issues.apache.org/jira/browse/MESOS-3231
Repository: mesos
Description
-------
Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
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 28ce1928877084f0e1a73fdad789224c86e53f46
3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
3rdparty/libprocess/src/CMakeLists.txt fb9bd04832779ac43151f2feb3dfbf58cb996434
3rdparty/libprocess/src/process.cpp 7abdf21a5784920251c3627f9820c12fdc356c6e
3rdparty/libprocess/src/realm_manager.hpp PRE-CREATION
3rdparty/libprocess/src/realm_manager.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/37999/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 37999: Implemented http::AuthenticatorManager
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. 16, 2015, 5:23 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Second round of reviews.
Bugs: MESOS-3231
https://issues.apache.org/jira/browse/MESOS-3231
Repository: mesos
Description
-------
Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
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 28ce1928877084f0e1a73fdad789224c86e53f46
3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
3rdparty/libprocess/src/CMakeLists.txt fb9bd04832779ac43151f2feb3dfbf58cb996434
3rdparty/libprocess/src/process.cpp 7abdf21a5784920251c3627f9820c12fdc356c6e
3rdparty/libprocess/src/realm_manager.hpp PRE-CREATION
3rdparty/libprocess/src/realm_manager.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/37999/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 37999: Implemented http::AuthenticatorManager
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. 16, 2015, 10:51 a.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Review changes.
Bugs: MESOS-3231
https://issues.apache.org/jira/browse/MESOS-3231
Repository: mesos
Description
-------
Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
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 28ce1928877084f0e1a73fdad789224c86e53f46
3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
3rdparty/libprocess/src/CMakeLists.txt fb9bd04832779ac43151f2feb3dfbf58cb996434
3rdparty/libprocess/src/authentication_manager.hpp PRE-CREATION
3rdparty/libprocess/src/authentication_manager.cpp PRE-CREATION
3rdparty/libprocess/src/process.cpp b45b5b14cc7868d73a6fe02c02b9baf8b44b891f
Diff: https://reviews.apache.org/r/37999/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 37999: Implemented http::AuthenticatorManager
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. 10, 2015, 4:59 a.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Rebase
Bugs: MESOS-3231
https://issues.apache.org/jira/browse/MESOS-3231
Repository: mesos
Description
-------
Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
Diffs (updated)
-----
3rdparty/libprocess/include/Makefile.am e6be2c4db121585bbe7f3c0627de048adc7cfb2c
3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
Diff: https://reviews.apache.org/r/37999/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 37999: Implemented http::AuthenticatorManager
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. 9, 2015, 11:29 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Changes after Bernd's review.
Bugs: MESOS-3231
https://issues.apache.org/jira/browse/MESOS-3231
Repository: mesos
Description
-------
Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
Diffs (updated)
-----
3rdparty/libprocess/include/Makefile.am e6be2c4db121585bbe7f3c0627de048adc7cfb2c
3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
Diff: https://reviews.apache.org/r/37999/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review105464
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/process.cpp (line 750)
<https://reviews.apache.org/r/37999/#comment164088>
s/advertise/advertised
- Bernd Mathiske
On Nov. 6, 2015, 9:07 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2015, 9:07 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
> -------
>
> Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
>
> No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37999: Implemented http::AuthenticatorManager
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. 6, 2015, 6:07 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Addresses issues in Bernd's review.
Bugs: MESOS-3231
https://issues.apache.org/jira/browse/MESOS-3231
Repository: mesos
Description
-------
Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
Diffs (updated)
-----
3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
Diff: https://reviews.apache.org/r/37999/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 37999: Implemented http::AuthenticatorManager
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. 5, 2015, 6:07 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
-------
Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
Diffs (updated)
-----
3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
Diff: https://reviews.apache.org/r/37999/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 37999: Implemented http::AuthenticatorManager
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. 4, 2015, 12:25 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Addresses Bernd's review issues.
Bugs: MESOS-3231
https://issues.apache.org/jira/browse/MESOS-3231
Repository: mesos
Description
-------
Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
Diffs (updated)
-----
3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
Diff: https://reviews.apache.org/r/37999/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/
-----------------------------------------------------------
(Updated Oct. 22, 2015, 2:57 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Rebase to account for changes in previous review.
Bugs: MESOS-3231
https://issues.apache.org/jira/browse/MESOS-3231
Repository: mesos
Description
-------
Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
Diffs (updated)
-----
3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
3rdparty/libprocess/include/process/http.hpp 591c1a959057155e1bf0f5bd73352e78d1c15cb3
3rdparty/libprocess/src/process.cpp 954d31424bc8f8ecfa78b80513c480f2514ec271
Diff: https://reviews.apache.org/r/37999/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Alexander Rojas <al...@mesosphere.io>.
> On Oct. 21, 2015, 1:46 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/process.cpp, line 885
> > <https://reviews.apache.org/r/37999/diff/9/?file=1102348#file1102348line885>
> >
> > Why are we doing this? Why isn't it a failure if there is no success and also no challenge? Please comment in the source code on this.
> >
> > My take is that if these authenticators would have liked to pose a challenge, they would already have done so.
Change the comment to:
```c++
// At this point on of the following is true:
// 1. Authentication succeeded, in which case the principal was
// already returned and this code is not executed.
// 2. It is a multi-step authentication protocol and challenges is
// not empty, because they are filled with steps.
// 3. Authentication failed for any reason, e.g. credentials were not
// provided in which case the authentication is reset to the first
// step.
```
Which makes clear why is this done here.
> On Oct. 21, 2015, 1:46 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/process.cpp, line 886
> > <https://reviews.apache.org/r/37999/diff/9/?file=1102348#file1102348line886>
> >
> > s/with those/those with
Comment changed completely.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review103387
-----------------------------------------------------------
On Nov. 4, 2015, 12:25 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 12:25 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
> -------
>
> Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
>
> No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review103387
-----------------------------------------------------------
3rdparty/libprocess/src/process.cpp (line 833)
<https://reviews.apache.org/r/37999/#comment161442>
s/Check the realm/Obtain the realms
3rdparty/libprocess/src/process.cpp (line 843)
<https://reviews.apache.org/r/37999/#comment161437>
s/ream's/realms'
3rdparty/libprocess/src/process.cpp (line 844)
<https://reviews.apache.org/r/37999/#comment161440>
Please add an s to this var name. list => plural. This will read better in the subsequent code. Give it a try!
3rdparty/libprocess/src/process.cpp (line 848)
<https://reviews.apache.org/r/37999/#comment161438>
Aligning the arguments of the foreach makes it easier to associated them with it instead of mistaking the second one for something new:
foreach (const std::unique_ptr<Authenticator>& authenticator,
authenticators_[realm]) {
3rdparty/libprocess/src/process.cpp (line 869)
<https://reviews.apache.org/r/37999/#comment161441>
Insert blank line.
3rdparty/libprocess/src/process.cpp (line 885)
<https://reviews.apache.org/r/37999/#comment161445>
Why are we doing this? Why isn't it a failure if there is no success and also no challenge? Please comment in the source code on this.
My take is that if these authenticators would have liked to pose a challenge, they would already have done so.
3rdparty/libprocess/src/process.cpp (line 886)
<https://reviews.apache.org/r/37999/#comment161444>
s/with those/those with
3rdparty/libprocess/src/process.cpp (line 2839)
<https://reviews.apache.org/r/37999/#comment161446>
Rather than a conditional expression and a complicated comment, let's break this up into clear if-then-else blocks with dedicated local comments.
- Bernd Mathiske
On Oct. 21, 2015, 3:01 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2015, 3:01 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
> -------
>
> Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
>
> No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/include/process/http.hpp 591c1a959057155e1bf0f5bd73352e78d1c15cb3
> 3rdparty/libprocess/src/process.cpp 954d31424bc8f8ecfa78b80513c480f2514ec271
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Alexander Rojas <al...@mesosphere.io>.
> On Oct. 21, 2015, 1:03 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/include/process/authenticator.hpp, line 52
> > <https://reviews.apache.org/r/37999/diff/9/?file=1102345#file1102345line52>
> >
> > Duplicate text.
I used a separate line because Doxygen uses the first line as a brief summary (It is also in our [style guide](https://github.com/apache/mesos/blob/c3940cd4da29eb6539096c6ec6dcab1a70993c42/docs/doxygen-style-guide.md#source-code-documentation-syntax)). However, given that I haven't seen the rendered result. I will remove this line.
> On Oct. 21, 2015, 1:03 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/include/process/authenticator.hpp, line 107
> > <https://reviews.apache.org/r/37999/diff/9/?file=1102345#file1102345line107>
> >
> > s/are/is (contents is singular)
> >
> > s/a HTTP/an HTTP
> >
> > (Please check all other places. If "a" is followed by "H" in an abbreviation, which is therefore pronounced "aytsh", this constitutes an "a" followed by a vowel sound, so it becomes "an)
Sorry, _contents_ is [plural](http://english.stackexchange.com/questions/13556/content-or-contents). Though there are different meanings of the word _content_.
> On Oct. 21, 2015, 1:03 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/process.cpp, line 793
> > <https://reviews.apache.org/r/37999/diff/9/?file=1102348#file1102348line793>
> >
> > Suggestion: break this up into two calls.
> >
> > removeAllAuthenticators()
> > removeAuthenticator(<param>)
I will let you decide this one with till, because he suggested the exact opposite (which has also been suggested by other reviewers), in his Sept. 10, 2015, 8:06 a.m. review (see above).
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review103383
-----------------------------------------------------------
On Nov. 4, 2015, 12:25 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 12:25 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
> -------
>
> Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
>
> No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Bernd Mathiske <be...@mesosphere.io>.
> On Oct. 21, 2015, 4:03 a.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/process.cpp, line 793
> > <https://reviews.apache.org/r/37999/diff/9/?file=1102348#file1102348line793>
> >
> > Suggestion: break this up into two calls.
> >
> > removeAllAuthenticators()
> > removeAuthenticator(<param>)
>
> Alexander Rojas wrote:
> I will let you decide this one with till, because he suggested the exact opposite (which has also been suggested by other reviewers), in his Sept. 10, 2015, 8:06 a.m. review (see above).
Till and I agreed on this:
clearAuthenticators();
removeAuthenticator(const &string realm);
Similar for removeEndpoint!
Why? For a single method with an option param you'd have to pick either singular or plural in the name and then it would be wrong in one of the two cases. Why clear and not removeAll? Because there is a lot of the former in the code base and none of the former.
- Bernd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review103383
-----------------------------------------------------------
On Nov. 4, 2015, 3:25 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 3:25 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
> -------
>
> Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
>
> No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a
> 3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review103383
-----------------------------------------------------------
3rdparty/libprocess/include/process/authenticator.hpp (line 52)
<https://reviews.apache.org/r/37999/#comment161405>
Duplicate text.
3rdparty/libprocess/include/process/authenticator.hpp (line 53)
<https://reviews.apache.org/r/37999/#comment161409>
Suggested rephrasing: "These parameters come in the form of a map<string, string>. Their interpretation depends on the specific authenticator instance that receives them. For example, a basic authenticator could expect mappings from user names to encrypted passwords."
3rdparty/libprocess/include/process/authenticator.hpp (line 69)
<https://reviews.apache.org/r/37999/#comment161411>
s/It/Its
better, simpler, replace these two sentences with:
"Possible outcomes:"
3rdparty/libprocess/include/process/authenticator.hpp (line 74)
<https://reviews.apache.org/r/37999/#comment161412>
s/possible/
3rdparty/libprocess/include/process/authenticator.hpp (line 77)
<https://reviews.apache.org/r/37999/#comment161413>
s/finish/finished
3rdparty/libprocess/include/process/authenticator.hpp (line 79)
<https://reviews.apache.org/r/37999/#comment161414>
s/instance of an
3rdparty/libprocess/include/process/authenticator.hpp (line 80)
<https://reviews.apache.org/r/37999/#comment161415>
s/The contents must be ready to be used/This will be used
3rdparty/libprocess/include/process/authenticator.hpp (line 86)
<https://reviews.apache.org/r/37999/#comment161416>
used? in what way? by whom?
Suggestion: "A standard HTTP authenticator will typically only read the 'WWW-Authenticate' header of the request."
3rdparty/libprocess/include/process/authenticator.hpp (line 98)
<https://reviews.apache.org/r/37999/#comment161417>
s/use/be used
s/a HTTP/an HTTP
3rdparty/libprocess/include/process/authenticator.hpp (line 107)
<https://reviews.apache.org/r/37999/#comment161419>
s/are/is (contents is singular)
s/a HTTP/an HTTP
(Please check all other places. If "a" is followed by "H" in an abbreviation, which is therefore pronounced "aytsh", this constitutes an "a" followed by a vowel sound, so it becomes "an)
3rdparty/libprocess/include/process/http.hpp (line 494)
<https://reviews.apache.org/r/37999/#comment161421>
s/for/with/
3rdparty/libprocess/src/process.cpp (line 662)
<https://reviews.apache.org/r/37999/#comment161426>
It may not be 100% clear to everybody what the parameters mean and what he method is supposed to accomplish. Probably better to explain here with a quick comment. (Instead of letting the implementation below become the specification of what might be intended.)
(No need to repeat such comments in the AuthenticatorProcess declaration.)
3rdparty/libprocess/src/process.cpp (line 665)
<https://reviews.apache.org/r/37999/#comment161425>
It is not immediately clear what this parameter does. Please write a comment for this method.
3rdparty/libprocess/src/process.cpp (line 686)
<https://reviews.apache.org/r/37999/#comment161428>
Please move this to line 667.
3rdparty/libprocess/src/process.cpp (line 691)
<https://reviews.apache.org/r/37999/#comment161431>
Please also document what the Future return type does.
3rdparty/libprocess/src/process.cpp (line 696)
<https://reviews.apache.org/r/37999/#comment161432>
s/provides a has/provide a hash
3rdparty/libprocess/src/process.cpp (line 698)
<https://reviews.apache.org/r/37999/#comment161433>
Insert a blank line, please!
3rdparty/libprocess/src/process.cpp (line 793)
<https://reviews.apache.org/r/37999/#comment161435>
Suggestion: break this up into two calls.
removeAllAuthenticators()
removeAuthenticator(<param>)
- Bernd Mathiske
On Oct. 21, 2015, 3:01 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2015, 3:01 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
> -------
>
> Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
>
> No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/include/process/http.hpp 591c1a959057155e1bf0f5bd73352e78d1c15cb3
> 3rdparty/libprocess/src/process.cpp 954d31424bc8f8ecfa78b80513c480f2514ec271
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/
-----------------------------------------------------------
(Updated Oct. 21, 2015, 12:01 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Addresses issues from Till's review.
Bugs: MESOS-3231
https://issues.apache.org/jira/browse/MESOS-3231
Repository: mesos
Description
-------
Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
Diffs (updated)
-----
3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
3rdparty/libprocess/include/process/http.hpp 591c1a959057155e1bf0f5bd73352e78d1c15cb3
3rdparty/libprocess/src/process.cpp 954d31424bc8f8ecfa78b80513c480f2514ec271
Diff: https://reviews.apache.org/r/37999/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/
-----------------------------------------------------------
(Updated Oct. 20, 2015, noon)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Updates to account for changes in previous tests.
Bugs: MESOS-3231
https://issues.apache.org/jira/browse/MESOS-3231
Repository: mesos
Description
-------
Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
Diffs (updated)
-----
3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
3rdparty/libprocess/include/process/http.hpp 591c1a959057155e1bf0f5bd73352e78d1c15cb3
3rdparty/libprocess/src/process.cpp 954d31424bc8f8ecfa78b80513c480f2514ec271
Diff: https://reviews.apache.org/r/37999/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/
-----------------------------------------------------------
(Updated Oct. 9, 2015, 12:53 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Fixes big bug of dlangling reference. Addresses till's review issues.
Bugs: MESOS-3231
https://issues.apache.org/jira/browse/MESOS-3231
Repository: mesos
Description
-------
Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
Diffs (updated)
-----
3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
3rdparty/libprocess/include/process/http.hpp 591c1a959057155e1bf0f5bd73352e78d1c15cb3
3rdparty/libprocess/src/process.cpp d1c81f1d244f02bf42cab97198587ce1b8c7c407
Diff: https://reviews.apache.org/r/37999/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review101816
-----------------------------------------------------------
Partial, entirely incomplete but maybe helpful review :)
3rdparty/libprocess/include/process/http.hpp (line 493)
<https://reviews.apache.org/r/37999/#comment159318>
THe RFCs always use `WWW-Authenticate`, shouldn't we as well here and everywhere else?
3rdparty/libprocess/src/process.cpp (line 2611)
<https://reviews.apache.org/r/37999/#comment159314>
s/Tecnically/technically/
s/principa/principal/
3rdparty/libprocess/src/process.cpp (lines 2643 - 2647)
<https://reviews.apache.org/r/37999/#comment159315>
This still makes me think we should fix the flaky fetcher tests before anything else. Lets put some combined efforts into that please.
- Till Toenshoff
On Sept. 30, 2015, 8:20 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2015, 8:20 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
> -------
>
> Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
>
> No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/include/process/http.hpp ba3f0bc7df33795e332c374fbad04106b9d56416
> 3rdparty/libprocess/src/process.cpp d1c81f1d244f02bf42cab97198587ce1b8c7c407
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/
-----------------------------------------------------------
(Updated Sept. 30, 2015, 10:20 a.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Rebase.
Bugs: MESOS-3231
https://issues.apache.org/jira/browse/MESOS-3231
Repository: mesos
Description
-------
Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
Diffs (updated)
-----
3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
3rdparty/libprocess/include/process/http.hpp ba3f0bc7df33795e332c374fbad04106b9d56416
3rdparty/libprocess/src/process.cpp d1c81f1d244f02bf42cab97198587ce1b8c7c407
Diff: https://reviews.apache.org/r/37999/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/
-----------------------------------------------------------
(Updated Sept. 28, 2015, 11:42 a.m.)
Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Changes
-------
Updated includes.
Bugs: MESOS-3231
https://issues.apache.org/jira/browse/MESOS-3231
Repository: mesos
Description
-------
Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
Diffs (updated)
-----
3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
3rdparty/libprocess/include/process/http.hpp fbd6cf7967173495875a8ea90ed28ade88b982a2
3rdparty/libprocess/src/process.cpp c03fba47435505484704e6c8a61e56123859781f
Diff: https://reviews.apache.org/r/37999/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Alex Clemmer <cl...@gmail.com>.
> On Sept. 23, 2015, 5:39 p.m., Alex Clemmer wrote:
> > Can we please add this file to the CMakeLists.txt file in 3rdparty/libprocess/src as well?
>
> Alexander Rojas wrote:
> The included file is a header and so far cmake file doesn't seem to care about the include directory (can you correct me if I'm wrong). Still, shouln't the headers be there too? otherwise cmake wouldn't do proper dependency analysis.
Oh, yes, my bad, those changes aren't committed yet. (I have too many reviews up and lose track of them all, sorry.)
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review100235
-----------------------------------------------------------
On Sept. 23, 2015, 8:57 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 8:57 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
> -------
>
> Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
>
> No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/include/process/http.hpp fbd6cf7967173495875a8ea90ed28ade88b982a2
> 3rdparty/libprocess/src/process.cpp 4afa30569b4d235637b49a624602e6b199c32e0e
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Alex Clemmer <cl...@gmail.com>.
> On Sept. 23, 2015, 5:39 p.m., Alex Clemmer wrote:
> > Can we please add this file to the CMakeLists.txt file in 3rdparty/libprocess/src as well?
>
> Alexander Rojas wrote:
> The included file is a header and so far cmake file doesn't seem to care about the include directory (can you correct me if I'm wrong). Still, shouln't the headers be there too? otherwise cmake wouldn't do proper dependency analysis.
>
> Alex Clemmer wrote:
> Oh, yes, my bad, those changes aren't committed yet. (I have too many reviews up and lose track of them all, sorry.)
To follow up, actually... we won't have to commit the header file lists after all! After investigation, it turns out that all CMake needs is to have the directory linked with `include_directory` and it just works. (I apologize for the lack of clarity here... I had never used CMake before I started this, and the truth is that when you write hundreds of lines of code in an unfamiliar language, you sometimes forget what is true and what is false. :) )
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review100235
-----------------------------------------------------------
On Sept. 23, 2015, 8:57 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 8:57 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
> -------
>
> Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
>
> No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/include/process/http.hpp fbd6cf7967173495875a8ea90ed28ade88b982a2
> 3rdparty/libprocess/src/process.cpp 4afa30569b4d235637b49a624602e6b199c32e0e
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Alexander Rojas <al...@mesosphere.io>.
> On Sept. 23, 2015, 7:39 p.m., Alex Clemmer wrote:
> > Can we please add this file to the CMakeLists.txt file in 3rdparty/libprocess/src as well?
The included file is a header and so far cmake file doesn't seem to care about the include directory (can you correct me if I'm wrong). Still, shouln't the headers be there too? otherwise cmake wouldn't do proper dependency analysis.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review100235
-----------------------------------------------------------
On Sept. 23, 2015, 10:57 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 10:57 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
> -------
>
> Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
>
> No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/include/process/http.hpp fbd6cf7967173495875a8ea90ed28ade88b982a2
> 3rdparty/libprocess/src/process.cpp 4afa30569b4d235637b49a624602e6b199c32e0e
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 37999: Implemented http::AuthenticatorManager
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review100235
-----------------------------------------------------------
Can we please add this file to the CMakeLists.txt file in 3rdparty/libprocess/src as well?
- Alex Clemmer
On Sept. 23, 2015, 8:57 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 8:57 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
> -------
>
> Introduces the authenticator manager, which is a class which handles the actual authentication procedure during the execution of `ProcessManager::handle()` and it also takes care of the life cycle of instances of http::Authenticator.
>
> No tests are added at this point since no public API is generated, the goal of this patch is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests proper come in a latter patch.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e
> 3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae
> 3rdparty/libprocess/include/process/http.hpp fbd6cf7967173495875a8ea90ed28ade88b982a2
> 3rdparty/libprocess/src/process.cpp 4afa30569b4d235637b49a624602e6b199c32e0e
>
> Diff: https://reviews.apache.org/r/37999/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>