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



src/master/master.cpp (lines 3190 - 3191)
<https://reviews.apache.org/r/39989/#comment167968>

    As I asked and pointed out in https://reviews.apache.org/r/39987/, this is 
not true for launch task operations.
    
    We can see below that launch task validation is performed after 
authorization. Couldn't we do the same thing?
    
    ```cpp
    Future<bool> authorization = authorizations.front();
    authorizations.pop_front();
    
    ...
    
    CHECK(!authorization.isDiscarded());
    
    if (authorization.isFailed() || !authorization.get()) {
      // authorization failed; drop or send status update.
    }
    
    Option<Error> error = <...>::validate(...);
    if (error.isSome()) {
      // validation failed: drop or send status update.
    }
    
    ...
    ```



src/master/master.cpp (lines 3201 - 3206)
<https://reviews.apache.org/r/39989/#comment167969>

    This case is actually a validation error, yet our error message is 
"authorization failure" which is a bit inaccurate. Seems to me like a result of 
bundling validation within authorization.


- Michael Park


On Nov. 30, 2015, 9:20 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> -----------------------------------------------------------
> 
> (Updated Nov. 30, 2015, 9:20 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
>     https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
>   src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> -------
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Reply via email to