Re: Review Request 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.

2016-05-17 Thread Alexander Rukletsov 

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




src/master/quota_handler.cpp (line 513)


No period at the end of log entries; fits onto the previous line.


- Alexander Rukletsov


On May 17, 2016, 12:39 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47274/
> ---
> 
> (Updated May 17, 2016, 12:39 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-5336
> https://issues.apache.org/jira/browse/MESOS-5336
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Authorize what quota can be seen by GET_QUOTA_BY_ROLE.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/master/quota.md 83913fac9d45ef25629222f572529493d0c66f05 
>   include/mesos/authorizer/acls.proto 
> 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
>   include/mesos/authorizer/authorizer.proto 
> 32492a59ad95df3bb673ec42321518f86c11af59 
>   src/authorizer/local/authorizer.cpp 
> e95435327bb3b6f447e814b8657bce8084535346 
>   src/master/http.cpp a0d67671c89b9794e721dc3ba012acd3bdc447e3 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
>   src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 
> 
> Diff: https://reviews.apache.org/r/47274/diff/
> 
> 
> Testing
> ---
> 
> Unit test;
> manual testing using local authorizer.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.

2016-05-17 Thread Zhitao Li 

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

(Updated May 17, 2016, 12:39 p.m.)


Review request for mesos, Adam B and Alexander Rukletsov.


Bugs: MESOS-5336
https://issues.apache.org/jira/browse/MESOS-5336


Repository: mesos


Description
---

Authorize what quota can be seen by GET_QUOTA_BY_ROLE.


Diffs
-

  docs/endpoints/master/quota.md 83913fac9d45ef25629222f572529493d0c66f05 
  include/mesos/authorizer/acls.proto 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
  include/mesos/authorizer/authorizer.proto 
32492a59ad95df3bb673ec42321518f86c11af59 
  src/authorizer/local/authorizer.cpp e95435327bb3b6f447e814b8657bce8084535346 
  src/master/http.cpp a0d67671c89b9794e721dc3ba012acd3bdc447e3 
  src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
  src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
  src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 

Diff: https://reviews.apache.org/r/47274/diff/


Testing (updated)
---

Unit test;
manual testing using local authorizer.


Thanks,

Zhitao Li

Re: Review Request 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.

2016-05-17 Thread Alexander Rukletsov 

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


Fix it, then Ship it!




I'll take care of the outstanding issues and commit this for you shortly. I've 
also manually tested that authz with local authorizer works.


src/master/http.cpp (lines 1273 - 1275)


Reformat this so it looks similar to previous lines.



src/master/master.hpp (lines 1010 - 1012)


Move this above other `authorize*()` functions for consistency.



src/master/master.hpp (lines 1021 - 1024)


Move this before `_set()`.



src/master/quota_handler.cpp (lines 428 - 429)


We backtick each type or variable name.

// Quotas can be updated during preparation of the response.
// Copy current view of the collection to avoid conflicts.



src/master/quota_handler.cpp (line 438)


Latest news: there will be filtering instead of batch request.

// TODO(alexr): Use an authorization filter here once they are available.



src/master/quota_handler.cpp (lines 444 - 449)


```
  return process::collect(authorizedRoles)
.then(defer(
master->self(),
[=](const list authorizedRolesCollected)
-> Future {
  return _status(request, quotaInfos, authorizedRolesCollected);
}));
```
for consistency with the rest of the codebase.



src/master/quota_handler.cpp (line 446)


const & ?



src/master/quota_handler.cpp (line 464)


// NOTE: This error-prone code will be removed with introduction of 
authorization filters.



src/master/quota_handler.cpp (lines 502 - 525)


Move it before `authorizeSetQuota()`.


- Alexander Rukletsov


On May 17, 2016, 5:06 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47274/
> ---
> 
> (Updated May 17, 2016, 5:06 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-5336
> https://issues.apache.org/jira/browse/MESOS-5336
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Authorize what quota can be seen by GET_QUOTA_BY_ROLE.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/master/quota.md 83913fac9d45ef25629222f572529493d0c66f05 
>   include/mesos/authorizer/acls.proto 
> 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
>   include/mesos/authorizer/authorizer.proto 
> 32492a59ad95df3bb673ec42321518f86c11af59 
>   src/authorizer/local/authorizer.cpp 
> e95435327bb3b6f447e814b8657bce8084535346 
>   src/master/http.cpp a0d67671c89b9794e721dc3ba012acd3bdc447e3 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
>   src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 
> 
> Diff: https://reviews.apache.org/r/47274/diff/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.

2016-05-16 Thread Zhitao Li 


> On May 16, 2016, 6:06 p.m., Alexander Rukletsov wrote:
> > src/master/master.hpp, line 1022
> > 
> >
> > Do you still need `request`?

We still need `jsonp` argument to be passed. I feel keeping request captured 
will be easier to handle in the future.


- Zhitao


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


On May 17, 2016, 5:06 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47274/
> ---
> 
> (Updated May 17, 2016, 5:06 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-5336
> https://issues.apache.org/jira/browse/MESOS-5336
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Authorize what quota can be seen by GET_QUOTA_BY_ROLE.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/master/quota.md 83913fac9d45ef25629222f572529493d0c66f05 
>   include/mesos/authorizer/acls.proto 
> 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
>   include/mesos/authorizer/authorizer.proto 
> 32492a59ad95df3bb673ec42321518f86c11af59 
>   src/authorizer/local/authorizer.cpp 
> e95435327bb3b6f447e814b8657bce8084535346 
>   src/master/http.cpp a0d67671c89b9794e721dc3ba012acd3bdc447e3 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
>   src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 
> 
> Diff: https://reviews.apache.org/r/47274/diff/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.

2016-05-16 Thread Zhitao Li 

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




src/master/quota_handler.cpp (lines 518 - 520)


It'll be great if we can either catch this in linter/commit hook during 
`git commit`, or publish .vimrc/etc so contributors don't need to remember this.


- Zhitao Li


On May 17, 2016, 5:06 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47274/
> ---
> 
> (Updated May 17, 2016, 5:06 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-5336
> https://issues.apache.org/jira/browse/MESOS-5336
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Authorize what quota can be seen by GET_QUOTA_BY_ROLE.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/master/quota.md 83913fac9d45ef25629222f572529493d0c66f05 
>   include/mesos/authorizer/acls.proto 
> 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
>   include/mesos/authorizer/authorizer.proto 
> 32492a59ad95df3bb673ec42321518f86c11af59 
>   src/authorizer/local/authorizer.cpp 
> e95435327bb3b6f447e814b8657bce8084535346 
>   src/master/http.cpp a0d67671c89b9794e721dc3ba012acd3bdc447e3 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
>   src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 
> 
> Diff: https://reviews.apache.org/r/47274/diff/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.

2016-05-16 Thread Zhitao Li 

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

(Updated May 17, 2016, 5:06 a.m.)


Review request for mesos, Adam B and Alexander Rukletsov.


Changes
---

Review comments.


Bugs: MESOS-5336
https://issues.apache.org/jira/browse/MESOS-5336


Repository: mesos


Description
---

Authorize what quota can be seen by GET_QUOTA_BY_ROLE.


Diffs (updated)
-

  docs/endpoints/master/quota.md 83913fac9d45ef25629222f572529493d0c66f05 
  include/mesos/authorizer/acls.proto 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
  include/mesos/authorizer/authorizer.proto 
32492a59ad95df3bb673ec42321518f86c11af59 
  src/authorizer/local/authorizer.cpp e95435327bb3b6f447e814b8657bce8084535346 
  src/master/http.cpp a0d67671c89b9794e721dc3ba012acd3bdc447e3 
  src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
  src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
  src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 

Diff: https://reviews.apache.org/r/47274/diff/


Testing
---

Unit test.


Thanks,

Zhitao Li

Re: Review Request 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.

2016-05-16 Thread Alexander Rukletsov 

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



Okay, almost there. We should be good after one more pass.


src/master/master.hpp (line 1022)


Do you still need `request`?



src/master/quota_handler.cpp (line 421)


Since we are adding support for authorizing GET now, we need to adjust 
wording in `QUOTA_HELP()` in "master/http.cpp" (and run 
"./support/generate-endpoint-help.py" to update the docs). It would be great to 
say that if a user GETs /quota, the results can be silently filtered.



src/master/quota_handler.cpp (lines 427 - 428)


Mind writing a concise comment on why you need this copy?



src/master/quota_handler.cpp (line 430)


Feel free to kill this blank line.



src/master/quota_handler.cpp (line 434)


Could you please restore the TODO from the previous version of the patch? 
Also your comment

// Create a list of authorization actions for each role we may return.
// TODO(alexr): Batch these actions once we have BatchRequest in authorizer.



src/master/quota_handler.cpp (line 435)


How about s/getQuotaByRoleAuthorized/authorizedRoles?



src/master/quota_handler.cpp (line 443)


Any reason you don't say `const list&`?

Also maybe s/authorized/authorizedRolesCollected ?



src/master/quota_handler.cpp (line 452)


s/authorized/authorizedRoles ?



src/master/quota_handler.cpp (line 461)


Feel free to use `auto` here.

Also, `quotaInfoIt` can be a better name.



src/master/quota_handler.cpp (line 462)


If you rename `authorized` to `authorizedRoles`, you can then rename 
`result` to `authorized`. I'd say `if (authorized)` is more descriptive.



src/master/quota_handler.cpp (lines 467 - 468)


Blank line, please.



src/master/quota_handler.cpp (line 495)


We separate implementations with two blank lines. Thanks!



src/master/quota_handler.cpp (line 504)


Now in retrospect it looks like it was a mediocre idea to use `LOG(INFO)` 
in this case. We may have to revisit our policy around log levels one day.



src/master/quota_handler.cpp (lines 516 - 517)


Let's add a blank line for consistency with other `authorize*()` functions 
in this file.



src/master/quota_handler.cpp (lines 518 - 520)


Two blanks here, please!


- Alexander Rukletsov


On May 13, 2016, 6:48 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47274/
> ---
> 
> (Updated May 13, 2016, 6:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-5336
> https://issues.apache.org/jira/browse/MESOS-5336
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Authorize what quota can be seen by GET_QUOTA_BY_ROLE.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
>   include/mesos/authorizer/authorizer.proto 
> 32492a59ad95df3bb673ec42321518f86c11af59 
>   src/authorizer/local/authorizer.cpp 
> e95435327bb3b6f447e814b8657bce8084535346 
>   src/master/http.cpp a0d67671c89b9794e721dc3ba012acd3bdc447e3 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
>   src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 
> 
> Diff: https://reviews.apache.org/r/47274/diff/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.

2016-05-13 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [47274]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On May 13, 2016, 6:48 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47274/
> ---
> 
> (Updated May 13, 2016, 6:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-5336
> https://issues.apache.org/jira/browse/MESOS-5336
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Authorize what quota can be seen by GET_QUOTA_BY_ROLE.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
>   include/mesos/authorizer/authorizer.proto 
> 32492a59ad95df3bb673ec42321518f86c11af59 
>   src/authorizer/local/authorizer.cpp 
> e95435327bb3b6f447e814b8657bce8084535346 
>   src/master/http.cpp a0d67671c89b9794e721dc3ba012acd3bdc447e3 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
>   src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 
> 
> Diff: https://reviews.apache.org/r/47274/diff/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.

2016-05-13 Thread Zhitao Li 

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

(Updated May 13, 2016, 6:48 p.m.)


Review request for mesos, Adam B and Alexander Rukletsov.


Changes
---

Remove dependency to the other patch.


Bugs: MESOS-5336
https://issues.apache.org/jira/browse/MESOS-5336


Repository: mesos


Description
---

Authorize what quota can be seen by GET_QUOTA_BY_ROLE.


Diffs
-

  include/mesos/authorizer/acls.proto 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
  include/mesos/authorizer/authorizer.proto 
32492a59ad95df3bb673ec42321518f86c11af59 
  src/authorizer/local/authorizer.cpp e95435327bb3b6f447e814b8657bce8084535346 
  src/master/http.cpp a0d67671c89b9794e721dc3ba012acd3bdc447e3 
  src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
  src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
  src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 

Diff: https://reviews.apache.org/r/47274/diff/


Testing
---

Unit test.


Thanks,

Zhitao Li

Re: Review Request 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.

2016-05-13 Thread Zhitao Li 

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

(Updated May 13, 2016, 6:47 p.m.)


Review request for mesos, Adam B and Alexander Rukletsov.


Changes
---

Address review comments.


Bugs: MESOS-5336
https://issues.apache.org/jira/browse/MESOS-5336


Repository: mesos


Description
---

Authorize what quota can be seen by GET_QUOTA_BY_ROLE.


Diffs (updated)
-

  include/mesos/authorizer/acls.proto 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
  include/mesos/authorizer/authorizer.proto 
32492a59ad95df3bb673ec42321518f86c11af59 
  src/authorizer/local/authorizer.cpp e95435327bb3b6f447e814b8657bce8084535346 
  src/master/http.cpp a0d67671c89b9794e721dc3ba012acd3bdc447e3 
  src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
  src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
  src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 

Diff: https://reviews.apache.org/r/47274/diff/


Testing
---

Unit test.


Thanks,

Zhitao Li

Re: Review Request 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.

2016-05-13 Thread Alexander Rukletsov 


> On May 13, 2016, 5:38 p.m., Zhitao Li wrote:
> > src/master/quota_handler.cpp, line 464
> > 
> >
> > I'd like to keep using vector because we know the expected size in the 
> > beginning and can reserve the capacity beforehand. It's slightly more 
> > memory efficient than list (which requires one memory per push_back IIUIC).
> > 
> > I can still avoid index by using vector::const_iterator to loop in it. 
> > It should be no difference interface wise.

Ok.


- Alexander


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


On May 12, 2016, 12:48 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47274/
> ---
> 
> (Updated May 12, 2016, 12:48 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-5336
> https://issues.apache.org/jira/browse/MESOS-5336
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Authorize what quota can be seen by GET_QUOTA_BY_ROLE.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
>   include/mesos/authorizer/authorizer.proto 
> 32492a59ad95df3bb673ec42321518f86c11af59 
>   src/authorizer/local/authorizer.cpp 
> e95435327bb3b6f447e814b8657bce8084535346 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
>   src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 
> 
> Diff: https://reviews.apache.org/r/47274/diff/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.

2016-05-13 Thread Zhitao Li 

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




src/master/quota_handler.cpp (line 464)


I'd like to keep using vector because we know the expected size in the 
beginning and can reserve the capacity beforehand. It's slightly more memory 
efficient than list (which requires one memory per push_back IIUIC).

I can still avoid index by using vector::const_iterator to loop in it. It 
should be no difference interface wise.


- Zhitao Li


On May 12, 2016, 12:48 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47274/
> ---
> 
> (Updated May 12, 2016, 12:48 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-5336
> https://issues.apache.org/jira/browse/MESOS-5336
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Authorize what quota can be seen by GET_QUOTA_BY_ROLE.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
>   include/mesos/authorizer/authorizer.proto 
> 32492a59ad95df3bb673ec42321518f86c11af59 
>   src/authorizer/local/authorizer.cpp 
> e95435327bb3b6f447e814b8657bce8084535346 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
>   src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 
> 
> Diff: https://reviews.apache.org/r/47274/diff/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.

2016-05-13 Thread Alexander Rukletsov 


> On May 13, 2016, 2:17 p.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, line 464
> > 
> >
> > If we eliminate intermediate vector, we will be looping thorugh hashmap 
> > and list simultaneously. I can't think of a nice way to do it; we should 
> > probably use `for` with iterators.

Though we need the intermediate collection, I'd still say having the same type 
(`list`) and avoid using indices can make the code more readable. What'd you 
say?


- Alexander


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


On May 12, 2016, 12:48 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47274/
> ---
> 
> (Updated May 12, 2016, 12:48 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-5336
> https://issues.apache.org/jira/browse/MESOS-5336
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Authorize what quota can be seen by GET_QUOTA_BY_ROLE.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
>   include/mesos/authorizer/authorizer.proto 
> 32492a59ad95df3bb673ec42321518f86c11af59 
>   src/authorizer/local/authorizer.cpp 
> e95435327bb3b6f447e814b8657bce8084535346 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
>   src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 
> 
> Diff: https://reviews.apache.org/r/47274/diff/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.

2016-05-13 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [47222, 47274]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On May 12, 2016, 12:48 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47274/
> ---
> 
> (Updated May 12, 2016, 12:48 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-5336
> https://issues.apache.org/jira/browse/MESOS-5336
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Authorize what quota can be seen by GET_QUOTA_BY_ROLE.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
>   include/mesos/authorizer/authorizer.proto 
> 32492a59ad95df3bb673ec42321518f86c11af59 
>   src/authorizer/local/authorizer.cpp 
> e95435327bb3b6f447e814b8657bce8084535346 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
>   src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 
> 
> Diff: https://reviews.apache.org/r/47274/diff/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.

2016-05-13 Thread Alexander Rukletsov 


> On May 13, 2016, 2:17 p.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, lines 441-447
> > 
> >
> > Do we really need this copy? Or we can directly use `master->quotas`?

After a second thought, most probably we need a copy here because 
`master->quotas` may change when we process authz response in the continuation.


- Alexander


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


On May 12, 2016, 12:48 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47274/
> ---
> 
> (Updated May 12, 2016, 12:48 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-5336
> https://issues.apache.org/jira/browse/MESOS-5336
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Authorize what quota can be seen by GET_QUOTA_BY_ROLE.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
>   include/mesos/authorizer/authorizer.proto 
> 32492a59ad95df3bb673ec42321518f86c11af59 
>   src/authorizer/local/authorizer.cpp 
> e95435327bb3b6f447e814b8657bce8084535346 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
>   src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 
> 
> Diff: https://reviews.apache.org/r/47274/diff/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.

2016-05-13 Thread Alexander Rukletsov 

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



Looks good. Could you please make this patch self-contained, i.e., remove 
dependency on /r/47222?

Did you manually test that authorization works? I mean setup ACLs, start a 
master, force-set some quotas and check that they are being properly filtered? 
Could you please do it and describe in the "testing done" section?


include/mesos/authorizer/acls.proto (line 206)


Let's move this line up, before `SetQuota`



include/mesos/authorizer/authorizer.proto (line 62)


Ditto.



src/authorizer/local/authorizer.cpp (lines 219 - 229)


Ditto (move up before set quota).



src/master/master.hpp (line 1013)


This can be simply named `authorizeGetQuota` after you remove 
coarse-grained authz.



src/master/master.hpp (line 1024)


Once you remove coarse-grained authz, you can prepare the authz action in 
`status()` and hence you don't need `principal` in `_status()`, right?



src/master/quota_handler.cpp (lines 441 - 447)


Do we really need this copy? Or we can directly use `master->quotas`?



src/master/quota_handler.cpp (lines 449 - 450)


Please blank line between comment sections, e.g.:
```
// The quick, brown fox jumps over a lazy dog. DJs flock by when MTV
// ax quiz prog. Junk MTV quiz graced by fox whelps. Bawds jog, flick
// quartz, vex nymphs.
//
// NOTE: Waltz, bad nymph, for quick jigs vex! Fox nymphs grab quick-jived
// waltz. Brick quiz whangs jumpy veldt fox. Bright vixens jump; fowl quack.
//
// TODO(zhitao): Quick wafting zephyrs vex bold Jim. Quick zephyrs blow,
// vexing daft Jim. Sex-charged fop blew my junk TV quiz. How quickly daft
// jumping zebras vex. Two driven jocks help fax my big quiz.
```



src/master/quota_handler.cpp (line 454)


We indent by 4 spaces if the previous line was a function call ending by '('



src/master/quota_handler.cpp (lines 459 - 470)


Let's make it a continuation. Since you'll be moving this code to 
`status()`, you can reuse `_status()` for continuation.



src/master/quota_handler.cpp (line 463)


How about a comment here:
```
// Create an entry (including role and resources) for each quota,
// except those filtered out based on the authorizer's response.
```



src/master/quota_handler.cpp (line 464)


If we eliminate intermediate vector, we will be looping thorugh hashmap and 
list simultaneously. I can't think of a nice way to do it; we should probably 
use `for` with iterators.



src/master/quota_handler.cpp (line 519)


Blank line



src/tests/master_quota_tests.cpp (line 1301)


"previous authorized quota"? Do you want to say "previously requested 
quota"?


- Alexander Rukletsov


On May 12, 2016, 12:48 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47274/
> ---
> 
> (Updated May 12, 2016, 12:48 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-5336
> https://issues.apache.org/jira/browse/MESOS-5336
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Authorize what quota can be seen by GET_QUOTA_BY_ROLE.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
>   include/mesos/authorizer/authorizer.proto 
> 32492a59ad95df3bb673ec42321518f86c11af59 
>   src/authorizer/local/authorizer.cpp 
> e95435327bb3b6f447e814b8657bce8084535346 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
>   src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 
> 
> Diff: https://reviews.apache.org/r/47274/diff/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Review Request 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.

2016-05-11 Thread Zhitao Li 

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

Review request for mesos, Adam B and Alexander Rukletsov.


Bugs: MESOS-5336
https://issues.apache.org/jira/browse/MESOS-5336


Repository: mesos


Description
---

Authorize what quota can be seen by GET_QUOTA_BY_ROLE.


Diffs
-

  include/mesos/authorizer/acls.proto 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
  include/mesos/authorizer/authorizer.proto 
32492a59ad95df3bb673ec42321518f86c11af59 
  src/authorizer/local/authorizer.cpp e95435327bb3b6f447e814b8657bce8084535346 
  src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
  src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
  src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 

Diff: https://reviews.apache.org/r/47274/diff/


Testing
---

Unit test.


Thanks,

Zhitao Li