You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2019/04/25 23:23:44 UTC
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/
-----------------------------------------------------------
Review request for mesos, 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_configs` is added.
This disambiguates with the old action `update_quota`
which are used for the old `SetQuota` and
`RemoveQuota` calls.
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 70549: Added authorization for `UpdateQuota` call
in the master.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70549/#review215073
-----------------------------------------------------------
include/mesos/authorizer/authorizer.proto
Lines 142-143 (patched)
<https://reviews.apache.org/r/70549/#comment301473>
An alternative would have been to stick with UPDATE_QUOTA and look for whichever of quota_info and quota_config is set.
After thinking about it, I'm guessing we don't want to do that since third party code is looking at these protos and might behave badly with the missing field..? Whereas it would fail more clearly with an unknown Action enum value?
Perhaps we can spell this out so it's clear in the code and description?
- Benjamin Mahler
On April 25, 2019, 11:23 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70549/
> -----------------------------------------------------------
>
> (Updated April 25, 2019, 11:23 p.m.)
>
>
> Review request for mesos, 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_configs` is added.
> This disambiguates with the old action `update_quota`
> which are used for the old `SetQuota` and
> `RemoveQuota` calls.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.proto e2740c402732bb37db991ec92b9301e58b33215b
> src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8
> src/master/quota_handler.cpp 5d449e6f027a69ccaa0ac3473ea4cf57441601f3
>
>
> Diff: https://reviews.apache.org/r/70549/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70549: Added authorization for `UpdateQuota` call
in the master.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On May 9, 2019, 6:43 a.m., Alexander Rukletsov wrote:
> > include/mesos/authorizer/authorizer.proto
> > Lines 138-140 (original), 139-144 (patched)
> > <https://reviews.apache.org/r/70549/diff/2/?file=2143530#file2143530line139>
> >
> > If my understanding is correct, both `UPDATE_QUOTA` and `UPDATE_QUOTA_CONFIG` are per role but use different objects in payload. Can we convert `QuotaConfig` to `QuotaInfo` for the purpose of authorization and spare extra action altogether? I see that authorization implementation is identical and does not rely on differences between `QuotaConfig` and `QuotaInfo`.
We cannot convert QuotaConfig to QuotaInfo without loss of information (e.g. limits).
While the local authorizer only looks at the role field, it is not guaranteed that an external module could look into other info of the QuotaInfo. This is why a new authorization is needed.
> On May 9, 2019, 6:43 a.m., Alexander Rukletsov wrote:
> > include/mesos/authorizer/authorizer.proto
> > Lines 143 (patched)
> > <https://reviews.apache.org/r/70549/diff/2/?file=2143530#file2143530line143>
> >
> > If you do plan to keep this action, the naming convention in this file suggests: `UPDATE_QUOTA_WITH_CONFIG` or `UPDATE_QUOTA_WITH_QUOTA_CONFIG`.
Sounds good.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70549/#review215160
-----------------------------------------------------------
On May 22, 2019, 5:48 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70549/
> -----------------------------------------------------------
>
> (Updated May 22, 2019, 5:48 a.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
>
>
Re: Review Request 70549: Added authorization for `UpdateQuota` call
in the master.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70549/#review215160
-----------------------------------------------------------
include/mesos/authorizer/authorizer.proto
Lines 138-140 (original), 139-144 (patched)
<https://reviews.apache.org/r/70549/#comment301682>
If my understanding is correct, both `UPDATE_QUOTA` and `UPDATE_QUOTA_CONFIG` are per role but use different objects in payload. Can we convert `QuotaConfig` to `QuotaInfo` for the purpose of authorization and spare extra action altogether? I see that authorization implementation is identical and does not rely on differences between `QuotaConfig` and `QuotaInfo`.
include/mesos/authorizer/authorizer.proto
Lines 143 (patched)
<https://reviews.apache.org/r/70549/#comment301683>
If you do plan to keep this action, the naming convention in this file suggests: `UPDATE_QUOTA_WITH_CONFIG` or `UPDATE_QUOTA_WITH_QUOTA_CONFIG`.
- Alexander Rukletsov
On May 6, 2019, 11 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70549/
> -----------------------------------------------------------
>
> (Updated May 6, 2019, 11 p.m.)
>
>
> Review request for mesos, 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_configs` 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 `update_quota_config`
> is introduced.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.proto e2740c402732bb37db991ec92b9301e58b33215b
> src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8
> src/master/quota_handler.cpp 5d449e6f027a69ccaa0ac3473ea4cf57441601f3
>
>
> Diff: https://reviews.apache.org/r/70549/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70549: Added authorization for `UpdateQuota` call
in the master.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 70549: Added authorization for `UpdateQuota` call
in the master.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On May 23, 2019, 4:23 a.m., Alexander Rukletsov wrote:
> > include/mesos/authorizer/authorizer.proto
> > Lines 149-150 (patched)
> > <https://reviews.apache.org/r/70549/diff/3/?file=2146379#file2146379line149>
> >
> > I would like us to challenge the necessity of passing `QuotaConfig` here. The built-in authorizer only looks at `role`, ignoring any information about resources. One might say that a custom authorizer might utilize that extra information, however, that extra information might not be enough to make a decision, because it does not include the current state or the state change, e.g., resource delta.
> >
> > Imagine an authorizer that allows decreasing quota for a number of principals, but only a few are allowed to increase. Passing `QuotaConfig` does not really help that authorizer to make a decision. Note that authorizer cannot tract previous requests to deduce the current quota state because previous request could have been dropped after successful authorization.
> >
> > I tend to keep it simple and use just the role. We should consult Till Tönshoff and Jan-Philip Gehrcke as maintainers of DC/OS authorizer for more input.
Thanks for the comment.
Yes, the two authorizers that I am aware of only looks into the role field.
I choose to pass `QuotaConfig` mainly to try to be parity with the legacy `QuotaInfo`, i.e. pass info regarding the requested quota setting.
For your example of needing more information (e.g. to also pass a role's current old quota config), I think if that need ever arise, we could add another optional object field. The Object message states that (https://github.com/apache/mesos/blob/a8c411d3f8d2895ff5e95c412ef2f3e94713520f/include/mesos/authorizer/authorizer.proto#L40-L46)
> An `Object` has a number of optional fields of which depending on the action, one or more fields must be set.
So we can use that as a wrapper for adding extra fields if needed.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70549/#review215484
-----------------------------------------------------------
On May 22, 2019, 5:48 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70549/
> -----------------------------------------------------------
>
> (Updated May 22, 2019, 5:48 a.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
>
>
Re: Review Request 70549: Added authorization for `UpdateQuota` call
in the master.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70549/#review215484
-----------------------------------------------------------
include/mesos/authorizer/authorizer.proto
Line 139 (original), 141-147 (patched)
<https://reviews.apache.org/r/70549/#comment302197>
How about something like:
```
// TODO(mzhu): Remove this action after associated API calls `SET_QUOTA` and `REMOVE_QUOTA` are no longer supported.
//
// NOTE: We cannot reuse this action for the `UPDATE_QUOTA` API call, because the associated `QuotaConfig` message contains more information than `QuotaInfo`.
```
include/mesos/authorizer/authorizer.proto
Lines 149-150 (patched)
<https://reviews.apache.org/r/70549/#comment302198>
I would like us to challenge the necessity of passing `QuotaConfig` here. The built-in authorizer only looks at `role`, ignoring any information about resources. One might say that a custom authorizer might utilize that extra information, however, that extra information might not be enough to make a decision, because it does not include the current state or the state change, e.g., resource delta.
Imagine an authorizer that allows decreasing quota for a number of principals, but only a few are allowed to increase. Passing `QuotaConfig` does not really help that authorizer to make a decision. Note that authorizer cannot tract previous requests to deduce the current quota state because previous request could have been dropped after successful authorization.
I tend to keep it simple and use just the role. We should consult Till Tönshoff and Jan-Philip Gehrcke as maintainers of DC/OS authorizer for more input.
- Alexander Rukletsov
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
>
>
Re: Review Request 70549: Added authorization for `UpdateQuota` call
in the master.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70549/
-----------------------------------------------------------
(Updated May 22, 2019, 5:48 a.m.)
Review request for mesos, Alexander Rukletsov, Andrei Sekretenko, and Benjamin Mahler.
Changes
-------
Addressed AlexR's comment.
Bugs: MESOS-9640
https://issues.apache.org/jira/browse/MESOS-9640
Repository: mesos
Description (updated)
-------
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/70549/diff/2-3/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 70549: Added authorization for `UpdateQuota` call
in the master.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70549/
-----------------------------------------------------------
(Updated May 6, 2019, 4 p.m.)
Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
Changes
-------
Clarified why a new action is introduced.
Bugs: MESOS-9640
https://issues.apache.org/jira/browse/MESOS-9640
Repository: mesos
Description (updated)
-------
A new authorizable action `update_quota_configs` 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 `update_quota_config`
is introduced.
Diffs (updated)
-----
include/mesos/authorizer/authorizer.proto e2740c402732bb37db991ec92b9301e58b33215b
src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8
src/master/quota_handler.cpp 5d449e6f027a69ccaa0ac3473ea4cf57441601f3
Diff: https://reviews.apache.org/r/70549/diff/2/
Changes: https://reviews.apache.org/r/70549/diff/1-2/
Testing
-------
make check
Thanks,
Meng Zhu