mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joerg Schad <jo...@mesosphere.io>
Subject Re: Review Request 49394: Added support for VIEW_FLAGS authorization action in HTTP API.
Date Thu, 30 Jun 2016 12:32:35 GMT

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




src/master/http.cpp (line 43)
<https://reviews.apache.org/r/49394/#comment205443>

    Could you link the Jira to this review?



src/master/http.cpp (line 1449)
<https://reviews.apache.org/r/49394/#comment205452>

    AFAIk we shouldn't use this _var pattern for parameters, but I saw the comment of you
sheperd so I assume it is ok....



src/master/http.cpp (line 1450)
<https://reviews.apache.org/r/49394/#comment205451>

    Shouldn't this be indendation of two



src/master/http.cpp (line 1458)
<https://reviews.apache.org/r/49394/#comment205450>

    This construct (free function in the class) seems surprising/non trivial. Could we maybe
add a short comment on top?



src/master/http.cpp (line 1505)
<https://reviews.apache.org/r/49394/#comment205444>

    Could we potentially expose sensitive information in the error message? If so, let us
use a generic "Could not ..." error message



src/master/http.cpp (line 1513)
<https://reviews.apache.org/r/49394/#comment205446>

    Do we need a Future<Try>> ? I assume you are using it for the custom error type,
maybe we can later extend FUTURE for custom failure types as well.
    
    AFAIK we should be at least very careful when using this, but given that this doesn't
touch the API we can change it later (once we introduced custom FUTURE error/failures).



src/master/http.cpp (line 1528)
<https://reviews.apache.org/r/49394/#comment205447>

    Looking at this once more (I know we discussed that :-)) the following looks better: 
    .then(defer(master->self(),
        [this](bool authorized) 
          -> Future<Try<JSON::Object, FlagsError>> {



src/master/http.cpp (line 1566)
<https://reviews.apache.org/r/49394/#comment205448>

    See formatting suggestion above



src/master/http.cpp (line 1579)
<https://reviews.apache.org/r/49394/#comment205453>

    serialize(
        contentType,
        evolve<v1::master::Response::GET_FLAGS>(flags.get())),
        stringify(contentType));


- Joerg Schad


On June 30, 2016, 11:34 a.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49394/
> -----------------------------------------------------------
> 
> (Updated June 30, 2016, 11:34 a.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adds an intermediate function `Master::http::_flags()` which performs
> authorization and it is called by both the endpoint `/flags` handler
> and the HTTP API v1 call `flags` handler.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp e5acdb8e0bbcd7a2b7e8a8bc7f4bbeaae2c4fea1 
>   src/master/master.hpp e2ab2110fe5a287ab16ac9ef4222fed633e02ebe 
> 
> Diff: https://reviews.apache.org/r/49394/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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