Re: Review Request 39520: Quota: Added authentication, authorization tests.

2015-12-18 Thread Jan Schlicht 


> On Dec. 14, 2015, 5:18 p.m., Alexander Rukletsov wrote:
> > I would suggest to add some more test cases:
> > - Request does not contain a principal (in absence of authz quota can be 
> > set);
> > - Request does not contain a principal, ACLs allow `ANY` to set quota for 
> > role "prod" (absence of principal maps to `ANY`).

The new test fixture `AuthorizedQuotaSetRequestWithoutPrincipal` now tests for 
authorization with an empty principal if authentication is disabled.
`AuthorizationTest.SetQuota` now includes a rule where `ANY` can set quota for 
role "test" and tests that rule.


- Jan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39520/#review110229
---


On Dec. 18, 2015, 10: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, 10: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.

2015-12-18 Thread Jan Schlicht 

---
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.

2015-12-18 Thread Jan Schlicht 

---
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.

2015-12-18 Thread Jan Schlicht 


> 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/#review66
---


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.

2015-12-18 Thread Alexander Rukletsov 


> On Dec. 14, 2015, 4:18 p.m., Alexander Rukletsov wrote:
> > I would suggest to add some more test cases:
> > - Request does not contain a principal (in absence of authz quota can be 
> > set);
> > - Request does not contain a principal, ACLs allow `ANY` to set quota for 
> > role "prod" (absence of principal maps to `ANY`).
> 
> Jan Schlicht wrote:
> The new test fixture `AuthorizedQuotaSetRequestWithoutPrincipal` now 
> tests for authorization with an empty principal if authentication is disabled.
> `AuthorizationTest.SetQuota` now includes a rule where `ANY` can set 
> quota for role "test" and tests that rule.

I still do not see a tests where **both** authn and authz are disabled. Let's 
add this test.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39520/#review110229
---


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.

2015-12-18 Thread Alexander Rukletsov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39520/#review66
---


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)


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)


How about a one-liner:
// The absense of credentials leads to authentication failure as well.



src/tests/master_quota_tests.cpp (line 1031)


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)


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)


Ditto as in the previous test.



src/tests/master_quota_tests.cpp (lines 1097 - 1101)


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)


See my comment in the previous test.



src/tests/master_quota_tests.cpp (line 1127)


Let's drag reader's attention that no credentials are provided.



src/tests/master_quota_tests.cpp (line 1136)


You can kill this balnk line



src/tests/master_quota_tests.cpp (line 1147)


backtick `ROLE1`, here and everywhere, please.



src/tests/master_quota_tests.cpp (line 1161)


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.

2015-12-18 Thread Jan Schlicht 


> On Dec. 18, 2015, 12:32 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 1070
> > 
> >
> > 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/#review66
---


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.

2015-12-18 Thread Jan Schlicht 

---
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.

2015-12-15 Thread Jan Schlicht 

---
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.

2015-12-15 Thread Jan Schlicht 


> On Dec. 14, 2015, 4:52 p.m., Till Toenshoff wrote:
> > src/tests/master_quota_tests.cpp, lines 78-79
> > 
> >
> > Comma, really? 8]

Hm, even an Oxford comma doesn't count here :D


- Jan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39520/#review110223
---


On Dec. 15, 2015, 11:13 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: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.

2015-12-15 Thread Jan Schlicht 

---
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

Re: Review Request 39520: Quota: Added authentication, authorization tests.

2015-12-15 Thread Alexander Rukletsov 


> On Dec. 14, 2015, 4:18 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 1002
> > 
> >
> > This could fail if we ever enforce `MIN_MEM` check. How about 
> > "cpus:1;mem:512", which we use frequently in this test file.
> > 
> > Here and below.
> 
> Till Toenshoff wrote:
> Very good point indeed. Maybe we can avoid explicit magics entirely and 
> find a way to reference some kind of default resource values for the tests in 
> the form of a static string / proper constant?

An argument against introducing a constant here is verbosity: we try to be 
explicit about what's going on in the test. Also, we do use some constants for 
agent resources: `defaultAgentResources`. I would say using a magic number is 
fine as long as we ensure it's sane, for example by checking 
`EXPECT_TRUE(agentTotalResources.get().contains(Resources::parse("magic-numbers").get()));`.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39520/#review110229
---


On Dec. 15, 2015, 10:13 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, 10: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.

2015-12-15 Thread Jan Schlicht 


> On Dec. 14, 2015, 11:16 p.m., Greg Mann wrote:
> > src/tests/master_quota_tests.cpp, line 1061
> > 
> >
> > In addition to verifying the OK response, is it possible to verify by 
> > another means that the quota has been set to the correct value for the 
> > correct principal?

It is possible to verify this by checking `QuotaInfo` that is provided to the 
allocator. Other tests do this as well and I will implement it. At the moment 
the principal is not stored in `QuotaInfo`, so it is not possible to check for 
that yet. But storing it will be necessary for the quota removal authorization, 
hence I'd add a TODO for that and implement it later.


- Jan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39520/#review110323
---


On Dec. 15, 2015, 12:09 p.m., Jan Schlicht wrote:
> 
> ---
> 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.
> 
> 
> 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.

2015-12-15 Thread Mesos ReviewBot 

---
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
> 
>