You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2017/02/09 00:58:06 UTC
Review Request 56474: Added support for multiple authenticators to
libprocess.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/
-----------------------------------------------------------
Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
Bugs: MESOS-7004
https://issues.apache.org/jira/browse/MESOS-7004
Repository: mesos
Description
-------
This patch updates the `AuthenticatorManager` to allow
multiple authenticators to be set for a single realm.
Diffs
-----
3rdparty/libprocess/src/authenticator_manager.cpp a22acd026a001788dc39b8005a56577e33c6800b
Diff: https://reviews.apache.org/r/56474/diff/
Testing
-------
Testing information can be found in the subsequent patch in this chain.
Thanks,
Greg Mann
Re: Review Request 56474: Added the 'CombinedAuthenticator'.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 10, 2017, 12:50 p.m., Jan Schlicht wrote:
> > src/authentication/http/combined_authenticator.cpp
> > Lines 172 (patched)
> > <https://reviews.apache.org/r/56474/diff/4/?file=1660838#file1660838line172>
> >
> > Is it okay to capture `this` here? I just remembered (MESOS-5629)[https://issues.apache.org/jira/browse/MESOS-5629] but didn't closely follow the discussion. Feel free to drop this one, if the capture isn't an issue.
Ah, thank you for catching this! Updated to copy `self()` and capture that instead.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/#review168602
-----------------------------------------------------------
On March 10, 2017, 5:49 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56474/
> -----------------------------------------------------------
>
> (Updated March 10, 2017, 5:49 p.m.)
>
>
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new default authenticator, the
> `CombinedAuthenticator`, which can load multiple authenticators.
> It calls installed authenticators serially, returning the first
> successful result. When no results are successful, it returns a
> single result obtained by merging all unsuccessful results.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION
> src/Makefile.am bb917c5d1b36f5106a74914fa3a864038a95e8bb
> src/authentication/http/combined_authenticator.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/56474/diff/5/
>
>
> Testing
> -------
>
> Testing information can be found in the subsequent patch in this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56474: Added the 'CombinedAuthenticator'.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/#review168602
-----------------------------------------------------------
include/mesos/authentication/http/combined_authenticator.hpp
Lines 61 (patched)
<https://reviews.apache.org/r/56474/#comment240863>
Add on `override`, the destructor should be virtual when inheriting.
src/authentication/http/combined_authenticator.cpp
Lines 20 (patched)
<https://reviews.apache.org/r/56474/#comment240860>
Include before `<string>.
src/authentication/http/combined_authenticator.cpp
Lines 71 (patched)
<https://reviews.apache.org/r/56474/#comment240859>
This function is neither used nor implemented. This definition can be removed.
src/authentication/http/combined_authenticator.cpp
Lines 106 (patched)
<https://reviews.apache.org/r/56474/#comment240861>
If, at this point, `combinedUnauthorized` didn't have a `WWW-Authenticate` header yet, this will result in
the header value starting with a comma, e.g.
```
WWW-Authenticate: ,Basic realm=foo
```
src/authentication/http/combined_authenticator.cpp
Lines 172 (patched)
<https://reviews.apache.org/r/56474/#comment240862>
Is it okay to capture `this` here? I just remembered (MESOS-5629)[https://issues.apache.org/jira/browse/MESOS-5629] but didn't closely follow the discussion. Feel free to drop this one, if the capture isn't an issue.
- Jan Schlicht
On March 10, 2017, 1:33 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56474/
> -----------------------------------------------------------
>
> (Updated March 10, 2017, 1:33 a.m.)
>
>
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new default authenticator, the
> `CombinedAuthenticator`, which can load multiple authenticators.
> It calls installed authenticators serially, returning the first
> successful result. When no results are successful, it returns a
> single result obtained by merging all unsuccessful results.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION
> src/Makefile.am bb917c5d1b36f5106a74914fa3a864038a95e8bb
> src/authentication/http/combined_authenticator.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/56474/diff/4/
>
>
> Testing
> -------
>
> Testing information can be found in the subsequent patch in this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56474: Added the 'CombinedAuthenticator'.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 10, 2017, 8:22 p.m., Greg Mann wrote:
> >
Below comments from a review session with Vinod:
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/#review168657
-----------------------------------------------------------
On March 10, 2017, 5:49 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56474/
> -----------------------------------------------------------
>
> (Updated March 10, 2017, 5:49 p.m.)
>
>
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new default authenticator, the
> `CombinedAuthenticator`, which can load multiple authenticators.
> It calls installed authenticators serially, returning the first
> successful result. When no results are successful, it returns a
> single result obtained by merging all unsuccessful results.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION
> src/Makefile.am bb917c5d1b36f5106a74914fa3a864038a95e8bb
> src/authentication/http/combined_authenticator.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/56474/diff/5/
>
>
> Testing
> -------
>
> Testing information can be found in the subsequent patch in this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56474: Added the 'CombinedAuthenticator'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/#review168657
-----------------------------------------------------------
include/mesos/authentication/http/combined_authenticator.hpp
Lines 46-50 (patched)
<https://reviews.apache.org/r/56474/#comment240900>
Remove?
include/mesos/authentication/http/combined_authenticator.hpp
Lines 64 (patched)
<https://reviews.apache.org/r/56474/#comment240902>
s/authenticator modules/authenticators/
include/mesos/authentication/http/combined_authenticator.hpp
Lines 69-79 (patched)
<https://reviews.apache.org/r/56474/#comment240906>
"If the `unauthorized` member is set on any received results, an Unauthorized result will be returned" or similar.
Provide examples.
include/mesos/authentication/http/combined_authenticator.hpp
Lines 70 (patched)
<https://reviews.apache.org/r/56474/#comment240903>
s/modules'/authenticators'/
src/authentication/http/combined_authenticator.cpp
Lines 23 (patched)
<https://reviews.apache.org/r/56474/#comment240914>
remove?
src/authentication/http/combined_authenticator.cpp
Lines 33 (patched)
<https://reviews.apache.org/r/56474/#comment240913>
remove?
src/authentication/http/combined_authenticator.cpp
Lines 66-68 (patched)
<https://reviews.apache.org/r/56474/#comment240916>
Move below.
src/authentication/http/combined_authenticator.cpp
Lines 81-86 (patched)
<https://reviews.apache.org/r/56474/#comment240931>
Refactor to use `strings::join` on containers of headers/bodies
src/authentication/http/combined_authenticator.cpp
Lines 83 (patched)
<https://reviews.apache.org/r/56474/#comment240923>
s/Option<AuthenticationResult>/AuthenticationResult/
src/authentication/http/combined_authenticator.cpp
Lines 84 (patched)
<https://reviews.apache.org/r/56474/#comment240924>
`combineFailed()` instead?
src/authentication/http/combined_authenticator.cpp
Lines 89 (patched)
<https://reviews.apache.org/r/56474/#comment240926>
Make this more explicit
src/authentication/http/combined_authenticator.cpp
Lines 92 (patched)
<https://reviews.apache.org/r/56474/#comment240925>
CHECK_NONE(result.principal)
src/authentication/http/combined_authenticator.cpp
Lines 181-182 (patched)
<https://reviews.apache.org/r/56474/#comment240917>
s/If we've reached the end of the authenticator vector, then //
src/authentication/http/combined_authenticator.cpp
Lines 187-188 (patched)
<https://reviews.apache.org/r/56474/#comment240919>
Add a test with an authenticator returning a `Failure`.
src/authentication/http/combined_authenticator.cpp
Lines 199-200 (patched)
<https://reviews.apache.org/r/56474/#comment240920>
Log the scheme here.
- Greg Mann
On March 10, 2017, 5:49 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56474/
> -----------------------------------------------------------
>
> (Updated March 10, 2017, 5:49 p.m.)
>
>
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new default authenticator, the
> `CombinedAuthenticator`, which can load multiple authenticators.
> It calls installed authenticators serially, returning the first
> successful result. When no results are successful, it returns a
> single result obtained by merging all unsuccessful results.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION
> src/Makefile.am bb917c5d1b36f5106a74914fa3a864038a95e8bb
> src/authentication/http/combined_authenticator.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/56474/diff/5/
>
>
> Testing
> -------
>
> Testing information can be found in the subsequent patch in this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56474: Added the 'CombinedAuthenticator'.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/#review169039
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On March 15, 2017, 6:17 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56474/
> -----------------------------------------------------------
>
> (Updated March 15, 2017, 6:17 p.m.)
>
>
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new default authenticator, the
> `CombinedAuthenticator`, which can load multiple authenticators.
> It calls installed authenticators serially, returning the first
> successful result. When no results are successful, it returns a
> single result obtained by merging all unsuccessful results.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION
> src/CMakeLists.txt e1f81a17c0b007e84a5fc9827ef6b90bc276c267
> src/Makefile.am 2eea11a4f18f5d0e4ac3cc6bffc8b80f00556d01
> src/authentication/http/combined_authenticator.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/56474/diff/9/
>
>
> Testing
> -------
>
> Testing information can be found in the subsequent patch in this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56474: Added the 'CombinedAuthenticator'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/
-----------------------------------------------------------
(Updated March 15, 2017, 6:17 p.m.)
Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
Changes
-------
Addressed comments.
Bugs: MESOS-7004
https://issues.apache.org/jira/browse/MESOS-7004
Repository: mesos
Description
-------
This patch adds a new default authenticator, the
`CombinedAuthenticator`, which can load multiple authenticators.
It calls installed authenticators serially, returning the first
successful result. When no results are successful, it returns a
single result obtained by merging all unsuccessful results.
Diffs (updated)
-----
include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION
src/CMakeLists.txt e1f81a17c0b007e84a5fc9827ef6b90bc276c267
src/Makefile.am 2eea11a4f18f5d0e4ac3cc6bffc8b80f00556d01
src/authentication/http/combined_authenticator.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/56474/diff/9/
Changes: https://reviews.apache.org/r/56474/diff/8-9/
Testing
-------
Testing information can be found in the subsequent patch in this chain.
Thanks,
Greg Mann
Re: Review Request 56474: Added the 'CombinedAuthenticator'.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 15, 2017, 1:50 p.m., Vinod Kone wrote:
> > include/mesos/authentication/http/combined_authenticator.hpp
> > Lines 123-130 (patched)
> > <https://reviews.apache.org/r/56474/diff/8/?file=1664836#file1664836line123>
> >
> > why do we need the `overrides`? do we expect devs to subclass this in the future?
The `override` specifier is useful here because it tells the compiler that this function must override a virtual function of the base class, otherwise the compiler will throw an error. It prevents situations in which the signature of the base class function is changed but this one is left the same, in which case this function would not override anything.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/#review168905
-----------------------------------------------------------
On March 15, 2017, 6:17 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56474/
> -----------------------------------------------------------
>
> (Updated March 15, 2017, 6:17 p.m.)
>
>
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new default authenticator, the
> `CombinedAuthenticator`, which can load multiple authenticators.
> It calls installed authenticators serially, returning the first
> successful result. When no results are successful, it returns a
> single result obtained by merging all unsuccessful results.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION
> src/CMakeLists.txt e1f81a17c0b007e84a5fc9827ef6b90bc276c267
> src/Makefile.am 2eea11a4f18f5d0e4ac3cc6bffc8b80f00556d01
> src/authentication/http/combined_authenticator.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/56474/diff/9/
>
>
> Testing
> -------
>
> Testing information can be found in the subsequent patch in this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56474: Added the 'CombinedAuthenticator'.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/#review168905
-----------------------------------------------------------
src/authentication/http/combined_authenticator.cpp
Lines 146 (patched)
<https://reviews.apache.org/r/56474/#comment241191>
call this `extractUnauthorizedHeaders` ?
src/authentication/http/combined_authenticator.cpp
Lines 345 (patched)
<https://reviews.apache.org/r/56474/#comment241192>
new line.
include/mesos/authentication/http/combined_authenticator.hpp
Lines 123-130 (patched)
<https://reviews.apache.org/r/56474/#comment241331>
why do we need the `overrides`? do we expect devs to subclass this in the future?
src/authentication/http/combined_authenticator.cpp
Lines 237-240 (patched)
<https://reviews.apache.org/r/56474/#comment241334>
You could've done
```
combinedResult.unauthorized = Unauthorized(
extractUnauthorizedHeaders(results),
strings::join("\n\n", extractUnauthorizedBodies(results)));
```
provided `extractUnauthorizedHeaders` returned a vector. can the extract functions return a vector instead of list?
- Vinod Kone
On March 14, 2017, 9:36 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56474/
> -----------------------------------------------------------
>
> (Updated March 14, 2017, 9:36 p.m.)
>
>
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new default authenticator, the
> `CombinedAuthenticator`, which can load multiple authenticators.
> It calls installed authenticators serially, returning the first
> successful result. When no results are successful, it returns a
> single result obtained by merging all unsuccessful results.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION
> src/CMakeLists.txt e1f81a17c0b007e84a5fc9827ef6b90bc276c267
> src/Makefile.am 2eea11a4f18f5d0e4ac3cc6bffc8b80f00556d01
> src/authentication/http/combined_authenticator.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/56474/diff/8/
>
>
> Testing
> -------
>
> Testing information can be found in the subsequent patch in this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56474: Added the 'CombinedAuthenticator'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/
-----------------------------------------------------------
(Updated March 14, 2017, 9:36 p.m.)
Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
Changes
-------
Added new cpp file to the cmake build.
Bugs: MESOS-7004
https://issues.apache.org/jira/browse/MESOS-7004
Repository: mesos
Description
-------
This patch adds a new default authenticator, the
`CombinedAuthenticator`, which can load multiple authenticators.
It calls installed authenticators serially, returning the first
successful result. When no results are successful, it returns a
single result obtained by merging all unsuccessful results.
Diffs (updated)
-----
include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION
src/CMakeLists.txt e1f81a17c0b007e84a5fc9827ef6b90bc276c267
src/Makefile.am 2eea11a4f18f5d0e4ac3cc6bffc8b80f00556d01
src/authentication/http/combined_authenticator.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/56474/diff/8/
Changes: https://reviews.apache.org/r/56474/diff/7-8/
Testing
-------
Testing information can be found in the subsequent patch in this chain.
Thanks,
Greg Mann
Re: Review Request 56474: Added the 'CombinedAuthenticator'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/
-----------------------------------------------------------
(Updated March 14, 2017, 5:07 p.m.)
Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
Changes
-------
Rebase.
Bugs: MESOS-7004
https://issues.apache.org/jira/browse/MESOS-7004
Repository: mesos
Description
-------
This patch adds a new default authenticator, the
`CombinedAuthenticator`, which can load multiple authenticators.
It calls installed authenticators serially, returning the first
successful result. When no results are successful, it returns a
single result obtained by merging all unsuccessful results.
Diffs (updated)
-----
include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION
src/Makefile.am 2eea11a4f18f5d0e4ac3cc6bffc8b80f00556d01
src/authentication/http/combined_authenticator.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/56474/diff/7/
Changes: https://reviews.apache.org/r/56474/diff/6-7/
Testing
-------
Testing information can be found in the subsequent patch in this chain.
Thanks,
Greg Mann
Re: Review Request 56474: Added the 'CombinedAuthenticator'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/
-----------------------------------------------------------
(Updated March 14, 2017, 5:30 a.m.)
Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
Changes
-------
Addressed comments.
Bugs: MESOS-7004
https://issues.apache.org/jira/browse/MESOS-7004
Repository: mesos
Description
-------
This patch adds a new default authenticator, the
`CombinedAuthenticator`, which can load multiple authenticators.
It calls installed authenticators serially, returning the first
successful result. When no results are successful, it returns a
single result obtained by merging all unsuccessful results.
Diffs (updated)
-----
include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION
src/Makefile.am bb917c5d1b36f5106a74914fa3a864038a95e8bb
src/authentication/http/combined_authenticator.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/56474/diff/6/
Changes: https://reviews.apache.org/r/56474/diff/5-6/
Testing
-------
Testing information can be found in the subsequent patch in this chain.
Thanks,
Greg Mann
Re: Review Request 56474: Added the 'CombinedAuthenticator'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/
-----------------------------------------------------------
(Updated March 10, 2017, 5:49 p.m.)
Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
Changes
-------
Addressed Jan's comments.
Bugs: MESOS-7004
https://issues.apache.org/jira/browse/MESOS-7004
Repository: mesos
Description
-------
This patch adds a new default authenticator, the
`CombinedAuthenticator`, which can load multiple authenticators.
It calls installed authenticators serially, returning the first
successful result. When no results are successful, it returns a
single result obtained by merging all unsuccessful results.
Diffs (updated)
-----
include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION
src/Makefile.am bb917c5d1b36f5106a74914fa3a864038a95e8bb
src/authentication/http/combined_authenticator.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/56474/diff/5/
Changes: https://reviews.apache.org/r/56474/diff/4-5/
Testing
-------
Testing information can be found in the subsequent patch in this chain.
Thanks,
Greg Mann
Re: Review Request 56474: Added the 'CombinedAuthenticator'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/
-----------------------------------------------------------
(Updated March 10, 2017, 12:33 a.m.)
Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
Bugs: MESOS-7004
https://issues.apache.org/jira/browse/MESOS-7004
Repository: mesos
Description
-------
This patch adds a new default authenticator, the
`CombinedAuthenticator`, which can load multiple authenticators.
It calls installed authenticators serially, returning the first
successful result. When no results are successful, it returns a
single result obtained by merging all unsuccessful results.
Diffs (updated)
-----
include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION
src/Makefile.am bb917c5d1b36f5106a74914fa3a864038a95e8bb
src/authentication/http/combined_authenticator.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/56474/diff/4/
Changes: https://reviews.apache.org/r/56474/diff/3-4/
Testing
-------
Testing information can be found in the subsequent patch in this chain.
Thanks,
Greg Mann
Re: Review Request 56474: Added the 'CombinedAuthenticator'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/
-----------------------------------------------------------
(Updated March 9, 2017, 11:43 p.m.)
Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
Summary (updated)
-----------------
Added the 'CombinedAuthenticator'.
Bugs: MESOS-7004
https://issues.apache.org/jira/browse/MESOS-7004
Repository: mesos
Description (updated)
-------
This patch adds a new default authenticator, the
`CombinedAuthenticator`, which can load multiple authenticators.
It calls installed authenticators serially, returning the first
successful result. When no results are successful, it returns a
single result obtained by merging all unsuccessful results.
Diffs
-----
include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION
src/Makefile.am bb917c5d1b36f5106a74914fa3a864038a95e8bb
src/authentication/http/combined_authenticator.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/56474/diff/3/
Testing
-------
Testing information can be found in the subsequent patch in this chain.
Thanks,
Greg Mann
Re: Review Request 56474: Added support for multiple authenticators to
libprocess.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/
-----------------------------------------------------------
(Updated March 9, 2017, 11:40 p.m.)
Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
Changes
-------
Reworked patch to implement the 'CombinedAuthenticator' in Mesos application-level code.
Bugs: MESOS-7004
https://issues.apache.org/jira/browse/MESOS-7004
Repository: mesos
Description
-------
This patch updates the `AuthenticatorManager` to allow
multiple authenticators to be set for a single realm.
Diffs (updated)
-----
include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION
src/Makefile.am bb917c5d1b36f5106a74914fa3a864038a95e8bb
src/authentication/http/combined_authenticator.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/56474/diff/3/
Changes: https://reviews.apache.org/r/56474/diff/2-3/
Testing
-------
Testing information can be found in the subsequent patch in this chain.
Thanks,
Greg Mann
Re: Review Request 56474: Added the 'CombinedAuthenticator'.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Feb. 13, 2017, 11:41 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/src/authenticator_manager.cpp
> > Lines 102-128 (patched)
> > <https://reviews.apache.org/r/56474/diff/2/?file=1630603#file1630603line102>
> >
> > Not so sure about all the continues and the handling of different cases here. How about doing something like first processing the `unauthorized` items and then the `forbidden` ones, i.e.
> >
> > ```c++
> > foreach (const AuthenticationResult& result, results) {
> > if (result.unauthorized.isNone()) {
> > continue;
> > }
> >
> > // ... Code to merge unauthorized.
> > }
> >
> > if (unauthorized.isSome()) {
> > return unauthorized;
> > }
> >
> >
> > foreach (const AuthenticationResult& result, results) {
> > if (result.forbidden.isNone()) {
> > continue;
> > }
> >
> > // ... Code to merge forbidden.
> > }
> >
> > if (forbidden.isSome()) {
> > return forbidden;
> > }
> >
> > return None();
> > ```
Thanks for this suggestion; code looks much better now!
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/#review165304
-----------------------------------------------------------
On March 9, 2017, 11:43 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56474/
> -----------------------------------------------------------
>
> (Updated March 9, 2017, 11:43 p.m.)
>
>
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new default authenticator, the
> `CombinedAuthenticator`, which can load multiple authenticators.
> It calls installed authenticators serially, returning the first
> successful result. When no results are successful, it returns a
> single result obtained by merging all unsuccessful results.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION
> src/Makefile.am bb917c5d1b36f5106a74914fa3a864038a95e8bb
> src/authentication/http/combined_authenticator.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/56474/diff/3/
>
>
> Testing
> -------
>
> Testing information can be found in the subsequent patch in this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56474: Added the 'CombinedAuthenticator'.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Feb. 13, 2017, 11:41 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/src/authenticator_manager.cpp
> > Lines 77-81 (patched)
> > <https://reviews.apache.org/r/56474/diff/2/?file=1630603#file1630603line77>
> >
> > I'm my C++ knowledge is correct, you don't need this block, doing:
> >
> > ```c++
> > authenticators_[realm].push_back(authenticator);
> > ```
> >
> > should be enough (from the [docs](http://en.cppreference.com/w/cpp/container/unordered_map/operator_at): _"operator[] is non-const because it inserts the key if it doesn't exist. If this behavior is undesirable or if the container is const, at() may be used."_)
No longer relevant, so dropping.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/#review165304
-----------------------------------------------------------
On March 9, 2017, 11:43 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56474/
> -----------------------------------------------------------
>
> (Updated March 9, 2017, 11:43 p.m.)
>
>
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new default authenticator, the
> `CombinedAuthenticator`, which can load multiple authenticators.
> It calls installed authenticators serially, returning the first
> successful result. When no results are successful, it returns a
> single result obtained by merging all unsuccessful results.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/combined_authenticator.hpp PRE-CREATION
> src/Makefile.am bb917c5d1b36f5106a74914fa3a864038a95e8bb
> src/authentication/http/combined_authenticator.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/56474/diff/3/
>
>
> Testing
> -------
>
> Testing information can be found in the subsequent patch in this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56474: Added support for multiple authenticators to
libprocess.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/#review165304
-----------------------------------------------------------
3rdparty/libprocess/src/authenticator_manager.cpp (lines 77 - 81)
<https://reviews.apache.org/r/56474/#comment237161>
I'm my C++ knowledge is correct, you don't need this block, doing:
```c++
authenticators_[realm].push_back(authenticator);
```
should be enough (from the [docs](http://en.cppreference.com/w/cpp/container/unordered_map/operator_at): _"operator[] is non-const because it inserts the key if it doesn't exist. If this behavior is undesirable or if the container is const, at() may be used."_)
3rdparty/libprocess/src/authenticator_manager.cpp (lines 102 - 128)
<https://reviews.apache.org/r/56474/#comment237162>
Not so sure about all the continues and the handling of different cases here. How about doing something like first processing the `unauthorized` items and then the `forbidden` ones, i.e.
```c++
foreach (const AuthenticationResult& result, results) {
if (result.unauthorized.isNone()) {
continue;
}
// ... Code to merge unauthorized.
}
if (unauthorized.isSome()) {
return unauthorized;
}
foreach (const AuthenticationResult& result, results) {
if (result.forbidden.isNone()) {
continue;
}
// ... Code to merge forbidden.
}
if (forbidden.isSome()) {
return forbidden;
}
return None();
```
3rdparty/libprocess/src/authenticator_manager.cpp (lines 158 - 160)
<https://reviews.apache.org/r/56474/#comment237163>
I think the keyword `auto` is prefered when extracting iterators, [reference](https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md#c11)
- Alexander Rojas
On Feb. 11, 2017, 1:44 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56474/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2017, 1:44 a.m.)
>
>
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updates the `AuthenticatorManager` to allow
> multiple authenticators to be set for a single realm.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/authenticator_manager.cpp a22acd026a001788dc39b8005a56577e33c6800b
>
> Diff: https://reviews.apache.org/r/56474/diff/
>
>
> Testing
> -------
>
> Testing information can be found in the subsequent patch in this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56474: Added support for multiple authenticators to
libprocess.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Feb. 13, 2017, 3:01 p.m., Alexander Rojas wrote:
> > So now I remembered why originally it didn't support multiple authenticators. In my original design I favored composition of authenticators, in facto, I think one of the first iteration of the patches included an `AndAuthenticator` and an `OrAuthenticator`. Just consider that instead of implementing this inside libprocess, Mesos implement a combiner authenticator which works more or less like this:
> >
> > ```c++
> > class CombinedAuthenticator : public Authenticator
> > {
> > public:
> > CombinedAuthenticator(const std::string &realm, const std::vector<Authenticator*> &authenticators);
> >
> > virtual Future<AuthenticationResult> authenticate(const Request& request) override;
> >
> > private:
> > std::vector<Owned<Authenticator>> authenticators_;
> > std::string realm_;
> > };
> >
> > CombinedAuthenticator::CombinedAuthenticator(const std::string &realm, const std::vector<Authenticator*> &authenticators)
> > : authenticators_(), realm_(realm)
> > {
> > for (const Authenticator* authenticator, authenticators) {
> > authenticators_.push_back(Owned<Authenticator>(authenticator));
> > }
> > }
> >
> > Future<AuthenticationResult> CombinedAuthenticator::authenticate(const Request& request)
> > {
> > // ... The code inside AuthenticatorManagerProcess::authenticate()
> > }
> > ```
> >
> > I personally would prefer this approach since it keeps the separation of [mechanism and policy](https://en.wikipedia.org/wiki/Separation_of_mechanism_and_policy), leaving users of libprocess to decide exactly how to perform authentication while the library itself only cares about a single interface.
> >
> > This is however just a peek in the considerations I had when I designed the multiple authenticators problem and I would suggest to only giving it a thought.
I like this approach - I'll update the review chain to reflect it. Thanks Alexander!!!
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/#review165328
-----------------------------------------------------------
On Feb. 11, 2017, 12:44 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56474/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2017, 12:44 a.m.)
>
>
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updates the `AuthenticatorManager` to allow
> multiple authenticators to be set for a single realm.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/authenticator_manager.cpp a22acd026a001788dc39b8005a56577e33c6800b
>
>
> Diff: https://reviews.apache.org/r/56474/diff/2/
>
>
> Testing
> -------
>
> Testing information can be found in the subsequent patch in this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56474: Added support for multiple authenticators to
libprocess.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/#review165328
-----------------------------------------------------------
So now I remembered why originally it didn't support multiple authenticators. In my original design I favored composition of authenticators, in facto, I think one of the first iteration of the patches included an `AndAuthenticator` and an `OrAuthenticator`. Just consider that instead of implementing this inside libprocess, Mesos implement a combiner authenticator which works more or less like this:
```c++
class CombinedAuthenticator : public Authenticator
{
public:
CombinedAuthenticator(const std::string &realm, const std::vector<Authenticator*> &authenticators);
virtual Future<AuthenticationResult> authenticate(const Request& request) override;
private:
std::vector<Owned<Authenticator>> authenticators_;
std::string realm_;
};
CombinedAuthenticator::CombinedAuthenticator(const std::string &realm, const std::vector<Authenticator*> &authenticators)
: authenticators_(), realm_(realm)
{
for (const Authenticator* authenticator, authenticators) {
authenticators_.push_back(Owned<Authenticator>(authenticator));
}
}
Future<AuthenticationResult> CombinedAuthenticator::authenticate(const Request& request)
{
// ... The code inside AuthenticatorManagerProcess::authenticate()
}
```
I personally would prefer this approach since it keeps the separation of [mechanism and policy](https://en.wikipedia.org/wiki/Separation_of_mechanism_and_policy), leaving users of libprocess to decide exactly how to perform authentication while the library itself only cares about a single interface.
This is however just a peek in the considerations I had when I designed the multiple authenticators problem and I would suggest to only giving it a thought.
- Alexander Rojas
On Feb. 11, 2017, 1:44 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56474/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2017, 1:44 a.m.)
>
>
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updates the `AuthenticatorManager` to allow
> multiple authenticators to be set for a single realm.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/authenticator_manager.cpp a22acd026a001788dc39b8005a56577e33c6800b
>
> Diff: https://reviews.apache.org/r/56474/diff/
>
>
> Testing
> -------
>
> Testing information can be found in the subsequent patch in this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56474: Added support for multiple authenticators to
libprocess.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/
-----------------------------------------------------------
(Updated Feb. 11, 2017, 12:44 a.m.)
Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
Changes
-------
Addressed comments.
Bugs: MESOS-7004
https://issues.apache.org/jira/browse/MESOS-7004
Repository: mesos
Description
-------
This patch updates the `AuthenticatorManager` to allow
multiple authenticators to be set for a single realm.
Diffs (updated)
-----
3rdparty/libprocess/src/authenticator_manager.cpp a22acd026a001788dc39b8005a56577e33c6800b
Diff: https://reviews.apache.org/r/56474/diff/
Testing
-------
Testing information can be found in the subsequent patch in this chain.
Thanks,
Greg Mann
Re: Review Request 56474: Added support for multiple authenticators to
libprocess.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Feb. 9, 2017, 3:03 p.m., Jan Schlicht wrote:
> > 3rdparty/libprocess/src/authenticator_manager.cpp, lines 168-204
> > <https://reviews.apache.org/r/56474/diff/1/?file=1628065#file1628065line168>
> >
> > How about we make this a function instead of using a lambda?
> > Seems to do a lot and even contains another lambda in line 180.
The outer lambda needs to be something like a mutable lambda so that it is instantiated once when `loop` is invoked, allowing us to capture the `results` list once and push onto it as we iterate. I suppose we could accomplish this with a struct bearing this code in its `()` operator, which we instantiate here, but I'm not sure I like that more. Perhaps there's a way of doing this that I'm not thinking of?
It would be possible to use `std::bind` with a static member function instead of the inner lambda, but since we use the `loop`-related constructs `Break` and `Continue` within that block, I think it actually makes the code a bit more readable to keep it all in one place. Let me know what you think.
> On Feb. 9, 2017, 3:03 p.m., Jan Schlicht wrote:
> > 3rdparty/libprocess/src/authenticator_manager.cpp, line 160
> > <https://reviews.apache.org/r/56474/diff/1/?file=1628065#file1628065line160>
> >
> > Could also be `vector<AuthenticationResult> results(authenticators_[realm].size());`. Of course, `combineFailedRequests` would need to be changed respectively.
Yep good point, that is another option. I think I'm going to do some benchmarking of this patch vs. the old authentication code, so perhaps I can benchmark the list vs. vector in this case as well.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/#review164924
-----------------------------------------------------------
On Feb. 11, 2017, 12:44 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56474/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2017, 12:44 a.m.)
>
>
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updates the `AuthenticatorManager` to allow
> multiple authenticators to be set for a single realm.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/authenticator_manager.cpp a22acd026a001788dc39b8005a56577e33c6800b
>
> Diff: https://reviews.apache.org/r/56474/diff/
>
>
> Testing
> -------
>
> Testing information can be found in the subsequent patch in this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56474: Added support for multiple authenticators to
libprocess.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56474/#review164924
-----------------------------------------------------------
3rdparty/libprocess/src/authenticator_manager.cpp (line 15)
<https://reviews.apache.org/r/56474/#comment236768>
Also `#include <list>` because you're using `std::list` below.
3rdparty/libprocess/src/authenticator_manager.cpp (line 31)
<https://reviews.apache.org/r/56474/#comment236769>
`using std::list;` above?
3rdparty/libprocess/src/authenticator_manager.cpp (line 160)
<https://reviews.apache.org/r/56474/#comment236767>
Could also be `vector<AuthenticationResult> results(authenticators_[realm].size());`. Of course, `combineFailedRequests` would need to be changed respectively.
3rdparty/libprocess/src/authenticator_manager.cpp (lines 168 - 204)
<https://reviews.apache.org/r/56474/#comment236756>
How about we make this a function instead of using a lambda?
Seems to do a lot and even contains another lambda in line 180.
- Jan Schlicht
On Feb. 9, 2017, 1:58 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56474/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2017, 1:58 a.m.)
>
>
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and Vinod Kone.
>
>
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updates the `AuthenticatorManager` to allow
> multiple authenticators to be set for a single realm.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/authenticator_manager.cpp a22acd026a001788dc39b8005a56577e33c6800b
>
> Diff: https://reviews.apache.org/r/56474/diff/
>
>
> Testing
> -------
>
> Testing information can be found in the subsequent patch in this chain.
>
>
> Thanks,
>
> Greg Mann
>
>