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/06/06 02:12:54 UTC
Review Request 22284: Authorized roles.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22284/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Ben Mahler.
Summary (updated)
-----------------
Authorized roles.
Bugs: MESOS-1307
https://issues.apache.org/jira/browse/MESOS-1307
Repository: mesos-git
Description (updated)
-------
Added authorization for roles during framework (re-)registration.
Diffs (updated)
-----
src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149
src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
src/tests/master_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22284/diff/
Testing (updated)
-------
make check
Thanks,
Vinod Kone
Re: Review Request 22284: Authorized roles.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 6, 2014, 8:53 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 664
> > <https://reviews.apache.org/r/22284/diff/1/?file=604519#file604519line664>
> >
> > Same comment as previous review and yan's review, how about having this return a validation result?
> >
> > i.e.
> >
> > // Returns the validation result.
> > Future<Option<Error>> validate(...);
done
> On June 6, 2014, 8:53 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1052-1064
> > <https://reviews.apache.org/r/22284/diff/1/?file=604520#file604520line1052>
> >
> > How much does this slow down the tests?
> >
> > One suggestion: what if we add a backoff (including initial jitter) akin to what's done in the slave? That way, we can scale better to a large number of frameworks and we don't have to have this hack in the master. Of course, we would want to make the backoff for framework registration smaller than what's done in the slaves.
> >
> > If the slowdown is not so bad, we could remove this re-queueing, leave a ticket and TODO in sched.cpp and follow up after this change. Each time I look at this, it's difficult to understand the invariants that make the re-queuing correct so my brain would appreciate it :)
> >
> > It means that we could move this case into 'validate' as an invalid case, which makes the code simpler.
It can slow them down up to a second and if there are tests that pause the clock during registration it will break them. Jitter is a good idea, but it is still a race. I would like to punt on this for now, and circle back later since this is not new and we already have TODO/ticket to track.
> On June 6, 2014, 8:53 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1097-1111
> > <https://reviews.apache.org/r/22284/diff/1/?file=604520#file604520line1097>
> >
> > Curious if these checks here and in _reregisterFramework are necessary for correctness or just good to have? Not saying that we should remove them, but if necessary for correctness might be good to spell out why.
it is only important in _reregisterFramework() when another framework is authenticated. in all other cases, it is just to do the right thing. updated the comment.
> On June 6, 2014, 8:53 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1099
> > <https://reviews.apache.org/r/22284/diff/1/?file=604520#file604520line1099>
> >
> > How about "in the interim" instead of "before we are here", seems to more precisely describe the situation?
if we say "in the interim" we have to say in the interim of "what". "before we are here" seems appropriate.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22284/#review44959
-----------------------------------------------------------
On June 9, 2014, 10:09 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22284/
> -----------------------------------------------------------
>
> (Updated June 9, 2014, 10:09 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1307
> https://issues.apache.org/jira/browse/MESOS-1307
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added authorization for roles during framework (re-)registration.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149
> src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
> src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22284/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22284: Authorized roles.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22284/#review44959
-----------------------------------------------------------
src/master/master.hpp
<https://reviews.apache.org/r/22284/#comment79568>
Same comment as previous review and yan's review, how about having this return a validation result?
i.e.
// Returns the validation result.
Future<Option<Error>> validate(...);
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79561>
How much does this slow down the tests?
One suggestion: what if we add a backoff (including initial jitter) akin to what's done in the slave? That way, we can scale better to a large number of frameworks and we don't have to have this hack in the master. Of course, we would want to make the backoff for framework registration smaller than what's done in the slaves.
If the slowdown is not so bad, we could remove this re-queueing, leave a ticket and TODO in sched.cpp and follow up after this change. Each time I look at this, it's difficult to understand the invariants that make the re-queuing correct so my brain would appreciate it :)
It means that we could move this case into 'validate' as an invalid case, which makes the code simpler.
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79569>
Curious if these checks here and in _reregisterFramework are necessary for correctness or just good to have? Not saying that we should remove them, but if necessary for correctness might be good to spell out why.
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79563>
How about "in the interim" instead of "before we are here", seems to more precisely describe the situation?
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79564>
Ditto.
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79565>
ditto
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79566>
ditto
- Ben Mahler
On June 6, 2014, 12:12 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22284/
> -----------------------------------------------------------
>
> (Updated June 6, 2014, 12:12 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1307
> https://issues.apache.org/jira/browse/MESOS-1307
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added authorization for roles during framework (re-)registration.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149
> src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
> src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22284/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22284: Authorized roles.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 10, 2014, 1:01 a.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 994-1002
> > <https://reviews.apache.org/r/22284/diff/2/?file=606172#file606172line994>
> >
> > This could be re-used from your other change right?
not sure what you mean. this is reused. just moved it up.
> On June 10, 2014, 1:01 a.m., Ben Mahler wrote:
> > src/master/master.hpp, lines 662-665
> > <https://reviews.apache.org/r/22284/diff/2/?file=606171#file606171line662>
> >
> > Ditto my comment on the previous review about saying validation fails for the invalid case:
> >
> > // Returns None if the framework is valid.
> > // Returns a validation Error if the framework is invalid.
> > // Returns a Failure if an authorization failure occurs.
done.
> On June 10, 2014, 1:01 a.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1034
> > <https://reviews.apache.org/r/22284/diff/2/?file=606172#file606172line1034>
> >
> > Remove period?
oops.
> On June 10, 2014, 1:01 a.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1101-1102
> > <https://reviews.apache.org/r/22284/diff/2/?file=606172#file606172line1101>
> >
> > Do we want to log the framework id here and below?
it is registration. there is no framework id yet.
> On June 10, 2014, 1:01 a.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1111
> > <https://reviews.apache.org/r/22284/diff/2/?file=606172#file606172line1111>
> >
> > Using "new" here and "another" below?
clarified the comments.
> On June 10, 2014, 1:01 a.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1222-1223
> > <https://reviews.apache.org/r/22284/diff/2/?file=606172#file606172line1222>
> >
> > For all these cases, rather than "before we are here" or "interim", how about a precise description:
> >
> > // This occurs when another authentication request arrived while validation was in progress.
> >
> > Does that seem like a more precise way to write these comments?
better. fixed.
> On June 10, 2014, 1:01 a.m., Ben Mahler wrote:
> > src/tests/master_authorization_tests.cpp, lines 840-843
> > <https://reviews.apache.org/r/22284/diff/2/?file=606173#file606173line840>
> >
> > Ditto naming.
> >
> > Is the lack of 'future1' here to imply that there is an implicit initial authorization?
yea. i didn't want the comment to say "second attempt" and the variable to be "future1".
> On June 10, 2014, 1:01 a.m., Ben Mahler wrote:
> > src/tests/master_authorization_tests.cpp, lines 724-727
> > <https://reviews.apache.org/r/22284/diff/2/?file=606173#file606173line724>
> >
> > A bit confusing, how about calling the futures: 'authorize1' and 'authorize2'? Would read better in the AWAIT_ statements below.
hmm. not sure. what about promises? it seems weird if authorization promise is called 'promise' and its future is called 'authorize'.
> On June 10, 2014, 1:01 a.m., Ben Mahler wrote:
> > src/tests/master_authorization_tests.cpp, lines 818-822
> > <https://reviews.apache.org/r/22284/diff/2/?file=606173#file606173line818>
> >
> > It seems like the duplicate tests should be grouped together and the removed tests should be grouped together. It is trivial to understand the 'DuplicateReregistration' test if reading it immediately after the 'DuplicateRegistration' test.
done
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22284/#review45180
-----------------------------------------------------------
On June 9, 2014, 10:09 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22284/
> -----------------------------------------------------------
>
> (Updated June 9, 2014, 10:09 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1307
> https://issues.apache.org/jira/browse/MESOS-1307
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added authorization for roles during framework (re-)registration.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149
> src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
> src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22284/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22284: Authorized roles.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22284/#review45180
-----------------------------------------------------------
Ship it!
src/master/master.hpp
<https://reviews.apache.org/r/22284/#comment79889>
Ditto my comment on the previous review about saying validation fails for the invalid case:
// Returns None if the framework is valid.
// Returns a validation Error if the framework is invalid.
// Returns a Failure if an authorization failure occurs.
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79890>
This could be re-used from your other change right?
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79891>
Remove period?
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79892>
Indentation is off here.
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79893>
Oh, now I see why you wanted to pass the failure message. :)
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79894>
Do we want to log the framework id here and below?
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79897>
Using "new" here and "another" below?
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79898>
"Master"
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79900>
For all these cases, rather than "before we are here" or "interim", how about a precise description:
// This occurs when another authentication request arrived while validation was in progress.
Does that seem like a more precise way to write these comments?
src/tests/master_authorization_tests.cpp
<https://reviews.apache.org/r/22284/#comment79904>
Nice test!!!
src/tests/master_authorization_tests.cpp
<https://reviews.apache.org/r/22284/#comment79903>
A bit confusing, how about calling the futures: 'authorize1' and 'authorize2'? Would read better in the AWAIT_ statements below.
src/tests/master_authorization_tests.cpp
<https://reviews.apache.org/r/22284/#comment79906>
It seems like the duplicate tests should be grouped together and the removed tests should be grouped together. It is trivial to understand the 'DuplicateReregistration' test if reading it immediately after the 'DuplicateRegistration' test.
src/tests/master_authorization_tests.cpp
<https://reviews.apache.org/r/22284/#comment79907>
Ditto naming.
Is the lack of 'future1' here to imply that there is an implicit initial authorization?
src/tests/master_authorization_tests.cpp
<https://reviews.apache.org/r/22284/#comment79908>
Ditto naming.
- Ben Mahler
On June 9, 2014, 10:09 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22284/
> -----------------------------------------------------------
>
> (Updated June 9, 2014, 10:09 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1307
> https://issues.apache.org/jira/browse/MESOS-1307
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added authorization for roles during framework (re-)registration.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149
> src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
> src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22284/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22284: Authorized roles.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22284/#review45158
-----------------------------------------------------------
Bad patch!
Reviews applied: [22148]
Failed command: git apply --index 22148.patch
Error:
error: patch failed: src/sched/sched.cpp:901
error: src/sched/sched.cpp: patch does not apply
- Mesos ReviewBot
On June 9, 2014, 10:09 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22284/
> -----------------------------------------------------------
>
> (Updated June 9, 2014, 10:09 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1307
> https://issues.apache.org/jira/browse/MESOS-1307
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added authorization for roles during framework (re-)registration.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149
> src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
> src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22284/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22284: Authorized roles.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22284/
-----------------------------------------------------------
(Updated June 10, 2014, 11:33 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's. NNFR.
Bugs: MESOS-1307
https://issues.apache.org/jira/browse/MESOS-1307
Repository: mesos-git
Description
-------
Added authorization for roles during framework (re-)registration.
Diffs (updated)
-----
src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149
src/master/master.hpp 26af1139a43a62b91712acd158b24a8977c81d3f
src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613
src/tests/master_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22284/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 22284: Authorized roles.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22284/
-----------------------------------------------------------
(Updated June 10, 2014, 7:18 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's and adam's comments. NNFR.
Bugs: MESOS-1307
https://issues.apache.org/jira/browse/MESOS-1307
Repository: mesos-git
Description
-------
Added authorization for roles during framework (re-)registration.
Diffs (updated)
-----
src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149
src/master/master.hpp 26af1139a43a62b91712acd158b24a8977c81d3f
src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613
src/tests/master_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22284/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 22284: Authorized roles.
Posted by Adam B <ad...@mesosphere.io>.
> On June 10, 2014, 1:37 a.m., Adam B wrote:
> > src/master/master.cpp, lines 1050-1051
> > <https://reviews.apache.org/r/22284/diff/2/?file=606172#file606172line1050>
> >
> > Please correct me if I'm misinterpreting the ReceiveOffers Message.
> > In the master ACL:
> > global-permissive: ANY Principal can ReceiveOffers for Role foo
> > global-restrictive: NONE Principal can ReceiveOffers for Role foo
> > But when calling authorize(ReceiveOffers request),
> > Use ANY Principal when the framework has No Principal (framework authentication disabled?), since that's the only time this framework will still be authorized.
> > Use NONE Principal never?!?
>
> Vinod Kone wrote:
> The acls.permissive bit only comes into play when none of the ACLs that were setup matches the authorization request. You can think of it as the default case in a switch statement.
>
> An authorization request will likely never have "NONE". I can't imagine why someone would ask that question from authorizer.
>
> In this particular case, we ask if ANY principal is allowed because the framework didn't set its principal (eg. auth disabled and FrameworkInfo.principal is not set). In the future, we might make FrameworkInfo.principal 'required' instead of 'optional' in which case we wont be making such request. For now we make it optional for smooth upgrade.
>
> Hope that clears things up.
Makes sense. Thanks for the explanation.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22284/#review45202
-----------------------------------------------------------
On June 10, 2014, 12:18 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22284/
> -----------------------------------------------------------
>
> (Updated June 10, 2014, 12:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1307
> https://issues.apache.org/jira/browse/MESOS-1307
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added authorization for roles during framework (re-)registration.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149
> src/master/master.hpp 26af1139a43a62b91712acd158b24a8977c81d3f
> src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22284/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22284: Authorized roles.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 10, 2014, 8:37 a.m., Adam B wrote:
> > src/master/master.cpp, lines 1097-1100
> > <https://reviews.apache.org/r/22284/diff/2/?file=606172#file606172line1097>
> >
> > These Option<Error>'s read weird to me. I would expect future.get().isSome() to be a positive case, not an error case.
> > Using Try<Nothing>'s instead gives "if(future.get().isError()) { //handle error }", which reads a lot better to me.
>
> Vinod Kone wrote:
> See the discussion in the "authorize tasks" ticket regarding this. Since the type is Option<Error>, a '.isSome()' tells one that it is an error. We have used this format before (see status update manager, group).
>
> Ben Mahler wrote:
> How about calling it 'validationError' so that 'validationError.isSome()' reads better?
>
> Adam B wrote:
> +1 I like this. "future" is so ambiguous.
done.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22284/#review45202
-----------------------------------------------------------
On June 10, 2014, 7:18 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22284/
> -----------------------------------------------------------
>
> (Updated June 10, 2014, 7:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1307
> https://issues.apache.org/jira/browse/MESOS-1307
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added authorization for roles during framework (re-)registration.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149
> src/master/master.hpp 26af1139a43a62b91712acd158b24a8977c81d3f
> src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22284/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22284: Authorized roles.
Posted by Adam B <ad...@mesosphere.io>.
> On June 10, 2014, 1:37 a.m., Adam B wrote:
> > src/master/master.cpp, lines 1097-1100
> > <https://reviews.apache.org/r/22284/diff/2/?file=606172#file606172line1097>
> >
> > These Option<Error>'s read weird to me. I would expect future.get().isSome() to be a positive case, not an error case.
> > Using Try<Nothing>'s instead gives "if(future.get().isError()) { //handle error }", which reads a lot better to me.
>
> Vinod Kone wrote:
> See the discussion in the "authorize tasks" ticket regarding this. Since the type is Option<Error>, a '.isSome()' tells one that it is an error. We have used this format before (see status update manager, group).
>
> Ben Mahler wrote:
> How about calling it 'validationError' so that 'validationError.isSome()' reads better?
+1 I like this. "future" is so ambiguous.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22284/#review45202
-----------------------------------------------------------
On June 10, 2014, 12:18 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22284/
> -----------------------------------------------------------
>
> (Updated June 10, 2014, 12:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1307
> https://issues.apache.org/jira/browse/MESOS-1307
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added authorization for roles during framework (re-)registration.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149
> src/master/master.hpp 26af1139a43a62b91712acd158b24a8977c81d3f
> src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22284/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22284: Authorized roles.
Posted by Ben Mahler <be...@gmail.com>.
> On June 10, 2014, 8:37 a.m., Adam B wrote:
> > src/master/master.cpp, lines 1097-1100
> > <https://reviews.apache.org/r/22284/diff/2/?file=606172#file606172line1097>
> >
> > These Option<Error>'s read weird to me. I would expect future.get().isSome() to be a positive case, not an error case.
> > Using Try<Nothing>'s instead gives "if(future.get().isError()) { //handle error }", which reads a lot better to me.
>
> Vinod Kone wrote:
> See the discussion in the "authorize tasks" ticket regarding this. Since the type is Option<Error>, a '.isSome()' tells one that it is an error. We have used this format before (see status update manager, group).
How about calling it 'validationError' so that 'validationError.isSome()' reads better?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22284/#review45202
-----------------------------------------------------------
On June 10, 2014, 7:18 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22284/
> -----------------------------------------------------------
>
> (Updated June 10, 2014, 7:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1307
> https://issues.apache.org/jira/browse/MESOS-1307
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added authorization for roles during framework (re-)registration.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149
> src/master/master.hpp 26af1139a43a62b91712acd158b24a8977c81d3f
> src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22284/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22284: Authorized roles.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 10, 2014, 8:37 a.m., Adam B wrote:
> > src/master/master.cpp, lines 1050-1051
> > <https://reviews.apache.org/r/22284/diff/2/?file=606172#file606172line1050>
> >
> > Please correct me if I'm misinterpreting the ReceiveOffers Message.
> > In the master ACL:
> > global-permissive: ANY Principal can ReceiveOffers for Role foo
> > global-restrictive: NONE Principal can ReceiveOffers for Role foo
> > But when calling authorize(ReceiveOffers request),
> > Use ANY Principal when the framework has No Principal (framework authentication disabled?), since that's the only time this framework will still be authorized.
> > Use NONE Principal never?!?
The acls.permissive bit only comes into play when none of the ACLs that were setup matches the authorization request. You can think of it as the default case in a switch statement.
An authorization request will likely never have "NONE". I can't imagine why someone would ask that question from authorizer.
In this particular case, we ask if ANY principal is allowed because the framework didn't set its principal (eg. auth disabled and FrameworkInfo.principal is not set). In the future, we might make FrameworkInfo.principal 'required' instead of 'optional' in which case we wont be making such request. For now we make it optional for smooth upgrade.
Hope that clears things up.
> On June 10, 2014, 8:37 a.m., Adam B wrote:
> > src/master/master.cpp, lines 1097-1100
> > <https://reviews.apache.org/r/22284/diff/2/?file=606172#file606172line1097>
> >
> > These Option<Error>'s read weird to me. I would expect future.get().isSome() to be a positive case, not an error case.
> > Using Try<Nothing>'s instead gives "if(future.get().isError()) { //handle error }", which reads a lot better to me.
See the discussion in the "authorize tasks" ticket regarding this. Since the type is Option<Error>, a '.isSome()' tells one that it is an error. We have used this format before (see status update manager, group).
> On June 10, 2014, 8:37 a.m., Adam B wrote:
> > src/tests/master_authorization_tests.cpp, lines 881-882
> > <https://reviews.apache.org/r/22284/diff/2/?file=606173#file606173line881>
> >
> > Why confirm the first rereg with an EXPECT_CALL(sched, reregistered) and the second rereg with FUTURE_PROTOBUF(FrameworkReregisteredMessage)?
yes because the second re-registered message is dropped by the driver as the driver is already registered.
> On June 10, 2014, 8:37 a.m., Adam B wrote:
> > src/master/flags.hpp, line 146
> > <https://reviews.apache.org/r/22284/diff/2/?file=606170#file606170line146>
> >
> > Deprecate this in favor of '--acls' too?
Interesting idea, but our current ACLs do not support this. Also, there is currently work being done by Alexandra Sava around node drain which might change this behavior.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22284/#review45202
-----------------------------------------------------------
On June 9, 2014, 10:09 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22284/
> -----------------------------------------------------------
>
> (Updated June 9, 2014, 10:09 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1307
> https://issues.apache.org/jira/browse/MESOS-1307
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added authorization for roles during framework (re-)registration.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149
> src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
> src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22284/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22284: Authorized roles.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22284/#review45202
-----------------------------------------------------------
Ship it!
Good stuff. I'm just a bit confused about how the ReceiveOffers Message uses ANY/NONE principals in the master ACL vs. a specific request.
src/master/flags.hpp
<https://reviews.apache.org/r/22284/#comment79922>
Deprecate this in favor of '--acls' too?
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79923>
Why is this wrapped now? I only count 77 chars, and it doesn't look modified otherwise.
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79925>
Please correct me if I'm misinterpreting the ReceiveOffers Message.
In the master ACL:
global-permissive: ANY Principal can ReceiveOffers for Role foo
global-restrictive: NONE Principal can ReceiveOffers for Role foo
But when calling authorize(ReceiveOffers request),
Use ANY Principal when the framework has No Principal (framework authentication disabled?), since that's the only time this framework will still be authorized.
Use NONE Principal never?!?
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79926>
These Option<Error>'s read weird to me. I would expect future.get().isSome() to be a positive case, not an error case.
Using Try<Nothing>'s instead gives "if(future.get().isError()) { //handle error }", which reads a lot better to me.
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79927>
Log 'from'?
src/master/master.cpp
<https://reviews.apache.org/r/22284/#comment79928>
Log frameworkInfo.id()?
src/tests/master_authorization_tests.cpp
<https://reviews.apache.org/r/22284/#comment79929>
Why confirm the first rereg with an EXPECT_CALL(sched, reregistered) and the second rereg with FUTURE_PROTOBUF(FrameworkReregisteredMessage)?
- Adam B
On June 9, 2014, 3:09 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22284/
> -----------------------------------------------------------
>
> (Updated June 9, 2014, 3:09 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1307
> https://issues.apache.org/jira/browse/MESOS-1307
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added authorization for roles during framework (re-)registration.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149
> src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
> src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22284/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22284: Authorized roles.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22284/
-----------------------------------------------------------
(Updated June 9, 2014, 10:09 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's comments.
Bugs: MESOS-1307
https://issues.apache.org/jira/browse/MESOS-1307
Repository: mesos-git
Description
-------
Added authorization for roles during framework (re-)registration.
Diffs (updated)
-----
src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149
src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
src/tests/master_authorization_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22284/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 22284: Authorized roles.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22284/#review45023
-----------------------------------------------------------
Bad patch!
Reviews applied: [22148]
Failed command: git apply --index 22148.patch
Error:
error: patch failed: src/sched/sched.cpp:901
error: src/sched/sched.cpp: patch does not apply
- Mesos ReviewBot
On June 6, 2014, 12:12 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22284/
> -----------------------------------------------------------
>
> (Updated June 6, 2014, 12:12 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1307
> https://issues.apache.org/jira/browse/MESOS-1307
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added authorization for roles during framework (re-)registration.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149
> src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3
> src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f
> src/tests/master_authorization_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22284/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>