Re: Review Request 58955: Adds authorization for the master's v1 API call SET_LOGGING_LEVEL.

[Till Toenshoff]

---
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.

[Alexander Rojas]

---
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.

[Till Toenshoff]

---
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)


+1



src/tests/api_tests.cpp
Lines 770 (patched)


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 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.

[Greg Mann]

---
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)


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.

[Alexander Rojas]

---
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.

[Alexander Rojas]

> On May 17, 2017, 10:51 a.m., Adam B wrote:
> > src/master/http.cpp
> > Lines 2056 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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.

[Adam B]

---
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)


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)


I don't care what clang-format says, this is much uglier than my preferred 
wrapping:
```
  [level, duration](const Owned& approver)
  -> Future {
Try approved = approver->approved((ObjectApprover::Object()));
```



src/tests/api_tests.cpp
Lines 772 (patched)


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.

[Alexander Rojas]

---
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.

[Alexander Rojas]

---
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.

[Greg Mann]

---
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)


s/launch nested container sessions/set the logging level/



src/tests/api_tests.cpp
Lines 742-744 (patched)


Could fit on one line?



src/tests/api_tests.cpp
Lines 757-774 (original), 768-803 (patched)


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 response = http::post( ... );
  AWAIT_READY(response);
}

{
  http::Headers headers = stuff;
  Future 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
> 
>

Review Request 58955: Adds authorization for the master's v1 API call SET_LOGGING_LEVEL.

[Alexander Rojas]

---
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