mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.
Date Thu, 09 Feb 2017 08:59:38 GMT


> On Feb. 3, 2017, 11:26 a.m., Adam B wrote:
> > src/authorizer/local/authorizer.cpp, line 244
> > <https://reviews.apache.org/r/56178/diff/1/?file=1621348#file1621348line244>
> >
> >     Seems like framework_info is always set, so how/why would we ever fall through
to the other cases?
> 
> Benjamin Bannier wrote:
>     Yes, this is currently dead code. I was also wondering whether it should be removed,
but decided against it since it provides some level of redundancy as long as `value` still
exists and code in the master and in authorizers might not evolve consistently.
>     
>     Do you believe it should be removed?
> 
> Alexander Rojas wrote:
>     The main reason the `object->value` is still there, is that the local authorizer
is a reference implementation for module writers who want to build their own modules, as such,
it does provide a reference. I myself will vote to remove the `value` field if possible. However,
we makred as deprecated in November 2016 which means we need it there until June (at the same
time it had said it is supposed to be removed on 1.2).
> 
> Adam B wrote:
>     Alright. Please add a comment explaining why we're keeping dead code around, so I
am not tempted to delete it next time I see it.
> 
> Benjamin Bannier wrote:
>     Yes, a comment is in order here, updated the patch. I also slipped in a `TODO` to
remove this branch when `value` is purged.
> 
> Adam B wrote:
>     This code is in the LocalAuthorizer, which doesn't support other custom authorizers.
While Mesos master/agent should continue to set both FrameworkInfo and `value`, to support
custom authorizers, an authorizer implementation like LocalAuthorizer doesn't need to look
at `value` if it's already looking at FrameworkInfo. Even as a reference implementation, we
should not show an example of using the deprecated field. I think we can safely remove this
entire block.
>     I am, however, happy that we now have MESOS-7073 to track the eventual removal of
the value field altogether.

This requires updating a number of tests which still set value. I would like to avoid pulling
this work into this change and created MESOS-7091 to track this work.


- Benjamin


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


On Feb. 9, 2017, 9:59 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56178/
> -----------------------------------------------------------
> 
> (Updated Feb. 9, 2017, 9:59 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7022
>     https://issues.apache.org/jira/browse/MESOS-7022
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This updates the local authorizer so that MULTI_ROLE frameworks can be
> authorized.
> 
> For non-MULTI_ROLE frameworks we continue to support use of the
> deprecated 'value' field in the authorization request's 'Object';
> however for MULTI_ROLE frameworks the 'value' field will not be set,
> and authorizers still relying on it should be updated to instead use
> the object's 'framework_info' field to extract roles to authorize
> against from.
> 
> 
> Diffs
> -----
> 
>   src/authorizer/local/authorizer.cpp b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
>   src/master/master.cpp 620919ecfe85367b5c1281afc5216cc20e5e2e3c 
> 
> Diff: https://reviews.apache.org/r/56178/diff/
> 
> 
> Testing
> -------
> 
> Tested on various configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message