mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.
Date Thu, 09 Feb 2017 21:59:24 GMT

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



Looks good to me, just a few questions, I will let Adam ship this.


src/authorizer/local/authorizer.cpp (line 246)
<https://reviews.apache.org/r/56178/#comment236851>

    newline between comment blocks? e.g.
    ```
    // We also update the deprecated `value` field to support custom
    // authorizers not yet modified to examine `framework_info`.
    //
    // TODO(bbannier): Clean up use of `value` here, see MESOS-7091.
    ```



src/authorizer/local/authorizer.cpp (line 247)
<https://reviews.apache.org/r/56178/#comment236854>

    Not yours, but I find it rather confusing as to what the object value is, looking at the
other code, is it the role? It would be nice to clarify how one figures out what `value` represents.



src/master/master.cpp (line 2173)
<https://reviews.apache.org/r/56178/#comment236845>

    comma after "frameworks"?



src/master/master.cpp (line 2178)
<https://reviews.apache.org/r/56178/#comment236841>

    newline between the comment blocks? e.g.
    
      // For non-`MULTI_ROLE` frameworks also propagate its single role
      // via the request's `value` field. This is purely for backwards
      // compatibility as the `value` field is deprecated. Note that this
      // means that authorizers relying on the deprecated field will see
      // an empty string in `value` for for `MULTI_ROLE` frameworks.
      //
      // TODO(bbannier): Remove this at the end of `value`'s deprecation
      // cycle, see MESOS-7073.



src/master/master.cpp (lines 2533 - 2535)
<https://reviews.apache.org/r/56178/#comment236840>

    Longer term, are there any thoughts on how we might be able to know which role is not
authorized? E.g. getting the authorization message via `Future<Option<Error>>`?


- Benjamin Mahler


On Feb. 9, 2017, 9:26 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:26 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