You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rojas <al...@mesosphere.io> on 2017/05/03 09:26:56 UTC
Review Request 58955: Adds authorization for the master's v1 API call
SET_LOGGING_LEVEL.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58955/
-----------------------------------------------------------
Review request for mesos, Adam B and Greg Mann.
Bugs: MESOS-7414
https://issues.apache.org/jira/browse/MESOS-7414
Repository: mesos
Description
-------
Enables authorization in the v1 API call `SET_LOGGING_LEVEL` following
the existing implementation for the agent call with the same name.
Likewise it reuses the authorizer action `SET_LOG_LEVEL`.
Diffs
-----
src/master/http.cpp 5aae52870451d883ef1ea1fda5a27764d7f318e8
src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09
Diff: https://reviews.apache.org/r/58955/diff/1/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 58955: Adds authorization for the master's v1 API
call SET_LOGGING_LEVEL.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On May 17, 2017, 10:51 a.m., Adam B wrote:
> > src/master/http.cpp
> > Lines 2056 (patched)
> > <https://reviews.apache.org/r/58955/diff/3/?file=1716633#file1716633line2056>
> >
> > Should we modify the v0 `/logging/toggle` endpoint to use this authorization::action instead of authorization::GET_ENDPOINT_WITH_PATH ? What's the compatibility/deprecation process there?
We discussed that at some point and decided to not modify it, though that could change, but I also wonder how the deprecation/transition here would work
> On May 17, 2017, 10:51 a.m., Adam B wrote:
> > src/tests/api_tests.cpp
> > Lines 772 (patched)
> > <https://reviews.apache.org/r/58955/diff/3/?file=1716634#file1716634line772>
> >
> > Is there a v0 equivalent of this test for `/logging/toggle`?
[logging_tests.cpp](https://github.com/apache/mesos/blob/master/src/tests/logging_tests.cpp)
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58955/#review175220
-----------------------------------------------------------
On May 12, 2017, 11:28 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58955/
> -----------------------------------------------------------
>
> (Updated May 12, 2017, 11:28 a.m.)
>
>
> Review request for mesos, Adam B and Greg Mann.
>
>
> Bugs: MESOS-7414
> https://issues.apache.org/jira/browse/MESOS-7414
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enables authorization in the v1 API call `SET_LOGGING_LEVEL` following
> the existing implementation for the agent call with the same name.
> Likewise it reuses the authorizer action `SET_LOG_LEVEL`.
>
>
> Diffs
> -----
>
> src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31
> src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09
>
>
> Diff: https://reviews.apache.org/r/58955/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 58955: Adds authorization for the master's v1 API
call SET_LOGGING_LEVEL.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58955/#review175220
-----------------------------------------------------------
Looks pretty good. Just some questions about the v0 /logging/toggle endpoint
src/master/http.cpp
Lines 2056 (patched)
<https://reviews.apache.org/r/58955/#comment248656>
Should we modify the v0 `/logging/toggle` endpoint to use this authorization::action instead of authorization::GET_ENDPOINT_WITH_PATH ? What's the compatibility/deprecation process there?
src/master/http.cpp
Lines 2062-2064 (patched)
<https://reviews.apache.org/r/58955/#comment248654>
I don't care what clang-format says, this is much uglier than my preferred wrapping:
```
[level, duration](const Owned<ObjectApprover>& approver)
-> Future<Response> {
Try<bool> approved = approver->approved((ObjectApprover::Object()));
```
src/tests/api_tests.cpp
Lines 772 (patched)
<https://reviews.apache.org/r/58955/#comment248657>
Is there a v0 equivalent of this test for `/logging/toggle`?
- Adam B
On May 12, 2017, 2:28 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58955/
> -----------------------------------------------------------
>
> (Updated May 12, 2017, 2:28 a.m.)
>
>
> Review request for mesos, Adam B and Greg Mann.
>
>
> Bugs: MESOS-7414
> https://issues.apache.org/jira/browse/MESOS-7414
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enables authorization in the v1 API call `SET_LOGGING_LEVEL` following
> the existing implementation for the agent call with the same name.
> Likewise it reuses the authorizer action `SET_LOG_LEVEL`.
>
>
> Diffs
> -----
>
> src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31
> src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09
>
>
> Diff: https://reviews.apache.org/r/58955/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 58955: Adds authorization for the master's v1 API
call SET_LOGGING_LEVEL.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58955/#review177605
-----------------------------------------------------------
Ship it!
Ship It!
- Till Toenshoff
On June 2, 2017, 12:25 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58955/
> -----------------------------------------------------------
>
> (Updated June 2, 2017, 12:25 p.m.)
>
>
> Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-7414
> https://issues.apache.org/jira/browse/MESOS-7414
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enables authorization in the v1 API call `SET_LOGGING_LEVEL` following
> the existing implementation for the agent call with the same name.
> Likewise it reuses the authorizer action `SET_LOG_LEVEL`.
>
>
> Diffs
> -----
>
> src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323
> src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54
>
>
> Diff: https://reviews.apache.org/r/58955/diff/5/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 58955: Adds authorization for the master's v1 API
call SET_LOGGING_LEVEL.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58955/
-----------------------------------------------------------
(Updated June 2, 2017, 2:25 p.m.)
Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.
Bugs: MESOS-7414
https://issues.apache.org/jira/browse/MESOS-7414
Repository: mesos
Description (updated)
-------
Enables authorization in the v1 API call `SET_LOGGING_LEVEL` following
the existing implementation for the agent call with the same name.
Likewise it reuses the authorizer action `SET_LOG_LEVEL`.
Diffs (updated)
-----
src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323
src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54
Diff: https://reviews.apache.org/r/58955/diff/5/
Changes: https://reviews.apache.org/r/58955/diff/4-5/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 58955: Adds authorization for the master's v1 API
call SET_LOGGING_LEVEL.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58955/#review176620
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/api_tests.cpp
Line 756 (original), 765 (patched)
<https://reviews.apache.org/r/58955/#comment249985>
Newline after.
- Greg Mann
On May 22, 2017, 3:57 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58955/
> -----------------------------------------------------------
>
> (Updated May 22, 2017, 3:57 p.m.)
>
>
> Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-7414
> https://issues.apache.org/jira/browse/MESOS-7414
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enables authorization in the v1 API call `SET_LOGGING_LEVEL` following
> the existing implementation for the agent call with the same name.
> Likewise it reuses the authorizer action `SET_LOG_LEVEL`.
>
>
> Diffs
> -----
>
> src/master/http.cpp 7060b8fa53e0682681c50e051908ffbbf50fb7da
> src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09
>
>
> Diff: https://reviews.apache.org/r/58955/diff/4/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 58955: Adds authorization for the master's v1 API
call SET_LOGGING_LEVEL.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58955/#review176636
-----------------------------------------------------------
src/tests/api_tests.cpp
Lines 766 (patched)
<https://reviews.apache.org/r/58955/#comment250006>
+1
src/tests/api_tests.cpp
Lines 770 (patched)
<https://reviews.apache.org/r/58955/#comment250010>
This is why we have added the `AWAIT_EXPECT_RESPONSE_STATUS_EQ` macro. Let's please use that instead, makes the code a lot slicker.
Future<http::Response> response = http::post(
master.get()->pid,
"api/v1",
headers,
serialize(contentType, v1Call),
stringify(contentType));
AWAIT_EXPECT_RESPONSE_STATUS_EQ(http::Forbidden().status, response);
There are already a couple of places outside these new additions using that unfortunate pattern - could you please clean those up in a follow up?
- Till Toenshoff
On May 22, 2017, 3:57 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58955/
> -----------------------------------------------------------
>
> (Updated May 22, 2017, 3:57 p.m.)
>
>
> Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.
>
>
> Bugs: MESOS-7414
> https://issues.apache.org/jira/browse/MESOS-7414
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enables authorization in the v1 API call `SET_LOGGING_LEVEL` following
> the existing implementation for the agent call with the same name.
> Likewise it reuses the authorizer action `SET_LOG_LEVEL`.
>
>
> Diffs
> -----
>
> src/master/http.cpp 7060b8fa53e0682681c50e051908ffbbf50fb7da
> src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09
>
>
> Diff: https://reviews.apache.org/r/58955/diff/4/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 58955: Adds authorization for the master's v1 API
call SET_LOGGING_LEVEL.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58955/
-----------------------------------------------------------
(Updated May 22, 2017, 5:57 p.m.)
Review request for mesos, Adam B and Greg Mann.
Bugs: MESOS-7414
https://issues.apache.org/jira/browse/MESOS-7414
Repository: mesos
Description
-------
Enables authorization in the v1 API call `SET_LOGGING_LEVEL` following
the existing implementation for the agent call with the same name.
Likewise it reuses the authorizer action `SET_LOG_LEVEL`.
Diffs (updated)
-----
src/master/http.cpp 7060b8fa53e0682681c50e051908ffbbf50fb7da
src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09
Diff: https://reviews.apache.org/r/58955/diff/4/
Changes: https://reviews.apache.org/r/58955/diff/3-4/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 58955: Adds authorization for the master's v1 API
call SET_LOGGING_LEVEL.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58955/
-----------------------------------------------------------
(Updated May 12, 2017, 11:28 a.m.)
Review request for mesos, Adam B and Greg Mann.
Bugs: MESOS-7414
https://issues.apache.org/jira/browse/MESOS-7414
Repository: mesos
Description
-------
Enables authorization in the v1 API call `SET_LOGGING_LEVEL` following
the existing implementation for the agent call with the same name.
Likewise it reuses the authorizer action `SET_LOG_LEVEL`.
Diffs (updated)
-----
src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31
src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09
Diff: https://reviews.apache.org/r/58955/diff/3/
Changes: https://reviews.apache.org/r/58955/diff/2-3/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 58955: Adds authorization for the master's v1 API
call SET_LOGGING_LEVEL.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58955/
-----------------------------------------------------------
(Updated May 9, 2017, 5:43 p.m.)
Review request for mesos, Adam B and Greg Mann.
Changes
-------
rebase.
Bugs: MESOS-7414
https://issues.apache.org/jira/browse/MESOS-7414
Repository: mesos
Description
-------
Enables authorization in the v1 API call `SET_LOGGING_LEVEL` following
the existing implementation for the agent call with the same name.
Likewise it reuses the authorizer action `SET_LOG_LEVEL`.
Diffs (updated)
-----
src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31
src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09
Diff: https://reviews.apache.org/r/58955/diff/2/
Changes: https://reviews.apache.org/r/58955/diff/1-2/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 58955: Adds authorization for the master's v1 API
call SET_LOGGING_LEVEL.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58955/#review173814
-----------------------------------------------------------
LGTM. Minor points below.
src/tests/api_tests.cpp
Lines 741 (patched)
<https://reviews.apache.org/r/58955/#comment246854>
s/launch nested container sessions/set the logging level/
src/tests/api_tests.cpp
Lines 742-744 (patched)
<https://reviews.apache.org/r/58955/#comment246855>
Could fit on one line?
src/tests/api_tests.cpp
Lines 757-774 (original), 768-803 (patched)
<https://reviews.apache.org/r/58955/#comment246895>
What do you think about placing each of these requests into an enclosing scope? It's a pattern we follow elsewhere, and I think it makes the code a bit more readable.
i.e.
```
{
http::Headers headers = stuff;
Future<Nothing> response = http::post( ... );
AWAIT_READY(response);
}
{
http::Headers headers = stuff;
Future<Nothing> response = http::post( ... );
AWAIT_READY(response);
ASSERT_EQ( ... );
}
```
- Greg Mann
On May 3, 2017, 9:26 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58955/
> -----------------------------------------------------------
>
> (Updated May 3, 2017, 9:26 a.m.)
>
>
> Review request for mesos, Adam B and Greg Mann.
>
>
> Bugs: MESOS-7414
> https://issues.apache.org/jira/browse/MESOS-7414
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enables authorization in the v1 API call `SET_LOGGING_LEVEL` following
> the existing implementation for the agent call with the same name.
> Likewise it reuses the authorizer action `SET_LOG_LEVEL`.
>
>
> Diffs
> -----
>
> src/master/http.cpp 5aae52870451d883ef1ea1fda5a27764d7f318e8
> src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09
>
>
> Diff: https://reviews.apache.org/r/58955/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>