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 46203: Added authorization of the '/flags' endpoint.
Date Tue, 26 Apr 2016 15:49:51 GMT


> On April 20, 2016, 8:35 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 658-660
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350392#file1350392line658>
> >
> >     Where did you come up with the magic number 3? What if we reorganize the operator
endpoints in the (1.0) future? How will we know what the new value should be here?
> >     What if the user setup a reverse proxy (like in dcos) and these requests are
actually coming from a different base url than expected?
> 
> Benjamin Bannier wrote:
>     @adam: The three here is needed so that this just strips the agent part of the path,
not everything up to the last `/`. An example endpoint would be `/slave(1)/monitor/statistics`.
> 
> Jan Schlicht wrote:
>     Seems like a hard problem to fully support both requirements. Maybe reverting back
to using `std::string` instead of `http::URL` as the function parameter for `endpoint` could
resolve this.
> 
> Benjamin Bannier wrote:
>     Please use some typed entity that the usual endpoint handlers are aware of. They
currently have a `Request`, but e.g., have no idea how they are being routed.
> 
> Jan Schlicht wrote:
>     I'll go back to using the "magic number 3". At this point `URL::path` will look like
this: "/slave(n)/name/of/endpoint". By splitting into 3 components we get rid of the "/slave(n)/".
The path is not the full URL that has been requested, hence reverse proxies shouldn't be an
issue here. I'll add a comment, explaining this.
> 
> Adam B wrote:
>     I see. And will this value be the same for the master's endpoints?
>     Good to hear that reverse proxies won't be affected since it's not a full URL.
> 
> Jan Schlicht wrote:
>     This values won't work for the master's endpoint. In that case `URL::path` will be
"/name/of/endpoint" and we wouldn't need to split. Because we're in `Slave::Http` we can expect
that this code is only called for agents.
> 
> Alexander Rojas wrote:
>     So here is my issue wit this, you break it into three, and pass only the second one
to the authorizer, but that just sets a bad precedent. There are endpoint that added with
more components, e.g. `/api/v1/scheduler`. The right way to solve this is to do something
like:
>     
>     ```c++
>     // … code to handle when `url.path` is empty.
>     
>     std::string path = url.path;
>     std::size_t position = path.find('/', 1); 
>     if (position != std::string::npos) {
>       path = path.substr(position);
>     }
>     
>     // Call the authorizer.
>     ```
>     
>     And we can add code to the authorizer module instead on how to handle objects which
encode paths (just like we dispatch to the endpoint handlers).
> 
> Alexander Rojas wrote:
>     Forget what I said, I now understand what split with parameter does.

Once we `tokenize` instead of `split`, 3 will become 2, which will make it easier to understand
where the magic number is coming from.


- Alexander


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


On April 26, 2016, 3:27 p.m., Jan Schlicht wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
> 
> (Updated April 26, 2016, 3:27 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5142
>     https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab 
>   include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 40d93ea257d1df8d22eee8a21667db90d579a8fe

>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
>   src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
>   src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46203/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>


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