mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mcyp...@gmail.com>
Subject Re: Review Request 36049: Added support for modularized Authorizer
Date Sun, 09 Aug 2015 07:15:16 GMT

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



src/local/local.cpp (lines 217 - 219)
<https://reviews.apache.org/r/36049/#comment149269>

    Does it make sense to log a warning for this?
    
    ```
    if (flags.authorizers != master::DEFAULT_AUTHORIZER && flags.acls.isSome()) {
      LOG(WARNNING) << /* acls are being ignored! */;
    }
    ```



src/local/local.cpp (lines 238 - 248)
<https://reviews.apache.org/r/36049/#comment149268>

    This behavior is noticeably different, since we used to simply `EXIT(EXIT_FAILURE)` on
this error. What's the rationale here?
    
    My thoughts: if an authorizer fails to initialize given some ACLs, it's seems likely that
the ACLs are ill-formed. In which case, the intent from the user is clear that they __want__
authorization, but they made a mistake in their ACL construction. Therefore, it seems more
reasonable for us to exit with an error rather than ignoring their intent and skipping authorization
altogether.



src/master/flags.cpp (line 417)
<https://reviews.apache.org/r/36049/#comment149270>

    Can we expand a little upon `Authorizer implementation to use.`?
    
    Maybe something like: `Authorizer implementation to use when authorizating actions that
required it.`?



src/master/flags.cpp (line 422)
<https://reviews.apache.org/r/36049/#comment149271>

    `s/than default/than the default`
    `s/contents//`



src/master/flags.cpp (line 425)
<https://reviews.apache.org/r/36049/#comment149272>

    I think we can follow what we do for `authenticators` in `src/master/master.cpp` to indicate
the lack of support for multiple authorizers.
    
    (1) split `flags.authorizers` by `,`
    (2) if empty -> no authorizers specified
    (3) if size > 1, multiple authorizers not supported
    (4) proceed with the first name.



src/master/main.cpp (lines 346 - 379)
<https://reviews.apache.org/r/36049/#comment149273>

    Same comments as `src/local/local.cpp`


- Michael Park


On Aug. 5, 2015, 9:15 a.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2015, 9:15 a.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Jan Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-2947
>     https://issues.apache.org/jira/browse/MESOS-2947
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adds and integrates helper classes needed to support an `Authorizer` module. Also adds
a flag to the master, allowing the selection of an `Authorizer` module.
> 
> 
> Diffs
> -----
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am 54eaf205eecb6bf1a9a5c4b5ddad55f46ad635ec 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
>   src/tests/cluster.hpp ba17c0c74a9dc36c595c4ad77fe68be94c5c7c0b 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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