You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2019/06/03 19:12:00 UTC
Re: Review Request 70549: Added authorization for `UpdateQuota` call
in the master.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70549/#review215654
-----------------------------------------------------------
Fix it, then Ship it!
include/mesos/authorizer/authorizer.proto
Line 53 (original), 53 (patched)
<https://reviews.apache.org/r/70549/#comment302436>
[deprecated = true]?
include/mesos/authorizer/authorizer.proto
Lines 59 (patched)
<https://reviews.apache.org/r/70549/#comment302435>
See comment below about just using the role.
include/mesos/authorizer/authorizer.proto
Line 139 (original), 147 (patched)
<https://reviews.apache.org/r/70549/#comment302433>
[deprecated = true]?
I believe we can deprecate enum values?
https://github.com/protocolbuffers/protobuf/issues/5567
include/mesos/authorizer/authorizer.proto
Lines 149-150 (patched)
<https://reviews.apache.org/r/70549/#comment302434>
Per alexr's comment, I also think we should just keep this simple and use the role only.
That means deprecating the `QuotaInfo` field as well, right?
src/master/master.hpp
Lines 1255-1257 (original), 1255-1257 (patched)
<https://reviews.apache.org/r/70549/#comment302437>
Add a comment noting that this is the deprecated path?
src/master/quota_handler.cpp
Lines 832-833 (patched)
<https://reviews.apache.org/r/70549/#comment302438>
Open / close quotes for role on the same line for readability?
```
<< "' to update quota config"
<< " for role '" << quotaConfig.role() << "'";
```
src/master/quota_handler.cpp
Lines 840 (patched)
<https://reviews.apache.org/r/70549/#comment302439>
Probably we should never use CopyFrom and stick with the following pattern:
```
*request.mutable_subject() = *subject;
```
That enables moving when the rhs is an rvalue, but in this case you need to explicitly move:
```
*request.mutable_subject() = std::move(*subject);
```
src/master/quota_handler.cpp
Lines 843 (patched)
<https://reviews.apache.org/r/70549/#comment302440>
Ditto here about avoiding copyFrom
- Benjamin Mahler
On May 22, 2019, 12:48 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70549/
> -----------------------------------------------------------
>
> (Updated May 22, 2019, 12:48 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Andrei Sekretenko, and Benjamin Mahler.
>
>
> Bugs: MESOS-9640
> https://issues.apache.org/jira/browse/MESOS-9640
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A new authorizable action `UPDATE_QUOTA_WITH_CONFIG` is added.
> This disambiguates with the old action `UPDATE_QUOTA` which
> are used for the old `SetQuota` and `RemoveQuota` calls.
> `UPDATE_QUOTA` action requires `QuotaInfo` as the object while
> the new `UpdatedQuota` call uses `QuotaConfig`. To keep it compatible
> with any external authorization modules, a new action is introduced.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.proto e2740c402732bb37db991ec92b9301e58b33215b
> src/master/master.hpp 5ad128d00d3cdf0dca89eb637ae196987bdce412
> src/master/quota_handler.cpp a18d8bafda5604d1844f7f7ed31d4ea80fbf6d04
> src/tests/master_authorization_tests.cpp ee69910a34416728bf14ed23f4a6faae6c1204a0
>
>
> Diff: https://reviews.apache.org/r/70549/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>