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:00:16 GMT


> On May 7, 2016, 10:24 a.m., Adam B wrote:
> > src/master/http.cpp, line 869
> > <https://reviews.apache.org/r/46784/diff/3/?file=1369724#file1369724line869>
> >
> >     Did you need to pass all ("=") the in-scope variables through to the lambda,
or just `request`?
> 
> Jan Schlicht wrote:
>     Yes, I only need to capture `request` here, but the way I understand the C++ style
guide, "default capture by value" ("=") is preferred over "explicit capture by value".

I'm not sure this is what the styleguide says. I read "Prefer default capture by value, explicit
capture by value, then capture by reference." as explicit and default captures both take precedence
over captures by reference. Anyway, if explicit capture is fine, this is the best use case
for it: a lambda taking one parameter, which is exactly your case. Please change this.


> On May 7, 2016, 10:24 a.m., Adam B wrote:
> > src/master/http.cpp, line 2846
> > <https://reviews.apache.org/r/46784/diff/3/?file=1369724#file1369724line2846>
> >
> >     There's a lot of code duplication between here and `Slave::Http::authorizeEndpoint()`.
Can you share/reuse code between the two?
> 
> Jan Schlicht wrote:
>     Indeed, that's quite some code duplication. But there are also some slight differences
in both functions. Had a discussion with @arojas about how to handle that case. On one hand
it's good to remove code duplication, on the other hand the extracted function would look
a bit more complicated to support the different requirements of master and agent. We decided
that in this particular case it's okay to stick with the two similar functions.

If we split this function in two (per suggestion below), the copy-paste wouldn't be so drastic:
we already have a lot of functions that do mapping principal->subject (it is unfortunate
we have to repeat it again and again) and a fucntion that extracts endpoint from an URL path.
Maybe we can add a helper in `Request` for this one.


- Alexander


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


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