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:32 UTC
Review Request 62594: Updated scheduler library for modularized HTTP
authenticatee use.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62594/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar, Armand Grillet, Benjamin Bannier, Greg Mann, Kapil Arya, and Vinod Kone.
Bugs: MESOS-8021
https://issues.apache.org/jira/browse/MESOS-8021
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/scheduler/flags.hpp 1e8efc06b40b17d6fa9d9516a83893485804fc72
src/scheduler/scheduler.cpp 78f53707364ab988bcc53ec2c95490df04cd9a6c
Diff: https://reviews.apache.org/r/62594/diff/1/
Testing
-------
Integration test in external project. Further tests upcoming.
Thanks,
Till Toenshoff
Re: Review Request 62594: Updated scheduler library for modularized
HTTP authenticatee use.
Posted by Till Toenshoff <to...@me.com>.
> On Sept. 27, 2017, 10:07 p.m., Greg Mann wrote:
> > src/scheduler/flags.hpp
> > Lines 57-59 (patched)
> > <https://reviews.apache.org/r/62594/diff/1/?file=1836050#file1836050line58>
> >
> > s/kept in in sync/kept in sync/
> >
> > Also, this seems bad :) (also for modules_dir) I wonder if we have a ticket for eliminating this code duplication thru common inheritance?
Yes, lets fix this particular stuff via a ticket and flags inheritance.
> On Sept. 27, 2017, 10:07 p.m., Greg Mann wrote:
> > src/scheduler/scheduler.cpp
> > Line 527 (original), 594 (patched)
> > <https://reviews.apache.org/r/62594/diff/1/?file=1836051#file1836051line623>
> >
> > Hmmm... is this CHECK correct? Since this continuation is registered as an `onAny` callback, couldn't the future be in DISCARDED state? You do handle the discarded case in your new continuation.
We do need to validate how this invariant is guaranteed.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62594/#review186483
-----------------------------------------------------------
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/62594/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2017, 10:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Armand Grillet, Benjamin Bannier, Greg Mann, Kapil Arya, and Vinod Kone.
>
>
> Bugs: MESOS-8021
> https://issues.apache.org/jira/browse/MESOS-8021
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/scheduler/flags.hpp 1e8efc06b40b17d6fa9d9516a83893485804fc72
> src/scheduler/scheduler.cpp 78f53707364ab988bcc53ec2c95490df04cd9a6c
>
>
> Diff: https://reviews.apache.org/r/62594/diff/1/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62594: Updated scheduler library for modularized
HTTP authenticatee use.
Posted by Till Toenshoff <to...@me.com>.
> On Sept. 27, 2017, 10:07 p.m., Greg Mann wrote:
> > src/scheduler/flags.hpp
> > Lines 57-59 (patched)
> > <https://reviews.apache.org/r/62594/diff/1/?file=1836050#file1836050line58>
> >
> > s/kept in in sync/kept in sync/
> >
> > Also, this seems bad :) (also for modules_dir) I wonder if we have a ticket for eliminating this code duplication thru common inheritance?
>
> Till Toenshoff wrote:
> Yes, lets fix this particular stuff via a ticket and flags inheritance.
https://issues.apache.org/jira/browse/MESOS-8044
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62594/#review186483
-----------------------------------------------------------
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/62594/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2017, 10:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Armand Grillet, Benjamin Bannier, Greg Mann, Kapil Arya, and Vinod Kone.
>
>
> Bugs: MESOS-8021
> https://issues.apache.org/jira/browse/MESOS-8021
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/scheduler/flags.hpp 1e8efc06b40b17d6fa9d9516a83893485804fc72
> src/scheduler/scheduler.cpp 78f53707364ab988bcc53ec2c95490df04cd9a6c
>
>
> Diff: https://reviews.apache.org/r/62594/diff/1/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62594: Updated scheduler library for modularized
HTTP authenticatee use.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On Sept. 28, 2017, 12:07 a.m., Greg Mann wrote:
> > src/scheduler/scheduler.cpp
> > Line 527 (original), 594 (patched)
> > <https://reviews.apache.org/r/62594/diff/1/?file=1836051#file1836051line623>
> >
> > Hmmm... is this CHECK correct? Since this continuation is registered as an `onAny` callback, couldn't the future be in DISCARDED state? You do handle the discarded case in your new continuation.
>
> Till Toenshoff wrote:
> We do need to validate how this invariant is guaranteed.
Interesting point you raise here, moreover because the same argument could be done before Till's patches (If I understand the old code right). I wonder how did anand ensure that the future won't be discarded. My only assumption is that the future is not hold by anybody before the continuation call nor afterwards.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62594/#review186483
-----------------------------------------------------------
On Oct. 2, 2017, 12:29 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62594/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2017, 12:29 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Armand Grillet, Benjamin Bannier, Greg Mann, Kapil Arya, and Vinod Kone.
>
>
> Bugs: MESOS-8021
> https://issues.apache.org/jira/browse/MESOS-8021
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated scheduler library for modularized HTTP authenticatee use.
>
>
> Diffs
> -----
>
> src/scheduler/flags.hpp 1e8efc06b40b17d6fa9d9516a83893485804fc72
> src/scheduler/scheduler.cpp 78f53707364ab988bcc53ec2c95490df04cd9a6c
>
>
> Diff: https://reviews.apache.org/r/62594/diff/2/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62594: Updated scheduler library for modularized
HTTP authenticatee use.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62594/#review186483
-----------------------------------------------------------
src/scheduler/flags.hpp
Lines 57-59 (patched)
<https://reviews.apache.org/r/62594/#comment263056>
s/kept in in sync/kept in sync/
Also, this seems bad :) (also for modules_dir) I wonder if we have a ticket for eliminating this code duplication thru common inheritance?
src/scheduler/scheduler.cpp
Lines 226 (patched)
<https://reviews.apache.org/r/62594/#comment263058>
Indented too far.
src/scheduler/scheduler.cpp
Lines 327 (patched)
<https://reviews.apache.org/r/62594/#comment263069>
See my comment in another review in this chain regarding underscores on similar variables within a scope. We should probably just give this one a new name.
src/scheduler/scheduler.cpp
Line 527 (original), 594 (patched)
<https://reviews.apache.org/r/62594/#comment263060>
Hmmm... is this CHECK correct? Since this continuation is registered as an `onAny` callback, couldn't the future be in DISCARDED state? You do handle the discarded case in your new continuation.
- 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/62594/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2017, 10:54 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Armand Grillet, Benjamin Bannier, Greg Mann, Kapil Arya, and Vinod Kone.
>
>
> Bugs: MESOS-8021
> https://issues.apache.org/jira/browse/MESOS-8021
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/scheduler/flags.hpp 1e8efc06b40b17d6fa9d9516a83893485804fc72
> src/scheduler/scheduler.cpp 78f53707364ab988bcc53ec2c95490df04cd9a6c
>
>
> Diff: https://reviews.apache.org/r/62594/diff/1/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62594: Updated scheduler library for modularized
HTTP authenticatee use.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62594/#review187254
-----------------------------------------------------------
src/scheduler/scheduler.cpp
Line 257 (original), 269 (patched)
<https://reviews.apache.org/r/62594/#comment264181>
Let's replace this TODO with one sayin something like:
_add support for multi-step authentication protocols_.
- Alexander Rojas
On Oct. 2, 2017, 12:29 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62594/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2017, 12:29 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Armand Grillet, Benjamin Bannier, Greg Mann, Kapil Arya, and Vinod Kone.
>
>
> Bugs: MESOS-8021
> https://issues.apache.org/jira/browse/MESOS-8021
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated scheduler library for modularized HTTP authenticatee use.
>
>
> Diffs
> -----
>
> src/scheduler/flags.hpp 1e8efc06b40b17d6fa9d9516a83893485804fc72
> src/scheduler/scheduler.cpp 78f53707364ab988bcc53ec2c95490df04cd9a6c
>
>
> Diff: https://reviews.apache.org/r/62594/diff/2/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62594: Updated scheduler library for modularized
HTTP authenticatee use.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62594/#review187325
-----------------------------------------------------------
src/scheduler/flags.hpp
Lines 22-25 (patched)
<https://reviews.apache.org/r/62594/#comment264245>
Are these really necessary?
src/scheduler/flags.hpp
Lines 48 (patched)
<https://reviews.apache.org/r/62594/#comment264242>
s/http_authenticatee/httpAuthenticatee/
Here and in the variable declaration below.
src/scheduler/flags.hpp
Lines 112 (patched)
<https://reviews.apache.org/r/62594/#comment264244>
Newline before the closing curly brace.
src/scheduler/scheduler.cpp
Lines 226-231 (patched)
<https://reviews.apache.org/r/62594/#comment264246>
Nit: would prefer if this change went into a separate patch.
src/scheduler/scheduler.cpp
Lines 331 (patched)
<https://reviews.apache.org/r/62594/#comment264247>
Should probably indent this line another 4 spaces?
- Greg Mann
On Oct. 2, 2017, 10:29 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62594/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2017, 10:29 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Armand Grillet, Benjamin Bannier, Greg Mann, Kapil Arya, and Vinod Kone.
>
>
> Bugs: MESOS-8021
> https://issues.apache.org/jira/browse/MESOS-8021
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated scheduler library for modularized HTTP authenticatee use.
>
>
> Diffs
> -----
>
> src/scheduler/flags.hpp 1e8efc06b40b17d6fa9d9516a83893485804fc72
> src/scheduler/scheduler.cpp 78f53707364ab988bcc53ec2c95490df04cd9a6c
>
>
> Diff: https://reviews.apache.org/r/62594/diff/3/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62594: Updated scheduler library for modularized
HTTP authenticatee use.
Posted by Till Toenshoff <to...@me.com>.
> On Oct. 10, 2017, 5:49 a.m., Greg Mann wrote:
> > src/scheduler/scheduler.cpp
> > Lines 312-318 (patched)
> > <https://reviews.apache.org/r/62594/diff/4/?file=1849799#file1849799line340>
> >
> > So, this means the basic HTTP authenticatee is loaded and called into when the user specifies no authenticatee and no credential, right? The unfortunate thing about this is that we do an unnecessary copy of all HTTP requests in the authenticatee when the user has not opted in to authentication at all.
> >
> > I can imagine a few different ways to help alleviate this:
> > 1) Store the authenticatee as an `Option`; the scheduler can call into it if it's SOME, otherwise synchronously invoke `_send()`
> > 2) Have a `NoOpAuthenticatee` which simply returns the same request
> > 3) Alter the logic in the basic authenticatee to avoid one of the extra copies if credential is NONE
> >
> > What do you think; is this worth addressing? Or not a big deal?
I am not really sure the scheduler should decide - seems the authenticatee should control on if or how authentication should happen. For optimizing away the extra copy, we should indeed simply return the request.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62594/#review187497
-----------------------------------------------------------
On Oct. 2, 2017, 10:29 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62594/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2017, 10:29 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Armand Grillet, Benjamin Bannier, Greg Mann, Kapil Arya, and Vinod Kone.
>
>
> Bugs: MESOS-8021
> https://issues.apache.org/jira/browse/MESOS-8021
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Allows the scheduler library to load an HTTP authenticatee module
> providing custom mechanisms for authentication.
>
>
> Diffs
> -----
>
> src/scheduler/flags.hpp 1e8efc06b40b17d6fa9d9516a83893485804fc72
> src/scheduler/scheduler.cpp 78f53707364ab988bcc53ec2c95490df04cd9a6c
>
>
> Diff: https://reviews.apache.org/r/62594/diff/4/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62594: Updated scheduler library for modularized
HTTP authenticatee use.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62594/#review187497
-----------------------------------------------------------
src/scheduler/scheduler.cpp
Lines 312-318 (patched)
<https://reviews.apache.org/r/62594/#comment264518>
So, this means the basic HTTP authenticatee is loaded and called into when the user specifies no authenticatee and no credential, right? The unfortunate thing about this is that we do an unnecessary copy of all HTTP requests in the authenticatee when the user has not opted in to authentication at all.
I can imagine a few different ways to help alleviate this:
1) Store the authenticatee as an `Option`; the scheduler can call into it if it's SOME, otherwise synchronously invoke `_send()`
2) Have a `NoOpAuthenticatee` which simply returns the same request
3) Alter the logic in the basic authenticatee to avoid one of the extra copies if credential is NONE
What do you think; is this worth addressing? Or not a big deal?
- Greg Mann
On Oct. 2, 2017, 10:29 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62594/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2017, 10:29 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Armand Grillet, Benjamin Bannier, Greg Mann, Kapil Arya, and Vinod Kone.
>
>
> Bugs: MESOS-8021
> https://issues.apache.org/jira/browse/MESOS-8021
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Allows the scheduler library to load an HTTP authenticatee module
> providing custom mechanisms for authentication.
>
>
> Diffs
> -----
>
> src/scheduler/flags.hpp 1e8efc06b40b17d6fa9d9516a83893485804fc72
> src/scheduler/scheduler.cpp 78f53707364ab988bcc53ec2c95490df04cd9a6c
>
>
> Diff: https://reviews.apache.org/r/62594/diff/4/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62594: Updated scheduler library for modularized
HTTP authenticatee use.
Posted by Till Toenshoff <to...@me.com>.
> On Oct. 10, 2017, 5:24 a.m., Greg Mann wrote:
> > src/scheduler/flags.hpp
> > Lines 53 (patched)
> > <https://reviews.apache.org/r/62594/diff/4/?file=1849798#file1849798line53>
> >
> > I checked a few places in the master flags, and in that file we advise users to load modules via the `--modules` flag, rather than using the environment variable as you've done here. Perhaps it would be more consistent to use `--modules` here?
The scheduler runnable does not parse for these flags on the commandline. The underlying library does and it does that only via environment variables. This means in this case we don't really offer both options, flag or environment variable but only environment variables.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62594/#review187493
-----------------------------------------------------------
On Oct. 2, 2017, 10:29 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62594/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2017, 10:29 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Armand Grillet, Benjamin Bannier, Greg Mann, Kapil Arya, and Vinod Kone.
>
>
> Bugs: MESOS-8021
> https://issues.apache.org/jira/browse/MESOS-8021
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Allows the scheduler library to load an HTTP authenticatee module
> providing custom mechanisms for authentication.
>
>
> Diffs
> -----
>
> src/scheduler/flags.hpp 1e8efc06b40b17d6fa9d9516a83893485804fc72
> src/scheduler/scheduler.cpp 78f53707364ab988bcc53ec2c95490df04cd9a6c
>
>
> Diff: https://reviews.apache.org/r/62594/diff/4/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62594: Updated scheduler library for modularized
HTTP authenticatee use.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62594/#review187493
-----------------------------------------------------------
Fix it, then Ship it!
src/scheduler/flags.hpp
Lines 53 (patched)
<https://reviews.apache.org/r/62594/#comment264516>
I checked a few places in the master flags, and in that file we advise users to load modules via the `--modules` flag, rather than using the environment variable as you've done here. Perhaps it would be more consistent to use `--modules` here?
- Greg Mann
On Oct. 2, 2017, 10:29 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62594/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2017, 10:29 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Armand Grillet, Benjamin Bannier, Greg Mann, Kapil Arya, and Vinod Kone.
>
>
> Bugs: MESOS-8021
> https://issues.apache.org/jira/browse/MESOS-8021
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Allows the scheduler library to load an HTTP authenticatee module
> providing custom mechanisms for authentication.
>
>
> Diffs
> -----
>
> src/scheduler/flags.hpp 1e8efc06b40b17d6fa9d9516a83893485804fc72
> src/scheduler/scheduler.cpp 78f53707364ab988bcc53ec2c95490df04cd9a6c
>
>
> Diff: https://reviews.apache.org/r/62594/diff/4/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62594: Updated scheduler library for modularized
HTTP authenticatee use.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62594/
-----------------------------------------------------------
(Updated Oct. 2, 2017, 10:29 a.m.)
Review request for mesos, Anand Mazumdar, Armand Grillet, Benjamin Bannier, Greg Mann, Kapil Arya, and Vinod Kone.
Changes
-------
addressed most comments - still needs a validation on the discarded state invariant check.
Bugs: MESOS-8021
https://issues.apache.org/jira/browse/MESOS-8021
Repository: mesos
Description (updated)
-------
Updated scheduler library for modularized HTTP authenticatee use.
Diffs (updated)
-----
src/scheduler/flags.hpp 1e8efc06b40b17d6fa9d9516a83893485804fc72
src/scheduler/scheduler.cpp 78f53707364ab988bcc53ec2c95490df04cd9a6c
Diff: https://reviews.apache.org/r/62594/diff/2/
Changes: https://reviews.apache.org/r/62594/diff/1-2/
Testing
-------
Integration test in external project. Further tests upcoming.
Thanks,
Till Toenshoff