You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Dominic Hamon <dh...@twopensource.com> on 2014/10/16 21:06:14 UTC
Review Request 26817: Split authorization from task validation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26817/
-----------------------------------------------------------
Review request for mesos and Vinod Kone.
Repository: mesos-git
Description
-------
split authorization from task validation. allows https://reviews.apache.org/r/26382/ to differentiate between validation issues and task authorization issues.
also replaced some > > with >> and changed the order of methods in the cpp to match the continuation order.
Diffs
-----
src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205
src/master/master.cpp 1b1ce0de3065ced45890777d42c69ef1091f5699
Diff: https://reviews.apache.org/r/26817/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 26817: Split authorization from task validation.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26817/#review57202
-----------------------------------------------------------
Hm.. could you please split the code movement out into a separate change so that it's easier to see what you're changing here? Hard to see what's changed inside within the continuation since it's been moved as well.
- Ben Mahler
On Oct. 17, 2014, 7:53 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26817/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2014, 7:53 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> split authorization from task validation. allows https://reviews.apache.org/r/26382/ to differentiate between validation issues and task authorization issues.
>
> also replaced some > > with >> and changed the order of methods in the cpp to match the continuation order.
>
>
> Diffs
> -----
>
> src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205
> src/master/master.cpp 1b1ce0de3065ced45890777d42c69ef1091f5699
>
> Diff: https://reviews.apache.org/r/26817/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 26817: Split authorization from task validation.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26817/#review57639
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On Oct. 21, 2014, 7:03 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26817/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2014, 7:03 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> split authorization from task validation. allows https://reviews.apache.org/r/26382/ to differentiate between validation issues and task authorization issues.
>
> also replaced some > > with >> and changed the order of methods in the cpp to match the continuation order.
>
>
> Diffs
> -----
>
> src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205
> src/master/master.cpp 0a5c9a374062a241c90ea238725fbb8dd2408ef4
>
> Diff: https://reviews.apache.org/r/26817/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 26817: Split authorization from task validation.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26817/
-----------------------------------------------------------
(Updated Oct. 21, 2014, 12:03 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Repository: mesos-git
Description
-------
split authorization from task validation. allows https://reviews.apache.org/r/26382/ to differentiate between validation issues and task authorization issues.
also replaced some > > with >> and changed the order of methods in the cpp to match the continuation order.
Diffs (updated)
-----
src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205
src/master/master.cpp 0a5c9a374062a241c90ea238725fbb8dd2408ef4
Diff: https://reviews.apache.org/r/26817/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 26817: Split authorization from task validation.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26817/
-----------------------------------------------------------
(Updated Oct. 21, 2014, 10:39 a.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Repository: mesos-git
Description
-------
split authorization from task validation. allows https://reviews.apache.org/r/26382/ to differentiate between validation issues and task authorization issues.
also replaced some > > with >> and changed the order of methods in the cpp to match the continuation order.
Diffs (updated)
-----
src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205
src/master/master.cpp 0a5c9a374062a241c90ea238725fbb8dd2408ef4
Diff: https://reviews.apache.org/r/26817/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 26817: Split authorization from task validation.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Oct. 20, 2014, 5:56 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 2546
> > <https://reviews.apache.org/r/26817/diff/4/?file=725867#file725867line2546>
> >
> > I think now you can just do foreach on authorizations now?
>
> Vinod Kone wrote:
> dropped without comment?
i don't think so - i need the index for the tasks, the validations, and the authorizations.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26817/#review57489
-----------------------------------------------------------
On Oct. 21, 2014, 10:39 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26817/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2014, 10:39 a.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> split authorization from task validation. allows https://reviews.apache.org/r/26382/ to differentiate between validation issues and task authorization issues.
>
> also replaced some > > with >> and changed the order of methods in the cpp to match the continuation order.
>
>
> Diffs
> -----
>
> src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205
> src/master/master.cpp 0a5c9a374062a241c90ea238725fbb8dd2408ef4
>
> Diff: https://reviews.apache.org/r/26817/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 26817: Split authorization from task validation.
Posted by Vinod Kone <vi...@gmail.com>.
> On Oct. 21, 2014, 12:56 a.m., Vinod Kone wrote:
> > src/master/master.cpp, line 2546
> > <https://reviews.apache.org/r/26817/diff/4/?file=725867#file725867line2546>
> >
> > I think now you can just do foreach on authorizations now?
dropped without comment?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26817/#review57489
-----------------------------------------------------------
On Oct. 21, 2014, 5:39 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26817/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2014, 5:39 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> split authorization from task validation. allows https://reviews.apache.org/r/26382/ to differentiate between validation issues and task authorization issues.
>
> also replaced some > > with >> and changed the order of methods in the cpp to match the continuation order.
>
>
> Diffs
> -----
>
> src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205
> src/master/master.cpp 0a5c9a374062a241c90ea238725fbb8dd2408ef4
>
> Diff: https://reviews.apache.org/r/26817/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 26817: Split authorization from task validation.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26817/#review57489
-----------------------------------------------------------
src/master/master.hpp
<https://reviews.apache.org/r/26817/#comment98205>
fix indentation of arguments.
src/master/master.cpp
<https://reviews.apache.org/r/26817/#comment98207>
s/only/Only/
src/master/master.cpp
<https://reviews.apache.org/r/26817/#comment98224>
I think now you can just do foreach on authorizations now?
- Vinod Kone
On Oct. 20, 2014, 5:25 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26817/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2014, 5:25 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> split authorization from task validation. allows https://reviews.apache.org/r/26382/ to differentiate between validation issues and task authorization issues.
>
> also replaced some > > with >> and changed the order of methods in the cpp to match the continuation order.
>
>
> Diffs
> -----
>
> src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205
> src/master/master.cpp 0a5c9a374062a241c90ea238725fbb8dd2408ef4
>
> Diff: https://reviews.apache.org/r/26817/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 26817: Split authorization from task validation.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26817/
-----------------------------------------------------------
(Updated Oct. 20, 2014, 10:25 a.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Repository: mesos-git
Description
-------
split authorization from task validation. allows https://reviews.apache.org/r/26382/ to differentiate between validation issues and task authorization issues.
also replaced some > > with >> and changed the order of methods in the cpp to match the continuation order.
Diffs (updated)
-----
src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205
src/master/master.cpp 0a5c9a374062a241c90ea238725fbb8dd2408ef4
Diff: https://reviews.apache.org/r/26817/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 26817: Split authorization from task validation.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26817/
-----------------------------------------------------------
(Updated Oct. 17, 2014, 1:46 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
made validation sychronous to simplify logic.
Repository: mesos-git
Description
-------
split authorization from task validation. allows https://reviews.apache.org/r/26382/ to differentiate between validation issues and task authorization issues.
also replaced some > > with >> and changed the order of methods in the cpp to match the continuation order.
Diffs (updated)
-----
src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205
src/master/master.cpp 1b1ce0de3065ced45890777d42c69ef1091f5699
Diff: https://reviews.apache.org/r/26817/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 26817: Split authorization from task validation.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26817/
-----------------------------------------------------------
(Updated Oct. 17, 2014, 1:09 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
moved the continuation back to make the changes more palatable.
Repository: mesos-git
Description
-------
split authorization from task validation. allows https://reviews.apache.org/r/26382/ to differentiate between validation issues and task authorization issues.
also replaced some > > with >> and changed the order of methods in the cpp to match the continuation order.
Diffs (updated)
-----
src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205
src/master/master.cpp 1b1ce0de3065ced45890777d42c69ef1091f5699
Diff: https://reviews.apache.org/r/26817/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 26817: Split authorization from task validation.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26817/
-----------------------------------------------------------
(Updated Oct. 17, 2014, 7:53 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
Adding myself as a reviewer.
--bmahler
Repository: mesos-git
Description
-------
split authorization from task validation. allows https://reviews.apache.org/r/26382/ to differentiate between validation issues and task authorization issues.
also replaced some > > with >> and changed the order of methods in the cpp to match the continuation order.
Diffs
-----
src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205
src/master/master.cpp 1b1ce0de3065ced45890777d42c69ef1091f5699
Diff: https://reviews.apache.org/r/26817/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 26817: Split authorization from task validation.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Oct. 17, 2014, 12:29 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 2358-2361
> > <https://reviews.apache.org/r/26817/diff/1/?file=723143#file723143line2358>
> >
> > We should only proceed with authorization if validations succeeded. Otherwise there is no point.
> >
> > Also, it is a bit weird that we do all validations for all tasks and then do all authorizations for all tasks. I wonder if we can solve the original problem (not able to distinguish between validation failure vs authorization failure) differently, e.g., by returning ValidationError instead of Error, where the former contains more context.
> >
> > struct ValidationError : Error
> > {
> > enum Reason;
> > }
> >
> > Ofcourse this assumes, the "Reason" patch lands first.
> >
> > Thoughts?
I thought the same, and originally started down the path of doing both, but doing both asynchronously was tricky.
If validation doesn't need to be a future, then we can just do the validations synchronously and then do the authentication, but returning the right thing would, indeed, need to include a Reason.
Landing the other patch first is hard because we can't differentiate between the authentication/validation cases without this patch :)
> On Oct. 17, 2014, 12:29 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 2586-2589
> > <https://reviews.apache.org/r/26817/diff/1/?file=723143#file723143line2586>
> >
> > The caller shouldn't call this method if authorization is disabled. Move this logic to the caller.
if not, then the logic gets more complex as we can't assume that authorizations and validations are vectors of the same size. This may change if validations aren't asynchronous and we can return a reason directly.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26817/#review57190
-----------------------------------------------------------
On Oct. 17, 2014, 1:09 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26817/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2014, 1:09 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> split authorization from task validation. allows https://reviews.apache.org/r/26382/ to differentiate between validation issues and task authorization issues.
>
> also replaced some > > with >> and changed the order of methods in the cpp to match the continuation order.
>
>
> Diffs
> -----
>
> src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205
> src/master/master.cpp 1b1ce0de3065ced45890777d42c69ef1091f5699
>
> Diff: https://reviews.apache.org/r/26817/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 26817: Split authorization from task validation.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26817/#review57190
-----------------------------------------------------------
src/master/master.hpp
<https://reviews.apache.org/r/26817/#comment97724>
// Returns true if task is authorized.
// Returns false if task is not authorized.
// Returns failure for transient authorization failures.
src/master/master.cpp
<https://reviews.apache.org/r/26817/#comment97729>
Hmm. This should be rethought now because there is no reason validateTask should return a future anymore. The only async part of validation was authentication, which is now split out of it.
src/master/master.cpp
<https://reviews.apache.org/r/26817/#comment97731>
We should only proceed with authorization if validations succeeded. Otherwise there is no point.
Also, it is a bit weird that we do all validations for all tasks and then do all authorizations for all tasks. I wonder if we can solve the original problem (not able to distinguish between validation failure vs authorization failure) differently, e.g., by returning ValidationError instead of Error, where the former contains more context.
struct ValidationError : Error
{
enum Reason;
}
Ofcourse this assumes, the "Reason" patch lands first.
Thoughts?
src/master/master.cpp
<https://reviews.apache.org/r/26817/#comment97735>
The caller shouldn't call this method if authorization is disabled. Move this logic to the caller.
- Vinod Kone
On Oct. 16, 2014, 7:06 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26817/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2014, 7:06 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> split authorization from task validation. allows https://reviews.apache.org/r/26382/ to differentiate between validation issues and task authorization issues.
>
> also replaced some > > with >> and changed the order of methods in the cpp to match the continuation order.
>
>
> Diffs
> -----
>
> src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205
> src/master/master.cpp 1b1ce0de3065ced45890777d42c69ef1091f5699
>
> Diff: https://reviews.apache.org/r/26817/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>