You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2014/05/20 22:34:48 UTC
Review Request 21723: Integrate Authorizer into Master.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21723/
-----------------------------------------------------------
Review request for mesos and Benjamin Hindman.
Bugs: MESOS-1387
https://issues.apache.org/jira/browse/MESOS-1387
Repository: mesos-git
Description
-------
Master just initializes the Authorizer with ACLs, doesn't do any authorization yet.
Diffs
-----
src/authorizer/authorizer.hpp 75b5a33cc2d83016598e5858d537ce1106784b41
src/master/flags.hpp db21ab02f799e1eb33de40b6aa964a7d0e6477c9
src/master/master.hpp 5e0d712de997bd10079655df9b07099284f8257f
src/master/master.cpp 075755cad5c50a57c92d7d82f2466b467796f673
src/tests/mesos.cpp 7e5e96a4de29b56a906716fc2e03ae4fce4a8584
Diff: https://reviews.apache.org/r/21723/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 21723: Integrate Authorizer into Master.
Posted by Vinod Kone <vi...@gmail.com>.
> On May 21, 2014, 11:39 p.m., Benjamin Hindman wrote:
> > src/authorizer/authorizer.hpp, line 284
> > <https://reviews.apache.org/r/21723/diff/1/?file=585504#file585504line284>
> >
> > Why is this an error? Why not just assume an empty flags.acls is equal to '{}'?
>
> Vinod Kone wrote:
> The main reason is that I want Master to instantiate an Authorizer only when ACLs are specified (they could be {} but that is still considered specified). That way we can turn off the Authorizer code path (for whatever reason) by not specifying the "acls" flag. Given these semantics, it seems odd to silently assume flags.acls.isNone() is equivalent to "{}". I would rather the user of Authorizer think about this. Do you have any use case in mind where your suggestion makes sense?
>
> Benjamin Hindman wrote:
> But the master is already doing this IIUC, by checking flags.acls.isSome() and only doing Authorizer::create when there are some ACLs. But otherwise it seemed like the ACLs being optional are just whether or not you have any ACLs, not whether or not you want an Authorizer, and if you've called Authorizer::create it seems like you definitely want an Authorizer!
Yes master does it. From your suggestion, flags.acls.isNone() means "don't use authorizer" in master but means "use it with no ACLs" in authorizer. That seems a bit weird considering the later can be specified explicitly. Also, per my reply to Dominic's comment, how about I just take JSON for now and punt on this altogether :)
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21723/#review43674
-----------------------------------------------------------
On May 22, 2014, 12:39 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21723/
> -----------------------------------------------------------
>
> (Updated May 22, 2014, 12:39 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1387
> https://issues.apache.org/jira/browse/MESOS-1387
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Master just initializes the Authorizer with ACLs, doesn't do any authorization yet.
>
>
> Diffs
> -----
>
> src/authorizer/authorizer.hpp 75b5a33cc2d83016598e5858d537ce1106784b41
> src/master/flags.hpp db21ab02f799e1eb33de40b6aa964a7d0e6477c9
> src/master/master.hpp 6f51eadbcfb8c332bf33bbbd0aa0c4ba3ad7b61d
> src/master/master.cpp dc078de7e323e00e29f8dd3c55baa2b126d314b3
> src/tests/mesos.cpp 7e5e96a4de29b56a906716fc2e03ae4fce4a8584
>
> Diff: https://reviews.apache.org/r/21723/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 21723: Integrate Authorizer into Master.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On May 21, 2014, 11:39 p.m., Benjamin Hindman wrote:
> > src/authorizer/authorizer.hpp, line 284
> > <https://reviews.apache.org/r/21723/diff/1/?file=585504#file585504line284>
> >
> > Why is this an error? Why not just assume an empty flags.acls is equal to '{}'?
>
> Vinod Kone wrote:
> The main reason is that I want Master to instantiate an Authorizer only when ACLs are specified (they could be {} but that is still considered specified). That way we can turn off the Authorizer code path (for whatever reason) by not specifying the "acls" flag. Given these semantics, it seems odd to silently assume flags.acls.isNone() is equivalent to "{}". I would rather the user of Authorizer think about this. Do you have any use case in mind where your suggestion makes sense?
>
> Benjamin Hindman wrote:
> But the master is already doing this IIUC, by checking flags.acls.isSome() and only doing Authorizer::create when there are some ACLs. But otherwise it seemed like the ACLs being optional are just whether or not you have any ACLs, not whether or not you want an Authorizer, and if you've called Authorizer::create it seems like you definitely want an Authorizer!
>
> Vinod Kone wrote:
> Yes master does it. From your suggestion, flags.acls.isNone() means "don't use authorizer" in master but means "use it with no ACLs" in authorizer. That seems a bit weird considering the later can be specified explicitly. Also, per my reply to Dominic's comment, how about I just take JSON for now and punt on this altogether :)
SGTM!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21723/#review43674
-----------------------------------------------------------
On May 22, 2014, 12:39 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21723/
> -----------------------------------------------------------
>
> (Updated May 22, 2014, 12:39 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1387
> https://issues.apache.org/jira/browse/MESOS-1387
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Master just initializes the Authorizer with ACLs, doesn't do any authorization yet.
>
>
> Diffs
> -----
>
> src/authorizer/authorizer.hpp 75b5a33cc2d83016598e5858d537ce1106784b41
> src/master/flags.hpp db21ab02f799e1eb33de40b6aa964a7d0e6477c9
> src/master/master.hpp 6f51eadbcfb8c332bf33bbbd0aa0c4ba3ad7b61d
> src/master/master.cpp dc078de7e323e00e29f8dd3c55baa2b126d314b3
> src/tests/mesos.cpp 7e5e96a4de29b56a906716fc2e03ae4fce4a8584
>
> Diff: https://reviews.apache.org/r/21723/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 21723: Integrate Authorizer into Master.
Posted by Vinod Kone <vi...@gmail.com>.
> On May 21, 2014, 11:39 p.m., Benjamin Hindman wrote:
> > src/authorizer/authorizer.hpp, line 284
> > <https://reviews.apache.org/r/21723/diff/1/?file=585504#file585504line284>
> >
> > Why is this an error? Why not just assume an empty flags.acls is equal to '{}'?
The main reason is that I want Master to instantiate an Authorizer only when ACLs are specified (they could be {} but that is still considered specified). That way we can turn off the Authorizer code path (for whatever reason) by not specifying the "acls" flag. Given these semantics, it seems odd to silently assume flags.acls.isNone() is equivalent to "{}". I would rather the user of Authorizer think about this. Do you have any use case in mind where your suggestion makes sense?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21723/#review43674
-----------------------------------------------------------
On May 20, 2014, 8:34 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21723/
> -----------------------------------------------------------
>
> (Updated May 20, 2014, 8:34 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1387
> https://issues.apache.org/jira/browse/MESOS-1387
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Master just initializes the Authorizer with ACLs, doesn't do any authorization yet.
>
>
> Diffs
> -----
>
> src/authorizer/authorizer.hpp 75b5a33cc2d83016598e5858d537ce1106784b41
> src/master/flags.hpp db21ab02f799e1eb33de40b6aa964a7d0e6477c9
> src/master/master.hpp 5e0d712de997bd10079655df9b07099284f8257f
> src/master/master.cpp 075755cad5c50a57c92d7d82f2466b467796f673
> src/tests/mesos.cpp 7e5e96a4de29b56a906716fc2e03ae4fce4a8584
>
> Diff: https://reviews.apache.org/r/21723/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 21723: Integrate Authorizer into Master.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On May 21, 2014, 11:39 p.m., Benjamin Hindman wrote:
> > src/authorizer/authorizer.hpp, line 284
> > <https://reviews.apache.org/r/21723/diff/1/?file=585504#file585504line284>
> >
> > Why is this an error? Why not just assume an empty flags.acls is equal to '{}'?
>
> Vinod Kone wrote:
> The main reason is that I want Master to instantiate an Authorizer only when ACLs are specified (they could be {} but that is still considered specified). That way we can turn off the Authorizer code path (for whatever reason) by not specifying the "acls" flag. Given these semantics, it seems odd to silently assume flags.acls.isNone() is equivalent to "{}". I would rather the user of Authorizer think about this. Do you have any use case in mind where your suggestion makes sense?
But the master is already doing this IIUC, by checking flags.acls.isSome() and only doing Authorizer::create when there are some ACLs. But otherwise it seemed like the ACLs being optional are just whether or not you have any ACLs, not whether or not you want an Authorizer, and if you've called Authorizer::create it seems like you definitely want an Authorizer!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21723/#review43674
-----------------------------------------------------------
On May 20, 2014, 8:34 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21723/
> -----------------------------------------------------------
>
> (Updated May 20, 2014, 8:34 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1387
> https://issues.apache.org/jira/browse/MESOS-1387
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Master just initializes the Authorizer with ACLs, doesn't do any authorization yet.
>
>
> Diffs
> -----
>
> src/authorizer/authorizer.hpp 75b5a33cc2d83016598e5858d537ce1106784b41
> src/master/flags.hpp db21ab02f799e1eb33de40b6aa964a7d0e6477c9
> src/master/master.hpp 5e0d712de997bd10079655df9b07099284f8257f
> src/master/master.cpp 075755cad5c50a57c92d7d82f2466b467796f673
> src/tests/mesos.cpp 7e5e96a4de29b56a906716fc2e03ae4fce4a8584
>
> Diff: https://reviews.apache.org/r/21723/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 21723: Integrate Authorizer into Master.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21723/#review43674
-----------------------------------------------------------
Ship it!
src/authorizer/authorizer.hpp
<https://reviews.apache.org/r/21723/#comment77963>
Why is this an error? Why not just assume an empty flags.acls is equal to '{}'?
src/tests/mesos.cpp
<https://reviews.apache.org/r/21723/#comment77965>
Why not just 'flags.acls = JSON::Object();'?
- Benjamin Hindman
On May 20, 2014, 8:34 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21723/
> -----------------------------------------------------------
>
> (Updated May 20, 2014, 8:34 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1387
> https://issues.apache.org/jira/browse/MESOS-1387
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Master just initializes the Authorizer with ACLs, doesn't do any authorization yet.
>
>
> Diffs
> -----
>
> src/authorizer/authorizer.hpp 75b5a33cc2d83016598e5858d537ce1106784b41
> src/master/flags.hpp db21ab02f799e1eb33de40b6aa964a7d0e6477c9
> src/master/master.hpp 5e0d712de997bd10079655df9b07099284f8257f
> src/master/master.cpp 075755cad5c50a57c92d7d82f2466b467796f673
> src/tests/mesos.cpp 7e5e96a4de29b56a906716fc2e03ae4fce4a8584
>
> Diff: https://reviews.apache.org/r/21723/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 21723: Integrate Authorizer into Master.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21723/
-----------------------------------------------------------
(Updated May 22, 2014, 1:21 a.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Authorizer now takes JSON acls instead of master flags. NNFR.
Bugs: MESOS-1387
https://issues.apache.org/jira/browse/MESOS-1387
Repository: mesos-git
Description
-------
Master just initializes the Authorizer with ACLs, doesn't do any authorization yet.
Diffs (updated)
-----
src/authorizer/authorizer.hpp a8fde5a15f95d5af4350f9562f04817f802964e5
src/master/flags.hpp db21ab02f799e1eb33de40b6aa964a7d0e6477c9
src/master/master.hpp 6f51eadbcfb8c332bf33bbbd0aa0c4ba3ad7b61d
src/master/master.cpp dc078de7e323e00e29f8dd3c55baa2b126d314b3
src/tests/mesos.cpp 7e5e96a4de29b56a906716fc2e03ae4fce4a8584
Diff: https://reviews.apache.org/r/21723/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 21723: Integrate Authorizer into Master.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21723/
-----------------------------------------------------------
(Updated May 22, 2014, 12:39 a.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
rebased.
Bugs: MESOS-1387
https://issues.apache.org/jira/browse/MESOS-1387
Repository: mesos-git
Description
-------
Master just initializes the Authorizer with ACLs, doesn't do any authorization yet.
Diffs (updated)
-----
src/authorizer/authorizer.hpp 75b5a33cc2d83016598e5858d537ce1106784b41
src/master/flags.hpp db21ab02f799e1eb33de40b6aa964a7d0e6477c9
src/master/master.hpp 6f51eadbcfb8c332bf33bbbd0aa0c4ba3ad7b61d
src/master/master.cpp dc078de7e323e00e29f8dd3c55baa2b126d314b3
src/tests/mesos.cpp 7e5e96a4de29b56a906716fc2e03ae4fce4a8584
Diff: https://reviews.apache.org/r/21723/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 21723: Integrate Authorizer into Master.
Posted by Vinod Kone <vi...@gmail.com>.
> On May 20, 2014, 9:38 p.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 339
> > <https://reviews.apache.org/r/21723/diff/1/?file=585507#file585507line339>
> >
> > are you going to add tests?
i did. by specifying acls in mesos.cpp :) more tests will be added in subsequent reviews when the authorizer is actually used to do something.
> On May 20, 2014, 9:38 p.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 341
> > <https://reviews.apache.org/r/21723/diff/1/?file=585507#file585507line341>
> >
> > would it make more sense to pass flags.acls here to avoid the check in the authorizer?
the reason i have flags here is because in the future i imagine there would be more ACL specific flags (e.g., --ldap_user, --ldap_password) that need to be passed in. im ok with takings JSON<Object> for now and revisit in the future. that likely also addresses benh's comment below. thoughts?
> On May 20, 2014, 9:38 p.m., Dominic Hamon wrote:
> > src/tests/mesos.cpp, line 103
> > <https://reviews.apache.org/r/21723/diff/1/?file=585508#file585508line103>
> >
> > Try<JSON::Object> acls = JSON::Object();
> >
> > should work.
done
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21723/#review43541
-----------------------------------------------------------
On May 20, 2014, 8:34 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21723/
> -----------------------------------------------------------
>
> (Updated May 20, 2014, 8:34 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1387
> https://issues.apache.org/jira/browse/MESOS-1387
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Master just initializes the Authorizer with ACLs, doesn't do any authorization yet.
>
>
> Diffs
> -----
>
> src/authorizer/authorizer.hpp 75b5a33cc2d83016598e5858d537ce1106784b41
> src/master/flags.hpp db21ab02f799e1eb33de40b6aa964a7d0e6477c9
> src/master/master.hpp 5e0d712de997bd10079655df9b07099284f8257f
> src/master/master.cpp 075755cad5c50a57c92d7d82f2466b467796f673
> src/tests/mesos.cpp 7e5e96a4de29b56a906716fc2e03ae4fce4a8584
>
> Diff: https://reviews.apache.org/r/21723/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 21723: Integrate Authorizer into Master.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21723/#review43541
-----------------------------------------------------------
src/master/master.cpp
<https://reviews.apache.org/r/21723/#comment77751>
are you going to add tests?
src/master/master.cpp
<https://reviews.apache.org/r/21723/#comment77749>
would it make more sense to pass flags.acls here to avoid the check in the authorizer?
src/tests/mesos.cpp
<https://reviews.apache.org/r/21723/#comment77750>
Try<JSON::Object> acls = JSON::Object();
should work.
- Dominic Hamon
On May 20, 2014, 1:34 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21723/
> -----------------------------------------------------------
>
> (Updated May 20, 2014, 1:34 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1387
> https://issues.apache.org/jira/browse/MESOS-1387
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Master just initializes the Authorizer with ACLs, doesn't do any authorization yet.
>
>
> Diffs
> -----
>
> src/authorizer/authorizer.hpp 75b5a33cc2d83016598e5858d537ce1106784b41
> src/master/flags.hpp db21ab02f799e1eb33de40b6aa964a7d0e6477c9
> src/master/master.hpp 5e0d712de997bd10079655df9b07099284f8257f
> src/master/master.cpp 075755cad5c50a57c92d7d82f2466b467796f673
> src/tests/mesos.cpp 7e5e96a4de29b56a906716fc2e03ae4fce4a8584
>
> Diff: https://reviews.apache.org/r/21723/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>