You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jan Schlicht <ja...@mesosphere.io> on 2015/12/15 11:13:50 UTC
Re: Review Request 39520: Quota: Added authentication,
authorization tests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39520/
-----------------------------------------------------------
(Updated Dec. 15, 2015, 11:13 a.m.)
Review request for mesos, Alexander Rukletsov and Till Toenshoff.
Bugs: MESOS-4082
https://issues.apache.org/jira/browse/MESOS-4082
Repository: mesos
Description
-------
Quota: Added authentication, authorization tests.
Diffs
-----
src/tests/authorization_tests.cpp f55e074806e7697e131fa6e8dc97fb9aaea3e365
src/tests/master_quota_tests.cpp 12820fcf7c0d2a791071464ad8ed738664ad85de
Diff: https://reviews.apache.org/r/39520/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 39520: Quota: Added authentication,
authorization tests.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39520/#review110495
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39211, 39018, 39102, 36913, 38059, 39285, 38110, 39288, 40345, 40346, 40347, 40348, 39520]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 15, 2015, 11:09 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39520/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2015, 11:09 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
>
>
> Bugs: MESOS-4082
> https://issues.apache.org/jira/browse/MESOS-4082
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Quota: Added authentication, authorization tests.
>
>
> Diffs
> -----
>
> src/tests/authorization_tests.cpp b5fb3c30e4c53bf3bf081c2a9a30a327f2c7c880
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/39520/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 39520: Quota: Added authentication,
authorization tests.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39520/#review111234
-----------------------------------------------------------
src/tests/master_quota_tests.cpp (line 1018)
<https://reviews.apache.org/r/39520/#comment171327>
Might be a good idea to use a string constant instead, here and everywhere else?
See
https://issues.apache.org/jira/browse/MESOS-4197
- Till Toenshoff
On Dec. 18, 2015, 12:27 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39520/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2015, 12:27 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
>
>
> Bugs: MESOS-4082
> https://issues.apache.org/jira/browse/MESOS-4082
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Quota: Added authentication, authorization tests.
>
>
> Diffs
> -----
>
> src/tests/authorization_tests.cpp dcf348acc75b143e3a854bde17e6042dc90599d5
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/39520/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 39520: Quota: Added authentication,
authorization tests.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39520/
-----------------------------------------------------------
(Updated Dec. 18, 2015, 1:27 p.m.)
Review request for mesos, Alexander Rukletsov and Till Toenshoff.
Bugs: MESOS-4082
https://issues.apache.org/jira/browse/MESOS-4082
Repository: mesos
Description
-------
Quota: Added authentication, authorization tests.
Diffs (updated)
-----
src/tests/authorization_tests.cpp dcf348acc75b143e3a854bde17e6042dc90599d5
src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
Diff: https://reviews.apache.org/r/39520/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 39520: Quota: Added authentication,
authorization tests.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39520/
-----------------------------------------------------------
(Updated Dec. 18, 2015, 1:18 p.m.)
Review request for mesos, Alexander Rukletsov and Till Toenshoff.
Changes
-------
Addressed issues.
Bugs: MESOS-4082
https://issues.apache.org/jira/browse/MESOS-4082
Repository: mesos
Description
-------
Quota: Added authentication, authorization tests.
Diffs (updated)
-----
src/tests/authorization_tests.cpp dcf348acc75b143e3a854bde17e6042dc90599d5
src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
Diff: https://reviews.apache.org/r/39520/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 39520: Quota: Added authentication,
authorization tests.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On Dec. 18, 2015, 12:32 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 1070
> > <https://reviews.apache.org/r/39520/diff/6-9/?file=1156962#file1156962line1070>
> >
> > quota set request;
> > backtick `ROLE1`
> > s/ that can be authorized././
> >
> > Or you can kill the comment entirely.
Deleted the comment.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39520/#review111166
-----------------------------------------------------------
On Dec. 18, 2015, 1:18 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39520/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2015, 1:18 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
>
>
> Bugs: MESOS-4082
> https://issues.apache.org/jira/browse/MESOS-4082
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Quota: Added authentication, authorization tests.
>
>
> Diffs
> -----
>
> src/tests/authorization_tests.cpp dcf348acc75b143e3a854bde17e6042dc90599d5
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/39520/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 39520: Quota: Added authentication,
authorization tests.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On Dec. 18, 2015, 12:32 p.m., Alexander Rukletsov wrote:
> > I would like to see one more test, where both authn and authz are disabled and quota set request succeeds.
While having this test makes sense, it would test neither authentication nor authorization, but the actual quota functionality. Therefore it shouldn't be part of these tests.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39520/#review111166
-----------------------------------------------------------
On Dec. 18, 2015, 1:18 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39520/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2015, 1:18 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
>
>
> Bugs: MESOS-4082
> https://issues.apache.org/jira/browse/MESOS-4082
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Quota: Added authentication, authorization tests.
>
>
> Diffs
> -----
>
> src/tests/authorization_tests.cpp dcf348acc75b143e3a854bde17e6042dc90599d5
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/39520/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 39520: Quota: Added authentication,
authorization tests.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39520/#review111166
-----------------------------------------------------------
I would like to see one more test, where both authn and authz are disabled and quota set request succeeds.
src/tests/master_quota_tests.cpp (lines 994 - 996)
<https://reviews.apache.org/r/39520/#comment171194>
It's not the fixture which is set up to expect certain credentials, it's the master which is set up in the fixture in a way that only a certain credentials will be authenticated. Let's be explicit about it.
How about:
The master is configured so that only requests from `DEFAULT_CREDENTIAL` are authenticated.
src/tests/master_quota_tests.cpp (lines 1010 - 1011)
<https://reviews.apache.org/r/39520/#comment171195>
How about a one-liner:
// The absense of credentials leads to authentication failure as well.
src/tests/master_quota_tests.cpp (line 1031)
<https://reviews.apache.org/r/39520/#comment171196>
It's not the operator, but a rather specific principal. How about
// `DEFAULT_CREDENTIAL.principal()` can request quotas for `ROLE1` (mind backticks).
src/tests/master_quota_tests.cpp (line 1055)
<https://reviews.apache.org/r/39520/#comment171197>
quota set request;
backtick `ROLE1`
s/ that can be authorized././
Or you can kill the comment entirely.
src/tests/master_quota_tests.cpp (line 1091)
<https://reviews.apache.org/r/39520/#comment171198>
Ditto as in the previous test.
src/tests/master_quota_tests.cpp (lines 1097 - 1101)
<https://reviews.apache.org/r/39520/#comment171199>
Let's refactor for readability:
// Disable authentication by not providing credentials.
master::Flags masterFlags = CreateMasterFlags();
masterFlags.acls = acls;
masterFlags.credentials = None();
src/tests/master_quota_tests.cpp (line 1118)
<https://reviews.apache.org/r/39520/#comment171200>
See my comment in the previous test.
src/tests/master_quota_tests.cpp (line 1127)
<https://reviews.apache.org/r/39520/#comment171205>
Let's drag reader's attention that no credentials are provided.
src/tests/master_quota_tests.cpp (line 1136)
<https://reviews.apache.org/r/39520/#comment171201>
You can kill this balnk line
src/tests/master_quota_tests.cpp (line 1147)
<https://reviews.apache.org/r/39520/#comment171202>
backtick `ROLE1`, here and everywhere, please.
src/tests/master_quota_tests.cpp (line 1161)
<https://reviews.apache.org/r/39520/#comment171203>
Let's kill this comment.
- Alexander Rukletsov
On Dec. 18, 2015, 9:12 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39520/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2015, 9:12 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
>
>
> Bugs: MESOS-4082
> https://issues.apache.org/jira/browse/MESOS-4082
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Quota: Added authentication, authorization tests.
>
>
> Diffs
> -----
>
> src/tests/authorization_tests.cpp dcf348acc75b143e3a854bde17e6042dc90599d5
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/39520/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 39520: Quota: Added authentication,
authorization tests.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39520/
-----------------------------------------------------------
(Updated Dec. 18, 2015, 10:12 a.m.)
Review request for mesos, Alexander Rukletsov and Till Toenshoff.
Changes
-------
Addressed issues.
Bugs: MESOS-4082
https://issues.apache.org/jira/browse/MESOS-4082
Repository: mesos
Description
-------
Quota: Added authentication, authorization tests.
Diffs (updated)
-----
src/tests/authorization_tests.cpp dcf348acc75b143e3a854bde17e6042dc90599d5
src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
Diff: https://reviews.apache.org/r/39520/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 39520: Quota: Added authentication,
authorization tests.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39520/
-----------------------------------------------------------
(Updated Dec. 15, 2015, 12:09 p.m.)
Review request for mesos, Alexander Rukletsov and Till Toenshoff.
Changes
-------
Rebased and addressed some issues.
Bugs: MESOS-4082
https://issues.apache.org/jira/browse/MESOS-4082
Repository: mesos
Description
-------
Quota: Added authentication, authorization tests.
Diffs (updated)
-----
src/tests/authorization_tests.cpp b5fb3c30e4c53bf3bf081c2a9a30a327f2c7c880
src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
Diff: https://reviews.apache.org/r/39520/diff/
Testing
-------
make check
Thanks,
Jan Schlicht