mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 46784: Added authorization of the '/flags' endpoint.
Date Mon, 09 May 2016 12:04:09 GMT


> On May 9, 2016, 9:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 2872
> > <https://reviews.apache.org/r/46784/diff/3/?file=1369724#file1369724line2872>
> >
> >     Why don't we return `MethodNotAllowed` here? Same applies to the agent, actually
: )
> 
> Jan Schlicht wrote:
>     Because `authorizeEndpoint` isn't supposed to create a `http::Response`. Its purpose
is to answer the question whether a principal is authorized to do things and it's the caller's
responsibility to create a HTTP response from that answer. But to create the answer, the function
needs informations that it extracts from the HTTP request. Because these informations may
be missing, we need to handle that case here, hence we fail the `Future`.

So now we return "HTTP/1.1 503 Service Unavailable" in this case. Do you think it's right?

In retrospect, there was a mistake to move request parsing into this function. You know check
that the request is correctly formed in `authorizeEndpoint()`, from where you can't return
`Response`. I suggest to split this function in two: one for parsing and another for mapping
principal to subject.


> On May 9, 2016, 9:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 879
> > <https://reviews.apache.org/r/46784/diff/3/?file=1369724#file1369724line879>
> >
> >     In r/46203/ you've cached flags and passed them into continuation. Here you
call `master->flags` in the continuation. Why is the approach different to the one for
the agent?
> 
> Jan Schlicht wrote:
>     See [MESOS-5293](https://issues.apache.org/jira/browse/MESOS-5293).
>     tldr; We can do it for the master, but (currently) not for the agent.

But it does not prevent us from following the pattern we used in the agent here, right?


- Alexander


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


On May 3, 2016, 12:42 p.m., Jan Schlicht wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> -----------------------------------------------------------
> 
> (Updated May 3, 2016, 12:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
>     https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>


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