You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jan Schlicht <ja...@mesosphere.io> on 2016/04/28 16:57:05 UTC
Review Request 46784: Added authorization of the '/flags' endpoint.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46784/
-----------------------------------------------------------
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 4f489fb9e5bc77b839cc4a44d7ec36af06f73e4a
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
Re: Review Request 46784: Added authorization of the '/flags'
endpoint.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On April 29, 2016, 11:20 a.m., Benjamin Bannier wrote:
> > src/master/http.cpp, line 868
> > <https://reviews.apache.org/r/46784/diff/1/?file=1364766#file1364766line868>
> >
> > Could you make this capture list explicit (`[this, request]`)?
>
> Jan Schlicht wrote:
> The style guide recommends to prefer default capture by value, that's why I'm doing it here. The case here is different from the one in `slave/http.cpp`. There we explicitly captured by value to be sure not to capture `this`, because that would lead to problems there. We don't have this problem here, hence the default capture by value.
Note that `[this, request]` *does* capture by value. I am just asking you to be explicit about what you capture instead of pulling in everything from the current scope.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46784/#review131062
-----------------------------------------------------------
On April 29, 2016, 11:49 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> -----------------------------------------------------------
>
> (Updated April 29, 2016, 11:49 a.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 4f489fb9e5bc77b839cc4a44d7ec36af06f73e4a
> 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
>
>
Re: Review Request 46784: Added authorization of the '/flags'
endpoint.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On April 29, 2016, 11:20 a.m., Benjamin Bannier wrote:
> > src/master/http.cpp, line 868
> > <https://reviews.apache.org/r/46784/diff/1/?file=1364766#file1364766line868>
> >
> > Could you make this capture list explicit (`[this, request]`)?
>
> Jan Schlicht wrote:
> The style guide recommends to prefer default capture by value, that's why I'm doing it here. The case here is different from the one in `slave/http.cpp`. There we explicitly captured by value to be sure not to capture `this`, because that would lead to problems there. We don't have this problem here, hence the default capture by value.
>
> Benjamin Bannier wrote:
> Note that `[this, request]` *does* capture by value. I am just asking you to be explicit about what you capture instead of pulling in everything from the current scope.
>
> Jan Schlicht wrote:
> As does `[=]`, and that's preferred by the style guide.
Hm, I read that part of the style guide differently, but well. I believe we value explicit over implicit, but even in this file there seems to be no preference either way, so feel free to drop this issue.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46784/#review131062
-----------------------------------------------------------
On April 29, 2016, 11:49 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> -----------------------------------------------------------
>
> (Updated April 29, 2016, 11:49 a.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 4f489fb9e5bc77b839cc4a44d7ec36af06f73e4a
> 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
>
>
Re: Review Request 46784: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 29, 2016, 11:20 a.m., Benjamin Bannier wrote:
> > src/master/http.cpp, line 868
> > <https://reviews.apache.org/r/46784/diff/1/?file=1364766#file1364766line868>
> >
> > Could you make this capture list explicit (`[this, request]`)?
>
> Jan Schlicht wrote:
> The style guide recommends to prefer default capture by value, that's why I'm doing it here. The case here is different from the one in `slave/http.cpp`. There we explicitly captured by value to be sure not to capture `this`, because that would lead to problems there. We don't have this problem here, hence the default capture by value.
>
> Benjamin Bannier wrote:
> Note that `[this, request]` *does* capture by value. I am just asking you to be explicit about what you capture instead of pulling in everything from the current scope.
As does `[=]`, and that's preferred by the style guide.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46784/#review131062
-----------------------------------------------------------
On April 29, 2016, 11:49 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> -----------------------------------------------------------
>
> (Updated April 29, 2016, 11:49 a.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 4f489fb9e5bc77b839cc4a44d7ec36af06f73e4a
> 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
>
>
Re: Review Request 46784: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On April 29, 2016, 11:20 a.m., Benjamin Bannier wrote:
> > src/master/http.cpp, line 868
> > <https://reviews.apache.org/r/46784/diff/1/?file=1364766#file1364766line868>
> >
> > Could you make this capture list explicit (`[this, request]`)?
The style guide recommends to prefer default capture by value, that's why I'm doing it here. The case here is different from the one in `slave/http.cpp`. There we explicitly captured by value to be sure not to capture `this`, because that would lead to problems there. We don't have this problem here, hence the default capture by value.
> On April 29, 2016, 11:20 a.m., Benjamin Bannier wrote:
> > src/master/http.cpp, line 874
> > <https://reviews.apache.org/r/46784/diff/1/?file=1364766#file1364766line874>
> >
> > Not your pattern and a general problem here, but this does not return any useful response should we fail to get an answer from the `authorizer` :(
It would return a failed future. Don't know what libprocess does with that, by I would expect that it would create a "server error" response.
> On April 29, 2016, 11:20 a.m., Benjamin Bannier wrote:
> > src/tests/master_authorization_tests.cpp, line 1024
> > <https://reviews.apache.org/r/46784/diff/1/?file=1364768#file1364768line1024>
> >
> > This looks very similar to the `SlaveEndpointTest` suite ;) Should we just also make this parameterized from the start?
I would like to, but unfortunately a parameterized test in GTest needs at least two values, at this point we have only one. It doesn't compile with a single parameter.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46784/#review131062
-----------------------------------------------------------
On April 28, 2016, 4:57 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> -----------------------------------------------------------
>
> (Updated April 28, 2016, 4:57 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 4f489fb9e5bc77b839cc4a44d7ec36af06f73e4a
> 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
>
>
Re: Review Request 46784: Added authorization of the '/flags'
endpoint.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46784/#review131062
-----------------------------------------------------------
src/master/http.cpp (line 868)
<https://reviews.apache.org/r/46784/#comment195014>
Could you make this capture list explicit (`[this, request]`)?
src/master/http.cpp (line 874)
<https://reviews.apache.org/r/46784/#comment195015>
Not your pattern and a general problem here, but this does not return any useful response should we fail to get an answer from the `authorizer` :(
src/tests/master_authorization_tests.cpp (line 1024)
<https://reviews.apache.org/r/46784/#comment195016>
This looks very similar to the `SlaveEndpointTest` suite ;) Should we just also make this parameterized from the start?
src/tests/master_authorization_tests.cpp (line 1027)
<https://reviews.apache.org/r/46784/#comment195019>
`s/agent/master/`
src/tests/master_authorization_tests.cpp (lines 1054 - 1057)
<https://reviews.apache.org/r/46784/#comment195018>
Once you have parameterized the fixture on the endpoint you should `s/Slave/Master/`. Before that it doesn't really fit.
src/tests/master_authorization_tests.cpp (line 1067)
<https://reviews.apache.org/r/46784/#comment195017>
`s/agent/master/`
- Benjamin Bannier
On April 28, 2016, 4:57 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> -----------------------------------------------------------
>
> (Updated April 28, 2016, 4:57 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 4f489fb9e5bc77b839cc4a44d7ec36af06f73e4a
> 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
>
>
Re: Review Request 46784: Added authorization of the '/flags'
endpoint.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46784/#review131140
-----------------------------------------------------------
Patch looks great!
Reviews applied: [46784]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 29, 2016, 9:49 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> -----------------------------------------------------------
>
> (Updated April 29, 2016, 9:49 a.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 4f489fb9e5bc77b839cc4a44d7ec36af06f73e4a
> 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
>
>
Re: Review Request 46784: Added authorization of the '/flags'
endpoint.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46784/
-----------------------------------------------------------
(Updated April 29, 2016, 11:49 a.m.)
Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
Changes
-------
Addressed Benjamin's issues (s/agent/master/)
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 (updated)
-----
src/master/http.cpp 4f489fb9e5bc77b839cc4a44d7ec36af06f73e4a
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
Re: Review Request 46784: Added authorization of the '/flags'
endpoint.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46784/#review130961
-----------------------------------------------------------
Patch looks great!
Reviews applied: [46784]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 28, 2016, 2:57 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> -----------------------------------------------------------
>
> (Updated April 28, 2016, 2:57 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 4f489fb9e5bc77b839cc4a44d7ec36af06f73e4a
> 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
>
>