You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Till Toenshoff <to...@me.com> on 2017/09/26 22:54:10 UTC
Review Request 62587: Added HTTP authenticatee interface definition.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62587/
-----------------------------------------------------------
Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
Bugs: MESOS-8016
https://issues.apache.org/jira/browse/MESOS-8016
Repository: mesos
Description
-------
see summary.
Diffs
-----
include/mesos/authentication/http/authenticatee.hpp PRE-CREATION
src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
Diff: https://reviews.apache.org/r/62587/diff/1/
Testing
-------
Integration test in external project. Further tests upcoming.
Thanks,
Till Toenshoff
Re: Review Request 62587: Added HTTP authenticatee interface
definition.
Posted by Till Toenshoff <to...@me.com>.
> On Oct. 6, 2017, 10:22 a.m., Alexander Rojas wrote:
> > include/mesos/authentication/http/authenticatee.hpp
> > Lines 34 (patched)
> > <https://reviews.apache.org/r/62587/diff/4/?file=1842299#file1842299line34>
> >
> > In order to keep it consistent with the `Authenticator`, could you add a method which returns the authentication scheme used?
> >
> > i.e.
> >
> > ```c++
> > /**
> > * Returns the name of the authentication scheme implemented.
> > */
> > virtual std::string scheme() const = 0;
> > ```
> >
> > If basic is used, it will return Basic, and so fort.
Good point, will add that.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62587/#review187246
-----------------------------------------------------------
On Oct. 2, 2017, 6:55 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62587/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2017, 6:55 p.m.)
>
>
> Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-8016
> https://issues.apache.org/jira/browse/MESOS-8016
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added HTTP authenticatee interface definition.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/authenticatee.hpp PRE-CREATION
> src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
>
>
> Diff: https://reviews.apache.org/r/62587/diff/4/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62587: Added HTTP authenticatee interface
definition.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62587/#review187246
-----------------------------------------------------------
include/mesos/authentication/http/authenticatee.hpp
Lines 34 (patched)
<https://reviews.apache.org/r/62587/#comment264179>
In order to keep it consistent with the `Authenticator`, could you add a method which returns the authentication scheme used?
i.e.
```c++
/**
* Returns the name of the authentication scheme implemented.
*/
virtual std::string scheme() const = 0;
```
If basic is used, it will return Basic, and so fort.
- Alexander Rojas
On Oct. 2, 2017, 8:55 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62587/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2017, 8:55 p.m.)
>
>
> Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-8016
> https://issues.apache.org/jira/browse/MESOS-8016
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added HTTP authenticatee interface definition.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/authenticatee.hpp PRE-CREATION
> src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
>
>
> Diff: https://reviews.apache.org/r/62587/diff/4/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62587: Added HTTP authenticatee interface
definition.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62587/#review187315
-----------------------------------------------------------
Fix it, then Ship it!
include/mesos/authentication/http/authenticatee.hpp
Lines 56 (patched)
<https://reviews.apache.org/r/62587/#comment264231>
s/a HTTP/an HTTP/
- Greg Mann
On Oct. 2, 2017, 6:55 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62587/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2017, 6:55 p.m.)
>
>
> Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-8016
> https://issues.apache.org/jira/browse/MESOS-8016
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added HTTP authenticatee interface definition.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/authenticatee.hpp PRE-CREATION
> src/Makefile.am da8af916652a80303f37b65056920676726f8dfe
>
>
> Diff: https://reviews.apache.org/r/62587/diff/5/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62587: Added HTTP authenticatee interface
definition.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62587/
-----------------------------------------------------------
(Updated Oct. 7, 2017, 1:06 a.m.)
Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
Changes
-------
comment.
Bugs: MESOS-8016
https://issues.apache.org/jira/browse/MESOS-8016
Repository: mesos
Description
-------
Added HTTP authenticatee interface definition.
Diffs (updated)
-----
include/mesos/authentication/http/authenticatee.hpp PRE-CREATION
src/Makefile.am da8af916652a80303f37b65056920676726f8dfe
Diff: https://reviews.apache.org/r/62587/diff/6/
Changes: https://reviews.apache.org/r/62587/diff/5-6/
Testing
-------
Integration test in external project. Further tests upcoming.
Thanks,
Till Toenshoff
Re: Review Request 62587: Added HTTP authenticatee interface
definition.
Posted by Till Toenshoff <to...@me.com>.
> On Oct. 6, 2017, 9:56 a.m., Alexander Rojas wrote:
> > include/mesos/authentication/http/authenticatee.hpp
> > Lines 46 (patched)
> > <https://reviews.apache.org/r/62587/diff/4/?file=1842299#file1842299line46>
> >
> > Any reason not to make it pure virtual?
The implementation may or may not need it - e.g. basic authentication wont hold any internal state.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62587/#review187244
-----------------------------------------------------------
On Oct. 2, 2017, 6:55 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62587/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2017, 6:55 p.m.)
>
>
> Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-8016
> https://issues.apache.org/jira/browse/MESOS-8016
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added HTTP authenticatee interface definition.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/authenticatee.hpp PRE-CREATION
> src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
>
>
> Diff: https://reviews.apache.org/r/62587/diff/4/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62587: Added HTTP authenticatee interface
definition.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62587/#review187244
-----------------------------------------------------------
include/mesos/authentication/http/authenticatee.hpp
Lines 46 (patched)
<https://reviews.apache.org/r/62587/#comment264178>
Any reason not to make it pure virtual?
- Alexander Rojas
On Oct. 2, 2017, 8:55 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62587/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2017, 8:55 p.m.)
>
>
> Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-8016
> https://issues.apache.org/jira/browse/MESOS-8016
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added HTTP authenticatee interface definition.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/authenticatee.hpp PRE-CREATION
> src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
>
>
> Diff: https://reviews.apache.org/r/62587/diff/4/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62587: Added HTTP authenticatee interface
definition.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62587/
-----------------------------------------------------------
(Updated Oct. 2, 2017, 6:55 p.m.)
Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
Changes
-------
addressed comments.
Bugs: MESOS-8016
https://issues.apache.org/jira/browse/MESOS-8016
Repository: mesos
Description
-------
Added HTTP authenticatee interface definition.
Diffs (updated)
-----
include/mesos/authentication/http/authenticatee.hpp PRE-CREATION
src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
Diff: https://reviews.apache.org/r/62587/diff/3/
Changes: https://reviews.apache.org/r/62587/diff/2-3/
Testing
-------
Integration test in external project. Further tests upcoming.
Thanks,
Till Toenshoff
Re: Review Request 62587: Added HTTP authenticatee interface
definition.
Posted by Armand Grillet <ag...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62587/#review186833
-----------------------------------------------------------
include/mesos/authentication/http/authenticatee.hpp
Lines 44 (patched)
<https://reviews.apache.org/r/62587/#comment263674>
`s/signalled/signaled`.
- Armand Grillet
On Oct. 2, 2017, 10:09 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62587/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2017, 10:09 a.m.)
>
>
> Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-8016
> https://issues.apache.org/jira/browse/MESOS-8016
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added HTTP authenticatee interface definition.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/authenticatee.hpp PRE-CREATION
> src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
>
>
> Diff: https://reviews.apache.org/r/62587/diff/2/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62587: Added HTTP authenticatee interface
definition.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62587/
-----------------------------------------------------------
(Updated Oct. 2, 2017, 10:09 a.m.)
Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
Changes
-------
addressed comments.
Bugs: MESOS-8016
https://issues.apache.org/jira/browse/MESOS-8016
Repository: mesos
Description (updated)
-------
Added HTTP authenticatee interface definition.
Diffs (updated)
-----
include/mesos/authentication/http/authenticatee.hpp PRE-CREATION
src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
Diff: https://reviews.apache.org/r/62587/diff/2/
Changes: https://reviews.apache.org/r/62587/diff/1-2/
Testing
-------
Integration test in external project. Further tests upcoming.
Thanks,
Till Toenshoff
Re: Review Request 62587: Added HTTP authenticatee interface
definition.
Posted by Benjamin Bannier <bb...@apache.org>.
> On Sept. 28, 2017, 5:54 p.m., Benjamin Bannier wrote:
> > include/mesos/authentication/http/authenticatee.hpp
> > Lines 48-49 (patched)
> > <https://reviews.apache.org/r/62587/diff/1/?file=1835964#file1835964line48>
> >
> > Since our Doxygen setup uses `JAVADOC_AUTOBRIEF` which makes the first paragraph a brief description, can we add one here?, e.g.,
> >
> > Create a HTTP request for authentication.
>
> Till Toenshoff wrote:
> Our docs/doxygen-style-guide.md does not mention this tag, hence I am not sure our site renderer swallows that properly. Could you please enhance it that way?
Looking at the Doxygen style guide, it seems we already make the preferred comment format explicit.
We currently have https://github.com/apache/mesos/blob/32ea75b8426fb3972680a397dc3a05ff266d166d/docs/doxygen-style-guide.md#L22-L53 which contains both an example and a link to the exact format of Javadoc comment blocks, https://en.wikipedia.org/wiki/Javadoc#Structure_of_a_Javadoc_comment. I am not sure that adding more information here would make this any clearer.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62587/#review186390
-----------------------------------------------------------
On Oct. 2, 2017, 8:55 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62587/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2017, 8:55 p.m.)
>
>
> Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-8016
> https://issues.apache.org/jira/browse/MESOS-8016
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added HTTP authenticatee interface definition.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/authenticatee.hpp PRE-CREATION
> src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
>
>
> Diff: https://reviews.apache.org/r/62587/diff/4/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62587: Added HTTP authenticatee interface
definition.
Posted by Till Toenshoff <to...@me.com>.
> On Sept. 28, 2017, 3:54 p.m., Benjamin Bannier wrote:
> > include/mesos/authentication/http/authenticatee.hpp
> > Lines 48-49 (patched)
> > <https://reviews.apache.org/r/62587/diff/1/?file=1835964#file1835964line48>
> >
> > Since our Doxygen setup uses `JAVADOC_AUTOBRIEF` which makes the first paragraph a brief description, can we add one here?, e.g.,
> >
> > Create a HTTP request for authentication.
Our docs/doxygen-style-guide.md does not mention this tag, hence I am not sure our site renderer swallows that properly. Could you please enhance it that way?
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62587/#review186390
-----------------------------------------------------------
On Sept. 26, 2017, 10:54 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62587/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2017, 10:54 p.m.)
>
>
> Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-8016
> https://issues.apache.org/jira/browse/MESOS-8016
>
>
> Repository: mesos
>
>
> Description
> -------
>
> see summary.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/authenticatee.hpp PRE-CREATION
> src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
>
>
> Diff: https://reviews.apache.org/r/62587/diff/1/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62587: Added HTTP authenticatee interface
definition.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62587/#review186390
-----------------------------------------------------------
include/mesos/authentication/http/authenticatee.hpp
Lines 32-33 (patched)
<https://reviews.apache.org/r/62587/#comment262946>
An implementation isn't necessarily thin, what about e.g.,
An abstraction enabling any HTTP API consumer to authenticate.
include/mesos/authentication/http/authenticatee.hpp
Lines 38 (patched)
<https://reviews.apache.org/r/62587/#comment262943>
`= default`
include/mesos/authentication/http/authenticatee.hpp
Lines 41-44 (patched)
<https://reviews.apache.org/r/62587/#comment262944>
Since we are in the generic interface where there are no caches, I'd suggest to make this comment more generic, e.g., `Reset the authenticatee to its initial state`.
include/mesos/authentication/http/authenticatee.hpp
Lines 48-49 (patched)
<https://reviews.apache.org/r/62587/#comment262945>
Since our Doxygen setup uses `JAVADOC_AUTOBRIEF` which makes the first paragraph a brief description, can we add one here?, e.g.,
Create a HTTP request for authentication.
- Benjamin Bannier
On Sept. 27, 2017, 12:54 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62587/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2017, 12:54 a.m.)
>
>
> Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-8016
> https://issues.apache.org/jira/browse/MESOS-8016
>
>
> Repository: mesos
>
>
> Description
> -------
>
> see summary.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/authenticatee.hpp PRE-CREATION
> src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
>
>
> Diff: https://reviews.apache.org/r/62587/diff/1/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62587: Added HTTP authenticatee interface
definition.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62587/#review186470
-----------------------------------------------------------
include/mesos/authentication/http/authenticatee.hpp
Lines 41 (patched)
<https://reviews.apache.org/r/62587/#comment263022>
s/token based/token-based/
include/mesos/authentication/http/authenticatee.hpp
Lines 42 (patched)
<https://reviews.apache.org/r/62587/#comment263023>
s/May get/May get/
include/mesos/authentication/http/authenticatee.hpp
Lines 45 (patched)
<https://reviews.apache.org/r/62587/#comment263034>
Am I correct in thinking that this method is primarily intended to enable the client to handle the case where requests fail authentication? If so, maybe we should call that out explicitly in the comment.
include/mesos/authentication/http/authenticatee.hpp
Lines 49 (patched)
<https://reviews.apache.org/r/62587/#comment263035>
s/authentication related/authentication-related/
- Greg Mann
On Sept. 26, 2017, 10:54 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62587/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2017, 10:54 p.m.)
>
>
> Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-8016
> https://issues.apache.org/jira/browse/MESOS-8016
>
>
> Repository: mesos
>
>
> Description
> -------
>
> see summary.
>
>
> Diffs
> -----
>
> include/mesos/authentication/http/authenticatee.hpp PRE-CREATION
> src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
>
>
> Diff: https://reviews.apache.org/r/62587/diff/1/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>