Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Alexander Rukletsov]

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


Fix it, then Ship it!




I'll fix the remamining issues and commit it for you shortly.


src/slave/http.cpp (line 627)


Indentation.



src/slave/http.cpp (line 628)


Indentation.



src/slave/http.cpp (line 637)


Indentation.



src/tests/slave_authorization_tests.cpp (lines 182 - 183)


Wrap differently to avoid jaggeddness.



src/tests/slave_authorization_tests.cpp (line 197)


Let's wrap `Return(true)` onto the next line for readability. It's much 
faster to grasp what is going on when each action is on a separate line.



src/tests/slave_authorization_tests.cpp (line 199)


You can omit `process::` prefix.



src/tests/slave_authorization_tests.cpp (line 207)


You use it once, hence it doesn't really make sense to introduce a variable.



src/tests/slave_authorization_tests.cpp (line 234)


Ditto.



src/tests/slave_authorization_tests.cpp (line 245)


Looks like the first "for" is redundant.



src/tests/slave_authorization_tests.cpp (lines 253 - 255)


Any reason why you don't just use `StartSlave`? It would also fit one line 
then.



src/tests/slave_authorization_tests.cpp (line 257)


Ditto.



src/tests/slave_authorization_tests.cpp (line 268)


"The tests are parameterized by the endpoint being queried."

Add a `TODO` about also considering parameterizing the authz action, e.g. 
`authorization::GET_ENDPOINT_WITH_PATH`



src/tests/slave_authorization_tests.cpp (lines 268 - 272)


Let's pull it up closer to `SlaveEndpointTest` fixture.


- Alexander Rukletsov


On April 27, 2016, 3:28 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 27, 2016, 3:28 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> `GTEST_FILTER="*SlaveEndpointTest*:*SlaveAuthorizationTest*" 
> ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

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

(Updated April 27, 2016, 3:28 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.


Bugs: MESOS-5164
https://issues.apache.org/jira/browse/MESOS-5164


Repository: mesos


Description
---

Added authorization to agents' `/monitor/statistics` endpoints.


Diffs
-

  src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
  src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
  src/tests/slave_authorization_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/46319/diff/


Testing (updated)
---

make check (OS X, clang w/o optimization)
`GTEST_FILTER="*SlaveEndpointTest*:*SlaveAuthorizationTest*" 
./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure`


Thanks,

Benjamin Bannier

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45922, 46318, 46203, 46319]

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 27, 2016, 12:41 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 27, 2016, 12:41 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

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

(Updated April 27, 2016, 2:41 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.


Changes
---

Added a `TODO`.


Bugs: MESOS-5164
https://issues.apache.org/jira/browse/MESOS-5164


Repository: mesos


Description
---

Added authorization to agents' `/monitor/statistics` endpoints.


Diffs (updated)
-

  src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
  src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
  src/tests/slave_authorization_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/46319/diff/


Testing
---

make check (OS X, clang w/o optimization)


Thanks,

Benjamin Bannier

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

> On April 27, 2016, 12:59 p.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, line 179
> > 
> >
> > { belongs on newline.

Ups. On Alex's request I now removed any additional state from the fixture and 
inline agent setup into each test. This leads to an empty class body which 
again fits on a single line.


> On April 27, 2016, 12:59 p.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, line 187
> > 
> >
> > "Prefer trailing underscores for use as member fields", so you've got 
> > this reversed.
> 
> Alexander Rukletsov wrote:
> Unfortunately, that rule is not really welcomed by all members of the 
> community, maybe we should even remove it (though I personally love it).

As I now removed this code this does comment does not apply anymore.


> On April 27, 2016, 12:59 p.m., Adam B wrote:
> > src/slave/http.cpp, line 629
> > 
> >
> > Is there any standard for what order these [pid, limiter, request] 
> > should go in?

I cannot find any explicit guidance on this in the style guide. My intuition 
here was to mention `Http` member-like variables (`pid` and `limiter`) before 
parameter-like variables (`request`).


> On April 27, 2016, 12:59 p.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, lines 204-207
> > 
> >
> > I thought this was done in SetUp() now

Ups. I removed the fixture's `SetUp` so this comment does not apply anymore.


> On April 27, 2016, 12:59 p.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, lines 252-254
> > 
> >
> > How is this testing with "no authorizer"? Doesn't SetUp use 
> > MockAuthorizer? Looks like MockAuthorizer::authorized() returns true by 
> > default. Might as well just remove this test.

Thanks for catching this. The original intention here was to create an agent 
w/o authorization, but at some point I regressed to passing in an authorizer. I 
now fixed the way the agent is created, and propose that we leave this test in.


- Benjamin


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


On April 27, 2016, 2:18 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 27, 2016, 2:18 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, line 635
> > 
> >
> > Indent the block by 2 spaces, not 4.
> 
> Benjamin Bannier wrote:
> ditto.

Fixed.


> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, line 637
> > 
> >
> > Indent the block by 2 spaces, not 4.
> 
> Benjamin Bannier wrote:
> ditto.

Fixed.


> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/tests/slave_authorization_tests.cpp, lines 172-173
> > 
> >
> > I was about to ask why don't you use parameterized tests, but noticed 
> > you do it in the next review. Any reason you don't want to fix it up in 
> > this patch?

Done.


> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, line 627
> > 
> >
> > Indent the block by 2 spaces, not 4.
> 
> Benjamin Bannier wrote:
> Knowing we care a lot about this, I did actually put some care into 
> making this consistent with the style guide.
> 
> Here and elsewhere: Our style guide states in the section *Function 
> Definition/Invocation* that
> > Newline when calling or defining a function: indent with four spaces.
> 
> Are you confusing this with the rule on continuations?,
> > Newline for an assignment statement: indent with two spaces.
> 
> I believe we are not performing an assignment, but a function invocation 
> here.
> 
> Note that further down in the style guide there are examples of `.then` 
> inconsistent with the indention rules.
> 
> Adam B wrote:
> After a quick grep, it seems that 2 spaces is indeed the de facto 
> standard. Even in /src/slave/, I can only find two (of 126) instances that 
> indent by 4 spaces.

Indenting by two spaces now.

It would be great if somebody in the know would unambiguously update the style 
guide to the latest source of truth.


- Benjamin


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


On April 27, 2016, 2:18 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 27, 2016, 2:18 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

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

(Updated April 27, 2016, 2:18 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.


Changes
---

Addressed comments from Alex and Adam.

* removed added fixture state,
* renamed parameterized fixture,
* fixed test for no authorization case,
* also tested `/flags` endpoint in parameterized test.


Bugs: MESOS-5164
https://issues.apache.org/jira/browse/MESOS-5164


Repository: mesos


Description
---

Added authorization to agents' `/monitor/statistics` endpoints.


Diffs (updated)
-

  src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
  src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
  src/tests/slave_authorization_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/46319/diff/


Testing
---

make check (OS X, clang w/o optimization)


Thanks,

Benjamin Bannier

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Alexander Rukletsov]

> On April 27, 2016, 10:59 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, line 187
> > 
> >
> > "Prefer trailing underscores for use as member fields", so you've got 
> > this reversed.

Unfortunately, that rule is not really welcomed by all members of the 
community, maybe we should even remove it (though I personally love it).


- Alexander


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


On April 27, 2016, 9 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 27, 2016, 9 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45922, 46318, 46203, 46319]

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 27, 2016, 9 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 27, 2016, 9 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Adam B]

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



Looking good. One more round.


src/slave/http.cpp (line 629)


Is there any standard for what order these [pid, limiter, request] should 
go in?



src/tests/slave_authorization_tests.cpp (line 179)


{ belongs on newline.



src/tests/slave_authorization_tests.cpp (line 187)


"Prefer trailing underscores for use as member fields", so you've got this 
reversed.



src/tests/slave_authorization_tests.cpp (lines 204 - 207)


I thought this was done in SetUp() now



src/tests/slave_authorization_tests.cpp (lines 252 - 254)


How is this testing with "no authorizer"? Doesn't SetUp use MockAuthorizer? 
Looks like MockAuthorizer::authorized() returns true by default. Might as well 
just remove this test.


- Adam B


On April 27, 2016, 2 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 27, 2016, 2 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Adam B]

> On April 26, 2016, 9:51 a.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, line 627
> > 
> >
> > Indent the block by 2 spaces, not 4.
> 
> Benjamin Bannier wrote:
> Knowing we care a lot about this, I did actually put some care into 
> making this consistent with the style guide.
> 
> Here and elsewhere: Our style guide states in the section *Function 
> Definition/Invocation* that
> > Newline when calling or defining a function: indent with four spaces.
> 
> Are you confusing this with the rule on continuations?,
> > Newline for an assignment statement: indent with two spaces.
> 
> I believe we are not performing an assignment, but a function invocation 
> here.
> 
> Note that further down in the style guide there are examples of `.then` 
> inconsistent with the indention rules.

After a quick grep, it seems that 2 spaces is indeed the de facto standard. 
Even in /src/slave/, I can only find two (of 126) instances that indent by 4 
spaces.


- Adam


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


On April 27, 2016, 2 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 27, 2016, 2 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

> On April 22, 2016, 10:22 a.m., Adam B wrote:
> > src/slave/http.cpp, line 616
> > 
> >
> > authorizeEndpoint() needs to be told that this is a GET request, since 
> > some endpoints may have different ACLs for different verbs. We'll have to 
> > resolve that in the /flags authz patch.

Dropped as `authorizeEndpoint` now takes the action into account. Either way, 
just following along what is done there.


- Benjamin


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


On April 26, 2016, 8:22 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 26, 2016, 8:22 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, line 655
> > 
> >
> > You remove the `Future` here. I believe this is on purpose as it seems 
> > the right way to pass the parameter (without future). Is it a bugfix or it 
> > doesn't influence the thrust?

Did you review the latest revision?


> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/tests/slave_authorization_tests.cpp, lines 198-200
> > 
> >
> > Maybe kill this blank lines?

Let's leave some room here.


> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, line 627
> > 
> >
> > Indent the block by 2 spaces, not 4.

Knowing we care a lot about this, I did actually put some care into making this 
consistent with the style guide.

Here and elsewhere: Our style guide states in the section *Function 
Definition/Invocation* that
> Newline when calling or defining a function: indent with four spaces.

Are you confusing this with the rule on continuations?,
> Newline for an assignment statement: indent with two spaces.

I believe we are not performing an assignment, but a function invocation here.

Note that further down in the style guide there are examples of `.then` 
inconsistent with the indention rules.


> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, line 635
> > 
> >
> > Indent the block by 2 spaces, not 4.

ditto.


> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, line 637
> > 
> >
> > Indent the block by 2 spaces, not 4.

ditto.


- Benjamin


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


On April 26, 2016, 8:22 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 26, 2016, 8:22 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

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

(Updated April 26, 2016, 8:22 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.


Changes
---

Rebased.


Bugs: MESOS-5164
https://issues.apache.org/jira/browse/MESOS-5164


Repository: mesos


Description (updated)
---

Added authorization to agents' `/monitor/statistics` endpoints.


Diffs (updated)
-

  src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
  src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
  src/tests/slave_authorization_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/46319/diff/


Testing
---

make check (OS X, clang w/o optimization)


Thanks,

Benjamin Bannier

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/slave/http.cpp (line 627)


Indent the block by 2 spaces, not 4.



src/slave/http.cpp (line 635)


Indent the block by 2 spaces, not 4.



src/slave/http.cpp (line 637)


Indent the block by 2 spaces, not 4.



src/slave/http.cpp 


You remove the `Future` here. I believe this is on purpose as it seems the 
right way to pass the parameter (without future). Is it a bugfix or it doesn't 
influence the thrust?



src/tests/slave_authorization_tests.cpp (lines 172 - 173)


I was about to ask why don't you use parameterized tests, but noticed you 
do it in the next review. Any reason you don't want to fix it up in this patch?



src/tests/slave_authorization_tests.cpp (lines 198 - 200)


Maybe kill this blank lines?


- Alexander Rukletsov


On April 26, 2016, 1:34 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 26, 2016, 1:34 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Alexander Rojas]

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


Ship it!




Ship It!

- Alexander Rojas


On April 26, 2016, 3:34 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 26, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

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

(Updated April 26, 2016, 1:34 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.


Bugs: MESOS-5164
https://issues.apache.org/jira/browse/MESOS-5164


Repository: mesos


Description (updated)
---

See summary.


Diffs
-

  src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
  src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
  src/tests/slave_authorization_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/46319/diff/


Testing
---

make check (OS X, clang w/o optimization)


Thanks,

Benjamin Bannier

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

> On April 26, 2016, 8:53 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 630-638
> > 
> >
> > Seems like these lines need to be indented more.

Indeed they need to be and now are.


> On April 26, 2016, 8:53 a.m., Adam B wrote:
> > src/slave/http.cpp, line 641
> > 
> >
> > Is this always an authoriaztion error/warning? Couldn't this also be a 
> > failure from limiter->acquire() or Slave::usage?

Good catch. Now we emit `"Could not collect statistics: "` 


- Benjamin


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


On April 26, 2016, 9:59 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 26, 2016, 9:59 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

> On April 22, 2016, 10:22 a.m., Adam B wrote:
> > src/slave/slave.hpp, lines 471-472
> > 
> >
> > How did this come up? The original `_statistics()` is not static, and 
> > it had no issues.
> 
> Benjamin Bannier wrote:
> The original `statistics` did not make any use of member data in any of 
> the `defer`ed actions (all the used data came from some 
> `Future` directly passed into the callback; the call of 
> `slave->self()` was not executed in the `defer`ed action). Now here we need 
> to access a rate limiter which is a member of `Http` which in turn might go 
> away before `_statistics` is executed.
> 
> Adam B wrote:
> And now that `_statistics()` is synchronous and no longer uses the 
> limiter, do we still need to worry about this?

Even if we do not use any member data, it is still undefined behavior to invoke 
a non-static member function on a destroyed object.


- Benjamin


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


On April 26, 2016, 9:59 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 26, 2016, 9:59 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

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

(Updated April 26, 2016, 9:59 a.m.)


Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.


Changes
---

Addressed comments from Adam (indention, error string).


Bugs: MESOS-5164
https://issues.apache.org/jira/browse/MESOS-5164


Repository: mesos


Description
---

Added authorization to agents' `/monitor/statistics` endpoints.


Diffs (updated)
-

  src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
  src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
  src/tests/slave_authorization_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/46319/diff/


Testing
---

make check (OS X, clang w/o optimization)


Thanks,

Benjamin Bannier

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Adam B]

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


Fix it, then Ship it!




Only minor nits left beyond my two remaining issues. Fix these and rebase once 
the previous patch in the chain lands, then we can commit this too.


src/slave/http.cpp (lines 630 - 638)


Seems like these lines need to be indented more.



src/slave/http.cpp (line 641)


Is this always an authoriaztion error/warning? Couldn't this also be a 
failure from limiter->acquire() or Slave::usage?


- Adam B


On April 25, 2016, 7:16 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 25, 2016, 7:16 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Adam B]

> On April 22, 2016, 1:22 a.m., Adam B wrote:
> > src/slave/slave.hpp, lines 471-472
> > 
> >
> > How did this come up? The original `_statistics()` is not static, and 
> > it had no issues.
> 
> Benjamin Bannier wrote:
> The original `statistics` did not make any use of member data in any of 
> the `defer`ed actions (all the used data came from some 
> `Future` directly passed into the callback; the call of 
> `slave->self()` was not executed in the `defer`ed action). Now here we need 
> to access a rate limiter which is a member of `Http` which in turn might go 
> away before `_statistics` is executed.

And now that `_statistics()` is synchronous and no longer uses the limiter, do 
we still need to worry about this?


- Adam


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


On April 25, 2016, 7:16 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 25, 2016, 7:16 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Adam B]

> On April 22, 2016, 1:22 a.m., Adam B wrote:
> > src/tests/slave_tests.cpp, line 1895
> > 
> >
> > Should we create a TYPED_TEST that tests this ACL in the local 
> > authorizer (direct and as a module), or do we only need these tests for one 
> > (of the many) ACCESS_ENDPOINT_WITH_PATH endpoints?
> 
> Benjamin Bannier wrote:
> I would prefer if we'd separate whether some authorizer works and whether 
> an endpoint correctly queries the authorizer. You already suggested testing 
> the former to Jan [here](https://reviews.apache.org/r/46203/#comment193252); 
> how about I migrate the current test to a *parameterized test* (on the 
> endpoint) so we have a single infrastructure to check authorization for all 
> `GET` requests against agent endpoints? Once we start tackling e.g., `POST` 
> requests we could extend this harness.
> 
> I made this test parameterized on the agent endpoint in 
> https://reviews.apache.org/r/46569/; if we'd want to go that direction we 
> could squash it into this patch.

Sounds great. Feel free to squash r/46569 into this patch or leave it separate.


- Adam


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


On April 25, 2016, 7:16 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 25, 2016, 7:16 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

> On April 22, 2016, 10:22 a.m., Adam B wrote:
> > src/tests/slave_tests.cpp, lines 1926-1927
> > 
> >
> > I would expect you to await/validate the response after validating the 
> > request.
> 
> Benjamin Bannier wrote:
> Wouldn't that be mixing two concerns in the same test (and e.g., lead to 
> bloat hiding what is the crux of the test)? I think since here I check only 
> whether an endpoint is authorized looking at the status code should be enough.
> 
> Adam B wrote:
> Sure, I didn't mean you need to validate more of the response's body. I 
> just meant that you currently `AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, 
> response)` then `AWAIT_READY(request)` (and validate its 
> subject/action/object), where I would expect the logical order to be to 
> await/validate the request first, then AWAIT_EXPECT_RESPONSE after.
> Request first, response after.

Thanks for being extra verbose; I have no idea how I could misread what you 
wrote originally.


- Benjamin


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


On April 25, 2016, 4:16 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 25, 2016, 4:16 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

> On April 25, 2016, 12:57 p.m., Alexander Rojas wrote:
> > src/slave/http.cpp, lines 647-649
> > 
> >
> > I was thinking that it makes sense to pull up the limiter related logic 
> > to `statistics()` so that method prepares everything up, while 
> > `_statistics()` only has logic related to build the response.

Good point! This also allows us to make `_statistics` purely synchronously 
which simplifies handling of `Future` failure scenarios.


- Benjamin


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


On April 25, 2016, 4:16 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 25, 2016, 4:16 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

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

(Updated April 25, 2016, 4:16 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.


Changes
---

Clearly separated async/sync parts of handling request for statistics.


Bugs: MESOS-5164
https://issues.apache.org/jira/browse/MESOS-5164


Repository: mesos


Description
---

Added authorization to agents' `/monitor/statistics` endpoints.


Diffs (updated)
-

  src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
  src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
  src/tests/slave_authorization_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/46319/diff/


Testing
---

make check (OS X, clang w/o optimization)


Thanks,

Benjamin Bannier

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Adam B]

> On April 22, 2016, 1:22 a.m., Adam B wrote:
> > src/tests/slave_tests.cpp, lines 1926-1927
> > 
> >
> > I would expect you to await/validate the response after validating the 
> > request.
> 
> Benjamin Bannier wrote:
> Wouldn't that be mixing two concerns in the same test (and e.g., lead to 
> bloat hiding what is the crux of the test)? I think since here I check only 
> whether an endpoint is authorized looking at the status code should be enough.

Sure, I didn't mean you need to validate more of the response's body. I just 
meant that you currently `AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, 
response)` then `AWAIT_READY(request)` (and validate its 
subject/action/object), where I would expect the logical order to be to 
await/validate the request first, then AWAIT_EXPECT_RESPONSE after.
Request first, response after.


- Adam


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


On April 25, 2016, 2:52 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 25, 2016, 2:52 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Alexander Rojas]

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




src/slave/http.cpp (lines 647 - 649)


I was thinking that it makes sense to pull up the limiter related logic to 
`statistics()` so that method prepares everything up, while `_statistics()` 
only has logic related to build the response.


- Alexander Rojas


On April 25, 2016, 11:52 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 25, 2016, 11:52 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

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

(Updated April 25, 2016, 11:52 a.m.)


Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.


Changes
---

Rebased onto Jan's last changes. Expected to fail until he fixes his 
`authorizeEndpoint` to support agent paths containing multiple `/`.


Bugs: MESOS-5164
https://issues.apache.org/jira/browse/MESOS-5164


Repository: mesos


Description
---

Added authorization to agents' `/monitor/statistics` endpoints.


Diffs (updated)
-

  src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
  src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
  src/tests/slave_authorization_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/46319/diff/


Testing
---

make check (OS X, clang w/o optimization)


Thanks,

Benjamin Bannier

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

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

(Updated April 25, 2016, 10:19 a.m.)


Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.


Changes
---

Reorder deps on Jan's request.


Bugs: MESOS-5164
https://issues.apache.org/jira/browse/MESOS-5164


Repository: mesos


Description
---

Added authorization to agents' `/monitor/statistics` endpoints.


Diffs
-

  src/slave/http.cpp f887a71684304a82ff0d81dbd240e29aa7e2ac67 
  src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
  src/tests/slave_authorization_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/46319/diff/


Testing
---

make check (OS X, clang w/o optimization)


Thanks,

Benjamin Bannier

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

> On April 19, 2016, 1:26 p.m., Jan Schlicht wrote:
> > The test case may need to be moved into `slave_authorization_tests.cpp` 
> > that was added in https://reviews.apache.org/r/46318/, but that really 
> > depends on whether that change there gets accepted or not.

I moved the test into `slave_authorization_tests.cpp` now.


- Benjamin


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


On April 22, 2016, 4:04 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 22, 2016, 4:04 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp f887a71684304a82ff0d81dbd240e29aa7e2ac67 
>   src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

> On April 22, 2016, 10:22 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 620-622
> > 
> >
> > Not sure why you reversed the boolean/order here, but ok.

You are right, putting the expected case first makes this more readable, fixed.


> On April 22, 2016, 10:22 a.m., Adam B wrote:
> > src/slave/http.cpp, line 613
> > 
> >
> > s/context/pid/? What is the 'context' referring to?

`context` is the execution context for `_statistics`, but you are right, we 
usually call this just some `pid`, adjusted.


> On April 22, 2016, 10:22 a.m., Adam B wrote:
> > src/slave/http.cpp, line 626
> > 
> >
> > s/discard/discarded/

Fixed.


> On April 22, 2016, 10:22 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 642-643
> > 
> >
> > Why change the wrapping/tabbing? If you were going to drop the first 
> > parameter down to the next line, then each parameter must be on its own 
> > line.
> > I would just leave the wrapping as is, personally.

Revert this auto-indention fat-fingering.


> On April 22, 2016, 10:22 a.m., Adam B wrote:
> > src/slave/slave.hpp, lines 471-472
> > 
> >
> > How did this come up? The original `_statistics()` is not static, and 
> > it had no issues.

The original `statistics` did not make any use of member data in any of the 
`defer`ed actions (all the used data came from some `Future` 
directly passed into the callback; the call of `slave->self()` was not executed 
in the `defer`ed action). Now here we need to access a rate limiter which is a 
member of `Http` which in turn might go away before `_statistics` is executed.


> On April 22, 2016, 10:22 a.m., Adam B wrote:
> > src/tests/slave_tests.cpp, line 1935
> > 
> >
> > These two parameters need to be on different lines if the whole command 
> > doesn't fit on one line. I'd be ok though if you moved the first param up 
> > to the `EXPECT_EQ(` line

Fixed.


> On April 22, 2016, 10:22 a.m., Adam B wrote:
> > src/tests/slave_tests.cpp, line 1956
> > 
> >
> > Shadows the `agent` variable on line 1905

I did this on purpose, but it is indeed confusing. Changed to now create 
specific agents in each block below.


> On April 22, 2016, 10:22 a.m., Adam B wrote:
> > src/tests/slave_tests.cpp, lines 1959-1965
> > 
> >
> > Can't you leave these out as default?

Indeed I can, changed.


> On April 22, 2016, 10:22 a.m., Adam B wrote:
> > src/tests/slave_tests.cpp, lines 1926-1927
> > 
> >
> > I would expect you to await/validate the response after validating the 
> > request.

Wouldn't that be mixing two concerns in the same test (and e.g., lead to bloat 
hiding what is the crux of the test)? I think since here I check only whether 
an endpoint is authorized looking at the status code should be enough.


> On April 22, 2016, 10:22 a.m., Adam B wrote:
> > src/tests/slave_tests.cpp, line 1895
> > 
> >
> > Should we create a TYPED_TEST that tests this ACL in the local 
> > authorizer (direct and as a module), or do we only need these tests for one 
> > (of the many) ACCESS_ENDPOINT_WITH_PATH endpoints?

I would prefer if we'd separate whether some authorizer works and whether an 
endpoint correctly queries the authorizer. You already suggested testing the 
former to Jan [here](https://reviews.apache.org/r/46203/#comment193252); how 
about I migrate the current test to a *parameterized test* (on the endpoint) so 
we have a single infrastructure to check authorization for all `GET` requests 
against agent endpoints? Once we start tackling e.g., `POST` requests we could 
extend this harness.

I made this test parameterized on the agent endpoint in 
https://reviews.apache.org/r/46569/; if we'd want to go that direction we could 
squash it into this patch.


- Benjamin


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


On April 22, 2016, 4:04 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> 

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

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

(Updated April 22, 2016, 4:04 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.


Changes
---

Address Adam's review comments.


Summary (updated)
-

Added authorization to agents' `/monitor/statistics` endpoints.


Bugs: MESOS-5164
https://issues.apache.org/jira/browse/MESOS-5164


Repository: mesos


Description (updated)
---

Added authorization to agents' `/monitor/statistics` endpoints.


Diffs (updated)
-

  src/slave/http.cpp f887a71684304a82ff0d81dbd240e29aa7e2ac67 
  src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
  src/tests/slave_authorization_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/46319/diff/


Testing
---

make check (OS X, clang w/o optimization)


Thanks,

Benjamin Bannier